Nested dictionary creation with setdefault Is there a better way of creating a nested dictionary than what I'm doing below? The result for the setdefault is because I don't know whether that particular key exists yet or not.
def record_execution_log_action(
        execution_log, region, service, resource, resource_id, resource_action
    ):
        execution_log["AWS"].setdefault(region, {}).setdefault(service, {}).setdefault(
            resource, []
        ).append(
            {
                "id": resource_id,
                "action": resource_action,
                "timestamp": datetime.datetime.now().strftime("%Y-%m-%d %H:%M:%S"),
            }
        )

Use a defaultdict like so:
from collections import defaultdict

resource_dict = lambda: defaultdict(list)
service_dict = lambda: defaultdict(resource_dict)
region_dict = lambda: defaultdict(service_dict)
execution_log = defaultdict(region_dict)

execution_log['AWS']['region']['service']['resource'].append({
                "id": 'resource_id',
                "action": 'resource_action',
                "timestamp": "%Y-%m-%d %H:%M:%S",
            })

execution_log

output:
defaultdict(<function __main__.<lambda>()>,
            {'AWS': defaultdict(<function __main__.<lambda>()>,
                         {'region': defaultdict(<function __main__.<lambda>()>,
                                      {'service': defaultdict(list,
                                                   {'resource': [{'id': 'resource_id',
                                                      'action': 'resource_action',
                                                      'timestamp': '%Y-%m-%d %H:%M:%S'}]})})})})


Simple router in PHP Before I start writing my first MVC framework, I want to improve my code quality as much as I can, can anyone give me some constructive criticism on how I can improve this?

It is a simple router, built for simple use. It should be pretty simple what's going on, and the usage should be easy to picture. It's nothing major but works well for simple functionality using it.

<?php

class Router {

    public $routes = [];
    public $routeMethods = [];

    public function get($route, $settings) {
        $this->add($route, $settings, ['GET']);
    }

    public function post($route, $settings) {
        $this->add($route, $settings, ['POST']);
    }

    public function add($route, $settings, $methods) {
        $this->routes[$route] = $settings;
        $this->routeMethods[$route] = $methods;
    }

    public function dispatch() {
        $requestUrl = $_SERVER['QUERY_STRING'];

        if (array_key_exists($requestUrl, $this->routes)) {
            $route = $this->routes[$requestUrl];
            $methods = $this->routeMethods[$requestUrl];
            $method = $_SERVER['REQUEST_METHOD'];

            if (!in_array($method, $methods)) {
                echo 'Method ' . $method . ' not allowed.';
            }
            else {
                $controller = $route['controller'];

                if (class_exists($controller)) {
                    $class = new $controller(); // grab from di cache when we have one
                    $action = $route['action'];

                    if (method_exists($class, $action)) {
                        $class->$action();
                    }
                    else {
                        echo 'We couldn\'t find action \'' . $action . '\ in controller \'' . $controller . '\'';
                    }
                }
                else {
                    echo 'We couldn\'t find controller \'' . $controller . '\'';
                }
            }
        }
        else {
            echo '404 not found';
        }
    }

}

set public $routes to private or protected if its jsut an internal resource of this class. Just use public if other classes/code explicitly should call the mothod/property
method_exists doesn't check if the method is callable. But that might be important if you execute legacy code
Avoid echo 'some text' Better use exceptions and an exception handler
If you use if (){} else{} you could think about using the if for the termination condition and the function continues regulary if its ok
Your note about the dependency injection cache is good. Keep your idea. You are on the rigth way.
I would recommend to use doc-blocks as annotation and for IDE autocompletion for all public functions as well as class properties. This will increase the readability
You didn't said which version of PHP you are using. Switch to >= 7.1 because 7.0 will reach its end of life soon. So you should use return types and type hinting


Make repeating numbers 0 and putting them at the beginning of array I am new to programming and I have made a program to make all repeating numbers in an array 0 and put those all 0s in left side:

import java.util.*;
class ArrayWork
{
    public static void main(String args[])
    {
        Scanner sc = new Scanner(System.in);
        int arr[],n,i,j,nr[]= new int[5];
        System.out.println("Enter the value for n: ");
        n = sc.nextInt();
        if(n>25)
            System.out.println("Invalid input");
        else
        {
            arr = new int[n];
            System.out.println("Enter the array elements: ");
            for(i=0;i<n;i++)
            {
                arr[i] = sc.nextInt();
            }
            int count =0;
            for( i=0; i< arr.length-1;i++)
            { for (j=i+1;j<arr.length;j++)
                {if(arr[i]==arr[j])
                   nr[i]=arr[i];
                    System.out.println(nr[i]);
                }}
            for(int v=0;v<n;v++){  for(i=0;i<n;i++)
            {
                if(arr[i]==nr[v])
                {
                    for(j=i-1;j>=0&&arr[j]>0;j--)
                    {
                        arr[j+1]=arr[j];
                    }
                    arr[j+1]=0;
                }
            }}

            System.out.println("The array is: ");
            for(i=0;i<n;i++)
            {
                System.out.print(arr[i] + " ");
            }
        }
    }

}

Can I do anything to make this program smaller although the output is coming right and the program is ok
but I have made many loop,
Hope you know any better way?

INPUT:

10,20,20,4,4,6

OUTPUT:

0,0,0,0,10,6

Declaration

  int arr[],n,i,j,nr[]= new int[5];

Consider to declare variables on separate lines. From Code Complete, 2nd Edition, p761:

  Use only one data declaration per line

  [...]

  It’s easier to modify declarations because each declaration is self-contained.

  [...]

  It’s easier to find specific variables because you can scan a single column rather than reading each line. It’s easier to find and fix syntax errors because the line number the compiler gives you has only one declaration >on it.

Unused Variables

  int count =0;

count gets declared and initialized but is never in use.. Unused variables should be removed.

Magic Number

  Your code uses a number that has a certain meaning to it.

