Same PyObject for a shared_ptr C++ object I have a similar problem as in this mail. I want that every time I return the same C++ managed object to Python, I get the same PyObject. With the standard converter I get always a new wrapper-PyObject. The solution should fulfill the following requirements:

get() == get() (the Python object is always equal, whatever function returns the same C++ object)
get() is get()
The same is valid for any objects returned in containers (which the solution from the mail lacks) getArray()[0] == get()
I can instance attributes (get().new_var = "xx" -> get().new_var == "xx")

I wrote a template converter which takes as input a shared_ptr. It looks in a static map, if this shared_ptr is already returned, than it returns the existing PyObject, else a new one is created. I haven't found a solution there I can automatically remove unused objects.

In this case, I added a function to manually remove an entry. This works because the object is managed in C++ and I can intercept than the object should be removed from the C++ side. If Python has still a reference to this object, it's ok to wait for the GC to collect the Python side and implicitly remove the C++ object. If the last reference is removed in Python, the PyObject is still valid, because the map holds still a reference. Also , as long as the PyObject inside the map exists, the C++ object cannot be removed from the heap.

I made a couple of test, which shows the validity, that can be found here.

The usage is:

boost::python::to_python_converter<boost::shared_ptr<A>,
        c_ptr_to_cached_python<A>>();

Is the following code enough to fulfill the requirements for any C++ class wrapped in a shared_ptr? Is the PyObject generation valid and can it be improved?

#pragma once
#include <boost/python.hpp>
#include <map>
#include <iostream>

template<typename C_CLASS>
using storage_map = std::map<C_CLASS*, PyObject*>;

template<typename C_CLASS>
storage_map<C_CLASS>* getCachedObjects() {
    static storage_map<C_CLASS> *storage = new storage_map<C_CLASS>();
    return storage;
}

template<typename C_CLASS>
using c_sptr = boost::shared_ptr<C_CLASS>;

template<typename C_CLASS>
using py_ptr = typename boost::python::pointee<boost::shared_ptr<C_CLASS>>::type;

template<typename C_CLASS>
PyObject * createObject(const c_sptr<C_CLASS>& obj) {
    return boost::python::objects::make_ptr_instance<py_ptr<C_CLASS>,
            boost::python::objects::pointer_holder<c_sptr<C_CLASS>,
                    py_ptr<C_CLASS>>>::execute(obj);
}

template<typename C_CLASS>
void cleanMap() {
    auto map = getCachedObjects<C_CLASS>();
    for (auto it = map->begin(); it != map->end();) {
        auto py_obj = it->second;
        Py_DecRef(py_obj);
        map->erase(it++);
    }
}

template<typename C_CLASS>
void removeObject(const c_sptr<C_CLASS>& obj) {
    auto map = getCachedObjects<C_CLASS>();
    auto raw_ptr = obj.get();
    auto it = map->find(raw_ptr);
    if (it == map->end()) {
        throw std::logic_error("Object was already removed.");
    }
    auto py_obj = it->second;
    Py_DecRef(py_obj);
    map->erase(it);
}

template<typename C_CLASS>
struct c_ptr_to_cached_python {
    static PyObject* convert(const c_sptr<C_CLASS> &obj) {
        auto raw_ptr = obj.get();
        auto map = getCachedObjects<C_CLASS>();
        auto it = map->find(raw_ptr);
        if (it == map->end()) {
            auto py_obj = createObject<C_CLASS>(obj);
            (*map)[raw_ptr] = py_obj;
            Py_INCREF(py_obj); // Increment to store in map
            return py_obj;
        } else {
            auto old_py_obj = it->second;
            Py_INCREF(old_py_obj);
            return old_py_obj;
        }
    }
};

I'm not sure I understand all the premises of this question. My kneejerk reaction was, "Would it help if you could assume that C_CLASS derived from std::enable_shared_from_this<C_CLASS>? Is it in your power to make that an invariant in your codebase?" If so, I would even go a step further and ask whether you should just write an enable_PyObject_from_this<C_CLASS> along the same lines.

Anyway, I see a bit of reducible complexity in your code, I think.

template<typename C_CLASS>
using py_ptr = typename boost::python::pointee<boost::shared_ptr<C_CLASS>>::type;

Isn't this just a really complicated way of saying

template<typename T> using py_ptr = T;

(since pointee<ptr<T>>::type is ptr<T>::element_type is T by definition)?  And then you use it in contexts like

template<typename C_CLASS>
PyObject * createObject(const c_sptr<C_CLASS>& obj) {
    return boost::python::objects::make_ptr_instance<py_ptr<C_CLASS>,
            boost::python::objects::pointer_holder<c_sptr<C_CLASS>,
                    py_ptr<C_CLASS>>>::execute(obj);
}

which ought to be more like

template<typename T>
PyObject *createObject(const std::shared_ptr<T>& obj) {
    using namespace boost::python::objects;
    return make_ptr_instance<T, pointer_holder<shared_ptr<T>, T>>::execute(obj);
}

Even that looks more complicated than it feels like it ought to be, but I'm not finding much documentation about what make_ptr_instance is supposed to do (or for that matter why the second template type parameter to pointer_holder isn't defaulted).

I see you use Py_INCREF (all caps) but Py_DecRef (camel case). This suggests to the reader that something really subtle (and thus dangerous) is happening here — or else of course you'd be consistently using Py_IncRef/DecRef or Py_INCREF/DECREF (both of which are provided by the underlying library). But is the reader's inference correct? Are you really doing something subtle and dangerous? Or are you just mixing styles because you felt like it?

Are you at all worried that getCachedObjects<Base>() returns a completely different map from getCachedObjects<Derived>(), even though I might have a std::shared_ptr<Base> b and a std::shared_ptr<Derived> d such that b == d?


FormBuilder code suggestions for POCO I am constructing a form builder in ASP.NET and I am wanting help with the structure of my POCO classes I have the following so far. I just don't see why I need to add to a list and then process it further again would their be a neater way to return a list of .net controls just to add to Panel using LoadControl(controlname).

POCO Class

public class FormClass
    {

        public string name { get; set; }
        public string displayName { get; set; }
        // This may actually work with Type, but I'm not so sure
        public string type { get; set; }
        public int length { get; set; }
        public bool key { get; set; }
        public bool required { get; set; }
        public string ControlType { get; set; }

    }

And then I'm using the following code to populate a list for further processing:

    PortalContext DBConext = new PortalContext();
    List<FormClass> formData = new List<FormClass>();
    List<FormStructure> frmstructList = new List<FormStructure>();
    public List<FormStructure> GetFormDataFromService()
    {   // XNamespace xNamespace = "http://CompanyName.AppName.Service.Contracts";

        Guid newGuid = Guid.NewGuid();
        XElement xdoc = XElement.Load(@"c:\form.xml");
        XNamespace ns = xdoc.GetDefaultNamespace();
        var classDetails = from classDetail in xdoc.Descendants(ns + "ClassDetails")
                           select new FormClass
                           {
                               name = (string)classDetail.Element(ns + "name"),
                               type = (string)classDetail.Element(ns + "type"),
                               displayName = (string)classDetail.Element(ns + "displayName"),
                               length = (int)classDetail.Element(ns + "length"),
                               key = (bool)classDetail.Element(ns + "key"),
                               required = (bool)classDetail.Element(ns + "required"),
                           };

        foreach (FormClass classDetail in classDetails)
        {
              FormStructure frmstruct = new FormStructure();
              frmstruct.FormStructureId = newGuid;
              frmstruct.name = classDetail.name;
              frmstruct.displayname = classDetail.displayName;
              frmstruct.FormKey = classDetail.key;
              frmstruct.Required = classDetail.required;
              frmstruct.ClassName = "Incident";

              if (classDetail.type == "System.String")
                  frmstruct.ControlType = "TextBox";

              if (classDetail.type == "System.Enum") //to be dropdown
                  frmstruct.ControlType = "DropDown";

              if (classDetail.type == "System.DateTime")//date picker
                  frmstruct.ControlType = "DateTimePicker";

              if (classDetail.type == "System.Boolean")
                  frmstruct.ControlType = "RadioButton";

              frmstructList.Add(frmstruct);

        }
        return frmstructList;

    }

XML Data

  <name>DisplayName</name>

  <displayName>Display Name</displayName>

  <parent>Object</parent>

  <type>System.String</type>

  <length>4000</length>

  <key>false</key>

  <required>false</required>

</ArrayOfClassDetails>

