In-Place Insertion Sort Purpose

Implementation for in-place Insertion Sort.

Discussion

The idea is to start with the second element in the input array (in the single element case, it's already sorted).

Moving backwards, compare the value against each prior element in the array until

You reach the beginning of the array
You find an element that is <= your value

As you continue backwards, move the last value you considered to the next index in the array.

Then repeat the same steps for the third, fourth, etc. elements, if they exist.

Implementation

public class InsertionSorter {
    public static void sort(int[] values) {
        for (int index = 1; index < values.length; index++) {
            int value = values[index];
            int nextIndex = index - 1;
            while (nextIndex >= 0 && values[nextIndex] > value) {
                values[nextIndex + 1] = values[nextIndex];
                nextIndex--;
            }
            values[nextIndex + 1] = value;
        }
    }
}

Naming index and nextIndex as i and j would have been OK.

There are no comments, which is not necessarily a problem. But a one-line javadoc for the function would be helpful. I know, I know, from the signature it's kind of obvious, but still. For example, what if caller passes in null? Well, we throw a fatal exception, and that seems appropriate, caller gets what he deserves, he violated the contract spelled out in the javadoc. But only if there is a javadoc explaining the caller's responsibility.

The inner while loop would be more naturally expressed as a for loop.

The inner while loop might become a helper function. Then you'd have the opportunity to give it an informative name, perhaps copy.

That said, overall the function as written is reasonably clear.


SemaphoreSlim extension method for safely handling cancellation and disposal I have often found myself using a try {semaphore.Wait()} finally {semaphore.Release()} pattern when using a semaphore, so decided I wanted to try and write an extension method to do this instead.

This stems from the problem where I wanted to dispose of my class containing the SemaphoreSlim instance, and was unsure how to safely and elegantly deal with the SemaphoreSlim instance as well.  I'm still not sure if this is the right approach:

public static class SemaphoreSlimExtension
{
    public static async Task RunAsync(this System.Threading.SemaphoreSlim semaphore, Func<Task> action, System.Threading.CancellationToken cancellationToken)
    {
        try
        {
            cancellationToken.ThrowIfCancellationRequested();

            try
            {
                await semaphore.WaitAsync(cancellationToken);
                await action();
            }
            finally
            {
                semaphore.Release();
            }
        }
        catch (OperationCanceledException ex)
        {
            if (cancellationToken != ex.CancellationToken || !cancellationToken.IsCancellationRequested)
            {
                throw;
            }
        }
        catch (ObjectDisposedException ex)
        {
            if (!cancellationToken.IsCancellationRequested)
            {
                throw;
            }
        }
    }
}

Example usage:

public class ClassA : IDisposable
{
    private CancellationTokenSource WorkSemaphoreCancellationTokenSource { get; } = new CancellationTokenSource();
    private SemaphoreSlim WorkSemaphore { get; } = new SemaphoreSlim(1, 1);

    public async Task DoWork()
    {
        await WorkSemaphore.RunAsync(() => Task.Delay(5000), WorkSemaphoreCancellationTokenSource.Token);
    }

    public void Dispose()
    {
        WorkSemaphoreCancellationTokenSource.Cancel();
        WorkSemaphore.Dispose();
        WorkSemaphoreCancellationTokenSource.Dispose();
    }
}

Does this seem like a good approach?  Is there a better way to safely manage the cancellation and disposal of SemaphoreSlim instances?

Review

Is there a better way to safely manage the cancellation and disposal of SemaphoreSlim instances?

You've made a pattern to safely manage completing / aborting a task that is acquiring or has acquired a lock on a semaphore, not to safely manage the cancellation and disposal of SemaphoreSlim instances.

You should only release the semaphore if you are sure you have acquired a lock. So take await semaphore.WaitAsync(cancellationToken); out the try block (as suggested in the comments).

You should also provide overloads or equivalent checks that return Task<T> and/or accept a CancellationToken as argument for the task.

In your exception handlers you might get false negatives, in that it's possible an OperationCanceledException or ObjectDisposedException was raised while condition cancellationToken.IsCancellationRequested was met, but coming from a different source than the cancellationToken. I don't know whether this is a big issue, since cancellation was requested anyway. But you should definately document in which scenarios exceptions could be propagated to the consumer.

Rather than the catch - if - throw blocks, you could use catch when (condition) blocks. For instance, catch (OperationCanceledException ex) when (cancellationToken == ex.CancellationToken). This is more compact, doesn't require you rethrow yourself and allows you to focus on silently capturing these exceptions on certain conditions. You might want to log something on such errors.

I'm not convinced this extension method should take the semaphore as source object. To me, the semaphore is more of a utility used to execute these tasks. Maybe a static Helper class would have been the better choice.


Is my unity component validation overkill? Unity scripts need access to components they will be using, my solution here guarantees that a script will have a valid reference to a component it needs, but is this solution overkill, or hard to discern intention from?
Note: null coalescence operators would be cleaner but cannot be used for unity objects or components for terrible reasons.
using System.Collections;
using System.Collections.Generic;
using UnityEngine;

[RequireComponent(typeof(Rigidbody))]  // disallows deletion of component from editor
public class CubeControll : MonoBehaviour
{
    [SerializeField, HideInInspector]
    private Rigidbody rigidbody;

    private void OnValidate()
    {
        rigidbody = GetComponent<Rigidbody>() != null ? // if has component attached
            GetComponent<Rigidbody>() :  // assign referance
            gameObject.AddComponent<Rigidbody>(); // else add new component
    }

    // start, update etc
}

This is a common situation, are there any general best practices to doing this?

Right now you are calling GetComponent<T>() twice if the first call return a value != null.
Well, I would do this in a very simple way.
private void OnValidate()
{
    rigidbody = GetComponent<Rigidbody>();
    if (rigidbody != null) { return; }

    rigidbody = gameObject.AddComponent<Rigidbody>();
}


A simple C WinAPI program for terminating processes via process image names - follow-up (See the previous (i.e. first) version of this tool.)
(See the next follow-up.)
After taking into consideration all the advice by Martin R, I ended up with the following tool for terminating all the processes with given process image names:
#include <stdio.h>
#include <stdlib.h>
#include <windows.h>
#include <TlHelp32.h>

int main(int argc, char* argv[]) {
    if (argc != 2) {
        fprintf(stderr, "processkiller.exe PROCESS_NAME");
        return EXIT_FAILURE;
    }

    PROCESSENTRY32 entry;
    entry.dwSize = sizeof(PROCESSENTRY32);
    HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, NULL);

    if (snapshot == INVALID_HANDLE_VALUE) {
        fputs("Error: could not get the process snapshot.", stderr);
        return EXIT_FAILURE;
    }

    size_t totalProcessesMatched = 0;
    size_t totalProcessesTerminated = 0;

    if (Process32First(snapshot, &entry)) {
        do {
            if (strcmp(entry.szExeFile, argv[1]) == 0) {
                totalProcessesMatched++;

                HANDLE hProcess = OpenProcess(PROCESS_TERMINATE,
                                              FALSE,
                                              entry.th32ProcessID);

                if (hProcess == NULL) {
                    fprintf(stderr,
                            "Error: could not open the process with ID = %d, "
                            "called \"%s\".\n",
                            entry.th32ProcessID,
                            entry.szExeFile);
                } else {
                    BOOL terminated = TerminateProcess(hProcess, 0);

                    if (terminated) {
                        totalProcessesTerminated++;

                        BOOL closed = CloseHandle(hProcess);

                        if (!closed) {
                            fprintf(stderr,
                                    "Warning: could not close a handle "
                                    "for process ID = %d, called \"%s\".\n",
                                    entry.th32ProcessID,
                                    entry.szExeFile);
                        }

                        printf("Terminated process ID %d\n", entry.th32ProcessID);
                    }
                }
            }
        } while (Process32Next(snapshot, &entry));
    }

    BOOL snapshotHandleClosed = CloseHandle(snapshot);

    if (!snapshotHandleClosed) {
        fputs("Warning: could not close the process snapshot.", stderr);
    }

    printf("Info: total matching processes: %d, total terminated: %d.\n",
           totalProcessesMatched,
           totalProcessesTerminated);

    return EXIT_SUCCESS;
}

Critique request
Am I going anywhere? Is this tool coded properly? Am I sufficiently verbose about statistics/error information?

Regarding:
size_t totalProcessesMatched = 0;
size_t totalProcessesTerminated = 0;
...
printf("Info: total matching processes: %d, total terminated: %d.\n",
       totalProcessesMatched,
       totalProcessesTerminated);

the printf() is trying to output a int, but the variables to output are size_t which should be output using a %zu format specifier.
If you compiler is not telling you about these kinds of problems, then enable the warnings until it does.
regarding;
fprintf(stderr, "processkiller.exe PROCESS_NAME");

This would be 1) left in the stderr output stream because the format string does not have a \n on the end of it.  2) any program can be executed via any name so hard coding the name is a bad idea.  Suggest:
fprintf(stderr, "%s PROCESS_NAME\n", argv[0] );

regarding:
fputs("Error: could not get the process snapshot.", stderr);

strongly suggest making use of the get_last_error() facility and printing the actual error text to stderr rather than some 'random' error message
regarding;
if (strcmp(entry.szExeFile, argv[1]) == 0) {

this does not compile because the statement:
#include <string.h>

is missing.


Armstrong numbers in a given range using Java 8 Was fiddling with java-8; trying to write a program for getting all Armstrong Numbers between 1 and 10_000_000.

Following is my working solution:

public class ArmstrongNumbers {

    public static void main(String[] args) {
        IntStream.range(1, 10_000_000)
                .filter((n) -> {
                    int c = 0, temp = n;
                    while (temp > 0) {
                        c += Math.pow(temp % 10, Integer.toString(n).length());
                        temp /= 10;
                    }
                    return c == n;
                }).forEach(System.out::println);
    }

}

Output:

1
2
3
4
5
6
7
8
9
153
370
371
407
1634
8208
9474
54748
92727
93084
548834
1741725
4210818
9800817
9926315

Why I'm here:

Can this be made shorter?
Can any other Java 8 concept be used here?
Can the use of the while loop be avoided and a stream be used somehow (given that it is more elegant)?
Review overall correctness

Here is a link to a list of Armstrong Numbers for verifying the outputs.

Please note that there is a follow up question.

Integer.toString(n).length() can be assigned to its own variable.

If you wanted to stream the while loop, I suppose you could split a toString() version of the number via toCharArray() and then calculate the value for each digit individually, then summing the result via the sum function.

I don't know whether that'd qualify as "more elegant", though. You would get rid of c and temp as variables by doing that.


Clean Calendar Object I am writing a small chunk of code (a module>?) that I will use to create a view on a page using angular. The angular directive will take care of generating the html.
The sole purpose of this module is expose a given month's data, specifically on what day it starts for the given year and how many days it has. I want to make this module as clean as possible and would appreciate anyfeedback.

function Calendar(month, year) {
var days = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'];
var months = [
    'January', 'February', 'March', 'April',
    'May', 'June', 'July', 'August', 'September',
    'October', 'November', 'December'
];
var daysInMonth = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31];

this.getMonthData = function() {
    var startingDay = new Date(this.year, this.month, 1).getDay();
    var monthLength = daysInMonth[this.month];

    if (this.month === 1) {
        if ((this.year % 4 === 0 && this.year % 100 !== 0) || this.year % 400 === 0) {
            monthLength = 29;
            }
        }
    };
}

