Reduce one for loop to decrease time complexity I'm using NumPy to find out langrage polynomial interpolation. I'm using 2 for loop to find out langrage polynomial, but I want to reduce 2nd for loop so that my code time complexity can be less. Can you guys please suggest me how I can cut a for loop to increase my time complexity.
import numpy as np
import matplotlib.pyplot as plt
class LagrangePolynomial:
    def __init__(self, data_x, data_y):

        assert type(data_x) == np.ndarray, "data_x is not numpy array"
        assert type(data_y) == np.ndarray, "data_y is not numpy array"
        assert len(data_x) == len(data_y), "length of data_x and data_y must be equal"

        self.x = data_x
        self.y = data_y

        self.degree = len(data_x) - 1

    def __str__(self):
        strL = f"LagrangePolynomial of order {self.degree}\n"
        strL += "p(x) = "
        count=0
        for i in self.y:
            if i == 0:
                continue
            elif i >= 0:
                strL += f"+ {i}*l_{count + 1}(x) "
            else:
                strL += f"- {-i}*l_{count + 1}(x) "
            count+=1
        return strL
    def __call__(self,x):
        y_interp = np.zeros(len(x))
        for i in range(len(x)):
            #remove this for loop.
            for j in range(self.degree + 1):
               upper=np.prod(np.delete(self.x - x[i],j))
               lower=np.prod(np.delete(self.x[j] - self.x, j))
               y_interp[i] +=(upper/lower)*self.y[j]
        return y_interp
data_x = np.array([-3.,-2.,-1.,0.,1.,3.,4.])
data_y = np.array([-60.,-80.,6.,1.,45.,30.,16.])

p = LagrangePolynomial(data_x, data_y)

#generating 100 points from -3 to 4 in order to create a smooth line
x = np.linspace(-3, 4, 100)
print(p)
y_interp = p(x)
plt.plot(x, y_interp, 'g--')
plt.plot(data_x, data_y, 'ro')
plt.legend(['interpolated', 'node points'], loc = 'lower right')
plt.xlabel('x')
plt.ylabel('y')
plt.title('Lagrange Polynomial')

plt.show()

The second loop can be eliminated by creating a 2D array by tiling the self.x array with the np.tile method. The removal of elements in de 1D array with np.delete can than be replaced with a boolean mask (unfortunately this results in a 1D array which needs to be reshaped).
The calculation of the 2D array can be done outside the first loop, as can be the calculation of lower as they do not depend on x.
See my rewrite of the __call__ method:
def __call__(self,x):

    y_interp = np.zeros(len(x))

    n = self.degree + 1
    mask =  ~np.eye(n, dtype=np.bool)

    tiled = np.tile(self.x, (n,1))
    masked = np.reshape(tiled[mask], (n, n-1))

    lower = np.prod(self.x[:,np.newaxis] - masked, axis=1)

    for i in range(len(x)):

        upper = np.prod(masked -x[i], axis=1)

        y_interp[i] = np.sum((upper/lower) * self.y)

    return y_interp


Removing node from a linked list I'm implementing the singly linked list data structure and functions to work with it. In particular I'm concerned about prepending and removing nodes. Currently I defined linked list as follows:

typedef struct string_list_node{
    int id;
    const char *value;
    struct string_list_node *next;
} string_list_node_t;

string_list_node_t **string_list_alloc(int id, const char *value){
    string_list_node_t *string_list_ptr = malloc(sizeof(*string_list_ptr));
    string_list_node_t **list = malloc(sizeof(*list));
    string_list_ptr -> id = id;
    string_list_ptr -> value = value;
    string_list_ptr -> next = NULL;
    *list = string_list_ptr;
    return list;
}

The thing I'm concerned about is the following implementation of prepend and delete_first_by_id functions:

void prepend(int id, const char *value, string_list_node_t ** list_ptr){
    string_list_node_t *node_ptr = *list_ptr;
    string_list_node_t *prepended_node = malloc(sizeof(*prepended_node));
    prepended_node -> id = id;
    prepended_node -> value = value;
    prepended_node -> next = node_ptr;
    *list_ptr = prepended_node;
}

//returns 0 if not found, 1 if found and list changed
int delete_first_by_id(string_list_node_t **source,
                       int id,
                       string_list_node_t ** deleted_value_out){
    string_list_node_t *prev = NULL;
    string_list_node_t *current = *source;
    while(current && current -> id != id){
        prev = current;
        current = current -> next;
    }

    if(current){
        if(!prev){
            *source = current -> next;
            *deleted_value_out = current;
        } else {
            *deleted_value_out = current;
            prev -> next = current -> next;
        }
        return 1;
    } else {
        return 0;
    }
}

Is this code looks ok in terms of sort of common C style? Is the behavior of the code undefined? I created a live example of usage here:

LIVE DEMO

Style

POSIX reserves type names ending with _t. This means that in the future, standard c may create standardized types with the same name as string_list_node_t, and your code would not compile. See https://stackoverflow.com/a/12727104 for more details. A common alternative is to use upper camel-case for typedefs, e.g. StringListNode.
There is a strong case against typedefing structures. The major benefit of this is to save keystrokes, but it removes important information available to programmers. To see some opinions on this, here is another stack overflow question.
I have never seen code that has spaces between -> when accessing struct members. Just  use prepended_node->id instead. You wouldn't write prepended_node . id, would you?

Code Review

Use the bool type

A boolean type, bool, was introduced in C99. Unless you have really good reasons not to, you should use this for true/false values instead of an integer. This type is found in the <stdbool.h> header.

Multi-use functions

Finding nodes in a linked list is a relatively common procedure. If your list supports lookup, it is better to have a separate function that can handle all of your lookup cases:

static void _find_node_by_id(int id, str_node *head,
                str_node **prev, str_node **node) {
  *prev = NULL;
  *node = head;
  while(node && (*node)->id != id) {
    *prev = *node;
    *node = (*node)->next;
  }
}

This way, you can use the search implementation in multiple places:

bool delete_first_by_id(string_list_node_t **source,
                       int id,
                       string_list_node_t ** deleted_value_out){
  string_list_node_t *prev = NULL;
  string_list_node_t *current = *source;

  _find_node_by_id(id, *source, &prev, &current);

  if(current){
    if(!prev){
      *source = current -> next;
      *deleted_value_out = current;
    } else {
      *deleted_value_out = current;
      prev -> next = current -> next;
    }
    return true;
  } else {
    return false;
  }
}

bool find_node_by_id(str_node *head, int id, str_node **return_ptr) {
  string_list_node_t *prev = NULL;
  *ret_ptr = head;

  _find_node_by_id(id, *source, &prev, return_ptr);

  if(*return_ptr) {
      return true;
  } else {
      return false;
  }
}

Note that I didn't test any of this code.

Try to avoid pointers to pointers

More specifically, don't use them when it's not necessary. There isn't a great reason for string_list_alloc to return string_list_node_t **. It makes much more sense to just return a pointer to the node. This does two things for you:

Non-destructive operations don't need a pointer to a pointer, so it avoids either
you or the function from needing to deference them.
It makes destructive operations more explicit, as you need to take the address of
the pointer to use the function.

Edit addressing question in comments:

  In case I want to preserve it across different function calls I should not allocate it on the stack shouldn't I?

There seems to be some confusion on how malloced memory and pointers work, especially in the context of function return values.

First, remember that a variable holds a value. In this case, the variable contains a memory location, whose value could be any integer greater than or equal to 0. We really don't care about the variable itself, we care about its value.

Second, lets talk about return values. When you return a value from a function, it is copied so that the code calling the function has access to it. Here's a little program that illustrates this:

#include <stdio.h>

struct test_struct {
  int a;
  int b;
};

struct test_struct return_struct() {
  struct test_struct test = { .a = 1, .b = 2 };
  printf("The pointer in the function is: %p\n", &test);
  return test;
}

int main(void) {
  struct test_struct main_struct;
  main_struct = return_struct();
  printf("The pointer in main is: %p\n", &main_struct);
  return 0;
}

On my system, this program prints out

The pointer in the function is: 0x7fffef30f048
The pointer in main is: 0x7fffef30f070

Notice that the addresses are different. Since the value being returned is copied, we don't need to worry about manually manipulating memory, as the compiler takes care of that for us. It works the same way when returning a pointer from function: the value we care about is copied, so that we can access it when the variable that originally held it no longer exists.

Lets look at this in a different context.

// what your alloc function is doing:
int *malloc_add(int a, int b) {
    int *result = malloc(sizeof(result));
    *result = a + b;
    return result;
}
// what it should be doing:
int add(int a, int b) {
    int result;
    result = a + b;
    return result;
}

While we get the value we want in both cases, in the first we need to perform two extra steps: we need to dereference the pointer to get the value, and we need to free the memory allocated on the heap.


Calculate min/max values for signed or unsigned integral number with any valid number of bits (up to 64) This came up when I answered another post.

Is there a better way?

public class Bits
{
    public static void MinMaxI(ulong _bits, out long _min, out long _max)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        _min = (long)1 << (int)(_bits - 1);
        _max = -1 - _min;
    }

    public static long MinI(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        return (long)1 << (int)(_bits - 1);
    }

    public static long MaxI(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        return -1 - ((long)1 << (int)(_bits - 1));
    }

    public static ulong MaxN(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 1 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("1 <= _bits <= 64"));
        }

        return _bits == 64 ? ~(ulong)0 : ~(~(ulong)0 << (int)_bits);
    }
}

First of all, I think there are misprints in your code that maximums are negative and minimums are positive.

Next. Since you need MinI, MaxI and MaxN values for a few bits only, it makes sense to calculate these values apriory. In this case we can use loop for calculation.
In the following code I've used recurrent formulas for calculation, which are much more readable in my opinion:

public class Bits
{
    public const int MaxBits = 64;

    public static readonly ReadOnlyCollection<long> MinI;
    public static readonly ReadOnlyCollection<long> MaxI;
    public static readonly ReadOnlyCollection<ulong> MaxN;

    static Bits()
    {
        long[] minI = new long[MaxBits + 1];
        long[] maxI = new long[MaxBits + 1];
        ulong[] maxN = new ulong[MaxBits + 1];

        maxN[1] = 1;

        for (int i = 2; i < MaxBits; i++)
        {
            maxI[i] = maxI[i - 1] * 2 + 1;
            minI[i] = -maxI[i] - 1;
            maxN[i] = maxN[i - 1] * 2 + 1;
        }

        maxI[MaxBits] = long.MaxValue;
        minI[MaxBits] = long.MinValue;
        maxN[MaxBits] = ulong.MaxValue;

        MinI = new ReadOnlyCollection<long>(minI);
        MaxI = new ReadOnlyCollection<long>(maxI);
        MaxN = new ReadOnlyCollection<ulong>(maxN);
    }
}

