Rails controller action methods corresponding to static web pages One of my rails controllers is horribly overcrowded with a bunch of methods that link to static web pages.

Controller

def adventure
end

def cooking
end

def dancing
end

def programming
end

def reading
end

def running
end

def sports
end

def writing
end

For each of the above actions I have seperate view files and routes defined for them.

View Files

adventure.html.erb
cooking.html.erb
dancing.html.erb
programming.html.erb
reading.html.erb
running.html.erb
sports.html.erb
writing.html.erb

Routes

match '/adventure', to: 'pages#adventure', via: 'get'
match '/cooking', to: 'pages#fighting', via: 'get'
match '/dancing', to: 'pages#first_person_shooter', via: 'get'

match '/programming', to: 'pages#programming', via: 'get'
match '/reading', to: 'pages#reading', via: 'get'
match '/running', to: 'pages#running', via: 'get'
match '/sports', to: 'pages#sports', via: 'get'
match '/writing', to: 'pages#writing', via: 'get'

It is quite embarrassing to have code that horrible. Is there any way I can optimize that?

You don't need to define empty actions in your Controller.

Your code should work even if you delete all the empty actions, leaving you only with the views and routes:

page_controller.rb:

class PageController < ApplicationController
end

views:

adventure.html.erb
cooking.html.erb
dancing.html.erb
programming.html.erb
reading.html.erb
running.html.erb
sports.html.erb
writing.html.erb

routes:

match '/adventure', to: 'pages#adventure', via: 'get'
match '/cooking', to: 'pages#cooking', via: 'get'
match '/dancing', to: 'pages#dancing', via: 'get'

match '/programming', to: 'pages#programming', via: 'get'
match '/reading', to: 'pages#reading', via: 'get'
match '/running', to: 'pages#running', via: 'get'
match '/sports', to: 'pages#sports', via: 'get'
match '/writing', to: 'pages#writing', via: 'get'

or, if you really want things super DRY:

%w(adventure cooking dancing programming reading running sports writing).each do |page|
  match "/#{page}", to: "pages##{page}", via: 'get'
end


Contest Solution: Remorseful Sam The Problem

  Sam Morose served for many years as communications officer aboard the U.S.S. Dahdit, a U.S. Coast Guard frigate
  deployed in the South Pacific. Sam never quite got over the 1995 decision to abandon Morse code as the primary ship-to
  shore
  communication scheme, and was forced to retire soon after because of the undue mental anguish that it was causing.
  After leaving the Coast Guard, Sam landed a job at the local post office, and became a model U.S. Postal worker…
  That said, it isn’t surprising that Sam is now holding President Clinton hostage in a McDonald’s just outside the
  beltway. The FBI and Secret Service have been trying to bargain with Sam for the release of the president, but they find it
  very difficult since Sam refuses to communicate in anything other than Morse code. Janet Reno has just called you to write a
  program that will interpret Sam’s demands in Morse code as English.
  Morse code represents characters of an alphabet as sequences of dits (short key closures) and dahs (longer key
  closures). If we let a period (.) represent a dit and a dash (-) represent a dah, then the Morse code version of the English
  alphabet is:

  Sample Input

  Your program must takes its input from the ASCII text file morse.in. The file contains periods and dashes representing
  a message composed only of the English alphabet as specified above. One blank space is used to separate letters and three
  blanks are used to separate words. Sample contents of the file could appear as follows:

.... . .-.. .-.. ---   -- -.--   -. .- -- .   .. ...   ... .- --

  Sample Output

  Your program must direct its output to the screen and must be the English interpretation of the Morse code found in the
  input file. There must be no blanks between letters in the output and only one blank between words. You must use all
  capital letters in the output. The output corresponding to the input file above is:

HELLO MY NAME IS SAM

code.py

import re

codes = {
    '.-':'A',
    '-...':'B',
    '-.-.':'C',
    '-..':'D',
    '.':'E',
    '..-.':'F',
    '--.':'G',
    '....':'H',
    '..':'I',
    '.---':'J',
    '-.-':'K',
    '.-..':'L',
    '--':'M',
    '-.':'N',
    '---':'O',
    '...':'S',
    '-':'T',
    '..-':'U',
    '...-':'V',
    '.--':'W',
    '-..-':'X',
    '-.--':'Y',
    '--..':'Z'
}

with open('morse.in') as f:
    for line in f:
        t = []
        for s in line.strip().split(' '):
            t.append(codes.get(s) if s else ' ')
        print(re.sub(' +', ' ', ''.join(t)))

Any advice on performance enhancement and solution simplification is appreciated, as are topical comments!

As long as you are using re.sub() to handle the spaces, you may as well use it to perform the entire task.  Note that I've added entries to codes that maps triple-space to a space, and space to nothing.  Also, since codes contains one entry per line, I prefer to put a comma after every entry, including a superfluous comma after the last one, to make it easy to add or remove entries.

import re

codes = {
    '.-': 'A',
    '-...': 'B',
    '-.-.': 'C',
    …
    '--..': 'Z',
    '   ': ' ',
    ' ': '',
}

with open('morse.in') as f:
    print(
        re.sub(
            '[.-]+|   | ',
            lambda match: codes[match.group()],
            f.read()
        ),
        end=''
    )


Combination and factorial Combinations is how many ways to take k elements from n element.  Combination is different from permutation in that order does not matter in combination. Combination is used a lot in poker calculations. Those are combinations used a lot in poker that I know are correct.

In Fast there are a number of terms that cancel out so I avoid them.

Please review for style and efficiency.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Diagnostics;
using System.Numerics;

namespace CodeReview01
{
    class Program
    {
        static void Main(string[] args)
        {
            for (int i = 2; i <= 10; i++)
            {
                BigInteger factorial = Factorial(i);
                Debug.WriteLine($"factorial({i}) =  {factorial}");
            }
            Debug.WriteLine("");
            int n = 3;
            int k = 2;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            n = 4;
            k = 2;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            n = 45;
            k = 2;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            n = 52;
            k = 2;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            n = 52;
            k = 5;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            n = 52;
            k = 7;
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
            Debug.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");
            Debug.WriteLine("");
        }

        public static BigInteger Factorial (int n)
        {
            if(n < 1)
            {
                throw new IndexOutOfRangeException();
            }
            BigInteger factorial = 1;
            for(int i = 2; i <= n; i++)
            {
                factorial *= i;
            }
            return factorial;
        }
        public static int CombinationFact(int n, int k)
        {
            if(n < k)
            {
                throw new IndexOutOfRangeException();
            }
            if (n < 1 | k < 1)
            {
                throw new IndexOutOfRangeException();
            }
            if (n == k)
            {
                return 1;
            }
            int combination = (int)(Factorial(n) / Factorial(k) / Factorial(n - k));
            return combination;
        }
        public static int CombinationFast(int n, int k)
        {
            BigInteger combination = 1;
            //   n * (n-1) * (n-2) * (n-3) * (n - 4) ... 2 * 1/
            // / k * (k-1) ... * 2 * 1
            // / (n-k) * ... 2 * 1_
            for(int i = k+1; i <=n; i++)
            {
                combination *= i;
            }
            BigInteger denominator = 1;
            for (int i = 2; i <= (n - k); i++)
            {
                denominator *= i;
            }
            combination /= denominator;
            return (int)combination;
        }
    }
}

int is IMO not an appropriate type in this domain. I would use ulong or maybe BigInteger

If you for instance run the following test, both CombinationFact() and CombinationFast() runs out of range:

  n = 52;
  k = 9;
  Console.WriteLine($"Combin({n}, {k}) = {CombinationFact(n, k)}");
  Console.WriteLine($"Combin({n}, {k}) = {CombinationFast(n, k)}");

I would use throw new ArgumentOutOfRangeException(nameof(n));

instead of throw new IndexOutOfRangeException();

You don't need the extra variable combinations:

  int combination = (int)(Factorial(n) / Factorial(k) / Factorial(n - k));
              return combination;

Just return the result of the calculation:

  return (int)(Factorial(n) / Factorial(k) / Factorial(n - k));

Both of your algorithms is rather limited in use due to the int-conversion, but you could change the fast version to something like:

public ulong CombinationFast(ulong n, ulong k)
{
  ulong count = n;

  for (ulong x = 1; x <= k - 1; x++)
  {
    count *= (n - x);
    count /= x;
  }

  return count / k;
}

Update

Because 10!/(4!(10-4)!) == (10!/6!(10-6)!) it is possible to optimize the calculation for k > n/2:

public ulong CombinationFast(ulong n, ulong k)
{
  if (k > n / 2)
    k = n - k;

  ulong count = n;

  for (ulong x = 1; x <= k - 1; x++)
  {
    count *= (n - x);
    count /= x;
  }

  return count / k;
}

The above algorithms is an implementation of this expression.

A more literal version should maybe be:

public ulong CombinationFast(ulong n, ulong k)
{
  if (k > n / 2)
    k = n - k;

  ulong count = 1;

  for (ulong x = 0; x < k; x++)
  {
    count *= (n - x);
    count /= (x + 1);
  }

  return count;
}


Filtering out neighboring ascending digits in quaternary numeral system Take in a natural number n ∈ N, then process and output a(n), with a(n) being the number of quaternary strings (with 0, 1, 2, 3) of length n, where (from left to right) there never follows a character (i + 1) directly after the character i.

  For example, n=3 results in a(3) = 42: 000, 002, 003, 020, 021, 022, 030, 031, 032,
  033, 100, 102, 103, 110, 111, 113, 130, 131, 132, 133, 200, 202, 203,
  210, 211, 213, 220, 221, 222, 300, 302, 303, 310, 311, 313, 320, 321,
  322, 330, 331, 332, 333.

How can performance be improved? n = 13 already takes 4 seconds.

import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Scanner;

public class Quaternary {

