C++: k-Nearest Neighbours with Lambdas and Priority Queues I implemented getting the \$k\$-nearest neighbours of an origin point to a set of points in C++17. I tried to use some more modern C++ lambda techniques and was looking for feedback on use of lambdas, auto return types, and using a custom comparison function with priority queue.

Should other lambdas be captured by referenced or by value?
For auto return types, when should you use the arrow notation to signify a return type?
Is there a cleaner way to declare the type of queue in the code below?

Other feedback is welcome too.
#include <algorithm>
#include <iostream>
#include <queue>

using Point = std::pair<int, int>;

auto knn(const std::vector<Point>& points,
         const Point& origin,
         const int k) -> std::vector<Point> {
  auto squared_distance = [&origin](const Point& p) {
    const int dx = std::get<0>(p) - std::get<0>(origin);
    const int dy = std::get<1>(p) - std::get<1>(origin);
    return dx*dx + dy*dy;
  };

  auto closer = [&squared_distance](const Point& p1, const Point& p2) {
    return squared_distance(p1) > squared_distance(p2);
  };
  std::priority_queue<Point, std::vector<Point>, decltype(closer)> queue(closer);

  for (const auto& point : points) {
    queue.push(point);
  }

  std::vector<Point> nearest_neighbours;
  for (int i = 0; i < k; ++i) {
    nearest_neighbours.emplace_back(std::move(queue.top()));
    queue.pop();
  }
  return nearest_neighbours;
}

Answers to your questions

Should other lambdas be captured by referenced or by value?

Functionally, for non-mutable lambdas, it doesn't matter. However, if a lambda is marked mutable then capturing by value will make a copy of the lambda's state, whereas if it's a reference no copy is made. Here is an example you can try out yourself:
#include <iostream>
#include <vector>
#include <algorithm>

int main() {
    std::vector<int> vs[2];
    auto iota = [i=0]() mutable {return i++;};
    auto generator = [&iota]() mutable {return iota();}; // try removing the &

    for(auto &v: vs) {
        std::generate_n(std::back_inserter(v), 5, generator); // try using iota directly
        for(auto i: v)
            std::cout << i << ' ';
        std::cout << '\n';
    }
}

It's probably best to capture them by reference, unless mutable lambdas are used and you want a copy of the state to be made.

For auto return types, when should you use the arrow notation to signify a return type?

You should use it when it is important to constrain the return type, so that you don't accidentily return the wrong type. For example, if I want to write a function that calculates the square root of something, and always return the same type as the input, you should write:
auto square_root(auto value) -> decltype(value) {
    return std::sqrt(value);
}

If you omit the trailing return type, then calling the above function with an int will return a double instead. And that might then be a problem if you would do something like:
printf("Square root of 4 is %d\n", square_root(4)); // runtime error

Is there a cleaner way to declare the type of queue in the code below?

Yes, if you can make the compiler deduce that it should use closer() to compare two Points. One option is to make Point a class and implement operator<(), another one (as pointed out by Mikael H) is to just define a global overload for operator< that takes two Points:
bool operator<(const Point& p1, const Point& p2) {
    return closer(p1, p2);
}

Then you can just write:
std::priority_queue<Point> queue;

Use std::partial_sort_copy
Using a priority queue is one way to solve it, but it is a bit cumbersome here, since there is no fast way to get the first k elements out. Also, by adding all input elements to the queue, you have sorted all elements, which means you have done more work than necessary. What you want is just to do a partial sort, where you sort the input array until you get the first k smallest elements, and you don't care about the order of the rest. std::partial_sort_copy() allows you to do that, while still allowing a const reference to the input vector:
std::vector<Point> nearest_neighbours(k);
std::partial_sort_copy(points.begin(), points.end(), nearest_neighbours.begin(), neareset_neighbours.end(), closer);
return nearest_neighbours;


Project Euler Problem #755 As the title indicates this is one of the last most problems of Project Euler. The problem is straightforward. However, similar to the rest of the Euler Project problems either Memory Error or long execution times are problems. An explanation of the problem is:

Consider the Fibonacci sequence \$\{1,2,3,5,8,13,21,\ldots\} \$.
We let \$f(n)\$ be the number of ways of representing an integer
\$n\ge 0\$  as the sum of different Fibonacci numbers. For example,
\$16 = 3+13 = 1+2+13 = 3+5+8 = 1+2+5+8\$ and hence \$f(16) = 4\$ . By
convention \$f(0) = 1\$ .
Further we define,  \$\displaystyle S(n) = \sum_{k=0}^n f(k)\$   You
are given \$S(100) = 415\$ and \$S(10^4) = 312807 \$  .
Find \$ \displaystyle S(10^{13})\$ .

I'll show my code, and then explain what I have done so far. Lastly, ask for any kind of help. Thanks in advance.
"""
    * EULER PROJECT PROBLEM 755
"""
import itertools

def sumOfList(aList):
    sumL = 0
    for x in aList:
        sumL += x
    yield sumL

mostList = [1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233, 377, 610, 987, 1597, 2584, 4181, 6765, 10946, 17711, 28657,
             46368, 75025, 121393, 196418, 317811, 514229, 832040, 1346269, 2178309, 3524578, 5702887, 9227465,
             14930352, 24157817, 39088169, 63245986, 102334155, 165580141, 267914296, 433494437, 701408733, 1134903170,
             1836311903, 2971215073, 4807526976, 7778742049, 12586269025, 20365011074, 32951280099, 53316291173,
             86267571272, 139583862445, 225851433717, 365435296162, 591286729879, 956722026041, 1548008755920,
             2504730781961, 4052739537881, 6557470319842]

def fibonList(x):
    fibonSeq = []
    firstT = 0

    while mostList[firstT] <= x:
        fibonSeq.append(mostList[firstT])
        firstT += 1

    yield fibonSeq

def powerset(iterable):
    b = list(set(iterable))
    for s in range(1, len(b) + 1):
        for comb in itertools.combinations(b, s):
            yield comb

def repWay(x):
    ways = 0
    for each in fibonList(x):
        for i, combo in enumerate(powerset(each), 1):
            for ea in sumOfList(combo):
                if ea == x:
                    ways += 1
    yield ways

def rangerepWays(x):
    generalSum = 0
    for i in range(x + 1):
        for x in repWay(i):
            generalSum += x
    yield generalSum + 1    # + 1 because 0 = 0, while creating fib list I didn't incorporate 0 into fiblist.

for x in rangerepWays(10 ** 2):
    print(x)

1- First: It works, but, unfortunately, is too slow.

I have tested the program with the given examples in the question and it works. It passed both.

2- Second: What I have done

As you can see, the functions don't return anything, instead, I used
generators to avoid Memory Error errors.
To prevent calculating Fibonacci numbers that are below a number at each recursion, I
first created mostList than created a list whose elements are
smaller than an upper limit.

3- Third: What else can be done?

I am aware of this is one of the last questions of the Euler Project. Therefore, probably it requires a deep knowledge of mathematics. However, I lack that. It is okay if you share just only the names of applicable formulas or theories. I can try them to implement my current code.
My code may also have algorithmic fallacies or misinterpretations of the question. If there is one, you can just mention where it is and why there is a problem, not the solution.

Thanks in advance!

A low hanging fruit first. You don't need to work through the entire powerset. Given that \$i_0 < i_1 < ... < i_k\$, and \$N = F_{i_0} + F_{i_1} + ... + F_{i_k}\$, how small \$F_{i_k}\$ could be?
Recall that \$F_0 + F_1 + ... + F_n = F_{n+2} - 2\$. As long as \$F_{n+2} - 2 < N\$, a representation is impossible, so the term \$F_n\$ cannot be the largest. Therefore, the largest term in the representation must satisfy both \$F_n \le N\$ and \$F_{n+2} - 2 \ge N\$. Since the Fibonacci numbers grow exponentially, this observation drastically reduces the choice of the largest term (one or two possibilities, depending on how \$N\$ compares to \$F_{n+1}\$).
That alone shall give you a boost.

Yet it is still bruteforcing. As you correctly say, there must be an underlying math.
I played with your program, printing the total number of representations for numbers between \$F_n\$ and \$F_{n+1}\$. The results are surprising:
Range     Total
3..4      3
5..7      5
8..12     11
13..20    21
21..33    43
34..54    85

Hmm... \$T_{n+1} = 2T_n - 1\$. Something to prove about, isn't it? Try. I don't want to spoil the fun.

For a proper review:

I don't see the reason for sumOfList, fibList, repWay, and rangerepWays being generators. yield only make sense if it breaks the natural control flow, while preserving some execution context. Those above have no context to preserve; they can safely return. Making them generators only adds overhead.
powerset is a different story. It yields in the midst of a (nested) loop.

Naming deserves some attention. It took me a while to understand what, say, repWays is supposed to accomplish. Along the same line, mostList is a list of the Fibonacci numbers, isn't it? - so name it accordingly.


Checking convergence of 2-layer neural network in python I am working with the following code:

import numpy as np

def sigmoid(x):
    return 1.0/(1.0 + np.exp(-x))

def sigmoid_prime(x):
    return sigmoid(x)*(1.0-sigmoid(x))

def tanh(x):
    return np.tanh(x)

def tanh_prime(x):
    return 1.0 - x**2

class NeuralNetwork:

    def __init__(self, layers, activation='tanh'):
        if activation == 'sigmoid':
            self.activation = sigmoid
            self.activation_prime = sigmoid_prime
        elif activation == 'tanh':
            self.activation = tanh
            self.activation_prime = tanh_prime

        # Set weights
        self.weights = []
        # layers = [2,2,1]
        # range of weight values (-1,1)
        # input and hidden layers - random((2+1, 2+1)) : 3 x 3
        for i in range(1, len(layers) - 1):
            r = 2*np.random.random((layers[i-1] + 1, layers[i] + 1)) -1
            self.weights.append(r)
        # output layer - random((2+1, 1)) : 3 x 1
        r = 2*np.random.random( (layers[i] + 1, layers[i+1])) - 1
        self.weights.append(r)

    def fit(self, X, y, learning_rate=0.2, epochs=100000):
        # Add column of ones to X
        # This is to add the bias unit to the input layer
        ones = np.atleast_2d(np.ones(X.shape[0]))
        X = np.concatenate((ones.T, X), axis=1)

        for k in range(epochs):
            if k % 10000 == 0: print 'epochs:', k

            i = np.random.randint(X.shape[0])
            a = [X[i]]

            for l in range(len(self.weights)):
                    dot_value = np.dot(a[l], self.weights[l])
                    activation = self.activation(dot_value)
                    a.append(activation)
            # output layer
            error = y[i] - a[-1]
            deltas = [error * self.activation_prime(a[-1])]

            # we need to begin at the second to last layer
            # (a layer before the output layer)
            for l in range(len(a) - 2, 0, -1):
                deltas.append(deltas[-1].dot(self.weights[l].T)*self.activation_prime(a[l]))

            # reverse
            # [level3(output)->level2(hidden)]  => [level2(hidden)->level3(output)]
            deltas.reverse()

            # backpropagation
            # 1. Multiply its output delta and input activation
            #    to get the gradient of the weight.
            # 2. Subtract a ratio (percentage) of the gradient from the weight.
            for i in range(len(self.weights)):
                layer = np.atleast_2d(a[i])
                delta = np.atleast_2d(deltas[i])
                self.weights[i] += learning_rate * layer.T.dot(delta)

    def predict(self, x):
        a = np.concatenate((np.ones(1).T, np.array(x)), axis=0)
        for l in range(0, len(self.weights)):
            a = self.activation(np.dot(a, self.weights[l]))
        return a

