Simple days between dates calculator I recently decided to try my hand at this problem of calculating the number of days between two given dates including leap years in the calculation. I chose to do it in C++ as it is the language I feel most comfortable with and was wondering about the quality of my code.

#include <iostream>
#include <vector>
#include <string>

using namespace std;

typedef struct Date{
    int day;
    int month;
    int year;

    Date(int d, int m, int y):day(d), month(m), year(y)
    {
    }

    bool operator== (const Date& rhs)
    {
        return day==rhs.day && month==rhs.month && year==rhs.year;
    }
}Date;

vector<int> splitBy(string input, char delimiter)
{
    vector<int> output;
    int startDayIndex = 0;
    int endIndex = 0;
    int length = input.length();
    while(input.find(delimiter, startDayIndex)!=string::npos)
    {
        endIndex = input.find(delimiter, startDayIndex);
        output.push_back(stoi(input.substr(startDayIndex,endIndex)));
        startDayIndex = endIndex+1;

    }

    output.push_back(stoi(input.substr(startDayIndex,length)));
    return output;
}

int NextMonth(int currentMonth)
{
    if(currentMonth == 12) return 1;
    return currentMonth+1;
}

int NextYear(int month, int currentYear)
{

    return month==1?currentYear+1:currentYear;
}

bool isLeapYear(int year)
{
    if(year%4!=0) return false;
    else if(year%100!=0) return true;
    else if(year%400!=0) return false;
    else return true;
}

int DaysInMonth(int month, int year)
{
    vector<int> days = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    if(isLeapYear(year)) days[1] = 29;
    return days[month-1];

}

bool errorCheck(Date start, Date end)
{

    if(start.year>end.year) return true;
    else if(start.month>end.month && start.year==end.year) return true;
    else if(start.day>end.day && start.month==end.month && start.year==end.year) return true;
    return false;
}

int start(string firstDate, string endDate)
{
    vector<int> initDate = splitBy(firstDate, '/');
    vector<int> finDate = splitBy(endDate, '/');
    Date startDay(initDate[0], initDate[1], initDate[2]);
    Date endDay(finDate[0], finDate[1], finDate[2]);
    if(errorCheck(startDay,endDay)) return -1;
    int count = 0;
    int days = DaysInMonth(startDay.month, startDay.year);

    while((startDay==endDay)!=1)
    {
        while(startDay.day<days)
        {
            startDay.day = startDay.day + 1;
            count++;
            if((startDay==endDay)==1)
            {
                return count;
            }
        }
        count++;
        startDay.day = 1;
        startDay.month = NextMonth(startDay.month);
        startDay.year = NextYear(startDay.month,startDay.year);
        days = DaysInMonth(startDay.month, startDay.year);
    }
    return count;

}

int main()
{
    cout << "Enter date in format DD/MM/YYYY\n";
    string startDayDate;
    string endDate;
    cout << "Enter first date\n";
    cin >> startDayDate;
    cout << "Enter second date\n";
    cin >> endDate;
    int result = start(startDayDate, endDate);
    result==-1?cout << "Invalid input\n":cout << "The number of days are: " << result << endl;

    return 0;

}

I know off the bat that using namespace std is an issue, are there any other issues with regards to readability, efficiency, etc.? Does the code I have written to accomplish this task qualify as "good" code?

I see a number of things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Sanitize user input better

The date check could be a little more robust.  In particular, verifying that the month is in the range of 1 to 12 is particularly important because that number is later used as an index into a vector.  If I enter the nonsensical date 40/40/1940 the errorCheck routine currently accepts it as valid and the program later crashes.  If I enter just 40/40, the program also crashes.

Simplify your code

Right now the code includes this line:

while((startDay==endDay)!=1)

It would be more straightforward to simply write this:

while(startDay != endDay)

This would only require the addition of an operator!= to the Date.

Don't complicate the code

The code currently contains this structure declaration:

typedef struct Date{
    // stuff
}Date;

First, the typedef is neither needed nor useful.  This isn't C.  Second, it would make sense to declare it as a class rather than as a struct.

class Date {
    // stuff
};

Use const where practical

The operator== member function of Date does not alter the underlying object and therefore should be declared const.

bool operator==(const Date& rhs) const;

Make your classes do more work!

The current Date class is a very lazy thing, leaving all of the work to separate functions.  I'd make it do much more of the work. For instance, I reimplemented your algorithm but did most of the work in class operators.  Here's an excerpt of that code:

int operator-(Date end, Date start) {
    bool swapped{false};
    if (end < start) {
        std::swap(start, end);
        swapped = true;
    }
    int diff = 0;
    for ( ; start != end; ++start) {
        ++diff;
    }
    return swapped ? -diff : diff;
}

int main() {
    Date start, end;

    std::cout << "Enter start date in format DD MM YYYY\n";
    std::cin >> start;
    std::cout << "Enter ending date in format DD MM YYYY\n";
    std::cin >> end;
    std::cout << end-start << "\n";
}

Consider the cost of object creation

The code currently contains this function:

int DaysInMonth(int month, int year)
{
    vector<int> days = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    if(isLeapYear(year)) days[1] = 29;
    return days[month-1];
}

This is computationally much more costly than it should be because it requires the creation and destruction of a std::vector on every invocation.  Those numbers don't change, so that first line could instead look like this:

static constexpr int days[12]{31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

In the case of a leap year, alter the returned value instead of the array itself.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


How to set state of multiple properties in one event handler (React) Pretty straight forwards really...I know that it's usually bad practice to repeat your code so I'm wondering how I could join handleAuthorChange() and handleContentChange() into one method. I guess it was hard for me to find a solution because I don't understand how I would know which part of the state I'm updating. I was thinking maybe embed a HTML id and check on the event target if IDs match but that seems pretty inefficient. Thanks in advance!

import React, { Component } from 'react';

class AddComment extends Component {
    state = {
        author: '',
        content: ''
    }

    handleAuthorChange = (e) => {
        this.setState({
            author: e.target.value
        });
    }

    handleContentChange = (e) => {
        this.setState({
            content: e.target.value
        });
    }

    handleSubmit = (e) => {
        e.preventDefault();
        this.props.addComment(this.state);
        this.setState({
            author: '',
            content: ''
        });

    }

    render() {
        return (

                <form onSubmit={ this.handleSubmit } className="card hoverable p-bottom">

ADD COMMENT

                    <input onChange={ this.handleContentChange } value={this.state.content} type="text" placeholder="comment" />
                    <input id="name" onChange={ this.handleAuthorChange } value={this.state.author} type="text" placeholder="name" />
                    <input value="submit" type="submit"/>
                </form>

        )
    }
}

export default AddComment;

I find it fine as it is, but if you really want to keep a single method, you can parametrize it by the name of the key to set:

handleChange = (keyName, e) => { this.setState({ [keyName]: e.target.value }); }

and call it like handleChange('content', event) or handleChange('author', event).

Problem is, now, to call it properly when creating your components. You could write things like:

<input onChange={(e) => this.handleChange('content', e)} value={this.state.content} type="text" placeholder="comment" />

But this syntax creates a new function each time the component is rendered, potentially triggering child re-render as well. React recommends to bind functions in the constructor instead:

import React, { Component } from 'react';

class AddComment extends Component {
    constructor(props) {
        super(props);
        this.state = { author: '', content: '' };
        this.handleAuthorChange = this.handleChange.bind(this, 'author');
        this.handleContentChange = this.handleChange.bind(this, 'content');
        this.handleSubmit = this.handleSubmit.bind(this);
    }

    handleChange(keyName, e) {
        this.setState({ [keyName]: e.target.value });
    }

    handleSubmit(e) {
        e.preventDefault();
        this.props.addComment(this.state);
        this.setState({ author: '', content: '' });
    }

    render() {
        return (

                <form onSubmit={this.handleSubmit} className="card hoverable p-bottom">

ADD COMMENT

                    <input onChange={this.handleContentChange} value={this.state.content} type="text" placeholder="comment" />
                    <input id="name" onChange={this.handleAuthorChange} value={this.state.author} type="text" placeholder="name" />
                    <input value="submit" type="submit"/>
                </form>

        );
    }
};

export default AddComment;


Word-counting script in C# My goal is to read from standard input, break up the input into words (Unicode letters and apostrophes), case-insensitively, and produce a report to standard output, where each line is a word followed by a space followed by its count. The output should be sorted by words.

This is my code:

using System;
using System.Text.RegularExpressions;
using System.Collections.Generic;

class TraditionalWordCountApp
{
    public static void Main(string[] args)
    {
        SortedDictionary<string, int> counts = new SortedDictionary<string, int>();
        Regex wordRegex = new Regex(@"[\p{L}']+");
        string line;
        while ((line = Console.ReadLine()) != null)
        {
            line = line.ToLower();
            foreach (Match m in wordRegex.Matches(line))
            {
                String word = m.Value;
                int count;
                counts.TryGetValue(word, out count);
                counts[word] = ++count;
            }
        }
        foreach (KeyValuePair<string, int> pair in counts)
        {
            Console.WriteLine("{0} {1}", pair.Key, pair.Value);
        }
    }
}

My concerns:

Should things be wrapped in a namespace?
Should either the class or the main function or both be public?
Is there a nicer way to declare the sorted dictionary? Java has <>, does C#?
I used the C-style idiom of assigning to a variable and then checking for null, all inside the while-condition! Surely there is a nicer way to do this in C#. Is there? I like neither the embedded assignment nor the billion-dollar-mistake null. :)
I searched StackOverflow a bit for a nicer way to update a word frequency dictionary. Most languages seem to have a built-in get-with-default method on dictionaries, but the answers I saw said no, and they recommended TryGetValue. These answers are old, though. Is there something nicer in the most modern C#?
Any nicer way to do the last loop?
Would a LINQ-based solution be nicer? More idiomatic? What would it look like? I think (though I am brand new to C#) I could whip something up, but should I? FWIW I do know how to do the LINQ-y stuff in Java but wonder if this is the right way to go in C#.

It's not mandatory but it's a widely accepted guideline. Break it only if you have a good reason to do so.
No, they don't. Class should be internal (default) and static (because it has not instance methods). Main() method should be private to make clear that it won't be called outside this class but it's not mandatory (someone prefers public because it's logically called from outside).

For the other points things are more complex that a paragraph...

You can create a generator to read from standard input (see very last paragraph for a discussion about this):

private static IEnumerable<string> ReadAllInputs()
{
    while (true)
    {
        string line = Console.ReadLine();
        if (line == null)
            break;

        yield return line;
    }
}

You do not need ToLower() on input, I do not even start to mention all the problems of case insensitive comparison if not done properly: a lower case string may be different from an upper case string (even in number of characters) but they may be compared equal. Don't put yourself in trouble and let the framework do the work for you:

var counts = new SortedDictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);

