Thread-safe Phoenix Singleton class template with Boost I've implemented the phoenix-singleton as a class template with boost inspired by Modern C++ Design. The code compiles fine with MSVC 2013 - and it seems to work, but I'm new
to multi-threaded programming and Boost. Are there any pitfalls in the code? Improvements?

TSingleton.h

#ifndef TSINGLETON_H
#define TSINGLETON_H

#include <cstdlib>
#include <boost/atomic.hpp>
#include <boost/thread/mutex.hpp>

template<typename T>
class TSingleton{
public:
    static T& getInstance();

protected:
    inline explicit TSingleton();
    TSingleton(const TSingleton&);
    TSingleton& operator=(const TSingleton&);
    virtual ~TSingleton();
    static void _CreateInstance();
    static void _Phoenix();
    static void _KillPhoenix();

    static boost::atomic<T*> _instance;
    static boost::atomic<bool> _destroyed;
    static boost::mutex _instantiation_mutex;
};

template<typename T>
boost::atomic<bool> TSingleton<T>::_destroyed = (boost::atomic<bool>) false;

template<typename T>
boost::atomic<T*> TSingleton<T>::_instance = (boost::atomic<T*>) nullptr;

template<typename T>
boost::mutex TSingleton<T>::_instantiation_mutex;

template<typename T>
TSingleton<T>::~TSingleton(){
    TSingleton::_destroyed = true;
    TSingleton::_instance = nullptr;
}

template<typename T>
T& TSingleton<T>::getInstance(){
    T* tmp = TSingleton::_instance.load(boost::memory_order_consume);
    if (!tmp){
        boost::mutex::scoped_lock guard(TSingleton::_instantiation_mutex);
        tmp = TSingleton::_instance.load(boost::memory_order_consume);
        if (!tmp){
            if (TSingleton::_destroyed){
                TSingleton::_Phoenix();
            }else{
                TSingleton::_CreateInstance();
            }
            tmp = TSingleton::_instance.load(boost::memory_order_consume);
        }
    }
    return *tmp;
}

template<typename T>
void TSingleton<T>::_CreateInstance(){
    static T instance;
    TSingleton::_instance.store(&instance, boost::memory_order_release);
}

template<typename T>
void TSingleton<T>::_Phoenix(){
    TSingleton::_CreateInstance();
    new(TSingleton::_instance) T;
    std::atexit(TSingleton::_KillPhoenix);
    TSingleton::_destroyed = false;
}

template<typename T>
void TSingleton<T>::_KillPhoenix(){
    T* tmp = TSingleton::_instance.load(boost::memory_order_consume);
    tmp->~T();
    TSingleton::_destroyed = true;
    TSingleton::_instance = nullptr;
}

#endif // TSINGLETON_H

Simple usage example:

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

class Foo{
public:
    void bar(){ std::cout << "bar" << std::endl; }
};

int main(int argc, char **argv){
    Foo& myfoo = TSingleton<Foo>::getInstance();
    myfoo.bar();
    return 0;
}

I'm not a threading specialist, so won't be able to tell you for sure if your code is 100% race condition free. However, there are a few other code smells that I would like to warn you about:

Reserved names: A name starting with an underscore and an uppercase letter is reserved in any scope. _CreateInstance, _Phoenix and _KillPhoenix are not safe to be used, even inside a class.
inline is redundant for a template class or function. The class is always inline, so you don't have to add the keyword.
explicit makes no sense for a constructor that takes 0 arguments. You usually apply explicit if you have a constructor taking 1 argument and you wish to avoid an implicit conversion.
Do you really plan on allowing classes to inherit from TSingleton? It doesn't seem like there is a need for that, since the singleton type is a template parameter. Maybe I'm missing something, but if it is not to be inherited, make all currently protected members private and remove virtual from the destructor.
This initialization style is mighty strange: _destroyed = (boost::atomic<bool>) false; A plain assignment _destroyed = false; or using the constructor syntax: _destroyed(false); would be much better.
When calling a static method or accessing a static variable of a class, inside the given class, prefixing the call with ClassName is optional. So you could, for example, simply use _instance instead of TSingleton::_instance.
VS 2013 should have most if not all C++11 features and libraries working. So you could replace Boost with the new threading and atomics libraries to reduce dependencies.
If you switch to C++11, you can delete the copy constructor and assignment operator.


Utilisation of multi-threading while reading a file and want to find the top 20 most frequently used words The purpose of this code per the title is to utilise the multi-threading (even though the improvement might be minor or even have negative impact, although comments on how to improve efficiency are appreciated). As part of the work, we are expected to read a file and print the 20 most frequently used words.

#include <iostream>
#include <iterator>
#include <set>
#include <vector>
#include <algorithm>
#include <map>
#include <mutex>
#include <fstream>
#include <pthread.h>
#include <queue>
#include "mingw-std-threads-master/mingw-std-threads-master/mingw.thread.h"
#include "mingw-std-threads-master/mingw-std-threads-master/mingw.mutex.h"

//bad practise but convenient for this code example
using namespace std;

//File to read so that we can find the most frequent used words.
#define FILENAME "mobydick.txt"

//The amount of words to find which are most used.
#define WORDS_TO_FIND 20

map<string,int> word_count;
queue<string> word_queue;
std::mutex m;

//Not great approach but condition_variable is not available in my environment.
bool keep_running = true;

//Ignore punctuations coming from the stream, and to read only valid "english" letters from the input stream.
//This is an improvement but it still doesn't cover all cases.
struct letter_only: ctype<char>
{
    letter_only(): ctype<char>(get_table()) {}

    static ctype_base::mask const* get_table()
    {
        static vector<ctype_base::mask> rc(ctype<char>::table_size,ctype_base::space);

        fill(&rc['a'], &rc['z'+1], ctype_base::alpha);
        return &rc[0];
    }
};

void insert()
{
    string word;

    while(keep_running || (!keep_running && !word_queue.empty()))
    {
        if(!word_queue.empty())
        {
            m.lock();
            word = word_queue.front();
            word_queue.pop();

            if (word_count.find(word) == word_count.end())
            {
                word_count.insert(pair<string, int>(word, 1));
            }
            else
            {
                word_count.find(word).operator*().second++;
            }
            m.unlock();
        }
    }
}

void read_file()
{
    ifstream file;

    file.imbue(locale(locale(), new letter_only()));

    file.open(FILENAME);

    if (!file.is_open())
    {
        exit(-1);
    }

    string word;
    while (file >> word)
    {
        m.lock();
        word_queue.push(word);
        m.unlock();
    }

    file.close();
    keep_running = false;
}

void print_results()
{
    int biggestNum = 0;
    string word;

    for(int i = 0; i < WORDS_TO_FIND; i++)
    {
        for(auto it = word_count.begin(); it != word_count.end(); ++it )
        {
            if (it->second > biggestNum)
            {
                biggestNum = it->second;
                word = it->first;
            }
        }

        if(word.length() > 0)
        {
            word_count.erase(word);
            cout << word + ": " << biggestNum << endl;
        }

        biggestNum = 0;
    }
}

int main()
{
    /** Thoughts...
    * The only ways I know to improve disk read performance are:
     * 1) read the data from a compressed source.
     * 2) use faster disks, or RAID array. or
     * 3) split the data onto separate disks and read 1 thread per disk. Usually, if a single thread can't keep up
     * with your disk read time, you have big problems.
    **/

    //An option to consider is task-based parallelism.. std::async which lets the platform decide when to spawn a thread.

    thread first(read_file);
    first.detach();

    thread second(insert);
    second.join(); //wait for read_file and insert to finish before printing the results

    print_results();

    return 0;
}

Code Review

A lot of people like to put some logical order on the includes.

#include <iostream>
#include <iterator>
#include <set>
#include <vector>
#include <algorithm>
#include <map>
#include <mutex>
#include <fstream>

I like to put containers together, streams stuff together, C stuff together; others like to order them alphabetically. But with this many includes some sort of logic is probably a good idea.

You don't need to use this old library.

#include <pthread.h>

Threading is now part of the C++ standard. All the threading and mutex classes are included.

Not sure what these are:

#include "mingw-std-threads-master/mingw-std-threads-master/mingw.thread.h"
#include "mingw-std-threads-master/mingw-std-threads-master/mingw.mutex.h"

But that seems awfully deeply nested. Which usually means that there are are top level includes that include these files indirectly. A comment with a link to the documentation would be nice especially since a maintainer is not likely to know about these.

Hmmm

//bad practise but convenient for this code example
using namespace std;

C Macro

No need to use macros. There are better C++ alternatives.

//File to read so that we can find the most frequent used words.
#define FILENAME "mobydick.txt"

//The amount of words to find which are most used.
#define WORDS_TO_FIND 20

Notice we can type these correctly now.

// http://en.cppreference.com/w/cpp/string/basic_string/operator%22%22s
static std::string constexpr fileName    = "mobydick.txt"s;
static int constexpr         wordsToFind = 20;

Globals

Globals is a bad idea. These should be wrapped in a class object.

map<string,int> word_count;
queue<string> word_queue;
std::mutex m;                // inconsistent use of std::

//Not great approach but condition_variable is not available in my environment.
bool keep_running = true;

Locals

Love it.
Not enough people work out how to use the streams and locals correctly.

//Ignore punctuations coming from the stream, and to read only valid "english" letters from the input stream.
//This is an improvement but it still doesn't cover all cases.
struct letter_only: ctype<char>
{
    letter_only(): ctype<char>(get_table()) {}

    static ctype_base::mask const* get_table()
    {
        static vector<ctype_base::mask> rc(ctype<char>::table_size,ctype_base::space);

        fill(&rc['a'], &rc['z'+1], ctype_base::alpha);
        return &rc[0];
    }
};

Insert

Your lock is over too large a surface area. You lock the queue even while you are working on word_count and this is not used by the reader thread. The reader thread could be actively using the queue while you play with the word count object.

You should also look into using RAII to correctly lock and unlock your lock.

You read loop is a busy loop. You are going to melt the processor doing this. It would be better to use a condition variable and force the thread to sleep if the queue is empty (which it will be most of the time).

Lastly you overcomplicated the addition.