Your naming conventions are a bit off. Property names (i.e. in FormClass) are commonly PascalCase.
If you are not married to the XML file structure (i.e. it's under your control) then you can simplify the de/serialization quite a lot by using the XmlSerializer. For example:

public class ClassDetails
{
    public List<FormClass> Models { get; set; }
}

public static class XmlUtils
{

    public static void Save<T>(T obj, string filename, string namespace)
    {
         using (var writer = new StreamWriter(filename))
         {
             var serializer = new XmlSerializer(typeof(T), namespace);
             serializer.Serialize(writer, obj);
         }
    }

    public static T Load<T>(string filename, string namespace)
    {
         using (var writer = new StreamWriter(filename))
         {
             var serializer = new XmlSerializer(typeof(T), namespace);
             return (T)serializer.Deserialize(writer);
         }
    }
}

then you can load/save the file with:

var details = XmlUtils.Load<ClassDetails>(@"c:\form.xml", "http://CompanyName.AppName.Service.Contracts");

...

XmlUtils.Save(details, @"c:\form.xml", "http://CompanyName.AppName.Service.Contracts");

Rather than having a big if-else cascade I'd use a dictionary to map the types:

Dictionary<string, string> ModelToFormTypeMap =
    new Dictionary<string, string> {
          { "System.String", "TextBox" },
          { "System.Enum", "DropDown" },
          ...
        };

then with an addition of a little bit of Linq you get

XmlUtils.Load<ClassDetails>((@"c:\form.xml", "http://CompanyName.AppName.Service.Contracts")
        .Models
        .Select(m => new FormStructure
                     {
                          FormStructureId = newGuid,
                          name = m.name,
                          displayname = m.displayName,
                          FormKey = m.key,
                          Required = m.required,
                          ClassName = "Incident",
                          ControlType = ModelToFormTypeMap[m.type]
                     })
         .ToList();


Iteration and memcache for selected categorized random elements The following code is a quick implementation since I only needed 3 random images sorted by category (a random eyes image, a random nose image and a random mouth image and then combine them):

    fileinfos = FileInfo.all().filter("category =", "eyes")
    fileinfo = fileinfos[random.randint(0, fileinfos.count()-1)]
    url = images.get_serving_url(str(fileinfo.blob.key()), size=420)
    fileinfos = FileInfo.all().filter("category =", "nose")
    fileinfo2 = fileinfos[random.randint(0, fileinfos.count()-1)]
    url2 = images.get_serving_url(str(fileinfo2.blob.key()), size=420)
    fileinfos = FileInfo.all().filter("category =", "mouth")
    fileinfo3 = fileinfos[random.randint(0, fileinfos.count()-1)]
    url3 = images.get_serving_url(str(fileinfo3.blob.key()), size=420)

This case is somewhat specific since the number of selection are fixed to 3. Surely iteration or a function to do this is preferred and memcache would also increase response time since there are not many files and the same file gets chosen sometimes then I'd like to use memcache and it seems memcache can cache the results from get_serving_url so I could cache the fileinfos or the results from get_serving_url. I know memcache is limited in memory.

    def get_memcached_serving_url(fileinfo):
        from google.appengine.api import memcache
        memcache_key = "blob_%d" % fileinfo.blob.key()
        data = memcache.get(memcache_key)
        if data is not None:
            return data

And I think I can't cache the blob itself and only the result from get_serving_url while it is the trip to the datastore that supposedly takes time.

Please tell me any thoughts or opinions about this quick prototyping how to get random elements while hoping to cache elements since the number of elements is still not very large (<100 elements and if the same is chosen twice then I'd like a fast and preferably cached response)

If you want you can have a look at the actual application here.

Regarding image caching, this seems relevant.

Secondly, DRY this code up, it'll make your code more maintainable:

fileinfos = FileInfo.all().filter("category =", "eyes")
fileinfo = fileinfos[random.randint(0, fileinfos.count()-1)]
url = images.get_serving_url(str(fileinfo.blob.key()), size=420)

Should have it's own function (forgive my py):

def get_random_image_url(category)
    fileinfos = FileInfo.all().filter("category =", category)
    fileinfo = fileinfos[random.randint(0, fileinfos.count()-1)]
    return images.get_serving_url(str(fileinfo.blob.key()), size=420)

eyes_url = get_random_image_url("eyes")
nose_url = get_random_image_url("nose")
mouth_url = get_random_image_url("mouth")


Java loop extracting month and year between two dates I have a ContratMercan with 2 Timestamps containing start and end dates.
I have to create a new entity SeguimientoSeccion for each month between this 2 dates.

I have 2 questions:

There is a way to improve it?
After this I will need to manipulate the List<SeguimientoSeccion> secciones to see which one matches other entity... I though about making a Map but there can be 2 or more sections in each month and year

private List<SeguimientoSeccion> createSectionsPerMonth(ContratMercan contrato)
{
    List<SeguimientoSeccion> secciones = new ArrayList<SeguimientoSeccion>();

    Calendar start = Calendar.getInstance();
    start.setTime(contrato.getPeriodoEntregaIni());
    start.set(Calendar.DAY_OF_MONTH, 1);

    Calendar end = Calendar.getInstance();
    end.setTime(contrato.getPeriodoEntregaFin());
    end.set(Calendar.DAY_OF_MONTH, 2);  // for loop comparison

    for (int month = start.get(Calendar.MONTH), year = start.get(Calendar.YEAR); start.before(end); month = start.get(Calendar.MONTH), year = start.get(Calendar.YEAR)) {
        SeguimientoSeccion seccion = new SeguimientoSeccion();
        seccion.setMonth(month + 1);  // natural month number
        seccion.setYear(year);

        start.add(Calendar.MONTH, 1);

        secciones.add(seccion);
    }

    return secciones;
}

The code would be simpler if you used a Calendar object as the loop variable.

Calendar current = …;
Calendar end = …;

for (; current.before(end); current.add(Calendar.MONTH, 1)) {
    SeguimientoSeccion seccion = new SeguimientoSeccion();
    seccion.setMonth(1 + current.get(Calendar.MONTH));  // natural month number
    seccion.setYear(current.get(Calendar.YEAR));
    secciones.add(seccion);
}

Consider implementing a SeguimientoSeccion.setMonthYear(m, y) method instead, as it's probably useless to set one of the fields without also setting the other.


Improved countdown program in x86 NASM A while back, I posted a question involving code of the same function. This post contains improved code.

From the last post, I have improved size, my work with conventions, and my comments. I was also able to minimalism memory interaction to speed things up/make things more efficient.

%define KERNEL_CALL int 80h

%define STDOUT 1
%define SYS_WRITE 4

%define EXIT 1

%define NEWLINE 0xA
%define ASCII_NUM 30h

; @params: AX - a number that is converted to ASCII, then printed
; @return: no return
%macro put_num 0
    push ax;        push ax onto the stack(for restoring value)
    add ax, ASCII_NUM;  numerical to ascii
    push ax;        push ax onto the stack(for reference by ESP)

    mov edx, 1;     message length
    mov ecx, esp;   the number from the stack pointer
    mov ebx, STDOUT
    mov eax, SYS_WRITE
    KERNEL_CALL;    logs the number to the console

    pop ax;         getting it out of the way
    pop ax;         restoring the original value of ax
%endmacro

section .text
    global _start;  for gnu linker
_start:
    push '10';      push '10' to the stack for reference by ESP

    mov edx, 2;     length
    mov ecx, esp;   message(number)
    mov ebx, STDOUT
    mov eax, SYS_WRITE
    KERNEL_CALL;    logs '10' to the console

    mov ax, 9;      initialize ax
countdown:
    put_num;        log the current number to stdout
.subtract:
    sub ax, 1;      subtract 1

    cmp ax, 0;      compare ax with 0...
    je .blast_off;  if equal, jump to .blast_off
    jmp countdown;  if not equal, go back to countdown
.blast_off:
    mov edx, len;   message length
    mov ecx, msg;   message
    mov ebx, STDOUT
    mov eax, SYS_WRITE
    KERNEL_CALL;    logs 'Blast off!' to the console

    xor ebx, ebx
    mov eax, EXIT
    KERNEL_CALL;    exit
section .data
    msg db "Blast off!", NEWLINE
    len equ $ - msg

Output (I know - the formatting still needs work):

  10987654321Blast off!

Assembled: nasm -f elf bo7_working.s

Linked:: ld -m elf_i386 -s -o bo bo7_working.o

Some concerns:

After I learned about macros, I started to use them a lot. Am I using them in place of headers (and is this bad), or is how I am using them fine?
Am I over-using/misusing %defines (more specifically, KERNEL_CALL)?
Do I have too many or too few comments?
Are my comments unclear?
Are there any unnecessary lines? I'm a little worried about the popping at the end of put_num...
Am I following convention, or are there more things I should do?
Is my code understandable?

•After I learned about macros, I started to use them a lot. Am I using them in place of headers (and is this bad), or is how I am using them fine?

Looks OK to me.

•Am I over-using/misusing %defines (more specifically, KERNEL_CALL)?

I wouln't use KERNAL_CALL in stead of int 80h. The latter is shorter and the value involved will not change any soon in the future.

•Do I have too many or too few comments?

I find commenting on .subtract: a bit stating the obvious.

•Are my comments unclear?

No, they are very readable.

•Are there any unnecessary lines? I'm a little worried about the popping at the end of put_num...

The popping at the end of put_num is correct (pushing/popping 2 words in a row nicely keeps the stack aligned at dwords) but a few lines seem unnecessary to me. You do not need to compare AX with 0 since subtracting already defines the flags.
You could also shave off an extra jump.

sub ax, 1;      subtract 1
cmp ax, 0;      compare ax with 0...
je .blast_off;  if equal, jump to .blast_off
jmp countdown;  if not equal, go back to countdown
.blast_off:

Better code:

sub ax,1
jnz countdown
.blast_off:

•Am I following convention, or are there more things I should do?

The ordering of the sections of the program is a matter of taste. But I see most people put the .data section before the .text section.

•Is my code understandable?

Very understandable.


Counting the positive integer solutions of an equation I have to solve this equation: \$ x + y + xy = n \$

#include <iostream>
#include <cstring>

using namespace std;

long c;

int main()
{
   long x = 0;
   cin >> c;
   if(c == 0)
   {
       cout << 1 << endl;
       return 0;
   }
   for(long i = 1; i <= c / 2; i ++)
   {
       for(long j = 1; j <= c; j ++)
           if(i + j + i * j == c)
           {
                       x ++;
                   break;
           }
   }
   cout << x + 2 << endl;
}

Of course this code is very slow. How can I find a positive number of solutions in a faster way? Maybe there is a specific algorithm?

(Remark: The question mentions "positive number of solutions", but from your code I assume that you meant "number of non-negative integer solutions".)

Coding style:

#include <cstring> is not needed here.
Don't use namespace std;, see for example Why is “using namespace std;” considered bad practice?.
long c; is only used in main(), so there is no need at all to declare it as
a global variable.
Always use braces { ... } for the body of for- and if-statements, even
if it has only one statement. It helps to avoid errors if additional statements
are added later.
Move the computation of the number of solutions to a separate function.
This keeps the main function small and is convenient if you add test cases.
I prefer a different spacing in for- and if-statements, but that may be a
matter of taste.
Variable names: If the equation is given as \$ x + y + xy = n \$, then why
not use the same names in your program? And x is a quite non-descriptive
name of a number of solutions.
It may not be immediately obvious why 2 is added to the number of solutions,
so an explaining comment would be appropriate here.

Then your code would look like this:

#include <iostream>

// Number of non-negative integer solutions to
//    x + y + x * y == n   .
long numberOfSolutions(long n) {

    if (n == 0) {
        return 1;
    }

    // Count all positive solutions:
    long count = 0;
    for (long x = 1; x <= n / 2; x++) {
        for (long y = 1; y <= n; y++) {
            if (x + y + x * y == n) {
                count++;
                break;
            }
        }
    }
    // Add 2 for the solutions x=0,y=n and x=0,y=n:
    return count + 2;
}

int main()
{
    long n;

    std::cin >> n;
    std::cout << numberOfSolutions(n) << std::endl;
}

Performance:

Your code tries all possible combinations of x, y to find solutions
and therefore runs in \$ O(n^2) \$. A small improvement could be to use the
symmetry of the problem, i.e. enumerate only pairs with x <= y:

for (long x = 1; x <= n / 2; x++) {
    for (long y = x; y <= n; y++) {
        if (x + y + x * y == n) {
            // Counts a 2 solutions if x != y:
            count = count + 1 + (x != y);
            break;
        }
    }
}

This cuts the total number of iterations down by a factor of 2, but it is still
\$ O(n^2) \$.

Better algorithm:

If you write your equation as
$$
  n + 1 = x + y + xy + 1 = (x + 1)(y + 1)
$$
then it becomes obvious that it can be solved by determining the
number of divisors of \$ n+1 \$. This known as the Divisor function and can be computed
efficiently.

Even the simplest implementation

long numberOfDivisors(long n) {
    long count = 0;

    for (long j = 1; j <= n; j++) {
        if (n % j == 0) {
            count++;
        }
    }
    return count;
}

runs in \$ O(n) \$ instead of \$ O(n^2) \$. More sophisticated methods use
the prime factorization  of \$ n \$ .


Validator Readability and Efficiency This is my first validator I have created myself and I was wondering if there is anything about it that I could improve as far as:

Code readability
Code efficiency

Note: The reason why I put an underscore before each validation method name is so that non-validation methods cannot be called. E.g. setRule() can't be called if it's specified in the rule unless there is an actual validation method called "_setRule".

Any other suggestions are also welcome!

Validator class:

class Validator
{
    private $rules  = [];
    private $errors = [];

    public function setRule($id, Array $methods)
    {
        if (!array_key_exists($id, $this->rules)) {
            $this->rules[$id] = $methods;
        } else {
            throw new Exception('Rule "' . $id . '" already exists.');
        }
    }

    public function setRules(Array $rules) {
        foreach ($rules as $key => $value) {
            $this->setRule($key, $value);
        }
    }

    public function validate($rule, $input)
    {
        foreach ($this->rules[$rule] as $key => $value) {
            if (is_int($key)) { //$key => rule
                $value     = '_' . $value;
                $method    = $value;
                $arguments = [$input];
            } else {            //rule => [args]
                $key       = '_' . $key;
                $method    = $key;
                $arguments = array_merge([$input], is_array($value) ? $value : [$value]);
            }

            return call_user_func_array([$this, $method], $arguments);
        }
    }

    public function setError($error)
    {
        return $this->errors[] = $error;
    }

    public function getErrors()
    {
        return $this->errors;
    }

    private function _required($input)
    {
        return empty($input) ?: $this->errors[] = substr(__FUNCTION__, 1);
    }

    private function _min($input, $min)
    {
        return mb_strlen($input, 'utf8') >= $min ?: $this->errors[] = substr(__FUNCTION__, 1);
    }

    private function _max($input, $max)
    {
        return mb_strlen($input, 'utf8') <= $max ?: $this->errors[] = substr(__FUNCTION__, 1);
    }

    private function _between($input, $min, $max)
    {
        if ($return = $this->_min($input, $min) !== true) {
            return $this->errors[] = $return;
        }

        if ($return = $this->_max($input, $max) !== true) {
            return $this->errors[] = $return;
        }

        return true;
    }

    private function _email($input)
    {
        $sanitizedInput = filter_var($input, FILTER_SANITIZE_EMAIL);
        $vInput         = filter_var($sanitizedInput, FILTER_VALIDATE_EMAIL);

        return $input === $sanitizedInput && $vInput ?: $this->errors[] = substr(__FUNCTION__, 1);
    }

    private function _alnum($input)
    {
        return ctype_alnum($input) ?: $this->errors[] = substr(__FUNCTION__, 1);
    }
}

Sample usage:

$v = new Validator();

$v->setRule('email', ['required', 'between' => [5, 255], 'email']);
$v->setRule('password', ['required']);

$vEmail        = $v->validate('email', $_POST['email']);
$vEmailConfirm = $v->validate('email', $_POST['emailConfirm']);
$vPassword     = $v->validate('password', $_POST['password']);

if ($vEmailConfirm === true && $_POST['email'] !== $_POST['emailConfirm']) {
    $vEmailConfirm = $v->setError('match');
}

if (!$v->getErrors()) {
    # Input(s) have passed validation
} else {
   # Input(s) have not passed validation
   # $vEmail, $vEmailConfirm and $vPassword contain the errors
   # You can do something with that, like displaying an error message
}

Although you used a Validator class, this is not really object orientated. Actually I'm missing a Rule class. Your rules are applied by comparing strings right now, which is error prone and really ugly if you need to refactor it. Extending your code with new rules also requires changing the Validator itself.

class Rule {
     /**
      * @return bool
      */
     abstract function check($value);
     /**
      * @return string
      */
     abstract function getErrorMessage($value)
}

class BetweenRule extends Rule{ //Maybe there is also an IntegerRule to collect some operations.
     private $min;
     private $max;

     public function __construct ($min,$max) {...}

     public check($value)
     {
          return this->min<=$value && $this->max>=$value;
     }

     public function getErrorMessage($value) { return $valus." is not between ..." ;}
}
...

And the usage:

$v = new Validator();
$r=new RequiredRule();

$v->setRule('email', [$r, new BetweenRule(5,255), new EmailRule()]);
$v->setRule('password', [$r]);

if (!$v->getErrors()) {  //either as a list of string or list of map fields and rules
     ...
} else {
     ...
}

As you can't type hint the content of an array, you might also want to replace setRule by addRule and append new rules to the existing ones, instead setting the array of rules.

$v = new Validator();
$v->addRule('email', new RequiredRule());
$v->addRule('email', new BetweenRule(5,255));
$v->addRule('email', new EmailRule());


Option functional type implementation and scenarios Have you ever being implementing Option<T> functional type? It is discussed here. Basically, it is about using IEnumerable<T> with no or only one element instead of potentially nullable object reference in C#. We can empower LINQ functionality to streamline processing reducing cyclomatic complexity because of no if(null) conditions anymore.

My adoption of the idea looks this way:

public struct Optional<T> : IEnumerable<T>
    where T : class
{
    public static implicit operator Optional<T>(T value)
    {
        return new Optional<T>(value);
    }

    public static implicit operator T(Optional<T> optional)
    {
        return optional.Value;
    }

    Optional(T value)
        : this()
    {
        Value = value;
    }

    public IEnumerator<T> GetEnumerator()
    {
        if (HasValue)
            yield return Value;
    }

    T Value { get; }
    bool HasValue => Value != null;
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    public override string ToString() => Value?.ToString();
}

It is not so bad. We can use it as argument/return type with automatic conversion to/from T when necessary.

Consumption example might look like this:

 var p = new Product("Milk");
 var basket = ShoppingBasket.Empty
        .Place(p)
        .Place(p)
        .Place(null);

It creates basket with single basket entry for two milk boxes if the following is defined:

public static class ShoppingBasket
{
    public static readonly IEnumerable<BasketItem> Empty =
        Enumerable.Empty<BasketItem>();

    public static IEnumerable<BasketItem> Place(
        this IEnumerable<BasketItem> basket,
        Optional<Product> product) =>
            basket
                .SetOrAdd(
                    i => product.Contains(i.Product),
                    i => i.OneMore(),
                    product.Select(p => new BasketItem(p, 1)))
                .ToArray();
}

Where:

public class BasketItem
{
    public BasketItem(Product product, int quantity)
    {
        Product = product;
        Quantity = quantity;
    }

    public Product Product { get; }
    public int Quantity { get; }
    public BasketItem OneMore() =>
        new BasketItem(Product, Quantity + 1);
}

public class Product
{
    public Product(string name)
    {
        Name = name;
    }

    public string Name { get; }
}

I use this general IEnumerable<T> extension:

public static class EnumerableHelper
{
    public static IEnumerable<T> SetOrAdd<T>(
        this IEnumerable<T> source,
        Func<T, bool> predicate,
        Func<T, T> set,
        params T[] add)
    {
        return source
            .SetOrAdd(predicate, set, add as IEnumerable<T>);
    }

    public static IEnumerable<T> SetOrAdd<T>(
        this IEnumerable<T> source,
        Func<T, bool> predicate,
        Func<T, T> set,
        IEnumerable<T> add)
    {
        var empty = Enumerable.Empty<T>();
        foreach (var item in source)
            if (predicate(item))
            {
                yield return set(item);
                add = empty;
            }
            else
                yield return item;

        foreach (var item in add)
            yield return item;
    }
}

Well, it is pretty functional as far as I can see. But the question is: does it make any sense? Cyclomatic complexity is very low in ShoppingBasket class. But the same time we still have three "virtual paths" of execution to test - for "add" and "replace" in the Place() method + we also need to test for null product argument.

The problem is that they are almost invisible. What do you say? Would you personally prefer to maintain such kind of code in C#?

I'm not a fan of your usage example. The line .Place(null) looks really fishy to me. It looks like something, that should not happen in my imaginary web store, like some kind of error, which I probably want to catch and debug. However Optional<T> class hides this potentioal error completely and my application keeps working as if nothing has happened. Now, if null is for some reason a valid value, then I guess its fine.

Overall, Optinal<T> class looks OK to me, even though I am having troubles coming up with a sceanrio, where I would find it useful. You could probably group public methods together though, it makes class easier to read.

Also I think you are overusing => operator. To the point where method's body becomes really hard to read. => is fine when your function is a short one-liner (i.e. IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();). But you should probably use braces, when it is not (i.e your Place method).


Attach list of files and email using secure connection Can I use this function to reliably attach a list of excel files to an email and create a secure connection to transmit said email? My python programming is self taught and I put this function together with a lot of google searches. (This function works, I use it, but I would like to have someone with more experience look it through, if possible)

Where can I improve my code? (Am I using the correct modules?)

import datetime
import smtplib
from email.MIMEMultipart import MIMEMultipart
from email.MIMEText import MIMEText
from email.MIMEBase import MIMEBase
from email import Encoders

sMDY = datetime.date.today().strftime('%m/%d/%y')
lpath = 'C:/Path/to/my/excel/files/'
flist = ['excelfile_1.xlsx', 'excelfile_2.xlsx']

def EMAIL_FILES(flist):
    uname = 'username'
    pword = 'password'

    emailfrom = 'sender@domain.com'
    emailto = [
        'recipient_1@domain.com',
        'recipient_2@domain.com',
        ]

    body = ["The information transmitted is ..."]
    subject = ('Subject with date %s' % sMDY)

    msg = MIMEMultipart()
    msg['From'] = emailfrom
    msg['To'] = ', '.join(emailto)
    msg['Subject'] = subject

    msg.attach(MIMEText(''.join(body)))

    ### ATTACH FILES
    for item in flist:
        part = MIMEBase('application', "octet-stream")

        part.set_payload(open(lpath + item, "rb").read())

        Encoders.encode_base64(part)

        part.add_header('Content-Disposition',
                        'attachment; filename="%s"' % item)
        msg.attach(part)

    ### SECURE CONNECTION
    server = smtplib.SMTP('smtp.domain.com:25')
    server.ehlo()
    server.starttls()
    server.ehlo()
    server.login(uname, pword)

    server.sendmail(emailfrom, emailto, msg.as_string())
    server.quit()

In general, the code looks good.  I just have a few miscellaneous remarks.

Code organization

Function names should be named lower_case_with_underscores by PEP 8.
I would split the message composition and sending into separate functions.
It is good practice to avoid embedding passwords into your source code.  I suggest storing the account information in a YAML file, or at least as constants in a separate Python module.
The parameters are hard-coded, so the function is of limited utility.  The function could accept parameters with defaults:

def compose_email(from='sender@example.com',
                  to=['recipient_1@example.com','recipient_2@example.com'],
                  attachments=[])
    …

def send_email(msg, username, password)
    …

Mail content

The MIME type for .xlsx files is application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.
Concatenating file paths using os.path.join(lpath, item) would let you not have to worry about the path component separator.  Personally, I would choose to pass full paths to the function instead, to avoid assuming that they are all in some hard-coded directory:

def EMAIL_FILES(attachments):
    …
    for path in attachments:
        with open(path, 'rb') as f:
            part = MIMEBase('application', 'vnd.openxmlformats-officedocument.spreadsheetml.sheet')
            part.set_payload(f.read())
            part.add_header('Content-Disposition',
                'attachment; filename="%s"' % os.path.basename(path))
            Encoders.encode_base64(part)
            msg.attach(part)
    …

EMAIL_FILES([os.path.join('C:/Path/to/my/excel/files/', f) for f in
    ['excelfile_1.xlsx', 'excelfile_2.xlsx']
])

I'm puzzled by why you defined body as a list of strings, only to join them all into one long string.  If the body contains non-trivial content, consider writing it as a """longstring""".  Also, you may need to encode the MIMEText using Encoders.encode_quopri().


Student Classroom class I'd like to know if the following class design is good or bad:

#include <iostream>
#include <vector>
#include <string>
#include <map>
using namespace std;
struct Student {
    string Firstname;
    string Surname;
    string IDNumber;
    int Age;
};

class Exam {
    string ExamNo;
    int Questions;
    int PassScore;
    string Subject;
    map <string,string> ExamTaken;
public:
    Exam(string exam,int Q , string s) : ExamNo(exam), Questions(Q),  Subject(s) {};
    void TakeExam(const string S) { ExamTaken[S] = ExamNo; };
};

class Teacher {
    int studentscore;
    string ClassRoom;
public:
    bool StudentPassed ();
    void setScore (int studentsc) ;
};

class ClassRoom {
    vector<Student> students;
    string ClassName;
    double ClassAverage;
public:
    ClassRoom(string classname) : ClassName(classname) {};
    void AddStudent(Student &s) { students.push_back(s); };
    void ListStudents() const;
    int StudentCount() const { return students.size(); };
    void StudentLookup () const;
    string getClassName() const { return ClassName; };
};
// list all students in class
void ClassRoom::ListStudents() const {
/* old c++ style iterator
    for (vector<Student>::const_iterator it = students.begin();
    it != students.end();++it){
        cout << it->Firstname << " , ";
        cout << it->Surname << " , ";
        cout << it->Age;
        cout << endl;
    }
*/
// c++11
    for (auto &x : students){
        cout << x.Firstname << ",";
        cout << x.Surname << ",";
        cout << x.Age << endl;
    }
}

int main(){
    Student a,b;
    ClassRoom c("CS1002");
    Exam e("A002", 200 , "Biology");

    a.Firstname = "John";
    a.Surname = "Doe";
    a.IDNumber = "8123295073081";
    a.Age = 30;

    e.TakeExam(a.IDNumber);

    b.Firstname = "Jane";
    b.Surname = "Doe";
    b.IDNumber = "8123225073281";
    b.Age = 27;

    c.AddStudent(a);
    c.AddStudent(b);

    c.ListStudents();
    cout << "There are "
         << c.StudentCount()
         << " students in the classroom "
         << c.getClassName() << endl;

}

Maybe the Teacher class should be removed and replaced with a database class, that keeps a record of grades for each student.

I was thinking of changing the student class and adding object composition, for the exam class. I'm not sure how to do it properly yet, so I'm still experimenting with that.

First of all I'd like to say that you did not provide guidelines, so I'm not completely sure what is required and what not.

ClassRoom

I would rename this to Course, since it's not really related to the room where it takes place. A course probably has a instance variable that identifies the location of the course "Building A4.4" for instance. It indeed holds a vector of students, a name. But I wouldn't store a class average, instead provide a method for calculating that getCourseAverage() for example. Perhaps also store the Teacher object that gives this course.

Teachers

Your Teacher class seems not logically constructed, it is a person.
If I were you I would create a Person class and inherit your Teacher and Student class from it. A Person has a firstname, lastname , age. Probably they both have an id value. You can even go a step further and make a Employee class, that has Person as base class and of which Teacher inherits.

Instead of string ClassRoom, I would use composition and have it contain a list of Course object (courses that are given by that teacher).

Constructors

Instead of using

 a.Firstname = "John";
 a.Surname = "Doe";
 a.IDNumber = "8123295073081";
 a.Age = 30;

create a constructor like: CreateStudent(name, surname, id, age);
This is more concise and reads easier.

You might take a look into using an abstract factory pattern in order to create your student/teacher objects.

Style

Normally one does not start member variables with a upper case. perhaps take a look at this

apart from that you seem to do pretty well, style wise. But I'm not a C++ expert, I'm certain other can help you further with this.


Validation function code review Any suggestions on cleaning up this code:

 public override bool Validate(Control control, object value)
    {
      bool res = false;
      string str = string.Empty;

      try
      {
        if (((string)value).Length > 0)
          str = value.ToString();
      }
      catch (Exception e)
      {
        res = false;
        this.ErrorText = "Bad values " + e.Message;
        this.ErrorType = ErrorType.Critical;
      }

      //Is it empty?
      if (String.IsNullOrEmpty(str))
      {
        res = false;
        this.ErrorType = ErrorType.Critical;
      }

      Regex RegNrShortRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3})");
      Regex RegNrLongRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3}[-][0-9]+$)");
      Regex RegNrShortRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3})");
      Regex RegNrLongRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3}[-][0-9]+$)");

      if (selectedItem == Constants.Item1)
      {
        this.ErrorText = "The correct form is: FAD-Zxxx oder FAD-Zxxx-x !";

        //has the form FAD-Zxxx
        if (RegNrShortRegexDarm.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAD-Zxxx-x
        if (RegNrLongRegexDarm.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }
      else if (selectedItem == Constants.Item2)
      {
        this.ErrorText = "The correct form is: FAB-Zxxx oder FAB-Zxxx-x !";

        //has the form FAB-Zxxx
        if (RegNrShortRegexBrust.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAB-Zxxx-x
        if (RegNrLongRegexBrust.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }

      return res;
    }

Later Edit:

I've reached the following solution:

public override bool Validate(Control control, object value)
{
  var str = value as string;
  var letter = (selectedItem == Constants.Item1) ? "D" : "B";
  var errText = String.Format("The correct form is: FA{0}-Zxxx or FA{0}-Zxxx-x !", letter);

  if (string.IsNullOrWhiteSpace(str))
  {
    this.ErrorType = ErrorType.Critical;
    this.ErrorText = errText;
    return false;
  }

  string regExString = String.Format(@"FA{0}-Z\d{{3}}", letter);
  Regex shortRegEx = new Regex(regExString);
  Regex longRegEx = new Regex(regExString + @"-\d+$");

  bool isMatch = ((shortRegEx.IsMatch(str) && IsValidShortLength(str)) || (longRegEx.IsMatch(str) && IsValidLongLength(str)));

  if (!isMatch)
  {
    this.ErrorText = errText;
  }
  return isMatch;

}

private static bool IsValidShortLength(string str)
{
  return str.Length == 8;
}

private static bool IsValidLongLength(string str)
{
  return (str.Length >= 10 && str.Length <= 12);
}

The first check in the try block is not needed at all. It just checks if the value is string, and it's unlikely to be something else. If this validation is attached to a control, who's value is not a string, then this is an error, and I would throw an exception "Bad values", so the code is corrected.

Then, if the value is null or empty, you have an error, why would you inspect all the other possibilities?

Also - where you use the control itself? Why you pass it, if you do not use it?

Anyway, if I was to check the string for valid formatting, I would do this:

        var str = value as String;

        var letter = (selectedItem == Constants.Item1) ? "D" : "B";
        var errText = String.Format("The correct form is: FA{0}-Zxxx oder FA{0}-Zxxx-x !", letter);

        if (str == null
            || str.Length < 8
            || !(str.Length >= 10 && str.Length <=12))
        {
            this.ErrorType = ErrorType.Critical;
            this.ErrorText = errText;
            return false;
        }

        var regExString = @"([F][A][" + letter + @"][-][Z]\d{3})";
        Regex shortRegEx = new Regex(regExString);
        Regex longRegEx = new Regex(regExString + @"[-][0-9]+$)");

        var isMatch = (shortRegEx.IsMatch(str) || longRegEx.IsMatch(str));
        if (!isMatch)
        {
            this.ErrorText = errText;
        }

        return isMatch;


Displaying user-defined functions and their docstrings Let's say I want to create dynamic documentation of functions I wrote, without using the help() function (I don't want the user to care about arguments accepted by my function, nor get cluttered with other stuff like the copyright info, or other stuff that aren't functions).

Here's an attempt:

import inspect

def foo():
    """Return 'foo' string"""
    return "foo"

def user_defined_functions():
    """Return names of user-defined functions"""
    for name in globals():
        if inspect.isfunction(eval(name)):
            docstring = eval(name + ".__doc__")
            print("{}:\n   {}".format(name, docstring))

And here's the pretty result:

>>> import test
>>> test.user_defined_functions()
user_defined_functions:
   Return names of user-defined functions
foo:
   Return 'foo' string

If I had just used the help function I could have gotten extra stuff I don't need (I'm not writing a library, so the user doesn't need all of this):

Help on module test:

NAME
    test

FUNCTIONS
    foo()
        Return 'foo' string

    user_defined_functions()
        Return names of user-defined functions

FILE
    /home/wena/src/utils/test.py

Use for name, value in globals().items(): then you don't need to use eval


RemoveDuplicateElement public class ArrayUtil {

    /**
     * Field INPUT_LESS_THAN_TWO. (value is 2)
     */
    private static final int INPUT_LESS_THAN_TWO = 2;

    /**
     * Method removeDuplicateElement. Given a sorted array of integers, this
     * method removes the duplicate elements. Input array is sorted array and
     * can have only negative values, only positive value or a mix of both.
     *
     * @param inputArray
     *            int[]
     *
     * @return int[]
     * @throws IllegalArgumentException
     */
    public int[] removeDuplicateElement(final int[] inputArray)
            throws IllegalArgumentException {

        int j = 0; // Marks the begining of the array
        int i = 1; // Points toward the second element in the array

        if (null == inputArray || 0 == inputArray.length) {
            throw new IllegalArgumentException(
                    "Input Array is either null or of zero length");
        } // end of null and 0 length check

        // Return if the input array has only one item
        if (inputArray.length < INPUT_LESS_THAN_TWO) {
            return inputArray;
        } // end of check for length less than 2
        while (i < inputArray.length) {
            if (inputArray[i] == inputArray[j]) {
                i++;
            } else {
                inputArray[++j] = inputArray[i++];
            }
        } // end of while loop

        // copy and return the array
        final int[] outputArray = new int[j + 1];
        for (int k = 0; k < outputArray.length; k++) {
            outputArray[k] = inputArray[k];
        }

        return outputArray;
    }

}

I believe that the time complexity of this algorithm is \$O(N)\$.

Style

Comments

I stated this in a comment (pun unintended but a happy side effect) that I think comments like this are unneeded and make the code harder to read.

if (null == inputArray || 0 == inputArray.length) {
    throw new IllegalArgumentException(
            "Input Array is either null or of zero length");
} // end of null and 0 length check

// Return if the input array has only one item
if (inputArray.length < INPUT_LESS_THAN_TWO) {
    return inputArray;
} // end of check for length less than 2

Compare that to this

if (null == inputArray || 0 == inputArray.length) {
    throw new IllegalArgumentException(
            "Input Array is either null or of zero length");
}

// Return if the input array has only one item
if (inputArray.length < INPUT_LESS_THAN_TWO) {
    return inputArray;
}

Because everything is properly indented I just know by looking that the if-block if over.

Lastly, the comments for i and j are poor. You state what they are originally pointing to but not what they're used for throughout the method which would be more useful. Something like this,

int j = 0; // Marks the end of the sorted subarray to return
int i = 0; // Points to the element being checked

(It may be better to switch the names of i and j because i is usually used for the outer most "loop" meaning it moves less.)

Doc Comments

Adding javadoc comments is good practice and it's good to see you do this. The only issue I see with it is it starts by saying it's a method and it gives the name. This is redundant. The name is beneath the doc comment and if you view the javadoc as html it is still clear what the name is. Just remove "Method removeDuplicateElement."

Throws IllegalArgumentException

See this if you do not know the difference between checked and unchecked exceptions. IllegalArgumentException extends RuntimeException. Because of this it is an unchecked exception.

The throws clause of a method is to alert the compiler that it can possibly throw the exception. Because this is an unchecked exception though the compiler doesn't care if the caller handles/throws it or not, and because of that it is redundant. I suggest either removing this or making your own exception that doesn't extend RuntimeException if you really want to make the called handle/throw the error. (I think this is a bad idea.)

Note: I did not say there was an issue with throw new IllegalArgumentException( ... ); when the array is invalid for your method, this is fine. However, one could argue that because the point of the method is to remove duplicate elements that a null/empty array has the elements removes and you could simply return the null/empty array instead of throwing the error. In fact, I think I like that better. It's best to throw little and handle quietly when it comes to exceptions.

Utility Class

I seem to be linking back to my own answer a lot. For the same reasons as here I suggest you make this into a "utility class". Because everything about this class is always the same no matter how you make an instance of it, there's no point in having to make an instance of it. Quick summary,

make it a final class,
add a private constructor, and
make all methods static.

private static final fields

There is nothing wrong with these but in this case I don't think making INPUT_LESS_THAN_TWO into one makes sense. You'd return the array if it only had one element no matter what, there's not something that's likely to change down the line that would make this value different.

inputArray.length < 2 is simply more readable in this case, or better yet inputArray.length == 1 because earlier you already checked if it was 0 and it should never be below 0. (If it's ever below 0 the JVM is messing up and there's bigger issues! It's safe to assume it won't.)

Code choice

System.arraycopy

  Edit: 200_success's suggestion of Arrays.copyOf is better than my suggestion here. System.arraycopy is better when you already have an array you want to put things in, Arrays.copyOf makes a new array for you. Better to do away with making outputArray altogether and simply doing return Arrays.copyOf(inputArray, j + 1);. System.arraycopy is still a good tool to know of so I will leave this section, bold italic text is text I added in this edit.

This is preferred to using a for-loop to make an array from an array specifically when you already have an array that you want to put the copied elements in.

for (int k = 0; k < outputArray.length; k++) {
    outputArray[k] = inputArray[k];
}

That becomes simply System.arraycopy(inputArray, 0, outputArray, 0, outputArray.length); (You may need to remove the final modifier for the output array, I am not entirely sure.)

Increment operators

This is simply my taste, no issue with your code, but I dislike inputArray[++j] = inputArray[i++]; because it's a little confusing. I prefer

j++;
inputArray[j] = inputArray[i];
i++;


Get the value of the name attribute of each element with class="class" I need a function to get the value of the name attribute of each element with class="class". I was wondering what is the best way to do this, performance-wise.

Currently I'm using jQuery and doing it like this:

$("#Button").click(function() {
    var myElements = $(".class");
    for (var i=0;i<myElements.length;i++) {
        alert(myElements.eq(i).attr("name"));
    }
});

Alert is just to show what it does, of course. For the following HTML:

<input type="checkbox" class="class" name="1">
<input type="checkbox" class="nope" name="2">
<input type="checkbox" class="class" name="3">

It would issue:

alert("1")
alert("3")

I was wondering if using pure Javascript would be faster, or if there is any other (better) way of doing it. I apologize if this question is not on this website's scope, please move it to Stackoverflow if that's the case.

Pure JS vs jQuery

Pure JavaScript will be significantly faster as you can see by this jsPerf which pits document.getElementByClassName vs the jQuery selector. Here are the results for Chrome 25:

$('.class') - 4355 operations per second
getElementsByClassName('class') - 94636 operations per second

As you can see, for this simple operation the jQuery option is approximately 22 times slower than the pure-JavaScript equivalent. You can easily see why this is the case by checking out the jQuery development source, since the jQuery 'selector function' is so general it needs to do a lot of checks to see what the input is before it can act upon it.

Pure JS implementation

I've left the jQuery click event in as it looks like you already have a dependency on jQuery so no point changing the way you're hooking up your events.

jsFiddle

$("#Button").click(function() {
    var items = document.getElementsByClassName('class');
    for (var i = 0; i < items.length; i++)
        alert(items[i].name);
});

getElementsByClassName

Also note that I used getElementsByClassName. It is pretty important to choose the ideal selector if you care about performance. Since we care about all elements with a certain class we can immediately dismiss the others like getElementsByTagName and querySelectorAll and opt for the function that was built for finding elements of a particular class.

jQuery implementation

This would be my implementation using jQuery, notice that I didn't bother getting the jQuery object for this (ie. $(this)) because the plain JavaScript attribute is much easier in this case.

jsFiddle

$("#Button").click(function() {
    $(".class").each(function() {
        alert(this.name);
    });
});


Brotli compression algorithm, translated from Python into Haskell I have converted some code in Python into its Haskell equivalent.

Python implementation:

def final_codes( bit_lengths, next_code ):
    tree_code = []
    for i in range(len(bit_lengths)):
        tree_code.append( next_code[ bit_lengths[i] ] )
        next_code[bit_lengths[i]] += 1
    return tree_code

#Inputs:
bit_lengths = [ 3, 3, 3, 3, 3, 2, 4, 4 ]
next_code = [0,0,0,2,14]
print final_codes( bit_lengths, next_code )
[2, 3, 4, 5, 6, 0, 14, 15]

Haskell implementation:

replaceNth :: (Eq a, Num a) => a -> a1 -> [a1] -> [a1]
replaceNth n newVal (x:xs)       # Picked this function from somewhere on stackoverflow.
    | n == 0 = newVal:xs
    | otherwise = x:replaceNth (n-1) newVal xs

final_codes' :: (Eq a, Num a, Num a1) => [a1] -> [Int] -> [a1] -> a -> [a1]
final_codes' lst bit_lngths next_cd 0 = lst
final_codes' lst bit_lngths next_cd n = final_codes' new_lst bit_lngths new_next_code (n-1)
    where tlen = bit_lngths !! (length lst)
          new_lst = (next_cd !! tlen) : lst
          new_next_code = replaceNth tlen ((next_cd !! tlen) + 1) next_cd  -- INEFFICIENT!

final_codes :: Num a => [Int] -> [a] -> [a]
final_codes bit_lengths next_code = reverse $ final_codes' [] bit_lengths next_code (length bit_lengths)

In the last where statement in final_codes' I create a new_next_code list by incrementing the value at position tlen by 1. This makes the algorithm \$O(N^2)\$ in comparison to the \$O(N)\$ implementation in Python.

This approach works but it feels a bit contrived. Any suggestions on how I could improve this code and its performance?

Note: I'd like to avoid using the lens library for now (I'm still a novice). I found this Stack Overflow link on replacing a single element in a list; but most approaches are \$O(N)\$. I haven't yet dug into the versions that use lens.

don't use lists

Almost any non-trivial operation on lists is O(n) or worse. Python is using mutable arrays. Haskell also mutable arrays, so why not use mutable arrays?

The only problem with Haskell's arrays is that there are so many choices to make:

pure vs. mutable
boxed vs. unboxed
array vs. vector
run in the ST monad or IO?

Here is the routine coded in Haskell which uses both pure and mutable unboxed vectors. It could be improved, but this is basically how you would go about it.

(code updated)

module Lib
where

import Control.Monad
import Control.Monad.Primitive
import Control.Monad.ST
import qualified Data.Vector.Unboxed.Mutable as UVM
import qualified Data.Vector.Unboxed as UV

calculate_codes :: PrimMonad m => UV.Vector Int -> UV.Vector Int -> m (UV.Vector Int)
calculate_codes tree codes = do
  next_codes <- UV.thaw codes
  UV.generateM (UV.length tree) $ \i -> do
    let x = tree UV.! i
    z <- if x /= 0
           then do y <- UVM.read next_codes x
                   UVM.write next_codes x (y+1)
                   return y
           else return 0
    return z

test = do
  let tree = UV.fromList [ 3, 3, 3, 3, 3, 2, 4, 4 ]
      codes = UV.fromList [ 0, 0, 0, 2, 14 ]
      next = runST $ calculate_codes tree codes
  print next

{-
tree      = [ 3, 3, 3, 3, 3, 2, 4, 4 ]
next_code = [0,0,0,2,14]

def calculate_codes( tree, codes ):
    tree_code = {}
    next_code = copy.copy( codes )
    for i in range(len(tree)):
        if tree[i] != 0:
            tree_code[i] = next_code[ tree[i] ]
            next_code[tree[i]] += 1             -- This "effectful" line is throwing me off.
    return tree_code

print calculate_codes( tree, next_code )
-}


Blackjack Game in Ruby I built a simple Blackjack game in Ruby on Rails, and am looking for feedback, because I believe I may have overcomplicated things. This was a homework assignment, where I was told to make 1 be an Ace card.

$number_array = []

def pull_push_random()
    number = Random.rand(1..10)
    $number_array.push(number)
    $array_sum = $number_array.inject(0){|sum,number_array| sum + number_array }
end

pull_push_random()
pull_push_random()
puts "This is an array #{$number_array}"

puts "Would you like to draw another card (random number)? y/n"
answer = gets.chomp
if answer == 'y'
    pull_push_random()
    puts "This is the array: #{$number_array}"
    if $array_sum < 16
        pull_push_random()
        puts "This is the array #{$number_array}"
    elsif $array_sum > 21
        puts "Done"
    else
        puts "Invalid option"
    end
elsif answer == 'n'
    puts "Done"
else
    puts "You did not enter a valid option."
end

You didn't implement the game of Blackjack

number = Random.rand(1..10)

This does not bear a strong resemblance to drawing from a shuffled deck of playing cards. A value of 10 should occur four times more often than 9, even if Aces are only valued as a 1. I understand the desire to simplify, but this goes a bit far.

To model a dealer with a large (or "infinite") number of decks shuffled into the shoe, draw with replacement by using Random.rand(1..13) and then map all values above 9 to 10, to account for J Q K.

To model a single shuffled deck, or a small number of them, that we draw from without replacement, allocate an array of size at least 52, and use Fisher-Yates-Knuth.

  aces are valued as either 1 or 11 according to the player's choice.

This rule is pretty central to the decision of whether to hit or pass.
You really can't use simple + (addition) when summarizing a player's hand.
Minimally you need to model the number of Ace cards he holds and (separately) the sum of non-Ace card values, to properly determine if he is "bust" yet. Also, please don't make $array_sum global. And a better name for pull_push_random() would be deal().

There are several other house rules, such as betting options, that would add more complexity if implemented. I agree with your design decision to avoid such complexities. One can still capture the essence of Blackjack in a fairly short program, if a handful of important rules are implemented.


SPOJ "SBANK - Sorting Bank Accounts" TLE I am new tro programming and have chosen python 2.7 to begin with. But, I often have to deal with a TLE while competitive coding like in the problem below, where my code runs fine on all test cases but runs out of the given time bound.

My code runs fine on Ideone.com for all the given test cases, but gives a TLE on SPOJ.

This is the SBANK problem at SPOJ.

  In one of the internet banks thousands of operations are being performed every day. Since certain customers do business more actively than others, some of the bank accounts occur many times in the list of operations. Your task is to sort the bank account numbers in ascending order. If an account appears twice or more in the list, write the number of repetitions just after the account number. The format of accounts is as follows: 2 control digits, an 8-digit code of the bank, 16 digits identifying the owner (written in groups of four digits), for example (at the end of each line there is exactly one space):

  30 10103538 2222 1233 6160 0142

  Banks are real-time institutions and they need FAST solutions. If you feel you can meet the challenge within a very stringent time limit, go ahead! A well designed sorting algorithm in a fast language is likely to succeed.

My code as follows :

def neww(k):
    neww=sorted(set(x+"%d"%(k.count(x)) for x in k))
    for each in neww:
        print each
for _ in range(int(raw_input())):
    p=raw_input()
    if p=='':
        p=raw_input()
    k=[raw_input() for _ in xrange(int(p))]
    neww(k)
    print ""

Time limit exceeded

When a solution is too slow, it's typically because the algorithm is one or more orders of magnitude worse than intended for the challenges. So take a hard look at the time complexity of the implementation.

In your implementation, it all comes down to this expression:

sorted(set(x+"%d"%(k.count(x)) for x in k))

k.count(x) : the centerpiece of the expression, to find the count of some value in a list. The count function does this by iterating over all elements k, and checking each for equality with x. Time complexity: \$O(N)\$, where N is the number of items in k
... for x in k : do something for each item in k. Again, \$O(N)\$, where N is the number of items in k
set(...) : probably \$O(N)\$
sorted(...) : probably \$O(N\log(N))\$

In total that gives: \$O(N^2) + O(N) + O(N\log(N))\$

Can this be better?

You need to sort, that's for sure. That's fast enough.
Do you really need a set? You used a set because the previous operations generate a list with duplicates. Using a set to remove duplicates is fine, but I would question why are duplicates at all in the first place.
The previous two items actually don't even matter. It's the slowest operation in the \$O(N^2) + O(N) + O(N\log(N))\$ that will drag everything down, the \$O(N^2)\$, so that's what you really need to optimize.

Instead of iterating over every account number for each account number,
you could iterate only once,
building a dictionary with counts along the way:

def print_sorted_with_count(accounts):
    counts = {}
    for account in accounts:
        counts[account] = counts.get(account, 0) + 1
    for account, count in sorted(counts.items()):
        print('{} {}'.format(account, count))

This implementation reduces the time complexity from \$O(N^2)\$ to \$O(N)\$,
and also gets rid of an \$O(N)\$ operation (no more need for the set.)

UPDATE

As @abarnert pointed out,
the dictionary of counts can be created with a single expression using collections.Counter, so the above code can become simply:

from collections import Counter

def print_sorted_with_count(accounts):
    counts = Counter(accounts)
    for account, count in sorted(counts.items()):
        print('{} {}'.format(account, count))

Naming

The variable names in the posted code are horrible,
and it makes it very hard to read.
Avoid single-letter variable names,
try to use descriptive names.

Coding style

I suggest to follow the recommendations of PEP8.


command-line demo app I started learning from the book: head first design patterns. As I am trying out different programs in the book, I am compiling them in this one demo app. Below is the code for its driver function. Can anyone guide me with any improvements to the program (related to style, programming, formatting, how to test, is it better to have try/catch/finally or throw exceptions and stuff like that or anything else) in this driver code?

Demo Driver Code:

package com.aviralgarg;

import java.util.Scanner;

import static com.aviralgarg.strategy.Main.runDuckSimulator;

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

        try {
            demoLoop();
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            System.err.println("Something went horribly wrong!");
        }
    }

    private static void demoLoop() {
        while (true) {
            showOptions();

            Scanner scanner = new Scanner(System.in);
            int option = scanner.nextInt();

            switch (option) {
                case 0 -> System.exit(0);
                case 1 -> showDesignPrinciples();
                case 2 -> runDuckSimulator();
                default -> System.err.println("Please select a valid option.");
            }

            showHorizontalLine();
        }
    }

    private static void showHorizontalLine() {
        System.out.println("------------------------------------------------------------------------------------");
    }

    private static void showOptions() {
        System.out.println("\nPick one of the following options:" +
                "\n\t1. Show design principles" +
                "\n\t2. Strategy pattern - Run the Duck Simulator" +
                "\n\t0. Exit");
    }

    private static void showDesignPrinciples() {
        System.out.println("Design Principles:" +
                "\n1. Identify the aspects of your application that vary and separate them from what stays the same." +
                "\n2. Program to an interface, not an implementation." +
                "\n3. Favor composition over inheritance." +
                "\n\n");
    }
}

