C++ rudimentary bitmap image edge detection What my code does is it reads a 24 bit bitmap image in color, runs a very simple edge detection algorithm and outputs a monochrome 1 bit bitmap with the edges in white. Here is an example of the output.

The basic algorithm is that I am looping through all of the pixels in the 24 bit bitmap two pixels at a time and if the difference between the average values of the two pixels is greater than 10 I set the equivalent pixels in the 1 bit file to white.

To give the code context, the variable "header" is an object that holds all of the information about the 24 bit bitmap file such as the height and width and the pointer to where the header ends and the bitmap that holds the pixel values begins. The "header" object was passed into the constructor of the object that has the ten() function.

The one_bit_bitmap "small" variable does the same thing for the 1 bit bitmap that I am outputting to.

The file format of the 24 bit is there is one byte per color (blue,green,red) per pixel. The format for the 1 bit is that there are two 32 bit pixels just after the header information and setting a bit to 0 points that pixel to the value of the first pixel  while setting it to 1 sets to to the value of the second pixel.

This is a personal project that I started to learn about file formats, C++ pointers and flipping individual bits.

void bmp_binary_edge::ten() {
    char * pointer_ahead = header.getBytesAhead();
    unsigned int width_bytes = header.getWidthBytes();
    unsigned int padding_bytes = header.getPaddingBytes();
    unsigned int height = header.getHeightPixels();
    one_bit_bitmap small = one_bit_bitmap(header);
    char * small_ahead = small.getBytesAhead();
    unsigned int small_position = 0;
    for (unsigned int y = 0; y < height; y++) {
        int small_bit_position = 7;
        for (unsigned int x = 0; x < width_bytes; x += 6) {
            unsigned int starting_position = x + (y * width_bytes) + y * padding_bytes;
            unsigned char blue = (unsigned char) pointer_ahead[starting_position];
            unsigned char green = (unsigned char) pointer_ahead[starting_position + 1];
            unsigned char red = (unsigned char) pointer_ahead[starting_position + 2];
            unsigned char blue2 = (unsigned char) pointer_ahead[starting_position + 3];
            unsigned char green2 = (unsigned char) pointer_ahead[starting_position + 4];
            unsigned char red2 = (unsigned char) pointer_ahead[starting_position + 5];
            unsigned char first_av = (blue + red + green) / 3;
            unsigned char second_av = (blue2 + red2 + green2) / 3;
            int difference = first_av - second_av;
            if (difference < 0) {
                difference *= -1;
            }
            //One
            if (difference > 10) {
                small_ahead[small_position ] ^= (-1 ^ small_ahead[small_position]) & (1 << small_bit_position);
                small_ahead[small_position ] ^= (-1 ^ small_ahead[small_position]) & (1 << (small_bit_position - 1));
            }
            if (small_bit_position == 1) {
                small_position++;
                small_bit_position = 7;
            }
            else {
                small_bit_position -= 2;
            }
        }
        small_position += small.getPaddingBytes();
    }
    small.fileWrite(ext_ten);
    cout << "bmp_binary_edge ten" << endl;
    cout << line_break;
    header.deleteBytes();
}

I'm just going to rattle off the things I see as I find them.

header.getBytesAhead() should return a void *
pointer_ahead should be a unsigned char const *
small_ahead should be a unsigned char *
Loop x over the pixels (width) not the bytes.
for (unsigned int x = 0; x < width; x += 2)
starting_position = x * 6 + y * ( width * 3 + padding_bytes )
int difference = abs(first_av - second_av)
small_position = x / 8 + y * ( small_width + small_padding_bytes )
small_bit_position = x % 8
small_ahead[ small_position ] |= 3 << small_bit_position
Use std::vector so header.deleteBytes is unnecessary.

Edit:

small_position = x / 8 + y * ( ( width + 7 ) / 8 + small_padding_bytes )
To handle an odd width: for (unsigned int x = 0; x + 1 < width; x += 2)


Sorting an array in C++ using Bubble sort Here is my bubble sort algorithm that I would like to improve in any way possible
#include<iostream>

int main(){
    int arr[6] = {5,2,3,7,2,6};
    int f = 0;
    int b = 0;
    for(int i = 1;i < 6;i++){
        if(arr[i] < arr[i-1]){
            f = arr[i];
            b = arr[i-1];
            arr[i] = b;
            arr[i-1] = f;
            i=1;
        }

    }
    for(int i = 0;i < 6;i++) std::cout << arr[i] << " ";
}

Anything is appreciated

Some notes:

I would avoid running a loop while changing i inside, it is very unclear to the reader. I prefer using two nested loops.
The swap can be done in one step less
I would initialize a size variable and use it throughout the code instead of hard-coding 6.
Printing the array to the console should be in a separate function (as well as the sorting function really).

#include<iostream>

int main(){
    const int sz = 6;
    int arr[sz] = {5,2,3,7,2,6};
    do{
        swapped = false;
        for(int i = 1;i < sz; i++){
            if(arr[i] < arr[i-1]){
                int tmp = arr[i];
                arr[i] = arr[i-1];
                arr[i-1] = tmp;
                swapped = true;
            }
        }
    } while(swapped);
    print_array(arr, sz);
}


Leetcode Three Sum in Python Here's my solution for the Leet Code's Three Sum problem -- would love feedback on (1) code efficiency and (2) style/formatting. This is Python 3.

Problem:

  Given an array nums of n integers, are there elements a, b, c in nums
  such that a + b + c = 0? Find all unique triplets in the array which
  gives the sum of zero.

  Note:

  The solution set must not contain duplicate triplets.

  Example:

  Given array nums = [-1, 0, 1, 2, -1, -4],

  A solution set is: [   [-1, 0, 1],   [-1, -1, 2] ]

My solution:

def threeSum(nums):
    """
    :type nums: List[int]
    :rtype: List[List[int]]
    """

    def _twoSum(nums_small, target):
        '''
        INPUT: List of numeric values, int target
        OUTPUT: list: pair(s) from nums_small that add up to target and -1*target
        '''
        nums_dict = {}
        return_lst = []

        for indx, val in enumerate(nums_small, target):
            if target - val in nums_dict:
                return_lst.append([val, target - val, -1*target])
            nums_dict[val] = indx
        return return_lst

    return_dict = {}

    for indx, val in enumerate(nums):
        for solution in _twoSum(nums[:indx] + nums[indx+1:], -1*val):
            if sorted(solution) not in return_dict.values():
                return_dict[str(indx)+str(solution)] = sorted(solution) # ... hacky way of storing multiple solutions to one index ...

    return list(return_dict.values())

Code

  "When all you have is a hammer, everything looks like a nail."

Your tool chest seems to have dict, which you are using to implement a container which doesn't contain any duplicates.  There is a name for that tool.  It is called as set.  Study it; you will find it very useful.

Consider:

    nums_dict = {}

    for indx, val in enumerate(nums_small, target):
        if target - val in nums_dict:
            pass
        nums_dict[val] = indx

You are never using the value indx; you are just storing it and never retrieving it.  You could just as easily write:

    nums_dict = {}

    for val in nums_small:
        if target - val in nums_dict:
            pass
        nums_dict[val] = True

which avoids the enumerate(), with the odd initial value, and the indices which did nothing.  But this is still storing the values True, and never retrieving them.  You don't need to do this, if you use a set:

    nums_dict = set()

    for val in nums_small:
        if target - val in nums_dict:
            pass
        nums_dict.add(val)

You again use a dict for return_dict to ensure you don't have any duplicate solutions.  Actually, this code is a worse that the former, because you are using in return_dict.values(), so instead of an \$O(1)\$ key lookup, you're doing an \$O(n)\$ search through the entire list of values!  If that wasn't bad enough, you are sorting solutions twice: once to see if they already exist in return_dict, and a second time to add it into the dictionary if it wasn't found.  Saving the sorted solution would avoid the second sort.

You could replace return_dict with a set as well, but there is a catch, you can't add a list into a set, because the items in a set must be hashable to a constant value but lists are unhashable (because they can be mutated).  You can get around this by using tuples, which can't be mutated, so they can be hashed.

 return_dict = set()

 for index, val in ...:
    for solution in ...:
        sorted_solution = tuple(sorted(solution))
        return_dict.add(sorted_solution)

 return list(return_dict)

The above returns a list of tuples.  If you need a list of lists, that can be easily obtained as well, using list comprehension ...

 return [ list(solution) for solution in return_dict ]

... which is another useful tool for your tool chest.

Algorithm

Using _twoSum() to solve the threeSum() problem is good reuse of code.  Kudos.

However, looping over all indices and for each index, constructing a new list omitting just that one value from the list nums[:indx] + nums[indx+1:] is extremely inefficient.

Since "Leetcode" is a puzzle solving site, I won't give you the better algorithm, but I will say that splitting nums into len(nums) different lists is not the way to go.  I would split it into 2 lists, or maybe 3 lists tops.  Good luck!


'Retry' mechanism with callback We've had a situation whereby we have to cater for certain exceptions and retry a particular method whenever these exceptions occur. This is required in various parts of the system. In addition, we (might) want to invoke a 'callback' upon the need to retry.

I'll show you the code, then give you an example of useage:

    public static void InvokeActionWithRetry(Action action, int retryAttempts, Action retryCallback = null, params Type[] exceptionTypes)
    {
        do
        {
            try {
                action();
                break;
            }
            catch (Exception ex)
            {
                   if ((exceptionTypes?.Length > 0 && exceptionTypes.Contains(ex.GetType()))
                        || retryAttempts == 0)
                    throw;

                if (retryCallback != null)
                    retryCallback();
            }

            retryAttempts--;
        }
        while (retryAttempts >= 0);
    }

So, an example (although not a real-world one), may be:

ActionHelper.InvokeActionWithRetry(() => {
    Print(document);
},
5,
() => {
    RebootPrinter();
},
typeof(PrinterNeedsRestartException));

Is there anything that can be improved with the static method I've written?

I'm pretty new to using actions/funcs so I'm open to suggestions about how I can refactor this.

I also feel the type evaluation is a little 'hacky'.

EDIT:

I just noticed I'm not checking the types collection passed in prior to calling Contains on it. Naughty. I'll rework that bit and update the question.

Consider this example:

InvokeActionWithRetry(() =>
        {
            Console.WriteLine("action");
            throw new Exception();
        },
    3);

It prints 'action' four times. That's not what I expect from the API (but is what I expected from reading the code). I'd expect the number I pass in to be the total number of invocations - not the number of retries after the first attempt.

You need to check this arguments for this method as there is a lot of scope to pass in the weird values. e.g. what if retryAttempts is a negative number? That doesn't make sense!

You also need to check action for null.