For one, I see your code is a bit over-engineered. So let's go back to the problem: Given a year and month, determine what day that month starts in a year and how many days it has.

To start, I would need 2 functions to keep the code understandable. Trying to jam a lot of functionality into one function would only cause problems in trying to understand things.

function getFirstDayOfMonthAtYear(month, year){...}

function getLengthOfMonthAtYear(month, year){...}

You could go for a constructor to house the month and year value, and the functions be methods on the prototype. Also, your lookup arrays need to come with the constructor, not just some free-floating variable. You can attach them to the constructor, since functions are also objects.

function Calendar(year, month){
  // Default to January 1970
  this.month = month || 0;
  this.year  = year || 1970;

  // Generate the Date object so we don't need to create on every call.
  // Also note that months are zero-indexed. So if we provide the
  // real month number, we need to subtract 1.
  this.date = new Date(year, month - 1, 1);
}

Calendar.prototype.getFirstDay = function(){
  return Calendar.DAY_NAMES[this.date.getDay()];
};
Calendar.prototype.getMonthLength = function(){
  // I'll show you a trick next
};

// We use pseudo-static variables. Essentially we're just attaching
// to the constructor. That way, our lookup comes with the constructor.
Calendar.DAY_NAMES = ['Sun', 'Mon', ..., 'Sat'];

So it comes to getting how long a month is. There's a trick for that. You can construct a Date object with the current year but on the next month with date 0. JavaScript dates that underflows/overflows the months date range automatically increments/decrements the month. Then it's all a matter of doing getDate.

// So the date is March 0, 2004... essentially Feb 29, 2004
var daysOnFeb2004 = (new Date(2004, 2, 0)).getDate(); // 29

In the end:

function Calendar(year, month) {
  this.month = month || 0;
  this.year = year || 1970;
  this.date = new Date(year, month - 1, 1);
}

Calendar.prototype.getFirstDay = function() {
  return Calendar.DAY_NAMES[this.date.getDay()];
};

Calendar.prototype.getMonthLength = function() {
  return (new Date(this.year, this.month, 0)).getDate();
};

Calendar.DAY_NAMES = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'];

// Usage
var date = new Calendar(2004, 2);
document.write('First day: ' + date.getFirstDay() + ' Length:' + date.getMonthLength());


Data Structures C# Dictionary and user has a property that is a list I am about to post a question to Stack Overflow about how to do a better job adding a number to the list inside the User class. But I feel if I refactored this I might solve my problem with coupling my data like this. So I am open to suggestions.

Ideone

public static void addOrUpdate(Dictionary<int, User> dic, int key, User user)
{
    //sets a new user data and just replaces it in the dictionary
    //var used = new User { ID = "id1", Name = "Harry" };
    var used = new User{ ID = "id3", Name = "Henry" ,AddressBook = new List<ContactNumber>(){new ContactNumber(3111),new ContactNumber(4444)}};

    if (dic.TryGetValue(key, out user))
    {
        // yay, value exists!
        dic[key] = used;

    }
    else
    {
        // darn, lets add the value
        dic.Add(key, used);
    }
}

You neither need TryGetValue nor ContainsKey if you don't care about the existence of the User in the Dictionary<TKey, TValue> which seems to be the case, because you want to either add  or update the User in the dict.

You can simply use the Item property setter to achieve this and after changing the method name to AddOrUpdate (based on the NET naming guidelines) your former method will look like so

public static void AddOrUpdate(Dictionary<int, User> dic, int key, User user)
{
    //sets a new user data and just replaces it in the dictionary
    //var used = new User { ID = "id1", Name = "Harry" };
    var used = new User{ ID = "id3", Name = "Henry" ,AddressBook = new List<ContactNumber>(){new ContactNumber(3111),new ContactNumber(4444)}};

    dic[key] = used;

}

This is because internally the Dictionary.Add() method calls Insert() with true for the add parameter like so

public void Add(TKey key, TValue value) {
    Insert(key, value, true);
}

and the Item property looks like so

public TValue this[TKey key] {
    get {
        int i = FindEntry(key);
        if (i >= 0) return entries[i].value;
        ThrowHelper.ThrowKeyNotFoundException();
        return default(TValue);
    }
    set {
        Insert(key, value, false);
    }
}

where the add parameter of the Insert() method call is false.


Generic extension method that will attempt to parse a string and return it's value or default(T) So I got sick of several things about the way TryParse works. I implemented a generic ParseOrDefault function using reflection. It appears to work as expected, but I'm not fool enough to say my code is bullet proof. What's wrong with it? If something is wrong with it, can it be fixed, or should this idea be tossed?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

public class Program
{
    public static void Main()
    {

        Console.WriteLine(("true").ParseOrDefault<bool>());
        Console.WriteLine(("false").ParseOrDefault<bool>());
        Console.WriteLine(("23").ParseOrDefault<int>());
        Console.WriteLine(("52").ParseOrDefault<int>());
        Console.WriteLine(("4/22/1989").ParseOrDefault<DateTime>());
        Console.WriteLine(("4/22/2016").ParseOrDefault<DateTime>());
    }
}

public static class DataProcessAbstractionExtensions
{
    public static T ParseOrDefault<T>(this string Value)
    {
        T ReturnedValue = default(T);
        //Since C# doesn't impliment an IParsable interface to filter generically
        //Reflection must be used to determine if object is in fact parsable
        var type = typeof(T);
        var MethodInfo = type.GetMethods().FirstOrDefault(MI =>
          {
              ParameterInfo[] ArgInfo = MI.GetParameters();
              return (ArgInfo.Length == 2 && ArgInfo[0].ParameterType == typeof(string) && ArgInfo[1].IsOut && ArgInfo[1].ParameterType.GetElementType() == type);
          });
        if (MethodInfo != null && MethodInfo.IsStatic)
        {
            object[] Arguments = new object[] { Value, ReturnedValue };
            MethodInfo.Invoke(null, Arguments);
            return (T)Arguments[1];
        };
        return default(T);
    }
}

For starters, I feel inclined to go meta and question the design. One possible shortcoming of your method is that it falls back to the default value, which - for primitives - isn't null. So "blah blah" would evaluate to (int) 0 or (bool) false, and unlike TryParse, your method won't tell me if this result actually came from the input, or whether it only reflects the inability to convert it. This could lead to issues with invalid data getting swept under the rug, and as such it's sort of buggy by design (especially when handling dates).
The biggest no-no though is that you're not handling exceptions. That's quite a leap of faith when invoking a method by reflection, especially as it's only loosely identified by its signature. I wouldn't blindly trust the return type, or that the method we happened to find actually does what we hope it does.