Entire repository can be found here: https://github.com/aviral-garg/design-patterns. Any improvement in rest of those functions would be appreciated as well. Or I can just ask a separate question for that since the strategy design pattern is split into several different files and is quite abstracted away from this driver code.

main

It is not considered good practice to just catch every possible exception like this. You should always specify which exceptions are possible and should be catched. In this specific case, the try-catch-statement is not necessary at all, because you are catching all possible exceptions in demoLoop already:

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

demoLoop

Are you trusting the user with making correct inputs? If the user does enter a String instead of a number, your program will crash. You can solve this problem by using the following snippet:

private static void demoLoop() {
    Scanner scanner = new Scanner(System.in);
    showOptions();
    int option;
    while(true) {
        try {
            option = scanner.nextInt();
            if(option > 2 || option < 0) {
                throw new InputMismatchException();
            }
            break;
        }
        catch(InputMismatchException e) {
            System.out.println("Enter 0, 1 or 2!");
            scanner.nextLine();
        }
    }
}

(don't forget to import java.util.InputMismatchException;)

I also don't like the way how you use the switch-case-statement, but this is probably just a matter of taste. I prefer the standard way:

switch (option) {
    case 0 :
        System.exit(0);
        break;
    case 1 :
        showDesignPrinciples();
        break;
    case 2 :
        runDuckSimulator();
        break;
    default :
        System.err.println("Please select a valid option.");
}


Dijkstra's implementation in c++ with STL priority_queue This is dijkstras shortest path algorithm implementation in c++ using priority_queue STL.

Looking for two things: a) Correctness of algorithm itself and b) Any improvement suggestions.

