Replacing IUnitOfWork with a Factory Pattern As a follow-up to this blog post and this question, I wanted to post my idea of replacing the IUnitOfWork with a factory pattern. It seems to me that in the case of this blog post the IUnitOfWork seems to be a type of sudo-factory pattern with each repository type and their accessors hardcoded. So my goal was to come up with a fully dynamic factory. I haven't had the chance to try this yet but wanted to post the idea to get some opinions.

IGenericRepository

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;

namespace DataAccess
{
    public interface IGenericRepository<T>
    {
        IQueryable<T> AsQueryable();

        IEnumerable<T> GetAll();
        IEnumerable<T> Find(Expression<Func<T, bool>> predicate);
        T Single(Expression<Func<T, bool>> predicate);
        T SingleOrDefault(Expression<Func<T, bool>> predicate);
        T First(Expression<Func<T, bool>> predicate);
        T GetById(int id);

        void Add(T entity);
        void Delete(T entity);
        void Attach(T entity);
    }
}

EFGenericRepository

using System.Data.Entity;
using DataAccess;

namespace EFDataAccess
{
    public class EFGenericRepository<T> : IGenericRepository<T> where T : class
    {

        private readonly DbSet<T> _dbSet;

        public EFGenericRepository(DbSet<T> dbSet)
        {
            _dbSet = dbSet;
        }

        // Implement IGenericRepository Methods here...
    }
}

RepositoryFactory

using System;
using System.Data.Entity;
using DataAccess;

namespace EFDataAccess
{
    class RepositoryFactory
    {

        private readonly DbContext _context;

        public RepositoryFactory(DbContext context)
        {
            _context = context;
        }

        public IGenericRepository<T> Get<T>() where T : Type
        {
            return new EFGenericRepository<T>(_context.Set<T>());
        }
    }
}

Edit:
Created a github repo here

Bad idea...?

Generally, I belong to the camp thinking generic repositories are a bad idea:

The abstraction will be leaky at best.
If you really need a repository per type, why not let the client new it up directly?
Except from basic CRUD, there is no generic functionality, so a generic interface will reduce your options.

The abstractions doesn't seem right. I know it follows the usual conventions and examples given on EF, but do you want the database to dictate the shape of your domain objects?

If you split the responsibilities between domain logic and persistence, it may be clearer what to do. A split like that will also open new possibilities on how to implement and optimize.

Here are some ideas:

Handle different responsibilities in different places

Given an imaginary customer and two use cases:

Upgrade to gold customer.
Get a list of gold customers.

Those are different responsibilities which can be expressed like:

public class Responsibilities
{
    public void UpdateCustomer()
    {
        var repository = new Repository();
        var customer = repository.GetCustomerById(42);

        customer.UpgradeToGold();

        repository.Save(customer);
    }

    public void QueryGoldCustomers()
    {
        var query = new CustomerQuery();
        var goldCustomers = query.GetGoldCustomers();
    }
}

Repository and state management

This is a naive implementation; it will only work with customers already stored in the database, plus it generates multiple calls to the database. But the implementation is separated from everything else, so we are free to optimize the database access when it starts to be a problem.

I haven't used EF in a while, so the Save method may be way off.

public class Repository
{
    readonly Database context = new Database();

    public Customer GetCustomerById(int id)
    {
        var state = context.CustomerState.Single(x => x.Id == id);
        var address = context.Addresses.Single(x => x.Id == state.AddressId);
        var snapshot = new Customer.Snapshot
            {
                State = state,
                Address = address
            };

        return new Customer(snapshot);
    }

    public void Save(Customer customer)
    {
        var provider = (IProvideSnapshot<Customer.Snapshot>) customer;
        var snapshot = provider.GetSnapshot();

        context.Addresses.Attach(snapshot.Address);
        context.CustomerState.Attach(snapshot.State);
        context.SaveChanges();
    }
}

Isolated queries

Same thing here; there are better ways to implement the query. Note that this query is only meaningful inside of the domain boundary, like when you want to send special offers to all gold customers.

UI views, reports and such would probably benefit from using dtos and not the domain object directly.

If it turns out to be hard to generate efficient queries with EF, we are free to select other tools, even inline SQL.

public class CustomerQuery
{
    readonly Database context = new Database();

    public IEnumerable<Customer> GetGoldCustomers()
    {
        var states = context.CustomerState.Where(state => state.IsGold);
        var addressIds = states.Select(state => state.AddressId);
        var addresses = context.Addresses.Where(address => addressIds.Contains(address.Id));

        return states
            .Select(state => new Customer.Snapshot
                {
                    State = state,
                    Address = addresses.Single(address => address.Id == state.AddressId)
                })
            .Select(snapshot => new Customer(snapshot))
            .ToList();
    }
}

Other classes for completeness

In this example, the shape of the domain class, the Customer, is not dictated by the database.

The shape of the Snapshot is, though. But that could be a good thing, because the object-to-database impedance mismatch is lower.

public class Customer : IProvideSnapshot<Customer.Snapshot>
{
    readonly Snapshot snapshot;

    public Customer(Snapshot snapshot)
    {
        this.snapshot = snapshot;
    }

    public void UpgradeToGold()
    {
        snapshot.State.IsGold = true;
    }

    Snapshot IProvideSnapshot<Snapshot>.GetSnapshot()
    {
        return snapshot;
    }

    public class Snapshot
    {
        public CustomerState State { get; set; }
        public Address Address { get; set; }
    }
}

public interface IProvideSnapshot<out T>
{
    T GetSnapshot();
}

public class Database : DbContext
{
    public DbSet<CustomerState> CustomerState { get; set; }
    public DbSet<Address> Addresses { get; set; }
}

public class CustomerState
{
    public int Id { get; set; }
    public int AddressId { get; set; }
    public bool IsGold { get; set; }
}

public class Address
{
    public int Id { get; set; }
}


Khan inspired Insertion Sort I just watched a Khan Academy video on Insertion Sort and I've tried to write an implementation in Python. Please suggest corrections or improvements on this program:

unsorted_list=[45,99,1,-22,7,3,294,10,36]

def insertion_sort(unsorted):
    for i in range(1,len(unsorted)):
        for j in range(i):
            if unsorted[i]<unsorted[j]:
                temp = unsorted[j]
                unsorted[j]=unsorted[i]
                unsorted[i]=temp

    return unsorted

print insertion_sort(unsorted_list)

I wouldn't call your implementation insertion sort as it's sorting in place using swaps, which as David pointed out makes it look more like a bubble sort.  An insertion sort sorts by building up a new list and inserting items into the proper place.  A very crude first pass at an insertion sort might look like:

def insertion_sort(unsorted_list):
    result = []
    for n in unsorted_list:
        inserted = False
        for i in range(len(result)):
            if n < result[i]:
                result.insert(i, n)
                inserted = True
                break
        if not inserted:
            result.append(n)
    return result

This could be greatly improved for larger data sets by making the inner search take advantage of the fact that s is sorted and doing a binary search for the insertion point.  Of course, this being Python, the whole mess could be optimized to sorted(unsorted_list). ;)


Replicating C#-style events in Java using reflection For something I'm doing, it would just make things so much easier, quicker, and cleaner to have C# style events. I've written a class I believe should replicate a C# style event using reflection. Since I've never actually used reflection before in any meaningful sense, I'm very much not happy using it without, at the very least, a second opinion.

I want to declare an event with:

public Event SomeEvent = new Event();

Raise the event (presumably within the declaring class) with (where SomeArguments is whatever class that may be specified in the Javadocs boxed in an Object):

SomeEvent.Raise(this, SomeArguments);

Add a method to be called when the event is raised with:

SomeEvent.AddListener(this, "onSomeEvent");

Stop a method being called when an event is raised with:

SomeEvent.RemoveListener(this, "onSomeEvent");

And cancel all listeners with:

SomeEvent.ClearListeners();

Methods to listen to the event should be formatted like:

public void onSomeEvent(object Sender, object SomeArguments)
{ }

And this is my class:

package puppy.hanii.library;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.ArrayList;

public class Event
{
    public Event()
    {
        Methods = new ArrayList<Method>();
        DeclaringClasses = new ArrayList<Object>();
    }

    ArrayList<Method> Methods;
    ArrayList<Object> DeclaringClasses;