Given that the name of the method promises to fall back to default if parsing is impossible, I would say not handling an exception breaks the principle of least surprise here.
var MethodInfo = type.GetMethods().FirstOrDefault(MI =>

How do you know the method you're looking for will always come up first on the list returned by GetMethods? If you only expect one, use SingleOrDefault. But FirstOrDefault indicates you consider the possibility that more than one method matches your given criteria. So, do we have some guarantee it would always be the first one in our way, or are we just "feeling lucky"? :) This looks pretty fragile to me.
This is subjective of course, but I have to say that since this method is actually less functional than TryParse and the implementation is rather brittle, I would personally veto this extension in a peer review, since I don't believe whatever readability improvement it brings to the table justifies the trade-offs.

I think this is a textbook example of what Jon Skeet calls evil code. It's not wrong as in "doesn't work", it's sort of clever, its "magic" can even have some appeal to it, but it's fundamentally unclear and dangerous. I highly recommend this talk: https://youtu.be/lGbQiguuUGc.
If you really want such syntactic sugar, I'd ditch generics and reflection-based approach, and replace it with hardcoded extensions for bool, DateTime, int - come on, it's not like there's dozens of use-cases anyway. Oh, and have them return a Nullable to distinguish between input actually converted to the default value, and not converted at all. Like so:

public static bool? ToBoolOrNull(this string str)
{
    bool result;
    return Boolean.TryParse(str, out result) ? result : (bool?) null;
}

And analogically for these few other types.

PS. Note that in C# lower-case names should be used for parameters and variables - so it would be this string value, methodInfo, mi etc. And I believe you don't really need ReturnedValue.


(Codewars) Linked Lists - Remove Duplicates Kata: https://www.codewars.com/kata/linked-lists-remove-duplicates/train/python

Description

  Write a RemoveDuplicates() function which takes a list sorted in increasing order and deletes any duplicate nodes from the list. Ideally, the list should only be traversed once. The head of the resulting list should be returned.

var list = 1 -> 2 -> 3 -> 3 -> 4 -> 4 -> 5 -> null
removeDuplicates(list) === 1 -> 2 -> 3 -> 4 -> 5 -> null

  If the passed in list is null/None/nil, simply return null.

  Note: Your solution is expected to work on long lists. Recursive solutions may fail due to stack size limitations.

My Solution

class Node(object):
    def __init__(self, data, nxt=None):
        self.data = data
        self.next = nxt

def remove_duplicates(head):
    node = Node(None, head)
    st = set()
    while node.next:
        if node.next.data in st:
            tmp = node.next
            node.next = node.next.next
            del tmp
        else:
            st.add(node.next.data)
            node = node.next
    return head

I am interested in performance and adhering to best practices.

The space complexity of your solution is \$O(n)\$. What if I told you that given the constraints in the question, you could solve this problem in \$O(1)\$ space?

Consider that you receive an already sorted list. This implies that all duplicates must be adjacent. This in turn means that for deduplicating these, you do not need to keep track of which values you already encountered.

Note that del is not necessary here. You never use tmp aside from assigning node.next to it in any case. del is not clearing the memory allocated to the variable (or the object it refers to), it only destroys the variable itself, the object is unaffected. As such it does not have any effect on the garbage-collection that python performs...

The following should already work just fine:

def remove_duplicates(head):
    node = Node(None, head)
    while node.next:
        if node.next.data == node.data:
            node.next = node.next.next
        else:
            node = node.next
    return head

Small sidenote: I really like the hack you used to ensure that you handle the head is None case, but you should be aware that it is just a hack. The cleaner solution to this would be:

def remove_duplicates(head):
    if not head:
        return head
    node = head
    while node.next:
        # Rest of the code...


Function that builds dictionary based on lambda params I've written a method in c# that allows me to do create a dictionary from a passed in object and N lambda expressions that reference that objects properties and methods. It's working the way I want it and seems to perform well, but I'm looking for criticism and seeking to improve the code.

Here's an example usage:

class Person
{
    public string Name { get; set; }
    public int Age { get; set; }

    public Dictionary<string, object> MakeDictionary()
    {
        return DictionaryThing.Data(this, x => x.Name, x => x.Age);

        /* example return dictionary:

          Name: "Ronnie"
          Age: 29

        */
    }
}

And here's the code for DictionaryThing:

static class DictionaryThing
{

    static ConcurrentDictionary<string, Delegate> expCache = new ConcurrentDictionary<string, Delegate>();

    public static Dictionary<string, object> Data<T>(T obj, params Expression<Func<T, dynamic>>[] expressions)
    {
        var dict = new Dictionary<string, object>();

        foreach (var exp in expressions)
        {
            string name = null;
            var body = exp.Body;

            var unaryExp = body as UnaryExpression;
            if (unaryExp != null)
                body = unaryExp.Operand;

            var memberExp = body as MemberExpression;
            if (memberExp != null)
                name = memberExp.Member.Name;

            var methodCallExp = body as MethodCallExpression;
            if (methodCallExp != null)
                name = methodCallExp.Method.Name;

            if (name == null)
                throw new InvalidExpressionException(
                    string.Format("The expression '{0}' is invalid. You must supply an expression that references a property or a function of the type '{1}'.",
                        exp.Body, typeof(T)));

            var key = typeof(T).FullName + "." + name;
            var func = (Func<T, dynamic>)expCache.GetOrAdd(key, k => ((LambdaExpression)exp).Compile());

            dict[name] = func(obj);
        }

        return dict;
    }

}

My concerns are:

Is there a better way to work with the expressions that are passed in.
Can the expressions parameter be better constrained by a more appropriate type?
Is there a better way to derive the cache key?

Ronnie,
You have a good starting point for the problem you are trying to solve.
To answer your concerns:

Is there a better way to work with the expressions that are passed in.
I think what you have done is fine. You are using Expression to let user specify methods/properties in type safe way. The only thing I would change is to use Expression<Func<T, object>>[] instead of Expression<Func<T, dynamic>>[] as dynamic will invoke the compiler at run time.

Can the expressions parameter be better constrained by a more appropriate type?
I don't think you can specify additional constraints at compile time but you can definitely add code to validate the expression at runtime.

Is there a better way to derive the cache key?
I think your cache key is fine. If you want to be more conservative, you can include Type.AssemblyQualifiedName. I would worry about it only if I was releasing to general public and not if I am using this class as part of a team.

With that said, here's the full re-factored code along with my notes. I hope it helps.
void Main()
{
    var ronnie = new Person();
    ronnie.Name = "Ronnie";
    ronnie.Address = "Greensboro, NC";

    var dictionary = ObjectToDictionaryConverter.GetDictionary(ronnie, x => x.Name, x => x.Address);

    // In following example, we are trying to pass in an expression which is not valid for our object.
    // Running this code will throw an appropriate exception like
    // Expression value(UserQuery+<>c__DisplayClass0).anotherDictionary.Keys is invalid.
    // Expression Property/Member Type System.Collections.Generic.Dictionary`2[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]
    //, expecting Type: UserQuery+Person

    //var anotherDictionary = new Dictionary<string, string>();
    //var dictionary = ObjectToDictionaryConverter.GetDictionary(ronnie, x => anotherDictionary.Keys);

    //I don't think you can enforce this constraint at compile time (I am not sure about it, I would love to be proved wrong.)

    Console.WriteLine(dictionary["Name"]); //Prints Ronnie
    Console.WriteLine(dictionary["Address"]); //Prints Greensboro, NC

}

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

    public string Address { get; set; }
}

// Why would you name a class like 'DictionaryThing'? It doesn't explain anything about what the class does.
// Let's give it a more descriptive 'ObjectToDictionaryConverter' name. You may prefer some other name but
// make sure that you put same thought in to naming your class as the code which goes inside the class.

// Next, the original code was doing two things
// 1. Inspecting the expression to derive a key
// 2. Create the dictionary from the input object (while also catching the expression details)

// Following SRP, let's separate it out in two classes so that each class is responsible for doing one thing.
// This also means that you can use the Expression inspection code somewhere else if you choose to.
public static class ObjectToDictionaryConverter
{
    //Always spell out the variable name in its full. (expCache -> expressionCache)
    static ConcurrentDictionary<string, Delegate> expressionCache = new ConcurrentDictionary<string, Delegate>();

    public static Dictionary<string, object> GetDictionary<T>(T obj, params Expression<Func<T, object>>[] expressions)
    {
        //Always validate your arguments.
        if(ReferenceEquals(obj, null))
            throw new ArgumentNullException("obj");

        if(expressions.Length == 0)
            throw new ArgumentException("You must specify at least one expression.");

        foreach (var expression in expressions)
        {
            if(expression == null)
                throw new ArgumentException("You can not specify NULL expression.");
        }

        var result = new Dictionary<string, object>();

        foreach (var expression in expressions)
        {
            //The purpose of ExpressionDetail is to inspect our expression
            var expressionDetail = ExpressionDetail.Create(expression);

            //A lambda expression can be a valid expression referring to a property or function.
            //But for our need, we will need to compile this expression to a delegate and run on object of Type T, let's make
            //sure that expression refers to the correct type.

            //IMPORTANT: We should not put this check in ExpressionDetail class. It is not
            //the responsibility of ExpressionDetail to enforce this type constraint.
            //This type constraint is only needed for ObjectToDictionaryConverter class.
            if(expressionDetail.DeclaringType != typeof(T))
                throw new InvalidExpressionException("Expression " + expression.Body + " is invalid. Expression Property/Member Type " + expressionDetail.DeclaringType.FullName + ", expecting Type: " + typeof(T).FullName);

            //expressionDetail has properties like 'Name', 'FullName', 'Delegate'
            var func = (Func<T, object>) expressionCache.GetOrAdd(expressionDetail.FullName, expressionDetail.Delegate);

            result[expressionDetail.Name] = func(obj);
        }

        return result;
    }
}

public class ExpressionDetail
{
    private ExpressionDetail()
    {

    }

    public MemberInfo MemberInfo { get; private set; }

    public LambdaExpression Expression { get; private set; }

    //By figuring out MemberInfo from the Expression,
    //we can now have all these read-only properties to get expression detail.
    public string Name { get { return MemberInfo.Name; } }

    public Type DeclaringType { get { return MemberInfo.DeclaringType; } }

    public string FullName { get { return DeclaringType.FullName + "." + Name; } }

    //Depending on performance requirement, you may want to use Lazy<T> to calculate this value
    //only once.
    public Delegate Delegate { get { return Expression.Compile(); } }

    //We are expecting a lambda expression which should either point to a method or a property access.
    //To get body, we have to handle the case of expression being UnaryExpression
    //To learn more: http://stackoverflow.com/questions/3567857/why-are-some-object-properties-unaryexpression-and-others-memberexpression
    private static Expression GetBody(LambdaExpression expression)
    {
        //We don't validate arguments here only because it's a private method.

        var unaryExpression = expression.Body as UnaryExpression;
        return unaryExpression != null ? unaryExpression.Operand : expression.Body;
    }

    //In your original method, you returned the name.
    //However, it could be even more useful to get the MemberInfo and store it.
    //Now we will have access to Name as well as the Type in which the property/method is declared.

    // There are lots of edge cases here.
    // Refer to: http://stackoverflow.com/questions/671968/retrieving-property-name-from-lambda-expression and update this method to handle the edge cases that you care about.
    private static MemberInfo GetMemberInfo(Expression expression)
    {
        //We don't validate arguments here only because it's a private method.

        var memberExpression = expression as MemberExpression;

        if(memberExpression != null)
            return memberExpression.Member;

        var methodCallExpression = expression as MethodCallExpression;

        if(methodCallExpression != null)
            return methodCallExpression.Method;

        return null;
    }

    public static ExpressionDetail Create(LambdaExpression expression)
    {
        if(expression == null)
            throw new ArgumentNullException("expression");

        var body = GetBody(expression);

        var memberInfo = GetMemberInfo(body);

        if (memberInfo == null)
                throw new InvalidExpressionException(
                    string.Format("The expression '{0}' is invalid. You must supply an expression that references a property or a function.",
                        expression.Body));

        return new ExpressionDetail { MemberInfo = memberInfo, Expression = expression};
    }

}


Code to ask for user string, then display the output # printstring.py
# 7/8/2016

import sys

# ask for user input
s_string = input('Enter a string: ')

# if no string, then exit()
if s_string == '':
    sys.exit()

# display the output
print(s_string)

Also wondering how to get "raw" input and output the text properly formatted with line breaks, etc. Also looking for general feedback and tips for improvement.

More elegant way to check if a string is empty
Read here

if not s_string:
    sys.exit()

You don't need a sys.exit(), just let the program exit on its own.

if s_string:
    print(s_string)

# the program will exit here anyway.

sys.exit() has some side-effects like killing the python interpreter, which might have some undesirable effects. Read here

You don't even need to check if the string is empty

print(s_string) # If string is empty, will print nothing except for a newline.

Name s_string just s or string.

It doesn't detract from the readability of the code, and makes it more pretty and easier to type(and read)

