C++17 Recursive Fibonacci calculation with memoization Compiler is g++ 4.2. I'm new to C++, but I've done a lot of data science, web scraping, and some socketing stuff in Python. This code generates the nth Fibonacci number, either with a naive implementation or with caching.
#include <iostream>
#include <string>
#include <unordered_map>
#define BIG unsigned long long int

std::unordered_map<int, BIG> umap;

int fib(int n) {
    if (n < 2) {
        return n;
    }
    return fib(n-1) + fib(n-2);
}

BIG fib_with_memo(int n) {
    if (umap.find(n) == umap.end()) { // if F_n not in cache, calculate, cache, and return it
        BIG val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap[n] = val;
        return val;
    }
    return umap[n]; // otherwise return the cached value
}

int main(int argc, char** argv) {
    umap[0] = 0;
    umap[1] = 1;

    int input = std::stoi( std::string(argv[1]) );

    std::cout << fib_with_memo(input) << std::endl;
    return 0;
}

I used a std::unordered_map (hash table) because its lookup/insertion times are \$O(1)\$ (compared to the std::map, which has \$O(log(n))\$ for both.)

Compiler is g++ 4.2.

Yikes. That compiler is ancient. I’ve met competitive programmers younger than that compiler. If you’re planning to do any serious C++ programming, you really should look into upgrading your toolchain, if possible.
#define BIG unsigned long long int

This is absolutely the wrong way to create a new type.
First, there is a trend in C++ to avoid the preprocessor completely. So, no #defines, no #ifs, and so on. It is ALMOST completely unnecessary in the most recent version (C++20), but even in C++11 it is almost completely avoidable.
Rather than a #define, you should use a type alias:
using BIG = unsigned long long int;

But there’s one more problem. By convention, ALL_CAPS variable names are reserved for preprocessor macros… which basically means you should never use them. That means you should do:
using big = unsigned long long int;

Or, even better, choose a more descriptive name, like:
using big_uint = unsigned long long int;

But even better still would be to create your own namespace, and put everything—this type alias and all the Fibonacci functions—in there:
namespace your_ns {

using big_t = unsigned long long int;

int fib(int n) {
    // ... etc. ...

} // your_ns

Now I know your focus is on the memoized version of the Fibonacci algorithm, but the non-memoized one is really problematic:
int fib(int n) {
    if (n < 2) {
        return n;
    }
    return fib(n-1) + fib(n-2);
}

Imagine what happens if you call fib(5). Internally, that results in two calls to fib(4) and fib(3):
fib(5)
  ↓
fib(4) + fib(3)

Except… fib(4) also degrades to fib(3) and fib(2), while fib(3) degrades to fib(2) and fib(1):
fib(5)
  ↓
fib(4)           + fib(3)*
  ↓
fib(3)* + fib(2) + fib(2) + fib(1)

As you can see, fib(3) is called twice. And it gets even worse, because fib(3) degrades to fib(2) and fib(1):
fib(5)
  ↓
fib(4)                              + fib(3)
  ↓
fib(3)           + fib(2)*          + fib(2)*          + fib(1)
  ↓
fib(2)* + fib(1) + fib(1)  + fib(0) + fib(1)  + fib(0) + 1

Now you can see fib(2) is called three times. Each one of those times becomes fib(1) and fib(0).
fib(5)
  ↓
fib(4)                                     + fib(3)
  ↓
fib(3)                   + fib(2)          + fib(2)          + fib(1)
  ↓
fib(2)          + fib(1) + fib(1) + fib(0) + fib(1) + fib(0) + 1
  ↓
fib(1) + fib(0) + 1      + 1      + 1      + 1      + 1      + 1

So the net result of a single call to fib(5) is:

1 call to fib(4)
2 calls to fib(3)
3 calls to fib(2)
5 calls to fib(1)
3 calls to fib(0)

And that’s just fib(5). Imagine how many repeated calls you’ll get with fib(50).
(This is obviously not such a big problem with the memoized version… but it’s still a problem!)
What you really want to look into is the tail-recursive Fibonacci algorithm. I’m not going to work it out for you, but I’ll give you the interface:
auto fib_impl(int n, big_t a, big_t b)
{
    // ...
}

auto fib(int n)
{
    // are you sure you don't want to check for negative numbers?
    if (n < 2)
        return big_t(n); // note: this line won't compile if you used BIG; this is why the preprocessor sucks

    return fib_impl(n, 0, 1);
}

Now you don’t say what version of C++ you’re working with, but if it’s one of the more recent versions, then you can make all of this constexpr. That means that, when possible, the function may be computed at compile time, meaning it will have (effectively) zero run-time cost.
BIG fib_with_memo(int n) {
    if (umap.find(n) == umap.end()) { // if F_n not in cache, calculate, cache, and return it
        BIG val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap[n] = val;
        return val;
    }
    return umap[n]; // otherwise return the cached value
}

Now, since umap has no purpose outside of this function, it makes more sense to hide it within the function as a static variable. If you don’t know what function static variables are, they’re basically global variables, but only visible within the function.
While you’re at it, you might as well replace:
umap[0] = 0;
umap[1] = 1;

with:
umap = std::unordered_map<int, big_t>{{0, 0}, {1, 1}};

Why? Because this will be MUCH more efficient. The first way means that each line first searches the map for the key, then creates the new pair. The second way doesn’t bother with the search, but rather just creates the two pairs directly.
So now your fib_with_memo() could look like this:
auto fib_with_memo(int n) {
    // umap is constructed only once, the first time the function is called,
    // and initialized with {0, 0} and {1, 1}.
    static auto umap = std::unordered_map<int, big_t>{{0, 0}, {1, 1}};

    // note everything else is exactly the same (except I replaced the BIG
    // with auto)

    if (umap.find(n) == umap.end()) { // if F_n not in cache, calculate, cache, and return it
        auto val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap[n] = val;
        return val;
    }
    return umap[n]; // otherwise return the cached value
}

Another small fix is that once you’re in that if block, you know for a fact that n isn’t in the map. So rather than doing umap[n], you can emplace the new pair directly:
    if (umap.find(n) == umap.end()) {
        auto val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap.emplace(n, val);
        return val;
    }

Depending on your implementation, that might be more efficient.
Another possibly major optimization comes from the fact that you use find()… and then just discard the result. Why not use it?
    if (auto p = umap.find(n); p == umap.end())
    {
        // we didn't find n
        auto val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap.emplace(n, val);
        return val;
    }
    else
    {
        // we found n
        return p->second;
    }

So this is about where we’re at right now:
auto fib_with_memo(int n) {
    static auto umap = std::unordered_map<int, big_t>{{0, 0}, {1, 1}};

    if (auto p = umap.find(n); p == umap.end())
        auto val = fib_with_memo(n-1) + fib_with_memo(n-2);
        umap.emplace(n, val);
        return val;
    }
    else
    {
        // we found n
        return p->second;
    }
}

Now we get to the real problem: the same issue as with the non-memoized version, where you’re calling the same function over and over and over. The fix is the same as with the non-memoized version: refactor:
auto fib_with_memo_impl(int n, big_t a, big_t b) {
    static auto umap = std::unordered_map<int, big_t>{{0, 0}, {1, 1}};

    if (auto p = umap.find(n); p == umap.end())
        auto val = // actual algorithm to calculate fib here
        umap.emplace(n, val);
        return val;
    }
    else
    {
        // we found n
        return p->second;
    }
}

auto fib_with_memo(int n) {
    // are you sure you don't want to check for n < 0?

    // trivial solutions, don't even bother diving into the calculation
    if (n < 2)
        return big_t(n);

    return fib_with_memo_impl(n, 0, 1);
}

It’s up to you to implement the actual Fibonacci calculation there (you have a and b to work with). But otherwise, that should be a drop-in replacement for your current design, except orders of magnitude faster.
An alternative design
As you said:

I used a std::unordered_map (hash table) because its lookup/insertion times are \$O(1)\$ (compared to the std::map, which has \$O(log(n))\$ for both.)

Unfortunately, this thinking is misguided.
It turns out that due to the way modern processors work—particularly with caching—that big-O notation can be WILDLY misleading. I recall that the inventor of C++ himself was floored when someone showed him that a std::list, which has \$O(1)\$ insertion in the middle, is slower than a std::vector, which has \$O(n)\$ insertion in the middle, even for huge numbers of elements.
Forget all that theory crap, and instead take a step back and consider your actual problem. Imagine that you’ve called fib_with_memo(10). Okay, now what would you expect to see in the memo-map?
It would be this, right?:
0  → 0
1  → 1
2  → 1
3  → 2
4  → 3
5  → 5
6  → 8
7  → 13
8  → 21
9  → 34
10 → 55

Now look at the keys. It’s just 0 to 10, right? In order, with no holes. Just like an index.
So… why do you need a map? You’re just “mapping” an index to a value. That’s just what a basic array, or a vector, does.
Let’s imagine that we’re using a vector for the memoizing, with the “key” just being the index. That means after memoizing fib_with_memo(10), the vector would contain:
[index 0 ] → 0
[index 1 ] → 1
[index 2 ] → 1
[index 3 ] → 2
[index 4 ] → 3
[index 5 ] → 5
[index 6 ] → 8
[index 7 ] → 13
[index 8 ] → 21
[index 9 ] → 34
[index 10] → 55

So, after fib_with_memo(10), the vector contains 11 elements, and the element at index n—or, vec[n]—is the nth Fibonacci number.
Simple, right?
So how do you tell whether a number is already memoized? Again, simple, just check the vector’s size. If we call fib_with_memo(13), the vector must have at least 14 elements in it for vec[13] to be valid. So:
if (vec.size() > n)
    return vec[n];
else
    // calculate the nth fibonacci number

