Calculating the count of integer partitions of a number (uses stateful vectors internally) I've written a Haskell function to count the number of partitions of an integer - it's basically an implementation of the same algorithm as this one, using Euler's Pentagonal Number Theorem:

$$P(n) = \sum_{k=1}^n (-1)^{k+1} \left[ P\left( n - \frac{k\left(3k-1\right)}{2} \right) + P\left( n - \frac{k\left(3k+1\right)}{2} \right) \right]$$

To get it to complete on large-ish inputs (say, 60,000 or so) on a reasonable time , I used a mutable vector, internally. (This cut the time by about half from using an immutable vector, and by a ... very large amount, I forget how much, from using "pure" memoization.) It now calculates \$p(60000)\$ in about 8 seconds or so on my machine, which is reasonable for my purposes. (Though three times slower than a C++ equivalent.)

I wondered if anyone had any suggestions for getting similar performance, but without using mutable state?

Gist

{-# LANGUAGE BangPatterns #-}

import Control.Monad (when, forM_, forM)

import Data.STRef
import Control.Monad.ST

import qualified Data.Vector.Generic.Mutable as GM
import Data.Vector.Generic.Mutable ( write )
import qualified Data.Vector.Mutable as VM

main :: IO ()
main = do
  print $ part 60000

fint :: (Num b, Integral a) => a -> b
fint = fromIntegral

part :: Int -> Integer
part n = runST $ do
  vec <- VM.replicate (n+1) (-3)
  write vec 0 1
  result <- newSTRef (-1)
  forM_ [1..n] $ \i' -> do
      let i = ((fromIntegral i') :: Integer)
      !sR <- newSTRef 0
      let -- loop :: Integer -> ST s ()
          loop k = do
              let f = (fint (i - k * (3 * k - 1) `div` 2))
              when (not (f < 0)) $ do
                   if k `mod` 2 /= 0
                      then do vec_f <- GM.read vec f
                              modifySTRef' sR (\s -> s + vec_f)
                      else do vec_f <- GM.read vec f
                              modifySTRef' sR (\s -> s - vec_f)
                   let f = (fint (i - k * (3 * k + 1) `div` 2))
                   let xx = f :: Int
                   when (not (f < 0)) $ do
                        if k `mod` 2 /= 0
                          then do vec_f <- GM.read vec f
                                  modifySTRef' sR (\s -> s + vec_f )
                          else do vec_f <- GM.read vec f
                                  modifySTRef' sR (\s -> s - vec_f)
                        loop (k + 1)
      loop 1 -- k starts at 1
      !s <- readSTRef sR
      write vec i' s
      when (i' == n) $
        writeSTRef result s
  readSTRef result

You can achieve comparable performance without resorting to mutable state by using IntMap and carefully avoiding unnecessary computations. Let's start with the code below, which is actually the very first Haskell solution at your  Pentagonal Number Theorem link:

import Data.IntMap ((!), fromList, insert, findWithDefault)

partition :: Int -> Integer
partition x = if x < 0 then 0 else foldl p (fromList [(0,1)]) [1..x] ! x where
    p s n = insert n (sum [(-1)^(k+1) * (r pred + r succ) | k <- [1..n],
        let r f = findWithDefault 0 (n - div (k * f (3 * k)) 2) s]) s

This code is pretty, concise and expressive, and it's very easy to see that it does implement Euler's formula indeed. Alas, it's not very efficient - on my laptop, it takes more than 5 seconds to compute partition 5000 (I didn't measure how long partition 60000 would take). How can we improve its performance? Well, there's some low-hanging fruit:

(-1)^(k+1) looks suspect: we don't really need exponentiation actually - what we need is 1 if k is odd and (-1) if k is even.
k <- [1..n] gives us way too many ks. We should only consider those k values for which at least one of $$n - \frac{k\left(3k-1\right)}{2}$$ and $$n - \frac{k\left(3k+1\right)}{2}$$ is non-negative (because if both of them are negative, then the sum within the square brackets in the formula above is zero). The largest such k is approximately $$\sqrt\frac{2n}{3}$$ (the exact maximum can be computed easily by solving a quadratic equation).
That is, instead of n possible values for k, we only need to consider fewer than sqrt n values. And that matters a lot because the code performs a lookup (findWithDefault) in the IntMap for each k value considered.
These two (i.e., the elimination of (-1)^(k+1) and the reduction of the number of lookups) are the biggest wins, but there's some additional minor stuff you can do to improve performance, e.g., you can eliminate one multiplication and one division by noticing that $$n - \frac{k\left(3k+1\right)}{2} = n - \frac{k\left(3k-1\right)}{2} - k$$
Also, it doesn't hurt to try using a strict IntMap instead of a lazy one.

After implementing all of these suggestions, the code looks like this:

import Data.IntMap.Strict ((!), fromList, insert)

partition :: Int -> Integer
partition x = if x < 0 then 0 else foldl p (fromList [(0,1)]) [1..x] ! x where
    p s n = insert n (sum [sign k * sumOfTwoEarlierPs s n k | k <- [1..maxK n] ]) s
    maxK n = round $ 1/6 + sqrt(1/36 + 2/3 * fromIntegral n)

sign k = if odd k then 1 else (-1)

sumOfTwoEarlierPs s n k = p1 + p2 where
  i1 = n - (k *(k+k+k - 1)) `div` 2
  i2 = i1 - k
  p1 = nonNegativeIndexOnly s i1
  p2 = nonNegativeIndexOnly s i2
  nonNegativeIndexOnly s i = if i >= 0 then s!i else 0

On my laptop, this computes partition 60000 in less than 5 seconds, and its running time is only 5% or so more than that of your code.


Insert or update a user in database The code uses SQLAlchemy Core. However, what I want to know is if writing code in this pattern is recommendable.

def saveUser(self, user):
    """
    Inserts or updates a user in database.
    The method first checks if the provided user's id already exists. If no, then the user will be inserted. Otherwise, the stored values are updated.

    Args:
        user (User)
    Returns:
        User object with inserted id
    """
    result = self._engine.execute(self._userTable.select().where(self._userTable.c.Id == user._id))
    if not result.first():
        # user doesn't exist in database.
        result = self._engine.execute(self._userTable.insert(), Email=user.email, PasswordHash=user.passwordHash,
                                      DisplayName=user.displayName, Active=user.active)
        user._id = result.inserted_primary_key[0]
    else:
        # user already exists in database.
        result = self._engine.execute(self._userTable.update().where(self._userTable.c.Id == user._id),
                                      Email=user.email, PasswordHash=user.passwordHash, DisplayName=user.displayName, Active=user.active)
    return user

def deleteUser(self, userId=None, email=None):
    """
    Deletes a user.
    Either userId or email can be passed. If both are passed, then a user containing both userId and email will be deleted.
    If none of the arguments are provided, then the method call will have not effect.
    Args:
        userId (int): If this is None, only email will be used to delete the user.
        email (str): If this is None, only userId will be used to delete the user.
    """
    if not userId and not email:
        return # Because an empty and_ clause generated later on will raise OperationalError.
    idClause = self._userTable.c.Id == userId if userId else None
    emailClause = self._userTable.c.Email == email if email else None
    finalClause = and_(idClause, emailClause)
    deleteQuery = self._userTable.delete().where(finalClause)
    result = self._engine.execute(deleteQuery)

The only thing I can think of that might make it more Pythonic would be to wrap the body of deleteUser() in a try and catch and handle the OperationalError rather than checking user and email for None before making the attempt.


Find almost-shortest path more efficiently I'm trying to solve a SPOJ problem, which is code SAMER08A at this link.

The problem is that when I ran my code in my IDE with all the examples given in the problem, the code works perfectly.  But when running it in SPOJ, it exceeds the time limit of 2 seconds.

I am frustrated because I have worked so hard on it.  I would love some advice and help.

#include <vector>
#include <iostream>
#include <queue>
using namespace std;

typedef pair <int, int> dist_node;
typedef pair <int, int> edge;
const int MAXN = 500;
const int INF = 1 << 30;
vector <edge> g[MAXN];
int d[MAXN];
vector<int> p[MAXN];
int answer[MAXN];
int last_node;
vector<vector<int> > paths;

int dijkstra(int s, int n,int t){
    for (int i = 0; i <= n; ++i){
        d[i] = INF;  p[s].push_back(-1);
    }
    priority_queue < dist_node, vector <dist_node>,greater<dist_node> > q;
    d[s] = 0;
    q.push(dist_node(0, s));
    while (!q.empty()){
        int dist = q.top().first;
        int cur = q.top().second;
        q.pop();
        if (dist > d[cur]) continue;
        for (int i = 0; i < g[cur].size(); ++i){
            int next = g[cur][i].first;
            int w_extra = g[cur][i].second;
            if (d[cur] + w_extra < d[next]){
                d[next] = d[cur] + w_extra;
                p[next].clear();
                p[next].push_back(cur);
                q.push(dist_node(d[next], next));
            }
            else if(d[cur] + w_extra == d[next]){
                p[next].push_back(cur);
            }
        }
    }
    return d[t];
}

void findpath (int t, int depth){
    if(t == 0){
        vector<int> camino;
        for(int i = depth-1; i >= 0; --i){
            camino.push_back(answer[i]);
        }
        camino.push_back(last_node);
        paths.push_back(camino);
        return;
    }
    for(int i = (int)p[t].size()-1; i >= 0; --i){
        answer[depth] = p[t][i];
        findpath(p[t][i], depth+1);
    }
}

void findAlmostShortestPath(){

    for(int k = 0; k<paths.size();k++){
        vector<int> camino = paths[k];
        for(int i = 0;i<camino.size()-1;i++){
            int x = camino[i];
            int y = camino[i+1];
            for(int j = 0; j < g[x].size() ; j++){
                if(g[x][j].first == y){
                    g[x][j].second =INF;
                    break;
                }
            }
            break;
        }
    }
}

int main(){
    int n,m;
    int w,s;
    bool seguir = true;
    while(seguir){
        cin>>n>>m;
        cin>>w>>s;
        last_node=s;

        if(n == 0 & m == 0){
            break;
        }
        for (int i = 0; i < m; ++i){
            int u, v, w;
            cin >> u >> v >> w;
            g[u].push_back(edge(v, w));
        }
        int i;
        i = dijkstra(w, n,s);
        findpath(s, 0);
        findAlmostShortestPath();
        i = dijkstra(w, n, s);

        if(i==INF){
            i =-1;
        }
        cout<<i<<endl;

        for(int i = 0; i < g[i].size(); i++){
            g[i].clear();
        }
    }

    return 0;
}

I would be so grateful if you could help me since I've worked on this for hours, and it's my first SPOJ problem that I have tried to solve.

Here's a partial/basic structured implementation of the problem. It works for the first two cases, but the last case comes back twice as large. I saw this question on SO and thought your version was highly over complicated and probably inefficient due to data structure choices. There isn't any reason to provide and pass around multi-dimensional vectors and such, and there's no doubt you could drop that priority_queue.

Take a look at mine and see if you could simplify yours. Since your logic, I'm assuming, works you should be able to catch the last case without too much effort. I wasn't intending to finish it I just wanted to see if it could easily be done without such complexity. It isn't meant to replace yours, it's just to provide a second opinion on it.

Take a look:

Note: Some of the variable are there because of the problem distription.
      Some validations are missing because the test data didn't require them.

#include <iostream>
#include <vector>
#include <algorithm>

using std::cin;
using std::cout;
using std::vector;
using std::sort;

const int START = 0;
const int END   = 1;
const int DIST  = 2;

vector<int> getPaths(int **, int, int, int);
int getDistance(int **, int , int , int , int , int&);

int main (void)
{

    int input = 0;
    int n = 0, m = 0, s = 0, d = 0, u = 0, v = 0, p = 0;
    int shortestPath = 0;
    int nextShortestPath = 0;
    int **pathPtr;
    vector<int> allDistances;

    cin >> n;
    if (n >= 2 && n <= 500) {
        cin >> m;
        if (m >= 1 && m <= 100000) {
            cin >> s;
            cin >> d;
            pathPtr = new int*[m];
            for (int i = 0; i < m; ++i) {
                pathPtr[i] = new int[3];
            }
            for (int i = 0; i < m; ++i) {
                for (int j = 0; j < 3; ++j) {
                    cin >> input;
                    pathPtr[i][j] = input;
                }
            }
            allDistances = getPaths(pathPtr, m, s, d);
            sort(allDistances.begin(), allDistances.end());
            for (unsigned int i = 0; i < allDistances.size(); ++i) {
                if (allDistances[i] != 0) {
                    shortestPath = allDistances[i];
                    if (i + 1 < allDistances.size()) {
                        nextShortestPath = allDistances[i + 1];
                        break;
                    } else {
                        nextShortestPath = -1;
                        break;
                    }
                }
            }
            cout << nextShortestPath;
        }
    }
    cin.get();
    return 0;
}

vector<int> getPaths(int **paths, const int m, const int s, const int d) {
    vector<int> distances;
    int distance = 0;
    int shortest = 0;
    int tempShort = 0;
    for (int i = 0; i < m; ++i) {
        if (paths[i][START] == s) {
            distance = 0;
            if (paths[i][END] == d) {
                distances.push_back(paths[i][DIST]);
            } else {
                distances.push_back(getDistance(paths, i, i + 1, m, d, distance));
            }
        }
    }
    return distances;
}

int getDistance(int **paths, int i, int j, int m, int d, int &distance) {
    if (paths[j][START] == paths[i][END]) {
        distance += paths[i][DIST];
        if (paths[j][END] != d) {
            if (j < m) {
                distance += getDistance(paths, j, j + 1, m, d, distance);
            } else {
                distance = 0;
            }
        } else {
            distance += paths[j][DIST];
        }
    } else {
        getDistance(paths, i, j + 1, m, d, distance);
    }
    return distance;
}

Let me know if it improved your running time any. Good Luck!


Format an integer with thousands separator (recursive implementation) The function accepts a number n and displays it recursively with commas.

As well as a general code review, I'm interested in suggestions for alternatives to counting to tell when to print the commas.  I tried if(mod%10000 == 0); that works on the first 3 numbers, but after that mod is always a multiple of 10000.

#include <iostream>
using namespace std;

void RecursiveCommas(long long n, int mod = 10, int i = 0);

int main()
{
    RecursiveCommas(123456789);
    return 0;
}

void RecursiveCommas(long long n, int mod, int i){
    if(n%mod==n){ //base case
        if(i%3 == 0 && i != 0)
            cout << (n%mod) / (mod/10) << ",";
        else
            cout << (n%mod) / (mod/10);
    }
    else{
        if(i%3 == 0 && i != 0){ //add commas
            RecursiveCommas(n, mod*10, i+1);
            cout << (n%mod) / (mod/10) << ",";
        }
        else { //no comma needed
            RecursiveCommas(n, mod*10, i+1);
            cout << (n%mod) / (mod/10);
        }
    }
}

ArtemyVysotsky has provided you with a improved solution, so I am just going to give some general advice:

Do not use using namespace std; since it not only lowers readability of your code (things that belong to the standard library are no longer easily discernible) but also opens the door for subtle name resolution bugs.
n is of type long long, but mod is only of type int. Since mod grows until n % mod == n, you are running into the risk of overflow here, so mod should also be of type long long. However, since mod should never be negative, it would be even better to have it be a unsigned long long/
Keep your spacing consistent. For example, why do you sometimes leave space around == and other times don't? There is no right coding style that everyone agrees on, so you are free do choose any option that fits you, but you should at the very least stick to the style you choose throughout your code (also, keep in mind that most programmers consider more space better than less space, so leaving a space around operators is generally better received than not doing so).
Consider wrapping your recursive function in a helper function. Anyone who doesn't know about your code would assume that all three parameters your function takes can and should be passed by the caller when the last two arguments aren't actually supposed to take anything but their default value.
Don't treat single characters as strings when treating them as chars would work fine. In particular, prefer std::cout << ... << ','; over std::cout << ... << "," since this is (likely to be) faster and more efficient.
Having a function output do std::cout directly severely harms its uses. For example, what if I want to send the formatted number to a file? Currently, your function doesn't help much with that. There are several possible improvements here, for example building and returning a std::string or taking an std::ostream as a parameter and output to that instead.
You don't actually need return 0; in main, the compiler is nice enough to add that automatically (Beware, however, as this only happens in main!).


Solve 6 simultaneous equations for 8 variables puzzle with Java I came across this mathematical puzzle, where one really cool answer was a python script to brute force answers.

Here's my take, which is essentially a translation of that to Java:

public class TerribleMath {

    public static void main(String[] args) {
        findAndPrintSolutions(1, 20);
    }

    private static void findAndPrintSolutions(int from, int to) {
        for (int a = from; a < to; a++) {
            for (int b = from; b < to; b++) {
                for (int c = from; c < to; c++) {
                    for (int d = from; d < to; d++) {
                        for (int e = from; e < to; e++) {
                            for (int f = from; f < to; f++) {
                                for (int g = from; g < to; g++) {
                                    for (int h = from; h < to; h++) {
                                        if (isSolution(a, b, c, d, e, f, g, h))
                                            printSolution(new int[] { a, b, c, d, e, f, g, h });
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }

    private static boolean isSolution(int a, int b, int c, int d, int e, int f, int g, int h) {
        if (a + b - 9 != 4)
            return false;
        if ((c - d) * e != 4)
            return false;
        if (f + g - h != 4)
            return false;
        if ((a + c) / f != 4)
            return false;
        if ((b - d) * g != 4)
            return false;
        if (9 - e - h != 4)
            return false;
        return true;
    }

    private static void printSolution(int[] variables) {
        StringBuilder output = new StringBuilder();
        for (int variable : variables) {
            output.append(variable + ", ");
        }
        output.deleteCharAt(output.length() - 1);
        output.deleteCharAt(output.length() - 1);
        System.out.println(output.toString());
    }
}

There are a few ways of abusing the findAndPrintSolutions method that I know of. Also, I get the feeling it could be recursive, although I'm not sure exactly how to write that. Lastly, the class name comes from the fact that the puzzle had to be solved by not doing operations in the pemdas order.

If willing to mix the solution checking logic into the candidate generation logic, we can do better if we drop the requirement that we brute force the entire answer.

private static void findAndPrintSolutions(int from, int to) {
    for (int a = from; a < to; a++) {
        // if a + b - 9 == 4, then
        int b = 13 - a;
        if (from > b || b > to) {
            continue;
        }

        for (int c = from; c < to; c++) {
            // if (a + c) / f == 4
            int f = (a + c) / 4;
            if (from > f || f > to) {
                continue;
            }

            for (int d = from; d < to; d++) {
                if (c == d || b == d) {
                    // these cause divide by zero errors later, so skip them
                    continue;
                }

                // if (c - d) * e == 4
                int e = 4 / (c - d);
                if (from > e || e > to) {
                    continue;
                }

                // if (b - d) * g == 4
                int g = 4 / (b - d);
                if (from > g || g > to) {
                    continue;
                }

                // if 9 - e - h == 4
                int h = 5 - e;
                if (from > h || h > to) {
                    continue;
                }

                if (isSolution(a, b, c, d, e, f, g, h)) {
                    printSolution(new int[] { a, b, c, d, e, f, g, h });
                }
            }
        }
    }
}

This would drop us from 25,600,000,000 iterations of the innermost loop to 8000 with a modest increase in complexity per iteration.  This works because the equations in the solution checker tell us constraints that we can use to determine some of the variables if we have others (comments in the code with more specific explanations).  With sufficient algebra you might eliminate one of the remaining three loops as well.  Note that we have an additional equation that we are not using (f + g - h == 4).

Added logic to verify that the calculated variables are within the from/to range.  This doesn't seem to have been necessary for this particular problem, but that seems likely to have been just luck.  I think that you could probably demonstrate that by setting from to 1 and to to 6.


Stored procedure to swap account holders I'm doing an exercise, and would like to know if what I'm doing is correct and follows the correct structure for rollback in case a transaction fails. I would like to know as well if my try...catch statements are well implemented.

create procedure spTeste2(
    @tit1 bigint = 111,
    @tit2 bigint = 222
) as
begin try
if exists(select primeiroTitular, segundoTitular from ContasBancarias where (primeiroTitular = @tit1 and primeiroTitular is not null) and (segundoTitular = @tit2 and segundoTitular is not null))
    begin
        declare @nib varchar(50) = (select nib from ContasBancarias where primeiroTitular = @tit1 and segundoTitular = @tit2)

        begin transaction
            update ContasBancarias set primeiroTitular = @tit2 where nib = @nib
            update ContasBancarias set segundoTitular = @tit1 where nib = @nib
        commit transaction
    end
else
    begin
        raiserror('Pelo menos um destes titulares não existe, ou não estão nesta ordem', 16, 1)
    end
end try
begin catch
    print ERROR_MESSAGE()

    IF @@TRANCOUNT > 0
        ROLLBACK transaction
end catch

The table ContasBancarias has the following structure:

nib
primeiroTitular
segundoTitular

This code aims to switch the value from primeiroTitular to segundoTitular, and vice versa.

For example, if on a particular row primeiroTitular contains the value 5678 and segundoTitular contains the value 9635, after this stored procedure is executed, primeiroTitular should have 9635 and segundoTitular should have 5678.

Overall, I think your handling of transaction commit/rollback in the try/catch block is reasonable.
Error handling
You may consider:

adding a bit more information to your returning error in the catch block; for instance, you can use RAISERROR to customize your error message with a lot more information. I strongly recommend to read that page on MSDN.

finding a better method than just printing the output to the console if an error occurs; after all, console output is very ephemeral and can get lost easily. Instead, you could log errors to a table in the database, along with some information about how it was called and with what parameters.

Premise
The premise behind the way you wrote this stored procedure is a bit strange. Your goal is to swap the account holders on one account. Your are accomplishing this by requiring both of the account holders' ID and then if an account exists where X is primary and Y is secondary, you swap them.
I think it would make more sense to additionally take in the account number nib as a parameter, and verify that the primary account holder on the account matches what is input, and likewise the secondary also matches, and if they don't, raise a different kind of error stating that the account holders input do not match the account number provided.

You can combine your 2 UPDATE statements into one, since they are both updating the same row in the table.

You also don't need to use your input parameters to make the update, but you can keep it that way if you feel it makes the intention more clear. In my opinion it would be more clear (and possibly less error-prone) to just directly swap the values into one statement. Demo:

declare @Test table (rownum int, ID1 int, ID2 int);
insert into @Test (rownum, ID1, ID2)
  values (1, 42, 99);
update @Test
  set ID1 = ID2,
      ID2 = ID1
  where rownum = 1;
select * from @Test;
/* Result: ***
rownum ID1 ID2
------ --- ---
1      99  42
**************/

Your naming of parameters should reflect what values you are expecting. @tit1 and @tit2 are always used to refer to primeiroTitular and segundoTitular, respectively. Why not just name them that?

The null-checks in your IF EXISTS clause are unnecessary, since your procedure parameters are not nullable, and as such the equals operator necessarily implies that the fields will not be null.

Some line breaks to shorten your long lines helps readability.

Some will disagree, but it is of my personal opinion that using semicolons ; to end statements makes things clearer. This is not required in SQL Server/Transact-SQL, but it is in almost every other database system.

Make sure to pick a name more descriptive than spTeste2 before you commit this into a production database :-)

Reformatted code:
create procedure spTeste2(
    @primeiroTitular bigint = 111,
    @segundoTitular bigint = 222
) as
begin try;
if exists (
        select primeiroTitular, segundoTitular
        from ContasBancarias
        where primeiroTitular = @primeiroTitular
          and segundoTitular = @segundoTitular
    )
    begin
        declare @nib varchar(50) = (
            select nib
            from ContasBancarias
            where primeiroTitular = @primeiroTitular
            and segundoTitular = @segundoTitular
        );

        begin transaction;
            update ContasBancarias
                set primeiroTitular = @segundoTitular,
                    segundoTitular = @primeiroTitular
                where nib = @nib;
        commit transaction;
    end;
else
    begin
        raiserror('Pelo menos um destes titulares não existe, ou não estão nesta ordem', 16, 1);
    end
end try;
begin catch;
    print ERROR_MESSAGE();

    if @@TRANCOUNT > 0
        rollback transaction;
end catch;


Powershell to Convert HTML Table to Array of PSCustomObject I created the below script to sent a request to a website, then convert the table in the results to an array of PSObjects which I can work with in PowerShell.  This uses some nasty hacks (e.g. using a regex to strip HTML tags from my XML to try to improve the likelihood of it parsing as valid XML), but I couldn't find cleaner solutions / this seems to work.

Any thoughts on where I may have used a nasty hack where a more elegant solution exists?

function Create-Url {
    [CmdletBinding()]
    param (
        #using parameter sets even though only one since we'll likely beef up this method to take other input types in future
        [Parameter(ParameterSetName='UriFormAction', Mandatory = $true)]
        [System.Uri]$Uri
        ,
        [Parameter(ParameterSetName='UriFormAction', Mandatory = $true)]
        [Microsoft.PowerShell.Commands.FormObject]$Form
    )
    process {
        $builder = New-Object System.UriBuilder
        $builder.Scheme = $url.Scheme
        $builder.Host = $url.Host
        $builder.Port = $url.Port
        $builder.Path = $form.Action
        write-output $builder.ToString()
    }
}

function ConvertFrom-HtmlTableRow {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true, ValueFromPipeline = $true)]
        $htmlTableRow
        ,
        [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
        $headers
        ,
        [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
        [switch]$isHeader

    )
    process {
        $cols = $htmlTableRow | select -expandproperty td
        if($isHeader.IsPresent) {
            write-output $cols
        } else {
            $colCount = ($cols | Measure-Object).Count
            <# extra overhead that I dont care about right now
            if(-not (($headers) -or ($headers -eq $null) -or (($headers | Measure-Object).Count -ne $colCount))) {
                $headers = 1..$colCount | %{("Column_{0:00000}" -f $_)}
            }
            #>
            $result = new-object -TypeName PSObject
            1..$colCount | %{
                $i = $_ - 1
                if($headers[$i] -ne $null) {
                    $colName = $headers[$i]
                    $colValue = $cols[$i]
                    write-debug "$colName = $colValue"
                    $result | Add-Member NoteProperty $colName $colValue
                }
            }
            write-output $result
        }
    }
}

function ConvertFrom-HtmlTable {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true, ValueFromPipeline = $true)]
        $htmlTable
    )
    process {
        #currently only very basic

...

 structure supported
        #could be improved to better understand tbody, th, nested tables, etc

        #$htmlTable.childNodes | ?{ $_.tagName -eq 'tr' } | ConvertFrom-HtmlTableRow

        #remove anything tags that aren't td or tr (simplifies our parsing of the data
        [xml]$cleanedHtml = '<root>' + ($htmlTable | select -ExpandProperty innerHTML | %{($_ | out-string) -replace '(?:(</?tr)|(</?td))[^>]*(/?>)|(?:<[^>]*>)','$1$2$3'}) + '</root>'
        $headers = $cleanedHtml.root.tr | select -first 1 | ConvertFrom-HtmlTableRow -isHeader
        if ($headers -gt [System.String]::Empty) {
            $cleanedHtml.root.tr | select -skip 1 | ConvertFrom-HtmlTableRow -Headers $headers | select $headers
        }
    }
}

clear-host

[System.Uri]$url = 'http://some.site.with.tables.com/Subnet_Audit.asp' #link to some website
[System.String]$subnet = '123.45.67' #this relates to a specific paramter in the form; in my case the site checks the AV versions of all computers within a given IP range

$rqst = Invoke-WebRequest $url -SessionVariable avsv
$form = $rqst.Forms[0]
$form.Fields["strsubnet"] = $subnet
$url = Create-Url -Uri $url -Form $form
$rqst = Invoke-WebRequest -Uri $url -WebSession $avsv -Method $form.Method -Body $form.Fields
$rqst.ParsedHtml.getElementsByTagName('table') | ConvertFrom-HtmlTable

Parsing HTML

I won't lay into you too badly about using RegEx on HTML since you clearly already know it's a bad idea and are trying to parse it as XML instead. Not a bad idea.

I have to recommend HTML Agility Pack though. It is designed for HTML and it works well with imperfect HTML (unlike the XML parser which is very strict).

It seems to be designed with C# in mind but I've used it in PowerShell before with great success.

Also, you're relying on the parsing done by Invoke-WebRequest, which relies on Internet Explorer. This can have some side effects, for example it won't work on Server Core (because IE is not installed), and it won't work if it's run as a user who has never opened Internet Explorer before. It also sometimes messed up its check of whether IE is available.

Using agility pack won't rely on any of that. You can still use Invoke-WebRequest -UseBasicParsing to retrieve the page without doing the DOM parsing and it doesn't need IE for that call.

Aliases

This is a personal choice, but I don't like aliases in finished scripts that are intended to be reused. I would replace % with ForEach-Object and ? with Where-Object.

Create-Url

There's a bug in this. You accept a parameter called $Uri but refer to $url in the function body.

Once you fix that, it has another problem: it ignores the existing path of the passed in URI, and it ignores whether the form action is absolute (will break), relative with leading / (will work), relative with no leading / (will work when original URI has no path, but will break otherwise).

ConvertFrom-HtmlTableRow

if($headers[$i] -ne $null)

This could just be written as:

if($headers[$i])

ConvertFrom-HtmlTable

if ($headers -gt [System.String]::Empty)

You can use ![String]::IsNullOrEmpty($headers) for this, but really, it can be shortened to just:

if ($headers)


JavaScript / Express error handling w/ promises So I'm wondering how to properly handle errors in JavaScript when utilizing promises. I'm currently throwing errors for everything so that I can break out of the promise - if I can't find something, if there was a missing param, an operation failed, etc.

It seems wrong to be throwing an exception for everything like this, but this is the best thing I've come up with so far after much thought. I am sure to catch all of my thrown errors in the routes which passes them on to my middleware.

Is there a better way?

// SERVICE / MODEL LOGIC
getUserByEmail() {
  if (!this._model.email) {
    throw new VError('Missing email.');
  }

  return UserModel.findOne({ email: this._model.email }).exec()
    .then((result) => {
      if (!result) {
        throw new VError(errors.NOT_FOUND, 'Could not find user.');
      }

      return result;
    })
    .catch((error) => {
      throw new VError(error, 'Could not find user.');
    });
}

// ROUTE
router.get('/users/:email', (req, res, next) => {
  const model = new UserModel({ email: req.params.email });

  res.setHeader('Content-Type', 'application/json');
  return new UserService(model).getUserByEmail()
    .then(result => res.status(200).send(result))
    .catch(error => next(error));
});

// MIDDLEWARE
const errorHandling = (error, req, res, next) =>
  res.status(VError.info(error).status || 500).send(error.message);

There's a big problem here.  getUserByEmail() can either return a promise or throw synchronously which is a bad design and very unfriendly for the caller.  Instead, you want any synchronous errors to be returned as a rejected promise.

The beginning of your code should be changed from this:

// SERVICE / MODEL LOGIC
getUserByEmail() {
  if (!this._model.email) {
    throw new VError('Missing email.');
  }

to this:

// SERVICE / MODEL LOGIC
getUserByEmail() {
  if (!this._model.email) {
    return Promise.reject(new VError('Missing email.'));
  }

The other throw statements are what you do from a .then() handler to turn the promise chain into a rejected promise.  You could also do return Promise.reject(...), but throw works just as well.


Complex spreadsheet script optimization Can someone have a look at my script and maybe find something which can be optimized, as there shall be later 100 of it running.

First of all this script shall be triggered once per day to receive data from a website into a sheet called "getdata". Some fields of "getdata" are transferred to "blindcopy" in a certain order. After that the found data shall be put into a sheet called "monitoring" by deleting first row of this sheet and filling the data from "blindcopy" to the row after the last row of "monitoring". These data is referenced to other sheets and finally to the sheet "Output". In the sheet "Output" some cells are colored depending on their relative difference to other cells (first set color of all text in E7:E10 red and if any cell in this range is bigger than its corresponding cell in column D then its text color is set to green).

To not overflow the importXML command to the website on the beginning of the script the given URL is set to a cell and from there it is deleted at the end of the script again. This is because I do not want to call the importxml command everytime when I or others view the "Output" sheet, where none of the data is displayed coming directly via importxml, but it would be called, even if the data will not be displayed because the displayed data on "Output" is referencing to sheets where the importxml commands are executed (I hope I could explain this properly).

OK, the script is working fine no problems with that. But as I want to use this script in 100 different spreadsheets triggered on different times of the day once every day I think I might run into "exceeding CPU time" error. And before that I want to make sure that I have stripped the script down to the minimum (like I changed the getValue to getValue*s*).

It would be fine if someone could have a look into it, as there might be something to improve.

function myFunction() {
  // Open spreadsheet

  var docid = "some_long_strange_id_following_the_key=_in_the_URL_of_the_spreadsheet"
  var doc = SpreadsheetApp.openById(docid);
  var getsheet = doc.getSheetByName("getdata");
  // Get url from C1 and insert to multiple-referenced cell C4
  var cellreference = getsheet.getRange("C1");
  var url = cellreference.getValue();
  var cellreference = getsheet.getRange("C4");
  cellreference.setValue(url);
  // Transferring data from "blindcopy" to "monitoring"
  var getsheet = doc.getSheetByName("monitoring");
  getsheet.deleteRow(1)
  var sheetFrom = doc.getSheetByName("blindcopy");
  var sheetTo = doc.getSheetByName("monitoring");
  var lastRow = sheetFrom.getLastRow();
  var cell = sheetFrom.getRange('A1').offset(lastRow, 0);
  var rowTo = sheetTo.getLastRow() + 1;
  for( var i = 1; i < 33; i++) {
     sheetTo.getRange(rowTo, i).setValues( sheetFrom.getRange(lastRow, i).getValues());
  }
  // Compare values1 to values2 for the coloring
  var s = doc.getSheetByName('Output');
  var values1Rule1 = s.getRange('E7:H10').getValues();
  var values2Rule1 = s.getRange('D7:D10').getValues();
  var matchList = [];
  for (var row in values1Rule1) {
    for (var col in values1Rule1) {
      for (var row2 in values2Rule1) {
        if (values1Rule1[row][col] >= values2Rule1[row][0])
          matchList.push(s.getRange('E7').offset(row, col, 1, 1).getA1Notation()); }}}
  // Turn values1 red first
  s.getRange('E7:H10').setFontColor('RED');
  // Then only turn matches green
  for (var m in matchList)
    s.getRange(matchList[m]).setFontColor('GREEN');
  // delete C4 in "getdata" to avoid too much traffic to URL by importxml due to many lookups of "Output"
  var getsheet = doc.getSheetByName("getdata");
  var blubb = "blockxmlimport";
  var cellreference = getsheet.getRange("C4");
  cellreference.setValue(blubb);
}

From my research, the slowest part of your script is by far SpreadsheetApp.openById(docid), there is nothing much you can do about that. I am not sure how this script will behave once you have a 100 instances of it running, but my prediction is that it will be bad..

Furthermore from a once over:

You never use cell in var cell = sheetFrom.getRange('A1').offset(lastRow, 0);
You never use row2 in for (var row2 in values2Rule1) {, I dont think you need it
You only have to declare cellreference and getsheet once
You should consider lowerCamelCasing: getsheet -> getSheet, cellreference -> cellRefeference
getSheet is a terrible name, you should name the variable of the sheet:

var monitoringSheet = doc.getSheetByName("monitoring");

You call doc.getSheetByName("monitoring") twice, you could simply:

var toSheet = monitoringSheet;

Instead of reading one cell value and then assigning it to another, you could simply use copyTo:

var dataSheet = doc.getSheetByName("getdata");
// Get url from C1 and insert to multiple-referenced cell C4
var c1 = getsheet.getRange("C1"),
var c4 = getsheet.getRange("C4");
c1.copyTo(c4);

At the end you already had a reference to C4, so you could simply:

c4.setValue( 'blockxmlimport' );


Creating a new JFrame on label click So I've been looking for examples on the internet for a while now and this seems the most common way everyone is suggesting, but now I ask is there a better way to make a new jframe pop up on click than this.

The code :

JLabel prevention = new JLabel("Prevencija");
    prevention.setBorder(BorderFactory.createEmptyBorder(0, 0, 0, 100));
    prevention.addMouseListener(new MouseAdapter() {

        @Override
        public void mouseClicked(MouseEvent e) {
            JFrame jf = new JFrame("Prevencija");
            jf.setContentPane(new PreventionPanel());
            jf.setSize(new Dimension(400, 400));
            jf.setVisible(true);
            jf.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        }
    });
    bottom.add(prevention);

In general the code is quite okay, here are a few remarks.

Maybe extract the body of the mouseClicked() method into a separate method, in case you have more code that would perform the same action, like when listening to keystrokes as well.
Consider using a JButton instead of a JLabel and use an ActionListener or even an Action instead of a MouseListener. Using a MouseListener for reacting on mouseClicked on the entire area of a JLabel seems like replicating something on a physical level for which Swing already provides something on a logical level.
I'd use WindowConstants.EXIT_ON_CLOSE instead of JFrame.EXIT_ON_CLOSE, as that's the original definition.
I'd use DISPOSE_ON_CLOSE instead of EXIT_ON_CLOSE and dispose all frames. Applications which use EXIT_ON_CLOSE become more difficult to test. When DISPOSE_ON_CLOSE doesn't work it usually means that you have a bad application design or bugs regarding threading.
Depending on what you want to achieve, you might be more interested in using JDialog or JOptionPane instead of JFrame for this use case. Maybe JOptionPane.showMessageDialog(). I can only recommend to look into those, not tell whether those are better for your use case, as your code and description don't give enough information for more.


Search script for autocomplete suggestions The following search script creates a JSON response for an autocomplete field. The tables and fields search from are flexible to allow using the same script for different type of searches - the idea is to search from each database field separately and see how often a term appears and sort them in descending order.
The part about ordering the found hits for a word by number of occurrences seems like it could be improved, but I don't know how to better approach the problem.
if (isset($_REQUEST['term']) && !empty($_REQUEST['term'])) {

    // define what table -> array(fields) to search from based on a possibly sent
    // parameter 'type', which is 'customer' or 'admin' or empty or some specific
    if (!isset($_REQUEST['type']) || $_REQUEST['type'] === 'customer') {
        $searchFrom = array(
            'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
            'tuotteet'    => array('varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
        );
    } else if ($_REQUEST['type'] === 'admin') {
        $searchFrom = array(
            'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
            'tuotteet'    => array('tuotenumero', 'kuvaus1', 'varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
        );
    } else if ($_REQUEST['type'] === 'tuotesarja') {
        $searchFrom = array(
            'tuotesarjat' => array('sarjanimi')
        );
    } else if ($_REQUEST['type'] === 'tuotenumero') {
        $searchFrom = array(
            'tuotteet'    => array('tuotenumero')
        );
    }

    $found = array();
    $terms = explode(' ', $_REQUEST['term']);

    // loop through all split up terms
    foreach ($terms as $key => $term) {
        if (!empty($term)) {
            $term = mysql_real_escape_string($term);

            // loop through tables
            foreach ($searchFrom as $table => $cols) {
                // for searches from table 'tuotesarjat' limit hits to published items
                $andWhere = $table == 'tuotesarjat' ? ' AND arkistoitu != 1 ' : '';

                // loop through fields
                foreach ($cols as $column) {
                    $sql = "SELECT $column FROM $table WHERE $column ";

                    // to limit searches, for 1-2 letter terms search string start,
                    // else from whole term
                    if (strlen($term) < 2) {
                        $sql .= "LIKE '$term%' $andWhere GROUP BY $column";
                    } else {
                        $sql .= "LIKE '%$term%' $andWhere GROUP BY $column";
                    }
                    // in case a field does not exist, fail silently
                    // (a json query will still be generated returning the
                    // _other_ fields' searches' results)
                    $res = @mysql_query($sql);
                    if (mysql_num_rows($res) > 0) {
                        while($row = mysql_fetch_assoc($res)) {
                            // insert new found word to array with count 1
                            // or increment already found words' counts
                            if (!key_exists($row[$column], $found)) {
                                $found[$row[$column]] = 1;
                            } else {
                                $found[$row[$column]] += 1;
                            }
                        }
                    }
                }
            }
        }
    }

    // sort by number of hits for each word desc
    arsort($found);

    // filter empty strings
    $found = array_filter(array_keys($found));

    // reduce array to global max hits
    while (count($found) > MAX_LIVESEARCH_SUGGESTIONS) {
        array_pop($found);
    }

    // generate and echo json
    $json = '[';
    foreach ($found as $sana) {
        $json .= '"' . $sana . '", ';
    }
    $json = substr($json, 0, -2) . ']';
    echo $json;
}

Upgrading PHP

Upgrade your PHP version. After reading this code I determined that you are running at the most 4.0.6. key_exists() was replaced with array_key_exists() in 4.1.1, and this was some time ago. An old version of PHP runs the risk of having security issues, and you won't be able to get much help because everyone is using the newer version. Some of these suggestions might still work, but others may not be supported in your version, such as this first one.

REQUEST vs. POST/GET

You should stop using $_REQUEST. It is insecure, being that you are running an older version of PHP this is understandable, but if you upgrade, you should definitely update this as well. If you know where the information is coming from, and you should, then you should just use that array. For example, if it is coming from POST use the $_POST array, if it is coming from GET use the $_GET array, etc...

switch vs. if/else

Think of using a switch. It is faster than if/else statements and, in my opinion, cleaner looking and more legible.

$searchFrom = array(
    'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
    'tuotteet'    => array('varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
);

if( isset( $_REQUEST[ 'type' ] ) {
    $type = $_REQUEST[ 'type' ];
    switch( $type ) {
        case 'admin':
            $searchFrom = array(
                'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
                'tuotteet'    => array('tuotenumero', 'kuvaus1', 'varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
            );
        break;
        //Use a case for each "type", exclude customer as it was defined above as a default
    }
}

Foreach Keys

No need to return $key in your foreach loop, especially since you're not using it. Just remove $key => from the declaration. You can also just use array_filter() after exploding it into an array to remove any empty array members. This will relieve some overhead and is cleaner. Also, since you probably don't want to search for the same term twice you can use array_unique() to remove duplicates.

Repetition

Try to make it so that you repeat yourself as little as possible. This is so that if you ever need to change something, you'll only have to do so once. So...

$term .= '%';
if (strlen($term) > 2) { $term = "%$term"; }

$sql .= "LIKE '$term' $andWhere GROUP BY $column";

You'll also find, if you upgrade your PHP version, that mysql() is being deprecated. mysqli() and pdo() are now the preferred methods.

Error Suppressors

DON'T USE ERROR SUPPRESSORS. This is a sign of bad code. If there is an error fix it. If you are doing it just so you don't have to type a couple extra lines, don't. Always ensure your code works 100%. The error suppressor should only be used during debugging.

Incrementing

There's got to be some better way to increment the "found" array... I thought of using a ternary operator, but that got a little messy, and at the moment, I can't think of anything else... I'll update this if I think of anything...

Question

How are there empty strings in $found?

MAX_LIVESEARCH_SUGGESTIONS

Not sure what the MAX_LIVESEARCH_SUGGESTIONS is, but would it not make more sense to perform this before creating the $found array?

$length = count( $terms );
while ( $length > MAX_LIVESEARCH_SUGGESTIONS) { array_pop( $terms ); }

Notice I moved count() out of the loop. With it in the loop, it will run it every time, which means more overhead. Remove it from the loop to remove the overhead. I'm having difficulties following the program, so unless $found ends up with more elements than $terms the above should work. If there are more, then you should limit how many are recorded by breaking out of the loops once that limit is reached.

$length = count( $found );
if( $length == MAX_LIVESEARCH_SUGGESTIONS ) { break 4; } //I think its 4... might be 3...
//perform actions to set $found here

JSON

Updating your PHP will also mean that you can use the JSON library that is now standard instead of needing to recreate it.

Hope this helps!


Formatting big integers as hexadecimal digits separated by colons I'm pretty sure this can be written as a couple of lines, but I'm unable to come up with a better solution.

package main

import (
    "fmt"
    "math/big"
)

func colonedSerial(i *big.Int) string {
    hex := fmt.Sprintf("%x", i)
    if len(hex)%2 == 1 {
        hex = "0" + hex
    }
    numberOfColons := len(hex)/2 - 1
    colonedHex := make([]byte, len(hex)+numberOfColons)
    for i, j := 0, 0; i < len(hex)-1; i, j = i+1, j+1 {
        colonedHex[j] = hex[i]
        if i%2 == 1 {
            j++
            colonedHex[j] = []byte(":")[0]
        }
    }
    // we skipped the last one to avoid the colon at the end
    colonedHex[len(colonedHex)-1] = hex[len(hex)-1]
    return string(colonedHex)
}

func main() {
    fmt.Println(colonedSerial(big.NewInt(1234)))
    // "04:d2"
    fmt.Println(colonedSerial(big.NewInt(123456)))
    // "01:e2:40"
    fmt.Println(colonedSerial(big.NewInt(1234567891011121314)))
    // "11:22:10:f4:b2:d2:30:a2"
}

Also the []byte(":")[0] is very ugly. Is there a better way to do that? (I did not wanted to write the ASCII value of colon (58) as it would be a magic number).

Go Playground version: https://play.golang.org/p/GTxajcr3NY

In Go, we usually expect reasonable performance and brevity. For example,

package main

import (
    "encoding/hex"
    "fmt"
    "math/big"
)

func formatSerial(serial *big.Int) string {
    b := serial.Bytes()
    buf := make([]byte, 0, 3*len(b))
    x := buf[1*len(b) : 3*len(b)]
    hex.Encode(x, b)
    for i := 0; i < len(x); i += 2 {
        buf = append(buf, x[i], x[i+1], ':')
    }
    return string(buf[:len(buf)-1])
}

func main() {
    fmt.Println(formatSerial(big.NewInt(1234)))
    // "04:d2"
    fmt.Println(formatSerial(big.NewInt(123456)))
    // "01:e2:40"
    fmt.Println(formatSerial(big.NewInt(1234567891011121314)))
    // "11:22:10:f4:b2:d2:30:a2"
}

Output:

04:d2
01:e2:40
11:22:10:f4:b2:d2:30:a2

Benchmark: serial := big.NewInt(1234567891011121314):

BenchmarkPeterSO         3000000       552 ns/op        72 B/op      3 allocs/op
BenchmarkKissgyorgy       500000      2545 ns/op       120 B/op      7 allocs/op
Benchmark200Success        30000     40675 ns/op     40675 B/op     35 allocs/op


Use generator to do inorder traversal I am not used to generators and wonder how to make code cleaner/Pythonic in the generator part.

The function is about validating a binary search tree. The inorder_gen function yield one element a time. The main function compares the value yielded and previous yielded value to determine whether BST's invariant is violated.

# Definition for a binary tree node.
# class TreeNode(object):
#     def __init__(self, x):
#         self.val = x
#         self.left = None
#         self.right = None

class Solution(object):

    def isValidBST(self, root):
        """
        :type root: TreeNode
        :rtype: bool
        """
        last_val = None
        for idx, v in enumerate(self.inorder_gen(root)):
            if idx != 0 and v <= last_val:
                return False
            last_val = v
        return True

    def inorder_gen(self, node):
        if node is None:
            return
        if node.left != None:
            for v in self.inorder_gen(node.left):
                yield v
        yield node.val
        if node.right != None:
            for v in self.inorder_gen(node.right):
                yield v

Your structure seems adequate, but there are a couple of mistakes, and some new language syntax in 3.3 that will make things even clearer:

That name!

def inorder_gen(self, node):

The name does make it obvious what order you're using. And it also explains that you're writing a generator - which is putting implementation details in the public interface. Don't do that. You could go with inorder(), but even better would be to make your class iterable using the standard Python approach. See this SO answer for an example of how.
You were off to a good start ...

if node is None:
    return
if node.left != None:

... but then you tried to use != None. Use is None or is not None instead. (Like you did on the first line!)
Use Python's yield from expression to connect a nested generator to your caller. This code:

if node.left != None:
    for v in self.inorder_gen(node.left):
        yield v
yield node.val
if node.right != None:
    for v in self.inorder_gen(node.right):
        yield v

Becomes this:

yield from self.inorder_gen(node.left) if node.left is not None else ()
yield node.val
yield from self.inorder_gen(node.right) if node.right is not None else ()

And if your nodes default to truthy (which most objects do), you can replace if node.left is not None with if node.left, for more readability!

yield from self.inorder_gen(node.left) if node.left else ()
yield node.val
yield from self.inorder_gen(node.right) if node.right else ()

Bonus: If you make your TreeNode class iterable, as mentioned in #1, and you use yield from as mentioned in #3, you can make your code even smaller! Since the nodes are iterable, and since yield from will accept an iterable, you can yield from the nodes directly, like this:

# NOTE: This has to be on the node class, so if you can't change
# that, it won't work. (But maybe you could subclass the TreeNode?)

def __iter__(self):
    if self.left: yield from self.left
    yield self.val
    if self.right: yield from self.right

(In fairness, this last example came from the PEP proposing the yield from syntax: http://www.cosc.canterbury.ac.nz/greg.ewing/python/yield-from/yf_current/Examples/binary_tree.py, and see overview page here: http://www.cosc.canterbury.ac.nz/greg.ewing/python/yield-from/yield_from.html)


PBKDF2 authorization I've discovered that using hashed passwords with salts is a much better idea than MD5/SHA256, so I'm not hashing them with PBKDF2. However, I'm wondering if this is a correct approach to authorizing my user. I also have logic for logging authorizations. When the same IP has entered an incorrect login/pass, it becomes banned for 5 minutes.

Administrator a is passing Username and Password.
Checking if Username exists.
Checking if the password is correct for this user.

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Authorize(Administrator a)
{
    // Check if there are no failed login attempts in last 5 minutes
    if (!this.CanAdminLogin)
    {
        TempData["loginTooManyAttempts"] = true;
        return RedirectToAction("index", "home");
    }

    // If model is not validated return login view to show error messages (javascript disabled)
    if (!TryValidateModel(a))
    {
        return View("Authorize", a);
    }

    // Check if username exists, if not then log and show that login failed
    var admin = _db.Administrators.Where(x => x.Username == a.Username).SingleOrDefault();
    if (admin == null || admin.Username != a.Username)
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
        return RedirectToAction("index", "home");
    }

    // Username exists, check if passwords match
    ICryptoService cryptoService = new PBKDF2();
    string hash = cryptoService.Compute(a.Password, admin.PasswordSalt);
    if (hash == admin.Password)
    {
        this.LogAuthorization(a, true);
        Session["adminId"] = admin.ID;
    }
    else
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
    }

    // Login successfull
    return RedirectToAction("index", "home");
}

I really think that you should merge all of these if statements, make them an if/elseif/else statement.  just move all the variable declarations to the top.

then it turns into this

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Authorize(Administrator a)
{
    var admin = _db.Administrators.Where(x => x.Username == a.Username).SingleOrDefault();
    ICryptoService cryptoService = new PBKDF2();
    string hash = cryptoService.Compute(a.Password, admin.PasswordSalt);
    if (!this.CanAdminLogin) // Check if there are no failed login attempts in last 5 minutes
    {
        TempData["loginTooManyAttempts"] = true;
        return RedirectToAction("index", "home");
    }
    else if (!TryValidateModel(a)) // If model is not validated return login view to show error messages (javascript disabled)
    {
        return View("Authorize", a);
    }
    else if (admin == null || admin.Username != a.Username) // Check if username exists, if not then log and show that login failed
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
        return RedirectToAction("index", "home");
    }
    else if (hash == admin.Password) // Username exists, check if passwords match
    {
        this.LogAuthorization(a, true);
        Session["adminId"] = admin.ID;
    }
    else
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
    }

    // Login successfull
    return RedirectToAction("index", "home");
}

I also moved the comments to the end of the line that they attach to.  Keep in mind that some of these comments are not needed at all.

The Comments that are needed here

// Check if there are no failed login attempts in last 5 minutes

This is only needed because otherwise I wouldn't have known why the Admin couldn't log in or that there was an instance that would change this boolean.

// If model is not validated return login view to show error messages (javascript disabled)

I can tell from the code what is going on, but I am not sure what (javascript disabled) has to do with this code?


Function to copy a file using sendfile I've just found out there is sendfile(). In the man page of sendfile(), it says:

  sendfile() is more efficient than the combination of read(2) and write(2).

So, I wrote a function to copy a file using sendfile().

Is really my code more efficient than the combination of read(2) and write(2)?

/* leftsize is initialized with total size of source file. */
fd_src = open(src_fname, O_RDONLY);
if (fd_src<0){
    printf("open failed(%s)\n", src_fname);
    return -1;
}
fd_dest = creat(dest_fname, 0666);
if (fd_dest<0){
    printf("create failed(%s)\n", dest_fname);
    return -1;
}
while (leftsize > 0) {
    n = sendfile(fd_dest, fd_src, NULL, max_size);
    if (n<0) {
        printf("sendfile failed. ret:%d errno:%d\n", n, errno);
    }
    leftsize -= n;
}

The first paragraph of the man page says it all:

  sendfile() copies data between one file descriptor and another.
         Because this copying is done within the kernel, sendfile() is more
         efficient than the combination of read(2) and write(2), which would
         require transferring data to and from user space.

So, sendfile is quite efficient.

However, your loop may use sendfile incorrectly because you always try to transfer max_size and this could return an error (EOVERFLOW) on the last chunk of the file (i.e. you try to read more than remains in the input file).

This may work just fine if you handle the error case properly.

It also depends upon what max_size and leftsize actually are [which you didn't specify].

I'll assume that max_size is some reasonable chunk limit (e.g. max_size = 1024 * 1024;) and that leftsize is the size of the input file that comes from:

struct stat st;
fstat(fd_src,&st);
leftsize = st.st_size;

So, just to be safe, here is what I'd do:

while (leftsize > 0) {
    // get chunk size
    n = max_size;

    // clip transfer size to remainder of file
    if (n > leftsize)
        n = leftsize;

    n = sendfile(fd_dest, fd_src, NULL, n);
    if (n<0) {
        printf("sendfile failed. ret:%d errno:%d\n", n, errno);
    }

    leftsize -= n;
}


Basic Array based stack in java Here is an array based stack. I tried to implement this basic stack from memory. I want to make sure it is correct.

import java.util.*;

public class Stack{
    private int[] stack;
    private int size;

    public Stack(){
        stack = new int[5];
        int size = 0;
    }

    public Stack(int capacity){
        stack = new int[capacity];
        int size = 0;
    }

    public void push(int pushedValue){
        if(size == stack.length){
            throw new fullStackException();
        }
        stack[size] = pushedValue;
        size++;
    }

    public int pop(){
        if(size == 0){
            throw new emptyStackException();
        }
        int poppedValue = stack[size - 1];
        size--;
        return poppedValue;
    }

    public int peek(){
        if(size == 0){
            throw new emptyStackException();
        }
        return stack[size - 1];
    }

    private class emptyStackException extends RuntimeException{
        public emptyStackException(){
            super();
        }
    }

    private class fullStackException extends RuntimeException{
        public fullStackException(){
            super();
        }
    }

}

Well, it seems to be ok, a simple unit test prove it works.

However, using such private exceptions is not a good idea because nobody can catch them. You should also use the CamelCase name for those exceptions and maybe provide more context information to those exceptions.


Dynamically Change Logging Level of a Python Process The intention of following code is to dynamically change the logging level of a running python process that has imported this module.

#!/usr/bin/python
import logging
import time
import signal

logging.basicConfig(level=logging.ERROR)
logger = logging.getLogger(__name__)
# create logger object with 'ERROR' level logging.

def handler1(signum, frame):
    logger.setLevel(logging.DEBUG)

def handler2(signum, frame):
    logger.setLevel(logging.INFO)

signal.signal(signal.SIGUSR1, handler1)
# if process receives SIGUSR1 signal, the logging level changes to DEBUG.

signal.signal(signal.SIGUSR2, handler2)
# if process receives SIGUSR1 signal, the logging level changes to INFO.

while True:
    logger.info('info logging')
    logger.debug('debug logging')
    logger.error('error logging')
    logger.warn('warn logging')
    time.sleep(2)

The above script runs as expected but I am not sure if this the right way to full fill the goal. Please review the code, as well as issues in above resolution.

Logger.setLevel() is the right way to dynamically change the logging level of a Logger object. Your script will work fine.

I'll nitpick:

Code is read from top to bottom. You should place the comments above the group of statements they are attached to, not below them. If the reader reads the comment first, they won't have to spend extra effort to decipher the meaning of the following lines, because they have read the annotation first.
Be wary of using asynchronous signal handlers when you are using the logging module. I know that this is a tiny example script, but always refer to the documentation when you are using the Python standard library.


Simple logger class in C# - Follow up I've asked a question about this particular code before, although it was a little different back then. I took the suggestions from people who replied to my question and decided to post a follow up to that question, in attempt to improve the class even more.

Simple console logger used instead of packages like log4net and nlog for minimalism.

public class ConsoleLogger <TClass> : ILogger
{
    private static readonly string FilePath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location + "/resources/logging/");

    private readonly Type _className;

    public ConsoleLogger()
    {
        _className = typeof(TClass);
    }

    public void Trace(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.White, logToFile);
    }

    public void Warn(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.Yellow, logToFile);
    }

    public void Debug(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.Cyan, logToFile);
    }

    public void Success(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.Green, logToFile);
    }

    public void Error(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.Red, logToFile);
    }

    public void Error(Exception e, bool logToFile = true)
    {
        Log("An error occurred: " + Environment.NewLine + e, ConsoleColor.Red, logToFile);
    }

    private void Log(string message, ConsoleColor color, bool logToFile = false)
    {
        var oldColor = Console.ForegroundColor;
        Console.ForegroundColor = color;
        Console.WriteLine($"[{DateTime.Now:MM/dd HH:mm:ss}] " + message);
        Console.ForegroundColor = oldColor;

        string logFile;

        switch (color)
        {
            case ConsoleColor.Yellow:
                logFile = "error.log";
                break;
            case ConsoleColor.Cyan:
                logFile = "debug.log";
                break;
            case ConsoleColor.Red:
                logFile = "error.log";
                break;
            default:
                logFile = "trace.log";
                break;
        }

        if (logToFile)
        {
            LogToFile(logFile, $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message);
        }
    }

    private static void LogToFile(string file, string content)
    {
        try
        {
            using (var fileWriter = new StreamWriter(FilePath + file, true))
            {
                fileWriter.WriteLine(content);
                fileWriter.Close();
            }
        }
        catch (DirectoryNotFoundException)
        {
            Directory.CreateDirectory(FilePath);
        }
        catch (FileNotFoundException)
        {
            File.Create(file);
        }
    }
}

ILogger interface:

public interface ILogger
{
    void Trace(string message, bool logToFile = false);
    void Warn(string message, bool logToFile = false);
    void Debug(string message, bool logToFile = false);
    void Success(string message, bool logToFile = false);
    void Error(string message, bool logToFile = false);
    void Error(Exception e, bool logToFile = true);
}

Quickfire review

I see no reason to make this a generic class. You're effectively using the generic type as a name. While having intellisense is nice, keep in mind that the compiler will generate an optimized class for every type you use in your code. E.g. if TClass can either be Foo, Bar or Baz in your codebase; then the compiler created three different ConsoleLogger classes in order to optimize the assembly (actually it creates four, but the point still stands)

Since you're going to cast your generic type to a Type object anyway, it's better to just pass the Type object via the constructor. Functionally equivalent, minor difference in syntax which is ultimately negligible, lessened impact on the compiled assembly.
Personally, I would store the string name (of the type) directly instead of the Type itself, but that's a matter of style.

It seems needlessly repetitive to specify a writeToFile boolean for every message that is logged. I suggest making it a public property or adding it to the constructor so you only have to set it once.
Use Path.Combine(folder, file) instead of folder + file. This has some extra logic to make sure that the appropriate slashes are present and not duplicated. Note that you can use more than two parameters: Path.Combine(folder, subfolder, anotherSubFolder, file)
$"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message You use interpolation for the first two values (good), but then concatenate the third value. You should interpolate that one as well $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: {message}"
In the comments, you mention that it's supposed to be threadsafe, but I see no actual implementation of thread safety in any of the posted code. Are you using separate loggers per thread? (that's different from wanting multple threads to use the same logger).

The rest will be addressed in separate chapters.

Conflated logic

switch (color)
{
    case ConsoleColor.Yellow:
        logFile = "error.log";
        break;
    case ConsoleColor.Cyan:
        logFile = "debug.log";
        break;
    case ConsoleColor.Red:
        logFile = "error.log";
        break;
    default:
        logFile = "trace.log";
        break;
    }

This is not great. What you're doing here is determining the file to write to based on the color you want the log to be written in.

Imagine if Word automatically saved your document to a different location when you change the text color. No one would like that feature.

What you've done here is merged two separate design decisions:

Where do I want to make my log?
What do I want my log to look like?

These questions have nothing to do with each other, but you've merged them. That being said, you're of course going to have to map both behaviors, but not like this.

Create a new enum:

public enum LogType { Success, Warn, Error, Debug }

Change your method call:

public void Warn(string message, bool logToFile = false)
{
    Log(message, LogType.Warn, logToFile);
}

Create separate mappings. There are a few ways to do this. For simple mappings, you can use dictionaries; but a method with a switch gives you more direct control if you need to apply some custom logic in certain cases.

public Dictionary<LogType, string> FilenameForLogType = new Dictionary<LogType, string>() {
    {LogType.Success , "trace.log"},
    {LogType.Warn ,    "error.log"},
    {LogType.Error ,   "error.log"},
    {LogType.Debug ,   "debug.log"},
};

public Dictionary<LogType, ConsoleColor> ColorForLogType = new Dictionary<LogType, ConsoleColor>() {
    {LogType.Success , ConsoleColor.Green},
    {LogType.Warn ,    ConsoleColor.Yellow},
    {LogType.Error ,   ConsoleColor.Red},
    {LogType.Debug ,   ConsoleColor.Cyan},
};

And then your Log() method can use these dictionaries based on the LogType you've passed in:

private void Log(string message, LogType logType, bool logToFile = false)
{
    // ...

    Console.ForegroundColor = ColorForLogType[logType];

    // ...

    if (logToFile)
    {
        LogToFile(FilenameForLogType[logType],  /* ... */);
    }
}

The main benefit here is that if you ever decide to change any of the colors or file names; then you won't need to actually care about the file names or colors (respectively).

Writing to file

private static void LogToFile(string file, string content)
{
    try
    {
        using (var fileWriter = new StreamWriter(FilePath + file, true))
        {
            fileWriter.WriteLine(content);
            fileWriter.Close();
        }
    }
    catch (DirectoryNotFoundException)
    {
        Directory.CreateDirectory(FilePath);
    }
    catch (FileNotFoundException)
    {
        File.Create(file);
    }
}

There are a few things to remark here.

Handling exceptions.

It doesn't make sense for you to create the directory/file after you've tried to write to it.

Suppose you do this:

for(int i = 0; i < 10; i++)
    myLogger.Trace(i.ToString(), true);

The console will show:

  0
  1
  2
  3
  4
  5
  6
  7
  8
  9

But the file will show:

  1
  2
  3
  4
  5
  6
  7
  8
  9

You've swallowed the first log message (only when writing to disk!), because the file did not exist, you did not create it, and then you didn't try to write it again.

A better approach is to first check if the file/directory exists, create it if it doesn't, and then log the file.

private static void LogToFile(string file, string content)
{
    string fullPath = Path.Combine(FilePath, file);

    if(!Directory.Exists(FilePath))
         Directory.CreateDirectory(FilePath);

    if(!File.Exists(fullPath))
         File.Create(fullPath);

    using (var fileWriter = new StreamWriter(fullPath, true))
    {
        fileWriter.WriteLine(content);
        fileWriter.Close();
    }
}

However, this point may be moot if you read the following suggestion. But it's important to understand that your approach had a bug in it.

Streams COULD be your friend.

If you're going to use a stream to write to file, then you should leverage the benefits of using a stream.

Currently, you are opening a new stream every time you write a message to a log file. Streams make more sense when you repeatedly write data to the same stream, and only close it once you're done with it.

Note that this can be dependent on what you're logging.

A log that catches an exception now and then will not receive much use, and keeping the stream open may be more harmful than beneficial.
A log that catches all the trace messages of a particular processing job is going to receive a high volume of messages in a short time. Streams make sense here.

There are many ways to solve this, without resorting to opening a stream for every message:

A ConsoleLogger opens a stream when the first message is written, but then holds on to that stream until it is manually told to close it, or the logger gets disposed himself.
An intermediary StreamHandler is created. A ConsoleLogger simply requests a stream from StreamHandler, which is able to decide (by itself) when it wants to cache stream and when it should create a new one. This allows you to e.g. not keep an error log stream open, but keep a trace log stream open for an extended time.
You can create your own StreamWriter (or use a StreamHandler) to implement a timeout feature, where streams are automatically closed when they haven't received a message in the last X seconds. This is nice, but not easy to implement.

You can make this as complex as you want it to be. I'll suggest the simplest improvement (the first option).

First, make a cache for your streams:

private Dictionary<string, StreamWriter> _streams = new Dictionary<string, StreamWriter>();

When writing to a file, check if you already have a streamwriter for the file, otherwise create it:

private static void LogToFile(string file, string content)
{
    string fullPath = Path.Combine(FilePath, file);

    //...

    if (!_streams.ContainsKey(file))
    {
        _streams.Add(file, new StreamWriter(fullPath , true));
    }

    _streams[file].WriteLine(content);
}

Don't stop yet. We still need to handle closing the stream! I prefer giving two options to consumers:

Manually close the streams. Should they write another message afterwards, then it will automatically open a new stream.
Automatically close the streams if the logger is disposed of.

Manually closing them can be done as follows:

public void CloseStreams()
{
     //close all streams
     foreach(var kvp in _streams)
         kvp.Value.Close();

     //empty the cache
     _streams.Clear();
}

Automatically closing them can rely on the same method; but it gets triggered by the IDisposable interface:

public class ConsoleLogger <TClass> : ILogger, IDisposable
{
    public void Dispose()
    {
       CloseStreams();
    }
}

Note that this allows you to use the using statement with your logger class:

using(var logger = new ConsoleLogger<Foo>());
{
     logger.Debug("Let's go!");
     MethodWhichAlsoLogsThings(logger);
     logger.Debug("Done! Streams will be closed automatically!");
}

//At this point, the streams are closed.


Reversal of singly linked list in JavaScript I have already implemented reversal of link list in javascript. Can somebody tell optimized way of doing it?I want to do it by less code.

Here I am providing the code:

var tempItem;
var finalLastNode;

function reverseLL(ll) {
  var currentObj = ll,
    prevItem, lastItem, currentTempObj;
  currentTempObj = tempItem;
  if (!currentObj) {
    console.log("No change")
  } else {
    while (currentObj.next) {
      prevItem = currentObj;
      currentObj = currentObj.next;
      lastItem = currentObj;
    }
    if (lastItem) {
      if (!tempItem) {
        tempItem = lastItem;
      } else {
        while (currentTempObj.next) {
          currentTempObj = currentTempObj.next;
        }
        if (tempItem && !tempItem.next) {
          tempItem.next = lastItem;
          finalLastNode = ll;
        } else {
          currentTempObj.next = lastItem;
        }
      }
      prevItem && (prevItem.next = null)
      reverseLL(ll);
    } else {
      while (currentTempObj.next) {
        currentTempObj = currentTempObj.next;
      }
      currentTempObj.next = finalLastNode;
    }
  }
  console.log(tempItem);
}
reverseLL(ll1)

"I want to do it by less code."

It's relatively easy: you keep another variable that denotes the head of the new list. Then, you remove the head node of the input list and make it a new head node of the new list being constructed:

function reverse(head) {
    new_head = head;
    old_head = head.next;
    new_head.next = null; // Terminate cycle.
    while (old_head) {
        current = old_head;
        old_head = old_head.next;
        current.next = new_head;
        new_head = current;
    }
    return new_head;
}

Usage

function node(val) {
    this.val = val;
    this.next = null;
}

a = new node(1);
b = new node(2);
c = new node(3);

a.next = b; b.next = c;
a = reverse(a); // Don't forget to store the new head node.

Hope that helps.


Generating multiple new arrays using numpy Say I have the following two different states:
state1 = [0, 1, 0, 1, 1]
state2 = [1, 1, 0, 0, 1]

And I want to generate n number of new states by only changing the values at where these states differ; and I want to do so randomly. So I did the following:
state1 = np.array(state1)
differ = np.where(state1 != state2)[0]  # --> array([0, 3], dtype=int64)

rand = [random.choices([1, 0], k=len(differ)) for _ in range(4)]
states = [state1.copy() for _ in range(4)]

for i in range(len(states)):
    states[i][differ] = rand[i]  # Will replace the values at locations where they are mismatched only

Out:
[array([1, 1, 0, 0, 1]),
 array([1, 1, 0, 1, 1]),
 array([0, 1, 0, 1, 1]),
 array([0, 1, 0, 1, 1])]

This does the job but I was wondering if there is a better way (to avoid 2 for loops), but this was the best I could come up with. Any ideas on how to do the same thing but better/cleaner?

You could use np.random.choice() to generate the states, while using a==b and a!=b as masks.
def new_states(a,b,n):
    return [
        a*(a==b) + np.random.choice(2,len(a))*(a!=b)
        for _ in range(n)
    ]

state1 = np.array([0, 1, 0, 1, 1])
state2 = np.array([1, 1, 0, 0, 1])
print(new_states(state1,state2,4))


Function to class migration: constructor best practice I am working on moving some complex script logging from functions to classes. In the functional version I would use a string like this

"{header-[cf][0][:12]}_Started: 11:30:21"

{????}_ is the identifying info, here I have a header item header- (which has formatting implications), it will log to both console and file [cf], have no initial indentation [0], and will pad the following string to provide 12 characters before the : that follows Started [:12], so 5 spaces of padding. I had functions to increment the tab value, change the destination (console, file or both), etc. All driven with complicated RegularExpressions to extract the identifying info from the start of the string.
Now, as I move to classes, I have

Enum PxLogType {
    blankLine
    header
    milieuCondition
}

class Px_LogItem {
    [PxLogType]$type
    [bool]$logToConsole
    [bool]$logToFile
    [int]$indent
    [char]$alignToSymbol
    [int]$alignSymbolLocation
    [string]$string

    # Constructor
    Px_LogItem ([PxLogType]$type, [bool]$logToConsole, [bool]$logToFile, [int]$indent, [string]$string) {
        $this.type = $type
        $this.logToConsole = $logToConsole
        $this.logToFile = $logToFile
        $this.indent = $indent
        $this.string = $string
    }
    Px_LogItem ([PxLogType]$type, [bool]$logToConsole, [bool]$logToFile, [int]$indent, [char]$alignToSymbol, [int]$alignSymbolLocation, [string]$string) {
        $this.type = $type
        $this.logToConsole = $logToConsole
        $this.logToFile = $logToFile
        $this.indent = $indent
        $this.alignToSymbol = $alignToSymbol
        $this.alignSymbolLocation = $alignSymbolLocation
        $this.string = $string
    }
}

Ultimately I will have more enumerations, a method to increase the tab indent, etc. This is working, but

$logItem = [Px_LogItem]::New([PxLogType]::header, $true, $true, 0, ':', 12, "Started: 11:30:21")

is not as readable to my eye. Especially the two instances of $true that replace [cf] to define the target for the log. And depending on where I go, I can see having a few more constructor variations, which starts to get messy. At least, messy in my eyes that are as yet not so familiar with the way constructors are so... specific.

So, I wonder if I am on the right track, and just not familiar enough yet with how class based code looks, or am I off track? I imagine I could do another enum for the log target, with valid values of c, f & cf. Or even have a version that doesn't take those arguments, and defaults both to true to simplify things. That would require method chaining for the constructors, which might actually be a good idea, but how many constructors is considered the threshold for that change? I realize this is all pretty non specific and basically just opinion, but I hope specific enough for some folks to weigh in with their opinions, since I'm too ignorant to have a valid opinion yet.

Having a default value is a good idea.
Also, instead of of using the constructor directly, there is a way to convert from a hash table.
This way only works if the class has a default (no-argument) constructor. (If you define a constructor that takes several arguments, you must also explicitly define a no-argument constructor.)

Enum PxLogType { BlankLine; Header; MilieuCondition }
[Flags()] Enum OutputDestination { Console = 1; File = 2 }

class Alignment {
    [char] $Symbol
    [int] $Location

    [string] ToString() { return "{{Symbol='{0}', Location={1}}}" -f $this.Symbol,$this.Location }
}

class PxLogItem {
    [PxLogType] $Type = 'BlankLine'
    [OutputDestination] $Destination = 'Console'
    [int] $Indent = 0
    [Alignment] $Alignment
    [string] $String = ''
}

You can write as follows.

@(
    New-Object PxLogItem -Property @{ Destination = 'File'; String = 'ABC' }
    [PxLogItem]@{ Indent = 12; String = 'DEF' }
    [PxLogItem]@{ Type = 'Header'; Destination = 'Console,File'; Alignment = @{ Symbol = '#'; Location = 10 }; String = 'GHI' }

) | Format-Table -AutoSize

The result:

     Type   Destination Indent Alignment                 String
     ----   ----------- ------ ---------                 ------
BlankLine          File      0                           ABC
BlankLine       Console     12                           DEF
   Header Console, File      0 {Symbol='#', Location=10} GHI

You can also convert strings to objects. This way requires defining a Parse() method.

Enum PxLogType { BlankLine; Header; MilieuCondition }
[Flags()] Enum OutputDestination { Console = 1; File = 2 }

class Alignment {
    [char] $Symbol
    [int] $Location

    [string] ToString() { return "{{Symbol='{0}', Location={1}}}" -f $this.Symbol,$this.Location }
    static [Alignment] Parse($str) { $s,$l = $str.Trim().Split(',', 2); return @{ Symbol = $s; Location = $l } }
}

class PxLogItem {

    [PxLogType] $Type = 'BlankLine'
    [OutputDestination] $Destination = 'Console'
    [int] $Indent = 0
    [Alignment] $Alignment
    [string] $String = ''

    static [PxLogItem] Parse($str) {
        $regex = '^(?:{(?<type>[a-z]+),(?<dest>[cf]+),(?<idt>[0-9]+)(?:,align=(?<align>\S,[0-9]+))?})?(?<text>.*)$'
        if ($str -notmatch $regex) { throw }

        $result = @{ String = $Matches['text'] }
        if ($Matches['type']) {
            $result.Add('Type', $Matches['type'])
            $result.Add('Destination', $Matches['dest'].ToCharArray().ForEach{ "$_" })
            $result.Add('Indent', $Matches['idt'])
        }
        if ($Matches['align']) {
            $result.Add('Alignment', $Matches['align'])
        }
        return $result
    }
}

All of the following are valid conversions.

@(
    [PxLogItem]@{ Destination = 'Console,File'; Indent = 12; Alignment = '=,15'; String = 'ABC' }
    [PxLogItem]'DEF'
    [PxLogItem]'{m,f,8}GHI'
    [PxLogItem]'{header,cf,4,align=#,10}JKL'

) | Format-Table -AutoSize

The result:

           Type   Destination Indent Alignment                 String
           ----   ----------- ------ ---------                 ------
      BlankLine Console, File     12 {Symbol='=', Location=15} ABC
      BlankLine       Console      0                           DEF
MilieuCondition          File      8                           GHI
         Header Console, File      4 {Symbol='#', Location=10} JKL

Using functions makes them more readable.

function New-PxLogItem {

    Param(
        [Parameter(Mandatory, Position=0)]
        [string]
        $String = '',

        [Parameter(Position=1)]
        [PxLogType]
        $Type = 'BlankLine',

        [Parameter(Position=2)]
        [ValidateRange(0,1000)]
        [int]
        $Indent = 0,

        [Parameter(Position=3)]
        [Alignment]
        $Alignment,

        [switch]
        $Console,

        [switch]
        $File
    )

    if (!$Console -and !$File) { $Console = $true }
    $PSBoundParameters.Add('Destination', !!$Console + 2 * !!$File)
    ('Console','File').ForEach{ [void]$PSBoundParameters.Remove($_) }

    [PxLogItem]$PSBoundParameters
}

$item1 = New-PxLogItem "Hello" Header -indent 4 -align "#,10" -c -f


Lazy string_view splitter range Trying something out of my comfort zone here, and attempting to implement a D-style range around std::string_view.

Looking for any and all suggestions or improvements.

One of the things I'm not exactly happy about is having to declare difference_type, pointer and reference just to fullfill the forward_iterator concept properly, yet not being able to actually use them.

I feel that this allowing me to initialize a vector straight from the iterator pair is worth the clunkiness, but I welcome any opinion on the subject, or suggestions on how to make it better.

Thanks!

#include <iterator>
#include <string_view>

template <typename CharT>
struct SplitStringIterator {
  using difference_type = std::ptrdiff_t;
  using value_type = std::basic_string_view<CharT>;
  using pointer = std::basic_string_view<CharT>*;
  using reference = std::basic_string_view<CharT>&;
  using iterator_category = std::forward_iterator_tag;

  SplitStringIterator(value_type str, CharT delim)
      : remainder_(str), delim_(delim) {
    advance_();
  }

  SplitStringIterator& operator++() {
    advance_();
    return *this;
  }

  SplitStringIterator operator++(int) {
    SplitStringIterator other = *this;
    advance_();
    return other;
  }

  value_type operator*() const { return sub_str_; }

  bool operator==(SplitStringIterator const& rhs) const {
    return sub_str_.data() == rhs.sub_str_.data() &&
           remainder_.data() == rhs.remainder_.data() && delim_ == delim_;
  }

  bool operator!=(SplitStringIterator const& rhs) const {
    return !(*this == rhs);
  }

 private:
  value_type sub_str_;
  value_type remainder_;
  const CharT delim_;

  void advance_() {
    auto delim_pos = remainder_.find(delim_);

    if (delim_pos == value_type::npos) {
      sub_str_ = remainder_;
      remainder_ = value_type();
    } else {
      sub_str_ = value_type(&*remainder_.begin(), delim_pos);
      remainder_.remove_prefix(delim_pos + 1);
    }
  }
};

template <typename CharT>
struct SplitString {
  SplitString(std::basic_string_view<CharT> src, CharT d)
      : source(src), delim(d) {}

  std::basic_string_view<CharT> source;
  CharT delim;

  SplitStringIterator<CharT> begin() const {
    return SplitStringIterator<CharT>(source, delim);
  }

  SplitStringIterator<CharT> end() const {
    return SplitStringIterator<CharT>(std::basic_string_view<CharT>(), delim);
  }
};

// Returns a lazy string splitter.
template <typename CharT>
SplitString<CharT> split(std::basic_string_view<CharT> str, CharT delim) {
  return SplitString<CharT>(str, delim);
}

Usage example:

#include <vector>
#include <iostream>

void foo() {
  std::string data = "aa:bb:cccc";
  auto parts = split(std::string_view(data), ':');

  for(auto p : parts) {
    std::cout << p << std::endl;
  }

  //if we need random access:
  std::vector<std::string_view> parts_indexable(parts.begin(), parts.end());
}

Implementation

operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).


Selecting which variables to read from file I'm reading in data from an external file. The data comes in different types, both integers and strings and is shown here:

{
    "height":170,
    "weight":73,
    "country":"UK",
    "city":"Sheffield",
    "year":2017,
    "month":7,
    "day": 2
}

I'd like to decide manually which fields to read without having to recompile the program. Therefore, I do this by listing in a text file which fields I am interested in. I have a file for integer-variables and one for strings:

ints.txt

weight
year
month

strings.txt

country

Now, the data I read I store in an object of class Read_Data (see code below). I keep the data in maps, one for integers and one for strings. I'd like to know where/how can improve on my program and make it more professional. I have myself been thinking if it is possible to merge the integer- and string map such that I don't have to treat them individually as I do currently.

Here is the complete code (and a link to nlohmann's json reader):

main.cpp

#include <iostream>
#include "data.h"

int main()
{
    ///create object to store data
    Read_Data read_data;

    ///read data from file
    read_data.read();

    return 0;
}

data.h

#ifndef SFE_H
#define SFE_H

#include <fstream>
#include "json.hpp"

class Read_Data
{
private:

  ///maps of the read variables
  std::map<std::string, int> int_map;
  std::map<std::string, std::string> string_map;

  ///vectors of which variables to read
  std::vector<std::string> int_list;
  std::vector<std::string> string_list;

public:
  Read_Data();
  int read_list(std::string file_name, std::vector<std::string> &list);
  void read();
};

#endif

data.cpp

#include "data.h"

///which variables we want to read
Read_Data::Read_Data()
{
  read_list("ints.txt", int_list);
  read_list("strings.txt", string_list);
}

int Read_Data::read_list(std::string file_name, std::vector<std::string> &list)
{
  std::string value;

  std::ifstream file_strm(file_name);
  if (file_strm.is_open())
  {
    while (file_strm >> value)
      list.push_back(value);
    file_strm.close();
    return 0;
  }
  else
  {
    std::cout << "Error" << "\n";
    return 1;
  }
}

void Read_Data::read()
{
  ///load data
  std::ifstream strm("data.json");
  nlohmann::json j = nlohmann::json::parse(strm);

  ///read ints from data
  for (auto const &value : int_list)
    int_map[value] = j[value];

  ///read strings from data
  for (auto const &value : string_list)
    string_map[value] = j[value];
}

Interesting: The object reads the data, but the data is not returned anywhere nor is it exposed. Is it by design, I guess not ;)

Peculiar function: read_list() returns an int. I believe the function was in the main() before becoming part of a class, but the return has no effect, since the result is discarded by the caller. No exceptions, just the error printed out. Since the code doesn't perform any error handling anyway, the vectors construction could look much better:

Read_Data::Read_Data():
        int_list(std::istream_iterator<int>(std::ifstream("ints.txt")), {}),
        string_list(std::istream_iterator<std::string>(std::ifstream("strings.txt")), {})
{}

But the std::istream_iterator has constructor that only takes non-const lvalue reference to the stream (not that const version would be useful), which leads me to the next point:

The object is an object, but pretends to be an algorithm: I think that the class should be refactored to be a function which accepts two vectors (with int and string list) and a parsed json. Object has different properties than algorithm, and in current situation clearly algorithm is desired and needed.

Usually I make bare bones interface which has greatest power and doesn't take usability into account. Then I make some higher level functions which delegate to lower level functions with simplicity of use in mind:

auto separate_info(const nlohmann::json& content,
                      const std::vector<std::string>& int_list,
                      const std::vector<std::string>& string_list)
{
    std::map<std::string, int> int_list;
    std::map<std::string, std::string> string_list;

    /*populate them*/

    return std::pair{int_list, string_list};
}

auto read_info() //some defaulted parameters if needed
{
    std::ifstream int_parameter_list{"..."};
    std::ifstream string_parameter_list{"..."};

    template <typename T>
    using isiter = std::istream_iterator<T>;

    std::vector<std::string> int_attributes(isiter<int>{int_parameter_list}, {});
    std::vector<std::string> text_attributes(isiter<std::string>{string_parameter_list}, {});

    //parse the json

    return separate_info(j, int_attributes, text_attributes);
}

Then on the call site:

auto [int_list, string_list] = separate_info(...);

The names could've been better: naming is hard. May be there is something in common between them? What do they refer to? I guess it is something related to a person. The maps could be packed into person_info if my guess was right. It won't be that significant, but maybe pair won't be a good choice if the read data is used in many places.

struct person_info
{
     std::map<...> int_info;
     std::map<...> text_info;
}

And the last, not so important point:

Premature optimization, or ignorance: I don't mean to be offending, but returning vector from a function is probably very cheap. One thing is NRVO, which will probably kick in since it is gonna be created on the first line and returned from one point. If NRVO will fail, move constructor will kick in, so it is gonna be something like copying/swapping of 3 pointers. Although standard doesn't require this, most compilers do it. If you're on mainstream, e.g. gcc/clang/vc++, then returning vector shouldn't be a problem.