Usage:

Console.WriteLine(Bits.MinI[3]);
Console.WriteLine(Bits.MaxI[3]);
Console.WriteLine(Bits.MaxN[3]);

Output:

-4
3
7

The only difference in functionality I can see is that arrays have values for bits 0 and 1.
Hope it is not a problem for you.


Ways to differentiate comments in source code (PyCharm) I want to differentiate between comments I make for

(1) myself,
(2) team members,
(3) on business rules,
(4) on watchout notes for hardcoded constants,
(5) on ideas for improvements, and
(6) on general flow of the code.

Right now all my comments look the same.

I'm open to any other tools. I don't know how bookmarking works, or if I really should be adding debug points as a "TODO" (the auto "TODO" font is weird and intrusive), but I'm open to anything.

I use PyCharm.

As for what this code accomplishes, it takes in an input of data on students who tested for a Science exam and filters out the students with errors. That means errors in scoring that a school needs to resolve. Therefore, it gives back a highly filtered list of students (200 rows) from a much larger (40,000 rows) dataset.

import pandas as pd
import numpy as np
import xlrd
import openpyxl
import win32com.client
import paths
import utils as u

'''
Extraction
'''
# (4) SXRX row starts at row 5
# (4) SXRX has 19 rows at its footer
SXRX_df = pd.read_excel(paths.corr_outputs_path, sheet_name='SXRX', header=4, skipfooter=19)
# (4) drop first row because it's blank
SXRX_df = SXRX_df[1:]

# (5) better to do writer?
corr_outputs_df = pd.read_excel(paths.corr_outputs_path, sheet_name='Correct Outputs')

'''
Operations
'''
def reason_incomplete(school_follow_up, site_follow_up, errors):
    """
    We expect mainly P.1 Not Scanned. Teacher errors, while rare, will still exist (especially in Charter schools)

    :param school_follow_up: RCSD school follow up
    :param site_follow_up: RCSD site follow up
    :param errors: RCSD errors, can come in the format of 046/57, sometimes none
    :return: text according to business rules
    """

    # (2) explicitly cast errors as string, because sometimes it will come in as a float and the "in errors" syntax will
    # not work
    errors = str(errors)
    if school_follow_up == 'P.1 Not Scanned':
        return 'P.1 Not Scanned'
    elif site_follow_up == 'FOLLOW UP':
        return 'P.2 Not Scanned'
    elif '40' in errors:
        return 'Abs + Response'
    elif '41' in errors:
        return 'Multiples in Student Answers'
    elif '47' in errors:
        return 'Omits in Teacher Answers'
    elif '46' in errors:
        return 'Multiples in Teacher Answers'
    else:
        return None

df = pd.DataFrame()

# (1) genericize later

df['Last Name'] = SXRX_df['Last Name']

df['First Name'] = SXRX_df['First Name']

df['Student ID'] = SXRX_df['Student Id']

df['School DBN'] = SXRX_df['School DBN']

df['Ofc Cls'] = SXRX_df['Cls']

df['Section'] = SXRX_df['Section']

df['Final Score'] = SXRX_df['Score.1']

df['Reason Incomplete'] = np.vectorize(reason_incomplete)(SXRX_df['Follow Up'],
                                                          SXRX_df['Follow Up.1'],
                                                          SXRX_df['Errors'])

df['Exam'] = 'SXRX'

# (6) filter out Reason Incomplete Nones

# (6) filter out Final Score INV and MIS

# (6) reorder columns
df = df.reindex(columns=corr_outputs_df.columns)

I think the major problem with this current system is this: Imagine you come back 6 month after having written these comments and have not used this commenting system elsewhere since then. What category does a comment like (4) ... belong to? Without looking it up! Now imagine someone else looks at your code and needs to figure out which comments are meant for them.

In other words, the system is not self explanatory.

(Ab)using multi-line strings as comments is also discouraged. You are not even using them as multi-line string anyway.

It would already help if you used words instead of numbers. Maybe this is already enough to cover all your cases:

# TODO NAME note to name or # FIXME important bugfix
# TODO TEAM note to team members,
# just plain comments for business rules
# CONSTANT description
# NOTE idea for improvement
# just plain comments on the flow of the code

At least in my editor, TODO, FIXME and NOTE all get the same special highlight.

Your code itself you should re-organize a bit. Ideally you define all your classes and functions first and put code calling it afterwards. Don't intersperse the two.

The definition of df can also be simplified a bit using extended indexing of pandas dataframes and pandas.DataFrame.rename:

df = SXRX_df[['Last Name', 'First Name', 'Student Id', 'School DBN', 'Cls', 'Section',
 'Score.1']]
df = df.rename(columns={'Cls': 'Ofc Cls', 'Score.1': 'Final Score'})
df['Exam'] = 'SXRX'
df['Reason Incomplete'] = reason_incomplete(SXRX_df['Follow Up'],
                                            SXRX_df['Follow Up.1'],
                                            SXRX_df['Errors'])

Note that the columns are in the order in which they appeared in the list, so you might not even need the reindex.

The np.vectorize can also already be used as a decorator at the time of function definition and save some real estate in the block above:

@np.vectorize
def reason_incomplete(school_follow_up, site_follow_up, errors):
    ...


More idiomatic way to move item from one dict value(list) to another? Is there a more idiomatic way to handle the following scenario? I'm open to lodash, etc. if needed. This does what I want, but I feel that there is probably a shortcut I'm missing:

const migrateValues = (obj, srcKey, destKey, value) => {
    updatedSrc = [];
    obj[srcKey].forEach(x => {
        if (x !== value) {
            updatedSrc.push(x);
        }
    });
    obj[destKey].push(value);
    obj[srcKey] = updatedSrc;
    return obj;
}

obj = {
    "key1": [1, 2, 3],
    "key2": [4, 5, 6],
    "key3": [7, 8, 9],
    "key4": [10, 11, 12]
};
valueToMove = 11;
srcKey = "key4";
destKey = "key2";

console.log(obj);
console.log(migrateValues(obj, srcKey, destKey, valueToMove));

I would propose something like so (given that you have assumed no dupes, etc)

function migrateValues(obj, srcKey, destKey, value) {
    return {
        ...obj,
        [srcKey]: obj[srcKey].filter(w => w !== value),
        [destKey]: obj[destKey].concat(value)
    };
}

We spread obj, and then overwrite the srcKey and destKey properties.

There are several reasonable ways to make obj[srcKey].filter and obj[destKey].concat avoid TypeErrors, which is best is dependent on what you have planned for the function.


Confirming whether a user is registered, using Optional class in Java 8 I start to learn Java 8 and I wondering is that kind of using Optional class is correct?

@Override
public Response confirmRegister(Request request) {
    return mailTokenRepository.findByToken(request.getToken()).map(mailToken -> {
        if (mailToken.getTypeToken().equals(TypeToken.REGISTER)
                && mailToken.getToken().equals(request.getToken())) {
            return Optional.ofNullable(userRepository.findOne(mailToken.getUser().getId())).map(user -> {
                user.setBlock(false);
                userRepository.save(user);
                mailTokenRepository.delete(mailToken);
                return jsonService.prepareResponseForUser(StatusMessage.SUCCESS, "SUCCESS_REGISTERED", null, null,
                        true);
            }).orElseGet(() -> jsonService.prepareErrorResponse("INCORRECT_DATA"));
        } else
            return jsonService.prepareErrorResponse("INCORRECT_DATA");
    }).orElseGet(() -> jsonService.prepareErrorResponse("INCORRECT_DATA"));

}

Concerning your question about the usage of Optional: yes, this usage is correct and it roughly respects the principles of functional programming.

However, I would recommend a few changes to increase the readability of this piece of code:

1) jsonService.prepareErrorResponse("INCORRECT_DATA") appears three times: it needs to be extracted into a dedicated method.

2) The main valid return case starting with return Optional.ofNullable should also be extracted into a dedicated method.

3) The condition should also be extracted.

After this refactoring, the code will resemble something like

public Response confirmRegister2(Request request) {
  return mailTokenRepository.findByToken(request.getToken())
                            .map(mailToken -> {
    if (isTokenValid(mailToken, request)) {
      return responseForUser(mailToken).orElseGet(this::errorResponse);
    } else {
      return this.errorResponse();
    }
  }).orElseGet(this::errorResponse);
}

private boolean isTokenValid(MailToken mailToken, Request request) {
  return mailToken.getTypeToken().equals(TypeToken.REGISTER)
         && mailToken.getToken().equals(request.getToken());
}

private Response errorResponse() {
  return jsonService.prepareErrorResponse("INCORRECT_DATA");
}

private Optional<Response> responseForUser(MailToken mailToken) {
  return Optional.ofNullable(userRepository.findOne(mailToken.getUser().getId()))
                .map(user -> {
                      user.setBlock(false);
                      userRepository.save(user);
                      mailTokenRepository.delete(mailToken);
                      return jsonService.prepareResponseForUser(StatusMessage.SUCCESS,
                                                              "SUCCESS_REGISTERED", null, null, true);
                    });
}

... which is definitely more readable and easier to understand. :)


more elegant way when call parent method with the same name My subclass extends the Formatter class,
I wonder know is there any elegant way, to rewrite my code

Parent

class Formatter(object):
    def resp_in_hash(self, resp):
        ...
        return rtn_hash

Child

from formatter import Formatter
# -----------------------------------------------------------------
class Request(Formatter):

    def req(self, url_path, is_no_cache=True):
        ...
    def resp_in_hash(self, url_path):
        resp, content = self.req(url_path)
        return super(Request, self).resp_in_hash(content)

Two ways to rewrite it:

When you know which version of the method you want to call you can name it directly, but in that case need to pass self as an argument:

    return Formatter.resp_in_hash(self, content)

Not necessarily more elegant.

Python 3 introduced a new cleaner super format for the common case when you want to step up from the current class and pass self (the first argument):

    return super().resp_in_hash(content)

This only works in python 3, though.


Pairwise programming challenge - Free code camp Challenge:

Given an array arr, find element pairs whose sum equal the second argument arg and return the sum of their indices.

If multiple pairs, that have the same numeric elements but different indices are possible, return the smallest sum of indices. Once an element has been used, it cannot be reused to pair with another.

For example pairwise([7, 9, 11, 13, 15], 20) returns 6.

My code (works, passes all validations) is the below:

function pairwise(list, sum) {
  // noprotect
  var innerCounter,
      outerCounter,
      used = {},
      returnValue = 0;
  for( outerCounter = 0 ; outerCounter < list.length-1 ; outerCounter++){
    if( used[outerCounter] )
      continue;
    for( innerCounter = outerCounter+1 ; innerCounter < list.length ; innerCounter++){
      if( used[innerCounter] )
        continue;
      if( list[innerCounter] + list[outerCounter] == sum ){
        used[innerCounter] = true;
        returnValue = returnValue + innerCounter + outerCounter;
        break;
      }
    }
  }

  return returnValue;
}

I had some internal debate on sum as a parameter name, since we also return a sum. But I stuck to returnValue for the return value and sum for the parameter. Any feedback on this is welcome, from style to algorithm.

My only comments would be stylistic so may be more a matter of opinion. Am open to any counter comments.

Whitespace

All your code is bunched together and is not as easy to read as it could be.

Single Statement if braces

It is often beneficial and consistent to always have braces on if and other blocks even if there is only a single statement. It's easy to introduce undesirable functionality if you come back to the code later and comment/add lines e.g.

if(someValue)
    //doThis();
alwaysDoThis();

Shorthand arithmetic

returnValue = returnValue + innerCounter + outerCounter;

can be shortened to

returnValue += innerCounter + outerCounter;


Generic Singly Linked List in C++ I am currently working on implementing some data structures as a way to learn C++. I'm coming mostly from a Java background so I was hoping to get some feedback regarding this implementation of a singly linked list in C++ using templates. This singly linked list will only support insertions at the head and tail positions and deletions from the head position. The plan is to use this as a base to build further data structures. I would like to create a rather complete library of data structures, so I was hoping to make them as extensible and reusable as possible.

Below follows the class definition.

#ifndef LINKEDLISTS_SINGLYLINKEDLIST_H_
#define LINKEDLISTS_SINGLYLINKEDLIST_H_

#include <cstddef>
#include <iostream>

template <typename E>
class SinglyLinkedList {

private:

    /** BEGIN NESTED NODE CLASS **/
    class Node {

    private:
        E data; //Should this be a reference?
        Node* next;

    public:
        Node(Node* n, const E& d) : data(d), next(n) {};

        Node* getNext() const { return next; }

        E& getData() { return data; }

        void setNext(Node* n) { next = n; }

    }; /** END NESTED NODE CLASS **/

    /** Begin fields of SinglyLinkedList class **/
    Node* head;
    Node* tail;
    int listSize;
    /** End fields of SinglyLinkedList class **/

public:

    SinglyLinkedList();
    ~SinglyLinkedList();

    //Accessor methods
    bool isEmpty() const;
    int size() const;
    const E& first() const;
    const E& last() const;

    //Update methods
    void addFirst(const E&);
    void addLast(const E&);
    E* removeFirst();

    void print() const;

}; /** END SINGLYLINKEDLIST CLASS HEADER **/

#include "SinglyLinkedList.tpp" //Include implementations of template code.

#endif /* LINKEDLISTS_SINGLYLINKEDLIST_H_ */

The implementations of the functions follow below:

/*
 * SinglyLinkedList.tpp
 *
 *  Created on: Jan 5, 2020
 *      Author: dii
 */

/** Constructor **/
template<typename E>SinglyLinkedList<E>::SinglyLinkedList() : head(nullptr), tail(nullptr), listSize(0) {}

/** Destructor **/
template<typename E>SinglyLinkedList<E>::~SinglyLinkedList() {
    while(!isEmpty()) {
        delete removeFirst();
    }
}

/** Begin functions **/

template<typename E> bool SinglyLinkedList<E>::isEmpty() const { return listSize == 0; }

template<typename E> int SinglyLinkedList<E>::size() const { return listSize; }

template<typename E> const E& SinglyLinkedList<E>::first() const { return head->getData(); }

template<typename E> const E& SinglyLinkedList<E>::last() const { return tail->getData(); }

template<typename E> void SinglyLinkedList<E>::addFirst(const E& d) {

    Node* temp = new Node(head->getNext(), d); //Create a new node.
    head = temp; // Set the head pointer to point to this object.
    listSize++; // Increase size of list by 1.

}

template<typename E> void SinglyLinkedList<E>::addLast(const E& d) {

    Node* temp = new Node(nullptr, d);
    if(isEmpty()) {
        head = temp;
        tail = temp;
    } else {
        tail->setNext(temp);
        tail = temp;
    }

    listSize++;

}

template<typename E> E* SinglyLinkedList<E>::removeFirst() {

    if(isEmpty()) {
        return nullptr;
    }

    E* temp = &(head->getData());
    head = head->getNext();
    listSize--;

    return temp;

}

template<typename E> void SinglyLinkedList<E>::print() const {
    Node* walk = head;
    while(walk != nullptr) {
        std::cout << walk->getData() << std::endl;
        walk = walk->getNext();
    }
}

I have a feeling that the destructor is not implemented as well as it could be. I am also still not entirely sure about when to use references, e.g., should the data field of the Node class be of type E& or is just E ok?

Design

It's relatively simple to create a double linked list!

Some things you should think about some more:

Please put your code into a namespace. Do you think that SinglyLinkedList is a very unique name in the global scope?
You need to look into const correctness.
You need to look into move semantics.

Major Bug.

You have not implemented the rule of three (or five). When a class contains an owned pointer the compiler generated copy constructor, copy assignment operator and destructor are not going to work correctly.