In fact, the implementation becomes trivial, too. That’s because if the function is called with an unmemoized n, all you need to do is resize the vector to n + 1, and fill in all the numbers between the old size and the new size. Something like:
auto fib_with_memo(int n) {
    // are you sure you don't want to check for n < 0?

    // trivial solutions, don't even bother diving into the calculation
    if (n < 2)
        return big_t(n);

    static auto vec = std::vector<big_t>{0, 1}; // start with fib(0) and fib(1)

    // note that if you really want, you could start with more than just
    // 0 and 1 - after testing your real-world use case, you could determine
    // that you will need, say, up to fib(8), so you could precompute that
    // much and put it in the vector initializer list above
    //
    // if you make the non-memoized fib function constexpr, then you could
    // even be *really* clever, and do something like:
    //      static auto vec = std::vector{fib(0), fib(1), fib(2), fib(3), ...
    // for as many precomputed values as you like.
    //
    // and then you could be *REALLY* clever (if you’re using a more modern
    // version of c++), and do something like:
    //      static auto vec = [](auto initial_size) {
    //          auto v = std::vector<big_t>(std::min(2, initial_size + 1));
    //          v[0] = 0;
    //          v[1] = 1;
    //
    //          for (auto p = std::next(v.begin(), 2); p != v.end(); ++p)
    //              *p = *(p - 1) + *(p - 2);
    //
    //          return v;
    //      }(4); // 4 to calculate fib(0) though fib(4), but you can use any number
    // but that's getting *really* fancy.

    // memoized, so use cached value
    if (vec.size() > n)
        return vec[n];

    // not memoized, so calculate all the values between what's been cached,
    // and what's desired

    auto old_size = vec.size();
    vec.resize(n + 1);

    // now fill in everything from vec[old_size] to vec[n] inclusive,
    // where vec[i] = vec[i - 1] + vec[i - 2]

    return vec[n];
}

Obviously I haven’t profiled this design, but I’d bet it is hundreds, if not thousands of times faster than a similar design using a hash map.
Questions
Difference between a macro and a type alias
When you do:
#define type long double

you’re talking to the preprocessor here… not the compiler.
The preprocessor is stupid (as in, unintelligent), it doesn’t understand C++ (often the same preprocessor is used for C, C++, and sometimes even other languages, like Objective-C, and sometimes Fortran, etc.). It just deals with meaningless tokens. In this case, it just does a blind textual substitution. Everywhere type appears in the code, it just gets literally replaced with long double—literally as if you manually did copy-paste—with no regard as to whether that makes sense.
To see where that causes problems, consider the following code:
struct thing
{
    std::string name;
    std::string type;
};

type value =
    type(0);

Everywhere type appears gets replaced with long double, so…:
struct thing
{
    std::string name;
    std::string long double; // compile error
};

long double value =
    long double(0); // compile error

Note that it didn’t just replace type when it is actually naming a type—which can still cause syntax errors, as it does for the second error above—it also replaced type when it was the name of a variable in the class. That’s why the preprocessor is so dangerous. It’s dumb. It’s indiscriminate. It blindly replaces everything, even when it makes no sense.
Now, when you do:
using type = long double;

you’re talking to the compiler here, not the preprocessor, and that makes all the difference.
This creates a type alias that the compiler understands. The compiler understands C++. The compiler doesn’t just blindly replace text, it knows when type is actually naming a type (and it’s not a variable name, for example), and even knows which type it’s naming (because namespace_1::type may not be the same as namesapce_2::type), and it does the replacement intelligently.
So when the compiler sees the original code:
struct thing
{
    std::string name;
    std::string type;
};

type value =
    type(0);

it will not replace the type in class thing, because that’s not a type, it’s a variable name. And it will replace the type in the last two lines… but not simply by literally replacing the text (which would break the cast on the last line). Instead, it will do the replacement intelligently. value will be a long double, and the last line is casting a 0 (an int) to a long double… everything just works.
In summary, the preprocessor is dumb. It just does blind text replacement, with no regard for what that text actually means… which often results in breaking code. By contrast, the compiler actually understands C++, so it knows when something is actually a type and when it’s not—and even which type it is—so it can do type substitutions intelligently, and not break code.
Refactored code
The standard on CodeReview is not to re-review code in the same question, but rather to post a new review request with the new code. So I can’t re-review your refactored code here. BUT! What I can do is give you suggestions to help you in your refactoring, and in general.
The number one most important thing you can do when coding, not just in this case, but always, is WRITE TESTS. This is the absolute most important skill as a programmer you could ever develop. If I were hiring, and I had two candidates, and one wrote tests and the other didn’t… I wouldn’t even look at the second, I wouldn’t even give them a shot, regardless of how “good” a coder they might be. A coder that doesn’t write tests would be worse than useless to me… yes worse than useless, because they’d actually be a net maintenance burden. I’d literally rather hire nobody than hire someone who doesn’t write tests.
Even if you’re just doing hobby programming, you should get in the habit of writing tests. I swear it will change your life. It makes everything so much easier. And you no longer have to wonder “does my code work?” You’ll just know. And if someone points out a bug you missed… no problem, just add a new test case that catches the bug, then fix it, and now you’re even more sure your code works.
Testing is so important, that I even recommend writing the tests before you write the first line of “real” code.
The first thing you should do is look into a proper testing system. I use Boost.Test but… it’s extremely heavyweight. (I mean, it’s awesome—by far the most powerful testing system—but, yeah, it’s not trivial to set up.) You might like Catch2. GoogleTest is another option but… meh.
The easiest way to set up Catch2 (other than using your system package manager, if you’re on Linux) is to download the two files mentioned at the start of the tutorial—catch_amalgamated.hpp and catch_amalgamated.cpp—and just put them in the same directory as your project. In that directory you should have (at least) fibonacci.hpp, fibonacci.cpp, fibonacci.test.cpp, catch_amalgamated.hpp, and catch_amalgamated.cpp:

fibonacci.hpp: your header
fibonacci.cpp: your implementation
fibonacci.test.cpp: your tests
catch_amalgamated.hpp: the Catch2 header
catch_amalgamated.cpp: the Catch2 code

and when you compile, you need to make sure to also compile the Catch2 code:
# only need to do this one time, which is good, because it will be slow
$ g++ --std=c++17 --pedantic -Wall -Wextra -c catch_amalgamated.cpp

# should do these every time
$ g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.test.cpp
$ g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.cpp
$ g++ --std=c++17 --pedantic -Wall -Wextra -o fibonacci.test fibonacci.test.o fibonacci.o catch_amalgamated.o
$ ./fibonacci.test

Or, better yet, put all those commands in a shell script or makefile. Also, you could use --std=c++11 or --std=c++14 or even --std=c++20, depending on what version of C++ you want to use.
Here’s a simple build script you could use:
#!/bin/sh

# Stop immediately if something fails.
set -e

# Only recompile Catch2 if we have to.
if [ ! -f catch_amalgamated.o ]
then
    printf '%s\n' 'g++ --std=c++17 --pedantic -Wall -Wextra -c catch_amalgamated.cpp'
    g++ --std=c++17 --pedantic -Wall -Wextra -c catch_amalgamated.cpp
fi

# Compile everything else.
printf '%s\n' 'g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.test.cpp'
g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.test.cpp

printf '%s\n' 'g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.cpp'
g++ --std=c++17 --pedantic -Wall -Wextra -c fibonacci.cpp

# Link.
printf '%s\n' 'g++ --std=c++17 --pedantic -Wall -Wextra -o fibonacci.test fibonacci.test.o fibonacci.o catch_amalgamated.o'
g++ --std=c++17 --pedantic -Wall -Wextra -o fibonacci.test fibonacci.test.o fibonacci.o catch_amalgamated.o

Put that in build.sh, make it executable, and you can just do:
$ ./build.sh
$ ./fibonacci.test

That will speed up your workflow, and anything that speeds up your workflow is a good thing. (Even better than a build script would be a proper makefile, but that’s too complicated for here.)
The test file doesn’t need to be complex:
// fibonacci.test.cpp

#define CATCH_CONFIG_MAIN
#include "catch_amalgamated.hpp"

#include <random>

#include "fibonacci.hpp"

// You should always test important edge cases:

TEST_CASE("Fibonacci(0) memoized is 0")
{
    CHECK(your_ns::fib_with_memo(0) == 0);
}

TEST_CASE("Fibonacci(1) memoized is 1")
{
    CHECK(your_ns::fib_with_memo(1) == 1);
}

// Doesn't hurt to test a few random cases:

TEST_CASE("Fibonacci(10) memoized is 55")
{
    CHECK(your_ns::fib_with_memo(10) == 55);
}

TEST_CASE("Fibonacci(20) memoized is 6765")
{
    CHECK(your_ns::fib_with_memo(20) == 6765);
}

// Test the *property* of Fibonacci numbers:

TEST_CASE("Fibonacci(n) memoized is Fibonacci(n - 1) + Fibonacci(n - 2)")
{
    constexpr auto lower_limit = 5;
    constexpr auto upper_limit = 50; // or maybe as high as 90-ish, if you like

    constexpr auto number_of_trials = 3;

    auto rng = std::mt19937{std::random_device{}()};
    auto dist = std::uniform_int_distribution<int>{lower_limit, upper_limit};

    for (auto trial = 1; trial <= number_of_trials; ++trial)
    {
        // random number between lower and upper limit, inclusive
        auto const n = dist(rng);

        DYNAMIC_SECTION("n = " << n)
        {
            auto const fib_n = your_ns::fib_with_memo(n);
            auto const fib_n_minus_1 = your_ns::fib_with_memo(n - 1);
            auto const fib_n_minus_2 = your_ns::fib_with_memo(n - 2);

            INFO("fib(n) = " << fib_n);
            INFO("fib(n - 1) = " << fib_n_minus_1);
            INFO("fib(n - 2) = " << fib_n_minus_2);

            CHECK(fib_n == (fib_n_minus_1 + fib_n_minus_2));
        }
    }
}

Test cases should be as simple as possible. Indeed, the last case is pretty complex for a test case in my opinion, but most of it is just generating random values for n and recording the intermediate results so you can see exactly where things went wrong.
At first this shouldn’t compile, because fib_with_memo() shouldn’t exist yet (assuming you’re writing the tests first, which you normally should). So you just write some placeholders:
// fibonacci.hpp

#ifndef INCLUDE_GUARD_fibonacci
#define INCLUDE_GUARD_fibonacci

namespace your_ns {

using big_t = unsigned long long int;

big_t fib_with_memo(int);

} // namespace your_ns

#endif // include guard

// fibonacci.cpp

#include "fibonacci.hpp"

namespace your_ns {

big_t fib_with_memo(int)
{
    return {};
}

} // namespace your_ns

Now it should compile and run… and generate a bunch of test case failures. But that’s good! A failing test case means that at least something is being checked, and it probably means that a bug was found.
I got 2 cases passed, and 3 failures. The 2 passes are because fib_with_memo(0) returns 0… which by fluke happens to be correct, and because since fib_with_memo(anything) returns 0, that means fib_with_memo(anything) == fib_with_memo(anything - 1) + fib_with_memo(anything - 2) is always true. This happens sometimes as you’re developing; broken code happens to return the “right” answer… or at least an answer that “works”. Other tests should catch that problem, though.
So now you start to develop your Fibonacci algorithm. Let’s say as a first step, you do:
big_t fib_with_memo(int n)
{
    if (n < 2)
        return n;

    return {};
}

