entab program which replaces 4 spaces with a tab I wrote an entab program which needs to replace every \$n\$ spaces with 1 tab. I picked \$n\$ to be 4, so every 4 spaces should be 1 tab. I have a feeling that my code is far too complicated than it should be, and the logic I made it with could be much simpler.

The logic of the code:

Note: \$n\$ = 4

Get line from input and put it in line array.
Whenever n spaces is found in that line kick n-1 of them from the array and the remaining space is replaced with tab.
Print the line.

#include <stdio.h>
#define MAXLINESIZE 100
#define TABSPACES 4

int mgetline(char *s, int lim);
void kick(char *x, char val);
int countx(char *x);
void swap(char* x, int i, int j);

int main(void) {
    char line[MAXLINESIZE];
    char *linep = line;
    char *tmp = 0;
    int tab = 0;
    int i = TABSPACES - 1;
    while (mgetline(line, MAXLINESIZE) > 0) {
        while (*linep) {
            if (*linep == ' ') {
                if (tab == TABSPACES - 1) {
                    while (i > 0) {
                        tmp = linep - i;
                        kick(tmp, ' ');
                        linep--;
                        i--;
                    }
                    *linep = '\t';
                    tab = 0;
                }
                else
                    tab++;
            }
            else
                tab = 0;
            linep++;
        }
        printf("%s", line);
        linep = line;
    }
    return 0;
}

int mgetline(char *s, int lim)
{
    int c;
    char *t = s;

    while (--lim > 0 && (c = getchar()) != EOF && c != '\n')
        *s++ = c;
    if (c == '\n')
        *s++ = c;

    *s = '\0';

    return s - t;
}

void kick(char *x, char val) {
    char *sp = x;
    int i = 1;
    while (*x) {
        if (*x == val) {
            while (countx(sp) - i != x - sp) {
                swap(sp, x - sp, countx(sp) - i);
                i++;
            }
            *(sp + countx(sp) - 1) = 0;
            return;
        }
        x++;
    }
}

int countx(char* x) {
    int i;
    for (i = 0; x[i]; i++);
    return i;
}

void swap(char* x, int i, int j) {
    char tmp = x[i];
    x[i] = x[j];
    x[j] = tmp;
}

The biggest thing you can do to improve your C style (and this also goes for any curly-brace language) is to learn to love the for loop.

foo(i);
while (bar(i)) {
    something;
    baz(i);
}

should almost always be recast as

for (foo(i); bar(i); baz(i)) {
    something;
}

Examples from your code:

char *linep = line;
while (...) {
    while (*linep) {
        // body
        linep++;
    }
    printf("%s", line);
    linep = line;
}

should be

while (...) {
    for (char *linep = line; *linep != '\0'; ++linep) {
        // body
    }
    printf("%s", line);
}