Most people tend to prefer double quotes " for string literals Read Here


.Net BouncyCastle - CSR and Private Key Generation The purpose of this code is to generate the CSR and the Private Key.

The scenario is that we have some clients for whom we run websites and for the SSL component we want to have a page on our site where they can input the fields like domain name, company name etc and then the system will generate the CSR (and return it to them on the screen so they can get it signed by a third party, e.g. geotrust) and the private key will automatically be put into the correct spot on the web server ready for use.

The 'main' part of this is below.

        /// <summary>
        ///     Generates certificate request in PKCS#10 format defined by RFC 2986.
        ///     This will also output the private key at the same time.
        ///     *******************************************
        ///     Notes / Handy references:
        ///     http://www.keylength.com/en/compare/
        ///     http://csrc.nist.gov/publications/nistpubs/800-57/sp800-57_part1_rev3_general.pdf
        /// </summary>
        private void GeneratePkcs10
            (string domainName, string companyName, string division, string city, string state,
             string countryIso2Characters, string email, RootLenght rootLength, out string csr, out string privateKey)
        {
            csr = null;
            privateKey = null;

            try
            {
                var rsaKeyPairGenerator = new RsaKeyPairGenerator();

                // Note: the numbers {3, 5, 17, 257 or 65537} as Fermat primes.
                // NIST doesn't allow a public exponent smaller than 65537, since smaller exponents are a problem if they aren't properly padded.
                // Note: the default in openssl is '65537', i.e. 0x10001.
                var genParam = new RsaKeyGenerationParameters
                    (BigInteger.ValueOf(0x10001), new SecureRandom(), (int) rootLength, 128);

                rsaKeyPairGenerator.Init(genParam);

                AsymmetricCipherKeyPair pair = rsaKeyPairGenerator.GenerateKeyPair();

                IDictionary attrs = new Hashtable();

                attrs.Add(X509Name.CN, domainName);
                attrs.Add(X509Name.O, companyName);
                attrs.Add(X509Name.L, city);
                attrs.Add(X509Name.ST, state);
                attrs.Add(X509Name.C, countryIso2Characters);

                if (division != null)
                {
                    attrs.Add(X509Name.OU, division);
                }

                if (email != null)
                {
                    attrs.Add(X509Name.EmailAddress, email);
                }

                var subject = new X509Name(new ArrayList(attrs.Keys), attrs);

                var pkcs10CertificationRequest = new Pkcs10CertificationRequest
                    (PkcsObjectIdentifiers.Sha256WithRsaEncryption.Id, subject, pair.Public, null, pair.Private);

                csr = Convert.ToBase64String(pkcs10CertificationRequest.GetEncoded());

                var pkInfo = PrivateKeyInfoFactory.CreatePrivateKeyInfo(pair.Private);
                privateKey = Convert.ToBase64String(pkInfo.GetDerEncoded());
            }
            catch (Exception ex)
            {
                // Note: handles errors on the page. Redirect to error page.
                ErrorHandler(ex);
            }
        }

        private enum RootLenght
        {

            RootLength2048 = 2048,

            RootLength3072 = 3072,

            RootLength4096 = 4096,

        }

Because the documentation for BouncyCastle for .net is limited, I am unsure if the above is 'correct'. It does compile, however:

Is it safe to always use BigInteger.ValueOf(0x10001)? This seems to be the default for OpenSSL so I presume this is correct. Or should this be the actual number 65537?
NIST does not have a 'certainty' recommendation for 4096 keys. Is it safe to use 128 always for the three lengths in the enum? I saw this answer but it is a bit dated. Should I be switching the value depending on the length of the key?
Am I missing anything?

ArrayList and Hashtable were superceded by List<T> and Dictionary<TKey, TValue>, respectively, in .NET 2.0.

The first parameter of the X509Name constructor, ordering, specifies the order in which the OID/string pairs will be processed. However we're using new ArrayList(attrs.Keys) for this parameter, and the documentation for Hashtable states "The order of the keys ... is unspecified", so we can't rely upon the keys being returned in the same order that we inserted them.

If the order isn't important in your case, replace ArrayList/Hashtable with List<T>/Dictionary<TKey, TValue>:

var attributes = new Dictionary<DerObjectIdentifier, string>
{
    { X509Name.CN, domainName },
    { X509Name.O, companyName },
    { X509Name.L, city },
    { X509Name.ST, state },
    { X509Name.C, countryIso2Characters }
};

if (division != null)
{
    attributes.Add(X509Name.OU, division);
}

if (email != null)
{
    attributes.Add(X509Name.EmailAddress, email);
}

var subject = new X509Name(attributes.Keys.ToList(), attributes);

If the order is important to you, then we need another solution since "The order of the keys in the Dictionary<TKey, TValue>.KeyCollection is unspecified".

The obvious solution is to use a List<KeyValuePair<DerObjectIdentifier, string>>, but this gets very verbose:

var attributes = new List<KeyValuePair<DerObjectIdentifier, string>>
{
    new KeyValuePair<DerObjectIdentifier, string>(X509Name.CN, domainName),
    ...
};

One trick I read about is to define a helper class like this

private class KeyValuePairList<TKey, TValue> : List<KeyValuePair<TKey, TValue>>
{
    public void Add(TKey key, TValue value)
    {
        this.Add(new KeyValuePair<TKey, TValue>(key, value));
    }

    public IEnumerable<TKey> Keys
    {
        get { return this.Select(item => item.Key); }
    }

    public IEnumerable<TValue> Values
    {
        get { return this.Select(item => item.Value); }
    }
}

The Add method lets us use the same initializer syntax we used for the Dictionary.

And then you can use the X509Name(IList, IList) constructor.

var attributes = new KeyValuePairList<DerObjectIdentifier, string>
{
    { X509Name.CN, domainName },
    { X509Name.O, companyName },
    { X509Name.L, city },
    { X509Name.ST, state },
    { X509Name.C, countryIso2Characters }
};

if (division != null)
{
    attributes.Add(X509Name.OU, division);
}

if (email != null)
{
    attributes.Add(X509Name.EmailAddress, email);
}

var subject = new X509Name(attributes.Keys.ToList(), attributes.Values.ToList());


Java 8 Stream to produce all permutations of an array using recursion I want to write a class that returns a Stream of permutations of an int[].

public class Permutations {
    public static Stream<int[]> of(int[] array) {
        return permute(array, array.length);
    }
    public static Stream<int[]> permute(int[] array, int n) {
        if (n == 1) {
            return Stream.of(Arrays.copyOf(array, array.length));
        } else {
            Stream tmp = Stream.empty();
            for (int i = 0; i < n; i++) {
                swap(array, i, n - 1);
                tmp = Stream.concat(permute(array, n - 1), tmp);
                swap(array, i, n - 1);
            }
            return tmp;
        }
    }
    private static void swap(int[] a, int i, int j) {
        if (i != j) {
            int tmp = a[i]; a[i] = a[j]; a[j] = tmp;
        }
    }
}

Problem with my solution is that it is nesting a lot of Streams with concat, many of them being just empty ones. Another issue is that it creates all permutations before returning the Stream, so it not really streaming...  :- )

I saw Minborg's solution, but I wanted to make something simpler, here based on  Sedgewick and Wayne's algorithm.

Can above code be improved?

Not a review, but an extended comment:

I strongly advise against recursion here. The permutations have a natural (lexicographic) ordering, and given a permutation it is easy to construct a next one.

This hints that to achieve true streaming: implement nextPermutation() method, and pass it to Stream.iterate() as an unary operator.


Using reflection in a test to check if a private variable is really null after a function, is this okay? I am using reflection to check if a private variable is set to null after the logout function. This is needed because the getUser function will always attempt to set and return an user if no user is set. Am I doing something wrong or can I do something better?

This is the bean that has the logout and getUser function:

@ManagedBean(name = "authBean")
@SessionScoped
public class AuthorizationBean implements Serializable{

    //Data access object for the users
    @Inject
    UserDao userDao;

    private User user; // The JPA entity.
    public User getUser() {
        if (user == null) {
            user = (User) FacesContext.getCurrentInstance().getExternalContext().getSessionMap().get("user");
            if (user == null) {
                Principal principal = FacesContext.getCurrentInstance().getExternalContext().getUserPrincipal();
                if (principal != null) {
                    user = userDao.findByEmail(principal.getName()); // Find User by j_username.
                }
            }
        }
        return user;
    }

    /**
     * Function that handles the logout
     * @return Redirect string that points to the login page
     */
    public String doLogout() {
        // invalidate the session, so that the session is removed
        FacesContext.getCurrentInstance().getExternalContext().invalidateSession();
        user = null;
        // return redirect to login page
        return "/login.xhtml?faces-redirect=true";
    }

}

This is the test class, with the reflection:

@RunWith(PowerMockRunner.class)
@PrepareForTest(FacesContext.class)
public class AuthorizationBeanTest {

    private AuthorizationBean authorizationBean;

    @Mock
    User user;

    @Mock
    FacesContext facesContext;

    @Mock
    ExternalContext externalContext;

    @Before
    public void setUp() {
        authorizationBean = new AuthorizationBean();
        Map<String,Object> sessionMap = new HashMap<>();
        sessionMap.put("user", user);

        //Mocking the static function getCurrentInstance from FacesContext,
        //so a mocked user can be returned for the test
        PowerMockito.mockStatic(FacesContext.class);
        PowerMockito.when(FacesContext.getCurrentInstance()).thenReturn(facesContext);
        when(facesContext.getExternalContext()).thenReturn(externalContext);
        when(externalContext.getSessionMap()).thenReturn(sessionMap);
    }

    @Test
    public void doLogoutTest() throws NoSuchFieldException, IllegalAccessException {
        assertNotNull(authorizationBean.getUser());
        assertEquals("/login.xhtml?faces-redirect=true", authorizationBean.doLogout());

        //Check through reflection if the private field user is now null
        Field userField = authorizationBean.getClass().getDeclaredField("user");
        userField.setAccessible(true);
        User testUser = (User) userField.get(authorizationBean);
        assertNull(testUser);
    }

}

Personally I wouldn't do this.

The reason why is when your field change name, your test fails because the field is hardcoded there and with refactoring this isn't persisted to the test.

What should I do?

Normally your AuthorizationBeanTest is in the same package as your AuthorizationBean.
When this is correct you could use a protected method.

Example :

protected boolean isUserNull () {
    return user==null;
}