/*                  3
    ----->  b   ----------->d
 10/ -1--  ^  |     1    ^/ |
  / /      |  |   ------// 1|
 x v      2|11|  / /-1--    |
  \        |  v / v         V
   \----->  c  -----------> e
     1             8
*/
class EdgeNode
{
  public:
  char label;
  int weight;
  EdgeNode(char l = 0, int w = 0) : label(l), weight(w) {}
};

using EdgeNodes = vector<EdgeNode *>;

class Graph
{
  public:
  unordered_map<char, EdgeNodes *> adj;

  void addNode(char node)
  {
    if(adj.find(node) == adj.end()) {
      adj[node] = new EdgeNodes();
    }
  }
  void addEdge(char start, char end, int weight)
  {
    if(adj.find(start) != adj.end()) {
      for(auto node : *adj[start]) {
        if(node->label == end) {
          node->weight = weight;
          return;
        }
      }
    } else {
      adj[start] = new EdgeNodes();
    }
    if(end) {
        adj[start]->push_back(new  EdgeNode(end, weight));
    }
  }

  void get_parent(unordered_map<char, char>& parent, char c)
  {
    cout << "Parent chain of \'" << c << "\' =";
    while((parent.find(c) != parent.end()) && parent[c]) {
      cout << parent[c] << ", ";
      c = parent[c];
   }
    cout << endl;
  }