(Notice that I added an explicit comparison with '\0' for clarity — since it doesn't affect the generated code — and I changed your statement "linep increment" to the more readable "increment linep." There's another reason to use preincrement over postincrement, but it doesn't matter until you get to C++.)

Here's another example:

void kick(char *x, char val) {
    char *sp = x;
    int i = 1;
    while (*x) {
        if (*x == val) {
            while (countx(sp) - i != x - sp) {
                swap(sp, x - sp, countx(sp) - i);
                i++;
            }
            *(sp + countx(sp) - 1) = 0;
            return;
        }
        x++;
    }
}

should be

void kick(char *sp, char val) {
    for (char *x = sp; *x != '\0'; ++x) {
        if (*x == val) {
            for (int i = 1; countx(sp) - i != x - sp; ++i) {
                swap(sp, x - sp, countx(sp) - i);
            }
            sp[countx(sp) - 1] = '\0';
            return;
        }
    }
}

Let's keep working on the kick function. The next thing I notice is that you repeatedly call this function countx(sp) inside a tight loop. Surely that's a performance hit; it would make more sense to cache the value once in a local variable and then use the variable. But let's see what countx(sp) actually does.

int countx(char* x) {
    int i;
    for (i = 0; x[i]; i++);
    return i;
}

Well, that's just strlen(x) (modulo some const-incorrectness). You should write

int countx(const char *x) {
    return strlen(x);
}

and then get rid of the countx function altogether. So now we have

void kick(char *sp, char val) {
    const int n = strlen(sp);
    for (char *x = sp; *x != '\0'; ++x) {
        if (*x == val) {
            for (int i = 1; n - i != x - sp; ++i) {
                swap(sp, x - sp, n - i);
            }
            sp[n - 1] = '\0';
            return;
        }
    }
}

Next, I notice that if (*x == val), we just do a bit more processing and then return. So we don't actually need that extra bit of processing to happen inside the loop; it makes more sense to say "loop until we find a val; then break the loop; then do more processing and return." Also, there's another convenient library function for the looping part of that: it's called strchr.

So the next most productive thing you can do for your programming skill is to learn to love the standard library.

void kick(char *sp, char val) {
    const int n = strlen(sp);
    char *x = strchr(sp, val);
    if (x != NULL) {
        for (int i = 1; n - i != x - sp; ++i) {
            swap(sp, x - sp, n - i);
        }
        sp[n - 1] = '\0';
    }
}

Okay, let's look at the remaining complexity in that kick function. What's this three-argument swap routine? I'm familiar with the idea of swapping two objects... does this rotate the three of them, or something?

...Ah, no, it's just doing some unnecessary array operations. Let's write it the normal two-argument way:

void swap(char *x, char *y) {
    char tmp = *x;
    *x = *y;
    *y = tmp;
}

... swap(&sp[x - sp], &sp[n - i]); ...

Ah, wait, that call can be just

... swap(x, &sp[n - i]); ...

So now what do we have?

void kick(char *sp, char val) {
    const int n = strlen(sp);
    char *x = strchr(sp, val);
    if (x != NULL) {
        for (int i = 1; i < sp + n - x; ++i) {
            swap(x, sp[n - i]);
        }
        sp[n - 1] = '\0';
    }
}

Well, that's a lot of calls to swap. Let's see what's actually happening here... aha, I see, you're trying to bubble *x from the first position all the way to the end of the array. (But doing it in a very odd way where the value *x actually pops into the right place immediately, and then you spend the rest of the O(n) time just trying to get all the other elements back into their proper places.)

Okay, why do we want *x at the end of the array? (After all, we know its value is val, so we wouldn't even technically need to swap it.) ...Aw, really?

sp[n - 1] = '\0';

All that work, just to null it out? Okay, seems like we don't care about *x; all this loop is really trying to do is bump the remaining elements down by one position in the array. Again, there's a library function for that.

void kick(char *sp, char val) {
    const int n = strlen(sp);
    char *x = strchr(sp, val);
    if (x != NULL) {
        memmove(x, x+1, sp + n - x);
    }
}

Notice that since the sp + nth character of sp is '\0', we can just memmove it with the rest of the characters; we don't need to treat it specially.

And now we can see that n is used in only one place, as a part of the expression sp + n; and now we know about strchr, so we can reuse it and move the expensive computation inside the if so that we won't waste time computing it if it's not going to be used.

void kick(char *sp, char val) {
    char *x = strchr(sp, val);
    if (x != NULL) {
        char *end = strchr(sp, '\0');
        memmove(x, x+1, end - x);
    }
}

Now we can go back up and look for the callers of kick. Turns out there's only one — kick(tmp, ' ') — so we don't need val to be a parameter; in fact we should inline the entire three-line body of kick straight into the one place it's called.

...And of course there's much more to clean up, but at this point I'll call it a day. I hope that by reading and studying answers like these, you'll learn how to apply the same kinds of thinking to your own code. A particularly good book on the subject is Kernighan and Plauger's The Elements of Programming Style. (Don't worry that it deals in languages that may be unfamiliar to you. That's part of the point: the techniques of programming are largely language-independent.)

Oh, I suppose I should also point out some problems starting from the top and working down. First, look up strstr. Then, try your program on the input "hello\tworld\nhello    world" (that's four spaces after the second "hello") and observe the trouble with it.

I vaguely recall that a working entab/detab program is one of the programs developed in Kernighan and Plauger's Software Tools, which I recommend not as highly as The Elements of Programming Style but it's still pretty good. I also have a working version here.


Formatting the opposite of some numbers, with decimal alignment Background

I have a list of strings containing numbers. Each string is 8 characters long. For example :

'-123.456'
' -12.345'
'  -1.234'
' 123.456'
'  12.345'
'   1.234'

The longest number possible is ' 999.999' ; the smallest number is '   0.000' and there always are 3 numbers in the decimal.

What I want to do is compute the opposite of each number, and return it as a string of length 8, with the opposite sign next to the number.

For example :

'-123.456' should yield ' 123.456'
' -12.345' should yield '  12.345'
'  -1.234' should yield '   1.234'
' 123.456' should yield '-123.456'
'  12.345' should yield ' -12.345'
'   1.234' should yield '  -1.234'

What I did

I wrote the following code, which works :

def opposite(x):
    if x.startswith('  -'):
        xopp = '   ' + x[3:]
    elif x.startswith(' -'):
        xopp = '  ' + x[2:]
    elif x.startswith('-'):
        xopp = ' ' + x[1:]
    elif x.startswith('   '):
        xopp = '  -' + x[3:]
    elif x.startswith('  '):
        xopp = ' -' + x[2:]
    elif x.startswith(' '):
        xopp = '-' + x[1:]
    return xopp

My question

I feel like this code is completely "unpythonic" and could be replaced by a one-liner. So the question is: does anyone have an idea to make it more pythonic or even a one-liner ?

There are only really two things you need here.

float. Which allows you to convert the input to a floating point number. If you change to needing more precision or larger numbers, decimal would be a better choice - thanks @200_success. And,
str.format. Which uses the Format String Syntax. Which you can use pad the left with spaces, so the output has a width of eight. {: >8}. This however needs to be adjusted for your "there always are 3 numbers in the decimal" requirement, and so you can force this too with {: >8.3f}.

And so I'd use:

def opposite(x):
    return '{: >8.3f}'.format(-float(x))

If however you don't want to use float then you can use str.lstrip. With just one if-else:

def opposite(x):
    x = x.lstrip()
    if x.startswith('-'):
        x = x[1:]
    else:
        x = '-' + x
    return '{: >8}'.format(x)


Quadrant Queries challenge Running the same test 10,000 times with cProfile tells me that most of the damage happens in the count() function. This is my attempt at a solution to the Quadrant Queries challenge from InterviewStreet (the problem statement can be found here).

According to InterviewStreet, I passed 3/11 testcases and I ran out of CPU time. I have no way of knowing whether I was 3 for 3 and ran out of time or say, 3 for 6 and ran out of time. I don't know if my code works on all input.

Please help me optimize this code:

def reflect_x(i, j, coordinates):
    for pair in xrange(i-1, j):
        coordinates[pair][1] = -coordinates[pair][1]
    return coordinates

def reflect_y(i, j, coordinates):
    for pair in xrange(i-1, j):
        coordinates[pair][0] = -coordinates[pair][0]
    return coordinates

def count(i, j, coordinates):
    quad_one, quad_two, quad_three, quad_four = 0, 0, 0, 0
    for pair in xrange(i-1, j):
        x, y = coordinates[pair][0], coordinates[pair][1]

        if x >= 0 and y >= 0:
            quad_one += 1
        elif x < 0 and y >= 0:
            quad_two += 1
        elif x < 0 and y < 0:
            quad_three += 1
        elif x >= 0 and y < 0:
            quad_four += 1

    print "%d %d %d %d" % (quad_one, quad_two, quad_three, quad_four)

def reflect(coordinates, queries):

    for query in queries:
        if query[0] == "X":
            reflect_x(query[1], query[2], coordinates)
        elif query[0] == "Y":
            reflect_y(query[1], query[2], coordinates)
        elif query[0] == "C":
            count(query[1], query[2], coordinates)
        else:
            print query

if __name__ == "__main__":
    N = int(raw_input())
    coordinates = [[int(pair[0]), int(pair[1])] for pair in (pair.split() for pair in (raw_input() for i in xrange(N)))]

    Q = int(raw_input())
    queries = [[query[0], int(query[1]), int(query[2])] for query in (query.split() for query in (raw_input() for i in xrange(Q)))]

    reflect(coordinates, queries)

def reflect_x(i, j, coordinates):
    for pair in xrange(i-1, j):
        coordinates[pair][1] = -coordinates[pair][1]
    return coordinates

Something like:

def reflect_x(i, j, coordinates):
    coordinates[i-1:j] = ((x,-y) for x,y  in coordinates[i-1,j])

Is clearer and possibly slightly faster. Also, modify or return, don't do both.

def count(i, j, coordinates):
    quad_one, quad_two, quad_three, quad_four = 0, 0, 0, 0

If you have a count attached to your variable, the probably means it should be a list

    quad = [0] * 4

    for pair in xrange(i-1, j):

You may be better off slicing the list. That will be happen entirely in C, but looking up the value each time happens in python.

        x, y = coordinates[pair][0], coordinates[pair][1]

Same as:

        x, y = coordinates[pair]

...

        if x >= 0 and y >= 0:
            quad_one += 1
        elif x < 0 and y >= 0:
            quad_two += 1
        elif x < 0 and y < 0:
            quad_three += 1
        elif x >= 0 and y < 0:
            quad_four += 1

    print "%d %d %d %d" % (quad_one, quad_two, quad_three, quad_four)

def reflect(coordinates, queries):

bad name, this function doesn't only reflect

    for query in queries:
        if query[0] == "X":
            reflect_x(query[1], query[2], coordinates)
        elif query[0] == "Y":
            reflect_y(query[1], query[2], coordinates)
        elif query[0] == "C":
            count(query[1], query[2], coordinates)
        else:
            print query

if __name__ == "__main__":
    N = int(raw_input())
    coordinates = [[int(pair[0]), int(pair[1])] for pair in (pair.split() for pair in (raw_input() for i in xrange(N)))]

You don't need to do everything on one line. You can also unpack the tuple in the for clause.

    coordinate_lines = (raw_input() for i in xrange(N))
    coordinates = [(int(x),int(y)) for x,y in line.split for line in coordinate_lines]

As for optimisation: Firstly, you need to realise that you don't actually care what the coordinates are. All you care about is the quadrants. The reflection operations merely move you from one quadrant to the other.

I have to disagree with Jeff Mercado, as I don't think his approach will work. (maybe I've missed what he's hinting at.) Basically what you have is a worst case O(N*Q) running time. Since both N and Q are large numbers you cannot get away with doing that. His approach reduces the complexity to O(1) for the count operation, but leaves O(N) for the reflect operations. That's still going to give you O(N*Q).

We need to reduce the cost of the reflect operations as well.

Basically, for each quadrant we need to keep track of the points in that quadrant. We'll put them in some sort of data structure. We need a data structure that will allow the efficient implementation of:

Finding all points with indexes between a start/stop index
Swapping those points between two different versions of the data structure
Finding the count of all points currently in the data structure

Basically, we need all of these operations in logarithmic time or better. Figuring out what kind of data structure gives you that is left as an exercise for the reader.


Dictionary app using WIn32 I'm creating a dictionary app using Win32 API. I stored the word meaning in a file called dictionary.dic in the following format:

Word  Meaning
A     The first letter
B     The second letter
... and so on

As my window is created, I load the dictionary file into the std::map object using this code:
std::map<wstring, wstring> dict;
...
std::wifstream stream(L"dictionary.dic");
std::wstring temp, fileContent = L"";
while (std::getline(stream, temp)) {
    fileContent += temp;
    fileContent.push_back(L'\n');
}
std::vector<wstring> lines = split(fileContent, L'\n');
for (std::wstring line : lines) {
    std::vector<wstring> word = split(line, L'\t');
    dict.insert(dict.end(), make_pair(word[0], word[1]));
}

And when the user search the word, I'm using this code to display the meaning:
std::wstring enteredWord;
map<wstring, wstring>::iterator mi;
mi = dict.find(toLower(enteredWord));
if (mi != dict.end()) {
    //display meaning
}
else {
    //word not found
}

I used these functions to convert text to lowercase and split text:
I'm using this function to split the text:
std::vector<std::wstring> &split(const std::wstring &s, WCHAR delim, std::vector<std::wstring> &elems) {
    std::wstringstream ss(s);
    std::wstring item;
    while (std::getline(ss, item, delim)) {
        elems.push_back(item);
    }
    return elems;
}

std::vector<std::wstring> split(const std::wstring &s, WCHAR delim) {
    std::vector<std::wstring> elems;
    split(s, delim, elems);
    return elems;
}

std::wstring toLower(std::wstring val) {
    std::wstring temp = val;
    std::transform(temp.begin(), temp.end(), temp.begin(), tolower);
    return temp;
}

Is there a more efficient way to create a dictionary in Win32 / C++?

Is there a more efficient way to create a dictionary in win32 / c++.

Yes :-)

Let's look at the code:

Reading the file

// Read a file line by line.
while (std::getline(stream, temp)) {

    // Build one long........ string in memory.
    fileContent += temp;
    fileContent.push_back(L'\n');
}

// Then you split the long...... line you just built
// back into a vector of lines (using the '\n' you just inserted)
// to mark the line boundries.
std::vector<wstring> lines = split(fileContent, L'\n');

Issues

So you are basically doing the same work twice.
You are building a string in the most inefficient way possible.
Because you did not reserve space you are reallocating that string again and again and again. Each reallocation requires the whole string to be copied.

Why not read the lines directly into the vector?

 std::vector<wstring> lines;
 std::wstring line;

 while(std::getline(stream, line)) {
     lines.push_back(line);
 }

We can even do better than that with a tiny bit of work.

 // Define a class that represents a line.
 // Then define the input operator so it knows how to read itself.
 // Then define a conversion operator so it can be converted into a string.

 class Line
 {
     std::wstring   data;
     friend std::istream& operator>>(std::istream& str, Line& value)
     {
         return std::getline(str, value.data);
     }
     operator std::wstring&() const
     {
         return data;
     }
};
using LineStreamIter = std::istream_iterator<Line>;
// now you can declare your vector and load it in a single line.
std::vector<std::wstring>  lines(LineStreamIter(source), LineStreamIter());

Moving on to loading the map:

for (std::wstring line : lines) {
    std::vector<wstring> word = split(line, L'\t');
    dict.insert(dict.end(), make_pair(word[0], word[1]));
}

So basically you are saving your map as two strings separated by the first tab onto a single line. I assume there are no '\n' characters in the meaning, otherwise your encoding is going to break.

But again this is ineffecient as you build a vector in place then use that to build the dictionary. Why not just build the dictionary directly from the file.

std::map<std::wstring, std::wstring> dict;

std::wstring word;
std::wstring meaning;

while(std::getline(source, word, '\t') && std::getline(source, meaning))
{
    dict.emplace_back(std::piecewise_construct, word, meaning);
}

Note: std::getline() allows you specify a third argument that defines the line terminator (by default this is '\n'). By setting this to tab '\t' you can read up-to (but not including) the tab and extract the word first then extract the meaning separately.

I would take this a step further and define a dictionary class with a specific interface then use std::map internally. This has several advantages.

You can replace std::map with std::vector later as suggested by @ilmale
You can write the load/save functions in a self contained way.

Here is an outline:

class Dict
{
    class DictFileElement
    {
        std::pair<std::wstring, std::wstring>   value;
        frined std::istream& operator>>(std::istream& s, DictFileElement& value);
        frined std::ostream& operator<<(std::ostream& s, DictFileElement const& value);
    };

    std::map<std::wstring, std::wstring>  data;
    public:
        Dict(std::string fileName);
        std::wstring getMeaning(std::wstring const& word);
};
// I leave the implementation to you .
// Should be easy given all the code I wrote above.


Colored image will become black and white For a project of mine, which is targeting java 6, I had been in the need to have different algorithms/methods to change a BufferdImage with a ColorSpace.TYPE_RGB to a plain ole black and white one.

To test these implementations I therefore created an interface

public interface BlackAndWhiteConverter {
    BufferedImage toBlackAndWhite(BufferedImage image);
    void setThreshold(int threshold);
}

and the, for me, most successful implementation is up for review

import java.awt.color.ColorSpace;
import java.awt.image.BufferedImage;
import java.awt.image.DataBuffer;
import java.awt.image.DataBufferInt;
import java.awt.image.Raster;
import java.awt.image.WritableRaster;

public class BlackAndWhiteRedThresholdConverter implements BlackAndWhiteConverter {

    @Override
    public BufferedImage toBlackAndWhite(final BufferedImage image) {

        final int imageType = image.getColorModel().getColorSpace().getType();

        if (imageType != ColorSpace.TYPE_RGB) {
            throw new IllegalArgumentException();
        }

        final int width = image.getWidth();
        final int height = image.getHeight();
        final int upperLimit = width * height;

        final DataBuffer buffer = image.getRaster().getDataBuffer();
        final int[] source = ((DataBufferInt) buffer).getData();

        byte[] destination = new byte[upperLimit];

        for (int i = 0; i < upperLimit; i++) {

            int red = (source[i] >> 16) & 0x000000FF;

            if (red > threshold) {
                destination[i] = -1;
            }
        }

        WritableRaster raster = Raster.createPackedRaster(DataBuffer.TYPE_BYTE, width, height, 1, 2, null);
        raster.setDataElements(0, 0, width, height, destination);

        BufferedImage destinationImage = new BufferedImage(width, height, BufferedImage.TYPE_BYTE_BINARY);
        destinationImage.setData(raster);
        return destinationImage;
    }

    private int threshold = 110;

    @Override
    public void setThreshold(int threshold) {
        this.threshold = threshold;
    }

}

Please comment on anything that springs into your face. No matter what, I can't use a java version higher than 6.

Input validation

If the received image is not RGB, throwing an IllegalArgumentException without a custom message is not very friendly. Users would have to look at the code to understand what went wrong exactly.

The long chain image.getColorModel().getColorSpace().getType() is extracted to an imageType local variable that's only used for validation.
I would be good to move the validation to a dedicated helper method, let's say validateImageType.

Java conventions

Member variables should be declared at the top, followed by constructors, followed by methods. Very roughly speaking. So move the declaration of threshold higher.

Naming

As the interface name already implies the conversion to black and white,
the method name toBlackAndWhite is redundant. I'd rename it to simply convert.

Misc

0x000000FF is the same as 0xFF, so you can shorten it.


Django Rest Framework - add/remove to a list I have a model Collection which has a many to many relation to a model Item.

I want to be able to add or remove items to this collection using Django Rest Framework.

Option1 - make an action:

class CollectionViewSet(viewsets.ModelViewSet):
    queryset = Collection.objects.all()
    serializer_class = CollectionSerializer

    @action()
    def update_items(self, request, **kwargs):
        collection = self.get_object()
        add_items_id = request.DATA.pop('add_items', [])
        remove_items_id = request.DATA.pop('remove_items', [])
        items_add = Item.objects.filter(id__in=add_items_id).all()
        collection.items.add(*items_add)
        items_remove = Item.objects.filter(id__in=remove_items_id).all()
        collection.items.remove(*items_remove)
        return Response()

I see two downsides with this:

I cannot make one single request to update the collection with items and other fields (not without also modifying these.
I do not get the "API documentation" for free.

Option2 - override the update method and use two different serializers

class CollectionSerializer(serializers.HyperlinkedModelSerializer):
    items = ItemSerializer(many=True, read_only=True)

    class Meta:
        model = Collection
        fields = ('id', 'title', 'items')

class CollectionUpdateSerializer(serializers.HyperlinkedModelSerializer):
    add_items = serializers.PrimaryKeyRelatedField(many=True, source='items', queryset=Item.objects.all())
    remove_items = serializers.PrimaryKeyRelatedField(many=True, source='items', queryset=Item.objects.all())

    class Meta:
        model = Collection
        fields = ('id', 'title', 'add_items', 'remove_items')

class CollectionViewSet(viewsets.ModelViewSet):
    queryset = Collection.objects.all()

    def update(self, request, *args, **kwargs):
        collection = self.get_object()
        add_items_id = request.DATA.pop('add_items', [])
        remove_items_id = request.DATA.pop('remove_items', [])
        items_add = Item.objects.filter(id__in=add_items_id).all()
        collection.items.add(*items_add)
        items_remove = Item.objects.filter(id__in=remove_items_id).all()
        collection.items.remove(*items_remove)
        # fool DRF to set items to the new list of items (add_items/remove_items has source 'items')
        request.DATA['add_items'] = collection.items.values_list('id', flat=True)
        request.DATA['remove_items'] = request.DATA['add_items']
        return super().update(request, *args, **kwargs)

    def get_serializer_class(self):
        if self.request.method == "PUT":
            return CollectionUpdateSerializer
        return CollectionSerializer

Unfortunately, option2 has this horrible and inefficient hack. Here is why:

Super is called to handle the other properties (in this case only 'title'), it will see that add_items/remove_items is on the source 'items', (and at this point add_items/remove_items params would be empty) so it would remove all items.

What would be the canonical way to handle add/remove of items in a list via django-rest-framework, such that we still get:
- 1 request to update the object (i.e. not via a separate action request)
- re-using DRF patterns / auto-generated API

If the collection is at another model then you must setup the serializer with the 'many' kwarg.

class CollectionContainerViewSet(viewsets.ModelViewSet):
    queryset = CollectionContainer.objects.all()
    collection_ids = serializers.PrimaryKeyRelatedField(
                      many=true,
                      queryset=Collection.objects.all()
                    )

Or maybe change the collection to accept updating many?

class CollectionViewSet(viewsets.ModelViewSet):
    queryset = Collection.objects.all()
    serializer_class = CollectionSerializer(many=True)

I guess that in most situations you don't explicitly add or remove, but you are change the collection to a known state - containing a specific set of items. If that is not the case and you really must add and remove explicitly I think it is better to have two actions, one for adding and one for removing items by id.


Caesar Cipher in Haskell, on every other character I'm learning Haskell, and I tried to do an exercise which it is an implementation of the Caesar Cipher method.

The problem asks for cipher a text which it is in lower case and can contain characters from 'a' to 'z' without numbers, uppercase letters, symbols etc.., only lower case letters.

If the letter it is in an even position inside the message, I need to change for itself but in upper case. Example 'a' will be 'A'

If the letter is in an odd position inside the message, I need to change it by an other letter "n" positions next in the alphabet by the original one.
Example 'a' will be 'd' if "n" is equal to 3

Important thing, the alphabet needs to be circular, so, after z, will be the 'a'

So you call the function with a text and a number "n" for example: alfabet 3
The result will be: AoFdBhT.

This is my personal implementation, I checked it, and works fine, but I think maybe it is not a very good implementation, and I would like some advice to improve it.

caesarCipher :: String -> Int -> String
caesarCipher = caesarCipherAux [] 0

caesarCipherAux :: String -> Int -> String -> Int -> String
caesarCipherAux codedMessage _ [] _ = codedMessage
caesarCipherAux codedMessage pos (xs:s) n =
if even (pos) then
    caesarCipherAux (codedMessage++[toUpper xs]) (pos+1) s n
else
    caesarCipherAux (codedMessage++ [cipher xs n]) (pos+1) s n

cipher :: Char -> Int -> Char
cipher xs n
| xs == 'x'='a'
| xs == 'y'='b'
| xs == 'z'='c'
| otherwise = [xs..'z'] !! n

I'm also in the (early) process of learning Haskell, so please take my advice with a grain of salt.

First, let's talk about your coding style

I am assuming the indentation is only messed up in your question, as the code in your post would not compile.
I am also going to assume that you are either supposed to implement the cipher recursively or don't know a better method, yet (map). You also seem not to know about function composition (using . and $). In case you do, you should generally* use it wherever possible (*unless it reduces readability, but I haven't seen such a case yet).
xs is not used to represent scalar (single) values, but values such as lists that can contain many x's. Pattern matching a list using cons is often written as x:xs, f:fs, etc.
Using guards to check the same value for equality multiple times is redundant. Using a case ... of expression is more concise and readable:

cipher x n = case x of
  'x' -> 'a'
  'y' -> 'b'
  'z' -> 'c'
  _   -> [xs..'z'] !! n
-- alternatively: cipher 'x' n = 'a' etc.

even (pos) can be written as even pos.
I think using guards instead of if .. then .. else would make your code more readable. Instead of

caesarCipherAux codedMessage pos (xs:s) n =
  if even (pos) then
    -- things
  else
    -- other things

write

caesarCipherAux codedMessage pos (x:xs) n
  | even pos  = -- things
  | otherwise = -- other things

Your use of whitespace is not always consistent (++ in lines 8 & 10).
Consider surrounding infix operators by whitespace to increase readability. In my opinion something like f (x+1) is fine, but | xs == 'x'='c' decreases readability.
Avoid ++. Appending lists repeatedly can get somewhat costly (in this case the cost is negligible, but when theres a better way why not choose it). Given that you only append a char, a better way of doing so is consing it: toUpper xs : codedMessage instead of codedMessage++[toUpper xs]. This means, of course, that the end result needs to be reversed: caesarCipher = reverse . caesarCipherAux [] 0 with function composition or caesarCipher = reverse (caesarCipherAux [] 0) without. In cases where you want to append two lists, such as two strings, you can use a list of strings, cons (:) the lists and concat them at the end.
Avoid !! and if you use it, make sure its safe. caesarCipher "Hello" 30 will result in an exception. Instead of [xs..'z'] !! n use something like

let abc = [xs..'z']
in abc !! mod (length abc) n

Unless abc is empty, this is safe. abc will be empty if xs > 'z', so you have to check that beforehand. Still, not using !! in the first place  would be better.

About the implementation

I think your implementation is a good effort for a beginner, but flawed. As long as letters are not shifted past z (or a, if n is negative), it works. It they are, your code either a) yields the wrong result or b) crashes.