    public boolean AddListener(Object Class, String MethodName)
    {
        try
        {
            Method MTR = Class.getClass().getMethod(MethodName, Object.class, Object.class); // MTR = Method To Register.

            if(Methods.contains(MTR))
                return false;
            else
            {
                Methods.add(MTR);
                DeclaringClasses.add(Class);
                return true;
            }
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overloads of method, exists."); }
    }

    public boolean RemoveListener(Object Class, String MethodName)
    {
        try
        {
            DeclaringClasses.remove(Class);
            return Methods.remove(Class.getClass().getMethod(MethodName, Object.class, Object.class));
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overload of method, exists."); }
    }

    public void ClearListeners()
    {
        Methods.clear();
        DeclaringClasses.clear();
    }

    public void Raise(Object Sender, Object Arguments)
    {
        for(int i = 0; i < Methods.size(); i++)
        {
            try
            { Methods.get(i).invoke(DeclaringClasses.get(i), Sender, Arguments); }
            catch(IllegalAccessException error)
            { }
            catch(InvocationTargetException error)
            { throw new RuntimeException("Error in event handler"); }
        }
    }
}

ArrayList<Method> Methods;
    ArrayList<Object> DeclaringClasses;

Two things: firstly, you're using C# naming conventions rather than Java. Secondly, code to the interface: the fields should be List<XYZ> rather than ArrayList<XYZ>.

    public boolean AddListener(Object Class, String MethodName)

Class isn't an obvious name for something which isn't a class. And given that you're using java.lang.Class inside the method without fully qualifying the name, it has even more potential to confuse.

            Method MTR = Class.getClass().getMethod(MethodName, Object.class, Object.class); // MTR = Method To Register.

If you have to add a comment explaining the name, perhaps you chose the wrong name? What's wrong with Method method = ...?

            if(Methods.contains(MTR))
                return false;

So I can't have the event notify two different objects of the same class?

    public boolean RemoveListener(Object Class, String MethodName)
    {
        try
        {
            DeclaringClasses.remove(Class);
            return Methods.remove(Class.getClass().getMethod(MethodName, Object.class, Object.class));
        }

Buggy. Collection.remove removes one instance of the object, but you're making no attempt to ensure it's the right one. If I register two methods on the same object, this could remove the wrong instance of the object, which would then make objects and methods not line up correctly.

In fact, it's even worse than that: if I register a method on one object and then remove it on another, or register one method on an object and then remove a different one, I'll end up with Methods and DeclaringClasses not even being the same size.

Also, why would you want to call getMethod again unless the method hasn't been registered? If the method has been registered, you've already resolved it; if it hasn't, you don't need to resolve it except to check for typos.

The best way to store the resolved methods is probably Map<Tuple<Object, String>, Method>: that allows you to iterate through the entry set in the Raise method and to efficiently implement RemoveListener.

            catch(IllegalAccessException error)
            { }

No. Swallowing exceptions like that is bad, especially when you've done nothing to prevent them occurring. You should check in AddListener whether the method is accessible, and throw an exception there if it isn't. Then here you should log an error for debugging purposes.

Other issues to consider:

Thread-safety. This has no thread-safety whatsoever.
Typing. If you're going to emulate C#'s events then don't just emulate the ungenericised EventHandler. At the very least, emulate EventHandler<T>. Because Java's generics aren't reified this requires a bit of boilerplate:

public class Event<T>
{
    private final Class<T> typeParam;

    public Event(Class<T> typeParam)
    {
        this.typeParam = typeParam;
    }

You can now use typeParam in the Class.getMethod call.


Creating a ticker thread A new question about a revised version of the code in this question, based on the advice in the accepted answer can be found here.

I need to signal my program to do something every X seconds. So I decided to make a separate thread that sends such a signal using ZeroMQ. All other behaviour is also triggered by ZeroMQ messages, so this seems like a safe way to do it, with respect to multithreading.

I have never used threads in C++ before though, so I'd like some feedback on my code. Note that in the code below I have replaced the actual task with a simple cout statement.

EDIT: I just realized the thread could very well terminate between setting run = false and calling join, which causes a std::system_error to be thrown. Putting the join statement in an if-block with isjoinable still suffers from the same problem. How should I handle this? try-catch? Using a locking mechanism instead of an atomic bool?

Ticker.hpp

#include <thread>
#include <chrono>
#include <atomic>

class Ticker {
private:
    const unsigned int interval;
    std::atomic_bool run;
    std::thread tickthread;
public:
    Ticker(unsigned int interval);
    Ticker(const Ticker& orig) = delete;
    virtual ~Ticker();
    void stop();
private:
    void tickfunction();
};

Ticker.cpp

#include "Ticker.hpp"
#include <iostream>

using namespace std;

Ticker::Ticker(unsigned int interval)
: interval(interval), run(true), tickthread(&Ticker::tickfunction, this) {
}

Ticker::~Ticker() {
    if (tickthread.joinable()) {
        stop();
    }
}

void Ticker::stop() {
    cout << "stopping..." << endl;
    run = false;
    // interrupt sleep?
    tickthread.join();
    cout << "joined" << endl;
}

void Ticker::tickfunction() {
    while (true) {
        this_thread::sleep_for(chrono::seconds(interval));
        if (run) {
            cout << "tick" << endl;
        } else {
            break;
        }
    }
}

There are also two specific things I'd like recommendations on:

How can I change this class to accept any std::chrono::duration object as input for the interval, instead of just an integer? I tried this, but couldn't figure out how to do it with the templates.
Is there a way to interrupt the sleep_for when I call stop()? Especially when interval is large, it'd be nice to not have to wait for the sleeping thread when stopping it.

I see some things that may help you improve your program.

Use include guards

You probably already know this, but the Ticker.hpp file should have include guards to allow it to be included in multiple files within the same project.  The ordinary form would be something like this:

#ifndef TICKER_HPP
#define TICKER_HPP
// contents of Ticker.hpp go here
#endif //TICKER_HPP

Avoid data races

The tickfunction accesses std::cout but that's a single resource that might simultaneously be used by other threads.  One fix for this is to use a std::mutex like this:

std::mutex cout_mutex;

// wherever cout is used:
some_function() {
    std::lock_guard<std::mutex> lock(cout_mutex);
    std::cout << "Now we can do this safely!\n";
}

Note that std::lock_guard is intended to be used via RAII so that the lock is obtained when the object is created and released when it's destroyed, making it easy to avoid the bug of forgetting to unlock the mutex.

If the actual function called within your code each "tick" uses shared resources, you should apply the same technique.

Use standard durations

To answer your question, one way to use a standard duration would be to specify a particular one.  For example, if you decided that millisecond resolution was enough for this program (and it probably is), the class declaration might look like this:

class Ticker {
private:
    const std::chrono::milliseconds interval;
    std::atomic_bool run;
    std::thread tickthread;
public:
    Ticker(std::chrono::milliseconds intval);
    Ticker(const Ticker& orig) = delete;
    virtual ~Ticker();
    void stop();
private:
    void tickfunction();
};

Use the chrono literals for more readable code

With the code modified to use a standards time interval, we can now use it with std::chrono_literals and make the code even easier to write and to read.

int main()
{
    using namespace std::chrono_literals;
    Ticker t(500ms);
    Ticker t10(10s);
    std::this_thread::sleep_for(15s);
}

Allow for thread cancellation

For various reasons, there is no simple standard way of cancelling a thread.  However, there are almost always ways you can accomplish the equivalent in your own code.  I'll show how this might be done in the next suggestion.

Prefer async to raw threads

C++ has a std::async mechanism that is a bit higher level than the more fundamental thread and mutex mechanism.  The advantage to using it is that you can spend more time thinking about the problem you're trying to solve rather than the details of threads and locks.  Here's one way of refactoring your code that doesn't use thread explicitly:

#include <iostream>
#include <future>
#include <chrono>

static std::mutex cout_mutex;

class Ticker
{
public:
    // create but don't execute yet
    Ticker(std::shared_future<void> f, std::chrono::milliseconds interval)
    : interval{interval}, done{f}
    { }
    // run the thread
    std::future<void> run() {
        return std::async(std::launch::async, &Ticker::tickWrapper, this);
    }
private:
    void tickWrapper() {
        std::future_status status;
        do {
            status = done.wait_for(interval); // waits for the signal from main()
            if (status == std::future_status::timeout) {
                tickfunction();
            }
        } while (status != std::future_status::ready);
    }
    void tickfunction() {
        std::lock_guard<std::mutex> lock(cout_mutex);
        std::cout << "tick (" << std::this_thread::get_id() << ")" << std::endl;
    }

    const std::chrono::milliseconds interval;
    std::shared_future<void> done;
};

Note that I've separated tickWrapper from tickfunction.  The latter is now just a simple function that doesn't contain any references to timers or futures.  This allows us to separate the actual function from the mechanics of a recurring timer implementation.  Here's a main function showing how to create and drive this new Ticker object:

int main()
{
    // for convenience in specifying time durations
    using namespace std::chrono_literals;

    // create a promise and associated shared_future
    std::promise<void> done;
    std::shared_future<void> done_future(done.get_future());

    // create the Ticker objects
    Ticker t1(done_future, 300ms);
    Ticker t2(done_future, 500ms);

    // start the threads
    auto x = t1.run();
    auto y = t2.run();

    // delay for a while
    std::this_thread::sleep_for(3s);

    // tell the other threads to stop
    done.set_value();

    std::cout << "Done\n";
}


Elegant way for parsing birthday from rodne cislo (czech republic ID) How can I refactor it to elegant way? This code is about parse birthday from Rodne Cislo string. I don't know how to do it clever and add more clean code style details

public static void extractBirthdayFromRodneCislo(String rodneCislo) {
    rodneCislo = rodneCislo.replace("/", "=");

    int year;
    int month;
    int day;

    if (rodneCislo.length() < 10) {
        year = Integer.parseInt("19" + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    } else {
        String century = Integer.parseInt(String.valueOf(rodneCislo.charAt(0)) + String.valueOf(rodneCislo.charAt(1))) < 53 ? "20" : "19";
        year = Integer.parseInt(century + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    }

    month = Integer.parseInt(String.valueOf(rodneCislo.charAt(2)) + String.valueOf(rodneCislo.charAt(3)));

    if (month > 12) {
        month -= 50;
    }
    day = Integer.parseInt(String.valueOf(rodneCislo.charAt(4)) + rodneCislo.charAt(5));

    System.out.println(year + " " + month + " " + day);
}

The form is YYXXDD/SSSC, where XX=MM (month of birth) for male (numbers 01-12) and XX=MM+50 for female (numbers 51-62), SSS is a serial number separating persons born on the same date and C is a check digit, but for people born before 1 January 1954 the form is without the check digit - YYXXDD/SSS. This enables the system to work until the year 2054. The whole number must be divisible by 11.

In addition to Doi9t's answer, which gives good advice, I'll add that your function name, extractBirthdayFromRodneCislo, is wrong, as it merely prints the birthday instead extracting it.

The easy fix is to rename the function extractBirthdayFromRodneCislo, which may fit you case, but doesn't allow you to do anything more fancy than that.

The better way to do things would be to return the date, allowing you to do anything with it after calling the method.

I'm completely new to Java, so I might miss something here, but it seems that the java.time.LocalDate datatype is a good fit for the job.

So your function becomes something like this:

public static LocalDate extractBirthdayFromRodneCislo(String rodneCislo) {

    // your logic here

    return LocalDate.of(year, month, day);
}

This approach has the added advantage that, should there be a mistake in the input string leading to an invalid date, it will throw an exception instead of silently accepting an invalid date – if you pass 622754/1234 to your function or Doi9t's, it will not care that 1962 -23 54 is not an acceptable output and print it just like any other date.

This illustrates to another problem: input validation. Your code doens't care if the input is indeed a Rodne Cislo. It will take any string as input. It may fail if Integer.parseInt encounter non-digit characters, but will accept strings that are too long, too short, has a properly placed / character or not (comments pointed out that Czech people don't always include the /, any other character in this place, etc., leading to a big risk of improperly parsing the string.

One possible tool for validating strings are regular expressions (regex). A lot of different patterns can be used to validate the input string, a simple one would be:

^[0-9]{6}\/[0-9]{3}[0-9]?$

If you are unfamiliar with regex, here is a breakdown of this expression:

^[0-9]{6}\/?[0-9]{3}[0-9]?$

^                                 # start with (no leading characater)
 [0-9]{6}                         # 6 digits
         \/?                      # may or may not include a "/" character
            [0-9]{3}              # 3 additional digits
                    [0-9]?        # 1 optional additional digit
                          $       # ends here (no trailing character)

Checking if the input string matches this pattern will call out a lot of invalid input strings, although it doesn't check if the month and day numbers are within valid ranges – it is definitely possible, but will get nasty quite quickly, and the LocalDate.of() method already takes care of it.

Another nice thing about regex is that you can extract specific ranges within the expression easily with capture groups. A little modifications on the previous expression gives (with explanation):

^([0-9]{2})([0-9]{2})([0-9]{2})\/?[0-9]{3}[0-9]?$

^                                                     # starts with (no leading characater)
 ([0-9]{2})                                           # 2 digits, capture them in group 1
           ([0-9]{2})                                 # 2 digits, capture them in group 2
                     ([0-9]{2})                       # 2 digits, capture them in group 3
                               \/?                    # an optional "/" character
                                  [0-9]{3}            # 3 digits
                                          ([0-9]?)    # 1 optional digit, capture it in group 4
                                                  $   # ends here (no trailing character)

You can then used the captured strings to parse the numbers (groups 1 to 3), or identify if the birth year is before or after 1954 (group 4).

My final take on the problem is this code:

import java.time.LocalDate;
import java.util.regex.Pattern;
import java.util.regex.Matcher;

class Main {
    public static void main(String[] args) {
        System.out.println(extractBirthdayFromRodneCislo("535223/1234"))
        // > 2053-02-23
        System.out.println(extractBirthdayFromRodneCislo("530223/123"))
        // > 1953-02-23
        System.out.println(extractBirthdayFromRodneCislo("7503121234"))
        // > 1975-03-12
        System.out.println(extractBirthdayFromRodneCislo("753312/1234"))
        // > Throws an exception (invalid month)
        System.out.println(extractBirthdayFromRodneCislo("753312/12345"))
        // > Throws an exception (doesn't match pattern)
    }

    public static LocalDate extractBirthdayFromRodneCislo(String rodneCislo){
        Pattern p = Pattern.compile("^([0-9]{2})([0-9]{2})([0-9]{2})\\/?[0-9]{3}[0-9]?$");

        Matcher m = p.matcher(rodneCislo);

        if (!m.matches()){
            // Input doesn't match the pattern
            // throw a relevant exception
            // although I can't seem to do that propely with Java
        }

        int year = Integer.parseInt(m.group(1));
        if (year < 54 && !m.group(4).isEmpty()){
          year += 2000;
        }else{
          year += 1900;
        }

        int month = Integer.parseInt(m.group(2));
        if (month > 12){month -= 50;}

        int day = Integer.parseInt(m.group(3));

        return LocalDate.of(year, month, day); // will throw an exception if month or day ends up invalid
    }
}


C# calculate total time worked with TimeSpan So there is an existing codebase which calculates the total time worked per month.

What the code is doing:

iterates through all the working days entries
retreives the value of net work time which is in format: HH:mm
calculates the minutes out of the net work time string
formats the net work time to the report day row (back again)
adds the daily calculated minutes to monthly total
formats the total monthly minutes to the report totals row

Problem

The code randomly calculates the data properly.
When data is calculated properly:

the total is calculated properly,
the day rows adds up to the total value

When data is not properly calculated:

the total has always 1h less
every entry which does not have minutes part has 1h less which means that it does not even adds up to the wrongly calculated total

To resolve the problem we obviously need to get rid of type conversion from double (ts.TotalHours) to int

But the questions are:

Why the calculations are wrong randomly (or this is just my impression)?
How to mimic the wrong calculation behavour in unit tests?

Based on this SO thread: https://stackoverflow.com/q/47302758/1498401 it seems that floating point calculation might be platform specific so additional information about server configuration:

There are 2 fronted servers with win 2k12r2

static int totalMinutesCounter { get; set; }

public static string GetFormatedTime(string s, out int minutesCounter)
{
    minutesCounter = 0;

    if (s == null)
    {
        return string.Empty;
    }

    int workedTimeMinutes = GetNumberOfMinutes(s);

    if (workedTimeMinutes < 0)
    {
        throw new ArgumentOutOfRangeException("workedTimeHours", "Negative values not allowed");
    }

    minutesCounter = workedTimeMinutes;

    TimeSpan ts = new TimeSpan(0, workedTimeMinutes, 0);

    return ((int)ts.TotalHours).ToString() + ts.ToString(@"\:mm");
}

static string GetFormatedTimeFromMinutes(int minutes, ref int minutesCounter)
{
    TimeSpan ts = new TimeSpan(0, minutes, 0);

    minutesCounter += minutes;

    return ((int)ts.TotalHours).ToString() + ts.ToString(@"\:mm");
}

private static int GetNumberOfMinutes(string s)
{
    if (string.IsNullOrWhiteSpace(s))
    {
        return 0;
    }

    char[] charSeparators = new char[] { ':' };
    string[] result = s.Split(charSeparators, StringSplitOptions.None);
    string sh = result[0].ToString();
    string sm = result[1].ToString();
    int hh = int.Parse(sh);
    int mm = int.Parse(sm);

    return mm + 60 * hh;
}

My unit test i would like to see failing ;)

[TestMethod]
public void TestMethod1()
{
    List<string> times = new List<string>()
    {
        "11:30","12:00","08:00","09:30","07:00","11:00","09:30","11:00","11:30","07:00","08:00","09:30","10:30","09:00","09:45","08:00","10:15","11:00","10:00"
    };

    for (int i = 0; i < 10000; i++)
    {
        foreach (var s in times)
        {
            int minutesCounter = 0;

            var rowValue = GetFormatedTime(s, out minutesCounter);

            totalMinutesCounter += minutesCounter;
        }

        int temp = 0;
        var result = GetFormatedTimeFromMinutes(totalMinutesCounter, ref temp);
        totalMinutesCounter = 0;

        Assert.AreEqual("184:00", result);
    }
}
```

Why does your code pointlessly reinvent the wheel? Microsoft provides methods to convert strings to DateTimes; why not use those instead of your own clunky GetNumberOfMinutes? Or perhaps even better: convert the string directly to a TimeSpan.

On the subject of GetNumberOfMinutes: why even do string sh = result[0].ToString();? The result of spring.Split is an array of strings, why then the need to convert a string to a string? And that's just one of the things that are wrong with that method, there are several more.

The whole flow of your code is IMHO needlessly complex. Why not simply convert each time to a TimeSpan, thus getting a List<TimeSpan>, then calculate the amount of minutes, add those up, and then convert those to a "readable" format?

I don't even understand why you do this: ((int)ts.TotalHours).ToString() + ts.ToString(@"\:mm");. Surely this can be expressed much more elegantly and readable.

On a general note: use proper, self-explanatory names for your variables, and not ts or sh etc.


Testing http routes in go I have never done testing, and I have 5 end points which have methods such as GET, POST, and DELETE to test. I wrote test cases using the go's in built testing package. I'm worried that I'm missing some cases which are not striking to my mind. For example: I have an end point which goes like this:

a.Router.HandleFunc("/1/sig/id/{id:[0-9]+}", a.sigHandler).Methods("GET", "POST", "DELETE")

And the Test case goes like this:

func TestSigHandler(t *testing.T){

    test_cases := []string{"2021205"}
    // GET Testing
    for _, method := range test_cases{
        usersUrl = fmt.Sprintf("%s/1/sig/id/%s", server.URL, method) //Grab the address for the API endpoint
        request, err := http.NewRequest("GET", usersUrl, nil)
        res, err := http.DefaultClient.Do(request)

        if err != nil {
            t.Error(err) //Something is wrong while sending request
        }

        if res.StatusCode != 200  {
            t.Errorf("Page not found : ", res.StatusCode) //Uh-oh this means our test failed
        }
    }

    // POST Testing
        sig := []byte( `{
        "raw": "a new sig"
    }`)
        usersUrl = fmt.Sprintf("%s/1/sig/id/2021205", server.URL) //Grab the address for the API endpoint
        request, err := http.NewRequest("POST", usersUrl, bytes.NewBuffer(sig))
        if err != nil{
            t.Error(err)
        }
        request.Header.Set("Content-Type", "application/json")
        res, err := http.DefaultClient.Do(request)

        if err != nil {
            t.Error(err) //Something is wrong while sending request
        }

        if res.StatusCode != 200  {
            t.Errorf(" Internal Server Error: ", res.StatusCode) //Uh-oh this means our test failed
        }

        // DELETE Testing

        sigs_delete_cases := []string{ "1000345"}
        for _, sig_to_be_deleted := range sigs_delete_cases{
            usersUrl = fmt.Sprintf("%s/1/sig/id/%s", server.URL, sig_to_be_deleted) //Grab the address for the API endpoint
            request, err := http.NewRequest("DELETE", usersUrl, nil)
            res, err := http.DefaultClient.Do(request)

            if err != nil {
                t.Error(err) //Something is wrong while sending request
            }

            if res.StatusCode != 200  {
                t.Errorf("Tried to delete a reserved Id : ", res.StatusCode) //Uh-oh this means our test failed
            }
        }

}

The problem is that, most of my test cases follow similar pattern where I post data in different formats and checking the response codes. I strongly feel like I'm missing something that will break my API when I push it to prod. I need some insight on testing these routes so that I can be confident to push the api to prod.

And my main test function goes like this:

func TestMain(m *testing.M) {
    server = httptest.NewServer(a.InitializeRouter())
    a.InitializeDB(fmt.Sprintf("postgres://****:****@localhost/db?sslmode=disable", os.Getenv("POSTGRES_URL")))
    code := m.Run()
    if code == 0 {
        fmt.Println("Success!")
    } else {
        fmt.Println("One or more Test Cases have failed! Please use  go test -v  for more details")
    }

    os.Exit(code)
}

Using the tips of the justforfunc #16 video, you could make your tests using a table (tt = testing table) and then iterate on it (tc = test case).

Your test could then look like this:

func TestSigHandler(t *testing.T) {
    jsonHeaders := map[string]string{"Content-Type": "application/json"}

    tt := []struct {
        name    string
        method  string
        uri     string
        payload []byte
        headers map[string]string
    }{
        {"get item", "GET", "/1/sig/id/2021205", nil, nil},
        {"post new item", "POST", "/1/sig/id/2021205", []byte(`{"raw": "a new sig"}`), jsonHeaders},
        {"delete item", "DELETE", "/1/sig/id/1000345", nil, nil},
    }

    for _, tc := range tt {
        t.Run(tc.name, func(t *testing.T) {
            var payload io.Reader
            if tc.payload != nil {
                payload = bytes.NewBuffer(tc.payload)
            }
            req, err := http.NewRequest(tc.method, server.URL+tc.uri, payload)
            if err != nil {
                t.Fatalf("could not create request: %v", err)
            }

            for key, value := range tc.headers {
                req.Header.Set(key, value)
            }

            res, err := http.DefaultClient.Do(req)
            if err != nil {
                t.Fatalf("could not send request: %v", err)
            }

            if res.StatusCode != http.StatusOK {
                t.Fatalf("expected status OK; got %v", res.Status)
            }
        })
    }
}

Looking at your TestMain, I have the feeling that you are doing only integration tests (from the request to the database).

You should split you tests to the individual layers (the current one if for routing: it could use a mocked DB).

With this approach, the TestMain could be deleted (do only the setup that is necessary for each test).


Java Temperature Converter I have made a program that converts Celsius to Fahrenheit and Fahrenheit to Celsius. How is my code?
import java.util.Scanner;
public class TemperatureConversion {
    public static void main(String[] args) {
        double fahrenheit, celsius;
        Scanner input = new Scanner(System.in);
        //  Celsius to Fahrenheit \u2103 degree Celsius symbol
        System.out.println("Input temperature (\u2103): ");
        celsius = input.nextDouble();
        System.out.println(celsius * 1.8 + 32 + " \u2109 \n");

        // Fahrenheit to Celsius \u2109 degree Fahrenheit symbol
        System.out.println("Input temperature (\u2109: ");
        fahrenheit = input.nextDouble();
        System.out.print((fahrenheit - 32) / 1.8 + " \u2103");
    }
}

It solves the problem, and that's good. Period.
But you're presenting your code here to get advice how to improve your programming skills, so here are a few remarks. Take them as a hint where to go from here, as soon as you feel ready for the "next level".
Separate user interaction from computation
Your main method contains both aspects in one piece of code, even within the same line (e.g. System.out.println(celsius * 1.8 + 32 + " \u2109 \n");). Make it a habit to separate tasks that can be named individually into methods of their own:

input a Celsius value (a method double readCelsius())
input a Fahrenheit value (a method double readFahrenheit())
convert from Celsius to Fahrenheit (a method double toFahrenheit(double celsius))
convert from Fahrenheit to Celsius (a method double toCelsius(double fahrenheit))
output a Fahrenheit value (a method void printFahrenheit(double fahrenheit))
output a Celsius value (a method void printCelsius(double celsius))

With separate methods, it will be easier to later change your program to e.g. use a window system, or do bulk conversions of many values at once, etc.
More flexible workflow
Your current program always forces the user to do exactly one C-to-F and then one F-to-C conversion. This will rarely both be needed at the same time. I'd either ask the user at the beginning for the conversion he wants, or make it two different programs. By the way, doing so will be easier if the different tasks have been separated.
Minor hints
Combine variable declaration and value assignment. You wrote double fahrenheit, celsius; and later e.g. celsius = input.nextDouble();, I'd combine that to read double celsius = input.nextDouble();. This way, when reading your program (maybe half a year later), you immediately see at the same place that celsius is a double number coming from some input.
I'd avoid the special characters like \u2109 and instead write two simple characters °F. Not all fonts on your computer will know how to show that \u2109 Fahrenheit symbol, so this might end up as some ugly-looking question mark inside a rectangle or similar. Using two separate characters does the job in a more robust way.


Caching all the prepared statements in thread safe way I have a below Singleton class where in my getStatement method, I populate a CHM by doing if check.

public class CacheHolder {
  private static final Map<String, PreparedStatement> holder = new ConcurrentHashMap<>();

  private static class Holder {
    private static final CacheHolder INSTANCE = new CacheHolder();
  }

  public static CacheHolder getInstance() {
    return Holder.INSTANCE;
  }

  private CacheHolder() {}

  public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps = holder.get(cql);
    if(ps == null) { // If "ps" is already present in cache, then we don't have to synchronize and make threads wait.
        synchronized {
          ps = holder.get(cql);
          if (ps == null) {
            ps = session.prepare(cql);
            holder.put(cql, ps);
          }
        }
    }
    return ps.bind();
  }
}

I am working with Cassandra and using datastax java driver so I have to re-use prepared statements and that's why I am caching it here. Prepared Statement and BoundStatement.

Is there any better way of making my getStatement method thread safe instead of using synchronized block like that? Any other data structure which might be thread safe for these kind of operations?

I am working with Java 7.

public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps = holder.computeIfAbsent(cql, s -> session.prepare(s));
    return ps.bind();
}

According to computeIfAbsent's Javadoc:

  The entire method invocation is performed atomically, so the function is applied at most once per key.

I also suggest that you declare holder of type ConcurrentHashMap instead of Map so that it becomes unambiguous for anyone reading getStatement that the synchronisation in computeIfAbsent is guaranteed.


Breaking a 32-bit value into bytes for an ArrayList I am very new to Java and its collections and I'm trying to figure out "best" way to populate an ArrayList<Byte> in Java.  In particular, I'm trying to take a 32 bit number, divide it into bytes, and put it into big-endian order into the list.  I'm not particularly interested in hyper-optimizing in this case; I'd rather the code be clean and readable.

I've got a couple issues that are obvious to me, but the biggest one is that I hate the fact that I am initializing the list with bogus values before I update them.  But my choices seem to be that, or adding the elements in ascending order, which seems to make the code to extract the bytes significantly more complex.

Am I missing a better solution?

public void Add32BitValue(int value) {
    // Adding 4 elements first to the list, so that
    // I can modify them so they'll be in big endian
    // order.
    // (The code to directly add them in big endian
    // order seems much more complex.)
    for (int i=0; i<4; i++) {
        list.add((byte)0);
    }

    // Now that the elements are in the arraylist, they
    // can be set.
    for (int i = 3; i >= 0; i--) {
        list.set(list.size() + i - 4, (byte)value);
        value = value >> 8;
    }
}

Assuming your list was defined elsewhere, you could append the bytes of value as follows.

for(int i = 3; i >= 0; i--) {
    list.add((byte)((value >>> (i*8)) & 0xFF));
}

It avoids pre-populating the arraylist
It avoids resetting the byte already stored
It avoids storing the shifted value back in value
It uses a logical right shift to avoid sign extension of negative integers


Simplify SQL query that utilizes GROUP BY and HAVING BY and uses aggregation I am taking Stanford's Introduction to Databases Self-Paced online course. I have gone through the videos in the SQL mini-course, and I am having trouble completing the exercises.

The following is the question from the SQL Movie-Rating Query Exercises, Question 6:

  For all cases where the same reviewer rated the same movie twice and
  gave it a higher rating the second time, return the reviewer's name
  and the title of the movie.

The database can be found here.

My answer to this question is very complex; so much so that I the length of my query exceeds the query character limit in class. I will post my solution in steps, since I think that might help surface some of my conceptual misunderstandings.

Step 1:Give me all the movies where the same reviewer reviewed the same movie twice

SELECT Rating.rID, Rating.mID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1;

Step 2: Let's join the three tables so that all the information is in one place:

SELECT Movie.mID, Movie.title, Movie.year, Movie.director, Rating.rID,
       Rating.stars, Rating.ratingDate, Reviewer.name
FROM Movie, Rating, Reviewer
WHERE Movie.mID = Rating.mID and Rating.rID = Reviewer.rID;

Step 3: Filter the result set in Step 2 so that it only contains tuples that are also in Step 1

SELECT *
FROM
(SELECT Movie.mID, Movie.title, Movie.year, Movie.director, Rating.rID,
       Rating.stars, Rating.ratingDate, Reviewer.name
FROM Movie, Rating, Reviewer
WHERE Movie.mID = Rating.mID and Rating.rID = Reviewer.rID) compiled
WHERE compiled.mID in
(SELECT Rating.mID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1) and compiled.rID in (SELECT Rating.rID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1);

Step 4: Return the result set from Step 3 twice so that we can compare them the rating, reviewer, movie, stars and date

SELECT view_one.name, view_one.title
FROM (SELECT *
FROM
(SELECT Movie.mID, Movie.title, Movie.year, Movie.director, Rating.rID,
       Rating.stars, Rating.ratingDate, Reviewer.name
FROM Movie, Rating, Reviewer
WHERE Movie.mID = Rating.mID and Rating.rID = Reviewer.rID) compiled
WHERE compiled.mID in
(SELECT Rating.mID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1) and compiled.rID in (SELECT Rating.rID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1)) view_one, (SELECT *
FROM
(SELECT Movie.mID, Movie.title, Movie.year, Movie.director, Rating.rID,
       Rating.stars, Rating.ratingDate, Reviewer.name
FROM Movie, Rating, Reviewer
WHERE Movie.mID = Rating.mID and Rating.rID = Reviewer.rID) compiled
WHERE compiled.mID in
(SELECT Rating.mID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1) and compiled.rID in (SELECT Rating.rID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1)) view_two
WHERE view_one.rID = view_two.rID and view_one.mID = view_two.mID and
view_one.ratingDate < view_two.ratingDate and view_one.stars < view_two.stars;

This seems like a very tortured query, and it seems to me like I am missing some important concepts. Can someone help me refactor this query?

SELECT Rating.rID, Rating.mID
FROM Rating
GROUP BY Rating.rID, Rating.mID
HAVING count(*) > 1;

This returns all ratings where the same person reviewed the same movie more than once, but that's not what we want.  We want all the ratings where the same person reviewed the same movie twice, giving it a higher rating the second time.

SELECT DISTINCT S2.rID, S2.mID
FROM Rating S1 INNER JOIN Rating S2 ON S1.rID = S2.rID AND S1.mID = S2.mID
WHERE S1.ratingDate > S2.ratingDate AND S1.stars > S2.stars;

Now let's look at your step 2:

SELECT Movie.mID, Movie.title, Movie.year, Movie.director, Rating.rID,
       Rating.stars, Rating.ratingDate, Reviewer.name
FROM Movie, Rating, Reviewer
WHERE Movie.mID = Rating.mID and Rating.rID = Reviewer.rID;

Why do we need so many columns?  The assignment is to return the movie title and reviewer name, so let's just do that:

SELECT Movie.title, Reviewer.name
FROM Movie
  INNER JOIN Rating S ON Movie.mID = S.mID
  INNER JOIN Reviewer ON S.rID = Reviewer.rID;

So we can finish with a new step 3:

SELECT Movie.title, Reviewer.name
FROM Movie
  INNER JOIN (
    SELECT DISTINCT S2.rID, S2.mID
    FROM Rating S1 INNER JOIN Rating S2 ON S1.rID = S2.rID AND S1.mID = S2.mID
    WHERE S1.ratingDate > S2.ratingDate AND S1.stars > S2.stars
             ) S ON Movie.mID = S.mID
  INNER JOIN Reviewer ON S.rID = Reviewer.rID;

This uses a single subselect to replace Rating from step 2 with a new version that only includes those Reviewer/Movie pairs where the second rating was higher than the first.


Calculating count of square numbers between two numbers The problem is to find out the squares between two numbers, inclusive of the numbers. The two numbers are in the range between 1 and 109.

long numberOne = in.nextLong();
            long numberTwo = in.nextLong();
            int count=0;
            for(long j=numberOne;j<=numberTwo;j++){
                double numSquareRoot=Math.sqrt(j);
                double numFloor=Math.floor(numSquareRoot);
                if(numSquareRoot == numFloor) count++;
            }

What changes can be made to work efficiently on large numbers?

Can be done in \$O(1)\$ time

The count should be:

\$\lfloor{\sqrt n}\rfloor -\lceil{\sqrt m}\rceil + 1\$

or in terms of your program:

return Math.floor(Math.sqrt(numberTwo)) - Math.ceil(Math.sqrt(numberOne)) + 1;


Extension method to list enum values I have the following enum extension method:

public static IList<SelectListItem> SelectListFor<T>()
{
  Type enumType = typeof(T);

  if (enumType.IsEnum)
  {
    return Enum.GetValues(enumType)
    .Cast<int>()
    .Where(i => !i.Equals(0))
    .Select(e => new SelectListItem()
            {
            Value = e.ToString(),
      Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
    })
    .ToList();
  }

  return null;
}

I can call this by doing

EnumExtensions.SelectListFor<BrochureTypes>()

And it will allow me to create a drop down list for the values in my BrochureTypes enum

However, when I compile the code, I get the following warning:

  CA1004    Consider a design where 'EnumExtensions.SelectListFor()' doesn't require explicit type parameter 'T' in any call to it.

Can I somehow change the above code to overcome this warning? Or should I just suppress the warning?

As written in the relative MSDN page, the cause of the warning is

  The parameter signature of an externally visible generic method does not contain types that correspond to all the type parameters of the method.

Looking at your code, you get the type of the generic param into the method body (which can be seen also as a violation of the Separation of Concerns principle). In order to solve the issue, you can pass the type as a parameter. The resulting code should be something like the following:

public static IList<SelectListItem> SelectListFor(Type enumType)
{
    if (enumType.IsEnum)
    {
        return Enum.GetValues(enumType)
            .Cast<int>()
            .Where(i => !i.Equals(0))
            .Select(e => new SelectListItem()
            {
                Value = e.ToString(),
                Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
            })
            .ToList();
    }

    return null;
}

and can be called in the following way:

EnumExtensions.SelectListFor(typeof(BrochureTypes))

As an alternative, you can make it an extension method of the Type class, and I'd suggest a rename of the method in this case:

public static IList<SelectListItem> GetSelectList(this Type enumType)
{
    if (enumType.IsEnum)
    {
        return Enum.GetValues(enumType)
            .Cast<int>()
            .Where(i => !i.Equals(0))
            .Select(e => new SelectListItem()
            {
                Value = e.ToString(),
                Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
            })
            .ToList();
    }

    return null;
}

and call it like this:

(typeof(BrochureTypes)).GetSelectList();

or:

enumInstance.GetType().GetSelectList();


Curry function with function constructor Shall I make a curry function this way, or is it best to avoid Function constructor in all means? What I want to know is if this is "bad practice" and how else currying can be done. For instance, one can bind arguments to a function with Function.bind, but it is more like a partial application thing. And will doing currying by recursion impact on performance?

var curry = function(fn) {
    var fnString = fn.toString();
    var arity = fn.length;
    var paramList = fnString.substring(
            fnString.indexOf("(") + 1, fnString.indexOf(")")).split(",");
    var body = fnString.substring(
            fnString.indexOf("return"), fnString.indexOf("}"));
    var newBody = paramList.slice(1).reduce(function(p,c) {
        return p + "return function(" + c + ") {";
    },"") + body + ( new Array(arity).join("}") );

    return new Function(paramList[0], newBody);

 };

var test = function(a,b,c,d) {
    return a * b * c * d;
};

var curriedTest = curry( test );
console.log( curriedTest(1)(2)(3)(4) ); \\-> 24

The problem here isn't the Function constructor in and of itself (although it's not a great idea): It's the rest of the code that'll cause trouble, I'm afraid.

You're discarding almost the entire body of the function, and retaining only the return line. And only the first return line at that.

If the test function instead looked like this:

var test = function(a,b,c,d) {
  var product = a * b * c * d;
  return product;
};

you just get an error, since the curried function becomes just:

function(a) {
  return function(b) {
    return function(c) {
      return function(d) {
        return product; // ReferenceError
      }
    }
  }
}

It'll also break if you've got nested functions, like:

var test = function(a,b,c,d) {
  function getRandomNumber() {
    return Math.random() * 10;
  }

  return a * b * c * d * getRandomNumber();
};

since your code only looks at the first return line, and thus the curried function ends up being nothing more than return Math.random() * 10.

And you can't have any close braces before a return line, so even something like this will break:

var test = function(a,b,c,d) {
  if(a == 1) {
    console.log("got: a == 1");
  }
  return a * b * c * d;
};

Since the if's } is before the return, you end up slicing the wrong stuff out of the function's body. This causes the new Function invocation to fail due to a syntax error.

Similarly, a function with no return statement will cause problems:

var test = function(a,b,c,d) {
  console.log(a * b * c * d);
};

And weird comments in the code can trip you up too:

var test = function(a,b,c,d /* more args, y'know */) { ...

Oh, and you're not propagating the context either, so within each level of nesting, this may not refer to the right/expected object.

Lastly, length is not a great way to check arity. Functions may be variadic, taking any number of arguments, whether listed or not (see below, for instance).

So, no, this is not a good way to go. Besides, the Function constructor is just eval in disguise, and should be avoided. Just use bind, or a function like below.

The usual approach is to just wrap the target function as-is. Something like:

function curry(func, args) {
  var spice = [].slice.call(arguments, 1);

  return function () {
    var args = [].slice.call(arguments);
    return func.apply(this, spice.concat(args));
  };
}

This lets you do something like:

var curried = curry(test, 1, 2, 3);
curried(4); // => 24

And it'll work for all the test function variations above.

If you do insist on rewriting code and recursively nesting things, the best I can offer (and it's not a good solution) is to not rewrite the body of the function at all.

Your current solution basically creates nested closures, meaning that the function's body can stay completely intact, only its arguments have to change. It'll just become the inner-most nested closure.

var curry = function(fn) {
  var source = fn.toString(),
      args = source.match(/\((.*?)\)/)[1].trim().split(/\s*,\s*/),
      body = source.replace(/(^.*?\{|\}[^\}]*$)/g, "");

  return args.reverse().reduce(function (nested, arg) {
    return new Function(arg, "return " + nested.toString())
  }, new Function(args.shift(), body));
};

Which produces a function like:

function anonymous(a) {
  return function anonymous(b) {
    return function anonymous(c) {
      return function anonymous(d) {
        // original function body
      }
    }
  }
}

Efficient? No. Robust? Heck no! I won't recommend using it.

But it'll work a lot better for some of the examples listed above. It'll still choke on weird comments, it doesn't do context propagation, can't handle variadics, and it's basically a big fragile hack.  But at least it'll work with local variables, nested functions, additional close braces, and missing return statements.


PHP array return array vs passing as reference I'm not sure if Code Review is the proper SE site. If not, please point me in the right direction.

Let's assume we have a simple function, where you can pass an array and it will do some work on it. For simplicity sake, let's just assume you pass an array of integers to the function and it doubles them

I was wondering what's the correct way to do this, either by passing an array, and returning that array, which is AFAIK a pass by value in PHP and finally overwriting the initial variable, or simply passing a reference to that array.

Option 1, pass by value:

public function doubleValue(array $input): array
{
    foreach ($input as $key=>$val) {
        $input[$key] += $val;
    }

    return $input;
}

// called as $foo = $someClass->doubleValue($foo);

Option 2, pass by reference:

public function doubleValue(array &$input): void
{
    foreach ($input as $key=>$val) {
        $input[$key] += $val;
    }
}

// called as $someClass->doubleValue($foo);

My assumption would be, that pass by reference has to be faster, as it doesn't need to copy the array to that function, copy it back and assign it to the variable, but I could be wrong (at least ex. the built in sort function works this way). Are there any metrics about it? What is the recommended approach?

Just never return an argument by reference. It is nearly as bad as using global. And even a memory management is not an excuse. In case your array is big, create a function that does the calculations, and then apply it to each element inside of an explicit loop.


Create a GUI to update parameters of an Object I have an Glob object which represents a glob of data. It has several parameters like size, the date it was created, who created it, and so on. I'm building a GUI to modify these Globs of data, and in my GUI is a JTextField for each of these parameters. Each JTextField has a FocusListener attached to it, which looks like:

public class GlobUpdaterGUI {

    HashMap<String, Glob> globs;

    // I know it's not great style to call main like this, but
    // it makes this snippet runnable.
    public static void main(String[] args) {
        new GlobUpdaterGUI();
    }

    public GlobUpdaterGUI() {
        initialize();
    }

    public void initialize() {
        JFrame frame = new JFrame("Glob Updater");
        frame.setMinimumSize(new Dimension(250,150));
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        JPanel panel = new JPanel(new GridLayout(4, 2));

        JTextField globNumber = new JTextField(10);

        JTextField dateTextField = new JTextField(10);
        dateTextField.addFocusListener(new FocusListener() {

            @Override public void focusGained(FocusEvent arg0) {}
            @Override
            public void focusLost(FocusEvent e) {

                /* "globNumber" lets the user select a glob from "globs". */
                Glob glob = globs.get(globNumber.getText());

                glob.date = dateTextField.getText();
            }
        });

        JTextField sizeTextField = new JTextField(10);
        sizeTextField.addFocusListener(new FocusListener() {

            @Override public void focusGained(FocusEvent arg0) {}
            @Override
            public void focusLost(FocusEvent e) {
                Glob glob = globs.get(globNumber.getText());

                glob.size = dateTextField.getText();
            }
        });

        JTextField creatorTextField = new JTextField(10);
        creatorTextField.addFocusListener(new FocusListener() {

            @Override public void focusGained(FocusEvent arg0) {}
            @Override
            public void focusLost(FocusEvent e) {
                Glob glob = globs.get(globNumber.getText());

                glob.creator = dateTextField.getText();
            }
        });

        panel.add(new JLabel("Glob Number:"));
        panel.add(globNumber);
        panel.add(new JLabel("Glob Date:"));
        panel.add(dateTextField);
        panel.add(new JLabel("Glob Size:"));
        panel.add(sizeTextField);
        panel.add(new JLabel("Glob Creator:"));
        panel.add(creatorTextField);

        frame.add(panel);
        frame.setVisible(true);
    }
}

My problem is that every FocusListener block is nearly identical to the others, but for a specific parameter.

This practice doesn't support readability, writeability, maintainability, or really any style guideline. Though there are only three blocks now, which isn't too bad, a Glob is going to end up with about a dozen parameters, so I'd like to solve this problem before heading too far down the wrong path.

I'd much rather create a new class which implements FocusListener, which takes in a JTextField (and/or whatever else it may need) and modifies its respective Glob parameter, but I don't know the proper practice if I need to modify a different variable in each one. Is there a way to generalize a Listener so that I don't need nearly identical code littered throughout my project?

I see a couple potential issues with the code beside the duplication of code in focus listeners that you mention:

you treat the text the user typed into a text field as a key of map that contains the Gobs without checking if such key exists - what will happen when the user types a nonexisting key?
the name of the texfield that identifies the Gob (gobNumber) indicates that the gob is identified by a numerical value, yet the key of the map is a String. Either a numerical value should be used as the key of the map or the textfield could be renamed to avoid the confusion

To deal with the duplication - a basic way of generalizing the focus listeners would be to use the Template method pattern. You could extract the code that would be duplicated for each listener to an abstract class and then delegate the parts that would be different to a specific subclass.

public abstract class UpdateGobFocusListener implements FocusListener
{
    private final Map<String, Glob> globs;
    private final JTextField globNumberTextField;

    public UpdateGobFocusListener(Map<String, Glob> globs, JTextField globNumberTextField)
    {
        this.globs = globs;
        this.globNumberTextField = globNumberTextField;
    }

    public void focusGained(FocusEvent e)
    {

    }

    public void focusLost(FocusEvent e)
    {
        Glob glob = globs.get(globNumberTextField.getText());
        updateGlob(glob);
    }

    protected abstract void updateGlob(Glob glob);

}

And an example of a subclass:

public class DateGlobUpdater extends UpdateGobFocusListener
{

    private final JTextField dateTextField;

    public DateGlobUpdater(JTextField dateTextField, Map<String, Glob> globs, JTextField globNumberTextField)
    {
        super(globs, globNumberTextField);
        this.dateTextField = dateTextField;
    }

    @Override
    protected void updateGlob(Glob glob)
    {
        glob.date = dateTextField.getText();
    }

}

Alternatively, if you prefer Composition over inheritance, you could extract the duplicated code to a general focus listener, create a GobUpdater interface and then move the different parts to a separate implementation of the interface each. The general focus listener could be injected with the specific implementation through a constructor.


Handling related generic type parameters In answering a question on StackOverflow, it occurred to me that a more generic version of GroupBy for C# that delegated membership in each group to a class could be useful.

This is my attempt, but I am unsatisfied with a few things. I don't particularly like the need to constrain the element type to IOrderable - I feel like it should be possible to delegate ordering to the group type somehow (I really wanted a static method constraint in the IGGrouping interface).

public interface IOrderable<T> {
    double ordering();
}

public interface IGGrouping<T> : IEnumerable<T> where T : IOrderable<T> {
    bool BelongsToGroup(T aT);
    IGGrouping<T> Add(T aT);
}

public static class IEnumerableExt {
    public static IEnumerable<IGGrouping<T>> GroupBy<TG,T>(this IEnumerable<T> src) where TG : IGGrouping<T>, new() where T : IOrderable<T> {
        var groups = new List<IGGrouping<T>>();
        void Add(T aT) {
            var found = false;
            foreach (var g in groups) {
                found = g.BelongsToGroup(aT);
                if (found) {
                    g.Add(aT);
                    break;
                }
            }
            if (!found)
                groups.Add(new TG().Add(aT));
        }

        foreach (var s in src.OrderBy(s => s.ordering()))
            Add(s);

        return groups;
    }
}

Here's a sample class to group by distance from center of current groups:

public class gRectangleGroup : IGGrouping<Rectangle> {
    List<Rectangle> members;
    public Point center;

    public gRectangleGroup() {
        members = new List<Rectangle>();
    }

    public gRectangleGroup Add(Rectangle r) {
        members.Add(r);
        center = new Point(members.Average(m => m.Loc.X), members.Average(m => m.Loc.Y));
        return this;
    }

    public bool BelongsToGroup(Rectangle r) => center.Distance(r.Loc) <= 5;

    public Rectangle Middle() => members.OrderBy(m => m.Loc.Distance(center)).First();

    public IEnumerator<Rectangle> GetEnumerator() => members.GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

    IGGrouping<Rectangle> IGGrouping<Rectangle>.Add(Rectangle aT) => Add(aT);
}

You use gRectangleGroup like so:

List<Rectangle> list;

var ans2 = list.GroupBy<gRectangleGroup, Rectangle>().Cast<gRectangleGroup>().Select(rg => rg.Middle());

I don't like having to list the group type and the element type explicitly in the call to GroupBy. I don't like the Cast to gRectangleGroup when the type is passed to the GroupBy, but if GroupBy returns TG then I have to give up the fluent Add call for new groups.

I also don't like how a class that implements IGGrouping (like gRectangleGroup) and has a fluent Add that returns this, must also have another Add to return IGGrouping<T> because of the interface.

Can anyone suggest how this might be improved, or what they would change to solve the same issue?

You don't need the type parameter on IOrderable:

  public interface IOrderable
  {
    double Ordering();
  }

I don't like that you sort the source vector without notifying the client. In this way mysterious things are going on and the method does more than one thing. At least you should have a flag argument that lets the client determine if the source should be sorted or not before grouping:

public static IEnumerable<IGGrouping<T>> GroupBy<TG, T>(this IEnumerable<T> src, bool sortBeforeGrouping = false) where TG : IGGrouping<T>, new() where T : IOrderable
{
  var groups = new List<IGGrouping<T>>();

  ...
}

Alternatively you should rename the method to SortAndGroupBy(...). But I would let it total up to the client if the source should be sorted or not.

I think the inner of the internal Add(...) function can be simplified a bit:

  void Add(T aT)
  {
    var group = groups.FirstOrDefault(g => g.BelongsToGroup(aT));
    if (group == null)
    {
      group = new TG();
      groups.Add(group);
    }
    group.Add(aT);
  }

On way to get rid of the custom interfaces is to let the group type implement ICollection<T> and then inject the belongsToGroup function:

public static IEnumerable<TKey> GroupBy<TKey, TElement>(this IEnumerable<TElement> source, Func<TElement, TKey, bool> belongsToGroup) where TKey : ICollection<TElement>, new()
{
  return source.Aggregate(new List<TKey>(), (groups, e) =>
  {
    TKey group = groups.FirstOrDefault(g => belongsToGroup(e, g));
    if (group == null)
    {
      group = new TKey();
      groups.Add(group);
    }
    group.Add(e);
    return groups;
  });

}

Usage

  Func<Rectangle, RectangleGroup, bool> belongsToGroup = (r, gr) => gr.BelongsToGroup(r);
  var midPoints = list.OrderBy(r => r).GroupBy(belongsToGroup).Select(gr => gr.Middle());
  foreach (var middle in midPoints)
  {
    Console.WriteLine(middle);
  }

If that isn't good enough then you can inject the addToGroup function too:

public static IEnumerable<TKey> GroupBy<TKey, TElement>(this IEnumerable<TElement> source, Func<TElement, TKey, bool> belongsToGroup, Action<TElement, TKey> addToGroup) where TKey : new()
{
  List<TKey> groups = new List<TKey>();

  foreach (TElement element in source)
  {
    TKey group = groups.FirstOrDefault(g => belongsToGroup(element, g));
    if (group == null)
    {
      group = new TKey();
      groups.Add(group);
    }

    addToGroup(element, group);
  }

  return groups;
}

Usage

  Func<Rectangle, RectangleGroup, bool> belongsToGroup = (r, gr) => gr.BelongsToGroup(r);
  Action<Rectangle, RectangleGroup> adder = (r, gr) => gr.Add(r);
  var midPoints= list.OrderBy(r => r).GroupBy(belongsToGroup, adder).Select(gr => gr.Middle());
  foreach (var middle in midPoints)
  {
    Console.WriteLine(middle);
  }


Consistent implementation of PHP class constants (arrays not allowed) I'm currently writing a class:

final class MyTestClass {
  const URL = 'http://www.example.org';
  private static $MY_ARRAY = array('...');

  // since this is not allowed
  // const MY_ARRAY = array('...');
}

So my question is whether I should make URL a private static variable, too?
These values are constant and are only used in the class itself.

See PHP Constants Containing Arrays? .  I think both are valid and also your current approach is fine. It's only important that you stick with one solution in your whole project.

Personally I think I would stay with your current solution.


C++20 Vector2D Template Pretty simple 2-Dimensional Vector template with operators and two utility functions making use of C++20 concepts. Header-only templates, function inlining and operator overloading, etc. is not exactly my strong foot, so that's why i wrote this hoping to get some feedback.
#pragma once

#include <cmath>
#include <type_traits>

namespace math {

    template<typename T>
    concept Arithmetic = std::is_arithmetic_v<T>;

    template<Arithmetic T>
    struct Vector2D
    {
        T X = 0;
        T Y = 0;

        Vector2D() = default;

        Vector2D(T x, T y);

        Vector2D(const Vector2D<T> &other);

        inline T Magnitude() const;

        inline Vector2D<T> Normal() const;
    };

    template<Arithmetic T>
    inline Vector2D<T>::Vector2D(T x, T y) : X(x), Y(y)
    {}

    template<Arithmetic T>
    inline Vector2D<T>::Vector2D(const Vector2D<T> &other) : X(other.X), Y(other.Y)
    {}

    template<Arithmetic T>
    inline T Vector2D<T>::Magnitude() const
    {
        return std::sqrt(X * X + Y * Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> Vector2D<T>::Normal() const
    {
        auto magnitude = Magnitude();

        return Vector2D<T>(X / magnitude, Y / magnitude);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &vec)
    {
        return Vector2D<T>(-vec.X, -vec.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator+(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator*(const Vector2D<T> &vec, const T factor)
    {
        return Vector2D<T>(vec.X * factor, vec.Y * factor);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator*(const T factor, const Vector2D<T> &vec)
    {
        return Vector2D<T>(factor * vec.X, factor * vec.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator/(const Vector2D<T> &vec, const T divisor)
    {
        return Vector2D<T>(vec.X / divisor, vec.Y / divisor);
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator+=(Vector2D<T> &first, const Vector2D<T> &second)
    {
        first.X += second.X;
        first.Y += second.Y;

        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator-=(Vector2D<T> &first, const Vector2D<T> &second)
    {
        first.X -= second.X;
        first.Y -= second.Y;

        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator*=(Vector2D<T> &vec, const T factor)
    {
        vec.X *= factor;
        vec.Y *= factor;

        return vec;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator/=(Vector2D<T> &vec, const T factor)
    {
        vec.X /= factor;
        vec.Y /= factor;

        return vec;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator==(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return (first.X == second.X) && (second.Y == second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator!=(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return !(first == second);
    }
}

Overview
Your code as written is perfectly fine (apart from the one bug in subtraction). Anything mentioned here is mainly to help future readers.
You overcomplicated the design by adding constructors to Vector2D.
The default constructors work perfectly and as expected in this type of situation.
There is an argument to make operators members of the class rather than free standing functions. But either work.
Your use of reference marker & is very C like. You put it next to the variable rather than next to the type. This is very subjective style thing (usually dictated by your style guide). But C++ guides skew one way while C guides skew the other.
 void doStuff(C const& param)
              ^^^^^^^^   param has a type: C const&

 void doStuff(C& param)
              ^^         param has a type: C&

This has happened because in C++ types are much more significant and important. So we pay much more attention to type things in C++.
Code Review
I am going to have to take your word that this is how concepts work. I look forward to this becoming available more consistently.
    template<typename T>
    concept Arithmetic = std::is_arithmetic_v<T>;

    template<Arithmetic T>
    struct Vector2D
    {

Sure but not needed if you don't define other constructors (see below).
        Vector2D() = default;

Sure but not needed as the brace initializers works and does this for you:
        Vector2D(T x, T y);

Sure: But not needed the default copy constructor should work here.
        Vector2D(const Vector2D<T> &other);

Don't need to declare these inline here. It has no effect. You put the inline next to the method definition to make sure that the compiler does not give you multiple definition warnings.
        inline T Magnitude() const;
        inline Vector2D<T> Normal() const;
    };

I would note that the inline keyword has nothing to do with in-lining the code. The compiler decides that without any help from the engineer (because it is better at than you).
I would have simplified that to:
    template<Arithmetic T>
    struct Vector2D
    {
        T  x;
        T  y;

        T Magnitude() const;
        Vector2D<T> Normal() const;
    };

Arithmetic Operators:
There is a debate about if these should be members or free standing functions. Either is fine. I will not ding you for doing it one way when I would have done it another.
There is an argument for making them free standing functions. But this is usually based around symmetry when using the operators.
   R = X + Y;

If you use methods:
If X is Vector2D and Y is not then the compiler can potentially convert Y into Vector2D and still apply the operation. But if it is the other way around: Y is Vector2D and X is not then the compiler can not convert X to a Vector2D.
If you use free standing functions:
Then if one parameter is Vector2D then the other can be converted to Vector2D and the function applied. This you have symmetrical conversions.
This is fine if the types are "Arithmetic" in nature and can easily convert from anything into your type; this is not the case here with Vector2D. So this argument does not stand. I would also argue that this applies to most types; you have to have a very good argument that your type should allow auto conversion (most types don't and use explicit on one argument constructors to prevent this).
Thus here you can use either technique and both are fine.
I personally would have made them members of the class (the sole reason is to add clarity so you can read the class and see all the things you can do to change them). You could argue you can have the same affect by adding all the declarations for arithmetic operations as friends to the class (or simply put the declarations directly under the class (and I could not argue against his).
Whatever way you decide. I would put the declarations beside the class and away from the definition so it is easy to see all the possible operations that apply to the class.

Normally we implement operator+ in terms of operator+=.
    template<Arithmetic T>
    inline Vector2D<T> operator+(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

I would write like this:
    template<Arithmetic T>
    inline Vector2D<T>& operator+=(Vector2D<T>& first, Vector2D<T> const& second)
    {
        first.X += second.X;
        first.Y += second.Y;
        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> operator+(Vector2D<T> first, Vector2D<T> const& second)
    {
        // Note Pass first by value.
        // This automatically gets us a new version of the object.
        // The compiler will be able to detect if we can use move/copy
        // automatically to get this move version.
        //
        // We can then do the += on this new copy.
        // and perfectly return this value as output.
        return first += second;
    }

This seems like a bug:
    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        // Should you not subtract here:
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }


Managing player data Here is a code example from my name, where I use a MySQL db and store players data.

In this code I'm using a DataReader when loading and saving the data in and out. Although there's only 10 fields, I have reduced the number for this example. I plan on having 50+.

The main focus of this question is to find a more efficient way of managing players' data, a way that doesn't require so much code and hard work to load / save it.

Adding new features is a pain as I have to code a whole new set of fields to load and save, not forgetting the features.

My only idea was to make a dict and load it as [columnName] = columnValue;, cache it, grab if loaded previously and save all items on save. The problem with that was I didn't like the fact I had to explicitly call by the columns name. What if I changed it?

using MySql.Data.MySqlClient;

namespace Pitaya.Game.Players.Data
{
    internal class PlayerData
    {
        public int Id;
        public string Username;
        public int Rank;
        public string Motto;
        public PlayerOutfit Outfit;
        public PlayerGender Gender;
        public long Credits;
        public long Pixels;
        public long Diamonds;
        public bool CanChange;

        public void AssignData(MySqlDataReader reader)
        {
            while (reader.Read())
            {
                Id = reader.GetInt32("id");
                Username = reader.GetString("username");
                Rank = reader.GetInt32("rank");
                Motto = reader.GetString("motto");
                Outfit = new PlayerOutfit(reader.GetString("look"));
                Gender = reader.GetString("gender").ToUpper() == "M" ? PlayerGender.Male : PlayerGender.Female;
                Credits = reader.GetInt64("credits");
                Pixels = reader.GetInt64("activity_points");
                Diamonds = reader.GetInt64("vip_points");
                CanChange = reader.GetInt32("can_change") == 1;
            }
        }

        public void SaveData()
        {
            using (var dbConnection = Program.Server.Database.Connection)
            {
                dbConnection.SetQuery("UPDATE `users` SET `username` = @username, `rank` = @rank, `motto = @motto, `look` = @look, `gender` = @gender, `credits` = @credits, `pixels` = @pixels, `diamonds` = @diamonds, `can_change` = @canChange WHERE `id` = @id");
                dbConnection.AppendParameter("id", Id);
                dbConnection.AppendParameter("username", Username);
                dbConnection.AppendParameter("rank", Rank);
                dbConnection.AppendParameter("motto", Motto);
                dbConnection.AppendParameter("look", Outfit.FigureCode);
                dbConnection.AppendParameter("gender", Gender == PlayerGender.Male ? "M" : "F");
                dbConnection.AppendParameter("credits", Credits);
                dbConnection.AppendParameter("pixels", Pixels);
                dbConnection.AppendParameter("diamonds", Diamonds);
                dbConnection.AppendParameter("canChange", CanChange ? 1 : 0);
                dbConnection.ExecuteNonQuery();
            }
        }
    }
}

Since you have a lot of fields, I would suggest using a DataTable where each column is a field.  This not only keeps your data organized and easily accessible, but you can very efficiently write the data to a database, and display the data in a DataGridView, which also gives you ready access to methods for adding and editing the data.


Dynamically inserting html from external file and updating the database I need some suggestions on how to approach this. I want to allow users to create sections of website. The code so far is working but since I am new to back-end I feel like there is better, more elegant way to approach this. I am open to any approaches, as long as the code is kept as concise as possible.