  void get_distance(unordered_map<char, int>& dist)
  {
    cout << "distances => ";
    for(auto a : dist) {
       cout << "(" << a.first << "=" << a.second << ")  ";
    }
    cout << endl;
  }

  void djikstras()
  {
    unordered_set<char> visited;
    unordered_map<char, int> dist;
    unordered_map<char, char> parent;

    using Pair = pair<char, int>;
    priority_queue<Pair, vector<Pair>, greater<Pair>> pq;
    char snode = 'x';

    visited.insert(snode);
    dist[snode] = 0;
    parent[snode] = 0;
    pq.push(make_pair(snode, 0));

    while(!pq.empty()) {
      Pair front = pq.top(); pq.pop();
      for(EdgeNode *edge : *adj[front.first]) {
        if(visited.find(edge->label) == visited.end()) {
          dist[edge->label] = dist[front.first] + edge->weight;
          visited.insert(edge->label);
          parent[edge->label] = front.first;
          pq.push(make_pair(edge->label, dist[edge->label]));
        } else {
          /* //Parent comparison not needed in directed graph for djikstras
             //Its anyways useless because distance back to parent will always be larger
          if(parent[front.first] == edge->label) {
             cout << "Parent " << front.first << " = " <<  parent[front.first] << endl;
             continue;
          }*/
          if(dist[edge->label] > (dist[front.first] + edge->weight)) {
            dist[edge->label] = dist[front.first] + edge->weight;
            parent[edge->label] = front.first;
            pq.push(make_pair(edge->label, dist[edge->label]));
          }
        }
      }
    }
    get_distance(dist);
    get_parent(parent, 'e');
  }
};