a) You tried to implement wrapping letters by hardcoding cases for 'x', 'y' and 'z', but those are only correct for n=3. If n is not 3, the result will be incorrect: caesarCipher "xyz" (-1) is "XbZ" in your implementation (y + 3 = b, where y, b are numerical representations of 'y', 'b'). But it should be "XxZ" because y - 1 = x.

b) Applying caesarCipher to "xwz" 10 results in an exception (*** Exception: Prelude.!!: index too large) because ['w'..'z'] only contains four characters and you did not harcode a case for 'w' (or any of the other letters).

Also, your function does not handle characters outside of ['a'..'z']. I assume that this is outside the scope of your exercise.

Fixing it

Prelude contains ord :: Char -> Int and chr :: Int -> Char to convert a Char to Int and vice-versa. In conjunction with mod, which returns the remainder of an Integral division, those functions are enough to implement letter shifting accurately and safely. Consider this example (w/o function composition):

shift :: Char -> Int -> Char
shift ch n = chr x'
    where r  = 26 -- length alphabet
          a  = ord 'a'
          x  = ord ch
          x' = a + mod (x - a + n) r

works      = shift 'a' 26 == 'a'
stillworks = shift 'b' (-26) == 'b'
wontbreak  = shift 'c' (26 * 4234789) == 'c'

This implementation fixes both problems because it does not use unsafe functions such as !! and works on any character (no hardcoded cases). If you apply it to characters outside the 'a'..'z' range, it will still evaluate successfully and the result will be constrained to said range (which is probably not what you want, so only apply this function to characters that are in the alphabet). To shift upper case letters properly, convert them to lower-case first before applying the function, then they can be converted back (again, not using composition):

foo (x:xs) n acc
    | isUpper x = foo xs n ((toUpper (shift (toLower x))):acc)
    | otherwise = foo xs n (shift x:acc)

Finally, instead of checking for an even/odd number, you can take two characters at once; one will be even, the other odd:

foo []       = "null"
foo [x]      = "x is even"
foo (x:y:xs) = "x is even, y is odd"

Thats all, I think. I hope it helps.


Querying all items that are in a many-to-many relationship with a child of an object I have the following Django models setup:

class Publication(models.Model):
    value=models.CharField(max_length=255, unique=True)

class Article(models.Model):
    filename = models.CharField(max_length=255)
    publication = models.ForeignKey(Publication, on_delete=models.CASCADE)

class Image(models.Model):
    name = models.CharField(max_length=255)
    articles = models.ManyToManyField(Article)

I wanted to add an 'images' property to the Publication model. I have implemented it like this

@property
def images(self):
    images = []
    for article in self.article_set.all():
        images += list(article.images.all())
    return list(set(images))

This works. I also considered trying something like

return Images.objects.filter(articles__publication=self)

