Mapping yielding lots of warnings in use Recently, I wrote a C header file that defines a new type called HashMap that allows for the storage of key/value pairs.

I am just learning about the wonders of void pointers and as I use them, I get lots and lots of warnings about making pointers without a cast. The header file itself does not yield any warnings, but when I use the header file, I get lots.

Here is the header file (I am leaving out all the guards and other preprocessor commands):

typedef struct {
    int size;
    void **k;
    void **v;
} HashMap;

void hashmap_init(HashMap *hm, int size) {
    hm->k = malloc(size);
    hm->v = malloc(size);
    hm->size = size;
}

void hashmap_push(HashMap *hm, void *k, void *v, int index) {
    hm->[index] = k;
    hm->[index] = v;
}

void *hashmap_get(HashMap *hm, void *k) {
    int i;
    for(i = 0; i < hm->size; i++)
        if(hm->k[i] == k)
            return hm->v[i];
}

And the file in which I am testing the header:

int main(void) {
    HashMap hm; // a new HashMap
    hashmap_init(&hm, 10); // Initialize the HashMap with 10 spots for memory

    hashmap_push(&hm, 3, 7, 0); // In the first index, put 3 as the key and 7 as it's value

    int v = (int)hashmap_get(&hm, 3); // Get the key dubbed 3 from the hashmap

    printf("%d\n", v); // => 7
    return 0;
}