int main(void)
{
  Graph g;
  g.addEdge('x', 'c', 1); g.addEdge('x', 'b', 10);
  g.addEdge('b', 'x', 1);  g.addEdge('b', 'c', 11); g.addEdge('b', 'd', 3);
  g.addEdge('c', 'b', 2);  g.addEdge('c', 'd', 1);  g.addEdge('c', 'e', 8);
  g.addEdge('d', 'c', 1);  g.addEdge('d', 'e', 1);
  g.addNode('e');

  g.djikstras();
}

Ohhh. Don't do this:

using EdgeNodes = vector<EdgeNode *>;
unordered_map<char, EdgeNodes *> adj;

Now you need to manage the memory of the EdgeNodes. Simply declare it as a value type:

using EdgeNodes = vector<EdgeNode>;      // Remove the star
unordered_map<char, EdgeNodes>      adj; // Remove the star.

To make the rest of your code simpler I would define a way to compare the EdgeNodes.

class EdgeNode
{
  public:
  char label;
  int weight;
  EdgeNode(char l = 0, int w = 0) : label(l), weight(w) {}

  // Compare a Node against a label
  bool operator==(char l) {return label == l;}
};

This makes your add functions simpler:

  void addNode(char node)
  {
      adj.insert({node, EdgeNodes{}});
  }
  void addEdge(char start, char end, int weight)
  {
      auto& dest = adj[node].second;

      // See if there is already a link to the destination.
      // This uses the `operator==` we defined above to compare
      // each node against `end`.
      auto  find = std::find(std::begin(dest), std::end(dest), end);

      if (find != std::end(dest)) {
          // If we already have it update the weight.
          find->weight = weight;
      }
      else {
          // otherwise add it to the end.
          dest.emplace_back(end, weight);
      }
  }