std::string getWord()
{
    std::lock_guard<std::mutex>  lock(m);   // Only lock as long as you need.
    std::word result = word_queue.front();
    word_queue.pop();
    return result;
}
void insert()
{
    string word;

    while(keep_running || (!keep_running && !word_queue.empty()))
    {
        // Add condition variable here.
        if(!word_queue.empty())
        {
            // Call getWord() that locks as part of the access
            // It is in a separate function so that the RAII lock
            // can simply unlock on exit.
            word = getWord();

            // If word does not exist. The [] operator inserts a value
            // using `word` as the key and zero-initializes the value
            // so for an int the value inserted is a zero.
            //
            // It returns a reference to the value, so you can simply
            // increment the result of the operation.
            ++word_count[word];
       }
    }
}

Optimization

This is very expensive.

    for(int i = 0; i < WORDS_TO_FIND; i++)
    {
        for(auto it = word_count.begin(); it != word_count.end(); ++it )
        {

You are basically looping over all the words 20 times. This list could be huge. There are 171,476 words in the english language (assume no spelling mistakes many more if you include these). You can replace this and loop over the words once and just keep track of the top 20.


Stack with 'getMinimum' operation Design a Data Structure SpecialStack that supports all the stack operations like push(), pop(), isEmpty(), isFull() and an additional operation getMin() which should return minimum element from the SpecialStack. All these operations of SpecialStack must be O(1). To implement SpecialStack, you should only use standard Stack data structure and no other data structure like arrays, list, .. etc.

Looking for code review, optimizations, best practices.

    public class StackMinimum<T>{
       /*
        * Composition triumphs over inheritance :)
        */
       private final Stack<T> stack1 = new Stack<T>();
       private final Stack<T> stack2 = new Stack<T>();

       public void push(T item) {
           stack1.push(item);
           if (stack2.isEmpty() || ((Comparable<T>) item).compareTo(stack2.peek()) < 0) {
               stack2.push(item);
           }
        }

        public T pop() {
            T item = stack1.pop();
            if (item.equals(stack2.peek())) {
                stack2.pop();
            }
            return item;
        }

        public T peek() {
            return stack1.peek();
        }

        public int size() {
            return stack1.size();
        }

        public T getMinimum () {
            return stack2.peek();
        }

        public boolean isEmpty() {
            return stack1.isEmpty();
        }
    }

public class StackMinimumTest {

    @Test
    public void test() {
        StackMinimum<Integer> stack1 = new StackMinimum<Integer>();
        stack1.push(1);
        stack1.push(2);
        stack1.push(3);

        assertEquals(1, (int)stack1.getMinimum());

        stack1.push(-1);
        assertEquals(-1, (int)stack1.getMinimum());

        stack1.pop();
        assertEquals(1, (int)stack1.getMinimum());

        while(!stack1.isEmpty()) {
            assertEquals(1, (int)stack1.getMinimum());
            stack1.pop();
        }
    }
}

You have a bug: .getMinimum() loses track if you push the same new minimum value twice.

You should use both inheritance and composition.

Use inheritance for the "main" stack, because your data structure is a stack — one with an extra feature.  That gives you the read-only operations .peek(), .size(), and .isEmpty() for free.

Use composition for the "minimum" stack, as you have currently done.


Merge two list and discarding duplicates I am trying to implement a function that merges two ordered lists into a third (ordered) one, but duplicates have to be discarded (basically the last step of a mergesort).

I think that this code can be generalized to an arbitrary number or lists, by keeping a list of indices, rather than 2 explicit ones.

def merge_no_duplicates(list_1, list_2):
    i = j = 0
    import collections
    result = collections.deque()

    while i < len(list_1) and j < len(list_2):
        if len(result) > 1 and list_1[i] == result[-1]:
            i += 1
            continue
        if len(result) > 1 and list_2[j] == result[-1]:
            j += 1
            continue

        if list_1[i] <= list_2[j]:

            result.append(list_1[i])
            i += 1
        elif list_2[j] < list_1[i]:
            result.append(list_2[j])
            j += 1

    # we still need to consume part of list_2
    if i == len(list_1):
        while j < len(list_2):

            if list_2[j] == result[-1]:
                j += 1
                continue

            result.append(list_2[j])
            j += 1

    # still need to consume part of list_1
    if j == len(list_2):
        while i < len(list_1):

            if list_1[i] == result[-1]:
                i += 1
                continue

            result.append(list_1[i])
            i += 1
    return result

Possible improvements: factor out the repetitive parts, for example by using something similar to this helper function:

check_duplicates(my_list, index):
    if my_list[index] == result[-1]:
        index += 1

Unfortunately this won't work (index is a parameter, and will not influence the behavior of i or j). Is this possible at all?

Pick a good return type

A deque is a Double Ended QUEue. Deques are great for when you have to insert and erase from the front as well as the back. You never need this for your problem - all you ever need is to insert at the end. You just need a normal list.

Generate when you can

Rather than necessarily giving the entire result all at once, it's better to just yield the next element as you go. This is a simple change in your algorithm (just yield x instead of result.append(x)), but could have serious performance implications down the line if you have lots of large iterables. If the caller wants a full list, then can always explicitly write list(merge_no_duplicates(a, b, c)).

Use the standard when you can

There is a function that already merges sorted inputs into a single sorted output: heapq.merge. It will give you duplicates, but that seems like a much better starting point than writing everything from scratch:

def merge_no_duplicates(*iterables):
    last = object()

    for val in heapq.merge(*iterables):
        if val != last:
            last = val
            yield val

If you don't want to use heapq.merge, then you can at least use this framework to separate the "merge sorted iterables" concern from the "remove duplicates" concern.


Tree Node class - deallocation in the destructor Is my destructor correct?  Does it properly deallocate the subtrees?

#ifndef HEADER_GUARD__TREE
#define HEADER_GUARD__TREE

#include <deque>

namespace Sandbox {
    class Node {
    public:
        Node();
        Node( Node* parent );
        virtual ~Node();

        Node* GetFirstChild();
        Node* GetChild( int index );
        Node* GetLastChild();

        void AppendChild( Node* child );
        void AddChild( Node* child ,int index );
        void RemoveChild( int index );

        int  GetIndex() { return _index; }
        void SetIndex( unsigned int index ) { _index = index; };
    protected:

    private:
        Node* _parent;
        int _index;
        std::deque<Node*>* _children;
    };
}

#endif // HEADER_GUARD__TREE

Node::Node(): _parent(NULL), _index(NULL), _children(NULL){
    _children = new std::deque<Node*>;
    _index = 0;
}
Node::Node( Node* parent ): _parent(parent), _index(NULL), _children(NULL){
    _children = new std::deque<Node*>;
    _index = 0;
    _parent->AppendChild( this );
}
Node* Node::GetFirstChild(){
    return _children->front();
}
Node* Node::GetChild( int index ){
    return _children->at(index);
}
Node* Node::GetLastChild() {
    return ( _children->back() );
}

void Node::AppendChild( Node* child ) {
    _children->push_back( child );
    unsigned int index = _children->size();
    child->SetIndex( index );
}
void Node::AddChild( Node* child ,int index ) {
    std::deque<Node*>::iterator i = _children->begin();
    i = i + index;
    _children->insert( i, child );
    child->SetIndex( index );
}
void Node::RemoveChild( int index ) {
    std::deque<Node*>::iterator i = _children->begin();
    i = i + index;
    _children->at(index)->~Node();
    _children->erase( i );
}
Node::~Node() {
    _parent = NULL;
    _index = 0;
    for(int i = 0; i >= _children.size(); i++ ) {
        _children->at(index)->~Node();
    }
    _children->clear();
    delete _children;
}

This is not real C++ code.
This is C written with a couple of classes.

Pointers are dangerous and should rarely be used in code. C++ has moved away from C in this regard over the last decade and all pointers should be either wrapped in a smart pointer or be part of a container structure.

For this a technique called RAII has been introduced. This is the concept that creation and destruction of an object are linked within the objects lifetime thus allowing you to control resources even in the face of exceptions.

There are a couple of ways to fix the code.
The simplest technique is to just use std::unique_ptr (or std::auto_ptr if you are using C++03).

    Node( Node* parent );

Are you passing ownership of the parent?
No. You have a seprat constructor for no parent and ownership is not being passed as a result you should pass the parent by reference (if it has a parent it must not be NULL and must exist).

    Node( Node& parent );

All these methods that return pointer.
Should they?

    Node* GetFirstChild();
    Node* GetChild( int index );
    Node* GetLastChild();

If there is a possibility of NULL then yes, otherwise return a reference.

Here you are adding a child:

    void AppendChild( Node* child );
    void AddChild( Node* child ,int index );

But there is no indication you are passing ownership. The person reading the code has to know how the internals of how your class works. Otherwise they do not know if the pointer passed should be dynamically allocated or not. You need to make this knowledge explicit in the interface. Here I would pass a unique_ptr to indicate that you are passing ownership of the pointer into the node.

OK. Parent is allowed to be a pointer.

    Node* _parent;

You do not own the object and it is possible to be NULL. There is no responsibility for this node to clean up its parent.

_children should not be a pointer. It should be an object. Thus allows automatic cleanup and no requirement to manually create it. Also this simplifies the copy construction and assignment operator (that you have currently failed to define).

Secondly the container should not be holding pointer. Here you have an option either it should be a container of smart pointers, or it should be a container designed specifically to hold ownership of pointers (so that it can be done safely).

    // Option 1:
    std::deque<std::uniqur_ptr<Node>> _children;

    // Option 2:
    boost::ptr_deque<Node>            _children;

Personally I prefer option 2. As the contain provides access to the pointers as if they were normal objects. This makes writing functors and the interacting with the standard algorithms much easier.

Also I would question the use of deque over vector but that is a minor detail.
    };
}

Secondary Comments:

Do initialization in the initialization list. You sort of try to NULL everything in the initialization list. It is best to actually initialize them and have an empty body.