I was thinking: In Java, when using a HashMap, you initialize it with 2 different values. Would it be better if I did that here? (this is just a second thought - you don't have to answer this too)

You have a bug

This doesn't compile for me:

hm->[index] = k;
hm->[index] = v;

I think you meant this:

hm->k[index] = k;
hm->v[index] = v;

Understanding the warnings and responding to them

  Recently, I wrote a C header file that defines a new type called HashMap that allows for the storage of key/value pairs.

As comments pointed out, this is not a hash map. It's a map. This wiki page explains what is a hash map.

  I am just learning about the wonders of void pointers and as I use them, I get lots and lots of warnings about making pointers without a cast.

Pay attention to the warnings.
Any unhandled warnings are bugs waiting to happen.

Since you mentioned you're new with pointers,
I'm getting the feeling that maybe you're just not comfortable reading warnings yet.
They are not that hard to read though,
and you don't really have a choice:
you must read and understand them so you could eliminate them.

Here's an example:

t.c: In function 'main':
t.c:31:5: warning: passing argument 2 of 'hashmap_push' makes pointer from integer without a cast [enabled by default]
     hashmap_push(&hm, 3, 7, 0);
     ^

As a reminder, here's the method signature:

void hashmap_push(HashMap *hm, void *k, void *v, int index);

The method takes as 2nd parameter a void*,
but you're passing it the int value 3.
Well, that just doesn't work: you're passing the wrong type.
To make it work,
the compiler casts this integer to void*,
and warns you about it.
Is this really what you wanted?
No. Casting an int variable to void* can't be good.

The next warning is related to the first:

t.c:15:6: note: expected 'void *' but argument is of type 'int'
 void hashmap_push(HashMap *hm, void *k, void *v, int index) {
      ^

The first warning was about calling hashmap_push with the wrong parameter types,
this one is about the hashmap_push receiving the wrong parameter types.

Next warnings:

t.c:31:5: warning: passing argument 3 of 'hashmap_push' makes pointer from integer without a cast [enabled by default]
     hashmap_push(&hm, 3, 7, 0);
     ^
t.c:15:6: note: expected 'void *' but argument is of type 'int'
 void hashmap_push(HashMap *hm, void *k, void *v, int index) {
      ^

I hope you guessed, this warning is about the same line,
but this time argument 3, which is the int value 7.
Same problem: casting an int to void*.

t.c:33:5: warning: passing argument 2 of 'hashmap_get' makes pointer from integer without a cast [enabled by default]
     int v = (int)hashmap_get(&hm, 3);
     ^
t.c:20:7: note: expected 'void *' but argument is of type 'int'
 void *hashmap_get(HashMap *hm, void *k) {
       ^

Based on the explanations earlier,
can you tell what this is trying to tell you?

   The 2nd argument of the call to hashmap_get is invalid, as the
 number 3 is of type int instead of void*. The warning after that
 is about the receiving side: the hashmap_get is expecting a void*
 but it's receiving an int.

Finally, this one:

t.c:33:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     int v = (int)hashmap_get(&hm, 3);
             ^

Can you tell what the warning is trying to tell you?

   The hashmap_get method returns a void*, but you're casting it to
 int. This is not valid.

In short,
all these warnings were trying to tell you that you're calling methods with the wrong types.
It's a fairly lucky accident that this code works at all and produces the expected output.
Without clearing up these warnings by calling the methods with the appropriate types,
you cannot trust this code.

  The header file itself does not yield any warnings, but when I use the header file, I get lots.

When the header file compiles without warnings,
but the source file using that header produces warnings,
that practically means the source file is misusing the header.
The header is like a contract:
it specifies what you can do.
Your source file twisted that contract.
You got a result that sort of magically seemed to work,
but it's in violation of the contract,
and it wasn't really supposed to work.

Using the map without warnings

This (by no means good) code eliminates the warnings and works:

int main(map) {
    HashMap hm;
    hashmap_init(&hm, 10);

    int key = 3;
    int value = 7;
    hashmap_push(&hm, &key, &value, 0);

    int v = * (int*) hashmap_get(&hm, &key);

    int key2 = 3;
    float value2 = 5.3;
    hashmap_push(&hm, &key2, &value2, 0);

    float v2 = * (float*) hashmap_get(&hm, &key2);

    printf("%d\n", v);
    printf("%f\n", v2);

    return 0;
}

This code passes the methods the types they expect,
and doesn't make invalid casts, namely:

&key is a int*, which is a subtype of void*, therefore valid
Same for &value
Casting the 1st call to hashmap_get to int* is valid, because it corresponds to the type of &value that was passed in earlier for &key
Casting the 2nd call to hashmap_get to float* is valid, because it corresponds to the type of &value2 that was passed in earlier for &key2

I used different value types for the sake of a demonstration,
but keep in mind that ideally (and in 99% of practical cases) all values of a map should be of the same type.
Among other things,
one reason for that is given some key k whose value you don't know,
you cannot know in advance the correct type of the value.
In this specific example, I knew in advance that the value of key2 is value2.

Finally, what do you think the program outputs?

       7
     5.300000

Well, this is a bit unexpected from a map.
Normally I would expect from a map that if I set the value of key 3 to x,
and then again set the value of key 3 to y,
the second call would update the existing entry and overwrite it with y.
This map works this way because of this code in hashmap_get:

    if (hm->k[i] == k)
        return hm->v[i];

That is, you are comparing the pointers of keys, rather than their values.
In a more common map implementation,
two distinct key objects both having the value 3 would be treated as the same key.
To make this code work that way,
it will take a substantial rewrite,
and rethinking of the types.

Other coding style issues

I suggest to use braces with all single-line statements, for example instead of:

for (i = 0; i < hm->size; i++)
    if (hm->k[i] == k)
        return hm->v[i];

Write like this:

for (i = 0; i < hm->size; i++) {
    if (hm->k[i] == k) {
        return hm->v[i];
    }
}

It's a bit longer, but it can save you from some simple but common mistakes.


Full-precision summation in Haskell I'm trying to speed up the following code for summing doubles with full precision, based on a Python recipe from Raymond Hettinger.  Any thoughts on how to make this go faster?

-- Full precision summation based on http://code.activestate.com/recipes/393090/
msum :: [Double] -> Double
msum [] = 0
msum (x:xs) = sum $ foldl' (\ps x' -> reverse (partials x' ps)) [x] xs

partials :: Double -> [Double] -> [Double]
partials x = foldl' (\acc p -> case hilo x p of (hi, 0) -> hi:acc
                                                (hi, lo) -> hi:lo:acc) []

hilo :: Double -> Double -> (Double, Double)
hilo x y | abs x < abs y = hilo y x
         | otherwise = (hi, lo) where hi = x + y
                                      lo = y - (hi - x)

This is already a second draft; you can see a previous version of the code in this gist.

You could write hilo as hilo :: Double -> Double -> [Double], which returns [hi,lo] for lo /= 0 and [hi] for lo = 0. Then you can write partials as

partials x = foldl' (\acc p -> (hilo x p) ++ acc) -> hi:acc []

BTW: How could lo ever become non-zero considering

lo = y - (hi - x) = y - (x + y - x) = y - y = 0

Rounding?


A recursive web-crawler created using vba I had a desire to make a recursive web crawler in vba. As I don't have much knowledge on vba programming, so it took me a while to understand how the pattern might be. Finally, I've created one. The crawler I've created is doing just awesome. It starts from the first page of a torrent site then tracking the site's next page link it moves on while extracting names until all links are exhausted. Any input on this to make it more robust will be a great help. Thanks in advance.

Here is what I've written:

Sub yify(dynamic_link As String)

    Application.ScreenUpdating = False
    Const main_link As String = "https://yts.ag"
    Dim http As New XMLHTTP60, html As New HTMLDocument
    Dim movie As Object, link As Object

    With http
        .Open "GET", dynamic_link, False
        .send
        html.body.innerHTML = .responseText
    End With
    For Each movie In html.getElementsByClassName("browse-movie-title")
        ActiveCell.Value = movie.innerText   ''Scraping movie names
        ActiveCell.Offset(1, 0).Select
    Next movie

    For Each link In html.getElementsByClassName("tsc_pagination")(0).getElementsByTagName("a")
        If InStr(link.innerText, "Next") > 0 Then
            yify (main_link & Split(link.href, ":")(1))  ''Feeding next page link to the crawler
        End If
    Next link
    Application.ScreenUpdating = True

End Sub

Sub RecursiveCrawler()
    Range("A1").Select
    yify ("https://yts.ag/browse-movies/0/all/documentary/0/latest")  ''Crawling process starts here
End Sub

This is quite clean in general. I would probably only add blank lines to visually separate blocks of code and put comments about what is happening in the following block; and, apply the "Extract variable" refactoring method:

With http
    .Open "GET", dynamic_link, False
    .send
    html.body.innerHTML = .responseText
End With

''Extracting movie names
Set movieTitles = html.getElementsByClassName("browse-movie-title")
For Each movie In movieTitles
    ActiveCell.Value = movie.innerText
    ActiveCell.Offset(1, 0).Select
Next movie

''Feeding next page link to the crawler
Set paginationLinks = html.getElementsByClassName("tsc_pagination")(0).getElementsByTagName("a")
For Each link In paginationLinks
    If InStr(link.innerText, "Next") > 0 Then
        yify (main_link & Split(link.href, ":")(1))
    End If
Next link

Application.ScreenUpdating = True

I don't particularly like the way you are getting the next link, but, since I don't see that we can use CSS selectors or XPaths with this html document API.


C++ limiting a method for an rvalue caller, an expression evaluator example There are many ways to implement a math expressions evaluator (I focus here just on the evaluator part, without any parsing).

I want to explore a certain implementation to support the following main:

int main() {
    Expression e = Sum( Sum(Number(2), Number(3)), Number(-1) );
    cout << e << "=" << e.eval() << endl;
}

The expected output is:

((2+3)+-1)=4

A basic notion: there are some temporary objects created there. We should better move them.
So I came up with the following code I would like to consult about.

BaseExpression - an abstract base for all expressions

class BaseExpression {
public:
    virtual ~BaseExpression() {}
    // what do you think about the concept of the method 'get_unique_copy()' below?
    virtual std::unique_ptr<BaseExpression> get_unique_copy() && = 0;
    virtual double eval()const = 0;
    virtual void print(ostream& out)const = 0;
};

Expression - the actual non-abstract to hold an expression

class Expression {
    std::unique_ptr<BaseExpression> _e;
public:
    Expression(nullptr_t) {}
    Expression(BaseExpression&& e)
    : _e(std::move(e).get_unique_copy()) {}
    ~Expression() {}
    Expression(Expression&& e) noexcept : _e(std::move(e._e)) {}
    Expression& operator=(Expression&& e) noexcept { std::swap(_e, e._e); return *this; }
    double eval() const { return _e->eval(); }
    friend ostream& operator<<(ostream& out, const Expression& e) {
        e._e->print(out);
        return out;
    }
};

Sum - an example of an actual expression

class Sum: public BaseExpression {
    Expression _e1{nullptr}, _e2{nullptr};
public:
    Sum(BaseExpression&& e1, BaseExpression&& e2)
        : _e1(std::move(e1)), _e2(std::move(e2)) {}
    Sum(const Sum& s) = delete;
    Sum& operator=(const Sum& s) = delete;
    Sum(Sum&& s) noexcept {
        std::swap(_e1, s._e1);
        std::swap(_e2, s._e2);
    }
    // what do you think about the implementation of 'get_unique_copy()' below? is it safe?
    virtual std::unique_ptr<BaseExpression> get_unique_copy() && {
        return std::make_unique<Sum>(std::move(*this));
    }
    virtual double eval()const override {
        return _e1.eval() + _e2.eval();
    }
    virtual void print(ostream& out)const override {
        out << '(' << _e1 << '+' << _e2 << ')';
    }
};

Number - the simplest actual expression

class Number: public BaseExpression {
    double _d;
public:
    Number(double d) : _d(d) {}
    // what do you think about the duplication of 'get_unique_copy()' in each derived?
    // is there a way to avoid the duplication, apart from ugly Macros?
    virtual std::unique_ptr<BaseExpression> get_unique_copy() && {
        return std::make_unique<Number>(std::move(*this));
    }
    virtual double eval()const override {
        return _d;
    }
    virtual void print(ostream& out)const override {
        out << _d;
    }
};

What do you think?

http://coliru.stacked-crooked.com/a/9843afbe4e9ed4d8

I had a bunch of sections detailing the various aspects of your design that could be improved, but I ended up scrapping most of it, because at the end of the day, it can be resumed in two specific things:

But first: The idea of forcing RValues here is severely misguided. Forcing RValue usage should be limited to cases where things would break otherwise, not to enforce coding practices. What if someone wants to create a complex sub-expression and reuse it a few times? Tough luck!

1. Expression as a wrapper is misguided.

I do not like wrappers that exist just for the sake of giving value semantics to something. Value vs reference semantics should be delegated to the user as much as humanly possible as long as type erasure is not invovled. Specifically, someone will inevitably write std::unique_ptr<Expression> at some point in the future.

2. This is unjustifiably complicated.

You can obtain the same functionallity with bog-standard code, no need to get fancy. The trick is to separate the Sum() function in the API from the Sum type in the implementation.

class Expression {
public:
  virtual ~Expression() {};
  virtual std::unique_ptr<Expression> clone() const = 0;
};

class SumExpr : public Expression {
  std::unique_ptr<Expression> e1_;
  std::unique_ptr<Expression> e2_;
public:
  SumExpr(std::unique_ptr<Expression> e1,
          std::unique_ptr<Expression> e2)
    : e1_(std::move(e1))
    , e1_(std::move(e2)) {}

  std::unique_ptr<Expression> clone() override {
    return std::make_unique<SumExpr>(e1->clone(), e2->clone());
  }
};

std::unique_ptr<Sum> Sum(std::unique_ptr<Expression> e1,
                         std::unique_ptr<Expression> e2) {
  return std::make_unique<SumExpr>(std::move(e1), std::move(e2));
}

Note that is is not "quite" how I'd go about implementing this, but this way matches your current functionality.

Bonus: Implement printing to ostream as operator <<

It's a shame that operator<<(std::ostream&, Sum const&) is not implemented. print() should delegate to it ideally, so that stream << Sum()...; can work

Now, at first glance, it would seem that this would require some really annoying boilerplate, but you can get around that using some crafty CRTP:

class Expression {
public:
  virtual std::ostream& print(std::ostream&) const = 0;
};

template<typename CRTP_T>
class ExpressionImpl : public Expression {
  std::ostream& print(std::ostream& stream) const override {
    return stream << *static_cast<CRTP_T const*>(this);
  }
};

class Sum : public ExpressionImpl<Sum> {
  friend std::ostream& operator<<(std::ostream&, Sum const&) {
    ...
  }
};

As an exercise to the reader, you can similarly create a BinaryExpression<CRTP_T> to avoid having to reimplement the clone() function in every single leaf class.


Generate random unit vectors around circle I'm trying to generate a bunch of uniformly distributed unit vectors around the unit circle. Here's my code, which is working, but unfortunately I have a for-loop. How do I get rid of this for-loop?

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

def gen_rand_vecs(dims, number):
    vecs = np.random.uniform(low=-1, size=(number,dims))
    mags = np.sqrt((vecs*vecs).sum(axis=-1))
    # How to distribute the magnitude to the vectors
    for i in range(number):
        vecs[i,:] = vecs[i, :] / mags[i]
    return vecs

theta = np.linspace(0, 2*np.pi, 100)
circle = np.array([np.cos(theta), np.sin(theta)])

fig = plt.figure()
plt.plot(circle[0], circle[1])
rand_vecs = gen_rand_vecs(2, 100)
for e in rand_vecs:
    plt.plot([0,e[0]], [0,e[1]], 'r')
plt.show()

To answer your question, you need to add a new dimension to the ndarray:

vecs /= mags[..., np.newaxis]

However

  uniformly distributed unit vectors around the unit circle

No it's not, at least not in \$\theta\$. You're generating uniformly distributed points on the unit n-sphere and modifying it to the unit circle; effectively reducing it to an angle. These angles will not be uniformly distributed, and this is easiest to show in 2D:

Notice how the corner piece is larger than the side piece, despite both being \$30^{\circ}\$.

Instead, generate the original points with a normal distribution.

This is because the standard normal distribution has a probability density function of

$$
\phi(x) = \frac{1}{\sqrt{2\pi}}\exp\left(-x^2/2\right)
$$

so a 2D one has probability density

$$
\phi(x, y) = \phi(x) \phi(y) = \frac{1}{2\pi}\exp\left(-(x^2 + y^2)/2\right)
$$

which can be expressed solely in terms of the distance from the origin, \$r = \sqrt{x^2 + y^2}\$:

$$
\phi(r) = \frac{1}{2\pi}\exp\left(-r^2/2\right)
$$

This formula applies for any number of dimensions (including \$1\$). This means that the distribution is independent of rotation (in any axis) and thus must be evenly distributed along the surface of an n-sphere.

Thanks to Michael Hardy for that explanation.

This is as simple as using instead

np.random.normal(size=(number,dims))

Generating mags can also be just

np.linalg.norm(vecs, axis=-1)

A few minor changes then gets one to

import numpy as np
import matplotlib.pyplot as plt

def gen_rand_vecs(dims, number):
    vecs = np.random.normal(size=(number,dims))
    mags = np.linalg.norm(vecs, axis=-1)

    return vecs / mags[..., np.newaxis]

def main():
    fig = plt.figure()

    for e in gen_rand_vecs(2, 1000):
        plt.plot([0, e[0]], [0, e[1]], 'r')

    plt.show()

main()

The plotting is quite painful, so here's a cleaner, faster version (credit HYRY):

import numpy
from numpy import linalg, newaxis, random
from matplotlib import collections, pyplot

def gen_rand_vecs(dims, number):
    vecs = random.normal(size=(number,dims))
    mags = linalg.norm(vecs, axis=-1)

    return vecs / mags[..., newaxis]

def main():
    ends = gen_rand_vecs(2, 1000)

    # Add 0 vector to start
    vectors = numpy.insert(ends[:, newaxis], 0, 0, axis=1)

    figure, axis = pyplot.subplots()
    axis.add_collection(collections.LineCollection(vectors))
    axis.axis((-1, 1, -1, 1))

    pyplot.show()

main()


Do I need to remove these event listeners? The following ActionScript code just downloads and attempts to parse some JSON from an API service. Inside the method I have defined a cleanUp() function, just to remove 2 event listeners.

I think these event listeners will be garbage collected anyway though, so is it worth it? I would prefer to just use 2 anonymous functions for the listeners.

private function sendRequest(cmd:String, params:Object = null) :Promise{

    var d:Deferred = new Deferred(),
        vars:URLVariables = new URLVariables('cmd=' + cmd),
        req:URLRequest = new URLRequest(),
        loader:URLLoader = new URLLoader();

    if(params){
        for(var paramKey:String in params){
            vars[paramKey] = params[paramKey];
        }
    }

    req.url = url;
    req.requestHeaders = [new URLRequestHeader('Accept', 'application/json')];
    req.method = URLRequestMethod.POST;
    req.data = vars;

    function onComplete(e:Event) :void{
        var res:Object;

        try{
            d.resolve(JSON.parse(e.target.data));
        }
        catch(err:Error){
            d.reject(err);
        }
        finally{
            cleanUp();
        }
    }

    function onError(err) :void{
        d.reject(err);
        cleanUp();
    }

    function cleanUp() :void{
        loader.removeEventListener(Event.COMPLETE, onComplete);
        loader.removeEventListener(IOErrorEvent.IO_ERROR, onComplete);
    };

    loader.addEventListener(Event.COMPLETE, onComplete);
    loader.addEventListener(IOErrorEvent.IO_ERROR, onError);

    loader.load(req);

    return d.promise;
};

Quick answer:

Maybe yes, maybe no. Yes, they'll be garbage collected now, but cleaning up after yourself is not a bad habit to get into. It doesn't hurt to do so.

What you do need to be careful of, however, is that you clean it up right.

Compare:

loader.addEventListener(Event.COMPLETE, onComplete);
loader.addEventListener(IOErrorEvent.IO_ERROR, onError);

With:

loader.removeEventListener(Event.COMPLETE, onComplete);
loader.removeEventListener(IOErrorEvent.IO_ERROR, onComplete);

and you'll soon notice the mistake: IOErrorEvent.IO_ERROR is mapped to onError, but the removeEventListener assumes it's mapped to onComplete!

Additionally, you have a unnecessary variable here:

var res:Object;

It's not used by anything, so I'd remove it.


Binary Search Tree implementation using templates I wrote this implementation using templates. It works fine and as expected. Just want to know if this can be improved. I have some specific questions too at the end. Please feel free to critique to your heart's content.

#include <iostream>

template <class T>
class Tree
{
    // Internal class which stores only Node related information.
    struct TreeNode
    {
        T data;
        TreeNode * left;
        TreeNode * right;

        TreeNode(T val):data(val),left(NULL),right(NULL)
        {
        }
    };
    TreeNode * root;
    void print(TreeNode*);
    void freeMemory(TreeNode*);

public:
    Tree();
    ~Tree();
    void insert(T);
    void print();
};

template <class T>
Tree<T>::Tree():root(NULL){}

template <class T>
Tree<T>::~Tree()
{
    freeMemory(root);
}

template <class T>
void Tree<T>::freeMemory(Tree::TreeNode *node)
{
    if (node==NULL)
        return;
    if (node->left)
        freeMemory(node->left);
    if (root->right)
        freeMemory(node->right);
    delete node;
}

template <class T>
//make it return value?
void Tree<T>::insert(T val)
{
    TreeNode * treeNode = NULL;
    try
    {
        treeNode = new TreeNode(val); // handle exception necessary?
    } catch (std::bad_alloc &exception)
    {
        std::cerr << "bad_alloc caught: " << exception.what() << std::endl;
        EXIT_FAILURE;
    }
    TreeNode *temp=NULL;
    TreeNode *prev=NULL;

    temp = root;
    while(temp)
    {
        prev = temp;
        if (temp->data < treeNode->data)
            temp = temp->right;
        else
            temp = temp->left;
    }
    if (prev==NULL)
        root = treeNode;
    else
    {
        if (prev->data<treeNode->data)
            prev->right = treeNode;  // use setter function?
        else
            prev->left = treeNode;
    }
}

template <class T>
void Tree<T>::print(TreeNode *root)
{
    if (root==NULL)
        return ;
    print(root->left);
    std::cout << root->data << std::endl;
    print(root->right);
}

template <class T>
void Tree<T>::print()
{
    print(root);
}

int main()
{
    Tree<int> tree;
    tree.insert(14);
    tree.insert(12);
    tree.insert(6);
    tree.insert(17);
    tree.insert(8);
    tree.print();

}

Should I handle possible exceptions that can be thrown from new?
Is it good practice to have a TreeNode class as I've used?  If not, how to make it better?
Should my insert return some value to indicate if insertion was successful?
Any other comments/suggestions?

Exception handling

While it might make sense to handle the bad_alloc exception, the way you've done so is likely to lead to problems. The basic point of exception handling is that it decouples the point at which the exception is handled from the point at which it is generated. Right now, you've couple those by catching the bad_alloc immediately after the point at which it could be generated. Worse, you haven't handled it well at all. It looks like you intended to exit (stop execution), but in fact you only have EXIT_FAILURE, which is just a constant (typically 1). At least in theory, that's evaluated by the compiler as an expression which does nothing. In fact, there's a good chance the compiler will optimize such an expression way completely, so it does nothing.

Even assuming you corrected that so it exited the program correctly, code at that level (allocating a node) has no business deciding that bad_alloc should lead directly to exiting the program. In your test code that may make sense, but depending on how the search tree is being used, it make may sense to react to a bad_alloc in some other way. Leave that decision to the higher level code that can handle it more appropriately.

For example, it would make much more sense to handle it in main:

int main()
{
    try {
        Tree<int> tree;
        tree.insert(14);
        tree.insert(12);
        tree.insert(6);
        tree.insert(17);
        tree.insert(8);
        tree.print();
    }
    catch (std::exception const &e) {
        std::cerr << e.what << "\n";
    }
}

With that in place, your code to allocate a node:

    TreeNode * treeNode = NULL;
    try
    {
        treeNode = new TreeNode(val); // handle exception necessary?
    } catch (std::bad_alloc &exception)
    {
        std::cerr << "bad_alloc caught: " << exception.what() << std::endl;
        EXIT_FAILURE;
    }

...becomes more like:

TreeNode *treeNode = new TreeNode(val);

...Only in really shouldn't be quite that either, because (as detailed below) you should really use unique_ptr instead of raw pointers.

bad_alloc specifics

bad_alloc, however, is kind of a special case, and I sometimes question whether it makes sense to handle it at all, at least in code intended for most typical desktop OSes. Memory allocation failure used to be pretty simple: you had N kilobytes of memory, and when/if that ran out, malloc returned NULL or new threw an exception.

Virtual memory changed that though. When you run out of memory, the OS swaps data to the swap file to make more room. In most typical cases, long before allocation has truly failed, the system will run slow enough for long enough that the user will kill the program long before the memory allocation actually fails. In fact, Linux actually kind of automates that process--it has an "OOM Killer" process that will kill processes to keep memory free so the system doesn't drag down to running too slowly. In most cases, that means your process will be killed without bad_alloc ever being thrown.

As such, it's quite debatable (at best) whether it makes sense to try to deal specifically with bad_alloc, at least in code for these systems.

For an interview, handling bad_alloc would depend. For embedded software, you typically do want to handle it. For desktop/server software, you may not want to, but if you don't you need to be prepared to explain why you don't.

"Raw" pointers

Most of what you've currently defined as pointers could really be instances of std::unique_ptr instead (again, at least assuming you're using a new enough compiler to support it). unique_ptr automates most of the "cleanup", so you could pretty much (probably completely) eliminate what you've currently implemented as freeMemory.

The downside of this is that a unique_ptr stores a deleter object that's used to clean up the pointee object when the unique_ptr goes out of scope. In the case of your binary tree, you'd at least normally assume that all the nodes in the tree to be deleted the same way. In that case, you probably either store no deleter object at all (and just use delete) or else store only one deleter for the entire tree, and internally use raw pointers. This can save quite a bit of storage space with (at worst) fairly minimal cost in code complexity.

Recursion pattern

Although I've already pointed out how it can probably be eliminated completely, let's assume for the moment that you're using raw pointers so you still need your freeMemory. If so, it can (IMO) be cleaned up a bit. Instead of checking for null pointers before you make the recursive call, just check for a null pointer once:

template <class T>
void Tree<T>::freeMemory(Tree::TreeNode *node)
{
    if (node==nullptr)
        return;
    freeMemory(node->left);
    freeMemory(node->right);
    delete node;
}

Each chain of recursive calls will stop when you reach a null pointer (thanks to the one if that remains). This reduces the complexity of this function pretty noticeably at essentially no cost1.

Print

As @Jamal already pointed out, your print function doesn't modify the tree, so it should be const. I'd also prefer to see it allow the user to pass a specific stream to which the contents will be printed. In fact, it's at least worth considering overloading the stream insertion operator (operator<<) for your tree type, so you can insert a tree in a stream just like you would any other type:

Tree my_tree;

// code to insert data into tree goes here

std::cout << my_tree;

Alternative to Print

I'd at least consider whether you really want a Tree::print at all, or whether it's better to have the ability to walk the tree and apply essentially any function to all its nodes (with print as just one of those possibilities).

Return value from insert

I would say that yes, it's a good idea for insert to return a value indicating whether the insertion was successful. Your code is roughly analogous to std::set, which does return such a value, and I've used that return value quite a few times, so if I were going to use this, I'd probably make use of the same.

Iterators

If you were going to put this to real use, it would be preferable to be able to use it via a standard iterator interface. Alternatively, you might prefer to support a range interface (I'd probably prefer to use a range-based interface, anyway). This is probably getting beyond the scope of what you're currently thinking about though. You also quickly run into the fact that this provides pretty much a subset of what std::set already provides, so its practical use is probably limited in any case.

1. And in case you're wondering about the apparent contradiction of using nullptr in code that's only needed prior to C++11 (when nullptr was introduced), it's really pretty simple: nullptr is sufficiently trivial that a fair number of compilers supported it before they implemented rvalue references, which are crucial for unique_ptr. As such, even if you're using an older compiler that doesn't have unique_ptr there's a good chance it does have nullptr. It's even possible to define a nullptr class that works with C++03 compilers (a little Googling should turn up at least one).


Checking user mouse movement I am trying to update my server to let it know that the user had some activity in the past 15 minutes. In order to do that, I made this implementation:

var page = {

    updated: false,

    change_state: function(e)   {
        var self = this;
        if(!this.updated){
            debugConsole.log('Moved mouse');
            setTimeout(function(){
                self.updated = false;
            },  //                10000); //10 seconds
                120000); //2 minutes
            this.updated = true;
            $.ajax({
                url: 'comet/update.php',
                success: function(data){
                    debugConsole.log('Moved mouse at: ' + data);
                }
            })
        }
    },

    mouse_click: function(e)    {
        this.updated = false;
    }

}

$("*").live('mousemove',function(e){
    e.stopPropagation();
    page.change_state(e); });

$("*").live('click',function(e){
    page.mouse_click(e); });

I made it so that it only takes effect after 2 minutes from the last action, unless the last action was a click.

Is this the best way of doing it?

I wouldn't recommend using $("*").live, you only need the events on the document body, so long as they are allowed to properly bubble.


Implementing convolution using SymPy I started using SymPy recently, and I implemented convolution using it.

def convolve(f,g,x,lower_limit,upper_limit):
    y=Symbol('y')
    h = g.subs(x,x-y)
    return integrate(f*h,(y,lower_limit,upper_limit))

It seems to work for a few tests I've done.

Would like to know what you think of it, any improvements are appreciated.

Your code shouldn't work:
You are calculating:
$$\int_a^b f(t) \, g(t - \tau) \; d\tau$$
but convolution is defined as:
$$f(t) \, * \, g(t) \equiv \int_{-\infty}^{\infty} f(\tau) \, g(t - \tau) \; d\tau$$
so the default limits of integration should be \$-\infty\$ to \$\infty\$. More importantly you should use the proper argument for f (the integration variable). Finally, naming the integration variable y feels unusual.

from sympy import oo, Symbol, integrate
def convolve(f, g, t, lower_limit=-oo, upper_limit=oo):
    tau = Symbol('__very_unlikely_name__', real=True)
    return integrate(f.subs(t, tau) * g.subs(t, t - tau),
                     (tau, lower_limit, upper_limit))


Fill out a spiral matrix I got this question during my interview:

  Given an integer N, output an N x N spiral matrix with integers 1 through N.

  Examples: Input:  3

  Output: [[1, 2, 3],
           [8, 9, 4],
           [7, 6, 5]]

  Input: 1

  output: matrix filled out as a spiral  [[1]]

/*PSEUDO:
rowMin = 0
rowMax = n - 1
colMin = 0
colMax = n - 1
counter = 1
matrix = []
create the matrix:
loop from 0 to n - 1
  array
  loop from 0 to n-1
    push 0 into array
while rowMin <= rowMax and colMin <= colMax
  loop on rowMin from colMin to colMax.  col
    matrix[rowMin][col] becomes counter++
  rowMin++
  loop on colMax and from rowMin to rowMax.  row
    matrix[row][colMax] becomes counter++
  colMax--
  loop on rowMax from colMax to colMin.  col
    matrix[rowMax][col] becomes counter++
  rowMax--
  loop on colMin from rowMax to rowMin.  row
    matrix[row][colMin] becomes counter++
  colMin++
return matrix
*/

const spiralMatrix = (n) => {
  const matrix = [];
  let rowMin = 0,
    rowMax = n - 1,
    colMin = 0,
    colMax = n - 1,
    counter = 1;

  for (let i = 0; i < n; i++) {
    matrix.push(new Array(n).fill(0));
  }

  while (rowMin <= rowMax && colMin <= colMax) {
    for (let col = colMin; col <= colMax; col++) {
      matrix[rowMin][col] = counter++;
    }
    rowMin++;
    for (let row = rowMin; row <= rowMax; row++) {
      matrix[row][colMax] = counter++;
    }
    colMax--;
    for (let col = colMax; col >= colMin; col--) {
      matrix[rowMax][col] = counter++;
    }
    rowMax--;
    for (let row = rowMax; row >= rowMin; row--) {
      matrix[row][colMin] = counter++;
    }
    colMin++;
  }

  return matrix;
}

console.log(spiralMatrix(10));

Seems mostly solid to me. I just have two small remarks:

const spiralMatrix = (n) => {

I'm not a big fan of using the arrow syntax here. The arrow syntax is (mostly) used for inline functions. For a "top level" function I'd prefer a normal, more readable (and hoisted) function declaration:

function spiralMatrix(n) {

The only disadvantage I see is that const prevents accidental overwriting, which I don't see as an serious problem in this case.

And .fill(0) during initialization of the arrays is unnecessary.


Javascript assert function I need to compare some values of a Javascript object. If they fail to be identical, I want to pretty print to console.log the mismatch.

The initial code I have written which I find fugly after a JS beautify looks like:

function assert(a, b, message) {
if (a != b) {
    console.log("Failed to match " + message + " " + a + " != " + b);
    return false;
}
return true;
}

    if (!assert(parsedInput.protocol, cu.protocol, "protocol")) {
        continue;
    }
    if (!assert(parsedInput.port, cu.port, "port")) {
        continue;
    }
    if (!assert(parsedInput.hostname, cu.hostname, "hostname")) {
        continue;
    }
    if (!assert(parsedInput.hash, cu.hash, "hash")) {
        continue;
    }

Please tell me I've missed the plot and I could write this a lot better. Feel free to critique the rest of the code. Thank you!

I (and many other people) expect certain behavior from a function called "assert"...namely that it throws an exception or otherwise halts execution if the asserted condition isn't true.  If your assert function did that, you wouldn't have to test the return value.

function assertEqual(a, b, message)
{
    if (a != b) throw new Error(message + " mismatch: " + a + " != " + b);
}

assertEqual(parsedInput.protocol, cu.protocol, "protocol");
assertEqual(parsedInput.port,     cu.port,     "port");
assertEqual(parsedInput.hostname, cu.hostname, "hostname");
assertEqual(parsedInput.hash,     cu.hash,     "hash");

If that's not what you want, then you should seriously consider renaming your function.  Maybe check or something, i dunno.  Assertions die when they're false.

Now, as for your equality tests, you could do something like

function propsEqual(obj1, obj2, propNames) {
    var result = true;
    for (var i = 0; i < propNames.length; ++i) {
        var prop = propNames[i];
        if (obj1[prop] != obj2[prop]) {
            console.log("Failed to match " + prop +": " + obj1[prop] + " != " + obj2[prop]);
            result = false;
        }
    }
    return result;
}

if (!propsEqual(parsedInput, cu, ["protocol", "port", "hostname", "hash"])) {
    console.log("Skipping.");
    continue;
}

Benefits of this approach being, you get to see all of what doesn't match, rather than stopping at the first thing.


Check if a binary tree is a binary search tree or not This checks if both the left and right subtrees are binary search trees. If they are, then it compares the nodes data with the maximum element in the nodes' left subtree and minimum element in the nodes' right sub tree.

class Node{
    int data;
    Node left;
    Node right;
    Node(int x){
        data = x;
    }
}
public class isBST {

    public static Node getMin(Node root)
    {
        while(root.left != null)
        {
            root = root.left;
        }

        return root;
    }
    public static Node getMax(Node root)
    {
        while(root.right != null)
        {
            root = root.right;
        }

        return root;
    }
    public static boolean isBST(Node root)
    {
        // for null node
        if(root == null )
        {
            return true;
        }
        // for single node
        if(root.left == null && root.right == null)
        {
            return true;
        }
        // for a non null node
        if(isBST(root.left ) && isBST(root.right) )
        {
            if(root.left != null && root.right != null)
            {
            return root.data < getMin(root.right).data && root.data > getMax(root.left).data;
            }
            else
            {
                if(root.left != null)
                {
                    return root.data > getMax(root.left).data;
                }
                else
                {
                    return root.data < getMin(root.right).data;
                }
            }
        }
        else
        {
            return false;
        }
    }

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        Node root = new Node(5);
        root.left = new Node(3);
        root.right = new Node(7);
        root.left.left = new Node(2);
        root.left.right = new Node(4);
        root.right.left = new Node(6);
        root.right.right = new Node(9);
        System.out.println(isBST(root));
    }

}

Some quick notes:

Respect "Egyptian Braces" for your block structures (if, while, ...) i.e., the opening brace should be on the same line as the signature statement. The style you used (opening brace on a new line) is ordinarily used for C#, not Java.
Node could and should be as immutable as possible:

class Node {
    final int data;
    Node left;
    Node right;
    Node (int x) {
        data = x;
    }
}

isBST is not a good class name. Class names are usually nouns. I'd suggest (as was already in the currently deleted other answer) BinarySearchTreeUtil.
getMin and getMax are factually only correct if you assume that the Tree is actually a BST. Only checking for those two is not a sufficient indicator, though. A BST is only fully a BST if each and every node satisfies the condition you propose. Your code does not check for that.

You should instead verify that each subtree is a BST recursively. Aside from that you're also assuming that the BST is ordered with the smaller elements to the left. It's also correct for a BST to be ordered with smaller elements to the right. Your code doesn't deal with that either.

Overall your code does not correctly solve the problem. To remedy this I strongly suggest you invest some time into unit-testing your code. Furthermore, you're disregarding conventions about bracing (but you're consistent so it's somewhat acceptable), as well as class-names (that's not acceptable).


Sound manager for Android I created a singleton class for managing sound effects on Android. This class will only be instanced and loaded once at the beginning, and each activity will use the loaded songs.

I don't know either if this is the good approach for Singleton, nor if this is the good way to play sounds in Android. This is working like a charm, and I'm wondering about the resource utilizations.

public class SoundManager {

    private static SoundManager mInstance;

    private SoundPool mSoundPool;

    private HashMap<Integer, Integer> mSoundPoolMap;

    private Vector<Integer> mAvailibleSounds = new Vector<Integer>();
    private Vector<Integer> mKillSoundQueue = new Vector<Integer>();

    private Handler mHandler = new Handler();
    private boolean mMuted = false;

    private static final int MAX_STREAMS = 2;
    private static final int KILL_AFTER = 3000;

    public static final int SOUND_SELECT = 0;
    public static final int SOUND_LOCKED = 1;

    @SuppressLint("UseSparseArrays")
    private SoundManager(Context context) {
        mSoundPool = new SoundPool(MAX_STREAMS, AudioManager.STREAM_MUSIC, 0);
        mSoundPoolMap = new HashMap<Integer, Integer>();

        loadSounds(context);
    }

    public static SoundManager getInstance(Context context){
        if(mInstance == null){
            mInstance = new SoundManager(context);
            Log.d("SPARTA", "Instanciation");
        }
        return mInstance;
    }

    /**
    * Load all sounds and put them in their respective keys.
    * @param context
    */
    private void loadSounds(Context context){
        addSound(context, SOUND_SELECT, R.raw.metallic_knock);
        addSound(context, SOUND_LOCKED, R.raw.licorice);
    }

    /**
    * Put the sounds to their correspondig keys in sound pool.
    * @param context
    * @param key
    * @param soundID
    */
    public void addSound(Context context, int key, int soundID) {
        mAvailibleSounds.add(key);
        mSoundPoolMap.put(key, mSoundPool.load(context, soundID, 1));
    }

    /**
    * Find sound with the key and play it
    * @param key
    */
    public void playSound(int key) {
        if(mMuted)
        return;

        //If we have the sound
        if(mAvailibleSounds.contains(key)) {

            //We play it
            int soundId = mSoundPool.play(mSoundPoolMap.get(key), 1, 1, 1, 0, 1f);

            mKillSoundQueue.add(soundId);

            //And schedule the current sound to stop after set milliseconds
            mHandler.postDelayed(new Runnable() {
                public void run() {
                    if (!mKillSoundQueue.isEmpty()) {
                        mSoundPool.stop(mKillSoundQueue.firstElement());
                    }
                }
            }, KILL_AFTER);
        }
    }

    /**
    * Initialize the control stream with the activity to music
    * @param activity
    */
    public static void initStreamTypeMedia(Activity activity){
        activity.setVolumeControlStream(AudioManager.STREAM_MUSIC);
    }

    /**
    * Is sound muted
    * @param muted
    */
    public void setMuted(boolean muted) {
        this.mMuted = muted;
    }
}

Let me start by saying I have very limited Android experience. That said, I did look into the SoundPool class a bit and have some questions/comments about your implementation:

If you aren't already aware of some of the reasons why people consider Singletons an anti-pattern, I suggest you do some research on the topic and make sure you're ok with the downsides.
Your class, for the most part, seems to be wrapping a SoundPool with a key value pair structure despite SoundPool already being a key value pair structure. I would imagine you could do away with most of this class and just use the SoundPool class directly.
If you don't want to use SoundPool directly for some reason, why not at least use the soundId as a key? It already appears to be unique, and it's what the calling code probably wants anyway. If not, you may want to do some checking to make sure that duplicate keys aren't entered, or take the key assignment out of the hands of the caller and simply return an id as the result of the addSound function.
Using both mSoundPoolMap and mAvailibleSounds means you have to manage keys in two locations which could lead to them getting out of sync or some confusion as to why there are two fields holding the same information. Using only mSoundPoolMap would be sufficient and avoid duplication. This highly voted question isn't quite the same, but it touches on a lot of good points why this kind of duplication can be bad.

Hope this helps!


c# writing tests for WriteFileOuput I am new to writing unit tests and i am facing some difficult writing unit tests for this class.
I am using Xunit and moq.
public class WriteFileOutput: IWriteFileOutput
{
    public string File { get; set; }
    public WriteFileOutput(string fileName)
    {
        File = fileName;
    }

    public void writeFile(List<CustomerDistanceRecord> invitees)
    {
        var result = JsonConvert.SerializeObject(invitees, Formatting.Indented);
        if (System.IO.File.Exists(File))
        {
            System.IO.File.Delete(File);
            using(var tw = new StreamWriter(File, true))
            {
                tw.WriteLine(result.ToString());
                tw.Close();
            }
        }
        else if (!System.IO.File.Exists(File))
        {
            using(var tw = new StreamWriter(File, true))
            {
                tw.WriteLine(result.ToString());
                tw.Close();
            }
        }
    }
}

The code below tests that the writefileoutput writes out a file. I am not sure if this is a good unit test and I was wondering if someone could show me a better way of testing it class. Also I am not sure it deleting the file in the constructor before it is called is a good idea either. Thanks for the help.
 private readonly string _path = "../../../TestOutput/CustomerRecords.json";

    public WriteFileTests()
    {
        if (File.Exists(_path))
        {
            File.Delete(_path);
        }
    }

    [Fact]
    public void WriteFileOuput_should_be_called()
    {
        var fileWriteFileOutput = new WriteFileOutput($"{_path}");
        fileWriteFileOutput.writeFile(It.IsAny<List<CustomerDistanceRecord>>());
        Assert.True(File.Exists($"{_path}"));
    }

IMHO there is nothing in this class that requires integration testing. At most you should be able to mock it, but all of its functionality is calling methods that are presented by the framework (or by a NuGet package, e.g. the JSON serialization). If you feel the need to test those, then why not write a test to check that the return value of some method that returns a string is actually a string and not some other type of object?
You could argue that perhaps it might fail to create a file because it needs to do so on a share and that share is inaccessible, but then it is perfectly possible that this happens on production and not a single integration test will catch it. That's why exceptions exist and logging etc.
And that's where your code really fails: at no point do you apply a try...catch and do you log any possible IO exceptions. Sure, maybe your exception will bubble up and get caught elsewhere and reported, but you need to be sure about that. For instance: it is entirely possible an existing file cannot be deleted because another process has it locked.
Perhaps you should catch any IO exceptions here and return a bool to report back whether the file was written without issues (if you care about that, if other code depends on the file existing,...).

Your WriteFileOutput class is also pretty bad in other ways. Its name sounds like a method and is fairly undescriptive anyway. Moreover, you've obviously copy-pasted code instead of thinking through its logic. This does exactly the same while using far less lines and has no duplication:
    if (System.IO.File.Exists(File))
    {
        System.IO.File.Delete(File);
    }

    using(var tw = new StreamWriter(File, true))
    {
       tw.WriteLine(result.ToString());
       tw.Close();
    }

Some quick remarks:

What is even the point of else if (!System.IO.File.Exists(File))? What else could the else to if (System.IO.File.Exists(File)) even be?

public void writeFile doesn't follow the standards WRT naming (methods should be PascalCase).

Why is this public: public string File { get; set; }? Why can it be set from outside?

Both File and fileName are incorrect names. You're clearly passing a path to a file, not the name of a file (and "File" is even less descriptive).

result is too generic a name.

Are you saving other objects in much the same way as List<CustomerDistanceRecord> invitees? If so, do you have a specialized class for each of those? Because that seems overkill to me when the logic of writeFile (except for var result = JsonConvert.SerializeObject(invitees, Formatting.Indented);) can easily be reused.


Project Euler 1 in Java I have following exercise:

  If we list all the natural numbers below 10 that are multiples of 3 or
  5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below 1000.

So I wrote below code which finds the sum of all multiples of 3 or 5 below 1000.

Main.java:

package pl.hubot.projecteuler.problem1;

public class Main {
    public static void main(String[] args) {
        int sum = 0;
        for (int i = 1; i < 1000; i++) {
            if (i % 3 == 0 || i % 5 == 0) sum += i;
        }
        System.out.print(sum);
    }
}

How it code works?

I declared sum variable, iterated through i = 1 to i = 999 and if i is multiply of 3 or 5 then I add it to the sum. Finally, I printed out the sum variable.

Questions

Is it a efficient method to solve this exercise?
How can I simplify code?
How can I increase performance of this code?

I think it is very concise, well done!

For this small problem this will be fast enough.

Code only

I would only add braces around the if body:

 for (int i = 1; i < 1000; i++) {
        if (i % 3 == 0 || i % 5 == 0) {
            sum += i;
        }
 }

For performance, please note that modulo is kind of slow. You could implement two variables that hold multiples of 3 and 5.

    public static void main(String[] args) {
        int threes = 0;
        int fives  = 0;
        int sum    = 0;
        for (int i = 1; i < 1000; i++) {
            if (i>threes)
                threes+=3;
            if (i>fives)
                fives+=5;
            if (i == threes || i == fives )
                sum+=i;
        }
        System.out.print(sum);
    }

Math

Better will be to go purely mathemathical:

  The formula for the sum of an arithmetic series is:

  Sn = (n/2) * (a1 + an)

  Sn = the sum of the n terms in the sequence.

  a1 = the first term in the sequence.

  an = the nth term in the sequence.

We need to determine the number of items 'n' in the sequence. For simplicity I name this occurrences.

This is calculated as dividing the limit - 1 by number. Because the integer division, it is rounded down.

The number an is the last multiple that fits below the limit, which is equal to occurrences * number

Calculate the sum om the series for threes, fives and fifteens. The solution is:

Sum(threes) + Sum(fives) - Sum(fifteens)

In code:

public static int sum( int limit, int number )
{
    int occurrences = (limit-1) / number;
    //Below is a rewrite ( thnx to Olivier Grégoire) of:
    // (int) ( ( occurrences / 2.0 ) * (  occurrences * number + number) );
    return number * occurrences * (occurrences + 1) / 2;
}
public static void main( String[] args )
{
    System.out.println(  sum(1000,3) + sum(1000,5) - sum(1000, 15));
}


Effective way to handle multiple time string to timestamp I have 4 cases of time string as follow:

s = "July 5, 2016, 12:04 a.m."
s = "July 5, 2016, 7 a.m."
s = "July 5, 2016, midnight."
s = "July 5, 2016, noon."

I want to convert string to timestamps. Based on this question, I need to declare 4 cases of pattern. I am looking forward to improve my solution.

The following code is my solution:

import time
import datetime

s1 = "July 5, 2016, 12:04 a.m."
s2 = "July 5, 2016, 7 a.m."
s3 = "July 5, 2016, midnight."
s4 = "July 5, 2016, noon."

def convert(timestring):
    # clear dot
    timestring = timestring.replace(".","")
    # convert s3 and s4 to s1 and s2
    timestring = timestring.replace("midnight", "12:00 am") if "midnight" in timestring else timestring
    timestring = timestring.replace("noon", "12:00 pm") if "noon" in timestring else timestring
    result = None
    try:
        # try to match s2
        print(timestring)
        result = time.mktime(datetime.datetime.strptime(timestring, "%B %d, %Y, %I %p").timetuple())
    except ValueError as e:
        try:
            result = time.mktime(datetime.datetime.strptime(timestring, "%B %d, %Y, %I:%M %p").timetuple())
        except ValueError as e:
            print(e)
    print(result)

def main():
    convert(s1)
    convert(s2)
    convert(s3)
    convert(s4)

if __name__ == '__main__':
    main()

It's not really appropriate to print the result or the exception message instead of returning the result or throwing the exception.

It's not right to eliminate all dots from the input before proceeding.

I think that a good strategy would be to transform the input into a canonical representation.  You've started along that path by replacing "midnight" and "noon" with "12:00 am" and "12:00 pm", respectively.  You could go further and set the minutes to ":00" if necessary.  To accomplish that, I would use regular expression matching.

A doctest seems very appropriate here, both for documentation and as a unit test.

from datetime import datetime
import re
from time import mktime

def convert(timestring):
    """
    Convert a time string, in one of four accepted formats, in the
    local time zone, into the number of seconds since the Epoch.

    >>> import os, time
    >>> os.environ['TZ'] = 'US/Pacific'
    >>> time.tzset()
    >>> convert("July 5, 2016, 12:04 a.m.")
    1467702240.0
    >>> convert("July 5, 2016, 7 a.m.")
    1467727200.0
    >>> convert("July 5, 2016, midnight.")
    1467702000.0
    >>> convert("July 5, 2016, noon.")
    1467745200.0
    >>> convert("2016-07-05 12:34:56")
    Traceback (most recent call last):
     ...
    ValueError: time data '2016-07-05 12:34:56' does not match format '%B %d, %Y, %I:%M %p'
    """
    match = re.match(
        '(?P<date>.*)\s+'
        '(?:(?P<noon>noon)|midnight|'
           '(?P<hour>1?[0-9])(?::(?P<minute>[0-5]?[0-9]))?'
           '\s+(?:(?P<pm>p)|a)\.?m)\.$',
        timestring
    )
    if match:
        # Canonicalize the time of day as "HH:MM am/pm"
        timestring = '{date} {hh}:{mm} {ampm}'.format(
            date=match.group('date'),
            hh=match.group('hour') or '12',
            mm=match.group('minute') or '00',
            ampm=['am', 'pm'][bool(match.group('pm') or match.group('noon'))]
        )
    return mktime(
        datetime.strptime(timestring, '%B %d, %Y, %I:%M %p').timetuple()
    )


Confirming the presence/create directories While preparing an automated process to manipulate files, I've made the following function to check for the base directory and sub-directories presence, as to allow or deny the remaining code to be executed:

/* Check Directories
 *
 * Confirm that the necessary directories exist, and if not,
 * tries to create them.
 *
 * @_param string $whereAmI    -> Script location
 * @_param string $backup_path -> target directory relative path
 * @_param array  $subDirArr   -> sub-directories array
 *
 * @return
 *  success -> Boolean
 *  failure -> string with failure path
 */
function check_directories($whereAmI, $backup_path, $subDirArr) {

  // initialize return variable
  $status = FALSE;

  // check for the merchant base directory
  if (!is_dir($whereAmI."/".$backup_path)) {

      // trying to create the directory with permissions
      exec("mkdir ".$backup_path." -m 777 2>&1", $output, $return_value);

      // error returned, stop and return status
      $status = (is_array($output) && count($output)>=1) ? $backup_path : TRUE;
  } else {
    $status = TRUE;
  }

  // base directory exists, continue
  if ($status===TRUE) {

      // run by each sub-directory
      foreach ($subDirArr as $subDir) {

          /* keep checking for status value, if changed,
           * one directory has failed and we can't proceed
           */
          if ($status===TRUE) {

              // check for the sub-directory presence
              if (!is_dir($whereAmI."/".$subDir)) {

                  // trying to create the directory with permissions
                  exec("mkdir ".$subDir." -m 777 2>&1", $output, $return_value);

                  // error returned, stop and update status
                  $status = (is_array($output) && count($output)>=1) ? $subDir : TRUE;
              }

          } else {
              return $status;
          }
      }
  }

  return $status;
}

Considerations:

Directory permissions when in production aren't 777.
Server is running PHP Version 5.3.10.

Can this be simplified, thus reducing the amount of code, and perhaps in such reduction, have it improved?

Quite a few things wrong with your PHPDoc. I'm actually learning quite a bit myself. Doccomments require two asterisks to begin, not one, no matter what language you are using. One asterisk just defines a multi-line comment. Your IDE will skip over this, essentially making these useless as doccomments. The underscore before param, the "yields" operator ->, and your return syntax all cause my IDE to ignore this comment because it is in the wrong syntax. The multi-return sequences are best done using the bitwise OR operator separating the types, but you can also use a "mixed" type to accomplish the same thing.

/** Check Directories
  * etc...
  * @param string $whereAmI Script location
  * @return bool|string String on success, FALSE on failure
//OR
  * @return mixed String on success, FALSE on failure
  */

Be careful with "clever" variables. For a few seconds there I was quite confused about your $whereAmI variable, thinking that last letter was an "L" and not an "I". I'm a bit slow sometimes and I was sitting there scratching my head trying to figure out what an "aml" was (in case you're curious, my best guess was animal). I would stick with simple variable names, such as $location or $position. Additionally, pick a style and be consistent. I see both camelCase AND under_score variables. One or the other, not both. You can do one for functions and the other for variables, if you wish. I've seen that done frequently enough that I wouldn't comment on it, but switching between the styles on the same datatype is just confusing.

Internal comments, especially ones that explain what is already in the doccomments, are just noise. Anyone reading this function will know that $status is important, and once they reach the end of the function they will know its also returned. If, like me, you skip to the bottom of the function to see what it returns and then work backwards from there, then you will already know that it is returned. Same for the rest of these comments. The only time I use internal comments is for explaining confusing lines, such as REGEX, compact(), extract(), or, in your case, that exec().

Alright, lets move on to actual code.

Why did you bother setting an initial value to $status, your very next line of code overwrites that value no matter what. Let the if/else statements assign it for you. Or lose the else and use that as the default value.

Avoid using "not" ! unless you have to. In this instance you have an if/else statement. Switch them around and remove the "not". In the above scenario you would keep the "not".

if( is_dir( $whereAmI . '/' . $backup_path ) ) {
    $status TRUE;
} else {

Explaining Not:

If you are doing a single if statement, then the not is fine and necessary. Though this could be argued that the empty() function should be used, either is fine, but I find the "not" syntax preferrable in this case.

if( ! $var ) {//fine
//OR
if( empty( $var ) ) {//also fine, though less preferrable IMO

If you are using an if/else statement, as I demonstrated in initial answer, the not is implied by else and should be removed for clarity as well as efficiency.

if( $var ) {
} else {//not implied here
}

This helps with clarity because the "not" operator ! is not always immediately noticeable, depending on how you space your statements (which is one of the reasons my statements are so spacious). This helps with efficiency because typically your "not" clause holds the least amount of coding and might even contain an early escape, resulting in the else syntax not even being necessary which meaning less indentation and increased clarity.

if( $var ) {
    return FALSE;
}
//contents of else

The only time to compare a value explicitly to a boolean is when doing a type comparison. Such as an integer "0" to a boolean "FALSE", or string "FALSE" to FALSE. And the above rules hold true for this as well.

if( $var !== FALSE ) {
//OR
if( $var === FALSE ) {
} else {
}
//OR
if( $var === FALSE ) {
    return FALSE;
}
//contents of else

End of Explanation

You mentioned security; You should be careful of functions such as exec(). Functions, by definition, are public. So, this script is public, therefore you can not guarantee the validity of those parameters being passed to it, or rather, you aren't validating them. This is a potential security issue, but only if you plan on using this in a public domain (internet). That and the "777" permissions. Directories should not be executable. I can't remember off the top of my head what the proper permissions to use are, but I DO know that "777" will get you in trouble. Though, admittedly I do this in my server as well, but mine is a small intranet and security isn't one of the things I'm particularly worried about.

Split up your function. It is concerned with entirely too much. check_directories() should only be concerned with checking the directories, but then you start adding on subdirectories. This is a different concern altogether. Take a look at the Single Responsibility Principle. Additionally, look at "Don't Repeat Yourself" (DRY) Principle. Your if/else statement that I mentioned above is repeated, therefore it can be made into a function to avoid this. This will be the biggest contributor to reducing the size of this program. That and removing those unnecessary comments. Besides that, I don't think there are any other ways to do this.


Calculating relative strength index over a period I'm looking for a review on my code. I am also looking for ways to transform this function into something more Pythonic. I'm fairly new to Python, so any advice would be appreciated.

def get_rsi(self, period):
    rsi = []

    for i in xrange(1, len(self.hist_d) - period + 1):
        gains = 0.0
        losses = 0.0

        for j in xrange(i, i + period):
            diff = self.hist_d[j][2] - self.hist_d[j - 1][2]

            if diff > 0:
                gains += diff
            elif diff < 0:
                losses += abs(diff)

        rsi.append(round(100 - (100 / (1 + gains / losses)), 2))

    return rsi

Your code is already pretty Pythonic.

There is a bug in your code. If no losses were reported in that window then losses == 0.0 which when you append to rsi will throw a ZeroDivisionError.

There are two suggestions I would make:

Use zip (or izip if possible)

The zip function takes two iterables and combines them. It essentially returns:

>>>zip([1,2,3], ['a','b','c'])
[(1, 'a'), (2, 'b'), (3, 'c')]

We can use this instead of directly indexing through j and j-1.
If-Else

Currently your if-statement looks like this:

if diff > 0:
    gains += diff
elif diff < 0:
    losses += abs(diff)

I would recommend making the elif into a simple else. This is because anything that the elif won't catch is if diff == 0 and since losses == losses + 0, it won't effect losses. Also, you remove a comparision using a simple else.

With these suggestions taken into account (plus the simple bug fix), we can refactor your code a little bit:

def get_rsi(self, period):
    rsi=[]
    for i in range(len(self.hist_d)-period):
        gains = 0.0
        losses = 0.0

        window = self.hist_d[i:i+period+1]
        for year_one, year_two in zip(window, window[1:]):
            diff = year_two - year_one

            if diff > 0:
                gains += diff
            else:
                losses += abs(diff)

        # Check if `losses` is zero. If so, `100/(1 + RS)` will practically be 0.
        if not losses:
            rsi.append(100.00)
        else:
            rsi.append(round(100 - (100 / (1 + gains / losses)), 2))

    return rsi


What would be a correct C++ style for function loading file contents? bool loadFileContents(const std::string& fileName, std::string &out) {
    bool res = false;
    FILE* file = fopen(fileName.c_str(), "r");
    char buf[128] = { 0 };
    if (file) {
        while (fread(buf, 1, sizeof(buf), file))
            out += std::string(buf);
        res = true;
    }
    fclose(file);
    return res;
}

The rationale behind using std::string for the out parameter is that it handles the buffer underneath. However, I feel like it's a move from C-style, but not quite a C++ one.

Should I consider using some kind of stream for the out param?

Here are some of the C-style features of your code:

FILE
fopen
char buf [128]
fclose

I personally liked the answer that was given in @Jamal's link.

std::ifstream fin(fileName);
std::ostringstream oss;

oss << fin.rdbuf();
out = oss.str();

The only problem with this is that it doesn't work well with binary files.
You could make out be a std::vector <char> instead.

std::vector <char> loadFileContents (const std::string &filename)
{
    std::ifstream ifile ;
    ifile.exceptions (std::ifstream::badbit | std::ifstream::failbit) ;

    std::vector <char> vecBuffer ;
    typedef std::vector <char>::size_type size_type ;

    try {
        ifile.open (filename) ;
        ifile.seekg (0, ifile.end) ;
        size_type size = static_cast <size_type> (ifile.tellg ()) ;
        ifile.seekg (0, ifile.beg) ;
        vecBuffer.resize (size) ;
        ifile.read (&vecBuffer [0], size) ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () << std::endl ;
    }

    return vecBuffer ;
}

NRVO allows us not to worry about returning the std::vector <char> by value.

If you have access to C++11, then you could do:

std::vector <char> loadFileContents (const std::string &filename)
{
    std::ifstream ifile ;
    ifile.exceptions (std::ifstream::badbit | std::ifstream::failbit) ;

    std::vector <char> vecBuffer ;

    try {
        ifile.open (filename) ;
        std::istreambuf_iterator <char> iter (ifile) ;
        std::istreambuf_iterator <char> eos ;
        vecBuffer = std::move (std::vector <char> (iter, eos)) ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () << std::endl ;
    }

    return vecBuffer ;
}

If you want to enter template territory, then you could try this:

template <typename Container>
Container loadFileContents (const std::string &filename)
{
    std::ifstream ifile ;
    ifile.exceptions (std::ifstream::badbit | std::ifstream::failbit) ;

    try {
        ifile.open (filename) ;
        std::istreambuf_iterator <char> iter (ifile) ;
        std::istreambuf_iterator <char> eos ;
        return Container (iter, eos) ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () << std::endl ;
    }

    return Container () ;
}

Then in your main, you could do:

int main (void)
{
    std::string strData = loadFileContents <std::string> (FILENAME) ;
    std::vector <char> vecData = loadFileContents <std::vector <char> > (FILENAME) ;

    return 0 ;
}

If we choose to not return the Container, then we could do:

template <typename Container>
void loadFileContents (const std::string &filename, Container &container)
{
    std::ifstream ifile ;
    ifile.exceptions (std::ifstream::badbit | std::ifstream::failbit) ;

    try {
        ifile.open (filename) ;
        std::istreambuf_iterator <char> iter (ifile) ;
        std::istreambuf_iterator <char> eos ;
        container = Container (iter, eos) ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () << std::endl ;
    }
}

int main (void)
{
    std::string strData ;
    std::vector <char> vecData ;

    loadFileContents (FILENAME, strData) ;
    loadFileContents (FILENAME, vecData) ;

    return 0 ;
}

The benefit of this one is that we wouldn't have to supply template arguments in main.


Speeding up Python program that converts DOCX to PDF in Windows This is meant to be a performance-centric question as this type of conversion is obviously very common. I'm wondering about the possibilities for making this process faster.
I have a program that creates several thousand QR codes from a list, embeds them in an MS Word docx template, and then converts the docx files to pdf. Problem is, what I've designed is very slow. When creating several thousand pdf files, it takes hours on a local machine.
What can I do to speed this program up? Is there a way to multithread it? (Total newb to that topic). Or, what about my program design is inherently flawed?
Repeatable program below, meant to be run in local Windows 10 directory:
import pyqrcode
import pandas as pd
from docx import Document
from docx.enum.text import WD_ALIGN_PARAGRAPH
import glob
import os
from docx2pdf import convert

def make_folder():
    os.mkdir("codes")
    os.mkdir("docs")

def create_qr_code():
    for index, values in df.iterrows():
        data = barcode = values["barcode"]
        image = pyqrcode.create(data)
        image.png("codes\\"+f"{barcode}.png", scale=3)

def embed_qr_code():
    qr_images = glob.glob("codes\\"+"*.png")
    for image in qr_images:
        image_name = os.path.basename(image)
        doc = Document()
        doc.add_picture(image)
        last_paragraph = doc.paragraphs[-1]
        last_paragraph.alignment = WD_ALIGN_PARAGRAPH.CENTER
        doc.save("docs\\"+f"{image_name}.docx")
        convert("docs\\"+f"{image_name}.docx")

def clean_file_names():
    paths = (os.path.join(root, filename)
        for root, _, filenames in os.walk("docs\\")
        for filename in filenames)
    for path in paths:
        newname = path.replace(".png", "")
        if newname != path:
            os.rename(path, newname)

data = {'barcode': ['teconec', 'tegovec', 'teconvec', 'wettrot', 'wetocen']}
df = pd.DataFrame(data)

make_folder()
create_qr_code()
embed_qr_code()
clean_file_names()

Thank you!

I recently learned how easy it is to incorporate multiprocessing and multithreading into a Python program, and would like to share it with you.
Python's built-in multiprocessing module offers a simple way to add multiprocessing to a program. However, since your program performs a lot of writes, I think it may be better to use the multiprocessing.dummy module instead. This module offers the same API as the multiprocessing module, but is used for multithreading instead of multiprocessing, and thus is better suited to programs that are IO intensive.
First, import the Pool class from the multiprocessing.dummy module:
import Pool from multiprocessing.dummy as ThreadPool
I aliased it as ThreadPool just for added clarity that we are using multithreading and not multiprocessing. Next, take a look at all the for loops in your code. To add multithreading, we will have to change those for loops to call a function on each iteration instead of performing a series of steps:
def create_qr_code(values):
    data = barcode = values["barcode"]
    image = pyqrcode.create(data)
    image.png("codes\\"+f"{barcode}.png", scale=3)

def create_qr_codes():
    for index, values in df.iterrows():
        create_qr_code(values)

def embed_qr_code(image):
    image_name = os.path.basename(image)
    doc = Document()
    doc.add_picture(image)
    last_paragraph = doc.paragraphs[-1]
    last_paragraph.alignment = WD_ALIGN_PARAGRAPH.CENTER
    doc.save("docs\\"+f"{image_name}.docx")
    convert("docs\\"+f"{image_name}.docx")

def embed_qr_codes():
    qr_images = glob.glob("codes\\"+"*.png")
    for image in qr_images:
        embed_qr_code(image)

def clean_file_name(path):
    newname = path.replace(".png", "")
    if newname != path:
        os.rename(path, newname)

def clean_file_names():
    paths = (os.path.join(root, filename)
        for root, _, filenames in os.walk("docs\\")
        for filename in filenames)
    for path in paths:
        clean_file_name(path)

Now for the fun part. In each of the functions that use for loops, we will replace the for loops with usages of ThreadPools as context managers. We can then call each ThreadPool's map method to perform the actions using multithreading:
def create_qr_codes():
    rows = (values for _, values in df.iterrows())
    with ThreadPool() as pool:
        pool.map(create_qr_code, rows)

def embed_qr_codes():
    qr_images = glob.glob("codes\\"+"*.png")
    with ThreadPool() as pool:
        pool.map(embed_qr_code, qr_images)

def clean_file_names():
    paths = (os.path.join(root, filename)
        for root, _, filenames in os.walk("docs\\")
        for filename in filenames)
    with ThreadPool() as pool:
        pool.map(clean_file_name, paths)

Note that when instantiating a Pool, you can pass a value to it to specify the number of processes (or in our case, threads) to use. According to the Python docs:

If processes is None then the number returned by os.cpu_count() is used.

Also, don't forget to change these two function calls...
create_qr_code()
embed_qr_code()

...to these:
create_qr_codes()
embed_qr_codes()

This should speed up your program significantly. If not, try using the multiprocessing module instead of multiprocessing.dummy.
Finally, one extra tip. You may want to refactor this function:
def make_folder():
    os.mkdir("codes")
    os.mkdir("docs")

To use the os.makedirs function instead to avoid raising an error if the directories already exist:
def make_folder():
    os.makedirs("codes", exist_ok=True)
    os.makedirs("docs", exist_ok=True)


Positive, Negative or zero in a 2D array python I'm trying to find a way to know if a number on a matrix is positive, negative or zero. Below is my code, but I'm wondering if there is a more Pythonic way to write that. Any ideas?
array = np.random.randint(-10,10, size=(10,10))

def function(array):
    case1 = np.where(array < 0, -1, array)
    case2 = np.where(case1 > 0, 1, case1)
    case3 = np.where(case2 == 0, 0, case2)
    return case3

print(function(array))

You should choose better functions names, function does not give any indication what the purpose of the function is.
In addition, a docstring comment can be used to give a short description.
But actually there is no need for a custom function because numpy.sign already provides the exact functionality:

Returns an element-wise indication of the sign of a number.

Example:
import numpy as np

array = np.random.randint(-10,10, size=(4,4))
print(np.sign(array))

Output:

[[-1  1  1  1]
 [ 1  1  0  0]
 [ 1  1 -1  1]
 [ 1 -1  1 -1]]


Is this a good implementation of the Factory Pattern? Suppose I have this code:

public interface BaseType {
         public void doSomething();
    }

public class ExtendedTypeA implements BaseType {
//No Instance Variables

    @Override
    public void doSomething() {
        //really do something
    }
}

public class ExtendedTypeB implements BaseType {
//No instance variables

    @Override
    public void doSomething() {
        //really do something, but different
    }
}

public enum BaseTypesValues { EXTENDED_TYPEA, EXTENDED_TYPEB }

public BaseTypeFactoryStandard {

    public BaseType getBaseType(BaseTypesValues baseTypeValue) {
        switch(baseTypes) {
            case BaseTypesValues.EXTENDED_TYPEA:
                return new ExtendedTypeA();
            case BaseTypesValues.EXTENDED_TYPEB:
                return new ExtendedTypeB();
            default:
                throw new NoSuchTypeException();
        }
    }
}

public BaseTypeFactoryMyWay {
    public static final Map<BaseTypesValues, BaseType> factoryMap = new HashMap<//...

    static {
        factoryMap.put(BaseTypesValues.EXTENDED_TYPEA, new ExtendedTypeA());
        factoryMap.put(BaseTypesValues.EXTENDED_TYPEB, new ExtendedTypeB());
    }

    public BaseType getBaseType(BaseTypesValues baseTypeValue) {
        return factoryMap.get(baseTypeValue);
    }

}

Is the last class a good/valid implementation of the factory pattern? Take into consideration the fact that classes that implement the BaseType DO NOT have a state (no non-final instance variables); this means that the objects are lightweight.

Also, can it be implemented in this way (using a map) for the general case (in which classes have state).

Or is this a dumb way in either cases?

Two things strike me:

There's no need to have a separate factory class when you could put the functionality into the enum, unless you expect to have other factory implementations
As you say, if the classes are stateless, there's no need to create a new instance on each call. (I've only just spotted your factory map class, which effectively does something like this, but there's no need to use a map.)

Combining these:

public enum BaseTypesValues {
    EXTENDED_TYPEA(new ExtendedTypeA()),
    EXTENDED_TYPEB(new ExtendedTypeB());

    private final BaseType instance;

    private BaseTypesValues(BaseType instance) {
        this.instance = instance;
    }

    public BaseType getBaseType() {
        return instance;
    }
}

Then to use:

BaseType type = EXTENDED_TYPEA.getBaseType();

Or:

public void doSomething(BaseTypesValue baseTypeValue) {
    baseTypeValue.getBaseType().someCallOnTheBaseType();
}

EDIT: Note that here, if you want to introduce a new type which isn't stateless, it could override the getBaseType method:

EXTENDED_TYPEC(null) {
    @Override public BaseType getBaseType() {
        return new ExtendedTypeC();
    }
};

Admittedly this would break code which assumed all implementations were stateless...


Removing duplicate hashes in an array while keeping count of the times the hash was present I have an array of hashes (price_params['items']) where each item has a key called quantity what I'm trying to do is to clean every duplicate of each item but keeping the count of the times the item was found in the array in the unique one that I'm leaving (using the quantity key to keep that value)

This is my code so far:

def clean_duplicated_offers
  price_params['items'].each do |item|
    max_count = price_params['items'].count(item)
    next unless max_count > 1
    price_params['items'].delete(item)
    item['quantity'] = max_count
    price_params['items'] << item
  end
end

It works well, but it feels weird to delete every occurrence of the item in the array just to add it back to it.

Is there any better way to achieve this?

Does it have to be an Array? I.e. is the order important?

If not, then there is actually a data structure that does exactly what you want: the multiset. A multiset is just like a set, except that its elements have a multiplicity. In other words, a set can tell you whether or not an element is a member, a multiset in addition can tell you how often it is a member.

Basically, if you use a multiset, you will not have to do anything, since the multiset keeps track of the multiplicity (i.e. your quantity) for you.

There is no multiset in the Ruby core library or standard library, but there are a couple of third-party libraries and gems. I'll just grab one randomly, it doesn't really matter which one; their APIs are fairly similar.

require 'multiset'

price_params_items = %w[item2 item1 item3 item2 item3 item3]

result = Multiset[*price_params_items]
#=> #<Multiset:#2 "item2", #1 "item1", #3 "item3">

And that's it! You might ask yourself, where is the algorithm gone? That is a general property of programming: if you find the right data structure(s), the algorithm(s) become(s) much simpler, or in this case, even vanishes completely.

Unfortunately, for this specific implementation of multiset, there is no direct way to retrieve the multiplicity of an element, but you can convert it to a hash, and then you get what you need:

result.to_hash
#=> { "item2" => 2, "item1" => 1, "item3" => 3 }


Is it normal to have a base type for Entities and Value Objects? Please see the code below, which I am currently using in a production application. :

public abstract class Entity
{
    public virtual long Id { get; protected set; }

    public override bool Equals(object obj)
    {
        var compareTo = obj as Entity;

        if (ReferenceEquals(compareTo, null))
            return false;

        if (ReferenceEquals(this, compareTo))
            return true;

        if (GetRealType() != compareTo.GetRealType())
            return false;

        if (!IsTransient() && !compareTo.IsTransient() && Id == compareTo.Id)
            return true;

        return false;
    }

    public static bool operator ==(Entity a, Entity b)
    {
        if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
            return true;

        if (ReferenceEquals(a, null) || ReferenceEquals(b, null))
            return false;

        return a.Equals(b);
    }

    public static bool operator !=(Entity a, Entity b)
    {
        return !(a == b);
    }

    public override int GetHashCode()
    {
        return (GetRealType().ToString() + Id).GetHashCode();
    }

    public virtual bool IsTransient()
    {
        return Id == 0;
    }

    public virtual Type GetRealType()
    {
        return NHibernateUtil.GetClass(this);
    }
}

Every Entity class inherits from it.

It is a slight variation of the code here: https://github.com/vkhorikov/DddInAction/blob/master/DddInPractice.Logic/Common/Entity.cs

I am trying to decide whether to remove this base entity type from my application because of the feedback I have received when talking about it.

Is it normal to have a base Entity type (and base ValueObject) that has a base implementation for comparisons e.g. Object.Equals, IEquality.Equals etc?

It seems logical to me.  Say I have the following entities:

Customer
Supplier
Product
Production
etc

Why is it a bad idea for them all to be compared by ID by default?

I have a separate domain model and data model if that has any bearing.

Review

The following methods are candidates for properties. A property is expected to return fast, without too much calculations and internal flow. I'm assuming NHibernateUtil.GetClass provides a fast lookup.

public virtual bool IsTransient()
{
    return Id == 0;
}

public virtual Type GetRealType()
{
    return NHibernateUtil.GetClass(this);
}

public virtual bool IsTransient => Id == 0;
public virtual Type RealType => NHibernateUtil.GetClass(this);

When to use a base class

Since C# allows for single class inheritance, pick a base class that adds sufficient state / operations to the class. Providing base classes like EqualityBase, ComparableBase, DisposableBase, HashCodeBase are bad choices, since they don't provide meaningful context except for a very specific mitigation of boiler-plate code. Which class should Entity implement of the above? You would like to implement all of them, but you can't!
Is Entity provided by the OP a good choice for a base class? If the IsTransient() and GetRealType() methods weren't provided, I would argue against using a base class. You would end up with a class like above (EqualityBase, HashCodeBase mix). Both added methods allow for sufficient context to justify a base class.
Use the power of interface inheritance, because you can implement as many interfaces as you want, and composition, because you can contain and delegate operations to as many contained objects you desire.

Dilemma

Using a base class allows for DRY code. The repetitive boiler-plate code sits in the base class, while the entities are very clean POCO's. However, you force the entities to derive from this base class, limiting the inheritance options for such classes drastically.

Solution

To allow for flexibility in inheritance options for your entities, and mitigating most boiler-plate code, you could provide an interface, a base class and let your entities implement both. The special entities that require to inherit from a different base class, should do so, and implement the interface.

public interface IEntity
{
    long Id { get; }
    bool IsTransient { get; }
    Type RealType { get; }
}

public abstract class EntityBase : IEntity
{
    public virtual long Id { get; protected set; }
    public virtual bool IsTransient => Id == 0;
    public virtual Type RealType => NHibernateUtil.GetClass(this);

    // .. other code (equals, operator overloads, ..)
}

Most entities would implement both the base class and interface:

public class Employee : EntityBase
{
    public string Name { get; set; }
    public string FirstName { get; set; }
    public string BadgeId { get; set; }
}

Allowing for special cases:

public class DocumentElement : TreeNode<DocumentElement>, IEntity
{
    // .. DocumentElement  members
    // .. IEntity members
}

Other layers using the entities don't care about the base class, only the interface.

public interface IRepository<T> where T : IEntity
{
    T Get(int id);
    void Save(T entity);
}


Date format function in JavaScript How can I improve the date format function. I have written in javascript (ES5/ES6)? This code is just a part of an exercise.

function formatDate() {
        let bookingDate = new Date();
        let dayNumber = bookingDate.getDay();
        let date = bookingDate.getDate();
        let month = bookingDate.getMonth();
        let year = bookingDate.getFullYear();

        return ` ${getWeekday(dayNumber)}, ${monthName(
      month
    )}  ${date},  ${year}`;

        function getWeekday(dayNumber) {
            let day = bookingDate.getDay();

            let DayName = [
                "Sunday",
                "Monday",
                "Tuesday",
                "Wednesday",
                "Thursday",
                "Friday",
                "Saturday"
            ];

            return DayName[dayNumber];
        }

        function monthName(month) {
            let monthNames = [
                "January",
                "February",
                "March",
                "April",
                "May",
                "June",
                "July",
                "August",
                "September",
                "October",
                "November",
                "December"
            ];

            return monthNames[month];
        }
    }

Why are you getting day in function getWeekday and never using it?
Use a IIF (Immediately Invoked Function) to close over constants so they do not need to be created each time you call the function that uses them (Note optimizer will cache such constants, though not on the first pass)
Don't add unneeded details to names. booking is unrelated to the functions role, and should not be part of the name.
You have added complexity where not needed.

The functions getWeekday and monthName are just array look ups and can be done directly via the index you pass the functions.
Almost all the variables you use are once off uses, you can side step the source and execution overhead by referencing the data directly.

Be consistent in naming getWeekday and monthName do about the same thing yet the names are completely different. dayName and monthName would be more consistent.
Use constants for variables that do not change.

Example 1

Use an IIF to define constants and returns a function that formats the date.
Added date as an argument that defaults to new Date

Rather than creating an array by hand I use a short cut to convert a delimited string to an array. I find this easier to create string arrays this way.

const formatDate = (() => {
    const DAYS = "Sunday,Monday,Tuesday,Wednesday,Thursday,Friday,Saturday".split(",");
    const MONTHS = "January,February,March,April,May,June,July,August,September,October,November,December".split(",");
    return (date = new Date()) => ` ${DAYS[date.getDay()]}, ${MONTHS[date.getMonth()]}  ${date.getDate()},  ${date.getFullYear()}`;
})();

Example 2

Keeping to your original logic and layout, but behind an IIF. Note all the constants.

const formatNow = (() => {
    const DAYS = ["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"];
    const MONTHS = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"];
    const dayName = idx => DAYS[idx];
    const monthName = idx => MONTHS[idx];
    function format() {
        const now = new Date();
        const day = now.getDay();
        const date = now.getDate();
        const month = now.getMonth();
        const year = now.getFullYear();
        return ` ${dayName(day)}, ${monthName(month)}  ${date},  ${year}`;
    }
    return format;
})();


WAKE UP! CodingBat alarm clock I'm brand new at Python, but I've been working in general at granulating my functions and being as self-readable as possible without suffering readability. This CodingBat question is a little interesting because it demands a function with a lot of repetitive case checks. I decided, rather than having a bunch of nested ifs, it might be better to break them out into separate functions, but I'm not sure if this actually goes -against- my goal of better readability. However, I decided to break out is_weekday because the number encoding wasn't immediately clear. I also wasn't sure if it'd be better to have alarm_time(is_weekday(day), '10:00', 'off') or do alarm_time(day, '10:00', 'off') and have alarm_time handle the is_weekday(day) check. I decided the first way because it seemed there was a separation of concern issue otherwise, but I'm not sure.

I was at first hesitant to ask because this might be gray-space into asking a general opinion, but I'm specifically asking in terms of this exercise to use in similar situations.

  Given a day of the week encoded as 0=Sun, 1=Mon, 2=Tue, ...6=Sat, and a boolean indicating if we are on vacation, return a string of the form "7:00" indicating when the alarm clock should ring. Weekdays, the alarm should be "7:00" and on the weekend it should be "10:00". Unless we are on vacation -- then on weekdays it should be "10:00" and weekends it should be "off".

def is_weekday(day):
    return 1 <= day <= 5

def alarm_time(is_weekday, weekday_time, weekend_time):
  if is_weekday:
    return weekday_time
  return weekend_time

def alarm_clock(day, vacation):
  if vacation:
    return alarm_time(is_weekday(day), '10:00', 'off')
  return alarm_time(is_weekday(day), '7:00', '10:00')

def test_fun(fun):
    cases = [[1, False, '7:00'], [5, False, '7:00'], [0, False, '10:00'], [6, False, '10:00'], [0, True, 'off'], [6, True, 'off'], [1, True, '10:00'], [3, True, '10:00'], [5, True, '10:00']]

    print('{:^30}     {:^11}'.format('Expected', 'Run'))
    for case in cases:
        funstr   = '{}({}, {})'.format(fun.__name__, case[0], case[1])
        result   = fun(case[0], case[1])
        expected = case[-1]
        success  = result == expected

        print('{:21} -> {:5} ==> {:5}  {}'.format(funstr, expected, result, success))

The test cases were positive:

>>> test_fun(alarm_clock)
           Expected                    Run
alarm_clock(1, False) -> 7:00  ==> 7:00   True
alarm_clock(5, False) -> 7:00  ==> 7:00   True
alarm_clock(0, False) -> 10:00 ==> 10:00  True
alarm_clock(6, False) -> 10:00 ==> 10:00  True
alarm_clock(0, True)  -> off   ==> off    True
alarm_clock(6, True)  -> off   ==> off    True
alarm_clock(1, True)  -> 10:00 ==> 10:00  True
alarm_clock(3, True)  -> 10:00 ==> 10:00  True
alarm_clock(5, True)  -> 10:00 ==> 10:00  True

Testing

I find it weird that alarm_clock is passed to test_fun as a parameter, but the cases are hard-coded within test_fun.  They should either both be hard-coded, or both passed as parameters.

Stylistically, cases would be better as a list of tuples.  Tuples have a connotation of being non-homogeneous collections of uniform length (like a single row in a database table).  Lists have a connotation of being homogeneous collections of undetermined length.

The way you split case[0], case[1], and case[-1] is awkward.  I would put the singular expected result first, then use tuple unpacking.

For clarity, you should print repr(param) instead of param.

Here's how I would write it:

def test_fun(fun, cases):
    print('{:^30}     {:^11}'.format('Expected', 'Run'))
    for expected, *params in cases:
        funstr = '{}({})'.format(fun.__name__, ', '.join(repr(p) for p in params))
        result = fun(*params)
        success = result == expected
        print('{:21} -> {:5} ==> {:5}  {}'.format(funstr, expected, result, success))

test_fun(alarm_clock, [
  ('7:00', 1, False),
  ('7:00', 5, False),
  ('10:00', 0, False),
  …
])

Better yet, consider dropping test_fun altogether and using a doctest instead.

Implementation

For a simple problem like this, there are many ways to write it, and choosing the "best" one is a matter of taste.  Personally, I would prefer to keep it simple so that you don't have to jump back and forth among three functions to understand the code.

def alarm_clock(day, vacation):
    """
    Given a day of the week encoded as 0=Sun, 1=Mon, 2=Tue, ...6=Sat, and a
    boolean indicating if we are on vacation, return a string of the form
    "7:00" indicating when the alarm clock should ring. Weekdays, the alarm
    should be "7:00" and on the weekend it should be "10:00". Unless we are on
    vacation -- then on weekdays it should be "10:00" and weekends it should
    be "off".

    >>> alarm_clock(1, False)
    '7:00'
    >>> alarm_clock(5, False)
    '7:00'
    >>> alarm_clock(0, False)
    '10:00'
    """
    weekday = 1 <= day <= 5
    return ( '7:00' if weekday and not vacation else
            '10:00' if weekday ==      vacation else
            'off')

The CodingBat editor seems to encourage two spaces per level of indentation.  PEP 8, the official Python style guide, says that you should use four spaces.