if __name__ == '__main__':

    nn = NeuralNetwork([2,2,1])

    X = np.array([[0, 0],
                  [0, 1],
                  [1, 0],
                  [1, 1]])

    y = np.array([0, 1, 1, 0])

    nn.fit(X, y)

    for e in X:
        print(e,nn.predict(e))

While this converges well and fast when using the tanh, it does converge much slower when using the sigmoid ( in def __init__(self, layers, activation='tanh') change tanh to sigmoid ).
I cannot find why that is. How do I improve the implementation for the sigmoid?

The reasons for the speed discrepancy

The reason for the differences in timing are because evaluating sigmoid_prime() takes far longer than tanh_prime().  You can see this if you use a line profiler such as the line_profiler module.
Is tanh_prime() supposed to be the derivative of tanh()? If so, you might want to double-check your formula.  The derivative of tanh(x) is 1. - tanh(x)**2, not 1. - x**2.
In fact, if you use the the actual definition of the derivative of tanh(), the timings become much more similar.

def tanh_prime_alt(x):
    return 1 - tanh(x)**2

foo = np.random.rand(10000)
%timeit -n 100 tanh_prime(foo)
%timeit -n 100 tanh_prime_alt(foo)
%timeit -n 100 sigmoid_prime(foo)

100 loops, best of 3: 10.2 µs per loop
100 loops, best of 3: 116 µs per loop
100 loops, best of 3: 279 µs per loop

So with this alternate tanh_prime(), the sigmoid method is now only 2× slower, not 20× slower.  I should emphasize that (a) I don't know enough about neural networks to know if 1. - x**2 is an appropriate expression or approximation to the actual derivative of tanh(), but if it is in fact OK, then (b) the reason that activation = 'tanh' is so much faster is because of this approximation/error.
The remaining 2× difference is because in your factored expression of sigmoid_prime(), you are needlessly evaluating sigmoid() twice.  I'd instead do this:

def sigmoid_prime_alt(x):
    sig_x = sigmoid(x)
    return sig_x - sig_x**2

As expected, this speeds things up two-fold relative to your original definition.

foo = np.random.rand(10000)
%timeit -n 100 sigmoid_prime(foo)
%timeit -n 100 sigmoid_prime_alt(foo)

100 loops, best of 3: 248 µs per loop
100 loops, best of 3: 132 µs per loop

Since the sigmoid() function and the tanh() function are related by tanh(x) = 2 * sigmoid(2*x) - 1, i.e. sigmoid(x) = (1 + tanh(x/2.))/2, then iff you are OK with the weird 1 - x**2 approximation for tanh_prime(), you should be able to work out a similar approximation for sigmoid_prime().
You might be interested in the autograd module, which provides a generalized capability to compute symbolic derivatives of most NumPy code.

Other comments

These comments aren't a thorough review, but just some things I noticed.

Why are your weights Python lists instead of NumPy arrays?  If you're already using NumPy, you might as well use it wherever you can.
You probably don't need the for l in range(len(self.weights)): loop, do you?  Can't you use NumPy array slicing and the matrix capabilities of np.dot() to replace this loop?
If you are going to loop, you don't need to do for l in range(len(self.weights)) and then reference self.weights[l].  You can do for weight in self.weights: and then reference weight in your loop code, for example.
Write some docstrings for your functions please!


Sum a list of string numbers I have a list of 32-bit signed numbers in range [-10e5, 10e5] in string form read from stdin, and I would like to compute and print their sum in the fastest way possible.

The bottleneck is in the fast_atoi function (checked using callgrind)

I thinking of two ways for improvement : cache misses seems to be an issue, and SIMD (ideally, only SSE and SSE2) instructions could come to help, however i don't really know how to use them...

The input is in this form (signed 32-bit integers between -10e5 and 10e5, separated by '\n') :

4
10
5
3
1

And the program should output :

19

Right now, my code is :

#include <cstdint>
#include <cstdlib>
#include <cstdio>
#include <cstring>
#include <unistd.h>
#include <limits.h>
#include <sys/stat.h>

#define likely(x)      __builtin_expect(!!(x), 1)
#define unlikely(x)    __builtin_expect(!!(x), 0)

char* data alignas(16) = nullptr;

inline void read_data()
{
    struct stat sb;
    int rc = fstat( STDIN_FILENO, &sb );
    data = (char*)malloc(sb.st_size + 1 );
    size_t totalRead = 0UL;
    while (totalRead  < sb.st_size)
    {
        ssize_t bytesRead = read(STDIN_FILENO, data + totalRead, sb.st_size - totalRead);
        if ( bytesRead <= 0 )
        {
            break;
        }
        totalRead += bytesRead;
    }
}

inline int fast_atoi(char ** str )
{
    int val = 0;
    char neg = 1;
    if (**str == '-')
    {
        neg = -1;
        (*str)++;
    }
    while(unlikely(**str != '\n'))
    {
        val = val*10 + (*(*str)++ - '0');
    }
    ++*str;
    return val*neg;
}
int32_t process()
{
    size_t count = fast_atoi(&data);
    int32_t sum = 0;
    for (unsigned ii = 0; unlikely(ii < count); ii++ )
    {
        sum += fast_atoi(&data);
    }
    return sum;
}
inline void write_ans(int32_t sum)
{
    printf("%d", sum);
}
int main()
{
    read_data();
    int32_t val = process();
    write_ans(val);
}

Callgrind report for this code : https://pastebin.com/Pnd0FzZb
Assembly output : https://godbolt.org/g/Ng3sVM

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

Add some error checking

Many of the calls used in the code, including fstat and malloc can fail.  It's generally better to check for and handle such errors instead of ignoring them.  Even though your main concern is performance, these calls are only made once and so will add almost nothing to the runtime.

Here's an alternative main showing how that might look:

int main() {
    struct stat sb;
    char *data;
    if (fstat(STDIN_FILENO, &sb) != 0 || (data = (char *)mmap(0, sb.st_size, PROT_READ, MAP_SHARED, STDIN_FILENO, 0)) == nullptr) {
        perror("Error reading stdin");
        return 1;
    }
    std::cout << sum(data, sb.st_size) << '\n';
}

Avoid the use of global variables

The data variable is used within read_data and the poorly named process().  It's generally better to explicitly pass variables your function will need or declare them within the appropriately smallest possible scope rather than using the vague implicit linkage of a global variable.

Avoid premature optimization

On my machine, omitting the use of the unlikely macro and the alignas(16) made no difference in the resulting speed of the program.  Generally, I only resort to such things if there is a measurable effect and performance is more important than portability.  Perhaps you've already measured and made this determination and perhaps the results are different on your machine.

Consider an alternative algorithm

An alternative algorithm, which yields times very similar to the ones with your original, might be employed here.  In particular, we could process the file from the end to the beginning instead of the usual direction.  This allows us to efficiently convert each number, only adding the converted number when encountering a newline.  Note that this also essentially throws away the first number (which is the count of numbers to follow) which is the correct behavior in this case, since that number is not intended to be included in the sum.  Here's one version of the sum routine for that algorithm:

int sum(const char *test, std::size_t len) {
    int result{0};
    int n{0};
    int mult{1};
    // start from the back and add digits
    for (const char *ptr = test+len-1; ptr >= test; --ptr) {
        switch(*ptr) {
            case '\n':
                result += n;
                n=0;
                mult=1;
                break;
            case '-':
                n = -n;
                break;
            case '0':
            case '1':
            case '2':
            case '3':
            case '4':
            case '5':
            case '6':
            case '7':
            case '8':
            case '9':
                n += mult * (*ptr - '0');
                mult *= 10;
                break;
            default:
                std::cerr << "Malformed input\n";
                exit(1);
        }
    }
    return result;
}


Generated social media links I'm still fairly new with JavaScript, and wrote something to generate some social media links on my page.  The idea was to have that script grab the necessary URL info and feed it to the social media APIs.

Use the var keyword to declare titleElement and linkElement in this scope, otherwise they will always be global variables and you'll have no choice about it.

Using var will still put them in the global scope until you wrap this logic in a closure:

(function() {
    // var titleLink...
})();

You should still take the advice of @NebulaFox - don't use document.write.


Binary searching the turning point of a function The problem

You are given a list of \$ n \$ positive integers. Your task is to split the list into \$ k \$ subarrays so that the largest subarray sum is minimized.

The input

On the first line you have two integers n and k; the size of the list and the number of subarrays. On the second line you have n numbers: x1, x2, ..., xN.

The limits

\$ 1 \leq n \leq 10^5 \$

\$ 1 \leq k \leq n \$

\$ 1 \leq x \leq 10^9 \$

An example

Input

5 3

2 4 7 3 5

Output:

8 (the subarrays are [2,4], [7], [3,5])

My solution

Let's define a function isPossible, that returns true if it is possible to divide the list into \$ k \$ subarrays so that the maximum sum is the parameter sum. The function loops through the user-inputted array and pushes elements into a vector as long as the elements do not exceed the parameter sum. When the vector can't take the next element without exceeding sum, a new vector is created. All these created vectors are pushed into a vector and if the size of the vector holding all the vectors is less than or equal to \$ k \$, return true. Else return false.

The method vectorsum is a shortcut to getting the sum of the elements inside a vector.

After that I handle the input in main() and binary search the turning point of isPossible.

What went wrong?

I can handle the small test cases easily, but with larger numbers I get a "time limit exceeded"-error. However the time limit is 1 second and I don't think the test even took so long, so I suspect there might be something off with my data types. Any other improvements are accepted, too!

#include "stdafx.h"
#include <iostream>
#include <vector>
#include <numeric>
#include <math.h>
using namespace std;

int arraysize;
int numberofsubarrays;
long long usernumber;
vector<long long> userArray; //Used in main()

