Median Filter Implementation In Python I implemented median filter in Python in order to remove the salt & pepper noise from the images. It is working fine and all but I would love to hear your advice or opinions.

def median_filter(data, filter_size):
    temp = []
    indexer = filter_size // 2
    for i in range(len(data)):

        for j in range(len(data[0])):

            for z in range(filter_size):
                if i + z - indexer < 0 or i + z - indexer > len(data) - 1:
                    for c in range(filter_size):
                        temp.append(0)
                else:
                    if j + z - indexer < 0 or j + indexer > len(data[0]) - 1:
                        temp.append(0)
                    else:
                        for k in range(filter_size):
                            temp.append(data[i + z - indexer][j + k - indexer])

            temp.sort()
            data[i][j] = temp[len(temp) // 2]
            temp = []
    return data

Code review

Peilonrays points out a mixup with the out-of-bounds testing that is valid. The statement if j ... must be within the loop for k .... One of the results is that you add a different number of elements to temp depending on which boundary you're at. But there are better ways to avoid out-of-bounds indexing, see below.

Your biggest bug, however, is that you write the result of the filter into the image you are processing. Median filtering cannot be done in-place. When you update data[i][j], you'll be reading the updated value to compute data[i][j+1]. You need to allocate a new image, and write the result there.

I would suggest not adding zeros for out-of-bounds pixels at all, because it introduces a bias to the output pixels near the boundary. The clearest example is for the pixels close to any of the corners. At the corner pixel, with a 3x3 kernel, you'll have 4 image pixels covered by the kernel. Adding 5 zeros for the out-of-bounds pixels guarantees that the output will be 0. For larger kernels this happens in more pixels of course. Instead, it is easy to simply remove the temp.append(0) statements, leading to a non-biased result. Other options are to read values from elsewhere in the image, for example mirroring the image at the boundary or extending the image by extrapolation. For median filtering this has little effect, IMO.

You set temp = [] at the very beginning of your function, then reset it when you're done using it, in preparation for the next loop. Instead, initialize it once inside the main double-loop over the pixels:

for i in range(len(data)):
   for j in range(len(data[0])):
      temp = []
      # ...

You're looping over i and j as image indices, then over z and c or k for filter kernel indices. c and k have the same function in two different loops, I would suggest using the same variable for that. z doesn't really fit in with either c or k. I would pick two names that are related in the way that i and j are, such as m and n. The choice of variable names is always very limited if it's just one letter. Using longer names would make this code clearer: for example img_column, img_row, kernel_column, kernel_row.

Out-of-bounds checking

This concludes my comments on your code. Now I'd like to offer some alternatives for out-of-bounds checking. These tests are rather expensive when performed for every pixel -- it's a test that is done \$n k\$ times (with \$n\$ pixels in the image and \$k\$ pixels in the kernel). Maybe in Python the added cost is relatively small, it's an interpreted language after all, but for a compiled language these tests can easily amount to doubling processing time. There are 3 common alternatives that I know of. I will use border = filter_size // 2, and presume filter_size is odd. It is possible to adjust all 3 methods to even-sized filters.

Separate loops for image border pixels

The idea here is that the loop over the first and last border pixels along each dimension are handled separately from the loop over the core of the image. This avoids all tests. But it does require some code duplication (all in the name of speed!).

for i in range(border):
   # here we loop over the kernel from -i to border+1
for i in range(border, len(data)-border):
   # here we loop over the full kernel
for i in range(len(data)-border, len(data)):
   # here we loop over the kernel from -border to len(data)-i

Of course, within each of those loops, a similar set of 3 loops is necessary to loop over j. The filter logic is thus repeated 9 times. In a compiled language, where this is the most efficient method, code duplication can be avoided with inlined functions or macros. I don't know how a Python function call compares to a bunch of tests for out-of-bounds access, so can't comment on the usefulness of this method in Python.

A separate code path for border pixels

The idea here is to do out-of-bounds checking only for those pixels that are close to the image boundary. For pixels within the border, you use a version of the filtering logic with out-of-bounds checking. For the pixels in the core of the image (which is the big majority of pixels), you use a second version of the logic without out-of-bounds checking.

for i in range(len(data)):
   i_border = i < border or i >= len(data)-border
   for j in range(len(data[0])):
      j_border = j < border or j >= len(data)-border
      if i_border or j_border:
         # filtering with bounds checking
      else:
         # filtering without bounds checking

Padding the image

The simplest solution, and also the most flexible one, is to create a temporary image that is larger than the input image by 2*border along each dimension, and copy the input image into it. The "new" pixels can be filled with zeros (to replicate what OP intended to do), or with values taken from the input image (for example by mirroring the image at the boundary or extrapolating in some other way).

The filter now never needs to check for out-of-bounds reads. When the filtering kernel is placed over any of the input image pixels, all samples fall within the padded image.

Since for this type of filtering it is necessary to create a new output image anyway (it is not possible to compute it in-place, as I mentioned before), this is not a huge cost: the original input image can now be re-used as output image.

This solution leads to the simplest code, allows for all sorts of boundary extension methods without complicating the filtering code, and often results in the fastest code too.


Weighted Uniform Strings Problem

Adapted from this HackerRank problem.

Instead of printing YES or NO, I just want to return the Set of all possible weights for an input String.

I found this example to be illustrative

Implementation

Some of my thoughts are in the implementation as comments

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

public class WeightedUniformStrings {
  public static Set<Long> getWeights(String s) {
    // I thought about using streams here, but I'm not using java 8
    Set<Long> weights = new HashSet<>();
    Map<Character, Long> consecutiveCharacterCounts = WeightedUniformStrings.getConsecutiveCharactersCounts(s);
    for (Map.Entry<Character, Long> entry : consecutiveCharacterCounts.entrySet()) {
      if (entry.getValue() != null && entry.getKey() != null) {
        long weight = WeightedUniformStrings.calculateWeight(entry.getKey());
        for (long i = 0; i < entry.getValue() + 1; i++) {
          weights.add(weight * i);
        }
      }
    }
    return weights;
  }

  private static long calculateWeight(char c) {
    // Could add a check to see if character is alphabetical
    return Character.toLowerCase(c) - 'a' + 1;
  }

  private static Map<Character, Long> getConsecutiveCharactersCounts(String s) {
    if (s.isEmpty()) {
      return new HashMap<>();
    }

    Map<Character, Long> consecutiveCharacterCounts = new HashMap<>();
    char[] chars = s.toCharArray();

    // I'm not a big fan of this initialization + for loop - but I haven't thought of a better alternative implementation
    char startingCharacter = chars[0];
    long consecutiveCharacterCount = 1;
    consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);

    for (int i = 1; i < chars.length; i++) {
      char currentCharacter = chars[i];

      if (currentCharacter == startingCharacter) {
        consecutiveCharacterCount++;
      }

      if (currentCharacter != startingCharacter || i == chars.length - 1) {
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount > characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }

      // Doing this logical check twice feels weird
      if (currentCharacter != startingCharacter) {
        startingCharacter = currentCharacter;
        consecutiveCharacterCount = 1;
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount > characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }
    }

    return consecutiveCharacterCounts;
  }
}

Avoid unnecessary math

        for (long i = 0; i < entry.getValue() + 1; i++) {

As you noted in your comment, this should start at 1.

        for (long i = 1; i < entry.getValue() + 1; i++) {

But we can also simplify the end.

        for (int i = 1; i <= entry.getValue(); i++) {

We want to process entry.getValue() and then stop, so do that.  No need to determine the next place where we don't want to be.  The other way works, but this is easier to read.

We also don't need to use long here.  Because Java arrays can't be indexed past the range of an int, we can be sure that none of the counts will exceed the capacity of an int.  Of course, if we change to int here, we should change everywhere else as well.

Use a helper

    consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);

    for (int i = 1; i < chars.length; i++) {
      char currentCharacter = chars[i];

      if (currentCharacter == startingCharacter) {
        consecutiveCharacterCount++;
      }

      if (currentCharacter != startingCharacter || i == chars.length - 1) {
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount > characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }

      // Doing this logical check twice feels weird
      if (currentCharacter != startingCharacter) {
        startingCharacter = currentCharacter;
        consecutiveCharacterCount = 1;
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount > characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }
    }

You can simplify this with a helper method:

public static void update(Map<Character, Long> counts, char c, int count) {
    Long oldCount = counts.get(c);
    if (oldCount == null || count > oldCount) {
        counts.put(c, count);
    }
}

Then we can change the bounds of the for loop.

    for (int i = 1, n = chars.length - 1; i < n; i++) {
      if (chars[i] == startingCharacter) {
        consecutiveCharacterCount++;
        continue;
      }

      update(consecutiveCharacterCounts, startingCharacter, consecutiveCharacterCount);
      startingCharacter = chars[i];
      consecutiveCharacterCount = 1;
    }

    if (chars[chars.length - 1] == startingCharacter) {
      consecutiveCharacterCount++;
    } else {
      update(consecutiveCharacterCounts, startingCharacter, consecutiveCharacterCount);
      consecutiveCharacterCount = 1;
    }

    update(consecutiveCharacterCounts, chars[chars.length - 1], consecutiveCharacterCount);

Now we do one fewer iteration and on exiting the loop we do different processing.  This saves us having to do the extra check on every iteration of the loop.

Now we don't have to compare currentCharacter and startingCharacter three times.  Once is enough.

Using the continue saves us a level of indent over using an else.

We don't need to update the Map for the first one of each letter.  The logic works without that.


Project Euler Problem 14 in Clojure I recently began learning Clojure. In order to get better acquainted with it, I've been tackling Project Euler challenges.

Problem 14 is not really a difficult one. It asks for the number that results in the longest Collatz sequence.

  The following iterative sequence is defined for the set of positive
  integers:

  n → n/2 (n is even) n → 3n + 1 (n is odd)

  Using the rule above and starting with 13, we generate the following
  sequence: 13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1

  It can be seen that this sequence (starting at 13 and finishing at 1)
  contains 10 terms. Although it has not been proved yet (Collatz
  Problem), it is thought that all starting numbers finish at 1.

  Which starting number, under one million, produces the longest chain?

  NOTE: Once the chain starts the terms are allowed to go above one
  million.

Although my solution does solve the problem, I am not sure if it conforms to the Clojure way of doing things, since it seems to be verbose.

Does this conform to functional practices?
Are there any mistakes which result in code being longer than needed?
Is there a way of omitting the loop/recur and doing the same with list functions such as map and apply
Any other suggestions in general?

(ns problem14)

(use '[clojure.test :only (is)])

(defn count-collatz
 "Returns a vector [a b] where b is the
 number that initiated the sequence, and
 a is the number of steps taken to reach 1."
  [input-num]
  (loop [num input-num count 1]
  (if (= num 1)
    (vector count input-num)
    (do (if (= (mod num 2) 0)
      (recur (/ num 2) (inc count))
      (recur (+ (* num 3) 1) (inc count)))))))

;; Test case from the project description.
(is (= (count-collatz 13) [10 13]))

(loop [number 1 peak [0 0]]
    (if (>= number 1e6)
      (str "Longest chain is " (last peak))
      (do (let [result (count-collatz number)]
        (do (if (> (first result) (first peak))
          (recur (inc number) result)
          (recur (inc number) peak)))))))

CAVEAT I am not a clojure programmer. (But hopefully you will agree with what I suggest ;))

Put all your code in functions such that all top level forms are defs. Similarly put your tests in deftest forms and run them with run-tests.

In clojure, too, extract meaningful expressions to their own functions and give them names.
Then promote baked in magic constants in them to parameters so that they are more testable, in REPL and elswhere.

You can use destructuring let to give meaningful names to a function returning a vector.

Applying these general rules

(defn longest-chain [limit]
  (loop [number 1 peak-len 0 peak-val 0]
    (if (>= number limit)
      peak-val
      (do (let [[curr-len curr-val] (count-collatz number)]
        (do (if (> curr-len peak-len)
          (recur (inc number) curr-len curr-val)
          (recur (inc number) peak-len peak-val))))))))))