    public static void start(){
        Scanner reader = new Scanner(System.in).useLocale(Locale.US);
        System.out.print("Enter number n: ");
        int n = Integer.parseInt(reader.nextLine());
        List<String> stringList = new ArrayList<String>();

        long startTime = System.nanoTime();

        for (int k=0; k < 4; k++){
        findString(n, k, "", stringList);
        }

        System.out.println(“Number of strings with n = " + n +  " : "+ stringList.size());

        long estimatedTime = System.nanoTime() - startTime;
        double time = (double)estimatedTime / 1000000000.0;
        System.out.println(time);
    }

    public static void findString(int n, Integer lastNumber, String string, List<String> stringList)
    {
        string += lastNumber.toString();

        if (string.length() == n)
        {
            stringList.add(string);
        }
        else {
            for (int i = 0; i < 4; i++)
            {
                if (i != lastNumber+1)
                {
                    findString(n, i, string, stringList);
                }
            }
        }
    }

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

We are dealing with a runtime upperbounded by something like 4^n here.

If you are building a list of strings anyway, why not reuse the list of valid strings in the list and append these values to a digit to get a new value?

public static void calculateQuarternary(int n) {

    int startPtr, endPtr;
    startPtr = endPtr = 0;

    ArrayList<String> alTemp = new ArrayList<String>();
    ArrayList<String> alCurrent = new ArrayList<String>();

    alCurrent.add("0");
    alCurrent.add("1");
    alCurrent.add("2");
    alCurrent.add("3");

    for (int i = 1; i < n; ++i) {

        endPtr = alCurrent.size();
        alTemp.clear();

        for (int j = 0; j < endPtr; ++j) {

            for (int k = 0; k < 4; ++k) {
                if (shouldAdd(alCurrent.get(j), k)) {
                    alTemp.add(k + alCurrent.get(j) + "");
                }
            }
        }

        alCurrent = new ArrayList<String>(alTemp);
    }
    System.out.println(alCurrent.size());
}

public static boolean shouldAdd(String s, int k) {

    int firstChar = (int)(s.charAt(0) - 48);
    return k + 1 != firstChar;
}


Merge two dict in separate list by id I have following data structure

>>> a = [{'id': 1, 'name': 'a'}, {'id': 2, 'name': 'b'}]
>>> b = [{'id': 1, 'age': 12}, {'id': 2, 'age': 21}]

I want to merge the dictionaries present in list b with list a by matching id. (a and b have the same number of entries, but the order can be different.)
So I want following:

[{'age': 12, 'id': 1, 'name': 'a'}, {'age': 21, 'id': 2, 'name': 'b'}]

I am doing it by following:

>>> for ta in a:
...   for tb in b:
...     if ta['id'] == tb['id']:
...       ta.update(tb)
...
>>> a
[{'age': 12, 'id': 1, 'name': 'a'}, {'age': 21, 'id': 2, 'name': 'b'}]
>>>

Is there a better Pythonic way of doing it?

You are saying

  I want to merge the dictionary present

but what makes your task a bit difficult is that you are not handling dictonnaries but lists. Among other things, if you keep that idea that way, you'll have to iterate over n*m pairs (ta, tb) (where n and m are respective size of a and b).

A probably better idea would be to reorganise data in a and b to have them as an actual dict mapping id to some additional information. Then, merging will go through maximum n + m elements.


National Insurance number generator I am trying to figure out how to DRY up this code:

nino = "QQ"
3.times { nino += Random.rand(10..99).to_s}
nino += ("A".."D").to_a.sample

I am trying to generate a sample National Insurance number starting with QQ, six digits and then ending with A, B, C or D.

Why do you need the 3 times? Is there a restriction, that the first number may not be 0?

If there  is no 0-restriction, you may generate a number between 0 and 999999.
To get the leading 0 I use the strftime-pattern %06i (6 digit with leading zeros).

  p "QQ%06i%s" % [ Random.rand(999999), ("A".."D").to_a.sample ]

For the letter, you may generate a random number from 65..68 and take the character for the selected ASCII-value (code example is ruby 1.9. Maybe you need a little variation with Ruby 1.8):

  p "QQ%06i%s" % [ Random.rand(999999), (65 + rand(4)).chr ]


Simple Configuration System I have created a simple ConfigManager in C# that loads each line of a file. If the line contains = then it gets the before = (the key) and the after = (the value).

For example, my config file would look something like this:

# Comment here
MyConfigKey1=MyValue1
MyConfigKey2=MyValue2
MyConfigKey3=MyValue3

Obviously my config file wouldn't be that, but that's the layout of it. If the file is unreadable or is not found then the defaults are used.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;

namespace Sirius.Core.Config
{
    internal sealed class ConfigManager
    {
        private readonly Dictionary<string, string> _configItems;

        public ConfigManager(string configFile)
        {
            _configItems = new Dictionary<string, string>();

            var logManager = Sirius.GetServer().GetLogManager();
            var stopwatch = Stopwatch.StartNew();

            try
            {
                if (File.Exists(configFile) && Sirius.GetServer().GetUtility().IsFileLocked(new FileInfo(configFile)))
                {
                    _configItems = File.ReadLines(configFile)
                        .Where(IsConfigurationLine)
                        .Select(line => line.Split('='))
                        .ToDictionary(line => line[0], line => line[1]);

                }
                else
                {
                    _configItems.Add("database.host", "localhost");
                    _configItems.Add("database.username", "root");
                    _configItems.Add("database.password", "");
                    _configItems.Add("database.name", "database");
                    _configItems.Add("database.port", "3306");
                    _configItems.Add("database.max_connections", "10000");
                    _configItems.Add("sockets.port", "30000");
                    _configItems.Add("sockets.backlog", "50");

                    logManager.Warn("Using the default configuration.");
                }
            }
            catch (Exception exception)
            {
                logManager.Error(exception.Message, exception);
            }
            finally
            {
                logManager.Information("Loaded Config Data [" + stopwatch.ElapsedMilliseconds + "ms]");
                stopwatch.Stop();
            }
        }

        private static bool IsConfigurationLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        public string GetConfigElement(string key)
        {
            string value;

            if (!_configItems.TryGetValue(key, out value))
            {
                Sirius.GetServer().GetLogManager().Error("Missing configuration key `" + key + "`");
            }

            return value;
        }
    }
}

The ConfigManager needs some clean-up because to my taste it's too tigthly coupled to the Sirius server and thus it's not testable.

First, make the constructor simpler. Constructors should not do any heavy job. Ideally the should only initialize fields or properties.

Move its logic into a new method like LoadFromOrDefault so that you know that it either loads the configuration from the specified file or gives you a default one that we move into a new DefaultSettings method.

The new LoadFromOrDefault not requries you to specify the file-name, a Func to determine whether the file is locked and a logger.

internal sealed class ConfigManager : IEnumerable<KeyValuePair<string, string>>
{
    private readonly Dictionary<string, string> _settings;
    private readonly ILogger _logger;

    public ConfigManager(IDictionary<string, string> settings, ILogger logger)
    {
        _settings = new Dictionary<string, string>(settings);
        _logger = logger;
    }

    public static IDictionary<string, string> DefaultSettings() => new Dictionary<string, string>
    {
        { "database.host", "localhost" },
        // ..
    };

    public static ConfigManager  LoadFlomOrDefault(string fileName, Func<string, bool> isLocked, ILogger logger)
    {
        var stopwatch = Stopwatch.StartNew();

        try
        {
            if (File.Exists(fileName) && isLocked(fileName))
            {
                var settings = File.ReadLines(fileName)
                    .Where(IsConfigurationLine)
                    .Select(line => line.Split('='))
                    .ToDictionary(line => line[0], line => line[1]);
                return new ConfigManager(settings, logger);
            }
            else
            {
                logger.Warn("Using the default configuration.");
                return new ConfigManager(DefaultSettings(), logger);
            }
        }
        catch (Exception exception)
        {
            logger.Error(exception.Message, exception);
            return null;
        }
        finally
        {
            logger.Information("Loaded Config Data [" + stopwatch.ElapsedMilliseconds + "ms]");
            stopwatch.Stop();
        }
    }

    private static bool IsConfigurationLine(string line)
    {
        return !line.StartsWith("#") && line.Contains("=");
    }

    public string GetConfigElement(string key)
    {
        string value;

        if (!_settings.TryGetValue(key, out value))
        {
            _logger.Error("Missing configuration key `" + key + "`");
        }

        return value;
    }

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

    public IEnumerator<KeyValuePair<string, string>> GetEnumerator()
    {
        return _settings.GetEnumerator();
    }
}

You can use it like this:

var config = ConfigManager.LoadFromOrDefault(
    "myconfig.ini",
    fileName => Sirius.GetServer().GetUtility().IsFileLocked(new FileInfo(configFile)),
    Sirius.GetServer().GetLogManager()
);

And agian, you can now test it without the Sirius server.

Desclimer: This is just a rough idea and not ready-to-use code so it might contain some small bugs or not entirely satisfy your needs yet.


Codefights subsequence I am solving https://codefights.com/challenge/Qjts7cukDvYpDW4Bc/main

My approach is simple.
Get all subsequences by combination.
Simple loop over combinations.
Find minimum.

closestSequence2 <- function(a, b) {
LA = length(a);
LB = length(b);
minimum = Inf;

indexes = utils::combn(1:LB, LA);

for(i in 1:ncol(indexes))
{
 suma = (sum(abs(b[indexes[,i]]-a)));
 if(suma < minimum )
 {
   minimum = suma;
 }
}
return(minimum);
}

I am not sure but I think it is failing due to the time because it pass the sample tests. Are there some easy tricks how to speed this up ot do I need different approach?

You can speed up your code by replacing the for loop with more efficient (vectorized) functions. Your code can then be reduced to a simple one-liner:

closestSequence2 <- function(a, b) min(colSums(abs(combn(b, length(a)) - a)))

Looking at it step-by-step:

combn is applied directly to b rather than 1:b. The output is a matrix where each column is a length(a)-long sub-sequence of b.
combn() - a takes advantage of R's recycling rules to compute, for each item in each sub-sequence, the signed distance to the corresponding element of a.
abs() converts to absolute (unsigned) distances.
colSums summarizes each column into a single value: the total distance from a for that sub-sequence.
min picks the minimum total distance across all candidates.

However, as pointed, an exhaustive search will not work well for large input vectors. A much faster approach is indeed to use dynamic programming. Here is a tentative implementation:

closestSequence2 <- function(a, b) {
  La <- length(a)
  Lb <- length(b)
  d <- abs(outer(a, b, FUN = `-`)) # matrix of all distances

  # x[1 + i, 1 + j] will hold the optimal distance between
  # the i first elements of a and the best sub-sequence
  # within the i+j first elements of b.
  x <- matrix(NA, La + 1, Lb - La + 1)
  x[ 1,  ] <- 0                             # init case where i = 0
  x[-1, 1] <- cumsum(d[cbind(1:La, 1:La)])  # init case where j = 0
  for (i in 1:La) {
     if (Lb > La) for (j in 1:(Lb - La)) {
        x[1 + i, 1 + j] <- min(x[1 + i, j],
                               x[i, 1 + j] + d[i, i + j])
     }
  }
  min(x[La + 1, ]) # min value on the last row
}


Freeing shared memory with a signal handler I have a homework question that asks me to get 2 integers from the user, put them in shared memory, fork a child, have the child add them and put the result in shared memory, and then the child will end and the parent will print out the sum.  The program must also loop until the user does CTRL+C, at which point it should catch the signal and the parent should detach and remove shared memory, SIGKILL the child, and then terminate itself.

I've already submitted this, and it got full credit, but I can't help but feel there is a better way.  Mainly, my concern is with the way I free the memory at the bottom of main() and, how I handle the signal.  I really wanted to free the memory in the signal handler, but couldn't figure out how to do that without making all the shared variables globals.  Those if (runFlag) statements don't seem particularly elegant to me either.

#define _POSIX_SOURCE

#include <stdio.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <string.h>
#include <sys/mman.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>

int runFlag = 1; //this flag will be set to 0 by a SIGINT

/* This function creates a shared memory area in the parent's address space of size size_of_shared_memory, and returns a void* pointer to it */
void *create_shared_memory(int size_of_shared_memory){
  int fd;
  void *area;

  fd = open("/dev/zero", O_RDWR);
  area = mmap(0, size_of_shared_memory, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
  close(fd);
  return area;
}

void intHandler()
{//toggle runFlag so main loop exits
    runFlag = 0;
    //shmdt(); //detaches shared memory
    //shmctl(shm_id, IPC_RMID, NULL); //removes shared memory

    return;
}

int main(int argc, char **argv)
{
    int *num1 = (int *)create_shared_memory(sizeof(int));
    int *num2 = (int *)create_shared_memory(sizeof(int));
    int *sum = (int *)create_shared_memory(sizeof(int));
    int *flag = (int *)create_shared_memory(sizeof(int));
    *flag = 1;
    int child_pid;

    while(runFlag)
    {//runFlag will be changed by a SIGINT
        //since runFlag could be changed anywhere we must test for it whereever the program will be sitting

        if (runFlag) printf("\nEnter the first number: ");
        if (runFlag) scanf("%d", num1);
        if (runFlag) printf("Enter the second number: ");
        if (runFlag) scanf("%d", num2);

        if (runFlag)
        {
            *flag = 1;
            child_pid = fork();
            signal(SIGINT, intHandler);

            if(child_pid < 0){
                printf("Fork error. \n");
                exit(-1);
            }
            if(child_pid == 0){ // Child code
                signal(SIGINT, SIG_IGN); //ignore ctrl+c
                *sum = *num1 + *num2;
                *flag = 0; //signal to parent that child is done
                exit(0);
            }

            /* Parent code */
            while (*flag); //wait for child to signal it is done

            /* Read the integers from shared memory */
            printf("The sum is: %d\n", *sum);
        }
    }
    //exit
    printf("\n");
    munmap(num1, sizeof(int)); //unmap memory
    munmap(num2, sizeof(int));
    munmap(sum, sizeof(int));
    munmap(flag, sizeof(int));
    if (child_pid > 0) kill(child_pid, SIGKILL); //kill child process
  exit(0); //kill parent
}

There are a lot of functions that aren't safe to call in a signal handler. For the few that are allowed, look at the Async-signal-safe functions section of the signal(7) man page.

Your current signal handler is almost okay, but any variable that might be changed from a signal handler needs to be declared volatile. Officially it must also be sig_atomic_t instead of int, though I'm not sure how important that is on real-world platforms.

You should be checking the return value of printf and scanf. They will return EOF (which is usually -1 but could theoretically be any negative number) with errno set to EINTR if the signal has been delivered during the underlying system call. That said, there is nothing to prevent the signal from being delivered before or after the syscall itself (before scanf is the nasty case, and doing the syscall yourself won't help unless the signal is blocked around the call).

If you want to handle signals in any sort of sane way, you need to sigblock them, at least some of the time. Either then unblock them during "safe" runs of code and check the flag periodically or else leave them blocked call sigwait (which will not invoke the handler).

My preference is to then use signalfd so that I can stuff it in a select or poll or epoll set along with any other read- or write- file descriptors.

There is no good reason for you to make 4 separate mmap calls. Most likely a single call for 4 * sizeof(int) would be enough, but even if you have a pressing need for them to be on separate pages, you should still mmap 4 pages in a single call.

You don't need to open "/dev/zero" to get anonymous memory, just pass -1 as the FD and add the MAP_ANONYMOUS flag.

Your child should call _exit, not exit.

PIDs should be stored in a variable of type pid_t.

Any memory that you read/write from multiple processes (or threads) must be protected either by some sort of mutex or by atomic operations with an appropriate memory order. It would not be enough to volatile.

So the following line may be optimized to be an infinite loop:

while (*flag);

Edit: It does indeed happen. Looking at the relevant assembly (at -O3):

# *flag = 1;
  400840:       mov    DWORD PTR [r12],0x1
# child_pid = fork();
  400848:       call   400720 <fork@plt>
  40084d:       mov    esi,0x4009c0
  400852:       mov    r14d,eax
  400855:       mov    edi,0x2
# signal(SIGINT, intHandler); - oh, this should be before the fork!
  40085a:       call   4006c0 <__sysv_signal@plt>
# 3-way branch for child_pid
  40085f:       test   r14d,r14d
  400862:       js     400885 <main+0x155>
  400864:       je     400897 <main+0x167>
# child_pid > 0
  400866:       mov    edx,DWORD PTR [r12]
# if (*flag) - condition only tested once
  40086a:       test   edx,edx
  40086c:       je     400870 <main+0x140>
# infinite loop
  40086e:       jmp    40086e <main+0x13e>
# else
  400870:       mov    esi,DWORD PTR [r13+0x0]
  400874:       mov    edi,0x400af2
  400879:       xor    eax,eax
# printf("The sum is: %d\n", *sum);
  40087b:       call   400680 <printf@plt>
  400880:       jmp    400772 <main+0x42>
# child_pid < 0
  400885:       mov    edi,0x400ae5
# printf("Fork error. \n");
  40088a:       call   400660 <puts@plt>
  40088f:       or     edi,0xffffffff
# exit(-1); - should use 1 or EXIT_FAILURE actually.
  400892:       call   400710 <exit@plt>
# child_pid == 0
  400897:       mov    edi,0x2
  40089c:       mov    esi,0x1
# signal(SIGINT, SIG_IGN); //ignore ctrl+c
  4008a1:       call   4006c0 <__sysv_signal@plt>
  4008a6:       mov    eax,DWORD PTR [rbx]
  4008a8:       add    eax,DWORD PTR [rbp+0x0]
  4008ab:       xor    edi,edi
# *sum = *num1 + *num2;
  4008ad:       mov    DWORD PTR [r13+0x0],eax
# *flag = 0;
  4008b1:       mov    DWORD PTR [r12],0x0
# exit(0);
  4008b9:       call   400710 <exit@plt>


Append a incrementing number to a slug username Im using Laravel 5.4 and I'm using a slug to look-up users in database.

Im adding first_name and last_name to the slug, and if there is a user with the same name, appending a number (increment it if there is more than one with same name) to the end ex John.doe.3.

This is what I have in RegisterController.php

protected function create(array $data)
    {
        // Set the may be name
        $maybe_slug = strtolower($data['first_name'] . '.' . $data['last_name']);

        // Check if there is another one in the DB with the same name
        $user = User::where('slug', 'like', $maybe_slug . '%')->orderBy('created_at', 'desc')->first();

        if($user){
            $split_user_name = explode('.', $user->slug);
            $number = isset($split_user_name[2]) ? $split_user_name[2] : null;
            if(isset($number)){
                $slug = $maybe_slug . '.' . ($number + 1);
            }elseif($user->slug == $maybe_slug){
                $slug = $maybe_slug . '.2';
            }
        }else{
                $slug = $maybe_slug;
        }
        return User::create([
            'first_name' => $data['first_name'],
            'last_name' => $data['last_name'],
            'slug' => $slug,
            'email' => $data['email'],
            'password' => bcrypt($data['password']),
        ]);
    }

This is how RegisterController looks like un-edited (GitHub)

Can I do this in a better way?

Searching for the slug duplicate in the database can be done in a more fast and straightforward way, by using = operator instead of like, and not using orderBy;
To be on the safe side, it's worth to make searches in a loop, incrementing the $next on each step;

So, it might look like this:

protected function create(array $data)
{
    $slug = $maybe_slug = strtolower($data['first_name'] . '.' . $data['last_name']);
    $next = 2;

    while (User::where('slug', '=', $slug)->first()) {
        $slug = "{$maybe_slug}.{$next}";
        $next++;
    }

    return User::create([
                'first_name' => $data['first_name'],
                'last_name' => $data['last_name'],
                'slug' => $slug,
                'email' => $data['email'],
                'password' => bcrypt($data['password']),
    ]);
}


Python function to hash a python function I'm working on a sort of distributed build system. The system allows execution of snippets of scripts as build steps. I need to be able to hash these snippets of code in a way that comments and doc strings don't affect the hash. I'm getting part way there by using the ast module to parse the code, then doing an ast.dump and hashing the resulting string. The natural next step for me is to clear the first Expr(str()) node in the body of every FunctionDef node.

This is the best solution I've found so far.

Is there a better way to solve this problem?

import ast
import hashlib
import inspect

def _remove_docstring(node):
    '''
    Removes all the doc strings in a FunctionDef or ClassDef as node.
    Arguments:
        node (ast.FunctionDef or ast.ClassDef): The node whose docstrings to
            remove.
    '''
    if not (isinstance(node, ast.FunctionDef) or
            isinstance(node, ast.ClassDef)):
        return

    if len(node.body) != 0:
        docstr = node.body[0]
        if isinstance(docstr, ast.Expr) and isinstance(docstr.value, ast.Str):
            node.body.pop(0)

#-------------------------------------------------------------------------------
def hash_function(func):
    '''
    Produces a hash for the code in the given function.
    Arguments:
        func (types.FunctionObject): The function to produce a hash for
    '''
    func_str = inspect.getsource(func)
    module = ast.parse(func_str)

    assert len(module.body) == 1 and isinstance(module.body[0], ast.FunctionDef)

    # Clear function name so it doesn't affect the hash
    func_node = module.body[0]
    func_node.name = ""

    # Clear all the doc strings
    for node in ast.walk(module):
        _remove_docstring(node)

    # Convert the ast to a string for hashing
    ast_str = ast.dump(module, annotate_fields=False)

    # Produce the hash
    fhash = hashlib.sha256(ast_str)
    result = fhash.hexdigest()
    return result

#-------------------------------------------------------------------------------
# Function 1
def test(blah):
    'This is a test'
    class Test(object):
        '''
        My test class
        '''
    print blah
    def sub_function(foo):
        '''arg'''
print hash_function(test)

#-------------------------------------------------------------------------------
# Function 2
def test2(blah):
    'This is a test'
    class Test(object):
        '''
        My test class
        '''
    print blah
    def sub_function(foo):
        '''arg meh'''
print hash_function(test2)

Looking at the source code for ast.get_docstring your code more or less does what it does. Here is their code for reference:

def get_docstring(node, clean=True):
    """
    Return the docstring for the given node or None if no docstring can
    be found.  If the node provided does not have docstrings a TypeError
    will be raised.
    """
    if not isinstance(node, (AsyncFunctionDef, FunctionDef, ClassDef, Module)):
        raise TypeError("%r can't have docstrings" % node.__class__.__name__)
    if not(node.body and isinstance(node.body[0], Expr)):
        return
    node = node.body[0].value
    if isinstance(node, Str):
        text = node.s
    elif isinstance(node, Constant) and isinstance(node.value, str):
        text = node.value
    else:
        return
    if clean:
        import inspect
        text = inspect.cleandoc(text)
return text

A couple of the things they mention are only a concern for Python 3+ like AsyncFunctionDef. Also, it seems Constant is a grammar rule introduced in Python 3+. (However, you may want to account for this in the future.)

You seem to be also removing docstrings from ClassDef, I am not one hundred percent sure, but you may also want to add Module as well.

There is also a ast.NodeTransformer. However, I find it rather weird stylistically compared to your current method of looping. So... I'm not confident in making any comments about whether you should be using it or not. Nevertheless, I'm putting it out there so you are aware in case you weren't.


Destructively appending one list to another I am implementing a list structure in C. My current function I am working on is a destructive append function that takes two lists and appends the second one onto the end of the first. Right now, it appears to be working without runtime errors and memory leaks and I think I have all my cases covered, but when I submit it to my testing server, it says it is not correct. I was wondering if anyone could see any flaws that could cause it to not do what I expect it to do.

The append function:

ilist iappend_destroy(ilist il1, ilist il2){
   if(il1 == NULL && il2 == NULL){
      free(il1);
      free(il2);
      return NULL;
   }else if(il1 == NULL){
      free(il1);
      return(il2);
   }else if(il2 == NULL){
      free(il2);
      return(il1);
   }else{

   ilist tmp = iempty();
   ilist clone = il1;

   while(il1 != NULL){
      tmp = icons_destroy(il1->first, tmp);
      il1 = il1->rest;
   }

   ilist tmpclone = tmp;

   while(tmp != NULL){
      il2 = icons_destroy(tmp->first, il2);
      tmp = tmp->rest;
   }

   idelete(tmpclone);
   idelete(clone);
   return il2;
   }

The declaration of the ilist structure:

struct ilist_ADT{
    struct ilist_ADT *rest;
    int first;
};

The icons_destroy function:

ilist icons_destroy(int in, ilist il){
   if (il == NULL) {
      ilist anewlist = malloc(sizeof(struct ilist_ADT));
      anewlist->first = in;
      anewlist->rest = NULL;
      return (anewlist);
   } else {
      ilist previous = malloc(sizeof(struct ilist_ADT));
      previous->first = il->first;
      previous->rest = il->rest;
      il->first = in;
      il->rest = previous;
      return il;
   }
}

Your temporary variable names are horrible and give a false impression of what they are doing.

ilist tmp = iempty();   // Is this really a temporary and a temporary of what.
                       // Looks like you are building a copy of il1 onto this list.

ilist clone = il1;      // This is not a clone of il1
                       // It is basically a pointer at the same head node as il1.
                       // I would have called this one il1Iter or loop1 then changed this
                       // value in the loop below thus leaving il1 pointing at the head.

ilist tmpclone = tmp;   // Same comment as for clone.

// Looks like you are copying the copy you just made onto the end of il2.
// This seems like a waste as you are doing two copies of each node.
while(tmp != NULL){
    il2 = icons_destroy(tmp->first, il2);
    tmp = tmp->rest;
}
// A deeper inspection of icons_destroy() finds that when you are
// actually making a copy of the list in reverse order by always inserting
// to the front.

// Thus copying il1 (to reverse it) and then copying it again (to put it back
// in the correct order) with il2 actually puts a copy of il1 in-front of il2.
// Not a very obvious way of implementing the append and as such I would
// suggest you find a way to re-write that so it is obvious what is happening.

// Seems correct but because of the bad naming convention it is hard to tell
// we actually need to go back and read the rest of the code to make sure this
// is correct in context.
idelete(tmpclone);
idelete(clone);


Reverse a string word by word Given an input string, reverse the string word by word.
For example: Given s = "the sky is blue", return "blue is sky the".
What constitutes a word?

A sequence of non-space characters constitutes a word.

Could the input string contain leading or trailing spaces?

Yes. However, your reversed string should not contain leading or trailingspaces.

How about multiple spaces between two words?

Reduce them to a single space in the reversed string.

public static String reverseWords(String s){
    StringTokenizer strTok = new StringTokenizer(s, " ");
    Stack<String> stack=new Stack<String>();
    StringBuilder buff=new StringBuilder();
    while(strTok.hasMoreElements()) {
        String str = (String)strTok.nextToken();
        if(!str.equals("")) stack.push(str);
    }
    while(!stack.isEmpty()){
        buff.append(stack.pop());
        if(!stack.isEmpty()) buff.append(" ");
    }
    return buff.toString();
 }

Stack seems more or less deprecated. Javadoc says the following:

  A more complete and consistent set of LIFO stack
  operations is provided by the Deque interface and its
  implementations, which should be used in preference to this class.

I guess an ArrayDeque is a good choice here.
It's seems that StringTokenizer is also deprecated. Javadoc says the following:

  StringTokenizer is a legacy class that is retained for
  compatibility reasons although its use is discouraged in new code.
  It is recommended that anyone seeking this functionality use the
  split method of String or the java.util.regex package instead.

I'd rename

buff to result
stack to words
str to word

to describe their purpose. It would be easier to read and help understanding code.
You could move the declaration of buff right before the second while loop.

StringBuilder buff = new StringBuilder();
while (!stack.isEmpty()) {
    buff.append(stack.pop());
    if (!stack.isEmpty())
        buff.append(" ");
}

Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

if(!str.equals(""))

You could use String.isEmpty() here. It's easier to read since it says in a similar language to English what does this condition do.

if(!str.equals("")) stack.push(str);

According to the Code Conventions for the Java Programming Language,

  if statements always use braces {}.

Omitting them is error-prone. I've found the one-liner above hard to read because if you scan the code line by line it's easy to miss that at the end of the line there is a statement (push).
It's good to know that there is a similar function in Apache Commons Lang: StringUtils.reverseDelimited. You can check the implementation online for other ideas.

(See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

Here is the modified code with the Deque and other suggestions:

public static String reverseWords(String input) {
    Deque<String> words = new ArrayDeque<>();
    for (String word: input.split(" ")) {
        if (!word.isEmpty()) {
            words.addFirst(word);
        }
    }
    StringBuilder result = new StringBuilder();
    while (!words.isEmpty()) {
        result.append(words.removeFirst());
        if (!words.isEmpty()) {
            result.append(" ");
        }
    }
    return result.toString();
}


Using sounds in Java I'm trying to apply sound today, and the goal in mind was a simple violin tuner. The actionListener seems repetitive/  How might I optimize it (what I tried just broke everything)? I also noticed the sounds bleed into each other.  I don't mind that so much for the different keys (though I'm thinking of a toggle option for this) but it also is the case for the same key, which is undesirable.  I'm not sure how to handle that.

/* Author: Luigi Vincent
A simple violin tuner, emits G, D, A, and E sounds to facilitate violin tuning
*/

import javax.swing.*;
import javax.sound.sampled.*;
import java.awt.*;
import java.awt.event.*;
import java.io.*;

public class ViolinTuner {
    public static void main(String[] args) {
        JFrame frame = new JFrame("Violin Tuner");
        // get and set icon for the program
        ImageIcon icon = new ImageIcon("Images/Icon.png");
        frame.setIconImage(icon.getImage());
        // Buttons
        final JButton G_KEY = new JButton("G");
        final JButton D_KEY = new JButton("D");
        final JButton A_KEY = new JButton("A");
        final JButton E_KEY = new JButton("E");
        // Listener to play sounds on click
        ActionListener violinSounds = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                if (e.getSource() == G_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/G.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == D_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/D.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == A_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/A.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == E_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/E.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    } catch(Exception x) { x.printStackTrace(); }
                }
            }
        };
        // Register buttons to listener
        G_KEY.addActionListener(violinSounds);
        D_KEY.addActionListener(violinSounds);
        A_KEY.addActionListener(violinSounds);
        E_KEY.addActionListener(violinSounds);
        // buttons to panel
        JPanel p = new JPanel();
        p.add(G_KEY);
        p.add(D_KEY);
        p.add(A_KEY);
        p.add(E_KEY);
        // Panel to frame
        frame.add(p);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(225, 75);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }
};

Layout

Calling .setSize() with fixed dimensions isn't portable.  For example, this is what happens on Mac OS X:

Instead, call frame.pack() to automatically set the frame size according to its contents.

If you want the 2 × 2 layout, then use a suitable LayoutManager — for example, p.setLayout(new GridLayout(2, 0, 10, 6));

Don't Repeat Yourself

The code is awfully repetitive.  You should avoid repeating yourself, since repetition makes maintenance difficult: cut-and-paste programming leads to cut-and-paste bugs.  I recommend using an Enum to represent the four notes.

Explicitness

It's bad practice to catch (Exception x).  I have no clue what kind of exception would likely cause the flow of execution to jump to the exception handler.  You, as the author, might intend for certain exceptions to be caught, but you don't know what will actually happen.  Starting with Java 7, you can list multiple exceptions to be handled by the same catch block, and that's what I recommend that you do.

Avoid overuse of wildcard imports.  There exists a small probability that a future version of some package could add a class that clashes and breaks your program.  Mostly, using too many wildcard imports makes it hard for human maintainers to resolve identifiers — the compiler may be good at searching the classpath, but I'm not.  My suggested guidelines are:

It's OK to use wildcard imports with the standard Java packages that almost every Java programmer is familiar with (such as java.io.* or java.util.*).
It's OK to use one wildcard import per file, maybe two, but not much more.
If in doubt, import individual classes explicitly.
List your imports alphabetically.

By the way, you never used anything in java.awt.*.

Suggested implementation

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import java.io.IOException;
import javax.sound.sampled.*;
import javax.swing.BorderFactory;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.ImageIcon;
import javax.swing.SwingUtilities;

public class ViolinTuner {
    private static enum Note {
        G, D, A, E;

        public final File file;

        Note() {
            this.file = new File("Sounds", this.name() + ".wav").getAbsoluteFile();
        }
    }

    private static JButton makeJButton(final Note note) {
        JButton button = new JButton(note.name());
        button.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                try {
                    AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(note.file);
                    Clip clip = AudioSystem.getClip();
                    clip.open(audioInputStream);
                    clip.start();
                } catch (IOException | UnsupportedAudioFileException | LineUnavailableException x) {
                    x.printStackTrace();
                }
            }
        });
        return button;
    }

    private static void createAndShowGUI() {
        JFrame frame = new JFrame("Violin Tuner");
        frame.setIconImage(new ImageIcon("Images/Icon.png").getImage());

        // buttons to panel
        JPanel notePanel = new JPanel(new GridLayout(2, 0, 8, 4));
        notePanel.setBorder(BorderFactory.createEmptyBorder(4, 8, 4, 8));
        for (Note note : Note.values()) {
            notePanel.add(makeJButton(note));
        }

        // Component to impose a minimum width on the JFrame to ensure that the
        // window title is not truncated
        JPanel widener = new JPanel();
        widener.setPreferredSize(new Dimension(250, 0));

        // Panels to frame
        frame.setLayout(new BorderLayout());
        frame.add(notePanel, BorderLayout.CENTER);
        frame.add(widener, BorderLayout.SOUTH);
        frame.pack();

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                createAndShowGUI();
            }
        }
    }
}


