Checking if a value is in a container I quite often find myself checking whether a given value is present in a given, possibly nested, container. For example, is int val present in the nested container vector<vector<int>> values? To me this looks like a good opportunity to practice with template functions while also cleaning my code. I wrote the following recursive template function for this.

Do you have any suggestion to improve this? E.g, can it be shortened, can it be made more readable or more efficient, is the concept of namespaces correctly used, or should I maybe use a different coding style?

Note: I use gco as an abbreviation for General Container Operations.

// gco.h
#pragma once

#include <algorithm>

namespace bs
{
  namespace gco
  {

    /*
     * This function checks whether 'val' is found in the
     * given container 'source'. The function searches multiple
     * levels deep until it finds the same type as 'val'
     */

    template<typename T1, template <typename> typename Container, typename T2>
    bool contains (Container<T1> const &source, T2 const &val)
    {
      if constexpr (std::is_same<T1, T2>::value)
        return std::find(std::cbegin(source), std::cend(source), val) != std::cend(source);

      else
      {
        for (auto const &el: source)
          if (contains(el, val))
            return true;

        return false;
      }
    }
  }
}

EDIT: minimum working example

#include "./gco.h"
#include <vector>

using namespace std;
using namespace bs;

int main()
{
  vector<int> source_1 = {1, 2, 3, 4};

  vector<vector<int>> source_2 = {{1,2}, {3,4}};

  /*
   * 1) search a single int in a vector
   * 2) search a single int in a nested vector
   * 3) search a vector in a nested vector
   *
   * The following should print 1 0 1 0 1 0
   */

  cout << gco::contains(source_1, 4) << endl
       << gco::contains(source_1, 5) << endl

       << gco::contains(source_2, 4) << endl
       << gco::contains(source_2, 5) << endl

       << gco::contains(source_2, vector<int>({1,2})) << endl
       << gco::contains(source_2, vector<int>({1,3})) << endl;
}

Neat idea.
template<typename T1, template <typename> typename Container, typename T2>
bool contains (Container<T1> const &source, T2 const &val)

I’m not sure a template template parameter is necessary here, or even a good idea. It prevents the function from being used with containers that aren’t templates… or that aren’t the kind of templates you expect.
There is a standard definition for “container”. (There’s also a more-or-less matching proposed concept in the range-v3 library.) Under that definition you could get T1 as typename Container::value_type.
if constexpr (std::is_same<T1, T2>::value)

I don’t think is_same is the test you really want to be applying here, because it precludes all kinds of useful cases. For example, let’s say you have a vector of strings, and the key you want to search for is a string_view:
auto strings = std::vector<std::string>{/*...*/};

auto const search_key = "to be found"sv;

// This won't work
if (contains(strings, search_key))
    // ...

What you want is really the C++20 concept std::equality_comparable_with. Lacking concepts in C++17, you could roll a type trait that does the same test.
return std::find(std::cbegin(source), std::cend(source), val) != std::cend(source);

Using the fully-qualified forms of std::cbegin and std::cend prevents ADL, which could break basically any container not defined in the std namespace.
You should use the standard pattern:
using std::cbegin;
using std::cend;

return std::find(cbegin(source), cend(source), val) != cend(source);

Also, I’d suggest using begin and end rather than cbegin and cend. Why? Because it doesn’t matter, since source is const, but third-party containers may not have bothered to add support for cbegin and cend (begin and end should be universal).
I think you really need some additional type traits (or concepts, if you’re targeting C++20) to make this work. First, a type trait is_container would be very handy for making the function produce better errors, and for helping with the internals:
template <typename Container, typename T>
auto contains(Container const& source, T const& val) ->
    std::enable_if_t<is_container_v<Container>, bool>