Note that you do not need a SortedDictionary here, ordering may be done (in this case) when printing output. If performance matters then you should profile by yourself.

When you increment word count you also increment local variable count, not a big performance hit (and compiler may be smart enough to elide local variable) but it's about code clarity. You can do:

counts[word] = count + 1;

Or simply:

++counts[word];

Note that this innocent code ++counts[word] (as well as original counts[word] = ++count) when word doesn't exist in dictionary (TryGetValue returns false) will perform another lookup vanishing part of the benefit of TryGetValue for the first insertion! You have to explicitly call Add().

If you want you can refactor foreach loop to print results with LINQ, not a big gain (IMO), let's see how:

Console.WriteLine(String.Join(Environment.NewLine,
    counts.Select(x => $"{x.Key} {x.Value}")));

If we put everything together with then have something like this:

namespace Test {
    static class TraditionalWordCountApp {
        static void Main(string[] args) {
            var counts = new Dictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);
            var wordRegex = new Regex(@"(?i)[\p{L}']+");

            foreach (var match in ReadAllInputs().SelectMany(x => wordRegex.Match(x))) {
                if (counts.TryGetValue(match.Value, out count))
                    counts[word] = count + 1;
                else
                    counts.Add(match.Value, 1);
            }

            Console.WriteLine(String.Join(Environment.NewLine,
                counts.OrderBy(x => x.Key).Select(x => $"{x.Key} {x.Value}")));
        }

        static IEnumerable<string> ReadAllInputs() {
            while (true) { // Yes, in C# this is "safe"
                string line = Console.ReadLine();
                if (line == null)
                    break;

                // No need to match any regex against an empty line
                if (line.Length == 0)
                    continue;

                yield return line;
            }
        }
    }
}

All these done you may think that...there should be another way to do it. You can use GroupBy(). Profile both versions to see which one has better performance (and the one you feel more easy to understand):

var counts = ReadAllInputs()
    .SelectMany(x => regex.Match(x))
    .GroupBy(x => x.Value, StringComparer.CurrentCultureIgnoreCase)
    .Select(x => new { Word = x.Key, Count = x.Count() })
    .OrderBy(x => x.Word);

Then you print it with:

Console.WriteLine(String.Join(Environment.NewLine,
    counts.Select(x => $"{x.Word} {x.Count}")));

One final note about word counting. It may apply or not to your case but don't forget that word is a concept with different meanings in different languages. Topic is vast but try to paste a - for example - Japanese text in Microsoft Word and check what the word count is: number of characters (another vague concept). If you have to deal with international text you may need to write specific code for different cultures.

Comments. Few notes collected from comments.

Sometimes you see this kind of code:

while (Console.In.Peek() != -1)
   yield return Console.ReadLine();

It works only if console input is redirected, when input is line buffered then it'll end immediately after first line. Nice and short but simply it doesn't work. If you really really love LINQ and you can't live without one-line functions then you may do this:

static IEnumerable<string> ReadAllInputs() {
    return Enumerable.Range(0, Int32.MaxValue)
        .Select(_ => Console.ReadLine())
        .TakeWhile(x => x != null);
}

Can we make our reader more "generic"? The obvious idea is to use it with files. Maybe yes, maybe not. First of all a console application can have piped input (from file or from another application) then we may not need this feature. If we do then we should introduce a separate class InputReader, implementation will be different for ConsoleInputReader, FileInputReader and HttpInputReader (for example); one function with a TextReader parameter will probably be the worst compromise for all our use-cases. What I'm suggesting is just to don't introduce complexity until you need it...


Merge Intervals from leetcode Given a collection of intervals, merge all overlapping intervals.

  input: [1,3],[2,6],[8,10],[15,18],

  output: [1,6],[8,10],[15,18].

The logic is very simple: just merge all the intervals iteratively from first to last.

# Definition for an interval.
# class Interval(object):
#     def __init__(self, s=0, e=0):
#         self.start = s
#         self.end = e

class Solution(object):
        def merge(self, intervals_struct):
            """
            :type intervals: List[Interval]
            :rtype: List[Interval]
            """
            def is_intersect((i1_l, i1_r), (i2_l, i2_r)):
                if i1_r >= i2_l and i1_r <= i2_r:
                    return True
                elif i2_r >= i1_l and i2_r <= i1_r:
                    return True
                return False

            def merge_interval((i1_l, i1_r), (i2_l, i2_r)):
                return [min(i1_l, i2_l), max(i1_r, i2_r)]

            intervals = []
            for i in intervals_struct:
                intervals.append([i.start, i.end])

            intervals = sorted(intervals, key=lambda x:x[0])

            i, result = 0, []
            while i < len(intervals):
                max_l, max_r = intervals[i]
                while i < len(intervals) and is_intersect([max_l, max_r], intervals[i]):
                    max_l, max_r = merge_interval([max_l, max_r], intervals[i])
                    i += 1
                result.append([max_l, max_r])
                if i < len(intervals) and intervals[i] == [max_l, max_r]:
                    i += 1
            return result

Python2 / Python3

Your code don't working under python 3 (PEP 3113). If possible better use newer python :).

Algorithm

no needs to change Interval to list with two elements. It's bad practise. You try low-level optimizations, not algorithm one. Code is less readable.
local functions is_intersect and merge_interval don't use knowledge that we have sorted intervals, e.g. i1_l <= i2_l
pattern while i < len(my_list): better change to for element in my_list: because we don't need index

Reduced code

class Interval(object):
    def __init__(self, s=0, e=0):
        self.start = s
        self.end = e

class Solution(object):
    def merge(self, intervals_struct):
        if not intervals_struct:
            return []
        intervals = sorted(intervals_struct, key=lambda x: x.start)

        result = [intervals[0]]
        for cur in intervals:
            if cur.start > result[-1].end:
                result.append(cur)
            elif result[-1].end < cur.end:
                result[-1].end = cur.end
        return result


Extract the data from Dicom files in an time efficient way I am trying to extract and save images from a Dicom dataset. It is taking approximate 4-5 hours to process 1000 files. (Also, I am reading and saving files in an external hard drive so there is a time lag due to this factor too)
Is there a way to expedite the program as I have more than 10k files and it'll take me forever to just extract the images.
Also, if there is any other suggestion or improvements, anyone could suggest then it is hugely appreciated as I am not from Computer Science background.
Below is my code -
import pandas as pd
import shutil
import os
import glob
import numpy as np
import pathlib
import torch
import random
import shutil
import pydicom
import matplotlib.pyplot as plt
from skimage.io import imread, imsave,imshow

data_dir = 'Path_to_dicom_files'
files = glob.glob(os.path.join(data_dir,'*.dcm'))
folders = []
counter = 1
Destination_path = 'Path_to_destination_folder_where_images_will_be_saved'
root_dirs = 'Path_from_where_files_are_being_read'
for i in files:
    print(i)
    dcm = pydicom.dcmread(i)
    name = dcm.AccessionNumber
    dest = os.path.join(Destination_path,name)
    if dcm.Modality == "XC":
        if os.path.isdir(dest):
            img = dcm.pixel_array
            name = dcm.AccessionNumber+'_'+str(counter)+'.png'
            counter+=1
            #img.save(name)
            imsave(name,img)
            shutil.move(os.path.join(root_dirs,name),dest)
        else:
            os.mkdir(dest)
            img = dcm.pixel_array
            name = dcm.AccessionNumber+'_0.png'
            #img.save(name)
            imsave(name,img)
            shutil.move(os.path.join(root_dirs,name),dest)

Big data requires big time. But there might be ways to speed up your processing somewhat. For each DICOM file you:

open the file
read metadata
read an image
write the image as PNG in the current directory
move the PNG file to it's final destination

Some complex file formats benefit from being read and written on a local drive, the software assumes drive access is fast and memory is expensive, so it reads small bits of data scattered around the file instead of reading larger chunks and using only the small bits it needs. These file types can benefit from being copied over in their entirety to a local drive for reading. However, DICOM is not a complex file format, and DICOM files are commonly stored on networked drives, so I don't think this causes any specific problems.
However, writing PNG files might be a bit faster if done on a local drive. You create the file in the current directory (wherever that is), then move it from root_dirs to Destination_path. So I presume that root_dirs is the current directory? Instead, you could create the file in the temporary directory, then move it to its final destination like you do:
import tempfile
temp_dir = tempfile.gettempdir()  # expected to be on the local drive
# ...
name = dcm.AccessionNumber + '_' + str(counter) + '.png'
name = os.path.join(temp_dir, name)
imsave(name, img)
shutil.move(name,dest)

Next, skimage.io.imsave is very flexible, but it just calls functions in other libraries:

By default, the different plugins are tried (starting with imageio) until a suitable candidate is found. If not given and fname is a tiff file, the tifffile plugin will be used.

Note also that imageio further knows a lot of formats, including two different PNG implementations. So this writing function again does a lot of logic to find out what format to write the file in. And it might end up using the PIL implementation of PNG, which I hear is not very fast.
Therefore, it might be a bit more performant to use a different file writer. I would experiment with different ones, even a small difference in timing will accumulate over so many images. For example, with OpenCV you'd do:
import cv2
# ...
cv2.imwrite(name, img)

Finally, since dcm.pixel_array is a property, not a function, it is likely that dcm = pydicom.dcmread(i) reads in all the data. If you have files for which dcm.Modality == "XC" is not true, you've read in data unnecessarily. Consider looking for a DICOM reader that can read only the metadata. I don't know anything about pydicom, so can't comment on its speed.