Caesar-Cipher - 2nd follow-up I've now used the tips, you can find here and there, to improve my code:

import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import java.awt.Dimension;
public class CaesarCipher {

    enum WhatToDo {ENCRYPT, DECRYPT};

    public static void main(String[] args) {

        String UserInput = JOptionPane.showInputDialog("Please enter text:");
        String text = UserInput.replaceAll("[^a-zA-Z]+", "");
        text = text.toUpperCase();

        String message = "Please enter shift to the right:";
        int shift = findShift(message);

        String out = "";
        message = "Encrypt or decrypt?";

        out = MakeDecision(message, text, shift);

        JTextArea msg = new JTextArea(out);
        msg.setLineWrap(true);
        msg.setWrapStyleWord(true);
        JScrollPane scrollPane = new JScrollPane(msg);
        scrollPane.setPreferredSize(new Dimension(300,300));
        JOptionPane.showMessageDialog(null, scrollPane);
    }

    public static int findShift(String msg) {
        String UserInput = JOptionPane.showInputDialog(msg);
        int shift;
        try {
           shift = Integer.parseInt(UserInput);
        }
        catch (NumberFormatException e) {
            shift = findShift("Please enter shift as a number:");
        }

        return shift;
    }

    public static String MakeDecision(String message, String text, int shift) {

        String UserInput = JOptionPane.showInputDialog(message);
        UserInput = UserInput.toUpperCase();
        String out = "";

        if(WhatToDo.ENCRYPT.name().equals(UserInput) == true) {
        boolean test = true;
        out = EncryptOrDecrypt(text, shift, test);
        }

        else if(WhatToDo.DECRYPT.name().equals(UserInput) == true) {
            boolean test = false;
            out = EncryptOrDecrypt(text, shift, test);

        }
        else {
            message = "Illegal Choice! Encrypt or decrypt?";
            out = MakeDecision(message, text, shift);
        }

        return out;

    }