Node::Node(): _parent(NULL), _index(NULL),             _children(NULL){
                      ^^^^ OK       ^^^^ Questionable  ^^^^^^^^ Should be an object

Node::Node( Node* parent ): _parent(parent), _index(NULL),     _children(NULL){
                                    ^^^^^^ OK       ^^^^ Questionable

In the constructor that takes a parent you don't check your input for NULL

// If somebody passes a NULL parent then this is no good.
_parent->AppendChild( this );

The use of your functions should be consistent.

Getting the first and last children can be badly defined if the node has no children. But the generic GetChild() function will throw an exception if you try and retrieve a child from an invalid index. These two different behaviors makes using your code hard to be consistent. I would change these so that they all or none throw an exception.

Node* Node::GetFirstChild(){
    return _children->front();
}
Node* Node::GetLastChild() {
    return ( _children->back() );
}
Node* Node::GetChild( int index ){
    return _children->at(index);
}

Manually calling the destructor is wrong here:

_children->at(index)->~Node();

Unless you allocated the memory in some other way the used 'Placement New' you should not be calling the destructor manually. But nothing in your interface indicates that. So this should be a normal call to delete.

delete _children->at(index);

When inserting a child into the middle is it intentional to only set the index of the inserted child?

void Node::AddChild( Node* child ,int index ) {
    std::deque<Node*>::iterator i = _children->begin();
    i = i + index;
    _children->insert( i, child );

    // You set this node
    // But what about the nodes from index+x to the end.
    // Currently their internal index is unchanged.
    //     This could be intentional but it is hard to tell without a comment.
    child->SetIndex( index );
}

I would try something like this:

#ifndef HEADER_GUARD__TREE
#define HEADER_GUARD__TREE

#include <boost/ptr_container/ptr_deque.hpp>

namespace Sandbox {
    class Node;
    class Node {
        public:
            Node();
            // Removed the constructor that passes a parent
            // this has problems when you need to define who owns the object.
            // Thus safer to just remove the interface.
            virtual ~Node() {}

            Node& GetFirstChild();
            Node& GetChild( int index );
            Node& GetLastChild();

            Node& AppendChild(std::auto_ptr<Node> child );
            Node& AddChild(std::auto_ptr<Node> child ,int index );
            void RemoveChild( int index );

            int  GetIndex() { return _index; }
            void SetIndex( unsigned int index ) { _index = index; };
        protected:

        private:
            Node* _parent;
            int _index;
            boost::ptr_deque<Node> _children;
    };
}

#endif // HEADER_GUARD__TREE

using namespace Sandbox;

Node::Node()
    : _parent(NULL)
    , _index(0)
{
}

// All node child node access fails with an exception if the child is not there.
// Thus we always return a reference to the node
Node& Node::GetFirstChild()
{
    return _children.at(0);
}

Node& Node::GetChild( int index )
{
    return _children.at(index);
}

Node& Node::GetLastChild()
{
    return _children.at(_children.size() - 1);
}

// Inserting a node will return a reference to the node after insertion.
// The nodes are passed by std::auto_ptr to indicate transfer or ownership into the tree
// Note an empty auto_ptr inserted into the boost_deque will result in an exception so
// the node must have been created.
Node& Node::AppendChild(std::auto_ptr<Node> child )
{
    _children.push_back( child );
    unsigned int index = _children.size();

    _children[index].SetIndex(index);
    return _children[index];
}

Node& Node::AddChild(std::auto_ptr<Node> child ,int index )
{
    boost::ptr_deque<Node>::iterator i = _children.begin();
    i = i + index;

    _children.insert( i, child );
    _children[index].SetIndex( index );
    return _children[index];
}

void Node::RemoveChild( int index )
{
    boost::ptr_deque<Node>::iterator i = _children.begin();
    i = i + index;

    _children.erase( i );
}
// Destruction is done automatically no code here.


Balanced smileys check algorithm (part 3) This is a follow-up for here, for bug fixes and applied advice.

  Problem

  Your friend John uses a lot of emoticons when you talk to him on
  Messenger. In addition to being a person who likes to express himself
  through emoticons, he hates unbalanced parenthesis so much that it
  makes him go :(

  Sometimes he puts emoticons within parentheses, and you find it hard
  to tell if a parenthesis really is a parenthesis or part of an
  emoticon.

  A message has balanced parentheses if it consists of one of the
  following:

  An empty string ""
  One or more of the following characters: 'a' to 'z', ' ' (a space) or ':' (a colon)
  An open parenthesis '(', followed by a message with balanced parentheses, followed by a close parenthesis ')'.
  A message with balanced parentheses followed by another message with balanced parentheses.
  A smiley face ":)" or a frowny face ":("
  Write a program that determines if there is a way to interpret his message while leaving the parentheses balanced.

I'm working on this balanced smileys checking algorithm, and my current solution is very naive, with just two rules:

At any point, the number of ) (close) should be less than the number of ( (open) + number of :( (frown)
At the end, the number of ( (open) should be less than the number of ) (close) and :) (smile)

I'm wondering if there are any bugs in my checking logic. Any advice on algorithm time complexity improvement or code style advice is highly appreciated as well.

def check_balance(source):
    left = 0
    right = 0
    smile = 0 # :)
    frown = 0 # :(
    pre = ''
    for c in source:
        if c == ')':
            if pre == ':':
                smile += 1
            else:
                right += 1
                if right > left + frown:
                    return False
        elif c == '(':
            if pre == ':':
                frown += 1
            else:
                left += 1
        pre = c
    if left > right + smile:
        return False
    return True

if __name__ == "__main__":
    raw_smile_string = 'abc(b:)cdef(:()' # true
    raw_smile_string = 'a)' # false
    raw_smile_string = ')('  # false
    print check_balance(raw_smile_string)

Correctness

I suggest you add a test case for :)(.  It should be False, but is not.

Testability

Consider adding assert tests for ease of testing and evaluation.  Perhaps something like

def verify(test_input, expected_result):
    actual_result = check_balance(test_input)
    assert actual_result == expected_result, "{} expected {} failed".format(test_input, expected_result)
    print(test_input, actual_result)

if __name__ == "__main__":
    verify(':)(', False)
    verify(':()', True)
    verify('abc(b:)cdef(:()', True)
    verify('a)', False)
    verify(')(' , False)

Besides, if provided, it is easier to refactor and propose alternatives when providing feedback. :)

Clarity

A key part of the challenge is that smiles and frowns are optionally allowed to balance parens.  This is an important not immediately obvious point that is worth documenting in the code.

Though somewhat minor for this simple example, I prefer not to repeatedly test for pre == ':' since ":" is the only character you care about.  Instead I would opt for something like eyes_flag which can be set at the end of loop as eyes_flag = c == ':'.

Though the criteria may change after fixing the implementation, expressions like this

if left > right + smile:
    return False
return True

can be simplified to

return left <= right + smile

Naming

This is a matter of opinion, but for me smiley and frown require too much effort to map to open and close parens.  I prefer calling them optional open parens (opt_open) and optional close parens (opt_close).


Formatting a type's name I'm writing a code generation library, I've exposed a couple of methods and of course in order to define a member you need to specify it's type, the easiest way is to use typeof(Type), which I later format so that it it's compilable and I can insert it as a code chunk e.g. System.Int32, which for non-generic types is basically concatenating the namespace and the type's name.

That works fine but generic types have the backtick followed by the number of arguments, so I needed a method to format all the types, so that they are always compilable e.g. System.Collection.Generics.List<System.Int32>.

public static class TypeExtensions
{
    public static string FormatTypeName(this Type type)
    {
        if(type == null) { throw new ArgumentNullException(nameof(type)); }

        if (type.IsGenericType)
        {
            var definedArguments = type.GetGenericArguments();
            var genericIdentifier = $"`{definedArguments.Length}";

            if (definedArguments.First().FullName == null)
            {
                var genericBraces = "<".PadRight(definedArguments.Length, ',') + ">";
                return $"{type.Namespace}.{type.Name.Replace(genericIdentifier, genericBraces)}";
            }

            var formattedTypes = definedArguments.Select(definedArgument => definedArgument.FormatTypeName());

            var newGenericArguments = $"<{string.Join(", ", formattedTypes)}>";
            return $"{type.Namespace}.{type.Name.Replace(genericIdentifier, newGenericArguments, StringComparison.InvariantCultureIgnoreCase)}";
        }

        return $"{type.Namespace}.{type.Name}";
    }
}

It supports indefinite amount of nested and linear generic arguments due to the recursion that is in-place.

The way it works is pretty straight forward, there are 2 possible incomes, we have no arguments defined e.g. typeof(Dictionary<,>) or all of the arguments are defined e.g typeof(Dictionary<int, int>).

Since the C# compiler doesn't allow partially defined arguments we can simply check if the first of the arguments has a FullName (note that Name is the name pf the generic argument not the type behind it), if it does we simply concatenate all of the arguments and surround them by <>. if the first argument doesn't have value then it's empty and we just leave the braces with commas separating the arguments.

Since this will be used in a code generation tool, performance is not crucial, but always welcome as long as it doesn't damage the readability and maintainability of the code, which I consider more important in this case.

However since it will be used in a code generation tool, I'm more worried about whether I've missed some edge cases.

You can let CSharpCodeProvider format Type names:

public static string FormatTypeName(this Type type)
{
    using (var c = new CSharpCodeProvider()) {
        var r = new CodeTypeReference(type);
        return c.GetTypeOutput(r);
    }
}


Trim NA values from left and right of Rcpp::NumericVector I receive data with NA values. I would like to trim off the leading and trailing NA values, but to leave the interior NA values in place.

I've written the below in Rcpp, but it is very much a hand rolled solution -- and i'm alive to the fact that i'm going to make mistakes doing things this way.

What is the idiomatic cpp / Rcpp (sugar?) way of doing  something like this?

#include<Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
Rcpp::NumericVector trimNA(Rcpp::NumericVector X) {

    // find first and last non-NA value

    double * F = X.begin();
    double * pBegin = X.begin();
    double * pEnd = X.end();
    double * L = X.end() - 1; // note you must decrement by one as .end() is AFTER

    // look for the values
    while( F < pEnd && NumericVector::is_na(*F)) F++;
    while( L > pBegin && NumericVector::is_na(*L)) L--;

    // create trimmed X vector
    Rcpp::NumericVector Xtrim(L - F + 1);

    for( int i = 0; i < Xtrim.size(); i++) {
        Xtrim[i] = *(F + i);
    }

    return Xtrim;
}

/*** R

x <- c(NA, NA, NA, 1:4, NA, 6:8, NA, 10, NA, NA, NA)
trimNA(x)
*/

Code Review

Code is good, except the naming. There are some pieces which could be improved

Use auto for iterators, as exact iterator type is not important, but it's properties are. Iterator properties are usually inherited from container it came from.

Use better constructor, there seems to be one which accepts pair of iterators, and in this case, pair of pointers denoting contiguous range.

Better (?) approach

The problem seems to be somewhat specific case of trimming in general. Note that trimming from the right and trimming from the left are symmetric, and std::reverse_iterator "normalizes" trimming from right (great improvements thanks to @Toby).

template <typename BidirIterator,
          typename Predicate>
auto trim(BidirIterator first, BidirIterator last, Predicate predicate) {
    auto left_edge = std::find_if_not(first, last, predicate);
    auto right_edge = std::find_if_not(std::reverse_iterator(last),
                                       std::reverse_iterator(left_edge),
                                       predicate).base();

    return std::pair{left_edge, right_edge};
}

When reversing a range, first becomes last, and vice versa, that is why std::reverse_iterator arguments were swapped. There is also C++17 feature in play (template class argument deduction).

Full code:

#include <utility>
#include <iterator>
#include <algorithm>

template <typename BidirIterator,
          typename Predicate>
auto trim(BidirIterator first, BidirIterator last, Predicate predicate) {
    auto left_edge = std::find_if_not(first, last, predicate);
    auto right_edge = std::find_if_not(std::reverse_iterator(last),
                                       std::reverse_iterator(left_edge),
                                       predicate).base();

    return std::pair{left_edge, right_edge};
}

#include <vector>
#include <stdexcept>

int main() {
    std::vector<int> initial_values{2, 2, 3, 4, 5, 6};
    auto predicate = [](int x) { return x % 2 == 0; };
    auto [new_first, new_last] = trim(initial_values.begin(),
                                      initial_values.end(),
                                      predicate);
    std::vector<int> resulting_values(new_first, new_last);
    std::vector<int> correct_result{3, 4, 5};
    if (resulting_values != correct_result) {
        throw std::logic_error("incorrect trimming occured");
    }
}

Demo on Wandbox.

To make trimming a little bit easier to use, one might write something like this:

template <typename Container, typename Predicate>
Container trim_copy(Container&& container, Predicate predicate) {
    auto [new_first, new_last] = trim(container.begin(), container.end(),
                                      predicate);
    return Container(new_first, new_last);
}

and pass a lambda calling the NumericVector::is_na:

auto predicate = [](auto x) { return NumericVector::is_na(x); };

or may be pointer to the function directly.


Custom Logging Class: Efficiency I wrote a logging class for several of my applications, but I have noticed that it is fairly in-efficient. Can anyone suggest improvements?

What I am looking for:

Pointers on improving efficiency
Tips for lowering the code-footprint
Suggested functional improvements

Please be as nit-picky as possible, I'd like to make this class as robust as possible.

import java.io.FileInputStream;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Properties;

class FunctionLogging
{
    private String path;
    private boolean appendToFile = false;

    public FunctionLogging(String file_path, int logLevel, boolean append_value)
    {
        path = file_path;   //used as a placeholder variable for the output path. Can be hard-coded but is preferredly configurable from properties file
        appendToFile = append_value; //Set as true when called in each class used in. true appends line to the end of the file, false overwrites file contents
    }
    public void writeToFile(int logType, String textLine) throws IOException
    {
        FileInputStream propFile = new FileInputStream("config.ini");
        Properties config = new Properties(System.getProperties());
        config.load(propFile);
        String tempLev = config.getProperty("loggingLevel");
        int logLevel = Integer.parseInt(tempLev);
        if(logType<=logLevel)
        {
            //This section is devoted to creating a timestamp of the instance when the output string is created.
            Date date = new Date();
            SimpleDateFormat format = new SimpleDateFormat();
            format = new SimpleDateFormat("dd-MM-yyyy|HH:mm:ss:SSSSSS|");
            String timeStamp = format.format(date);
            SimpleDateFormat app = new SimpleDateFormat();
            app = new SimpleDateFormat("dd-MM-yyyy");
            String dateApp = app.format(date);
            int id = 0;

            String[] messages = new String[4];
            messages[0]="FATAL";
            messages[1]="WARN";
            messages[2]="NOTICE";
            messages[3]="DEBUG";
            FileWriter logWriter = new FileWriter(path+dateApp+".log", appendToFile);
            PrintWriter loggerLines = new PrintWriter(logWriter);

            loggerLines.println(timeStamp+id+++"|"+messages[logType]+"|"+textLine);
            loggerLines.close();
        }
    }
}

Efficiency

the biggest improvement will be to not open the config file each time you log something, but only once when instantiating the logger.
you could also open the log file only once, but it might not be a good idea to have it open for the whole lifetime of the program.
create only one instance of the two SimpleDateFormat objects and store it in a class level field instead of creating them each time.
also create your messages array only once as a field.

Robustness

use an enum instead of string for your warning levels
you are never closing your propFile stream.

Readability

follow the standard Java indent style: curly brackets go on the same line as the opening statement.
use more spaces (around <=, =, etc). This point and the previous one can be fixed by any IDE.
don't do this: timeStamp+id+++"|".
use camelCase for all variable names, don't mix it with snake_case except for final static fields.
use JavaDoc style comments.
remove variables that are only used in one place.
use try with resources to save the close statement.
choose better variable names. app would be clearer as shortDateFormat, format as extendedDateFormat, dateApp as logFileName, appendValue as appendToFile, logLevel as maximalLogLevel (should this maybe be minimal?), logType as logLevel.

There are probably more improvements, but with this your code already looks a bit nicer:

public class FunctionLogging {

    private String path;
    private boolean appendToFile = false;
    private int logLevel;
    private String[] messages = new String[] {"FATAL", "WARNING", "NOTICE", "DEBUG"};

    private SimpleDateFormat format = new SimpleDateFormat("dd-MM-yyyy|HH:mm:ss:SSSSSS|");
    private SimpleDateFormat app = new SimpleDateFormat("dd-MM-yyyy");

    /**
     *
     * @param filePath used as a placeholder variable for the output path. Can be hard-coded but is preferredly configurable from properties file
     * @param logLevel set as true when called in each class used in. true appends line to the end of the file, false overwrites file contents
     * @param append_value TODO
     */
    public FunctionLogging(String filePath, int logLevel, boolean appendValue) throws IOException {
        path = filePath;
        appendToFile = appendValue;

        loadConfig();
    }

    private void loadConfig() throws IOException {
        try (FileInputStream propFile = new FileInputStream("config.ini")) {
            Properties config = new Properties(System.getProperties());
            config.load(propFile);
            logLevel = Integer.parseInt(config.getProperty("loggingLevel"));
        }
    }

    public void writeToFile(int logType, String textLine) throws IOException {
        if (logType <= logLevel) {
            Date date = new Date();
            String timeStamp = format.format(date);
            String dateApp = app.format(date);

            FileWriter logWriter = new FileWriter(path + dateApp + ".log", appendToFile);
            try (PrintWriter loggerLines = new PrintWriter(logWriter)) {
                loggerLines.println(timeStamp + "1|" + messages[logType] + "|" + textLine);
            }
        }
    }
}

Or with enums:

public class FunctionLogging {

    public enum Level {

        FATAL("FATAL", 1),
        WARNING("WARNING", 2),
        NOTICE("NOTICE", 3),
        DEBUG("DEBUG", 4);

        private final String message;
        private final int level;

        private Level(String message, int level) {
            this.message = message;
            this.level = level;
        }

        @Override
        public String toString() {
            return message;
        }

        public int getLevel() {
            return level;
        }
    }

    // [...]

    public void writeToFile(Level level, String textLine) throws IOException {
        if (level.getLevel() <= logLevel) {
            // [...]
            try (PrintWriter loggerLines = new PrintWriter(logWriter)) {
                loggerLines.println(timeStamp + "1|" + level.toString() + "|" + textLine);
            }
        }
    }

    // [...]

// usage:

writeToFile(FunctionLogging.Level.DEBUG, "something happened");


Mapping interface between POJOs and DTOs Following up on this question DTO interfaces for implementing web service integration I came up with this implementation.

Data Transfer Object interface following a java.util.Map protocol. The implementation uses Guava's ForwardingMap.

public interface Dto<K, V> extends Map<K, V> {}

public class DtoImpl<K, V> extends ForwardingMap<K, V> implements Dto<K, V> {

    private final Map<K, V> delegate;

    @Override
    protected Map<K, V> delegate() {
        return this.delegate;
    }

    private DtoImpl(final Map<K, V> map) {
        this.delegate = map;
    }

    public static <K, V> Dto<K, V> of(final Map<K, V> map) {
        return new DtoImpl<K, V>(map);
    }
}

Mapper interface between a type T and a Dto.

public interface Mapper<T> {

    public <K, V> Dto<K, V> toDto(T obj);

    public <K, V> T fromDto(Dto<K, V> dto, Class<T> clazz);
}

public class MapperImpl<T> implements Mapper<T> {

    public <K, V> Dto<K, V> toDto(T obj) {
        ObjectMapper objectMapper = new ObjectMapper();
        Map<K, V> map = objectMapper.convertValue(obj, Map.class);
        return DtoImpl.of(map);
    }

    public <K, V> T fromDto(Dto<K, V> dto, Class<T> clazz) {
        ObjectMapper objectMapper = new ObjectMapper();
        T obj = objectMapper.convertValue(dto, clazz);
        return obj;
    }

}

Comments and improvements on this implementation are welcome!
Is it worth it to define the Mapper interface with something like interface Mapper<T, K, V>? Ideally I would like to say interface Mapper<T, S extends Dto<K, V>> but this is not possible or is it?

public interface Dto<K, V> extends Map<K, V> {} why don't you use simply a Map? Again, it is the same discussion related to marker interfaces we already had in your original question.

I'd define Mapper in the following way:

public interface Mapper<T, K, V> {
    public Dto<K, V> toDto(T obj);
    public T fromDto(Dto<K, V> dto);
}

I don't understand the reason why you need MapperImpl. What you need are concrete classes imlementing your interface for your specific types.

Imagine you want to map a Date to a Map<String,Integer>. You need something like this:

class DateMapper implements Mapper<Date, String, Integer>
{
    public Dto<String, Integer> toDto(Date date)
    {
        Dto<String, Integer> dto = new Dto<String, Integer>();
        dto.put("Day", date.Day());
        dto.put("Month", date.Month());
        dto.put("Year", date.Year());
    }

    public Date fromDto(Dto<String, Integer> dto)
    {
        return new Date(dto.get("Year"), dto.get("Month"), dto.get("Day"));
    }
}

I think that at this point you might be thinking at how you could obtain an instance of DateMapper. I think you should use some sort of factory or build it using reflection depending on what is better in your use case.


Get neighbour sides of a Rubics cube I'm writing 2x2x2 rubics cube simulator. In my code I have a concept of Face which is individual side of the cube, and State which is agregate of all six sides. When I create new State, in addition to creating faces I need to set all neighbour faces for a given face. Currently I do this in manual way, as in:

faces[Side.Top][Direction.Up] = faces[Side.Back];
faces[Side.Top][Direction.Down] = faces[Side.Front];
faces[Side.Top][Direction.Left] = faces[Side.Left];
faces[Side.Top][Direction.Right] = faces[Side.Right];

faces[Side.Bottom][Direction.Up] = faces[Side.Front];
faces[Side.Bottom][Direction.Down] = faces[Side.Back];
faces[Side.Bottom][Direction.Left] = faces[Side.Left];
faces[Side.Bottom][Direction.Right] = faces[Side.Right];
...

However I feel theres more algorithmical approach which I can't figure out. Does anyone know better way to do this?

Here's full code I've got so far if anyone's interested.

One more algorithmic approach could be to use a different object model more closely relating to the actual cube. That is instead of keeping the state of faces, keep the state of each part of the cube, i.e.The 8 corner pieces of a 2x2x2 cube, and then keep track of the state of each of these, and then have methods to handle the cube and to get the different faces based on the pieces.

If you do this kind of modelling, then your code would also expand easily to larger cube dimensions, as the general movement methods would/should be generic and be applicable to any dimension of the cube.

Edit: Added pseudo code

As always when approaching a problem you need to visualise and simplify into something you are able to comprehend and code. Some givens for this particular problem:

The cube is 3D, but at any given time you are only revolving one plane/face of the cube
When revolving any given face, pieces keep their internal place in that face, albeit the global position has changed, i.e. the corner piece remains the corner piece, but the top-left corner piece could become the top-right corner piece
All pieces in a plane is related and will move as one
For a 2x2x2 cube, use positions from [0, 0, 0] through [1, 1, 1], where [0, 0, 0] is the bottommost, leftmost, frontfacing piece

How does this help us? Well, one can define and code the 2d operations available, and apply them to the cube, which in turn applies them to each piece in turn. Lets define the operations as Rot + Plane  + Direction, where Plane can be X, Y, and Z, and direction is either CW - Clockwise or CCW - CounterClockWise. Operations then become: RotXCW, RotXCCW, RotYCW, and so on. But we need to know which of n=2 planes we're rotating, so lets pass that as an argument.

In order to execute the operation RotXCW(1) on the cube, that is rotate the face to the right clockwise, we need to loop through all pieces having x==1, and call RotXCW on each piece:

Change piece orientation correctly
Let the piece calculate its own position:

        x = prevX
        y = ( prevZ == 0 ) ?    0  : (n-1)
        z = ( prevY == 0 ) ? (n-1) :    0

  To find the new position I wrote down all combinations based on previous position, and looked for the easiest way to represent this acknowledging that the coordinate of a corner piece is always 0 or n-1. Here is the table for rotating clockwise:

  Old Z   Old Y     New Z   New Y
    0      0     >     1      0
    1      0     >     1      1
    1      1     >     0      1
    0      1     >     0      0

  This table leads to implementation given that the new Yis the previous Z, and the new Z is the opposite of the previous Y

In order to implement this you need a Cube class of dimension n, and this concists of Piece's which has a position, and an orientation. Both classes needs to have rotational methods, and in addition you need a method to display the face, and rotate the face of a given piece.

Hopefully this should give you enough to start coding. It might seem heavy, but isolating and diving the concern into handling single pieces will, in my experience, be well worhth it. Now you can call the rotation operation on the cube, and the single concern of that method is to call rotate on the pieces.


Merge sort in Python I've written a implementation of a merge sort in Python. It seems correct and I have tested it with this:

l = list(range(1000))
random.shuffle(l) # random.shuffle is in-place
mergesort(l) == list(range(1000)) # returns True

# Stable sort
cards = list(itertools.product(range(4), range(13)))
random.shuffle(cards)
cards = mergesort(cards, key=lambda x: x[1]) # Sort by rank
cards = mergesort(cards, key=lambda x: x[0]) # Sort by suit
cards == list(itertools.product(range(4), range(13))) # returns True

I've also tested its performance, comparing it to sorted and the merge sort implementation in rosetta code:

rl = list(range(100))
random.shuffle(rl)

%timeit sorted(rl)
# 100000 loops, best of 3: 11.3 µs per loop

%timeit mergesort(rl) # My code
# 1000 loops, best of 3: 376 µs per loop

%timeit rcmerge_sort(rl) # From rosetta code
# 1000 loops, best of 3: 350 µs per loop

I'm looking for any suggestions on how to improve this code. I suspect there is a better way to do the mergelist function, particularly in how I tried to avoid code duplication like:

if top_a <= top_b:
    nl.append(top_a)
    try:
        top_a = next(it_a)
    except:
        ...
else:
    # duplicates above code

In my code I placed the iterators and first values in a list, then use the variable k as index, but this leads to hacks like abs(k-1) and placing magic numbers 0 and 1 in the code.

def mergesort(l, key=None):
    # Split the list into sublists of length 1
    sublists = [[x] for x in l]

    while len(sublists) > 1:
        new_sublists = []

        # Create a generator that yields two sublists at a time
        sublists_pairs = ((sublists[2*x], sublists[2*x+1])
                          for x in range(len(sublists)//2))

        for a, b in sublists_pairs:
            new_sublists.append(mergelist(a, b, key))

        # If the length is odd, then there is one sublist that is not merged
        if len(sublists) % 2 != 0:
            new_sublists.append(sublists[-1])

        sublists = new_sublists

    return new_sublists[0]

def mergelist(a, b, key=None):

    nl = []

    # Iterators that yield values from a and b
    its = iter(a), iter(b)

    # The top of both lists
    tops = [next(it) for it in its]

    while True:
        # Determine the iterator that the next element should be taken from
        if key:
            k = 0 if key(tops[0]) <= key(tops[1]) else 1
        else:
            k = 0 if tops[0] <= tops[1] else 1
        nl.append(tops[k])

        try:
            # Update the top of the iterator
            tops[k] = next(its[k])
        except StopIteration:
            # Unless the iterator is empty, in which case get the rest of
            # the values from the other iterator
            # abs(k-1) is similar to (0 if k == 1 else 1)
            nl.append(tops[abs(k-1)])
            for e in its[abs(k-1)]:
                nl.append(e)

            return nl

The while loop can be written as follows (although I think it does not speed things up):

while True:
    # Determine the iterator that the next element should be taken from
    if key:
        low,high = (0,1) if key(tops[0]) <= key(tops[1]) else (1,0)
    else:
        low,high = (0,1) if tops[0] <= tops[1] else (1,0)
    nl.append(tops[low])

    # Update the top of the iterator
    tops[low] = next(its[low], None)
    if not tops[low]:
        # Unless the iterator is empty, in which case get the rest of
        # the values from the other iterator
        nl.append(tops[high])
        nl.extend(its[high])

        return nl


Observing PEP maximum line size, particularly when formatting localized timestamps I have this function that I use to publish a message but I need to do this long conversion for time and date. Maybe there's a better way to convert it to make smaller. Otherwise I'm stuck with this long line. I guess I could use slashes, is guidelines on readability for using slashing or is arbitrary.

  SMS.publish('/jobs',{
                           'batchdate': job.get('batchdate'),
                           'job': job.get('name'),
                            'Script execution date (EST)':pytz.utc.localize(datetime.datetime.utcfromtimestamp(time.time())).astimezone(pytz.timezone('US/Eastern')).strftime('%Y-%m-%d %H:%M:%S'),
                             'Start time(EST)':pytz.utc.localize(datetime.datetime.utcfromtimestamp(float(job.get('starttime')))).astimezone(pytz.timezone('US/Eastern')).strftime('%Y-%m-%d %H:%M:%S'),
                            'Current runtime': change_format_num( cur_runtime),
                            'Average runtime': change_format_num(avg_time)
                                     })

First of all, notice the duplicated parts in these long expressions:

pytz.utc.localize(
    datetime.datetime.utcfromtimestamp(time.time())).astimezone(
    pytz.timezone('US/Eastern')).strftime('%Y-%m-%d %H:%M:%S')

pytz.utc.localize(
    datetime.datetime.utcfromtimestamp(float(job.get('starttime')))).astimezone(
    pytz.timezone('US/Eastern')).strftime('%Y-%m-%d %H:%M:%S')

Having duplicated code segments is both inconvenient and error-prone.
If you want to / need to change anything later,
you have to remember to do that in all places,
and it's easy to make a mistake.

So it will be good to extract this to a helper function:

def get_formatted_time(val):
    return pytz.utc.localize(
        datetime.datetime.utcfromtimestamp(val)).astimezone(
        pytz.timezone('US/Eastern')).strftime('%Y-%m-%d %H:%M:%S')

You can break lines within parentheses.
Now this looks fairly tidy, and it's PEP8 compliant.

Using this helper you can rewrite the original code becomes simpler:

SMS.publish('/jobs', {
    'batchdate': job.get('batchdate'),
    'job': job.get('name'),
    'Script execution date (EST)': get_formatted_time(time.time()),
    'Start time(EST)': get_formatted_time(float(job.get('starttime'))),
    'Current runtime': change_format_num(cur_runtime),
    'Average runtime': change_format_num(avg_time)
})


Numpad number layout Think of the numpad number layout.

hvd = horizontal, vertical or diagonal

coord = 1,2 or 3 if horizontal or vertical and 1 or 2 if diagonal (1 is up, 2 is down)...

hello('v', 2) - Asking for the second vertical column etc

def hello(hvd, coord):
    if hvd == 'h':
        print "horizontal it is"
        if coord == 1:
            print "789"
        elif coord == 2:
            print "456"
        elif coord == 3:
            print "789"
        else:
            print "coord is wrong"
    elif hvd == 'v':
        print "vertical it is"
        if coord == 1:
            print "741"
        elif coord == 2:
            print "852"
        elif coord == 3:
            print "963"
        else:
            print "coord is wrong"
    elif hvd == 'd':
        print "diagonal it is"
        if coord == 1:
            print "159"
        elif coord == 2:
            print "753"
        else:
            print "coord is wrong"
    else:
        print "hvd is wrong"

I just want a critique on this function. I am interested in knowing if there is a nicer way to do what I am doing and if there is anything wrong with my code.

It seems quite hard to look at especially since it contains place holder statements and the actual statements I want might be 10 lines long each... Would be unwieldy surely?

A nicer way to do the same would be to use a dictionary:

def hello(hvd, coord):
    d = {'h': ["789", "456", "123"],
         'v': ["741", "852", "963"],
         'd': ["159", "753"]}
    try:
        print d[hvd][coord-1]
    except IndexError:
        print "coord is wrong"
    except KeyError:
        print "hvd is wrong"


Binary search algorithm in Clojure I'm absolutely new to Clojure (started learning it yesterday). After I went through some tutorials and checked out the basics I implemented the following Binary search algorithm:

(defn log-search
    [elements, elem-to-find]
        (loop [left 0
            right (- (count elements) 1)]
          (when (<= left right)
          (def m (int (Math/floor (/ (+ left right) 2))))
          (def actual (nth elements m))
              (cond
                  (= actual elem-to-find) m
                  (< actual elem-to-find) (recur (+ m 1) right)
                  (> actual elem-to-find) (recur left (- m 1))))))

The followings are intentional in the implementation:

returning nil on empty sequence
I return nil if the element is not present
I do not care if (+ left right) overflows on very big sequence (I'm focusing only on the language features)

My questions:

Is there any Clojure (or funcional programming) antipattern in the
code?
What would be a more functional (or Clojure) approach?

Any other suggestions are more than welcomed.

Thank you.

Your implementation is pretty similar to this one: https://programming-pages.com/2012/01/31/binary-search-in-clojure/

I think the overall algorithm is OK.
You should avoid using def inside function - that should be used only for global top-level vars -> use let instead.
Also, the code formatting is incorrect - use 2 spaces and proper indentation:

(defn log-search
  [elements elem-to-find]
  (loop [left 0
         right (dec (count elements))]
    (when (<= left right)
      (let [m (int (Math/floor (/ (+ left right) 2)))
            actual (nth elements m)]
        (cond
          (= actual elem-to-find) m
          (< actual elem-to-find) (recur (+ m 1) right)
          (> actual elem-to-find) (recur left (- m 1)))))))


Toggle between two options in an object? I have just written this code for toggling between two possible states of an object.

// This can't change, and only ever has two states
const MODES = {
  ON: 'ON',
  OFF: 'OFF'
}

// this is my method
const toggleMode = currentMode => {
  const modes = Object.values(MODES);
  const newMode = modes.find(mode => mode !== currentMode);
  return newMode
};

// toggle state
const firstState = MODES.ON;
console.log(firstState);
const secondState = toggleMode(firstState);
console.log(secondState);
const thirdState = toggleMode(secondState)
console.log(thirdState)

Is this a good way of toggling between the two states? I'm concerned it's less readable than the verbose if one state, then pick the other, else pick the current state. But this way also means that so long as we know there's only two states, then nothing needs to be hardcoded in the function doing the toggling.

You currently only print the currently selected mode to the console, but you probably will use this mode for other purposes, such as the business logic of your application. To that end, I suggest splitting the toggling logic from the presentation and use booleans to store the value which you can easily use for branching depending on the currently selected mode.

// Logic: Define the two modes and toggling logic
const MODES = { ON: true, OFF: false }
const toggleMode = currentMode => !currentMode;

// Presentation: Display a selected mode
const displayMode = currentMode => currentMode ? 'ON' : 'OFF';

// Demo
const firstState = MODES.ON
console.log(displayMode(firstState));
const secondState = toggleMode(firstState);
console.log(displayMode(secondState));
const thirdState = toggleMode(secondState);
console.log(displayMode(thirdState));


Simple calculator that sums numbers This is a calculator that only sums numbers:

module Data.Calculator where

data Expr = Plus Expr Expr | Value Int

evaluate :: Expr -> Int
evaluate (Value a) = a
evaluate (Plus (Value a) (Value b)) = a + b
evaluate (Plus (Plus left right) (Value b)) = (evaluate left) + (evaluate right) + b
evaluate (Plus (Value a) (Plus left right)) = a + (evaluate left) + (evaluate right)
evaluate (Plus a@(Plus left right) b@(Plus left' right')) = (evaluate a) + (evaluate b)

The evaluate function seems way to verbose. How do I make this better?

As a general rule, when you are trying to define a recursive function that operates on a data type like this:

data Expr = Plus Expr Expr | Value Int

you want to first try to define your function using one pattern per constructor:  one pattern for Plus and another pattern for Value.  For some functions, you may find that this isn't sufficient, and then you will have to add additional patterns, but you'll be surprised how many useful functions can be written using this "one pattern per constructor" rule.

So, you want a pattern that can match all Value x expressions, and you already have that:

evaluate (Value a) = a

If you want a second pattern that can match ALL Plus e1 e2 expressions, then you can't write something like this:

evaluate (Plus (Value a) (Value b)) = ...

because that will only match some Plus expressions (namely those that are adding two Value expressions).  If you want to match them all, you need to start with:

evaluate (Plus left right) = ...

Now, on the right-hand side, left and right are expressions, and the only thing we can do with them is evaluate them to integers, and then it becomes pretty obvious that you want to write:

evaluate (Plus left right) = evaluate left + evaluate right

You basically already figured this out, but you did it inside special cases like the pattern Plus (Plus left right) (Value b) instead of realizing that you could do it in the general Plus left right case.

And that's it!  The full definition is:

evaluate :: Expr -> Int
evaluate (Value a) = a
evaluate (Plus left right) = evaluate left + evaluate right

and you'll find it works perfectly on all expressions.  For more complicated expressions, the recursion automatically breaks it down:

evaluate (Plus (Value 1) (Plus (Value 2) (Value 3))
-- apply second pattern w/ left=(Value 1) and right=(Plus (Value 2) (Value 3))
=== evaluate (Value 1) + evaluate (Plus (Value 2) (Value 3))
-- apply first pattern w/ a=1
=== 1 + evaluate (Plus (Value 2) (Value 3))
-- apply second pattern w/ left=(Value 2) and right=(Value 3)
=== 1 + (evaluate (Value 2) + evaluate (Value 3))
-- apply first pattern w/ a=2
=== 1 + (2 + evaluate (Value 3))
-- apply first pattern w/ a=3
=== 1 + (2 + 3)
=== 6


Distinct List of attribute values from XML, using a pre-defined set of attribute names I have an XML stream that looks like the following (attribute and node quantity , and node depth reduced for demonstration purposes.)

<itinerary>
  <organizer>
    <company name="ACME" billingLocation="London" adminOffice="Manchester">
  </organizer>
  <traveller name="John Smith" homeCity="London" />
  <flight date="20160317" code="BA101" origin="London" destination="New York" />
  <vehicleHire start="20160317" end="20160317">
    <depot type="pickup" location="London" />
    <depot type="return" location="Heathrow" />
  </vehicleHire>
  <hotel location="New York" checkin="20160317" checkout="20160318" />
  <journey>
    <train date="20160318">
      <origin station="New York" />
      <destination station="Boston" />
    </train>
</itinerary>

I'd like to get a distinct list of attribute values where the attribute name is in (billingLocation,adminOffice,homeCity,origin,destination,location,station), but my final/actual list of attribute names will number around 30.

I have this C# code working using XmlDocument and an XPath query, but I'm unsure if this is the most efficient method.

XmlDocument xd = new XmlDocument();
xd.Load(customProvider.GetStream());
XmlNodeList nl = xd.SelectNodes(@"//@billingLocation|//@adminOffice|//@homeCity|//@origin|//@destination|//@location|//@station");
var lst = nl.Cast<XmlAttribute>().Select(a => a.Value).Distinct().ToList();

The final XML will be more complex and have a deeper hierarchy, and the attributes might occur across different nodes.

Is XmlDocument and XPath the most efficient aproach, or would Linq to XML perhaps be faster, and what would that look like?

As long as you don't face noticable performance issues I would chose whatever method seems to be better for you. The XPath works so you may stick to it.

I would however build the path in a more generic way by specifying an array of attribute names first:

var attributeNames = new[]
{
    "billingLocation",
    "adminOffice",
    "homeCity",
    "origin",
    "destination",
    "location",
    "station"
}

and the combining them into a query:

var xPath = string.Join("|", attributeNames.Select(n => $"//@{n}"));
...xd.SelectNodes(xPath);

In this cas however it is a little bit easier to do the same with linq:

Parse the xml first (which is btw invalid in your example - some elements are not closed):

var xElement = XElement.Parse(text);

Then use the same array for the linq query:

var cities =
    (from attr in xElement.Descendants().Attributes()
     where attributeNames.Contains(attr.Name.LocalName)
     select attr.Value).Distinct();


Should I create two seperate Services and then combine them or have one large? Basically, I have two separate tables, Campaign and CampaignDetails. They relate via CampaignID. I want a service that can handle both at the same time, as they're closely related and when one gets updated- so will the other.

At the moment I'm combining the two but it's starting to get a bit hard to look at

public interface ICampaignService
{
    //INSERT
    void InsertCampaign(Campaign campaign);
    void InsertCampaignDetails(CampaignDetails campaign);

    //UPDATE
    void UpdateCampaign(Campaign campaign);
    void UpdateCampaignDetails(CampaignDetails campaign);

    //DELETE
    void DeleteCampaign(Campaign campaign);
    void DeleteCampaign(object id);
    void DeleteCampaignDetails(CampaignDetails campaign);
    void DeleteCampaignDetails(object id);

    //ALL
    IList<Campaign> AllCampaigns();

    //FIND
    Campaign FindCampaignBy(Expression<Func<Campaign, bool>> expression);
    CampaignDetails FindCampaignDetailsBy(Expression<Func<CampaignDetails, bool>> expression);

    //FILTER
    IList<Campaign> FilterCampaignBy(Expression<Func<Campaign, bool>> expression);
    IList<CampaignDetails> FilterCampaignDetailsBy(Expression<Func<CampaignDetails, bool>> expression);

    //GET
    Campaign GetCampaignByID(object id);
    CampaignDetails GetCampaignDetailsByID(object id);

    Campaign GetLatestCampaign();
    Campaign GetMostRecentSentCampaign();

    //SAVE
    void Save();
}

This is just my interface, so the implementation is obviously a lot more.. And it seems a lot of similar code.

I came across this article http://bobcravens.com/2010/09/the-repository-pattern-part-2/ and I quite like the DataService that wraps all the individual services. But is it bad practise to say have one variable DataService mainService = new DataService(repo1, repo2, repo3, repo4) that can the access every service?

So I should I split it into CampaignService and CampaignDetailsService or keep it as one?

EDIT

Example of update method

    public void UpdateCampaign(Campaign campaignToUpdate)
    {
        _campaignRepository.Update(campaignToUpdate);
    }

    public void UpdateCampaignDetails(CampaignDetails campaignToUpdate)
    {
        _campaignDetailsRepository.Update(campaignToUpdate);
    }

Doing the same thing basically but with different entities.

Example of using method above:

    [HttpPost]
    public ActionResult Create(Campaign campaign)
    {
        if (ModelState.IsValid)
        {
            try
            {
                _campaignService.InsertCampaign(campaign);
                var campaignDetails = new CampaignDetails()
                {
                    CampaignId = campaign.CampaignId
                };

                _campaignService.InsertCampaignDetails(campaignDetails);
                _campaignService.Save();

                return RedirectToAction("Index");
            }
            catch (Exception ex)
            {
                ModelState.AddModelError(string.Empty, "Something went wrong. Message: " + ex.Message);
            }
        }

        return View(campaign);
    }

Overview of your service looks like this. I've put T-SQL commands that execute in each methods comment.

interface ICampaignService
{
    //INSERT = INSERT INTO CAMPAIGN....
    void InsertX( ... );

    //UPDATE = UPDATE CAMPAIGN....
    void UpdateX( ... );

    //DELETE = DELETE FROM CAMPAIGN
    void DeleteX( ... );

    //ALL = SELECT * FROM CAMPAIGN
    IList<Campaign> AllCampaigns();

    //FIND = SELECT * FROM CAMPAIGN WHERE ...
    //FILTER
    //GET
    Campaign FindX( ... );
    Campaign GetCampaignByID(object id);

    //SAVE = COMMIT TRANSACTION;
    void Save();
}

And you yourself say:

  I have two separate tables, Campaign and CampaignDetails. They relate via CampaignID.

The problem is your service is not just a thin layer over Entity Framework it still is a thin layer over SQL. In fact even your action names mirror database commands.

The service interface must be a must be the interface to your business logic layer.
And repositories be your gateway to the data access layer.

Let's look at the Create(Campaign campaign) action for a second:

Create(Campaign campaign)
{
// BOILER PLATE IGNORED
    _campaignService.InsertCampaign(campaign);
    var campaignDetails = new CampaignDetails()
    {
        CampaignId = campaign.CampaignId
    };

    _campaignService.InsertCampaignDetails(campaignDetails);
    _campaignService.Save();
// BOILER PLATE IGNORED
}

When your user pressed the submit button one time, did he intend 4 different things?

Create(Campaign campaign)
{
// BOILER PLATE IGNORED
    _campaignService.CreateCampaign(campaign);
// BOILER PLATE IGNORED
}

It is clear that this more closely captures user's intent.

void CreateCampaign(Campaign campaign)
{
    _campaignRepository.InsertCampaign(campaign);
    var campaignDetails = new CampaignDetails()
    {
        CampaignId = campaign.CampaignId
    };

    _campaignRepository.InsertCampaignDetails(campaignDetails);
    _campaignRepository.Save();
}

As you do the above transformation -in other words above refactoring- for each action there will be unused methods on the service interface. Remove them and your service interface will be cleaner.

  I want a service that can handle both at the same time, as they're closely related and when one gets updated- so will the other.

This means that Campaign and CampaignDetails form an aggregate. And Campaign is the aggregate root. (I'm just giving you the standard terminology, so you can more easily look up the situation on the Internet.)

In situation where you have closely related [entities] and [such that] when one gets updated- so will the other, or in database-speak a master table and one ore more detail tables, you service should not contain direct references to the detail entities, or in other words you service should only deal with the aggregate root, in this particular case Campaign. It only makes sense that ICampaignService only deals with Campaigns directly.

I had asked for the UpdateCampaignDetails example usage so that I can give a more concrete example of this but we will have to make do with the made up example below:

Suppose we are talking about sales campaigns here and one thing that is stored in CampaignDetails is a discount rate for the Campaign. After the above refactoring to your service and controller instead of UpdateCampaignDetails your service will have methods like below:

void ChangeDiscountRate(Campaign campaign, decimal newDiscountRate)

or

void ChangeDiscountRate(Guid CampaignId, decimal newDiscountRate)

whose implementation would be something like:

var campaignDetailsToUpdate = _campaignDetailsRepository.GetById(CampaignId);
campaignDetailsToUpdate.DiscountRate = newDiscountRate;
_campaignDetailsRepository.Update(campaignDetailsToUpdate);
_campaignDetailsRepository.Save();

You need not stop there. Ideally after a next round of refactorings (Including defining One-to-One and one-to-many relations where necessary) your service methods will read like below:

var campaignToUpdate = _campaignRepository.GetById(CampaignId);
campaignToUpdate.ChangeDiscountRate(newDiscountRate);
_campaignRepository.Update(campaignToUpdate );

or more generally:

var aggregateRoot = repository.GetById(id);
aggregateRoot.ModifyThisEntityAndItsDetails(...);
repository.Update(aggregateRoot);

The other thing to do would be separate the methods that update campaigns (hint those methods that return void, who are also the methods that are called from [HttpPost] controller methods from search methods (hint they are the method that return SomeCollection and take Expression<Func<Campaign, bool>> expression as parameters) that are called from elsewhere.


PHP Register Page ( MYSQLI ) I have a website written in PHP but the whole script is old so today I started upgrading from Mysql to Mysqli.

So here's my register page. I still have to add account confirmation and send mail part but that part can wait for now.

/* PUBLIC */
$IP = $_SERVER['REMOTE_ADDR'];

/* PRIVATE */
if(isset($_POST['submit']))
{
    $email= $_POST['email'];
    $email_safe = $mysqli->real_escape_string($email);
    $password = $_POST['password'];
    $password_safe = $mysqli->real_escape_string($password);
    $password1 = $_POST['password1'];
    $password1_safe = $mysqli->real_escape_string($password1);
    @$referrer = $_POST['referrer'];
    $referrer_safe = $mysqli->real_escape_string($referrer);

    if ($password_safe  == $password1_safe){
    if(strlen($password) < 8){
    die('Password must be at least 8 characters');
    } else {
    $result = $mysqli->query("SELECT email FROM user WHERE email='$email_safe'");
    if ($result->num_rows > 0) {
    die ("Duplicated email. Please use other email.");
    } else {
    $result = $mysqli->query("SELECT reg_ip FROM user WHERE reg_ip='$IP'");
    if ($result->num_rows > 0) {
    die ("Duplicated IP.");
    } else {
    $sql = "INSERT INTO user (email,passwd,ref) VALUES ('$email_safe','$password_safe','$referrer_safe')";
    if(!$result = $mysqli->query($sql)){
    die('There was an error running the query [' . $mysqli->error . ']');
    } else {
    echo"Registering....";
    }
    }
    }
    }
    } else {
    die ("Password didn't match. Please go back and fix this error.");
    }
} else {
    HTML FORM
    }

Well ? How it's looking ? It is secure ? ( I mean any open gate :) )

Thanks :)
P.S : The code works 100%.

While it's great to see someone actually take the trouble to refactor code, to move away from the depreacted mysql_* extension, it's not so great to see that, like most, you're actually trying to use the replacement (mysqli_*) as a 1-on-1 copy of the old extension.
On a personal note, though I've restricted myself to only dealing with the mysqli_* extension, I couldn't help noticing you're using the OO API it offers. Personally, I find the mysqli_* API, both procedural and OO a tad, erm, ... messy. If you're collaborating with others, there's nothing to stop them from switching/inserting procedural-style code, and mixing it in with your OO code. That's just nasty. PDO is another, valid alternative. It's API is OO-only, a lot cleaner and waaaay more easy to use. It's said to be slower, but in reality, the differences are so minute, they don't outweight the upsides in terms of code maintainability. Look into PDO, too, is what I'm trying to say... :)

Some history:
The old extension was created back in the days of MySQL version 3. Today, MySQL is already at version 5.6. Most hosts, if not all, offer mysql 5.x as standard DB. The list of features that weren't around back in the MySQL 3 era, but that are in common use now is rather impressive, and long:

New and better storage engines
Cursors
Stored procedures, functions, views and triggers
Foreign Key Contraints
Cluster support (Though I'm no big fan of MySQL clusters)
transactions (Commit, rollBack)
Prepared statements!
Start here, browse back and forth between releases, and compare for even more differences

Of course, there were other reasons why the mysql_* extension was deprecated. Its source wasn't very maintainable, for example. It's been around longer than the ZendEngine2, too. In short: it'd become sort of historic ballast, that wouldn't have worked, considering the direction PHP is going in (compiled language, OOP supprt, namespaces...)

Applied to your code:
All in all, there's a lot of changes, and some of them are quite useful when it comes to security. Take prepared statements for example (Note: here's some info on what they are, what they do). Rather than calling $mysqli->escape_string(), which as it happens requires you to set the charset!, you can easily use a prepared statement, much to the same effect:

$emailStmt = $mysqli->prepare('SELECT email FROM user WHERE email= ?');
$emailStmt->bindParam('s', $_POST['email');
$emailStmt->bind_result($dbEmail);//fetches result into this variable
if (!$emailStmt->execute() || $emailStmt->num_rows === 0)
{
    header('Location: '.basename($_SERVER['PHP_SELF']));//302 redirect is fine here
    $emailStmt->close();
    $mysqli->close();//be tidy!
    exit();// I hate die statements
}
$emailStmt->close();

So, no tedious escaping of all params all the time anylonger, you don't even have to worry about the types anymore. Mysql is, after all, fairly picky when it comes to data-types. Sure it can handle an_int_field >= '123', but really, it's best to avoid forcing the DB server having to cast the data to the right type. That's not what it's for.
As I said in the comments, mysqli->escape_string is considered as safe as prepared statements, But:

When working on your code, it's easy to forget that call to the escape_string method. Once you've forgotten that, your code is vulnerable to attacks.
If you forget to set the correct char-set, your queries are no longer as safe as prepared statements would be
getting into the habbit of using prepared statements is a good thing. You can prepare a single statement once, then use it to query your DB as much as you want. I've explained this here, with an example of how to do this.
Preparing a statement for a simple query might seem like a bit much, and in some ways it is. That's why the PDO extension emulates the prepare calls. That's one of the benefits of PDO: the statements are prepared, without any intervention of the DB server, so there's very little overhead when using prepared stmts. Honestly, there's no reason not to use them.

Anyway, I'm drifting off, slightly. What I'm actually trying to say is this:

Since you're refactoring anyway, why not look into what sort of new goodies mysqli_* brings to the party? the i is for improved after all. Don't use it as a 1-on-1 copy of the old extension. Time has, as it tends to do, moved on. There are new features. Learn about them, and use them. You're code will be more reliable, more maintainable, more future proof and 9/10 more safe, too, because of it.
Begin with using prepared statements, then move along to transactions. That's just a tip.

Some other thoughts
I see (or rather don't see) a worrying lack of data validation in your code. You just assume $_POST['email'] is going to be set, and that its value is going to be an email address. At no point in time, do I see you use something like this:

if (isset($_POST['email'])) && filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{
    //query DB
}
else
{
    exit('no valid email address provided');
}

The only time your code reflects any form of awareness that, perhaps, some POST data might not be set is here:

 @$referrer = $_POST['referrer'];

But the way it's dealt with just gets to me. As I've said before, and as I will keep on saying until either the supressor of death (@, that is) or I am no more:

  Argh... the @ suppressor of death! I hate that, don't use it... save for those few rare use-cases where you can't help a notice from being issued (seen this only 2 or 3 times in 10 years). If there's a notice, there's an issue, if there's an issue: don't ignore it fix it!

In this case, there isn't even an issue that needs fixing, except for the @ itself. Why not simply write:

$referrer = null;//declare var and assign null value
if (isset($_POST['referrer']))
{
    $referrer = $_POST['referrer'];
}

isset is a language construct, and therefore it's fast. supressing a notice isn't fast, it's slow, and completely pointless in most (if not all) cases. Don't.
If you don't want your code to be too long, because of all the if(isset branches, you can easily replace them with something like this:

if (isset($_POST))
{
    $email = $password= $password1 = $referrer = false;//all default value
    foreach($_POST as $key => $value)
    {
        if (isset($$key) && $$key === false)
        {//variable variable, check if it exists, and if its value is false
            $$key = trim($value);//if it is, assign POST val... perhaps trim while you're at it?
        }
    }
}

With this code, though not very maintainable, you save a lot of space, and you know that if any of the variable's used holds false as a value, it wasn't posted. All other variables (the ones that aren't false) have been reassigned the posted data.


Generic menu class for console applications Expanding the title: my goal (for hobby project) was to create simple, generic (customizable) and cross-platform menu class for console applications (mostly simple, text-based games). That's what I came up with:

#include <functional>
#include <iostream>
#include <map>
#include <string>

class ConsoleMenu {
  public:
    ConsoleMenu(const std::string& message,
        const std::string& invalidChoiceMessage,
        const std::string& prompt,
        const std::map<std::string, std::function<void()>>& commandsByChoice) :
            message_(message), invalidChoiceMessage_(invalidChoiceMessage),
            prompt_(prompt), commandsByChoice_(commandsByChoice) {}
    void use() {
      std::string userChoice;
      std::cout << '\n';
      // Based on https://isocpp.org/wiki/faq/input-output#istream-and-ignore {
      while ((std::cout << message_) && printChoices() &&
          (std::cout << prompt_) && (!(std::getline(std::cin, userChoice)) ||
          !validChoice(userChoice))) {
        std::cout << '\n' << invalidChoiceMessage_;
        // Should there be calls to std::cin::clear and std::cin::ignore, even
        // if I'm using std::getline?
      }
      // }
      commandsByChoice_[userChoice]();
    }
  private:
    bool printChoices() {
      bool successful = false;
      for (auto commandByChoice : commandsByChoice_) {
        successful = static_cast<bool>(
            std::cout << '[' << commandByChoice.first << ']' << '\n');
      }
      return successful;
    }
    bool validChoice(const std::string& choice) {
      return commandsByChoice_.find(choice) != commandsByChoice_.end();
    }
    std::string message_; // label_?
    std::string invalidChoiceMessage_;
    std::string prompt_;
    std::map<std::string, std::function<void()>> commandsByChoice_;
};

int main() {
  bool running = true;
  do {
    ConsoleMenu menu("What should this program do?\n", "Invalid operation; ", "> ", {
      {"run", [&running] {
        running = true;
      }},
      {"stop", [&running] {
        running = false;
      }}
    });
    menu.use();
  } while (running);
}

As you can see, while the general look of the menu is hard-coded, user can (and have to) specify the message (label) of the menu, list of choices, commands that are keyed to these choices, and even error message, and input prompt. "Generic"? Check!

If someone would like to suggest improvement requiring C++17, please, include also some non-standard equivalent(s) with similar interface, for example from The Boost library.

I see some things that may help you improve your program.

Separate the interface and implementation

Perhaps you posted it as just one file for the sake of simplicity and compactness, but if you were to actually use this, I think it's more likely that it would be useful as separate header and implementation files.  This has the additional benefit of making the interface part of the code (the part that a user would have to see) a little shorter and easier to comprehend.

Allow the user to specify streams

Right now, the code is tied to std::cin and std::cout, but what if I wanted to use the menu, say, remotely over a socket connection?  This could very easily be allowed by adding input and output stream references to the class.  One way to do it would be to have std::istream and std::ostream arguments with defaults to std::cin and std::cout.

Prefer const references

The printChoices routine is currently this:

bool ConsoleMenu::printChoices() {
  bool successful = false;
  for (auto commandByChoice : commandsByChoice_) {
    successful = static_cast<bool>(
        out << '[' << commandByChoice.first << ']' << '\n');
  }
  return successful;
}

The for loop should instead use const auto & rather than auto to assure no copies are made of the strings.  Further, the successful flag isn't really necessary.  Instead we could write it like this:

bool ConsoleMenu::printChoices() const {
  for (const auto &commandByChoice : commandsByChoice_) {
    out << '[' << commandByChoice.first << ']' << '\n';
  }
  return !!out;
}

The !!out construct assures that out is interpreted as a bool but you could use a cast if you think it's more readable.  (Perhaps I've been writing too much Javascript lately!)

Prefer unordered containers for performance

The std::map container has logarithmic search performance, but the std::unordered_map container has constant time performance.  For that reason, it makes sense to prefer the unordered container unless there's actually a need for a sorted one.  There isn't in this case.

Use const where practical

Right now, it's not possible to declare and use a const ConsoleMenu.  There's no reason that shouldn't be possible.  To do that, all of the functions except the constructor would have to be const.  Mostly that means simply adding const to the declaration of the function, but we need to do a little more to adapt use.  The reason is the last line:

commandsByChoice_[userChoice]();

There are a few problems with this simple line.  First, the operator[] has a side effect you probably don't want.  Specifically, if the key is not found, it is inserted into the map.  That's definitely not what's wanted here whether we are trying for const or not.  Two alternatives are using at (which can throw an exception) or find.  A way to do that would be this:

const auto result{commandsByChoice_.find(userChoice)};
if (result != commandsByChoice_.end()) {
    result->second();
}

Since you're already calling find in the helper function, it would make sense to only call it once and save (and use) the result.  I'll show that later on.

Rename use

The use method is a bit awkwardly named in my view.  I'd suggest instead to use operator()().  This would allow us to rewrite the sample main like this:

int main() {
    bool running{true};
    const ConsoleMenu menu{"What should this program do?\n",
            "Invalid operation; ", "> ",
        {
            {"run", [&running] { running = true; }},
            {"stop", [&running] { running = false; }}
        }};
    while(running) {
       menu();
    }
}

Note that I consistently use the C++11 uniform initialization syntax to reduce ambiguity.

Avoid re-doing work

Because there is no way within this code to insert or delete menu choices once the ConsoleMenu is created, we can create the entire menu as a static string once (perhaps within the constructor) and simply use that rather than iterating through the menu choices and making multiple calls to operator<< every time printChoices() is called.  Further, one could create a single string that includes message_, all choices and prompt_.

Results

Using all of the suggestions above, we can simplify considerably and remove the two private helper functions.  Here's the header file:

menu.h

#ifndef MENU_H
#define MENU_H
#include <functional>
#include <iostream>
#include <unordered_map>
#include <string>

class ConsoleMenu {
  public:
    ConsoleMenu(const std::string& message,
        const std::string& invalidChoiceMessage,
        const std::string& prompt,
        const std::unordered_map<std::string, std::function<void()>>& commandsByChoice,
        std::istream &in = std::cin,
        std::ostream &out = std::cout);
    void operator()() const;
  private:
    std::string invalidChoiceMessage_;
    // the prompt_ string contains the message, all choices and prompt
    std::string prompt_;
    std::unordered_map<std::string, std::function<void()>> commandsByChoice_;
    std::istream &in;
    std::ostream &out;
};
#endif // MENU_H

And this is the implementation.

menu.cpp

#include "menu.h"
#include <sstream>

ConsoleMenu::ConsoleMenu(const std::string& message,
    const std::string& invalidChoiceMessage,
    const std::string& prompt,
    const std::unordered_map<std::string, std::function<void()>>& commandsByChoice,
    std::istream &in, std::ostream& out) :
        invalidChoiceMessage_{invalidChoiceMessage},
        commandsByChoice_{commandsByChoice},
        in{in},
        out{out}
{
    std::stringstream ss;
    ss << message;
    for (const auto &commandByChoice : commandsByChoice_) {
      ss << '[' << commandByChoice.first << ']' << '\n';
    }
    ss << prompt;
    prompt_ = ss.str();
}

void ConsoleMenu::operator()() const {
    std::string userChoice;
    const auto bad{commandsByChoice_.cend()};
    auto result{bad};
    out << '\n';
    while ((out << prompt_) && (!(std::getline(in, userChoice)) ||
            ((result = commandsByChoice_.find(userChoice)) == bad))) {
        out << '\n' << invalidChoiceMessage_;
    }
    result->second();
}

Additional features

There are some additional things that might be nice.  One thing that comes immediately to mind is that it would be useful to be able to create a constexpr ConsoleMenu.  One way to do that might be to use C++17's std::string_view instead of std::string and to use an array instead of a map.

Another feature that might be nice would be the ability to optionally use wide strings.  This is pretty easy to accommodate by turning the class into a templated class.