Compile and run… now 3 tests pass, and 2 fail. Progress!
This is how you do it. Take tiny steps, and check as you go. It may sound tedious, but once you get in the habit, it becomes automatic.
So the next thing I’ve done is just copy-pasted your refactored code into fib_with_memo() (I also needed to #include <vector>, and I needed to rename big to big_t because I didn’t notice I’d got the name wrong), and…
ALL TESTS PASSED!
Congratulations!  Check it in!
So your refactored code works… but… can it be improved? Well, that’s a question for another code review. Catch2 also supports benchmarking, so you can even do performance checks in your testing code, and as you make changes, you can see if things speed up or not.
But the point is, if you adopt this style of development—testing, testing, testing, testing—you will write better code, quicker, and when it comes time to refactor, you can do so with confidence because if you “break” anything, the tests will catch it immediately. It’s such a nice way of coding that once you get in the habit, you can’t quit; you feel naked and unsteady when you’re trying to code without tests.
Some other tips:

I suggested putting the memo map (or vector) in the function as a static variable, and that’s definitely what you should do for the final code. However, while developing, you could pull it out and make it a global, so you can inspect what’s in the map/vector directly, and make sure that it’s working properly. It’s okay to break stuff while you’re working on it, once you remember to put things back in order when developing/testing is done.

If you do pull the memo map/vector out and make it a global (for development only), then you can observe its capacity to make sure it’s not wasting space. You can also use a tracing allocator to track allocations, and see if you’re losing efficiency there.
A simple tracing allocator is:

template <typename T>
class tracing_allocator
{
public:
    using value_type = T;

    // not needed in C++17
    using propagate_on_container_move_assignment = std::true_type;

    auto allocate(std::size_t n)
    {
        std::cout << "allocating " << n << " bytes\n";
        return std::allocator<T>{}.allocate(n);
    }

    auto deallocate(T* p, std::size_t n)
    {
        std::cout << "deallocating address " << static_cast<void*>(p) << ", which is " << n << " bytes\n";
        return std::allocator<T>{}.deallocate(p, n);
    }
};

template <typename T1, typename T2>
constexpr auto operator==(tracing_allocator<T1> const&, tracing_allocator<T2> const&) noexcept
{
    return true;
}

// not needed in C++20
template <typename T1, typename T2>
constexpr auto operator!=(tracing_allocator<T1> const&, tracing_allocator<T2> const&) noexcept
{
    return false;
}

And you’d use it by declaring your vector as:
static std::vector<big, tracing_allocator<big>> mem = {0, 1};

And now you will get trace statements printed to standard output, for every allocation (and deallocation). This will help you observe how your memo map/vector is growing, and whether there are unnecessary/overlarge allocations happening.


Summing all parameters, except the last, which should be subtracted instead Working on some python koans and got one which states:

def f(a, b, z):

    """Rewrite this function to take an arbitrary number of arguments.

    It should add up all but the last argument and then subtract the
    last argument from the total.

    If no arguments are provided then return 0.
    """
    return a + b - z

My solution:

def f(*args):
    return sum(args[:-1]) - args[-1] if args else 0

This gives 2 out of 3 stars for a solution. I am not asking for help to get 3 stars specifically, but, rather for a review of what I have, and to see if there is perhaps something I am missing, or even if you consider my solution to be good as it is.

Out of interest, I've have also tried:

try:
    return sum(args[:-1]) - args[-1]
except IndexError:
    return 0

but that did not get 3 stars either.

What I am looking for is whether there is or isn't a more 'Pythonic' way of doing this, maybe reduce it even further than what I had there.

If there is only 1 value then the logic should give you the negative of your element.

Using an iterator is commonly considered more Pythonic than indexing, and avoiding an if makes code more beautiful in any language:

def f(*args):
    r = reversed(args)
    return -next(r, 0) + sum(r)


Getting JSON and putting it in a table using jQuery and PHP How can I improve this code?

jQuery:

$.ajax({
   url: "get_attributes.php",
   type: "post",
   datatype: "json",
   data: {
       wtype: red_type
   },
   success: function (data) {
       var toAppend = '';
       if (typeof data === "object") {
           toAppend += "
";
           toAppend += "
	Type:	" + data['type'] + "
";
           toAppend += "
Health:	" + data['health'] + "
";
           toAppend += "
Attack:	" + data['attack'] + "
";
           toAppend += "
Defense:	" + data['defense'] + "
";
           toAppend += "
Speed:	" + data['speed'] + "
";
           toAppend += "
Evade:	" + data['evade'] + "
";
           toAppend += "
Special:	" + data['special'] + "
";
           toAppend += "
";
           $("tbody").remove();
           $("#red_form table").append(toAppend);
       }
   }
   });

PHP:

    <?php
header("Content-Type: application/json");

$wtype = $_POST['wtype'];

$attributes = array();

if($wtype == 'Ninja'){
    $attributes['type'] = 'Ninja';
    $attributes['health'] = '40-60';
    $attributes['attack'] = '60-70';
    $attributes['defense'] = '20-30';
    $attributes['speed'] = '90-100';
    $attributes['evade'] = '0.3-0.5';
    $attributes['special'] = 'There is a 5% chance of 2x attack';
}
else if($wtype == 'Samurai'){
    $attributes['type'] = 'Samurai';
    $attributes['health'] = '60-100';
    $attributes['attack'] = '75-80';
    $attributes['defense'] = '35-40';
    $attributes['speed'] = '60-80';
    $attributes['evade'] = '0.3-0.4';
    $attributes['special'] = '10% chance of restoring +10 health when evade is successful';
}
else if($wtype == 'Brawler'){
    $attributes['type'] = 'Brawler';
    $attributes['health'] = '90-100';
    $attributes['attack'] = '65-75';
    $attributes['defense'] = '40-50';
    $attributes['speed'] = '40-65';
    $attributes['evade'] = '0.3-0.35';
    $attributes['special'] = 'increased +10 defense when health is below 20%';
}

echo json_encode($attributes);
?>

There are a few improvements that come to my mind for the given code:

you may want to return an status code for your JSON output instead of checking whether the result is an object. It would allow you to handle error messages and such in a more convenient manner.
as a minor improvement, you can use replaceWith() to replace the table body instead of removing it and then appending a new one to the table.
you don't have an error handler to your script... if you won't ever need it, that's cool, but it's always best to know when a script fails :)
to improve concatenation performance, you might want to use Array's join() method instead of doing += for each table row.
you're now removing all table bodies on the whole page, which could be kind of risky if you have more than one table there. Furthermore, the selector is extensively greedy, as jQuery has to find all table bodies on the whole page. It's better to use ID selectors when possible, as they are the fastest one to execute.
PHP-wise, you don't handle invalid $wtype, so if no condition is met, you won't know what went wrong, since nothing relevant will be returned

Enough of the theory, this is what I have in mind:

$.ajax({
                url: "get_attributes.php",
                type: "post",
                datatype: "json",
                data: {wtype: red_type},
                success: function(data) {
                    if (data["status"] == 200) {
                        var toAppend = [];
                        toAppend.push(
                            "
",
                            "
	Type:	"+data['type']+"
",
                            "
Health:	"+data['health']+"
",
                            "
Attack:	"+data['attack']+"
",
                            "
Defense:	"+data['defense']+"
",
                            "
Speed:	"+data['speed']+"
",
                            "
Evade:	"+data['evade']+"
",
                            "
Special:	"+data['special']+"
",
                            "
"
                        );
                        $("#red_form table tbody").replaceWith(toAppend.join(""));
                    } else {
                        alert('An error has occured. Please try again.');
                        console.log('Status: ' + data["status"] + ', message: ' + data["message"]);
                    }
                },
                error: function(jqXHR, textStatus, errorThrown) {
                    alert('An error has occured. Please try again.');
                    console.log('ErrStatus: ' + textStatus + ', error: ' + errorThrown);
                }
            });

And the PHP:

<?php
header("Content-Type: application/json");
$wtype = (isset($_POST['wtype']) ? $_POST['wtype'] : 'unknown');
$attributes = array('status' => 200, 'message' => 'ok', 'type' => $wtype);

switch ($wtype) {
    case 'Ninja':
        $attributes['health'] = '40-60';
        $attributes['attack'] = '60-70';
        $attributes['defense'] = '20-30';
        $attributes['speed'] = '90-100';
        $attributes['evade'] = '0.3-0.5';
        $attributes['special'] = 'There is a 5% chance of 2x attack';
        break;

    case 'Samurai':
        $attributes['health'] = '60-100';
        $attributes['attack'] = '75-80';
        $attributes['defense'] = '35-40';
        $attributes['speed'] = '60-80';
        $attributes['evade'] = '0.3-0.4';
        $attributes['special'] = '10% chance of restoring +10 health when evade is successful';
        break;

    case 'Brawler':
        $attributes['health'] = '90-100';
        $attributes['attack'] = '65-75';
        $attributes['defense'] = '40-50';
        $attributes['speed'] = '40-65';
        $attributes['evade'] = '0.3-0.35';
        $attributes['special'] = 'increased +10 defense when health is below 20%';
        break;

    default: $attributes['status'] = 400;
             $attributes['message'] = 'Invalid class info has been requested.';
}

echo json_encode($attributes);
?>


PDO insert from check boxes I've seen so many different ways of doing this all seem very complex and all seem to be done in MySQL. I'm using PDO in my project so I've had a go myself to see what I could come up with.

Is this safe?
Is there something I've missed a better way of doing it?

    //build query to insert users_contracts
$sth = "INSERT INTO `users_contracts` (users_id,contracts_id) VALUES (:user,:contract)";
$q = $conn->prepare($sth);

//take each item in the contract array from _POST and bind to the above INSERT query
foreach($contract as $contract => $contract_id)
{
    try {
       $q->execute(array(':user'=>$last_id,':contract'=>$contract_id));
    } catch (PDOException $e) {
       $errors[] = array(
          'message' => "Could not insert \"$contract\", \"$contract_id\".",
          'error' => $e
          );
    }
}

Its intended purpose is to allow the admin user to assign contracts to users via a checkbox list.

There are a couple of minor things I'd suggest you look into:

Names matter: you're assigning the query string to a variable called $sth, and assinging the prepared statement to a variable called $q. To me, that looks wrong, and it should be the other way around. A variable called $q or $query should be a query string, and a variable called $sth or $stmt should be a prepared statement.
Transactions: if, somewhere down the line, one of the INSERT queries failed, I wouldn't carry on. To me, that smells of bad/invalid/malicious data, and I'd want to stop inserting and even undo whatever inserts that were already executed. By starting a transaction before the loop, and committing it at the end, you can do this easily
Because you are re-using a statement several times (which is good), you might want to add a closeCursor call. The MySQL drivers don't really care about this, but some other drivers do. Depending on what DB you're using, your code might behave differently if you leave it out.