  The user clicks "add section" button (there would be different types of sections to choose from), the html of each section will be stored in an external file
  The id of the section should be inserted into the database (alongside some other details such as user uniqid)
  The section should be inserted inside a div

At the moment this is how I have approached it:

if (isset($_POST["add_section"])) {
    $section_id = $_POST["add_section"];
    $stmt = $conn->prepare('INSERT INTO sections (user_id, section_id) VALUES (?, ?)');
    $stmt->bind_param('ss', $row['user_id'], $section_id);
    $stmt->execute();
}

The value of the button is the id of the section passed to the database.

<form method="post">
    <button class="button_1" value="navigation_ht" name="add_section>Seleccionar</button>
</form>

I want to allow the user to be able to include up to 3 sections.

$sql = "SELECT * FROM sections WHERE user_id = '{$row['user_id']}'";
$result = $conn->query($sql);

if ($result->num_rows > 0) {
    while($row = $result->fetch_assoc()) {
        include('shared/'.$row['section_id'].'.php');
    }
}

I was considering to do the following, but if the section is not created, there is error message:

$sql = "SELECT * FROM sections WHERE user_id = '{$row['user_id']}'";
$result = $conn->query($sql);
while($row = $result->fetch_assoc()) {
    include('shared/'.$row['section_id'].'.php');
    include('shared/'.$row['section_id'].'.php');
    include('shared/'.$row['section_id'].'.php');
}

This looks very clean, there is not much to improve upon.

However, there is a glaring security issue:
https://www.owasp.org/index.php/Cross-site_Scripting_(XSS)

Your code should not allow malicious JavaScript to be stored inside of the db.


Find the length of the longest consecutive elements The task:

  Given an unsorted array of integers, find the length of the longest
  consecutive elements sequence.

  For example, given [100, 4, 200, 1, 3, 2], the longest consecutive
  element sequence is [1, 2, 3, 4]. Return its length: 4.

  Your algorithm should run in O(n) complexity.

My solution:

const sample = [1, 100, 2, 90, 3, 88, 4, 9, 5];
const compare = prefix => (a,b) => prefix * (a - b);
const ascending = compare(1);

const maxObj = {
  maxResult: 0,
  maxCurrent: 1,
};

const getMax = (max, current) => current > max ? current : max;
const countConsecutive = (maxObj, _, idx, src) => {
  if ((src[idx] + 1) === src[idx + 1]) {
    maxObj.maxResult = ++maxObj.maxCurrent > maxObj.maxResult ?
        maxObj.maxCurrent : maxObj.maxResult;
  }
  return maxObj;
}
const result = sample
  .sort(ascending)
  .reduce(countConsecutive, maxObj);
console.log(result.maxResult);

Can you write the code so it is faster (and still be readable/maintainable)?

Complexity

The sort is about  \$O(n log(n))\$ depending on the JS engine so this makes your function above \$O(n)\$

Redundancies

Your code is full of redundancies.

compare and ascending can just be ascending and if you needed descending you would just swap the arguments.
The object named maxObj has redundant naming. The max prefix is not needed in the properties. If you use it for minObj would you thus need to rename the two properties?
Does the max object need the name Obj ?
You ignor the second argument of reduce and then index the array to find it. The indexing is redundant as you already have the value stored in _.
The function getMax is not used.
The () around src[idx] + 1 is not needed, the operator + has precedence over ===
You can use Math.max if only complexity is important rather than the ternary that only gives a slight performance benefit over Math.max
The line maxObj.maxResult = ++maxObj.maxCurrent > maxObj.maxResult ? maxObj.maxCurrent : maxObj.maxResult; is redundant. Each time you call it maxCurrent is always greater than maxResult. Not only is the line redundant but it makes the need for maxObj redundant as well.

Same function without the redundant code

You can thus remove most of the code, wrap it in a function, use closure to access samples to get 4 line and many fewer objects, arguments, and overhead to do the same thing. More readable and faster, (not less complex)

From about 16 lines to 4 for the same result.

const countConsecutive = arr => {
    const count = (count, val, i) => count += val + 1 === arr[i + 1] ? 1 : 0;
    return sample.sort((a, b) => a - b).reduce(count, 1);
}

However

With all that said I am not sure that the result is correct?

For [1, 100, 2, 90, 3, 88, 4, 9, 5]