This is completely safe in your pojo, nobody could even acces the User object.
And outside the package no one will ever see this method.

The advantage is when you refactor user or the method name, your test is automatically updated.


Type checking to perform a variation of the strategy pattern So, consider the following code:

public class OneConcreteTypeOfOutputFormat
{
    private Dictionary<Type, IEntityWriter> writers;
    public Dispatcher()
    {
        this.writers = new Dictionary<string, IEntityWriter();
        CreateWriters(writers);
    }

    private void CreateWriters(Dictionary<string, IEntityWriter> writers)
    {
        string nspace = typeof(OneConcreteWriter).Namespace;
        Assembly assembly = Assembly.GetExecutingAssembly();
        foreach (var writer in assembly.GetAllFromNamespace<IEntityWriter>(nspace))
            writers.Add(writer.EntityType.FullName, writer);
    }

    private bool HasWriterForAllPropTypes(IEnumerable<Entity> data)
    {
        return data.All(ent => writers.ContainsKey(ent.GetType().FullName));
    }

    public void WriteEntities(IEnumerable<Entity> entities)
    {
         Require.That(() => HasWriterForAllPropTypes(outputData));
         foreach(Entity entity in entities)
             writers[entity.GetType().FullName].Write(entity);
    }
}

Here, I am using the actual type of the Entity (which is a baseclass) to write it. I've heard that it's considered inelegant to use type-checking to perform operations, but I really don't want each Entity to write itself. This is a very easy and comfortable way to match something like this up. Do you find this clashing with the tenets of object-oriented programming? Sadly, each writer need to cast to it's specific type in the Write method.

UPDATE

Note that the code above is completely updated - changes include:

Added writer initialization code. It is delegated to a unit-tested extension method.
Changed the name of the class to reflect that it is only one of different types of output formats - so it would require more than just creating one writer for each Entity.
Added a little bit of the validation code.

I don't know if it will be useful for you, but here is one variant:
Add to your Entity base class a method called GetWriter. This method should return IEntityWriter.
This IEntityWriter should have method Write() without parameters.

Then an implementation of an Entity would be something like:

class XXXEntity: Entity
{
   public override IEntityWriter GetWriter()
   {
      return new XXXEntityWriter(this);
   }
}

And then the method WriteEntities in your example would be

public void WriteEntities(IEnumerable<Entity> entities)
{
   foreach(Entity entity in entities)
      entity.GetWriter().Write();
}

The 'mappings' approach (no matter if it is in the code or in the xml file) have a couple of problems:
1. It's easy to do wrong configuration of mappings. You can map a XXXEntity to YYYEntityWriter.
2. It's easy to forget to add an EntityWriter when you add new Entity or you can forget to add the mapping. And this is a real problem.

By the way. Judging from the description of the task, a concrete EntityWriter have to know about a concrete Entity.
So mapping is not as flexible as it seems, because of this explicit connection between EntityWriter and Entity.
It's not a problem to use GetType(). The problem here is to separate (hide) connections between classes that should work together.


Optimize Clock Update Code To Prevent Bottleneck Resulting In Time Lost I use the following PHP Code to output the user's local time and the servers time (office time). $local_time and $remote_time are the corresponding UNIX timestamps:

<span class='icon-time' data-time='<?php echo $local_time; ?>' id='local-time'>
Local : <?php echo date('D, M j, h:ia', $local_time); ?>

<span class='icon-time text-<?php

echo (6 > date('N') && 9 <= date('G') && date('G') <= 17) // change class if open/closed
    ? 'success'
    : 'error';

?>' data-time='<?php echo $remote_time; ?>' id='remote-time'>
Office : <?php echo date('D, M j, h:ia', $remote_time); ?>

I then have the following JavaScript to update the clocks in real-time:

// update local/office clocks
window.setInterval(function () {

    lclock = $('#local-time');
    rclock = $('#remote-time');

    // convert to UNIX timestamp, add 60 secs since setInterval() will no start onload but after 60 secs
    ltime = new Date((lclock.data('time') * 1000) + 60000);
    rtime = new Date((rclock.data('time') * 1000) + 60000);

    lhours = ltime.getHours();
    rhours = rtime.getHours();

    // make 12hr time format and left-pad with zero
    lshours = ((0 === lhours) ? 12 : (lhours - ((12 < lhours) ? 12 : 0)));
    rshours = ((0 === rhours) ? 12 : (rhours - ((12 < rhours) ? 12 : 0)));

    lmins = ltime.getMinutes();
    rmins = rtime.getMinutes();

    rday = rtime.getDay();

    // determine if office is closed and change class
    ostatus = 'icon-time ' + ((6 > rday && 0 < rday && 9 <= rhours && 17 >= rhours)
        ? 'text-success'
        : 'text-error');

    // update clock, increment timestamp by 60 secs
    lclock.text('Local : ' + days[ltime.getDay()] + ', '
    + months[ltime.getMonth()] + ' ' + ltime.getDate() + ', '
    + ((10 > lshours) ? '0' + lshours : lshours) + ':'
    + ((10 > lmins) ? '0' + lmins : lmins)
    + ((12 <= lhours) ? 'pm' : 'am')).data().time += 60;

    // update clock, change CSS class, increment timestamp by 60 secs
    rclock.text('Office : ' + days[rtime.getDay()] + ', '
    + months[rtime.getMonth()] + ' ' + rtime.getDate() + ', '
    + ((10 > rshours) ? '0' + rshours : rshours) + ':'
    + ((10 > rmins) ? '0' + rmins : rmins)
    + ((12 <= rhours) ? 'pm' : 'am')).attr('class', ostatus).data().time += 60;

}, 60000);

It works great. However, after a few hours the time can end up being off by up to -7 minutes. I tried caching as many objects as I could and optimizing it the best I could to reduce the amount of variable value assignments, and used chaining as well, but can't see how else to speed it up to make it not lose time.

How can I optimize this further, in either my method, code being used, or is there something I am doing wrong?

The thing to understand with setInterval() and other timer-based functions in general is that they do not guarantee precise timing.  You request an interval of 60000ms, and your mistake is assuming that what you get is in fact an interval of exactly 60000ms.

The main trick is to measure the actual amount of elapsed time between intervals, rather than assuming that each interval occurs exactly 60000ms after the previous one.  Do that and you should see better results.

Note that there may also be secondary effects caused by the execution time of your code itself.  To get around that, instead of adding deltas on each invocation you could track the initial starting timestamp at which your script starts running, and then just always compute the total amount of time that has elapsed between that starting timestamp and now, and add that amount to the starting timestamp when displaying the time.  That should give you very accurate output, and it solves the problem of the imprecise timer as well.

Here's a similar JavaScript timer that I coded awhile back that you can use as a reference.


Computing the correlation of an analyzed stock ticker I have an enum of stock tickers, then from the database I get details for an analysed ticker (stock). In the loop, I compare the details of the analysed ticker to details of the rest of tickers, compute the correlation, then add it to the treeSet.

This is my first time using multithreading and I wanted the method executed in a shorter time. I'm not sure about this solution. Could you review my code and judge it?

This is my service:

@Service public class StatisticService{

    @Inject
    private StockDetailsRepository stockDetailsRepository;

    @Inject
    private StockRepository stockRepository;

    private List<GpwStockTicker> tickersWithOutOneAnalysed;
    private TreeSet<StockStatistic> correlationTreeSet;
    private double [] closePricesAnalysed;
    private int size;
    private PearsonsCorrelation pearsonsCorrelation;

    public TreeSet<StockStatistic> computePearsonCorrelation(Stock stock, int size) {
        this.size = size;
        long start = System.currentTimeMillis();
        correlationTreeSet = new TreeSet<>();
        pearsonsCorrelation = new PearsonsCorrelation();
        GpwStockTicker mainTicker = stock.getTicker();
        List<StockDetails> stockDetailsAnalysed = getContent(mainTicker, size);
        closePricesAnalysed = getClosePrices(stockDetailsAnalysed);
        EnumSet<GpwStockTicker> tickers = complementOf(EnumSet.of(mainTicker));
        tickersWithOutOneAnalysed = new ArrayList<GpwStockTicker>(tickers);

        Thread threadOne = new Thread(new Runnable() {
            public void run() {
                firstPartTickers();
            }
        });

        Thread threadTwo = new Thread(new Runnable() {
            public void run() {
                secondPartTickers();
            }
        });

        threadOne.start();
        threadTwo.start();

        try {
            threadOne.join();
            threadTwo.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        long end = System.currentTimeMillis();
        System.out.println("Time of method (s): "+ (end-start) / 1000.0);
        return correlationTreeSet;
    }

    private void firstPartTickers() {
        for(int i = 0; i< tickersWithOutOneAnalysed.size()/2; i++) {
            calculateCorrelation(i);
        };
    }

    private void secondPartTickers() {
        for(int i = tickersWithOutOneAnalysed.size()/2; i< tickersWithOutOneAnalysed.size(); i++) {
            calculateCorrelation(i);
        };
    }

    private void calculateCorrelation(int indexOfTicker) {
        GpwStockTicker ticker = GpwStockTicker.valueOf(tickersWithOutOneAnalysed.get(indexOfTicker).name());
        double [] closePricesToCompare = getClosePrices(getContent(ticker, size));
        Double correlation  = pearsonsCorrelation.correlation(closePricesAnalysed, closePricesToCompare);
        StockStatistic stockCorrelation = new StockStatistic(correlation, ticker);
        correlationTreeSet.add(stockCorrelation);
    }

    private List<StockDetails> getContent(GpwStockTicker ticker, int size) {
        return stockDetailsRepository.findByStockTickerOrderByDateDesc(ticker, new PageRequest(1, size)).getContent();
    }

    private double[] getClosePrices(List<StockDetails> stockDetails) {
        double[] closePrices = new double[stockDetails.size()];

        int idx = 0;
        for(StockDetails stds: stockDetails){
            closePrices[idx++] = stds.getClosePrice().doubleValue();
        }

        return closePrices;
    }
}

The main goal was achieved: the time before multithreading was 77.277 sec and after was 43.534 sec. But I would like to know if my way of thinking was good.

private void firstPartTickers() {
        for(int i = 0; i< tickersWithOutOneAnalysed.size()/2; i++) {
            calculateCorrelation(i);
        };
    }