(defn -main []
  (println "Longest chain is" (longest-chain 1e6)))

Here is how you can rewrite these functions using core library.

(= x 0) is (zero? x)
(zero? (mod num 2)) is (even? x)
repeatedly applying a function without explicit loop/recur is iterate.
You can use for comprehension of a range instead of loop/recur with incing a parameter.
You can also use core library functions that operate on sequences (map, reduce et al) for a more functional style.
You can use threading macros to reduce nesting levels as necessary; and to make your functions read top-down instead of more counter-intuitive inside-out.
You can use (apply max-key k coll) to get "the [value] for which (k x), a number, is greatest."

Code after above changes:

(defn count-collatz [n]
  (let [f (fn [n] (if (even? n) (/ n 2) (+ (* n 3) 1)))]
   (->> n
     (iterate f)
     (take-while #(> % 1))
     count
     inc)))

(defn longest-chain [limit]
    (->> limit
      (range 1)
      (map #(vector (count-collatz %) %))
      (apply max-key first)
      second)

Or using the fact that max-key can get arbitrary function as its first parameter, we get:

(defn longest-chain [limit]
  (apply max-key count-collatz (range 1 limit)))


Longest Common Prefix in an array of Strings Please be brutal, and treat this as if I was at an interview at a top 5 tech firm.

  Question: Write a function to find the longest common prefix string
  amongst an array of strings.

Time it took: 17 minutes

Worst case complexity analysis: n possible array elements, each can have length m that we are traversing, hence O(n*m); m could be a constant, since it's rare to find a string with length, so in a sense, I imagine this could be treated as O(n *constant length(m)) = O(n)?

Space complexity analysis: O(n)

public String longestCommonPrefix(String[] strs) {
    String longestPrefix = "";
    if(strs.length>0){
        longestPrefix = strs[0];
    }
    for(int i=1; i<strs.length; i++){
        String analyzing = strs[i];
        int j=0;
        for(; j<Math.min(longestPrefix.length(), strs[i].length()); j++){
            if(longestPrefix.charAt(j) != analyzing.charAt(j)){
                break;
            }
        }
        longestPrefix = strs[i].substring(0, j);
    }
    return longestPrefix;
}

Pure functions should generally be declared static.

You shouldn't need to take substrings in a loop — that's inefficient.  Think of scanning a two-dimensional ragged array of characters.  Check that all of the first characters match, then that all of the second characters match, and so on until you find a mismatch, or one of the strings is too short.

public static String longestCommonPrefix(String[] strings) {
    if (strings.length == 0) {
        return "";   // Or maybe return null?
    }

    for (int prefixLen = 0; prefixLen < strings[0].length(); prefixLen++) {
        char c = strings[0].charAt(prefixLen);
        for (int i = 1; i < strings.length; i++) {
            if ( prefixLen >= strings[i].length() ||
                 strings[i].charAt(prefixLen) != c ) {
                // Mismatch found
                return strings[i].substring(0, prefixLen);
            }
        }
    }
    return strings[0];
}

Space complexity: O(1).

Worst-case time complexity: O(n m) to scan every character in every string.


LeetCode: Trapping Rain Water DP C# Please review for performance and C# style.

I already solved this question in 1 way.
LeetCode: trapping rain water C#

I tried also the dynamic programming approch

https://leetcode.com/problems/trapping-rain-water/

using System;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/trapping-rain-water/
    /// </summary>
    [TestClass]
    public class TrappingRainWater
    {
        [TestMethod]
        public void TestMethod1()
        {
            int[] height = { 0, 1, 0, 2, 1, 0, 1, 3, 2, 1, 2, 1 };
            Assert.AreEqual(6, TrapDynamicProgramming(height));
        }

        public int TrapDynamicProgramming(int[] height)
        {
            if (height == null || height.Length == 0)
            {
                return 0;
            }

            int ans = 0;
            int size = height.Length;
            var leftMax = Enumerable.Repeat(0, size).ToList();
            var rightMax = Enumerable.Repeat(0, size).ToList();

            //compute the max height from the left for each cell
            //starting from the left
            leftMax[0] = height[0];
            for (int i = 1; i < size; i++)
            {
                leftMax[i] = Math.Max(height[i], leftMax[i - 1]);
            }

            //compute the max height from the right for each cell
            //starting from the right..
            rightMax[size - 1] = height[size - 1];
            for (int i = size - 2; i >= 0; i--)
            {
                rightMax[i] = Math.Max(height[i], rightMax[i + 1]);
            }

            //the amount of water is the minimal between left and right height.
            // and we need to remove the height of the cell, think of this
            // like where the ground starts
            for (int i = 1; i < size - 1; i++)
            {
                ans += Math.Min(leftMax[i], rightMax[i]) - height[i];
            }

            return ans;
        }
}

There is no reason to use lists instead of arrays, new int[size] will give you an array full of 0s without any overhead.

You can get away with creating only one array (leftMax) and then while computing the values of rightMax, instead of saving them into an array, complete the full calculation of how much to add to ans. Like this:

public int TrapDynamicProgramming(int[] height)
{
    if (height == null || height.Length == 0)
    {
        return 0;
    }

    int ans = 0;
    int size = height.Length;
    int[] leftMax = new int[size];

    leftMax[0] = height[0];
    for (int i = 1; i < size; i++)
    {
        leftMax[i] = Math.Max(height[i], leftMax[i - 1]);
    }

    int rightMax = height[size - 1];
    for (int i = size - 2; i >= 0; i--)
    {
        rightMax = Math.Max(height[i], rightMax);
        ans += Math.Min(leftMax[i], rightMax) - height[i];
    }

    return ans;
}


Test all properties with single test function In writing unit tests for c# functions, there is a good case made for testing properties. In testing those properties, I felt there should be a few goals.

Provide a wide range of inputs
Test additional features like PropertyChanged
Don't write a ton of very repetitive test code

So I came up with this function for a specific class (although it could even be generalized with a templated method for all classes) that essentially gives a property name, a value to set the property to, the expected value to receive out, and a list of PropertyChanged names that should be fired as a result. This is accomplished by taking advantage of MSTestv2's DataTestMethod and DataRow attributes.

Looking for feedback on the quality/durability/usefulness of this code.

[TestMethod()]
[DataTestMethod]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", null, null, new string[] { "Name" })]
[DataRow("Name", "", "", new string[] { "Name" })]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", "A really very super duper long string to test the overall allowed length", "A really very super duper long string to test the overall allowed length", new string[] { "Name" })]
[DataRow("Address", 0, 0, new string[] { "Address", "AbsAddress"})]
[DataRow("Address", -1, null, new string[] { "Address", "AbsAddress" })]
[DataRow("Address", 300, 300, new string[] { "Address", "AbsAddress" })]
[DataRow("Address", int.MaxValue, int.MaxValue, new string[] { "Address", "AbsAddress" })]
public void ConfigItem_AllProperties_Test(string propName, object set, object expected, string[] propChangedNames)
{
    var item = new ConfigItem();
    var propsChanged = new Dictionary<string, bool>();
    foreach (var s in propChangedNames)
    {
        propsChanged.Add(s, false);
    }
    item.PropertyChanged += (sender, args) =>
    {
        if (propsChanged.ContainsKey(args.PropertyName))
        {
            propsChanged[args.PropertyName] = true;
        }
    };
    typeof(ConfigItem).GetProperty(propName).SetValue(item, set);
    object actual = typeof(ConfigItem).GetProperty(propName).GetValue(item);
    Assert.AreEqual(expected, actual);
    foreach (var pair in propsChanged)
    {
        Assert.IsTrue(pair.Value, pair.Key);
    }
}

too vague & hiding test cases

[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", null, null, new string[] { "Name" })]
[DataRow("Name", "", "", new string[] { "Name" })]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", "A really very super duper long string to test the overall allowed length", "A really very super duper long string to test the overall allowed length", new string[] { "Name" })]

Tests should be testing very specific things. This test however isn't doing that. I looks like it's not simply testing the PropertyChanged event. If it was, then a single DataRow would be enough to tell whether the event fires or not.

It seems to also test some conditions that might prevent it from firering. Such tests should be done separately because here it's not clear what conditions you are testing.

too many hardcoded strings

Technically I find it's ok but to my taste there are too many hardcoded strings. I wouldn't say anything if it was just data but since they are property names I don't like them being strings.

You could use nameof to replace them with something more dynamic but then the test would not be very readable anymore.

extend Assert & use expressions

Instead it'd be a better idea to write an extension for the Assert.That property that you could use for testing the event in this way:

Assert.That.PropertyChanged(
    new Customer(),
    x => x.FirstName = "John",
    x => x.FirstName,
    x => x.FullName
);

where the signature of the method is:

public static void PropertyChanged<T>(
    this Assert assert,
    T obj,
    Action<T> setProperty,
    params Expression<Func<T>>[] changedProperties
)
{
    ...
}

now you can do the reflection magic here.

This is just an idea but you could of course have a couple of such extensions. One very gneric and another one that e.g. assumes that the property being set always fires an event for it.

confusing naming

foreach (var s in propChangedNames)

If you must give variables single letter names then let them at least be derived from the main name. Here p or pcn would be appropriate. s does not stand for anything. The only exception is x in lambda expressions which stands for current-item.

misses the point

  although it could even be generalized with a templated method for all classes

Then do this because otherwise you failed to reach the third goal which is:

  Don't write a ton of very repetitive test code


Simple TCP Send Receive with multithreading c# Wondering how this sample program can be made better. Specifically what exceptions should I expect to handle? Right now it's working as expected. I put a breakpoint after the statement:

int numBytesReadFromStream = stream.Read(bytes, 0, bytes.Length);

to check to see if the bytes variable had data in it and it did.

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

namespace ConsoleApp2
{
    class Program
    {
        public static void Main()
        {
            var localIP = IPAddress.Parse("10.10.10.117");
            var listenerPort = 20000;
            var senderPort = listenerPort + 2;
            IPEndPoint listenerEndpoint = new IPEndPoint(localIP, listenerPort);
            IPEndPoint senderEndpoint = new IPEndPoint(localIP, senderPort);
            TcpListener tcpListener = new TcpListener(listenerEndpoint);
            StarttcpListenerThread(tcpListener);
            Thread.Sleep(300);
            StartTCPSendingThread(listenerEndpoint, senderEndpoint);
            while (true)
            {
            }
        }

        static void StarttcpListenerThread(TcpListener tcpListener)
        {
            tcpListener.Start();
            Thread tcpListenerThread = new Thread(() =>
            {
                while (true)
                {
                    try
                    {
                        byte[] bytes = new byte[1024];
                        TcpClient currentConnection = tcpListener.AcceptTcpClient();
                        NetworkStream stream = currentConnection.GetStream();
                        int numBytesReadFromStream = stream.Read(bytes, 0, bytes.Length);
                    }
                    catch (Exception e)
                    {
                        Console.WriteLine(e.Message);
                    }

                }
            });
            tcpListenerThread.Start();
        }

        static void StartTCPSendingThread(IPEndPoint listenerEndpoint, IPEndPoint senderEndpoint)
        {
            TcpClient tcpClient = new TcpClient(senderEndpoint);
            tcpClient.Connect(listenerEndpoint);
            Thread tcpSendingThread = new Thread(() =>
            {
                while (true)
                {
                    try
                    {
                        tcpClient.Client.Send(new byte[] { 1, 2, 8 });
                    }
                    catch (Exception e)
                    {
                        Console.WriteLine(e.Message);
                    }
                }
            });
            tcpSendingThread.Start();
        }
    }
}

There are several things that can be improved on the current code.

Starting by the listener, if you want to create a TcpListener you can use

var tcpListener = new TcpListener(IPAddress.Loopback, 20000); or
var tcpListener = new TcpListener(IPAddress.Any, 20000);

The Main method looks a bit like spaghetti code.
Specially because you keep alternating statements between things you need for the TcpListener and things that you need for TcpClient.

One possible thing you could do about this is to let StartTcpListenerThread create the TcpListener himself.
Same goes for StartTcpSendingThread.

  var senderPort = listenerPort + 2;

You don't have to be explicit about the port the client should use, since the operating system
will attribute a port dynamically as long as you use TcpClient tcpClient = new TcpClient("localhost", 20000);

  while (true)    {   }

This is great, if you want to increase your power bill by putting your CPU usage on 100.
If you don't want to do that however, you can use Console.ReadLine(); or Thread.Sleep(Timeout.Infinite);

If possible you should also be consistent on whatever you use var or not.

Here is the code with the things I stated applied to it.

public static readonly int Port = 20000;

public static void Main()
{
    StartTcpListenerThread();
    Thread.Sleep(300);
    StartTcpSendingThread();
    Console.ReadLine();
}

static void StartTcpListenerThread()
{
    var tcpListener = new TcpListener(IPAddress.Any, Port);
    tcpListener.Start();
    Thread tcpListenerThread = new Thread(() =>
    {
        while (true)
        {
            try
            {
                var bytes = new byte[1024];
                var currentConnection = tcpListener.AcceptTcpClient();
                var stream = currentConnection.GetStream();
                var numBytesReadFromStream = stream.Read(bytes, 0, bytes.Length);
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }

        }
    });
    tcpListenerThread.Start();
}

static void StartTcpSendingThread()
{
    var tcpSendingThread = new Thread(() =>
    {
        var tcpClient = new TcpClient("localhost", Port);
        while (true)
        {
            try
            {
                tcpClient.Client.Send(new byte[] { 1, 2, 8 });
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }
        }
    });
    tcpSendingThread.Start();
}

Regarding your question about exceptions it seems reasonable to me, to catch the exception and display what is wrong to the console. Most of the time this will be due to some network issue and there isn't much you can do on those scenarios. Keeping the listener listening requests may or may not be a good decision but I would say it's at least a somewhat decent decision for a first approach.


Stack-based state machine Is everything alright with this code? What can I add to it?

state.h:

#pragma once

class State
{
public:
    virtual void update(float dt) = 0;

    virtual void draw(float dt) = 0;
};

state_machine.h:

#pragma once
#include <memory>
#include <stack>
#include "state.h"

using StateRef = std::shared_ptr<State>;

class StateMachine
{
public:
    void push_state(const StateRef& state);

    void push_state(State* state);

    template<class T>
    void push_state();

    StateRef pop_state();

    StateRef get_current_state() const;

    void update(float dt);
private:
    std::stack<StateRef> stack;
};

template<class T>
inline void StateMachine::push_state()
{
    push_state(StateRef(new T));
}

state_machine.cpp:

#include "state_machine.h"
#include "state.h"

void StateMachine::push_state(const StateRef& state)
{
    stack.emplace(state);
}

void StateMachine::push_state(State* state)
{
    push_state(StateRef(state));
}

StateRef StateMachine::pop_state()
{
    auto state = get_current_state();

    stack.pop();

    return state;
}

StateRef StateMachine::get_current_state() const
{
    return stack.top();
}

void StateMachine::update(float dt)
{
    if(!stack.empty())
    {
        auto state = get_current_state();

        state->update(dt);
        state->draw(dt);
    }
}

Dummy usage example:

#include <sm/state_machine.h>
#include <iostream>

struct Foo: public State
{
    void update(float dt) override
    {
        std::cout << "(updated)" << std::endl;
    }

    void draw(float dt) override
    {
        std::cout << "(drawed)" << std::endl;
    }
};

int main()
{
    StateMachine machine;

    machine.push_state<Foo>();

    machine.update(0.0f);

    machine.pop_state();

    machine.update(0.0f);

    return 0;
}

GitHub repo

How can I add and use "Controller" component? (component that handles input and etc.). Is it necessary? (MVC pattern)

Good stuff, I particularly like State being a purely abstract interface, that's exactly how to best do modern inheritance.

State needs a virtual destructor

This is 100% required in this case, since you delete states from pointers of State.

Reduce API surface

You have too many push_state() functions

void push_state(const StateRef& state);
void push_state(State* state);

template<class T>
void push_state();

You would be better off having just the single:

void push_state(const StateRef& state);

What looks like "convenience" to you just ends up being "confusing" for possible users of the class as they have to wonder "which one of these should I be using?"

Take objects by value if you are taking ownership

void push_state(const StateRef& state);

should be:

void push_state(StateRef state);

Your function currently will always go through a copy-constructor, and for shared_ptr<> this is definitely non-trivial as that has to be thread safe.

By taking the StateRef by value, this gives users control over whether to provide a RValue, move in a LValue, or provide a copy of some other object. Maximum flexibility without any overloads.

This brings us to:

Prefer unique_ptr over shared_ptr

I see no shared ownership scemantics here, unique_ptr would be better simply for performance reasons. But you may have other parts of the larger codebase that warrant it. That comment is just about the code I see in isolation.

That's pretty much all I've got here.


Write nested loops as linq query I am tracking trains and trying to identify individual trains seen multiple times at different points through the IDs of the wagons on them when spotted.

// create a lookup of all tracked trains for each wagon
IEnumerable<Train> trains = GetTrains();

var wagonTrains = new Dictionary<int, List<Train>>();
foreach (Train t in trains)
{
  foreach (int w in t.WagonsInTrain)
  {
    if (!wagonTrains.ContainsKey(w))
    {
      wagonTrains.Add(w, new List<Train>());
    }
    wagonTrains[w].Add(t);
  }
}

Is there a better way do to what I am doing in this code segment? Perhaps some chained linq operations? Is there a particular name for the kind of operation I am using here?

create a lookup of all tracked trains for each wagon

That's pretty much what the ToLookup() method is for. You just need a bit more LINQ  to get a collection of (train, wagon) pairs, so that ToLookup() can work:

var wagonTrains =
    (from train in trains
     from wagon in train.WagonsInTrain
     select new { train, wagon })
        .ToLookup(x => x.wagon, x => x.train);


Merge Sort from Scratch in Python I built a merge sort in python just to have a more thorough understanding of how the algorithm works.  What I have written works fine, but I assume there is room for improvement and ways to make my implementation more efficient.

I would appreciate any feedback or critiques!

"""
2019-10-17

This project simply re-creates common sorting algorithms
for the purpose of becoming more intimately familiar with
them.
"""

import random
import math

#Generate random array
array = [random.randint(0, 100) for i in range(random.randint(5, 100))]
print(array)

#Merge Sort
def merge(array, split = True):
    split_array = []
    #When the function is recursively called by the merging section, the split will be skipped
    continue_splitting = False
    if split == True:
        #Split the array in half
        for each in array:
            if len(each) > 1:
                split_array.append(each[:len(each)//2])
                split_array.append(each[len(each)//2:])
                continue_splitting = True
            else:
                split_array.append(each)
    if continue_splitting == True:
        sorted_array = merge(split_array)
        #A return is set here to prevent the recursion from proceeding once the array is properly sorted
        return sorted_array
    else:
        sorted_array = []
        if len(array) != 1:
            #Merge the array
            for i in range(0, len(array), 2):
                #Pointers are used to check each element of the two mergin arrays
                pointer_a = 0
                pointer_b = 0
                #The temp array is used to prevent the sorted array from being corrupted by the sorting loop
                temp = []
                if i < len(array) - 1:
                    #Loop to merge the array
                    while pointer_a < len(array[i]) or pointer_b < len(array[i+1]):
                        if pointer_a < len(array[i]) and pointer_b < len(array[i+1]):
                            if array[i][pointer_a] <= array[i + 1][pointer_b]:
                                temp.append(array[i][pointer_a])
                                pointer_a += 1
                            elif array[i + 1][pointer_b] < array[i][pointer_a]:
                                temp.append(array[i + 1][pointer_b])
                                pointer_b += 1
                        #If the pointer is equal to the length of the sub array, the other sub array will just be added fully
                        elif pointer_a < len(array[i]):
                            for x in range(pointer_a, len(array[i])):
                                temp.append(array[i][x])
                                pointer_a += 1
                        elif pointer_b < len(array[i + 1]):
                            for x in range(pointer_b, len(array[i + 1])):
                                temp.append(array[i + 1][x])
                                pointer_b += 1
                else:
                    for each in array[i]:
                        temp.append(each)
                sorted_array.append(temp)
            if len(sorted_array) != 1:
                #Recursively sort the sub arrays
                sorted_array = merge(sorted_array, False)
    return sorted_array

sorted_array = merge([array])[0]
print()
print(sorted_array)

At the end of the code, it looks like merge() is called by nesting the array in a list and the result is the 0th element of the returned list.  This is an unusual calling convention, is different than the way the built in sorted() is called, and it is not documented, which would likely lead to many errors.  If this interface is needed for the algorithm to work, it would be better if merge() handled these details an then called another function.  For example:

def merge(array):
    return _merge([array], True)

def _merge(array, True):
    ... same as your original code ...

The boolean argument split appears to control whether merge() splits the array into two smaller parts or merges two smaller parts together.  This complicates the logic and makes it more difficult to follow.  It would be better two have two separate functions.

It is also appears that the code takes parts of a recursive implementation and parts from an iterative implementation.  For example, merge is called recursively to split the array into smaller arrays, but for i in range(0, len(array), 2): loops over a list of subarrays and merges them in pairs.

The essence of merge sort is: if the array isn't sorted, split it into two parts, sort both parts separately, and then merge the two parts together.  Here is a recursive implementation:

def merge_sort(array):
    if is_not_sorted(array):
        # split array in two
        middle = len(array) // 2
        left_half = array[:middle]
        right_half = array[middle:]

        # sort each part
        left_half = merge_sort(left_half)
        right_half = merge_sort(right_half)

        array = merge_halves(left_half, right_half)
        return array

    else:
        # array is already sorted
        return array

Usually, if is_not_sorted(): is implemented as if len(array) > 1: (if there is more than one item in the array, it is presumed to be unsorted).  But some implementations (like Python's builtin sorted or list.sort) check small lists to see if they are already sorted.  Also, the two halves are usually sorted using a recursive call to merge_sort(), but some implementations may use a different sorting algorithm if the array is small.

merge_halves() is then basically the same as the while pointer_a < len(array[i])... loop in your code (with a few simplifications):

def merge_halves(left, right):
    merged_array = []
    pointer_a = 0
    pointer_b = 0
    while pointer_a < len(left) and pointer_b < len(right):
        if left[pointer_a] <= right[pointer_b]:
            merged_array.append(left[pointer_a])
            pointer_a += 1

        elif right[pointer_b] < left[pointer_a]:
            merged_array.append(right[pointer_b])
            pointer_b += 1

    #At the end of one subarray, extend temp by the other subarray
    if pointer_a < len(left):
        merged_array.extend(left[pointer_a:])

    elif pointer_b < len(right):
        merged_array.extend(right[pointer_b:])

    return merged_array

An iterative version might look something like this (code on the fly, so may have errors):

def merge(array):
    # make all the sub arrays 1 item long.  A more intelligent version might
    # split it into runs that are already sorted
    array = [[item] for item in array]

    while len(array) > 1:
        temp = []

        for i in range(0, len(array), 2):
            #Pointers are used to check each element of the two mergin arrays
            pointer_a = 0
            pointer_b = 0

            #Loop to merge the array
            while pointer_a < len(array[i]) and pointer_b < len(array[i+1]):
                if array[i][pointer_a] <= array[i + 1][pointer_b]:
                    temp.append(array[i][pointer_a])
                    pointer_a += 1

                else:
                    temp.append(array[i + 1][pointer_b])
                    pointer_b += 1

            if pointer_a < len(array[i]):
                temp.extend(array[i][x])

            elif pointer_b < len(array[i + 1]):
                temp.extend(array[i + 1][x])

        #there were an odd number of sub_arrays, just copy over the last one
        if len(sub_arrays) % 2:
            temp.append(sub_arrays[-1])

        array = temp

    return array[0]


Printing binary string in assembly I'm writing a program to print binary string of a hardcoded word. Here is how it looks like currently:

main.asm

section .text
    global _start
    extern _print_binary_content

_start:
    push word [word_to_print] ; pushing word. Can we push just one byte?
    call _print_binary_content

    mov rax, 60
    mov rdi, 0
    syscall

section .data
    word_to_print: dw 0xAB0F

printer.asm

SYS_BRK_NUM equ 0x0C
BITS_IN_WORD equ 0x10
SYS_WRITE_NUM equ 0x01
STD_OUT_FD equ 0x01
FIRST_BIT_BIT_MASK equ 0x01
ASCII_NUMBER_OFFSET equ 0x30

section .text
    global _print_binary_content

_print_binary_content:
    pop rbp

    xor ecx, ecx ;zeroing rcx
    xor ebx, ebx ;zeroing rbx
    pop bx  ;the word to print the binary content of

    ;sys_brk for current location
    mov rax, SYS_BRK_NUM
    mov rdi, 0
    syscall
    ;end sys_brk

    mov r12, rax ;save the current brake location

    ;sys_brk for memory allocation 16 bytes
    lea rdi, [rax + BITS_IN_WORD]
    mov rax, SYS_BRK_NUM
    syscall
    ;end sys_brk

    xor ecx, ecx
    mov cl, byte BITS_IN_WORD - 1; used as a counter in the loop below

    loop:
        mov dx, bx
        and dx, FIRST_BIT_BIT_MASK
        add dx, ASCII_NUMBER_OFFSET
        mov [r12 + rcx], dl
        shr bx, 0x01
        dec cl
        cmp cl, 0
        jge loop

    mov rsi, r12
    mov rax, SYS_WRITE_NUM
    mov rdi, STD_OUT_FD
    mov rdx, BITS_IN_WORD
    syscall

    push rbp ; pushing return address back
    ret

If I compile link and run this program it works. But the question is about performance and maybe conventions of writing assembly programs. In the file printer.asm I cleaned ecx twice which looks kind of not optimal. Maybe some registers were used not by their purpose (I used intel-manual).

Can you please help me to improve this very simple program?

Low level programming is as much about optimization as it is legibility.
As you've noted, yours works, but I'd like to inject a few of my personal preferences here.

_start:

    ; Grab a buffer from stack suitably large enough to convert a 64 bit number

        push    rbp
        mov     rpb, rsp
        sub     rsp, 64                 ; Always keep RSP at least DWORD aligned

I prefer to keep functions distinct. Even though encorporating print into the conversion function
is doable, but maybe this conversion code could be used somewhere that printing isn't required.

        mov     esi, Value              ; Establish pointer to 16 bit value
        mov     rdi, rbp                ; Point to end of ASCII buffer
        call    Bin2Asc                 ; Return a pointer in RSI and characters in RDX

To accomodate SYSCALLs, have the function return values in register(s) that will be required.

        mov     eax, SYS_WRITE
        syscall

        leave                           ; Kill procedure frame
        xor     eax, eax
        mov     edi, eax
        mov      cl, SYS_EXIT
        syscall

NOTE: A lot of times I'm using 32 bit registers and that is because I'm taking advantage of
processors sign extension mechanism, where some instruction sign extend into 64 bits.

Bin2Asc:

        xor     eax, eax                ; Clear accum
        mov     edx, eax                ; EDX is what SYS_WRITE is going to need
        lodsw
        std                             ; Change direction so buffer is populated in reverse

  .L0:  push    rax
        and      al, 1                  ; Isolate bit
        xor      al, '0'                ; Convert to "0" or "1"
        stosb                           ; Write to buffer
        inc      dl                     ; Bump character cound
        pop     rax
        shr     eax, 1                  ; Until RAX = 0
        jnz     .L0

        cld                             ; Reset direction flag
        inc     edi                     ; Point back to first character
        ret

Might this code have ERRORS, it may very well have, but as I'm not doing it on a Linux box and
I usually like to check my work, but it does demonstrate a concept that is optimized at least a bit.


Assigning values from a numerical array to an associative array I have this function here:

<?php
public function assignData($data)
{
    $aSqlData['company'] = $data[0];
    $aSqlData['tx_trouserextend_contactperson'] = $data[1];
    $aSqlData['username'] = $data[2];
    $aSqlData['tx_trouserextend_commission'] = $data[3];
    $aSqlData['tx_trouserextend_wincap_version'] = $data[4];
    $aSqlData['tx_trouserextend_wincap_link'] = $data[5];
    $aSqlData['tx_trouserextend_contract_due'] = $data[6];
    $aSqlData['tx_trouserextend_pos_version'] = $data[7];
    $aSqlData['tx_trouserextend_pos_link'] = $data[8];
    $aSqlData['tx_trouserextend_quick_select_version'] = $data[9];
    $aSqlData['tx_trouserextend_quick_select_link'] = $data[10];
    $aSqlData['password'] = $data[11];

    $this->userdata = $aSqlData;
}

This goes on for like 10 more items with this way of assigning. Is there maybe a better way to iterate through it? For instance, just adding the indices and run assign every numerical index to the corresponding associative array-key?

Even though it works, it is not that nice to maintain, imho. Ideas how to improve this? As you can imagine, if the csv-file grows and gets more fields, I'd have to add a line of code for every new column. Thought there would/could be a better way.

This has been answered here: https://stackoverflow.com/a/3000342/561731

array_combine

In your case:

$this->userdata = array_combine(['company','tx_trouserextend_contactperson',...], $data);

Can also just have anarray of $keys to match to your data and then use array_combine


Checking user permission, where permission is granted for specific EmployeeGroup that has self reference The users get permissions to do specific things for a specific group of employees. These employee groups have a self reference. So an employee group (C) can be a child of group (B), and group (B) is child of (A). The user is granted permission to employee group (A), so if I want to check if the permission is granted for that user on employee group (C) it should give me true because the user is granted the permission on the parent or grandparent group. Currently, I am using this method and it works fine but I find it ugly and complicated a bit. Is there a more elegant way to do it?
This is the method:
public static bool CheckPermission(Session session, string right, int? emplooyeGroupID = null)
{
    using (Context db = new Context())
    {
        bool pass = false;
        var assigns = db.SecurityAssigns
            .Where(g => g.UserID == session.UserID);

        // if employeeGroupID is supplied we need to do some work here...
        if (emplooyeGroupID.HasValue)
        {
            // first, we get all 'groups' that have the requested SecurityRight
            assigns = assigns.Where(g => g.SecurityGroup.SecurityRightsInGroups.Any(r => r.SecurityRight.Right == right));

            foreach (var grp in assigns)
            {
                // if the employee group matches, then its a pass
                if (grp.EmployeeGroupID == emplooyeGroupID)
                {
                    pass = true;
                    break;
                }

                // if we reach here, it means the EmployeeGroupID does not match the EmployeeGroupID assigned for the SecurityRight.
                // so we need to check if the requested EmployeeGroupID is a child of the EmployeeGroupID assigned to the SecurityRight
                var curGroup = db.EmployeeGroups.Single(g => g.ID == emplooyeGroupID);

                // loop through parents of the requested EmployeeGroup all the way up
                while (curGroup.ParentGroupID.HasValue)
                {
                    // is the parent authorized?
                    if (curGroup.ParentGroupID == grp.EmployeeGroupID)
                    {
                        pass = true;
                        break;
                    }

                    // parent is not authorized, go up one more step
                    curGroup = db.EmployeeGroups.Single(g => g.ID == curGroup.ParentGroupID);
                }
            }

        }
        else
        {
            pass = assigns.Any(g => g.SecurityGroup.SecurityRightsInGroups.Any(r => r.SecurityRight.Right == right));
        }

        if (!pass)
            throw new FaultException("Not Authorized!", new FaultCode(Common.ErrorCodes.NOT_AUTHORIZED_ERROR_CODE));

        return true;
    }
}

This is some the tables I am working with in the code above:

P.S. I am not a programmer and I didn't get any education on programming, it is just a hobby. So go easy on me if my coding is ugly :)

No. I don't think your question is bad. It's only hard to answer on, since there are so many different ways to solve the problem your method addresses. And if they are better or not is kind of subjective. With that said here are some comment on your method:

Your method is too long and does too many things. I would recommend breaking it up into smaller method that does one thing; one that get the stuff from the database, one that checks the group permission and one that checks the parent group permission. Then the responsibility of the main method is to invoke the other if needed (and in what order).

I don't think the signature of the method is the best. Having int? emplooyeGroupID = null as a parameter is a clear indication that your method does two different major things; one if int? emplooyeGroupID is null and one if not. I would break it into two methods:

static bool CheckPermission(Session session, string right);
static bool CheckGroupPermission(Session session, string right, int emplooyeGroupID);

Then it will be up to the caller (client) to choose the right one. And since the caller knows if the emplooyeGroupID is null or not, the CheckPermission method don't need to "if" on it.

Why do you return true on the last line? I would definitely return false!

Otherwise you should be happy if the method does what you want it to.


Function to trigger animation with mouse wheel events I am new in the JavaScript world and I just want to know if I can optimize my function by making it shorter. It already works but I plan to add five timelines to trigger with my slidenum variable. Here is the code :

var slidenum = 0;
var skrollViewport = document.getElementById('wrapper');
skrollViewport.addEventListener('wheel', skrollSlide);

function skrollSlide(event) {
    var delta;
    if (event.wheelDelta) {
        delta = event.wheelDelta;
    } else {
        delta = -1* event.deltaY;
    }
    if (delta <0) {
        slidenum++;
    } else if (delta >0) {
        slidenum = slidenum>0 ? --slidenum : 0;
    }
    if (slidenum > 0) {
        slide1.play()
    } else if (slidenum < 1) {
        slide1.reverse()
    }
    if (slidenum > 1) {
        slide2.play()
    } else if (slidenum < 2) {
        slide2.reverse()
    }
    console.log(slidenum);
}

This:

var delta;
if (event.wheelDelta) {
    delta = event.wheelDelta;
} else {
    delta = -1* event.deltaY;
}

could be simplified to:

var delta = event.wheelDelta ? event.wheelDelta : -event.deltaY;

or even more succinctly:

var delta = event.wheelDelta || - event.deltaY;

For the if/else:

if (slidenum > 0) {
    slide1.play()
} else if (slidenum < 1) {
    slide1.reverse()
}

I personally find your end points take me a while to understand because of the way they overlap. I would find it easier like this:

if (slidenum >= 1) {
    slide1.play()
} else if (slidenum < 1) {
    slide1.reverse()
}

Or without the else which is redundant in this case.

if (slidenum > 0) {
    slide1.play()
} else {
    slide1.reverse()
}


The best way to place callback definition In JavaScript we can write callback definition in  many different ways:

Definition after usage:

var fs = require('fs')
fs.readFile('movie.mp4', finishedReading)

function finishedReading(error, movieData) {
  if (error) return console.error(error)
  // do something with the movieData
}

Definition before usage:

var fs = require('fs')

function finishedReading(error, movieData) {
  if (error) return console.error(error)
  // do something with the movieData
}

fs.readFile('movie.mp4', finishedReading)

In-line definition:

var fs = require('fs')

fs.readFile('movie.mp4', function finishedReading(error, movieData) {
  if (error) return console.error(error)
  // do something with the movieData
})

Which way is the best for code readability and maintainability?

In my opinion, if you use the same callback multiple times, or if the callbacks are similiar enough to be extracted and called with parameters, they should be extracted to keep DRY principle. In this case for me this does not make much difference if the callback was defined before first call, after it, or even if it exists in some other file, provided I can easily find it - and editors do help with that. For me there is slight preference to callback definition after the call, see below.

In case of single-use callbacks, I personally prefer to read them in-line. Let's consider these three options and how my brain works with them:

callback inline: I'm reading through the code in one go, learning what happens once the calling back action finishes;
callback definition after call: I perform mental GOTO, jumping to definition in order to understand what happens;
callback definition before call: I either skip it until I need it and make the same mental GOTO at some point later, or read through trying to remember and keep in mind what the callback does before I even know if and when it would be needed. I guess this could be called 'mental label' for the sake of consistent terminology.

I am lazy and prefer not to strain my brain trying to juggle multiple informations and contexts in order not to let them slip from short-term memory.


Copying files in Haskell I recently watched a video in which the speaker said that Haskell is not suited for real-world tasks, such as copying files from one folder to another.
So I tried to write a program that copies a given file system tree to a new location. The usage of the program is

copy_tree <source path> <target path>

If the target path does not exists, it is created. Then all folders and files under <source path> are copied to <target path>. Here is my copy_tree.hs source. I would like to have feedback and suggestions on how to improve it. Especially, I would like to make it as succinct and as idiomatic as possible.

One thing that I do not like very much is that the code calls doesDirectoryExist twice.

import System.Directory
import System.Environment
import System.FilePath

getSubitems path = getSubitemsRec ""
  where
    getChildren path = do
        isDir <- doesDirectoryExist path
        if isDir
          then fmap (filter (`notElem` [".", ".."])) (getDirectoryContents path)
          else return []

    getSubitemsRec relPath = do
        children <- getChildren (path </> relPath)
        let relChildren = [relPath </> p | p <- children]
        nephewItems <- mapM getSubitemsRec relChildren
        return (relChildren ++ (concat nephewItems))

copyItem baseSourcePath baseTargetPath relativePath = do
    let sourcePath = baseSourcePath </> relativePath
    let targetPath = baseTargetPath </> relativePath

    putStrLn $ "Copying " ++ sourcePath ++ " to " ++ targetPath
    isDir <- doesDirectoryExist sourcePath
    if isDir
      then createDirectoryIfMissing False targetPath
      else copyFile sourcePath targetPath

copyTree s t = do
    subItems <- getSubitems s
    createDirectoryIfMissing True t
    mapM_ (copyItem s t) subItems

main = do
    args <- getArgs
    if (length args) == 2
      then copyTree (args !! 0) (args !! 1)
      else putStrLn "Usage: copy_tree <source> <target>"

One thing that I do not like very much is that the code calls
  doesDirectoryExist twice.

You can return the result of the doesDirectoryExist from getSubitems and pass it to copyItem, obviating the duplicate call.

  Especially, I would like to make it as succinct and as idiomatic as
  possible.

You can use low precedence <$> instead of fmap to remove some parentheses.

Also "." and ".." should be returned once from getDirectoryContents, so you can replace the

(filter (`notElem` [".", ".."]))

with (\\ [".", ".."]).

Slight Behavior Difference between copyTree and copyTree'

Behavior of copyTree' is slightly different from copyTree, in that it will accept files as well as directories as parameter s. This is due to the fact that getSubitems' includes the input parameter (path, root of the subtree) in the descendants list it returns. You can get back the previous behavior by ignoring the first value returning from the getSubitems' in copyTree'. But I left the change in copyTree' as is for a behavior more consistent with cp -R of POSIX.

Modified Code

import Data.Functor ((<$>))
import Data.List ((\\))

getSubitems' :: FilePath -> IO [(Bool, FilePath)]
getSubitems' path = getSubitemsRec ""
  where
    getChildren path =  (\\ [".", ".."]) <$> getDirectoryContents path

    getSubitemsRec relPath = do
        let absPath = path </> relPath
        isDir <- doesDirectoryExist absPath
        children <- if isDir then getChildren absPath else return []
        let relChildren = [relPath </> p | p <- children]
        ((isDir, relPath) :) . concat <$> mapM getSubitemsRec relChildren

copyItem' baseSourcePath baseTargetPath (isDir, relativePath) = do
    let sourcePath = baseSourcePath </> relativePath
    let targetPath = baseTargetPath </> relativePath

    putStrLn $ "Copying " ++ sourcePath ++ " to " ++ targetPath
    if isDir
      then createDirectoryIfMissing False targetPath
      else copyFile sourcePath targetPath

copyTree' s t = do
    createDirectoryIfMissing True t
    subItems <- getSubitems' s
    mapM_ (copyItem' s t) subItems


Sharing a database connection with multiple modules I am building what essentially could be viewed as a glorified database wrapper as a Python package, where I'm having several classes and functions spread out into different modules. My current problem is how to share the database connections with the different modules.

An example class in user.py:

class User(object):
    def __init__(self, user_name, db_con):
        self.user_name = user_name
        self._db_con = db_con

Which results in my __init__.py looking like this:

from .user import User as _User

def _connect_dbs():
    econ = _mdb.connect(host='db.com',
                        user='user', passwd=pass,
                        db='db', charset='utf8')
    return econ

_db = _connect_dbs()

def User(user_name):
    return _User(user_name, _db)

Is this a good solution, or could I implement it in another manner? While this avoids global variables, it does result in some code I would rather not have to write for quite a few functions.

If you are creating a database wrapper, why not make a lib out of the wrapper and stick it into a lib directory that other modules/classes can import? So, your Users class would be in a directory structure like this:

project/
    account.py
    lib/
        database.py
    user.py
    zombie.py

Your database.py would look something like:

import pymysql

class Database:
    @staticmethod
    def connect_dbs():
        econ = _mdb.connect(host='db.com',
                            user='user', passwd=pass,
                            db='db', charset='utf8')
        return econ

Then, in the user.py, you'd simply do an import like:

from .lib.database import Database as MyDatabase

Then, to get the connection, something along the lines of:

my_connection = MyDatabase.connect_dbs()

EDIT: As an example, see this repl.it:

class Database:
    connection = None

    def __init__(self):
        print "Instantiating!"

    def connect_dbs(self):
        if self.connection is not None:
            return self.connection

        self.connection = 1
        return self.connection

class User:
    db = None

    def __init__(self):
        self.db = Database()

    def save_user(self):
        print self.db.connect_dbs()

U = User()
for i in range(30):
    U.save_user() # Only instantiates the connection once, then reuses it 30x


Function that checks two objects with same fields and assigns old non-null values to new object if new values are empty or null I have a function that checks two reference objects. They both have the same properties.

The purpose of this method is to ensure that any non-null old values are not overwritten with null or empty values after an API call. I'd appreciate any feedback.

evaluateEmptyValues: function(reference, originalReference) {
    var vm = this;

    // Get length
    referenceLength = Object.entries(reference).length;
    originalReferenceLength = Object.entries(originalReference).length;

    // Evaluate both if they are the same length -- they always should be
    if (referenceLength == originalReferenceLength) {
        try {
            for (var prop in reference) {
                // First check for undefined or null
                if (reference[prop] != undefined || reference[prop] != null) {
                    if (typeof (reference[prop]) == 'string' && reference[prop].trim() == '') {
                        // Assign original value to new object if new value is empty string
                        reference[prop] = originalReference[prop];
                    }

                    // Check if current prop in both objects is an object
                    if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object') {
                        var length = Object.keys(reference[prop]).length;

                        // Do another loop
                        for (var property in reference[prop]) {
                            // Check for undefined or null value in original
                            if (originalReference[prop][property] != undefined) {
                                if (originalReference[prop][property] != null) {
                                    if (reference[prop][property] == null || reference[prop][property] == '') {
                                        // Assign old non-null value to new object if new value is empty or null
                                        reference[prop][property] = originalReference[prop][property];
                                    }
                                }
                            }
                        }
                    }

                    // Check for array
                    if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {
                        // Recurse if both are arrays
                        reference[prop].forEach((item, index) => vm.evaluateEmptyValues(item, originalReference[prop][index]));
                    }
                } else {
                    if (originalReference[prop] != undefined) {
                        if (originalReference[prop] != null) {
                            // Assign original value to new object
                            reference[prop] = originalReference[prop];
                        }
                    }
                }
            }
        } catch(err) {
            console.log(err);
        }
    }
},

Very bad code

Your code is very poorly written making it difficult to workout what your intent is.

The try catch was a red flag and immediately told me not to trust the code.

Looking further at your code I found it full of redundant code and conflicting logic.

List of problems

if (reference[prop] != undefined || reference[prop] != null) {

Is the same as if(A == true || A == false) {

This is always true, if A is not true, then it is true that A is false

That makes the else and the associated block redundant (will never happen)
if ( /*...*/ && typeof Array.isArray(originalReference[prop])) {

typeof creates a non empty string. Thus the second half of the above statement is always true and thus redundant.
if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object')   and

if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {

If reference[prop] is an array, null, or an object and originalReference[prop] is an array, null, or object then both the above statements are true, which I think is not your intent.
var length = Object.keys(reference[prop]).length;

length is never used making this line redundant.
reference[prop].forEach((item, index) => vm.evaluateEmptyValues(item, originalReference[prop][index]))

It is common for objects to contain references to themselves, or have properties that reference them selves. Thus this type of recursion can end up in an infinite loop. Luckily JS has a finite call stack that will cause an exception to be thrown, but as you catch the errors rather than stop, it will grind to a stop as it throws overflow error after error till it gets past all the self references.

Minor points

var vm = this;

The reference to vm is used in the previous point. The forEach is using an arrow function that maintains the outer context of this. The variable vm is thus redundant.
referenceLength = Object.entries(reference).length;

Object.entries creates an array of arrays. [[key, value], [key, value], ...] You should use Object.keys or Object.values if you want the number of properties.
typeof (reference[prop]) == 'string'

typeof is a token not a function and does not need to be followed by a pair of ()
Use === or !== rather than == or !=

Comments just add to the mess

Your comments are noise if they repeat what is self evident in the code.

Or worse, comments are lies when in direct conflict with the code

You should only comment when the code can not represent the higher level abstraction that the logic is implementing. This is usually rare.

The better the code the less comments are needed to understand what is going on.

Some examples

// Do another loop

for (var property in reference[prop]) {

Really!... a loop you say.
// Check if current prop in both objects is an object

if (typeof(reference[prop]) == 'object' && typeof(originalReference[prop]) == 'object') {

Again the comment is repeating what is self evident in the code.
if (Array.isArray(reference[prop]) && typeof Array.isArray(originalReference[prop])) {

// Recurse if both are arrays

No its not?... The comment is in direct conflict the the statement above it.

Summing up

Sorry to be so harsh, but your code is boarder line on topic for code review. You need to be much more careful when writing code.

I think part of the problem is the very long naming and indirect referencing (eg originalReference[prop][property]) you are using which is making it hard to see the logic from the referencing.

Your code is nested up to 10 times, and one line is over 120 characters long. In that line of the 100 characters (ignoring indent/white spaces) 71 characters are naming, and only 29 are part of the logic. Little wonder you are making obvious logic mistakes in the code, you can not see the wood from the trees.

There are a variety of other problems in your code that I did not address.


Using c++11 random header to generate random numbers I'm new to programming. I have a question regarding the usage of c++11 random header to generate random numbers.
I tried to learn it, but was unsuccessful.
Recently, I've tried the following approach and it worked. Any advice on how to improve it. Is there a good source to learn more about this?
Here's the code:
#include <iostream>
#include <random>
#include <ctime>
int main()
{
   std::default_random_engine random_engine(time(0));
   for (int i = 1; i <= 100; i++)
   {
    std::uniform_int_distribution<int> num(1, 100);
    std::cout << i << "==> " << num(random_engine) << std::endl;
   }
    return 0;
}

First of all, note that std::default_random_engine and std::uniform_int_distribution<int> are both non-portable in the same sense as rand() is non-portable: they won't produce the same sequence of numbers on different platforms, because their behavior is implementation-defined. For uniform_int_distribution there effectively is no portable alternative, so it's the best game in town. But for the PRNG itself, I don't think any expert would ever recommend default_random_engine. Just pick a known engine, such as std::mt19937. (Which isn't good but again it's the only game in town at the moment.)
For seeding: notice that you're giving only 32 bits of seed (and a very predictable seed at that) to the PRNG's constructor. You should be seeding from a random_device, something like this (stolen from here):
std::random_device rd;
int data[624];
std::generate_n(data, std::size(data), std::ref(rd));
std::seed_seq sseq(data, std::end(data));
std::mt19937 g(sseq);

(Of course nobody does this in practice.)

for (int i = 1; i <= 100; i++)

Prefer
for (int i = 0; i < 100; ++i)

even if it means you have to refer to i+1 inside the loop. Using half-open ranges that start at 0 is good practice for everywhere in C and C++ and every modern language.

std::uniform_int_distribution<int> num(1, 100);

Personally I'd write this as
auto dist = std::uniform_int_distribution<int>(1, 100);

to get that nice clear = separation visible at a glance. Also, notice my conventional variable names throughout: rd for the random device, g for the PRNG itself, dist for the distribution. It might make sense to use a more descriptive name than dist... but certainly num is not more descriptive, and num is less truthful — a distribution is not a single number!

Finally, you don't need that final return 0 (main returns 0 on success by default), and you don't need std::endl because '\n' (or "\n") will do just as well.
But basically you're doing the dance correctly: PRNG created outside the loop, distribution called-like-a-function from inside the loop.
You could move the variable definition of the distribution outside the loop as well, if you wanted. It probably doesn't matter in practice because uniform_int_distribution is unlikely to have any state worth preserving between iterations. But if it were a std::normal_distribution, say, then destroying and recreating the distribution every time through the loop would be costing you basically 50% of your speed.
auto dist = std::uniform_int_distribution<int>(1, 100);
for (int i = 0; i < 100; ++i) {
    std::cout << (i+1) << "==> " << dist(g) << '\n';
}


Finding the last ten digits of \$\sum_{n=1}^{1000} n^n\$ This is my solution to Project Euler Problem 48.

Problem:

  The series, \$1^1 + 2^2 + 3^3 + ... + 10^{10} = 10405071317\$ .

  Find the last ten digits of the series, \$1^1 + 2^2 + 3^3 + ... + 1000^{1000}\$.

I would like feedback/advice to possibly increase efficiency and/or correct incorrect practices.

public static void main(String[] args) {
    BigInteger start, sum = BigInteger.valueOf(0);

    for (int i = 1; i <= 1000; i++) {
        start = BigInteger.valueOf(i);
        sum = sum.add(start.pow(i));
    }
    String sumStr = sum.toString();
    System.out.println(sumStr.substring(sumStr.length() - 10));
}

Use the right data type. The last ten digits of the sum is same as the sum modulo \$10000000000 = 10^{10}\$, which is smaller than \$2^{34}\$. All computations can be comfortably done with 64-bit integers; invoking BigInteger is a definite overkill. Extracting ten digits via string operations is also quite suboptimal.
Reuse your computations. Once you computed \$k = n^n\$, use it to compute \$(2n)^{2n} = 2^{2n} n^{2n} = 2^{2n}k^2\$.
Devise an algorithm. The logic of the previous bullet applies to any \$mn\$: \$(mn)^{mn} = m^{mn}n^{mn} = (m^m)^n (n^n)^m\$. This observation lends itself to the scheduling of computations very close to be optimal. Think about divisibility in general and prime numbers in particular.
Any multiple of 10 raised to the corresponding power is surely divisible by \$10^{10}\$, and can be safely omitted from summation, but this is a minor optimization.


String Split a phrase into sentences Hello this is a Console App(.Net Core) in Visual Studio 2019 and i am trying to split a string into sentences and then add them into a dictionary with Key the first word of the sentence and Value the sentence itself.
I have the following code and i wonder if the way i did it has of course a better way to handle it.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;

namespace StringParse
{
    class Program
    {
        static void Main(string[] args)
        {
            string phrase = @"MHD001|LOCCOD|GRCACS|20191117|133601|CB|trx_grcacs_tsk_20191117133601498|
TSK001|GLS|GRCACS|59144|59144|2|TU|93000161357|TRACE|INF|DE 800|please send us the completely address information-thanks|68481|20191117|133555|
TSA001|X|
TSI001|3655|please send us the completely address information-thanks|66510|
TSI001|5637|80337|61335|
TSI001|5841|DE|61313|
MTR001|5|";
            /////////////////////////////////////////////////////////////////////
            string sentenceSeparator = @"\b[A-Z]{3}\d{3}\b";
            string[] words = phrase.Split('|');

            StringBuilder sb = new StringBuilder();

            for (int i = 0; i < words.Length; i++)
            {
                //If current word array matches the regex
                if (words[i].Equals(sentenceSeparator))
                {
                    // If the current array value doesn't match, then continue the current line.
                    sb.Append(words[i]);
                    sb.Append(" ");
                }
                else
                {
                    if (i == 0)
                    {
                        // on the 1st iteration, we don't need a new line before the text value.
                        sb.Append(words[i]);
                        sb.Append(" ");
                    }
                    else
                    {
                        sb.Append(words[i]);
                        sb.Append(" ");
                    }
                }
            }
            string[] sentences = sb.ToString().Split('\r');

            ////////////////////////////////////////////////////////////////////////

            Regex regx = new Regex(sentenceSeparator);

            MatchCollection matches = Regex.
                Matches(phrase, sentenceSeparator,
                RegexOptions.Compiled |
                RegexOptions.CultureInvariant);

            for (int i = 1; i < matches.Count; ++i)
            {
                phrase = phrase
                    .Replace(matches[i].Value,
                    Environment.NewLine + matches[i].Value);
            }
            ///////////////////////////////////////////////////////////////////////////////

            Dictionary<Match, string> dic = matches.Zip(sentences, (x, y)
                 => new { Key = x, Value = y }).ToDictionary(x => x.Key, y => y.Value);

            foreach (KeyValuePair<Match, string> kvp in dic)
            {
                Console.WriteLine("Key = {0}" + Environment.NewLine + "Value = {1}", kvp.Key, kvp.Value);
            }

        }

    }

}

output will be like that:

Key = MHD001
Value = MHD001 LOCCOD GRCACS 20191117 133601 CB trx_grcacs_tsk_20191117133601498
Key = TSK001
Value = TSK001 GLS GRCACS 59144 59144 2 TU 93000161357 TRACE INF DE 800 please send us the completely address information-thanks 68481 20191117 133555
Key = TSA001
Value = TSA001 X
Key = TSI001
Value = TSI001 3655 please send us the completely address information-thanks 66510
Key = TSI001
Value = TSI001 5637 80337 61335
Key = TSI001
Value = TSI001 5841 DE 61313
Key = MTR001
Value = MTR001 5

The whole if..else..if..else inside the first loop is senseless because each branch does exactly the same.

Using Split('|') and combining the resulting array with one space " " is the same like replacing | by a space.

The Regex regx = new Regex(sentenceSeparator); is superflous because you don't use regex at all.

The loop

for (int i = 1; i < matches.Count; ++i)
{
    phrase = phrase
        .Replace(matches[i].Value,
        Environment.NewLine + matches[i].Value);
}

is superflous because you aren't using phrase afterwards.

As it seems (untested) you can use Zip() with a MatchCollection if you use .NET Core 3.0.100 but to make your code runnable without .NET Core you should change

Dictionary<Match, string> dic = matches.Zip(sentences, (x, y)
             => new { Key = x, Value = y }).ToDictionary(x => x.Key, y => y.Value);

to

Dictionary<Match, string> dic = matches.Cast<Match>().Zip(sentences, (x, y)
             => new { Key = x, Value = y }).ToDictionary(x => x.Key, y => y.Value);

It comes for sure from copying the code (which seems to be posted using Shift + Enter for phrase) from this question to the IDE....if you inspect the result of dict you will see that from the second item on the Value is prepended by a \n. This can be prevented by using TrimStart('\n') on y at new { Key = x, Value = y }.

Implementing the mentioned points will lead to

   static void Main(string[] args)
    {
        string phrase = @"MHD001|LOCCOD|GRCACS|20191117|133601|CB|trx_grcacs_tsk_20191117133601498|TSK001|GLS|GRCACS|59144|59144|2|TU|93000161357|TRACE|INF|DE 800|please send us the completely address information-thanks|68481|20191117|133555|TSA001|X|TSI001|3655|please send us the completely address information-thanks|66510|TSI001|5637|80337|61335|TSI001|5841|DE|61313|MTR001|5|";

        string[] sentences = phrase.Replace('|',' ').Split('\r');
        string sentencePattern = @"\b[A-Z]{3}\d{3}\b";

        MatchCollection matches = Regex.
            Matches(phrase, sentencePattern,
            RegexOptions.Compiled |
            RegexOptions.CultureInvariant);

        Dictionary<Match, string> dic = matches.Cast<Match>().Zip(sentences, (x, y)
             => new { Key = x, Value = y.TrimStart('\n') }).ToDictionary(x => x.Key, y => y.Value);

        foreach (KeyValuePair<Match, string> kvp in dic)
        {
            Console.WriteLine("Key = {0}" + Environment.NewLine + "Value = {1}", kvp.Key, kvp.Value);
        }
    }


Convert 'rgb()' string to hex string The scenario is the following. I get the string from an HTML element via outerHTML and I need to convert the 'rgb(r, g, b)' part into the hexadecimal representation. I tried this with regex and it seems to work but I have a feeling there are better ways to deal with this.

One particular thing I don't like is where I directly access color[1] from my regex match. I'm open to any feedback.

private cleanupRichText(richText: string) {
    const rx = /rgb\((.+?)\)/ig
    const color = rx.exec(richText)
    if (color !== null) {
        const hexparts = color[1].split(',').map(str => parseInt(str.trim(), 10).toString(16))
        let hexString = '#'
        hexparts.forEach(element => {
        hexString += element.length === 1 ? '0' + element : element })
        text = text.replace(/rgb\(.+?\)/ig, hexString)
    }
}

To give an example input:

'rgb(234, 112, 30)'

Expected output:

'#ea701e'

While the given function does work for simple cases, it will not perform as expected if there are multiple colors within richText. The first color will override all other colors.

First, a look at your implementation:

Why is richText used in some places and the (global?) text in others? It looks like these are expected to be equivalent, so you should just use richText since it is explicitly passed in.
Instead of mutating the global text, you should return the replaced result. This makes testing the function easier and debugging much less painful.
Why is this a class function? It seems to have well defined inputs & outputs that does not depend on the state of any object.
Instead of manually padding numbers with element.length === 1 ? '0' + element : element, you should use element.padStart(2, '0') if possible. The polyfill isn't that large if it isn't supported everywhere you need it yet (IE), and it is considerably easier to read.
parseInt ignores leading whitespace, so there's no need to do str.trim().
String.prototype.replace accepts a replace function, which will be called with each match. Use this to extract the rgb values instead of doing re.exec yourself. In the vast majority of cases, you should not need re.exec.
Should text inside the element really be converted? If you are manipulating HTML as text, you probably shouldn't be. If this rgb() text is in the style element of the element, you could be using element.getAttribute('style') instead.
What should happen if I wrote rgb(1, 2, 3, 4, 5, 'asdf')?

Here is how I would write this function, keeping your assumption that all rgb strings are well formed.

function cleanupRichText(richText) {
  return richText.replace(/rgb\((.+?)\)/ig, (_, rgb) => {
    return '#' + rgb.split(',')
      .map(str => parseInt(str, 10).toString(16).padStart(2, '0'))
      .join('')
  })
}

const tests = [
  ['rgb(234, 112, 30)', '#ea701e'],
  ['rgb(0, 0, 0)', '#000000'],
  ['RGB(255, 255,255)', '#ffffff'],
  ['rgb(0, 0, 0) rgb(170, 170, 170)', '#000000 #aaaaaa']
]

for (const [text, expected] of tests) {
  const result = cleanupRichText(text)
  console.log(result === expected ? 'PASS' : 'FAIL', text, '->', result)
}


Increase performance in sub-line detection I have an algorithm for checking whether one line is contained within another. The lines I have are made up of an ordered array of points, which are just data structures with values for x, y and z. The lines are no necessarily straight. The subline does not need to be going in the same direction as the main line.

The algorithm I have currently is:

public static bool IsSubline(Point[] subline, Point[] line)
{

    int prevIndex = int.MaxValue;
    bool contains = false;

    foreach (Point point in subline)
    {
        for (int i = 0; i < line.Length; i++)
        {
            if (Equal(point, line[i]))
            {
                if (prevIndex == int.MaxValue || System.Math.Abs(prevIndex - i) == 1)
                {
                    prevIndex = i;
                    contains = true;
                    break;
                }
            }

            contains = false;
        }

        if (!contains)
        {
            break;
        }
    }

    return contains;
}

Equal is:

internal static bool Equal(Point point1, Point point2, float epsilon = CoordinateEpsilon)
{
    return System.Math.Abs(point1.x - point2.x) < epsilon &&
        System.Math.Abs(point1.y - point2.y) < epsilon &&
        System.Math.Abs(point1.z - point2.z) < epsilon;
}

I feel like this could be done in a faster way, anyone have any suggestions?

Yes it can be done faster. Your algorithm has a \$O(n^2)\$ time.

Idea: store the indexes of the points of the line in a dictionary and use the points as key (I assume that two distinct points never have the same coordinates. If this is not the case, then an index list would have to be stored for each point).

var lineIndexes = new Dictionary<Point, int>();

//TODO: add the points of the line to the dictionary

foreach (int i = 0; i < subline.Length - 1; i++) {
    Point sublinePoint1 = subline[i];
    Point sublinePoint2 = subline[i + 1];

    int k; // Index in line.
    If (lineIndexes.TryGetValue(sublinePoint1, out k) &&         // We found one matching point AND
        ( k > 0 && sublinePoint2.Equals(line[k - 1]) ||              // (previous one matches OR
          k < line.Lengh - 1 && sublinePoint2.Equals(line[k + 1]))) {//  next one matches)

        return true;
    }
}
return false;

This is \$O(n)\$, since the dictionaries have a constant access time.

Equal vs. Equals

Every object (other than System.Object) inherits Equals and GetHashCode from System.Object. Do not introduce your own Equal method but override Equals and GetHashCode in your own objects. This is necessary for right functioning of the dictionary and other .NET data structures.

If you cannot do that, there are overloads of the dictionary's constructor accepting an IEqualityComparer<TKey>. Provide your own implementation of IEqualityComparer<Point>.


make_auto implementation for C++03 compilers I want to provide, in the the spirit of C++11/boost make_shared and C++14 make_unique, a production-ready make_auto for C++03 compilers.

So, inspired boost's make_shared implementation for C++03 compilers, I provide N overloads (0 to 10, currently) of the same make_auto function.

template <typename T>
inline std::auto_ptr<T> make_auto()
{
   return std::auto_ptr<T>(new T()) ;
}

template <typename T, typename T_0>
inline std::auto_ptr<T> make_auto(const T_0 & p_0)
{
   return std::auto_ptr<T>(new T(p_0)) ;
}

template <typename T, typename T_0, typename T_1>
inline std::auto_ptr<T> make_auto(const T_0 & p_0, const T_1 & p_1)
{
   return std::auto_ptr<T>(new T(p_0, p_1)) ;
}

// etc.

Is there something wrong with this code?

Implementation details:

the const reference is inspired by boost's shared_ptr. After trying multiple alternatives (including GManNickG's excellent answer here), I had to gave up because of some challenged compilers. To pass non-const references, I'll use boost::ref.
the inline keyword is mandatory for reasons that have nothing to do with the problem.

Background info:

I need this for multiple reasons, among them:

I am stuck with C++03 compilers (and I suspect some of them of not even being that... I'm looking at you, SunStudio). This means even C++03 emulations of unique_ptr don't work (we tried)
avoiding writing new/delete in my C++ code (see Stroustrup's C++11 style video)
offering my coworkers the means to use a more modern C++ coding style despite aforementioned compilers limitations (RAII, that is)
more exception safety
I need a lightweight smart pointer with unique-ownership semantics, able to transfer ownership, so boost::shared_ptr and boost::scoped_ptr are not an option

Disclaimer: this is not an attempt to answer your technical question, but suggesting alternatives to reach some compromise of your stated goals. It wouldn't fit in a comment :-)

I am not sure it is a good idea to actually write a make_auto. It is akin to rearranging the deck chairs on the Titanic. After it hit the iceberg.

You'd be better off with emulating a std::unique_ptr using C++98/03 features. Howard Hinnant has one version on his website. You might even get lucky and get its macro variadic version to work nicely with the std::make_unique proposal by Stephan T. Lavavej.

I'd take that as the preferred option, with using boost::shared_ptr or boost_scoped_ptr as the preferred alternative option. If you insist on keeping std::auto_ptr, make its use as painful as possible. Without pain, you'll find it more difficult to convince your colleagues to upgrade to C++11.

Please also make sure to introduce your team to the other language emulation features that Boost provides (Foreach, Move).


Reading a .raw file while looking for .jpeg signatures The code reads 512 bytes at a time and stores them in a buffer. When the code sees a .jpeg signature it will proceed by writing to a .jpeg file until another signature is found, thereafter the process repeats.

#include <stdio.h>
#include <stdlib.h>
#include <cs50.h>

#define infile "card.raw"
#define BLOCK 512

/**
 * recover.c
 *
 * Computer Science 50
 * Problem Set 4
 *
 * Recovers JPEGs from a forensic image.
 */

// Declarations and prototype(s)
char * name();

FILE* outptr = NULL;
int increment = 0;

int main(void)
{

    // Open input file, plus error check
    FILE* inptr = fopen(infile, "r");
    if (inptr == NULL)
    {
        printf("Could not open %s.\n", infile);
        return 3;
    }

    // Allocate memory for one BLOCK and read from infile
    unsigned char * buffer = malloc(BLOCK);

    if (buffer == NULL)
        return 3;

    while (fread(buffer, BLOCK, 1, inptr) != 0) {

        /* Shall modify to operate on bitwise operations in the future. */

        // If the first four bytes of buffer is equal to a jpg file signature,
        // then copy all contents to a new jpg until another signature is found
        if (buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        buffer[3] >= 0xe0 && buffer[3] <= 0xef) {

            // If outptr does not yet refer to a file, close it
            if (outptr != NULL)
                fclose(outptr);

            // Open ###th output file, plus error-checking
            outptr = fopen(name(), "w");
            if (outptr == NULL) {
                fprintf(stderr, "Could not create outfile.\n");
                fclose(outptr);
                return 3;
            }

        }
        if (outptr != NULL)
            fwrite(buffer, BLOCK, 1, outptr);
    }
    // Check if the file was looped through correctly
    if ( !(feof(inptr)) )
        return 3;

    free(buffer);

    fclose(inptr);

    return 0;
}

// Name files incrementally starting from '000'
int filenum = 0;
char filename[7];

char * name()
{
    if (filenum < 10)
        sprintf(filename, "00%d.jpg", filenum);
    else
        sprintf(filename, "0%d.jpg", filenum);

    filenum++;
    return filename;
}

Binary mode
You should use binary mode to open your file instead of text mode.  So change:

FILE* inptr = fopen(infile, "r");

to
FILE* inptr = fopen(infile, "rb");

Otherwise, your CR/LF characters may get converted when you don't want them to be.
Buffer overrun
Your filename buffer is too short:

char filename[7];

char * name()
{
    if (filenum < 10)
        sprintf(filename, "00%d.jpg", filenum);
    else
        sprintf(filename, "0%d.jpg", filenum);
    // ...

Here, your filenames will be something like "000.jpg" which require 7 characters plus a terminating null character, or 8 characters in total.  Also, if you generate 100 or more files, you will need even more characters.
Pad string with leading zeros
There is a better way to generate filenames padded with leading zeros.  Instead of:

if (filenum < 10)
    sprintf(filename, "00%d.jpg", filenum);
else
    sprintf(filename, "0%d.jpg", filenum);

you can do this:
sprintf(filename, "%03d.jpg", filenum);


Find max in-order difference in array Given an array A of integers, find the maximum of j - i subjected to the constraint of A[i] <= A[j].

If there is no solution possible, return 0.

Example :

A : [3 5 4 2]

Output : 2
for the pair (3, 4)

My Code :

def maximumGap(self, A):
    if len(A)==1: return  0
    a_list = sorted(([Val,indeX] for indeX,Val in enumerate(A)), reverse=True)

    rest = [indeX for val,indeX in a_list] # rest stores all the index from a_list which is sorted in decreasing order
    len_rest = len(rest)
    del a_list[:]
    results = []

    for i in range(len_rest):
        if i != len_rest-1:
            results.append(rest[i] - min(rest[i+1:]))
        else: continue

    ans = max(results)
    if ans < 0: return 0
    else: return ans

Sample input taken for explanation:

A = [34, 8, 10, 3, 2, 80, 30, 33, 1]

Brief summary of my algorithm:

1. a_list is a list of lists which will store all the numbers from the orginal list (list A) in sorted order along with their indexes.

`a_list = [[80, 5], [34, 0], [33, 7], [30, 6], [10, 2], [8, 1], [3, 3], [2, 4], [1, 8]]`

2. rest is a list which will store all the indexes from a_list.

`rest = [5, 0, 7, 6, 2, 1, 3, 4, 8]`

(After this, a_list is deleted to empty the space)

3. Now, the crux of the logic is:
Since list is sorted in descending order, I need not take care of the constraint: A[i] <= A[j]

I will just iterate the list: rest and in each iteration, say 0th iteration, rest[0] = 5 is subtracted from the smallest remaining elements in list: rest

Each result of the iteration is stored in list: results

4. results = [5, -1, 6, 5, 1, -2, -1, -4]

5. Max of list results will give me the answer.

(It is important to note that while sorting the list we must also store the original index of the values instead of blindly sorting it.)

Now, coming to the actual problem:

On submitting my solution at interviewbit website, my code is giving correct answers to all test cases but time limit is exceeded

I am not able to figure out what how to optimise this further.

Time complexity of my code,correct me if i am wrong, is O(N log N)

Sorting takes O(N log N) time in the worst case when using sort function in python.

My main question is how to optimise this code further?

Found on www.interviewbit.com

Actually in the last part, during the iteration over list named rest, its O(N^2), so, its not working.
Refer to Geeksforgeeks for O(n).