Your SinglyLinkedList contains the owned pointer: head. An owned pointer is one that you have taken responsibility for the lifespan of (and are thus going to call delete on it when it the object is destroyed.

{
     SinglyLinkedList<int>    list1;
     list1.addFirst(1);

     SinglyLinkedList<int>    list2(list1);
}
// This code is broken as you have a double delete.

Upgrade you need to do.

The standard library defines things called concepts. A concept is a set of properties that a class has. One of the big concepts of the standard is the concept of a "Container" (of which a singly linked list is). Ao you should probably make your list "Container" compliant.

Code Review

I hate useless comments.

    /** BEGIN NESTED NODE CLASS **/
    class Node {

That counts as a useless comment.

OK good start.

        Node(Node* n, const E& d) : data(d), next(n) {}   ;
                 // Not Needed                           ^^^

But it does mean that you are required to copy the object into the list. Modern C++ has the concept of move semantics. So you should allow for moving the object into the list.

        Node(Node* n, E&& d) : data(std::move(d)), next(n) {}

Also you should allow the object to be constructed in place:

        template<typename... Args>
        Node(Node* n, Args&&... args)
            : data(std::forward<Args>(args)...)
            , next(n)
        {}

Here you get a reference to the object.
That's fine.

        E& getData() { return data; }

But what happens if your list has been passed by const reference to a function. Now you can no longer accesses the data as you can't get a const reference to the data.

        E const& getData() const {return data;}

Its a bit strange that you only return reference from the node. But you return only const references from the list?

    const E& first() const;
    const E& last() const;

I actually hate your get/set mentatility here. I see no need for it inside the node object itself.

        void setNext(Node* n) { next = n; }

That's fine.

    void print() const;

But two things to note.

There is more streams than the std::cout so allow this to be pramertized.
The normal way of streaming is via the output operator: operator<<()

void print(std::ostream& stream = std::cout);
friend std::ostream& operator<<(std::ostream& str, SinglyLinkedList const& data)
{
    data.print(str);
    return str;
}


Unit testing with dependency injection and MOQ I'm just learning how dependency injection and mocking work, but I'd like some feedback on how I'm setting up a couple of tests.  I can get them to pass, but I'm not sure this is all I need.

This is an MVC application that makes Web API calls to return data.  For this example I'm running queries in the Web APIs that populate dropdowns.

Please give me any and all suggestions about what I'm doing right or wrong here or anything I should be doing differently.

Setup file for Dependency Injection - Unity.WebAPI (NuGet Package)

UnityConfig.cs

public static class UnityConfig
{
    public static void RegisterComponents()
    {
        var container = new UnityContainer();

        // register all your components with the container here
        // it is NOT necessary to register your controllers

        // e.g. container.RegisterType<ITestService, TestService>();

        container.RegisterType<IDropDownDataRepository, DropDownDataRepository>();

        GlobalConfiguration.Configuration.DependencyResolver = new UnityDependencyResolver(container);
    }
}

Controller

public class DropDownDataController : ApiController
{
    private IDropDownDataRepository _dropDownDataRepository;

    //Dependency Injection (I'm using Unity.WebAPI)
    public DropDownDataController(IDropDownDataRepository dropDownDataRepository)
    {
        _dropDownDataRepository = dropDownDataRepository;
    }

    [HttpGet]
    public HttpResponseMessage DateList()
    {
        try
        {
            return _dropDownDataRepository.DateList();
        }
        catch
        {
            throw new HttpResponseException(new HttpResponseMessage(HttpStatusCode.NotFound));
        }
    }
}

Repository

public class DropDownDataRepository : IDropDownDataRepository
{
    //Is this fine in here, or should it be injected somehow too?
    private MyDatabaseEntities db = new MyDatabaseEntities();

    public HttpResponseMessage DateList()
    {
        var sourceQuery = (from p in db.MyProcedure()
                           select p).ToList();

        string result = JsonConvert.SerializeObject(sourceQuery);
        var response = new HttpResponseMessage();
        response.Content = new StringContent(result, System.Text.Encoding.Unicode, "application/json");

        return response;
    }
}

INTERFACE

public interface IDropDownDataRepository
{
    HttpResponseMessage DateList();
}

Unit tests

/// <summary>
/// Tests the DateList method is run
/// I pieced this kind of test together from examples online
/// I'm assuming this is good for a simple test
/// </summary>
[TestMethod]
public void DateListTest1()
{
    //Arrange
    var mockRepository = new Mock<IDropDownDataRepository>();
    mockRepository.Setup(x => x.DateList());
    var controller = new DropDownDataController(mockRepository.Object);

    //Act
    controller.DateList();

    //Assert
    mockRepository.VerifyAll();
}

/// <summary>
/// Tests the DateList method returns correct status code.
/// This will run with success, but I'm not sure if that's just
/// because I'm telling it to return what I'm expecting.
/// I welcome suggestions for improvement.
/// </summary>
[TestMethod]
public void DateListTest2()
{
    //Arrange
    var mockRepository = new Mock<IDropDownDataRepository>();
    mockRepository
        .Setup(x => x.DateList())
        //This will only succeed if I have the Returns property here,
        //but isn't that just bypassing the actual "test" of whether or
        //not this works?
        .Returns(new HttpResponseMessage(HttpStatusCode.OK));

    DropDownDataController controller = new DropDownDataController(mockRepository.Object);
    controller.Request = new HttpRequestMessage();
    controller.Configuration = new HttpConfiguration();

    //Act
    var response = controller.DateList();

    //Assert
    Assert.AreEqual(HttpStatusCode.OK, response.StatusCode);
}

There are a number of issues in your code.

Naming. Methods should use verb phrases to denote the action they perform and try to be specific. So your DateList method should become something like GetWorkingDates, or just GetDates if kind of dates is obvious in your case.
Test naming. Do spend some time on composing a name for your unit test method that describes what exactly is being verified. DateListTest2 does not provide any information except that it deals with DateList() method somehow.
Incorrect responsibilities. Let's look at IDropDownDataRepository interface. It is an interface to obtain a list of dates (probably). And this functionality may be used in different places, not just to render this list in the HTML. But instead of returning IEnumerable<DateEntity> or any other type of collection, it returns HttpResponseMessage, which is only useful for HTTP communication. The only class that knows about specifics of how the list of dates should be used - is your DropDownDataController class. And in your case (no special HTTP response or different HTTP codes) - DropDownDataController.DateList method can return IEnumerable<DateEntity> as well.
Do use AsNoTracking() method when you perform read-only queries.
Test expected resulting behavior instead of the way it was done. If your class manages to satisfy all your tests without calling the mocked instance, then either result of the mocked instance call is insignificant, or you should improve tests by adding new test(s) to verify the change in behavior depending on outcome of the call to mocked instance. Your DateListTest1 test currently expose such a problem, as the only thing that it verifies is that controller calls IDropDownDataRepository.DateList() to render the output. Instead, you should setup IDropDownDataRepository.DateList() to return some list, and verify that the return value of the controller method returns the data in expected way. If the class under test does not call the mocked instance - it will fail such a verification anyway. In general the usage of Mock<T>.VerifyAll is not recommended.

As a result you may get something like this:

public interface IDropDownDataRepository
{
    IEnumerable<DateEntity> GetDates();
}

public class DropDownDataRepository : IDropDownDataRepository
{
    //Should be injected as well
    private MyDatabaseEntities db = new MyDatabaseEntities();

    public IEnumerable<DateEntity> GetDates()
    {
        return from date in db.MyProcedure().AsNoTracking()
               select date;
    }
}

public class DropDownDataController : ApiController
{
    private IDropDownDataRepository _dropDownDataRepository;

    public DropDownDataController(IDropDownDataRepository dropDownDataRepository)
    {
        _dropDownDataRepository = dropDownDataRepository;
    }

    [HttpGet]
    public IEnumerable<DateEntity> GetDates()
    {
        try
        {
            return _dropDownDataRepository.GetDates();
        }
        catch
        {
            // I would leave the default error handling which would return 500 error
            throw new HttpResponseException(HttpStatusCode.NotFound);
        }
    }
}

public class DropDownDataControllerTest
{
    [TestMethod]
    public void GetDatesShouldReturnDatesFromRepository()
    {
        //Arrange
        var mockRepository = new Mock<IDropDownDataRepository>();
        var dateEntities = new[] { new DateEntity { Date = new DateTime(1990, 1, 1) }, new DateEntity { Date = new DateTime(1990, 1, 1) } };
        mockRepository
            .Setup(x => x.GetDates())
            .Returns(dateEntities);

        DropDownDataController controller = new DropDownDataController(mockRepository.Object);

        //Act
        var response = controller.GetDates();

        //Assert
        Assert.AreEqual(dateEntities, response);
    }
}


Simple Prefix Matching Library in Haskell For some context, I'm trying to write a streaming tokenizer in Haskell and noticed that I was writing a lot of functions with the type signature String -> Maybe Int that attempted to consume "the longest possible token matching a particular pattern" from the input stream. The pattern might be something like a quoted string literal in JSON.

I tried using Parsec for a while to get a "prefix matcher", but I kept getting tripped up over what the appropriate userstate is supposed, and what the m argument in ParsecT means. Parsec is probably a strict superset of the functionality that this little library exposes.

A "Matcher" is anything with the type String -> Maybe Int. Matchers are supposed to be greedy and produce the longest prefix that they possibly can. There's also an unenforced property that, in cases where the match is a proper prefix of the input string, adding more characters to the end of a string doesn't extend the prefix. The library also encourages / permits users to write sloppy grammars (for instance using symOrExn) that fail in ugly ways.

I could use some helping figuring out how to generalize String -> Maybe Int. I don't know what class I should insist on for the input Eq a => [a] is the first thing that comes to mind, but it might be possible to make something more general than that. As for the output type maybe a type that's constrained to be a monoid or a group? I really just need a notion of zero and the ability to increment for the index. ... although it might also be possible to generalize the notion of a "prefix" to tree-like structures.

I'm hoping to structure this library eventually as a collection of really concrete implementations for a handful of commonly-used text-like types, as well as a generic implementation that can be used in other circumstances. Advice on how to generalize the library is much appreciated.

module Text.PrefixMatcher.String where
import Data.List

-- A simple non-generic prefix matching library
-- with a few simple combinators
--
-- a Matcher has type (String -> Maybe Int)
--
-- It takes a String and returns a number corresponding
-- to the length of the matched prefix.
--
-- The intended use case here is to provide a simple utility
-- to help in converting a string to a stream of tokens.
--
-- makeMatcher -- make a matcher for a constant string
--

type Matcher = String -> Maybe Int

identityMatcher :: Matcher
identityMatcher str = Just 0

makeMatcher :: String -> Matcher
makeMatcher spec str = case isPrefixOf spec str of
    True -> Just (length spec)
    False -> Nothing

andThen :: Matcher -> Matcher -> Matcher
andThen m1 m2 str = ltotal where
    l1 = m1 str
    ltotal = case l1 of
        Nothing -> Nothing
        (Just l1) ->
            let rest = drop l1 str in
            let l2 = m2 str in
            case l2 of
                Nothing -> Nothing
                (Just l2) -> Just (l1 + l2)

symOrExn :: Matcher -> Matcher -> Matcher
symOrExn m1 m2 str = res where
    l1 = m1 str
    l2 = m2 str
    res = case (l1, l2) of
        -- good cases
        (Just l1, Nothing) -> Just l1
        (Nothing, Just l2) -> Just l2
        -- bad cases
        (Nothing, Nothing) -> Nothing
        -- if you reach this case, the grammar was
        -- constructed badly
        (Just l1, Just l2) -> error "ambiguous match"

leftOr :: Matcher -> Matcher -> Matcher
leftOr m1 m2 str = result where
    l1 = m1 str
    l2 = m2 str

    result = case (l1, l2) of
        (Just l1, _) -> Just l1
        (Nothing, Just l2) -> Just l2
        (Nothing, Nothing) -> Nothing

andThenList :: [Matcher] -> Matcher
andThenList [] = identityMatcher
andThenList [x] = x
andThenList (x:xs) = andThen x (andThenList xs)

leftOrList :: [Matcher] -> Matcher
leftOrList [] = identityMatcher
leftOrList [x] = x
leftOrList (x:xs) = leftOr x (leftOrList xs)

Library functions, particularly Maybe's Applicative/Alternative instances, can make your code more consise:

makeMatcher spec str = length spec <$ guard (isPrefixOf spec str)

andThen m1 m2 str = do
  l1 <- m1 str
  l2 <- m2 $ drop l1 str
  Just $ l1 + l2

symOrExn m1 m2 str = case catMaybes [m1 str, m2 str] of
  [] -> Nothing
  [l] -> Just l
  -- if you reach this case, the grammar was
  -- constructed badly
  [_, _] -> error "ambiguous match"

leftOr = liftA2 (<|>)

andThenList = foldr andThen identityMatcher

leftOrList = foldr leftOr identityMatcher

A more potent approach is to return not the length at which to split, but the results of the split. In fact, StateT String Maybe String makes this work out of the box:

type Matcher = StateT String Maybe String

makeMatcher :: String -> Matcher
makeMatcher spec = StateT $ fmap (spec,) . stripPrefix spec

runMatcher :: Matcher -> String -> Maybe (String, String)
runMatcher = runStateT

andThen = liftA2 (++)

symOrExn m1 m2 = m1 <* (m2 *> error "ambiguous match" <|> pure ()) <|> m2
  -- I'll admit, this one looks clunky

leftOr = (<|>)

andThenList = fmap concat . sequenceA

leftOrList = asum


Pasting Values To/From a Calculation Model I have a Database worksheet (SoapUI - CollisionsSingle) that have around 1500 records and 38 columns of data per record. I have a VBA script that copies the records into a algorithm worksheet (COL_STpremcalc) and then copies the results back to the database worksheet. However it takes around 1-2 minutes to perform 1 record, and all but breaks my laptop to the point I can't do anything while it runs.

Sub SingleRating()
Dim i As Long
Dim iteration As Variant
Dim seleciton As Variant
Dim ws1 As Worksheet, ws2 As Worksheet
Set ws1 = Worksheets("SoapUI - CollisionsSingle")
Set ws2 = Worksheets("COL_STpremcalc")

Application.ScreenUpdating = False
Application.EnableEvents = False
Application.DisplayStatusBar = False

iteration = 0
iteration = InputBox("Please Select Row Iteration", "", "1")
seleciton = iteration + 3

    For i = 4 To seleciton

        ws2.Range("B3").Value = ws1.Range("D" & i).Value

        ws2.Range("B4").Value = ws1.Range("E" & i).Value

        ws2.Range("B5").Value = ws1.Range("F" & i).Value

        ws2.Range("B6").Value = ws1.Range("G" & i).Value

        ws2.Range("E3").Value = ws1.Range("H" & i).Value

        ws2.Range("E4").Value = ws1.Range("I" & i).Value

        ws2.Range("E5").Value = ws1.Range("J" & i).Value

        ws2.Range("E6").Value = ws1.Range("K" & i).Value

        ws2.Range("G3").Value = ws1.Range("L" & i).Value

        ws2.Range("G4").Value = ws1.Range("M" & i).Value

        ws2.Range("G5").Value = ws1.Range("N" & i).Value

        ws2.Range("J3").Value = ws1.Range("O" & i).Value

        ws2.Range("J6").Value = ws1.Range("P" & i).Value

        ws2.Range("C9:E9").Value = ws1.Range("Q" & i, "S" & i).Value

        ws2.Range("C10:E10").Value = ws1.Range("T" & i, "V" & i).Value

        ws2.Range("C11:E11").Value = ws1.Range("W" & i, "Y" & i).Value

        ws2.Range("C12:E12").Value = ws1.Range("Z" & i, "AB" & i).Value

        ws2.Range("C13:E13").Value = ws1.Range("AC" & i, "AE" & i).Value

        ws2.Range("C14:E14").Value = ws1.Range("AF" & i, "AH" & i).Value

        ws2.Range("C15:E15").Value = ws1.Range("AI" & i, "AK" & i).Value

        ws2.Range("C16:E16").Value = ws1.Range("AL" & i, "AN" & i).Value

        '''''''''''''''''''''''''''''''''''''''''''''''''

        ws1.Range("AO" & i).Value = ws2.Range("M4").Value

        Application.StatusBar = "Current iteration: " & (i - 1) & "/" & iteration
    Next i

Application.ScreenUpdating = True
Application.EnableEvents = True
Application.DisplayStatusBar = True

End Sub

The reason I don't just do the calculation in the database worksheet is that there is about 100+ calculations that happen before the final results and the calculations are complex.

The calculation happens almost instantly, it is the copying of the data back and forth that I believe is slowing the whole thing down.

Is there a way to make this more efficient to the point it would only take around 20-30 minutes?

Excel is not a database

I cannot stress this strongly enough. Excel has many excellent uses, but please don't ever get in the habit of thinking that it is a viable alternative to a proper Database.

Never assume that you know where the performance bottleneck is

If you could accurately forecast where a performance bottleneck would be, then you wouldn't have an unbearably slow Macro in the first place.

Rule #1 of performance increases is to benchmark everything, because it will never be where you thought it would be.

Poor man's VBA benchmarking (only accurate to the nearest 1/100th of a second):

Public Sub SplitTimer(ByVal printMessage As String, ByRef oldTimer As Double)
    '/ prints message and time diff between current timer and previous timer
    '/ then updates previous timer with current timer

    Dim currentTimer As Double
    currentTimer = Timer()

    Debug.Print printMessage & " - " & (currentTimer - oldTimer) & " s"

    oldTimer = currentTimer

End Sub

Then, in your Macros:

    Dim timerStore As Double
    timerStore = Timer()

    ws2.Range("B3").Value = ws1.Range("D" & i).Value
    SplitTimer "B3", timerStore

    ws2.Range("B4").Value = ws1.Range("E" & i).Value
    SplitTimer "B4", timerStore

    ws2.Range("B5").Value = ws1.Range("F" & i).Value
    SplitTimer "B5", timerStore

    ws2.Range("B6").Value = ws1.Range("G" & i).Value
    SplitTimer "B6", timerStore

And so on. You'll then get a list of how long each section of your code took to execute. In general, there will be one section that's responsible for ~98% of the runtime:

'/ (hypothetical output)
B3 - 1.40625 s
B4 - 0 s
B5 - 0 s
B6 - 0.041796875 s

Take that operation, split it into smaller pieces, benchmark. Repeat until you've found the source of your slow runtime. Then you can experiment yourself, or take that very specific opeation to SO for performance ideas.

In your specific case, I have a hunch.

Copy/Pasting actually doesn't take a lot of time (per operation). 20 operations in 2 minutes, I'd expect it to be less than 1% of runtime. What takes time is all the Events that trigger when you change a worksheet.

In particular, every calculation is going to re-calculate every time you paste a new value in.

Try this:

Application.Calculation = xlCalculationManual

For i = 4 To selection

    '/ Paste operations

    '/ Paste operations

    '/ Paste operations

    Application.Calculate '/ we only calculate once per iteration, when everything's set up and ready.

    ws1.Range("AO" & i).Value = ws2.Range("M4").Value

Next i

And see if it makes a noticeable difference to your performance.


Assert LinkedHashMap element in jUnit test So I have some test, maybe the given side of that test is not so important to catch the point of my confusion. I wrote

List<LinkedHashMap> failedEvents = ...
//...
assertEquals(failedEvents.size(),1 );
assertTrue(failedEvents.stream().allMatch(x -> x.get("eventId").equals("EVENT3")));

and in the result my code review said I should avoid stream on single element. But I think is clear and readable to show that all elements pass specific matching rule.
Also I though about assertThat, but I ended with much bigger and more complicated piece of code.

One cannot say anything against

assertEquals(failedEvents.size(), 1);
assertEquals(failedEvents.get(0).get("eventId"), "EVENT3");

It is even shorter and more clear.

However your argument of an invariant on the entire collection is valid, though
you should have said so. Better would be to make a reusable assertion:

assertEquals(failedEvents.size(), 1);
assertTrue(failedEventsHaveId(failedEvents, "EVENT3"));

/**
 * Invariant on the failedEvent: the eventId remaining one.
 */
private static boolean failedEventsHaveId(..., String eventId) {
    return ...;
}

If not reusable, then the code is unnecessarily "nice."


Two threads working on shared data I have two threads, called Fun and Boring.  Each thread takes the same read-only data and runs some calculations over it and returns a [0.0, 1.0] result.

In the current setup, Fun acts as the main thread.  It gathers, prepares, and stores the data in a global variable; a memory barrier is used.  It then signals (via an event) the Boring thread that there is data to work on.  Fun and Boring now both operate on the data.

Here is code which illustrates the set up:

struct Data
{
    void*   someData;
} g_data;

struct Results
{
    float fun;
    float boring;
} g_results;

Fun::Run()
{
    g_data = somePointer;   // Prepare data
    // *memory barrier*

    SetEvent(dataIsPrepared);
        g_results.fun = FunCalculate(g_data);
    WaitForSingleObject(boringComplete);

    // Do something with g_results
}

Boring::Run()
{
    WaitForSingleObject(dataIsPrepared);
        g_results.boring = BoringCalculate(g_data);
        // *memory barrier*
    SetEvent(boringComplete);
}

Is there a "better" way to share data and results?  Are there any red flags that stand out?

Thanks for any feedback.

This is correct code. It could go into production.

You don't need the memory barrier because SetEvent will do that for you (if it didn't a lot of code would be broken).

You could change this to use futures (provided that you have some C++ lib that supports them. I don't really know C++ well):

var data = PrepareData();
var boringComputation = StartSomeFuture([&data] { ... });
var funComputation = ComputeFun(data);
var boringResult = boringComputation.ResultAndWait();

Much better.


Alphabetic Pattern Pyramid I want to know if I can improve this program. Its goal is to print a pattern in a pyramid form, by first receiving a letter. The first row is always 'A', only. Then 'ABA', then 'ABCBA'. Increasing order, then decreasing.

#include <stdio.h>

int main(void)
{
    char alphabet[26] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    char letter;

    printf("Enter an uppercase letter: ");
    scanf("%c", &letter);

    letter -= 65;

    for (int row = 0; row <= letter; row++)
    {
        for (int spaces = letter; spaces > row; spaces--)
            printf(" ");

        for (char letter2 = 0; letter2 <= row; letter2++)
            printf("%c", alphabet[letter2]);

        for (char letter3 = 0 + row; --letter3 >= 0; )
            printf("%c", alphabet[letter3]);

        printf("\n");
    }
}

Instead of having a static array of chars You could just use ASCII values from 0x41 to 0x5A. If You still insist having a static array, please consider marking it as a const.

Leaving Your int main() without any return value --- sign of the bad taste, to say the least. Consider having some proper error codes or at least return 0 on proper exit.

Your index of chars (letter2, letter3) can't be negative, so why would You use int instead of uint_* or size_t? Same applies to row and spaces variables.

In case of single char, putchar() is way faster then printf("%c", character).


Synchronized Queue Wrapper C++11 I am trying to write a SynchronizedQueue wrapper class to provide a simple synchronized interface to underlying standard std::queue.
Could you have a review and let me know if any pitfalls and improvements you see ?
Too many mutexes/locks ? I wanted to to use condition variables to wait rather than mutexes for push & pop notifications.
Any insights is highly appreciated as I have rarely written multi-threaded programs.

#pragma once

#include <mutex>
#include <condition_variable>
#include <queue>
#include <cstdint>

template<typename T>
class SynchronizedQueue
{
public:
  SynchronizedQueue(size_t maxItems)
    : synchronizedQueue_()
    , queueMutex_()
    , pushMutex_()
    , popMutex_()
    , pushCV_()
    , popCV_()
    , MAX_ITEMS(maxItems)
  {
  }

  void push(const T& item)
  {
    std::unique_lock<std::mutex> scopedLock(pushMutex_);
    while (isFull())
    {
      std::unique_lock<std::mutex> pushLock(queueMutex_);
      popCV_.wait(pushLock);
    }

    {
      std::unique_lock<std::mutex> scopedLock(queueMutex_);
      synchronizedQueue_.push(item);
      pushCV_.notify_one();
    }
  }

  T pop()
  {
    std::unique_lock<std::mutex> scopedLock(popMutex_);
    while (isEmpty())
    {
      std::unique_lock<std::mutex> scopedLock(queueMutex_);
      pushCV_.wait(scopedLock);
    }

    T t;
    {
      std::unique_lock<std::mutex> scopedLock(queueMutex_);
      t = synchronizedQueue_.front();
      synchronizedQueue_.pop();
      popCV_.notify_one();
    }

    return t;
  }

private:
  bool isEmpty()
  {
    std::unique_lock<std::mutex> scopedLock(queueMutex_);
    return synchronizedQueue_.empty();
  }

  bool isFull()
  {
    std::unique_lock<std::mutex> scopedLock(queueMutex_);
    return synchronizedQueue_.size() == MAX_ITEMS;
  }

  std::queue<T> synchronizedQueue_;
  std::mutex queueMutex_, pushMutex_, popMutex_;
  std::condition_variable pushCV_, popCV_;
  size_t MAX_ITEMS;
};

Synchronization

pushMutex_ and popMutex_ aren't needed. Even worse, they prevent other threads from waiting on a condition variable! Just lock queueMutex_ instead (that also prevents having to re-lock queueMutex_).

Assume that the queue is full, and thread 1 and 2 both want to push an item into the queue. Thread 1 was faster and got the lock on pushMutex_, so it proceeds to wait on pushCV_. Thread 2 in the meanwhile spins trying to lock pushMutex_ - which is still held by thread 1!

Avoid using std::condition_variable::notifiy_one() while still holding the corresponding lock! The notified thread will wake up, and the first action it has to do is reacquiring the lock that is still hold by the caller of notify_one(), so it will instantly block again (this time on acquiring the lock though, not wait()).

Implementation

The outer scopedLock in push() and pop() get shadowed by the inner scopedLock. This can lead to extra confusion in an already complex code!

Since MAX_SIZE isn't supposed to change once the object is initialized, consider marking it as const. (It can still be set with the initialization list of the constructor, but not changed afterwards.)

Further considerations

Maybe add fast returning bool try_push(const T&) and bool try_pop(T&) member functions, so users of the queue don't have to wait if they don't need to?


Merge two sorted lists There are many if-else conditions in my code
Question 1:
I am looking for suggestions to minimize if else conditions. Also, Code in Step 1 and Step2 is almost same. Is there any way to make it neat and improve readability
Question 2:
I am looking for suggestions to improve main method and which methods can be called with an instance or class variables. That technique I will apply to all my LinkedList related questions

public class MergeTwoSortedLists {

    public Node head;
    static class Node {
        int data;
        Node next;

        Node(int d) {
            data = d;
            next = null;
        }
    }

    public void push(int data) {
        Node n = new Node(data);
        if ( head == null ) {
            head = n;
        } else {
            Node temp = head;
            while (temp.next != null ) {
                temp = temp.next;
            }
            temp.next = n;
        }
    }

    public static void printList(Node head) {
        Node temp = head;
        while ( temp != null ) {
            System.out.print(temp.data + " ");
            temp = temp.next;
        }
        System.out.println();
    }

    public static void main(String[] args) {
        MergeTwoSortedLists l1 = new MergeTwoSortedLists();
        l1.push(1);
        l1.push(2);
        l1.push(13);
        l1.push(14);
        l1.push(50);
        printList(l1.head);

        MergeTwoSortedLists l2 = new MergeTwoSortedLists();
        l2.push(10);
        l2.push(20);
        l2.push(55);
        printList(l2.head);

        Node res = sortTwoLists(l1.head, l2.head);
        printList(res);

    }

    public static Node sortTwoLists(Node headA, Node headB) {
        Node s = null;
        Node res = null;
        if ( headA == null ) return headB;
        if ( headB == null ) return headA;

        /* Step 1: Set the s with less value */
        if ( headA!= null && headB!= null ) {
            if ( headA.data < headB.data ) {
                s = headA;
                headA = s.next;
            } else {
                s = headB;
                headB = s.next;
            }
        }
        res = s;

        /* Step2: Iterate oer two lists and keep moving s accordingly */
        while ( headA!= null && headB!= null ) {
            if ( headA.data < headB.data ) {
                s.next = headA;
                s = headA;
                headA = s.next;
            } else {
                s.next = headB;
                s = headB;
                headB = s.next;
            }
        }
        if ( headA == null ) {
            s.next = headB;
        }
        if ( headB == null ) {
            s.next = headA;
        }

        return res;

    }
}

General comments:

Your push method takes time proportional to the length of the list. Usually I would expect this method to take constant time. How can you change your data structure to achieve this?

Your push method could also check (in constant time) that the sorted order is preserved, and throw an exception if the client tries to push an illegal int.

Probably your class ought to be called SortedList, since this is the object you’re implementing. Then your sortTwoLists() (which could be called mergeTwoLists()) would be an instance method taking a SortedList object as a parameter. Note that both SortedLists involved are changed by this method.

printList() would probably be better as an instance method (in which case, it needn’t take any parameters).

Question 1:
Here’s one possible approach. You could build an auxiliary MergingNode class, with instance variables:

Node mergedNode; // a node in the merged list
Node thisNode; // current node in the first list you’re merging
Node thatNode; // current node in the second list you’re merging

This class could have methods getMergedNode(), which just returns mergedNode, and nextMergeStep(), which replaces mergedNode by the next node in the merged list and updates either thisNode or thatNode, depending on which list we took a node from. Note that nextMergeStep() modifies the original SortedLists.

Then, Step 1 can be replaced by

SortedList merged = new SortedList();
MergingNode mergingNode = new MergingNode(null, headA, headB);
mergingNode.nextMergeStep();
merged.head = mergingNode.getMergedNode();

Step 2 can be replaced by

Node curNode = merged.head;
while (curNode != null) {
  mergingNode.nextMergeStep();
  Node nextNode = mergingNode.getMergedNode();
  curNode.next = nextNode;
  curNode = nextNode;
}

This way the nextMergeStep() method does all the work of your if statements.

Question 2:
To clean up your main method, you could, for example, write a constructor for SortedList that takes an array of integers as a parameter, and constructs the SortedList containing those integers (meanwhile checking that the array was sorted).


Jolly jumper sequence Another challenge from CodeEval that's causing me great pain...

Trying to find out if a sequence if a Jolly Jumper, which is

  A sequence of n > 0 integers is called a jolly jumper if the absolute values of the difference between successive elements take on all the values 1 through n-1.

I ended up adapting this idea into this:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

public class Main {

    public static void main(String[] args) throws IOException {
        //File file = new File(args[0]);
        File file = new File("test.txt");
        BufferedReader buffer = new BufferedReader(new FileReader(file));
        String line;
        while ((line = buffer.readLine()) != null) {
            line = line.trim();
            String[] numbersString = line.split(" ");
            int[] numbers = new int[numbersString.length];
            for (int i = 0; i < numbers.length; i++){
                numbers[i] = Integer.parseInt(numbersString[i]);
            }
            System.out.println(isJolly(numbers));
        }
        System.exit(0);
    }

    public static String isJolly(int[] array) {
        List listResults = new ArrayList();
        for (int i = 0; i < array.length - 1; i++) {
            int result = Math.abs(array[i] - array[i + 1]);
            if (result == 0) {
                return "Not jolly";
            }
            if (result >= array.length) {
                return "Not jolly";
            }
            if (listResults.contains(result)) {
                return "Not jolly";
            }
            listResults.add(result);
        }
        return "Jolly";
    }
}

I tested it with a few examples locally and it worked, but when I submit the code I don't get any sequence to appear as jolly. What mistakes am I making?

As a simple comment, a function called isJolly should probably return a boolean. Then, the calling function can use that boolean to print either "Jolly" or "Not jolly".


Shell script to create a boilerplate proyect This is actually my first shell script and just by looking at it I know this can look better. I use Elixir and Phoenix for most of my projects and I started using Zurb Foundation 6, changing from Bootstrap to Foundation is kinda tiresome sometimes so I decided to make a script to automate this process.

This is the script

#!/bin/sh

JAVI_PATH="$HOME/.javi"

name=${1}
path=${2}
rest=${@:3}

### Funcs
get_files() {
  cd ${name}/assets

  echo "Installing extra dependencies"
  npm install --save jquery sass-brunch foundation-sites normalize-scss

  echo "Getting files to replace"

  get_brunch
  get_scss

  cd ../..

  echo "Got and replaced files on 'assets' folder"

  # Replaces content from 'app.html.eex' file
  get_apphtml
}

get_brunch() {
  rm -rf brunch-config.js && \
  wget https://raw.githubusercontent.com/aguxez/javi/master/brunch-config.js
}

get_scss() {
  cd css && \
  rm -rf app.css phoenix.css && \
  wget https://raw.githubusercontent.com/aguxez/javi/master/app.scss
}

get_apphtml() {
  cd lib/${name}_web/templates/layout && \
  rm -rf app.html.eex && \
  wget https://raw.githubusercontent.com/aguxez/javi/master/app.html.eex

  # Here we replace the title with the 'name' variable
  uppercased_name=`sed -e "s/\b\(.\)/\u\1/g" <<< "${name}"`
  eval sed -i -e 's/CHANGE_TITLE/${uppercased_name}/g' app.html.eex
}

## If installing
INSTALL=0
if [ "$1" = "install" ] ; then
  INSTALL=1
fi

if [ "$INSTALL" = 1 ] ; then
  if [ ! -d "$JAVI_PATH" ] ; then
    mkdir $JAVI_PATH && \
    curl -sSL https://raw.githubusercontent.com/aguxez/javi/master/javi > $JAVI_PATH/javi && \
    chmod +x $JAVI_PATH/javi

    echo "Javi has been configured"
  fi
elif [ -z "${name}" ]; then
  echo "Please give a name to your project"
elif [[ -d ${path} ]]; then
  echo "Creating project in ${path}"
  cd ${path}
  mix phx.new ${name} ${rest}
  # Runs function
  get_files
else
  mix phx.new ${name} ${rest}
  # Function
  get_files
fi

Maybe I could think of a better way of handling arguments here

if [ "$INSTALL" = 1 ] ; then
  if [ ! -d "$JAVI_PATH" ] ; then
    mkdir $JAVI_PATH && \
    curl -sSL https://raw.githubusercontent.com/aguxez/javi/master/javi > $JAVI_PATH/javi && \
      chmod +x $JAVI_PATH/javi

    echo "Javi has been configured"
  fi
elif [ -z "${name}" ]; then
  echo "Please give a name to your project"
elif [[ -d ${path} ]]; then
  echo "Creating project in ${path}"
  cd ${path}
  mix phx.new ${name} ${rest}
  # Runs function
  get_files
else
  mix phx.new ${name} ${rest}
  # Function
  get_files
fi

But I'm not sure what I can change or HOW can I change it.

I'm not a shell expert, but I think I could help a bit. The most relevant tips I could give are:

1. Quote your expansions

Unquoted expansions are the most common source of bugs and security issues in shell scripts. There are cases where it's safe to leave expansions unquoted (as in foo=$bar) but, unless extremely necessary, it's preferable to double-quote every expansion you have.

2. Prefer $(command) over `command`

The $(command) syntax is POSIX-compliant and it can be easily nested.

3. Be consistent

You mix different styles in your code.

Quotes: There are both quoted and unquoted expansions. As I said before, prefer quoted expansions unless you are explicitly looking for side effects like word splitting.
Braces: When doing parameter expansions, sometimes you use braces, sometimes not. Braces are optional in most of the simple substitutions but they are useful when you have two-digit positional parameters or to avoid ambiguity. I personally prefer to always put my parameters within braces.
[ and [[: If portability is a concern, use [. If not, use [[.

4. Be careful with eval

Avoid it if you are not completely sure of what you are doing.

5. Arrays

I suppose you use the rest variable to save your remainder arguments. The problem is that you are saving them as a single string and you are relying on word splitting to handle it.

If your shell support arrays, this would be a safer and cleaner solution:

rest=( "${@:3}" )
...
mix phx.new "${name}" "${rest[@]}"

6. Shell builtins

If available, prefer them over external commands.

For example, if you use a recent version of bash and portability is not a big deal, you could rewrite uppercased_name=`sed -e "s/\b\(.\)/\u\1/g" <<< "${name}"`  as uppercased_name="${name^}"

7. Arguments parsing

First, I recommend you to read the Utility Conventions chapter of the POSIX specification. It's useful to understand how argument syntax works in standard utilities and serves as a guide to the rules you should follow when you create some program.

The goal is to make sure your script follows the program [options] [operands] syntax. I mean, instead of rely on the position of arguments to assign variables (as in name=${1}), your script should parse --name foo (or something similar) to change the value of the name variable.

There are basically two approaches:

Using utilities like getopts

Pros:

getopts is a POSIX builtin command.
It can handle things like -abc without effort.
Easy and intuitive.

Cons:

Can't handle long options.

Writing your own parser

Pros:

Full control of what and how it is parsed.
Handling of long options.
Could implement non-standard syntax.

Cons:

It's harder to handle things like -abc and edge cases.

Greg's and Bash Hackers' (1, 2) wikis have tutorials for both approaches, so check them out.

I've also written an example script to show how to do manual parsing and colored output:


Task Execution with Cancel Logic In my assignment I am executing a long running NodeJs script from C# using EdgeJs. Since the script may take a long time to execute, I also want to provide user an option to Cancel the script execution in the middle. The cancellation status is maintained in the database and function IsScripteExecutionCancelled returns true if the user decides to cancel the execution.

To achieve this I am executing script in a background thread using Task.Run(). The main thread contains a while loop that runs until the ScriptExecution is over or is cancelled. I can solve this problem using native System.Threading.Thread, but here I am trying to do it using TPL. Here is the code I have put in place:

bool executionStarted = false;
bool taskCompleted = false;
Result returnValue;

CancellationTokenSource tokenSource = new CancellationTokenSource();
CancellationToken token = tokenSource.Token;

Task executionTask;
while (!(taskCompleted || token.IsCancellationRequested))
{
    if (!executionStarted)
    {
        executionStarted = true;
        executionTask = Task.Run(() =>
        {
            returnValue = ExecuteScript();
            taskCompleted = true;
        }, token);
    }
    bool taskExecutionCancelled = IsScripteExecutionCancelled();
    if (taskExecutionCancelled)
    {
        tokenSource.Cancel();
    }
}

if(token.IsCancellationRequested)
    returnValue= GetDefaultValueForCancellation();

return returnValue;

So, if IsScripteExecutionCancelled returns true the program will come out of the loop and will check if the script was cancelled, in which case I will return a default response. If the ExecuteScript completes my program will return the value returned by function ExecuteScript. I am able to get the desired results using this implementation however I would like to get it validated.

Question: Is it the right way to address this problem?

Is there a better way of doing Task cancellation than the one I implemented

There's no task cancellation implemented here. By passing a token to Task.Run you just make the returned task IsCanceled equal to true in case it cancels itself (without, it gets IsFaulted by the OperationCanceledException). But you need (at least) to pass the token to DoSomeWork, because that has to check the token and cancel itself.

Task cancellation works through cooperation of the to-be-canceled task and has nothing to do with Thread.Abort.


MemoryError in Python while combining multiple JSON files and outputting as single CSV I have a number of JSON files to combine and output as a single CSV (to load into R), with each JSON file at about 1.5GB.  While doing a trial on 4-5 JSON files at 250MB each, the code works when I only use 2-3 files but chokes when the total file sizes get larger.

I'm running Python version 2.7.6 (default, Nov 10 2013, 19:24:24) [MSC v.1500 64 bit (AMD64)] on 8GB RAM and Windows 7 Professional 64 bit.

I'm a Python novice and have little experience with writing optimized code.  I would appreciate guidance on optimizing my script below.

Python MemoryError

Traceback (most recent call last):
  File "C:\Users\...\tweetjson_to_csv.py", line 52, in <module>
    for line in file:
MemoryError
[Finished in 29.5s]

JSON to CSV conversion script

# csv file that you want to save to
out = open("output.csv", "ab")

filenames = ["8may.json", "9may.json", "10may.json", "11may.json", "12may.json"]
open_files = map(open, filenames)

# change argument to the file you want to open
for file in open_files:
    for line in file:
        # only keep tweets and not the empty lines
        if line.rstrip():
            try:
                tweets.append(json.loads(line))
            except:
                pass

for tweet in tweets:
    ids.append(tweet["id_str"])
    texts.append(tweet["text"])
    time_created.append(tweet["created_at"])
    retweet_counts.append(tweet["retweet_count"])
... ...

print >> out, "ids,text,time_created,retweet_counts,in_reply_to,geos,coordinates,places,country,language,screen_name,followers,friends,statuses,locations"
rows = zip(ids,texts,time_created,retweet_counts,in_reply_to_screen_name,geos,coordinates,places,places_country,lang,user_screen_names,user_followers_count,user_friends_count,user_statuses_count,user_locations)

csv = writer(out)

for row in rows:
    values = [(value.encode('utf8') if hasattr(value, 'encode') else value) for value in row]
    csv.writerow(values)

out.close()

You're storing lots of results in lists, which could be streamed instead. Fortunately, using generators, you can make this ‘streaming’ relatively easy without changing too much of the structure of your code. Essentially, put each ‘step’ into a function, and then rather than appending to a list, yield each value. Then you’d have something that might look like this:

def load_json():
    for file in open_files:
        for line in file:
            # only keep tweets and not the empty lines
            if line.rstrip():
                try:
                    datum = json.loads(line)
                except:
                    pass
                else:
                    yield datum

Then you could replace

for tweet in tweets:

with

for tweet in load_json():

But this will still store the IDs into ids, texts into texts, etc. You could use a bunch of generators and zip them together with itertools.izip, but the better solution would be to extract the columns from the tweet when writing each line. Then (omitting the UTF-8 encoding piece, which you'd want to rewrite to work on dictionaries) you’d have

for tweet in load_json():
    csv.writerow((tweet['id'], tweet['text'], ...))

Lastly, since this is Code Review, you might consider putting all the keys you want to pull out into a list:

columns = ['id', 'text', ...]

Then your row-writing code can be simplified to

csv.writerow([tweet[key] for key in columns])

Rewriting it this way, you can then UTF-8 encode rather easily (using a helper function):

def encode_if_possible(value, codec):
    if hasattr(value, 'encode'):
        return value.encode(codec)
    else:
        return value

csv.writerow([encode_if_possible(tweet[key], 'utf-8') for key in columns])


Extended Euclidean algorithm I have programmed the extended euclidean algorithm. Is this a good approach?

def ext_ggT(a, b):
    x,y, u,v = 0,1, 1,0
    while a != 0:
        q, r = b//a, b%a
        m, n = x-u*q, y-v*q
        b,a, x,y, u,v = a,r, u,v, m,n
    gcd = b
    return gcd, x, y

Not being familiar with the algorithm in question, it was very non-obvious to me what the code did, and in general I think it's good to write code in such a way that it's comprehensible with minimal reasonable effort even if the reader isn't a domain expert (i.e. they should be able to look up documentation, look at your code, and see how the two relate).  The main hindrances I faced were:

It wasn't immediately obvious that x, y, u, v represented two distinct series (where u was always a prior value of x, etc).  In general there are a lot of variables in this code to keep track of, and not a lot of explanation of what they're all for.
Dependencies between the different values likewise were not obvious.  Having all the assignments smushed onto one line made it hard to discern this visually; it's nice to use compact tuple assignments when the relationships are obvious, but it doesn't necessarily always improve readability.
There're no doc/comments explaining what's going on.
The name ext_ggT doesn't follow Python's snake_case naming convention and is a bit cryptic.

After reading the wiki link (thx Peilon) I was able to sort of reverse-engineer it and then I made some changes so that the code matches up more with my understanding based on the wiki article (and is commented so that anyone looking at this code side by side with the wiki article will immediately see what goes with what).

from collections import deque
from typing import Tuple

def extended_euclidean(a: int, b: int) -> Tuple[int, int, int]:
    """
    Returns (gcd, x, y) such that:
       gcd  = greatest common divisor of (a, b)
       x, y = coefficients such that ax + by = gcd
    """
    # We only need to keep the last two elements of each series.
    r = deque([b, a], 2)
    s = deque([0, 1], 2)
    t = deque([1, 0], 2)

    # The next element of each series is a function of the previous two.
    # We stop building these series once r (the remainder) is zero;
    # the final result comes from the iteration prior to that one.
    while r[-1] != 0:
        q = r[-2] // r[-1]
        r.append(r[-2] % r[-1])
        s.append(s[-2] - s[-1] * q)
        t.append(t[-2] - t[-1] * q)

    return r[-2], s[-2], t[-2]

assert extended_euclidean(240, 46) == (2, -9, 47)

The biggest change is that I've represented the various series described in the wiki article as iterables, rather than representing each as two scalars; this doesn't make much difference to the way the code actually runs, but the fact that these six values (previously a, b, x, y, u, and v) represent three distinct series is now very obvious to the reader.  The three series are initialized and extended in such a way as to "make alike look alike" -- you can see at a glance how each successive element is computed from the prior two, and easily discern where there are and aren't dependencies between these values.

You could initialize these series as simply:

r = [b, a]
s = [0, 1]
t = [1, 0]

and the code would return the correct result, but to preserve the behavior of only keeping the last two elements (which I agree is a good space optimization) I've converted them to deques with maxlen=2.  The deque abstracts away the business of automatically popping the unneeded values off of the left side, which helps declutter the "interesting" part of the implementation while still preserving the property of having it use constant space.


Check if user selects correct Spanish word article All Spanish nouns have a gender (masculine [el] or feminine [la]; e.g. la casa, el perro). I'm using some javascript and jquery to display a random word from a list of nouns (2D array), as well as buttons corresponding to the two Spanish articles ("el" or "la"). If the user clicks on the correct article for the word, a new one loads; else, the user sees an alert and is prompted to choose the correct article before loading a new word. I made this work on codepen, but I wasn't able to get it to work correctly without reloading the page. All feedback will be well received.

$(document).ready(function() {
  setWord();

  function setWord() {

    //returns either 0 or 1
    var elOrLa = Math.round(Math.random());

    //wordList[0] contains 'la' nouns; wordList[1] contains those 'el' nouns
    var wordList = [["casa", "comida", "chica"],["perro", "amigo", "muchacho"]];

    // Return random int between min (included) and max (excluded)
    function getRandomInt(min, max) {
      min = Math.ceil(min);
      max = Math.floor(max);
      return Math.floor(Math.random() * (max - min)) + min;
    }

    //maximum length of each subarray
    var laLength = wordList[0].length;
    var elLength = wordList[1].length;

    //random index for subarrays
    var randomLaIndex = getRandomInt(0, laLength);
    var randomElIndex = getRandomInt(0, elLength);

    var article1 = "la ";
    var article2 = "el ";
    var word;
    var articlePlusWord;

    //sets random word and articlePlusWord
    if (elOrLa === 0) {
      word = wordList[0][randomLaIndex];
      articlePlusWord = article1.concat(word);
    } else {
      word = wordList[1][randomElIndex];
      articlePlusWord = article2.concat(word);
    }

    $("#test-word").text(word);

    if (elOrLa === 0) {
      $("#la-btn").on("click", function() {
        location.reload(true);
      });
      $("#el-btn").on("click", function() {
        alert("TOMA NOTA: ".concat(articlePlusWord));
        $("#el-btn").fadeOut();
        $("#la-btn").css({"background":"limegreen"});
        $("#test-word").addClass('bg-danger').removeClass('bg-warning');
      });
    } else {
      $("#el-btn").on("click", function() {
        location.reload(true);
      });
      $("#la-btn").on("click", function() {
        alert("TOMA NOTA: ".concat(articlePlusWord));
        $("#la-btn").fadeOut();
        $("#el-btn").css({"background":"limegreen"});
        $("#test-word").addClass('bg-danger').removeClass('bg-warning');
      });
    }
  };
});

  <button type="button" class="btn btn-primary btn-lg" id="el-btn">EL</button>
  <button type="button" class="btn btn-primary pull-right btn-lg" id="la-btn">LA</button>

Message for moderators

As the OP already noticed, there is a strange issue when executing this in the context of the SO snippet:

  uncaught exception: unknown (can't convert to string)

I experienced the same with my totally refactored version, while it works without error in the usual context of my browser.

I can't see a good way to inquire about that, so TIA if one of the SO developers could look at it and explain it!

Now the review

This is properly written, and I see no big flaws.
But the code might be significantly reduced, by merely avoiding useless parts and repetition.

Useless parts consist of preparing data for both "la" and "el" cases, while each execution will only use one of those two articles.
So I suggest to prepare data for the involved one only, as soon as it was chosen.

Repetition appears in the click events handling, where you precisely define what must happen for each button, and in both cases depending it's the right one or not.
There too, we can compact this by acting for the two buttons at once, and evaluating if it's right during the flow.

Here is a working snippet based on yours, where I included these suggestions:

function setWord() {

    // Return random int between min (included) and max (excluded)
    function getRandomInt(min, max) {
        min = Math.ceil(min);
        max = Math.floor(max);
        return Math.floor(Math.random() * (max - min)) + min;
    }

    const words = [
        ["casa", "comida", "chica"],
        ["perro", "amigo", "muchacho"]
    ];
    const articles = ['la', 'el'];

    var $buttons = $('.btn');
    var elOrLa = Math.round(Math.random());
    var article = articles[elOrLa];
    var $goodBtn = $('#' + article + '-btn');

    var $word = $('#test-word');
    var word = words[elOrLa][getRandomInt(0, words[elOrLa].length)];
    $word.text(word);

    $buttons.click(function() {
        if (this.id == $goodBtn[0].id) {
            location.reload(true);
        } else {
            alert('TOMA NOTA: ' + article + ' ' + word);
            $buttons.not($goodBtn).fadeOut();
            $goodBtn.css({'background': 'limegreen'});
            $word.addClass('bg-danger').removeClass('bg-warning');
        }
    });
}

setWord();

  <button type="button" class="btn btn-primary btn-lg" id="el-btn">EL</button>
  <button type="button" class="btn btn-primary pull-right btn-lg" id="la-btn">LA</button>

Apart from the changes exposed above, you can see that:

I dropped the $(document).ready() wrapper: in most cases (and sure here) it's not needed, as soon as the script appears after the HTML description part
I also moved the getRandomInt() function, which is now the 1st thing encountered in the setWord() function: this is a good habit to take now, since it becomes mandatory if you're using "use strict"; (see this page)


Checking if two strings are anagrams Here is my code for checking to see if two strings are anagrams. This seems a little short. Am I missing anything? Both time and space complexity seem to be \$O(1)\$.

//Determine if a string is an anagram
//Time complexity: O(1)
//Space complexity: O

#include <iostream>
#include <string>
using namespace std;

bool anagram(string one, string two)
{

    if (one.length() != two.length())
    {
        return false;
    }

    //sorting the strings
    sort(one.begin(), one.end());
    sort(two.begin(), two.end());

    return (one == two);

}

int main() {

    string one;
    string two;

    cout << "please enter in a string: ";
    cin >> one;
    cout << "please enter in another string: ";
    cin >> two;

    cout << anagram(one, two) <<endl;

    return 0;

}

Dos and donts

Don't use using namespace std. It's fine for very small programs, but std:: immediately tells someone that you're using a standard function and not some self-written sort.

Use all #include's necessary. std::sort is defined in <algorithm>. It seems like your C++ distribution includes <algorithm> or stl_algo in one of the other headers. That's not portable.

Use proper names. anagram doesn't tell anything about the function. Does it create an anagram? Does it check whether something is an anagram of something else? are_anagrams or something similar is less ambiguous.

The issue of \$\mathcal O(1) \$

You cannot get \$\mathcal O(1)\$ for this. You have to view each letter in each word at least once, therefore ending up with \$\mathcal O(n+m)\$ if one.size() \$\mathcal O(n)\$ and two.size() is \$\mathcal O(m)\$. However, since one.size() == two.size() \$n = m\$, otherwise we can find an answer in \$\mathcal O(1) \$.

Regardless, you can solve this in \$\mathcal O(1)\$ additional memory, if you use std::array<int, 128> or another fixed/variable size \$O(1)\$ indexable container:

typedef std::array<int, 256> character_count_type;

bool is_anagram(const std::string & one, const std::string & two)
{
    if(one.size() != two.size())
    {
        return false;
    }
    character_count_type character_count_one;
    character_count_type character_count_two;

    for(std::size_t i = 0; i < one.size(); ++i){
        assert(0 <= one[i] && one[i] < 256);
        assert(0 <= two[i] && two[i] < 256);
        character_count_one[one[i]]++;
        character_count_two[two[i]]++;
    }
    return character_count_one == character_count_two;
}

This has \$\mathcal O(n)\$ worst time complexity, and due to the constant size of std::array<...> \$\mathcal O(1)\$ additional space.


Checking address fields for presence of a Post Office Box I'm just trying to learn how to write jQuery better so I wanted some smart peoples opinions.

The code is checking the billing and shipping address fields to see if it contains a PO Box address by looking for variations of "PO" and then displays a warning message after the input if it does contain it.

The code is duplicated for each one so there has to be a better way to write this.

jsFiddle

(function ($) {
    'use strict';

    // Check for PO Box text in address fields on checkout
    var $shipAddress = $('#checkout input[name="user_data[s_address]"]'),
        $billAddress = $('#checkout input[name="user_data[b_address]"]'),
        message = '

We don\'t ship to PO Boxes. Sorry for the inconvenience.

';

    $shipAddress.after(message);
    $billAddress.after(message);

    $shipAddress.keyup(function () {
        var $value = $(this).val();

        if ($value === 'po' || $value === 'Po' || $value === 'PO' || $value === 'P.O' || $value === 'p.o') {
            $shipAddress.siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $shipAddress.siblings('.message').addClass('hidden');
        }
    });

    $billAddress.keyup(function () {
        var $value = $(this).val();

        if ($value === 'po' || $value === 'Po' || $value === 'PO' || $value === 'P.O' || $value === 'p.o') {
            $billAddress.siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $billAddress.siblings('.message').addClass('hidden');
        }
    });

}(jQuery));

There's 2 obvious places to clean this up a bit.  The first is the duplicated keyup handler and the second is the check for PO.

Most jQuery functions can be called on a collection of elements.  Since you are calling the same code on both your shipping address and billing address, we can combine them like so:

var $addresses = $('#checkout input[name="user_data[s_address]"]')
                 .add($('#checkout input[name="user_data[b_address]"]));

For the PO check, you can just use a regular expression:

//p, optional period, o
if(/p\.?o/i.test($value)) { ... }

So the whole thing becomes:

(function ($) {
    'use strict';

    // Check for PO Box text in address fields on checkout
    var $addresses = $('#checkout input[name="user_data[s_address]"]')
        .add($('#checkout input[name="user_data[b_address]"]')),
        message = "

We don't ship to PO Boxes. Sorry for the inconvenience.

";

    $addresses.after(message);

    $addresses.keyup(function () {
        var $value = $(this).val();

        if(/p\.?o/i.test($value)) {
            $(this).siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $(this).siblings('.message').addClass('hidden');
        }
    });

}(jQuery));

See: http://jsfiddle.net/6RnxG/7/


One way encoding a password I wrote a script that one way encrypts a user's password by generating a key, and multiplies by the ASCII value of the character and the ASCII value of the key character at (the position that the character has in the password string) and repeats this for the previous positions in the string meaning a key string with ASCII values of:

  49, 50, 51, 52, 53, 54, 55, 56, 57, 48

and a password string of

  112, 97, 115, 115, 119, 111, 114, 100, 46, 49

Would be multiplied like:

  [(112 * 49)] + [(97 * 50) + (97 * 49)] + ...

and so on.

As an example, the string password.1 encrypted with the key 1234567890 (auto generated) would return 203456.

$pass = $_GET["pass"];
$key = $_GET['key'];
$newPass = go($pass, $key);
echo $newPass;
function go($pass, $key){
    $passArray = str_split($pass);
    $keyArray = str_split($key);

    $total = 0;
    $pos = 0;
    foreach ($passArray as $char){
        $posInitial = $pos;
        while($pos !== 0){
            $total = $total + (ord($keyArray[$pos]) * ord($char));
            $pos--;
        }
        $pos = $posInitial + 1;
     }
     return $total;
}
function authorise($pass, $passEncode, $key){
    $passEncodeTest = go($pass, $key);
    if ($passEncode == $passEncodeTest){
        return true;
    } else {
        return false;
    }
}
authorise('password.2', $newPass, $key); ==> returns false
authorise('password.1', $newPass, $key); ==> returns true

(I will add a password sanitiser in later, but for now, plaintext)

My question is, what can I do to improve this code?

Reinventing the wheel

Please note that encryption should, in principle, always be two way. A 'one way encryption' just sounds weird. Normally something is encrypted with the intention to decrypt it at a later stage. This is not what you do. What you do is called hashing or perhaps 'encoding' as you indicated in your title.

You could improve the formatting of your code. I really miss the empty lines before and after the functions, and a space before { in a function. Your code is also not working because of the ==> in it at the end. This should be // ==>. Is is just nice if your code is working like published here.

The naming of your variables seems somewhat sloppy. In the beginning you call an encoded password $newPass. Is it a new password? I don't think so. Then in the authorise() function you rename it correctly to $passEncode, by which you probably mean $passEncoded.

So the big question is, why do you define your own hashing routine? Is it any better than the existing routines? PHP offers many of them!

See this link to tutsplus which addresses some issues that are encountered in hashing passwords. Is your routine resistent to all these issues? I don't think so.

It is far better to use a good existing password hashing library that is present in PHP. Have a look here: http://php.net/manual/en/faq.passwords.php

There's a lot of information about hashing and PHP out there. Find it and use it. I do really support new and inventive ways to solve problems but in this case it is definately not a good idea to reinvent the wheel.