  "...the longest consecutive element sequence..."

is 5 items.

That makes sense 1,2,3,4,5

But you return 7 for [1, 100, 2, 90, 3, 88, 89, 4, 9, 5]?

To me 1, 2, 3, 4, 5, 88, 89, 90 is not a "consecutive... sequence" ???

An example solution

For a \$O(n)\$ solution you need to join sequences as you find them.

Use a Set to do the lookups (if the next number exists). Iterate the set deleting numbers as you go lets you only check values you have not encountered.

Counting forwards sequences as you find them and storing the result in a map. When the next in a sequence is in the map add to the sequence length.

That brings the solution to \$O(2n + m)\$ The \$2n\$ as one pass is needed to create the Set and where \$m\$ is the number of broken sequences that at max is \$n\$. So \$O(3n)\$ is the same as \$O(n)\$

There is room for performance improvement as you can do some of the counting as you create the Set

const longestSequence = (arr) => {
    const numbers = new Set(arr), counts = {};
    var max = 1;
    for (const num of numbers.values()) {
        let counting = true, next = num + 1;
        numbers.delete(num);
        while (counting) {
            counting = false;
            while (numbers.has(next)) { numbers.delete(next++) }
            if (counts[next]) { counting = numbers.has(next += counts[next]) }
        }
        max = Math.max(counts[num] = next - num, max);
    }
    return max;
}


Using ::CreateToolhelp32Snapshot I have a process, let's call it Proc1. It is installed in two different directories and one instance of each is running:

C:\DirA\Proc1.exe
C:\DirB\Proc1.exe

When iterating through all currently running processes, I need to be able to differentiate one from the other. I do this by looking at the full path to the process.

Unfortunately, a little piece of documentation from MSDN has forced me to make this code pretty ugly:

MSDN