Basically, using bindParam as Pinoniq suggested is a good idea. However, make sure $last_id stays valid. It's not being altered in your loop, so make sure the code between bindParam and the execute call doesn't do anything untoward with that variable.
All things aside, your code could be made to look something like this:

$query = 'INSERT INTO users_contracts (users_id,contracts_id)
    VALUES (:user,:contract)';
try
{//begin try-block here
    $conn->beginTransaction();//start the transaction
    $stmt = $conn->prepare($query);
    $stmt->bindParam('user', $lastId);
    $stmt->bindParam('contract', $contractId);
    foreach($contract as $contract => $contractId)
    {
        $stmt->execute();
        $stmt->closeCursor();//<-- doesn't hurt
    }
    $conn->commit();//everything went smoothly, save inserts
}
catch (PDOException $e)
{
    $conn->rollBack();//undo changes!
    //handle error
    printf(
        '%d - %s insert failed: (%d) %s',
        $lastId,
        $contractId,
        $e->getCode(),
        $e->getMessage()
    );
}


Checking if a number fits in a primitive type I made a program that asks for a specified amount of numbers and checks if the input number is in the primitive type range. If it is, it "fits" on the primitive type.

Each input number, it checks if the number is in the range of each primitive type MIN and MAX range. If it is between the range, it prints the type(s) it is ranged on.

import java.math.BigInteger;
import java.util.Scanner;

public class Main {

private Scanner in = new Scanner(System.in);

public Main()
{
    // Get input on how many numbers to ask for
    int t = in.nextInt();
    String input;
    for(int times = 0; times < t; times++) {
        // Ask for the number
        input = in.next();
        try {
            BigInteger number = new BigInteger(input);
            System.out.println(input + " can be fitted in:");
            if(number.longValue() < Byte.MAX_VALUE && number.longValue() > Byte.MIN_VALUE)
            {
                System.out.println("* byte");
            }
            if(number.longValue() < Short.MAX_VALUE && number.longValue() > Short.MIN_VALUE)
            {
                System.out.println("* short");
            }
            if(number.longValue() < Integer.MAX_VALUE && number.longValue() > Integer.MIN_VALUE)
            {
                System.out.println("* int");
            }
            // If the longValueExact() returns a greater number than the long max value, then it throws an ArithmeticException
            if(number.longValueExact() < Long.MAX_VALUE && number.longValue() > Long.MIN_VALUE)
            {
                System.out.println("* long");
            }
        }
        catch(NumberFormatException | ArithmeticException e)
        {
            System.out.println(input + " * nothing");
        }
    }
}

public static void main(String[] args)
{
    new Main();
}
}

I got this "challenge" from this challenge website. It may explain the challenge better.