But I don't think that I can do this because then I have to define Images above Publications, which won't work with Articles and Images.

Is there a way for me to improve this query to handle less of the filtering in application logic and more of it in the database?

I also considered trying something like
return Images.objects.filter(articles__publication=self)

But I don't think that I can do this because then I have to define Images above
Publications, which won't work with Articles and Images.

Did you try this?  It seems like it would work and would be the best approach.  You could add a .distinct() clause at the end of the query to avoid converting to a set.
It is best practice to set the related_name and related_query_name variables on your model related fields.  This will make your queries less verbose; article_set and image_set become articles and images, respectively.  See example below
class Article(models.Model):
    filename = models.CharField(max_length=255)
    publication = models.ForeignKey(to=Publication,
                                    on_delete=models.CASCADE,
                                    related_name='articles',
                                    related_query_name='article')

class Image(models.Model):
    name = models.CharField(max_length=255)
    articles = models.ManyToManyField(to=Article,
                                      related_name='images',
                                      related_query_name='image')

Look at the prefetch_related documentation.  You are querying the db for every image set on each article.
@property
def images(self):
    images = []
    for article in self.article_set.all():
        images += list(article.images.all())
    return list(set(images))

Using prefetch_related:
@property
def images(self):
    images = []
    publication = self.objects.prefetch_related('articles__images')
    for article in publication.articles.all():
        for image in article.images.all():
            images.append(image)

    return set(images)


Print the number of unique pairs Stored in a HashSet I'm trying to solve this fairly straight forward challenge:

  Problem Statement

  In computer science, a set is an abstract data type that can store
  certain values, without any particular order, and no repeated
  values. {1,2,3} is an example of a set, but {1,2,2} is not
  a set. Today you will learn how to use sets in java by solving this
  problem.

  You are given n pairs of strings. Two pairs (a,b) and (c,d) are
  identical if a=b and c=d. That also implies (a,b) is not same as
  (b,a). After taking each pair as input, you need to print number of
  unique pairs you currently have.

  Note: Brute force solution will not earn full points.

This works, but I'm getting a timeout on the last test case (10.000 elements to go through).

import java.io.*;
import java.util.*;

public class Solution {

    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        Set<Pair> pairs = new HashSet<>();
        final int N = s.nextInt();

        for(int i = 0; i < N; i++) {
            String first = s.next();
            String second = s.next();

            pairs.add(new Pair(first, second));
            System.out.println(pairs.size());
        }
    }

    public static class Pair {
        String first, second;
        Pair(String first, String second) {
            this.first = first;
            this.second = second;
        }

        @Override public int hashCode() {
            return first.hashCode();
        }

        @Override public boolean equals(Object other) {
            if (other == this) {
                return true;
            }
            if (!(other instanceof Pair)) {
                return false;
            }

            Pair otherPair = (Pair) other;
            return (first.equals(otherPair.first) && second.equals(otherPair.second))
                || (first.equals(otherPair.second) && second.equals(otherPair.first));
        }

    }
}

Clearly, janos solution is the simplest, but it'd probably break for the input

a b
a  b