Abstract factory design for different cars Is there anything wrong with this?

    namespace WindowsFormsApplication1
    {
      public interface ICarfactory
      {
        ICar getCar(CarType carType);
      }

      public interface ICar
      {
      }

      public class Sedan: ICar
      {
      }

      public class Hatchback : ICar
      {
      }

      public class SUV : ICar
      {
      }

      public class HondaSUV : SUV
      {
      }

      public class HondaSedan : Sedan
      {
      }

      public class HondaHatchback : Hatchback
      {
      }

      public class HyundaiSUV : SUV
      {
      }

      public class HyundaiSedan : Sedan
      {
      }
      public class HyundaiHatchback : Hatchback
      {
      }

      public class MarutiHatchback : Hatchback
      {
      }

      public class MarutiSUV : SUV
      {
      }

      public class MarutiSedan : Sedan
      {
      }

      public class Hyundai : ICarfactory
      {
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
              return new HyundaiSedan();
            case CarType.HatchBack:
              return new HyundaiHatchback();
            case CarType.SUV:
              return new HyundaiSUV();
            default:
              return null;
          }
        }

      }

      public class Maruti : ICarfactory
      {
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
              return new MarutiSedan();
            case CarType.HatchBack:
              return new MarutiHatchback();
            case CarType.SUV:
              return new MarutiSUV();
            default:
              return null;
          }
        }
      }

      public class Honda : ICarfactory
      {
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
               return new HondaSedan();
            case CarType.HatchBack:
               return new HondaHatchback();
            case CarType.SUV:
              return new HondaSUV();
            default:
              return null;
          }
        }
      }

      public enum CarType
      {
        Sedan,
        HatchBack,
        SUV
      }
    }

 private void button4_Click(object sender, EventArgs e)
  {
  ICar myCar;
  ICarfactory carfactory;
  carfactory = new Hyundai();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);

  carfactory = new Honda();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);

  carfactory = new Maruti();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);
  }

The factory in itself is nice (except for one thing), but I have one big problem with your code.

Approach:

You are heavily relying on inheritance over composition. Instead of something so complicated as running that against a Factory, where you return types after a switching an enum you should IMO do the following:

public abstract class Car
{
    private Manufacturer _manufacturer;
    public Manufacturer Manufacturer
    {
        get
        {
            return _manufacturer;
        }
        set
        {
            _manufacturer = value;
        }
    }

    private CarType _carType;
    public CarType CarType
    {
        get
        {
            return _carType;
        }
        //You can even make that stuff read-only by removing the setter
        set
        {
             _carType = value;
        }
     }
}

Implementation:

When you pass an invalid CarType to the Factory you silently return null. That's somewhat unfriendly to the user. Factories are expected to either return a meaningful value or throw an exception. If you allow returning of null, you make the user of the factory do additional null-checks. That is not nice...

Instead you should do the following:

default:
    throw new IllegalArgumentException("CarType not found");


C# Async/Await networking server I made a simple async networking server and I'd like to get some input on the code, whether things can be done better and whether the code is stable (able to hold more than 1000 connections without lag) for a MMORPG game.

GameNetworkListener.cs:

using System;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;

namespace Rain.Networking.Game
{
    public class GameNetworkListener
    {
        private TcpListener _listener;

        public async Task StartListening(int port)
        {
            _listener = new TcpListener(IPAddress.Any, port);
            _listener.Start();

            TcpClient lastClient;

            while ((lastClient = await _listener.AcceptTcpClientAsync()) != null)
            {
                Console.WriteLine($"New connection from {lastClient.Client.RemoteEndPoint}");

                var gameNetworkClient = new GameNetworkClient(lastClient);
                await gameNetworkClient.StartReceiving();
            }
        }

        public void Stop()
        {
            _listener.Stop();
        }
    }
}

GameNetworkClient.cs:

using System;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

namespace Rain.Networking.Game
{
    public class GameNetworkClient
    {
        private readonly TcpClient _client;
        private readonly NetworkStream _stream;

        public GameNetworkClient(TcpClient client)
        {
            _client = client;
            _stream = client.GetStream();
        }

        public async Task StartReceiving()
        {
            while (_client.Connected)
            {
                var bytes = new byte[512];
                var amount = await _stream.ReadAsync(bytes, 0, bytes.Length);

                var raw = Encoding.UTF8.GetString(bytes);

                if (amount > 0)
                {
                    if (raw.StartsWith("<policy-file-request/>"))
                    {
                        await SendData(Encoding.UTF8.GetBytes("<?xml version=\"1.0\"?>\r\n" +
                                                                 "<!DOCTYPE cross-domain-policy SYSTEM \"/xml/dtds/cross-domain-policy.dtd\">\r\n" +
                                                                 "<cross-domain-policy>\r\n" +
                                                                 "<allow-access-from domain=\"*\" to-ports=\"*\" />\r\n" +
                                                                 "</cross-domain-policy>\x0"));
                        _client.Dispose();
                    }
                    else
                    {
                        Console.WriteLine($"Received {raw}");
                    }
                }
                else
                {
                    _client.Dispose();
                }
            }
        }

        public async Task SendData(byte[] data)
        {
            await _stream.WriteAsync(data, 0, data.Length);
        }
    }
}

Following rules start the server:

_gameNetworkListener = serviceProvider.GetService<GameNetworkListener>();
await _gameNetworkListener.StartListening(30000);

I see two major issues when you want to use this code for a MMORPG game.

Exception Handling

Make your code robust against connection hick-ups (when connection goes down for a fraction). It's up to be the client to reconnect asap on connection loss (out of scope of your code). The server should dispose client connections on error in order to clean up resources.

Resource Management

The server does not care about the number of clients connected, nor does it own the life time scope of these connections.

I would expect the code below to close the connections.

public void Stop()
{
    _listener.Stop();
    // _connectedClients.ForEach(Disconnect); // cleanup resources
}


Finding index in list in C++ I need to find correct index in a array by comparing ranges. Here is my first try:

int TunerDriver::TunerLookUpTable(double Freq) {
  Freq /= 1e6;
  if (Freq >= 60 and Freq < 140)
    return 1;
  else if (Freq >= 180 and Freq < 280)
    return 2;
  else if (Freq >= 280 and Freq < 460)
    return 3;
  else if (Freq >= 140 and Freq < 180)
    return 4;
  else if (Freq >= 2720 and Freq < 4020)
    return 5;
  else if (Freq >= 4020 and Freq <= 6000)
    return 6;
  else if (Freq >= 2000 and Freq < 2720)
    return 7;
  else if (Freq >= 830 and Freq < 1420)
    return 8;
  else if (Freq >= 1420 and Freq < 2000)
    return 9;
  else if (Freq >= 460 and Freq < 830)
    return 10;
  else {
    //TODO: throw exception
  }
}

I believe I can use std::pair and for to increase readability. Can you assist me in this case?

It's a short bit of code, but enough to work with, I think.  Here are some suggestions to help you improve your code.

Minimize the number of comparisons

There is not really a need to check both upper and lower bounds for each possibility if the comparisons are done in order.  That is, we could compare the incoming Freq to 60, then 140, then 180, etc.  That might look like this:

int FreqLookup(double Freq) {
    if (Freq < 60e6 || Freq > 6000e6) {
        return 0;  // or throw an error
    }
    if (Freq < 140e6) return 1;
    if (Freq < 180e6) return 4;
    if (Freq < 280e6) return 2;
    if (Freq < 460e6) return 3;
    if (Freq < 830e6) return 10;
    if (Freq < 1420e6) return 8;
    if (Freq < 2000e6) return 9;
    if (Freq < 2720e6) return 7;
    if (Freq < 4020e6) return 5;
    return 6;
}

Put constants in a structure

Having the constants in a structure allows the code to be more data driven.  Here's what I'd suggest:

struct FreqTableEntry {
    double freq;
    int divisor;
    operator double() const { return freq; }
};
static constexpr std::array<FreqTableEntry, 11> lookup {{
    { 60e6, 0 }, // error if below 60e6
    { 140e6, 1 },
    { 180e6, 4 },
    { 280e6, 2 },
    { 460e6, 3 },
    { 830e6, 10 },
    { 1420e6, 8 },
    { 2000e6, 9 },
    { 2720e6, 7 },
    { 4020e6, 5 },
    { 6000e6, 6 },
}};

Now we have a nice, neat, compile-time lookup table.  Note also that we can avoid division by simply using the full value in the table.

Use a standard algorithm

The std::upper_bound algorithm returns an iterator to the first entry that is greater than the given value.  We can use it:

auto it{std::upper_bound(lookup.cbegin(), lookup.cend(), Freq)};

It would be nice to simply return it->divisor but we have to handle a few special cases.  First, if the frequency is exactly 6000e6, we need to return 6.  Next, if the frequency is < 60e6 or > 6000e6, we need to indicate an error.  I've chosen to return 0 as an indication of error, but one could also throw if that's more appropriate.  Putting it all together we have this:

#include <array>
#include <algorithm>

int alt(double Freq) {
    struct FreqTableEntry {
        double freq;
        int divisor;
        operator double() const { return freq; }
    };
    static constexpr std::array<FreqTableEntry, 11> lookup {{
        { 60e6, 0 }, // error if below 60e6
        { 140e6, 1 },
        { 180e6, 4 },
        { 280e6, 2 },
        { 460e6, 3 },
        { 830e6, 10 },
        { 1420e6, 8 },
        { 2000e6, 9 },
        { 2720e6, 7 },
        { 4020e6, 5 },
        { 6000e6, 6 },
    }};
    // special case the last entry
    if (Freq == lookup.back().freq) {
        return lookup.back().divisor;
    }
    auto it{std::upper_bound(lookup.cbegin(), lookup.cend(), Freq)};
    if (it == lookup.cend() || it->divisor == 0) {
        return 0;  // could throw here as alternative
    }
    return it->divisor;
}

Provide a test program

It's often helpful to write a test program and to provide it to reviewers to make it clear what the program is expected to do and how it will be called.  I modified your routine slightly to return 0 as an error indication and made it a standalone function.  The test program in this case, was just to compare the original routine to the alternative (named alt) shown above:

#include <iostream>

int main() {
    for (double f{30e6}; f <= 6020e6; f += 10e6) {
        if (TunerLookUpTable(f) != alt(f)) {
            std::cout << "ERROR: TunerLookUpTable(" << f/1e6 << " MHz) = "
                << TunerLookUpTable(f) << ", but alt(f) = " << alt(f) << '\n';
        }
    }
}


Render Polygon Method (Documentation) I've written a method to render a polygon in C# using the SharpDX libraries and would like a review of my code's maintainability and the documentation around it. The method works fine as is, but I feel that perhaps the documentation could be improved, or that there could be simpler and more maintainable method of executing the algorithm.