  If the function fails with ERROR_BAD_LENGTH when called with TH32CS_SNAPMODULE or TH32CS_SNAPMODULE32, call the function again until it succeeds.

Please assume that C++11 is not available for this project.

bool MyClass::IsCorrectPath (const DWORD dwPid, const std::string &strPath) const
{
    DWORD dwError = 0 ;
    NormalHandle hModSnap ; // RAII wrapper for HANDLE

    do {
        hModSnap = ::CreateToolhelp32Snapshot (TH32CS_SNAPMODULE, dwPid) ;
        if (hModSnap == INVALID_HANDLE_VALUE) {
            dwError == ::GetLastError () ;
            if (dwError == ERROR_PARTIAL_COPY) {
                // This occurs when trying to read a 64 bit process from a 32 bit process.
                return false ;
            }

            else if (dwError == ERROR_BAD_LENGTH) {
                // We have to repeatedly call ::CreateToolhelp32Snapshot () if we get this error
                continue ;
            }

            else {
                ThrowFunctionFailure ("MyClass::IsCorrectPath ()",
                    "::CreateToolhelp32Snapshot ()", ::GetLastError ()) ;
            }
        }

    } while ((hModSnap == INVALID_HANDLE_VALUE) && (dwError == ERROR_BAD_LENGTH)) ;

    MODULEENTRY32 me ;
    me.dwSize = sizeof (me) ;
    if (::Module32First (hModSnap.Get (), &me) == FALSE) {
        ThrowFunctionFailure ("MyClass::IsCorrectPath ()", "::Module32First ()", ::GetLastError ()) ;
    }

    return (::_stricmp (strPath.data (), me.szExePath) == 0) ;
}

I think you don't need to check for errors twice, and that will make your code cleaner.

bool MyClass::IsCorrectPath (const DWORD dwPid, const std::string &strPath) const
{
    DWORD dwError = 0 ;
    NormalHandle hModSnap ; // RAII wrapper for HANDLE
    ::SetLastError (0) ;

    do {
        // repeat while the expected error occurs
        hModSnap = ::CreateToolhelp32Snapshot (TH32CS_SNAPMODULE, dwPid) ;
        dwError == ::GetLastError () ;
    } while ((hModSnap == INVALID_HANDLE_VALUE) && (dwError == ERROR_BAD_LENGTH)) ;

    if (dwError == ERROR_PARTIAL_COPY) {
         // This occurs when trying to read a 64 bit process from a 32 bit process.
         return false ;
    }

    if (hModSnap == INVALID_HANDLE_VALUE) {
        ThrowFunctionFailure ("MyClass::IsCorrectPath ()",
            "::CreateToolhelp32Snapshot ()", ::GetLastError ()) ;
    }

    MODULEENTRY32 me ;
    me.dwSize = sizeof (me) ;
    if (::Module32First (hModSnap.Get (), &me) == FALSE) {
        ThrowFunctionFailure ("MyClass::IsCorrectPath ()", "::Module32First ()", ::GetLastError ()) ;
    }

    return (::_stricmp (strPath.data (), me.szExePath) == 0) ;
}

So, instead of checking for all errors in every step of the loop, only check for the expected error.

Once you're out of the loop, it means you received a different error or you have a valid handle. Check if success and continue.

Does that make sense to you?

edit: Changed dwError == ERROR_SUCCESS to dModSnap == INVALID_HANDLE_VALUE.


DAOs for Person, Employee, Staff, etc I have an abstract PersonDao class, extended by several other DAO classes, for example EmployeeDao, StaffDao.

The non-abstract DAOs correspond to different tables. The tables have some common fields, and also some unique fields. Operations on the common fields are implemented in the abstract parent PersonDao class, operations on the unique fields are implemented in the specialized DAOs.

The parent DAO looks like this:

public abstract class PersonDao {