Or perhaps, rather than using SFINAE, you could use a static_assert:
template <typename Container, typename T>
auto contains(Container const& source, T const& val) -> bool
{
    static_assert(is_container_v<Container>);

You could also use an equality_comparable_with trait, to allow for more flexibility:
template <typename Container, typename T>
auto contains(Container const& source, T const& val) -> bool
{
    if constexpr (is_equality_comparable_with_v<typename Container::value_type, T>)
        // ...

Those two type traits will also help when testing for whether recursion is possible.
template <typename Container, typename T>
auto contains(Container const& source, T const& val) -> bool
{
    if constexpr (is_equality_comparable_with_v<typename Container::value_type, T>)
        // just use std::find
    else if constexpr (is_container_v<typename Container::value_type>)
        // recurse
    else
        // error

Those type traits will help a lot when there are problems; they can make a huge difference in the size and readability of the error messages you get if you’re dealing with deeply nested containers.
Other than that, the only other thing I can think to suggest is to make the function constexpr. Because why not?
Questions
You say "containers that aren’t templates" and " templates you expect", can you add 1 or 2 examples of this?
In order for a container-like type to work with your template template parameter, 3 things must be true:

It must be a template.
It must have 1 template type parameter (with any subsequent template parameters aliased or defaulted away).
That parameter must be the same as the container’s value type.

For an example that fails the first case: std::filesystem::directory_iterator. It is not a template. So this works:
auto dir = std::filesystem::directory_iterator(".");
if (std::find(begin(dir), end(dir), "foo") != end(dir))
    std::cout << "foo is in current working directory";

but this won’t work:
auto dir = std::filesystem::directory_iterator(".");
if (gco::contains(dir, "foo"))
    std::cout << "foo is in current working directory";

For an example that fails the second case: std::array. Your function expects a template with a single type parameter (and anything else defaulted away). std::array has a type parameter… and a non-type parameter (which is not defaulted). It’s certainly a container, and a template, but not the kind of template you expect.
So this works:
auto numbers = std::array<int, 5>{1, 2, 3, 4, 5};
if (std::find(begin(numbers), end(numbers), 3) != end(numbers))
    std::cout << "found 3";

but this won’t work:
auto numbers = std::array<int, 5>{1, 2, 3, 4, 5};
if (gco::contains(numbers, 3))
    std::cout << "found 3";

For an example that fails the third case… I can’t think of anything off the top of my head. This would require a container template whose first template parameter is not the container’s value type. That would be an odd thing to do (there’s certainly nothing like that in the standard library)… but it could happen!
You suggest to use the standard definition for container. Can you show me how this is done?
In a perfect world, the standard library would have done this for you. (And it will do it for you, but not until C++20.)
But if we’re going to get into this, we really need to tighten up some terminology. I don’t think you want contains() to be a container operation. I think you want it to be a range operation.
In C++ lingo, a container is a type that holds a bunch of stuff. std::vector, std::list, std::map, and so on, they’re all containers. They own their contents; when the container is destroyed, so are all the contents.
But there are other things that are container-like but not containers. As of C++17, there are also views.
A view is a type that provides a container-like view of stuff, but does not hold that stuff. As of C++17, the only view in the standard library (unless I’m forgetting something) is std::string_view, though there are lot more in the pipes, like C++20’s std::span. std::string_view does not own its contents. By contrast, std::string, which is a container, does.
I’m pretty sure you’d want gco::contains() to work with std::string_view. So obviously you want to support more than just containers.
And there are still other things that are neither containers nor views, but still seem like they should be able to work with gco::contains(). There’s nothing really in the standard library yet, but there’s a lot coming in future. For example, coroutines would bring generators. I can imagine a function that checks whether a number is part of the Fibonacci sequence that works something like this:
// fibonacci_sequence() is a generator that returns successive
// elements in the Fibonacci sequence: 1, 2, 3, 5, 8, 13, ...
if (gco::contains(fibonacci_sequence(), number))
    std::cout << number << " is in the Fibonacci sequence";

The generic C++ concept that covers both containers and views and more (including types that provides a container-like view of stuff, but not only don’t own the stuff, the stuff might not actually exist) is range. C++20 will be bringing a long-awaited range library, but C++ has been taking baby steps to support ranges since all the way back in C++11, with the range-for-loop.
I think what you’re making is not a library of generalized container operations, but rather a range library.
Now, if this were C++20, everything would be trivial. You could just do:
template <typename> struct dependent_false : std::false_type {};

template <typename T>
constexpr auto dependent_false_v = dependent_false<T>::value;

template <std::input_range Container, typename T>
constexpr auto contains(Container const& source, T const& val) -> bool
{
    using value_type = std::ranges::range_value_t<Container>;

    using std::begin;
    using std::end;

    if constexpr (std::equality_comparable_with<value_type, T>)
        return std::find(begin(source), end(source), val) != end(source);
    else if constexpr (std::input_range<T>)
        return std::any_of(begin(source), end(source), [&val](auto&& el) { return contains(el, val); });
    else
        static_assert(dependent_false_v<Container>);
}

Unfortunately, in C++17, you don’t have concepts, so you have to roll your own type traits to do the tests.
Okay, so… how? Well, the first thing you need to do is figure out what the requirements are. A minimal range is anything that works with range-for. That requires at least:

begin()
end()
that the return types of begin() and end() can be equality-compared
that the return type of begin() supports operator++; and
probably a few other things I’m forgetting.

You can test the first two requirements with something like:
template <typename Range>
constexpr auto adl_begin(Range&& r)
{
    using std::begin;
    return begin(r);
}

template <typename Range>
constexpr auto adl_end(Range&& r)
{
    using std::end;
    return end(r);
}

template <typename T, typename = void>
struct has_begin_support : std::false_type {};

template <typename T>
struct has_begin_support<T, std::void_t<decltype(adl_begin(std::declval<T>()))> : std::true_type {};

template <typename T, typename = void>
struct has_end_support : std::false_type {};

template <typename T>
struct has_end_support<T, std::void_t<decltype(adl_end(std::declval<T>()))> : std::true_type {};

Then make similar type traits to check that the return type of begin() is equality comparable with the return type of end(), and that the return type of begin() is an input iterator, and so on… and eventually put it all together as a single type trait.
Yes, that’s a shitload of work (unless you use a library like range-v3 which I HIGHLY recommend), but that’s why C++ programmers get the big bucks.
Now, this next bit is an opinionated rant:

 Honestly, I’ve started suggesting to programmers to consider targeting C++20, rather than C++17, even though C++20 isn’t fully officially out yet, and compiler support is as yet anemic. C++20 is the biggest update to C++ since C++11… and it might be even bigger. EVERYTHING will change once C++20 is the norm, probably even far more so than it did when C++11 came around. The amount of code you’d have to write to do contains() well in C++17 is dozens and dozens, if not hundreds of lines of arcane SFINAE crap that only experts understand with bullshit like enable_if and void_t. In C++20? It’s like, 10-12 lines, all obvious, simple, and brief (well, except maybe for the dependent_false trick, but that might not even be necessary, depending on how clear compiler errors are in C++20 mode).

 So it might be worth your while to hold off on writing a container ops library, and for now spend the time reading up on what’s coming in C++: like concepts, modules, and particularly the ranges library. Then when you’re ready, hopefully compiler support will have caught up, and you can roll out this library easily.

 If you really want to target C++17 with your library, that’s fine—there’s very little you can’t do in C++17 (it’s just MUCH easier in C++20). Just understand that you will be diving into a deep, deep well of very, very complicated stuff, and—and this is the worst part—even if you just want to do it for the learning experience, it will be mostly a waste of time, because the skills you’ll learn will be obsolete in a few months.

you suggest either using 'std::enable_if_t' or a static_assert. Is one of the two preferred or more common?
That’s an engineering question, meaning the answer is: there is no right answer; it depends on what you want.
Both methods will trigger a compile error if the constraints are not met. The difference is how.
static_assert is easier to understand. If the constraint fails, the compile just dies. That’s it. Nothing fancy. It’s so brutally simple, that it actually requires tricks to make it not fail when you don’t want it to. If you look at the C++20 version of contains() I wrote above, the static assert uses dependent_false:
static_assert(dependent_false_v<Container>);

If I’d just done:
static_assert(false);

it would just fail, always. Literally always—no code that uses contains() would compile. “But,” you say, “that static_assert is in the else part of an if constexpr!” Doesn’t matter. static_assert doesn’t care. When that function is compiled, if the compiler sees static_assert(false), it just dies immediately, before even bothering to check the rest of the function, before even realizing which branch of the if constexpr it’s in. The dependent_false trick is a way to confuse the compiler temporarily: it doesn’t see the false until after it parses everything in the function… at which point it then sees it’s in an else that doesn’t apply, so it ignores the assertion.
enable_if is more complicated. What it does (assuming you use it right) is remove the function from the overload set temporarily.
To understand what that means, imagine you have two overloads of template <typename T> foo(T) with different return types, then you call foo(0) (with T as an int):
template <typename T>
auto foo(T) -> T { std::cout << "int"; return {}; }

template <typename T>
auto foo(T) -> typename T::value_type { std::cout << "T::value_type"; return {}; }

foo(0); // what happens?

Well, first the compiler collects all the possibilities. So the overload set consists of both foo() functions.
Then the compiler eliminates obvious failures. In this case, foo(0) can’t possibly be calling the overload that returns T::value_type… because int does not have a value_type subtype: int::value_type is nonsense. So that overload is eliminated, leaving only the overload that returns T.
With only one possibility left, the compiler accepts that foo(0) means the first overload. The program compiles, and prints “int”.
If you try to use a std::string (which has a value_type) instead of an int, both overloads are valid, and you’ll get an error because the call is ambiguous.
What enable_if does is gives you control over the elimination step. It basically says “only include this function in the set of potential overloads if the condition is true”. If the condition is false, then it’s as if the function doesn’t exist.
Why is that cool? Well consider if you did this:
template <typename Container, typename T>
auto contains(Container const& source, T const& val) -> bool
{
    static_assert(is_container_v<Container>);

Then when you call contains(something, other) and something is not a container, the static_assert fires, and the whole compile dies. That may be exactly what you want.
But!… Maybe if something is not a container, there is some other overload of contains() that might be suitable (“contains” is a common name, after all!).
So if you did this:
template <typename Container, typename T>
auto contains(Container const& source, T const& val) ->
    std::enable_if_t<is_container_v<Container>, bool>

(or used a concept; a concept basically does the same thing as enable_if), then called contains(something, other) where something is not a container, you don’t immediately get a compile failure. Instead, this function just… disappears. Like it was never written. The compiler goes on searching for all the other overloads of contains() (for everything that is not a container), and if it finds a match, then good! If not, well, then that’s an error as always.
So that’s the difference:

With static_assert, if someone calls contains() with inappropriate arguments, the compile just dies immediately, even if there are other (and better) possibilities that might work.
With enable_if, if someone calls contains() with inappropriate arguments… the compiler just pretends the function doesn’t exist, and keeps looking for other possibilities. If it finds one, it uses it. If not, then that’s an error as always.

Which one is preferred?  It depends on what you want.
If you can be reasonably sure that no one will ever name another function contains() that might be confused with your function (which is a perfectly reasonable assumption!), then static_assert is just fine. But if you think you might be sharing the name contains() with other stuff… maybe you might not want to trigger a hard error right away; maybe you might want your function to “step aside” to give other functions with the same name a chance.
Hope all this helps!


Writing to file using TPL with ConcurrentQueue Here is a sample code to explain the approach I am going for.

public class LogWriter
{
    #region Private Members

    // If writer is not static class, still need to keep single message list; same for other members
    private static ConcurrentQueue<string> _logMessages = new ConcurrentQueue<string>();
    private static object locker = new object();
    private static bool _stopAfterCurrentQueue = false;
    private static bool _discardQueueAndStop = false;

    private static CancellationTokenSource _tokenSource = new CancellationTokenSource();
    #endregion

    public void Write(string text)
    {
        if (!_stopAfterCurrentQueue && !_discardQueueAndStop)
        {
            Task.Run(() =>
           {
               _logMessages.Enqueue(text);

           });

            Task.Run(() =>
            {
                while (!_logMessages.IsEmpty)
                {
                    foreach (var item in _logMessages)
                    {

                        _logMessages.TryDequeue(out string current);
                        lock (locker)
                        {
                            // Will be replaced by StreamWriter
                            File.AppendAllText("Log_" + DateTime.Now.ToString("yyyyMMMdd") + ".txt", current + Environment.NewLine);
                        }
                    }
                }
            }, _tokenSource.Token);
        }
    }

    public void ProcessCurrentAndStop()
    {
        // Only stops accepting new messages, will process the current queue
        _stopAfterCurrentQueue = true;
    }

    public void DiscardQueueAndStop()
    {
        // Cancels subsequent Enqueue
        _tokenSource.Cancel();

        // No more writing even if there is something in the queue
        _discardQueueAndStop = true;
    }

    public void RestartLogging()
    {
        _stopAfterCurrentQueue = false;
        _discardQueueAndStop = false;

        _tokenSource.Dispose();
        _tokenSource = new CancellationTokenSource();
    }
}

The idea is to write to file in asynchronous way. I am trying to make it thread safe as well. There is not much to process here except to write message to a file.

I would like to understand what could be potential issues with this approach and also if it would be better to keep class static.

Let’s try to get rid of ConcurrentQueue. And Task.Run – those are expensive ones. It also makes sense to use async file access – which is a way more lightweight.

public class LogWriter
{
    public static LogWriter Instance = new LogWriter();

    LogWriter()
    {
        Enabled = true;
        Cts = new CancellationTokenSource();
        Task = Task.CompletedTask;
    }

    bool Enabled { get; set; }
    CancellationTokenSource Cts { get; set; }
    Task Task { get; set; }
    string Path => $"Log_{DateTime.Now:yyyyMMMdd}.txt";

    public void Start() => Enabled = true;

    public void Stop(bool discard = false)
    {
        Enabled = false;
        if (discard)
        {
            Cts.Cancel();
            Cts = new CancellationTokenSource();
            Task = Task.ContinueWith(t => { });
        }
    }

    public void Write(string line) =>
        Write(line, Path, Cts.Token);

    void Write(string line, string path, CancellationToken token)
    {
        if (Enabled)
            lock(Task)
                Task = Task.ContinueWithAsync(
                    t => AppendAllTextAsync(path, line + NewLine, token),
                    token);
    }
}

Where missing part would be:

static class AsyncContinuations
{
    public static async Task ContinueWithAsync(this Task task, Func<Task, Task> continuationFunction, CancellationToken cancellationToken)
    {
        await task;
        cancellationToken.ThrowIfCancellationRequested();
        await continuationFunction(task);
    }
}


refactoring and optimization the class There is repetition here and would appreciate help in improving this code.  Suggestions? Here's the framework for it:

Class Author {
  private Integer id;

  public static List<Book> sortBooksAlphabetically( Author a ) {
    Integer id = a.id;
    if (id == null) { return null; }
    List<Book> list = getAllBooksForAuthor( id );
    // do some sorting and return list
  }

  public static List<Book> sortBooksReverseAlphabetically( Integer id ) {
    if (id == null ) { return null; }
    List<Book> list = getAllBooksForMusician( id );
    // do some sorting and return list
  }

  public static List<Book> sortBooksByPopularity( Integer id ) {
    if (id == null ) { return null; }
    List<Book> list = getAllBooksForAuthor( id );
    // do some sorting and return list
  }

  public static List<Book> getAllBooksForAuthor( Integer id ) {
     if (id == null ) throw new NullPointerException();
     ...
     return listOfBooks;
  }
}

Tentative answer...

First of all, it looks like you didn't include the source of the full Author class. Where is the list of Books for that author, for instance?

Second: you seem to want methods to sort this inner list of books for a given author; why, then, pass the id of the author as an argument? You should have a lookup method for an Author given its id -- but this lookup method should not be a method of one Author instance; at worst, it should be a static method in the Author class. What is your backend like? How do you look up an author by its id eventually?

Third: the JDK has Collections.sort(); and given that you want to have a sorted list of books according to several criteria, you want to write several Comparators for that book. See below for a solution to that.

Fourth, and not the least: you should not operate on the list in the Author class itself, but on a copy of that list.

Given these four points above, here are some code extracts...

First: how to return a List<Book> from an author -- don't return the list itself but at least an unmodifiable version of it:

// in class Author; it is supposed that the list has name "bookList" in the class
public List<Book> getBookList()
{
    return new Collections.unmodifiableList(bookList); // not modifiable!
}

Second: you want book lists sorted in various ways; one solution is to create an enum:

public enum BookSort
{
    BY_NAME (new Comparator<Book>()
    {
        @Override
        public int compare(final Book a, final Book b)
        {
            return a.getName().compareTo(b.getName());
        }
    }),
    // other values

    private final Comparator<Book> comparator;

    BookSort(final Comparator<Book> comparator)
    {
        this.comparator = comparator;
    }

    // Return a sorted copy of books
    public List<Book> sort(final List<Book> list)
    {
        final List<Book> ret = new ArrayList<Book>(list);
        Collections.sort(ret, comparator);
        return ret;
    }
}

Then, in your code:

final List<Book> sorted = BookSort.BY_NAME.sort(original);

But all this code is moot to begin with if you do not provide the full code of both the Author and Book class; along with the existing mechanism to grab instances of these two classes.


Height of a general tree in C++ I need to calculate the height of an optional (not AVL, binary) tree. For input I receive an n integer positive number: parent(0), parent(1), ... parent(n-1). Here, parent(i) is a parent of node i. If parent(i) == -1, that i is the root of the tree. It is guaranteed that the sequence has only one root and presets a tree.

Limitation:

\$1 \le n \le 1e5\$

Example input:

5
4 -1 4 1 1

Output:

3

 1
/ \
3  4
   /\
   0 2

My program on C++ works but it looks like that the algorithm is not optimal. The program is verified automatically and one of the test failed with error "Time limit exceeded". Could you advise me, please, on how I can optimize the program?

#include "iostream"

int tree[10000];
int n = 0;

int Height(int parent);

int main ()
{
    std::cin >> n;

    for (int i = 0; i < n; ++i)
    {
        std::cin >> tree[i];
    }

    std::cout << Height(-1) - 1;
    return 0;
}

int Height(int parent)
{
    int height = 1;
    for (int i = 0; i < n; ++i)
    {
        if (tree[i] == parent)
        {
            height = std::max(height, 1 + Height(i));
        }
    }
    return height;
}

Your code is compact, and yet still highly readable. Nice work! Here are some suggestions:
Avoid Global Data
The tree and n variables are global data meaning that any function anywhere in your application can change them. This makes it very difficult to follow the flow of control and figure out where data is modified. It's not a big deal for this little program, but there's no reason you can't put them into main().
Also, while we're talking about your variables, n is not a terribly descriptive name. I would call it something like numNodes or something along those lines.
Don't Traverse the Tree So Many Times
In your Height() function you traverse the tree n^2 times! At the very least, once you've found the parent you shouldn't traverse the rest of the array. But an even better way is to only traverse the entire tree once.
That's what I did in the example below. Note that I couldn't try it out on the website you mentioned because it requires one to sign up for an online course, and as you mentioned, the site was in Russian which I don't understand.
The first thing I did differently was to make a data structure to hold my node data. While the input only contains integers, that doesn't mean that you have to only put integers into your array. I created a type called Node that holds the parent index, a pointer to the actual parent node, and an array of pointers to child nodes. It looks like this:
typedef struct Node {
    int                 parentIndex = { kParentUnset };
    Node*               parent      = { nullptr };
    std::vector<Node*>  children;
} Node;

Which reminds me of my next point - it's a good idea to avoid magic numbers like -1 bare in your code. I made some enums to describe different values for parentIndex:
enum {
    kNoParent       = -1,
    kParentUnset    = -2
};

This makes the code a little easier to read and understand.
You'll notice that I also didn't actually use C arrays, but instead went with std::vector<>. I did this because it allows me to add and remove items without knowing the size ahead of time, and to avoid putting a large block of data on the stack. It's also more standard C++.
So what this code does is read in the number of nodes and the parents of each node. As it reads in a parent, it not only sets the next node's parent index, it also actually sets the pointer to the parent and adds the current node to the parent's list of children. By the time the for loop in main() is complete, we have the entire tree built in memory. The fact is, we don't even need the parent indexes anymore. We could just discard that field, but I've left it to be more clear.
Next, we traverse the tree and calculate the maximum height. This is done using a depth first search. I implemented the search without recursion again to save space on the application stack and avoid a potential stack overflow. We start with the root node and push it onto our local stack variable. Then we enter a loop where we increment the current height, and see if it's greater than the max. If so, we set the max to the current height. Next, we pop the top off the stack. If it has no children, we've hit a leaf node. We decrement the current height and continue on. If it does have children, we push each one onto the stack and loop again.
Here's the code:
int findTreeHeight(Node* rootNode)
{
    int maxHeight   = 0;

    std::stack<Node*>   nodeStack;
    nodeStack.push(rootNode);

    int currentHeight   = 0;
    while (nodeStack.size() > 0)
    {
        currentHeight++;
        if (currentHeight > maxHeight)
        {
            maxHeight = currentHeight;
        }

        Node*   top = nodeStack.top();
        nodeStack.pop();

        if (top->children.size() == 0)
        {
            currentHeight--;
        }

        for (auto iter = top->children.begin(); iter < top->children.end(); iter++)
        {
            nodeStack.push(*iter);
        }
    }

    return maxHeight;
}

int main ()
{
    int numNodes = 0;
    std::cin >> numNodes;

    std::vector<Node> tree(numNodes);

    int rootNodeIndex   = -1;
    for (int i = 0; i < numNodes; ++i)
    {
        int parentIndex;
        std::cin >> parentIndex;

        tree [ i ].parentIndex = parentIndex;

        if (parentIndex == kNoParent)
        {
            rootNodeIndex = i;
        }
        else
        {
            tree [ i ].parent = &tree [ parentIndex ];
            tree [ parentIndex ].children.push_back(&tree [ i ]);
        }
    }

    std::cout << findTreeHeight(&tree [ rootNodeIndex ]) << "\n";

    return 0;
}


Decrease execution time in updating rows in PHP Any tips on how the below code can be optimised for faster execution?

I have a table in postgres called staging with the below columns;

|person|colour|sport|
|Tom   |blue  |Tennis|

and a multi dimensional associative array that contains updates for each entry of the staging table, like so:

$updateArray =  array();
$updateArray['Tom']['color']='orange';
$updateArray['Tom']['sport']='golf';

I need to loop through the updateArray and update the entries of the staging data.

Here is what I have so far, where result2 is a query to select all the rows from the staging table. For each row returned I am looping through every entry of the updateArray, and then checking if it exists as one of the keys.

while ($row2 = pg_fetch_assoc($result2))
        {
            foreach ($array as $key => $value)
            {
                if ($key == $row2['person'])
                {
                    foreach ($value as $column_name => $column_value)
                    {
                        $row2[$column_name] = $column_value;
                    }
                }

            }
            fputcsv($fp, $row2);
        }

The above example is a simplified one, I need to update about 120,000 people and about 700 columns for each person multiple times so I am trying to get the above code more efficient.

You can replace the outer loop with a simple test:

if (array_key_exists($row2['person'], $array)) {
    $value = $array[$row2['person']];
}

Next, the inner loop can be replaced with a single merge:

$row2 = array_merge($row2, $value);

After some small refactorings, the code would look like this:

while ($row2 = pg_fetch_assoc($result2))
{
    if (array_key_exists($row2['person'], $array)) {
        fputcsv($fp, array_merge($row2, $array[$row2['person']]));
    }
}


C++ lazy constructor I'm implementing a lazy constructor in C++. The goal is that for a type T, lazy<T>(args...) returns a callable object which, when called, returns T(args...).

This is what I have done so far:

#include <functional>
#include <utility>

/**
 * Returns an object of type `T` constructed from `args`.
 */
template<typename T, typename... Args>
T make(Args&&... args)
{
    return T(std::forward<Args>(args)...);
}

/**
 * Wrap a value of type `T` and perfect-forward it when accessed.
 */
template<typename T>
class Forward {
public:
    Forward(T &&value) noexcept : value_(std::forward<T>(value))
    {}

    operator T()
    {
        return std::forward<T>(value_);
    }

private:
    T value_;
};

/**
 * Returns a callable object which, when called, returns
 * an object of type `T` constructed from `args`.
 */
template<typename T, typename... Args>
auto lazy(Args&&... args) -> decltype(
        std::bind(
            make<T, Args...>,
            Forward<Args>(std::forward<Args>(args))...))
{
    return std::bind(
            make<T, Args...>,
            Forward<Args>(std::forward<Args>(args))...);
}

(Contrived) Examples:

// rvalue arguments
auto makePtr = lazy<std::unique_ptr<int>>(new int(123));
auto ptr = makePtr();

// lvalue arguments
int *rawPtr = new int(456);
auto makePtr2 = lazy<std::unique_ptr<int>>(rawPtr);
auto ptr2 = makePtr2();

Can this code be improved (particularly in terms of efficiency)?

UPDATE: The Forward class is used only as arguments to std::bind. Rationale:

Suppose we have a type Foo with constructor Foo(const std::unique_ptr<int> &). We can construct it by calling make<Foo, const std::unique_ptr<int> &>(someUniquePtr), for example.

However, std::bind(make<Foo, const std::unique_ptr<int> &>, someUniquePtr)() won't work (I think), because the reference-ness of the second argument to std::bind will be dropped. Wrapping it inside a std::cref works, but for packed arguments it seems the Forward class is the only solution I can come up with that works for all types.

It's possible that I misinterpreted the cause of this but here is an example of what I mean: https://godbolt.org/g/8w2Msa.

I think you can really simplify your code with the use of lambdas.

template<typename T, typename... Args>
auto make_lazy(Args&&... args)
{
    return [=]{return T(std::move(args)...);};
}

Usage example:

#include <iostream>
class T
{
    int v;
    public:
        T(int v): v(v) {std::cout << "Building T\n";}
        friend std::ostream& operator<<(std::ostream& s, T const& out) {return s << "T: " << out.v << "\n";}
};
class S
{
    int v;
    public:
        S(int v): v(v) {std::cout << "Building S\n";}
        friend std::ostream& operator<<(std::ostream& s, S const& out) {return s << "S: " << out.v << "\n";}
};

template<typename T, typename... Args>
auto make_lazy(Args&&... args)
{
    return [=]{return T(std::move(args)...);};
}

int main()
{
    auto lazyT = make_lazy<T>(1);
    auto lazyS = make_lazy<S>(2);

    std::cout << "Building\n";
    std::cout << lazyT() << "\n";
    std::cout << lazyS() << "\n";
}


Lower the volume of my computer over time in Python I wrote a program to gradually lower the volume of my computer when I go to sleep so I can fall asleep with TV or music on.  This was my first time using argparse and I feel like there should be a cleaner way to deal with default values of optional arguments.  Any other feedback of course also welcomed.

import vol
from time import sleep
from subprocess import call, check_output
import argparse

parser = argparse.ArgumentParser()
parser.add_argument("-d", "--delay", help="time (in minutes) until volume begins to lower", type=int)
parser.add_argument("-m", "--minutes_until_mute", help="time (in minutes) to set the volume to mute", type=int)
a = parser.parse_args()
minutes_until_mute = a.minutes_until_mute if a.minutes_until_mute else 30
delay = a.delay if a.delay else 0
sleep(delay*60)
current_volume = check_output(['osascript', '-e', 'set ovol to output volume of (get volume settings)'])

for i in range(minutes_until_mute):
    percent_of_volume = (-1.0/(minutes_until_mute**2))*(i**2) + 1 #quadratic function with maximum at (0, 100%) and crossing at (minutes_until_mute, 0)
    new_volume = int(float(current_volume) * percent_of_volume)
    call(["vol", "out", str(new_volume)])
    call(["vol", "info"])
    sleep(60)

call(["vol", "mute"]) #rounding errors mean we don't get exactly to 0

"I feel like there should be a cleaner way to deal with default values of optional arguments"

Yes, there is. Per the documentation of ArgParse, you can specify a keyword argument default and give it the value you want.

parser.add_argument("-d", "--delay", help="Delay in minutes until starting mute", type=int, default=0)

Documentation here
Tutorial here


What could be bad from using this form of dependancy injection? I have a custom framework that I am not sure if certain things I have applied are wrong and if so, just why.

I am not well versed in any framework, so I was forced to use mine.

Here is an example of a controller found in /app/controllers/UserProfile.php

<?php

    class UserProfile
    {
       public function showUserProfile($import)
       {
         // get view object
         $view =  $import["load"]::view();
         // get orm object
         $db   =  $import["load"]::orm();
         // get form builder object
         $form =  $import["load"]::formBuilder();
         //  extract username & id from current url. i.e. dev.foo.com/john/11
         list($userName, $id) = $import["args"];
         // get view object, for loading templates
         $view::header();   // ~ require  .. view/header.php
         // DATABASE QUERY.
         $users = $db->table("users");
         // SELECT username, age, country from users where username = $username AND id = $id;
         $select = $users->read(["username, age, country", [$username, $id]]);
         $view::body(["select"=>$select]);
       }
    }
?>

And if you are wandering how the DI is being handled, here is how:

<?php

class ServiceProvider
{

    function service($getService)
    {

        switch ($getService) {
            case 'orm':
                return new \foo\bar\orm(new \pdo('....'))
                break;

            case 'view':
                $views_dir = __DIR__.'../path/to/views/dir';
                return new \foo\bar\viewCreatror($views_dir);
                break;

            case 'formBuilder':
                return new \foo\bar\formBuilder('..');
                break;

            default:
                # code...
                break;
        }

    }
}
?>

Now, since I also need to get the URL fragments like :blog-title/:id.
I allow my router to handle that like this.

<?php
namespace foo\bar;
class Router
{
    function add(){}
    function get()
    {
        $service = new \foo\bar\ServiceProvider;
        $url     = $this->getExplodedUrl;
        // IF URL matches, and controller + method exists.
        return (new $controller)->$method(["load"=>$service, "args"=>$url]);
    }
}

The thing is, that everything works well, but to improve it, I need to know which parts need the most urgent attentions.

General impression

You are not actually injecting any of the dependencies into UserProfile, you are
injecting a service provider that has to locate the dependencies.

It may feel like dependency injection but it really isn't.

What you are doing here is, basically, creating spaghetti code.

Suggested improvements

My question would be: What is the UserProfile responsible for?

Your UserProfile class depends on a View, ORM and FormBuilder class.
It is in control of retrieving the user from the DB, outputting a header and
outputting the user in the view body. The FormBuilder does not even seem to be used.

There are several ways this could be cleaned up, for now I'll use "classic" MVC.

Assuming (based on the function name) that this class is primarily a View, it
should only need the user Data and your ViewCreator to function. Your router
would call the Controller that would retrieve the Model and pass it on to your
View.

The suggestion below completely removes the service locator from the picture.
Please note that the code is written for clarity in conveying the message, it is
not meant to be 100% working or correct (especially the static call syntax in the controller).

namespace \foo\bar;

class Router
{
    function get()
    {
        $url = $this->getExplodedUrl();

        // get view object
        $viewsDir = __DIR__ . '/../path/to/views/dir'; //@TODO: Read this from a config file
        $view =  new ViewCreator($viewsDir)

        // get orm object
        $db   =  new Orm(new \Pdo('...'))

        // get form builder object
        $formBuilder = new FormBuilder('...');

        // IF URL matches, and controller + method exists.
        return (new $controller)->$method($url, $db, $view, $formBuilder);
    }
}

class UserController
{
    public function showUserProfile($args, $db, $view)
    {
        // extract username & id from current url. i.e. dev.foo.com/john/11
        list($userName, $id) = $args;
        $select = UserRecord::fetchUserFromDb($db, [$userName, $id]);
        UserProfile::showUserProfile($view, ["select"=>$select]);
    }
}

class UserProfile
{
    /**
     * Display a given User with a given View
     */
    public function showUserProfile($view, $viewData)
   {
     // get view object, for loading templates
     $view::header();   // ~ require  .. view/header.php
     $view::body($viewData);
   }
}

class User
{
    /**
     * Get a given User from a given Database
     */
    public function fetchUserFromDb($db, $user)
    {
         // DATABASE QUERY.
         $users = $db->table("users");

         // SELECT username, age, country from users where username = $username AND id = $id;
         $select = $users->read(["username, age, country", $user]);

         return $select;
    }
}

Things could be cleaned up even further by not initiating classes from the router
but injecting them into the router from your delivery mechanism/root/index.php/etc.

Further Suggestions

You say you're not well versed in any framework but what you seem to be doing
here is very similar to the way Silex works, which uses Pimple (which was
mentioned in the comments to your question). The learning curve is quite flat so
it could be worth the effort of seeing how Silex does things.

If studying an entire framework takes too much of an effort I would strongly
suggest you take a look at PHP-DI to
observe the way a Dependency Injector should work.


Implementation of a generic thread-safe object wrapper I'm currently trying to write useful helper classes for easier concurrent programming in C++ while at the same time also teaching myself 'modern C++'.

This is supposed to be a synchronized object class template, which essentially means that it wraps an object and it's supposed to guarantee that if the owner of the object hasn't gone out of his/her way to screw things up then the object should be thread safe.

It supports two kinds of operations. The first is the overloaded arrow operator which locks the object, performs an action and then unlocks it. The second one is the dereference operator that returns a handle that is a 'safe' reference to the object.

It's essentially supposed to be an easy way of making almost any object thread safe.

#pragma once

#ifndef SDK_SYNCHRONIZED_HPP
#define SDK_SYNCHRONIZED_HPP

#include <type_traits>
#include <mutex>

namespace sdk {

    template<typename T, typename Mutex = std::recursive_mutex>
    class synchronized {
        class object {
        public:

            object(T* obj, Mutex& lock) noexcept
                : m_obj(obj), m_lock(lock) {
            }

            inline object(object&& that) noexcept
                : m_obj(std::move(that.m_obj)), m_lock(std::move(that.m_lock)) {
            }

            inline auto operator->() noexcept {
                return m_obj;
            }

            inline auto operator->() const noexcept {
                return m_obj;
            }

        private:
            T* m_obj;
            std::unique_lock<Mutex> m_lock;
        };

    public:

        template<typename ... A,
         typename = std::enable_if_t<std::is_constructible_v<T, A ...>>>
        synchronized(A&& ... args)
            : m_mutex(), m_obj { std::forward<A>(args) ... } {
        }

        template<typename ... A,
         typename = std::enable_if_t<std::is_constructible_v<T, A ...>>>
        synchronized(Mutex&& m, A&& ... args)
            : m_mutex(m), m_obj { std::forward<A>(args) ... } {
        }

        synchronized(synchronized&& that)
            : m_mutex(std::move(that.m_mutex)), m_obj(std::move(m_obj)) {
        }

        // used for several actions

        [[nodiscard]] inline object operator*() {
            return object(&m_obj, m_mutex);
        }

        [[nodiscard]] inline const object operator*() const {
            return object(&m_obj, m_mutex);
        }

        // synchronized operations

        inline auto operator->() {
            object obj(&m_obj, m_mutex);
            return obj;
        }

        inline auto operator->() const {
            object obj(&m_obj, m_mutex);
            return obj;
        }

    private:

        Mutex m_mutex;
        T m_obj;

    };
}

#endif

And the usage is (normally) supposed to be something like:

// this is supposed to be safe

synchronized<Foo> foo(1, 2.f, true);
// alot of other stuff
auto dostuff = std::thread([&foo](){
    // more stuff here
    foo->bar(false);
    // you get the idea
});
//...
foo->bar(true);

Correctness
foo can be destructed while another thread is performing an operation on it, because the destructor or its caller don't have to acquire a lock to do so. Technically destroying a mutex while it is locked already is undefined behavior, but it gets worse if another thread happens to modify the now invalid object.
Same reasoning for the move constructor: The object from which will be moved could still be modified concurrently by another thread, so locking would be required.
Also, rule of 3/5 violation: There's a custom move constructor (and there will hopefully be a corrected destructor), so copy constructor, move assignment operator and copy assignment operator should also be provided (currently, they copy without locking neither this nor the other object).
Also, m_mutex should be marked mutable, otherwise a const synchronized<Foo>& can't be accessed.
Other nitpicks

In the second constructor, m should be moved.
object::object(T*, Mutex&) should probably take it's first argument as T& instead, as it should never be nullptr.
object as a name is very broad. Maybe change it to something more concise, like with_lock or locked_handle?

Suggestion
If targeting C++17 or later, consider providing a partial specialization for std::shared_mutex: const access paths only require a std::shared_lock(read only), while non-const access paths should be locked with std::unique_lock (write access). Also, for convenience, a as_const() member function to explicitly acquire a const reference would help. See this talk by Andrei Alexandrescu for more information on that topic.


Please have a look at my HTML I have built a website using HTML and CSS. Actually I wanted to learn by implementing real world examples. I have built a site like Google. The search box does not work(I actually did not wanted to go further in the form action part and also did not wanted to use any search engine) and I have not copied anything from their site. Its just an effort to learn HTML and CSS. Please have a look and review it.

CODE:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">

        <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
        <title>MyGoogle</title>

Web

Images

News

Translate

Books

Scholar

Gmail

        <form action="http://www.datafeedfile.com/examples/javascript_search.php" method="get">

        <input type="text" title="Search Text" accesskey="s" alt="Search Text" size="60px" maxlength="80" value="" />

      <input type="submit" value="Google Search"/>
      <input type="submit" value=" I'm Feeling Lucky">

</form>

Just a few quick things I see to help improve your code:

I would start using the HTML5 Doctype, unless you have a specific reason not to. <!doctype html>
Instead of having an
 tag why not just put a border-bottom:2px solid #06f; on your #header div.  And then add like a padding-bottom:5px;
Instead of having

Link

Another Link

Why not just do this:

Link

Another Link

And just change your CSS accordingly:

 .menu { text-decoration:none; margin:0; padding:0; }
 .menu li { display:inline; }


Custom Script Editor I'm making my own script editor (in this case for Arma: Cold War Assault) because I want to learn and this is challenging enough.

Let me just get this out of the way: please don't tell me that I should do easier things. I want to do this anyway.

So, basically I have the simple GUI for now with a working new/open/save file menu.

I've managed to highlight certain words with different colors (because I want to tackle the hardest part first) but it's not efficient.

I've come up with several ideas for the algorithm (didn't implement them all), but I want to know what you'do, if there's a certain way and what I am doing wrong.

This all happens inside a JTextPane class.

The arrays containing the reserved words:

Collections.addAll(keywords, "private", "public", "if", "not", "then", "else", "else if");
Collections.addAll(operators, "+", "-", "=", "==", "?", "!","(", ")","{", "}", "_", "-", "^", "<", ">");

ArrayList<String> keywords = new ArrayList<String>();
ArrayList<String> operators = new ArrayList<String>();

Everytime the user makes an update to the document, it get's updated:

@Override
public void insertUpdate(DocumentEvent e) {
    update();
}

@Override
public void removeUpdate(DocumentEvent e) {
    update();
}

When the user stops typing, it waits 500 ms to update the screen:

Timer t;

/**
 * Updates the text when user stops typing
 */
public void update(){

    if (t != null) {
        if (t.isRunning())
            t.stop();
    }

    t = new Timer(250, new ActionListener() {

        @Override
        public void actionPerformed(ActionEvent e) {

            long start = System.currentTimeMillis();

            String text = getText();

            SimpleAttributeSet attrs = new SimpleAttributeSet();
            StyleConstants.setForeground(attrs, Color.BLACK);
            StyledDocument doc = getStyledDocument();
            doc.setCharacterAttributes(0, text.length(), attrs, true);

            int index = 0, carriage = 0;

            while ((index < text.length())) {

                if (text.codePointAt(index) == 10) {
                    carriage += 1;
                }

                for (String s : Keywords.listKeywords()) {
                    if (text.startsWith(s, index)){
                        changeColor(doc, reserved.get(s), index, carriage, s.length(), false);
                        index += s.length();
                        break;
                    }
                }

                for (String s : Keywords.listOperators()) {
                    if (text.startsWith(s, index)){
                        changeColor(doc, reserved.get(s), index, carriage, s.length(), false);
                        index += s.length();
                        break;
                    }
                }

                index++;
            }
            wordCount.setText("word count: " + index);
            System.out.println("Iterations took: "
                    + (System.currentTimeMillis() - start) + " ms");

            t.stop();
        }
    });
    t.start();
}

private void changeColor(StyledDocument doc, Color color, int index, int carriage, int length, boolean replace) {
    SimpleAttributeSet attrs = new SimpleAttributeSet();
    StyleConstants.setForeground(attrs, color);
    doc.setCharacterAttributes(index - carriage, length, attrs, replace);
}

How would I go about doing this more efficiently?

As of right now, with almost 145.000 words i have a delay of about 400 ms with 12 keywords and other words.

As a general direction for improvement, your technique for finding those keywords uses startsWith, which will effectively compare any given character once per potential keyword or operator. If you switch to regular expressions, by compiling a Pattern that matches any keyword or any operator, you build (what amounts to) a lookup table that typically allows one comparison per character, reducing the total number of comparisons significantly.

You can even use Pattern.quote to make building the String easier.

public Pattern fromList(List<String> strings) {
  StringBuilder builder = new StringBuilder();
  boolean isFirst = true;
  for (String string : strings) {
    if (!isFirst) {
      builder.append('|');
    }
    builder.append(Pattern.quote(string));
    isFirst = false;
  }
  return Pattern.compile(builder.toString());
}

After that, search for every occurrence of the String using either Pattern.matcher() (or Scanner.next()), which will save you those repeated comparisons. At that point you can use the start and end methods on Matcher (or the same on Scanner.match()).

I do notice that your algorithm will tend to highlight the first half of terms like publicValue even if the compiler does not detect it as a keyword. For this, you may want to customize the patterns you generate to match word boundaries.


Checksum computation in x86_64 This is an implementation of the TCP/IP checksum computation as described in https://tools.ietf.org/html/rfc1071.

The function is writen in DynASM, an assembler preprocessor that is part of LuaJIT. I didn't use much of the fancy stuff of DynASM so pretty much it looks like native X86_64 I think.

  -- Prologue.
  | push rbp
  | mov rbp, rsp
  -- Accumulative sum (16-bit).
  | xor r9, r9                  -- Clear out r9. Stores accumulated sum.
  | mov rcx, 0                  -- Set index to 0.
  | 1:
  | cmp rcx, rsi                -- Rsi stores size arg. Compare index to size.
  | jg >2                       -- Go to branch '2' if equals.
  | mov ah, [rdi + rcx]         -- Read 16-bit of data and swapping byte order.
  | mov al, [rdi + rcx+1]
  | add r9, rax                 -- Sum and accumulate into r9.
  | add rcx, 2                  -- Increase index by 2.
  | jmp <1                      -- Go to beginning of loop.
  | 2:
  | mov rax, r9                 -- Save accumulated sum in rax.
  -- Sum carry until done.
  | 4:
  | shr r9, 16                  -- Right-shift r9 16-bit to obtain value of carry.
  | cmp r9, 0                   -- Check whether there's carry.
  | jz >5                       -- In case not, go to '4'.
  | and rax, 0xffff             -- Clear out higher part of rax.
  | add rax, r9                 -- Sum carry to rax.
  | mov r9, rax                 -- Assign rax to r9 to check if there's carry again.
  | jmp <4                      -- Go to beggining of loop.
  -- One-complement.
  | 5:
  | not rax                     -- One-complement of rax.
  | and rax, 0xffff             -- Clear out higher part of rax.
  -- Epilogue.
  | mov rsp, rbp
  | pop rbp
  -- Return.
  | ret

I verified against a Lua implementation the result is correct. I was wondering if I could write the code in a different way to use less instructions. For instance, the RFC says it's possible to sum the octets as a dword. I tried but I didn't manage to get a correct checksum.

Your code, as shown, has a bug in that it will read past the end of the buffer you're computing the checksum for (by either 1 or 2 bytes, depending on if the length is odd or even).  As that is easy enough to fix, I'll assume you make the appropriate changes to the code and move on to answering your question.

If you read over Section 2, paragraphs (B), of the RFC, you'll see that the addition can be done in either byte order.  So rather than using two instructions to byte-swap the two bytes you're reading, just do one (mov ax,[rdi+rcx]).  This will result in the final checksum being byte-swapped, so you'll have to do the swap (xchg al,ah) to get the same value to return from this function.  If you're not doing anything with the checksum other than storing it into the TCP/IP packet, don't swap the return value, and don't swap the bytes when you write them out.

You can remove the cmp r9,0 instruction in the 4 block, as the shr instruction will set the z flag if the new register value is 0.

Now, to expand this method to utilize 32 or 64 bit registers, you can replace the (newly added) mov ax instruction with mov eax or mov rax (with appropriate changes to the value of ecx).  This will need adjustments at the end to shift, add, and handle the carry to collapse the multiple bytes pairs into one, and the carry handling with the 64 bit register will also need to be different (inserting an adc r9,0 after the add r9,rax instruction).  Also, the handling for packets with sizes that are not multiples of 4 (or 8) will need to be added.

Details of this technique are discussed in the RFC.


Is my 4x4 matrix multiplication on a 1D float array correct? I'm trying to develop my own math library to use on a Voxel engine, but I'm worried about Matrices multiplication. Everywhere I saw people using 4x4 Matrices on a 2D Float Array but I'm using a 1D Float Array because of performance needs.

    public float[] multiplicate(float[] factor){
        float[] result = new float[factor.length];
        for (int i = 0, a = 0, b = 0; i < factor.length; i++){
            a = (i/4)*4;
            b = (i%4);
            result[i] = (mat4f[a] * factor[b]) + (mat4f[a+1] * factor[b+4]) + (mat4f[a+2] * factor[b+8]) + (mat4f[a+3] * factor[b+12]);
        }
        return result;
    }

EDIT: I'm using a 1D array to store the 4x4 Vector. If I use 16 float variables instead, should I gain in performance or memory usage? The code should be like that:

    r00 = a00*b00+a01*b10+a02*b20+a03*b30;
    r01 = a00*b01+a01*b11+a02*b21+a03*b31;
    r02 = a00*b02+a01*b12+a02*b22+a03*b32;
    r03 = a00*b03+a01*b13+a02*b23+a03*b33;

    r10 = a10*b00+a11*b10+a12*b20+a13*b30;
    r11 = a10*b01+a11*b11+a12*b21+a13*b31;
    r12 = a10*b02+a11*b12+a12*b22+a13*b32;
    r13 = a10*b03+a11*b13+a12*b23+a13*b33;

    r20 = a20*b00+a21*b10+a22*b20+a23*b30;
    r21 = a20*b01+a21*b11+a22*b21+a23*b31;
    r22 = a20*b02+a21*b12+a22*b22+a23*b32;
    r23 = a20*b03+a21*b13+a22*b23+a23*b33;

    r30 = a30*b00+a31*b10+a32*b20+a33*b30;
    r31 = a30*b01+a31*b11+a32*b21+a33*b31;
    r32 = a30*b02+a31*b12+a32*b22+a33*b32;
    r33 = a30*b03+a31*b13+a32*b23+a33*b33;

Where r00~r33 are 16 float variables where I store the result and later set to my 16 float main fields...The variables a00~a33 and b00~b33 are the factors variables.

So if I use fixed fields instead of a float array, should I gain in performance or memory usage?

My final code is:

public float[] multiplicate(float[] factor){
    int a = 0;
    int b = 0;
    float[] result = new float[factor.length];
    for (int i = 0; i < factor.length; i++){
        a = (i/4)*4;
        b = (i%4);
        result[i] = (mat4f[a] * factor[b]) + (mat4f[a+1] * factor[b+4]) + (mat4f[a+2] * factor[b+8]) + (mat4f[a+3] * factor[b+12]);
    }
    return result;
}

I'll keeping using 1D float array because I'll need to use a FloatBuffer to send it to OpenGL (using LWJGL). So i'll get less overhead by calling one FloatBuffer.put(myArrFloatVar) then FloatBuffer.put(new float[]{r00, r01...r33} or event calling FloatBuffer.put(r00) 16 times.


Null Object pattern with simple class hierarchy I have a simple two-class hierarchy to represent U.S. ZIP (12345) and ZIP+4 (12345-1234) codes. To allow clients to allow both types for a field/parameter or restrict it to one type or the other, the specific types inherit from a common generic ZipCode interface.

Update: A ZIP code is five digits. A ZIP+4 code consists of the primary five-digit ZIP code plus a four-digit plus-4 (+4) code. You can create a ZIP+4 from a regular ZIP and get the primary ZIP from a ZIP+4. Thus, the interface which the two classes implement knows about the two classes, and they know about each other.

interface ZipCode
    boolean isPlusFour()
    ZipPlusFour plusFour(String code)
    Zip primary()
    boolean hasSamePrimary(ZipCode other)

class Zip implements ZipCode
    String code

class ZipPlusFour implements ZipCode
    Zip primary
    String plusFourCode

Introducing Null Object Pattern

To avoid duplicating code that checks for null values throughout the application, I would like to introduce the Null Object pattern. However, I'm afraid the only way to do so that supports the features above is to add three new classes instead of just one:

class NullZipCode implements ZipCode
class NullZip extends Zip
class NullZipPlusFour extends ZipPlusFour

Worse, since the last two extend concrete classes they will need to pass special values to their superclass that will pass validation but not block the possibility of using real values. For example, NullZip would call super("00000").

Here are my main questions, though please don't hesitate to throw out any suggestions you have.

Is there a way to solve this with just one new class to cover all bases?
Is it worth extracting interfaces from Zip and ZipPlusFour so that NullZip won't extend the concrete Zip implementation (same for ZipPlusFour)?

ZipCode
  Zip
    NullZip
    RealZip

Another option is to forego separate classes altogether and check for a special value in Zip and ZipPlusFour to signify a missing ZIP code.

class Zip
    boolean isNone() {
        return code.equals("00000");
    }

    boolean hasSamePrimary(ZipCode other) {
        if (isNone() || other.isNone())
            return false;
        else
            return code.equals(other.primary().code);
    }

At least the complicated checks are encapsulated in these classes which is the whole point of introducing the pattern. The downside is that this logic can be more fragile than inheritance. Is this a good trade-off?

This is a preliminary answer:

  Assume for the sake of this question that going that route isn't
  practical.

Assuming this is about some group of similar value objects.
Everything is meaningful in a context. So we need to determine what are the use cases for this object. Suppose we have the following use cases (I'm trying to guess how you could end up with the provided interfaces.):

We need to display a ZipCode on screen
We need to print ZipCode on envelopes
We also need to lookup some external service for demographic data using the 5-digit ZIP as a key.

Simplest thing that could work would then be something like:

interface ZipCode
    Zip primary() // for service lookup
    String getScreenRepresentation()
    String getPrintRepresentation()
    // you may one to use Visitor pattern
    // if you have much more different formats

Now we may try to design a Null Object that could work for all these situations.
for ZIP and ZIP+4 their implementations are straight forward.
For NullZipCode we can have getScreenRepresentation to return "No ZIP provided" and
getPrintRepresentation to return empty string. You definitely would not want "00000" or some other invalid value on the label. (Unless in the improbable case that US postal service requires people put it to designate unknown ZIP, but you get what I mean)

My criticism of the below code

interface ZipCode
    boolean isPlusFour()
    ZipPlusFour plusFour(String code)
    Zip primary()
    boolean hasSamePrimary(ZipCode other)

class Zip implements ZipCode
    String code

class ZipPlusFour implements ZipCode
    Zip primary
    String plusFourCode

isPlusFour is the back door where all the null checks sneak back in.

plusFour(code) does not belong in the interface. It is a factory method. One sign it does not belong there is Zip and ZipPlusFour should not be able to provide different versions.

hasSamePrimary is unnecessary. Value objects should provide their equality, comparison, hash, display string etc. by overriding equals, hashCode etc in Java, (or deriving Show, Eq in Haskell and so on). By value object semantics; zipCode1.hasSamePrimary(zipCode2) should be equivalent to zipCode1.primary().equals(zipCode2.primary()), it is not an overridable behavior (Closed for modification principle). If it is there for brevity, it can be placed in an abstract base class or as a static method in a Utility class (or extension method if you are using C#, etc.)

What you should do for Dealer class cannot be a simple interface, if it is to be a value object also.

class ZipOrZipPlusFour implements ZipCode
     //fields depend on what sort of persistence you use
     // factory methods:
     newNullZipCode()
     newZip(String code)
     newZipPlusFour(Zip primary, String ext)

class Dealer
    ZipOrZipPlusFour zipCode


The start of a game in pygame, blitting images to the screen The small bit of code below blits and image to the screen within a class. I call this 4 times to display the image 4 times, each image has a different y coordinate and doesn't need different x but when they are blitted they make the code MUCH slower. I know it is the blitting as well due to the fact that if i just draw rects instead, the program moves smoothly.

tableTex = pygame.transform.scale(pygame.image.load('./Sources/Table.png'), (270, 240))

    def display(self):
        if not self.occupied:
            screen.blit(tableTex, (self.x, self.y))
        else:
            pygame.draw.rect(screen, (127, 0, 0), self.rect, 0)

A link to the full code can be found here if needed.
I just want to know if there is any reason this is so slow and if there are any other methods I could use that are faster?

Thanks

As noted in the beginners guide, you should convert your image right after loading it. So instead of

tableTex = pygame.transform.scale(pygame.image.load('./Sources/Table.png'), (270, 240))

Do

tableTex = pygame.transform.scale(pygame.image.load('./Sources/Table.png').convert(), (270, 240))

As noted in the guide linked above, this can lead to a speed-up of about a factor 6. The reason is:

  The 'format' that convert() refers to isn't the file format (ie PNG,
  JPEG, GIF), it's what's called the 'pixel format'. This refers to the
  particular way that a surface records individual colors in a specific
  pixel. If the surface format isn't the same as the display format, SDL [what pygame uses underneath]
  will have to convert it on-the-fly for every blit -- a fairly
  time-consuming process. Don't worry too much about the explanation;
  just note that convert() is necessary if you want to get any kind of
  speed out of your blits.


Short function to remove unnecessary whitespace I have a function consisting of one line of code:

def trimString(string):
    """ Remove unnecessary whitespace. """
    return re.sub('\s+', ' ', string).strip()

But I've been debating with myself whether the following would be better, seeing as explicit > implicit.

def trimString(string):
    """ Remove unnecessary whitespace. """
    string = re.sub('\s+', ' ', string).strip()
    return string

So which is preferable, the former or the latter? And why is that the case?

The question may be off-topic but I find myself asking it often enough and thought this was the place to ask.

I think the shorter, the better. Since you are in Python 3, if you really want to make explicit that the function is returning a string, you can use type hints:

def trimString(string) -> str:

You can also specify it in the parameter:

def trimString(string: str) -> str:

(keep in mind that Python will ignore type hints, but some IDEs like PyCharm use it to detect warnings and errors)

On a side note, you should try to follow the PEP 8 styling conventions; the function name should be in camel case, so trim_string.


Client Side JavaScript [aka Greasemonkey] Key Event Handler Description

The code captures keypress events that do not occur in a text field and dispatches on the properties of a $command object in the application execution context. I have provided a sketch of the execution context in order to assist your review of the code of interest.

Execution Context Sketch

This is a sketch the dependencies of the code for review. The larger context is a user side script running in Firefox using Greasemonkey. The Greasemonkey script is part of a suite of user side tools. This code is not intended for review.

(function () {

  // There is additional application context for making this global
  window.$snatch = function ($class) {
    return document.getElementsByClassName($class)[0];
  };

  // There is additional application context for making this global
  window.$commands = {};

  /* Define links */
  // There is additional application context supporting defining links
  // independently of the code utilizing them.
  var yes = $snatch('yes');
  var maybe = $snatch('rmaybe');
  var no = $snatch('no');

  /* Create Commands */
  // There is additional application context that makes multiple
  // keypresses a more practical approach than it might appear
  $commands.ccc = () => { close(); };
  $commands.yyy = () => { location = yes };
  $commands.mmm = () => { location = maybe };
  $commands.nnn = () => { location = no };
})();

JavaScript for Review

This code for handling key events originated with a  copy-paste of this example on MDN. The modifications have been primarily to implement dispatch.

/* Key Event Handler */
(function () {
  const commandLength = 3;
  var nOffset = 0;
  var keyQueue = "";

  // Add a new property listing all the properties
  $commands.keys = Object.keys($commands);

  document.onkeypress = function (oPEvt) {
    var oEvent = oPEvt || window.event, nChr = oEvent.charCode, sNodeType = oEvent.target.nodeName.toUpperCase();
    if (nChr === 0 || oEvent.target.contentEditable.toUpperCase() === "TRUE" || sNodeType === "TEXTAREA" || sNodeType === "INPUT" && oEvent.target.type.toUpperCase() === "TEXT") { return true; }
    if (keyQueue.length < commandLength) {
      keyQueue += String.fromCharCode(nChr);
    }
    else if (keyQueue.length == commandLength) {
      if ($commands.keys.includes(keyQueue)) {
        $commands[keyQueue]();
        keyQueue = "";
      }
      else {
        keyQueue = keyQueue.slice(-2);
      }
    }
    else { alert("too many characters!"); }
    return true;
  }
})();

Future-proofing
The unreachable else that's not entirely unreachable
In your final else-statement you have an alert that screams at the user "too many characters!". First of all, don't use alert(..), especially for something the user has no control over. Use console.error(..) instead, or just ignore it.
If each handler is executed sequentially after the last one is executed, the else statement can not be reached. There is however no guarantee that this is the case as far as I know. Consider writing at least a bit defensively and notice that the situation where more than commandLength character actually means that your character was added before the other handler cut the characters.
The keys command that is actually not a command
You are abusing the $commands variable to store the keys of that array. If in the future you decide that you want to have commands of length 4... you have a problem. Instead use let keys = Object.keys($commands);.
Clearing the keyQueue
You are clearing the queue after you are executing $commands[keyQueue]();. Depending on how long executing that function takes, that can take a long time. Do it as soon as you can.
Magic number
The -2 in keyQueue.slice(-2); is a magic number. What you actually meant to say is likely keyQueue.slice(-commandLength + 1);. Future-proof your code by eliminating that number and replace it by the actual calculation. You can probably even move this code to the top to group together with the other code to determine the key combination.
Issues with style
ES6 and var
You are using const in your code. If you decide to use const, also use let for non-const variables, as they behave much better than variables declared with var.
Comma-seperated declaration of variables
I am not a fan of comma-seperated declaration of variables. If the declaration is split over multiple lines, it is error-prone and easily allows you to drop variables in the global scope. If it is on one line, like you did, it becomes unreadable and falls off the right side of the screen. Just declare each variable on a single line.
Braces
Your usage of braces is inconsistent. I recommend always having a newline after an opening brace.
Mixing of logical or and logical and
Know which operator is evaluated first, the logical or operator or the logical and operator? Yes? Congratulations, because many developers don't. To avoid ambiguity and possible errors in your code, add parenthesis when you have both a logical or and a logical and in a boolean expression.
Unused variable
nOffset does not seem to be used. Lets just delete it.

I would rewrite your code like this:
/* Key Event Handler */
(function () {
  const commandLength = 3;

  //Last pressed keys
  let keyQueue = "";

  let keys = Object.keys($commands);

  document.onkeypress = function (oPEvt) {
    let oEvent = oPEvt || window.event;
    let nChr = oEvent.charCode;
    let sNodeType = oEvent.target.nodeName.toUpperCase();

    if (nChr === 0 ||
        oEvent.target.contentEditable.toUpperCase() === "TRUE" ||
        sNodeType === "TEXTAREA" ||
        (sNodeType === "INPUT" && oEvent.target.type.toUpperCase() === "TEXT")
       ) {
      return true;
    }

    keyQueue = keyQueue.slice(-commandLength + 1) + String.fromCharCode(nChr);
    if (keys.includes(keyQueue)) {
      keyQueue = "";
      $commands[keyQueue]();
    }

    return true;
  }
}());


"Do you owe the government?" classes Should my class pass parameters internally or reference class level scoped variables?

I'm not sure on the best approach or style for procedure calls that have parameters. Should I go with the class level scoped variables?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount();
    }

    private void CalculateAmount()
    {
        this.AmountToPay = (this.Salary * (this.ArbitraryTaxRate / 100));
    }
}

Or explicitly pass parameters into a procedure?

public class YouOweTheGovernment
{
    public float AmountToPay { get; private set; }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;

        CalculateAmount(this.Salary, this.ArbitraryTaxRate);
    }

    private void CalculateAmount(float salary, float taxRate)
    {
        this.AmountToPay = (salary * (taxRate / 100));
    }
}

In my contrived example I think the first is clearer, but as a class grows in size and complexity it would make it harder to track what is coming from where.

Don't store what you can calculate:

public class YouOweTheGovernment
{
    public float AmountToPay
    {
        get { return Salary * (ArbitraryTaxRate / 100); }
    }
    public float ArbitraryTaxRate { get; private set; }
    public float Salary { get; private set; }

    public YouOweTheGovernment(float taxRate, float salary)
    {
        this.ArbitraryTaxRate = taxRate;
        this.Salary = salary;
    }
}

Your AmountToPay field is basically a cache, and caching is notoriously problematic. Case in point: even your tiny code here has a cache invalidation bug. If you change the tax rate or salary, you aren't recalculating the amount to pay.

Every time you add a field, ask yourself "does this field store unique information that no other field stores?" If the answer is no, don't create the field. The less state you have, the easier it is to understand your code.


Trimming email address to hide the name I am looking to hide an email id name, by only showing first 4 letters of the email. The code works fine. Looking for opinion if I could make it better, more readable and maybe even more efficient if possible. I am using Commons lang library for this and wondering if that is an unnecessary overhead.

Please look for the comment <-- HERE -->. I am looking for opinion in this region. I am using Java 7, no way around that.

import org.apache.commons.lang.StringUtils;
import java.util.HashMap;
import java.util.Map;

public class App {
    public static void main(String[] args) {

        //all these prints out as expected.

        String newEmail1 = getEmail("sample1");
        System.out.println("Expected: some**@gmail.com | actual: " + newEmail1);

        String newEmail2 = getEmail("sample2");
        System.out.println("Expected: null | actual: " + newEmail2);

        String newEmail3 = getEmail("short");
        System.out.println("Expected: q**@gmail.com | actual: " + newEmail3);

        String newEmail4 = getEmail("noname");
        System.out.println("Expected: null | actual: " + newEmail4);
    }

    private static String getEmail(String param){
        //added these solely for testing to show
        Map<String, String> emails = new HashMap<>();
        emails.put("sample1", "someone@gmail.com");
        emails.put("short", "qw@gmail.com");
        emails.put("noname", "@gmail.com");
        //added these solely for testing to show

        //I am looking for opinions from here onwards. <-- HERE -->
        String emailAddress = emails.get(param);
        if(emailAddress != null){
            String emailAddressFront = StringUtils.substringBefore(emailAddress, "@");
            String emailAddressBack = StringUtils.substringAfter(emailAddress, "@");

            // i need to hide emails by only revealing first 4 characters.
            // but also need to check in case email name is shorter than 4.
            if(emailAddressFront.isEmpty()){
                return null;
            }
            int shortenedLength = 4;
            int emailNameLength = emailAddressFront.length();
            if(emailNameLength < 4){
                shortenedLength = emailNameLength - 1;
            }
            emailAddress = emailAddressFront.substring(0, shortenedLength) + "**@" + emailAddressBack;
        }
        return emailAddress;
        //I am looking for opinions till here. <-- HERE -->
    }
}

Guard clauses are easier to read than nested blocks.

It will be somewhat more efficient to track the index of the @ sign rather than using substrings. You definitely don’t need Apache commons.

Try not to reassign variables if you can help it. In this case, you can leverage a StringBuilder to mutate your email address into a new String. The replace method is particularly applicable.

Using the above ideas, you could change your code to look more like:

private static String getEmail(final String param){
    final Map<String, String> emails = new HashMap<>();
    emails.put("sample1", "someone@gmail.com");
    emails.put("short", "qw@gmail.com");
    emails.put("noname", "@gmail.com");

    final String emailAddress = emails.get(param);
    if (emailAddress == null) {
        return null;
    }

    final int atIndex = emailAddress.indexOf('@');
    if (atIndex == 0) {
        return null;
    }

    final StringBuilder maskedEmailAddress = new StringBuilder(emailAddress);
    if (atIndex < 4) {
        maskedEmailAddress.replace(1, atIndex, "**");
    } else {
        maskedEmailAddress.replace(4, atIndex, "**");
    }

    return maskedEmailAddress.toString();
}


Wrapper for REST API calls This is a wrapper that can be used to do additional external REST call to some API. Since I have to provide support to legacy code so I added three static functions to support them.

However, the question that comes to my mind is where to actually set default value either in the constructor or it should be in the main function.

is_optional = kwargs.get('is_optional', False) can also be set directly in constructor. So what is correct way to handle such situations?

class ExternalRestService:
    def __init__(self, endpoint, method=sc.REQUEST_GET):
        self.endpoint = endpoint
        self.method = method
        self.timeout = sc.REQUEST_TIMEOUT

    @staticmethod
    def get(endpoint, request_headers, timeout):
        """
          Handle GET External Call
        """
        logging.debug("About to make an external *** GET *** to url %s", endpoint)
        response = requests.get(url=endpoint, headers=request_headers,
                                timeout=timeout)
        return response

    @staticmethod
    def post(endpoint, request_headers, json_data, timeout):
        """
          Handle POST External Call
        """
        logging.debug("About to make an external *** POST *** to url %s", endpoint)
        request_headers["Content-type"] = "application/json"
        response = requests.post(url=endpoint, headers=request_headers, json_data=json_data,
                                 timeout=timeout)
        return response

    @staticmethod
    def put(endpoint, request_headers, json_data, timeout, fallback=False):
        """
          Handle PUT External Call
        """
        logging.debug("About to make an external *** PUT *** to url %s", endpoint)
        request_headers["Content-type"] = "application/json"
        response = requests.put(url=endpoint, headers=request_headers, json_data=json_data,
                                timeout=timeout)
        if fallback and response.status_code == 405:
            logging.debug("PUT action failed, applying fallback strategy")
            response = requests.put(url=endpoint, headers=request_headers, json_data=json_data,
                                    timeout=timeout)
        return response

    def invoke(self, **kwargs):
        """
        Execute the external call
        """
        is_fallback = kwargs.get('is_fallback', False)
        is_optional = kwargs.get('is_optional', False)
        request_headers = kwargs.get('request_headers', {})
        json_data = kwargs.get('json_data', {})

        response_list = None
        try:
            logging.info("About to make an external *** %s *** call", self.method)
            if self.method == sc.REQUEST_POST:
                response = self.post(self.endpoint, request_headers, json_data, self.timeout)
            elif self.method == sc.REQUEST_PUT:
                response = self.put(self.endpoint, request_headers, json_data,
                                    self.timeout, fallback=is_fallback)
            else:
                response = self.get(self.endpoint, request_headers, self.timeout)
            response_list, error_list = self.__response_handler(response, is_optional)
        except requests.RequestException as err:
            error_list = err
        return response_list, error_list

    @staticmethod
    def __response_handler(response, is_optional=False):
        """
       Handle API response
       """
        status_code = response.status_code
        if is_optional and status_code == 404:
            return response, None
        if not 200 <= status_code < 300:
            return None, response
        response_list = response.json()
        return response_list, None

Overall the existence of this code does not seem justified. It seems like boiler paste that doesn't offer a whole lot of new functionality, and people are better off calling requests directly.

It also doesn't make sense as a class. There's enough static content that it should be a module, not a class. It's good that you're thinking about factoring the fallback feature into a common module, but you need to take a different approach.

One thing you can do is make a subclass of requests.Session, but even that doesn't really seem necessary. Things to avoid:

Don't add logging for stuff that's already logged by requests.
Pass args and kwargs implicitly instead of explicitly copying stuff.
Explicitly pass is_fallback instead of passing it implicitly in kwargs.
Just return the response. Don't convert it to JSON. It's like seven more characters at the calling side, and simplifies everything.
Don't rename request to invoke. Just call it request.
Don't rename url to endpoint. Just call it url.
Don't rename json to json_data. Don't even pass it explicitly. It's quite simple to pass it as json in the kwargs.
Don't set the content type as you are. When requests sees the json kwarg it will do this for you.

In my opinion, at the absolute most, you should condense this to one utility method:

import logging
import requests

def request(method, url, is_fallback=False, **kwargs):
    response = requests.request(method, url, **kwargs)
    if is_fallback and response.status_code == 405:
        logging.debug('%s action failed, applying fallback strategy', method)
        return requests.request(method, url, **kwargs)
    return response

Anything else is unnecessary boilerplate.


Tips on my multi-threading intuition? I wrote a program to copy a .doc template, add some text to it, then save and close it.  I have to do this many times, and saving and closing a word document is slow. I decided to use multi-threading, but I'm a noob at this stuff and just wrote what came to me. Any feedback would be greatly appreciated (on any aspect, not just the multi-threading).

Here is my code for a task:

public Task
{
    public string Value;
    public bool HasRun;
    public Task(string value)
    {
        Value = value;
        HasRun = false;
    }
}

I create a list of Tasks that is a class level variable, accessible by all threads:

Tasks = db.table.select(a => new Task(a.value)).ToList();

Next, I run the following in each thread:

while(tasks.Any(a => a.HasRun == false))
{
    var value = "";
    lock(tasks)
    {
        var task = tasks.Where(a => a.HasRun == false).First();
        value = task.Value;
        task.HasRun = true;
    }

    CreateNewDocumentAppendValueAndSave(value);
}

Let the framework do multithreading for you:

List<string> tasks = new List<string>();
foreach (string value in db.table.select(a => a.value))
{
    tasks.Add(Task.Factory.StartNew(() => CreateNewDocumentAppendValueAndSave(value)));
}
Task.WaitAll(tasks.ToArray());

Where Task is System.Threading.Tasks.Task.


Is this a workable UnitOfWork with Repository pattern design? I am building a web app with using UOW and Repository pattern. I have seen many different samples and found each one to be different, so not sure which is the correct way to go. I have a basic understanding of both these patterns and I wanted to know if I should keep one UOW implementation for all the tables in my project or keep a separate one as per functionality like for:

public interface IHomeUOW
{
    IGenericRepository<User> Users { get; }
    IGenericRepository<TableA> Table_A { get; }
    IGenericRepository<TableB> Table_B{ get; }
}

public interface IBusinessCaseUOW
{

    IGenericRepository<TableA> Table_A { get; }
    IGenericRepository<TableXYZ> Table_XYZ{ get; }
}

As you can see, TableA is available in both Home UOW as well as a particular business case UOW. One UOW partially implemented as below:

public class UnitOfWork : IUnitOfWork
{

    private readonly ObjectContext _context;
    private UserRepository _userRepository;

    public UnitOfWork(ObjectContext Context)
    {

        if (Context == null)
        {
            throw new ArgumentNullException("Context wasn't supplied");
        }
        _context = Context;
    }

    public IGenericRepository<User> Users
    {
        get
        {
            if (_userRepository == null)
            {
                _userRepository = new UserRepository(_context);
            }

            return _userRepository;
        }
    }
 }

My repositories will be like so

public interface IGenericRepository<T>
    where T : class
    {
        //Fetch records
        T GetSingleByRowIdentifier(int id);
        T GetSingleByRowIdentifier(string id);

        IQueryable<T> FindByFilter(Expression<Func<T, bool>> filter);

        // CRUD Ops
        void AddRow(T entity);
        void UpdateRow(T entity);
        void DeleteRow(T entity);

    }

    public abstract class GenericRepository<T> : IGenericRepository<T>
            where T : class
    {
        protected IObjectSet<T> _objectSet;
        protected ObjectContext _context;

        public GenericRepository(ObjectContext Context)
        {
            _objectSet = Context.CreateObjectSet<T>();
            _context = Context;
        }

        //Fetch Data
        public abstract T GetSingleByRowIdentifier(int id);
        public abstract T GetSingleByRowIdentifier(string id);

        public IQueryable<T> FindByFilter(Expression<Func<T, bool>> filter)
        {
            //
        }

        //CRUD Operations implemented

    }

   public class UserRepository : GenericRepository<User>
   {
         public UserRepository(ObjectContext Context)
         : base(Context)
         {
         }

         public override User GetSingleByRowIdentifier(int id)
         {
          //implementation
         }

         public override User GetSingleByRowIdentifier(string username)
         {
          //implementation
         }
   }

What do you think? If this is not the correct implementation of UOW and Repository pattern for DDD, will it fail as just a bunch of code written to abstract the call to the EF tables?

The UoW pattern has a couple of properties. The UoW will:

track changes to entities, wether that be adding, removing or updating.
coordinate the persisting of these changes in one atomic action. Which also means rolling back if an error occurs.

Since you're using EF, you don't have to implement this stuff, it's done for you. The ObjectContext is actually the UoW object. So unless you plan to switch out Persistence frameworks later on, any additional abstraction will be wasted effort.

I'm guessing you want to make sure your domain isn't aware of how it's persisted. Your domain should only know of the repository interfaces. The repository interfaces are actually part of your domain. The repository implementation though, is part of the persistance layer.

The repository implementation looks OK. I'm only curious about FindByFilter; if you're returning an IQueryable<T> object, you're saying you can query it. OTOH the FindByFilter function already takes an Expression<Func<T, bool>> filter parameter. I'd pick one of the two like the following:

public IEnumerable<T> FindByFilter(Expression<Func<T, bool>> filter)
{
    //
}

or

public IQueryable<T> FindAll()
{
    //
}

Hope this helps.


Write a procedure stream-limit that finds From SICP:

  Exercise 3.64.  Write a procedure
  stream-limit that takes as arguments a
  stream and a number (the tolerance).
  It should examine the stream until it
  finds two successive elements that
  differ in absolute value by less than
  the tolerance, and return the second
  of the two elements. Using this, we
  could compute square roots up to a
  given tolerance by

(define (sqrt x tolerance)
  (stream-limit (sqrt-stream x) tolerance))

I wrote this answer:

(define (stream-limit s tol)
  (if (stream-null? s)
      (error "End of stream found before limit reached:" tol)
      (let* ((a (stream-car s))
             (b (stream-car (stream-cdr s)))
             (diff (abs (- a b))))
        (if (> tol diff) b (stream-limit (stream-cdr s) tol)))))

Can it be improved?

Looks fine to me.  I would only suggest changing (> tol diff) to (< diff tol) because it reads more like the problem specification and is easier to understand.


improving speed of this numpy-based diffraction calculator I'm going to simulate diffraction patterns of a normal incident gaussian profile beam from a 2D array of point scatterers with a distribution in heights.

The 2D array of scatterer positions X, Y and Z each have size N x N and these are summed-over in each call to E_ab(a, b, positions, w_beam). This is done M x M times to build up the diffraction pattern.

If I estimate ten floating point operations per scatter site per pixel and a nanosecond per flop (what my laptop does for small numpy arrays) I'd expect the time to be 10 M^2 N^2 1E-09 seconds. For small N this runs a factor of 50 or 100 slower than that, and for large N (larger than say 2000) it slows down even further. I am guessing these have something to do with the paging of the large arrays in memory.

What can I do to increase the speed for large N?

note: Right now the height variation Z is random, in the future I plan to include an additional systematic height variation term as well, so even though purely gaussian variation might have an analytical solution, I need to do this numerically.

Since I'm distributing Z height randomly here, plots will look a little different each time. My output (run on a laptop) is as follows, and I can not even begin to understand why it takes longer (~ 16 seconds) when w_beam is small than when it is large (~6 seconds). (When I run this on an older Python 2 that came with IDLE all four are between 10 and 10.5 seconds)

My estimator 10 M^2 N^2 1E-09 suggests 0.25 seconds, these are roughly 50 times slower so there may be room for substantial improvement.

1 16.460583925247192
2 14.861294031143188
4 8.405776023864746
8 6.4988932609558105

Total time: about 46 seconds on 2012 MacBook and recent Anaconda Python 3 installation.

Python script:

import numpy as np
import matplotlib.pyplot as plt
import time

def E_ab(a, b, positions, w_beam, k0):
    X, Y, Z = positions
    Rsq = X**2 + Y**2
    phases = k0 * (a*X + b*Y + (1 + np.sqrt(1 - a**2 - b**2))*Z)
    E = np.exp(-Rsq/w_beam**2)  * np.exp(-j*phases)
    return E.sum() / w_beam**2 # rough normalization

twopi, j = 2*np.pi, np.complex(0, 1)

wavelength = 0.08
k0  = twopi/wavelength

z_noise = 0.05 * wavelength

N, M = 100, 50
x = np.arange(-N, N+1)
X, Y = np.meshgrid(x, x)
Z = z_noise * np.random.normal(size=X.shape) # use random Z noise for now
positions = [X, Y, Z]

A = np.linspace(0, 0.2, M)

answers = []
for w_beam in (1, 2, 4, 8):
    E = []
    tstart = time.time()
    for i, a in enumerate(A):
        EE = []
        for b in A:
            e = E_ab(a, b, positions, w_beam, k0)
            EE.append(e)
        E.append(EE)
    print(w_beam, time.time() - tstart)
    answers.append(np.array(E))

if True:
    plt.figure()
    for i, E in enumerate(answers):
        plt.subplot(2, 2, i+1)
        plt.imshow(np.log10(np.abs(E)), vmin=0.0001)
        plt.colorbar()
    plt.show()

I managed to make your script a bit faster. See the comments in the
source code:

import numpy as np
import matplotlib.pyplot as plt
import time

# Makes it so numpy always outputs the same random numbers which is
# useful during development. Comment out the line in production.
np.random.seed(1234)

def main():
    twopi, img = 2*np.pi, np.complex(0, 1)

    wavelength = 0.08
    k0  = twopi/wavelength

    z_noise = 0.05 * wavelength

    N, M = 100, 50
    x = np.arange(-N, N+1)

    # Use random Z noise for now
    Z = z_noise * np.random.normal(size = (x.shape[0], x.shape[0]))

    A = np.linspace(0, 0.2, M)

    tstart = time.time()
    answers = []

    # X and Y with shapes (x, 1) and (1, x) contains the same data.
    X = x[np.newaxis,:]
    Y = x[:,np.newaxis]

    # Compute squared Euclidean distances with shape (x, x).
    Rsq = X**2 + Y**2

    # Compute other distances with shape (M, M).
    a = A[:,np.newaxis]
    b = A[np.newaxis,:]
    SQ = 1 + np.sqrt(1 - a**2 - b**2)

    # Add noise thing (M, M, x, x).
    SQ_Z = SQ[:,:,np.newaxis,np.newaxis] * Z

    # Gets shape (M, 1, x) and (M, x, 1).
    A_X = A[:,np.newaxis,np.newaxis] * X
    A_Y = A[:,np.newaxis,np.newaxis] * Y

    # Calculates A*X + A*Y with shape (M, M, x, x).
    A_X_Y = A_X[:,np.newaxis] + A_Y[np.newaxis,:]

    # e^{-j*phases} with shape (M, M, x, x).
    A_stuff = np.exp(-img * k0 * (A_X_Y + SQ_Z))
    for w_beam in (1, 2, 4, 8):
        coeff = np.exp(-Rsq/w_beam**2)
        # Sums over the last two dimensions, shape becomes (M, M).
        E = np.sum(A_stuff * coeff, axis = (2, 3)) / w_beam**2
        answers.append(E)

    print(f'Time: {time.time() - tstart:.3}')
    # Print checksum. Useful for debugging.
    print(sum(np.sum(E) for e in answers))
    if True:
        plt.figure()
        for i, E in enumerate(answers):
            plt.subplot(2, 2, i+1)
            plt.imshow(np.log10(np.abs(E)), vmin=0.0001)
            plt.colorbar()
        plt.show()

main()

I have wrapped all your code in a main function. It is good practice
to do so, even for small scripts, but it makes it easy to see if
there's any dependencies on global variables.

The script runs about four times faster. Most of the speedup comes
from replacing your explicit loop with vector functions. So instead of

for i, a in enumerate(A):
    EE = []
    for b in A:
        e = E_ab(a, b, positions, w_beam, k0)
        EE.append(e)
    E.append(EE)

those lines now read

coeff = np.exp(-Rsq/w_beam**2)
E = np.sum(A_stuff * coeff, axis = (2, 3)) / w_beam**2
answers.append(E)

To get the data in a format that vector functions can handle I've
added a lot of
broadcasting. It
can be used to compute, for example, the cartesian
product
of arbitrary numpy arrays.

I think someone who understands the math (which I don't) could
optimize your script even more. Some of the calculations seem
redundant to me, but I have no idea how to factor them out.