String input;
for(int times = 0; times < t; times++) {
    // Ask for the number
    input = in.next();

There's no need to declare input outside of the loop. It has no advantage at all as you don't need to keep it between iterations. Always minimize the scope.

       // If the longValueExact() returns a greater number than the long max value, then it throws an ArithmeticException

As pointed in a comment, I misread longValueExact as longValue. Updated the answer, so that both are covered.

For longValue, this isn't true and the Javadoc says it pretty clearly:

  if this BigInteger is too big to fit in a long, only the low-order 64 bits are returned

For longValueExact, the following is said

  If the value of this BigInteger is out of the range of the long type, then an ArithmeticException is thrown.

So checking for exception is correct. If it gets thrown, the number doesn't fit in long and obviously in no smaller type either.

        if(number.longValueExact() < Long.MAX_VALUE && number.longValue() > Long.MIN_VALUE)
        {
            System.out.println("* long");
        }

By definition, there's no long bigger than the biggest long, is it?

So this test is a tautology.

Using longValueExact makes the program correct. With longValue, it wouldn't work and instead of testing if a number fits in a type, you test if its lowest 64 bits do.

Still, letting an exception be thrown for a normal program flow is extremely inefficient as the JIT assumes that exceptions are well... exceptional. Moreover, filling their stacktrace is pretty complicated and slow, as they actually don't exists in an optimized code.

In order to get a faster code, you need to do it the other way round: Convert to BigInteger and then compare. As a bonus to speed, you get compatibility with Java 7.

You should also fix your spacing and braces. Let your IDE do it, it's free.


Spritesheet cut to individual frames I need a hoverable transparent animation for a website, so that it glows when the mouse is over it. I could use two gifs or svgs one on top of another, but (as far as I know) there's no control of the current frame so I tried spritesheets with JavaScript and an if/else inside of every image in order to show the hovered, glowing version or the non-glowing depending on the users's mouse position.

However, there is yet another thing I tried: instead of a single spritesheet, why not cut it and put it all in the same position within a container div? They all start in display:none and they appear at the rate JavaScript tells them to. It works quite nicely. Here is my HTML:

There are many more frames, of course. This is the JavaScript controller (for two frames):

var animation_hovered = false;

animation.onmouseover = function(){animation_hovered = true;}
animation.onmouseout = function(){animation_hovered = false;}

window.setInterval(function(){
  if(animation_hovered == false){
  document.querySelector('#animation img:nth-child(1)').style.display = "initial";
  document.querySelector('#animation img:nth-child(4)').style.display = "none";
  document.querySelector('#animation img:nth-child(3)').style.display = "none";
  }
else{
  document.querySelector('#animation img:nth-child(2)').style.display = "initial";
  document.querySelector('#animation img:nth-child(4)').style.display = "none";
  document.querySelector('#animation img:nth-child(3)').style.display = "none";
  }
window.setTimeout(function(){
  if(animation_hovered == false){
    document.querySelector('#animation img:nth-child(3)').style.display = "initial";
    document.querySelector('#animation img:nth-child(1)').style.display = "none";
    document.querySelector('#animation img:nth-child(2)').style.display = "none";
    }
  else{
    document.querySelector('#animation img:nth-child(4)').style.display = "initial";
    document.querySelector('#animation img:nth-child(1)').style.display = "none";
    document.querySelector('#animation img:nth-child(2)').style.display = "none";
   }
  }, 300);
}, 600);

It seems bulky, but this approach gives you absolute control over your frames and it works nicely. Also, by not using a spritesheet, you can keep your layout pretty simple (or at least a big spritesheet seems to me a bit difficult to hide in my web interface; cut it seems simpler).

Does anyone think I am doing something wrong or could be enhanced in any way?

First of all, this kind of manipulation is done using CSS. E.g. http://blog.teamtreehouse.com/css-sprite-sheet-animations-steps

Nevertheless, let's see what can be improved:

Instead of nth-child selectors, use classes instead

It provides some meaning to the images, and you can replace the src easily afterwards:

Refactor the code into a general function that handles the styling

var style_switcher = function(first, second, third) {
    document.querySelector(first).style.display = "initial";
    document.querySelector(second).style.display = "none";
    document.querySelector(third).style.display = "none";
};

Use named constants

var TIMEOUT = 300;
var INTERVAL = 600;

This will make things easier when you want to change the values.

Put everything together

window.setInterval(function(){
    if(animation_hovered == false){
        style_switcher('.first-frame', '.second-frame-glow', '.second-frame');
    }else{
        style_switcher('.first-frame-glow', '.second-frame-glow', '.second-frame');
    }
    window.setTimeout(function(){
        if(animation_hovered == false){
            style_switcher('.second-frame', '.first-frame', '.first-frame-glow');
        }else{
            style_switcher('.second-frame-glow', '.first-frame', '.first-frame-glow');
        }
    }, TIMEOUT);
}, INTERVAL);

If there is more common behavior, you can abstract that into a function.

Good luck!


Showing events on a multi room scheduler I write a web app that shows events on a multi room scheduler. I use VueJS for the first time on a real project.

Here, I loop through the events array every room column (two nested v-for). In jQuery I need to loop once through this array to fill all columns at once.

My question: is there any other more efficient way to write this code that fills rooms divs with events in Vue?

var vm = new Vue({
        el: '#app',
        data: {
            rooms: [
               {id: 1, title: 'Room 1' },
               {id: 2, title: 'Room 2' }
            ],
            events: [
              { id: 1, start: 230, duration: 30, title: 'Event 1', room_id: 2},
              { id: 2, start: 400, duration: 45, title: 'Event 2', room_id: 1}
            ]
        }
    })
.agenda-wrapper { max-width: 100%; overflow-x: scroll; }

  .agenda { padding-top: 50px; overflow: hidden; }

.room {
    width:300px;
    height:1170px;
    padding: 0 10px;
    margin-left: 5px;
    background:#ECECEC;
    position:relative;
    float: left;
}

.room span {
    position: absolute;
    top: -20px;
    height: 10px;
    text-align: center;
    width: 300px;
    font-weight: bold;
}

.event {
    display: block;
    background:#fff;
    color:#4B6EA8;
    border:1px solid #ccc;
    border-left:4px solid #c4183c;
    position:absolute;
    padding:0 10px;
    font-weight:bold;
    font-size:13px;
    border-radius: 5px;
    width: 280px;
}

      {{ room.title }}

{{ event.title }}

I asked you how you would do it in jQuery and you said that you would do something like:

$.each(events, function(event){
    var el = $("

")
        .addClass("event")
        .css({ "height": event.duration + "px", "top": event.start + "px" })
        .text(event.title);
    $('.room').data('place', event.place_id).append(el);
});

And sure, this would only loop through the events array once. But what happens in every iteration? It does a jQuery search for $('.room').data('place', event.place_id), which is not a constant-time lookup operation, so this approach might be slower than what you think it is.

Now, for your Vue approach. You said:

  maybe I need to do it on backend to make a rooms array look like that: { id: 1, title: 'Room 1', events: [{ id: 1, title: 'Event 1', start: 90, duration: 30 }] }

And yes, I agree with this. Putting the events inside your rooms makes much more sense.

As for your Vue template, overall the code looks fine. I'm not a CSS expert so I don't know if there's something you can improve there, but there probably is. Maybe consider using CSS Grids, where each room could be one column, and time-slots could be rows? The current approach of having 1px = 1 minute seems a bit unnatural to me.

As for this part:

  {{ room.title }}

{{ event.title }}

The only thing I would change is to put v-for and v-if before your :style-binding, as they are more important to be aware about, this is mostly my personal opinion though.

  {{ room.title }}

{{ event.title }}

It would be nicer if your events would be inside the rooms, as then you could write:

  {{ room.title }}

{{ event.title }}

I would recommend in the future to use a Room component and a RoomEvent component so that you could write:

<Room v-for="room in rooms" :key="room.id" />

And the room-component:

  {{ room.title }}
  <RoomEvent v-for="event in room.events" />

I think that the
{{ event.title }}
 belongs inside the RoomEvent

Overall a nice job!


Clean code for array comparison Following snippet reads CSV Line count using BinaryReader.

Currently it checks \r and \n for line delimiters.

        private static int GetLineCount(string fileName)
        {
            BinaryReader reader = new BinaryReader(File.OpenRead(fileName));
            int lineCount = 0;

            char lastChar = reader.ReadChar();
            char newChar = new char();

            do
            {
                newChar = reader.ReadChar();
                if (lastChar == '\r' && newChar == '\n')
                {
                    lineCount++;
                }
                lastChar = newChar;
            } while (reader.PeekChar() != -1);
            return lineCount;
        }

I want to use Environment.NewLine string and make it work on windows\unix.

I want to refactor above to find word occurance and then match for the word Environment.NewLine. The issue is that I am not able to refactor following for word (more specifically change lastChar , newChar into Array.

                do
                {
                    newChar = reader.ReadChar();
                    if (lastChar == '\r' && newChar == '\n')
                    {
                        lineCount++;
                    }
                    lastChar = newChar;
                } while (reader.PeekChar() != -1);

Environment.NewLine always returns \r\n so it won't help you in parsing different line endings.

If your task is to count the number of lines then it would be much easier just to do smth. like:

    private static int GetLineCount(string fileName)
    {
        return File.ReadLines(fileName).Count();
    }

ReadLines method automatically parses different line endings.


Calculate function with optional operator argument I am currently learning javascript at university. We have an assignment where we shall write various functions for calculating values. In one case, we shall write a function which returns the sum of two values, if only those two values were passed as arguments. If there is the third optional argument, which shall be an operation like subtract or multiply, than it should return the result of this operation.

Now, my question is: Is this code written in good way or is it like really bad style?

Btw: One assignment before, we should write the add, subtract and multiply functions in inside of this calculate function. So this was set.

 let rechne2b = function(a, b, ...operator){

   let addiere = function(a, b){
     return a + b;
   }

   let subtrahiere = function(a, b){
     return a - b;
   }

   let multipliziere = function(a, b){
     return a * b;
   }

   let argsLength = arguments.length

   if(argsLength > 2){
     switch(arguments[2].operation){

      case "subtrahiere":
       return subtrahiere(a, b);
       break;

     case "multipliziere":
      return multipliziere(a, b);
      break;
     }
   } else {
     return a + b;
   }

}

console.log(rechne2b(4, 5, {operation: "multipliziere"}))

Use a function declaration function rechne2b(a, b, {operator}){ rather than a function expression let rechne2b = function(a, b, {operator}){
There is only a single optional argument so don't use the rest operator ...
The optional argument is passed in an object so you should use destructuring assignment  function rechne2b(a, b, {operator}) to extract the operator if given.
Avoid switch statements if you can by using a lookup (see example)

Rewrite
function rechne2b(a, b, {operator}) {
    const ops = {
        subtrahiere(a, b) { return a - b },
        multipliziere(a, b) { return a * b },
    };
    return ops[operator] ? ops[operator](a, b) : a + b;
}


Find the next prime number — flow control of nested `for` loops This code works perfectly, but it bothers me.  Having a labeled, nested for loop, with a true condition, a break statement, and a continue label...it really bothers me.  But I couldn't figure out any other way to arrange the flow control here without sacrificing the efficiency.  Any ideas?

private static ArrayList<Integer> primeList = new ArrayList<Integer>(Arrays.asList(2,3));

public static int getNextPrime() {
    int testNum;
    search:
        for (testNum = (primeList.get(primeList.size() - 1) + 2); true; testNum += 2) {
            int prime = 0;
            for (int index = 1; prime * prime <= testNum; index++) {
                prime = primeList.get(index);
                if (testNum % prime == 0) continue search;
            }
            break;
        }
    primeList.add(testNum);
    return testNum;
}

From the names I am using, this should be fairly self-documenting, but to say it in English: This is part of a public class Primes that I wrote, specifically the part that actually does the finding of primes.  My various other methods (fillListToIndex, fillListToValue, findPrimeN—not shown) all ultimately rely on getNextPrime() for the actual primality testing.

All it does is find the next prime that has not already been found, memoize it, and return it.

Factor out the inner loop into a method (not just for the sake of structure, but because it is a method bool is_next_prime(int testNum)):

    for (...) {
        if (testNum % prime) == 0)
            return false;
    }
    return true;

Then

    int getNextPrime() {
        int testNum = primeList.get(primeList.size() - 1) + 2;
        while (!is_next_prime(testNum)) {
            testNum += 2;
        }
        return testNum;
    }


Error-handling #ifdefs for AFNetworking requests I am using AFNetworking 1.4.3 to send and receive network messages in iOS. My application works slightly differently in DEBUG and RELEASE mode, so I need to use #ifdef clauses. How can I simplify the following fragments without making these macros:

#ifdef DEBUG
        if (failure) failure([NSError fromDict:responseObject]);
#else
        if (failure) failure([NSError networkError]);
#endif

#ifdef DEBUG
        if (failure) failure(error);
#else
        if (failure) failure([NSError networkError]);
#endif

Whole code:

@implementation ELHTTPClient

- (instancetype)initWithBaseURL:(NSURL *)url {
    self = [super initWithBaseURL:url];
    if (self) {
        __weak ELHTTPClient *weakSelf = self;
        [self setParameterEncoding:AFJSONParameterEncoding];
        [self registerHTTPOperationClass:[AFJSONRequestOperation class]];
        [self setDefaultHeader:@"Accept" value:@"application/json"];
        [self setReachabilityStatusChangeBlock:^(AFNetworkReachabilityStatus status) {
            [weakSelf.delegate httpClient:weakSelf connectionStateChanged:status];
        }];
    }
    return self;
}

- (void)myPostPath:(NSString *)path parameters:(NSDictionary *)parameters success:(void (^)(NSDictionary *))success failure:(ELErrorBlock)failure {
    [self postPath:path parameters:parameters success:^(AFHTTPRequestOperation *operation, id responseObject) {
        if ([responseObject isResponseValid]) {
            if (success) success(responseObject);
        }
        else {
#ifdef DEBUG
            if (failure) failure([NSError fromDict:responseObject]);
#else
            if (failure) failure([NSError networkError]);
#endif
        }
    } failure:^(AFHTTPRequestOperation *operation, NSError *error) {
//        [self.delegate httpClient:self networkError:error];
//        NSLog(@"Network error. Operation: %@, \n Error: %@", operation, error);
#ifdef DEBUG
        if (failure) failure(error);
#else
        if (failure) failure([NSError networkError]);
#endif
    }];
}

- (void)myGetPath:(NSString *)path parameters:(NSDictionary *)parameters
          success:(void (^)(NSDictionary *dict))success
          failure:(ELErrorBlock)failure {
    [self getPath:path parameters:parameters success:^(AFHTTPRequestOperation *operation, id responseObject) {
        if ([responseObject isResponseValid]) {
            if (success) success(responseObject);
        }
        else {
#ifdef DEBUG
            if (failure) failure([NSError fromDict:responseObject]);
#else
            if (failure) failure([NSError networkError]);
#endif
//            NSError *error = [self.serializer errorFromDictionary:responseObject];
//            [self.delegate httpClient:self requestFailedError:error];
        }
    }failure:^(AFHTTPRequestOperation *operation, NSError *error) {
#ifdef DEBUG
        if (failure) failure(error);
#else
        if (failure) failure([NSError networkError]);
#endif
    }];
}

@end

My implementation of NSError category:

@implementation NSError (My)

+ (NSError *)networkError {
    NSMutableDictionary *userInfo = [[NSMutableDictionary alloc] init];
    [userInfo setValue:@"Some problems with connection to network. Please try again later." forKey:NSLocalizedDescriptionKey];//FIXME: add localized string.
    return [NSError errorWithDomain:@"Connection error" code:1024 userInfo:userInfo];
}

+ (NSError *)fromDict:(NSDictionary *)dict {
    NSString *errorDesc = dict[@"description"];
    errorDesc = errorDesc ? : [NSString stringWithFormat:@"Dictionary: \n%@", dict];
    NSMutableDictionary* details = [NSMutableDictionary dictionary];
    details[NSLocalizedDescriptionKey] = errorDesc;
    NSNumber *errorCodeNum = dict[@"errorCode"];
    return [NSError errorWithDomain:@"world" code:[errorCodeNum intValue] userInfo:details];
}
@end

I would put ifdef DEBUG inside the failure method, so it's handled in one place only.

(void) failure(id response, NSError error) {

#ifdef DEBUG

#else

#endif

}


Iterating X times (where X > Y) over Y element dictionary I need to write a procedure, that will take a dictionary consisting of server names and their available slots and a number of slots required, and redispatch it over the servers, so that every server would accept only that many assignments as he has free slots; servers with higher number of free slots would be assigned in first place; whole procedure would be robust and not prone to any stupid math errors I might have done. So far I've came up with something like that:

import operator

slots_required = 8
hosts = {'server1': 3, 'server2': 7, 'server3': 1}
jobs = {}

if slots_required > sum(hosts.values()):
    print " [!] Not enough resources! Exiting."
else:
    print " [x] entering while loop"
    while slots_required > 0:
        print " [x] while loop on interation: %s" % slots_required
        for host in sorted(hosts.iteritems(), key=operator.itemgetter(1), reverse=True):
            # if for any reason slots_required number is 0 or less, break
            if slots_required <= 0:
                break
            # if the host has no slots free we're not iterating over it and removing it
            if host[1] == 0:
                hosts.pop(host[0])
                break
            else:
                print " [x] for loop, host: %s" % host[0]
                jobs[slots_required] = host
                slots_required -= 1
                hosts[host[0]] = host[1] - 1
                print " [x] slots_required value after decreasing: %s" % slots_required

        print " [x] exited the for loop"
    print " [x] Iteration down to ZERO! Exiting!"
    print " [x] exiting while loop"

print jobs
print hosts
print slots_required

Feel free to play around and change server numbers, their free slots and slots required of course. The numbers I've added are only for example and testing purposes, and the print functions can be taken out as well, the only thing I am interested in at the end is the jobs dictionary with server names and their assignments (there will be jobID inserted next to server name in jobs dictionary instead of a number, as it is right now). Is there a better, nicer, safer and quicker way of doing that? I've feeling the code although working in my tests, is extremely ugly and could be more functional.

You can solve this mathematically. Let's suppose there are \$m\$ hosts and \$n\$ jobs. Sort the hosts into ascending order by their slots, so that \$s_i\$ is the number of slots for host number \$i\$, and we have \$s_0 ≤ s_1 ≤ \dotsb ≤ s_{m−1}\$.

Now we can immediately figure out how many jobs host 0 gets. Case (i): if \$s_0m ≥ n\$, then we can distribute jobs evenly across the hosts, so that every host gets either \$\lceil{n \over m}\rceil\$ or \$\lfloor{n \over m}\rfloor\$ jobs. Because jobs are distributed to the hosts with higher capacity first, we can be sure that host 0 gets \$\lfloor{n \over m}\rfloor\$ jobs. Case (ii): if \$s_0m < n\$, then we have to give each host at least \$s_0\$ jobs (and some hosts more).

So host 0 gets \$\min(\lfloor{n \over m}\rfloor, s_0)\$ jobs, and then we can subtract 1 from \$m\$ and go on to host 1. Like this:

import operator

class CapacityError(Exception):
    pass

def schedule(hosts, n):
    """
    Generate a schedule for picking `n` slots from `hosts` (a
    dictionary mapping hostname to capacity). Raise CapacityError if
    no such schedule is possible.

    >>> sorted(schedule({'A': 3, 'B': 7, 'C': 1}, 8))
    [('A', 3), ('B', 4), ('C', 1)]
    """
    if n > sum(hosts.itervalues()):
        raise CapacityError()
    m = len(hosts)
    for host, slots in sorted(hosts.iteritems(), key=operator.itemgetter(1)):
        j = min(n // m, slots)
        n -= j
        m -= 1
        yield host, j
    assert(n == m == 0)

Update 1: generators

I've implemented this function in the form of a generator because that's the most flexible way in Python to produce a sequence of things (here, elements of a schedule). You can easily turn the result into a dictionary:

>>> dict(schedule({'A': 3, 'B': 7, 'C': 1}, 8))
{'A': 3, 'C': 1, 'B': 4}

or a list:

>>> list(schedule({'A': 3, 'B': 7, 'C': 1}, 8))
[('C', 1), ('A', 3), ('B', 4)]

or just process the items one at a time:

>>> for host, slots in schedule({'A': 3, 'B': 7, 'C': 1}, 8):
...     print("{}: {}".format(host, slots))
...
C: 1
A: 3
B: 4

(If you're not happy with using generators, you can easily change the implementation to return a dictionary instead. But it's worth learning to use them. See the Python Tutorial, or this question on Stack Overflow.)

Update 2: job ids

You asked in comments how to assign individual jobs. Well, one idea would be to pass in a list of jobs to the scheduler, and get out a list of jobs for each host, like this:

from itertools import islice

def schedule2(hosts, jobs):
    """
    Generate a schedule for assigning the list `jobs` to `hosts` (a
    dictionary mapping hostname to capacity). Raise CapacityError if
    no such schedule is possible.

    >>> jobs = ['j{}'.format(i) for i in range(8)]
    >>> dict(schedule2({'A': 3, 'B': 7, 'C': 1}, jobs))
    {'A': ['j1', 'j2', 'j3'], 'C': ['j0'], 'B': ['j4', 'j5', 'j6', 'j7']}
    """
    n = len(jobs)
    if n > sum(hosts.itervalues()):
        raise CapacityError()
    m = len(hosts)
    jobs = iter(jobs)
    for host, slots in sorted(hosts.iteritems(), key=operator.itemgetter(1)):
        j = min(n // m, slots)
        n -= j
        m -= 1
        yield host, list(islice(jobs, j))
    assert(n == m == 0)

Update 3: more explanation

You asked in comments for more explanation. Well, I'm going to assume that you know all about Python iterables, iterators, and generators (see the Python tutorial and the answers to this question on Stack Overflow).

So the remaining difficult bit is why I write jobs = iter(jobs) and list(islice(jobs, j)). Remember that jobs is a sequence of jobs, and we want to split it up into lists of jobs assigned to each host. In the line

        j = min(n // m, slots)

we compute the number of jobs to assign to the current host, so what we need to do is to take the next j elements from the list jobs. One way to do that would be able to keep a counter k of how far we got through the list jobs, and then take a slice of the next j elements starting at k. Like this:

    k = 0
    for host, slots in sorted(hosts.iteritems(), key=operator.itemgetter(1)):
        j = min(n // m, slots)
        n -= j
        m -= 1
        yield host, jobs[k:k+j]
        k += j

There's nothing wrong with doing it like this, except that it's a bit of a pain keeping the counter k up to date. We don't really care about the value of k, we just want the next j elements from jobs, whatver they are. That's where itertools.islice comes in. The call islice(iterator, j) returns an iterator that takes j elements from iterator. Which is almost exactly what we need, except that it's an iterator, but we need a list. So we convert it to a list by calling the built-in function list.

Here's an example showing islice in action:

>>> from itertools import count, islice
>>> c = count(1) # an iterator that counts upwards from 1
>>> a = islice(c, 5) # an iterator that gets 5 elements from c
>>> list(a)
[1, 2, 3, 4, 5]
>>> list(islice(c, 3)) # get another 3 elements from c
[6, 7, 8]

The only remaining problem with using islice to split up jobs is that this relies upon jobs being an iterator (a list won't work). So we use the built-in function iter to ensure that jobs is an iterator.

You should try out all these functions in the interactive interpreter to get a feel for what they do and how they work.

You also asked about assert. An assertion is a statement that tests a value and raises an exception if it's not true. It declares a condition that must be true at this point in the program. This is useful for programmers who want to read and understand the code, and it also catches implementation errors if these lead to the condition being violated. See Wikipedia on assertions.

In this case, I want to declare that the program has generated an assignment for every host (that is, m == 0 since m starts at the number of hosts and counts down for each one) and that every job has been assigned to some host (that is, n == 0, since n starts at the number of jobs, and gets reduced by j when we assign j jobs to some host).


is it good practice to use relative path in xpath more than once I am using Selenium Webdriver-Java to automate a web application.

Suppose the html is somewhat like below:

          sometext

To locate a element i am using xpath as below:

//div[@id='a1']//span[@id='s1']

is this xpath a good practice

or is it bad practice to use relative path(//) twice in a single xpath?

is it bad practice to use relative path(//) twice in a single xpath?

It would depend on the HTML source of the code - how many elements are there in the path between the nodes, how unique are the elements and their attributes, in general.

In this case though, it is perfectly okay - it is even better than having:

//div[@id='a1']/div/div/span[@id='s1']

because here we check more nodes on the way to span and we start to depend on the markup even more than before - this means our locator is more fragile, less resistant to a design/layout change.

In general, you should avoid using XPath locators if possible - they are the slowest and the least readable. Selenium offers much more options for locating elements:

by CSS selector:

driver.findElement(By.cssSelector("#a1 #s1"));

by ID:

WebElement parent = driver.findElement(By.id("a1"));
WebElement child = parent.findElement(By.id("s1"));

Note that we are not checking tag names here at all - it is a good idea to not use "container"-type tags like div and span inside locators.

IDs are the fastest available locators - locating an element by id internally comes down to executing getElementById() which is optimized by the modern browsers. IDs are also unique by definition (even though nothing prevents you to create two or more elements with the same ID).


Java controller to farm out concurrent tasks I've recently given a coding interview on a Java concurrency task and unfortunately didn't get the job.  The worst part is I've given my best but now I'm not even sure where went wrong.  Can anyone help give me some ideas about things I can improve on below code?

The question is pretty vague.  Given 4 generic interfaces which on a high level divides a task into small pieces, work on each piece and combine the partial result into final result. I'm asked to implement the central controller piece of the interface.  The only requirement is to use concurrency in the partial result processing and "code must be production quality".

My code is as below (the interfaces were given).  I did put in a lot of comment to explain my assumptions which are removed here.

// adding V,W in order to use in private fields types
public class ControllerImpl<T, U, V, W> implements Controller<T, U> {

    private static Logger logger = LoggerFactory.getLogger(ControllerImpl.class);

    private static int BATCH_SIZE = 100;

    private Preprocessor<T, V> preprocessor;
    private Processor<V, W> processor;
    private Postprocessor<U, W> postprocessor;

    public ControllerImpl() {
        this.preprocessor = new PreprocessorImpl<>();
        this.processor = new ProcessorImpl<>();
        this.postprocessor = new PostprocessorImpl<>();
    }

    public ControllerImpl(Preprocessor preprocessor, Processor processor, Postprocessor postprocessor) {
        this.preprocessor = preprocessor;
        this.processor = processor;
        this.postprocessor = postprocessor;
    }

    @Override
    public U process(T arg) {
        if (arg == null) return null;

        final V[] parts = preprocessor.split(arg);
        final W[] partResult = (W[]) new Object[parts.length];

        final int poolSize = Runtime.getRuntime().availableProcessors();
        final ExecutorService executor = getExecutor(poolSize);

        int i = 0;
        while (i < parts.length) {
            final List<Callable<W>> tasks = IntStream.range(i, i + BATCH_SIZE)
                .filter(e -> e < parts.length)
                .mapToObj(e -> (Callable<W>) () -> partResult[e] = processor.processPart(parts[e]))
                .collect(Collectors.toList());
            i += tasks.size();

            try {
                logger.info("invoking batch of {} tasks to workers", tasks.size());
                long start = System.currentTimeMillis();
                final List<Future<W>> futures = executor.invokeAll(tasks);
                long end = System.currentTimeMillis();
                logger.info("done batch processing took {} ms", end - start);
                for (Future future : futures) {
                    future.get();
                }
            } catch (InterruptedException e) {
                logger.error("{}", e);// have comments to explain better handling according to real business requirement
            } catch (ExecutionException e) {
                logger.error("error: ", e);
            }
        }

        MoreExecutors.shutdownAndAwaitTermination(executor, 60, TimeUnit.SECONDS);

        return postprocessor.aggregate(partResult);
    }

    private ExecutorService getExecutor(int poolSize) {
        final ThreadFactory threadFactory = new ThreadFactoryBuilder()
            .setNameFormat("Processor-%d")
            .setDaemon(true)
            .build();
        return new ThreadPoolExecutor(poolSize, poolSize, 60, TimeUnit.SECONDS, new LinkedBlockingDeque<>(), threadFactory);
    }
}

Hints

I would have named the generic types properly.

Maybe I would have divided Logging from processing with help of a listener pattern.

I would have expected the input parameter to never be null, so returning null is obsolete.

I would have extracted some methods to have parts of code be named.

Code

On a very high level view I would have expected exactly what you described what the code should do. Splitting an input in parts, processing these parts in parallel, collect the results and aggregate them. This should have been expressed in the "process"-method.

public class Controller<INPUT, RESULT, PARTIALINPUT, PARTIALRESULT> {

    ...

    public RESULT process(INPUT input) {

        PARTIALINPUT[] partialInputArray = preprocessor.split(input);

        List<Future<PARTIALRESULT>> partialResultFutures = processAsynchronously(partialInputArray);
        List<PARTIALRESULT> partialResults = collectPartialResults(partialResultFutures);

        RESULT result = postprocessor.aggregate(partialResults);

        return result;
    }

    private List<PARTIALRESULT> collectPartialResults(List<Future<PARTIALRESULT>> partialResultFutures)
            throws InterruptedException, ExecutionException {

        List<PARTIALRESULT> partialResults = new ArrayList<>();

        for (Future<PARTIALRESULT> future : partialResultFutures) {
            partialResults.add(future.get());
        }

        return partialResults;
    }

    private List<Future<PARTIALRESULT>> processAsynchronously(PARTIALINPUT[] partialInputArray) {

        List<Future<PARTIALRESULT>> partialResultFutures = new ArrayList<>();

        for (PARTIALINPUT partialinput : partialInputArray) {
            Future<PARTIALRESULT> future = processAsyncronously(partialInput);
            partialResultFutures.add(future);
        }

        return partialResultFutures;
    }

    ...

}


Thread-efficient nonce generations I need to create unique nonces for cryptographic purposes in a library that I am writing.  If the nonces ever fail to be unique, the consequences may be up to and including remote execution of arbitrary code (by means of allowing injection of untrusted input into an unmarshalling operation).

While my particular application does not need to be thread-safe yet, I want the code to be thread-safe so that it can work in multithreaded applications without modification.  I also want it to be fairly efficient – more so than (say) using a global lock for every operation.

The code has not (yet) been tested, mostly because I have no idea how to write a test case for multi-threaded (and mostly lock-free) code.  It makes the assumption that all thread-local storage is initialized to zero when a new thread is created – otherwise, it will fail miserably.

The header file nonce.h:

#ifndef NONCE_H_INCLUDED
#define NONCE_H_INCLUDED NONCE_H_INCLUDED
#pragma once
#ifdef __cplusplus
extern "C" {
#elif 0
}
#endif

typedef struct {
   char nonce[12];
} nonce;

nonce generate_new_nonce(void);

#if 0
{
#elif defined __cplusplus
}
#endif // defined __cplusplus

#endif // !defined NONCE_H_INCLUDED

And the implementation file nonce.c:

#include "nonce.h"
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#define STATIC_ASSERT(x)                                \
   (0*sizeof(struct {                                   \
      int static_assertion_failed : 1 - 2*((int)!(x)); \
      int dummy: 31;                                    \
   }))

static __thread uint32_t local_counter;
static __thread uint64_t local_snapshot;
static uint64_t global_counter;
static uintptr_t lock;

static uint64_t get_new_snapshot(void);

#ifdef __cplusplus
extern "C"
#endif
nonce generate_new_nonce(void) {
   STATIC_ASSERT(sizeof(local_counter) == 4);
   STATIC_ASSERT(sizeof(local_snapshot) == 8);
   STATIC_ASSERT(sizeof(global_counter) == 8);
   nonce retval;
   if (0 == local_counter) {
      local_snapshot = get_new_snapshot();
   }
   memcpy(retval.nonce, &local_counter, sizeof local_counter);
   memcpy(retval.nonce + 4, &local_snapshot, sizeof local_snapshot);
   local_counter += 1;
   return retval;
}

static uint64_t get_new_snapshot(void) {
   while(__sync_lock_test_and_set(&lock, 1)) {}
   const uint64_t retval = global_counter;
   global_counter++;
   if (global_counter == 0) {
      /* Wraparound */
      abort();
   }
   __sync_lock_release(&lock);
   return retval;
}

If all you do in your lock is global_counter++ you may as well make it a c++11 atomic, and just write local_counter = global_counter++.

Or, using what you've used so far, local_counter = __sync_fetch_and_add(&global_counter,1)

You may drop the wraparound check. You can increment once per nanosecond and it'd flip in a few hundred years.

After all that's been said in the comments I'm pretty much convinced there are no simple (read: 3 lines of code) solutions given 4g threads will get created and we have only 32 bit atomics.

There are 3 valid solutions:

Stay with the mutex. I guess if you're spending the time to create a thread, it's not the end of the world to lock a mutex once per the thread's lifetime.
Reuse those threads so you never get to 4g. Maybe you can not coz your client's app created them?
Reuse the nonce ranges. Having so many threads means most of them exit way before they consume the 64bit range per 32bit snapshot.
Now, you can have a destructor on the TLS entry of the snapshot. Lookup pthread_key_create.
You can use this destructor to push the current snapshot and counter into a lock free stack and pop them out in get_new_snapshot.
You can init that array/stack with 1000 nonce ranges (999,0), (998,0),... (0,0)
The first thread would pop (0,0). When it exits it'd push back whatever it got to. Say (0,122283).
If a thread ever consumes it's 64bit range of nonces (if your app runs for a millennia) it can pop another range.
You can pre-allocate a stack large enough for as many concurrent threads as you like. If it's ever consumed default to dynamically allocating more ranges. But this would would probably never get executed.

I can give more detail about a pre-allocated lock free stack, if you like.


How can I simplify these if/else clauses? I have a few if, else-if, else clauses.  The only different thing in the context of these clauses is my LINQ query, one clause has StartsWith, one clause has Contains, and the other does a simple match.  Is there a way to have all of this in one clause and somehow create a variable that can change between StartsWith, Contains, etc.?  My ultimate goal is to reduce the lines of code.

if (parameters[0].ToUpper() == "STARTSWITH" && parameters[1] != null)
    {
        string searchname = parameters[1];
        ViewBag.MyMessage = "Displaying device names beginning with \"" + searchname + "\"";

        SampleEntities sampleEntities = new SampleEntities();

        try
        {
            var Model = (from dev in sampleEntities.NetworkDevices
                         where dev.Name.StartsWith(searchname)
                         from inter in sampleEntities.DeviceInterfaces
                         where inter.NetworkDevice.Id == dev.Id
                         select new DeviceInterfaceModel
                         {
                             DeviceName = dev.Name,
                             InterfaceName = inter.Name,
                             IPv4Address = inter.IPv4Address,
                             IPv4SubnetMask = inter.IPv4SubnetMask,
                             CIDR = inter.CIDR,
                             Subnet = inter.Subnet
                         }).ToList();

            return View(Model);
        }
        catch
        {
        }

        return View();
    }
    #endregion
    #region Name Contains
    else if (parameters[0].ToUpper() == "CONTAINS" && parameters[1] != null)
    {
        string searchname = parameters[1];
        ViewBag.MyMessage = "Displaying device names containing \"" + searchname + "\"";

        SampleEntities sampleEntities = new SampleEntities();

        try
        {
            var Model = (from dev in sampleEntities.NetworkDevices
                         where dev.Name.Contains(searchname)
                         from inter in sampleEntities.DeviceInterfaces
                         where inter.NetworkDevice.Id == dev.Id
                         select new DeviceInterfaceModel
                         {
                             DeviceName = dev.Name,
                             InterfaceName = inter.Name,
                             IPv4Address = inter.IPv4Address,
                             IPv4SubnetMask = inter.IPv4SubnetMask,
                             CIDR = inter.CIDR,
                             Subnet = inter.Subnet
                         }).ToList();

            return View(Model);
        }
        catch
        {
        }

        return View();
    }
    #endregion
}

else
{
    ViewBag.MyMessage = "Diplaying device names equal to \"" + name + "\"";
    SampleEntities sampleEntities = new SampleEntities();

    try
    {
        var Model = (from dev in sampleEntities.NetworkDevices
                     where dev.Name == name
                     from inter in sampleEntities.DeviceInterfaces
                     where inter.NetworkDevice.Id == dev.Id
                     select new DeviceInterfaceModel
                     {
                         DeviceName = dev.Name,
                         InterfaceName = inter.Name,
                         IPv4Address = inter.IPv4Address,
                         IPv4SubnetMask = inter.IPv4SubnetMask,
                         CIDR = inter.CIDR,
                         Subnet = inter.Subnet
                     }).ToList();

        return View(Model);
    }
    catch
    {
    }

    return View();
}

This should be more simplified and more clear, now we make the query to the devices context first, then we get the result, instead of repeating the same thing three times. Remember, always use using when dealing with objects that inherits IDisposable:

using (SampleEntities sampleEntities = new SampleEntities())
{
    string searchname = parameters[1];

    var devices = (from dev in sampleEntities.NetworkDevices
        select dev);

    if (parameters[0].ToUpper() == "STARTSWITH"
        && parameters[1] != null)
    {
        ViewBag.MyMessage = "Displaying device names beginning with \"" + searchname + "\"";
        devices = (from dev in devices
            where dev.Name.StartsWith(searchname));
    }
    else if (parameters[0].ToUpper() == "CONTAINS"
        && parameters[1] != null)
    {
        ViewBag.MyMessage = "Displaying device names containing \"" + searchname + "\"";
        devices = (from dev in devices
            where dev.Name.Contains(searchname));
    }
    else
    {
        ViewBag.MyMessage = "Diplaying device names equal to \"" + name + "\"";
        devices = (from dev in devices
            where dev.Name == name);
    }

    var Model = (from dev in devices
                 from inter in sampleEntities.DeviceInterfaces
                 where inter.NetworkDevice.Id == dev.Id
                 select new DeviceInterfaceModel
                 {
                     DeviceName = dev.Name,
                     InterfaceName = inter.Name,
                     IPv4Address = inter.IPv4Address,
                     IPv4SubnetMask = inter.IPv4SubnetMask,
                     CIDR = inter.CIDR,
                     Subnet = inter.Subnet
                 })
    .ToList();

    return View(Model);
}


Simplify XML parsing using HXT Here is code that will output maven coordinates from given pom.xml file. How can I remove boilerplate (possibly convert it to xml picklers)?

import Control.Lens ((&))
import Text.XML.HXT.Core
import System.Environment (getArgs)
import Text.XML.HXT.Arrow.XmlArrow
import Control.Monad (liftM)
import Data.Maybe
import Control.Applicative

main = do
  (filename:_) <- getArgs
  _groupId <- extractOne filename $ project >>> groupId
  _artifactId <- extractOne filename $ project >>> artifactId
  _version <- extractOne filename $ project >>> version
  _parentGroupId <- extractOne filename $ project >>> parent >>> groupId
  _parentArtifactId <- extractOne filename $ project >>> parent >>> artifactId
  _parentVersion <- extractOne filename $ project >>> parent >>> version
  let coordinate = (,,) <$> _groupId <*> _artifactId <*> _version
      parentCoordinate = (,,) <$> _parentGroupId <*> _parentArtifactId <*> _parentVersion
      in do
        print coordinate
        print parentCoordinate

extract :: String -> IOSArrow XmlTree a -> IO [a]
extract filename cat = runX $ readDocument [withValidate no] filename >>> cat

extractOne :: String -> IOSArrow XmlTree a -> IO (Maybe a)
extractOne filename cat = extract filename cat >>= return . listToMaybe

version :: ArrowXml a => a XmlTree String
version = getChildren >>> hasName "version" >>> getChildren >>> getText

artifactId :: ArrowXml a => a XmlTree String
artifactId = getChildren >>> hasName "artifactId" >>> getChildren >>> getText

groupId :: ArrowXml a => a XmlTree String
groupId = getChildren >>> hasName "groupId" >>> getChildren >>> getText

parent :: ArrowXml a => a XmlTree XmlTree
parent = getChildren >>> hasName "parent"

project :: ArrowXml a => a XmlTree XmlTree
project = getChildren >>> hasName "project"

The main issue I see is that the code opens the XML file six times to
read a single value each time.  Edit:  With a bit more a work I have a
now a rather nice solution for this.  The original minified version is
below.

So the point here is to only read the file once, then filter the tree in
two stages, once for the top-level coordinate and once for the "parent"
node below.  By combining the two stages with &&& and handling the
edge-cases of missing values, the output is same (I hope) and
performance, as well as the number of lines is improved significantly.
(There is still potential for reduction in at least the coordinate
function, but I'll leave that to a kind editor for now.)

import Control.Lens ((&))
import Text.XML.HXT.Core
import System.Environment (getArgs)
import Text.XML.HXT.Arrow.XmlArrow
import Control.Monad (liftM)
import Data.Maybe
import Control.Applicative

type Coordinate = (String, String, String)

main = do
  (filename:_) <- getArgs
  (coordinate, parentCoordinate):_ <- runX (readFileDoc filename >>> coordinatesFilter)
  print coordinate
  print parentCoordinate

readFileDoc = readDocument [withValidate no]

coordinatesFilter :: ArrowXml a => a XmlTree (Maybe Coordinate, Maybe Coordinate)
coordinatesFilter =
  project >>>
  maybeSelector coordinate &&&
  maybeSelector (parent >>> coordinate)

maybeSelector :: ArrowList a => a b c -> a b (Maybe c)
maybeSelector selector = withDefault (selector >>> arr Just) Nothing

namedChildren :: ArrowXml a => String -> a XmlTree XmlTree
namedChildren name = getChildren >>> hasName name

project :: ArrowXml a => a XmlTree XmlTree
project = namedChildren "project"

parent :: ArrowXml a => a XmlTree XmlTree
parent = namedChildren "parent"

field :: ArrowXml a => String -> a XmlTree String
field name = namedChildren name >>> getChildren >>> getText

threeAndSelector :: Arrow a => (t -> a b c) -> t -> t -> t -> a b (c, c, c)
threeAndSelector f a b c = (f a &&& f b &&& f c) >>> arr (\(x, (y, z)) -> (x, y, z))

coordinate :: ArrowXml a => a XmlTree Coordinate
coordinate = threeAndSelector field "groupId" "artifactId" "version"


Encrypt data before sending it to server in a way that makes it unreadable to anyone except the user who sent it I don't have any experience in cryptography and I've written some crypto code that seem to be working, but I'm not sure if it safe and if the tools and methods I used are right.

Objective

Make it possible to store data (JSON) on remote server without this server being able to read it in plaintext.

My Approach

Upon registration, don't send the real user password to the server, use Argon2 hash of the real password instead. In my understanding, it should be pretty hard for a server to infer the real password based on Argon2 hash and without salt.
Use sha256(email + password) to create deterministic salt. It allows users to log in from other devices and still pass password validation. In my understanding, deterministic salt doesn't make the resulting "API password" weaker.
Using the password and salt, create an "API password" (Argon2(password, salt))
Use API password to sign up. Server would only know the derived password and not the original one (entered and known by the user).
Create another Argon2 hash and use it as a key to AES-encrypt the JSON payload (AES-256/CBC/Pkcs).
Attach IV and random salt to encrypted payload. All of that is necessary to decrypt the payload later.
Push the resulting data to back end server.

Code

use argon2::{self, Config};
use sha2::{Digest, Sha256};

use crypto::{ symmetriccipher, buffer, aes, blockmodes };
use crypto::buffer::{ ReadBuffer, WriteBuffer, BufferResult };

use rand::{ Rng };

fn main() {
    // Step 1: Generating "password" that will be used for API sign up
    let email = "foo@bar.com";
    println!("Email: {}", email);
    let password = "foobar";
    // Never leaves client
    println!("Password: {}", password);
    let mut sha_hasher = Sha256::new();
    sha_hasher.input(format!("{}{}", email, password));
    let salt = sha_hasher.result();
    let argon_config = Config::default();
    let password_hash = argon2::hash_encoded(password.as_bytes(), &salt, &argon_config).unwrap();
    println!("Password hash (full): {}", password_hash);
    let password_hash_parts: Vec<&str> = password_hash.split("$").collect();
    let password_hash = password_hash_parts[5];
    // will be used for API sign up
    println!("Password hash (short): {}", password_hash);

    // Step 2: Encrypting some JSON client side
    let payload = "[{ name: \"Secret Name\" }]";
    println!("Original payload: {}", payload);
    let encrypted_payload = encrypt_payload(&payload, &password);
    let decrypted_payload = decrypt_payload(encrypted_payload, &password);
    println!("Decrypted payload: {}", decrypted_payload);

    // Step 3: Pushing encrypted payload to server
    // (omitted, out of scope of review)
}

fn encrypt_payload(payload: &str, password: &str) -> Vec<u8> {
    let salt = rand::thread_rng().gen::<[u8; 16]>();
    let argon_config = Config::default();
    let key = argon2::hash_encoded(password.as_bytes(), &salt, &argon_config).unwrap();
    let iv = rand::thread_rng().gen::<[u8; 16]>();
    let encrypted_bytes = encrypt(payload.as_bytes(), key.as_bytes(), &iv).ok().unwrap();
    let encrypted_bytes : Vec<u8> = salt.iter().chain(&iv).chain(&encrypted_bytes).cloned().collect();
    encrypted_bytes
}

fn decrypt_payload(encrypted_payload: Vec<u8>, password: &str) -> String {
    let salt = &encrypted_payload[0..16];
    let iv = &encrypted_payload[16..32];
    let encrypted_message = &encrypted_payload[32..];
    let argon_config = Config::default();
    let key = argon2::hash_encoded(password.as_bytes(), &salt, &argon_config).unwrap();
    let decrypted_message = decrypt(&encrypted_message[..], key.as_bytes(), &iv).ok().unwrap();
    String::from_utf8(decrypted_message).unwrap()
}

Full code (Rust crate)

Here is the full code with encrypt/decrypt boilerplate based on this example:

crypto.tar.gz

a few comments, from high to low-level:

Usability

users are generally pretty terrible with passwords!  having them "lose" all their data if they forget their password often isn't something they expect.  i.e. they just want to do a "password reset" and get access to everything again.  I know this is at odds with what you (and lots of similar designs) are trying to do, but thought it useful to point out

Separate authentication from encryption

I'd be tempted to use a more conventional protocol for authentication, probably also separating out passwords used for authentication and encryption.  This would let you, e.g., incrementally increase the Argon2 complexity without invalidating their login (this is something you really need to allow for).  It would also let you use some sort of federated authentication system which might be nice

Crypto usage

CBC doesn't provide any authenticity.  i.e. you won't know if the right password has been used / some attacker is trying to decieve you.  I'd suggest using another block mode, or maybe a stream cipher like chacha20-poly1305
I'd probably just use the hash of the email as the salt, using password as well doesn't seem to help much.
your use of Argon2 in encrypt_payload seems to misunderstand its purpose as a general KDF.  You should just request the amount of output you want, e.g., 48 bytes of keying material from Argon2 and use 32 bytes of that as key and 16 bytes as IV.  that means you just need a single 128bit "key" per file rather than a "salt" and an "IV".  your usage isn't any more secure, 128bits is a big enough state space to stop a brute force attack while access to your storage server would give them the header anyway

API usage

argon2::hash_encoded is mostly designed for hashing passwords for authentication.  you almost certainly want to be using hash_raw to just get the keying material out
it might be useful if your design allowed the Config parameters over time to increase computation complexity.  you really want to record the parameter values used somewhere so the right ones can be used


Find the next empty column on a chessboard I was playing around with the "eight queens" problem, just to see how I could use C++ algorithms to write less code.

I figured out a way to find the next unused column using find_if_not.  I am showing both ways of writing it.  Assume we are looping over every valid "file" on a given "rank" ( in C++ terms, I am looping over rows and columns in an 8x8 matrix.)

used is simply a set of column indexes that are currently occupied by a queen.
col is the current index of the column where we will try to place the next queen.

Note for efficiency, the array of column indexes would not be created where it's shown in the code.

Method1:

set<unsigned>::iterator it;
for (; col < 8; ++col)
{
    it = used.find(col);
    if (it == used.end())
        break;
}
if (it != used.end())
    return false;

Method2:

array<unsigned, 8> colIdx{ 0, 1, 2, 3, 4, 5, 6, 7 };
auto it = find_if_not(colIdx.begin() + col, colIdx.end(),
    [&used](unsigned colIdx) { return used.find(colIdx) != used.end(); });
if (it == colIdx.end())
    return false;
else
    col = *it;

Manually listing the numbers from 0 to 7 is a bad sign; I would definitely consider your first method more readable.

Have you considered using an array(-like) of booleans instead of a set, though?

// given array<bool, 8> used;
auto const it = std::find(used.cbegin() + col, used.cend(), false);

if (it == used.cend())
    return false;

col = std::distance(used.cbegin(), it);

It should be faster, too, with no downside that I can think of (since your set and array capacity will be the same).

Then there’s the possibility that the overall code can be structured so you only need an iterator and never an index (except to produce results)…

Alternatively, it still makes the first method simpler.

for (; col < 8; ++col)
    if (!used[col])
        break;

if (col == 8)
    return false;

or

while (used[col])
    if (++col == 8)
        return false;