Moreover, it circumvents the learning effect (I guess, it's about learning to write a class with equals and hashCode).

As often, Lombok makes it trivial:

@EqualsAndHashCode
public static class Pair {
    private final String first, second;
}

hashCode

The inefficiency was most probably caused by

@Override public int hashCode() {
    return first.hashCode();
}

which is correct, but leads to many collisions due to ignoring second. Something like

    return first.hashCode() + 123456789 * second.hashCode();

would solve it (the multiplier should be odd, anything else doesn't matter much).

equals

  return (first.equals(otherPair.first) && second.equals(otherPair.second))
            || (first.equals(otherPair.second) && second.equals(otherPair.first));

This contradicts the part

  implies (a,b) is not same as (b,a).

If it ever worked, then only because of the asymmetry in hashCode.

Why?

Two equals objects must have the same hashCode, otherwise the HashSet doesn't work properly (as the hash determines the index where the lookup starts). Violating this rule is a serious bug with which you may spend many funny debugging days.

This time, it helped you. After

set.put(new Pair("a", "b"));

the query

set.contains(new Pair("b", "a"));

erroneously returns false, although according to equals, the pair is there. However, it doesn't get found due to the non-equal hashCode. Analogously, after

set.put(new Pair("b", "a"));

you managed to contradict

  set is an abstract data type that can store certain values, without any particular order, and no repeated values.

Note that this is not guaranteed to happen as the hashes may coincide and only a part of them gets used.


Finding the max product of a 2D array Given a 2D array of unsigned integers and a maximum length n, find a path in that matrix that is not longer than n and which maximizes the product of the elements in the path. The output should consist of both the path and the product.

A path consists of neighbouring integers that are either all in the same row, or in the same column, or down a diagonal in the down-right direction. The code for this problem is below:

def maxPathProduct(self, matrix, maxLen):
    """
    Take a 2D Array of ints
    and return the largest Product of list of ints of length maxLen
    and the product of the list integers
    """
    maxProduct = -1
    maxList = []
    if len(matrix) == 0:
        return maxProduct, maxList
    numRows = len(matrix)
    numCols = len(matrix[0])

    for direction in range(1, 4):
        if direction == 1:
            numLines = numCols
        elif direction == 2:
            numLines = numRows
        elif direction == 3:
            numLines = numRows + numCols -1
        for line in range (0,numLines):
            pathProduct = 1
            pathLen = 0
            if direction == 1:
                headRow = 0
                headCol = line
            elif direction ==2:
                headCol = 0
                headRow = line
            elif direction == 3:
                headRow =  0 if line  >= numRows else line
                headCol =  line-numRows if line  >= numRows else 0
            tailRow = headRow
            tailCol = headCol
            pathList = []
            while headRow < numRows  and headCol < numCols:
                pathList.append(matrix[headRow][headCol])
                pathProduct *= matrix[headRow][headCol]
                pathLen+=1
                if pathLen > maxLen:
                    pathProduct /= matrix[tailRow][tailCol]
                    pathList = pathList[1:]
                    if direction== 1:
                        tailRow += 1
                    elif direction == 2:
                        tailCol += 1
                    elif direction == 3:
                        tailCol += 1
                        tailRow += 1
                if pathProduct > maxProduct:
                    maxProduct = pathProduct
                    maxList = pathList
                if direction== 1:
                        headRow += 1
                elif direction == 2:
                    headCol += 1
                elif direction == 3:
                    headCol += 1
                    headRow += 1
        return maxProduct, maxList
matrix = [
    [1,  2,  3,  4,  5],
    [1,  1,  2,  3,  5],
    [3,  4,  5,  5,  5],
    [3,  4,  5, 9,  5],
    [1,  1,  5,  5, 25],
]

The issue here is that the solution constitutes changing the values of the "head" and "tail" variables based on the current direction being considered in the 2D requiring the use of multiple if/else statements. This makes the code a look longer and possibly harder to follow. I'm basing this off of these errors listed by pylint:

[pylint] C0103:Invalid function name "maxPathProduct"
[pylint] R0914:Too many local variables (17/15)
[pylint] C0103:Invalid argument name "maxLen"
[pylint] C0103:Invalid argument name "maxLen"

I'm not sure if these errors actually warrant any changes but if they do, is there anyway to possibly refactor the if/else statements or any unwieldy part of the code while maintaining the functionality of the solution?

First of all, if this function is not in a class you must remove the self parameter, there's no need of that. Then you should insert a check if the parameters are correct, that is the rows number and the columns number must be at least equals to maxLength:

if len(matrix) < maxLen and len(matrix[0]) < maxLen:
        return -1 # return error code

In this cases, when you have to move across a matrix, I suggest you to use a sort of direction tuples, which are tuples of the form (1,0), (0,1), (1,1), (-1,0) etc..; so for each element of the matrix you add to the indexes of that element one of this tuples multiplied for a number. Maybe it's more complicate to speak about it than show it:

def maxPathProduct(matrix, maxLen):
    if len(matrix) < maxLen and len(matrix[0]) < maxLen:
        return -1
    maxProduct = -1
    maxList = []
    directions = [(1, 0), (0, 1), (1, 1)]
    for i in range(len(matrix)):
        for j in range(len(matrix[0])):
            for dir in directions:
                tempProduct = 1
                tempList = []
                inc = 0
                indexOutOfBound = False
                while len(tempList) < maxLen and not indexOutOfBound:
                    row = i + dir[0] * inc
                    col = j + dir[1] * inc
                    if row < len(matrix) and col < len(matrix[0]):
                        tempProduct *= matrix[row][col]
                        tempList.append(matrix[row][col])
                        inc += 1
                    else:
                        indexOutOfBound = True
                if tempProduct > maxProduct:
                    maxProduct = tempProduct
                    maxList = tempList
    return maxProduct, maxList

In this case you have only to move from left-to-right ((0,1)), up-to-down ((1,0)) and diagonally-up-to-down ((1,1)). With those direction tuples you can avoid that terrible long list of if-elif-else. I've tried this solution a bit, I'm not completely sure that it works, but I hope that you understand the trick.


Dijkstra's algorithm without relaxation I'm trying to implement Dijkstra's algorithm in Python, but the issue is that Python doesn't have support for key-based heaps out of the box, so applying relaxation step of the classical algorithm presented in CLRS becomes cumbersome.

My idea is to keep maintaining current total distance and then pushing frontier nodes with it. It seems to work both with the basic examples I can think of and pass codeforce's 20-C: Dijkstra?, which literally just tests algorithm implementation without need for modifications.

However, I don't have enough experience to reason about correctness and I can't figure out if my implementation is actually correct. Please take a look at my implementation and see if you can spot any problems?

from heapq import heappop, heappush

def dijkstra(g, s, f):
    h = [(0,s,None)]
    visited, prev = set(), {}

    while len(h) > 0:
        d,v,p = heappop(h)
        if v not in visited:
            visited.add(v)
            prev[v] = p
            if v == f:
                break
            for adj, adj_d in g[v]:
                if adj not in visited:
                    heappush(h, (adj_d + d, adj, v))

    path = [f]
    while f in prev and prev[f] != None:
        f = prev[f]
        path.append(f)
    return path[::-1], d

if __name__ == '__main__':
    g = {'s':[('v',1),('w',4)],'v':[('w',2),('t',6)],'w':[('t',3)],'t':[]}
    print(dijkstra(g, 's', 't') == (['s', 'v', 'w', 't'], 6))
    print(dijkstra(g, 's', 'e') == (['e'], 7))

I see you have tested the case when there is no path to the destination, but I'm not convinced that returning seemingly valid data in that case is a good idea. You can easily catch such case by adding an else clause to the main while loop. You will land there if the loop ends without break.
Disregarding the no path case you can simplify the second while loop to

path = []
while f in prev:
    path.append(f)
    f = prev[f]

The visited set is redundant because it is the same as the set of keys of prev, and v not in prev has similar performance to v not in visited.


Best structure for a Person in PHP classes (and databse) I am (in the process of) creating a system to store People and their details - names, date of birth (dob), addresses, phone numbers, etc. - and I'm curious how it is best achieved. Below is an an example of some of the SQL code with test data (the PHP classes match the structure in the db):

CREATE TABLE IF NOT EXISTS `person` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `title` varchar(16) DEFAULT NULL,
  `forename` varchar(50) DEFAULT NULL,
  `middlenames` varchar(50) DEFAULT NULL,
  `surname` varchar(50) DEFAULT NULL,
  `display` varchar(100) NOT NULL,
  `dob` datetime DEFAULT NULL,
  `dod` datetime DEFAULT NULL,
  `numViews` int(4) unsigned NOT NULL DEFAULT '0',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 AUTO_INCREMENT=3 ;

INSERT INTO `person` (`id`, `title`, `forename`, `middlenames`, `surname`, `display`, `dob`, `dod`, `views`) VALUES
(1, 'Miss', 'Julie', 'Nicola Louise', 'Smith', 'Mum', '1958-09-21 00:00:00', NULL, 1),
(2, 'Mr', 'David', NULL, 'Jones', 'Dave', '1932-06-19 00:00:00', NULL, 0);

CREATE TABLE IF NOT EXISTS `email` (
  `id` int(4) unsigned NOT NULL AUTO_INCREMENT,
  `contactId` int(10) unsigned DEFAULT NULL,
  `address` varchar(250) DEFAULT NULL,
  `type` enum('Personal','Work') DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 AUTO_INCREMENT=2 ;

INSERT INTO `email` (`id`, `contactId`, `address`, `type`) VALUES
(1, 1, 'j.smith@example.com', 'Personal');

CREATE TABLE IF NOT EXISTS `address` (
  `id` int(4) unsigned NOT NULL AUTO_INCREMENT,
  `line1` varchar(50) NOT NULL,
  `line2` varchar(50) DEFAULT NULL,
  `city` varchar(30) DEFAULT NULL,
  `county` varchar(25) DEFAULT NULL,
  `postcode` varchar(8) DEFAULT NULL,
  `type` enum('Home','Office','Holiday','Other') DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 AUTO_INCREMENT=3 ;

INSERT INTO `address` (`id`, `line1`, `line2`, `city`, `county`, `postcode`, `type`) VALUES
(1, '32 Test Road', NULL, 'Some City', NULL, 'PO1 2ST', 'Home'),
(2, '6 High Street', 'New Road', 'Chatham', NULL, 'TN6 8BL', 'Home');

CREATE TABLE IF NOT EXISTS `personaddress` (
  `contactId` int(10) unsigned NOT NULL,
  `addressId` int(4) unsigned NOT NULL,
  PRIMARY KEY (`contactId`,`addressId`),
  KEY `addressId` (`addressId`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO `personaddress` (`contactId`, `addressId`) VALUES
(1, 1),
(2, 2);

I did move the dob & dod fields into another Personal table, as I realised that at times - such as showing email addresses for a Person - that it wasn't necessary to load the dob & dod fields. However, when I did this and when I then wanted to get People with upcoming birthdays, to display the person's name I either had to load a Person object at the same time or add the display field to the Personal class and join the tables in the SQL - which I really didn't want to do (loose coupling). So I ended up moving the fields back to Person.

So my question is what is the best way of implementing this structure?

Note that in a class I currently have standard getter/setter methods, then additional methods (such as getFullName, getFullLegalName, etc. for Person) and finally methods for db interaction – load, save, etc. I've also been informed that really I should put these other methods into different classes. So my second question (which I suppose is tied to the first really) is should I also have separate classes for different sets of methods?

First Question: Don't let potential use-cases of your application drive how you model the data. As you experienced, this causes a lot of headache later on.

Extracting the dob & dod for performance reasons is premature in this case. It is absolutely ok to load data even when not sure if it will be required. Every (un-optimized) application has a percentage of data it doesn't load. That's fine until you really experience performance issues. And then this probably is not the bottle neck you are going to optimize first. Loading those two extra values goes almost for free .

If you have larger data-structures (e.g. huge object tress by relations), you can use the lazy-loading pattern for this (which is what many ORM do to avoid joins). This keeps your model intact but avoids loading too much data. Lazy-loading scalars probably is more expensive then just loading them in the first place.

Excursus: What you tried to do is to model two different roles of the same entity. This is a valid approach some times. Yet this doesn't say the information is not loaded, it's just limiting the available interface. To be able to use its full power, generic functions are required though. An example might look like:

interface Person {
    // shared information
}

interface PersonName extends Person {
    // names
}

interface PersonDates extends Person {
    // dob & dob
}

public find<? extends Person>(int id) {
    return PersonImpl();
}

class PersonImpl implements PersonName, PersonDates {
}

repository.find<PersonName>(1);
repository.find<PersonDates>(1);

This is some very expensive pattern though, in terms of complexity and effort to create. You'll rarely use this. Especially as this doesn't make data loading faster. The entity is always loaded completely too.

Second Question: Well, you know my opinion here :). Separate your model from how it is stored. The repository (its mapper) should be the only one to know how to save the model to the database.

Database structure:

I prefer to to name tables in plural (as they represent collections). Other do as you. Some kind a personal, stick to one convention though.
Use InnoDb's foreign keys in your model. They help greatly to enforce data integrity.
Normalization would require to extract city, postcode and county from your address table into a separate one.


Load JSON file into model using Javascript / jQuery (deferred, asynchronous) As a beginner I am always determined to improve myself. I've got written the following Code using jQuery that is working fine. However, I am sure that there are cleaner ways for achieving the same.

Right now, I am struggling with the this keyword and the asynchronous loading of the JSON file. Moreover, I am not sure whether you should call an function for initialization the way I did.

Do you have any suggestions for improvements?

$(function(){

    function Model() {
        this.data = null;
        this.init();
    };

    Model.prototype = {

        deferred: $.Deferred(),

        config: {
            jsonFile: 'dump.json'
        },

        init: function() {
            this.loadJson();
        },

        loadJson: function() {
            var self = this;
            jQuery.getJSON(
                this.config.jsonFile,
                function(data) {
                    self.data = data;
                    self.deferred.resolve();
                }
            )
            .fail(function() {
                console.error("Loading JSON file failed");
            })
            .always(function() {});
        },

        getData: function(callback) {
            console.log("getData")
            var self = this;

            $.when(this.deferred)
            .done(function() {
                callback(self.data)
            })
        },
    };

    var model = new Model();
    model.getData(function(data) {
        console.log(data)
    });

});

(duplicate of https://stackoverflow.com/q/23142089/3546614)

Update 1

I've just updated my code (and truncated unimportant stuff) taking @jgillich's advices into account. For the moment I feel better reading the JSON file when the object is generated which is why I outsourced the operation into separate function.

$(function(){
    function Model() {
        this.loadJson();
    };

    Model.prototype = {

        deferred: $.Deferred(),
        jsonFile: 'dump.json',

        loadJson: function() {
            $.getJSON(this.jsonFile)
                .done(function (data) {
                    this.data = data;
                    this.deferred.resolve(data);
                }.bind(this))
                .fail(this.deferred.reject);
        },

        getData: function() {
            return this.deferred.promise();
        },
    };

    var model = new Model();
    model.getData().done(function(data) {
        console.log('done')
        console.log(data)
    });
});

Storing deferred in the model and then passing a callback to getData is a bit obscure. You could write it like this instead:

getData: function() {
    var deferred = $.Deferred();

    if(!this.data) {
        $.getJSON('...',).done(function (data) {
            this.data = data;
            deferred.resolve(data);
        }.bind(this)).fail(deferred.reject);
    } else {
        deferred.resolve(this.data);
    }

    return deferred.promise();
}

model.getData().done(function (data) { /* ... */});

Use either $ or jQuery, don't mix them.
Use Function.prototype.bind instead of var self = this.
Use semicolons after each statement (or leave them out entirely if you prefer that).
this.data = null; seems pointless, newly created objects don't have such a property.


Extending method chaining IEnumerable with Add..If I'm trying to write an extension (method chaining) for playing around a bit.
What I made work is an extension to add an item to the list (what is pretty easy):
IEnumerable<int> myList = originalList.Add(12);

What I now want to do is:
IEnumerable<int> myList = originalList.Add(12).If((list, @new) => !list.Contains(@new));

What it should do:
If the originalList doesn't already contain @new (12), then it may not add it to the list.
If the If-sequence is not added, the element gets added.
So this is my code:

public static class EnumerableExtension
{
    public static IAddEnumerable<T> Add<T>(this IEnumerable<T> @this, T element)
        => new AddEnumerable<T>(@this, element);

}

public interface IAddEnumerable<T> : IEnumerable<T>
{
    IEnumerable<T> If(Func<IEnumerable<T>, T, bool> expression);
}

internal class AddEnumerable<T> : IAddEnumerable<T>
{
    private IEnumerable<T> _sequence;
    private readonly T _elementToAdd;

    private bool _added;

    public AddEnumerable(IEnumerable<T> sequence, T elementToAdd)
    {
        _sequence = sequence;
        _elementToAdd = elementToAdd;
    }

    public IEnumerable<T> If(Func<IEnumerable<T>, T, bool> expression)
    {
        _added = true;

        if (expression(_sequence,_elementToAdd))
            _sequence = _sequence.AddItem(_elementToAdd);

        return this;
    }

    public IEnumerator<T> GetEnumerator()
    {
        if (_added)
            return _sequence.GetEnumerator();

        _sequence = _sequence.AddItem(_elementToAdd);
        _added = true;

        return _sequence.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        if (_added)
            return _sequence.GetEnumerator();

        _sequence = _sequence.AddItem(_elementToAdd);
        _added = true;

        return _sequence.GetEnumerator();
    }
}

It works, but I wanted to ask, if there are any problems that could come up with, or if there is something I could do better.

I like the idea - but have the following comments:
  var originalList = Enumerable.Range(1, 12).ToList();
  IEnumerable<int> myList = originalList.Add(12).If((list, item) => !list.Contains(item));

If the originalList is defined explicit as something that implements ICollection, then you get a compiler error saying: "Operator '.' cannot be applied to operand of type 'void'", because Add() is already a method returning void.

    public IEnumerator<T> GetEnumerator()
    {
        if (_added)
            return _sequence.GetEnumerator();

        _sequence = _sequence.AddItem(_elementToAdd);
        _added = true;

        return _sequence.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        if (_added)
            return _sequence.GetEnumerator();

        _sequence = _sequence.AddItem(_elementToAdd);
        _added = true;

        return _sequence.GetEnumerator();
    }

Don't repeat code - the second method can call the first:
IEnumerator IEnumerable.GetEnumerator()
{
  return GetEnumerator();
}

public interface IAddEnumerable<T> : IEnumerable<T>
{
    IEnumerable<T> If(Func<IEnumerable<T>, T, bool> expression);
}

By renaming this interface you could extend its usability to for instance a sequence like:
data.Remove(12).If(<predicate>);

Maybe IPredicate or something like that.

Although I know that @new, @this etc. are valid variable names, I personally always avoid them, because they distract the reading. I have never been in a situation where it was unavoidable to use them.

I understand, that you want a fluid approach, and therefore define the If method separately, but I think, I would concatenate the behavior to a single function like:
public static IEnumerable<T> AddIf<T>(this IEnumerable<T> source, T element, Func<IEnumerable<T>, T, bool> predicate)
{
  return predicate(source, element) ? source.AddItem(element) : source;
}

An idea came to my mind: Because you return this from If(), I think, you can avoid the repetitive code in there by saving the predicate and then handle everything in GetEnumerator():
  internal class AddEnumerable<T> : IAddEnumerable<T>
  {
    private IEnumerable<T> _sequence;
    private readonly T _elementToAdd;
    private Func<IEnumerable<T>, T, bool> _expression;

    private bool _added;

    public AddEnumerable(IEnumerable<T> sequence, T elementToAdd)
    {
      _sequence = sequence;
      _elementToAdd = elementToAdd;
    }

    public IEnumerable<T> If(Func<IEnumerable<T>, T, bool> expression)
    {
      _expression = expression;
      return this;
    }

    public IEnumerator<T> GetEnumerator()
    {
      if (_added)
        return _sequence.GetEnumerator();

      if (_expression == null || _expression(_sequence, _elementToAdd))
      {
        _sequence = _sequence.AddItem(_elementToAdd);
      }
      _added = true;

      return _sequence.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
      return GetEnumerator();
    }
  }


Custom implementation of python queue to use dict instead of dqueue A bit of context:
I have 100 addresses that can send transactions to a blockchain, and I have a web app that works like this:

Get request and handle it in a thread
Get a free address from the queue
Send a transaction
Return address to the queue

It's important to know that one address cannot send two transactions at once.

My approach was to make a custom python queue that uses a dict of "address: free/taken" instead of the normal queue.

Its good for two reasons:

I need to be able to get a random address for the dict
The base queue get and put method already handle thread-locking and timeout.

This seems to work, but I would like some feedback on it.

from queue import Queue
from enum import Enum
import random

class ChannelStatuses(Enum):
    FREE = 0
    TAKEN = 1
    UNDERFUNDED = 2

class ChannelDict(Queue):
    def __init__(self, channels_seeds):
        # Init base queue
        super(ChannelDict, self).__init__(len(channels_seeds))
        # Change queue from a 'dqueue' object to a dict full of free channels
        self.queue = {channel: ChannelStatuses.FREE for channel in channels_seeds}

    def _get(self):
        # Get a list of all free channels
        free_channels = self.get_free_channels()
        # Select a random free channel
        selected_channel = random.choice(free_channels)[0]
        # Change channel state to taken
        self.queue[selected_channel] = ChannelStatuses.TAKEN
        return selected_channel

    def _put(self, channel):
        # Change channel state to free
        self.queue[channel] = ChannelStatuses.FREE

    def _qsize(self):
        # Base queue checks if the queue is not empty by checking the length of the queue (_qsize() != 0)
        # We need to check it by checking how many channels are free
        return len(self.get_free_channels())

    def get_free_channels(self):
        # Get a list of channels with "FREE" status
        return list(filter(lambda item: item[1] == ChannelStatuses.FREE, self.queue.items()))

"channel" is another word for "address"

Used this answer as inspiration: https://stackoverflow.com/a/16506527/2126254

More context on how get/put are used,these methods are exposed:

@contextmanager
def get_channel(self, timeout=CHANNEL_QUEUE_TIMEOUT):
    """
    Get the next available channel
    :param float timeout: (Optional) How long to wait before raising an exception
    :return next avilable channel
    :rtype str

    :raises KinErrors.ChannelBusyError
    """
    try:
        channel = self.channels.get(True, timeout=timeout)
    except queue.Empty:
        raise ChannelsBusyError()

    try:
        yield channel
    finally:
        self.put_channel(channel)

def put_channel(self, timeout=CHANNEL_PUT_TIMEOUT):
    """
    Return a channel to the queue
    :param float timeout: (Optional) How long to wait before raising an exception

    :raises KinErrors.ChannelsFullError
    """
    try:
        self.channels.put(True, timeout=timeout)
    except queue.Full:
        raise ChannelsFullError()

And the user will use:

with get_channel() as channel:
    #send transaction

1. Review

There is no docstring for ChannelDict. The text in the post would make a good start.
A ChannelDict is not a dictionary (it doesn't support the mapping interface, __getitem__ and __setitem__ and so on), so the name is a bit misleading.
ChannelDict is not specialized for channels: it would work for any kind of object. A name like RandomQueue would make it easier for the reader to understand the purpose of the code.
ChannelDict inherits from queue.Queue but has a different interface for initialization (it takes an iterable of channels instead of a maximum size). Perhaps this is convenient for your use case but it makes the code a little harder to understand because you can't just say "it's just like a Queue except that it gets items in random order", you have to explain the difference in initialization and that you can't set a maximum size.
The algorithm for getting a random item from the queue takes time proportional to the number of items, because get_free_channels has to loop over all the items looking for any that are free.
ChannelStatuses.UNDERFUNDED is declared but not used.

2. Revised code

One way to efficiently pick a random item (instead of looping over all the items) is to keep the available items in a list, and to swap the randomly selected item with the last item in the list before popping the selected item.

from queue import Queue
from random import randrange

class RandomQueue(Queue):
    """Variant of Queue that retrieves items in a random order."""
    def _init(self, maxsize):
        self.queue = []

    def _qsize(self):
        return len(self.queue)

    def _put(self, item):
        self.queue.append(item)

    def _get(self):
        queue = self.queue
        i = randrange(len(queue))
        queue[i], queue[-1] = queue[-1], queue[i]
        return queue.pop()


Report for a restaurant showing hourly sales information for a given timespan I have been tasked with creating a report for a high-volume restaurant that shows hourly sales information for a given timespan. When generating this report for a single day, it's very quick, but when generated for even two days, the length of time grows tremendously.

I was originally doing subqueries to get the data that are shown as TempGiftCard and TempPayment in the WITH block at the top of this query. After doing some research online, I was under the impression that placing them in the WITH blocks will make them only execute once and the results would be reused. However, when I look at the Actual Execution Plan that SSMS produces, it doesn't appear that that is the case. According to the execution plan, about 60% of the time for this query to run is in the TempGiftCard portion of the query.

Running the full query for a single day takes about 1 second, but when running it for two days, it takes about 90-100 seconds.

Given this information, I know this is a query issue and not a data issue, but I'm not sure what else to try.

WITH TempGiftCard AS
(SELECT OrderTrackDate, OrderNum, SUM(ItemPrice) AS GiftCardsSold
    FROM RMArchive.dbo.OrderItems
    WHERE ItemName = 'GIFT CARD'
        AND OrderTrackDate BETWEEN '12/7/2015' AND '12/8/2015'
    GROUP BY OrderTrackDate, OrderNum),
TempPayment AS
(SELECT OrderNum, SUM(CASE WHEN PaymentCoupon = - 1 THEN PaymentAmount ELSE 0 END) AS Coupons,
                  SUM(CASE WHEN PaymentPromo = - 1 THEN PaymentAmount ELSE 0 END) AS Promo,
                  OrderTrackDate
    FROM RMArchive.dbo.Payment
    WHERE OrderTrackDate BETWEEN '12/7/2015' AND '12/8/2015'
    GROUP BY OrderNum, OrderTrackDate)
SELECT DATEPART(hour, OrderCloseTime) AS Hour,
       OrderType,
       SUM(OrderGuests) AS GuestCount,
       ROUND(CASE WHEN SUM(OrderGuests) = 0 THEN 0 ELSE SUM(OrderSubTotal) / SUM(OrderGuests) END, 2) AS AvgPerGuest,
       SUM(OrderSubTotal) AS Subtotal,
       ISNULL(SUM(Coupons), 0) * - 1 AS Coupons,
       ISNULL(SUM(Promo), 0) * - 1 AS Promo,
       SUM(OrderSubTotal) - (SUM(CASE WHEN OrderTaxExempt = - 1 THEN OrderSubTotal ELSE 0 END) + SUM(OrderNonTaxableAmount)) AS Taxable,
       SUM(CASE WHEN OrderTaxExempt = - 1 THEN OrderSubTotal ELSE 0 END) + SUM(OrderNonTaxableAmount) AS NonTaxable,
       ROUND(SUM(CASE WHEN OrderTaxExempt = 0 THEN OrderDueTax ELSE 0 END), 2) AS TaxCollected,
       SUM(OrderTaxableInc) AS TaxIncludedSales,
       SUM(OrderTaxInc) AS TaxIncluded,
       ROUND(SUM(OrderSubTotal) + SUM(OrderTaxableInc), 2) AS TotalNetSales,
       ROUND(SUM(CASE WHEN OrderTaxExempt = 0 THEN OrderDueTax ELSE 0 END) + SUM(OrderTaxInc), 2) AS TotalTax,
       ROUND(SUM(OrderSubTotal) + SUM(CASE WHEN OrderTaxExempt = 0 THEN OrderDueTax ELSE 0 END), 2) AS GrossSales,
       ISNULL(SUM(GiftCardsSold), 0) AS GiftCardsSold
FROM RMArchive.dbo.OrderInfo
LEFT JOIN TempPayment ON OrderInfo.OrderNum = TempPayment.OrderNum AND OrderInfo.OrderTrackDate = TempPayment.OrderTrackDate
LEFT JOIN TempGiftCard ON OrderInfo.OrderNum = TempGiftCard.OrderNum AND OrderInfo.OrderTrackDate = TempGiftCard.OrderTrackDate
WHERE OrderInfo.OrderTrackDate BETWEEN '12/7/2015' AND '12/8/2015' AND OrderType IS NOT NULL
GROUP BY DATEPART(hour, OrderCloseTime), OrderType
ORDER BY DATEPART(hour, OrderCloseTime), OrderType
OPTION (RECOMPILE);

Style

First, I like the compact style of the CTEs, and putting the closing
parentheses on the same line. I would say, though, that I find the
select statements a bit hard to read.  Especially the main one.

Use prefix aliases for columns

This is not standard SQL, but t-sql can do this: prefix aliases.
In general, when writing long expressions, it gets harder to read
the query and to find what you are looking for.  So:

SELECT DATEPART(hour, OrderCloseTime) AS Hour,
       OrderType,
       SUM(OrderGuests) AS GuestCount,
       ROUND(CASE WHEN SUM(OrderGuests) = 0 THEN 0 ELSE SUM(OrderSubTotal) / SUM(OrderGuests) END, 2) AS AvgPerGuest,
       SUM(OrderSubTotal) AS Subtotal,
       ISNULL(SUM(Coupons), 0) * - 1 AS Coupons,

can be more clear when you write it as

SELECT Hour = DATEPART(hour, OrderCloseTime)
     , OrderType
     , GuestCount = SUM(OrderGuests)
     , AvgPerGuest = ROUND(CASE WHEN SUM(OrderGuests) = 0 THEN 0 ELSE SUM(OrderSubTotal) / SUM(OrderGuests) END, 2)
     , Subtotal = SUM(OrderSubTotal)
     , Coupons = ISNULL(SUM(Coupons), 0) * - 1

You can even make it a bit more fancy and align the = signs, which
could improve readability if the expressions are much alike
(differences stand out more clearly), but would be harder to maintain.

Order columns consistently

Not just in the GROUP BY, but also in the SELECT list.  This
especially makes sense when you join on them: list the primary
key(s) in a consistent order.  This may be a minor pick, but it
helps when reviewing; when you see them at the top, you won't miss
them.  For now, I'm guessing that OrderTrackDate is a combined
key with OrderNum and that neither of them is unique by itself.

   WITH TempGiftCard AS
(SELECT OrderTrackDate
      , OrderNum
      , GiftCardsSold = SUM(ItemPrice)
   FROM RMArchive.dbo.OrderItems
  WHERE ItemName = 'GIFT CARD'
    AND OrderTrackDate BETWEEN '12/7/2015' AND '12/8/2015'
  GROUP BY OrderTrackDate, OrderNum)
      , TempPayment AS
(SELECT OrderTrackDate
      , OrderNum
      , Coupons = SUM(CASE WHEN PaymentCoupon = - 1 THEN PaymentAmount ELSE 0 END)
      , Promo   = SUM(CASE WHEN PaymentPromo  = - 1 THEN PaymentAmount ELSE 0 END)
   FROM RMArchive.dbo.Payment
  WHERE OrderTrackDate BETWEEN '12/7/2015' AND '12/8/2015'
  GROUP BY OrderTrackDate, OrderNum)

Use the ISO 8601 date format

At first when I read the query, I thought you accidentally queried
data for a whole month, but I guess I am used to the DD-MM-YYYY
format.  You might want to check whether the date format settings
of the database are correct, so that SQL interprets the strings
correctly.

If that setting may vary, you can always fall back on the ISO
notation, which will be interpreted correctly.

AND OrderTrackDate BETWEEN '2015-12-07' AND '2015-12-08'

If, for whatever reason, the OrderTrackDate column is
a char(10) instead of a date, you will certainly have queried a
whole month.  In that case, you would probably need to do something
with convert.  And as that does not index well, you might need to
SELECT the result of that convert into another table, first.

Use variables as constants or parameters

Both dates are specified three times.  If you declare two variables
at the top, you need to change them only in one place.  You could
also more easily use them as parameters if you go and implement the
query as a stored procedure or as a script you call from another
client (a client application you might write or maintain later).

DECLARE @startDate date = '2015-12-07';
DECLARE @endDate date = '2015-12-08';
-- ...
   AND OrderTrackDate BETWEEN @startDate AND @endDate

CTEs are not per definition more efficient

See also this excellent answer.  Syntactically, it looks so much
nicer to write a CTE instead of a temp table (which also needs
cleanup), but they are quite different to the SQL Server that has to
execute them.

In this case, it would probably help to rewrite both of the CTEs to
temp tables first, then join on them in the final query.

IF OBJECT_ID(N'tempdb..#TempGiftCard') IS NOT NULL DROP TABLE #TempGiftCard;
SELECT OrderTrackDate
     , OrderNum
     , GiftCardsSold = SUM(ItemPrice)
  INTO #TempGiftCard
  FROM RMArchive.dbo.OrderItems
-- ...

I cannot test this without table definitions, but I would not be
surprised if this helped performance.


Node.js package that retrieves data from an API, formats it and sends it out via email to a distribution list I've created a Node.js package that retrieves data from Icinga (a monitoring platform), formats it and passes it off to a class that generates some HTML and then sends it all out as an email.
The email, in its simplest form, looks something like this:

I come from a Ruby/Python background and this is the first time I've ever delved into Node. The code below works and does what I need it to but I have the feeling it isn't using some of Node's best practices. I make 2 API calls, after the first one happens, inside the callback, I make another API call and then when all the data is returned I invoke a function to send off the email. I'm pretty sure this could be using async/await and/or Promises but I'm just not sure where to start in order to refactor it.
// Get the data
const warning = 1;
const error = 2;
const icingaServer = new icingaApi(icingaConfig.url, icingaConfig.port, icingaConfig.username, icingaConfig.password);
const clients = [
        { 'Client 1': '**.**.client_1.**.**' },
        { 'Client 2': '**.**.client_2.**.**' }
    ];
const data = [];

function sendEmail() {
    nodemailerMailgun.sendMail({
        from: 'some@email.com',
        to: appConfig.sendees,
        subject: 'Some subject',
        html: new tableHtmlGenerator(data).run()
    }).then((_res) => {
        let emailAddresses = appConfig.sendees.join(', ');
        console.log(`Email sent successfully to the following addresses: ${emailAddresses}`);
    }).catch((err) => {
        console.log(`Error: ${err.message}`);
    });
}

function allDataRetrieved() {
    return data.length === clients.length;
}

clients.forEach((clientMap) => {
    Object.entries(clientMap).forEach(([client, hostnameWildcard]) => {
        let totalHosts;
        let totalServices;
        let errors;
        let warnings;
        icingaServer.getServiceFiltered({
            "filter": "match(service_name, service.host_name)",
            "filter_vars": {
                "service_name": hostnameWildcard
            }
        }, (err, res) => {
            if (err) return `Error: ${err}`;

            warnings = res.filter(o => o.attrs.state === warning).length;
            errors = res.filter(o => o.attrs.state === error).length;
            totalServices = res.length;

            icingaServer.getHostFiltered({
                "filter": "match(host_name, host.name)",
                "filter_vars": {
                    "host_name": hostnameWildcard
                }
            }, (err, res) => {
                if (err) return `Error: ${err}`;

                warnings += res.filter(o => o.attrs.state === warning).length;
                errors += res.filter(o => o.attrs.state === error).length;
                totalHosts = res.length;

                data.push({
                    name: `${client} (${totalHosts}/${totalServices})`,
                    errors: errors,
                    warnings: warnings
                });

                if (allDataRetrieved()) sendEmail();
            });
        });
    });
});

I've omitted all the require and const definitions at the top of this file as they're not really needed in order to understand the code in my opinion.
The main issue is that one API call happens inside the callback of another API call and this feels nasty to me. I also wait for all the data to be pushed to the data variable by doing a simple but crude if statement to check if all the data has been retrieved and pushed to the array, if it has then the email is sent.
I also feel like I need to add that I'm aware this code could be improved by dumping all this business logic into a class or splitting it out into separate files. I'm not after help in that sense, it's more of how to handle API requests and waiting for the requests to finish and when/how/if to use Promises.

When you have a lot of asynchronous requests to make, and you want to wait for all to finish, the proper method to use is to first promisify the requests (if they don't return a Promise already), and then to use Promise.all, which returns a Promise which resolves once all promises in the passed array have resolved.
Unfortunately, icingaServer looks to be callback-based, and you have to multiple methods from it. Luckily, there's a package called promisify that makes turning these callback APIs into Promises much easier.
You can reduce the nesting level by one by changing
clients.forEach((clientMap) => {
    Object.entries(clientMap).forEach(([client, hostnameWildcard]) => {

into a use of flatMap instead: clients.flatMap(Object.entries)
DRY You have two very similar callbacks in getServiceFiltered and getHostFiltered. All that differs is the function invoked and the parameter passed, so it'd be better to make another function to which you can pass the parts that change.
Parallel processing Rather than two calls which are made in serial, consider making them both at once, in parallel, if the API supports it - this'll let your script finish quicker.
Filtered array length While you can do arr.filter(callback).length, you might consider using reduce instead, since you don't care about the resulting array, you just care about the number of matching elements.
Unquoted keys In JS, there's no need to quote object keys unless the keys contain characters that are invalid for idenfifiers. Most prefer not to use quotes so as to keep the code free of unnecessary noise.
Refactored:
const { promisify } = require('util');
const icingaServer = new icingaApi(icingaConfig.url, icingaConfig.port, icingaConfig.username, icingaConfig.password);
const getServiceFiltered = promisify(icingaServer.getServiceFiltered).bind(icingaServer);
const getHostFiltered = promisify(icingaServer.getHostFiltered).bind(icingaServer);

const processClient = async ([client, hostnameWildcard]) => {
    const getClientData = async (method, filterKey) => {
        const result = await method({
            filter: 'match(host_name, host.name)',
            filter_vars: {
                [filterKey]: hostnameWildcard
            }
        });
        return {
            warnings: result.reduce((count, o) => count + (o.attrs.state === warning), 0),
            errors: result.reduce((count, o) => count + (o.attrs.state === warning), 0),
            totalCount: result.length,
        };
    };
    const [serviceData, hostData] = await Promise.all([
        getClientData(getServiceFiltered, 'service_name'),
        getClientData(getHostFiltered, 'host_name'),
    ]);
    return {
        name: `${client} (${hostData.totalCount}/${serviceData.totalCount})`,
        errors: serviceData.errors + hostData.errors,
        warnings: serviceData.warnings + hostData.warnings,
    };
};

Promise.all(
    clients.flatMap(Object.entries).map(processClient)
)
    .then(sendEmail)
    .catch((error) => {
        // handle errors
    });

where sendEmail now takes a parameter of the data to send.
Also note that // handle errors shouldn't just log an error if it occurs - ideally, you'd have a system in which such an error results in a developer being able to look at a dashboard or something to see at a glance what's failed recently, to see when an issue arises that needs looking into (for example, if the API changes and all requests start failing).


The Next Palindrome - CodeChef This is codechef beginner problem.
How I can make it efficient, It is taking 2.09 sec while time limit is 2.0764 secs.
Problem is:
For a given positive integer K of not more than 1000000 digits, write the value of the smallest palindrome larger than K to output. Numbers are always displayed without leading zeros.
https://www.codechef.com/problems/PALIN
#include<iostream>
using namespace std;

long int reverse_num(long int n){
  long int reversenumber=0,remainder1;
    while(n != 0){
    remainder1 = n%10;
    reversenumber = (reversenumber*10)+remainder1;
    n /= 10;
}
return reversenumber;
}

int main(){

int T;
cin>>T;

while(T--){

long int n,reversenum;
bool chek;
cin>>n;
n = n+1;
bool flag = false;
while(!flag){

    reversenum = reverse_num(n);
    (n == reversenum) ? flag = true : flag = false;
    n++;
}
cout<<reversenum;
}
return 0;
}

You can simplify the loop in main. flag is unnecessary. Just use an infinite loop with a break statement. Also, because you know that any number with a zero as the last digit won't be a palindrome, you can skip those.
Putting those together, you get:
for (;;) {
    reversenum = reverse_num(n);
    if (reversenum == n) break;
    ++n;
    if (n % 10 == 0) ++n;
}

This will skip about 10% of the possible candidates.
An additional possible improvement (which is left as an exercise for the reader) involves determining what the largest digit of n is, then only looking at numbers that have a last digit that matches. This would allow you to skip 90% of the possible candidates with little additional overhead.


RGB to HEX (and vice versa) converter Please review the code.

<!DOCTYPE html>

    <meta charset="utf-8">
    <title>RGB to HEX (and vice versa) converter</title>
    <link rel="stylesheet" href="http://cdnjs.cloudflare.com/ajax/libs/normalize/3.0.0/normalize.min.css">

<input id="rgb" type="text" placeholder="rgb(255, 255, 255)">

<input id="hex" type="text" placeholder="#ffffff">

Jsbin - http://jsbin.com/fokayomo/1/

Two things jump out at me.

Decouple from the HTML elements.

var inputRgb = document.getElementById("rgb"),
    inputHex = document.getElementById("hex"),

I would do something like:

function RGB2Hex (inputRgb, inputHex){
    var converter = {
        toHex: ...
        toRGB: ...
    };

    inputRgb.addEventListener("input", converter.toHex, false);
    inputHex.addEventListener("input", converter.toRgb, false);

    return converter;
}

// Usage:
var mainHexConversion = new RGB2Hex(document.getElementById("rgb"), document.getElementById("hex"));
var secondHexConversion = new RGB2Hex(document.getElementById("mini-rgb"), document.getElementById("mini-hex"));

keep the background colour changing code outside the converter.

function RGB2Hex (inputRgb, inputHex, onColourChange)
{
   var converter = {
        toHex: ...
        toRGB: ...
        colourChanged: function(colourValue){
            if(typeof onColourChanged == "function")
            {
                onColourChange(colourValue);
            }
        }
    };
}

// Usage:
var mainHexConversion = new RGB2Hex(
    document.getElementById("rgb")
    , document.getElementById("hex")
    , function(color)
    {
      document.body.style.backgroundColor = color;
    }
);


Recursive linear search - branch style and layout Is my layout of the if/else statement reasonable?

It feels clumsy to me to spread the termination condition over the first three lines of the function. Can I squeeze it into one or two lines? Would it help if I used the ternary operator? How can I make the JavaScript more idiomatic (while still using recursion)?  Any other improvements?

var containsA = function(sentence) {
  if (sentence.length == 0) {
    return false;
  }
  else {
    return sentence[0] === 'A' || containsA(sentence.substr(1));
  };
};

var containsA = function(sentence) {
  if (sentence.length == 0) {
    return false;
  }
  else {
    return sentence[0] === 'A' || containsA(sentence.substr(1));
  };
};

// Unit tests
var assert = function(code) {
  if (eval(code)) {
    document.write(code + " test passed.
");
  }
  else {
    document.write(code + " test FAILED.
");
  };
};

assert("!containsA('')");
assert("containsA('A')");
assert("!containsA('a')");
assert("containsA('HAH!')");
assert("!containsA('carrots')");

I have simplified details that are not relevant to my question. This simpler version attempts to reimplement String.prototype.contains(). If I convert my string to an array I could use Array.prototype.find() but my hunch is that would be less readable.

How can I make the JavaScript more idiomatic?

By using indexOf:

var containsA = function(sentence) {
  return sentence.indexOf('A') != -1;
};

As your update specified that you want to use recursion, let's see what we can do.

Cleaning up, we can remove the else (and the extraneous semi-colon).

var containsA = function(sentence) {
  if (sentence.length === 0) {
    return false;
  }
  return sentence[0] === 'A' || containsA(sentence.substr(1));
};

Or you can write it more succinctly, but I would say at the cost of readability

var containsA = function(sentence) {
  return sentence.length > 0 &&
    (sentence[0] === 'A' || containsA(sentence.substr(1)));
};

Now we could end up creating a lot of substrings. To avoid this, we can pass an index into the string

var containsA = function(sentence, i) {
  i = i || 0;
  if (i >= sentence.length) {
    return false;
  }
  return sentence[i] === 'A' || containsA(sentence, i + 1);
};

Or

var containsA = function(sentence, i) {
  i = i || 0;
  return i < sentence.length &&
    (sentence[i] === 'A' || containsA(sentence, i + 1));
}

In case this is not homework or a thought exercise...

Don't do this. By using recursion (and substr) to solve this, you're simultaneously making your code less efficient, in terms of both time and memory, and less readable. As @Guffa pointed out, it may even blow the stack.

The function containsA has no reason to exist. Just use indexOf where appropriate.


Processing names and meanings from .txt files I wrote this program to process the names and meanings from .txt files. From the while loops I wrote, I noticed they are very similar loops containing Scanners. I am wondering if there is any way to reduce the redundancy in the code.
Also, for this program my instructor only allows us to use Scanners, while and if/else loops, boolean and file input to solve this problem.
Here are samples for names.txt and meanings.txt:
names.txt

Jax    M  0  0 0 0  0  0 0  0  0 0 0  0  347
Randall   M 974 620  650 467  369  232  70 59 100 136 192  440  841
Minnie  F  8 22 50 87 122  168  258  516  995  0 0  0  0
Brianna F  0 0 0 0 0 0  0  0  0 340 61  15  30

meanings.txt

MUBINA f Arabic Feminine form of MUBIN
ANDRONIKOS m Ancient Greek Means "victory of a man" from Greek
(andros) "of a man" and (nike) "victory".
SALVATORE m Italian Italian cognate of SALVADOR

And here is my code:
import java.io.*;
import java.util.*;

public class BabyNames {
   public static void main(String[] args) throws FileNotFoundException {
      Scanner inputName = new Scanner(new File("names.txt"));
      Scanner inputMeanings = new Scanner(new File("meanings.txt"));
      Scanner console = new Scanner(System.in);

      intro();
      System.out.print("Name: ");
      String nameIn = console.next();
      System.out.print("Gender (M or F): ");
      String genderIn = console.next();

      nameAndMeaning(inputName, inputMeanings, nameIn, genderIn);
   }

   public static void intro() {
      System.out.println("This program allows you to search through the");
      System.out.println("data from the Social Security Administration");
      System.out.println("to see how popular a particular name has been");
      System.out.println("since 1890.");
      System.out.println();
   }

   public static void nameAndMeaning(Scanner inputName, Scanner inputMeanings, String nameIn, String genderIn) {
      boolean hasLine = false;
      while(inputName.hasNextLine()) {
         String line = inputName.nextLine();
         Scanner lineScan = new Scanner(line);
         if(lineScan.hasNext()) {
            String name = lineScan.next();
            String gender = lineScan.next();
            if(name.equalsIgnoreCase(nameIn) && gender.equalsIgnoreCase(genderIn)) {
               hasLine = true;
               System.out.println(line);
            }
         }
      }
      if(hasLine == false) {
         System.out.println("\""+ nameIn + "\" not found.");
      } else {
         while(inputMeanings.hasNextLine()) {
            String line = inputMeanings.nextLine();
            Scanner lineScan = new Scanner(line);
            if(lineScan.hasNext()) {
               String name = lineScan.next();
               String gender = lineScan.next();
               if(name.equalsIgnoreCase(nameIn) && gender.equalsIgnoreCase(genderIn)) {
                  String meaning = line;
                  System.out.println(line);
               }
            }
         }
      }
   }

I would start from the declaration of main. You are throwing an exception from it but no other class will handle it. You should put your Scanners inside a try-catch and handle the exception properly.

I would also recommend reading everything from the files at once and use that collection of data in your code logic. A Scanner locks the file until it's finished with it.

Furthermore, you don't have to call System.out.println(...) just for the sake printing a new line. Use the \n escape sequence for doing the same

System.out.println("This program allows you to search through the \ndata from the Social Security Administration \nto see how popular a particular name has been \nsince 1890.\n");

The rest is just a search function to find the meaning based on the name and gender. I recommend creating a name Baby class with name and gender attributes and use a Hashtable<Baby, String> to search for the meaning. The class can be as simple as

class Baby{
    private String name;
    public String Name(){
        return name;
    }

    private boolean gender;
    public boolean Gender(){
        return gender;
    }

    public Baby(String name, boolean gender){
        this.name = name;
        this.gender = gender;
    }
}

The time complexity of searching a value from a Hashtable is close to O(1) while a typical looping mechanism is O(n). The speed becomes n times slower as the size of the collection increases.

You also have a lot of duplicate code in the two while statements. To resolve that I'd recommend extracting out the if-else logic outside and calling a common function with a single loop. Something like

bool foundMatch = findMatch(...);
if(foundMatch) {
  findMatch(...) // with different params now
}
else {
  // print error message
}

Solving with Constraints

Given the constraints for the solution, I'd solve it as below

public static void main(String[] args){
    Scanner consoleInput = new Scanner(System.in);

    // Welcome message
    System.out.println("This program allows you to search through the \ndata from the Social Security Administration \nto see how popular a particular name has been \nsince 1890.\n");

    // Receive user input
    System.out.print("Name: ");
    String inputName = consoleInput.next();
    System.out.print("Gender: ");
    String inputGender = consoleInput.next();

    consoleInput.close(); // Closed as not needed anymore

    try (Scanner namesScanner = new Scanner(new File("names.txt"));
            Scanner meaningsScanner = new Scanner(new File("meanings.txt"))) {

        String foundMatch = findMatch(inputName, inputGender, namesScanner);
        if(!foundMatch.equals("")){
            System.out.println(foundMatch);
            System.out.println(findMatch(inputName, inputGender, meaningsScanner));
        } else{
            System.out.println("\""+ inputName + "\" not found.");
        }

    } catch (FileNotFoundException ex){
        System.out.println("File Not Found");
    }
}

private static String findMatch(String name, String gender, Scanner fileScanner){
    while(fileScanner.hasNextLine()){
        String currentLine = fileScanner.nextLine();

        // Split it to a String array and check the first two values
        String contents[] = currentLine.split(" ");
        if(name.equalsIgnoreCase(contents[0]) && gender.equalsIgnoreCase(contents[1])){
            return currentLine;
        }
    }
    // if no returns occured till the end of the file
    return "";
}

Please note that

The Scanners should be closed once you're done using them. Using Try-with-resources statement is a better alernative (as recommended in the comments). You don't have to worry about the Scanners being closed. The try block functions like a using from C# and releases all resources after the block itself is finished. The defined resources must implement the java.io.Closeable interface for this functionality and the Java project must use Java SE 7 or above.
Both name matching operations can be handled by the same function. The significance of positive matches is handled by the calling function. This also enforces Single Responsibility Principle (SRP) and improves code maintainability
Use equalsIgnoreCase(...) instead of equals() to avoid using toLowerCase() on all strings
return is called immediately from the function on the first match to avoid redundant checks

Now if you'll be very strict and won't accept the use of a String array. You can replace it by extracting the first and second values using substring(...) function on the first two whitespaces