Did I convert this C++ class to Common Lisp correctly? Here is the original C++ class I converted to Lisp at the very bottom of this post.

class TrainingData
{
public:
    TrainingData(const string filename);
    bool isEof(void) { return m_trainingDataFile.eof(); }
    void getTopology(vector<unsigned> &topology);

    // Returns the number of input values read from the file:
    unsigned getNextInputs(vector<double> &inputVals);
    unsigned getTargetOutputs(vector<double> &targetOutputVals);

private:
    ifstream m_trainingDataFile;
};

It is a class from a C++ Neural Network I am converting to Common Lisp. The entire 400 line Neural Net is here.

Below is the conversion I came up with.  Based on the Neural Network I linked above, can anyone tell me if I converted the C+ class, above, correctly with the following Lisp code?  I'd rather have a little assurance before I start converting the entire 400 lines.

(defclass training-data ()
  ((training-data-stream :reader training-data-stream
                         :writer |set training data stream (using a funky name)|
                         :initarg :training-data-stream))
  )

;;; initialize-instance :after -- This is your 'constructor' code.

(defmethod initialize-instance :after ((td training-data)
                                       &key
                                       training-data-stream
                                       &allow-other-keys)
  ;; Initialize the slot.

  ;; Note that 'opening' a stream, as it is implied by the C++ class
  ;; definiton and leaking the open stream handle is very impolite.

  (let ((is (etypecase training-data-stream
              ((string pathname) (open training-data-stream :direction :input))
              (stream (if (input-stream-p training-data-stream)
                          training-data-stream
                          (error "Not an input stream.")))))
        )
    (|set training data stream (using a funky name)| is td)
    ))

(defgeneric is-eof (td)
  (:method ((td training-data))
   ;; Are you sure you want this method?
   ))

EDIT:  As far as tests go, please let me explain:

Due to the fact I'm just a intermediate at Linear Algebra I had a wee bit of a time understanding Neural Networks.  This conversion into Lisp is the way I figured out how to figure out what Neural Networks do.  I'm best at Lisp, pretty good at C, and sort of good at C++, so I'm trying to learn Neural Networks by converting a C++ nn to Lisp.

I don't have enough info to test any specific piece of the code I posted here or at the link yet.  I'm still in the process of learning std::stringstream and I've just learned what bitwise XOR was.  I can post the instructions that tell how to run the C++ nn at the link in its entirety, and the code to create the data file for it, so you can see how it runs or explain how the incomplete snippet I posted ties in with the rest of the code.  The incomplete member functions I added in this post have completed counterparts within the rest of the code at the link I just added.

Code review - mostly style

You did a litteral translation from C++ to CL that seems good. I have some stylistic issues with it:

(defclass training-data ()
  ((training-data-stream :reader training-data-stream
                         :writer |set training data stream (using a funky    name)|
                         :initarg :training-data-stream)))

I guess |set training data stream (using a funky    name)| is an attempt at obfuscating the writer method, so that you can have a kind of "private" setter. Don't do that.

:writer is optional: if you don't specify it, you won't generate a specific setter for your slot. You can always use slot-value to alter a slot. However, I don't recommend this appropach.
You can define a :accessor method, which acts as both a reader and a writer method (resp. (my-accessor instance) and (setf (my-accessor instance) value)). Since your C++ field is private and has no public getter/setter, this is an appropriate approach:

Define only an accessor, say training-data-stream, and
Do not export the training-data-stream symbol from your package.
Visibility is handled at package level in CL.

Your initarg could be made shorter: it is used when making an instance of a training-data class, so :training-data-stream is redundant, just use :input-stream (:stream would be too vague).

Then...

;;; initialize-instance :after -- This is your 'constructor' code.

(defmethod initialize-instance :after ((td training-data)
                                       &key
                                       training-data-stream
                                       &allow-other-keys)
  ;; Initialize the slot.

So far, so good; just rename training-data-stream as input-stream.

  ;; Note that 'opening' a stream, as it is implied by the C++ class
  ;; definiton and leaking the open stream handle is very impolite.

Your own comment highlights that

the original C++ class might not be good at following C++ best practices (RAII, for example), and
you are doing a verbatim clone of the C++ class instead of having a more idiomatic CL approach. Since you are just copying an existing codebase in order to understand it, it is not so bad. Maybe you could try to refactor it later, as an exercise.

Besides, "impolite" is not the word I would have used, because it is not accurate (it sounds as-if you ignore, or side-step, the real problems behind leaking resources).

Hereafter, your let binding is useless:

  (let ((is (etypecase training-data-stream
              ((string pathname) (open training-data-stream :direction :input))
              (stream (if (input-stream-p training-data-stream)
                          training-data-stream
                          (error "Not an input stream.")))))
        )
    (|set training data stream (using a funky name)| is td)
    ))

In other word, (let ((x (something))) (use x)) could simply be (use (something)).
Moreover, you changed the original behavior by allowing the constructor to be initialized with streams, and not only strings (was it necessary?).

Note that instead of etypecase, which is good, you could also let initialize-instance call a new generic method, say initialize, that dispatches on the type of the stream, either string or stream (we can't dispatch on &key arguments, otherwise initialize-instance would suffice).

    (setf (training-data-stream td)
          (etypecase input-stream
            ((string pathname) (open input-stream :direction :input))
            (stream (if (input-stream-p training-data-stream)
                        training-data-stream
                        (error "Not an input stream."))))))

Your objective

  I'm trying to learn Neural Networks by converting a C++ nn to Lisp

Learning high-level concepts like neural networks using an implementation like the one you linked is not very effective, in my opinion.

Even though you have difficulties with C++, this is not the language that prevents you from understanding neural networks; for example:

    Layer &outputLayer = m_layers.back();
    m_error = 0.0;

    for (unsigned n = 0; n < outputLayer.size() - 1; ++n) {
        double delta = targetVals[n] - outputLayer[n].getOutputVal();
        m_error += delta * delta;
    }
    m_error /= outputLayer.size() - 1; // get average error squared
    m_error = sqrt(m_error); // RMS

Those are just math operations, and the fact that they are written in C++ is not what makes them difficult to grasp. In fact, I never really studied neural networks and all the above code is cryptic, even though I am familiar with the language.

I doubt that rewriting it in CL will help you.

What you need first is a book and tutorials for high-level concepts.
Experiment with a easy-to-use library, like for example PyBrain, which has a good documentation.

Maybe when you have a good understanding of neural networks, you can go back to the linked code and inspect the implementation to see how it was designed, and why.


Generating histograms from "chunked" large data streams I've been working in some tools lately that analize long streams of data and extract frequent patterns, histograms, etc. for personal use. Lately I've been coding a histogram generator; that is, given a long chunked data stream, generate a histogram with all distinct chunks; typical case, given a text, generate a word histogram.

The algorithm and code I've come up with is the following:

public static class HistogramGenerator
{
    public static IEnumerable<HistogramItem<T>> GetHistogram<T>(IEnumerable<IEnumerable<T>> stream)
        => BuildHistogram(BuildTree(stream));

    private static Tree<T> BuildTree<T>(IEnumerable<IEnumerable<T>> stream)
    {
        var tree = new Tree<T>();

        foreach (var chunk in stream)
        {
            var currentTree = tree;

            foreach (var item in chunk)
            {
                currentTree = currentTree.Add(item);
            }

            currentTree.Count++;
        }

        return tree;
    }

    private static IEnumerable<HistogramItem<T>> BuildHistogram<T>(Tree<T> tree) =>
        getAllCurrentTreeReversedPaths(tree).Select(histogram =>  new HistogramItem<T>(histogram.Count, histogram.Path.Reverse()))
                                        .OrderByDescending(h => h.Count);

    private static IEnumerable<HistogramItem<T>> getAllCurrentTreeReversedPaths<T>(Tree<T> tree)
    {
        foreach (var branch in tree.Branches)
        {
            if (branch.Value.Count > 0)
            {
                //Make sure we return a valid path included in a longer one. For example: "the" inside "therefore"
                yield return new HistogramItem<T>(branch.Value.Count, new[] { branch.Key });
            }

            foreach (var subBranch in getAllCurrentTreeReversedPaths(branch.Value))
            {
                yield return new HistogramItem<T>(subBranch.Count, subBranch.Path.Concat(new[] { branch.Key }));
            }
        }
    }

    //TODO: Check if this is should be kept as a struct.
    public struct HistogramItem<T>
    {
        public int Count { get; }
        public IEnumerable<T> Path { get; }

        public HistogramItem(int count, IEnumerable<T> path)
        {
            Path = path;
            Count = count;
        }
    }

    private class Tree<T>
    {
        public int Count { get; set; }
        public Dictionary<T, Tree<T>> Branches { get; }

        public Tree()
        {
            Branches = new Dictionary<T, Tree<T>>();
            Count = 0;
        }

        public Tree<T> Add(T value)
        {
            Tree<T> branch;

            if (!Branches.TryGetValue(value, out branch))
            {
                branch = new Tree<T>();
                Branches.Add(value, branch);
            }

            return branch;
        }
    }
}

I'm reasonably happy with how it performs (aprox. 60 ms to analyze a 1.4+ million character long stream with 260k+ chunks).

Still, as a learning process, and because I know my limitations writing software (I'm relatively new though I'm trying hard to learn fast) I'd appreciate any feedback that could help optimize my code, make it more elegant and readable or pointers to alternative "simple" (if any) algorithm that would perform better.

var currentTree = tree;
foreach (var item in chunk)
{
     currentTree = currentTree.Add(item);
}
currentTree.Count++;

That function shouldn't need to know how to traverse a tree. That functionality belongs to Tree, not to the invoking site.

A simple tree.Add(chunk); is much cleaner in this context. The complexity of building and traversing the tree should be hidden entirely hidden inside the Tree class:

public void Add(IEnumerable<T> path)
{
    var node = this;
    foreach (var item in path)
    {
        node = node.GetChild(item);
    }
    node.Count++;
}

There is no longer a need to expose the original Tree::Add publicly, as the only legit caller is now inside the same class:

private Tree<T> GetChild(T value)
{
    Tree<T> node;

    if (!Branches.TryGetValue(value, out node))
    {
        node = new Tree<T>();
        Branches.Add(value, node);
    }
    return node;
}

The second occurrence where the functionality should have been encapsulated inside Tree is getAllCurrentTreeReversedPaths. This function requires specific knowledge about how the storage is implemented, so it's inseparable from it.

This also raises concerns regarding the visbility of Tree::Count and Tree::Branches. These properties don't need to be exposed, if the methods belonging to Tree are actually moved into that class.

For the invoking side, only the capabilities of storing, counting, and reciting stored paths are of interest. Nothing else, especially not how the tree is implemented. Not even the fact that it is implemented as a tree at all.