Don't be lazy:

  Pair front = pq.top(); pq.pop();

Split it over two lines. Its easy to write new code. Its hard to read other people's code. Don't make it difficult for them.

  Pair front = pq.top();   // Get the top item
  pq.pop();                // Pop it from the queue.

The dijkstras algorithm looks ok.

Things to look at:

I find it a bit dense to read and it took me a while to understand it but nothing technically wrong with it.
I might have used a single map for parent/distance calculations rather than two distinct structures.
Checking inclusion in the visited list is usually done on the node as it is popped of the dq not when pushing it onto the list. This may be a bug.
Normall you pass start and end as parameters to djikstras

Let me re-try a refactor:

void djikstras(char snode, char end)
{
    using ParentEdge = std::pair<char, EdgeNode>;
    auto  comp = [](ParentEdge const& l, ParentEdge const& r){return l.second.weight < r.second.weight;};

    unordered_map<char, EdgeNode>                        retrace;
    priority_queue<ParentEdge, vector<ParentEdge>, comp> pq;

    // special case the snode is its own parent.
    retrace[snode]   = EdgeNode(snode, 0);
    pq.push(ParentEdge(snode, EdgeNode(snode, 0)));

    while(!pq.empty()) {
        // Get details of next node.
        ParentEdge front   = pq.top();
        char&      parent  = front.first;
        char&      current = front.second.label;
        int&       weight  = front.second.weight;
        pq.pop();

        if (current === end) {
            // Did we find the destination.
            printRoute(retrace, end);
            return;
        }

        if (retrace.find(current) != retrace.end()) {
            // Already found cheapest route to here.
            continue;
        }

        // Found cheapest route to this point. Add info to the structures.
        retrace[current]  = EdgeNode(parent, retrace[parent].weight + weight);

        // Add children to the frontier list
        for(EdgeNode edge : adj[current]) {
            pq.push(ParentEdge(current, edge));
        }
    }
    std::cout << "Failed to find route from start to finish\n";
}


Recursion in Chess AI I am currently in the process of completely rewriting an older Chess project and have written the main recurring function in the AI:

//Finds and applies the best move for the ai.
function aiTurn() {
//Recursive function. recurBoard is the board array passed to the function.
//Colour is either -1 or 1, -1 means it's calculating for black and 1, white.
//depth is simply how deep the recursion has gone.
var recur = function(recurBoard, colour, depth) {
    //Stops recurring when max depth is reached. Max is defined in index.html.
    if (depth === max) return getEvaluation(recurBoard);
    //Setting a baseline for the biggest value of all deeper moves.
    var mVal = -10000*colour;
    //Cycling through the entire board array.
    for (var i = 0; i < 64; i++) {
        //Defines unit as the numeric representation of the current board position
        //(EG. -5 is a black queen, 1 is a white pawn and 6 is a white king).
        var unit = recurBoard[i];
        //Checks to see if the unit is a pawn
        //(currently only have programmed pawn movement for the ai).
        if (unit*colour != 1) continue;
        //Makes an array of all the moves the specific unit can make
        //(formated like this: [[9,17],[9,25]]
        //This would be the moves of a pawn on square 9).
        var moves = getMoves(unit, i, recurBoard);
        //Loops through all the moves.
        for (var j = 0; j < moves.length; j++) {
            //Defines movToUnt as the unit the current move will move on top of.
            var movToUnt = recurBoard[moves[j][1]];
            //If moving on to a king simply return a massive advantage.
            if (movToUnt === -6*colour) return 9000*colour;
            //Recurs. move(from,to,board) returns a board after a given move.
            var val = recur(move(moves[j][0], moves[j][1], recurBoard), -colour, depth+1);
            //Checks to see if the value of the move is bigger than that of the current best move
            if (val*colour > mVal*colour) mVal = val;
        }
    }
    //Returns the value of the best move.
    return mVal;
}

//This next part does pretty much the same thing as the for loop above, but stores the values.
var aiMoves = []
for (var i = 0; i < 64; i++) {
    var u = board[i];
    if (-u*playerSide != 1) continue;
    var uMoves = getMoves(u, i, board);
    for(var j = 0; j < uMoves.length; j++) {
        if (board[uMoves[j][1]] === 6*playerSide) {         //Shouldn't be necessary.
            uMoves[j][2] = -9000*playerSide;                //
            aiMoves[aiMoves.length] = uMoves[j];            //
            continue;                                       //
        }                                                   //
        uMoves[j][2] = recur(move(uMoves[j][0], uMoves[j][1], board), playerSide, 0);
        aiMoves[aiMoves.length] = uMoves[j];
    }
}
return aiMoves;
}

This properly recurs up to a given depth, but this gives the Chess AI the infamous "horizon effect" where it will make stupid decisions because it doesn't calculate far enough. This can be fixed simply by telling it to not stop recurring if it just killed a piece in its simulation.

While this is a reasonable initial attempt, this is not exactly what a modern chess engine would do. In fact, even if you would code the remaining pieces (you only have pawn now), the engine wouldn't be strong.

Before that, we should clear up some chess terminology:

We don't say simulation, we say search
The term AI is okay but even a better term would be engine

There're some very serious issues with your implementation:

You will need to account for transposition. This can be done by hashing.
You should take account of horizon effect by Quiescene Search.
You should really consider alpha-beta search or the related nega-max search.
In your code, you're just trying to brute force all moves in the deep-first fashion, this is absolutely unacceptable for chess programming. The search space will blow your code off. I repeat, you can't search every single position in chess, it's pointless.
What kind of move representation are you using? This looks like a simple array to me, maybe you'd consider a faster implementation?
Why is moving a pawn to the king is an advantage?

Depends on what you want to achieve, if you just want to code something that moves, it's fine. Unfortunately, the current code is nowhere close to anything what a modern chess engine is expected. As an experienced engine developer, I'd rate the code zero. It has lots improvement opportunities.

Please read about alpha-beta for your next attempt.


Turn a non-blocking concurrent queue into a blocking concurrent queue Microsoft Visual Studio offers a non-blocking concurrent queue class, concurrency::concurrent_queue, based on Intel's TBB. I am using this as a base for a blocking single producer, single consumer concurrent queue.

I am overriding the push and try_pop methods:

#include <concurrent_queue.h>
/**
 * Single producer, single consumer blocking concurrent queue.
 */
template<typename T>
class SPSCBlockingQueue final : public concurrency::concurrent_queue<T>
{
  public:

    /**
     * Push an element to the back of the queue.
     */
    void push(const T& src)
    {
        _push(src);
    }

    /**
     * Push an element to the back of the queue.
     */
    void push(T&& src)
    {
        _push(std::move(src));
    }

    /**
     * Pop an element from the head of the queue. This method will wait
     * until there is an item in the queue that can be dequeued.
     *
     * @param dest a reference to a location to store the dequeued item
     * @return {@code true} if an item was successfully dequeued, otherwise {@code false}
     */
    bool try_pop(T& dest)
    {
        std::unique_lock<std::mutex> lock(_mtx);

        auto res = concurrency::concurrent_queue<T>::try_pop(dest);
        if (!res)
        {
            // The queue must be empty.
            hasData = false;

            // Wait until there is some data...
            _cv.wait(lock, [&] { return hasData; });

            // This should now work.
            res = concurrency::concurrent_queue<T>::try_pop(dest);
        }

        // res should be true.
        return res;
    }

    /**
     * Pop an element from the head of the queue. This method will wait
     * until there is an item in the queue that can be dequeued.
     *
     * @return the dequeued item.
     */
    T pop()
    {
        T dest;
        try_pop(dest);
        return dest;
    }

  private:

    std::mutex _mtx;
    std::condition_variable _cv;
    bool hasData = false;

    template<typename E>
    void _push(E&& src)
    {
        bool notify = false;

        std::unique_lock<std::mutex> lock(_mtx);
        if (!hasData)
        {
            notify = true;
            hasData = true;
        }

        concurrency::concurrent_queue<T>::push(std::forward<E>(src));

        lock.unlock();

        if (notify)
            _cv.notify_one();
    }
};

Here is some unit test code, using the Google test framework:

TEST(CheckTools, BlockingQueue)
{
    enum Data { D1, D2, D3, DataEnd };
    SPSCBlockingQueue<Data> q;

    auto dataElements = {
        D2, D1, D2, D3, D2, D1, D1, D3, D3, D1,
        D1, D2, D1, D3, D2, D2, D3, D2, D1
    };

    vector<Data> dataToPush = dataElements;
    vector<Data> result;

    // Consumer thread...
    auto t = thread([&]
    {
        while (true)
        {
            auto dataEle = q.pop();

            if (dataEle == DataEnd)
                break;

            result.push_back(dataEle);
        }
    });

    // Producer code...
    auto i=0;
    for (auto e : dataToPush)
    {
        if (i++ % 3 == 0)
            this_thread::sleep_for(chrono::milliseconds(250));

        q.push(e);
        i++;
    }

    q.push(DataEnd);
    t.join();

    ASSERT_EQ(result, vector<Data>{ dataElements });
}

I am interested in comments regarding efficiency and correctness.

As a matter of consistency, we don't need to separate typename declaration for this private function void _push(T&& src), and we can depend on the typename declaration on the class level.

void _push(T&& src)
{
    bool notify = false;

    std::unique_lock<std::mutex> lock(_mtx);
    if (!hasData)
    {
        notify = true;
        hasData = true;
    }

    concurrency::concurrent_queue<T>::push(std::forward<T>(src));

    lock.unlock();

    if (notify)
        _cv.notify_one();
}

bool try_pop(T& dest) is not part of public interface, so we can define it as private.
We can depend on std::forward and modify the interfaces for push as follows:

void push(const T&& src)
{
    _push(std::forward<T>(src));
}
void push(T&& src)
{
   _push(std::forward<T>(src));
}

After adapting all these changes it should be as follows. Please note I don't have the system to compile this code from my end.

#include <concurrent_queue.h>
/**
 * Single producer, single consumer blocking concurrent queue.
 */
template<typename T>
class SPSCBlockingQueue final : public concurrency::concurrent_queue<T>
{
  public:

    void push(const T&& src)
    {
        _push(std::forward<T>(src));
    }
    void push(T&& src)
    {
        _push(std::forward<T>(src));
    }

    /**
     * Pop an element from the head of the queue. This method will wait
     * until there is an item in the queue that can be dequeued.
     *
     * @return the dequeued item.
     */
    T pop()
    {
        T dest;
        try_pop(dest);
        return dest;
    }