    private SimpleJdbcTemplate simpleJdbcTemplate;

    public PersonDao() {
        final String tableName = getTableName();
        fieldNamesCSV = getFieldNames();

        String select = String.format("SELECT %s FROM %s ", fieldNamesCSV, getTableName());
    }

    abstract String getTableName();

    abstract String getHistoricalTableName();

    public void setDataSource(DataSource dataSource) {
        this.simpleJdbcTemplate = new SimpleJdbcTemplate(dataSource);
    }

    // more methods, common to all persons
}

An example specialized DAO looks like this:

public class EmployeeDao extends PersonDao {

    private static final String TABLE_NAME = "EMPLOYEE";
    private static final String HISTORICAL_TABLE_NAME = "EMPLOYEE_HIST";

    @Qualifier("employeeDS")
    @Override
    public void setDataSource(DataSource dataSource) {
        super.setDataSource(dataSource);
    }

    @Override
    String getTableName() {
        return TABLE_NAME;
    }

    @Override
    String getHistoricalTableName() {
        return HISTO_TABLE_NAME;
    }

    // more methods, unique to employees
}

My question is about the code duplication in the specialized DAOs. They all have the boilerplate code of setting the data source (configured by Spring), and the overridden methods for the table names, so that the common methods in the parent class work with any table. Code quality analysis tools like Sonar detect and flag an error for these blocks.

Do you suspect something wrong with my design that necessitates the boilerplate code? If so, please give some tips to improve it.
If the design looks legit, do you see a way to reduce the duplication? Moving the table name constants to the methods as hard-coded strings would fool the code analysis tool, but that's hardly the solution.
Any other tips come to mind?

UPDATE

As @pinoniq pointed out, the setDataSource was smelly. I vaguely remembered that Spring forced me to do it that way, and differentiate the specialized classes with the @Qualifier annotation. It seems that's not the case, I could move data source to the constructor of the specialized classes, and everything seems to work well.

Smelly code

The parent method smells:

public void setDataSource(DataSource dataSource) {
    this.simpleJdbcTemplate = new SimpleJdbcTemplate(dataSource);
}

You ask a DataSource, but in fact you need a SimpleJdbcTemplate. this smells...
Let's assume I need to test your code. I run my unit tests and pass in a DataSource, It somehow throws this really weird exception class SimpleJdbcTemplate not found.

This smells because now, your DAO classes are tightly coupled with your SimpleJdbcTemplate class. What if the constructor changes? what if you whant to use something different then SimpleJdbc? ... If you need a class, ask for it.

the fix

public void setSimpleJdbcTemplate(SimpleJdbcTemplate template) {
    this.simpleJdbcTemplate = template;
}

I don't trust my parent

The override in the child classes smell even harder. Your setDataSource method does exactly the same, but yet you @override ? But the only thing you do is delegate everything to the parent. why? what is the logic here?

public, static, private, mess

Some methods are declared public, some are not. And whats it with those private statics? what is the point of those?

Wouldn't it be better to have just one private variable called table that holds the table where you DAO class is looking for persons? And then simply create 2 Objects, one for your new table and one for you historical table?

What happened to this?

Sometimes you use this. in your code, and sometimes you don't? smelly

public PersonDao() {
    final String tableName = getTableName(); // what happened to this?
    fieldNamesCSV = getFieldNames(); // what happened to this?
    String select = String.format("SELECT %s FROM %s ", fieldNamesCSV, getTableName()); //again, where is this?
}

What does your class need

Looking at your constructor, Your class needs nothing. Not even a datasource, or table name or ...

Again, if you need it, ask for it.

The best code quality analysis tool is 'grandparents'

Code should read like poetry. You know what is going on, but it doesn't feel like perfectly written English. If your code is readable, it's good. If it reads a little bit weird, it smells...

for instance your setDataSource method:

  The public returns void method setDataSource needs a DataSource
  object. this simepleJdbcTemplate is now a new SimpleJdbcTemplate that
  needs a DataSource object.

smells right?

Or your @Override method:

  I override the parrent method setDataSource. The new method needs a
  DataSource and passes it to the parent.

hmmm...


Refactoring code duplication so the same textArea is not used in 4 different classes Looking for the best way to do this.
Currently in 4 different classes I have:

textArea = new JTextArea(10, 55) {
    {
        setOpaque(false);
    }

    @Override
    protected void paintComponent(Graphics g) {
        for (int i = 0; i < list.size(); i++) {
            if(i == lineIndex)
                g.setColor(Color.black);
                g.drawRect(0, 20, 750, 20);
        }
        super.paintComponent(g);
    }
};

I've simplified down what I actually have.
As you can see there is duplication. The only data that is different is what is in the list.
A list is passed into each of these classes
There are some other stuff in the class which depends on the textarea and the lineIndex variable changes in the class depending on what the user does. I call the repaint() method of the textarea when I need the textarea to draw something else. The paintcomponent does a simple draw.

I'm not sure of the best way for each of the classes to contain the textarea so I dont have duplication. In each of the classes I have an addList(List list) method which the textareas need to use.

Hope I explained this ok.

Thanks

Write your own class:

class MyTextArea extends JTextArea{

    private final List<Foo> list;

    MyTextArea(int row, int col, List<Foo> list) {
        super(row, col);
        this.list = list;
        setOpaque(false);
    }

    @Override
    protected void paintComponent(Graphics g) {
        for (int i = 0; i < list.size(); i++) {
            if(i == lineIndex)
                g.setColor(Color.black);
            g.drawRect(0, 20, 750, 20);
            //use list...
        }
        super.paintComponent(g);
    }
}