vector<vector<long long>> subarrays; //Used in isPossible()

int vectorsum(vector<long long> userVector) {
    int ans = accumulate(userVector.begin(), userVector.end(), 0);
    return ans;
}

bool isPossible(long long n) {
    subarrays = {};
    subarrays.push_back({});
    for (auto u : userArray) {
        subarrays.back().push_back(u);
        if (vectorsum(subarrays.back()) <= n) {
            continue;
        }
        else {
            subarrays.back().pop_back();
            subarrays.push_back({ u });
        }
    }
    if ((long long)subarrays.size() > numberofsubarrays) {
        return false;
    }
    else {
        return true;
    }
}

int main()
{
    cin >> arraysize >> numberofsubarrays; //Input handling
    for (int u = 0; u < arraysize; u++) {
        cin >> usernumber;
        userArray.push_back(usernumber);
    }

    long long left = 1;
    long long middle;
    long long right = pow(10, 10);

    while (left + 1 != right) { //Binary search
        middle = ceil((left + right) / 2);
        /*cout << left << "  " << middle << "  " << right << endl;*/
        if (isPossible(middle)) {
            right = middle;
        }
        else {
            left = middle;
        }
    }
    cout << left + 1 << endl;
    return 0;
}

Don't use using namespace std; Writing std:: a few times isn't that bad, and it helps keep the std lib types identifiable in your code.

Don't use globals so much. It is much better to limit the scope of variables to where they are needed and pass them around (possibly by reference) when needed. Usernumber for example is only used when filling the array, So something like this is much better:

for (int u = 0; u < arraysize; u++) {
    long long number;
    std::cin >> number;
    userArray.push_back(number);
}

vectorsum has a vector passed by value, this will copy the entire vector. instead you'll want to pass by const ref:

int vectorsum(const std::vector<long long>& userVector) {
    int ans = std::accumulate(userVector.begin(), userVector.end(), 0);
    return ans;
}

You don't really need this function at all though, you can keep a running total instead and save yourself the loop.

You keep rebuilding the vectors over and over again, you don't even need to do that. Instead you only need to keep track of the sum of the current subarray you are testing and how many subarrays you have built already.

You can also keep track of the largest subarray sum and pass it back as new upperbound. This will reduce the iteration time once the options have been narrowed down somewhat because then you will never try 2 numbers that will create the same partition.

long long isPossible(const std::vector<long long> &user_array, long long max_sum, int max_number_subarrays) {

    long long running_sum = 0;
    long long largest_sum = 0;
    int subarray_count = 0;
    for (auto current : userArray) {
        if (running_sum + current <= max_sum) {
            running_sum += current;
        } else {
            if(running_sum > largest_sum)
                 largest_sum = running_sum;

            running_sum = current;

            subarray_count++;
            if(subarray_count > max_number_subarrays)
                return -1; //early out

        }
    }

    return largest_sum;
}

and in the while loop.

while (left + 1 != right) { //Binary search
    middle = ceil((left + right) / 2);
    long long result = isPossible(userArray, middle, numberofsubarrays)
    if (result >= 0) {
        right = result;
    }
    else {
        left = middle;
    }
}
std::cout << right << endl;

You can initialize left with the largest number in the input Because if it was less than at least one sub array sum will be larger than it.

right can be initialized with the sum of the array where every element is in a single subarray.

int left = 0;
int right = 0;

for (int u = 0; u < arraysize; u++) {
    long long number;
    std::cin >> number;
    userArray.push_back(number);

    if(left < number)
        left = number;
    right+= number;
}


Snippet to get all local classes from a module I'm working on a project which due to the complexity of the business logic has had to pull out some classes to do computation related to some values in a database. To link the code and database, both when inserting and selecting, I need to get a list of all the class names contained within a single file. It works for me, but is this a sane approach?

import inspect

import my_module

my_module_class_names = [
    name for name, clazz in inspect.getmembers(my_module, inspect.isclass)
    if clazz.__module__ == my_module.__name__
]

inspect.getmembers(my_module, inspect.isclass) gets any class members of my_module.
if clazz.__module__ == my_module.__name__ ensures that classes imported into my_module are excluded from the list. This is the main reason I'm asking for a review - I only thought to include this clause because I had already imported other classes, and therefore the list had extraneous members.

Alternatively, from within my_module:

import inspect
import sys

def class_names() -> List[str]:
    return [
        name for name, clazz in inspect.getmembers(sys.modules[__name__], inspect.isclass)
        if clazz.__module__ == sys.modules[__name__].__name__
    ]

I have already used similar code and it looks rather fine. Some nitpicks:

I’d name the variable cls to mimic the name often used as first parameters of @classmethods; or class_ as it is more common;
I’d store a list of classes intead of a list of names, this feels more directly usable (and names are still stored as cls.__name__ if need be);
sys.modules[__name__].__name__ should be just __name__.

Alternatively, since these classes seems related to each other, you may have an inheritance tree; or maybe a common base class. In this case, you could be even more specific using something such as:

[cls for _, cls in inspect.getmembers(my_module, inspect.isclass) if issubclass(cls, my_module.CommonBase)]

or

my_module.CommonBase.__subclasses__()

if there really is a single level of inheritance, but I wouldn't count much on it as it can break so easily.


Comparer that compares by int than by enum type Below is the implementation of a comparer that compares objects of type Region first by index and then by it's type. The sort order of the type is defined within the comparer.

I know, it's a trivial piece of code, however I am happy about any comments about the realization. :)

public class Region
{
    public RegionType Type { get; set; }
    public int Index { get; set; }
}

public enum RegionType
{
    DeletionBeforeNamedRegion,
    DeletionWithinNamedRegion,
    DeletionAfterNamedRegion,
    InsertionBetweenNamedRegions,
    InsertionWithinNamedRegion,
    NamedRegion,
}

public class RegionComparer : IComparer<Region>
{
    private static readonly List<RegionType> sortOrderByType = new List<RegionType>
        {
            RegionType.DeletionBeforeNamedRegion,
            RegionType.NamedRegion,
            RegionType.DeletionWithinNamedRegion,
            RegionType.InsertionWithinNamedRegion,
            RegionType.DeletionAfterNamedRegion,
            RegionType.InsertionBetweenNamedRegions,
        };

    public int Compare(Region x, Region y)
    {
        if (x == null && y == null)
            return 0;
        if (x == null) return -1;
        if (y == null) return 1;

        var result = x.Index.CompareTo(y.Index);
        if (result != 0) return result;

        var xSort = sortOrderByType.IndexOf(x.Type);
        var ySort = sortOrderByType.IndexOf(y.Type);

#if DEBUG
        if (xSort < 0 || ySort < 0)
            throw new NotImplementedException($"Sort order for region type '{(xSort < 0 ? x.Type : y.Type)}' is not defined. Please extend the '{nameof(sortOrderByType)}' list." );
#else
        if (xSort < 0) xSort = int.MaxValue;
        if (ySort < 0) ySort = int.MaxValue;
#endif
        return xSort.CompareTo(ySort);
    }
}

private static readonly List<RegionType> sortOrderByType = new List<RegionType>
{
    RegionType.DeletionBeforeNamedRegion,
    RegionType.NamedRegion,
    RegionType.DeletionWithinNamedRegion,
    RegionType.InsertionWithinNamedRegion,
    RegionType.DeletionAfterNamedRegion,
    RegionType.InsertionBetweenNamedRegions,
};

Sorting needs to be fast, right? So how about using a dictionary for the oder lookup and not the O(n) List.IndexOf?
private static readonly Dictionary<RegionType, int> _regionTypeOrder = new RegionType[]
{
    RegionType.DeletionBeforeNamedRegion,
    RegionType.NamedRegion,
    RegionType.DeletionWithinNamedRegion,
    RegionType.InsertionWithinNamedRegion,
    RegionType.DeletionAfterNamedRegion,
    RegionType.InsertionBetweenNamedRegions,
}
.Select((x, i) => new { Key = x, Order = i })
.ToDictionary(x => x.Key, x => x.Order);

with it, the lines

var xSort = sortOrderByType.IndexOf(x.Type);
var ySort = sortOrderByType.IndexOf(y.Type);

can become
var xSort = _regionTypeOrder[x.Type];
var ySort = _regionTypeOrder[y.Type];

var xSort = sortOrderByType.IndexOf(x.Type);
var ySort = sortOrderByType.IndexOf(y.Type);

if (xSort < 0) xSort = int.MaxValue;
if (ySort < 0) ySort = int.MaxValue;

the index should never be less then zero. If it is, then there is an error. Its an enum and the property isn't nullable so I think the protection isn't necessary.
After getting ther RegionType order from the dictionary you should be able to immediately call this

return xSort.CompareTo(ySort);


Fetching of JSON data through an API with AngularJS I have been trying out AngularJS and developing a mobile application to test out its features and I wanted to see if I could somehow refactor to make my JSON response hopefully quicker in fetching the data. At the moment I have a search functionality that can display from 10 to 30.000 results and usually the application takes over 2 minutes to fetch large amount of results (which is way slower than I hoped for).

I think the reason might lie somewhere where I have the for loop because I am declaring variables inside it which is probably not a good habit but I can't seem to figure out a way to remove the variables out of the loop without breaking the app.

Here is the function fetch() to get the JSON data:

function fetch(){
$http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25000")
.success(function(response){
$scope.songs = [];
$scope.details = response;

for(var i=0; i < response.tracks.length; i++){

  var labelName = response.tracks[i].label_fk;
  var labelLow = labelName.toLowerCase();
  var resultUrl = "http://APIcall.com/stream_music_file.php?lbl="+labelLow+"&alb="+response.tracks[i].album_num+"&trk="+response.tracks[i].track_num+"";
  var imgUrl = "http://www.APIcall.com/online/music/display_album_cover/"+labelLow+"/"+response.tracks[i].album_num+"";

  var allTracks = {
    id: response.tracks[i].track_id,
    lbl_fk: response.tracks[i].label_fk,
    alb_num: response.tracks[i].album_num,
    track_num: response.tracks[i].track_num,
    track_descr: response.tracks[i].description,
    track_dur: response.tracks[i].duration,
    title: response.tracks[i].primary_title,
    artist: response.tracks[i].composers,
    img: imgUrl,
    url: resultUrl
  };

  $scope.songs.push(allTracks);
  }
  });
}

I'm pretty sure this code could be cleaned up better but I would really hope that also it might somehow be faster in fetching the data with some modifications to it.

Your loop is only executed after the data has been fetched, so improving it will not have any effect on how long it takes for the data to be fetched.