  private:

    std::mutex _mtx;
    std::condition_variable _cv;
    bool hasData = false;

    bool try_pop(T& dest)
    {
        std::unique_lock<std::mutex> lock(_mtx);

        auto res = concurrency::concurrent_queue<T>::try_pop(dest);
        if (!res)
        {
            // The queue must be empty.
            hasData = false;

            // Wait until there is some data...
            _cv.wait(lock, [&] { return hasData; });

            // This should now work.
            res = concurrency::concurrent_queue<T>::try_pop(dest);
        }

        // res should be true.
        return res;
    }

    void _push(T&& src)
    {
        bool notify = false;

        std::unique_lock<std::mutex> lock(_mtx);
        if (!hasData)
        {
            notify = true;
            hasData = true;
        }

        concurrency::concurrent_queue<T>::push(std::forward<T>(src));

        lock.unlock();

        if (notify)
            _cv.notify_one();
    }
};


JavaScript Good Patterns - Is this a good example? I have been trying various formats of namespaces and modules patterns, however I have not come across a solution that I would use for all my projects.

I've been developing the following, that would allow me to separate my application through applications, modules, functions and global variables. Possibly I'm going to add the possibility to have public and private variables and methods.

/*--------------------------------------------------------------------------
 * Project name
 *
 * Created      : 01/01/2014
 * Modified     : 01/01/2014
 * Version      : 0.1
 * Developer    : Developer Name
 *
---------------------------------------------------------------------------*/
/**
 * @class mainClassName
 * @constructor
 */
window.APP_NAME = window.APP_NAME || {};
window.APP_NAME.moduleName = window.APP_NAME.moduleName || {};
window.APP_NAME.deepExtend = window.APP_NAME.deepExtend || {};

window.APP_NAME.moduleName.globalVariableAvailableForEveryModuleFunctions = 0;

window.APP_NAME.moduleName.functionInsideModule1 = function (options) {

    "use strict";
    /*jslint nomen: true*/
    /*global $, jQuery*/

    // Check for arguments here.
    if (arguments.length !== 1) {
        throw new Error('[ClassName here] Please provide your options.');
    } // if

    // Increase global variable.
    window.APP_NAME.moduleName.globalVariableAvailableForEveryModuleFunctions += 1;

    // Store module defaults.
    window.APP_NAME.moduleName.config = {
        name: 'John',
        age: 29
    };

    // Parse incoming object that stores config files.
    window.APP_NAME.deepExtend(window.APP_NAME.moduleName.config, options);

    // Rest of the application here.
    alert(window.APP_NAME.moduleName.config.name);

    // Call function in same module, to alert the age
    window.APP_NAME.moduleName.functionInsideModule2();

};

window.APP_NAME.moduleName.functionInsideModule2 = function () {

    "use strict";
    /*jslint nomen: true*/
    /*global $, jQuery*/

    // Store global in local var.
    var age = window.APP_NAME.moduleName.config.age;

    // Alert age stored in Module config.
    alert(age);

};

// Helper function to change config defaults.
window.APP_NAME.deepExtend = function (destination, source) {
    for (var property in source) {
        if (source[property] && source[property].constructor &&
            source[property].constructor === Object) {
            destination[property] = destination[property] || {};
            arguments.callee(destination[property], source[property]);
        } else {
            destination[property] = source[property];
        }
    }
    return destination;
};

// Let's run the application!
(function () {

    new window.APP_NAME.moduleName.functionInsideModule1({
        name: 'Mary'
    });

}());

I would like to receive some feedback on this kind of approach, and what you would change.

Variable and method names are long to provide a better understand of context.

Lets look at a couple of improvements:

new is creating an object that you aren't using. read edit for clarification

new window.APP_NAME.moduleName.functionInsideModule1({
    name: 'Mary'
});

The function functionInsideModule1 doesn't return an object and doesn't assign anything to this. So there is no need for the new.
You're referencing the whole namespace window.APP_NAME.moduleName every time. Instead try putting it in a variable. Or maybe on the inside of the module  refer to this instead.

(function () {

    var moduleName = window.APP_NAME.moduleName;

    moduleName.functionInsideModule1 = function (options) {
        /* snip ... */

        moduleName.globalVariableAvailableForEveryModuleFunctions += 1;
        // OR
        this.globalVariableAvailableForEveryModuleFunctions += 1;
    };

    moduleName.functionInsideModule1({
         name: 'Mary'
    });

}());

Edit:-

To create a new instance of moduleName you need to do this:

(function () {
    // private variables go here
    var globalPrivateCount = 0;

    var moduleName = window.APP_NAME.moduleName = window.APP_NAME.moduleName || function moduleName(initialval) {
        globalPrivateCount ++;

        this.globallyAccessible = 'unique for each instance';

        this.functionInsideModule1 = function (options) { ... };
        this.functionInsideModule2 = function (options) { ... };

        var instancePrivateVariable = initialVal;

        this.incr = function () { instancePrivateVariable++; };

        return this;
    };
    moduleName.nonUniqueGloballyAccessible = 0;
}());

and the usage is something like:

var moduleName = window.APP_NAME.moduleName;
var firstInstance = new moduleName(10);
// OR
var anotherInstance = new window.APP_NAME.moduleName(20);


jQuery accordion list I'd like some tips on optimizing the following code.

I'm a bit concerned that I'm 'polluting the global namespace' by not wrapping it in a function. The commented out var rotator { init { came from a JavaScript tutorial but I'm not sure it's the jQuery way? Should I wrap it in $(function()... instead?

$(document).ready(function() {

//Promos rotation
var $accordionList = $('.accordion').find('li');

// var rotator = {
//      init: function() {
        var numberOfItems = $accordionList.length;
        var currentItem = 0;

        // Set first item to active
        $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {});
        var infiniateLoop = setInterval(function() {
            if(currentItem == numberOfItems - 1){
                currentItem = 0;
            }
            else {
                currentItem++;
            }

            // Remove active class, if is has it, and close content
            $accordionList.parent().find('li.active').removeClass('active')
                .find('.content').slideToggle(800, function() {
            });

            // Add active class and open content
            $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {
            });

        }, 4000 );

        //$('.accordion li').on('click', function () {
        $accordionList.on('click', function () {
            // Stop rotation
            clearInterval(infiniateLoop);

            var $accordionHead = $(this);

            // Remove active class, if is has it, and close content
            $accordionHead.parent().find('li.active').removeClass('active')
                .find('.content').slideToggle(800, function() {
            });

            // Add active class and open content
            $accordionHead.addClass('active').find('.content').slideToggle(800, function() {
            });
        });
//      }
// };

// rotator.init();

});

To your question ( confirmed by the comments ), you are not polluting the namespace because you are

Wrapping already inside $(document).ready(function() {
Using var for every variable.

Furthermore, from what I can see that is left over from rotator, it was probably a better approach than what you have now.

From a once over:

Spelling is important: infiniateLoop -> infiniteLoop
slideToggle(800, function() {}); can simply be written as slideToggle(800);
You could use a ternary for increasing currenItem : currentItem = currentItem == numberOfItems - 1 ? 0 : ++currentItem;
Please remove commented out code, also please re-indent after taking out rotator
It is considered good practice to have one big var at the start instead of multiple vars:

var $accordionList = $('.accordion').find('li'),
    numberOfItems = $accordionList.length,
    currentItem = 0;

    // Set first item to active
  $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800);

Jshint could not find a single flaw


Mongoose / MongoDB schemas for blog posts and authors I'm programming a model for a blog with authors and posts. I was following this doc: Mongoose Query Population

My concern is that the createPost function is inefficient.
Another option is to nest the posts in the Author, but how I will get all posts? Should I use MySQL? Am I doing it right? (Take a look at the functions as well)

Some more details:

Pastebin

let mongoose = require("mongoose");
let Schema = mongoose.Schema;

let PostSchema = Schema({
    title: {type: String},
    body: {type: String},
    date: {type: Date},
    tags:[{type: String}],
    _author: {type: Schema.Types.ObjectId, ref: 'author'}
});

let AuthorSchema = Schema({
    name: {type: String},
    photo: {type: String},
    bio: {type: String},
    username: {type: String, index: true},
    posts:[{type: Schema.Types.ObjectId, ref: 'post'}],
    password: {type: String}
});

let Author = mongoose.model('author', AuthorSchema);
let Post = mongoose.model('post', PostSchema);

module.exports = Author;
module.exports = Post;

module.exports.createAuthor = (newAuthor, callback)=>{
    newAuthor.save(callback)
};

module.exports.createPost = (username, newPost, callback)=>{
        Author.findOne({username:username}).then((author)=>{
            newPost._author = author._id
            author.posts.push(newPost);
            newPost.save().then(err, auth)=>{
                author.save(callback);
            };
        },(err)=>{
            if(err)
            throw err;
        });
};

module.exports.getAuthorByPostTitle = (postTitle, callback)=>{
        Post.findOne({title:postTitle}).populate('_author').exec((err, post)=>{
            if(err)
                throw err;
            else
                return post._author;
        });
};

module.exports.getPostsByAuthorId = (authorId, callback)=>{
        Post.find({_author:authorId}).exec((err, posts)=>{
            if(err)
                throw err;
            else
                return posts;
        });
};

You can use the forEach function like this

const allPosts = [];

Author.find({}, function(err, users) {
    if (err) throw err;

    users.forEach(function(user) {
        const a = user.posts;
        a.forEach(us => allPosts.push(us));
    });
});

Now the allPosts array will have all the posts.


Converting a vector image to matrix I am playing with the cifar-10 dataset (available here) and for now I would like to plot one of the images of a batch.

The images are represented as vectors when I get them from pickle:

  From the cifar-10 documentation:
  The first 1024 entries (of an image) contain the red channel values, the next 1024 the green, and the final 1024 the blue. The image is stored in row-major order, so that the first 32 entries of the array are the red channel values of the first row of the image.

What I came up with to plot an image is this:

import numpy as np
import matplotlib.pyplot as plt

# get the dataset
a = unpickle('./cifar-10/data_batch_1')
# get the first image
img = np.array(a[b'data'][0])
# transform it to a 3 x 1024 array, one row per color channel
# and transpose it to a 1024 x 3 array, one row per rgb pixel
img = img.reshape(3, 1024).T
# reshape it so we can plot it as a 32 x 32 image with 3 color channels
img = img.reshape(32, 32, 3)

# plot
plt.imshow(img)
plt.show()

It's my first attempt at matrix manipulation so even if this is concise, I feel like it could be simpler. What do you guys think?

One alternative is to transform it to the right shapes, then use moveaxis. I don't know how much simpler this is than what you've got, I guess it avoids one reshaping operation.

img = img.reshape(3, 32, 32)
img = np.moveaxis(img, 0, -1) # move the first axis to the end

or as a one-liner:

img = np.moveaxis(img.reshape(3, 32, 32), 0, -1)

Note that moveaxis returns a view, meaning that no data is copied.