/// <summary>
/// Render an n-sided polygon from a list of allowed polygons.
/// </summary>
/// <param name="context2D">The 2D context used to render the polygon.</param>
/// <param name="polygon">The n-sided polygon to render.</param>
/// <param name="center">The central position of the polygon in 2D space</param>
/// <param name="radius">The radius of the polygon.</param>
/// <param name="rotation">The global rotation of the polygon (in degrees).</param>
public void RenderPolygon(DeviceContext context2D, PolygonType polygon, Vector2 center, float radius, float rotation) {
    float numberOfPoints = (float)polygon;
    Vector2? firstPoint = null, previousPoint = null, currentPoint = null;
    for (int n = 0; n < numberOfPoints; n++) {
        float theta = ((360.0f / numberOfPoints) * n + rotation) * (float)Math.PI / 180.0f;
        currentPoint = center + new Vector2((float)Math.Cos(theta), (float)Math.Sin(theta)) * radius;

        if (previousPoint != null && currentPoint != null)
            context2D.DrawLine((Vector2)previousPoint, (Vector2)currentPoint, brush);

        previousPoint = currentPoint;
        if (firstPoint == null)
            firstPoint = currentPoint;
    }

    if (firstPoint != null && currentPoint != null)
        context2D.DrawLine((Vector2)currentPoint, (Vector2)firstPoint, brush);
}

With regards to the PolygonType argument, this is an enum with int values for each type; for example:

public enum PolygonType {
    Trigon = 3,
    Tetragon = 4,
    Pentagon = 5
}

There are a total of 30 polygon types offered stopping at Chiliagon which has 1000 sides.

The things I am most interested in are:

Ensuring documentation provides as much detail as possible (while remaining concise).
Ensuring that the algorithm is easy to follow and maintain.

The one thing I don't currently like about the algorithm:

Casting from Vector2? to Vector2 seems redundant (though required in the current case).

Since Vector2's default value is 0, 0, I'm not sure I should use it and have chosen to use null; but since Vector2 is not a nullable type, I have to cast back to the normal Vector2 prior to using my variables.

The most confusing thing in your code is names of arguments and variables and their types.

DeviceContext context2D

Why not deviceContext?

PolygonType polygon

It's not polygon (which is collection of points as I expect), it's polygon type, so name it polygonType

float numberOfPoints = (float)polygon;

Why are you casting to float? Number of points can be non-integer? Also in my opinion it's not good to give another meanings to enum values except what they mean by their names. Your enum defines polygons types, not numbers of points. I recommend to create dictionary:

private static readonly Dictionary<PolygonType, int> NumberOfPoints =
    new Dictionary<PolygonType, int>
    {
        [PolygonType.Trigon] = 3,
        // ...
    };

and use it:

var numberOfPoints = NumberOfPoints[polygonType];

Why are you using for polygon types such strange names and not Triangle and Square?

If it's public API method, add arguments checking using appropriate exceptions (ArgumentOutOfRangeException, InvalidEnumArgumentException and so on). All possible exceptions should be documented via <exception> tags.


Singleton-ish RMI Server In this program there is a single remote server which performs miscellaneous work. Because there is only one server, it seems like a singleton pattern could fit well.

The problem that a distributed program introduces is that every client will have its own version of a static variable. Therefore I have defined a basic ComputeInterface which is implemented by two classes. ComputeServer is the object which performs the work asked for by clients. ComputeService is a class which performs an RMI lookup to the ComputeServer and acts like singleton server in the client program.

The location information for the Server is maintained in a Configuration class:

public class Configuration {
    public static final int COMPUTE_REMOTE_PORT = 8900;
    public static final String COMPUTE_REMOTE_ID = "COMPUTE_REMOTE_ID";
    public static final String COMPUTE_REMOTE_HOST = "localhost";
}

A ComputeTask implements Serializable and is performed by the ComputeInterface which implements Remote. The ComputeInterface contains an isConnected method which is used for determining if there is connectivity to the server from the service:

public interface ComputeTask extends Serializable{
    void doWork();
}

public interface ComputeInterface extends Remote {
    ComputeTask compute(ComputeTask task) throws RemoteException;
    boolean isConnected() throws RemoteException;
}

This is the implementation for the server:

final class ComputeServer extends UnicastRemoteObject implements ComputeInterface {
    public static void main(String[] args) throws RemoteException, AlreadyBoundException {
        ComputeServer computeServer = new ComputeServer();
        Registry registry = LocateRegistry.createRegistry(Configuration.COMPUTE_REMOTE_PORT);
        registry.bind(Configuration.COMPUTE_REMOTE_ID, computeServer);
        System.out.println("Compute Server Running");
    }

    private ComputeServer() throws RemoteException {
        super();
    }

    @Override
    public synchronized ComputeTask compute(ComputeTask task) throws RemoteException{
        task.doWork();
        return task;
    }

    @Override
    public synchronized boolean isConnected() throws RemoteException{
        return true;
    }
}

And here is the Service. If the server has not yet been connected, the service will attempt to do a registry lookup on the server. If the registry lookup up was successful (or has already been performed), it will check the server connectivity before performing any tasks.

final public class ComputeService implements ComputeInterface{

    public static final ComputeInterface service = new ComputeService();
    private static ComputeInterface computeServer;

    private ComputeService(){}

    @Override
    public synchronized boolean isConnected() throws RemoteException{
        return ComputeService.connected();
    }

    private static boolean connected(){
        if (computeServer == null) {
            try{
                Registry reg = LocateRegistry.getRegistry(Configuration.COMPUTE_REMOTE_HOST, Configuration.COMPUTE_REMOTE_PORT);
                computeServer = (ComputeInterface) reg.lookup(Configuration.COMPUTE_REMOTE_ID);
                System.out.println("New Connection to Compute Server");
            }
            catch (RemoteException | NotBoundException e){
                System.err.print(e);
                computeServer = null;
                return false;
            }
        }
        try{
            return computeServer.isConnected();
        }
        catch (RemoteException e){
            System.err.print(e);
            computeServer = null;
        }
        return false;
    }

    @Override
    public synchronized ComputeTask compute(ComputeTask task) throws RemoteException{
        if(!ComputeService.connected()) return task;
        return computeServer.compute(task);
    }
}

Here is an example of how a client can use the service to perform work on the server:

CustomComputeTask task = new CustomComputeTask();
task = ComputeService.service.compute(task);

In this program there is a single remote server which performs miscellaneous work. Because there is only one server, it seems like a singleton pattern could fit well.

Let us see the definition from Wikipedia:

  In software engineering, the singleton pattern is a design pattern
  that restricts the Instantiation of a class to one object. This is
  useful when exactly one object is needed to coordinate actions across
  the system. The concept is sometimes generalized to systems that
  operate more efficiently when only one object exists, or that restrict
  the instantiation to a certain number of objects. The term comes from
  the mathematical concept of a singleton.

So basically you are on the right track but what can't a simple private final object accomplish what the singleton can ?

Furthermore, which disadvantages has using a singleton ?

Quoting from here

  Singletons are essentially static classes, relying on one or more static methods and properties. All things static present real, tangible problems when you try to do Unit Testing because they represent dead ends in your code that cannot be mocked or stubbed. As a result, when you test a class that relies on a Singleton (or any other static method or class) you are not only testing that class but also the static method or class.