Since this is Code Review, I'll give you some pointers towards better code:
Python people have strict rules they follow for code style (encoded in PEP 8). You break some of these rules, but I won't go into that, I think code style is personal and as long as you're consistent, you're OK.
Repeating code is bad though. It is easy to fix a bug in one place, and then forget it needs fixing in the other copy too. It causes there to be more code to read, making the code harder to understand. And it makes you type more, giving you more possibilities to make errors. The following bit of code is repeated, with only a trivial change:
counter = 1
# ...
        if os.path.isdir(dest):
            img = dcm.pixel_array
            name = dcm.AccessionNumber+'_'+str(counter)+'.png'
            counter+=1
            imsave(name,img)
            shutil.move(os.path.join(root_dirs,name),dest)
        else:
            os.mkdir(dest)
            img = dcm.pixel_array
            name = dcm.AccessionNumber+'_0.png'
            imsave(name,img)
            shutil.move(os.path.join(root_dirs,name),dest)

Instead, write:
counter = 0
# ...
        if !os.path.isdir(dest):
            os.mkdir(dest)
        img = dcm.pixel_array
        name = dcm.AccessionNumber+'_'+str(counter)+'.png'
        counter+=1
        imsave(name,img)
        shutil.move(os.path.join(root_dirs,name),dest)

You could also put the os.mkdir(dest) call outside the loop. Having less code inside the loop makes the loop faster.


Optimised library database code for sorted array of structures I have an array of structure records and function intersection and difference.

The intersection function take list1,list2,list3(an array of records),and size of both list1 and list2.

Here, intersection and difference both are the boolean operators ( like : A U B , A - B , A intersection B). Also list1 and list2 are given as input and the result is copied to list3.

My both functions are working fine. But given that the two lists are already sorted (on author name and if same author name then name of the book), how can I optimize the code?  intersection is of O(n2) and difference is less than O(n2).

copy() copies a record to first argument from second argument.

//Intersection of two lists
void intersection(struct books *list1,struct books *list2,struct books *list3,int n1,int n2)
{
    int i,j,size1,size2;
    if(n1<n2){size1=n1;size2=n2;}else{size1=n2;size2=n1;}
    for(i=0;i<size1;i++)
    {
        for(j=0;j<size2;j++)
        {
            if(strcmp(list1[i].name,list2[j].name)==0 && strcmp(list1[i].author,list2[j].author)==0)
            {
                if(list1[i].copies < list2[j].copies)
                {
                    copy(&list3[i],&list1[i]);
                }
                else
                {
                    copy(&list3[i],&list2[j]);
                }
            }
        }
    }
}

//set difference on lists (optimised)
void difference(struct books *list1,struct books *list2,struct books *list3,int n1,int n2)
{
    int i,j,k=0,exists=0;
    for(i=0;i<n1;i++)
    {
        exists=0;
        for(j=0;j<n2 && exists==0;j++)
        {
            if(strcmp(list1[i].author,list2[j].author)==0 && strcmp(list1[i].author,list2[j].author)==0)
            {
                exists=1;
            }
        }
        if(exists==0)
        {
            copy(&list3[k],&list1[i]);
            k++;
        }
    }
}

Given that the two lists are already sorted, you can do MUCH better than just the naive O(|A| * |B|) implementation. Let me reduce the problem to just intersecting lists of chars, and let's say our lists are:

A: [... a elems ..., 'C', 'D', ... ]
B: [... b elems ..., 'C', 'E', ... ]

Let's say we just in the outer looped matched A's 'C' to B's 'C'. Cool, we got that right. Now, we're going to try to check for 'D'. Do we really need to start looking at B[0]? We know that B is sorted... and we know that B[b] == 'C', so we know for sure that if there is a 'D' in B then it cannot be in the first b+1 elements.

If you change your inner loop from looping over every element in the 2nd list, to just making sure you end up only walking over the list once, you can reduce your complexity to O(|A| + |B|), which is pretty huge.


Iterate through slicer settings and print to PDF I have a spreadsheet with seven tables (tbl_1, tbl_2, ..., tbl_7) each controlled by its own slicer. Each slicer has six buttons (10, 20, 30, 40, 50, 60) referring to Team Codes. I use the code below to select one team on every slicer, then create a PDF for each team / slicer setting.

As of now, the code takes anywhere from 5-7min to run.

Sub SlicerTeam()
Dim wb As Workbook
Dim sc As SlicerCache
Dim si As SlicerItem

On Error GoTo errHandler
Application.ScreenUpdating = False
Application.EnableEvents = False

Set wb = ThisWorkbook

For x = 1 To 6
    For i = 1 To 7
    Set sc = wb.SlicerCaches("tbl_" & i)
        sc.ClearAllFilters
        For Each si In sc.VisibleSlicerItems
            Set si = sc.SlicerItems(si.Name)
                If Not si Is Nothing Then
                    If si.Name = x * 10 Then
                        si.Selected = True
                    Else
                        si.Selected = False
                    End If
                Else
                    si.Selected = False
                End If
        Next si

    Next i
Call PDFCreate
Next x

exitHandler:
Application.ScreenUpdating = True
Application.EnableEvents = True
Exit Sub

errHandler:
MsgBox ("Error in updating slicer filters.")
Resume exitHandler

End Sub

Sub PDFCreate()
Dim Fname As String
Dim path As String

path = "S:\MyFilePath\"
Fname = path & Sheets("Detail").Range("T1").Value & " - " & [TEXT('Population Detail'!B1,"mmm, yyyy")] & ".pdf"

    Sheets("Detail").ExportAsFixedFormat Type:=xlTypePDF, _
        FileName:=Fname, Quality:=xlQualityStandard, IncludeDocProperties:=True, _
        IgnorePrintAreas:=False, OpenAfterPublish:=False

End Sub

Variables
Let's discuss variables a bit.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type. Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

Variable names - give your variables meaningful names.

Personal preference - I avoid variables like i and j because they aren't descriptive. But, there's technically nothing wrong with using them as it's standard practice to iterate with i.

Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

That being said -
You don't declare x or i. But those aren't vary descriptive are they? Try:
Dim slicerButtonNumber As Long
Dim tableNumber As Long

You don't have great names for the variables you do declare -
wb - targetWorkbook
sc - targetSlicerCache
si - targetSlicerItem

Extending the names using descriptive words costs you nothing and saves you headaches!
Also, here:
Set targetSlicerCache = targetBook.SlicerCaches("tbl_" & tableNumber)

You're using a constant - "tbl_", so
Const TABLE_PREFIX as String = "tbl_"
Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)

It may not seem like much, but by declaring that at top, if it ever changes, you only need to change it in one place.
Same goes for PDFCreate:
Const SHEET_NAME As String = "Detail"
Const POPULATION_SHEET_NAME As String = "Population Detail"
Const SEPARATOR As String = " - "
Dim detailSheet As Worksheet
Set detailSheet = ThisWorkbook.Sheets(SHEET_NAME)
Dim populationSheet As Worksheet
Set populationSheet = ThisWorkbook.Sheets(POPULATION_SHEET_NAME)
Dim file_Name As String
Dim file_Path As String
Dim aPathValue As String

file_Path = "S:\MyFilefilePath\"
aPathValue = detailSheet.Range("T1").Value & SEPARATOR
file_Path = file_Path & aPathValue
file_Name = filePath & [TEXT(populationsheet.range("B1"),"mmm, yyyy")] & ".pdf"

detailSheet.ExportAsFixedFormat Type:=xlTypePDF, _
    filename:=file_Name, Quality:=xlQualityStandard, IncludeDocProperties:=True, _
    IgnorePrintAreas:=False, OpenAfterPublish:=False

You don't need to Call subs, it's obsolete. Instead just use Sub argument, argument

Speed
Your set here is redundant
        For Each targetSlicerItem In sc.VisibleSlicerItems
            Set targetSlicerItem = targetSlicerCache.SlicerItems(targetSlicerItem.Name)

The For sets the items.
This is also unnecessary:
If Not targetSlicerItem Is Nothing Then

It won't be Nothing since you're only addressing the items that exist in the cache.
You can condense down
For slicerButtonNumber = 1 To 6
    For tableNumber = 1 To 7
    Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)
        targetSlicerCache.ClearAllFilters
        For Each targetSlicerItem In targetSlicerCache.VisibleSlicerItems
            If targetSlicerItem.Name = slicerButtonNumber * 10 Then
                targetSlicerItem.Selected = True
            Else
                targetSlicerItem.Selected = False
        End If
        Next targetSlicerItem
    Next i

You can however, remove one of your loops, the slicerButtonNumber loop
Const BUTTON_NUMBERS As String = "10,20,30,40,50,60"
Const DELIMITER As String = ","
Dim slicerButtonNumbers As Variant
slicerButtonNumbers = Split(BUTTON_NUMBERS, DELIMITER)

 For tableNumber = 1 To 7
    Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)
        targetSlicerCache.ClearAllFilters
        For Each targetSlicerItem In targetSlicerCache.VisibleSlicerItems
            If UBound(Filter(slicerButtonNumbers, targetSlicerItem.Name)) > -1 Then
               targetSlicerItem.Selected = True
            Else
                targetSlicerItem.Selected = False
            End If
        Next targetSlicerItem
    Next tableNumber


String appending function (location-independent) Very useful operation that hasn't been merged into a function.. until now.
This is supposed to insert a substring into a previously d.allocated source string.

void strapp (char *source_offset, int position, char *appendage)
{
    size_t appendage_size = strlen(appendage) + 1;

    char copy[strlen(source_offset)];
    strcpy(copy, source_offset);

    source_offset = realloc(source_offset, strlen(source_offset) + appendage_size);
    memcpy(&source_offset[position], appendage, strlen(appendage));
    sprintf(source_offset + (position + strlen(appendage)), &copy[position]);
}

USAGE:

int main(void)
{
    char *str1 = malloc(11 + 1);

    sprintf(str1, "Hello World");

    strapp(str1, 5, " Horrible");

    printf(str1);
    free(str1);

    return 0;
}

Produces the output:

Hello Horrible World

This is more of a string-inserting function than a string-appending function, and should be renamed as such.

When calling realloc(…), the string could very well move to a different memory location.  The caller of strapp() has no way of knowing where the new string is, though.  Furthermore, if the string got moved, and the caller continues to use the string as if were still at source_offset, then that's a memory-use-after-free bug.

There are two possible remedies:

/**
 * s points to the address of the old and new string.
 */
string_insert(char **s, int insertion_pt, const char *to_insert)

or

/**
 * Returns the location of the new string, which may or may not be at
 * the original s.  Callers must assume that the string at s may be
 * invalid after this call.
 */