    //Encryption
    public static String EncryptOrDecrypt(String text, int n, boolean test) {
        int count = 0;
        int alphabetLength = 0;
        int decryptFactor = 1;

        if(!test) {
            decryptFactor = -1;
        }

        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (count < text.length()) {
            final char currentChar = text.charAt(count);
            if (currentChar >= 'A' && currentChar <= 'Z') {
                if (currentChar + n > 'Z') {
                    alphabetLength = 26;
                }
                out.append((char)(currentChar + (n*decryptFactor) - (alphabetLength*decryptFactor)));
            }
            else {
                out.append(currentChar);
            }
            count++;
            alphabetLength = 0;
        }
        return out.toString();
    }

}

Do you think that this now is good code? Do you have any tips for further improvement?

I'm going to copy and paste snippets from your other reviews that you havne't implemented as well as my own review:

  shortened names like out or d don't really have any benefit over
  slightly longer, clearer ones

You still use the exact name "out" in your code:

String out = "";

  stick with code style guides

Java in particular has common naming standards. You should use lowerCamelCase for class & method variables such as:

String UserInput

You may even notice StackExchange has given it different highlighting. Normally UpperCamelCase is used for classes.

while (count < ...

I'd suggest using a for-each loop here instead since you never use the 'count' variable

WhatToDo

I really don't like this name. I missed it at the top and it really surprised me when I first saw it used. "WhatToDo" Is not a good name.

"test" is also a really bad name. There are lots of information available online & on this site about variable namings. As a general rule, try to look at the name by itself and see if it's at all descriptive.

I don't see the point in declaring the variable "test". Just pass a boolean true/false directly to the method.


Enabling or disabling input fields In my Rails app, I have a form.  The form has radio buttons for entity type and has a text field for SSN and another for EIN. Based on the radio button selected, one of the text fields will be disabled and the other enabled (disabled = false).

If the user selects the radio button for entity_type_i, it should disable the EIN field, and enable SSN.  For the other four radio buttons, it should do the opposite - disable SSN and enable EIN.  Since I don't know JavaScript, I ended up copying and pasting the same code and just changing the name of the id (i.e.
#user_entity_type_c).  Basically #user_entity_type_c, #user_entity_type_p, #user_entity_type_l and #user_entity_type_o should behave the same.

Without knowing any better, the code seems like it's redundant.  Is there a more efficient way to write the JavaScript?

Radio buttons for the form:

    <input class="radio_buttons required" required="required" aria-required="true" type="radio" value="I" name="user[entity_type]" id="user_entity_type_i" />
    Individual / Sole Proprietor

    <input class="radio_buttons required" required="required" aria-required="true" type="radio" value="C" name="user[entity_type]" id="user_entity_type_c" />
    Corporation

    <input class="radio_buttons required" required="required" aria-required="true" type="radio" value="P" name="user[entity_type]" id="user_entity_type_p" />
      Partnership

    <input class="radio_buttons required" required="required" aria-required="true" type="radio" value="L" name="user[entity_type]" id="user_entity_type_l" />
    Limited Liability Company

     <input class="radio_buttons required" required="required" aria-required="true" type="radio" value="O" name="user[entity_type]" id="user_entity_type_o" />
       Other

The JavaScript:

From your code it looks like you're changing one of "enabled" options only in one of the five cases. You can do that with a loop, using a class selector.

Also, it may be just me, but I don't think you should use value in an input box / radio button to store something different than the default value that you want that item to have. I suggest changing it to something like value_type.

So, what you can do is add another class to your input elements, select by that and do what you need, something like:

$(function() {
    $('input.entity-option').change(function() {
    if (this.value_type === "I") {
        $("#user_ssn").prop("disabled", false);
        $("#user_ein").prop("disabled", true);
      } else {
        $("#user_ssn").prop("disabled", true);
        $("#user_ein").prop("disabled", false);
      }
    });
  });

And in your HTML you need something like:

    <input class="radio_buttons required entity-option" required="required" aria-required="true" type="radio" value_type="I" name="user[entity_type]" id="user_entity_type_i" />
    Individual / Sole Proprietor

    <input class="radio_buttons required entity-option" required="required" aria-required="true" type="radio" value_type="C" name="user[entity_type]" id="user_entity_type_c" />
    Corporation

    <input class="radio_buttons required entity-option" required="required" aria-required="true" type="radio" value_type="P" name="user[entity_type]" id="user_entity_type_p" />
      Partnership

    <input class="radio_buttons required entity-option" required="required" aria-required="true" type="radio" value_type="L" name="user[entity_type]" id="user_entity_type_l" />
    Limited Liability Company

     <input class="radio_buttons required entity-option" required="required" aria-required="true" type="radio" value_type="O" name="user[entity_type]" id="user_entity_type_o" />
       Other


Binomial coefficient I wrote a C++ function to calculate the binomial coefficient, trying to avoid overflows as much as possible.

/*!
 * @brief Calculates the binomial coefficient indexed by n and k
 *
 * This implementation is based on the algorithm described here: http://blog.plover.com/math/choose.html
 * and the relevant follow-up: http://blog.plover.com/math/choose-2.html
 *
 * @param n                     The number of elements
 * @param k                     The number of elements in each subset
 * @return                      The number of k-element subsets in a n-element set
 *
 * @throw std::overflow_error   If the computation caused an overflow
 */
uint64_t binomial_coefficient(uint64_t n, uint64_t k)
{
    if (k > n)
        return 0;

    if (n - k < k)
        k = n - k;

    uint64_t r = 1;

    for (uint64_t d = 1; d <= k; d++)
    {
        uint64_t mult = n;

        bool divided = true;

        if (mult % d == 0)
            mult /= d;
        else if (r % d == 0)
            r /= d;
        else
            divided = false;

        const uint64_t r_mult = r * mult;
        if (r_mult / mult != r)
            throw std::overflow_error("Overflow");

        r = r_mult;

        if (!divided)
            r /= d;

        n--;
    }

    return r;
}

I have implemented the algorithm described here with a slight modification (try to divide n or r by d before multiplying n by r) as described in the article follow-up.

I also added an overflow check, which I believe should be reliable, as unsigned arithmetic overflow should not yield undefined behaviour.

Do you see any blatant or subtle bug in my implementation, and/or any room for improvement (besides lookup tables for common values and alike, which I am not willing to pursue at the moment)?

Mostly minor things, but could still be helpful:

Functions should generally be named in a verb form, so rename the function to something like calculate_binomial_coefficient().
This is a little hard to read:

if (r_mult / mult != r)

and could use more parenthesis for clarity:

if ((r_mult / mult) != r)

Consider using curly braces for these single-line conditional statements.  It could also help with avoiding some additional bugs, if you end up adding anything to them.
Since this is C++ and not C, use std::uint64_t.  Try to use all the equivalents that are found in the std namespace.


Removing a specific key of ModelState for a specific controller's method I have a controller for Post and Put request for the same entity: MyModelDTO with some ModelState's attributes.

The users can make a Post request without the Id attribute, because it is being generated on server side, but for Put request the Id attribute is needed (And has to be different than an empty guid).

I want to allow get a Post request without the "Id" property, and still use the ModelState functionality, the best solution I've came across is overriding the OnActionExecuting and removing the specific key:

public class ExceptPropertiesAttribute : ActionFilterAttribute
{
    private IEnumerable<string> _propertiesKeys;

    public ExceptPropertiesAttribute(string commaSeperatedPropertiesKeys)
    {
        if (!string.IsNullOrEmpty(commaSeperatedPropertiesKeys))
        {
            this._propertiesKeys = commaSeperatedPropertiesKeys.Split(',');
        }
    }

    public override void OnActionExecuting(ActionExecutingContext actionContext)
    {
        if (this._propertiesKeys != null)
        {
            foreach (var propertyKey in this._propertiesKeys)
            {
                if (actionContext.ModelState.ContainsKey(propertyKey))
                {
                    actionContext.ModelState.Remove(propertyKey);
                }
            }
        }
    }
}

MyModelDTO class:

public class EventModelDTO : IBaseModel
{
    [IsNotEmpty(ErrorMessage = "Guid Id Is Empty")]
    public Guid Id { get; set; }
    [Required]
    public string Name { get; set; }
}

public class IsNotEmptyAttribute : ValidationAttribute
{
    public override bool IsValid(object value)
    {
        if (value == null) return false;

        var valueType = value.GetType();
        var emptyField = valueType.GetField("Empty");

        if (emptyField == null) return true;

        var emptyValue = emptyField.GetValue(null);

        return !value.Equals(emptyValue);
    }
}

Usage:

[HttpPost]
[ExceptPropertiesAttribute("Id")]
public ActionResult Post([FromBody] MyModelDTO itemDTO = null)
{
    try
    {
        if (!ModelState.IsValid)
        {
            return BadRequest();
        }

        MyModel item = _mapper.Map<MyModel>(itemDTO);
        ResultService result = this._myService.Insert(item);
        if (result.Success)
        {
            return CreatedAtAction("Post", new { item.Id }, itemDTO);
        }
        else
        {
            return StatusCode(500, result.ErrorCode);
        }
    }
    catch (Exception ex)
    {
        return StatusCode(500, ErrorCode.Unknown);
    }
}

[HttpPut("{id}")]
public ActionResult Put([FromBody] MyModelDTO itemDTO)
{
    try
    {
        if (!ModelState.IsValid)
        {
            return BadRequest();
        }

        MyModel item = _mapper.Map<MyModel>(itemDTO);
        ResultService result = this._eventsService.Update(item);
        if (result.Success)
        {
            return Ok();
        }
        else
        {
            return StatusCode(500, result.ErrorCode);
        }
    }
    catch (Exception ex)
    {
        return StatusCode(500, ErrorCode.Unknown);
    }
}

While your approach might work, I consider it a hack. The preferred solution would be to create different models for each request so that the user does not have a chance of passing values that might not only not be required but even be forbidden.

Your new entities could be:

class NewUser
{
    public string Name { get; set; }
}

Where there are is no Id if the AIP does not require it.

It is a very bad idea to expose your database models to the caller. Create more specific models and map their properties to your database entities.

If you let the user the possibility of passing anything he wants, you would need to validate even more values to be sure he does not try to do something else than you want him to.

One such example could be when you have an API for updating User.Name but that user has a navigation property Books and the caller sends you some books too that will also get updated... just because he can. This might cause you a lot of trouble.


Time Machine service Recently I had someone describe a "Time Machine" service to me; that is, a service that could be used to change how a chunk of code perceives time.

Normally through calls to DateTime.Now and other DateTime statics, time is directly tied to the system clock. A Time Machine service is used instead of the static DateTime calls, and allows external code to change the time it reports. This is especially useful when writing unit tests for services that behave different as time passed due to timers, triggers, etc.

I liked the idea, and decided to implement my own version. Here are the relevant interfaces:

// A service that can travel through time
public interface ITimeTraveler
{
    ITimeMachine TimeMachine { get; set; }
}

// Interface for a Time Machine
public interface ITimeMachine
{
    DateTime Now();
    DateTime UtcNow();
    DateTime Today();
    DateTime UtcToday();
    void TimeTravel(TimeSpan adjustment);
    void TimeTravelTo(DateTime newDateTime);
    void FreezeTime();
    void UnfreezeTime();
    void RevertAllTimeTravel();
    bool IsCurrentlyTimeTraveling();
}

Here is the Time Machine Gist with the full implementation.

This includes two revisions. The first revision was the code I linked to originally, and additional revisions take the feedback here into consideration.

I'm looking for any feedback or suggestions on both the concept of this service and my current implementation.

I like it.

For some very specific cases where a class needs to be tested and its functionality depends on what System.DateTime.Now returns, then you need an abstraction to wrap the static method calls, if you want to be able to write unit tests that have full control over that dependency.

Your ITimeMachine does that nicely. While a meaningful name, IDateTimeService seems more appropriately generic though. ITimeTraveler might be superfluous.

The dependent code would look like this:

public class MyClass
{
    private readonly IDateTimeService _service;

    public MyClass(IDateTimeService service)
    {
        _service = service;
    }

    public void DoSomething()
    {
        var now = _service.Now(); // instead of DateTime.Now;
        //...
    }
}

I also like that you made Now() a method. It abstracts away the fact that DateTime.Now is really a method in a property suit.


JSON API call in an Angular service I have followed this format in my application to call the API and get data from it:

getCheckoutDetails(): Observable<UserDetail> {
    let query = `7668`;
    return this.http
        .get(this.appConfig.getAPIUrl()
            + `/buy/getDetails?${query}`)
        .map(this.extractData)
        .catch(this.handleErrors);
}

private extractData(res: Response) {
    let data = res.json();
    return data.body ? data.body.message ? data.body.message : {} : {};
}

private handleErrors(error: Response | any) {
    let errMsg: string;
    if (error instanceof Response) {
        const body = error.json() || '';
        const err = body.error || JSON.stringify(body);
        errMsg = `${error.status} - ${error.statusText || ''} ${err}`;
    } else {
        errMsg = error.message ? error.message : error.toString();
    }
    console.error(errMsg);
    return Observable.throw(errMsg);
}

This is sample code on one service. Now this has been followed in all services to handle data and error. When I run gulp cpd command to detect the duplicate code, it lists down all the files. Is there a way to handle this without duplicating?

Create one httpservice and extend it in your service, for example:

HttpService:

export class HttpService  {
    protected userInfo:IUserModel;

    constructor(public http: Http, public user: User) {
        this.userInfo = this.user.getInfo();
    }

    /**
     * method http get
     * @param url
     * @param params
     * @returns {Http}
     */
    fetch(url, params, addData) {
        let headers = new Headers();

        headers.append('Content-Type', 'application/json');
        headers.append('User-Id',  this.userInfo.id);
        headers.append('Token',  this.userInfo.accessToken);

        if(addData.accessType) {
            headers.append('Access-Type', '1');
        }

        if(addData.limit) {
            headers.append('Offset-Step',  addData.limit);
        }

        if(addData.page || addData.page === 0) {
            let count = (parseInt(addData.page )- 1) * parseInt(addData.limit);
            headers.append('Count',  count.toString());
        }

        let options = new RequestOptions(
            {
                headers: headers,
                search: params,
            });

        return this.http
            .get(
                url,
                options
            )
             .map(this.extractData)
            .catch(this.handleErrors);
    }

    /**
     * method http post
     * @param url
     * @param data
     * @returns {Http}
     */
    send(url, data) {

        let body = JSON.stringify(data);
        let headers = new Headers({
            'Content-Type': 'application/json',
            'User-Id':  this.userInfo.id,
            'Token':  this.userInfo.accessToken
        });

        let options = new RequestOptions({ headers: headers });

        return this.http.post(url, body, options)
             .map(this.extractData)
            .catch(this.handleErrors);
    }

    /**
     * method http put
     * @param url
     * @param data
     * @returns {Http}
     */
    stick(url, data) {
        let body = JSON.stringify(data);

        let headers = new Headers();

        headers.append('Content-Type', 'application/json');

        headers.append('User-Id',  this.userInfo.id);
        headers.append('Token',  this.userInfo.accessToken);

        return this.http
            .put(url, body, {headers:headers})
            .map(this.extractData)
            .catch(this.handleErrors);
    }

    /**
     * method http delete
     * @param url
     * @param params
     * @returns {Http}
     */
    remove(url, params) {
        //var data = Object.keys(params).map(function(k) {
        //    return encodeURIComponent(k) + '=' + encodeURIComponent(params[k])
        //}).join('&');
        //url += '?' + data;

        let headers = new Headers();

        headers.append('Content-Type', 'application/json');

        headers.append('User-Id',  this.userInfo.id);
        headers.append('Token',  this.userInfo.accessToken);

        for(var i in params) {
            headers.append(i,  params[i]);
        }

        return this.http.delete(url, {headers:headers})
             .map(this.extractData)
            .catch(this.handleErrors);
    }

    private extractData(res: Response) {
        let data = res.json();
        return data.body ? data.body.message ? data.body.message : {} : {};
    }

    private handleErrors(error: Response | any) {
        let errMsg: string;
        if (error instanceof Response) {
            const body = error.json() || '';
            const err = body.error || JSON.stringify(body);
            errMsg = `${error.status} - ${error.statusText || ''} ${err}`;
        } else {
            errMsg = error.message ? error.message : error.toString();
        }
        console.error(errMsg);
        return Observable.throw(errMsg);
    }

}

And in your child service extends by HttpService:

export class SomeService  extends HttpService {

  constructor(private urls: Urls, http: Http, user: User) {
    super(http, user);
  }

  getCheckoutDetails(data): Observable<UserDetail> {
    let query = `7668`;
    header = {};
    return  this.fetch(this.appConfig.getAPIUrl() + `/buy/getDetails?${query}`);
  }

}

Maybe you need a correct HttpService.


Async Task using RxJava2 in Android I have tried to create the async task using the RxJAVA2. The code is working fine. WHat enhancement can I do in this code?

private void createObservable(){

         progressDialog =new ProgressDialog(ProductActivityRx.this);
        progressDialog.setTitle("Data fetching...");
        progressDialog.setCanceledOnTouchOutside(false);
        progressDialog.show();

        Observable<List<Product>> booksObservable =Observable.fromCallable(new Callable<List<Product>>() {
            @Override
            public List<Product> call() throws Exception {
                return getProductList();
            }
        }).doOnNext(new Consumer<List<Product>>() {
            @Override
            public void accept(List<Product> strings) throws Exception {
            }
        });

        productSubscription = booksObservable.
                subscribeOn(Schedulers.io()).
                observeOn(AndroidSchedulers.mainThread()).
                subscribe(new Consumer<List<Product>>() {
                    @Override
                    public void accept(List<Product> productList) throws Exception {

                        displayData(productList);
                    }
                });

    }

I am updating the UI in displayData().

in general, I believe it would be better if you included this in some sort of chain. What I mean here:

don't return void, rather an Observable
don't show the progress dialog in the function, rather delegate it to the part responsible for UI. Same with displayData()
where's the whole function called? What calls it? Where is it located, in an Activity?

doOnNext() can be gotten rid of - there's no action in here
Are you getting just one result at a time? I mean, is the function called perpetually on and on and on, and you just evaluate the result each time? Perhaps you could use Single or Maybe, which is better suited for single results.

Good luck and hope we'll both learn something along the way.


Interactive HTML stories creator I decided to write a small script to make writing HTML interactive stories less tedious:

import doctest
import re
import sys

import tkinter
import tkinter.filedialog

HTML_START = """<!DOCTYPE html>

    <title>{}</title>

{}

     """

HTML_END = """

"""

SEPARATOR = '\n---\n\n'

def link_to_self(word):
    """
    Generates an html link to the given word.

    >>> link_to_self("spam")
    'spam'
    """
    return '{}'.format(word.capitalize(),word)

def inside_square_brackets(text):
    """
    Returns all the text inside square brackets.

    >>> inside_square_brackets('Some nice [brackets]')
    ['[brackets]']
    """
    return re.findall('\[.*?\]',text)

def insert_links(text):
    """
    Insert links from square brackets.

    >>> insert_links("A [cake]?")
    'A cake?'
    """
    for link in inside_square_brackets(text):
        text = text.replace(link,link_to_self(link.replace('[','').replace(']','')))
    return text

def html_page(title,page):
    """
    Generates an html page with title and text.
    """
    return HTML_START.format(title.capitalize(),title.capitalize()) + insert_links(page) + HTML_END

def create_html_file(filename,text):
    with open(filename+'.html','w+') as f:
        f.write(html_page(filename,text))

def gen_interactive_story(titles_and_texts):
    for title,text in titles_and_texts:
        create_html_file(title,text)

def parse_stories_and_titles(text):
    """
    Spits out tuples (title,text) from a text containg paragraphs
    separated by the SEPARATOR.
    """
    chunks = text.split(SEPARATOR)
    lines_from_chunks = [chunk.splitlines() for chunk in chunks]
    return [(line[0].capitalize(),''.join(line[1:]))
                for line in lines_from_chunks]

def find_txt_file(lst):
    """
    Returns the first '.txt' file in a list.

    >>> find_txt_file(['a.txt','b.foo','c.py'])
    'a.txt'
    """
    txts = [i for i in lst if '.txt' in i]
    if txts: return txts[0]

def contains_txt_file(lst):
    return bool(find_txt_file(lst))

def parse_and_generate_stories(filename):
    with open(filename) as f:
        titles_and_texts = parse_stories_and_titles(f.read())
    gen_interactive_story(titles_and_texts)

def ask_filename():
    root = tkinter.Tk()
    root.withdraw()
    return tkinter.filedialog.askopenfilenames()[0]

def _test():
    doctest.testmod()

def main():
    _test()
    if contains_txt_file(sys.argv):
        filename = find_txt_file(sys.argv)
    else:
        filename = ask_filename()
    parse_and_generate_stories(filename)

if __name__ == "__main__":
    main()

To simplify testing, I include a properly written story:

a scary choice

It was an extremely [dark] night and around me I could only
see a [cave] and a [wood].

---

dark

I mean, extremely dark, even without moon. But I could not lose time,
it was time to make [a scary choice].

---

cave

The cave was frightening, but I decided to go in nonetheless,
what is life without risk?

---

wood

The wood was emitting scary and creepy sounds but I decided
to enter it because it seemed less scary than the wood.

for starters, have it output valid html....so don't put a div and an h1 into your head...
also align is not a valid attribute. you should run your markup output through a validator


Online command line JavaScript optimization Here is a light command line I built with JavaScript. I would like to ask if there are many stupidities, bad code and such? The only test command right now is add arg1 arg2 so writing add 1 1 should get you >>> 2. It will print Command *** not found with other commands than add. See the fiddle below.

jsfiddle.net
http://jsfiddle.net/Nr2Am/26/

JavaScript

function cli_focus() {
    var cli = document.getElementById("commandline");
    cli.focus();
}

function arguments_to_array(args) {
    var arr = new Array();
    for (var i=0; i<args.lentgh; ++i) {
        arr[i] = args[i];
    }
    return arr;
}

function cli_go(input) {
    var lines = input.value;
    var lines_arr = lines.split(/\n+/);
    var cmd = lines_arr[lines_arr.length-1];

    cli_run(cmd);
    return false;
}

function cli_parse(cmd) {
    return cmd.split(/\s+/);
}

function cli_remove_blank_words(words) {

    while (words.length>0 && words[0]==="") {
        words = words.slice(1);
    }
    while (words.length>0 && words[words.length-1]==="") {
        words = words.slice(0, words.length-1);
    }

    return words;
}

function cli_run(cmd) {
    var words = cli_parse(cmd);
    words = cli_remove_blank_words(words);

    var last_word = null;

    for (var i=0; i<words.length; ++i) {
        var func_name = words.slice(0, i+1).join("_");
        if (window[func_name] === undefined) {
            break;
        } else {
            last_word = i;
        }
    }

    if (last_word===null || words.length===0) {
        document.getElementById('commandline').value = document.getElementById('commandline').value + '\n>>> Command ' + words[0] + ' not found';
        return;
    }

    var func_name = words.slice(0, last_word+1).join( "_" );
    var func = window[func_name];
    var args = words.slice(last_word+1);
    func.apply(this, args);
}

function add(a,b) {
    var result = parseInt(a)+parseInt(b);
    document.getElementById('commandline').value = document.getElementById('commandline').value + '\n>>> ' + result;
}

Looks like arguments_to_array() can be removed.

You may want to consider having cli_parse() call cli_remove_blank_words() rather than cli_run().

Also, depending on the direction you're going with the command line, you can probably simplify cli_run() in other ways. That long document.getElementById expression should probably be isolated in its own function, since it seems it's going to be used the same way every time for the most part. By doing shift() to the cmd array, you can get the function you want to call from that (and the remaining elements in the array are simply your args to that function). Then you can use a logical operator to check whether that function exists, otherwise call a function that handles errors and such.

Here's how I'd do it:

function cli_parse(cmd) {
    return cli_remove_blank_words(cmd.split(/\s+/));
}

function cli_run(cmd) {
    var words = cli_parse(cmd);
    var func_name = words.shift();
    var func = window[func_name] || window.cli_error;
    func.apply(this, words);
}

function cli_print(txt) {
    document.getElementById('commandline').value =  document.getElementById('commandline').value + '\n>>> ' + txt;
}

function cli_error(error_info) {
    cli_print("Unknown command");
}

function add(a,b) {
    var result = parseInt(a)+parseInt(b);
    cli_print(result);
}

Although, I'd really say using a more object oriented approach would probably facilitate expansion easier, but this works fine.


Count Even Digits In Number I was asked to complete this CodeStepByStep problem. Here's a quick summary:

  Write a function named count_even_digits that accepts two integers as
  parameters and returns the number of even-valued digits in the first
  number. An even-valued digit is either 0, 2, 4, 6, or 8. The second
  value represents how many digits the number has. The second value is
  guaranteed to match the number of digits in the first number.

  For example, the number 8546587 has four even digits (the two 8s, the
  4, and the 6), so the call count_even_digits(8346387, 7) should return
  4.

  You may assume that the values passed to your function are
  non-negative.

My implementation:

def count_even_digits(digit, nums):
    count = 0
    digits = str(digit)
    for i in range(nums):
        if int(digits[i]) % 2 == 0:
            count += 1

    return count

I'm looking for any feedback on my implementation, and how to make this code more efficient. It seems weird that I have to cast the num to a string, then look at each char, convert it to int, then determine if it's even. Having to cast (int => string => analyzing as char => int) seems unnecessary to me.

You should have a look at Loop like a native!. Explicitly iterating over the indices is usually not the best way to do something in Python. By directly iterating over the string representation of n you won't even need the second argument (and you should remove it if the assignment/defined interface would not require it). This will help you a lot later, when you start using generators or other iterables which are not indexable.

In addition, you can use the built-in sum together with the fact that bools are integers with True == 1 and False == 0 and a generator expression to make your function a lot shorter:

def count_even_digits(n, n_digits):
    """Return the number of digits of n which are even.
    Second argument (number of digits) is unused.
    """
    return sum(int(d) % 2 == 0 for d in str(n))

I also added a docstring describing what the function does.

An alternative that does not use the casting to str is to make a digits function:

def digits(n):
    """Yields the digits of `n`, from smallest to largest."""
    while n > 10:
        n, d = divmod(n, 10)
        yield d
    yield n

def count_even_digits2(n, n_digits):
    return sum(d % 2 == 0 for d in digits(n))

Timing wise, these two solutions are both very fast, but the conversion to str stays faster, now tested up to 10**308. I also added the functions shown in the answer by @vurmux and the answer by @spyr03, which are even faster than these two solutions:

Interestingly the manual for loop is faster than the generator expression with sum.

For numbers with a lot of digits, it becomes a bit more complicated. This includes a function proposed in an answer to my question on SO about why the for loop is better than the sum. This function is even faster, but I would only recommend using it if you need that last bit of speed.

def count_even_digits_count_unrolled(n):
    s = str(n)
    return s.count("0") + s.count("2") + s.count("4") + s.count("6") + s.count("8")


Linked list: removing first element Method 1:

public void removeFirst() {
    if (first == null) throw new NoSuchElementException();
    if (first == last) first = last = null;
    else {
        var second = first.next;
        first.next = null;
        first = second;
    }
}

Method 2:

public void removeFirst() {
    if (first == null) throw new NoSuchElementException();
    if (first == last) first = last = null;
    else               first = first.next;
}

Which method is preferable, speed and memory-wise? Stepping through the debugger, I see no differences in first, next, and last objects or their value attributes - so 'correctness' appears the same, though I ponder about garbage collection.

Base class excerpt:

public class LinkedList {
    private class Node {
        private int value;
        private Node next;

        public Node(int value) {
            this.value = value;
        }
    }
    private Node first;
    private Node last;
}

public void addLast(int value){
    var new_node = new Node(value);

    if (first == null){
        first = last = new_node;
    } else {
        last.next = new_node;
        last = new_node;
    }
}

Review points:

removeFirst on an empty list should be a no-op, (or throw an exception).
last was dangling.
Setting things to null of the removed first node, is not done in OOP, is
left to the garbage collection. This also removes the need for an extra variable. And the resulting binary code is smaller.

So:

public void removeFirst() {
    if (first != null) {
        if (first == last) { // Either this.
            last = null;
        }
        first = first.next;
        if (first == null) { // Or this.
            last = null;
        }
    }
}

One can only remove the first, when there is one.
Then the last might be the biblical first.

This is for the single linked list, where a node has just one next pointer.


UICollectionView button target Within my app i have a UIButton (avatar button which shows a profileVC) within a UICollectionView this button shows up in about 4-5 other views. I'm currently adding the target within cellForItemAtIndexPath in each view controller and pushing the view from a public function pushNewViews. I was wondering if they was a better way to do this? (less repetition)

    class ShotsViewController: UIViewController{
    //CollectionView
        override func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {
            let cell = collectionView.dequeueReusableCellWithReuseIdentifier("cell2", forIndexPath: indexPath) as!  mainCell
            cell.userImg.addTarget(self, action: #selector(ShotsViewController.showProfileView(_:)), forControlEvents: .TouchUpInside)
    }
//Target Func
    func showProfileView(sender: AnimatableButton) {
    let profileUser = shots[sender.tag].user
    pushNewViews.showProfileViewController(profileUser, navigation: navigationController!, storyboard: storyboard!)
}

}

class pushNewViews{
    class func showProfileViewController(user: User, navigation: UINavigationController, storyboard: UIStoryboard){
        let vc = storyboard.instantiateViewControllerWithIdentifier("profileView") as! ProfileViewController
        vc.user = user
        navigation.pushViewController(vc, animated: true)
    }
}

You have a couple of options here depending on how explicitly you want to couple this view to the "push" action.

One option which I like is to rely on the existing responder chain. This gives you a lot of flexibility to allow views to emit events from anywhere in the view hierarchy and handle those events in a parent view, view controller, or the app delegate. I like that it avoids the overhead of connecting cells to view controllers (either through target action patterns as you have now or through a delegate protocol) but with the tradeoff that it becomes less obvious which class will respond to an event and requires more integration testing.

// Traverse the responder chain looking for the first responder who conforms to a provided generic type
extension UIResponder {
    func handoffEvent<T>(@noescape eventHandler: (T) -> Void) {
        var nextResponder: UIResponder?
        nextResponder = self
        while((nextResponder) != nil) {
            if let responder = nextResponder as? T {
                eventHandler(responder)
                return
            }
            nextResponder = nextResponder?.nextResponder()
        }
        // FIXME: you probably want some warning if you fail to find a responder of the expected type
    }
}

// Define a protocol for our responder
protocol ControllerRouter {
    func pushUserProfileViewController(user: User)
}

// Views can then send events up the responder chain to be handled by some responder who adopts our protocol but the view doesn't need to know anything about who that responder is
class SomeCustomView: UIView {
    @IBAction func didPressProfileButton() {
        self.handoffEvent { (handler: ControllerRouter) in
            handler.pushCurrentUserProfileViewController(self.user)
        }
    }
}

// Some class in the responder chain needs to implement our protocol to handle the event
class ViewControllerOrAppDelegateOrWhatever: ControllerRouter {
    func pushCurrentUserProfileViewController(user: User) {
        guard
            let navigationController = self.navigationController,
            let profileViewController = self.storyboard.instantiateViewControllerWithIdentifier("profileView") as? ProfileViewController
        else {
            // FIXME: do not ignore unexpected nils
            return
        }
        profileViewController.user = user
        navigationController.pushViewController(profileViewController, animated: true)
    }
}


My Pythonic take on Psexec I've created a little program that basically does the same thing as psexec. It will connect to a host via computer name, or IP address, run the given command, and log the output into a file (little extra for myself). I would like some critique on what I've done, what can I do better, what did I do well, etc.. Some key points I would like to look at are (of course critique everything):

Logging to a file, is there better ways to do so?
Connecting and executing a shell command quicker

import wmi
import sys
import socket
import subprocess
import logging
import getpass
import os
import time
from colorlog import ColoredFormatter

log_level = logging.INFO
logger_format = "[%(log_color)s%(asctime)s %(levelname)s%(reset)s] %(log_color)s%(message)s%(reset)s"
logging.root.setLevel(log_level)
formatter = ColoredFormatter(logger_format, datefmt="%I:%M:%S")
stream = logging.StreamHandler()
stream.setLevel(log_level)
stream.setFormatter(formatter)
LOGGER = logging.getLogger('pyshellConfig')
LOGGER.setLevel(log_level)
LOGGER.addHandler(stream)

def create_log(dest_ip, host_ip, command, data):
    """
    :param dest_ip: Destination IP address
    :param host_ip: IP address of where the command was run
    :param command: Command that was run
    :param data: Output of the command

    Write the output of the run command to a log file for further analysis
    """
    log_path = "C:\\Users\\{}\\AppData\\pyshell".format(getpass.getuser())
    if not os.path.isdir(log_path):
        os.mkdir(log_path)

    with open("{}\\{}_LOG.LOG".format(log_path, __file__), "a+") as log:
        log.write("![{}][LOG FROM:{} TO:{}]COMMAND RUN: {} OUTPUT:{}\n".format(time.strftime("%m-%d-%Y::%H:%M:%S"),
                                                                               host_ip, dest_ip,
                                                                               command, data))

def connect(hostname, command=None):
    """
    :param hostname: Computer name or IP address
    :param command: Shell command to be run on the host

    Connect to the host and run a shell command on their system
    """
    LOGGER.info("Attempting to connect to host: '{}'".format(hostname))
    try:
        connection = wmi.WMI(hostname)  # Attempt to connect to the hostname
        if connection:
            LOGGER.info("Connected successfully, running command: '{}'".format(' '.join(command)))
            call = subprocess.Popen(command,
                                    shell=False,
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.PIPE)
            output = call.communicate("Running remote command...")  # Start running the command and save output to variable
            create_log(socket.gethostbyname(hostname), socket.gethostbyname(socket.gethostname()),
                       command, output)
            LOGGER.info("Command completed successfully.")
    except Exception, e:  # I don't think there's really anyway of getting the exact error from the shell
        error_message = "Failed to connect due to %s. " % e
        error_message += "\nThis could mean that '%s' is not the correct " % hostname
        error_message += "computer name, or that the host has refused it. "
        error_message += "You can try to connect via IP address, if the "
        error_message += "user is able to get it."
        LOGGER.fatal(error_message)

if __name__ == '__main__':
    help_page = """python shell.py [HOST] [COMMAND]"""
    error_message = ""

    try:
        if not sys.argv[1::]:  # User ARGV for the arguments to more match Psexec
            error_message += "You did not supply a hostname to run against\n{}".format(help_page)
            LOGGER.fatal(error_message)
            exit(1)
        elif not sys.argv[2::]:
            error_message += "You did not supply a command to run\n{}".format(help_page)
            LOGGER.fatal(error_message)
            exit(1)

        connect(sys.argv[1].upper(), command=sys.argv[2::])
    except IndexError:
        pass

Keep your imports grouped. From PEP 8.

Imports should be grouped in the following order:

  standard library imports
  related third party imports
  local application/library specific imports

Follow Python's EAFP principle instead of checking if something exists. Plus when it comes to folder creation it will also help you avoid race-condition.

import errno

try:
    os.mkdir(log_path)
except Exception as e:
    if e.errno != errno.EEXIST:
        # If error is something other than file
        # path already exists then handle it here.
        pass

Note my use of except Exception as e here instead of except Exception, e. The latter syntax is deprecated and has been removed in Python 3. Hence go for as based format.

Your try-except block is huge. Cover minimum statements at once, it will help you in narrowing down the actual problem easily and you will be able to show a much better error message to user. Have multiple try-excepts as long as they don't make the code too unreadable.

Instead of dealing with sys.argv you could use the arsparse module to handle command line args in much better way.

error_message = "" statement is not required. You can simply define it wherever you need it instead of defining a default value.

[1::] and [2::] can be simply written as [1:] and [2:] respectively.

The IndexError at the end is redundant because you're already checking for if not [1::] earlier and exiting the script right away, hence you would never hit that exception. Note that slicing never raises IndexError.

call.communicate("Running remote command...") returns both STDOUT and STDIN. You can consider using them separately.

Plus you could also check the status code by doing call.returncode. A non-zero exit code means an error.

log_path = "C:\\Users\\{}\\AppData\\pyshell".format(getpass.getuser())

A hard-coded path is not a good idea. Perhaps ask the user for a path?

You can also get path to user's home directory using os.path.expanduser.

log_path = r"{home}\AppData\pyshell".format(home=os.path.expanduser('~'))

Note that I also added r"" so that we can simply use a single backslash.