    private void secondPartTickers() {
        for(int i = tickersWithOutOneAnalysed.size()/2; i< tickersWithOutOneAnalysed.size(); i++) {
            calculateCorrelation(i);
        };
    }

You have two almost identical methods.  Is this really necessary?  Consider the following method:

    private void calculateTickers(int start, int increment) {
        for (int i = start; i < tickersWithOutOneAnalysed.size(); i += increment) {
            calculateCorrelation(i);
        }
    }

Note that I also removed the unnecessary ; after the for loop.

Now you can just call

                calculateTickers(0, 2);
                calculateTickers(1, 2);

instead of

                firstPartTickers();
                secondPartTickers();

And if you later wanted to do a third thread, you could say

                calculateTickers(0, 3);
                calculateTickers(1, 3);
                calculateTickers(2, 3);


Minimum cost path of matrix using Python I was reading this article about finding the minimum cost path from (0,0) to any (m,n) point in a matrix. Using python the author has provided 2 solutions in Python.

The first one solves it by using a backward induction technique through recursion while the second one uses an auxiliary table (tc). I was wondering if I could solve it using forward induction and without the need of an additional table. Here's my solution:

def test(target_matrix, cost, i, j, m, n):
    if i == m and j == n:
        return cost
    if i + 1 > m:
        cost += target_matrix[i][j + 1]
        return test(target_matrix, cost, i, j + 1, m, n)
    if j + 1 > n:
        cost += target_matrix[i + 1][j]
        return test(target_matrix, cost, i + 1, j, m, n)
    if i + 1 <= m and j + 1 <= n:
        ret_cost, i, j = min(target_matrix[i + 1][j], target_matrix[i][j + 1], target_matrix[i + 1][j + 1], i, j)
        cost += ret_cost
        return test(target_matrix, cost, i, j, m, n)

def min(x, y, z, i, j):
    if x < y:
        if x < z:
            return x, i + 1, j
        else:
            return z, i + 1, j + 1
    else:
        if y < z:
            return y, i, j + 1
        else:
            return z, i + 1, j + 1

if __name__ == '__main__':
    input = [
        [11, 9, 3],
        [3, 1, 0],
        [1, 3, 2]
    ]
    res = test(input, input[0][0], 0, 0, 2, 2)
    print(res)

What are your comments on this? What do you think are its drawbacks compared to the two solutions provided in the article? I'm particularly interested in comments regarding the time and space complexity of my algorithm.

You can do it without using any additional table. I want to do it without recursion, but you can create recursive version very easily yourself.

First, let's see what is the second table in the link solution. In that table, the value of each table cell is the minimum cost for accessing that cell. But what about the cost table? in the cost table, the value of each cell is simply the cost of that cell.

So, for turning the cost table to the minimum cost table of each cell, we need to run the minimum-path algorithm on it.

For each cell, we will choose the minimum value of {cost of upper cell + cost of the current cell, cost of top-left cell + cost of the current cell, cost of left cell + cost of current cell}. Don't forget that we only allowed moving to the right, down and right-down, and because now we are moving in the opposite direction in each row, we should consider opposite movements.

Talking is enough, let's code speaks a little:

def test(cost, m, n):
    # Initializing the first column
    for i in range(1, m + 1):
        cost[i][0] += cost[i - 1][0]
    # Initializing the first row
    for j in range(1, n + 1):
        cost[0][j] += cost[0][j - 1]
    for j in range(1, n + 1):
        for i in range(1, m + 1):
            cost[i][j] += min(cost[i - 1][j - 1], cost[i][j - 1], cost[i - 1][j])
    return cost

Two first steps are like the solution in the link. In the first column of the table, we don't have any left cell, so for avoiding complex works on indexes, we first calculate the weight of cells those belong to the first column of the table.

Again, we should do different work for the first row of the table. Because this row has no top and top left cell.

Then, we start to calculate the minimum value for moving to each cell for 1..=m column and 1..=n row. We Don't calculate the weights of cells that are after the desired cell, because it is not necessary.

In the cost table, for calculating the values of each row, we only need the values of the previous row. So, As you can see, we can use the cost table as the minimum-cost paths table without any problem.

Here is the final code:

def trip_min(num1, num2, num3):
    return min([num1, num2, num3])[0]

def test(cost, m, n):
    for i in range(1, m + 1):
        cost[i][0] += cost[i - 1][0]
    for j in range(1, n + 1):
        cost[0][j] += cost[0][j - 1]
    for j in range(1, n + 1):
        for i in range(1, m + 1):
            cost[i][j] += min(cost[i - 1][j - 1], cost[i][j - 1], cost[i - 1][j])
    return cost

if __name__ == '__main__':
    input_list = [[i+j for i in range(10)] for j in range(10)]
    res = test(input_list, 8, 8)
    print(res)
    print(res[8][8])

I changed the trip_min function and used the min function instead of writing the code of minimum by myself. Why? Because using builtin functions is always better than writing your own code, unless those behavior or performance is not exactly what you want. That reduces the probability of bug in your code any many times makes your code faster and cleaner.

Also, I changed the name of the cost table form input to the input_list. input is the name of a build-in function of the Python and it's a very bad idea to declear a variable with the same name of other things. That will make you and your collaborators confused about the code in the future.

You can get the minimum cost to each cell by calling res[row][col] on the result of the test function (You can't do that on the indexes after m and n as described).

The last thing I should say is if you use the cost table as the minimum-cost paths table,  you can not re-use that costs. Because we actually changed the value of the input_list. As you may know, if you send a list as a parameter to a function, and you change the values of that list, the original list will change. Why? Because Python will not copy the lists when you pass them as function parameters.

So if you want to calculate minimum paths for different cells, it's better to first calculate the minimum-path table for entire cost table (by passing m and n to the lengths of the table), and then access values from the result table.

And, I promise it's the final note, don't forget that res and input_list both are exactly same lists (Actually, you can remove the return in the test function).


Shakespeare and dictionaries The following snippet will return a list containing the words in all of the works of Shakespeare:

>>>def shakespeare_tokens(path = 'shakespeare.txt', url = 'http://inst.eecs.berkeley.edu/~cs61a/fa11/shakespeare.txt'):
        """Return the words of Shakespeare's plays as a list"""
        import os
        from urllib.request import urlopen
        if os.path.exists(path):
                return open('shakespeare.txt', encoding='ascii').read().split()
        else:
                shakespeare = urlopen(url)
                return shakespeare.read().decode(encoding='ascii').split()
>>>tokens = shakespeare_tokens()

Problem 1:

  Here's the idea: We start with some word - we'll use "The" as an example. Then we look through all of the texts of Shakespeare and for every instance of "The" we record the word that follows "The" and add it to a list, known as the successors of "The". Now suppose we've done this for every word Shakespeare has used, ever.

  Let's go back to "The". Now, we randomly choose a word from this list, say "cat". Then we look up the successors of "cat" and randomly choose a word from that list, and we continue this process. This eventually will terminate in a period (".") and we will have generated a Shakespearean sentence!

  The object that we'll be looking things up in is called a 'successor table', although really it's just a dictionary. The keys in this dictionary are words, and the values are lists of successors to those words.

  Here's an incomplete definition of the build_successors_table function. The input is a list of words (corresponding to a Shakespearean text), and the output is a successors table. (By default, the first word is a successor to '.').

  >>> table = build_successors_table(tokens)

Problem 2:

  Let's generate some sentences Suppose we're given a starting word. We can look up this word in our table to find its list of successors, and then randomly select a word from this list to be the next word in the sentence. Then we just repeat until we reach some ending punctuation. (Note: to randomly select from a list, first make sure you import the Python random library with import random and then use the expression random.choice(my_list)) This might not be a bad time to play around with adding strings together as well.

  >>> construct_sent('The', table)

Below is the solution for above 2 problems:

#Shakespeare and Dictionaries
def build_successors_table(tokens):
    table = {}
    prev = '.'
    for word in tokens:
        if prev in table:
            table[prev].append(word)
        else:
            table[prev] = [word]
        prev = word
    return table

def construct_sent(word, table):
    import random
    result = ''
    while word not in ['.', '!', '?']:
        result = result + word
        word = random.choice(table[word])
    return result + word

Python's standard library includes a class that would simplify your first snippet: collections.defaultdict. This class allows you to set a default for missing keys, so if you make it a list you can append without having to check if it already exists.

You could also simplify the iteration using zip; see e.g. "Iterate a list as pair (current, next) in Python".

Using that would make it:

from collections import defaultdict

def build_successors_table(tokens):
    table = defaultdict(list)
    for prev, word in zip(['.']+tokens, tokens):
        table[prev].append(word)
    return table

For the second snippet note that, per the style guide, all imports should happen at the start of your script - there's no benefit to trying to import random every time the function is called.

Continually appending strings is relatively inefficient, as a new string object is created each time. Instead, it's typical to create a list of words and then use ' '.join(words) to combine them back together.

It is more efficient to test membership (x in y) on a set (O(1)) than a list (O(n)).

import random

def construct_sent(word, table):
    words = []
    while word not in {'.', '!', '?'}:
        words.append(word)
        word = random.choice(table[word])
    return ' '.join(words) + word

Both functions would benefit from docstrings explaining what they do and how to use them (e.g. what the parameters are expected to be).


Point2D.Double wrapper I have made a Point2D.Double wrapper so I can add my own functionality (like a dominate method) and alter some (like toString) and control some (like not having public access to x and y).

Point class:

public class Point implements Serializable {

    private final Point2D.Double point2D;

    public Point(double x, double y) {
        this.point2D = new Point2D.Double(x, y);
    }

    public Point(String line, String delimiter) {
        line = line.trim();
        String[] lineArray = line.split(delimiter);
        double x = Double.parseDouble(lineArray[0]);
        double y = Double.parseDouble(lineArray[1]);
        this.point2D = new Point2D.Double(x, y);
    }

    public boolean dominates(Point point) {
        return (this.getX() <= point.getX() && this.getY() < point.getY())
                || (this.getY() <= point.getY() && this.getX() < point.getX());
    }

    public double getX() {
        return point2D.getX();
    }

    public double getY() {
        return point2D.getY();
    }

    @Override
    public boolean equals(Object o) {
        return point2D.equals(o);
    }

    @Override
    public int hashCode() {
        return point2D.hashCode();
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append('(').append(point2D.getX()).append(", ").append(point2D.getY()).append(')');
        return builder.toString();
    }
}

I took a look and came up with a couple of suggestions:

Extend rather than wrap

I think it will be more useful if you extend the Point2D.Double class, instead of writing a wrapper around it. This way you will be able to use your custom point class everywhere that a normal Point2D.Double is expected. It also spares you from implementing and overriding many of the Double's methods like getX() and the hashCode() and toString() methods.

Use static method for construction from line

When subclassing Double it is not possible to have a constructor that takes a line, does some calculations and then calls super(), as super has to be the first call in the constructor. Therefor we need to add a static utility class to create a Point from a line. See the fromLine() method below.

I think this is not a problem, as it leads to very readable line:

Point start = Point.fromLine(line);

Documentation

Your code does not include any documentation in form of comments, so that for example the input format for the fromLine() function is not entirely clear. Also it only uses one point from a line (which should be two points?), so maybe it should be called fromLineStart() or fromLineEnd() depending on which you mean.

Class Naming

The current name Point doesn't tell us a lot about what the class does. Maybe use something a little more descriptive like PointDouble2D? Maybe add some indication as that it does more than the normal Point2D.Double, by naming it: PointDouble2DAdvanced or something like that, not totally sure myself :)

Final Result

Here is the entire code example, without documentation comments:

import java.awt.geom.Point2D;

public class PointDouble2D extends Point2D.Double {

    public Point(double x, double y) {
        super(x, y);
    }

    public boolean dominates(Point2D.Double point) {
        return (getX() <= point.getX() && getY() < point.getY())
                || (getY() <= point.getY() && getX() < point.getX());
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append('(').append(getX()).append(", ").append(getY())
                .append(')');
        return builder.toString();
    }

    public static PointDouble2D fromLineStart(String line, String delimiter) {
        line = line.trim();
        String[] lineArray = line.split(delimiter);
        double x = java.lang.Double.parseDouble(lineArray[0]);
        double y = java.lang.Double.parseDouble(lineArray[1]);
        return new PointDouble2D(x, y);
    }
}

What do you think?


"Find the number" puzzle A friend gave this puzzle:

Find 3 numbers (say, A B C) such that

  \$ABC+ABC+ABC = CCC\$

ABC means \$((A*100) + (B*10) + C)\$. Also, these 3 digits must be distinct.

I wrote a quick code to find out the answer:

int main(void) {
    // your code goes here
    int hun,dec,unit;
    int final_num = 0;
    int temp_num = 0;
    for(hun = 0; hun <10; hun++)
    {
        for(dec = 0; dec < 10; dec++)
        {
            for(unit = 0; unit < 10; unit++)
            {
                // Calculate the number which would be formed with this unit place
                final_num = (unit *100) + (unit * 10) + (unit);

                // Values should not be same
                if((unit!=dec) && (unit != hun) &&( dec != hun))
                {
                    // Number with current combo of ABC
                    temp_num = ((hun * 100) + (dec * 10) + unit) * 3;
                    if(temp_num ==final_num )
                    {
                        printf("Number is %d \n",temp_num);
                        printf(" Hun %d Dec %d Unit %d",hun,dec,unit);
                    }
                }
            }
        }
    }
    printf("\nEnd of File ");
    return 0;
}

And the Answer was 185 (185 + 185 + 185 = 555).

That was very amateur code. Upon thinking, I realized I could have implemented equation \$300A + 30B + 3C = 111C\$. But still couldn't avoid for loop.
What could be the better way to implement this? I know only C so I solved it with C.

I'm sorry, but the best solution to this problem has to be a logic solution, not a code solution. There are only 2 digits that, when multiplied by 3, have the same last digit... 0, and 5. So, 0+0+0 is 0, and 5+5+5 is 15.

Since C cannot be 0, it means that C can only be 5. Now, if C is 5, and we know that there is a 'carry' of 1 in to the tens column, it means we need to find a number other than 5 that when added together 3 times ends in a 4.

There is only one value that does that, 8. 8 + 8 + 8 is 24, and with the carry of 1, we have 25 (and a carry of 2 in to the 100's column).

So, now we need a digit that sums three times to 3, and that's 1.

There is only one possible solution where ABC + ABC + ABC is CCC, and that's 185. It can be deduced using logic alone, and brute-forcing it is overkill.


Mouseover effects for five buttons which may be enabled or disabled function prepareEventHandlers() {
    var sectionButton1 = document.getElementById("sectionButton1");
    var sectionButton2 = document.getElementById("sectionButton2");
    var sectionButton3 = document.getElementById("sectionButton3");
    var sectionButton4 = document.getElementById("sectionButton4");
    var sectionButton5 = document.getElementById("sectionButton5");

    var enabled1 = true;
    var enabled2 = false;
    var enabled3 = false;
    var enabled4 = false;
    var enabled5 = false;

    function checkEnabled() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonEnabled");
        }

    }

    checkEnabled();
    sectionButton1.onmouseover = function() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton1.onmouseout = function() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton2.onmouseover = function() {
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton2.onmouseout = function() {
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton3.onmouseover = function() {
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton3.onmouseout = function() {
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton4.onmouseover = function() {
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton4.onmouseout = function() {
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton5.onmouseover = function() {
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton5.onmouseout = function() {
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonEnabled");
        }
    };
}

window.onload = function() {
    prepareEventHandlers();
};

Well first you should think about refactoring your code.  The following

if (enabled1) {
    sectionButton1.setAttribute("class", "sectionButtonEnabled");
}

is repeated a few times with just 2 differences.  The enabled flag and the button.  So this could easily go into a new function

function SetClassIfEnabled(enabled, btn) {
    if (enabled) {
        btn.setAttribute("class", "sectionButtonEnabled");
    }
}

The following is also repeated:

sectionButton1.onmouseover = function() {
    if (enabled1) {
        sectionButton1.setAttribute("class", "sectionButtonOver");
    }
};
sectionButton1.onmouseout = function() {
    if (enabled1) {
        sectionButton1.setAttribute("class", "sectionButtonEnabled");
    }
};

so this could easily go into another function as well.  But I would change one thing. You are adding the mouse events to every button but they only do something if they are enabled.  So I would check that first and then add the mouse events.

function AddHoverIfEnabled(enabled, btn) {
     if (enabled) {
        btn.onmouseover = function () {
            btn.setAttribute("class", "sectionButtonOver");
        };
        btn.onmouseout = function () {
            btn.setAttribute("class", "sectionButtonEnabled");
        };
    }
}

Doing just the above will shorten your code quite a bit because you can then call the functions like this

SetClassIfEnabled(enabled1, sectionButton1);
SetClassIfEnabled(enabled2, sectionButton2);
...
AddHoverIfEnabled(enabled1, sectionButton1);
AddHoverIfEnabled(enabled2, sectionButton2);
...

But I took it a step farther.  By putting the buttons and the enabled flags into an array you can just iterate over each and do the work if necessary.  Doing it this way will make it extremely easy to add another button in the future.  The following code can replace all the above mentioned changes.

function prepareEventHandlers() {

    var btnArray = [];
    btnArray[0] = [document.getElementById("sectionButton1"), true];
    btnArray[1] = [document.getElementById("sectionButton2"), false];
    btnArray[2] = [document.getElementById("sectionButton3"), false];
    btnArray[3] = [document.getElementById("sectionButton4"), false];
    btnArray[4] = [document.getElementById("sectionButton5"), false];

    for (i = 0; i < btnArray.length; i++) {
        if (btnArray[i][1] == true) { // section/button is enabled

            var btn = btnArray[i][0];
            btn.setAttribute("class", "sectionButtonEnabled"); // set default class

            // Add Hover Events
            btn.onmouseover = function () {
                this.setAttribute("class", "sectionButtonOver");
            };
            btn.onmouseout = function () {
                this.setAttribute("class", "sectionButtonEnabled");
            };
        }
    }
}

I would actually take the array out of this function and pass it in as an argument but you get the point.

I'm also assuming the enabled flags may change for any of the buttons.  If not and it is only one (as you are showing) you could just eliminate all the code that has nothing to do with button1 because none of the others are enabled so they are not manipulated at all.


BigInteger class, supporting addition only Here is just addition, no multiplication or subtraction, but I will implement them later. Before going on, I'm here to ask for advice about the design.

#pragma once
#include <stdlib.h>
#include <crtdbg.h>
#include "stdafx.h"
#include <string>
#include <iostream>
#include <vector>
#include <cassert>
#include <algorithm>
#define to_int(x) (x - '0')
#define to_char(x) (char)(x + '0')
struct InvalidBigInt {};
class BigInteger
{
private:
    std::vector<char> vnum;
    std::string snum;
public:
    static bool flag;
    BigInteger() = default;
    BigInteger(const std::vector<char>& v) : vnum(v) {}
    BigInteger(std::string&& str) : snum(str)
    {
        vnum.clear();
        vnum = { str.crbegin(), str.crend() };
        try
        {
            if (std::find_if(str.begin(), str.end(),
                [](char c) { return !(isdigit(c)); }) != str.end())
            {
                throw InvalidBigInt();
            }
        }
        catch (InvalidBigInt)
        {
            std::cout << "*****************      Input invalid integer      *****************" << std::endl << \
                "*****************      Please input again         *****************" << std::endl;
        }
        flag = true;
    }
    friend std::ostream& operator<<(std::ostream& out, BigInteger bi)
    {
        std::for_each(bi.vnum.rbegin(), bi.vnum.rend(), [&](char c) {out << c; });
        return out;
    }
    BigInteger operator+(BigInteger b)
    {
        std::string result;
        auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;
        int temp = 0, digit = 0, diff = more.size() - less.size();
        for (size_t i = 0; i < diff; i++) { less.push_back('0'); }
        for (auto IteratorMore = more.begin(), IteratorLess = less.begin();
            IteratorMore < more.end() && IteratorLess < less.end();
            IteratorMore++, IteratorLess++)
        {
            digit = to_int(*IteratorMore) + to_int(*IteratorLess) + temp;
            if (digit > 9) { temp = 1; digit -= 10; }
            else { temp = 0; }
            result = to_char(digit) + result;
        }
        if (temp) { result = to_char(temp) + result; }
        return BigInteger(std::move(result));
    }
    bool operator==(BigInteger b)
    {
        return this->vnum == b.vnum;
    }
    ~BigInteger() noexcept {}
};
bool BigInteger::flag{ false };

int main()
{
    /*********************************PLUS*******************************/
    {
        assert(BigInteger("0") + BigInteger("0") == BigInteger("0"));
        assert(BigInteger("0") + BigInteger("1") == BigInteger("1"));
        assert(BigInteger("1023") + BigInteger("1") == BigInteger("1024"));
        assert(BigInteger("987") + BigInteger("34") == BigInteger("1021"));
        assert(BigInteger("98765") + BigInteger("23456") == BigInteger("122221"));
        assert(BigInteger("9230842083418927893758317489127418924781923748912") + BigInteger("81923743214617823686412783461237846178463")
            == BigInteger("9230842165342671108376141175540202386019769927375"));
    }
}

Abusing the use of macros

I think there is no good reason for implementing to_int() and to_char() as macros - such an implementation may result in hard to find bugs as you don't have any type checking. Lack of parameter types also makes it harder to understand what the function is expected to do in case of more complex functions.

Also, the names of these functions are misleading as they don't inform in a clear way that you are expecting an input to represent a single digit and any other input is invalid.

If you stick to the macro anyway, it's a common convention to use uppercase letters for macro names - most people would expect an ordinary function after seeing lowercase to_int()/to_char().

Single responsibility principle violation

Using catch this way:

catch (InvalidBigInt)
        {
            std::cout << "*****************      Input invalid integer      *****************" << std::endl << \
                "*****************      Please input again         *****************" << std::endl;
        }

you are restricting the use of your class to console applications. Things such as text display should be moved outside of your class as the responsibility of your class is to calculate numbers, not display them nor interact with a user via an interface.

Unclear variable names

Tha name flag is very vague and it's necessary to read the code to see what it's supposed to do (assuming it is used in the intended way). I'm not sure what is its purpose anyway since it's only set and not referred to anywhere in the code. I also suspect it shouldn't be a static variable.

Organizing code into headers and implementation files

Don't put all your code within your main.cpp file. It's more versatile and easier to manage if you separate it and put declarations in headers and definitions in .cpp files - you'll see that as soon as you decide to separate the text display functionality mentioned above from your main class.

Inconsistent and uncommon naming convention

Here:

for (auto IteratorMore = more.begin(), IteratorLess = less.begin();
    IteratorMore < more.end() && IteratorLess < less.end();
    IteratorMore++, IteratorLess++)

the names of the variables IteratorMore and IteratorLess start with a capital 'I', whereas it is a common practice to start the name of a class with the capital letters and start variable names with a lowercase. You are also following this convention correctly throughout your code, except in this part.

Const correctness

You don't always adhere to the concept of const correctness: your operators don't modify the BigInteger argument and thus it should be passed as a const reference to avoid unnecessary copies and make room for possible compiler optimizations.

Code lines' length

In some places in your code, you are writing very long lines, such as:

auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;

Even though most modern IDE's have built-in line wrapping, it's a good practice to write your code in such a way that a single line comprises approximately 80 characters. It's more comfortable and faster to read and comprehend the code if you are not forced to scroll it or move your eyeballs repeatedly from one side of a wide screen to another. Also that way you have control over where you wrap your code, whereas line wrapper functionality may visually organize the code in a way that makes it less readable. Also it's easier to use diff tools and open several editor windows side-by-side if you stick to this rule. See the discussion e.g. here:

https://stackoverflow.com/questions/276022/line-width-formatting-standard

Redundant variables

It seems you are not using snum anywhere in your code right now and even if you do use it in the future, it is probably to serve exactly the same puropse
as vnum, so it's better to convert one type to another instead of keeping both and using only one of them depending on what's passed to the constructor - right now you would have to double your code by checking which variable is set inside each of your functions.

I would ditch vnum and use snum everywhere instead as std::string is a semantic equivalent of std::vector<char>, except you can benefit from small string optimization.


Simple logger class in C# I've been using logging libraries recently and somebody told me to try and write my own. I've kept it minimal but the only drawback is I can't log in static classes, due to TClass.

Follow-up

I'm looking for constructive criticism on how to improve it.

How do I declare a logger variable?

private static readonly Logger<CatalogController> Logger = new Logger<CatalogController>();

I guess another drawback is that its pretty long, including the class name (twice) and then all the access modifiers and class names of the logger, it an get pretty ugly.

public class Logger <TClass>
{
    private readonly Type _className;

    public Logger()
    {
        _className = typeof(TClass);
    }

    public void Trace(string message, bool log = false)
    {
        Log(message, ConsoleColor.White, log);
    }

    public void Warn(string message, bool log = false)
    {
        Log(message, ConsoleColor.Yellow, log);
    }

    public void Debug(string message, bool log = false)
    {
        Log(message, ConsoleColor.Cyan, log);
    }

    public void Success(string message, bool log = false)
    {
        Log(message, ConsoleColor.Green, log);
    }

    public void Error(string message, bool log = false)
    {
        Log(message, ConsoleColor.Red, log);
    }

    public void Error(Exception e)
    {
        Log("An error occurred: " + Environment.NewLine + e, ConsoleColor.Red);
    }

    private void Log(string message, ConsoleColor color, bool log = false)
    {
        var oldColor = Console.ForegroundColor;
        Console.ForegroundColor = color;
        Console.WriteLine($"[{DateTime.Now:MM/dd HH:mm:ss}] " + message);
        Console.ForegroundColor = oldColor;

        string logFile;

        switch (color)
        {
            case ConsoleColor.Yellow:
                logFile = "error.log";
                break;
            case ConsoleColor.Cyan:
                logFile = "debug.log";
                break;
            case ConsoleColor.Red:
                logFile = "error.log";
                break;
            default:
                logFile = "trace.log";
                break;
        }

        LogToFile(logFile, $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message);
    }

    private static void LogToFile(string file, string content)
    {
        var executionPath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location);
        var fileWriter = new StreamWriter(executionPath + "/resources/logging/" + file, true);

        fileWriter.WriteLine(content);
        fileWriter.Close();
    }
}

I have two notes to make here,

Extract a public interface from this code, like ILogger<T>, and store (or inject) an ILogger<T> instead of a Logger<T>

This will allow you to create different implementations of your logger type and pass it to the consumers of your logger without them knowing/caring whether it's logging to a console, to a file, or to a typewriter.

An important use case for that is creating an ILogger implementation for unit tests. You probably don't want your unit tests to have any side effects, so you could create a TestLogger class that implements ILogger and records all the logs to a list instead of a to a file or console.

Handle write access in a safer way

Currently you are not disposing of your StreamWriter instance. Even though you are calling Close you really should be either using a using statement or having a single instance of the stream writer that is accessed through an actor or other thread safe method and is disposed when your logger is disposed.

(Less important, but I would also store the execution path in a static readonly

private static readonly string executionPath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location + "/resources/logging/" + file;

private static void LogToFile(string file, string content)
{
    using (var fileWriter = new StreamWriter(executionPath, true))
    {
        fileWriter.WriteLine(content);
        fileWriter.Close();
    }
}


Printing log messages with varying length fit into screen width I'm writing a simple sanity tool for a simple USB driver.

The customer is directly facing the output so I have to care about how it looks.
The log messages that I need to print are varying length, and they have "[  SUCCESS ]" and "[  FAIL  ]" trailing part too and the middle is filled with dots.

So what I do is calculate the number of dots that should be in the message.
For a example,

#define SCREEN_SIZE_X            80

void print_message(const char* pMsg, bool isPassed)
{
#define SPRINTF_BUFFER_SIZE    1024
    char* sprintfBuffer=(char*) malloc(SPRINTF_BUFFER_SIZE);
    int iLengthMsg = strlen( pMsg);
    const char* strTrailingPart= isPassed?"[  PASSED  ]":\
                     "[  FAILED  ]";
    int iLengthTrailingPart=strlen(strTralingPart);
    int iNumDots= SCREEN_SIZE_X - iLengthMsg - iLengthTrailingPart;

    sprintf( sprintfBuffer,pMsg);

    for(int i=0; i< iNumDots; i++)
    {
        sprintf( sprintfBuffer+strlen( sprintfBuffer),\
                  "%c" , '.');
    }

    sprintf(sprintfBuffer+strlen(sprintfBuffer), \
                 strTrailingPart);
    printf( sprintfBuffer);
    free (sprntfBuffer);
}

Any better way to do this thing?

Let me assume you're writing in C++ (leaving a quick review of C++ Cish code for the end.)

For clarity I prefer to split code in separate functions, they may be even reused.

void print_message(const std::string& message, bool isPassed)
{
    const std::string result = resolve_result_message(isPassed);
    std::cout << message << get_line_padding(message, result) << result << std::endl;
}

std::string resolve_result_message(bool isPassed) {
    // Avoid hard-coded strings, move these to constants
    return isPassed ? "[  FAILED  ]" : "[  PASSED  ]";
}

std::string get_line_padding(const std::string& message, const std::string& result) {
    const int requiredPadding = SCREEN_WIDTH - message.length() - result.length();

    if (requiredPadding <= 0)
        return "";

    return std::string(requiredPadding, '.');
}

About original C code:

You have a maximum output size (which, BTW, will be unaligned with other lines) set to SPRINTF_BUFFER_SIZE. Note that if input is bigger than this then your application may crash and you do not perform any check about this. You may want to cut output (in this case just reused SCREEN_SIZE_X + 1 constant).

If you're using a C compiler then do not cast return value of malloc().

sprintf() to print characters is absolutely an overkill, after all this is enough (don't forget to add a NUL character at the end):

for (int start = strlen(sprintfBuffer), i = 0; i < iNumDots; ++i)
    sprintfBuffer[start + i] = '.';

Now you can see it's equivalent to (after checking if iNumDots is positive):

memset(sprintfBuffer + strlen(sprintfBuffer), '.', iNumDots);

If this function isn't invoked by multiple threads you can avoid to allocate/free the buffer each time marking it as static.

If you're working on resource limited environment you may want to avoid unnecessary allocations, for this example let me assume your code is single threaded and message is short enough to fit one line, let me write it in C:

void print_message(const char* message, bool isPassed) {
    static char buffer[SCREEN_WIDTH + 1] = { 0 };

    const char* passed = isPassed ? "[  FAILED  ]" : "[  PASSED  ]";

    memset(buffer, '.', SCREEN_WIDTH);
    memcpy(buffer, message, strlen(message);
    memcpy(buffer + SCREEN_WIDTH - strlen(passed), passed, strlen(passed);

    puts(buffer);
}