char *string_insert(char *s, int insertion_pt, const char *to_insert)


Improving on John Resig's Simple JavaScript Inheritance: avoiding `new` I've recently read this article, and I agree that the new keyword is not good practice.

Thus, I've made an improvement on John Resig's Simple JavaScript Inheritance in order to use the Class.create method instead of new:

// The dummy class constructor
function Class() {
   // I remove the initialization procedure in constructor function,
   // Initialization will done by Class.create which I defined below
}

// Enforce the constructor to be what we expect
Class.prototype.constructor = Class;

// And make this class extendable
Class.extend = arguments.callee;

// What I improved
Class.create = function () {
  var instance = new this();
  if (instance.init) {
      instance.init();
  }
  return instance;
}

The initialization case in his article could be rewritten like this:

var Person = Class.extend({
  init: function(isDancing){
    this.dancing = isDancing;
  }
});

var p = Person.create();

Have I done this well or not? Please help me check it.

If you really want to create your own inheritance pattern then John Resig's Simple Inheritance pattern is not a good candidate to build upon. The reason is because John Resig's code is very slow. The reason it's so slow is because of the way _super is handled:

// Copy the properties over onto the new prototype
for (var name in prop) {
  // Check if we're overwriting an existing function
  prototype[name] = typeof prop[name] == "function" &&
    typeof _super[name] == "function" && fnTest.test(prop[name]) ?
    (function(name, fn){
      return function() {
        var tmp = this._super;

        // Add a new ._super() method that is the same method
        // but on the super-class
        this._super = _super[name];

        // The method only need to be bound temporarily, so we
        // remove it when we're done executing
        var ret = fn.apply(this, arguments);
        this._super = tmp;

        return ret;
      };
    })(name, prop[name]) :
    prop[name];
}

I'll try to explain what's happening in the above code:

We loop through each property which we wish to copy onto the newly extended object.
If the property is a function and it overrides another function of the same name and it needs to call the overridden function then we replace it with a function which changes the value of this._super within the function to the overridden function for that particular function call.
Otherwise we simply copy the property as it is.

This little indirection allows you to call overridden methods using this._super. However it also makes the code very slow. Hence I suggest you don't use John Resig's Simple JavaScript Inheritance.

Since this is a code review site I'm also obliged to point out flaws in your code. The main problem with your code is in the Class.create function:

Class.create = function () {
  var instance = new this();
  if (instance.init) {
      instance.init();
  }
  return instance;
};

Compare that with John Resig's original dummy class constructor:

// The dummy class constructor
function Class() {
  // All construction is actually done in the init method
  if ( !initializing && this.init )
    this.init.apply(this, arguments);
}

The problem is that any argument applied to Class.create is lost. You need to apply the arguments passed to Class.create to instance.init as follows:

Class.create = function () {
  var instance = new this;
  if (instance.init) instance.init.apply(instance, arguments);
  return instance;
};

Beside that there's not much more scope for improvement if you plan to stick to John Resig's Simple Inheritance Pattern.

If you really want to create your own inheritance pattern in JavaScript then it pays to invest some time learning about how inheritance works in JavaScript. For example the following answer explains prototype-class isomorphism in JavaScript: https://stackoverflow.com/a/17893663/783743

Prototype-class isomorphism simply means that prototypes can be used to model classes. Armed with this knowledge we can write a function which takes an object (a prototype) and returns a class (a constructor function):

function CLASS(prototype) {
    var constructor = prototype.constructor;
    constructor.prototype = prototype;
    return constructor;
}

Using the above function we may now create and instantiate classes as follows:

var Person = CLASS({
    constructor: function (isDancing) {
        this.dancing = isDancing;
    },
    dance: function () {
        return this.dancing;
    }
});

var p = new Person(true);

Although this pattern does not have inheritance yet it is a good base to build upon. With a little bit of modification we can make it behave the way we want it to. There are a few important points to note:

We want to be able to use the extend and create functions on any function. Hence it's better to add them to Function.prototype instead of creating a separate Class constructor.
For a function F it need not be true that F.prototype.constructor === F. Hence we may use F for extending and F.prototype.constructor for creating.
Instead of passing a prototype to extend it makes much more sense to pass a blueprint of a prototype instead. This makes looping over the prototype unnecessary.

Taking advantage of the above mentioned points we can implement extend and create as follows:

Function.prototype.extend = function (body) {
    var constructor = function () {};
    var prototype = constructor.prototype = new this;
    body.call(prototype, this.prototype);
    return constructor;
};

Function.prototype.create = function () {
    var instance = new this;
    instance.constructor.apply(instance, arguments);
    return instance;
};

That's all. Using two simple functions we may now create classes as follows:

var Person = Object.extend(function () {
    this.constructor = function (isDancing) {
        this.dancing = isDancing;
    };

    this.dance = function () {
        return this.dancing;
    };
});

var Ninja = Person.extend(function (base) {
    this.constructor = function () {
        base.constructor.call(this, false);
    };

    this.swingSword = function () {
        return true;
    };
});

To create an instance of the class we use create as follows:

var p = Person.create(true);
p.dance(); // => true

var n = Ninja.create();
n.dance(); // => false
n.swingSword(); // => true

// Should all be true
p instanceof Person && p instanceof Object &&
n instanceof Ninja && n instanceof Person && n instanceof Object

This code is similar to my very own augment function. However augment makes use of new instead of create. Because of the way JavaScript works, using new is the fastest way to create an instance which is why augment doesn't have create. Nevertheless if you want it, a functional version of new can be easily implemented as follows:

function Factory(constructor, args) {
    return constructor.apply(this, args);
}

Function.prototype.new = function () {
    Factory.prototype = this.prototype;
    return new Factory(constructor, args);
};

For more information read the following answer on StackOverflow.


Script to load controllers dynamically for AngularJS I would like to hear your thoughts, idea's or feedback on the following code.

Ive added to code to github.

https://github.com/redbullzuiper/angularjs-dynamic-controllers

Usually when you attach a controller to a DOM element using ng-controller="myController", you need to include the controller somewhere in your page; for instance:

<script src="src/application/controllers/my-controller.js"></script>

However, when including the script I wrote in the head, it will traverse across the DOM and search for all ng-controller attributes.

It will then automatically append the correct controller into the head of the page. After it's done including all the controllers, it will manually bootstrap the application using:

angular.bootstrap(document, ['module_name']);

This way all controllers are getting loaded dynamically, without the need of including all controllers in your head. But they will instead only get loaded when called.

Can't create a demo using a fiddle, but a working demo can be found here

The code:

window.onload = function()
{
    var promises = [];
    var ngApp = document.querySelectorAll('[ng-app]')[0];
    var ngControllers = document.querySelectorAll('[ng-controller]');

    // Throw warning if angular is not found
    if(typeof angular == 'undefined')
    {
        console.warn("simplify error: Angular not found, operation canceled.");
        return;
    }

    // Throw warning if ng-app directive exists. Script will bootstrap the application manually
    if(ngApp)
    {
        console.warn("Please remove the ng-app directive. `Simplify` will bootstrap the application manually.");
        console.warn("This will also most likely fix the 'Uncaught Error: [$injector:modulerr]' error.");
    }

    // Append the scripts
    for(var i = 0;i < ngControllers.length;i++)
    {
        var promise = new Promise(function(resolve, reject) {
            var src = 'src/application/controllers/'+ ngControllers[i].getAttribute('ng-controller') + '.controller.js';
            var script = document.createElement('script');
                script.setAttribute('src', src);

            document.head.appendChild(script);

            script.addEventListener('load', function() {
                resolve(script);
            });
        });

        // Push promises to array to resolve them all together later on
        promises.push(promise);
    }

    // Resolve all promises then bootstrap the app
    // Without the use of promises, the bootstrap will start before all scripts are included
    // This results into an erro
    Promise.all(promises).then(function() {
        angular.bootstrap(document, ['app']);
    });
}

Feedback

This is an interesting way of loading controllers. I like the application of the promises. It does feel a little weird not to have an ng-app directive but it makes sense given the nature of your code.

Suggestions

The suggestions below can simplify the code, but may not necessarily be optimal or have desired browser support.

Iterating functionally

One could utilize a functional approach instead of the declarative for loop:

for(var i = 0;i < ngControllers.length;i++)

Because each iteration pushes a promise into the array promises, that loop can be replaced by a call to Array.map():