JavaScript doesn't have block scope but does have function scope, so variables declared within a function are hoisted to the top of that function. So the fact that you declare your variables inside the for loop has no effect on the speed of your loop.

function foo () {
    var fruits = ["apple", "banana", "cherry"];

    for (var i = 0; i < fruits.length; i++) {
        var fruit = fruits[i];
        console.log(fruit);
    }
}

is the same as

function foo() {
    var fruits = ["apple", "banana", "cherry"];
    var fruit;

    for (var i = 0; i < fruits.length; i++) {
        fruit = fruits[i];
        console.log(fruit);
    }
}

I think your problem is that you are fetching large amounts of data. Displaying 30000 results in one go is not good practice. Does the api you are calling support paging in any way? Can you specify an offset (or something similar) in the request? For example, can you do

$http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25&offset=0") // get results 1-25

$http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25&offset=25") // get results 25-50

$http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25&offset=60") // get results 60-85

If you can't restrict the number of results returned that way, you can at least make sure that you only display a manageable number of results in one go. I.e don't add all the results to the DOM at once, only add them when the user clicks "Next" (if you add some paging controls), or scrolls down far enough (if you implement an "infinite scrolling" solution).

Having said the above, there a couple of small improvements you can make to your loop i.e. cache response.tracks.length and response.tracks[i].

function fetch(){
    $http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25000").success(function(response){
        $scope.songs = [];
        $scope.details = response;

        for(var i=0, numOfTracks = response.tracks.length; i < numOfTracks; i++){
          var track = response.tracks[i];
          var labelName = track.label_fk;
          var labelLow = labelName.toLowerCase();
          var resultUrl = "http://APIcall.com/stream_music_file.php?lbl="+labelLow+"&alb="+track.album_num+"&trk="+track.track_num+"";
          var imgUrl = "http://www.APIcall.com/online/music/display_album_cover/"+labelLow+"/"+track.album_num+"";

          var allTracks = {
              id: track.track_id,
              lbl_fk: track.label_fk,
              alb_num: track.album_num,
              track_num: track.track_num,
              track_descr: track.description,
              track_dur: track.duration,
              title: track.primary_title,
              artist: track.composers,
              img: imgUrl,
              url: resultUrl
          };

          $scope.songs.push(allTracks);
      }
  });
}

Seeing as I don't think the loop is the bottleneck, I would be tempted to use map which arguably results in more readable code.

    var trackToSong = function (track) {var labelName = track.label_fk;
          var labelLow = labelName.toLowerCase();
          var resultUrl = "http://APIcall.com/stream_music_file.php?lbl="+labelLow+"&alb="+track.album_num+"&trk="+track.track_num+"";
          var imgUrl = "http://www.APIcall.com/online/music/display_album_cover/"+labelLow+"/"+track.album_num+"";

          return {
              id: track.track_id,
              lbl_fk: track.label_fk,
              alb_num: track.album_num,
              track_num: track.track_num,
              track_descr: track.description,
              track_dur: track.duration,
              title: track.primary_title,
              artist: track.composers,
              img: imgUrl,
              url: resultUrl
          };
    }

    function fetch(){
      $http.get("http://www.APIcall.com/phrase=" + $scope.search + "&perpage=25000").success(function(response){
            $scope.details = response;
            $scope.songs = response.tracks.map(trackToSong);
          }
      });
    }


Create another Singleton object from a Singleton class I want to create a singleton object of a third party caching library (ThirdPartyKVStore). But I don't want to reference this class all over the place from where I want to access the cache. To address this, I created a wrapper CacheManager which is managing the lifecycle of ThirdPartyKVStore. Does it look ok?

public class CacheManager {

     private static ThirdPartyKVStore thirdPartyCache = null;
     private static volatile CacheManager cacheManager = null;

     private CacheManager(){

     }

     public static CacheManager getConnection(CacheConfig cacheConfig) {
       if (cacheManager == null) {
         synchronized (CacheManager.class) {
           if (cacheManager == null) {
            try {
              thirdPartyCache = new ThirdPartyKVStore(cacheConfig);
              cacheManager = new CacheManager();
           } catch (Throwable e) {
            logger.error("Fn=getConnection Msg=Cache_Initialization_Error_Occurred.", e);
          }
        }
      }
     }
    return cacheManager;
    }

    public static void closeCacheConnection(){
      if(thirdPartyCache != null){
          thirdPartyCache.shutdown();
        }
    }

    public <T> T getCacheData(String key){
      T cachedObject = null;
      String cachedStr = null;

      Entry<String> e = thirdPartyCache.get(key);
      if(e != null){
        cachedStr = e.getPayload();
       }
      if (cachedStr != null) {
             TypeToken<T> typeToken = new TypeToken<T>() {};
             cachedObject = deserializeStr2POJO(cachedStr, typeToken);
     }
    return cachedObject;
    }

    public <T> void setCacheData(String key, T t){
        if (thirdPartyCache != null) {
              try {
                  Entry<String> e = new Entry.Builder<String>(new Gson()
                          .toJson(t))
                          .withVersion(DEFAULT_VERSION)
                          .withApplication(DEFAULT_APP)
                          .build();

                  thirdPartyCache.set(key, e, Integer.MAX_VALUE);
                } catch (Exception e) {
              }
          }
     }
}

It has been some time since I last wrote Java, but I have some general suggestions.

Before focusing on specific details of the code I would recommend you to spend some time on indentation. Having consistent indentation helps reduce the cognitive load when reading your code.

Second I would urge you to consider if a singleton is the appropriate solution for this problem. Using a static class makes testing both the implementation and consuming code difficult. The first being your implementation sharing state between test-cases and you therefore have to spend extra time ensuring you reset the state between each test-case. The latter is due to the consuming code hiding its dependencies. You aren't able to test a consuming class in isolation as you would implicitly also test the cache manager, which in turn also involves the third party code. Furthermore imagine that if the third party implementation relies on I/O operations your tests of the consuming code now also relies on I/O operations. Unit tests especially shouldn't be concerned with I/O such as: file operations, network, databases etc.

To remedy this I would recommend looking into creating an interface, which defines only the required methods of your cache manager. From the provided code an example of an interface could be:

interface ICacheManager
{
    <T> T getCacheData(String key);

    <T> void setCacheData(String key, T value);

    void closeCacheConnection();
}

Then have your cache manager class implement this interface and use the inversion of control principle in your consuming code to makes its dependency on the cache manager explicit. This is done by requiring an instance of your interface in its constructor (dependency injection). Now in your test-cases you are able to create mock instances of your cache manager when testing consuming code. Using an interface also allows you to provide alternative cache manager implementation in your consuming code without changing a thing. The only requirement being all cache manager implementations adheres to your ICacheManager interface.

The next thing, which caught my eye was how the getConnection() method requires an instance of CacheConfig in its parameter list. Imagine that every time your consuming code needs access to your cache manager (by calling this method) it would also need to know about your cache configurations and have an instance of this. This breaks the separation of concerns principle. You can fix this easily by using the previously proposed solution by using  the inversion of control principle.

If this isn't an option I would create a factory class, which I would ensure is always seeded with an instance of the appropriate cache configurations when starting the application. Then have the factory call the getConnection() method and return the cache manager instance. You would therefore use the factory in your consuming code. You could also seed the cache manager directly with the appropriate cache configurations, but I find that more people understand the principle of the factory pattern and it would therefore make your code easier to understand.

  Note that this solution still hides the dependency of either the
  factory or the cache manager in consuming code as mentioned previously
  and therefore retains all the mentioned side-effects.

Next I find it enjoyable to see you checking for NULL after entering the synchronized block in the getConnection() method. Not many people remember to do this. Great job!

You should also include a check to see if the connection has already been closed in the closeCacheConnection() method. Have you checked how the third party code behaves if you try to close the connection twice? Your code should not depend on such things.

In the getConnection() method you write the name of the method in the error log message. I would recommend you to not do that. If the method is renamed and the log message isn't updated the log entries wont correspond with code. As you are already providing the exception a stacktrace accompanies the message and the name of the method would therefore already be available to the logging implementation along with line numbers etc.

Now lastly a minor pain-points of mine. In the getCacheData() and setCacheData() methods you name the variables containing the cache entry as e. This name is often associated with an exception and in the setCacheData() method this is actually the case. While this not being a syntax error I find it easier to define unique names for each variable, which is also descriptive. In this case I would recommend renaming the variables to entry as it's the contents.


Haskell code to verify credit number I am new to Haskell, and wrote a script to verify credit number.
I did some tests, the script worked, but can it be improved further?

isCreditCardNumber :: String -> Bool
isCreditCardNumber number =
    0 == creditCardReminder ( creditCardDouble ( blowupCreditCardNumber ( reverse number)))

blowupCreditCardNumber :: String -> [(Int,Char)]
blowupCreditCardNumber creditCardNumber = zip [1..] creditCardNumber

creditCardDouble :: [(Int,Char)] -> [Int]
creditCardDouble [] = []
creditCardDouble ((index,digit):rest)
    | even index = (numberDoubleToList ((*2) $ digitToInt digit)) ++ creditCardDouble rest
    | otherwise = digitToInt digit : creditCardDouble rest

numberDoubleToList:: Int -> [Int]
numberDoubleToList number
    | number > 9 = map digitToInt (show number)
    | otherwise  = [number]

creditCardReminder :: [Int] -> Int
creditCardReminder xs = sum xs `mod` 10

You can eta-reduce blowupCreditCardNumber:

blowupCreditCardNumber = zip [1..]

Using composition, you can also make isCreditCardNumber pointfree:

isCreditCardNumber = (==0) . creditCardReminder . creditCardDouble . blowupCreditCardNumber . reverse

You don't need to optimize numberDoubleToList by special-casing the one-digit case.

numberDoubleToList = map digitToInt . show

The explicit recursion in creditCardDouble can be averted by using library functions that specialize in particular recursive patterns:

creditCardDouble = concatMap foo where
  foo (index, digit) | even index = numberDoubleToList $ (*2) $ digitToInt digit
                     | otherwise = [digitToInt digit]

foo's name makes foo look like a crutch, and that is good, because it is one.

I would inline definitions that are only used once and do not deserve to be in a library:

isCreditCardNumber = (==0) . (`mod` 10) . sum . concatMap foo . zip [1..] . reverse where
  foo (index, digit) | even index = map digitToInt $ show $ (*2) $ digitToInt digit
                     | otherwise = [digitToInt digit]

digitToInt digit is used in both cases of foo, and so can be factored out:

isCreditCardNumber = (==0) . (`mod` 10) . sum . concatMap foo . zip [1..] . reverse where
  foo (index, digit) = bar index $ digitToInt digit
  bar index | even index = map digitToInt . show  . (*2)
            | otherwise = pure