and here

  It is because of this specific design choice that the pattern introduces several potential long-term problems:

  Inability to use abstract or interface classes;
  Inability to subclass;
  High coupling across the application (difficult to modify);
  Difficult to test (can't fake/mock in unit tests);
  Difficult to parallelize in the case of mutable state (requires extensive locking);

  and so on.

  None of these symptoms are actually endemic to single instances, just the Singleton pattern.

  What can you do instead? Simply don't use the Singleton pattern.

So basically this boils down, if you really need to use it then use it, but make sure you understand the possible pitfalls and possible upcoming problems.

That being said, let me review some of your code.

public class Configuration {
    public static final int COMPUTE_REMOTE_PORT = 8900;
    public static final String COMPUTE_REMOTE_ID = "COMPUTE_REMOTE_ID";
    public static final String COMPUTE_REMOTE_HOST = "localhost";
}

What will happen if the port or the remote host needs to be changed ? You will need to recompile this class because all members are static and can't be changed. How about testability of the server /client code ?

Using this setup also restricts the server to be on the same computer than the client(s).

Based on how you have implemented the ComputeService and the ComputeInterface I would like to suggest that you add another method to the ComputeTask interface returning if the task really has been processed. Right now if the ComputeService can't connect to the server the returned ComputeTask object doesn't tell the client that it hadn't been processed.

The private static boolean connected() method doesn't throw a RemoteException and therefor either the isConnected() method of the ComputeInterface shouldn't have a throws RemoteException or better, let the connected() really throw that exception.

Right now if this method returns false the client doesn't know what the problem is.

In addition, I would name the connected() method just isConnectedToServer() or something along this line. But nevertheless this method is doing to much because it is doing a registry lookup so to say it is connecting to the server, but is read like a simple "property getter" .

I would change isConnected() method like so

@Override
public synchronized boolean isConnected() throws RemoteException{
    if (computeServer == null)
    {
        return false;
    }
    return computeServer.isConnected();
}

Then I would replace the connected() method by a, to the ComputeInterface added, boolean connect() method which does exactly what the name implies like so

@Override
public synchronized boolean connect() throws RemoteException{
    if (computeServer == null) {
        try{
            Registry reg = LocateRegistry.getRegistry(Configuration.COMPUTE_REMOTE_HOST, Configuration.COMPUTE_REMOTE_PORT);
            computeServer = (ComputeInterface) reg.lookup(Configuration.COMPUTE_REMOTE_ID);
            System.out.println("New Connection to Compute Server");
        }
        catch (RemoteException | NotBoundException e){
            System.err.print(e);
            computeServer = null;
            throw e;
        }
    }
    return true;
}

and if you want to add some kind of authentication, I would add a boolean connect(someparameters) method to the ComputeInterface which then is called instead of just returning true.

The most recent question for me is if you really should use RMI for this application. RMI has some disadvantages like

it is using blocking IO
if you want to use callbacks, you will likely come into trouble if a router or firewall is set up
and some more

IMHO a much better alternative would be SIMON ( Simple Invocation of Methods Over Network), which is actively developed and maintained. I use this myself for a multi-server/multi-client system and don't regret its use.


Count common factors I have written some code for calculating a count of common factors. Please help me remove anything unnecessary and shorten it.

Please tell me if any function in Python 3 can calculate common factors directly!

# a string of 2 numbers
# is input e.g. '10 15'

a,b=str(input()).split(' ')
a,b=int(a),int(b)
n=0
if a>=b:
    lim=a
else:
    lim=b
for num in range(1,lim):
    if a%num==0 and b%num==0:
        n+=1
print(n)

Coding style

There is a well-established coding style for Python, the PEP8 coding style, and conformance to that style can be checked online at PEP8 online.

In your case it reports “missing space around operator” in almost every
code line. Fixing those issues increases the legibility of your code.

Program structure

It is better to separate the I/O from the actual computation. With a function to compute the number of common divisors you make your code more readable and reusable. In addition you can document each function and add test cases easily.

A check for main allows to run the script directly, or import it as a module.

So the overall program structure should be

def num_common_factors(a, b):
    """ Return the number of common factors of a and b. """
    # ... TODO ...

if __name__ == "__main__":
    a, b = map(int, input().split())
    print(num_common_factors(a, b))

Note also that reading the input line and the mapping to integers can be combined into a single statement.

Simplify (and fix) your code

Your solution is the “brute-force” approach: Test every possible number (in a suitable range) if it is a divisor of both numbers. That can be improved considerably (see below), but let's first discuss how your solution can simplified. First, the limit can be computed using the built-in min() function:

lim = min(a, b)

Then note that a range() excludes the upper bound, so you probably want

for num in range(1, lim + 1):

I would also avoid abbreviations, i.e. limit instead of lim. Finally the code can be written more concisely by summing a generator expression:

def num_common_factors(a, b):
    """ Return the number of common factors of a and b. """
    limit = min(a, b)
    return sum(1 for i in range(1, limit + 1) if a % i == 0 and b % i == 0)

Mathematics helps!

The problem can be solved more efficiently using some maths. Every common divisor of a and b is a divisor of the “greatest common divisor” of a and b, and vice versa. So the task can be split into two separate problems:

Determine the greatest common divisor \$ g = \gcd(a, b) \$. This can be done efficiently using the Euclidian algorithm, but even better, Python has a built-in function math.gcd for that.
Count the number of divisors of \$ g \$.

So our function would be

from math import gcd

def num_common_factors(a, b):
    """ Return the number of common factors of a and b.

    Example 1: 10 and 25 have the 2 common factors 1, 5.
    >>> num_common_factors(10, 25)
    2

    Example 2: 720 and 24 have the 8 common factors 1, 2, 3, 4, 6, 8, 12, 24.
    >>> num_common_factors(720, 24)
    8
    """
    g = gcd(a, b)
    return num_divisors(g)

where I also have added two test cases as examples.

Now we are left with the second task: counting the number of divisors. A simple implementation would be

def num_divisors(n):
    """Return the number of divisors of n."""

    count = 0
    for i in range(1, n+1):
        if n % i == 0:
            count += 1
    return count

which again can be done by summing a generator expression:

def num_divisors(n):
    """Return the number of divisors of n."""

    return sum(1 for i in range(1, n+1) if n % i == 0)

But this can be done far more efficiently using the prime number factorization: If
$$
 n = p_1^{e_1} \, p_2^{e_2} \cdots p_k^{e_k}
$$
is the factorization of \$ n \$ into prime numbers \$ p_i \$
with exponents \$ e_i \$, then
$$
 \sigma_0(n) = (e_1+1)(e_2+1) \cdots (e_k+1)
$$
is the number of divisors of \$ n \$, see for example
Wikipedia: Divisor function. Example:
$$
 720 = 2^4 \cdot 3^2 \cdot 5^1 \Longrightarrow
  \sigma_0(720) = (4+1)(2+1)(1+1) = 30 \, .
$$

Here is a possible implementation (translated from the JavaScript here to Python):

def num_divisors(n):
    """Return the number of divisors of n."""

    count = 1   # Number of divisors
    factor = 2  # Candidate for prime factor of n

    # If n is not a prime number then it must have one factor
    # which is <= sqrt(n), so we try these first:
    while factor * factor <= n:
        if n % factor == 0:
            # factor is a prime factor of n, determine the exponent:
            exponent = 1
            n /= factor
            while n % factor == 0:
                exponent += 1
                n /= factor

            # factor^exponent is one term in the prime factorization of n,
            # this contributes as factor exponent + 1:
            count *= exponent + 1

        # Next possible prime factor:
        factor = 3 if factor == 2 else factor + 2

    # Now n is either 1 or a prime number. In the latter case,
    # it contributes a factor 2:
    if n > 1:
        count *= 2

    return count

Testing

As already mentioned above, test cases can be added to each function, and verified with the doctest module. Running the tests after each modification of the code helps to find programming errors.

We already added two test cases to the num_common_factors() function. Now we'll do this for the num_divisors() divisors function, which is essential to the program, and where a programming error can easily lead to wrong results.

One option is to compute a bunch of values using an alternative program and use the results as test cases. I chose PARI/GP to compute the number of divisors of \$ 1, \ldots, 20 \$, \$ 100, \ldots, 119\$, and \$ 10000, \ldots, 10019 \$:

? vector(20, n, numdiv(n))
%1 = [1, 2, 2, 3, 2, 4, 2, 4, 3, 4, 2, 6, 2, 4, 4, 5, 2, 6, 2, 6]
? vector(20, n, numdiv(99+n))
%2 = [9, 2, 8, 2, 8, 8, 4, 2, 12, 2, 8, 4, 10, 2, 8, 4, 6, 6, 4, 4]
? vector(20, n, numdiv(9999+n))
%3 = [25, 4, 8, 4, 12, 16, 4, 2, 24, 2, 32, 8, 6, 8, 8, 4, 12, 16, 4, 4]

and added these as test cases to the num_divisors() function:

def num_divisors(n):
    """Return the number of divisors of n.

    >>> [num_divisors(n) for n in range(1, 21)]
    [1, 2, 2, 3, 2, 4, 2, 4, 3, 4, 2, 6, 2, 4, 4, 5, 2, 6, 2, 6]

    >>> [num_divisors(n) for n in range(100, 120)]
    [9, 2, 8, 2, 8, 8, 4, 2, 12, 2, 8, 4, 10, 2, 8, 4, 6, 6, 4, 4]

    >>> [num_divisors(n) for n in range(10000, 10020)]
    [25, 4, 8, 4, 12, 16, 4, 2, 24, 2, 32, 8, 6, 8, 8, 4, 12, 16, 4, 4]
    """
    ... remaining function omitted ...

Testing is done by running

python -m doctest -v countcommonfactors.py

on the command line (assuming that “countcommonfactors.py” is the name of your Python script).


Breaking down a dollar amount into bill and coin denominations I've started coding C++ again after a 6 month break to prepare for a class I'm taking this quarter. I found this idea and thought it would be fun to try and code.

I originally started with input as a string, and wanted to split it into two ints at ., but this way seemed easier.  Then came the rounding errors. This is why I multiply by such a large number, and 100 was still giving me incorrect values.  I'm really just looking for any feedback on ways I could improve my code, stylistically or otherwise.

cout << "Welcome to Dollar Break Down!" << endl; //print welcome message
cout << "Please enter a dollar amount: ";        // prompt for input of a dollar amount

double amt;                                                         // var to store the amount
cin >> amt;                                                         // read in and store amount
cout << endl;                                                       // spacing

int dollars = amt;                                                  // dollars = (int)amt ex: int(45.97) = 45

// compute dollars
dollars / 100 == 1 ?                                                // check if bill amount is single or plural
    cout << dollars / 100 << " hundred" << endl :                   // print singe
    cout << dollars / 100 << " hundreds" << endl;                   // print plural
dollars = dollars % 100;                                            // compute remainder after removing bill amount
dollars / 50 == 1 ?
    cout << dollars / 50 << " fifty" << endl :                      // repeat for each bill type
    cout << dollars / 50 << " fifties" << endl;
dollars = dollars % 50;
dollars / 20 == 1 ?
    cout << dollars / 20 << " twenty" << endl :
    cout << dollars / 20 << " twenties" << endl;
dollars = dollars % 20;
dollars / 10 == 1 ?
    cout << dollars / 10 << " ten" << endl :
    cout << dollars / 10 << " tens" << endl;
dollars = dollars % 10;
dollars / 5 == 1 ?
    cout << dollars / 5 << " five" << endl :
    cout << dollars / 5 << " fives" << endl;
dollars = dollars % 5;
dollars / 1 == 1 ?
    cout << dollars / 1 << " one" << endl:
    cout << dollars / 1 << " ones" << endl;

cout << endl;                                                   // spacing between dollars and cents

dollars = amt;                                                  // reset dollars to amt for cents calculation
amt *= 10000;                                                   // multiply to remove rounding errors
dollars *= 10000;                                               // same thing
int cents = (amt - dollars) / 100;                              // compute cents without rounding errors

// compute cents
cents / 25 == 1 ?
    cout << cents / 25 << " quarter" << endl :
    cout << cents / 25 << " quarters" << endl;
cents = cents % 25;
cents / 10 == 1 ?
    cout << cents / 10 << " dime" << endl :
    cout << cents / 10 << " dimes" << endl;
cents = cents % 10;
cents / 25 == 1 ?
    cout << cents / 5 << " nickel" << endl :
    cout << cents / 5 << " nickels" << endl;
cents = cents % 5;
cents / 25 == 1 ?
    cout << cents / 1 << " penny" << endl :
    cout << cents / 1 << " pennies" << endl;

Stylistically:

just write std:: instead of using namespace std; (which you didn't show but was implicit)
use a regular if() instead of the ternary operator ?
use compound assignment operators like dollars %= 10; instead of dollars = dollars % 10;
I'm not a big fan of end-of-line comments, but at the very least, try and put them inside the 80 column mark (most editors will show a marker)

Furthermore, it's a good exercise to make your code more general:

put the bills (1, 5, 10, 20, 50, 100) and their strings in a std::map, and write your code as a loop over that map. This way you can start to test with only small bills, and then expand to the general problem
a propos, where are your tests?


Guess the random number game I have created the classic "guess the random number" game:

Generator Interface

 package ishan.guessingGame.randomNumberGenerators;

 public interface RandomNumber {

 public int generate();
}

Generator Implementation

package ishan.guessingGame.randomNumberGenerators;

import java.util.Random;

public class SimpleRandomNumberGenerator implements RandomNumber {

private int limit=50;

public SimpleRandomNumberGenerator(){

}
public SimpleRandomNumberGenerator(int limit){
    this.limit=limit;
}
@Override
public int generate() {

    return generateRandomNumber();
}

private int generateRandomNumber(){
    Random random=new Random();
    return random.nextInt(limit+1);
}
}

Game interface

package ishan.guessingGame.game;

public interface Game {

public void startGame();
}

A Game Impl

package ishan.guessingGame.game;

import java.util.Scanner;

import ishan.guessingGame.randomNumberGenerators.RandomNumber;
import ishan.guessingGame.randomNumberGenerators.SimpleRandomNumberGenerator;

public class SimpleGuessingGame implements Game{

private int randomNumber;
private int difficultyLevel;
private Scanner scanner;
private RandomNumber random;
private int input=999;

public SimpleGuessingGame(){
    scanner = new Scanner(System.in);
}

@Override
public void startGame() {
    init();
    play();
    endGame();
}

private void init(){

    System.out.println("Enter The Difficulty : 1 = Easy , 2 = Moderate , 3 =Difficult");

    String difficulty;
    difficulty=scanner.next();

    if(!checkInputIsNumber(difficulty, true)){
        endGame();
    }

    difficultyLevel=Integer.parseInt(difficulty);

    if(difficultyLevel == 1){
        random=new SimpleRandomNumberGenerator();
    }
    else if (difficultyLevel == 2){
        random=new SimpleRandomNumberGenerator(100);
    }
    else{
        random=new SimpleRandomNumberGenerator(200);
    }
    randomNumber=random.generate();
}

private void play(){
    System.out.println("Input any character to exit..");

    while(true){
        System.out.println(" Guess The Number");
        String tempInput=scanner.next();

        if(!checkInputIsNumber(tempInput , false)){
            endGame();
        }

        input=Integer.parseInt(tempInput);
        if(input==randomNumber){
            System.out.println("You won.Bye!!");
            endGame();
        }
        else if(input > randomNumber){
            System.out.println("Easy there. Guess a smaller Number");
        }
        else{
            System.out.println("Pick a larger one");
        }
    }
}

private boolean checkInputIsNumber(String input , boolean checkForDifficulty) {

    if(checkForDifficulty){
        if(input.matches("^[0-3]")){
            return true;
        }
    }

    else if (input.matches("^[0-9]*")){
        return true;
    }
    return false;
}

private void endGame() {
    scanner.close();
    System.exit(0);
}
}

Main class

package ishan.apps;

import ishan.guessingGame.game.Game;
import ishan.guessingGame.game.SimpleGuessingGame;

public class MainApplication {

public static void main(String a[]){
    Game game=new SimpleGuessingGame();
    game.startGame();
}
}

I have tried to code to interfaces and would like to get suggestions around that area and my code in general.

Looking at the class SimpleRandomNumberGenerator :

Having generate() delegate to a private method is, in this case a bit overkill.
You create a new Random instance for each method invocation. This makes the SimpleRandomNumberGenerator not genrating an even distirbution of random numbers. Make it a private final field initialized at the declaration or in the constructor.
Good use of coding to interfaces, by hiding the implementation details behind an interface.

Then MainApplication :

Spot on : kept simple this class' responsibility is to wire everything together and kick things off.

And now for the meat SimpleGuessingGame :

Several fields of this class can be truned into local variables in the one method that uses them : difficultyLevel, random and input.
startGame() is well implemented, easy to read.
On the whole the class has at least 2 responsibilities :

handling user interaction
keeping track of game state

That is too much. Imagine here you'd want to be able to play the same game but with a graphical UI.

So we want to split this into more classes using interfaces to hide implementation from one another.

I would suggest using these two interfaces :

interface UserInteraction {

    String askDifficulty();

    void close();

    String askToGuess();

    void showHowToEndMessage();

    void showWinMessage();

    void showGuessSmallerMessage();

    void showGuessLargerMessage();
}

interface GameState {

    Result guess(int guess);
}

public enum Result {

    CORRECT, GUESS_LARGER, GUESS_SMALLER
}

checkInputIsNumber() should be split into two separate methods : one for the difficulty and one for the guesses. They can then be named more aptly to increase readability (boolean parameters say so little at the call site)
the loop in the play() method is implemented as an infinite loop, while it is obviously driven by the game state. Meanwhile endGame() quite harshly exits, this makes it harder to embed the game in a larger program. Only the MainApplication class should call System.exit()

There's more to do as a follow up on these remarks, but I'd start there.


HTTP Request/Response class I would like to get feedback on this, which are just simple Response/Request classes.

I tried to keep it simple, so I made two interfaces and two classes:

RequestInterface.php
Request.php
ResponseInterface.php
Response.php

The use Foo\Storage\ArrayHandler as access; is just a class to access and set arrays, so I wouldn't have to use functions like array_key_exists all over the place to check for keys, set, and unset arrays.

RequestInterface.php

namespace Foo\Http;
interface RequestInterface
{
    public function get($key, $default = null);
    public function post($key, $default = null);
    public function server($key, $default = null);
    public function getMethod();
}

ResponseInterface.php

namespace Foo\Http;
interface ResponseInterface
{
    public function setStatusCode($statusCode);
    public function addHeader($name, $value);
    public function setContentType($contentType);
    public function redirect($url);
}

Request.php

namespace Foo\Http;
use Foo\Storage\ArrayHandler as access;
class Request implements RequestInterface
{

    protected $get;
    protected $post;
    protected $files;
    protected $server;
    protected $cookies;

  public function __construct(
         access $get,
         access $post,
         access $files,
         access $server,
         access $cookies
    ) {
        $this->get = $get;
        $this->post = $post;
        $this->files = $files;
        $this->server = $server;
        $this->cookies = $cookies;
    }

    public function get($key, $defaultValue = null){
      return $this->get->get($key, $defaultValue);
    }

    public function post($key, $defaultValue = null){
        return $this->post->get($key, $defaultValue);
    }

    public function server($key, $defaultValue = null){
        return $this->server->get($key, $defaultValue);
    }

    public function getMethod(){
        return $this->server->get('REQUEST_METHOD');
    }
}

Response.php

namespace Foo\Http;

class Response implements ResponseInterface
{

    private $statusCode = 'HTTP/1.1 200 OK';
    private $headers = [];

    public function addHeader($name, $value){
        $this->headers[$name][] = $value;
    }

    public function setHeader($name, $value){
        $this->headers[$name] = [
            (string) $value,
        ];
    }

    public function redirect($url){
        $this->setHeader('Location', $url);
    }

}

You should take a look at Symfony's HttpFoundation component.

They've written various Request/Response objects similar to what you're attempting to do and they are extremely well done. Either I'd use them directly(save your own time). Or if this is an exploratory exercise then take a look at their code base to see how you can improve on your own solution.

It's not as sexy as writing your own objects, but it'll save you a lot of time and effort in the long run.


Lazy computation of primes in Haskell I wanted to give Haskell a try, and I started with a simple exercise I found here.

  Write isPrime :: Integer -> Bool, which determines whether a given
  integer is prime.
  Define primes :: [Integer], the list of all primes.
  Revise isPrime so that it only tests divisibility by prime factors.

My answer to Q1

isPrime :: Integer -> Bool
isPrime v = let maxDiv = floor(sqrt (fromIntegral v))
            in all (\x -> (v `rem` x) /= 0) [2..maxDiv]

This one was easy, except for the explicit numeric types conversions I had a hard time to get right.

My answer to Q2

primes = filter isPrime [0..]

This one was easy, too.

My answer to Q3

It took me several hours to answer this one. I quickly understood that isPrime could leverage the values already computed in the array primes. So, my first attempt was :

isPrime :: Integer -> Bool
isPrime v | v < 2 = False
          | otherwise = all (\x -> (v `rem` x) /= 0) (takeWhile (<v) primes)

isPrime works fine for v=0 and v=1, but hangs forever for v>1. Ok, I get it : the mutual recursion creates an infinite loop, due to takeWhile trying to access a primes element which is not yet computed.

One thing I don't understand though, is why primes !! 0 and primes !! 1 hang forever, too.

I tried many approaches for replacing this (takeWhile (<v) primes) by an expression that would stop before reaching a not-yet-computed value. I came to this solution :

isPrime :: Integer -> Bool
isPrime v | v < 2 = False
          | v == 2 = True
          | otherwise = all (\x -> (v `rem` x) /= 0) (takeWhile' (\t -> t*t<=v) primes)

takeWhile' :: (a -> Bool) -> [a] -> [a]
takeWhile' p (x:xs) = if p x then x : takeWhile' p xs else [x]

This solution works. But I had to define a takeWhile' that works just like takeWhile, but also includes the first non-matching element.

Here are my questions to you :

QA : why do primes !! 0 and primes !! 1 hangs in my first attempt ?

QB : is there a magical takeWhileOnlyForAlreadyComputedElements function ? Or a construct that would prevent the infinite loop of this mutual recursion ?

QC : does takeWhile' already exist in the stdlib ? Or maybe the same effect can be achieved in a simpler way ?

Not an answer to your questions, but a correction for your first, looping attempt for Q3:

Things get much easier if you initially have at least one prime number in your list. You could ensure this by a separate case in isPrime, but why not cheat a little bit, and add it directly?

primes = 2 : filter isPrime [3..]

Then you can use your solution for Q1 without much changes (I took the freedom to kill some parens etc):

isPrime :: Integer -> Bool
isPrime v = let maxDiv = floor $ sqrt $ fromIntegral v
            in all ((/= 0).(v `rem`)) $ takeWhile (<= maxDiv) primes

However, this is not the best way to generate a list of primes. Read the excellent paper http://www.cs.hmc.edu/~oneill/papers/Sieve-JFP.pdf for details.

[Edit]

I forgot to mention http://www.haskell.org/haskellwiki/Prime_numbers with many interesting techniques.


Python - Activity Selection - Greedy Algorithm I've done this activity selection piece of code, which is a greedy approach. I need some feedback/suggestions.

# Python 3
import random
import operator
begin = [random.randint(1, 10) for i in range(10)]
end = [x + random.randint(1, 4) for x in begin]
pair = sorted(list(zip(begin, end)), key=operator.itemgetter(1))
ls = list()
ls.append(pair[0])
for i in pair[1:]:
    if i[0] >= ls[-1][1]:
        ls.append(i)
print(ls, '>>>', len(ls), 'out of', len(pair), 'activities.')

Code readability and style -

I believe that good code should have good style and should be more readable and concise.

Let's introduce you to f-strings -

  To create an f-string, prefix the string with the letter “ f ”.
  The string itself can be formatted in much the same way that you would
  with
  str.format().
  f-strings provide a concise and convenient way to embed python
  expressions inside string literals for formatting.

Which means, instead of using the outdated way of formatting strings -

print(ls, '>>>', len(ls), 'out of', len(pair), 'activities.')

You could make it more concise -

print(f'{ls} >>> {len(ls)} out of {len(pair)} activities.')

NOTE - Requires Python 3.6 and above.

If you don't have Python 3.6 and above, you could use str.format() -

  str.format() is one of the string formatting methods in Python 3, which
  allows multiple substitutions and value formatting. This method lets
  us concatenate elements within a string through positional formatting.

So you could write it like this -

print('{} >>> {} out of {} activities.'.format(ls, len(ls), len(pair)))

I would also put your code into a function -

  A function is a block of organized, reusable code that is used to
  perform a single, related action. Functions provide better modularity
  for your application and a high degree of code reusing.

  As you already know, Python gives you many built-in functions like
  print(), etc. but you can also create your own functions. These
  functions are called user-defined functions.

With this as the function name -

def activity_selection():

You should also consider using an if __name__ == '__main__': guard, like this -

if __name__ == '__main__':
    activity_selection()

Your variables should be more descriptive -

ls = list()

could be -

activity_list = list()

So, overall, your code would then look like this (in terms of code readability and style) -

import random
import operator

def activity_selection():
    begin = [random.randint(1, 10) for i in range(10)]
    end = [x + random.randint(1, 4) for x in begin]
    pair = sorted(list(zip(begin, end)), key=operator.itemgetter(1))
    activity_list = list()
    activity_list.append(pair[0])
    for i in pair[1:]:
        if i[0] >= activity_list[-1][1]:
            activity_list.append(i)
    print(f'{activity_list} >>> {len(activity_list)} out of {len(pair)} activities.')
    # or
    # print('{} >>> {} out of {} activities.'.format(activity_list, len(activity_list), len(pair)))

if __name__ == '__main__':
    activity_selection()

To remain concise and immaculate in writing Python programs, I suggest you have a look at PEP 8, which is Python's official style guide.

Hope this helps!


The Josephus problem in JavaScript I'm totally hooked on CodeEval, and one of the problems on there caught my attention. Here it is, copied from here:

Challenge Description:

Flavius Josephus was a famous Jewish historian of the first century, at the time of the destruction of the Second Temple. According to legend, during the Jewish-Roman war he was trapped in a cave with a group of soldiers surrounded by Romans. Preferring death to capture, the Jews decided to form a circle and, proceeding around it, to kill every j'th person remaining until no one was left. Josephus found the safe spot in the circle and thus stayed alive.Write a program that returns a list of n people, numbered from 0 to n-1, in the order in which they are executed.
Input sample:

Your program should accept as its first argument a path to a filename. Each line in this file contains two comma separated positive integers n and m , where n is the number of people and every m'th person will be executed. e.g.

10,3
5,2

Output sample:

Print out the list of n people(space delimited) in the order in which they will be executed. e.g.

2 5 8 1 6 0 7 4 9 3
1 3 0 4 2

Here's my solution in JavaScript, which succeeded in all test cases:

var fs  = require("fs"); //object for reading in a file
fs.readFileSync(process.argv[2]).toString().split('\n').forEach(function (line) {
    if (line != "") {
        var lineSplit = line.split(",");
        var maxNum = parseInt(lineSplit[0], 10); //the number of people in the circle (n)
        var spacer = parseInt(lineSplit[1], 10); //Every m'th person is executed
        var counter = spacer - 1; //We're working with a zero-based array, so decrement accordingly
        var people = [];
        var deadPeople = [];
        for (var i = 0; i < maxNum; i++) {
            var person = [];
            person = new Array(i.toString(), 1);
            people[i] = person; //a new person is added with a status of 1 (alive)
        }

        while(deadPeople.length < maxNum){
            people[counter][1] = 0; //sadness
            deadPeople.push(people[counter][0]);
            counter += spacer;
            while(people.length > 0 && counter >= people.length){
                counter = counter - people.length;
                regroup(people);
            }
        }

        console.log(deadPeople.join(" "));
    }
});

function regroup(arr) {
    arr.forEach(function(element, index, array) {
        if (element[1] === 0) {
            array.splice(index, 1);
        }
    });
}

All comments are welcome, but I'm especially interested in efficiency.

You can easily find the next death-index with the modulus operation: next = (current + space) % totalStillAlive . No need for fancy loops (see end of answer).
Instead of making an array of arrays (btw, don't create js arrays with new Array, use the array literal []), simply store an array of original indexes, and splice from that on each around until there are no more people left.
The way you're extracting the data is awkward, specifically the spacer definition - why not define it right off the bat as parseInt(...) - 1 ?
It's probably not very important with this input size, but the synchronous line reading is frowned upon; you should use the asynchronous versions unless there's a reason not to.
if (line != "") can just be if (line)
With #1 you don't need regroup, but your forEach loop is an implementation filter (docs)

(as a continuation of #2:

var person = [];
person = new Array(i.toString(), 1);

I don't get what went through your mind at the time, but why not just people[i] = [i, 1] ?)

So, your main algorithm can be simplified to 1 nice loop instead of 3 (discounting the instantiation loop):

function josephus (n, interval) {
    var people = [],
        deaths = [];
    for (var i = 0; i < n; i += 1) {
        people[i] = i;
    }

    var idx = 0,
        len = people.length;
    while (len = people.length) {
        idx = (idx + interval) % len;
        deaths.push(people[idx]);
        people.splice(idx, 1);
    }

    return deaths;
}


Returning an array containing values from two passed in arrays This is a simple piece of code which takes two arrays supplied as parameters, then appends them to  a new array that contains the values from the supplied arrays. Any suggestions for improvement would be greatly appreciated.

public static int[] append(int[] list1, int[] list2) {
    int[] appendedList = new int[list1.length + list2.length];
    for(int i = 0; i < list1.length; i++) {
        appendedList[i] = list1[i];
    }
    for(int i = 0; i < list2.length; i++) {
        appendedList[i + list1.length] = list2[i];
    }
    return appendedList;
}

You could offload some of the work to Arrays.copyOf and System.arraycopy:

public static int[] append(int[] first, int[] second) {
  int[] result = Arrays.copyOf(first, first.length + second.length);
  System.arraycopy(second, 0, result, first.length, second.length);
  return result;
}


Determine all ways a string can be split into valid words, given a dictionary of all words This function word_break(s, dictionary) receives a string s and a set of all valid words dictionary, and must return all possible ways the string s can be split into valid words, if any.

For example, a string catsanddog with dictionary {'cat', 'cats', 'sand', 'and', 'dog'} can be broken in two ways: cats and dog and cat sand dog.

My solution to this recursively breaks the input string at various index (offset), and uses memoization as I realized we may be breaking at the same offset multiple times.

Here is the full code along with a test case, in Python 2.x:

def memoize(fn):
    memo = {}
    def wrapped_fn(*args):
        if args not in memo:
            memo[args] = fn(*args)
        return memo[args]
    return wrapped_fn

def word_break(s, dictionary):
    @memoize
    def _word_break(offset):
        if offset == len(s):
            return []

        breaks_from_offset = []
        if s[offset:] in dictionary:
            breaks_from_offset.append([s[offset:]])

        for i in range(offset+1, len(s)):
            if s[offset:i] in dictionary:
                for break_from_i in _word_break(i):
                    breaks_from_offset.append([s[offset:i]] + break_from_i)

        return breaks_from_offset

    return [' '.join(words) for words in _word_break(0)]

# print word_break('catsanddog', {'cat', 'cats', 'sand', 'and', 'dog'})
# -> [cat sand dog', 'cats and dog']

Based on LeetCode question Word Break II: https://leetcode.com/problems/word-break-ii/description/

As this is an "interview" algorithm question, I would love feedback that keeps that setting in mind. That said, I also appreciate any feedback to help me write more production-ready Python code :)

Honestly it looks like you're doing this wrong. Currently you're splitting the word into \$n^2\$ words and checking if they are in the dictionary. Rather than doing this you can traverse a Trie and loop through the input once. To do this you'd just need a 'partial words list' which is a set of Trie nodes. You then just need to make some lists for the output, and then return the one that you think is best. This can be a bit tricky to do, but the challenge is finding which output list(s) to append to, which you can do when adding new tries to the partial words list.

I'd implement this in the following way:

import collections.abc

class TrieNode(collections.abc.MutableMapping):
    def __init__(self, k):
        self._data = {}
        self._value = k
        self.end = False

    @staticmethod
    def build(iterable):
        root = TrieNode(None)
        for key in iterable:
            root[key] = True
        return root

    @property
    def value(self):
        return self._value

    def _keys(self, key):
        partial = ''
        for k in key:
            partial += k
            yield k, partial

    def _walk(self, data, key, *, build=False):
        if not key:
            raise ValueError()

        node = data
        if not build:
            for k in key[:-1]:
                node = node._data[k]
        else:
            for k, key_ in self._keys(key[:-1]):
                node = node._data.setdefault(k, TrieNode(key_))
        return key[-1], node

    def __getitem__(self, key):
        key, node = self._walk(self, key)
        return node._data[key]

    def __setitem__(self, key, value):
        k, node = self._walk(self, key, build=True)
        node = node._data.setdefault(k, TrieNode(key))
        node.end = value

    def __delitem__(self, key):
        key, node = self._walk(self, key)
        del node._data[key]

    def __iter__(self):
        return iter(self._data)

    def __len__(self):
        return len(self._data)

def word_break(word_string, words):
    words = TrieNode.build(words)
    output = {0: [[]]}
    partials = []
    for i, k in enumerate(word_string, 1):
        new_partials = []
        for partial in partials + [words]:
            partial = partial.get(k)
            if partial is None:
                continue

            new_partials.append(partial)
            if not partial.end:
                continue

            val = partial.value
            prevs = output.get(i - len(val))
            if prevs is not None:
                output.setdefault(i, []).extend([p + [val] for p in prevs])
        partials = new_partials
    return output[len(word_string)]

if __name__ == '__main__':
    words = word_break('catsanddog', {'cat', 'cats', 'sand', 'and', 'dog'})
    print(words)


Replacing certain characters in a QString This question is kinda similar to mine. However, I am using C++ with Qt instead of C#.

How would I efficiently and easily remove all accents and special characters like !"§$%&/()=? etc. from a QString?

So "áche" should turn into "ache" or "über dir" to "ueber dir" (in german ü,ä,ö can be changed into the normalized character with an e appended) or at least "uber dir".

Note: Some people use a $ instead of s in some words so I want to make sure if a file is called "Ke$ha" that it will come out as "Kesha" or at least "KeSha".

The way I do it so far, incomplete, is like this:

void Utils::replaceInvalidChars(QString &str)
{
    if( str.size() == 0 )
        return;

    while( str.at(0) == '.' ) {
        str.remove(0,1);
    }

    str.replace( "/", "-" );
    str.replace( "|", "" );
    str.replace( ":", "-" );
    str.replace("\"", "" );
    str.replace( "?", "" );
    str.replace( "$", "s" );
    str.replace( "*", "" );
    str.replace( ",", "" );
    str.replace( "¿", "" );
    str.replace( "¡", "" );
    str.replace( "!", "" );
    str.replace( "'", "" );

    str.replace( "ë", "e" );
    str.replace( "ê", "e" );
    str.replace( "é", "e" );
    str.replace( "è", "e" );

    str.replace( "ç", "c" );

    str.replace( "ó", "o" );
    str.replace( "ö", "oe" );

    //U's...
    str.replace( "ü", "ue" );
    str.replace( "Ü", "U" );
    str.replace( "ù", "u" );
    str.replace( "Ù", "U" );
    str.replace( "û", "u" );
    str.replace( "Û", "u" );

    //ns
    str.replace( "ñ", "n" );

    //as
    str.replace( "ä", "ae" );
    str.replace( "Ä", "ae" );
    str.replace( "á", "a" );
    str.replace( "Á", "A" );
    str.replace( "à", "a" );
    str.replace( "À", "A" );

    str.replace( "ï", "i" );
}

So at first I remove all dots from the beginning. No matter how many there are. Then I replace certain characters with no character at all and some with a character like 's' or a depending on what it is.

My way is very long, tedious and chaotic. I am about to organize it a little with comments like "N's", "U's" etc. but still, if I make a mistake somewhere it will take way too long until I (eventually) find it.

I would start by separating the data from the logic:

std::vector<std::pair<QString, QString>> replacements {
    { "/", "-" },
    { "|", ""  },
    // ...
    { "ï", "i" }
};

for ( auto const &r : replacements) {
    str.replace(r.first, r.second);
}

I'm not sure the comments about the groups of letters being replaced really add a lot though.

Then I'd at least consider moving the data out of the program itself, and into a data file the program uses, so the replacements you do can be adjusted without re-compiling the code (this is the sort of thing that frequently seems to need a fair amount of "tweaking", since there's no one way of doing it that's obviously correct and the other ways are wrong).


Create database connection and run the insert, delete, update queries class-Update Based on the feedback I have got from the guys reviewing the code as posted in my previous question.

Create database connection and run the insert, delete, update queries class

I have used interfaces and wrote the following code

/* */

namespace SampleInterfaceT1
{
public interface IDbConSetup
{
    DbConnection OpenConnection(string connectionString);

    void CloseConnection();
}

public interface ISqlDataProvider : IDbConSetup
{
    int ExecuteCmd(string dbCmd, SqlParameter param);
}

public class SqlClient : ISqlDataProvider
{
    private SqlConnection _dbconnect;

    private string _conString;

    public DbConnection OpenConnection(string connectionString)
    {
        if (String.IsNullOrWhiteSpace(connectionString))
        {
            throw new ArgumentException("Invalid Connection String");
        }
        _conString = connectionString;

        Console.WriteLine("Sql Connection Opened.");

        _dbconnect = new SqlConnection(_conString);

        _dbconnect.Open();

        return _dbconnect;
    }

     public int ExecuteCmd(string dbCmd, SqlParameter param)
    {
        int _rc;

        if (String.IsNullOrWhiteSpace(dbCmd))
        {
            throw new ArgumentException("Invalid Connection String");
        }
        using (SqlCommand _sqlCmd = new SqlCommand(dbCmd, _dbconnect))
        {
            _sqlCmd.CommandType = CommandType.Text;

            _sqlCmd.Parameters.Add(param);

            _rc = _sqlCmd.ExecuteNonQuery();
        }

        Console.WriteLine("Command executed");

        return _rc;
    }

    public void CloseConnection()
    {
        _dbconnect.Close();
        Console.WriteLine("Sql Connection Closed.");
        _dbconnect.Dispose();
    }

   }
}

/*  Main Program execution */

namespace SampleInterfaceT1
{
 public class Program
 {
    private static void Main(string[] args)
    {
        string conString = @""; /* Connection String*/

        string dbCommand = @"INSERT INTO [dbo].[TestTable]([Name]) VALUES (@Name)";

        ISqlDataProvider SqlDataProviderObject = new SqlClient();

        SqlDataProviderObject.OpenConnection(conString);

        SqlDataProviderObject.ExecuteCmd(dbCommand, new SqlParameter("Name", "HellofromnewDictionary"));

        SqlDataProviderObject.CloseConnection();

        Console.ReadLine();
    }
}
}

I'm a newbie to the concept of interfaces. I'm I on the right path or did I got it completely wrong?

You won't gain any advantage by using abbreviations for your parameter/variable names but you will loose readability. Therefor I would like to suggest to use more descriptive names. Otherwise you will come back in a couple of weeks/months and you will be wondering what e.g int _rc; should represent.

This

if (String.IsNullOrWhiteSpace(dbCmd))
{
    throw new ArgumentException("Invalid Connection String");
}

in your ExecuteCmd() method shows why copy&pasta should be done in a wise way.

In addition you never check if the passed in SqlParameter para is null. In a public method you should always validate all passed in parameters.

As DbConnection is implementing IDisposable you should enclose its usage into a using block. This would also remove the need to have a CloseConnection() method. Hence this would result in removing the IDbConSetup interface.

Instead of having a public OpenConnection method you could pass the connection string to the constructor and make the method private. In addition you won't really need the class level field private SqlConnection _dbconnect;.

Methodlevel variables shouldn't be prefixed with an underscore. This is usually only done for class level variables.

A sample of a simplified DAL which still needs to be expanded.

Note this isn't tested

using System;
using System.Collections.Generic;
using System.Data;
using System.Data.Common;

public class SimplifiedDal
{
    private readonly DbProviderFactory dbFactory;
    private readonly string connectionString;
    public SimplifiedDal(string connectionString, string providerFactory)
    {
        if (string.IsNullOrWhiteSpace(connectionString))
        {
            throw new ArgumentException("Value may not be null or whitespace", "connectionString");
        }
        if (string.IsNullOrWhiteSpace(providerFactory))
        {
            throw new ArgumentException("Value may not be null or whitespace", "providerFactory");
        }
        dbFactory = DbProviderFactories.GetFactory(providerFactory);
        this.connectionString = connectionString;
    }
    public int ExecuteNonQuery(string sql, CommandType commandType, Dictionary<string, object> parameters)
    {
        if (string.IsNullOrWhiteSpace(sql))
        {
            throw new ArgumentException("Value may not be null or whitespace", "sql");
        }

        using (var connection = dbFactory.CreateConnection())
        {
            connection.ConnectionString = connectionString;
            connection.Open();
            using (var command = connection.CreateCommand())
            {
                AddParameters(command, parameters);
                return command.ExecuteNonQuery();
            }
        }
    }
    private void AddParameters(DbCommand command, Dictionary<string, object> parameters)
    {
        if (parameters == null || parameters.Count == 0) { return; }
        string dbFactoryName = dbFactory.GetType().Name;
        if (dbFactoryName == "OleDbFactory" || dbFactoryName == "OdbcFactory")
        {
            AddOleDbOdbcParameters(command, parameters);
            return;
        }

        string parameterPrefix = GetPrefixByFactoryName(dbFactoryName);
        foreach (var kvp in parameters)
        {
            var parameter = command.CreateParameter();
            parameter.ParameterName = parameterPrefix + kvp.Key;
            parameter.Value = kvp.Value;
            command.Parameters.Add(parameter);
        }
    }
    private string GetPrefixByFactoryName(string factoryName)
    {

        switch (factoryName)
        {
            case "SqlClientFactory":
                return "@";
            case "OracleClientFactory":
                return ":";
            case "MySqlClientFactory":
                return "?";
        }
        throw new NotImplementedException();
    }
    private void AddOleDbOdbcParameters(DbCommand command, Dictionary<string, object> parameters)
    {
        foreach (var kvp in parameters)
        {
            var parameter = command.CreateParameter();
            parameter.ParameterName = "?";
            parameter.Value = kvp.Value;
            command.Parameters.Add(parameter);
        }

    }

}

which can be used like so

//MS Access
var msConnectionString = "A MS Access connection string";
var msDal = new SimplifiedDal(msConnectionString, "System.Data.OleDb");

//MySql
var mysqlConnectionString = "A MySQL connection string";
var mysqlDal = new SimplifiedDal(mysqlConnectionString, "System.Data.OleDb");

//MS Sql
var sqlConnectionString = @"A Sql connection string";
var sqlDal = new SimplifiedDal(sqlConnectionString, "System.Data.SqlClient");

you can easily check which factories are installed by using

DataTable table = DbProviderFactories.GetFactoryClasses();


Adding elements of 2 linked list in reverse order I solved a problem with JavaScript which has the following requirements:

Two non-empty linked lists representing two non-negative integers.
Digits stored in reverse order
Each node contains a single digit
Add the two numbers and return it as a linked list

Sample I/O:

Input: (2 -> 4 -> 3) + (5 -> 6 -> 4)
Output: 7 -> 0 -> 8
Explanation: 342 + 465 = 807

My code:

 /**
 * Definition for singly-linked list.
 * function ListNode(val) {
 *     this.val = val;
 *     this.next = null;
 * }
 */
/**
 * @param {ListNode} l1
 * @param {ListNode} l2
 * @return {ListNode}
 */
var addTwoNumbers = function(l1, l2) {
  let l1Str = "";
  let l2Str = "";
  while (l1 !== null) {
    l1Str += l1.val.toString();
    l1 = l1.next;
  }

  while (l2 !== null) {
    l2Str += l2.val.toString();
    l2 = l2.next;
  }

  let sum = (BigInt(l1Str.split("").reverse().join("")) + BigInt(l2Str.split("").reverse().join(""))).toString();

  let resultNodeList = new ListNode(sum[sum.length - 1]);
  let resultHead = resultNodeList;
  for (let i = sum.length - 2;i >= 0;i--) {
    resultNodeList.next = new ListNode(sum[i]);
    resultNodeList = resultNodeList.next;
  }

  return resultHead;

};

Note: I had to use BigInt cause parseInt won't work on big integers as expected.

Although it passed all test cases, what are some ways to improve this code in terms of both time and space complexity?

Here are a list of improvements:

Use ECMAScript features and syntax
Create own implementation of BigInt
Don't use strings !
Create functions to regroup similar code and reduce code duplication

Detailed Explanation:

Generates a ListNode from a given array (array should already be reversed).

generateListNode = (list) => {
 const [ele] = list;
 //create the sentinel (starting point of the node list)
 const sentinel = new ListNode(ele);
 let current = sentinel;
 for(let i = 1; i < list.length; i++){
   //update the next with the new list node
   //set the current to the new next
   current = current.next = new ListNode(list[i]);
 }
 return sentinel;
}

Convert a ListNode to an Array

const convertToValueList = (list) => {
  const res = [];
  //as long as the value "list" is not null it'll continue the loop
  //push the list.val to the result array
  do { res.push(list.val); } while(list = list.next);
  return res;
}

addTwoNumbers has it's own implementation of BigInt

const addTwoNumbers = function(l1, l2) {

  //Convert the ListNode to arrays
  const l1Values = convertToValueList(l1);
  const l2Values = convertToValueList(l2);

  //find the longest of either list
  const len = Math.max(l1Values.length, l2Values.length);

  //when adding a column, value should not exceed 9, otherwise it'll be set to the remainder
  //i.e.: 10 -> 1, 23 -> 2, 30 -> 3
  let remainder = 0;
  //final result in reverse
  const sum = [];
  for (let i = 0; i < len; i++) {
    //add the sum of each value of the list at the "i" position
    //if the value does not exist (i.e.: undefined) use default 0
    //add the remainder if it exists
    const tempSum = (l1Values[i] || 0) + (l2Values[i] || 0) + remainder;
    //update the remainder (any value under 10 is set to 0 because of Math.floor)
    remainder = Math.max(Math.floor(tempSum/10), 0);
    //add the value (modulo 10 so only numbers 0 - 9 are added)
    sum.push(tempSum % 10);
  }

  console.log(sum);
  //generate the list node and return final result
  return generateListNode(sum);
};

Working Example:

class ListNode {
  constructor(val){
    this.val = val;
    this.next = null;
  }
}

generateListNode = (list) => {
  const [ele] = list;
  const sentinel = new ListNode(ele);
  let current = sentinel;
  for(let i = 1; i < list.length; i++){
    current = current.next = new ListNode(list[i]);
  }
  return sentinel;
}

const list1 = generateListNode([2, 4, 3]);
const list2 = generateListNode([5, 6, 4]);

const convertToValueList = (list) => {
  const res = [];
  do { res.push(list.val); } while(list = list.next);
  return res;
}

const addTwoNumbers = function(l1, l2) {

  const l1Values = convertToValueList(l1);
  const l2Values = convertToValueList(l2);

  const len = Math.max(l1Values.length, l2Values.length);

  let remainder = 0;
  const sum = [];
  for(let i = 0; i < len; i++){
     const tempSum = (l1Values[i] || 0) + (l2Values[i] || 0) + remainder;
remainder = Math.floor(tempSum/10);
     sum.push(tempSum % 10);
  }

  console.log(sum);
  return generateListNode(sum);
};

const res = addTwoNumbers(list1, list2);
console.log(res);

Additional:

The best case scenario is if you have the option to add methods to ListNode.

class BodyNode {
  constructor(val){
    this.val = val;
    this.next = null;
  }
}

class ListNode extends BodyNode {

  static FromArray(list){
     if(list.length === 0) throw new Error("Array cannot be of length 0");
     const clone = list.slice();
     const instance = new ListNode(clone.shift());
     while(clone.length > 0){
       instance.add(clone.shift());
     }
     return instance;
  }

  constructor(val){
    super(val);
    this.last = this;
  }

  add(val){
    this.last = this.last.next = new BodyNode(val);
  }

  toArray(){
    const res = [];
    let current = this;
    do { res.push(current.val); } while(current = current.next);
    return res;
  }

}

const list1 = ListNode.FromArray([2, 4, 3]);
const list2 = ListNode.FromArray([5, 6, 4]);

const addTwoNumbers = function(l1, l2) {

  const l1Arr = l1.toArray();
  const l2Arr = l2.toArray();

  const len = Math.max(l1Arr.length, l2Arr.length);

  let remainder = 0;
  const sum = [];
  for(let i = 0; i < len; i++){
     const tempSum = (l1Arr[i] || 0) + (l2Arr[i] || 0) + remainder;
     remainder = Math.floor(tempSum/10);
     sum.push(tempSum % 10);
  }

  console.log(sum);
  return ListNode.FromArray(sum);
};

const res = addTwoNumbers(list1, list2);
console.log(res);


c# IEnumerable Update object I have this code that I am using to update OriginalId using the Id value. I there a better way to do this then using all the loops?

Controller

TemplatesDataService.UpdateRegimenCodeId(template.Regimens);

Interface

void UpdateDosageInstructionsCodeId(IEnumerable<Regimen> Regimens);

DataService

public void UpdateDosageInstructionsCodeId(IEnumerable<Regimen> regimens)
        {
            if (regimens != null)
            {
                foreach (var regimen in regimens)
                {
                    Debug.Write(string.Format("{0} {1}", "regimenId", regimen.Id));
                    regimen.OriginalId = regimen.Id;
                    foreach (var regimenPart in regimen.RegimenParts)
                    {
                        regimenPart.OriginalId = regimenPart.Id;
                        Debug.Write(string.Format("{0} {1}", "regimenPart", regimenPart.Id));
                        foreach (var entries in regimenPart.RegimenEntries)
                        {
                            entries.OriginalId = entries.Id;
                            Debug.Write(string.Format("{0} {1}", "RegimenEntries", entries.Id));
                            foreach (var dosage in entries.DosageInstructions)
                            {
                                Debug.Write(string.Format("{0} {1}", "DosageInstructions", dosage.Id));
                                dosage.OriginalId = dosage.Id;
                                UnitOfWork.Save();
                            }
                        }
                    }
                }
            }
        }

I like the suggestions to let SQL do the foreign key cascading for you. Also, have you considered generating the id's outside the system (e.g. Guid.NewGuid()) and pushing them in?

Whatever the constraints and limitations you're working under, here's my take on a class hierarchy to handle your use case (via inheritance rather than encapsulation).

While it might be a bit over-designed for just handling Id's, the idea here is to create an object model upon which you can expand the application to arbitrary size.  The example also aims to minimize code repetition.

using System;
using System.Collections.Generic;
using System.Diagnostics;

class App_DosageSchedule
{
    public void Run()
    {
        var regimens = new List<Regimen>();
        var schedule = new Schedule(regimens);
        schedule.PreserveIds();
        schedule.Save();
    }
}

public abstract class Component
{
    public Guid Id { get; protected set; }
    public Guid OriginaId { get; protected set; }
    public void PreserveId()
    {
        OriginaId = Id;
        Debug.Write($"{GetType().Name} {Id}");
    }
}

public abstract class ComponentWithChildren : Component
{
    public void PreserveIds()
    {
        PreserveId();
        preserveChildIds();
    }

    protected abstract void preserveChildIds();
}

public class Schedule
{
    public List<Regimen> Regimens { get; private set; }
    public Schedule(List<Regimen> regimens) => Regimens = regimens;

    public void PreserveIds() => Regimens.ForEach(r => r.PreserveIds());

    public void Save()
    {
        ///write to SQL
    }
}

public class Regimen: ComponentWithChildren
{
    public List<RegimenPart> Parts { get; private set; }
    protected override void preserveChildIds() => Parts.ForEach(p => p.PreserveIds());
}

public class RegimenPart : ComponentWithChildren
{
    public List<DosageInstruction> Dosages{ get; private set; }
    protected override void preserveChildIds() => Dosages.ForEach(d => d.PreserveId());
}

public class DosageInstruction : Component
{
}


Retrieve a city list for a country and state The code below is working properly but how do I make this code faster and more efficient. How do I perfect this code?

Let's assume I have 1000 country, state and city records in a mongodb collection. I want to retrieve all the city data by matching country id and state id. Please tell me the perfect way to code.

// @route   GET citylist/:countryid/:stateid
// @desc    get all the city from mongodb for countryid and stateid

router.get("/citylist/:countryid/:stateid", (req, res) => {
  const isValidCountryid = mongoose.Types.ObjectId.isValid( req.params.countryid);
  const isValidStateid = mongoose.Types.ObjectId.isValid(req.params.stateid);

  if (isValidCountryid && isValidStateid) {
    CityModel.find({
      country_id: req.params.countryid,
      state_id: req.params.stateid
    })

      .then(cities => {
        if (!cities || !cities[0]) {
          return res.status(404).json({ msg: "City not found" });
        } else {
          res.json(cities);
        }
      })
      .catch(error => res.status(500).send(error));

  } else {
    res.json({ msg: "Country or State Id is not valid" });
  }
});

how to make this code highly fast, efficient, great time complexity and perfect way of code.

Utilize the query object, this type of querying is more suitable there than as params.
Use object destructuring.
Dry your code, see how we're mapping over [countryId, stateId] to check the validity of the input. Ideally, you would have been using a module like Joi for that.
Maintain a happy path and break out of the function early when erroring.
Use async/await instead of .then().
Return a 400 when the input is invalid.
User proper camelCase (in both the DB and the backend).
Check the array length instead of checking the existence of the first element. So if (arr.length) not if (arr[0]). The reason for that is at some cases the first element of the array could be 'falsy' (such as '' and 0) which can be valid sometimes.
When erroring, return an error field, not a msg one.
Study javascript's array methods such as map, reduce, filter, and every. They will save your life more often than you thought.

router.get("/citylist", async (req, res) => {
  try {

    const { countryid, stateId } = req.query

    const validInput = [countryid, stateid].map(mongoose.Types.ObjectId.isValid).every(Boolean)
    if (!validInput) return res.status(400).json({ error: "Country or State Id is not valid" })

    const cities = await CityModel.find({ countryId, stateId })
    if (!cities || cities.length) return res.status(404).json({ error: "City not found" })

    res.json(cities);

  } catch (error) {
    res.status(500).json({ error })
  }
});


Non-persistent storage for key-value pairs I got an interesting interview question related to JavaScript:

  In JavaScript please create a storing function called storingFn with the following characteristics:

  Read Cafefully

  Save immutable key-values.
  The values can only be accessed by storingFn and should not be exposed in the global scope.
  It can't use Browser storage (localStorage, sessionStorage), cookies, global scope or any other object that is part of the Browser.
  The only global function for this implementation is storingFn (Ignore the assert function).
  And it doesn't matter if the key-values don't persist if you refresh the browser.

I wrote an IIFE as a solution. What do you think of it? I couldn't find any solution online, which makes this question very interesting for me.

var storingFn = (function(){
  var immObj = {};

  function set(key, value){
    // check if key already exist
    if (key in immObj){
      return false;
    }

    // it's a new key , assign it
    immObj[key]  = value;
    return true;
  }

  // verbose but looks better organize
  function get(key){
    return immObj[key];
  }

  var initStore = function(key, value){
    //console.log(immObj);
    // if value is defined
    if (typeof value != 'undefined'){
      return set(key, value);
    }
    // if value is not defined
    else {
      return get(key);
    }
  }

  return initStore;
})();

storingFn('name', 'Nic') //Should return true
storingFn('name') // Should return "Nic"'
storingFn('name', 'Robert') //Should return false

You didn't say if you were able to use ES6 or not, but here's how I would do it if you can.  Note this solution ensures that you won't return or overwrite values that are on the Object prototype of the storage. It's also somewhat more terse, if you get rid of my comments.

const storingFn = (function(){
  const store = {};

  return (key, value) => {
    // you only want to return things that are on 'store' not on its ancestral prototype.
    if(store.hasOwnProperty(key)) {
      return store[key];
    }

    // If we get here, we know it's a set operation, so I'm assuming we need to quietly avoid setting null keys. Throw an error if you prefer.
    if (value === 'undefined') {
      return null;
    }
    // I didn't test with < ES6, as written it doesn't allow overwrite of 'toString' for example.
    store[key] = value;
    return value;
  }
})();

EDIT:

I also decided to actually return the value in the key, regardless of whether we are setting or getting, as your question didn't specify which was appropriate and I thought this gave a more consistent interface.

If you need to return false if the key is already set (right now it'll return the unchanged value) then that's a simple change to the first conditional block.