var promises = Array.prototype.map.call(ngControllers, function(ngController)

Or .map could be called on an array after converting the nodeList to an array via Array.from() (but beware of Browser compatibilty on that)

var promises = Array.from(ngControllers).map(function(ngController)

Or for a completely es-6 approach, use the spread operator:

var promises = [...ngControllers].map(function(ngController)

At the end of each iteration, simply return the promise instead of pushing it into the array.

    // Push promises to array to resolve them all together later on
    return promise;//promises.push(promise);
}); //end the call to .map()

Bear in mind that the functional approach would likely be somewhat slower because a function call is added for each iteration, but for a small number of controllers this should be a negligible difference. If you would like to become more familiar with functional JavaScript, I recommend these exercises.

Use partial(-ly applied) functions for callbacks

Instead of making a closure just to call resolve():

script.addEventListener('load', function() {
    resolve(script);
});

Use Function.bind() (different than (angular.bind()) to create a partially applied function for each time resolve() is called.

script.addEventListener('load', resolve.bind(null, script));

Bear in mind that the event listener would pass an event argument to the callback, but that should be the second argument when script is bound as the first argument.

The same is true for calling the bootstrap function after all promises have been resolved:

Promise.all(promises).then(function() {
    angular.bootstrap(document, ['app']);
});

Can be simplified like below:

Promise.all(promises).then(angular.bootstrap.bind(null, document, ['app']));

Use addEventListener instead of onload

This likely wouldn't be an issue in a small application but in case there was other code (potentially multiple functions) that needed to be executed when the page is loaded, it might be simpler just to use window.addEventListener('DOMContentLoaded', function() {...}) instead of window.onload = function() {...};


Pig Latin Translator Could someone review the following code?

class PigLatin

  attr_accessor :phrase
  ending = "ay"

  def initialize the_phrase
    @phrase = the_phrase
  end

  def translate
    translated_phrase = String.new
    words = @phrase.split
    words.each do |word|
      puts word
      if vowel_is_first word
        translated_phrase +=  translate_with_vowel(word)
      else
        translated_phrase += translate_with_consonant(word)
      end
    end

    clean_up translated_phrase
  end

  def vowel_is_first test_word
    true if test_word[0] =~ /aeiou/ #will this make false since 0??
    false
  end

  def translate_with_vowel word
    word + "-" + ending + " "
  end

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    split_location = word =~ /a|e|i|o|u/
    word_array_length = word.size - 1

    first_segment = word[split_location,word_array_length]
    second_segment = "-" + word[0,split_location-1] + word[split_location-1]  + "ay" + " "

    first_segment + second_segment
  end

  def clean_up to_be_cleaned
    to_be_cleaned = to_be_cleaned[0..to_be_cleaned.size-2]
    to_be_cleaned.capitalize
  end
end

I'm sure it could use some work as I'm just getting going in Ruby, so anything and everything would be helpful. The clean-up function, I think, could especially use work.  It gets the job done, but w/o it there is always a space at the end, and str.strip wasn't getting the job done.

First of all there are two mistakes in the code:

ending = "ay" is a local variable only visible directly in the class body, not inside of the defs. For this reason translate_with_vowel will throw a NameError when it is called. You should make it a constant, so it'll be visible everywhere.
The reason that you did not notice the first mistake is that translate_with_vowel is never called because vowel_is_first will always return false. You should use if ... else ... end instead of the inline if. The reason that it doesn't work the way you wrote it is that since true if test_word[0] =~ /aeiou/ is not the last expression in the method, so its return value simply gets discarded and it doesn't have any side-effects.

From a design perspective, I'm a bit doubtful that having a PigLatin object for each string you want to translate is really a good idea. Each PigLatin object has one instance variable, which is set in initialize and used by exactly one method (translate). All other methods only depend on their arguments and not on the state of the object. So from a programmer's point of view, having the string encapsulated in an object buys you nothing over taking it as an argument to translate.

From the user's point of view it also makes the API more cumbersome than it needs to be. There's no reason to prefer PigLatin.new(string).translate over PigLatin.translate(string) if there's no reason why I'd ever call more than one method on an object, there really is no reason for the object to exist at all.

If the translation was parametrized by some configuration options, it would make sense to have a class which is instantiated with the options and a translate method which takes the string, like this:

pig_latin = Translator.new(:input => :english, :output => :pig_latin)
string1 = pig_latin.translate("Hello World")
string2 = pig_latin.translate("Goodbye World")

However since in this case there are no options, I'd recommend using a PigLatin module and defining all the methods as module_functions. This way the usage would be PigLatin.translate("Hello World") or include PigLatin; translate("Hello World").

Also since there's no reason why a user should ever call any of your methods except translate, the other methods should probably be private.

  def translate
    translated_phrase = String.new
    words = @phrase.split
    words.each do |word|
      puts word
      if vowel_is_first word
        translated_phrase +=  translate_with_vowel(word)
      else
        translated_phrase += translate_with_consonant(word)
      end
    end

    clean_up translated_phrase
  end

I assume the puts is just an old debug statement you forgot to remove. If it isn't, be advised that mixing IO with logic is usually bad design. If it is, a little tip: If you use p instead of puts for debug statements, you will get output which is more useful because you can see whether the object has the type you expect and, in case of strings, whether it contains any unprintable meta characters.

String.new is just a more verbose way to say "", so you should use that instead. However instead of appending to an empty string in an each loop, I'd rather use map and join. join will also take care of inserting spaces between the elements for you (if you tell it to, that is), so you don't need to add those spaces yourself in the translate_ methods and you don't have to remove the space at the end.

So I'd write translate like this:

 def translate(string)
    words = @phrase.split
    translated_words = words.map do |word|
      if vowel_is_first word
        translate_with_vowel(word)
      else
        translate_with_consonant(word)
      end
    end
    translated_phrase = translated_words.join(" ")
    translated_phrase.capitalize
  end

You can also define another helper method translate_word which contains the above if statement and then simplify the above to translated_words = words.map(&:translate_word).

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    split_location = word =~ /a|e|i|o|u/
    word_array_length = word.size - 1

    first_segment = word[split_location,word_array_length]
    second_segment = "-" + word[0,split_location-1] + word[split_location-1]  + "ay" + " "

    first_segment + second_segment
  end

The whole split_location is overly complicated. Just use String#split (which accepts an integer as a second argument, which you can use to tell it that you only want to split the string into two parts). You may also want to use string interpolation instead of concatenation. And as I said earlier, you can get rid of the + " " as that's now handled by join.

I'd write it like this:

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    second_segment, first_segement = word.split(/a|e|i|o|u/, 2)

    "#{ first_segment }-#{ second_segment }ay"
  end


Print tree level by level including Empty node Here is my code to print tree in a level by level way. It needs to print an empty node as well to keep tree structure like full binary tree for elegancy reasons.

I'm wondering if anything you think could improve, including the efficiency and not sure if any bugs (I did testing, not found bugs). I'm especially wondering if for part of code -- "print the remaining all empty leaf node part" could be optimized (I mean if any smarted way to optimize the while loop to cover all logic, and skip this code snippet especially handling the last level).

The tree I tested in this example looks like:

 1
/ \
2  3
/\ /\
4  5 6

Here is the related output:

[1]
[2, 3]
[4, '$', 5, 6]
['$', '$', '$', '$', '$', '$', '$', '$']

class Node:
    def __init__(self):
        self.value = 0
        self.left = None
        self.right = None

def printTree(root):
    buf = []
    output = []
    if not root:
        print '$'
    else:
        buf.append(root)
        count = 1
        nextCount = 0
        while count > 0:
            node = buf.pop(0)
            if node:
                output.append(node.value)
                count -= 1
            else:
                output.append('$')
            if node and node.left:
                buf.append(node.left)
                nextCount += 1
            else:
                buf.append(None)
            if node and node.right:
                buf.append(node.right)
                nextCount += 1
            else:
                buf.append(None)
            if count == 0:
                print output
                output = []
                count = nextCount
                nextCount = 0
        # print the remaining all empty leaf node part
        for i in range(len(buf)):
            output.append('$')
        print output

if __name__ == "__main__":
    root = Node()
    root.value = 1
    leftNode = Node()
    leftNode.value = 2
    root.left = leftNode
    rightNode = Node()
    rightNode.value = 3
    root.right = rightNode
    leftleftNode = Node()
    leftleftNode.value = 4
    leftNode.left = leftleftNode
    rightleftNode = Node()
    rightleftNode.value = 5
    rightNode.left = rightleftNode
    rightrightNode = Node()
    rightrightNode.value = 6
    rightNode.right = rightrightNode

    printTree(newRoot)

A few small changes:
Python has a styleguide, PEP8, which programmers are recommended to follow. One of its recommendations is to use lower_case names for both variables and functions. camelCase is also accepted, if applied consistently (which it is in your code).

Your if conditions in the while loop repeatedly check if node exists. Just put all of them in if node:. Additionally, the checks for node.left and node.right are basically identical, they can be combined by looping over for n in [node.left, node.right]:.

Whenever you do a buffer.pop(0) you probably want a collections.deque. While for lists, popping on the left is O(n) and O(1) on the right, both operations are O(1) for a deque.

Python has the nice feature of multiple assignments, allowing things like:

count, nextCount = nextCount, 0

You are always creating an instance of a node and directly afterwards you assign the value. It is better to add a parameter for the value to the class definition:

class Node:
    def __init__(self, value=0):
        self.value = value
        self.left = None
        self.right = None

With these changes I get this code:

from collections import deque

class Node:
    def __init__(self, value=0):
        self.value = value
        self.left = None
        self.right = None

def printTree(root):
    buf = deque()
    output = []
    if not root:
        print '$'
    else:
        buf.append(root)
        count, nextCount = 1, 0
        while count:
            node = buf.popleft()
            if node:
                output.append(node.value)
                count -= 1
                for n in (node.left, node.right):
                    if n:
                        buf.append(n)
                        nextCount += 1
                    else:
                        buf.append(None)
            else:
                output.append('$')
            if not count:
                print output
                output = []
                count, nextCount = nextCount, 0
        # print the remaining all empty leaf node part
        output.extend(['$']*len(buf))
        print output

if __name__ == "__main__":
    root = Node(1)
    leftNode = Node(2)
    root.left = leftNode
    rightNode = Node(3)
    root.right = rightNode
    leftleftNode = Node(4)
    leftNode.left = leftleftNode
    rightleftNode = Node(5)
    rightNode.left = rightleftNode
    rightrightNode = Node(6)
    rightNode.right = rightrightNode

    printTree(newRoot)

This is still not very efficient code. If you give Node a magic method to decide if it is True or False, your code can be greatly simplified. This method is called __nonzero__ in python 2.x and __bool__ in python3.x.

class Node:
    def __init__(self, value='$', left=None, right=None):
        self.value = value
        self.left = left
        self.right = right

    def __nonzero__(self):
        return self.value != '$'

    def __str__(self):
        buf, out = [self], []
        while buf:
            out.append("{}".format([node.value for node in buf]))
            if any(node for node in buf):
                children = []
                for node in buf:
                    for subnode in (node.left, node.right):
                        children.append(subnode if subnode else Node())
                buf = children
            else:
                break
        return "\n".join(out)

if __name__ == "__main__":
    root = Node(1, Node(2, Node(4)), Node(3, Node(5), Node(6)))
    print root

This gets rid of all the counting and so on. It does, however, not print the last line of all missing nodes (which seemed odd to me anyways). Normally I would use None for an empty Node, not '$', but it also works this way.

I also added left and right, as parameters to the Node, allowing a nested definition, which might be less readable if it becomes too big, but might come handy, when quickly generating a tree like here.

If you also add the magic method __str__ to Node, you can just do print root (or any other node and all its subnodes).

If you insist on having each level printed like a list (to ease copy&pasting it), replace the line

out.append(" ".join(str(node.value) for node in buf))

with

out.append("{}".format([node.value for node in buf]))

If you want to learn more about magic methods, have a look here.


LeetCode: FloodFill Rust Hi I'm implementing a few leetcode examples in Rust. https://leetcode.com/problems/flood-fill/
(looks to be same question as LeetCode: FloodFill C#)
The input is one matrix, a position and its new colour. The output is the modified matrix
// Input:
// image = [[1,1,1],[1,1,0],[1,0,1]]
// sr = 1, sc = 1, newColor = 2
// Output: [[2,2,2],[2,2,0],[2,0,1]]
// Explanation:
// From the center of the image (with position (sr, sc) = (1, 1)), all pixels connected
// by a path of the same color as the starting pixel are colored with the new color.
// Note the bottom corner is not colored 2, because it is not 4-directionally connected
// to the starting pixel.

I use q.pop_front() to fill the neighbouring cells first. Is VecDeque the best data structure for this que?
I do a lot of casting from between i32 and usize as I want to be able to check if index is out of bounds by subtracting 1. Is there a better way?
Is if (0..image.len()).contains(x1) better than if x1>=0 && x1<image.len() for the range check?
use std::collections::VecDeque;

  fn flood_fill(mut image: Vec<Vec<i32>>, sr: i32, sc: i32, new_color: i32) -> Vec<Vec<i32>> {

      let mut q:VecDeque<(i32,i32)> = VecDeque::new();
      q.push_back((sr,sc));

      let c0 = image[sr as usize][sc as usize];
      if c0 != new_color {
          while ! q.is_empty() {
              if let Some((sr,sc))=q.pop_front() {
                  if image[sr as usize][sc as usize] == c0 {
                      image[sr as usize][sc as usize] = new_color;
                      for delta in vec![(-1,0),(1,0),(0,-1),(0,1)] {
                          let new_r:i32 = sr+delta.0;
                          let new_c:i32 = sc+delta.1;
                          if (0..image.len() as i32).contains( &new_r ) && (0..image[0].len() as i32).contains( &new_c){
                              q.push_back((new_r,new_c));
                          }
                      }
                  }
              }
          }
      }
      image
  }

#[cfg(test)]
mod test{
#[test]
    fn test_lc_default(){
        assert_eq!(super::flood_fill(vec![vec![1 as i32 ,1,1],vec![1,1 as i32,0],vec![1,0,1]],1, 1, 2),vec![vec![2,2,2],vec![2,2,0],vec![2,0,1]]) ;
    }
}

It looks like I could use some memory optimization too.

Formatting
I assume the indentation to be a copy-paste error when removing the
boilerplate struct Solution mandated by LeetCode.
Run cargo fmt on your code to conform to the standard Rust
formatting guidelines.
Interface
LeetCode made a poor choice by using Vec<Vec<i32>> to represent a
two-dimensional array, which is generally inefficient as it stores
elements in multiple segmented allocations, incur double indirection
costs, and fail to maintain proper structure (against jagged arrays).
We can't do anything against this, though, so I'll ignore it for the
rest of the post.
Similarly, i32 is not ideal for color representation — a
dedicated type (e.g., struct Color(i32);) is clearly superior.  At
the very least, we can somehow improve readability by using a type
alias and use it consistently for colors in the code:
type Color = i32;

i32 vs. usize
Generally, it is preferred to store and compute indexes in usize,
which is the natural type for this purpose.  One way you can avoid
subtracting 1 is by using usize::MAX and wrapping_add.
To convert from the i32 values that the function receives,
try_from is preferred over as, since the latter silently
truncates invalid values.  unwrap can be used here since it's for
LeetCode; in real-world code, the error should be handled accordingly.

Is if (0..image.len()).contains(x1) better than if x1>=0 && x1<image.len() for the range check?

I'd say yes.  It indicates the intent more clearly.
Simplifying control flow
Instead of wrapping everything in an if expression, check for
image[sr][sc] == new_color and return early.
while let
This:
while !cells.is_empty() {
    if let Some((sr, sc)) = cells.pop_front() {

is just a convoluted way of saying
while let Some((sr, sc)) = cells.pop_front() {

Here's how I modified your implementation:
type Color = i32;

pub fn flood_fill(
    mut image: Vec<Vec<Color>>,
    sr: i32,
    sc: i32,
    new_color: Color,
) -> Vec<Vec<Color>> {
    use std::collections::VecDeque;
    use std::convert::TryFrom;

    let sr = usize::try_from(sr).unwrap();
    let sc = usize::try_from(sc).unwrap();

    let initial_color = image[sr][sc];

    if initial_color == new_color {
        return image;
    }

    let height = image.len();
    let width = image[0].len();

    let mut cells: VecDeque<(usize, usize)> = VecDeque::new();
    cells.push_back((sr, sc));

    while let Some((sr, sc)) = cells.pop_front() {
        let cell = &mut image[sr][sc];

        if *cell != initial_color {
            continue;
        }

        *cell = new_color;

        const OFFSETS: &[(usize, usize)] = &[(0, usize::MAX), (usize::MAX, 0), (0, 1), (1, 0)];

        for (delta_r, delta_c) in OFFSETS.iter().copied() {
            let new_r = sr.wrapping_add(delta_r);
            let new_c = sc.wrapping_add(delta_c);

            if new_r < height && new_c < width {
                cells.push_back((new_r, new_c));
            }
        }
    }

    image
}


Generating 1000 random numbers I have got the following C code and I feel it is crazy stupid and there must be a better way to do this, but just can't think of one and have yet to learn algorithms.

/*generates 1000 random numbers, lists frequency*/
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(void)
{
    int count, generated_num ;
    char nums[10] = {0};
    srand(time(NULL));
    for (count = 0; count < 1000; count++) {
        generated_num = rand() % 10 + 1;
        nums[generated_num-1] += 1;
    }

    for (count = 0; count < 10; count++) {
        printf("%d occured %d times\n", count+1, nums[count]);
    }

    return 0;
}

How do I learn to write code that is both efficient in speed but also efficient in the sense it is readable/modifiable/not-a-mess?

Speed and efficiency

Just don't worry about it.  You need to generate 1000 random numbers, and do a little bit of accounting work.  Any reasonable solution, such as yours, will perform similarly.

Types

Why use an array of char to keep track of the number of occurrences?  You're putting 1000 random numbers into 10 bins.  What if one of those bins gets more than 127 items?  You would get an overflow.

In summary, those bins should be of type int.  Don't even think about skimping on a few bytes of storage, if you want your code to be not-a-mess.

Naming

nums actually stores counts.  count, as you've used it in the second loop, actually refers to the generated numbers.  I suggest the following renaming:

nums → bins
count → i


Bishop game - python A bishop can capture any other bishop if it lies along the same
  diagonal line, so long as no other bishop is in its path.
  Given a list of coordinates of the bishops locations, and a board size, determine how many bishops are unsafe.

This code has a time complexity of \$O(N*K)\$ where N is the number of bishops, K is (N- current index).

Is there a way to improve this?

bishops = [(0, 0),
           (1, 1),
           (0, 2),
           (1, 3),
           (2, 0),
           (2, 2)]
size = 5

moves = [(1, 1), (1, -1), (-1, 1), (-1, -1)]

captured = []
for index, coordinates in enumerate(bishops):
    remaining = bishops[index + 1:]
    seen = bishops[:index + 1]
    for dx, dy in moves:
        x, y = coordinates
        while 0 <= x + dx < size and 0 <= y + dy < size:
            x += dx
            y += dy
            if (x, y) in seen:
                break
            if (x, y) in remaining:
                remaining.remove((x, y))
                capture = f"{coordinates} takes {x, y}"
                captured.append(capture)
                break

print(f"There are {len(captured)} moves")
print(*captured, sep = "\n")

O(n)

You can transform the (row,column) coordinate of a bishop to a (row+column, row-column) coordinate. The row+column coordinate tells you which upper-left to lower right diagonal the bishop is on.  The row-column coordinate tells you which upper-right to lower-left diagonal the bishop is on.

      row + col                     row - col
7  8  9 10 11 12 13 14       7  6  5  4  3  2  1  0
6  7  8  9 10 11 12 13       6  5  4  3  2  1  0 -1
5  6  7  8  9 10 11 12       5  4  3  2  1  0 -1 -2
4  5  6  7  8  9 10 11       4  3  2  1  0 -1 -2 -3
3  4  5  6  7  8  9 10       3  2  1  0 -1 -2 -3 -4
2  3  4  5  6  7  8  9       2  1  0 -1 -2 -3 -4 -5
1  2  3  4  5  6  7  8       1  0 -1 -2 -3 -4 -5 -6
0  1  2  3  4  5  6  7       0 -1 -2 -3 -4 -5 -6 -7

Now create two dictionaries.  One that maps each row+col value to a list of bishops on that diagonal.  The other one similarly maps the row-col value.  Any dict entry with more than one bishop, means all of those bishops are in danger.

bishops = [(0, 0),
           (1, 1),
           (0, 2),
           (1, 3),
           (2, 0),
           (2, 2)]

nwse = {}
nesw = {}

for row,col in bishops:
    nwse_index = row + col
    if nwse_index not in nwse:
        nwse[nwse_index] = [(row,col)]
    else:
        nwse[nwse_index].append((row,col))

    nesw_index = row - col
    if nesw_index not in nesw:
        nesw[nesw_index] = [(row,col)]
    else:
        nesw[nesw_index].append((row,col))

in_danger = set()

for k,v in nwse.items():
    if len(v) > 1:
        in_danger.update(v)

for k,v in nesw.items():
    if len(v) > 1:
        in_danger.update(v)

safe_bishops = set(bishops) - in_danger

print(len(safe_bishops))

Improved code

based on AJNeuman's answer and my comment below.

The row - col index is offset by adding -8 so that all the values will be < 0.
The row + col index are all > 0. So just one dict is needed.

from collections import defaultdict

bishops = [(0, 0),
           (1, 1),
           (0, 2),
           (1, 3),
           (2, 0),
           (2, 2)]

occupied_diagonals = defaultdict(list)

for bishop in bishops:
    row, col = bishop
    occupied_diagonals[row + col].append(bishop)
    occupied_diagonals[row - col - 8].append(bishop)

in_danger = set()

for bishop_list in occupied_diagonals.values():
    if len(bishop_list) > 1:
        in_danger.update(bishop_list)

print(len(in_danger))


Progress indication with animation I have a game in which the player has to do something within a defined time.

E.g.: the player has 10 seconds to complete a level.

My current code animates an imageview (it's only a horizontal line) when the game starts:

progressAnimation = new TranslateAnimation(0, -Settings.screenWidth, 0, 0);
progressAnimation.setDuration(level * 10000);
progressAnimation.setFillAfter(true);
progressAnimation.setInterpolator(new LinearInterpolator());

progressView = new ProgressView(context);
progressView.startAnimation(progressAnimation);

Simultaneously I start a handler:

handler.postDelayed(new Runnable() {
    @Override
    public void run() {
        fireResultActivity(-1); // -1 means, time's up.
    }
}, level * 10000);

If the user can finish the game within 10 seconds an activity comes up and congratulates, if they can't another, "fail" activity comes up.

Is it a good practice to use animation to indicate the progress and start a handler simultaneously?

Could you recommend other (better) solution?

First of All you should be using setAnimationListener on animation object. so no need of Handler

progressAnimation.setAnimationListener(new Animation.AnimationListener() {
    @Override
    public void onAnimationStart(Animation animation) {

    }

    @Override
    public void onAnimationEnd(Animation animation) {
       fireResultActivity(-1); // call your method here
    }

    @Override
    public void onAnimationRepeat(Animation animation) {

    }
});

As it has built-in listener for Animation, there is no need to create new thread using handler


C++ Dirent.h "Wrapper" I do not want to call this a wrapper, but no other name comes to mind right now, so I'll use it incorrectly for the rest of the question - It's just a class that adds C++ functions to dirent.h but it's very specific to my use case.

All of the code works well, but I've never worked with the dirent.h library and jumping right into a C++ wrapper is something, so I'm really insecure about it's performance and memory usage in practical scenarios.

~But, of course please also judge the readability - I want to know if I should add comments or if my naming scheme and layout is self explanatory.

Quick Description

I'm working on C++ based file manager that needs functions that return vectors of whole directories, of course, that it can display it in one loop. It will also need some more info (metadata I guess), like it's type so it can do all the fancy color syntax.

Here is the final version so far (with some debug code):

inode.h

#ifndef INODECLASS
#define INODECLASS

#include <dirent.h>
#include <string.h>

#include <iostream>

#include <vector>
#include <string>

class Inode
{
public:
  struct InodeMeta {
    std::string name;
    std::string path;
    unsigned char type;
  };

private:
  std::string pwd_path_;
  std::vector<InodeMeta *> buffer_;

public:
  Inode(const std::string &);
  ~Inode();

  void ClearBuffer();
  bool ReloadBuffer(const std::string &);

  std::vector<InodeMeta> DumpBuffer(const std::string &);
};

#endif

inode.cc

#include "inode.h"

Inode::Inode(const std::string &arg_path = ".") {
  if ( !ReloadBuffer(arg_path) )
    ClearBuffer();
}

Inode::~Inode() {
  ClearBuffer();
}

void Inode::ClearBuffer() {
  for (auto &item : buffer_)
    delete item;
  buffer_.clear();
}

bool Inode::ReloadBuffer(const std::string &arg_path = "") {
  if (arg_path.length() != 0) pwd_path_ = arg_path;
  DIR *dirstream;
  dirent *diritem;

  ClearBuffer();

  if ( (dirstream = opendir(pwd_path_.c_str())) != NULL )
  {
    while ( (diritem = readdir(dirstream)) != NULL ) {
      if ( strcmp(diritem->d_name, ".") != 0 &&
           strcmp(diritem->d_name, "..") != 0 )

        buffer_.emplace_back( new InodeMeta{ std::string(diritem->d_name),
          std::string(pwd_path_) + "/" + std::string(diritem->d_name),
      diritem->d_type} );
    }
    closedir(dirstream);
  }

  else return false;
  return true;
}

std::vector<Inode::InodeMeta> Inode::DumpBuffer(const std::string &arg_path = "") {
  if (arg_path.length() != 0) ReloadBuffer(arg_path);

  std::vector<InodeMeta> ret_list;

  for (auto &item : buffer_)
    ret_list.push_back(*item);

  return ret_list;
}

//DEBUG ONLY//

#include <iostream>

int main() {
  Inode I;

  std::vector<Inode::InodeMeta> inodelist = I.DumpBuffer();

  for (auto &item : inodelist)
    std::cout << item.name << " ; " << item.path << '\n';

  I.ReloadBuffer("/tmp/");
  inodelist.clear();
  inodelist = I.DumpBuffer();

  for (auto &item : inodelist)
    std::cout << item.name << " ; " << item.path << '\n';
}

Just in case you either want to see the whole project (I started it like 3 days ago, don't judge me too hard), or just make a quick git clone and test it yourself here.

Parameter names
This is one of my pet peeves:
Inode(const std::string &);

The point of a header file / class definition at least in my head is to separate the interface to your code from the implementation of your code.  Not putting parameter names in your function declarations makes your interface incomplete.  Sure, you know what the parameter is, but your wrote the code.  From reading your class header I don't have a clue what the string might represent, there's no comments to tell me, no name, so I'm either relying on your documentation (?) or have to look at the implementation to figure it out.
Default parameters
Default parameters only effect code that can see them.  You've defined default parameters for your methods, but only in your implementation file.  I find that a little odd.  If it's a good default, it should be in the header.
Naming
Descriptive naming is important and significantly reduces the learning time for people new to your code.  I had to look Inode up to see what it was.  It probably makes sense if you have experience in that area, but as an implementation of a folder structure it's not the first name I'd be looking for.  pwd to me means 'print working directory', so pwd_path_ seems like an odd name.  Perhaps wd_path would be better.  buffer_ is very non-descript.  It's a list of metadata, why not call it that?
You have a similar issue with your method naming.  DumpBuffer doesn't really say 'read the directory and return the meta data to me`.
Blank Lines
Some of your blank lines don't really make sense to me, they make your code harder to read.  Consider this:
if ( strcmp(diritem->d_name, ".") != 0 &&
           strcmp(diritem->d_name, "..") != 0 )

        buffer_.emplace_back( new InodeMeta{ std::string(diritem->d_name),
          std::string(pwd_path_) + "/" + std::string(diritem->d_name),
      diritem->d_type} );

You set up an unbraced if statement and then leave a blank line between the if and the code that belongs to it.  This is an invitation to be misinterpreted. I feel much the same about the else condition at the end of the function:
}

  else return false;

You close the if, leave a blank line then put the else.  It makes the else feel detached.
Pointers
What are you getting from storing pointers in your buffer_, rather than instances of InodeMeta.  Since you return copies anyway from your DumpBuffer method, it feels like you've just added extra handling requirements for little to no benefit?  If you want to save duplication then storing and returning a shared_ptr instead might be more efficient.
Hidden Behaviour
I'm not sure that having DumpBuffer behave differently depending on whether or not you pass it an empty string is a great idea.  If I pass in "/someFolder" then the folder is read.  If I pass it in again, "/someFolder", then the folder is read again (so any new files would be noticed for example).  If instead for the second call I had passed in "" then the original folder contents would have been returned.  If this is desired behaviour, then I'd at least put a comment in the header / documentation because it's not what I would expect.


Taking a list of numbers and finding the two largest The code ive written takes the user entered numbers and sorts them into a list and finds the two biggest numbers, im looking to make the code less verbose. any help/advice is greatly appreciated.

def printTwoLargest():
    numList = []
    num = 'holder'
    x=0
    y=0
    while num != '':
        num = input('Please enter a number: ')
        if num != '':
            numList.append(float(Decimal(num)))
        else:
            break
    numList.sort(reverse=True)
    for i in range(len(numList)):
        if numList[i]>x:
            x = numList[i]
        elif numList[i]>y:
            y = numList[i]
        else:
            pass
    if x>0 and y>0:
        print('The largest number is:',x)
        print('The second largest number is:',y)
    elif x>0:
        print('The largest number is:',x)
    else:
        print('No positive numbers were entered.')

You should change your function to take a list and return the two largest of it.
And so you should move the creation of numList out into it's own function.
You should also move the printing out of the function too, and instead return a list of the two largest.
This should spilt your code up to achieve:

def getNumbers():
    numList = []
    num = 'holder'
    while num != '':
        num = input('Please enter a number: ')
        if num != '':
            numList.append(float(Decimal(num)))
        else:
            break
    return numList

def twoLargest(numbers):
    x=0
    y=0
    numbers.sort(reverse=True)
    for i in range(len(numbers)):
        if numbers[i] > x:
            x = numbers[i]
        elif numbers[i] > y:
            y = numbers[i]

    if y > 0:
        return x, y
    if x > 0:
        return x,
    return (,)

if __name__ == '__main__':
    numbers = getNumbers()
    largest = twoLargest(numbers)
    print(largest)

However you can improve getNumbers, rather than checking to break from the loop twice, you can instead check once.
This removes the need for num = 'holder', and can reduce the amount of lines needed.

def getNumbers():
    numList = []
    while True:
        num = input('Please enter a number: ')
        if num == '':
            break
        numList.append(float(num))
    return numList

However it can further be simplified by making it a generator function.
To do this you can just yield the values, rather than numList.append them.

def getNumbers():
    while True:
        num = input('Please enter a number: ')
        if num == '':
            break
        yield float(num)

Moving onto twoLargest, it doesn't work with a generator, as you don't cast numList to a list.
You also use sort the list in descending order, so rather than using a loop to find the largest two, you should instead use slicing.
This can get you:

def twoLargest(numbers):
    return sorted(numbers, reverse=True)[:2]

Going forward, you can make an 'n' largest function.
But rather than using sorted you can use; heapq.nlargest, and max.
Making the interface as standard as possible you can get:

from heapq import nlargest

def nLargest(numbers, n=1, key=None, sort=False):
    kwargs = {'key': key}
    if key is None:
        del kwargs['key']
    if n == 1:
        try:
            return [max(numbers, **kwargs)]
        except ValueError: # if passed an empty iterable
            return []
    if sort:
        return sorted(numbers, reverse=True, **kwargs)[:n]
    else:
        return nlargest(n, numbers, **kwargs)

The reason for the three methods is described under nlargest:

  The latter two functions perform best for smaller values of n.
  For larger values, it is more efficient to use the sorted() function.
  Also, when n==1, it is more efficient to use the built-in min() and max() functions.
  If repeated usage of these functions is required, consider turning the iterable into an actual heap.


Realizing a SQL ResultSet into a Map in Scala I am trying to realize a java.sql.ResultSet into a map, in Scala.

import java.sql.{ResultSet, ResultSetMetaData}

class DbRow extends java.util.HashMap[java.lang.String, Object] {
}

object freeFunctions {

  def realize(queryResult: ResultSet): Vector[DbRow] = {
    val md = queryResult.getMetaData
    val colNames = for (i <- 1 to md.getColumnCount) yield md.getColumnName(i)
    var rows: Vector[DbRow] = Vector.empty
    while (queryResult.next()) {
      val row = new DbRow
      for (n <- colNames) {
        row.put(n, queryResult.getObject(n))
      }
      rows = rows :+ row
    }
    rows
  }

}

I feel this could (should) be less verbose. If while/yield comprehensions existed, or some
comprehension for create a map. The client code needs a java map for now, but if an elegant
enough solution can produce a Scala map, I could convert it.

I don't have the time to test this, so I hope you can do that for me. I decided not to use HashMap straight away because type conversion makes things a bit more complicated than they should, but let me know if this is important for you and causes you trouble.

Here's option 1:

def realize(queryResult: ResultSet): Vector[Map[String, Object]] = {
    val md = queryResult.getMetaData
    val colNames = for (i <- 1 to md.getColumnCount) yield md.getColumnName(i)
    val buildMap = () => (for (n <- colNames) yield n -> queryResult.getObject(n)).toMap
    Iterator.continually(queryResult.next()).takeWhile(identity).map(_ => buildMap()).toVector
}

I don't like it much because some parts of the code rely on side effects of other parts of the code, so even if it works, it's still ugly.

Here's option 2, which I like more:

def buildMap(queryResult: ResultSet, colNames: Seq[String]): Option[Map[String, Object]] =
    if (queryResult.next())
        Some(colNames.map(n => n -> queryResult.getObject(n)).toMap)
    else
        None

def realize(queryResult: ResultSet): Vector[Map[String, Object]] = {
    val md = queryResult.getMetaData
    val colNames = (1 to md.getColumnCount) map md.getColumnName
    Iterator.continually(buildMap(queryResult, colNames)).takeWhile(!_.isEmpty).map(_.get).toVector
}

Nicer, IMHO, you can save some dots over there but I decided to keep them. Maybe something like this:

 (Iterator continually (buildMap(queryResult, colNames)) takeWhile (!_.isEmpty) map (_.get)).toVector

Again, I didn't test it, I don't know how to do it without a DB and I don't feel like setting one up now, sorry for that.

Finally, I'm not sure if you realized this, but "realize" is probably not the best name for this method (or for any method, unless you are programming some epistemic system, IMHO).


Find Binary Tree Path Sum I am trying to resolve following problem. Please advice which part of the code can be improved.
Given a binary tree and a sum, determine if the tree has a root-to-leaf path such that adding up all the values along the path equals the given sum.
Example:
Given the below binary tree and sum = 22,
      5
     / \
    4   8
   /   / \
  11  13  4
 /  \      \
7    2      1

return true, as there exist a root-to-leaf path 5->4->11->2 which sum is 22.
Implementation:
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

typedef struct stTree
{
    struct stTree * left;
    struct stTree * right;
    int value;
} stTree;

struct stTestCase
{
   stTree *root;
   int sum;
   bool result;
};

stTree* createNode(int value, stTree *leftNode, stTree *rightNode)
{
    stTree *node = malloc(sizeof *node);
    if (!node) abort();
    node->left = leftNode;
    node->right = rightNode;
    node->value = value;
    return node;
}

bool hasPathSum(const stTree *root, int sum)
{
    if (!root) return false;

    if (!root->left && !root->right)
    {
        if (sum == root->value)
            return true;
        else
            return false;
    }
    return (hasPathSum(root->left, sum - root->value)
      | hasPathSum(root->right, sum - root->value));
}

static stTree* findBottomLeft(stTree *node)
{
    while (node->left)
        node = node->left;
    return node;
}

void freeTree(stTree *node)
{
    if (!node) return true;
    stTree *bottomLeft = findBottomLeft(node);

    while (node)
    {
        if (node->right)
        {
            bottomLeft->left = node->right;
            bottomLeft = findBottomLeft(bottomLeft);
        }
        stTree *old = node;
        node = node->left;
        free(old);
    }
}

int main(void)
{

   struct stTestCase test_cases[3]={
        {
           createNode(5,
            createNode(4,
                createNode(11,
                    createNode(7, NULL, NULL),
                    createNode(2, NULL, NULL)), NULL),
            createNode(8,
                createNode(13, NULL, NULL),
                createNode(4, NULL,
                    createNode(1, NULL, NULL)))),
            22,
            true
         },
         {
            createNode(5,
             createNode(1,
                 createNode(11,
                     createNode(3, NULL, NULL),
                     createNode(2, NULL, NULL)), NULL),
             createNode(7,
                 createNode(13, NULL, NULL),
                 createNode(4, NULL,
                     createNode(1, NULL, NULL)))),
            22,
            false
          },
          {
             createNode(12,
              createNode(4,
                  createNode(11,
                      createNode(7, NULL, NULL),
                      createNode(51, NULL, NULL)), NULL),
              createNode(8,
                  createNode(13, NULL, NULL),
                  createNode(4, NULL,
                      createNode(6, NULL, NULL)))),
               22,
               false
           }
   };

   for(int i=0; i<3; i++)
   {
      if (hasPathSum(test_cases[i].root, test_cases[i].sum)  == test_cases[i].result)
         printf("test case [%d], PASS\n", i);
      else
        printf("test case [%d], FAIL\n", i);
      freeTree(test_cases[i].root);
   }

}

RESULT:
test case [0], PASS
test case [1], PASS
test case [2], PASS

We don't normally do this in real (non-toy) programs:

stTree *node = malloc(sizeof *node);
if (!node) abort();

It's convenient and probably okay for a simple exercise like this, though.

Code of the form if (c) then return true; else return false; simplifies to just return c;, so we can reduce
    if (sum == root->value)
        return true;
    else
        return false;

to just
    return sum == root->value;

I think we can do better than that, if we're happy to consider an empty tree as having a sum of zero:
bool hasPathSum(const stTree *node, int sum)
{
    if (!node) {
        return sum == 0;
    }

    sum -= node->value;
    return hasPathSum(node->left, sum) || hasPathSum(node->right, sum);
}

Notice that I've used || rather than | in my version - there's no need to continue looking once we've found a successful result.  Also, because this function is recursive, the name root is misleading for the parameter - it could be any node, and it's only the root in the top-level call.

void freeTree(stTree *node)
{
    if (!node) return true;

That's a simple error - can't return a value from a function declared void(...).  That suggests you're not enabling enough compiler warnings.

Good to see unit tests.  I'm not sure I'd pile all the test data together like that - I'd probably create a small function for each test, so that each test is independent.  Consider using one of the available test frameworks to help relieve the repetitive work of testing.
When writing tests, it's a good idea to write the smallest test you can think of (e.g. the null tree) as the first test, and start with it failing (so you know the test works).  Then make it pass, and write the next simplest failing tests (tree with one node - make sure you test calls that should return different values).
Here's how I would begin (using Google Test, which is C++, but it's easy to test C code by using extern "C"):
#include <gtest/gtest.h>

extern "C" {
    struct stTree;

    stTree* createNode(int value, stTree *leftNode, stTree *rightNode);
    bool hasPathSum(const stTree *node, int sum);
    void freeTree(stTree *node);
}

TEST(PathSum, empty)
{
    stTree *tree = NULL;
    EXPECT_FALSE(hasPathSum(tree, 1));
    EXPECT_TRUE(hasPathSum(tree, 0));
}

TEST(PathSum, one_node)
{
    stTree *tree = createNode(2, NULL, NULL);
    EXPECT_FALSE(hasPathSum(tree, 0));
    EXPECT_TRUE(hasPathSum(tree, 2));
}

TEST(PathSum, two_nodes)
{
    stTree *tree = createNode(2, NULL,
                              createNode(3, NULL, NULL));
    EXPECT_FALSE(hasPathSum(tree, 0));
    EXPECT_TRUE(hasPathSum(tree, 2));
    EXPECT_TRUE(hasPathSum(tree, 5));
}


Generate video thumbnail I wrote a simple, yet disgusting video thumbnail image generator method—which uses the MediaToolkit .NET library.

The code is not self-explanatory whatsoever, and it's extremely inefficient.

It accepts a video byte array & MIME type—and writes it to a video file. Then, it obtains the video file's duration, stores the middle frame, and writes it to an image file. Finally, it retrieves the image file bytes and returns them ... after deleting the video & image file.

How can I make it more efficient?

    public static byte[] GetVideoThumbnail(byte[] bytes, string mediaType)
    {
        var video = new MediaFile
        {
            Filename = Path.GetTempPath() + Guid.NewGuid() + MimeTypeMap.GetExtension(mediaType)
        };
        var image = new MediaFile { Filename = Path.GetTempPath() + Guid.NewGuid() + ".jpg" };
        File.WriteAllBytes(video.Filename, bytes);
        using (var engine = new Engine())
        {
            engine.GetMetadata(video);
            engine.GetThumbnail(video, image,
                new ConversionOptions { Seek = TimeSpan.FromSeconds(video.Metadata.Duration.TotalSeconds / 2) });
        }
        var thumbnail = File.ReadAllBytes(image.Filename);
        File.Delete(video.Filename);
        File.Delete(image.Filename);
        return thumbnail;
    }

The most inefficient thing in your code is usage of byte[] to hold video or image data. There is a concept of Large Object Heap in .NET which is not compacted unless explicitly otherwise made-to starting .Net 4.5.1.

Red Gate's Explanation of Large Object Heap

Latest behavior of Large Object Heap

All your objects with a size of more that 85000 bytes are allocated on large object heap. These are not collected like  mentioned in .Net garbage collection and dealt with separately. You can read better in the links above.

As a sidenote, You can cross-check if you do need engine.GetMetadata(video); to generate the thumbnail.

The answer to your question would be

Like you are returning image as a byte array, you can choose to return a image url from the disk. Some part of byte array will be reduced.
With .net 4.5.1, you can call .net LOH compaction programmatically.
You can also do this processing in completely separate appdomain in memory, and then unloading it.
Or you can generate thumbnails from a completely separate windows service or console application, you simply restart them.

Read few more things on Large Object Heap