In fact, we don't need to generate the index and pass it to foo if all we do with it is put it into bar later:

isCreditCardNumber = (==0) . (`mod` 10) . sum . concatMap foo . zip (cycle [pure, map digitToInt . show . (*2)]) . reverse where
  foo (doubler, digit) = doubler $ digitToInt digit

foo is almost trivial, lets get rid of it entirely:

isCreditCardNumber = (==0) . (`mod` 10) . sum . concat . zipWith ($) (cycle [pure, map digitToInt . show . (*2)]) . map digitToInt . reverse

map digitToInt . show is pure on single digits, so we can factor it out of that list and then even factor out the (*):

isCreditCardNumber = (==0) . (`mod` 10) . sum . concatMap (map digitToInt . show) . zipWith (*) (cycle [1, 2]) . map digitToInt . reverse

Note that if you know the parity of the length of the credit card number, you can get rid of the reverse.


Extreme use of pointers in adjacency list graph construction? Playing around with pointers in C++, implemented an adjacency list graph.
Am I going too far/extreme trying to place data onto the heap?
graph.h
#include <memory>
#include <unordered_map>
#include <forward_list>
#include <utility>

template <typename TKey, typename TWeight>
class Graph{

    struct Adjacent {
        TKey key;
        TWeight weight;
        Adjacent(const TKey key, const TWeight weight): key(key), weight(weight) {};
    };

    struct Vertex {
        TKey value;
        std::unique_ptr<std::forward_list<Adjacent>> adjVertexList;
        explicit Vertex(const TKey value){
            this->value = value;
            adjVertexList = std::make_unique<std::forward_list<Adjacent>>();
        }
    };

private:
    // Since keys can be non-numeric,
    // use a hashmap to index into individual adj lists
    std::unordered_map<TKey, std::unique_ptr<Vertex>> vertices;

public:
    void addVertex(TKey value);

    // Everything other than addDirectedEdge is just for a nicer API
    void addDirectedEdge(TKey src, TKey dst, TWeight weight);
    void addUndirectedEdge(TKey src, TKey dst, TWeight weight);
    void addDirectedEdge(TKey src, TKey dst);
    void addUndirectedEdge(TKey src, TKey dst);

};

graph.cpp
#include <stdexcept>
#include "graph.h"

template <typename TKey, typename TWeight>
void Graph<TKey, TWeight>::addVertex(TKey value) {
    vertices.insert(std::make_pair(value, std::make_unique<Vertex>(value)));
}

template <typename TKey, typename TWeight>
void Graph<TKey, TWeight>::addDirectedEdge(TKey src, TKey dst, TWeight weight) {

    auto srcPtr = vertices.find(src);

    if(srcPtr == vertices.end())
        throw std::invalid_argument("Cannot find source");
    else if(vertices.find(dst) == vertices.end())
        throw std::invalid_argument("Cannot find destination");

    Vertex* vertex = srcPtr->second.get();
    vertex->adjVertexList->push_front(Adjacent(dst, weight));

}

template <typename TKey, typename TWeight>
void Graph<TKey, TWeight>::addUndirectedEdge(TKey src, TKey dst, TWeight weight) {
    addDirectedEdge(src, dst, weight);
    addDirectedEdge(dst, src, weight);
}

template <typename TKey, typename TWeight>
void Graph<TKey, TWeight>::addDirectedEdge(TKey src, TKey dst) {
    addDirectedEdge(src, dst, 0);
}

template <typename TKey, typename TWeight>
void Graph<TKey, TWeight>::addUndirectedEdge(TKey src, TKey dst){
    addUndirectedEdge(src, dst, 0);
}
```

Am I going too far/extreme trying to place data onto the heap?

Yes. Containers like std::unordered_map and std::forward_list already store contents on the heap. If you declare a std::unordered_map on the stack, only a little bit of administrative data goes on the stack. But the same happens with std::unique_ptr. So in your code, the use of std::unique_ptr is superfluous and only increases indirection, which might be bad for performance. So just write:
template <typename TKey, typename TWeight>
class Graph {
    ...
    struct Vertex {
        ...
        std::forward_list<Adjacent> adjVertexList;
        ...
    };
    ...
    std::unordered_map<TKey, Vertex> vertices;
    ...
};


Add user to a database and perform relevant checks Below is a function that adds a new user to the database using their email and password. I am using Flask and SQLAlchemy.

@app.route('/users', methods = ['POST'])
@require_appkey
def registerUser():
    errorsList = []
    userInfo = request.get_json()

    # Get "email" and "password"
    email = userInfo.get('email')
    password = userInfo.get('password')
    if email is None or password is None:
        errorsList.append(Error("email/password","Email or Password not entered"))
        return make_error(MainError('400','Incorrect Input',errorsList))

    # Check password meets criteria
    if len(password) > 1024:
        errorsList.append(Error("password","Password maximum length is 1024 characters"))
    if len(password) < 8:
        errorsList.append(Error("password","Password must be at least 8 characters"))

    # Check email meets criteria
    email = email.strip()
    # Check if email address is valid (syntactically)
    match = re.match("[^@]+@[^@]+\.[^@]+", email)
    if match is None:
        errorsList.append(Error("email","Not a valid email address"))
    # Check if email address is already in database
    with contextlib.closing(DBSession()) as session:
        try:
            if session.query(USER).filter_by(USERSEMAIL=email).count():
                errorsList.append(Error("email","This email address already exists"))
        except Exception as error:
            session.rollback()
            logger = logging.getLogger('__name__')
            logger.error(error)
            errorsList.append(Error("database","An error occurred with the database. Please try again later."))
            return make_error(MainError('500','Database error',errorsList))

    # If errors exist, return all errors
    if errorsList:
        return make_error(MainError('400','Incorrect Input',errorsList))

    # Add user to database
    user = USER(email, password)
    with contextlib.closing(DBSession()) as session:
        try:
            session.add(user)
            session.commit()
        except Exception as error:
            session.rollback()
            logger = logging.getLogger('__name__')
            logger.error(error)
            errorsList.append(Error("database","An error occurred with the database. Please try again later."))
            return make_error(MainError('500','Database error',errorsList))
        return jsonify(data=user.serialize())

def make_error(mainError):
    response = jsonify(data=mainError.serialize())
    response.status_code = int(mainError.code)
    return response

class MainError:
    def __init__(self, code, message, errorsList):
        self.code = code
        self.message = message
        # List of Error objects
        self.errorsList = errorsList

def serialize(self):
    return {
    'errorCode': self.code,
    'message': self.message,
    'errorList': [error.serialize() for error in self.errorsList],
    'success': 'false'
    }

My specific questions (not all directly related to Python) are:

Should I be using return in so many different places?
Is there a better way to structure my code? I would love to hear any thoughts you all have on how this could be written better.
Is the way in which I am handling my errors correct?
Seeing as I will probably be using the Database error often, is it worth putting into a function?

MainError can be simplified as follows,

class MainError:
    def __init__(self, code, message, errorsList):
        self.resp=jsonify(dict(errorCode=code,
                        message=message,
                        errorList=[error.serialize() for error in errorsList],
                        success=False))
        self.resp.status_code = int(code)
    def get_error(self):
        return self.resp

Now an error can be returned by return MainError('400','Incorrect Input',errorsList).get_error()

A simple function can do the above thing,

def get_error(code, message, errorsList):
    resp = jsonify(dict(errorCode=code,
                            message=message,
                            errorList=[error.serialize() for error in errorsList],
                            success=False))
    resp.status_code = int(code)

    return resp

For multiple returns define an error object like error=None and use if and elif and return error if you have errors. f.e,

error = None
if email is None or password is None:
        errorsList.append(Error("email/password","Email or Password not entered"))
        error = MainError('400','Incorrect Input',errorsList).get_error()
elif len(password) > 1024:
        errorsList.append(Error("password","Password maximum length is 1024 characters"))
        error = MainError('400','Incorrect Input',errorsList).get_error()
return error if error else jsonify(data=user.serialize())


Breadth-first and depth-first searches I searched the net for some bfs and dfs code but rarely found some as reference. I wrote this for an assignment, I am getting the results fine. I want to make my code more compact and remove unwanted stuff or need suggestions to make the code better. I used stack and queue data structures for the implementation
#include <stdio.h>
#include<stdlib.h>
#include <stdbool.h>
#include <string.h>
int F=-1,R=-1;char q[10];
char v[10],check[10];int a[100][100];char stack[10];int top=-1;
char vertex;int pos[10];int s;char Dfsv[10]; int u=9;
int n=0;
int dequeue(){
  if(F==-1)
  {
    printf("underflow\n");
  }
  else{
        vertex=q[F];
        printf("%c",vertex);
         F++;
         if(F<10)
        { return pos[F];}
        else{ return -1;}
}
}

void insert(int j){
  if(R==9)
  {
    printf("overflow\n");
  }
  else if(F==-1&&R==-1)
  {  F++;R++;
    check[j]='v';
      vertex=v[j];
      q[R]=vertex;
  }
  else{ if(check[j]!='v')
          { check[j]='v';R++;
            vertex=v[j];pos[R]=j;

            q[R]=vertex;}
  }
}
void BFS(int s){

 int k=s;

  while(k!=-1)
    {
           for(int j=0;j<n;j++)
            {
               if(a[k][j]==1)
               {
                insert(j);

                }
             }
    k=dequeue();
  }
}
void PUSH(int j){
  if(top==9)
  {
    printf("overflow\n");
  }
  else{ if(check[j]!='v')
          { check[j]='v';top++;
            vertex=v[j];pos[top]=j;

            stack[top]=vertex;

  }
}}
int POP(){
  if (top==-1)
  {
    printf("underflow\n");
  }
  else{  Dfsv[u]=stack[top];

          top--;u--;
        if(top!=-1)
       { return pos[top+1];}
       else{ return -1;}

  }
}

void DFS(int s){
 int k=s;
  while(k!=-1)
    {
           for(int j=0;j<n;j++)
            {
               if(a[k][j]==1)
               {
                PUSH(j);

                }
             }
    k=POP();
  }
}
void BDFST(){
printf("enter source number\n");
scanf("%d",&s );
vertex=v[s];insert(s);

printf("BFS: ");BFS(s);printf("\nDFS: ");
memset(check, 0, 10);
PUSH(s);
DFS(s);
for(int u=0;u<10;u++)
{
  printf("%c",Dfsv[u] );
}
}

void print(int a[100][100],int n,char v[])
{ int j=0;
  for(int i=0;i<n;i++)
  {
     for(j=0;j<n;j++)
     { printf("%c%c[%d][%d]: ",v[i],v[j],i,j);
       printf("%d\t",a[i][j]);
     }
  printf("\n");
  }
}

void main(){
int i=0,j=0;
  printf("enter number of vertex\n");
  scanf("%d",&n);
    printf("enter vertex's\n");
     scanf("%s",&v);

  printf("enter the edges\n");
  for( i=0;i<n;i++)
  {  printf("enter 1 for edges of %c\n",v[i]);
     for(j=0;j<n;j++)
     {
       printf("%c%c[%d][%d]: ",v[i],v[j],i,j);
       scanf("%d",&a[i][j] );
      printf("\n");
     }
     printf("\n");
  }
    print(a,n,v);

    BDFST();

}

```