foreach (var subBranch in getAllCurrentTreeReversedPaths(branch.Value))
{
    yield return new HistogramItem<T>(
      subBranch.Count,
      subBranch.Path.Concat(new[] { branch.Key })
    );
}

This doesn't look right. You are essentially recreating and discarding HistogramItem instances over and over again, while you are technically only appending to the Path property.

HistogramItem::Path should either be mutable, or simply add the current path suffix as a parameter to getAllCurrentTreeReversedPaths, so you can create HistogramItem instances with the correct properties right away.


Codility Cyclic rotation in C# I'm learning C#, I programmed some things in java, and I would like to have a feedback on my C# code. The exercise is from codility.

  The goal is to rotate array A K times; that is, each element of A will
  be shifted to the right K times.

  Write a function:

  class Solution { public int[] solution(int[] A, int K); }

  that, given an array A consisting of N integers and an integer K,
  returns the array A rotated K times.

  For example, given

A = [3, 8, 9, 7, 6]
K = 3 the function should return [9, 7, 6, 3, 8]. Three rotations were made:

[3, 8, 9, 7, 6] -> [6, 3, 8, 9, 7]
[6, 3, 8, 9, 7] -> [7, 6, 3, 8, 9]
[7, 6, 3, 8, 9] -> [9, 7, 6, 3, 8] For another example, given

A = [0, 0, 0]
K = 1 the function should return [0, 0, 0]

  Given

A = [1, 2, 3, 4]
K = 4 the function should return [1, 2, 3, 4]

And this is my code. It gives 100% but maybe something can be improved.Thanks in advice.

using System;
// you can also use other imports, for example:
// using System.Collections.Generic;

// you can write to stdout for debugging purposes, e.g.
// Console.WriteLine("this is a debug message");

class Solution {
    public int[] solution(int[] A, int K) {
        int len = A.Length;
        int[] B = new int[len];
        if(len > 0 && K % len != 0 )
        {
            for ( int i = 0; i < len; i++)
            {
                B[(K + i) % len] = A[i];
            }
        }
        else
        {
            return A;
        }
        return B;
    }
}

Good Practices

✔ I like the fact you don't rotate if the array's length is a multiplication of K.
✔ As you stated in the comments, the second array B gets created too early, since in some occasions it would be created in vain.

Review

The challenge is clearly written for Java:class Solution { public int[] solution(int[] A, int K); }. The provided signature deserves its own review (casing conventions, static members, meaningful member names). However, if you 'cannot' change the signature given to you, I would be consistent and use N instead of len for the array's length.
You sometimes return the provided array and sometimes create a new one. I would make this very clear in the specification. A consumer should know whether or when a copy is returned. This affects further usage of the object.
You should guard against the source array being null.
How would you handle a negative value of K? If you think about it, a left rotation is just the inverse of a right rotation.
Nested statements could have been prevented by inverting the if-statement and exiting early. Less nested statements generally increase readability.
For bigger arrays you should check Buffer.BlockCopy to optimize copying sections of an array to another array. You could see a rotation as copying 2 sections from one array to another given a pivot. Check out this article about the subject.
If you can change the signature, make a static class with static method, use a meaningful method name RotateRight with arguments array and count and make it clear whether in-place rotation is performed or a new array is created.


Minecraft auto-backup utility I live in a country where an electricity blackout happens after every hour, which corrupts the Minecraft save files, so I came up with this auto backup utility which backs up the %appData%.minecraft\saves folder constantly after a specified time.

It works fine but I'm still not satisfied with it.  Tell me what more I can do to improve/enhance it.

import time
import os
import sys

#class for parsing settings.ini
class parseIni(object):
    def __init__(self, fileStream):
        self.lines = fileStream.readlines()
    self.options = []
        for line in self.lines:
            if "=" not in line:
                continue
            else:
                self.options.append(tuple(line.replace("\n", "").split("=")))
    def getValueOf(self, option):
        for op, value in self.options:
                if op==option:
                    return value

#Tries to open the settings file, if unsuccessful then exit program
settingsFileStream = None
try:
    settingsFileStream = open("settings.ini")
except IOError:
    print("ERROR: SETTINGS FILE NOT FOUND.", file=sys.stderr)
    raw_input("Press <ENTER> to exit....")
    sys.exit(0)

#Initialize parser
settings = parseIni(settingsFileStream)

#Precondition: settings file was opened successfully
#Assigning values to required variables
timeDelayInMins = eval(settings.getValueOf("timeDelay")) #For user information
timeDelay = timeDelayInMins * 60 #Converting to seconds, for time.sleep() method
sourcePath = settings.getValueOf("sourcePath")
destinationPath = settings.getValueOf("destinationPath")

#Runs the game
os.system("Minecraft.exe")

#Prints out Welcome message
print()
print("|--------------------------------------------------------|")
print("|WELCOME TO MINECRAFT AUTO BACKUP UTILITY!               |")
print("|--------------------------------------------------------|")
print("|REFER TO THE settings.ini FILE TO EDIT SETTINGS FOR THIS|")
print("|PROGRAM. e.g. TIME DELAY FOR AUTO BACKUP, PATHS etc     |")
print("|--------------------------------------------------------|")
print("|TIME DELAY FOR AUTO BACKUP IS {:0.2f} MIN(S)        |".format(timeDelayInMins))
print("|--------------------------------------------------------|")
print()

#Variable to keep tract of backups done per session
backupsDone = 0

#mainLoop runs forever, backups "saves" folder every after ever timeDelay mins
while True:
    time.sleep(timeDelay)
    os.system('robocopy "{}" "{}" /e'.format(sourcePath, destinationPath))
    backupsDone += 1
    print
    print
    print("|---------------------------------|")
    print("|{} BACKUP(S) DONE THIS SESSION.  |".format(backupsDone))
    print("|---------------------------------|")

And the settings.ini file with the program is:

[TIME]
timeDelay=1
[PATHS]
sourcePath=%appData%\.minecraft\saves
destinationPath=%appData%\.minecraft\SavesBackup

import time
import os
import sys

#class for parsing settings.ini
class parseIni(object):

Python style guide recommends names like ParseIni

    def __init__(self, fileStream):

Python style guide recommends argument like file_stream.

        self.lines = fileStream.readlines()

You don't use the lines again, so why are you storing them on the object. There is not a whole lot of point in calling deadlines, because you can actually just pass the file object in the for loop for the same effect

    self.options = []
        for line in self.lines:
            if "=" not in line:
                continue

I suggest almost always avoiding continue. As it is continue does nothing, replacing with pass will have the same effect. The whole if can be simplified by inverting the condition

            else:
                self.options.append(tuple(line.replace("\n", "").split("=")))

For clearing out excess whitespace, I recommend using line.strip(). I'd also suggest having options be a dictionary. Then you could do

 key, value = line.strip().split('=')
 self.options[key] = value

It'll make lookup more efficient and is a bit simpler

    def getValueOf(self, option):

Awkward and longish name. I'd have this be __getitem__, and then you can access it with the [] operator

        for op, value in self.options:
                if op==option:
                    return value

This is the for-if anti-pattern. Whenever you find yourself writing an if which should only be true for one instance of the surrounding loop, you should look for a better solution.

Also, what happens if the option is missing? You should probably raise an exception.

#Tries to open the settings file, if unsuccessful then exit program
settingsFileStream = None

No point in doing this.

try:
    settingsFileStream = open("settings.ini")
except IOError:
    print("ERROR: SETTINGS FILE NOT FOUND.", file=sys.stderr)
    raw_input("Press <ENTER> to exit....")
    sys.exit(0)

You should probably print the exception itself. As it is you are throwing away potentially useful error information.

#Initialize parser
settings = parseIni(settingsFileStream)

#Precondition: settings file was opened successfully
#Assigning values to required variables
timeDelayInMins = eval(settings.getValueOf("timeDelay")) #For user information

Using eval is a bad idea. It'll try to evaluate any valid python you pass to it which could allow an external file to cause your application to do bad things. You also can't be sure what kind of data you got back. Instead pass the incoming string to int or float to safely convert.

timeDelay = timeDelayInMins * 60 #Converting to seconds, for time.sleep() method
sourcePath = settings.getValueOf("sourcePath")
destinationPath = settings.getValueOf("destinationPath")

#Runs the game
os.system("Minecraft.exe")

#Prints out Welcome message
print()
print("|--------------------------------------------------------|")
print("|WELCOME TO MINECRAFT AUTO BACKUP UTILITY!               |")
print("|--------------------------------------------------------|")
print("|REFER TO THE settings.ini FILE TO EDIT SETTINGS FOR THIS|")
print("|PROGRAM. e.g. TIME DELAY FOR AUTO BACKUP, PATHS etc     |")
print("|--------------------------------------------------------|")
print("|TIME DELAY FOR AUTO BACKUP IS {:0.2f} MIN(S)        |".format(timeDelayInMins))
print("|--------------------------------------------------------|")
print()

#Variable to keep tract of backups done per session
backupsDone = 0

#mainLoop runs forever, backups "saves" folder every after ever timeDelay mins
while True:
    time.sleep(timeDelay)
    os.system('robocopy "{}" "{}" /e'.format(sourcePath, destinationPath))

Python has function for copying files. Consider using that instead of calling to another program.

    backupsDone += 1
    print
    print
    print("|---------------------------------|")
    print("|{} BACKUP(S) DONE THIS SESSION.  |".format(backupsDone))
    print("|---------------------------------|")

It would also be a good idea to move all this logic into a main() function.


Python Converting numbers to words, using Recursive My function recursively calls itself until it decodes every numbers. rec_ntw

face_value_count -> (Incremental variable) denotes ones, tens, hundreds and thousands
face_value -> (Dictionary) denotes the dictionary containing corresponding the word value for numbers.

result -> (String) Contains final result to return.

teen -> (Integer) to decode the teen numbers from eleven to ninteen

number % 10 to get the last element

number // 10 to remove the last element and pass it on to recursive the function.