One magic number is hidden in the code.

  if(n>25) {

You can create a new constant variable MAX_SIZE

  if(n>MAX_SIZE) {
``

Formatting

  for( i=0; i< arr.length-1;i++)
{ for (j=i+1;j<arr.length;j++)
    {if(arr[i]==arr[j])
       nr[i]=arr[i];
        System.out.println(nr[i]);
    }}

The code has an inconsistent formatting. Some key words are on a new-line and some times on the same line of a {.

Additional the code is format in a more C-like style, which I respect but you should have a look into Oracles Code Conventions for the Java.

Reduce Code Complexity Via Methods

When we consider a Code Smell Metrics and look at the code, we will find at least two points, that apply:

  Methods with more than 20 lines of code
  Methods with nesting more than 2 (?) levels deep
  Any global or static variables

To reduce the complexity we can introduce some new methods. For example

  for(i=0;i<n;i++)
{
    arr[i] = sc.nextInt();
}

can be wrapped into a method storeUserInput.


Binary Search Tree insert while keeping track of parent for node to be added - iteration 2 Follow up question to
  Binary Search Tree insert while keeping track of parent for node to be added

I am implementing a red black tree for fun and am wondering how I should modify my basic BST insert. Note: this happens before the red black tree rules are applied, it just finds the correct place within the tree to add the node, places it, sets references, value and defaults the color to RED. I am mainly struggling to see if there may be a better way to tack on the parent reference for the newly added node. The implementation I have here looks ahead one step with a NULL check where a BST insert that does not need to track the parent would not need.

struct node * bstInsert(struct node *n, int x) {

  if (n != NULL) {

    int isGreater = (n->val < x) ? -1 : (n->val > x);

    if (isGreater == -1) {

      if (n->left == NULL) {
        n->left = createAChild(n, n->left, x);
      } else {
        bstInsert(n->left, x);
      }

    } else if (isGreater == 1) {

      if (n->right == NULL) {
        n->right = createAChild(n, n->right, x);
      } else {
        bstInsert(n->right, x);
      }

    }

  } else{

    n = createAChild(NULL, n, x);

  }

  return n;
}

struct node * createAChild(struct node *par, struct node *n, int x) {

  n = malloc(
    sizeof(struct node)
  );
  n->parent = par;
  n->left = n->right = NULL;
  n->val = x;
  n->color = RED;

  return n;

}

Is there a cleaner solution to setting the parent reference for the node to be added?

You should check the result given by malloc, because otherwise your program may crash. Maybe not on your system right now, but basically, memory allocation can fail, and if it does, your program will crash afterwards by dereferencing a null pointer.

Additionally, your createAChild function should not take a node pointer. What happens if I pass in an existing object? You just overwrite it. That's a cause for memory leaks. Treat createAChild as a constructor: either give it a cleanly allocated node struct, or let it allocate its own struct.


Testing to see if tree is BST I found a function online that tests if a tree is a binary search tree:

private boolean isBST(Node node) {
  if (node==null) return(true);

  // do the subtrees contain values that do not
  // agree with the node?
  if (node.left!=null && maxValue(node.left) > node.data) return(false);
  if (node.right!=null && minValue(node.right) <= node.data) return(false);

  // check that the subtrees themselves are ok
  return( isBST(node.left) && isBST(node.right) );
}

I don't quite understand why maxValue and minValue are being used.

Here is the code for both functions:

private int minValue(Node node) {
  Node current = node;
  while (current.left != null) {
    current = current.left;
  }

  return(current.data);
}

private int maxValue(Node node) {
  Node current = node;
  while (current.right != null) {
    current = current.right;
  }

  return(current.data);
}

I made some edits to the isBST1 in my own way, but I'm not sure if there are any flaws in it or not.

static boolean isBST1(BinaryTreeNode root) {
    if (root == null) {
        return true;
    }
    if (root.left != null && root.left.value > root.value) {
        return false;
    }
    if (root.right != null && root.right.value < root.value) {
        return false;
    }
    return isBST1(root.left) && isBST1(root.right);

}

I am wondering if there is any sort of logic I am missing.

Here's a simple tree that can demonstrate a couple things:

    5
   / \
  3   7
 / \   \
1   6   8

minValue and maxValue are written so that the algorithm can catch the 6 that is out of place. And it seems like even the original versions of those functions are not doing that right. Your new algorithm will check the left value (3) and say it's okay, then check the 3-1-6 subtree and also say it's okay, when there's obviously a 6 that is on the left side and that is bigger than the root.

Your new algorithm, however, is broken in the same way as the old algorithm that uses minValue and maxValue is broken, so you translated code with functions into code with no functions "correctly" (didn't change anything).


SemaphoreSlim limit tasks I find myself having to loop through a lot of async calls all over the place in my code base.  The current pattern we are using is to declare SemaphoreSlim(maxcount) and then await sem.WaitAsync(), Create a new task, add it to a List and then repeat.  To control the release the new task itself has a reference to sem and does the release on final.  There is a step in there to look for and remove completed tasks.  This is a pattern that was inherited and is used for multiple different types of async calls.  As I develop new code I was hoping to simply this into a single helper class where I can just queue the work until I hit a set limit and then have to wait to add the next once a slot as freed up.

The calls are all async (as this is a 3rd party lib that we don't control).  Also, the library I maintain has to be used in both winform and asp.net processes, so keeping it async seems ideal.  The targets are usually waiting web services (5-10s range), in which we don't want to slam, but at the same time we typically have 1000's of items in the queue (3rd part has no bulk update implementation -- one element at a time type situation).

This is what I have come up with (naming is still a work in progress):

// Assume this is the parameter to the method we're calling over and over again
private class _ProcessArg
{
  public Guid ID { get; set; }
}

// The method that we are calling over and over again
private async Task _Process(_ProcessArg arg, CancellationToken ct)
{
  await Task.Run(() => { System.Diagnostics.Debug.WriteLine(arg.ID.ToString())});
}

// The main loop where we generated the data for the call.
private async Task _RunMainLoop(CancellationToken ct)
{
  int maxThreads = 10;
  ConcurrentQueue<Guid> queue = new ConcurrentQueue<Guid>();

  // Typically this would be the database load/whatnot
  for (int i = 0; i < 100000; i++)
  {
    queue.Enqueue(Guid.NewGuid());
  }

  AsyncTaskMutex mutex = new AsyncTaskMutex(maxThreads);
  while (true)
  {
    Guid id;
    if (queue.TryDequeue(out id))
    {
      await mutex.QueueTask<_ProcessArg>(_Process, new _ProcessArg
      {
        ID = id,
      }, ct);
    }
    else
    {
      await mutex.DrainQueue(ct);
      break;
    }
  }
}

// Class I'm looking for peer review on
public class AsyncTaskMutex
{
  private SemaphoreSlim _sem;
  private List<Task> _tasks;

  public AsyncTaskMutex()
    : this(10)
  {
  }

  public AsyncTaskMutex(int maxTasks)
  {
    _sem = new SemaphoreSlim(maxTasks, maxTasks);
    _tasks = new List<Task>();
  }

  public async Task DrainQueue(CancellationToken ct)
  {
    await Task.WhenAll(_tasks);
    _tasks.RemoveAll(t => t.IsCompleted);
  }

  public async Task QueueTask<T>(Func<T, CancellationToken, Task> func, T args, CancellationToken ct = default(CancellationToken))
  {
    await _sem.WaitAsync(ct);
    try
    {
      Task task = func(args, ct);
      task.GetAwaiter().OnCompleted(_OnCompleted);
      _tasks.Add(task);
    }
    catch (OperationCanceledException)
    {
      // Intentional ignore
      return;
    }
  }

  private void _OnCompleted()
  {
    _sem.Release(1);
    _tasks.RemoveAll(t => t.IsCompleted);
  }

}

It seems you simply need process a fixed set of work items in parallel with a fixed degree of parallelism and in an async compatible way. Stephen Toub has written a very elegant way to do that in just a couple lines of code.

public static Task ForEachAsync<T>(this IEnumerable<T> source, int dop, Func<T, Task> body)
{
    return Task.WhenAll(
        from partition in Partitioner.Create(source).GetPartitions(dop)
        select Task.Run(async delegate {
            using (partition)
                while (partition.MoveNext())
                    await body(partition.Current);
        }));
}

Your new code would be:

await ForEachAsync(GetWorkItems(), dop: 16, body: async item => {
 await ProcessItem(item); //TODO
});

There is no need to explicitly maintain a queue. But if you want to, you can du that by feeding ConcurrentQueue.GetConsumingEnumerable() into that ForEachAsync helper.


Comparing each item from dir with each item from another dir The task is to compare students homework SQL files with mentors SQL files.

I've written two functions, which return a two-dimensional array (1st elements are an absolute path, 2nd are a relative).

Then I'm going to compare the relative path of students and mentors and execute SQL files (finding using absolute path) if these values are equal

Is there a more elegant implementation?

The folder structure of mentors dir:

Homework (folder)
  ├ 1 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ 2 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ n (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql

The folder structure of students dir:

├Students Homework (folder)
 ├Student1(folder)
  ├ 1 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ 2 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ n (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
 ├Student2(folder)
  ├ 1 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ 2 (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql
  ├ n (folder)
  | ├ 1.sql
  | ├ 2.sql
  | └ n.sql

"Mentors" function:

def find_mentors_sql(config):

    mentors_sql_abs = []
    mentors_sql_rel = []

    for dirpath, subdirs, files in walk(config["MAIN_DIR"] + '\\Homework'):
        mentors_sql_abs.extend(path.join(dirpath, x) for x in files if x.endswith(".sql"))
        mentors_sql_rel.extend(path.join(path.basename(dirpath), x) for x in files if x.endswith(".sql"))

    mentors_sql = [[0] * 2 for i in range(len(mentors_sql_abs))]

    iter = 0
    for _ in mentors_sql_abs:
        mentors_sql[iter][0] = mentors_sql_abs[iter]
        iter += 1

    iter1 = 0
    for _ in mentors_sql_rel:
        mentors_sql[iter1][1] = mentors_sql_rel[iter1]
        iter1 += 1

    return mentors_sql

"Students" function (the logic is similar to the previous one:

def find_students_sql(config):

    students_sql_abs = []
    students_sql_rel = []

    for dirpath, subdirs, files in walk(config["MAIN_DIR"] + '\\Students Homework'):
        students_sql_abs.extend(path.join(dirpath, x) for x in files if x.endswith(".sql"))
        students_sql_rel.extend(path.join(path.basename(dirpath), x) for x in files if x.endswith(".sql"))

    students_sql = [[0] * 2 for i in range(len(students_sql_abs))]

    iter = 0
    for _ in students_sql:
        students_sql[iter][0] = students_sql_abs[iter]
        iter += 1

    iter1 = 0
    for _ in students_sql:
        students_sql[iter1][1] = students_sql_rel[iter1]
        iter1 += 1

    return students_sql
```

It's recommended to use enumerate rather than iter (renamed i), _ and indexing.

for i, abs_path in enemerate(mentors_sql_abs):
    mentors_sql[i][0] = abs_path

It's better to use zip rather than build mentors_sql manually.
Your function could further be simplified if you don't extend mentors_sql_* and just yield the values.
Please only use one string delimiter, either ' or ".
x is a pretty bad variable name, I would use file. Even for a comprehension it's pretty poor, as x isn't short hand for anything.
The only difference between the two functions as you walk different paths. And so you can change your input to account for this, and use one function.
I don't see the need for returning both relative and absolute paths, and so won't comment on it too much. You may want to return one and convert when needed.

def find_sql(path):
    for dirpath, subdirs, files in walk(path):
        for file in files:
            if file.endswith('.sql'):
                yield (
                    path.join(dirpath, file),
                    path.join(path.basename(dirpath), file)
                )

mentors = find_sql(config["MAIN_DIR"] + '\\Homework')
students = find_sql(config["MAIN_DIR"] + '\\Students Homework')


Comparing multiple variables and assigning new values respectively I know there is a more elegant way to write this.  The logic is solid and doing what I want it to, but I feel like when written like this, it is awfully cumbersome to have these many if statements essentially doing the same thing.

if (forward) {
    if (counter == listEnd) {
        counter = 0;
    }
    else {
        counter++;
    }

    if (nextCounter == listEnd) {
        nextCounter = 0;
    }
    else {
        nextCounter++;
    }

    if (prevCounter == listEnd) {
        prevCounter = 0;
    }
    else {
        prevCounter++;
    }
} // end if

Introduce a function:

var boundedIncrement = function(current, maximum) {
    return current === maximum ? 0 : current + 1;
};

Then use it:

if (forward) {
    counter = boundedIncrement(counter, listEnd);
    nextCounter = boundedIncrement(nextCounter, listEnd);
    prevCounter = boundedIncrement(prevCounter, listEnd);
}

No more logic duplication.

You could also close over listEnd if you wanted to have a single argumented function:

if (forward) {
    var boundedIncrement = function(current) {
         return current === listEnd ? 0 : current + 1;
    };
    counter = boundedIncrement(counter);
    nextCounter = boundedIncrement(nextCounter);
    prevCounter = boundedIncrement(prevCounter);
}


Particle Swarm Optimization I wrote my first python code to conduct PSO. I am wondering about the best practices for Python. While my code works, I want to make sure it's orthodox as well. For example, here is my class I use followed by a function that initializes the list of Particles that I use for the algorithm:

########### data representation
pList = []
class Particle:
    #value, x_pos, y_pos
    gBest     = [0.0, 0, 0]
    bestIndex = 0

    #takes index in pList as constructor argument
    def __init__(self, i):
        #x,y coords, randomly initialized
        self.x          = randint(-worldWidth/2,worldWidth/2)
        self.y          = randint(-worldHeight/2,worldHeight/2)
        #x,y velocity
        self.velocity_x = 0.0
        self.velocity_y = 0.0
        #personal best
        #[fitness value, x coord, y coord]
        self.pBest      = [Q(self.x, self.y), self.x, self.y]
        self.index      = i
        #local best
        self.lBest      = []
        self.lBestIndex = 0
        #array for neighbor indicies
        self.neighbors  = []
    #for printing particle info
    def __str__(self):
        if k > 0:
            return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n  l: '+str(self.lBest)+'\n'
        else:
            return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n'

###########

def createParticles():
    global pList
    global numParticles
    global k
    #create particle list
    for i in range(0,numParticles):
        pList.append(Particle(i))

    #fill neighbor lists
    if k > 0:
        for p in pList:
            for x in range(p.index-(k/2),p.index+(k/2)+1):
                if x > numParticles:
                    p.neighbors.append(x%numParticles)
                elif x < 0:
                    p.neighbors.append(numParticles+x)
                elif x == numParticles:
                    p.neighbors.append(0)
                else:
                    p.neighbors.append(x)
        updatelBest()

    #initialize global and local bests
    updategBest()

My main questions are:

Is this a correct class structure?
Should I have made createParticles() part of the class?

If anyone would care to see the whole code, I'd love to get any feedback you'd care to provide. The code is on Github here. Also, if anyone cares to comment on my .md file, I wouldn't mind!

I had a quick look at the github repo. I'd highly recommend splitting your project into multiple files as this will make things easier to manage. You might also want to consider writing some unit tests.

Design comments:
The use of global in createParticles is a bit concerning, it looks like you want to have some sort of ParticleManager class (or similar) that will explicitly manage the particles. This will be much easier to maintain than keeping a list in global scope. Things like gBest and bestIndex really work better when they are in an appropriate management class.

From looking at the Github code it looks like you have a bunch of different functions that manipulate a global list of particles. By creating a class that stores the list of particles plist and has methods that operate on it you will save yourself a lot of headaches when dealing with the data. This encapsulation will help you keep maintaining the code simple. So to answer your question, definitely make createParticles a method of a class. For example if you have a problem with the data in your current design you first have to search through a whole bunch of different functions to see which ones could possibly have side effects that modified your data before you can be sure you have fixed your problem. By having the dedicated particle management class you immediately know exactly which functions could be responsible and you immediately know where they are.

Python specific comments:
You really should get in the habit of using docstrings. For example:

class Particle:
    """This class models a particle in the system, it does a,b,c...."""

def createParticles():
    """This function creates the particle objects used in the system"""

You have some really long string lines, Python will let you concatenate these strings. So instead of:

if k > 0:
      return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n  l: '+str(self.lBest)+'\n'

you could instead do:

if k > 0:
       return('  i: '+str(self.index)+'\n'
              '  x: '+str(self.x)+'\n'
              '  y: '+str(self.y)+'\n'
              'v_x: '+str(self.velocity_x)+'\n'
              'v_y: '+str(self.velocity_y)+'\n'
              '  b: '+str(self.pBest[0])+'\n'
              '  l: '+str(self.lBest)+'\n')

Which is easier to read already. However you can go further here by using string formatting. Additionally there's duplicated code in building the strings for returning, just make the first string and if K > 0 just append what you need in that case.

So the complete __str__ implementation I might go with looks something like this:

def __str__(self):
    """Creates string representation of particle"""
    ret = """  i: {self.index!s}
  x: {self.x!s}
  y: {self.y!s}
v_x: {self.velocity_x!s}
v_y: {self.velocity_y!s}
  b: {self.pBest[0]!s}""".format(**locals())

    if k > 0:
        return ret+'  l: '+str(self.lBest)+'\n'
    else:
        return ret


Find the shortest whole repetitive substring (part 2) This is a continued improvement from discussion here (Find the shortest whole repetitive substring), since code has changed a lot dues to improvement, I post it here. Major smart ideas are from arvindpdmn.

Major algorithm ideas,

since we need to find wholly repetitive sub-strings, we only build Trie tree if the sub-string length is a factor of total string length (if not a factor, such sub-string are not eligible dues to not able to wholly repetitive)
If wholly repetitive, all nodes need to have only one child node. I did the checking in method checkChildNumber.

Questions,

Trie tree is the best way I can think of to resolve this problem for the purpose of reducing algorithm time complexity, not sure if any other better ideas to make it even faster (in terms of algorithm time complexity perspective).

from __future__ import print_function
from collections import defaultdict

class TrieNode:
    def __init__(self):
        self.children = defaultdict(TrieNode)
        self.isEnd = False
    def addNode(self, word):
        node = self
        for ch in word:
            node = node.children[ch]
        node.isEnd = True
    def checkChildNumber(self):
        node = self
        while node.isEnd == False:
            if len(node.children) != 1:
                return False
            for child in node.children.values():
                node = child
        return True

if __name__ == "__main__":
    word = "catcatcat" # output is 3
    #word = "aaaaaaa" # output is 1
    #word = "aaaaaab" # output is 7
    result = len(word)
    for subLength in range(1, len(word)//2 + 1):
        if len(word) % subLength == 0:
            start = 0
            root = TrieNode()
            while start <= len(word) - subLength:
                root.addNode(word[start:start+subLength])
                start += subLength
            if root.checkChildNumber():
                result = min(result, subLength)

    print (result)

You don't follow PEP8's naming conventions, which makes your code slightly harder to read for those used to it.
You should use is when comparing to booleans. Or you can remove the comparison and use not.
Which reads better, while not node.isEnd.
While loops are normally better written as for loops.
As then you don't have to define a variable outside it's scope, and then increment it in the loop.
Using a trie is a bit much here.
It makes the code more complex for no measurable improvement to the code.

Instead I'd change your code to just be the code in you __name__ == '__main__'.
This is as it's the part that's doing most of the work.

Keeping the outer for loop, and the if. You can change the while loop to be a generator of substrings,
and then you compare all slices to the first slice, and if they are all the same yield the size.
If any of them are not go to the next sub-length.

def whole_substrings(word):
    for sub_length in range(1, len(word) // 2 + 1):
        if len(word) % sub_length == 0:
            sub_slices = (
                word[start:start+sub_length]
                for start in range(0, len(word), sub_length)
            )
            first = next(sub_slices, None)
            for i in sub_slices:
                if i != first:
                    break
            else:
                yield sub_length
    yield len(word)

You can use this in two ways the less cryptic min, or the more cryptic next.
As it guarantees the items are returned in sorted order.

To further improve this you can change the for sub-length loop to be \$O(\sqrt{n})\$, rather than \$O(n)\$.
If you yield the product and the 'other half' of the product, i.e. 3 * 5 == 15, so you'd yield 3 and 5, by just by finding 3.


Tesing a simple calculator class with JUnit/JMock I'm playing around with JMock and trying to understand whether I understand the idea correctly.

There's an interface Printer aimed to print integers somewhere. There's an interface Calculator aimed to perform math. There's a class CalculatingMachine aimed to connect Calculator and Printer: first, it calculates the result with Calculator and then this result is passed to Printer. Here's the code

public class CalculatingMachine {
    private final Printer printer;
    private final Calculator calculator;

    public CalculatingMachine(Printer printer, Calculator calculator) {
        this.printer = printer;
        this.calculator = calculator;
    }

    public void processAdd(int x, int y) {
        int result = calculator.add(x, y);
        printer.print(result);
    }

    public static interface Printer {
        void print(int x);
    }

    public static interface Calculator {
        int add(int x, int y);
    }
}

As far as I understand CalculatingMachine only requires one test: we need to make sure IT USES calculator to do the math and then we need to make sure IT USES printer to print the result. So, here's my test:

public class CalculatingMachineTest {
    @Test
    public void testCalculatingMachine() {
        Mockery context = new JUnit4Mockery();

        final Printer printer = context.mock(Printer.class);
        final Calculator calculator = context.mock(Calculator.class);

        context.checking(new Expectations() {{
            oneOf(calculator).add(1, 2);
            will(returnValue(3));

            oneOf(printer).print(3);
        }});

        CalculatingMachine machine = new CalculatingMachine(printer, calculator);
        machine.processAdd(1, 2);

        context.assertIsSatisfied();
    }
}

Pretty straightforward and does what it supposed to do. There are 2 points I'm curious about:

Is it the right approach even though CodePro says that CalculatingMachine itself has 19 lines of code, though test for it is 23 lines of code? I'm just not sure: is it normal to write more tests then code that does something?
Is there any other way to test all the same but with less code?

This is technically correct code, whether it's right depends...

This might be the first test for an object that will become more complicated. It's reasonable to start small and work your way up.

If the Calculator is a self-contained implementation that doesn't pull in other dependencies, it might be better to just use a real one. In this case, it looks more like a function.

In the end, there might well be more test than production code because you want to exercise all the paths. These days, I find myself writing unit tests with mocks at a slightly higher level, around a small cluster of objects.

If the call to the Calculator doesn't change the state of the world outside the Machine, I'd probably use an allowing clause, we "Stub Queries, Expect Actions"

allowing(calculator).add(1,2); will(returnValue(3));

it's the same implementation underneath but expresses the intent more clearly.

You might try the JUnitRuleMockery with JUnit's @Rule. It takes care of the assertion housekeeping at the right time.

And before someone jumps in with their favourite alternative mocking framework, I'm not sure that's really the point. We can shave a few lines off here and there, but it's more important to understand the design issues. And everything will change with Java 8.


Finding the bounding box of a polygon array I have a Polygon array, and I need to get a Rectangle object which bounds all of the polygons inside the array. Think convex hull - except this is a rectangle and not a polygon.

Here's my current code which works:

public static Rectangle getBoundingBox(final Polygon[] polygons) {
    if (polygons == null) {
        return new Rectangle(-1, -1, 0, 0);
    }

    int minX = Integer.MAX_VALUE;
    int minY = Integer.MAX_VALUE;
    int maxX = Integer.MIN_VALUE;
    int maxY = Integer.MIN_VALUE;

    for (final Polygon polygon : polygons) {
        final Rectangle polygonBounds = polygon.getBounds();

        int ax = polygonBounds.x;
        int ay = polygonBounds.y;
        int bx = ax + polygonBounds.width;
        int by = ay + polygonBounds.height;

        minX = Math.min(ax, minX);
        minY = Math.min(ay, minY);
        maxX = Math.max(bx, maxX);
        maxY = Math.max(by, maxY);
    }

    final Rectangle boundingBox = new Rectangle(minX, minY, 1, 1);
    boundingBox.add(maxX, maxY);

    return boundingBox;
}

  Remember: Execution time is top priority for me

Your code looks remarkably similar to the actual awt code used in Polygon to get the boundingBox...., start with extreme min/max values, and then go from there.

I often find that the choice to use min()/max() functions instead of if conditions is also fastest, so I agree with your code there.

What has me somewhat concerned, is the use of:

 final Rectangle polygonBounds = polygon.getBounds();

That's an interesting call, which may, or may not do a lot of work. Consider its implementation:

if (bounds == null) {
   calculateBounds(xpoints, ypoints, npoints);
}
return bounds.getBounds();

If the bounds have been previously calculated, then it will be very fast. If not, it will have to do a lot of work, and additionally create a bunch of instances of Rectangle.

In essence, if you check the bounds often and don't change the Polygons, then your bound-check is amortized quite well. If not, then you are doing a lot of unnecessary work.

In order to improve the elapsed time (execution time), about the only two things I can recommend are:

parallelism
keep primitive.

The combined parallelism/primitive would be easy to implement using Java 8 streams, and abusing that the Polygon data is public...

IntSummaryStatistics xstats = Stream.of(polygons)
     .parallel().
     .flatMapToInt(poly -> IntStream.of(poly.xpoints).limit(poly.npoints))
     .summaryStatistics();

IntSummaryStatistics ystats = Stream.of(polygons)
     .parallel().
     .flatMapToInt(poly -> IntStream.of(poly.ypoints).limit(poly.npoints))
     .summaryStatistics();

return new Rectangle(xstats.min(), ystats.max(),
     xstats.max() - xstats.min() + 1, ystats.max() - ystats.min() + 1);

The above code will be fine because it introduces the parallelism easily. If you are content with single-thread, an primitives only, I would recommend something like:

for (Polygon poly : polygons) {
    for (int i = 0; i < poly.npoints; i++) {
        xmin = Math.min(poly.xpoints[i], xmin);
        xmax = Math.max(poly.xpoints[i], xmax);
        ymin = Math.min(poly.ypoints[i], ymin);
        ymax = Math.max(poly.ypoints[i], ymax);
    }
}

The above will be faster when you would otherwise have to recalc all the poly bounds anyway....

Unfortunately, it all depends on your use case, and your ability to multithread.

If you are constrained, then you may as well stick with what you have, it's fast, neat, and otherwise fine.


Generate parentheses solution I have coded a solution to build all valid permutations of parentheses.

My code is below.

I have a question on my code based on a comment by my PEP8 checker. It said that there was no need to include the line return anywhere in the code (initially I included one). The solution works but I have never had a case using recursion where I didn't have to use the return line in the base-case.

How come I don't need it here?

The following expression initially had a return statement but I was told this was obsolete.

    if number_open == number_pairs and number_closed == number_pairs:
        print(output)
        build_parentheses.counter += 1
        return

Python 3.7 code:

"""Module builds all valid permutations of n parentheses
For example: n = 3:
    ((()))
    (()())
    (())()
    ()(())
    ()()()
Total = 5
"""

def build_parentheses(number_pairs: int,
                      output="",
                      number_open=0,
                      number_closed=0)-> str:
    """The function that builds the parentheses. Output as a string:
        number_pairs: number of parentheses pairs user desired
        All other parameters are private
    """
    if number_open == number_pairs and number_closed == number_pairs:
        print(output)
        build_parentheses.counter += 1
    else:
        if number_open < number_pairs:
            output += "("
            build_parentheses(number_pairs, output, number_open + 1, number_closed)
            output = output[:-1]
        if number_closed < number_open and number_open:
            output += ")"
            build_parentheses(number_pairs, output, number_open, number_closed + 1)

if __name__ == "__main__":
    build_parentheses.counter = 0
    build_parentheses(5)
    print(f"=========\n{build_parentheses.counter} solutions")

By comparison, in this post I made, I did use the return statement.

As mentioned here I'll provide a short answer to summarize what we discussed. A return statement only impacts code that short-circuits any remaining code that would have been called if omitted.

pseudo code snippets below

The return statement here skips snippet 2.

if (condition) {
  // .. snippet 1
  return;
}
// snippet 2

The return statement here is unnecessary.

if (condition) {
  // .. snippet 1
  return;
} else {
  // snippet 2
}


RAII-style single-linked list After watching Herb Sutter describe single-linked lists in terms of unique_ptr I decided to implement my own. In particular I want to know if my move semantics are correct and if any unnecessary copies are made. It passes my simple cases with int and I plan to add insertion/deleting later.

#include <iostream>
#include <memory>
#include <cstddef>
#include <utility>

template<typename Type>
class Node {
public:
  Node(const Type& data) : data {data} {}
  std::unique_ptr<Node<Type>> next = nullptr;
  Type data;
};

template<typename Type>
class LinkedList {
public:
  void push_back(const Type& data) {
    if (!head) {
      head = std::make_unique<Node<Type>>(data);
      return;
    }

    Node<Type>* end = head.get();

    while (end->next) {
      end = end->next.get();
    }

    end->next = std::make_unique<Node<Type>>(data);
  }

  Type pop_back() {
    if (!head) {
      throw;
    }

    Node<Type>* end = head.get();
    Node<Type>* previous = nullptr;

    while (end->next) {
      previous = end;
      end = end->next.get();
    }

    const Type data = end->data;

    if (previous) {
      previous->next = nullptr;
    }

    return data;
  }

  void push_front(const Type& data) {
    if (!head) {
      head = std::make_unique<Node<Type>>(data);
      return;
    }

    std::unique_ptr<Node<Type>> new_head = std::make_unique<Node<Type>>(data);
    new_head->next = std::move(head);
    head = std::move(new_head);
  }

  Type pop_front() {
    if (!head) {
      throw;
    }

    const Type data = head->data;
    head = std::move(head->next);
    return data;
  }

  std::size_t size() const {
    if (!head) {
      return 0;
    }

    std::size_t size = 1;
    Node<Type>* traverse = head.get();

    while (traverse->next) {
      traverse = traverse->next.get();
      size++;
    }

    return size;
  }

  /* iterative destroy to avoid recursive deletes */
  void destroy() {
    if (!head) {
      return;
    }

    while (head->next) {
      head = std::move(head->next);
    }

    head = nullptr;
  }

private:
  std::unique_ptr<Node<Type>> head = nullptr;
};

Edit: With Deduplicator's response I ended up with this code

#include <iostream>
#include <memory>
#include <cstddef>
#include <utility>

template<typename Type>
class Node {
public:
  Node(const Type& data) : data {data} {}
  std::unique_ptr<Node<Type>> next = nullptr;
  Type data;
};

template<typename Type>
class LinkedList {
public:
  void push_back(const Type& data) {
    auto end = &head;

    while (*end) {
      end = &(*end)->next;
    }

    *end = std::make_unique<Node<Type>>(data);
  }

  Type pop_back() {
    auto end = &head;
    auto previous = &head;

    while (*end) {
      previous = end;
      end = &(*end)->next;
    }

    const Type data = (*previous)->data;
    *previous = nullptr;
    return data;
  }

  void push_front(const Type& data) {
    auto new_head = std::make_unique<Node<Type>>(data);
    new_head->next = std::move(head);
    head = std::move(new_head);
  }

  Type pop_front() {
    const Type data = head->data;
    head = std::move(head->next);
    return data;
  }

  ~LinkedList() {
    auto end = &head;

    while (*end) {
      *end = std::move((*end)->next);
    }
  }

private:
  std::unique_ptr<Node<Type>> head = nullptr;
};

Almost always auto. Only write down the specific type when you have to, it's shorter, clearer, and less error-prone.

You currently handle an empty list as a special case. Don't, it's neither efficient nor elegant.
Instead, use a pointer to pointer, and everything is simplified. As an example:

void push_back(const Type& data) {
    auto p = &head;
    while (*p)
        p = &(*p)->next;
    *p = std::make_unique<Node<Type>>(data);
}

Good of you to avoid recursive descent in the dtor. But, you know that's not spelled destroy()?


PHP form XSS vulnerability I'd like to ask if this PHP/HTML code has any flaws which can be exploited by the potential aggressor:

<form method='post' action='<?php echo $_SERVER['PHP_SELF']; ?>'>
<input type='text' name='foo' value='<?php echo htmlspecialchars($_POST['foo']); ?>' />
<input type='submit' value='bar' />
</form>

I'm aware that it can be exploited by calling script like this http://foo.bar/baz.php/'><script>alert('xss')</script><foo' and

<?php echo $_SERVER['PHP_SELF']; ?>

should be replaced with

<?php echo htmlentities($_SERVER['PHP_SELF']); ?> or <?php echo htmlspecialshars($_SERVER['PHP_SELF']); ?>

Are there any other possible ways to conduct XSS attack on this piece of code? If so, how can I secure this form?

@Update: Ok, I've just found another possible attack - entering code like ' onmouseover='alert(String.fromCharCode(88,83,83,33)); (with the apostrophe in the beginning) into the text field, which causes simple 'XSS!' alert. It can be avoided by changing <?php echo htmlspecialchars($_POST['foo']); ?> to <?php echo str_replace('\'', '&#39;', htmlspecialchars($_POST['foo'])); ?>

For single-quoted attributes, use htmlspecialchars($text, ENT_QUOTES).

Depending on what this does, you might want to implement some sort of protection against CSRF attacks.

You might also want to check $_SERVER['SCRIPT_NAME'].

Other than that, this code seems reasonable.


Printing a singly linked list in reverse order I am preparing for interview and I want to write the function in the way that the interviewers will expect me to write.

private static void printReverse(ListNode l1){
        if(l1 != null){
            printReverse(l1.next);
            System.out.print(l1.val + " ");
        }
    }

However, this function does not do anything if l1 is null. Should I take care of this case? If yes, how?

Even with such a short piece of code there is a lot to say.

Firstly, your example code is an example, designed to test just one piece of skill. A real list should never expose the fact that the implementation relies on a ListNode. In fact, your method is private, indicating that the detail is not public. The real printReverse method should have no argument, and should be public, because it should start with the head node of the list, and use that to start the recursive reverse print.

Also, that would be the right place to handle the nulls....

Recursive methods often (normally?) come in pairs, a setup method, and the actual recursive method. Your pair would look like:

public void printReverse() {
    if (head == null) {
        System.out.println("null");
    } else {
        printReverseRecursive(head);
    }
}

private void printReverseRecursive(ListNode node) {
    if (node == null) {
        return;
    }
    printReverseRecursive(node.next);
    System.out.print(node.val + " ");
}

Note, that while we are there, you have one of those wtf moments in your variable names. the name l1 is a really, really bad name. l is very easy to confuse with 1, and should never be used as a simple 1-letter variable name, and then, to make it worse, you put them together? Huh.


Simple Dictionary Chrome Extension I have created a Simple Dictionary Chrome Extension. It is using JavaScript/ some HTML/ some CSS. The Manifest.jso file talks about the extension. The HTML is for a small/simple pop up and the JS is for the popup and user interactions.

MANIFEST.JSON

{
"manifest_version": 2,
"name": "My Simple Dictionary Extension",
"description": "A simple dictionary extension where users can look up a word in the extension and find its definition.",
"author": "Tristan Sun",
"version": "0.1",
"content_scripts":[
    {
        "matches": ["<all_urls>"],
        "js":["jquery-3.1.1.js"]
    }
],
"browser_action": {
    "default_title": "Dictionary Popup",
    "default_popup": "popup/popup.html",
    "default_icon": "dic.png"
},
"background": {
    "scripts": ["jquery-3.1.1.js","background.js"],
    "persistent": false
},
"permissions": ["tabs"]
}

POPUP.JS

(function () {
'use strict';

// add enter key functionality to click 'define' button.
document.getElementById('term').addEventListener("keyup", function (event) {
    event.preventDefault();
    //keycode is 13 when the key released is 'enter' key.
    if (event.keyCode === 13) {
        document.getElementById('define').click();
    }
});

//when define button is clicked, then handleButton in backgroundpage.
document.querySelector('button').addEventListener("click", function () {
    var backgroundpage = chrome.extension.getBackgroundPage();
    var term = document.querySelector('input').value;
    if (backgroundpage !== null) {
        backgroundpage.handleButtonClick(term);
    }

});
}());

//retrieve message from background.js getting the response from the dictionary api and adding it to the popup.
chrome.runtime.onMessage.addListener(function (response) {
'use strict';

var b = document.getElementById('seemore');
//    document.getElementById('label').innerHTML =
//set text as definition to be shown
var definition = document.getElementById('definition');
definition.innerText = response.results[0].lexicalEntries[0].entries[0].senses[0].definitions[0];

//set see more text
b.innerText = "Click for more info on this word.";
//link see more text
b.href = "";
//when href is clicked, create a new tab with the actual dictionary entry for the word at oxford dictionary.
b.addEventListener("click", function () {
    chrome.tabs.create({url: "http://en.oxforddictionaries.com/definition/" + response.results[0].id.toLowerCase()});
});
});

POPUP.HTML

<!DOCTYPE html>

    <title></title>
    <link rel="stylesheet" href="popup.css" />

    Insert a word:
    <input id="term" type="text">
    <button id="define">Define</button>

POPUP.CSS

body {
min-width: 295px;
}

label{
font-family: fantasy;
font-size: 1.5em;
}

#define{
background-color: crimson;
border: none;
color: white;
padding 10px 20px;
text-align: center;
font-size: 1em;
}

If you are using jQuery than why use vanilla JS DOM manipulation such as document.getElementById and others, why not use jQuery DOM manipulation like jQuery("#term")?


No more filthy words Challenge

Given a list of words mixed with extra symbols. Write a program that will clean up the words from extra numbers and symbols.

Specifications

The first argument is a path to a file.
Each line includes a test case.
Each test case is a list of words.
Letters are both lowercase and uppercase, and mixed with extra symbols.
Print the words separated by spaces in lowercase letters.

Constraints

The length of a test case together with extra symbols can be in a range from 10 to 100 symbols.
The number of test cases is 40.

Input Sample

  (--9Hello----World...--)
  Can 0$9 ---you~
  13What213are;11you-123+138doing7

Output Sample

  hello world
  can you
  what are you doing

Source

Solution:

#include <stdio.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

void to_lowercase(char * input) {
    for(int i = 0; input[i]; i++){
        input[i] = tolower(input[i]);
    }
}

char * sanitize(char * input) {
    char *sanitized = malloc(sizeof (char) * 1024);
    int iterator = 0;
    int character_value;
    bool wordMatched = false;

    for (int i = 0; i < strlen(input); i++) {
        int character_value = input[i];
        // a-z = 97-122, A-Z = 65-90
        if (character_value >= 97 && character_value <= 122
        ||  character_value >= 65 && character_value <= 90) {
            sanitized[iterator++] = input[i];
            wordMatched = true;
        } else if (wordMatched) {
            wordMatched = false;
            sanitized[iterator++] = ' ';
        }
    }

    sanitized[iterator] = '\0';
    to_lowercase(sanitized);
    return sanitized;
}

int main(int argc, const char * argv[]) {
    FILE *file = fopen(argv[1], "r");
    char line[1024];
    while (fgets(line, 1024, file)) {
        printf("%s\n", sanitize(line));
    }

    return 0;
}

Look here:

for (int i = 0; i < strlen(input); i++) {

Note that strlen(input) is O(n), proportional to the length of the input.  That makes your algorithm O(n2), which is slower than it should be.  If you need to call strlen(), make sure to call it just once.  However, this problem is easily solvable without using strlen() at all.

Others have pointed out your memory leak.  What should be done about it?  The output is never longer than the input, right?  Therefore, I would say that your best option is to overwrite the input with the sanitized output.  There is no need to allocate any additional memory, and no need to worry about the buffer size.  (If the caller wants to keep the original string, then the caller can duplicate it first.)

There is no need to write a to_lowercase() function that calls tolower() on each character in the string.  Just call tolower() as part of the loop.

The character_value comparisons could be simplified to isalpha(character_value).  Your naming style is inconsistent between character_value and wordMatched.  I would change int character_value to char c.

With this code…

} else if (wordMatched) {
    wordMatched = false;
    sanitized[iterator++] = ' ';
}

… you output a space when transitioning from a letter to a non-letter.  However, that would output a space corresponding to the 7 at the end of 13What213are;11you-123+138doing7.  In my opinion, it would be better if it didn't output a space at the end.

Suggested solution

I've put some miscellaneous remarks in comments.

#include <assert.h>
#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>              // You missed this for printf(3)
#include <stdlib.h>

/**
 * Replaces consecutive non-alphabetic characters in the input
 * string with a single space.  Non-alphabetic characters at the
 * beginning and end are trimmed off as well.  The remaining ASCII
 * letters are replaced with their lowercase counterparts.
 *
 * The input string will be overwritten.
 *
 * Returns the length of the sanitized output.
 */
size_t sanitize(char *s) {
    bool needSpace = false;
    char *out = s;
    for (char *in = s; *in != '\0'; in++) {
        assert(out <= in);
        if (isalpha(*in)) {
            if (needSpace) *out++ = ' ';
            needSpace = false;
            *out++ = tolower(*in);
        } else if (out > s) {
            needSpace = true;
        }
    }
    *out = '\0';
    return out - s;
}

int main(int argc, const char *argv[]) {
    char line[1024];
    FILE *file = stdin;        // Read from stdin if no filename given
    if (argc > 1 && !(file = fopen(argv[1], "r"))) {
        perror(argv[1]);       // Some error handling that you didn't have
        return EXIT_FAILURE;
    }
    while (fgets(line, sizeof(line), file)) {
        sanitize(line);
        puts(line);            // Don't need printf() when puts() will do
    }
}


Building a Bill of Materials I have this loop that builds a BOM but it is so slow as I scale it out to even modest data.  All the data resides in two database tables.  I am pulling the data in as list of lists and then iterating over them.

The orders table contains the product serial number and the top level root part numbers that are ordered for that serial number.  The parts_h (hierarchy) table contains every parent - child relationship for every part defined where I work.  I am trying to build one master table by marrying them together.

Here is some sample data:

order = [['ABC1200','1234567'],['ABC1300','1234567'],['ABC1400','2456693']]
# format is [serial number, top level part]

parts_h = [['1234567','3445678'],['3445678','5K9091'],['2456693','4J5689'],
             ['4J5689','09981'],['4J5689','09982'],['09981','2K5050']]
# format is [parent, child]

def bom_build(o,p):
    '''
    This function builds the bill of material for a serial number.
    It takes two inputs.
    1.  O which is a list of lists with each element of a list
    containing the serial number, root part number
    2. P which is a list of lists with each element of a list containing
    the parent part and child part as defined in Parts master
    '''

    bom=[]
    for product in o:
        for part in p:
            if product[1] == part[0]:
                # build first level of BOM
                bom.append([product[0],product[1],part[1],"/"+product[1]+"/"+part[1]])

    # build children
    for part in p:
        for row in bom:
            if row[2] == part[0]:
               bom.append([row[0],part[0],part[1],row[3]+"/"+part[1]])
        continue
    return bom

This is the output, which is correct but slow:

[['ABC1200', '1234567', '3445678', '/1234567/3445678'],
 ['ABC1300', '1234567', '3445678', '/1234567/3445678'],
 ['ABC1400', '2456693', '4J5689', '/2456693/4J5689'],
 ['ABC1200', '3445678', '5K9091', '/1234567/3445678/5K9091'],
 ['ABC1300', '3445678', '5K9091', '/1234567/3445678/5K9091'],
 ['ABC1400', '4J5689', '09981', '/2456693/4J5689/09981'],
 ['ABC1400', '4J5689', '09982', '/2456693/4J5689/09982'],
 ['ABC1400', '09981', '2K5050', '/2456693/4J5689/09981']]

I'm then going to write this to an Impala table.

Some time is lost by having to traverse the p list for each product. For large data, you could preprocess the p list, and create a dict that is keyed by the part, and which provides a list of sub parts for it. Then the lookup for a given product will be faster.

Secondly, the second part of the code will find deeper nested parts, but will fail to find them all when the nesting is deeper.

Here is suggested improvement:

def bom_build(o,p):
    # Create a hash keyed by parts, providing their sub parts as list
    d = dict()
    for [part, subpart] in parts_h:
        if part in d:
            d[part].append(subpart)
        else:
            d[part] = [subpart]

    bom = []

    def recurse(bom, d, serial, part, path, required):
        if part in d:
            for subpart in d[part]:
                nextpath = path + '/' + subpart
                bom.append([serial, part, subpart, nextpath])
                recurse(bom, d, serial, subpart, nextpath, False)
        elif required: # when there are no sub parts
            bom.append([serial, part, None, path])

    for [serial, part] in order:
        recurse(bom, d, serial, part, part, True)

    return bom

See it run on repl.it

Alternative: Breadth First Search

As an alternative you could also use a BFS algorithm -- the first part is the same:

def bom_build_bfs(o,p):
    # Create a hash keyed by parts, providing their sub parts as list
    d = dict()
    for [part, subpart] in parts_h:
        if part in d:
            d[part].append(subpart)
        else:
            d[part] = [subpart]

    # add start elements to bom, which later will be removed
    bom = [[serial, None, part, part] for [serial, part] in order]
    i = 0
    # treat it as a queue, adding to it while looping
    while i < len(bom):
        [serial, parent, part, path] = bom[i]
        i += 1
        if part in d:
            for subpart in d[part]:
                bom.append([serial, part, subpart, path + '/' + subpart])
        elif i < len(order): # when there are no sub parts
            bom.append([serial, part, None, path])

    # return the part without the starter elements
    return bom[len(order):]

In my limited tests this ran slower than the DFS solution, but it might depend on how the input data is distributed and how deeply nested it is.


Multiples of 3 and 5 Project Euler problem #1:

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

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

My code is correct, but I don't know how I can optimize and make that faster.

N = gets.to_i
N.times do
  arr = []
  (0...gets.to_i).each { |j| arr << j if j % 3 == 0 || j % 5 == 0 }
  puts arr.reduce(:+)
end

You use a constant N as your variable... not dreadful, but a little confusing.

You first ask the user for how many times they want to run the code, then for which number to sum the mods of 3 & 5 for; it would be simpler to just write the code which asks for a number and works the sum - if the user wants to run it multiple times, then let them! :-)

No need to start at zero - 0,1, & 2 are never going to be divisible by three (although we'd need to add some validation in case the user adds a very low number)

Instead of setting up an intermediate arr value and iterating with each, you can select to get the numbers that return true for the block.

But your main question is about speed - have you benchmarked your code to find out if the speed is an issue?

n = gets.to_i
return 0 unless n > 2 # you could do this as a ternary or `if` statement if you prefer
(3...n).select { |j| j % 3 == 0 || j % 5 == 0 }.reduce(:+)


Portable (Linux, Cygwin,...) Makefile for my own Shell OpenSSL file en-/decryption scripts Let it be clear that in this question I ask for further guidance with my current version of Makefile used for my own Shell OpenSSL file en-/decryption scripts, not those scripts themselves.

I did my best, but this is my own first Makefile, so I better give it to you for review. I may only think the work is done, there may be many issues I don't know about.

Requirements:

The implicit destination path must be easily overridden without having to edit the Makefile
The implicit target must check for integrity (hashsum) stored in the file SHA512SUMS
The file must account for non-standard Linux environments like Cygwin
I myself tested it under normal circumstances in Linux Mint 18.3 and Cygwin
All files can be simply acquired from GitHub page of this little project

The Makefile follows:

DESTDIR?=/usr/local/bin
install_path=$(DESTDIR)

script_name_1=encrypt-file-aes256
script_name_2=decrypt-file-aes256

user_id=$(shell whoami)
group_id=$(shell id -gn)

.PHONY: check
.PHONY: install
.PHONY: uninstall

check:
    [ -f $(script_name_1) ] && [ -f $(script_name_2) ] && sha512sum --check SHA512SUMS --status && echo "OK: Files are prepared. You may use make install command now." || echo "ERROR: Files are missing and / or hashsum mismatch!"

install:
    install --verbose --mode=0755 --owner=$(user_id) --group=$(group_id) --target-directory=$(install_path) $(script_name_1) $(script_name_2)

uninstall:
    rm $(install_path)/$(script_name_1) $(install_path)/$(script_name_2)

The check target always succeeds (because the last command in the pipeline is echo).  We'd like it to fail if any of the tests fail, so we should just write them one per line.  Instead of testing for the existence of the files, we should make the check target depend on them:
check: $(script_name_1)  $(script_name_2)
    sha512sum --check SHA512SUMS
    @echo "OK: Files are prepared. You may use make install command now."

BTW, script_name_1 and script_name_2 are completely unhelpful variable names.  How about encrypt and decrypt, for example?

I think the install target should depend on check - we really don't want to install if the check fails:
install: check

I'm not convinced that this is a good thing:
user_id=$(shell whoami)
group_id=$(shell id -gn)

When we install software, we normally make it owned by an appropriate user on the system, not the person doing the installing.  In most cases, programs are owned by root, so that no ordinary user can accidentally overwrite them (or any other harmful action).

Make provides a $(RM) definition to give you a more portable command for deleting files.

It's great that you've thought about install location and about overridability of such parameters.  You probably ought to know the conventional variables that are used for these:

DESTDIR specifies where the root directory is.  This is usually empty, but packaging tools set this to a suitable working directory to build a (partial) filesystem image that can be archived.
PREFIX works somewhat like your DESTDIR or install_path (I'm not sure why you have two names for the same thing).

This is a rare Makefile that doesn't need .DELETE_ON_ERROR: (since it contains no non-PHONY targets).

Modified code
This is what I ended up with:
PREFIX = /usr/local

install_path = $(DESTDIR)$(PREFIX)/bin

encrypt = encrypt-file-aes256
decrypt = decrypt-file-aes256

check: $(encrypt)  $(decrypt)
    sha512sum --check SHA512SUMS
    @echo "OK: Files are prepared. You may use make install command now."

install: check
    install --d -D $(install_path)
    install -m 0755 -t $(install_path) $(encrypt) $(decrypt)

uninstall:
    $(RM) $(install_path)/$(encrypt) $(install_path)/$(decrypt)

.PHONY: check install uninstall

Note that Make variables specified on the command line will override those in the Makefile, so you can write
make install PREFIX="$(HOME)/testing"

to install to your test directory, for example.


Constructor Chaining in Java When creating multiple constructors, I usually do the following;

public ConstructorName(int count)
{
    this(count, 0);
}

public ConstructorName(String count)
{
    this(Integer.parseInt(count), 0);
}

public ConstructorName(int count, int other)
{
    // Do something here
}

However, I can also do it this way;

public ConstructorName(String count)
{
    this(Integer.parseInt(count)); // Chain to the constructor that just takes a number
}

public ConstructorName(int count)
{
    this(count, 0);
}

public ConstructorName(int count, int other)
{
    // Do something here
}

Is there any actual difference here? Which would be better to use when coding?

If there is more than one way to construct an instance, then static factory methods backed by one private constructor, can really help readability, since they can be named differently.

Range halfOpen = Range.largerThan(5);
Range closed = Range.between(7, 12);

If it is really about setting some properties and not others, then the Builder pattern can be what you want.

Person john = Person.named("John")
    .lastName("Lennon")
    .gender(Male)
    .yearOfBirth(1940)
    .build();

Both of these approaches also allow for hiding which implementation class is really returned. Our Person instance may be an instance of subclass Man, and the half open Range may be an instance of HalfOpenRange. Both methods allow returning a cached instance, rather than an actual new instance.


Simple wrapper to use IEnumerable as a producer in a producer-consumer pattern I have a relatively simple problem: I have an IEnumerable<T>, which takes considerable time to yield each term. This is then used by another piece of code, which takes considerable (but not identical) time to process each term.

As I understand it, this is a classic producer-consumer scenario, so I'm choosing to use a BlockingCollection<T> so that I can produce and consume concurrently.

To support this, I have a simple wrapper:

class EnumerableProducer<T>
{
    public readonly BlockingCollection<T> ProducerCollection;

    public EnumerableProducer(IEnumerable<T> source)
    {
        ProducerCollection = new BlockingCollection<T>();
        AddItemsToBlockingCollection(ProducerCollection, source).Start();
    }

    private static Task AddItemsToBlockingCollection(BlockingCollection<T> collection, IEnumerable<T> source)
    {
        return new Task(() =>
        {
            foreach (var item in source)
            {
                collection.Add(item);
            }
            collection.CompleteAdding();
        });
    }
}

Any code having an IEnumerable<T> and wanting to use it as a producer passes it to the constructor, then accesses it through the ProducerCollection.

Interested in a general review, but in particular:

Is this the simplest/most idiomatic way to do achieve the overall goal?
Any comments/suggestions on naming.
Extensibility (e.g. adding cancellation or parallelizing the enumeration of the source) probably fall under YAGNI for me at the moment, but if there are alternative designs which would make this kind of extension in general easier to implement, I would be interested.

I think the implementation is too leaky - it should not expose how the blocking internals are achieved since if if decide do change this you might have to change a whole lot of application code which violates the open-closed-principle (the O part of SOLID). This can be easily fixed by making the class IEnumerable<T> and using GetConsumingEnumerable of the BlockingCollection.
I think the more idiomatic way to start a new task is to use Task.Factory.StartNew.
I would rename it into BlockingEnumerable since that what it is.
Since you're talking about long running operations you may want to add a method of canceling the task.
It might be useful to add a capacity for the intermediate blocking collection as well to provide better control of how much data is cached.

Slightly refactored code:

class BlockingEnumerable<T> : IEnumerable<T>
{
    private readonly BlockingCollection<T> _ProducerCollection;

    public BlockingEnumerable(IEnumerable<T> source)
    {
        _ProducerCollection = new BlockingCollection<T>();
        AddItemsToBlockingCollection(source);
    }

    public IEnumerator<T> GetEnumerator()
    {
        return _ProducerCollection.GetConsumingEnumerable().GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    private void AddItemsToBlockingCollection(IEnumerable<T> source)
    {
        Task.Factory.StartNew(() =>
        {
            foreach (var item in source)
            {
                _ProducerCollection.Add(item);
            }
            _ProducerCollection.CompleteAdding();
        });
    }
}


C++ Wrapper and Correctly Disposing Unmanaged Resources using ArrayHandle I have the following class

public class ExrDepthMapExtractor : IDepthMapExtractor
{
    public RawDepthMap GetDepthMap(string filePath)
    {
        return DepthMapExtractorService.ExtractDepthMapAs1DArray(filePath, Callbacks.ProgressCallback);
    }
}

This calls the static C++ API wrapper

public static class DepthMapExtractorService
{
    [DllImport("Blundergat.OpenExr.Adapter.dll", CallingConvention = CallingConvention.Cdecl)]
    [return: MarshalAs(UnmanagedType.I1)]
    private static unsafe extern bool ExtractDepthMapAs1DArray(
        out ItemsSafeHandle vectorHandle,
        out double* points,
        out int width,
        out int height,
        string filePath,
        Callbacks.ProgressFunc progressCallback);

    [DllImport("Blundergat.OpenExr.Adapter.dll", CallingConvention = CallingConvention.Cdecl)]
    private static unsafe extern bool Release(IntPtr itemsHandle);

    public static unsafe RawDepthMap ExtractDepthMapAs1DArray(string filePath,
        Callbacks.ProgressFunc progressCallback)
    {
        using (InternalExtractDepthMapAs1DArrayWrapper(filePath, out RawDepthMap rdm, progressCallback))
        {
            rdm.Source = filePath;
            return rdm;
        }
    }

    private static unsafe ItemsSafeHandle InternalExtractDepthMapAs1DArrayWrapper(
        string filePath, out RawDepthMap rdm, Callbacks.ProgressFunc progressCallback)
    {
        double* pixels;
        int width, height;
        ItemsSafeHandle itemsHandle;

        if (!ExtractDepthMapAs1DArray(out itemsHandle, out pixels, out width, out height, filePath, progressCallback))
            throw new InvalidOperationException();

        var pixelList = new List<double>();
        for (int i = 0; i < width * height; i++)
            pixelList.Add(pixels[i]);

        rdm = new RawDepthMap()
        {
            Source = filePath,
            DepthMapArray = pixelList,
            Height = height,
            Width = width
        };

        return itemsHandle;
    }

    public class ItemsSafeHandle : SafeHandleZeroOrMinusOneIsInvalid
    {
        public ItemsSafeHandle() : base(true) { }

        protected override bool ReleaseHandle()
        {
            return Release(handle);
        }
    }
}

In my C++ I Release the ArrayHandle as follows

#include "wrappers.h"

bool ExtractDepthMapAs1DArray(ArrayHandle* arrayHandle,
    double** pixels,
    int* width,
    int* height,
    const char* filePath,
    ProgressFunc progressCallback)
{
    try
    {
        auto v = new std::vector<double>();
        ExrDepthMapExtractor exrExtractor;
        *v = exrExtractor.extractDepthMap(filePath, *width, *height, progressCallback);

        *arrayHandle = reinterpret_cast<ArrayHandle>(v);
        *pixels = v->data();

        if (progressCallback != NULL)
        {
            std::string strFilePath(filePath);
            std::string message = "Extracted depth map from \"" + strFilePath + "\" successfully";
            if (progressCallback != NULL)
                progressCallback(message.c_str());
        }
        return true;
    }
    catch (...)
    {
        return false;
    }
}

bool Release(ArrayHandle arrayHandle)
{
    auto items = reinterpret_cast<std::vector<double>*>(arrayHandle);
    delete items;
    return true;
}

I have not used this before and the implementation was taken from [SO] (https://stackoverflow.com/questions/31417688/passing-a-vector-array-from-unmanaged-c-to-c-sharp), however, the way this is done does not look right to me and MSDN is different. The code seems to work fine with no memory leaks, but am I doing this the right way?

What don't you like about it? There's a very small space where a C++ exception could occur leaking the vector memory. If the ExrDepthMapExtractor or extractDepthMap throws, there's no way to clean the vector memory. You could just put something in the catch block.

You could memcpy the pixel memory from C++ to C# instead of looping. Or share memory the other direction, by pinning it and having the C++ side write into the provided memory. That's nice because it allows the garbage collector safely deal with the memory, but requires safeguards to prevent .NET from doing anything to the memory while C++ is using it.


Project Euler #35 - Circular primes solution is slow This is my code for project euler #35 (in Python):

The number, 197, is called a circular prime because all rotations of the digits: 197, 971, and 719, are themselves prime.
There are thirteen such primes below 100: 2, 3, 5, 7, 11, 13, 17, 31, 37, 71, 73, 79, and 97.
How many circular primes are there below one million?

The program takes about 2 minutes to run, and I don't really know how to improve the efficiency of it. I'm using a sieve to generate the primes, and I'm only checking rotations until the rotation isn't prime.
def primes_sieve(limit):
    limitn = limit+1
    not_prime = [False] * limitn
    primes = []

    for i in range(2, limitn):
        if not_prime[i]:
            continue
        for f in range(i*2, limitn, i):
            not_prime[f] = True

        primes.append(i)

    return primes

primes = set(primes_sieve(1000000));
exclude = ['2','4','5','6','8','0']
def circularPrime(n):
    ss = str(n);
    for i in range(n):
        if ss[-1] in exclude:
            return 0;
        if int(ss) not in primes:
            return 0;
        ss = ss[-1] + ss[:-1];
    return 1;

gg = 0;
for num in primes:
    gg += circularPrime(num);
print gg;

Any help is greatly appreciated.

BUG

Your program gives the wrong answer, yielding 53 instead of 55. The reason for this is you over-eagerly exclude rotations in circularPrime if anything ends in an even digit or 5. But 2 and 5 are prime, yet end in a digit that you exclude!

It turns out that the exclusion check (in addition to giving you the wrong answer) doesn't actually help you anyway. Timing your function run ten times:

with check:    6.094s
w/o check:     6.075s

So drop it, and you get the correct answer, and it runs faster (or the same, within noise).

Semicolons

Semicolons are a C/C++/Java/etc. thing. While they're technically correct Python syntax, you don't need them, and they look weird, so you can simply drop them.

Sieve

Typically, we'd start our sieve with something like is_prime = [True] * limitn. It's a little weird to see it backwards, but I guess that's ok.

More efficient sieving

Let's take a look at the sieve part of your code, swapping bools like I suggested:

for i in range(2, limitn):
    if not is_prime[i]:
        continue
    for f in range(i*2, limitn, i):
        is_prime[f] = False

    primes.append(i)

First of all, having the continue makes it harder to read - better to keep the flow mono-directional:

for i in range(2, limitn):
    if is_prime[i]:
        primes.append(i)
        for f in range(i*2, limitn, i):
            is_prime[f] = False

Now, there's two issues here. In Python 2.7, range() gives you a list. The full list. That's hugely wasteful since you never want the full list, only the next element one at a time. For that, there's xrange(). Just swapping out those two functions:

range()    6.075s
xrange()   4.913s

Now, let's take a look at your inner loop:

for f in xrange(i*2, limitn, i):
    is_prime[f] = False

f is a bad variable name for this, prefer something like j, but that's not important. Consider the bounds. At this point, we know that \$i\$ is prime. We also know that this is the first time we've gotten to it, so every multiple of \$i\$ is still marked as being potentially prime, except for those divisible by some other prime \$p < i\$. So \$2i\$ we already know isn't prime, because it's divisible by \$2\$. \$3i\$ likewise. And so on. The first composite number that we have to cross off will be the first one not divisible by any such prime \$p\$... which is \$i^2\$.

for j in xrange(i*i, limitn, i):
    is_prime[j] = False

That improves us to:

OP, w/ bugfix         6.075s
range() -> xrange()   4.913s
inner loop from i^2   4.343s

Bring back the bug

I love deliberately provocative headings. Anyway, you had the right idea of checking for the even digits, but we can be more aggressive with it. Before we check any of the rotations, let's check for the presence of any of those digits first:

def circularPrime(n):
    ss = str(n)
    if any(c in ss for c in '024568'):
        return 0

    # now check everything else

This way, we can avoid a whole bunch of work for all of our 6-digit primes that start with an even number. However, from the very first thing I wrote, we know that this will give us the wrong answer. However, we know how to fix it. We know we are erroneously excluding 2 and 5, so let's just add them back:

gg = 2 # because 2,5
for num in primes:
    gg += circularPrime(num)
return gg

That drops us down to:

OP, w/ bugfix         6.075s
range() -> xrange()   4.913s
inner loop from i^2   4.343s
excluding again       3.721s

I had originally said your check didn't give you any performance benefit, but this one does - since building up all the string rotations and doing all the int casting is expensive and in this way we're able to avoid all of that up front. Since the number of circular primes is sparse (only 55 out of ~75k primes!), anything we can do to aggressively weed our set down is good.


Displaying a table of shell aliases I'm just getting into bash and sh scripting.  I mostly just stub out little convenience or exercise scripts for myself, but I recognize I may be flaunting best practices at times.

My question is specifically related to best practices (or just common sense) when using rm inside a shell script.  My googles have only turned up suggestions to alias rm to rm -i or somesuch.

Anyway, enough talk, here's the script (note: the rm appears at the very end of the script):

#!/bin/sh -e
# Format output of alias print dump into more readable format

if [ -f "TEMP.AWK" ]; then
    echo "Somehow 'TEMP.AWK' already exists"
    exit 1
fi

cat ${HOME}/.zshrc | grep -e '^alias' | awk -F'=' 'BEGIN {
        print "ALIAS                         | COMMAND";
        print "---------------------------------------";
    }
    {

        # replace all multi-spaces with a single space
        gsub(/\s+/, " ", $0);

        col1Len  = 20;
        col2Len  = 60;
        aliasLen = length($1);
        cmdLen   = length($2);

        tab1len  = col1Len - aliasLen;
        tab2len  = col2Len - cmdLen;

        printf " " $1 " "

        for (i = 0; i < tab1len; i++)
            printf "-"

        print " " $2

    } END {
        print "---------------------------------------";
    }' > TEMP.AWK

cat TEMP.AWK | grep --color -e '[#].*'

rm TEMP.AWK

Any other feedback would not go unappreciated.

The best practice for dealing with temporary files is simply not to use them in the first place.  Many things can go wrong with temporary files:

Lack of permissions to write to a directory
Lack of disk space
Read-only filesystem
File or symlink already exists
File or symlink didn't exist when you checked, but got created immediately after, in a race condition
Script crashed, such that it created a temporary file but didn't get a chance to clean it up

Though there are ways to mitigate these risks, I see no reason to use a temporary file here at all.  For that matter, cat and the first grep are also superfluous.

#!/bin/sh

awk -F= '
    BEGIN {
        TABLE_FMT = "%-18s | %-58s"
        row = sprintf(TABLE_FMT, "ALIAS", "COMMAND");
        print row;
        gsub(".", "-", row);
        print row;
    }
    END {
        print row;
    }

    /^alias/ {
        gsub(/\s+/, " ", $0);  # Condense consecutive whitespace
        printf TABLE_FMT "\n", $1, $2;
    }
' < "$HOME/.zshrc" | grep --color '#.*\|$'

I can't tell for sure, but it looks like the second grep is meant to highlight comments.  Unfortunately, it actually ends up stripping out all uncommented lines, including the header and footer.

Instead of parsing the $HOME/.zshrc configuration file, you might want to query the live alias definitions in the current shell instead.  In that case, you should write a shell function that processes the output of the bare alias command.


Applying a string-identified effect on an image Task:

Given an image object and an effect identifier (string), apply the effect identified by the string on the image. The library of effects can grow as the program grows.

Code 1:

def apply_fx(img, fx):
    if fx == 'sepia':
        return sepia(img)
    elif fx == 'b&w':
        return contrast(grayscale(img), 2)
    elif fx == 'faded':
        return faded(img)
    elif fx == 'hdr':
        return hdr(img)

Code 2:

effects = {
    'sepia': sepia,
    'b&w': lambda x: contrast(grayscale(x), 2),
    'faded': faded,
    'hdr': hdr
}

def apply_fx(img, fx):
    return effects[fx](img)

Opinion: I personally lean toward the second approach.

Note: the concern here generalizes to: dictionary of lambda functions vs if-elif in such scenarios.

Extra code: as suggested by @PeilonRayz.

sepia_filter = np.array([[.393, .769, .189],
                         [.349, .686, .168],
                         [.272, .534, .131]])

def sepia(img):
    sepia_img = np.dot(img, sepia_filter.T)
    return sepia_img / np.max(sepia_img)

IMO your second solution (using a dict to store callables) is exactly right.

The first solution, with a series of if/else statements is not only inefficient, it also encourages and facilitates writing unrelated code inside the apply_fx function itself. To wit:

elif fx == 'b&w':
    return contrast(grayscale(img), 2)

Once you have put something other than a simple return func(img) in the code, a future maintainer will be tempted to "just write this inline here" rather than creating a lambda or a named function to encapsulate their code:

elif fx = 'grayscale':
    from thirdpartymodule import grayscale, formatconverter

    gsh = grayscale.Handle()
    img3p = formatconverter(img)
    result = grayscale.grayscale(gsh, img3p)
    return formatconverter(img3p, format=img)

Your second alternative, on the other hand, has a much more restricted, and thereby much cleaner interface: all operations must be encapsulated as callables.

Now, they could be functions, or they could be lambdas. They could be functools.partials or dynamically loaded or web api calls to a satellite orbiting planet Nebulon. But from the maintenance point of view, "create a callable, plug it in here with a name" is as good as you're likely to get. You could even do it dynamically, in response to a configuration file or plugin module:

def img2grayscale(img):
    from thirdpartymodule import grayscale, formatconverter

    gsh = grayscale.Handle()
    img3p = formatconverter(img)
    result = grayscale.grayscale(gsh, img3p)
    return formatconverter(img3p, format=img)

effects['grayscale'] = img2grayscale


LRU Cache in ECMAScript I wrote this for a CodeWars challenge while trying to learn ECMAScript and would really like to have some advice on how it could be improved.

What I don't like about this code myself, but am unsure on how to improve:

I didn't yet found a way to have a class in ECMAScript that mixes public and private methods in a way, where the public methods (and only them) can still access private methods and vars.
(Partly caused by the point above) I think the code is way too verbose and bloated. Too much of it is wasted on language specific implementation details. There must be a smarter way to do this - I feel like I could write the same functionality in PHP using 10% of the lines I needed now.

function LRUCache(capacity, init) {

  this._capacity = capacity;
  this._size = 0;
  this._accessMap = [];
  this._items = [];
  this._frozenMethods = ['cache', 'delete'];
  this._functionalProps = ['size', 'capacity'];

  this.cache = function(key, value) {

    if( this._accessMap.indexOf(key) == -1 ) {
      this._addNewItem(key);
    }

    this._items[key] = this[key] = value;
    this._updateAccessMap(key);

    return this;
  },
  this.delete = function(key) {

    if(this._frozenMethods.indexOf(key) !== -1 ||
    this._functionalProps.indexOf(key) !== -1) {
      return false;
    }

    if( this._accessMap.indexOf(key) === -1 ) {
      return true;
    }

    delete this[key];
    delete this._items[key];
    this._size--;

    this._updateAccessMap(key, true);

    return true;
  },
  this._removeOldest = function() {
    var key = this._accessMap[0];

    this.delete(key);
  },
  this._updateAccessMap = function(key, remove) {

    index = this._accessMap.indexOf(key);

    if( index !== -1) {
      this._accessMap.splice(index, 1);
    }

    if(!remove) {
      this._accessMap.push(key);
    }
  },
  this._addNewItem = function(key) {

    this._size++;

    if(this._size > this._capacity) {
      this._removeOldest();
    }

    Object.defineProperty(this, key, {
      get: function() {
        this._updateAccessMap(key);

        return this._items[key];
      },
      set: function(v) {
        this._items[key] = v;
        this._updateAccessMap(key);
        return this;
      },
      enumerable: true,
      configurable: true
    });
  };

  var obj = this;

  this._frozenMethods.map( function(v) {
    Object.defineProperty(obj, v, {
      enumerable: false,
      writable: false,
      configurable: false
    });
  });

  ['_items', '_capacity', '_accessMap', '_frozenMethods','_size', '_functionalProps',
  '_removeOldest','_updateAccessMap','_addNewItem'].map( function(v) {
    Object.defineProperty(obj, v, {
      enumerable: false,
      configurable: true
    });
  });

  Object.defineProperty(this, 'capacity', {
    set: function(v) {

      while(this.size > v) {
        this._removeOldest();
      }

      this._capacity = v;
    },
    get: function() {
      return this._capacity;
    },
    configurable: false
  });

  Object.defineProperty(this, 'size', {
    set: function(v) {
      return false;
    },
    get: function() {
      return this._size;
    },
    configurable: false
  });

  if(init) {
    for(var attr in init) {
      if( !init.hasOwnProperty(attr) ) {
        continue;
      }
      this.cache(attr, init[attr]);
    }
  }
}

I didn't yet found a way to have a class in ECMAScript that mixes public and private methods in a way, where the public methods (and only them) can still access private methods and vars.

JavaScript ain't classical OOP. Don't force it to be like one. The default inheritance pattern in JS is prototypal, and everything is public and methods are shared across instances.

You could emulate private by defining your data not as properties but as vars in the constructor. However, this requires that your methods be defined in the constructor to have access to them. This means that your methods are created on a per-instance basis, rather than shared via the prototype.

Anyways, off to a review of your code:

this._items = [];

I'm pretty sure you wanted a hash {} instead of an array.

this._items[key] = this[key] = value;

I'm not sure why you want to add the key-value pair as a property of the cache object. I'm pretty sure this is why you have to guard against overriding your methods. I suggest you don't. Make your object just an interface. Don't put your data on it. Just put it in _items.

this._frozenMethods.map( function(v) {
  Object.defineProperty(obj, v, {
    enumerable: false,
    writable: false,
    configurable: false
  });
});

['_items', '_capacity', '_accessMap', '_frozenMethods','_size', '_functionalProps',
'_removeOldest','_updateAccessMap','_addNewItem'].map( function(v) {
  Object.defineProperty(obj, v, {
    enumerable: false,
    configurable: true
  });
});

map is a special iterator method. It runs on each array item and expects a return value for each item to form the new array it creates. If you just end us using it to loop through, then forEach is a better choice. Less confusing.

Additionally, naming variables properly is a must. Explicit is better than implicit, and naming your variables explicitly is better for readability than having to guess what they're for. It takes 5 seconds for a developer to know a purpose of a variable if named properly, rather than 5 mins because the developer has to read through the implementation.

this.delete = function(key) {

IIRC, IE8 and some other browsers have issues when using some keywords as property/function names. Best use alternative names like remove.

Anyways, here's a simplified implementation. Should be straightforward:

function LruCache(capacity){
  this._capacity = capacity || 1;
  this._accessMap = [];
  this._items = {};
}

LruCache.prototype.addItem = function(key, value){
  if(this._accessMap.length >= this._capacity) this.removeOldest();
  this._accessMap.push(key);
  this._items[key] = value;
};

LruCache.prototype.remove = function(key){
  this._accessMap.splice(this._accessMap.indexOf(key), 1);
  delete this._items[key];
};

LruCache.prototype.removeOldest = function(){
  var oldestKey = this._accessMap.shift();
  delete this._items[oldestKey];
};

LruCache.prototype.getCapacity = function(){
  return this._capacity;
};

LruCache.prototype.getSize = function(){
  return this._accessMap.length;
};

LruCache.prototype.getItems = function(){
  var instance = this;
  return Object.keys(this._items).reduce(function(carry, key){
    carry[key] = instance._items[key];
    return carry;
  }, {});
};

If you really want privates by way of closures and throw away the entire concept of shared methods, you can do this:

function LruCache(capacity){
  var _capacity = capacity || 1;
  var _accessMap = [];
  var _items = {};

  function removeOldest(){
    var oldestKey = _accessMap.shift();
    delete _items[oldestKey];
  };

  this.addItem = function(key, value){
    if(_accessMap.length >= _capacity) removeOldest();
    _accessMap.push(key);
    _items[key] = value;
  };

  this.remove = function(key){
    _accessMap.splice(_accessMap.indexOf(key), 1);
    delete _items[key];
  };

  this.getCapacity = function(){
    return _capacity;
  };

  this.getSize = function(){
    return _accessMap.length;
  };

  this.getItems = function(){
    return Object.keys(_items).reduce(function(carry, key){
      carry[key] = _items[key];
      return carry;
    }, {});
  };

}


Getting HTTP Status Code I have code that take in a URL (via URL or String) and returns the status code.  I am new to web development, so it is very possible that I may be taking a poor approach at this.  One thing I don't like about my code is the need for the InetSocketAddress to be hardcoded, but I have not been able to find a way around this short of passing it in as parameters. As an inexperienced programmer and complete newbie to web development, what improvements to the following code would you suggest?

import java.io.IOException;
import java.net.*;

public class HttpStatusCodeRequester {
    public static int getHttpStatusCode(URL url) {
        try {
            Proxy proxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress("proxy.mycompany.com", 8099));
            HttpURLConnection connection = (HttpURLConnection) url.openConnection(proxy);
            connection.setRequestMethod("GET");
            connection.connect();
            int responseCode = connection.getResponseCode();
            connection.disconnect();
            return responseCode;
        } catch (IOException e) {
            System.err.println("HTTP Status Code Request failed for " + url + " in HttpStatusCodeRequester.getHttpStatusCode(URL)!");
            e.printStackTrace();
            return -1;
        }
    }

    public static int getHttpStatusCode(String url) {
        try {
            return getHttpStatusCode(new URL(url));
        } catch (MalformedURLException e) {
            System.err.println("Malformed URL passed into HttpStatusCodeRequester.getHttpStatusCode(String)!");
            e.printStackTrace();
            return -1;
        }
    }
}

You could use Properties and read from them... IMHO, I think almost everything that goes quoted ("like this") can be either a constant or sent to a Properties file, you could have one for messages and one for configuration for example.

Also, I've noticed that you have two return statements per method, I would recommend to have only one, you don't really need one inside the catch block since the exception is suppossed to be handled then returned. It is also good for readability.

Bear in mind that exceptions could happen before you reach the connection.disconnect(); statement so I would recommend a try, catch, finally and perform all your clean up un the finally block.

Another thing... I think it is better to handle the exception differently depending on the scenario or error that caused it, for example, a connection couldn't been stablished, wrong URL, timeout (I don't know), etc.


Vertical alignment with CSS I'm starting up a new project, and I've always had an issue getting vertical alignment right in CSS. Is there anything I could do better that what I've currently come up with? Replacing the header and nav with divs, and changing the rgba values to hex values this seems to work all the way back to IE7 which I'm happy with.

CSS

html, * {
    -moz-box-sizing:border-box;
    -webkit-box-sizing:border-box;
    -ms-box-sizing:border-box;
    -o-box-sizing:border-box;
    box-sizing:border-box;
}

body {
    margin:0;
    padding:0;
}

nav {
    background-color:rgba(0, 0, 0, 0.1);
}

nav a.tab {
    text-align:center;
    padding:0 15px;
    color:#FFFFFF;
    display:inline-table;

    vertical-align:middle;  text-decoration:none;
}

nav a.tab > span {
    display:table-cell;
    vertical-align:middle;
}

nav a.tab:hover {
    background-color:rgba(0, 0, 0, 0.1);
    text-decoration:underline;
}

header {
    background-color:#27AE60;
    font-size:1.2em;
    color:#FFF;
    display:table;
    width:100%;
}

header, header h1, header nav, header nav a.tab {
    height:80px;
    vertical-align:middle;
}

header nav {
    display:inline-block;
}

header h1 {
    margin:0 20px;
    display:inline;
    text-shadow:2px 2px 2px #333333;
}

HTML

Projects

            Home

            Projects

            Contact

jsFiddle

I only have some nitpicks about formatting and readability.
Hopefully somebody else will comment specifically about good approaches for treating vertical alignment.

It would be more readable (and the common convention) to put a space after colons.
So instead of:

background-color:rgba(0, 0, 0, 0.1);
text-decoration:underline;

Write as:

background-color: rgba(0, 0, 0, 0.1);
text-decoration: underline;

And don't put two definitions on the same line like this:

vertical-align:middle;  text-decoration:none;

It's better when code reads from top to bottom,
and your eyes can stay focusing on the left,
rather than dipping deeper into the right part, like this:

vertical-align: middle;
text-decoration: none;