Welcome to the Code Review Community.
General Observations
The use of small functions is very good and makes the code more readable, but overall the readability can be improved.
The code does seem to follow the concept of the single responsibility principle.
I'm going to address the code in 2 phases, one is dealing with improving the use of C programming and the second is coding style in general.
The use of structures to represent the queues and the stacks might have decreased the complexity of the code.
For the future it might be good if you provided an example input and expected output.
C Programming Improvements
Check for Warnings
When you initially compile with C use the -Wall compiler flag to list any warnings as well as any errors. In many cases the warnings can indicate possible bugs in the code.
While this code compiles it does have the following warnings using the -Wall flag:
gcc -Wall dfsbfs.c
dfsbfs.c:133:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
  133 | void main(){
      |      ^~~~
dfsbfs.c: In function ‘main’:
dfsbfs.c:138:14: warning: format ‘%s’ expects argument of type ‘char *’, but argument 2 has type ‘char (*)[10]’ [-Wformat=]
  138 |      scanf("%s",&v);
      |             ~^  ~~
      |              |  |
      |              |  char (*)[10]
      |              char *
dfsbfs.c: In function ‘dequeue’:
dfsbfs.c:22:1: warning: control reaches end of non-void function [-Wreturn-type]
   22 | }
      | ^
dfsbfs.c: In function ‘POP’:
dfsbfs.c:87:1: warning: control reaches end of non-void function [-Wreturn-type]
   87 | }
      | ^

The warning about control reaches end of non-void function indicates that the code is missing a return statement if F == -1 in the function dequeue().
The warning about main () indicates that this program is not following the C programming standards about the main() function which should always return an integer value indicating the success or failure of the program to the operating system.
Avoid Global Variables
It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.
Magic Numbers
There are many Magic Numbers in the global declarations (-1, 10, 100 and 9), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.
Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.
Readability and Coding Style
Code Consistency
The indentation of the code is inconsistent. This makes the code much harder to read and maintain. For instance it would be better if the function dequeue() looked something like this:
int dequeue(){
  if (F == -1)
  {
    printf("underflow\n");
  }
  else{
    vertex = q[F];
    printf("%c", vertex);
    F++;
    if (F < 10)
    {
      return pos[F];
    }
    else
    {
      return -1;
    }
  }
}