def rec_ntw(number, face_value_count=0):
        result = ""
        teen = 0
        if number <= 0:
                if face_value_count == 0:
                        result = "Zero"
                print (result)
                #return result
        else:
                if face_value_count == 0:
                        result = (singleton[int(number % 10)])
                        teen = number % 10
                elif face_value_count == 1:
                        if number % 10 == 1:
                                result = (face_value[1][1][int(teen)]) + " "
                        else:
                                result = (face_value[int(face_value_count)][int(number % 10)]) + " " + result
                                teen = 0
                elif face_value_count == 2:
                        if number % 10 != 0:
                                result = (singleton[int(number % 10)] + " " + str(face_value[int(face_value_count)]) + " ") + result
                else:
                        result = (singleton[int(number % 10)] + " " + str(face_value[int(face_value_count)]) + " ") + result
                face_value_count += 1
                rec_ntw(number // 10, face_value_count)

Please review my code and lemme know how to write effectively.

What is your desired output? Your function has no return statements (other the one commented out), and the only print statement is in the number <= 0 block. Have you run this function? Unless I'm missing something, it won't do anything if you input 0. And when you strip out the last digit, you don't actually store it anywhere, so it's just gone.

You seem to be doing double tabs for your indents, which makes it hard to read once you get several levels deep. Also, you can save yourself an indent level after the number <= 0 block; since you have nothing but an else block after this, you can put a return at the end of the if block. Then you won't ever get to the else block unless the if condition is false, making the else redundant. So you can get rid of that and move everything in an indent level.

You don't show what face_value is, so it's hard to evaluate your code when it comes to that part.

Also, unless I'm misunderstanding, the if number <= 0 block should be something like this:

if face_value_count == 0:
     result = "Zero"
     print (result)
     return(result)
return('negative '+rec_ntw(-number, face_value_count))

You could replace the if else blocks after the number <= 0 block with a case statement.

Using recursion in this case is rather odd. The basic structure of your function is:

def f(x):
  #do action 1
  #do action 2
  #do action 3

But you're implementing it as:

def f(x,instruction_address):
   if instruction_address == 1:
       # do action 1
   if instruction_address == 2:
       # do action 2
   if instruction_address == 3:
       # do action 3
   f(x, instruction_address+1)

That's just ... really weird.

Consider the following code:

def numbers_to_words(number):
    numbers = ['zero','one','two','three','four','five','six','seven','eight','nine',
              'ten','eleven','twelve','thirteen','fourteen','fifteen','sixteen',
              'seventeen','eighteen','nineteen']
    tens_places = ['twenty','thirty','fourty','fifty','sixty','seventy','eighty','ninety']
    if number <20:
        return(numbers[number])
    #print(number)
    if number%10 == 0:
        return(tens_places[number//10-2])
    return(tens_places[int(str(number)[0])-2]+'-'+numbers[int(str(number)[1])])

This turns any number from 0 to 99 to words, no recursion. It can easily be extended to -999 to 999, and a little more work would get it to, say, -10^33 to 10^33.


Python date compression I m making a script to store date information in the smallest size in bits

(I don't mind of the actually memory use in the program / runtime)

#!/usr/bin/env python
import random
import datetime

MAGIC = "DAT"

def extract(data, size):
    return (((data >> size) << size) ^ data)

def compress_date(year, month, day, hour, minute):
    assert year <= 31
    assert month <= 12
    assert day <= 31
    assert hour <= 24
    assert minute <= 60

    data = 0
    data = (data << 5) + year
    data = (data << 4) + month
    data = (data << 5) + day
    data = (data << 5) + hour
    data = (data << 6) + minute
    return (data)

def decompress_date(data):
    minute = extract(data, 6)
    data >>= 6

    hour = extract(data, 5)
    data >>= 5

    day = extract(data, 5)
    data >>= 5

    month = extract(data, 4)
    data >>= 4

    year = extract(data, 5)
    data >>= 5
    return (year, month, day, hour, minute)

date = serialgen.compress_date(10, 11, 30, 24, 60)
print date
print hex(date)
print decompress_date(date)

output:

11269692
0xabf63c
(10, 11, 30, 24, 60)

Can I do something to clear up the ugly parts:

data = 0
data = (data << 5) + year
data = (data << 4) + month
data = (data << 5) + day
data = (data << 5) + hour
data = (data << 6) + minute

minute = extract(data, 6)
data >>= 6

hour = extract(data, 5)
data >>= 5

day = extract(data, 5)
data >>= 5

month = extract(data, 4)
data >>= 4

year = extract(data, 5)
data >>= 5

Do you have any suggestions to improve this code?

Weird limits

I have absolutely no idea of what date is "the 12th of June at 24 hour 60 minutes", yet you consider it a valid date as regard to your bounds. You should assert hours and minutes to be respectively 23 and 59 at max.

Using a two-digit long value for the date seems rather useless too. What century does it encode exactly? And assuming it is for years in the 2000 era, why limit to 2031?

Unused stuff

Both your imports and constant are unused, you should remove them.

Bitfield manipulation

Extracting informations out of a bitfield is generally done using a mask. You need to create the mask corresponding to size bits set using mask = (1 << (size + 1)) - 1 and then you can use data & mask to get the desired value. The advantage being that you can easily add an offset to grab the exact portion you’re interested in:

def extract(data, size, offset):
    mask = (1 << (size + 1)) - 1
    return (data & (mask << offset)) >> offset

def decompress_date(data):
    minute = extract(data, 6, 0)  # minutes are at the end, no offset
    hour = extract(data, 5, 6)  # hours are after minutes (6)
    day = extract(data, 5, 11)  # days are after hours (5) and minutes (6)
    month = extract(data, 4, 16)  # you get it
    year = extract(data, 5, 20)
    return (year, month, day, hour, minute)

Value creation

You can simplify the compressed data creation by dropping useless initialization:

def compress(...):
    data = year
    data = (data << 4) + month
    ...

Knowing the offsets, you can also use them to create the compressed data all at once. To simplify reading and understanding, you can also define a bunch of constants:

MINUTES_SIZE = 6
HOURS_SIZE = 5
DAYS_SIZE = 5
MONTHS_SIZE = 4
YEARS_SIZE = 5

MINUTES_OFFSET = 0
HOURS_OFFSET = MINUTES_OFFSET + MINUTES_SIZE
DAYS_OFFSET = HOURS_OFFSET + HOURS_SIZE
MONTHS_OFFSET = DAYS_OFFSET + DAYS_SIZE
YEARS_OFFSET = MONTHS_OFFSET + MONTHS_SIZE

def decompress_date(data):
    minute = extract(data, MINUTES_SIZE, MINUTES_OFFSET)
    hour = extract(data, HOURS_SIZE, HOURS_OFFSET)
    day = extract(data, DAYS_SIZE, DAYS_OFFSET)
    month = extract(data, MONTHS_SIZE, MONTHS_OFFSET)
    year = extract(data, YEARS_SIZE, YEARS_OFFSET)
    return (year, month, day, hour, minute)

def compress_date(year, month, day, hour, minute):
    assert year <= 31
    assert 0 < month <= 12
    assert 0 < day <= 31
    assert 0 <= hour < 24
    assert 0 <= minute < 60

    return (
        (year << YEARS_OFFSET) +
        (month << MONTHS_OFFSET) +
        (day << DAYS_OFFSET) +
        (hour << HOURS_OFFSET) +
        (minute << MINUTES_OFFSET)
    )

Timestamps

The "classic" condensed representation of a point in time is given by a timestamp: the elapsed time in seconds since January 1st 1970. Since you’re only interested in minutes, you can create a simple wrapper around Python's datetime.timestamp():

import datetime

def compress(date):
    return int(date.timestamp() // 60)

def compressed_date(year, month, day, hour, minute):
    return compress_date(datetime.datetime(year, month, day, hour, minute)

def uncompress(truncated_timestamp):
    return datetime.datetime.fromtimestamp(truncated_timestamp * 60)

And support a wider variety of dates.


Create Form Controls based on the properties of a class Is there a better way to create Form Controls for properties of a class? Right now, I iterate through all properties of a class and have a method to create the Form Control for that property based on the type of the property.

private void AddControl(PropertyInfo p, string name = null, Color? c = null, int count = 0)
{
    Label newLabel = new Label();
    TableLayoutPanel tp = new TableLayoutPanel();
    Control ctrl = new Control();

    tp.ColumnCount = 2;
    tp.ColumnStyles.Add(new ColumnStyle(SizeType.Absolute, 200F));
    tp.ColumnStyles.Add(new ColumnStyle(SizeType.Absolute, 215F));
    tp.Margin = new Padding(0);
    tp.Name = "tp";
    tp.RowCount = 1;
    tp.RowStyles.Add(new RowStyle(SizeType.Absolute, 28F));
    tp.Size = new System.Drawing.Size(380, 28);
    tp.Location = new Point(0, 14 + num * 28);

    newLabel.Name = "lbl_" + num;
    newLabel.Text = GetPropertyAttributes(p); // gets a custom attributes for the property
    newLabel.AutoSize = true;
    newLabel.Anchor = AnchorStyles.Left;

    tp.Controls.Add(newLabel);
    ToolTip myToolTip = new ToolTip();

    switch (p.PropertyType.Name)
    {
        case "String":
            ctrl = new TextBox();
            ctrl.Tag = "String";
            ctrl.Size = new System.Drawing.Size(170, 20);
            goto case "common";
        case "Boolean":
            ctrl = new CheckBox();
            goto case "common";
        case "Array[]":
            Console.WriteLine("Array");
            break;
        case "Int32":
            ctrl = new TextBox();
            ctrl.Tag = "Int";
            ctrl.Size = new System.Drawing.Size(40, 20);
            goto case "common";
        case "Double":
            ctrl = new TextBox();
            ctrl.Tag = "Double";
            ctrl.Size = new System.Drawing.Size(40, 20);
            goto case "common";
        case "IList`1":
            if (ClassExists(p.Name)) // ClassExists checks if the property is a class itself
            {
                ctrl = new Button();
                ctrl.Text = "Add";
                ctrl.Tag = p;
                ctrl.AutoSize = true;
                ctrl.Click += new EventHandler(AddButton_Click);
            }
            else
            {
                // TODO create gridview
            }
            goto case "common";
        case "common":
            ctrl.Anchor = AnchorStyles.Left;
            ctrl.Name = name ?? GetPropertyAttributes(p);
            ctrl.Font = new Font("Consolas", 9);
            ctrl.KeyPress += new KeyPressEventHandler(textBoxKeyPress);
            if (c != null)
            {
                tp.BackColor = c ?? Color.Black;
            }
            tp.Controls.Add(ctrl);
            formControls.Add(tp); // List<Control>
            break;
        default:
            ctrl = new Button();
            ctrl.Text = "Add";
            ctrl.Tag = p;
            ctrl.AutoSize = true;
            ctrl.Click += new EventHandler(AddButton_Click);
            goto case "common";
    }
}

private void AddControls()
{
    foreach (Control ctrl in formControls)
    {
        MainPanel.Controls.Add(ctrl); // MainPanel is a Panel Control
    }
}

Can it be simplified? I may have to add more types to it.

In my commented I suggested a class that could do what you want it to do out of the box. However sometimes having a custom piece of code to show things in a better way is the goal. I can see this going in two different ways. One is to use a variation of the Visitor pattern and a list, and the other is to use a dictionary with Type and the key, and the class that represents how to view it as the value.

The first way you could make the IVisitor interface one of two ways.

public interface IVisotor
{
    //option 1
    void Accept(Control control);
    //option 2
    bool Accept(Control control);
}

then to implement you'd make a new class per type that you want to show a specific way. Here is an example with both.

public class StringVisitor : IVisitor
{
    public bool Accept(PropertyInfo property, Control control)
    {
        if(property.PropertyType != typeof(string))
            return false;

        Accept(control);
        return true;
    }

    public void Accept(Control control)
    {
        control = new TextBox();
        control.Tag = "String";
        control.Size = new System.Drawing.Size(170, 20);
    }
}

Now in your class you have two options, make a List<IVisitor> or make a Dictionary<Type, IVisitor>. Here is an example of what I mean. It is by no means complete (and might even have a compiler error as I did it in Notepad++)

public class PropertyViewer
{
    private readonly List<IVisitor> visitors1;
    private readonly Dictionary<Type, IVisitor> visitors2;

    public PropertyViewer()
    {
        visitors1 = new List
        {
            new StringVisitor(),
            //...
        };

        visitors2 = new Dictionary<Type, IVisitor>()
        {
            {String, new StringVisitor()},
            //...
        }
    }
    private void AddControl(PropertyInfo p, string name = null, Color? c = null, int count = 0)
    {
        Control control = null;
        //option 1
        foreach(var visitor in visitors1)
        {
            if(visitor.Accept(p, control))
                break;
        }
        //option 20
        if(visitors2.ContainsKey(p.PropertyType))
        {
            visitors2[p.PropertyType].Accept(control);
        }

        //do your common control stuff here.
    }
}

There are other ways as well but that should encapsulate your work some and make it easier to find where a mistake is. Because of the nature of your work on this it won't be super easy to test that everything is visually pleasing, but the code should be encapsulated enough to make it easy to adjust what you need adjusted.


Minimal Python logging I tend to include something like the following in small and medium-sized Python projects, either at the top of the executable or as logger.py:

import logging
import sys

logger = logging.getLogger(__name__)
stoh = logging.StreamHandler(sys.stderr)
fmth = logging.Formatter("[%(levelname)s] %(asctime)s %(message)s")
stoh.setFormatter(fmth)
logger.addHandler(stoh)
logger.setLevel(logging.DEBUG)

Then import logger from logger (as needed), and proceed to make calls throughout the project like logger.level('message').

Are there any obvious (or non-obvious) red flags or anti-patterns with this approach?

Setting up logging

You might be interested in logging.basicConfig, which can setup everything you setup there as well. It's not sufficient if you want for example both a file and a stream logger, but for a small, static configuration like this it's perfectly suitable. Your code up there becomes a simple:

logging.basicConfig(format="[%(levelname)s] %(asctime)s %(message)s", level=logging.DEBUG)
logger = logging.getLogger(__name__)

And you're done. (The default stream is sys.stderr.)

Do note that in no way, shape or form is the long form wrong - it's easier extendable, for one. However, for a quick script it might be easier to use basicConfig.

Module-level logging

I'd recommend you use a different logger for every different file, and have all of them be named the way you did that one. However, if you then also include the %(name) parameter in your format string, your logging messages will each contain the filename of the file they're logging from. This will help in debugging if you don't know exactly where the problems come from. I personally also include %(lineno) for the line number. Personally, I always use the .format method for my logging strings, which makes it look like this:

logging.basicConfig(format="{levelname: <8}:{asctime}:{name: <30}:{lineno: <4}:{message}", level=logging.DEBUG, style="{")
# Or with % formatting:
logging.basicConfig(format="%(levelname)-8s:%(asctime)s:%(name)-30s:%(lineno)-4s:%(message)s", level=logging.DEBUG)

Which tells me exactly where a logging statement was made as well. You won't have to search for your own exact wording anymore when you resume debugging your code for that one weird issue 3 months from now. You can just grab filename and linenumber and you're at the right location.


Phone number extraction with regular expressions I've been working on my own phone number extraction snippet of Regex code and I'd appreciate your feedback on it. I've tried to make it match as many different types of numbers as I knew existed in the US. (I'm sure there's some I missed.) Is there anything I can do to improve this?

(?:\+?(\d{1})?-?\(?(\d{3})\)?[\s-\.]?)?(\d{3})[\s-\.]?(\d{4})[\s-\.]?

Here's a picture of it in action:

You can test the code out here.

Also for this interested, here is the flow of the "logic":

(Made from pasting the code into here)

Interesting proposal! I could hardly read the expression that you've been working with, so I started from scratch and have built the following:

\D?(\d{0,3}?)\D{0,2}(\d{3})?\D{0,2}(\d{3})\D?(\d{4})$

I tried to take out some of the conditional and capturing groups to clear it up. So we can split this into four part:

\D?(\d{0,3}?)

Is there anything that isn't a number? it's probably a + or something so we'll make sure not to capture that. Then we look for a country code. You had only a check for a single digit country code, and if that's what you want, fine. However, know that some nations have codes more than one character in length.
\D{0,2}(\d{3})?

Is there a dash or a parenthesis? Or both? If so, count those out. Is there an area code? If so, capture it.
\D{0,2}(\d{3})

Same as the previous section, except we're expecting to find those 3 digits, as they're necessary.
\D?(\d{4})$

We expect only one non-digit character between the 3 digit and 4 digit sections. We need to pull the matching to the right with the $. This way we make sure to get the very last digit in the phone number.

I'm sure there are many ways to do this, and this is only my interpretation. Have you tried Googling this to see if there's an established expression?


Implementation of quicksort algorithm I'm currently doing an algorithms course and implemented this version of the quicksort algorithm. I would like to know if it is an efficient implementation. I've seen a couple others where there is a partition function that partitions the list is that a superior method to what I did?

def quickSort(aList):
    first = []
    second = []
    size = len(aList)
    if size ==0:
        return []
    elif size == 1:
        return aList
    elif size ==2:
        if(aList[1]<aList[0]):
            return list([aList[1],aList[0]])
        else:
            return aList
    else:
        key = aList[0]
        for i in range(1,size):
            if aList[i]<key:
                first.append(aList[i])
            else:
                second.append(aList[i])
        first.append(key)
    return quickSort(first) + quickSort(second)

You've implemented a very basic version of the Quicksort algorithm, where you choose the first element as pivot element.

The running time of the quicksort algorithm is however very dependent on the quality of the pivot element. (The pivot should try to split the problem in equal problems). Making it fixed isn't really useful.

For example: say you want to run quicksort on an already sorted list. If you keep using the first element as pivot element, you will end up with a quadratic running time.

A 'solution' is to use random pivot elements, where in each recursive call you choose the pivot randomly (each element has an equal chance). You can find more on this approach online. But the idea is to end up with an average running time of \$O(n*log(n))\$

In addition you implement it by creating 2 new lists and merging them together. Quicksort normally has the advantage that everything can happen in-place,  which is a great advantage memory wise.


Injecting an object in each function in Go My current project uses a database represented as object. Furthermore I want to implement methods like CreateUser(), DeleteUser() etc, but my current code requires the injection of the db-object as parameter in each function.

Making the db-object global would solve the problem, but I learnt that global variables are mostly seen as a bad practice. Are there any other solutions?

package main

import (
    "fmt"
    "time"

    "github.com/jinzhu/gorm"
    _ "github.com/lib/pq"
    _ "github.com/mattn/go-sqlite3"
)

type User struct {
    ID        int
    Name      string `sql:"size:50"`
    Username  string `sql:"size:50"`
    CreatedAt time.Time
    UpdatedAt time.Time
}

func CreateUser(db gorm.DB, name, username string) {
    user := User{
        Name:     name,
        Username: username,
    }

    db.Create(&user)
}

func main() {
    db, err := gorm.Open("sqlite3", "data.sqlite3")

    if err != nil {
        fmt.Println(err)
    }

    db.DB()
    fmt.Println("Programm is running...")

    CreateUser(db, "John Doe", "johndoe")
}

Usually, when you have a lot of functions operating on the same data, it's worth considering making those functions methods on this data:

type UserDB struct {
    *gorm.DB
}

func (udb *UserDB) Create(name, username string) error {
    user := User{
        Name:     name,
        Username: username,
    }

    return udb.Create(&user).Error
}

// etc.


Find the smallest number in the first array that is not in the second one I have solved the given assignment below is the question and getting correct answer. but my question is is there any way to reduce the complexity of my code or there is any other efficient way to write the code?

  Given two arrays of integers output the smallest number in the first
  array not present in the second one.

  Input Specification:

  The first line contains the size N1 of the first array.   Next line
  give the contents of the first array.   Next line contains the size N2
  of the second array.   Next line give the contents of the second
  array.

  Output Format:

  Output must be a single number which is the smallest number occurring
  in the first array that does not occur in the second. In case there is
  no such number, output NO.

  Variable Constraints:

  The sizes of the arrays are smaller than 20. Each array entry is an
  integer which fits an int data type.

  Example Input:

3
2 3 4
4
1 3 5 7

  Output: 2

  Example Input:

1
1
2
1 2

  Output: NO

Here is the code.

#include<stdio.h>
int main(void)
{
    int a[10],b[10];
    int small=0,l1,l2,i,flag=0;
    scanf("%d",&l1);
    for(i=0;i<l1;i++)
    {
        scanf("%d",&a[i]);
    }
    scanf("%d",&l2);
    for(i=0;i<l2;i++)
    {
        scanf("%d",&b[i]);
    }
    small=a[0];
    for(i=1;i<l1;i++)
    {
        if(small>a[i])
          small=a[i];
    }
    for(i=0;i<l2;i++)
    {
        if(small==b[i])
          flag=1;
    }
        if(flag==1)
        {
            printf("NO");
        }
        else
        {
            printf("%d",small);
        }
   return 0;
}

Why do you make your arrays only 10 elements large when the task explicitly says 20?

Even worse, you don't protect the program from writing outside the array. The C programming language doesn't protect against this wrong behavior, therefore you as the programmer must do this.

Before every write to an array you must check that the array index is valid. For example:

assert(0 <= idx && idx < arr_len);
arr[idx] = value;

Only if you can prove mathematically that the assert condition is always true, can you leave out the assert. Follow this advice, and your programs will be better than 80% of the existing C programs.

To reduce the complexity of the main function, you should define a function like this:

bool intarray_contains(const int *arr, size_t arr_len, int value) {
    ...
}

Then you don't need to write this code in main.

After that, to ensure that your program works correctly (it currently doesn't), you should extract the calculation into a separate function:

int min_not_present_in(const int *arr1, size_t arr1_len, const int *arr2, size_t arr2_len) {
    ...
}

This function definition allows you to do the calculation without entering the values every time. Like this:

int positive[] = { 1, 2, 3 };
int odd[] = { 1, 3, -1 };
int even[] = { -2, 0, 2 };

assert(min_not_present_in(positive, 3, odd, 3) == 2);
assert(min_not_present_in(positive, 3, even, 3) == 1);

To make all this work, you need the following lines at the top of your file:

#include <assert.h>
#include <stdbool.h>


Adding and removing classes at different heights on page using jQuery I want to remove/add classes when the user is at different distances from the top by using jQuery.

I have successfully done it, and it works fine, but I think I'm doing it wrong, and I would like your help to optimize the code.

The html is simple, basically the sections(including the header), have 100% width. and different colors. I want to make the header change color when its over the first section(for aesthetical purposes).
And I also want it to have a shadow when the page has been scrolled more than 1 pixel.

I'm doing it by adding/removing classes.

When I use one big else if statement it doesn't work well because whenever any any condition is matched js stops checking for other matches, so it doesn't apply all the classes needed.

The next code works, however as I said, I think that it's not optimal/badly written.
Here is the HTML markup:

  Header

  Please Scroll Down to see the header changes...

  The header color Should change when you pass through me.

And here is the jQuery code:

var header = $('header'),
        blueSection = $('section.blue'),
    // Calculate when to change the color.
        offset = blueSection.offset().top + blueSection.height() - header.height();

$(window).scroll(function(){
  var scroll = $(window).scrollTop();

    // Remove Class "dark" after scrolling over the dark section
  if (scroll >= offset) {
    header.removeClass('dark');
  } else {
    header.addClass('dark');
  }

    // Remove Class "no-shadows" whenever not on the top of the page.
  if (scroll >= 1) {
    header.removeClass('no-shadow');
  } else {
    header.addClass('no-shadow');
  }

});

And for those of you who like to use JSFiddle(like me!):
https://jsfiddle.net/shock/wztdt077/6/

I know you tagged this jQuery, but I believe that loading in jQuery for this seems like a lot of processing for little benefit, so I decided to write it in a more universal approach, which I think is the main problem with your code.

What you want to accomplish is to have a little script that can be included and will work predictably in as many circumstances without being to specific, so first I went about actually creating a structure:

The element the class will be applied to will have an attribute named data-scroll-group - it will define the group name for these scrollable items.
The elements that your header will respond to will contain an attribute constructed of the above group name and prefixed by data. It's contents will be the classname(s) you want to apply.

This structure looks like this:

This means you could have many scroll groups on the page, allowing you to reuse the code with different aspect and different headers, different responders, etc... It makes your code less connected to the DOM and more based in a structure.

The rest is pretty simple. Use getBoundingClientRect to get the elements position compared to the viewport. I am currently applying the classes using the position of the viewport, but you could simply get the same getBoundingClientRect on the header and add the values together to get a result for your header specifically.

Check out the snippet and tell me if you have any questions about it. You could replace my header.className with jQuery if you wanted - you could replace most of it with jQuery, but I think coupling your code to jQuery has no benefit here, as you want this to work anywhere, and pure vanilla Javascript is about as long.

// This is a handy wrapper function that will return an array of matching element instead of a nodeList
function querySelectorArray(query, root){
  return Array.prototype.slice.call((root || document).querySelectorAll(query));
}

// Get all headers that are designated 'scroll-group'
var headers = querySelectorArray('[data-scroll-group]');

// Loop through the headers
headers.forEach(function(header){
  // Get the name of the group from the headers [data-scroll-group] attribute
  var group = header.getAttribute('data-scroll-group');
  // Get all the sections with a matching data-[data-scroll-group] attribute
  var sections = querySelectorArray('[data-' + group + ']');
  // Create an Event Listener for scrolling
  window.addEventListener('scroll', function(){
    // Declare a lastSection variable that can store the last class that scrolled by
    var lastSection = false;
    sections.forEach(function(section){
      // Get the elements position compared to the viewport
      var offset = section.getBoundingClientRect();
      // If the position is smaller than 0 it has scrolled further than that section
      // The same is true for the scroll being smaller than the negative height - if so, it is out of view.
      if(offset.top < 0 && offset.top > -offset.height) lastSection = section.getAttribute('data-' + group + '');
    });
    // Apply the class to your header
    header.className = lastSection || '';
  })
});
body {
  padding: 0;
  margin: 0;
  padding-top: 20px;
  height: 405vh;
  font-family: Arial, serif;
}
header {
  position: fixed;
  width: 100%;
  top: 0;
  left: 0;
  background: #dd0300;
  -webkit-transition: all 1s;
  transition: all 1s;
  color: #fff;
  padding: 5px 20px;
  box-sizing: border-box;
}
header.blue {
  background: #4e88ff;
}
header.shadow {
  box-shadow: 0 0 20px #000;
}
section {
  box-sizing: border-box;
  width: 100%;
  height: 100vh;
  background: #ccc;
  padding: 20px;
}

  Header

  Scroll Down (header will become blue)

  Scroll Down (header will have a shadow, no longer blue)

  Scroll Down (header will have a shadow and be blue)

  Scroll Down (header will reset)


Generating all unique permutations of a string Here is a plan for generating the permutations of S: For each item x
  in S, recursively generate the sequence of permutations of S - x, and
  adjoin x to the front of each one. This yields, for each x in S, the
  sequence of permutations of S that begin with x. Combining these
  sequences for all x gives all the permutations of S:

The plan to generate all the
permutations of a set S

I would like to use the process described above to generate the permutations of a string.

I used LinkedHashSet because I'd like to get rid of all the duplicate permutations if there are the same characters in the string. Besides, the add operation is constant.
I will have to remove a character from a string lots of times. But Syntactically, since this is recursive, I will only do the removing of characters once in the code definition (str.substring(0, i)+str.substring(i+1)). Do I still need to put this code to another method? The only advantage I can think of is clarity but this code seems too small for that to matter much.
I don't know much about how to analyze my code, but since we need to find the ways to order the set while removing every character and putting this character in front of each one, I think the running time is n factorial+n. For example, if we have a string with 3 characters, we would find the ways of ordering 3 subsets (the subsets where the first, second and third characters are removed). Each of those subsets results in two ways, that's why we get 3*2 ways after putting the removed character in the front of each. The +n part is the substring operations which is O(n). Is this all correct?
Regardless of if my code runs at n factorial or not, I think this is absolutely, horribly, extremely slow. How can I make it faster?
I found that java string's immutability is helpful to what my code is doing. But I still feel that I am wasting a lot of string objects because most of them will be trashed anyway when I append a character to it. How can I improve management of my strings??

Please review my code.

public class Permutation {

    public Set<String> permute(String str) {
        Set<String> res = new LinkedHashSet<>();
        if (str.isEmpty()) {
            res.add(str);
        } else {
            for (int i = 0; i < str.length(); i++) {
                for (String s : permute(str.substring(0, i)+str.substring(i+1))) {
                    res.add(str.charAt(i)+s);
                }
            }
        }
        return res;
    }

}

Pay attention to your substrings. Given s = "abc", then the following return empty strings.

String pre = s.substring(0,0);
String post = s.substring(3);

Consider (temporarily) adding logging statements inside your innermost loop and examining the results of calling permute("abc") to see how many times you add the same strings to the set. Try to avoid adding to the set a value already in the set concatenated to an empty string, since this gives you no new values in the set.


Simple http helper in Go Can you please share your thoughts on the following simple httphelper in Go? Thanks in advance.
I mainly would like to know whether returning a channel of a custom type seems like a good idea and if so , how could I improve it for only receive chans.
package services

import (
    "bytes"
    "encoding/json"
    "io/ioutil"
    "net/http"
    "time"
)

type HHResult struct {
    Result     string
    StatusCode int
    Error      error
}

type IHttpHelper interface {
    Request(method string, url string, headers map[string][]string, payload interface{}) chan HHResult
    RequestWithOptions(method string, url string, headers map[string][]string, payload interface{}) chan HHResult
}

var singleton IHttpHelper

type HttpHelper struct{}

func (h *HttpHelper) Request(method string, url string, headers map[string][]string, payload interface{}) chan HHResult {
    var result chan HHResult = make(chan HHResult)
    go func(output chan HHResult) {
        payl, err := json.Marshal(payload)
        if err != nil {
            result <- HHResult{
                Result:     "",
                StatusCode: 0,
                Error:      err,
            }
        }
        req, err := http.NewRequest(method, url, bytes.NewReader(payl))
        client := http.Client{
            Timeout: time.Second * 30,
        }
        resp, err := client.Do(req)
        if err != nil {
            result <- HHResult{
                Result:     "",
                StatusCode: 0,
                Error:      err,
            }
        }
        respbytes, err := ioutil.ReadAll(resp.Body)
        defer resp.Body.Close()
        result <- HHResult{
            Result:     string(respbytes),
            StatusCode: resp.StatusCode,
            Error:      nil,
        }

    }(result)
    return result
}

func (h *HttpHelper) RequestWithOptions(method string, url string, headers map[string][]string, payload interface{}, timeout int) chan HHResult {
    var result chan HHResult = make(chan HHResult)
    go func(output chan<- HHResult) {
        payl, err := json.Marshal(payload)
        if err != nil {
            result <- HHResult{
                Result:     "",
                StatusCode: 0,
                Error:      err,
            }
        }
        req, err := http.NewRequest(method, url, bytes.NewReader(payl))
        client := http.Client{
            Timeout: time.Second * timeout,
        }
        resp, err := client.Do(req)
        if err != nil {
            result <- HHResult{
                Result:     "",
                StatusCode: 0,
                Error:      err,
            }
        }
        respbytes, err := ioutil.ReadAll(resp.Body)
        defer resp.Body.Close()
        result <- HHResult{
            Result:     string(respbytes),
            StatusCode: resp.StatusCode,
            Error:      nil,
        }

    }(result)
    return result
}

func NewHttpHelper() IHttpHelper {
    if singleton == nil {
        singleton = &HttpHelper{}
    }
    return singleton
}

Example of usage
func main() {
  helper := services.NewHttpHelper()
  res := <-helper.Request("GET", "https://jsonplaceholder.typicode.com/users/1", nil, nil)

  if res.Error != nil {
      fmt.Println(res.Error.Error())
      return
  }
  fmt.Println(res.StatusCode)
  fmt.Println(res.Result)
}

Listing in order the things I made note of/changed looking through your code:

output chan HHResult is declared but never used in go funcs(output chan HHResult). It's fine that it's not used, the go func has access to  Result in the parent function, but you don't need to declare it if it's not needed.

The headers map argument is never used either.

Looking at Request and RequestWithOptions they're the same function except for the variable timeout. Request can be a small wrapper that just sets a default timeout then calls the more general function

Similarly, you repeatedly return the same HHResult only substituting which err is used so that can be pulled into it's own function sendErr(chan, err)

Any exported function or type (any that start with capital letters) should be commented

If you look in the net/http package you'll see it's standard to capitalize HTTP in variable names. Same with JSON.

Bugs:

a. One bug in your code is you send JSON via http.Request but you don't actually set the Content-Type to application/json which can cause issues with how your request is interpreted on the other end
b. Another bug is that the error in respbytes, err := ioutil.ReadAll(resp.Body) is never used and you actually return nil for it whether or not you could actually read the body.
c. Timeout: time.Second * 30 throws an error

As written, I don't see the need for the IHTTPHelper interface. HttpHelper has the only two methods used hanging directly from the struct and there's no way to inject a different type implementing the same interface into any of your code. As is HttpHelper alone does everything you need.

Purely stylistically, I think some of the variable names like HHResults are confusing and rewrote them

package services

import (
    "bytes"
    "encoding/json"
    "io/ioutil"
    "net/http"
    "time"
)

// HelperResponse is the response returned by an HTTPHelper.Request()
type HelperResponse struct {
    Result     string
    StatusCode int
    Error      error
}

// HTTPHelper hangs Request and RequestWithTimeout methods
type HTTPHelper struct{}

var singleton *HTTPHelper

// NewHTTPHelper returns a pointer to the HTTPHelper singleton
func NewHTTPHelper() *HTTPHelper {
    if singleton == nil {
        singleton = &HTTPHelper{}
    }
    return singleton
}

func sendErr(out chan HelperResponse, err error) {
    out <- HelperResponse{
        Result:     "",
        StatusCode: 0,
        Error:      err,
    }
}

// Request wraps RequestWithTimeout with default 30 second timeout
func (h *HTTPHelper) Request(method string, url string, payload interface{}) (out chan HelperResponse) {
    return h.RequestWithTimeout(method, url, payload, 30)
}

// RequestWithTimeout returns Response, StatusCode, and any Errors from an http request
func (h *HTTPHelper) RequestWithTimeout(method string, url string, payload interface{}, timeout int) (out chan HelperResponse) {
    go func() {
        payloadJSON, err := json.Marshal(payload)
        if err != nil {
            sendErr(out, err)
        }

        req, err := http.NewRequest(method, url, bytes.NewReader(payloadJSON))
        req.Header.Add("Content-Type", "application/json")

        client := http.Client{Timeout: time.Second * time.Duration(timeout)}

        resp, err := client.Do(req)
        if err != nil {
            sendErr(out, err)
        }

        respBytes, err := ioutil.ReadAll(resp.Body)
        defer resp.Body.Close()

        out <- HelperResponse{
            Result:     string(respBytes),
            StatusCode: resp.StatusCode,
            Error:      err,
        }
    }()

    return out
}


Find and print all prime numbers (less than 10000) I want to improve my JS skills by doing challenges. My current challenge is: Write a script which allow user to enter number and then print all prime numbers from 0 to that number.

const PrimeFinder = (function() {

  const PRIMES = [];
  const MAX_RANGE = 10000;

  // cache DOM
  const EL_FIND = document.getElementById('find');
  const EL_RESULT = document.getElementById('result');
  const EL_SEARCH = document.getElementById('search');

  // bind events
  EL_FIND.addEventListener('click', findPrimes);

  function _renderResult() {
    for (let i = 0; i < PRIMES.length; i++) {
      const EL_PRIME = EL_RESULT.appendChild(document.createElement('div'));
      EL_PRIME.textContent = PRIMES[i];
    }
  }

  function _resetResult() {
    PRIMES.length = 0;
    EL_RESULT.textContent = '';
  }

  function _validUserInput(N) {
    return !isNaN(parseFloat(N)) &&
      isFinite(N) &&
      N >= 0 &&
      N <= MAX_RANGE;
  }

  function isPrime(n) {
    let start = 2;
    while (start <= Math.sqrt(n)) {
      if (n % start++ < 1) return false;
    }
    return n > 1;
  }

  function findPrimes(val) {
    const USER_INPUT = (typeof val === 'number') ? val : EL_SEARCH.value;
    _resetResult();

    if (_validUserInput(USER_INPUT)) {
      for (let i = 2; i <= USER_INPUT; i++) {
        if (isPrime(i)) PRIMES.push(i);
      }
      _renderResult();

      return PRIMES;
    }
  }

  const publicAPI = {
    isPrime: isPrime,
    findPrimes: findPrimes
  }

  return publicAPI;

})();
body,
html {
  padding: 0;
  margin: 0;
}

.container {
  max-width: 320px;
  margin: 0 auto;
}

#search,
#find {
  background: #fff;
  color: #000;
  height: 26px;
  line-height: 26px;
  margin: 0;
  padding: 0;
  box-sizing: border-box;
  border: 1px solid #666;
}

#find {
  width: 120px;
}

#search {
  width: 190px;
}

#result {
  margin: 10px 0;
}

#result>div {
  width: 20%;
  float: left;
  box-sizing: border-box;
  text-align: center;
  color: #fff;
  line-height: 48px;
  background: coral;
  border: 2px solid #fff;
}

#result div:hover {
  background-color: red;
}

Prime Number Finder

  <input placeholder="" id="search" type="number">
  <button id="find">Find</button>

I would like to know what could be done better and why.

This is a very nice start! It is clear that you have put some effort into making your code clear and easy to read - it paid off. That said, there are a few things I would recommend changing.

Don't take input as a float. This opens up the possibility for confusion without providing any benefit to the user.
Include USER_INPUT in the range tested for primes. If I input 5 using the form I only get 2 and 3, while I expected 2, 3, and 5.
Why do you cache PRIMES only to reset it every time findPrimes is called? If you are going to spend the memory to save a list of primes, you might as well make use of it.
I see you have started to optimize your solution by only going up to Math.sqrt(n), but you are still using an inefficient algorithm. I would encourage you to look into the Sieve of Eratosthenes as your input is bounded.
Don't mix your logic and display code. While for a small project like this it isn't much of an issue, if you want to use PrimeFinder somewhere else in your application you would have to rewrite it to strip out all of the page specific code.

With this in mind, here is how I would write PrimeFinder as a module without handling DOM manipulations.

const PrimeFinder = (function() {
    let PRIMES = [];
    const MAX_RANGE = 10000;

    function _generatePrimes() {
        // Computing 10000 is not much different than computing 500, so fill the entire array.
        let marks = Array(MAX_RANGE + 1).fill(false)
        for (let i = 2; i <= MAX_RANGE; i++) {
            // Already marked as non-prime?
            if (marks[i]) continue;
            // Mark all multiples
            for (let j = i * 2; j <= MAX_RANGE; j += i) {
                marks[j] = true;
            }
            // This number hasn't been marked, and therefore is prime
            PRIMES.push(i);
        }
    }

    function isPrime(n) {
        if (Number.isNaN(n)) throw Error('n must be a number');
        if (n > MAX_RANGE || n < 0) throw Error('n is out of range.');
        if (!PRIMES.length) _generatePrimes();
        // Could be sped up with a binary search.
        return PRIMES.includes(n);
    }

    function findPrimes(upperBound) {
        if (Number.isNaN(upperBound)) throw Error('upperBound must be a number');
        if (upperBound > MAX_RANGE) throw Error(`upperBound must be less than ${MAX_RANGE}`);
        if (upperBound < 0) throw Error('upperBound must be at least 0');

        if (!PRIMES.length) _generatePrimes();

        let index = PRIMES.findIndex(n => n > upperBound);
        // Return a copy of the array so users can't mess with the module
        return index == -1 ? [...PRIMES] : PRIMES.slice(0, index);
    }

    return { isPrime, findPrimes };
}());

function displayArray(parent, items) {
  parent.innerHTML = '';
  items.forEach(value => {
    let el = document.createElement('li');
    el.textContent = value;
    parent.appendChild(el);
  });
}

document.querySelector('input').addEventListener('input', event => {
  let num = parseInt(event.target.value, 10);
  if (num > 10000) event.target.value = num = 10000;
  if (num < 0 || Number.isNaN(num)) event.target.value = num = 0;
  displayArray(
    document.querySelector('ul'),
    PrimeFinder.findPrimes(num)
  );
});
<input type="number" />

The snippet behaves similarly to the provided snippet without mixing DOM logic into the PrimeFinder functionality - this makes it possible to easily pull out PrimeFinder for use in other projects (even those where the DOM might not exist).


Stock performance app I'm building an app that tells you a stock's percentage change in price between two dates by using Yahoo's API. Given a stock ticker and two dates, it currently outputs the percentage return to the console.

I would like to later expand this app to give it a list of stock tickers and dates. For each, it would return the performance relative to another stock between the dates.

I want the app to try to pull stock data from the Yahoo API, but to then try another API if the data for that stock is not available from Yahoo.

Here's what I have so far. I believe I'm using the revealing module pattern (although maybe not correctly). Any advice on how I can structure this code to maintain simplicity and comprehensibility as I add functionality would be appreciated. I'm open to input on whether that should be another design pattern or programming style altogether.

$(function () {

    // This object provides the stock ticker,
    // as well as the dates to compute the percentage
    // change of the stock's price between.
    var stock = {
        ticker: 'fb',
        startDate: '2015-08-06',
        endDate: '2016-04-12'
    };

    yahooFinance.getStockData(stock);
});

var utility = {};
(function () {

    var removeLeadingZero = function removeLeadingZero (strNumber) {
        if (strNumber.charAt(0) === '0') {
            return strNumber.substr(1);
        }else{
            return strNumber;
        }
    };

    var formatForConsole = function formatForConsole (dateString) {

        var dateStringArray = dateString.split('-');

        var month = removeLeadingZero(dateStringArray[1]);
        var day = removeLeadingZero(dateStringArray[2]);
        var year = dateStringArray[0];

        return month + '/' + day + '/' + 'year';
    };

    var getPercentChange = function getPercentChange (startingPrice, endingPrice) {
        return ((endingPrice - startingPrice) / startingPrice) * 100;
    };

    $.extend(utility, {
        formatForConsole: formatForConsole,
        getPercentChange: getPercentChange
    });
}());

var yahooFinance = {};
(function () {

    // Returns the URI to request daily price data for the stock in JSON format.
    var getUri = function getUri (stockObj) {

        return 'http://query.yahooapis.com/v1/public/yql?q=select%20*%20from%20yahoo'
            + '.finance.historicaldata%20where%20symbol%20%3D%20%22'
            + stockObj.ticker
            + '%22%20and%20startDate%20%3D%20%22'
            + stockObj.startDate
            + '%22%20and%20endDate%20%3D%20%22'
            + stockObj.endDate
            + '%22&format=json&env=store%3A%2F%2Fdatatables.org%2Falltableswithkeys';
    };

    var getStockData = function getStockData (stockObj) {

        $.getJSON(getUri(stockObj), function (priceData) {

            var startDate = utility.formatForConsole(stockObj.startDate);
            var endDate = utility.formatForConsole(stockObj.endDate);
            console.log(startDate + ' to ' + endDate);

            var pricesArray = priceData.query.results.quote;

            var startingPrice = pricesArray[pricesArray.length - 1].Adj_Close;
            var endingPrice = pricesArray[0].Adj_Close;
            var percentChange = utility.getPercentChange(startingPrice, endingPrice);
            console.log(percentChange);
        });
    };

    $.extend(yahooFinance, {
        getStockData: getStockData
    });
}());

Whenever I see a module named "utility", it always spells danger. It will almost always suffer bloat because one is easily tempted to put everything that's uncategorized into it and will be forgotten. Consider splitting it off while still early. It appears one is a string formatter, the other a console helper. Split them by that instead, much more meaningful.

Also, function names and variables occupy the same namespace. Defining a variable and assigning a named function is overkill. I prefer a just a function declaration over a function expression due to debugability because the names will appear in the stack trace.

var removeLeadingZero = function removeLeadingZero (strNumber) {
    if (strNumber.charAt(0) === '0') {
        return strNumber.substr(1);
    }else{
        return strNumber;
    }
};

Consider trimming the string prior to checking if the leading character is 0. A leading space could throw this function off. Additionally, this can be shortened to:

function removeLeadingZero(string){
  return string.charAt(0) === '0' ? string.trim().substr(1)
                                  : string;
}

return ((endingPrice - startingPrice) / startingPrice) * 100;

Beware of JavaScript's precision when it comes to numbers. While this works for small applications or where precision isn't really important, it may throw you off when dealing with more complex calculations. You have been warned. 0.1 + 0.2 !== 0.3 in JS :D

var getUri = function getUri (stockObj) {

    return 'http://query.yahooapis.com/v1/public/yql?q=select%20*%20from%20yahoo'
        + '.finance.historicaldata%20where%20symbol%20%3D%20%22'
        + stockObj.ticker
        + '%22%20and%20startDate%20%3D%20%22'
        + stockObj.startDate
        + '%22%20and%20endDate%20%3D%20%22'
        + stockObj.endDate
        + '%22&format=json&env=store%3A%2F%2Fdatatables.org%2Falltableswithkeys';
};

Since you said you used jQuery, consider using $.param to build the query portion of your url. It will do the proper escaping for you.

getStockData doesn't look complete, but if you want to be functional and all that, the function must return something. $.getJSON returns a Promise-like object which you can listen to for resolution/rejection. You can also parse and return in a resolve handler to do additional logic before handing it off to the caller. You can use this to restructure the data, do additional computations etc.

var getStockData = function getStockData (stockObj) {

    return $.getJSON(getUri(stockObj)).then(function (priceData) {

        var startDate = utility.formatForConsole(stockObj.startDate);
        var endDate = utility.formatForConsole(stockObj.endDate);
        var pricesArray = priceData.query.results.quote;
        var startingPrice = pricesArray[pricesArray.length - 1].Adj_Close;
        var endingPrice = pricesArray[0].Adj_Close;
        var percentChange = utility.getPercentChange(startingPrice, endingPrice);

       // Return all the needed data
       return {
         startDate: startDate,
         endDate: endDate,
         startingPrice: startingPrice,
         endingPrice: endingPrice,
         percentChange: percentChange
       };

    });
};

// Usage
yahooFinance.getStockData(stock).then(function(results){
  console.log(results.startDate);
});

A good practice is to push off anything UI-related nearer to the caller instead of anywhere in the API (unless the API is for rendering). Most of the time, your API will be just sending/grabbing/computing data and it's good that way. It makes it easy to test with lesser things to stub/mock.

Additionally, it's usually the caller that's nearer the UI. This is more apparent in frameworks like Angular, where API consumers reside in the controller, the last level of JS before the templates. API gets called in the controller, therefore the results should also end up in the controller.


Going from Spaghetti PHP OOP to SOLID OOP I am beyond stuck in a half way point between PHP structural and OOP, but I can never achieve proper SOLID patterns despite reading DI, IOC, and reading about the interfaces.

Rather than to read logical examples (i.e. a storage class that takes a book class and you typecast an interface etc...).  I want to know how I can take my brain off thinking I got SOLID down but I actually didn't. There is talk about trying to refactor a small chunk but then that small chunk isn't SOLID enough.

Here are the code segments - my thought and goals are after:

class DataID
{
    protected $_id_array;
    protected $_storage;

    public function __construct(StorageInterface $db_conn, $tables = array())
    {
       $this->_storage = $db_conn;
        foreach ($tables as $table) {
            $sql = "SELECT `id` as `id`, `name` as `value` FROM $table ORDER BY `value` ASC";
            $result = $this->_storage->read($sql);
            if (!empty($result)) {
                foreach ($result as $row) {
                    $this->_id_array[$table][$row['value']] = $row['id'];
                }
            }
        }
    }
    public function getID($table, $value)
    {
        if (!empty($this->_id_array[$table])) {
            if (isset($this->_id_array[$value])) {
                return $this->_id_array[$table][$value];
            }
        }

        return null;
    }
}

Goal:

I have tables that has ID vs Value data. I'm going to use this class to run through data (value) and I want to retrieve the ID of it. Best way to avoid too many DB request is to put it inside an array of [value] = ID I believe and then check corresponding index as value and fetch ID.

Storage is an container that goes into the DB via SQL and go fetch me all the data over. This way if the method of the storage changes (Mongo, SQL, pod, MySQLi) I can change how I treat my read.

Tables is a list of known tables in the DB that I need that Data from. That list will be from an array config file that shall be loaded in a main.

Issues:

I have a strong feeling that I can make this better but I don't know how - because to me a constructor initialize things and well I'm going into the DB to initialize my array but for some reason i feel that SOLID fails for me here.
If I have tables that have too many values (i.e. let's say if a table look up is more than 50) I want to hit the DB and not store it inside an array.

If that's the case, I'll have to make queries inside getID to use storage to go fetch it.  How do I maintain SOLID while adding that extra logic?

Reading through this code and your goals, it seems this class has two responsibilities:

Provide a Key-Value store
Get data from the database (potentially with caching)

To make it more SOLID, you can pick one of these responsibilities for this class and put the other one somewhere else.

If you wanted to use this class to provide a KV store, you would inject an array pre-filled with all of the data into the constructor rather than injecting DB information. You could then use a factory that takes those parameters to build the DataID object with the data. This removes a dependency on anything database related, simplifies your constructor, and reduces the class to a thin veneer over the underlying array. It cannot, however, get new information by itself, nor can it provide database caching. You could create an adapter class around the database result set and inject that instead of the array. The result set adapter could then handle getting additional information and caching, but this might end up replicating the concerns you have here, just somewhere else.

If you wanted to use this class as an adapter over the StorageInterface class, you would not do all the work in the constructor and simply make calls to the underlying storage object in the getID method. This allows you to simplify the constructor once again, but does not in itself provide caching. You said the StorageInterface implementation provides query-level caching, but you can create a separate adapter wrapper around it that encapsulates your SQL and caches a whole table whenever one element is retrieved from it, and can limit those caches based on table size. Once again you can use a factory method so that your external application does not need to know about this internal adapter. This option seems to fit most closely with what you want to achieve while still having better SOLID-ness.

EDIT:

Here's how this achieves better adherence to the SOLID principles:

S: This class's responsibilities have been reduced from two to one.
O: I can add additional functionality by adding decorators and adapters around the various objects to add additional functionality, without having to change those objects directly.
L: StorageInterface uses Liskov Substitution, but an adapted class around the StorageInterface can also follow Liskov Substitution by adding decorators around it to add functionality.
I: The adapters are small, client-focused APIs.
D: StorageInterface is an abstraction, and your StorageInterface adapters/decorators can also be build as abstractions.

Bonus Points:

By removing the work out of the constructor, you are adhering more closely to the Law of Demeter. Rather than asking for something to get something through it, you are asking for what you really want.


Extract strings from nested array in Perl Extract the strings from nested arrays in Perl.

It prints: a, b, c, d, E

use strict;
use warnings;
use feature qw(say signatures current_sub);
no warnings qw(experimental::signatures);

my $nested = [1, 'a', [2, 3, 'b'], [4, [5, 6, 7, ['c']], [7, [8, 9, [10, 'd']]], 11, 'E']];

print join ', ', @{ extract_strings($nested) };

sub extract_strings($input) {
    my @output = ();

    my $loop = sub ($val) {
        if (ref $val eq 'ARRAY') {
            __SUB__->($_) for @{ $val };
        } else {
            push(@output, $val) if $val =~ /^[a-zA-Z]/;
        }
    };

    $loop->($input);

    return \@output;
}

Any idea for improvement without not core dependencies?

Your concept of "string" seems incomplete.  I would look to firm up that definition and precisely match the need.  Is "" a string?  Do you want "valid identifiers" (/^[^\d\W]\w+$/) or "plausible ASCII words"  (/^[A-Za-z]+$/) or just "not numbers"  (!/ ^ ( [+-]? \d* \.? \d+ (?:[Ee][+-]?\d+)? ) $/x)?

I like use warnings FATAL => 'all'; so that I don't miss a warning in the midst of other output.

A fat arrow between arguments of different purpose can enhance readability.

Coding in functional style (with grep and map instead of @output) is a natural fit to this kind of problem.

Plain old recursion is suited to the task and would obviate the need for experimental features.

Having extract_strings take a ref (as it does now) and return a list simplifies the logic even further.

print join ', ' => extract_strings($nested);

sub extract_strings {
    grep /^[a-zA-Z]/ => map { ref eq 'ARRAY' ? extract_strings($_) : $_ } @{ $_[0] }
}

If returning a ref is necessary, wrap and unwrap accordingly:

sub extract_strings {
    [ grep /^[a-zA-Z]/ => map { ref eq 'ARRAY' ? @{ extract_strings($_) } : $_ } @{ $_[0] } ]
}


Removing points from slow riders I have a CSV like the following.
Category,Position,Name,Time,Team,avg_power,20minWKG,Male?,20minpower
A,1,Tom Smith ,00:41:58.95,7605,295,4.4,1,299.2
A,2,James Johnson,00:41:58.99,2740,281,4.5,1,283.95
A,3,Tom Lamb,00:41:59.25,1634,311,4.2,1,315
B,1,Elliot Farmer,00:45:06.23,7562,306,3.9,1,312
B,2,Matt Jones,00:45:10.10,4400,292,4.0,1,300
B,3,Patrick James,00:45:39.83,6508,299,4.1,1,311.6

I get the time of the rider in first place of category 'A'.
I then give any rider that is 15% slower than this 0 points in a new CSV.
For example if the first rider take 1 minute 40 seconds, then anyone slower than 1 minute 55 seconds will get 0 points.
def convert(seconds):  # function to convert amount of seconds to a time format
    seconds = seconds % (24 * 3600)
    hour = seconds // 3600
    seconds %= 3600
    minutes = seconds // 60
    seconds %= 60
    return "%d:%02d:%02d" % (hour, minutes, seconds)

with open("results.csv", 'rt', encoding='UTF-8', errors='ignore') as file:  # opening the full results file
    reader = csv.reader(file, skipinitialspace=True, escapechar='\\')  # skipping headers
    MaleCategoryList = []  # setting category as blank so a change is recognised
    for row in reader:
             if row[0] not in MaleCategoryList:
                    if row[0] == "A":
                        firstPlaceTime = datetime.strptime(row[3], "%H:%M:%S.%f")
                        timeInSecs = firstPlaceTime.second + firstPlaceTime.minute * 60 + firstPlaceTime.hour * 3600
                        timeDifference = timeInSecs * 1.15
                        MaxTime = datetime.strptime(convert(timeDifference), "%H:%M:%S")
# some code here which is not relevant i.e calculate points
             if cat == "A" and datetime.strptime(row[3], "%H:%M:%S.%f") > MaxTime:
                    points = int(0)
                    position_for_file = "DQ Time-Cut"
                    cat = "Time Cut"
             data = {'Position': position_for_file, 'Category': cat, 'Name': name, 'Club': club,
                        'Points': points, 'Time': time}  # dictionary of data to write to CSV

I feel that my code is very messy and inefficient. This is as there are lots of if in my loop and I believe a lot of the calculations seem unnecessary. Do you have any ideas of how I could improve my code?
I would prefer to not use Pandas if possible. This is as I have never used. However if there is a fast and easily maintainable solution to this with Pandas then I would be interested.

Time calculation
timeInSecs = firstPlaceTime.second + firstPlaceTime.minute * 60 + firstPlaceTime.hour * 3600

The first thing to do is attempt to get this as a timespan and avoid your own time math. In other words,
from datetime import datetime, time, timedelta

MIDNIGHT = time()

# ...

first_place_time = datetime.strptime(row[3], "%H:%M:%S.%f")
time_span = first_place_time - datetime.combine(first_place_time, MIDNIGHT)
time_difference = time_span.total_seconds() * 1.15

Unpacking
You can unpack your row and avoid row[0], etc. fixed indexing, like:
category, position, name, race_time = row[:4]

Call reuse
This is written twice:
datetime.strptime(row[3], "%H:%M:%S.%f")

so store it to a temporary variable.
PEP8
MaleCategoryList should be male_category_list, and MaxTime should be max_time.
Convert
Given the above code, you can get rid of most of convert. You should not put the time through another round-trip using a string.


Try/catch block inside a method that returns String Is this try/catch block written correctly?

public String execute() throws Exception{
    try{
        //Do blah
        return "success"; //Assuming everything goes well, return success.
    }catch (Exception e){
        e.printStackTrace();
    }
    return "error";
}

Should the return "error" be inside the catch clause?

If you can process exception inside of your method, process it, otherwise throw it, to let the caller process it or throw further.

Printing stack trace is not exception processing, it is more about logging. And there it is not necessary to return something. Below, there's my approach to handle errors. Please note that it is encouraged to throw more specific Exception class instances rather than Exception or Throwable. For instance: IOException, etc.

public void execute() throws IOException, MyCustomException {
    // Do something
    // if there's an error you can't process, throw a new or this exception
    // otherwise just return nothing.
}


Thread-safe arraylist with get, put, add, remove functions I am preparing for multithreading interview questions and I don't have much experience with multithreading. I referred some online article to come up with a solution. Is this solution appropriate?

public class ThreadSafeArrayList<E> {
    private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
    private final Lock readLock = readWriteLock.readLock();
    private final Lock writeLock = readWriteLock.writeLock();
    private final List<E> list = new ArrayList<>();
    public void set(E o) {
        writeLock.lock();
        try {
            list.add(o);
        } finally {
            writeLock.unlock();
        }
    }

    public E get(int i) {
        readLock.lock();
        try {
            return list.get(i);
        } finally {
            readLock.unlock();
        }
    }

    public void add(E num) {
        List<E> sync = Collections.synchronizedList(list);
        sync.add(num);
    }

    public E remove(int i) {
        List<E> sync = Collections.synchronizedList(list);
        if (sync.isEmpty()) return null;
        return sync.remove(i);
    }
}

Definitely not!

While using the ReadWriteLock pattern instead of simple intrinsic locking for collections is a good thing to do, you are doing almost everything wrong.
I recommend you to take a deeper look at Synchronizers in Java and also the Decorator Pattern. Following points are done wrong in your snippet:

You are having a ReadWriteLock attribute but don't use it.
The synchronized Collection Wrapper is used in some methods but
doing that will not create any kind of Thread-Safety since the Mutex aquired is chaning every time the method is invoked.
You are having two other locks where as one should be the read and the
other one the write lock. But using those to guard code sections in useless,
since they both have no connection to each other and you don't even lock
every critical section.

Important for you to understand is, that when using locking every access must be serialized / guarded or your locking is worthless. Before you are having an important interview you may take a look at the Book Concurrency In Practice which gives a very good introduction to Java Concurrency.


Matching whitespace and non-whitespace characters with RegEx I just started watching some regex tutorials on Plural Sight just to learn how to write a proper regex expression. So far so good, it's been alright and I decided to test my knowledge on Hackerrank. Although, the tutorial explains how to use PCRE (Perl Compatible Regular Expressions) and ECMAScript engines but I'm comfortable using PCRE at the moment so hence my code is running on PHP.
A brief overview of the question is:

Task
You have a test string S. Your task is to match the pattern XXxXXxXX
Here, x denotes whitespace characters, and X denotes non-white space characters.
Note
This is a regex only challenge. You are not required to write code.
You have to fill the regex pattern in the blank (_________).

$handle = fopen ("php://stdin","r");
$Test_String = fgets($handle);
if(preg_match($Regex_Pattern, $Test_String, $output_array)){
    print ("true");
} else {
    print ("false");
}

fclose($handle);
?>

I don't really know much about PHP, all I had to do was fill in the blank so I wrote 2 test cases to see how my regex was doing so far in regex 101 e.g AA AA AA, BB BB BC.

$Regex_Pattern = "/\S{2}\s{1}\S{2}\s{1}\S{2}/"; //Do not delete '/'. Replace __________ with your regex.

Final thoughts: I'm fully aware the example isn't meant to be complicated as it's meant to show how to use \S and \s. I attempted using [\S\s]+\S{2}$ but that wasn't greedy enough as it permits multiple spacing.
Are there more refined ways to do this without using \S & \s and if S &\s can be used differently to achieve the same results?

{1} is redundant, it's better to remove it.
When {2} follows a specific character,
it's is longer to type than the character itself twice,
so I would simply write the character twice.

/\S\S\s\S\S\s\S\S/

As Mike Brant pointed out, some duplication could be reduced by grouping:

/(\S\S\s){2}\S\S/

But this is not as clear as the first example, so I would stick with that.


Efficiently compare 2 lists to replace and order I have 2 list containing some Objects (Fruit class). I am using a 3rd list to add these elements based on following 2 criteria.

I want every object in the 1st list added to 3rd list. But if I have a matching object in the 2nd list (matching based on id and isChecked), I want to add the object from the 2nd list to the 3rd list and ignore the one in 1st list.
If I did the switch mentioned on point one, I want to move that object up to the first element of the 3rd list.

I have it working with following code. But I find it very inefficient. Is there a better way around it?

Bear in mind I have no control over the second list, but the first list is coming from a Rest endpoint and I am currently capturing it as a list. Unsure if I should have opted for a Map. Please advice.

Example:

In the following example, expected list output is [f2, f5, f1, f3, f4] (based on name).

It is cos I have all the elements from the first list. f2 and f5 went in front of the order as they came from second list (they matched elements in first list and had isChecked set to true).

import lombok.*;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class App {
    public static void main(String[] args) {

        Fruit fruit1 = new Fruit("1", "f1", false);
        Fruit fruit2 = new Fruit("2", "f2", false);
        Fruit fruit3 = new Fruit("3", "f3", false);
        Fruit fruit4 = new Fruit("4", "f4", false);
        Fruit fruit5 = new Fruit("5", "f5", false);

        List<Fruit> firstList = Arrays.asList(fruit1, fruit2, fruit3, fruit4, fruit5);

        Fruit fruit6 = new Fruit("2", "f2", true);
        Fruit fruit7 = new Fruit("7", "f7", false);
        Fruit fruit8 = new Fruit("5", "f5", true);
        Fruit fruit9 = new Fruit("9", "f9", false);
        Fruit fruit10 = new Fruit("10", "f10", false);

        List<Fruit> secondList = Arrays.asList(fruit6, fruit7, fruit8, fruit9, fruit10);

        List<Fruit> finalList = new ArrayList<>();

        // expected list = [f2, f5, f1, f3, f4]

        // this loop is checking and adding objects to finalList.
        // must match the first list and isChecked.
        // in this case, only f6 and f8 matches the first list (id match) and is also 'checked'.
        for (Fruit first : firstList){
            for (Fruit second : secondList){
                if(first.getId().equals(second.getId()) && second.isChecked()){
                    finalList.add(second);
                    break;
                }
            }
        }

        // not done yet. Still need to loop and add back the elements from the first list
        // which were not added in the above loop
        boolean addedFirst = false;
        outer:
        for(Fruit first : firstList){
            for(Fruit finalFruit : finalList){
                if(first.getId().equals(finalFruit.getId())){
                   continue outer;
                }
            }
            finalList.add(first);
        }

        for(Fruit fruit : finalList){
            System.out.println(fruit);
        }
    }
}

@Getter
@Setter
@ToString
class Fruit{
    private String id;
    private String name;
    private boolean isChecked;

    Fruit(String id, String name, boolean isChecked) {
        this.id = id;
        this.name = name;
        this.isChecked = isChecked;
    }
}

It sounds like you want the fruits unique to the first list to go at the end of the final list, and the fruits in both the first and second list go at the beginning of the final list.

For a problem like this, it might be better to use a LinkedList instead of an ArrayList, at least for the final list. Usually you're better off with an ArrayList, but in this case you can benefit from inserting an element at the front of the list is an \$O(1)\$ operation instead of \$O(n)\$.

You're on the right track with using a Map to speed this up with large data sets. Here is a potentially more efficient implementation:

Loop over the first list, filling a map firstMap with key: firstItem value: false
Loop over the second list, and for each item, insert the result of
firstMap.containsKey(secondItem) into firstMap with a key of secondItem
Finally, loop over the first array, if firstMap.get(firstItem) is true, insert the item at the front of the final list (it was in both lists). Else add it to the end of the final list (it was only in the first list).