Some other inconsistencies are that variables are both upper and lower case characters and in most cases are single characters while in some other cases meaningful names. In the C programming language some common practices are that variable and function names are all lower case, and that variable and functions names with multiple words are snake_case. Symbolic constants, which are not used in this program are all upper case.
Horizontal and Vertical Spacing
In most programming languages it is common to improve readability by using horizontal spacing between operators and operands as shown above in the if statements and the printf statements.
The code might be slightly more readable of the open braces { lined up with the closing braces '}', however, this would be determined by local coding standards.


Open, write and close a file I have a small 10-liner function that writes some data to a file using an std::ofstream. I did not explicitly call .close() at the end of my function, but it failed code review with the reason that it is better to explicitly call it for style and verbosity reasons. I understand there is no harm in calling .close() explicitly, but does calling it explicitly just before a return statement indicate a lack of understanding or faith in RAII?

The C++ standard says:

  §27.8.1.2

  virtual ~ basic_filebuf ();

  [3] Effects: Destroys an object of class basic_filebuf<charT,traits>. Calls close().

Am I justified in my argument that calling .close() at the end of a function is redundant and/or unnecessary?

bool SomeClass::saveData()
{
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl;
    saveFile << m_member2 << std::endl;

    saveFile.close(); // passed review only with this line
    return true;
}

The function is only supposed to return false if the file could not be opened for writing.

I would argue the exact opposite.

Explicitly closing a stream is probably not what you want to do. This is because when you close() the stream there is the potential for exceptions to be thrown. Thus when you explicitly close a file stream it is an indication you both want to close the stream and explicitly handle any errors that can result (exceptions or bad-bits) from the closing of the stream (or potentially you are saying if this fails I want to fail fast (exception being allowed to kill the application)).

If you don't care about the errors (ie you are not going to handle them anyway). You should just let the destructor do the closing. This is because the destructor will catch and discard any exceptions thus allowing code to flow normally. When dealing with the closing of a file this is what you normally want to do (if the closing fails does it matter?).


Stop Watch class in C++ using chrono I have written a class that acts like a stopwatch using C++11's std::chrono library. I wrote this because I find it difficult to benchmark my cpp code properly, this class just combines chrono's functionality and gives it a nice interface
I have two classes, stopwatch.h and lapwatch.h. The second one has the functionality of a stop watch but also has a lap function which you can use to multiple intervals
stopwatch.h
#ifndef STOPWATCH_H_
#define STOPWATCH_H_
#include <chrono>

template<typename duration = std::chrono::milliseconds>
class StopWatch
{
public:
  using clock      = std::chrono::steady_clock;
  using time_point = std::chrono::time_point<clock, duration>;

  StopWatch() = default;
protected:
  enum class State : unsigned char;

  static constexpr time_point current_time() noexcept
  {
    return std::chrono::time_point_cast<duration>(clock::now());
  }
public:
  virtual void reset() noexcept
  {
    clock_state = State::idle;
  }

  time_point go() noexcept
  {
    if (clock_state != State::stopped)
      start_point = current_time();

    clock_state = State::running;
    return start_point;
  }

  void stop() noexcept
  {
    if (clock_state == State::running)
    {
      stop_point = current_time();
      clock_state = State::stopped;
    }
  }

  duration elapsed_time() const noexcept
  {
    switch (clock_state)
    {
    case State::idle:
      return std::chrono::duration_cast<duration>(clock::duration::zero());

    case State::running:
      return std::chrono::duration_cast<duration>(current_time() - start_point);

    case State::stopped:
      return std::chrono::duration_cast<duration>(stop_point - start_point);

    default:
      return std::chrono::duration_cast<duration>(clock::duration::zero());
      break;
    }
  }

protected:
  enum class State : unsigned char { idle, running, stopped };

  time_point start_point;
  time_point stop_point;
  State      clock_state = State::idle;
};

lapwatch.h

#ifndef LAPWATCH_H_
#define LAPWATCH_H_
#include "stopwatch.h"
#include <vector>

template<typename duration = std::chrono::milliseconds>
class LapWatch : public StopWatch<duration>
{
  using clock      = typename StopWatch<duration>::clock;
  using time_point = typename StopWatch<duration>::clock;
  using StopWatch<duration>::State;

public:
  LapWatch() = default;
  struct Lap
  {
    duration total_time;
    duration split_time;
  };

  void lap()
  {
    if (this->clock_state != StopWatch<duration>::State::running)
      return;

    Lap current;
    if (laps.size() == 0)
      current.total_time = current.split_time = StopWatch<duration>::elapsed_time();
    else
    {
      current.total_time = StopWatch<duration>::elapsed_time();
      current.split_time = (current.total_time - laps[laps.size() - 1].total_time);
    }
    laps.push_back(static_cast<Lap&&>(current));
  }

  void reset() noexcept
  {
    laps.clear();
  }

  std::vector<Lap> laps;
};
#endif // !LAPWATCH_H_

usage example
#include "lapwatch.h"

int main()
{
  LapWatch<> watch;

  watch.go() // start the timer
  // Some time passes

  watch.lap() // Adds the total and split time, but doesn't stop it

  // Some more time passes
  watch.lap() // Adds another lap

  watch.stop() // stop the watch now

  // Laps are added to a vector in watch.laps

  for(auto const& lap : watch.laps) {
    lap.total_time // Total time passed since the start
    lap.split_time // Total time passed since the previous lap
  }
}

Concerns

Is this a good idea? Would you use it?

Is this a nice design?

I have a github repository for it too, if anyone prefers reading code that way.

Use clock's native resolution everywhere
If you cast time points and durations to something with a different resolution than that of clock, you risk losing precision. So for all internal state of your classes, I would definitely store everything as clock::duration and clock::time_point types. You might consider keeping the template parameter to ensure the return types of public member functions have the requested time resolution, but that just makes it easy for the caller to make the same mistake.
To give an example of where it can go wrong, suppose I write:
StopWatch<std::chrono::seconds> watch;
watch.go(); // done at 11:59:59.999
...
watch.stop(); // done at 12:00:00.000
auto elapsed = watch.elapsed_time();

With your implementation, elapsed will be equal to one second, even though only one millisecond passed.
If the issue is just convenience so the caller doesn't have to call std::chrono::duration_cast<>() themselves, I would remove the template parameter from the class, and make the member function elapsed_time() a template instead, like so:
template <typename duration = clock::duration>
duration elapsed_time() const noexcept
{
    ...
}

current_time() should not be constexpr
If you mark a function constexpr, you promise that this function can be evaluated at compile-time. But that does not make sense here.
Consider not storing lap results in LapWatch
Having struct Lap and a function to mark when a lap happened is good. However, by making lap() store the results in a member variable, you now gave LapWatch the responsibility to store laps. That might sometimes be exactly what you want, but maybe the caller wants to do something completely different. Maybe the previous laps don't need to be stored, or they need to be stored in a different way. I would make lap() return a Lap instead, and have the caller decide whether it wants to add those to some container. So:
Lap lap() {
    Lap current;
    ...
    return current;
}

And then the caller can write:
LapWatch<> watch;
std:vector<Lap> laps;
laps.push_back(watch.lap());

Use std::move() instead of static_cast<Lap&&>()
If you really want to move an object, use std::move(), it is shorter and less error-prone.
Note that neither the cast nor std::move() will do anything in this case, since neither Lap nor std::chrono::duration has a move constructor, but it doesn't hurt either.


Sorting possibly unstable and general jsfiddle/jquery problem I have a simple sorting algorithm that I was told is possibly unstable. I'm wondering what in the sort is unstable. I have test cases written to test all fields with no issues.

A secondary problem is that jQuery is not appending the rows for some reason (works fine in local and server environments).

http://jsfiddle.net/SucGH/

var applyData = function (dataSet) {
    $('#healthplans tbody').empty();
    for (var i = 0; i < dataSet.length; i++) {
        var itemRow = '';
        for (item in dataSet[i]) {
            var newItem = dataSet[i][item];
            if (typeof newItem === 'number') newItem = "$" + newItem;
            itemRow += '' + newItem + '	';
        }
        $('#healthplans tbody').append('
' + itemRow + '
');
    }
}

var order = '';
var sortDataByField = function (field, newOrder, test) {
    if (!newOrder) order = (order == 'asc') ? 'desc' : 'asc';
    else order = newOrder;
    if (field) {
        jsonData.sort(function (a, b) {
            if (typeof b[field] === 'string') {
                var bfield = b[field].toUpperCase();
                var afield = a[field].toUpperCase();
                if (bfield < afield) return (order == 'asc') ? -1 : 1;
                if (bfield > afield) return (order == 'asc') ? 1 : -1;
                return 0;
            } else if (typeof b[field] === 'number') {
                if (order == 'desc') return b[field] - a[field]
                else if (order == 'asc') return a[field] - b[field]
                return 0;
            }
        });
        if (!test) {
            applyData(jsonData);
        }

    } else {
        return 'Field and/or order not properly defined';
    }
}

You are using the built-in sort of Array, which is not guaranteed to be stable per the documentation. You will have to write your own sort algorithm from scratch.

Other than that,

It is considered better form to have a single comma separated var statement
Calling append() inside a loop gives poor performance, just collect everything in a string, and call append() once
You could cache $('#healthplans tbody') since you call it more than once
You should not skip the newline after an if statement
You should consider using hasOwnProperty when using for (item in dataSet[i])
You did not declare item with var
You call too many things item, I am assuming that the property in dataSet is more of itemId ?
All that gives this :

var applyData = function (dataSet) {
    var i,
        itemId
        item,
        itemRow,
        html = '';
        $table = $('#healthplans tbody'),

    $table.empty();
    for (i = 0; i < dataSet.length; i++) {
        itemRow = '';
        for (itemId in dataSet[i]) {
            item= dataSet[i][itemId];
            if (typeof item=== 'number')
              item= '$' + item;
            itemRow += '' + item+ '	';
        }
        html += '
' + itemRow + '
';
    }
    $table.append( html );
}

I did not comment on the sort function, since that most likely will be throw away.


Mixed PHP with jQuery AJAX I have a quick script I came up with and I was wondering if this is a bad idea.

First let me explain what I want to do. I want to create a dynamic script which will add custom AJAX calls for whatever tab element I need it to. For instance, if the user clicks on Profile, then I want to load profile.php from my files.

This is simple enough, however, I have a lot of calls, and I came up with a quick fix. I am creating an array with all the scripts I need to call in PHP, and when the page loads PHP it creates AJAX call functions from the array list:

<?php
$tab_options = array('home', 'profile', 'news', 'api');
foreach ($tab_options as $option)
{
    ?>
    $(<?php echo $option; ?>).click(function() {
        $.ajax({
            type: "POST",
            url: "_template/<?php echo $option; ?>.php",
            dataType: "html",
            success: function(response){
                window.top.document.title = '<?php echo ucwords($option); ?>';
                window.history.pushState("string", "<?php echo ucwords($option); ?>", "?tab=<?php echo $option; ?>");
                $("#<?php echo $option; ?>").html(response);
            },
            error: function(){
                console.log("Problem found!");
            }
        });
    });
    <?php
}
?>

So in this case I would have PHP loop 4 times over the code, and create the 4 functions for me. It works well, and I am able to switch between tabs smoothly. However, I am not sure if this is good practice, or if it should be avoided.

However, I am not sure if this is good practice, or if it should be avoided.

While I don't think it is bad practice, it could be simplified. One technique to simplify it is using event delegation.

For example, the array of tab options could be added to a JavaScript array:

$tab_options = array('home', 'profile', 'news', 'api');
echo 'var tab_options = '.json_encode($tab_options).';';

Then add a click handler to the document and check if the id attribute corresponds to one of those tab options.

$(document).click(function(clickEvent) {
    var targetId = clickEvent.target.id;
    if (tab_options.indexOf(targetId) > -1) {

In that code Array.indexOf() is used but one could also use Array.contains() though it isn't supported by as many (mostly older) browsers (e.g. IE).

Then to get the equivalent of ucwords(), a solution like from this answer can be used:

var properCaseOption = targetId.replace(/\b[a-z]/g, function(letter) {
    return letter.toUpperCase();
});
window.top.document.title = properCaseOption;

With this approach, the JavaScript code is not duplicated once for each tab option. Perhaps it may not be a big issue because that code could easily be updated with the PHP code, it would yield a larger HTML/JS page size.

See a demonstration of this in this playground example but note that the AJAX requests won't yield the pages as expected so the code to update the containers has been commented out. However I made a phpfiddle that has the response from the AJAX request utilized.


Add application root to path I am creating a package to be used in our company that adds the root of the project to the system path.

I will add a source_root.config file in our root directory.

add_root_path.py

import sys
import inspect
from pathlib import Path
from inspect import getsourcefile

CUR_FILE = Path(getsourcefile(lambda:0)).name
SOURCE_ROOT_DEFAULT_FILE = 'source_root.config'

def _get_dir_of_importer_file():
    """ the directory of the file importing the package"""
    frames_stack = inspect.stack()
    for frame in frames_stack:
        # first file that's not importlib._bootstrap and not current file
        if 'py' in frame.filename and Path(frame.filename).name != CUR_FILE:
            file_name = frame.filename
            break

    return Path(file_name).resolve().parent

def find_source_root(source_root_file=SOURCE_ROOT_DEFAULT_FILE):
    importer_dir = _get_dir_of_importer_file()
    while True:  # can go wrong if package is not used correctly
        files_in_dir = map(Path.resolve, importer_dir.glob('*'))
        for file in files_in_dir:
            if file.name == source_root_file:
                return importer_dir
        importer_dir = importer_dir.parent

def append_root_to_path(source_root_file=SOURCE_ROOT_DEFAULT_FILE):
    source_root = find_source_root(source_root_file)
    sys.path.append(source_root.resolve().as_posix())

This is done in order to properly import the files inside the project and because using PYTHONPATH in windows is not that convenient for pytest and alembic (meaning not running the files directly via python.exe).

Your project is setup wrong

Pytest

The simplest solution to most import problems is to just make your project a setuptools package. And install your package.

Whether or not your project is a library or application to fix pytest is really, really simple. You make your project a setuptools package (Which you should already have done if you're making a library). This is by configuring the setup.py file that explains to pip, setuptools, etc how to install your project.

From here you install your package and things just work.

$ pip install .
$ pytest

Applications

Now you might be saying that's cool and all it works for pytest. But now it's broken when I run my program, using python. There are two solutions to that.

Execute your application as a module. Add a __main__.py to the top level directory which will be your application's entry point. Make sure you move your if __name__ == '__main__' code here. And then just use:

$ python -m my_project

Setup an entry point for the setuptools package.

Once you get the above working, then all you need to do is ensure your main guard is only calling a function main. If this is the case, then you can say main is your entry point in your setup.py.

entry_points={
    'console_scripts': [
        'my_project=my_project.__main__:main',
    ],
},

Usage is then just:

$ my_project

This is how the cool kids publish applications to PyPI.

But what about my code? Where's my code review?

Oh yeah, your code is an unneeded hack job. Seriously just make a setuptools package. You get some benefits from it like your project always being on the path, being able to install projects from a private PyPI repository, being able to use tox and nox, having a customizable entry point mapped to a cool name, and not having to use hacks to get your tests to work. I feel I'm biased here, but I really don't see any downsides. Heck I now only use pip to install to Apache.

From here you can just use either of the import strategies you want. I prefer relative imports, but absolute imports might be your jam.

Absolute imports.

from my_project import subpackage
subpackage.foo()

Cool relative imports.

from . import subpackage
subpackage.foo()

MVCE of all of the above

I remember when I was trying to convert to relative imports I Googled and all that I could find was hack jobs. People saying the names of some PEPs that talk about what I'm on about, but only talk about __name__, __files__ and '__main__'. Overall I feel the subjects a shit show mess.

And so if you're like I was and just trying to make some sense of all this nonsense, I have a small MVCE for you. If you copy the files and commands verbatim then you too can have a properly configured Python project.

./src/my_project/__init__.py

./src/my_project/__main__.py (Works with from my_project.foo import FOO too.)

from .foo import FOO

def main():
    print(FOO)

if __name__ == '__main__':
    main()

./src/my_project/foo.py

FOO = 'Bar'

./tests/test_foo.py

import my_project.foo

def test_foo():
    assert my_project.foo.FOO == 'Bar'

./setup.py

from setuptools import setup, find_packages

setup(
    name='my_project',
    packages=find_packages('src'),
    package_dir={'': 'src'},
    entry_points={
        'console_scripts': [
            'my_project=my_project.__main__:main',
        ],
    },
)

Running it

$ cd src
$ python -m my_project
Bar
$ cd ..
$ pip install .
$ pytest
===== 1 passed in 0.05s =====
$ my_project
Bar

In Addition

If you configure your project correctly then your problems should magically disappear. If you still have problems then you should double check you have installed your project.

You may want to use the -e flag when you install the project, so that you don't need to pip install . each time you change a file.

Better yet use this as a stepping stone to upgrade to modern Python development and use tox or nox to run you tests. Not only do these come with the benefit that they test your setuptools package is correctly configured. They also let you, and your coworkers, not fret over having to install a virtual environment and ensure pip install -e . has ran. Just setup a config file and then you all only need to run tox or nox for things to just workTM.


Converting Confluence markup to a simpler XML format I have a code that converts one xml structure to another. I am removing unnecessary tags and replacing some temporarily. I think I have way too many replace operations and wondering if there is a way to optimize this. Can someone pour in suggestions?

Note: I am not using regex to convert xml to plain text. All I am doing using regex is to do away with certain unwanted tags not supported by target xml format, and temporarily replace a few inline elements and replace them back after converting (so that I dont need to parse the inline elements). After sanitizing the string, I am parsing it using jquery xml parser.

Here's the code snippet:

  str = str.replace(/\r?\n|\r/g, ''); //Remove all new line characters
  // Replace <strong> tags to retain and convert them in the end
  str = str.replace(/<strong>/g, '(strong)');
  str = str.replace(/<\/strong>/g, '(/strong)'); //
  // Replace <code> tags to retain and convert them in the end
  str = str.replace(/<code>/g, '(code)');
  str = str.replace(/<\/code>/g, '(/code)');
  // Remove these tags as they arent required
  str = str.replace(/<ac:rich-text-body>/g, '');
  str = str.replace(/<\/ac:rich-text-body>/g, '');
  // Remove 'ac:' from macros elements
  str = str.replace(/ac:/g, '');
  str = str.replace(/<\/*span.*?>/g, ''); //Remove all span tags
  str = str.replace(/<\/*div.*?>/g, ''); //Remove all div tags
  str = str.replace(/<br.*?>/g, ''); //Remove br tags
  str = str.replace(/&nbsp;/g, ''); //Remove non-breaking white spaces
  str = str.replace(/<\/*a.*?>/g, ''); //Remove a tags
  str = str.replace(/<\/*u>/g, ''); //Remove u tags
  str = str.replace(/<\/*em>/g, ''); //Remove em tags

You are using the wrong tool for the job, as explained here:

https://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags

Use XSLT for this job.


Extract version string from text file with Powershell I need to pull the version # out of a Vim script that might look something like:

 " some header blah blah
 " Version: 0.1
 [a bunch of code]
 " Version: fake--use only the first version

The expected output is 0.1. If a Version # doesn't exist, it should return the empty string or something like that.

I'm new to Powershell, so this is what I have so far:

Get-Content somescript.vim -ErrorAction SilentlyContinue |
    Select-String '^" Version: (.*)' |
    select -First 1 -ExpandProperty Matches |
    select -ExpandProperty Groups |
    select -Index 1 |
    select -ExpandProperty Value

It's just that it feels... kind of verbose. For comparison, here's my *nix version:

perl -ne '/^" Version: (.*)/ && do { print "$1\n"; exit }' somescript.vim 2>/dev/null

Or you could write a similarly concise awk script

Is there any hope for my Windows version being as concise?

I haven't found a terser approach using only built-ins, but having a little more confidence now in Powershell, I think I'd simply refactor out the group matching code:

filter Select-MatchingGroupValue($groupNum) {
    if (! $groupNum) {
        $groupNum = 0
    }
    select -InputObject $_ -ExpandProperty Matches |
        select -ExpandProperty Groups |
        select -Index $groupNum |
        select -ExpandProperty Value
}

Then you could use it like:

Get-Content .\somescript.vim -ErrorAction SilentlyContinue |
    Select-String '^" Version: (.*)' |
    select -First 1 |
    Select-MatchingGroupValue 1

In the end, you haven't saved that many lines, but refactoring out the tedious expansions of Matches, Groups, and Value makes the resulting code much clearer IMO.


Adding product with attributes I've been working on refactoring a piece of my code that allows a user to add items to their cart. Each product can also be added with different attributes. Thus if somebody adds the same product, with matching attributes it shouldn't be added again. It should just increase the quantity by += X where X is the quantity they've chosen.

Now I feel like it can be refactored even more, but I'm not sure how to deal with that when it comes to for example making it have less queries or be faster. As it's one of the slowest processes on our application.

The add_product gets a parameter containing product_id, line_item_attributes_attributes and quantity. What I do now is check initially if the cart has that product already in the cart. If it does, it checks if the attributes are a complete match if so adds quantity, if not adds it as a new product.

  // Cart.rb
  def add_product(parameters)
    product  = Product.find(parameters[:product_id])
    quantity = parameters[:quantity].to_i
    product_attributes = parameters[:line_item_attributes_attributes].try(:to_unsafe_h) || {}
    values             = product_attributes.values
    collect_ids        = values.map { |attribute| attribute[:product_attribute] }
                               .compact
                               .flatten
                               .reject(&:empty?)
    attribute_values = collect_ids.map(&:to_i)
    attach_store_to_cart(product.store) if line_items.empty?

    current_item = line_items.where(product_id: product.id)
    if current_item
      existing_item = look_for_exact_item_match(current_item, attribute_values)
      if existing_item
        current_item = line_items.find(existing_item)
        current_item.update_attribute(:quantity, current_item.quantity += quantity)
      else
        current_item = line_items.create(product: product, quantity: quantity)
        add_attributes(current_item.id, attribute_values) if attribute_values.present?
      end
    else
      current_item = line_items.create(product: product, quantity: quantity)
      add_attributes(current_item.id, attribute_values) if attribute_values.present?
    end
    current_item
  end

  def look_for_exact_item_match(similar_items, attribute_values)
    line_item_id = nil
    similar_items.each do |item|
      if item.line_item_attributes.map(&:product_attribute_id).sort == attribute_values.sort
        line_item_id = item.id
        return line_item_id
      end
    end
    line_item_id
  end

  def add_attributes(current_item_id, attributes)
    current_time = Time.current
    columns = 'line_item_id, product_attribute_id, created_at, updated_at'
    values = attributes.map { |attribute| "(#{current_item_id},#{attribute}, TIMESTAMP '#{current_time}', TIMESTAMP'#{current_time}')" }.join(',')
    ActiveRecord::Base.connection.execute("INSERT INTO line_item_attributes (#{columns}) VALUES #{values}")
  end

  def attach_store_to_cart(store)
    update_attribute(:store, store)
  end

I feel like I might be able to improve the checking of attributes, and the collect_ids variable as it's currently doing 3 different things for the array. Which I predict is quite intensive.

I've tried to improve the process already by inserting all the attributes in one query.

I'd be interested to see how this can be improved in terms of efficiency and speed but also in an OOP way. Any feedback?

You're dealing with a lot of incidental complexity here. Especially extracting the product attributes from the parameters seems to be quite involved. If you have control over the form that's producing those there might be more opportunity for making things easier. As things currently are, I decided to create a factory class to allow us to break things down into smaller methods.

I didn't think deeper about the performance problem but how to structure the code in a more OO way. The main idea is to always construct a new line item and compare that to all existing line items until a match is found. The matching logic can go into the LineItem class. We can further simplify things if the LineItem could also take care about its product attributes. I punted on this and created a serialized attribute, but you might as well set up a proper association for this, maybe with a custom writer method for convenience.

Of course you need good test coverage to do a refactoring like this.

class LineItemFactory
  def self.build(parameters)
    new(parameters).build
  end

  def initialize(parameters)
    @parameters = parameters
  end

  def build
    LineItem.build(
      product: product,
      quantity: quantity,
      product_attributes: product_attributes
    )
  end

  private

  attr_reader :parameters

  def product
    @product ||= Product.find(parameters[:product_id])
  end

  def quantity
    @quantity ||= parameters[:quantity].to_i
  end

  def product_attributes
    @product_attributes ||=
      begin
        product_attributes = parameters[:line_item_attributes_attributes].try(:to_unsafe_h) || {}
        product_attributes.values.map { |attribute| attribute[:product_attribute] }
          .flatten
          .reject(&:empty?)
          .map(&:to_i)
      end
  end
end

class LineItem
  serialize :product_attributes
  delegate :store, to: :product

  def matches?(other)
    self.product_id == other.product_id &&
      self.product_attributes.sort == other.product_attributes.sort
  end
end

class Cart
  # [...]

  def add_product(parameters)
    line_item = LineItemFactory.build(parameters)
    attach_store_to_cart(line_item.store) if line_items.empty?
    existing = line_items.detect { |li| li.matches?(line_item) }
    if existing
      existing.quantity += line_item.quantity
      existing.save
    else
      line_items << line_item
    end
  end

  def attach_store_to_cart(store)
    update_attribute(:store, store)
  end
end


Converting colours in an image using Python Imaging Library I have written the beginning of a map engine which runs off PIL and some JavaScript client side. Ignoring the rest of the code that is in the same file as this function, can you suggest any improvements to this code here?

def convert_colour(region_number, incolour, outcolour):
    #print(region_list.regions_d[region_number][0].split('\\')[7], incolour, outcolour)
    img = Image.open(region_list.regions_d[region_number][0])
    img = img.convert("RGBA")
    newData = []
    newData = [outcolour if item==incolour else item for item in img.getdata()]
    img.putdata(newData)
    img.save(region_list.regions_d[region_number][0], "PNG")
    colour_change_single(region_number, outcolour)

Further to a suggestion I have tried to include numpy but recieve the following error:

Traceback (most recent call last):
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 227, in <module>
    run_function_tests()
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 219, in run_function_tests
   run_engine_test(change_list)
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 161, in run_engine_test
    convert_colour(x[0], region_list.regions_d[x[0]][1], x[1])
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 115, in convert_colour
    new_img = _convert_colour(img, incolour, outcolour)
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 108, in _convert_colour
    img[_colour_mask(img, incolour)] = outcolour
ValueError: shape mismatch: value array of shape (3,) could not be broadcast to indexing result of shape (216,4)

I realised there needs maybe some clarity about this traceback.
run_function_tests() this has code to run a short test with my old code.

def run_engine_test(change_list):
for x in change_list:
    convert_colour(x[0], region_list.regions_d[x[0]][1], x[1])

change_list = [(n,random.choice(colour_list.colour_list)) for n in region_list.regions_d]

We also have a function called colour detect which will check for patterns in the colour

def colour_detect():
#Detect if colours in the image match the colours on the palette and re-maps the dictionary if a region colour is changed
    detected_colours = []
        for x in region_list.regions_d:
            region_colour = print_img_data(region_list.regions_d[x][0])
            region_list.regions_d[x] = [region_list.regions_d[x][0], region_colour]
        if region_colour not in detected_colours:
            detected_colours.append(region_colour)

This is intended to be run at the beginnig of a "game or round" and then in between the engine ticks when actions like wars are comitted. Hence why it checks all of the images to ensure that the dictionary has been updated correctly.

After that the traceback comes from your functions.

[Edit]
The error was related to a typo in my code. The suggestion provided reduced the time to do the functions by 2/3rds!!!!

This is the image i am editing (c)

The easiest way is to use the numpy interface for this, since it allows you to do operations on the whole image:

from PIL import Image
import numpy as np

def _colour_mask(img, colour):
    """Finds all indices of a single colour in a PIL.Image"""
    if len(img.shape) == 3:
        return (img == colour).all(axis=2).nonzero()
    elif len(image.shape) == 2:
        return (img == colour).nonzero()
    else:
        raise ValueError("Invalid image shape {}".format(img.shape))

def _convert_colour(img, incolour, outcolour):
    """Replaces incolour with outcolour in a PIL.Image.

    Returns a new PIL.Image.
    Assumes that img has as many channels as len(incolour) and len(outcolour).
    """
    img = np.array(img)
    img[_colour_mask(img, incolour)] = outcolour
    return Image.fromarray(img)

def convert_colour(region_number, incolour, outcolour):
    file_name = region_list.regions_d[region_number][0]
    img = Image.open(file_name)
    new_img = _convert_colour(img, incolour, outcolour)
    new_img.save(file_name, "PNG")
    colour_change_single(region_number, outcolour)