Index mechanics for Rock Paper Scissors I just went through a very easy online example creating a Rock-Paper-Scissors game, but it seemed like it was not a great use of computing power.

It seemed like I was writing out results for every possible outcome instead of declaring properties or attributes for variables that will dictate the results depending on the situation.

Could an index or something be used to set up the variables?

var rock = ["rock", "paper", "scissors"];
var paper = ["paper", "scissors", "rock"];
var scissors = ["scissors", "rock", "paper"];
    index           0         1        2

Player 1 could select a variable, then player 2 could select a string in the index, and the position of the selection in the index would determine the winner (0 = tie, 1 = p1Loss, 2 = p1Win). Is this possible?

I think the code in the example below would start to get messy and bloated if you were to expand on the choices/options. i.e. Instead of 3 options you have 9, any one of which will beat 4 options and loose to the other 4.

I guess I'm just wondering if there is a better way to do this that would scale better with larger numbers.

I've included the code from the example below:

var user1Choice = prompt("Do you choose rock, paper or scissors?");
var user2Choice = prompt("Do you choose rock, paper or scissors?");

var compare = function(choice1, choice2) {
    if (choice1 === choice2) {
        return "its a tie";
    }

    else if (choice1 === "rock") {

        if (choice2 === "scissors") {
            return "rock wins";
        }
        else {
            return "paper wins";
        }
    }

    else if (choice1 === "paper") {
        if (choice2 === "rock") {
            return "paper wins";
        }
        else {
            return "scissors wins";
        }
    }

    else if (choice1 === "scissors") {
        if (choice2 === "paper") {
            return "scissors wins";
        }
        else {
            return "paper wins";
        }
    }

}

compare(user1Choice, user2Choice)

Your idea

Your idea is nice, but it will cause problems when extending the game. For example, we will add spock and lizard:

var spock = ["spock", "paper", "lizard", "scissors", "rock"];

Now, index 1 and 2 mean p1Loss, and index 3 and 4 mean p1Win. This can easily cause bugs in the future.

Maybe a better idea

You could instead maintain arrays that say what items beat. For example:

var spockBeats = ["scissor", "rock"];

Then, check at the beginning if the input of the users is equal, return "tie" if it is. If it is not, find the appropriate array for user1 (I would use a switch instead of many if-else) and see if it contains the choice of user2.

In this case, extending the game is easier. New item? Add a new array and a new switch option, and add the item to some of the other arrays.

In case you really want to extend the game (I mean like adding a couple dozen or more options), this approach too would become somewhat annoying. In that case you might want to read up on graph theory, as this is basically just a directed graph. I think that an adjacency matrix might work well then.

For rock, paper, scissor, the matrix would look like this (it contains duplicate information, but I don't think that memory is a concern and removing it would make it harder to use):

   R  P  S
R  0 -1  1
P  1  0 -1
S -1  1  0

And you use it like this:
Player1 plays paper, Player2 plays rock.
Look in the paper row for the value under rock. it is 1 and player1 wins. -1 would mean they lose, and 0 means tie.
For just rock, paper, scissors this is overkill, but for a lot more options this might work well.

Comments on current code

I would save the strings (rock, paper, scissors) in fields at the top of the file. It's an easy source of bugs because of typos
I would return which player won, not with what choice they won. Or both, but the current way seems just odd to me


Better way to create samples I've done this piece of code for creating all possible samples without repetitions, but I think that it is a so heavy algorithm because every time I call this recursive function I create a new vector that is passed to it. What means create any samples without repetitions? it means creating every combination given a population. Is there a way for making this algorithm faster?

Here is the algorithm and the description:

As already said, this function creates samples without repetitions recursively. If n, that is the number of elements that should be found, is equals 0 I print the sample.

Else to the sample that is passed I add the elements of the population that there aren't yet in the combination in a way that there should't be samples that are equals between every samples(I have to create a vector for everyone of this new samples), and I call the function with this new sample and n(the number of element to be found) decremented and incrementing the element from wich I'll start to add.

private static void distribuzioneInBlocco(int n, int firstElementConsidered, Vector<Float> population, Vector<Float> sample){
    // exit condition, when n equals 0 the fuction doesn't self-calls
    if(n==0){
        JOptionPane.ShowMessageDialog(null, sample);
        return;
    }

    // for every element of the population the function adds this element
    // at the previous combination
    for(int x = firstElementConsidered; x<population.size();x++){
        Vector<Float> aggiunta = new Vector<Float>(sample);
        aggiunta.add(population.elementAt(x));
        distribuzioneInBlocco(n-1, x+1, population, aggiunta);
    }
}

for example if I give as population the vector 0 2 4 6 and n = 3 the answer will be

0 2 4
0 2 6
0 4 6
2 4 6

if I give the same population but n = 2 the result will be:

0 2
0 4
0 6
2 4
2 6
4 6

if n = 4

0 2 4 6

Moreover is there a way for doing it without recursion?

Your solution requires that you create a new Vector for every combination. You are right that there is no better algorithm. The way you have it implemented though, is not as efficient as it should be.

Float should be a primitive.... float. Using a Float object instead of the primitive float will require a lot more memory than you should need.

The Java Vector class is a synchronized implementation. Use of the Vector class is discouraged. If you need synchronization then use one of the java.util.concurrent.* classes. I would also use an array-of-primitives for the input population too):

private static void distribuzioneInBlocco(int n, int firstElementConsidered, float[] population, float[] sample){
    // not sure what this is.....
// exit condition, when n equals 0 the function doesn't self-calls
    //if(n==0){
    //    JOptionPane.ShowMessageDialog(null, campione);
    //    return:
    //}

    // for every element of the population the function adds this element
    // at the previous combination
    for(int x = firstElementConsidered; x < population.length; x++) {
        float[] aggiunta = Arrays.copyOf(sample, sample.length + 1);
        aggiunta[aggiunta.length - 1] = population[x];
        distribuzioneInBlocco(n-1, x+1, population, aggiunta);
    }
}

So, using an array of primitive will make a big difference for your performance.

I note that you are not keeping any of these samples. If you do not need to actually keep the agguinta array, it is possible that you can save a lot of time by moving the initialization to be outside the loop:

private static void distribuzioneInBlocco(int n, int firstElementConsidered, float[] population, float[] sample){
    // not sure what this is.....
// exit condition, when n equals 0 the function doesn't self-calls
    //if(n==0){
    //    JOptionPane.ShowMessageDialog(null, campione);
    //    return:
    //}

    // for every element of the population the function adds this element
    // at the previous combination
    // move the array initialization outside the loop, and reuse it for each
    // recursion at this depth.
    float[] aggiunta = Arrays.copyOf(sample, sample.length + 1);
    for(int x = firstElementConsidered; x < population.length; x++) {
        aggiunta[aggiunta.length - 1] = population[x];
        distribuzioneInBlocco(n-1, x+1, population, aggiunta);
    }
}


Summing some datum for requests in each domain that fall in a date range I have a LINQ query which works fine. I am however very interested to understand if this can be written in a more optimum way...

var query = (from r in Results.All.AsEnumerable()
                       where r.RequestType.Id == Id &&
                             r.DateFrom >= sDate &&
                             r.DateTo <= eDate
                       group r by new {r.Data_1} into g
                       select new {data = g.Key.Data_1,
                                    sum = g.Sum(s => int.Parse(s.Data_2))}).ToList();

I am trying to sum Data_2 (stored as a string) by a unique list of Data_1 all by a given date range.

I have provided some sample data below:

For example: facebook.com = 51, m.facebook.com = 94 etc.

This is a case where a foreach loop might work better.  Your LINQ query iterates for your data several times.  Where as a foreach loop and a dictionary, should be able to accomplish this in one loop.  Something like this:

Dictionary<string,int> query = new Dictionary<string, int>();
foreach(var r in Results)
{
    if(r.RequestType.Id == Id && r.DateFrom >= sDate && r.DateTo <= eDate)
    {
         if(query.ContainsKey(r.Data_1))
         {
             query[r.Data_1] += int.Parse(r.Data_2);
         }
         else
         {
             query.Add(r.Data_1, int.Parse(r.Data_2));
         }
    }
}

While there is a Dictionary lookup for each item it should be more than offset by calculating the sum on the fly rather than at the end.


Adding prefixes to headers in markdown I'm adding prefixes to headers in markdown using a script. For example, if I have:

# Hello

## World

### Let's add

## Some headers

### Yay!

# Foo

## Bar

I transform it to:

# 1 Hello

## 1.1 World

### 1.1.1 Let's add

## 1.2 Some headers

### 1.2.1 Yay!

# 2 Foo

## 2.1 Bar

I currently have the following working code, but it feels a bit hacky and prone for error. :

preg_match_all('/^(#+)\s(.*)$/m', $markdown, $headers);
$levelCount = [];
$currentLevel = 9999;
foreach ($headers[2] as $idx => $header) {
    $level = strlen($headers[1][$idx]);
    if ($level < $currentLevel) {
        // reset:
        for ($i = $level; $i != $currentLevel; $i += 1) {
            array_pop($levelCount);
        }
    }
    if (!isset($levelCount[$level])) {
        $levelCount[$level] = 1;
    } else {
        $levelCount[$level] += 1;
    }
    $prefix = implode('.', array_values($levelCount));
    $currentLevel = $level;
    $markdown = preg_replace(
        '/' . preg_quote($headers[0][$idx]) . '/',
        $headers[1][$idx] . ' ' . $prefix . ' ' . $header,
        $markdown,
        1
    );
}

What are your thoughts on this?

I may not be considering all fringe cases (please enlighten me if this breaks with any realistic input), but all of the logic can be packed into a single preg_replace_callback() call.

I start by declaring a 1-dimensional array which will contain all numeric counters.
My pattern ~^(#+)\K~m says:
From the start of each line, capture one or more hash symbols as capture group #1, then restart the fullstring match .  In doing so, my replacement string to be determined inside the custom function will not actually be replacing any characters, but adding new characters at the zero-length position marked by \K.  This spares you needing to match the rest of the line and adding that to the replacement.
Making $levels modifiable by reference with & means that $level will be updated with each iteration of the regex matches.
After counting the elements in $levels and counting the length of the matched string, precise modifications can be done to $levels to provide the desired set of numbers.
The return value is a space followed by the dot-imploded $levels array.
array_slice() avoids poping in a loop.
I don't think your array_values() call is necessary.

Code: (Demo)

$headers = <<<HEADERS
# Hello

## World

### Let's add

## Some headers

### Yay!

# Foo

## Bar

## Bar Again
HEADERS;

$levels = [];
echo preg_replace_callback(
        '~^(#+)\K~m',
        function($m) use (&$levels) {
            $hashes = strlen($m[1]);
            $index = $hashes - 1;
            $reduction = sizeof($levels) - $hashes;
            if (!isset($levels[$index])) {
                $levels[$index] = 1;
            } else {
                ++$levels[$index];
            }
            if ($reduction > 0) {
                $levels = array_slice($levels, 0, -$reduction);
            }
            return " " . implode('.', $levels);
        },
        $headers
    );

Output:

# 1 Hello

## 1.1 World

### 1.1.1 Let's add

## 1.2 Some headers

### 1.2.1 Yay!

# 2 Foo

## 2.1 Bar

## 2.2 Bar Again


MemoryCache - Login Security I have a WCF service which should authenticate user against any bruteforce attack.
Below is the code to avoid any bruteforce attack. I have used MemoryCache to keep track of invalid attempts by a user.

Could you please review the code and let me know if there are any issues?

Following defined in config file:

<InvalidLoginAttemptsThreshold value = "5"/>
<InvalidLoginLockoutInMinutes value = "20"/>

Code to add invalid login to cache and lock the account if invalid logins reaches threshold set in config.

private readonly CacheItemPolicy cacheInvalidLoginPolicy = new CacheItemPolicy();
private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}

public class UserLogin
{
    public string UserName { get; set; }
    public List<DateTime> InvalidLoginTimestamp { get; set; }
    public bool IsLocked { get; set; }
}
private bool AuthenticateUser(string userName, string password)
{
    bool validCredentials = false;
    string userNameUpperCase = userName.ToUpper(); //MemoryCache treats 'a' and 'A' as different names
    if (cacheInvalidLogins.Contains(userNameUpperCase) && ((UserLogin)cacheInvalidLogins[userNameUpperCase]).IsLocked)
    {
        log.ErrorFormat("UserId: {0} tried to access locked account at {1}", userName, DateTime.Now);
        throw new FaultException("Your account is locked out. Please try after some time.", new FaultCode("AccountLocked"));
    }

    try
    {
        validCredentials = securityService.Authenticate(userName, password);

        if (!validCredentials)
        {
            AddInvalidLoginToCache(userNameUpperCase);
        }
        else if (validCredentials && cacheInvalidLogins.Contains(userNameUpperCase))
        {
            //For valid credentials - remove from cache
            cacheInvalidLogins.Remove(userNameUpperCase);
        }
    }
    catch (Exception exception)
    {
        log.Error(exception);
    }

    return validCredentials;
}

private void AddInvalidLoginToCache(string userName)
{
    if (cacheInvalidLogins.Contains(userName))
    {
        string clientId = userName.Split('\\')[0];
        //Increment InvalidLoginCount for this user
        UserLogin invalidLogin = (UserLogin)cacheInvalidLogins[userName];
        invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now);
        cacheInvalidLogins[userName] = invalidLogin; //todo: update cache policy here

        //Lock user account if InvalidLoginCount reach threshold set in config
        if (invalidLogin.InvalidLoginTimestamp.Count == invalidLoginThresholdFromConfig))
        {
            invalidLogin.IsLocked = true;
            cacheInvalidLogins.Set(userName.ToUpper(), invalidLogin, LockOutCachePolicyFromConfig());
            log.ErrorFormat("Invalid Login attempted by UserId: {0} at these timestamps: {1}", userName, String.Join(",", invalidLogin.InvalidLoginTimestamp));
            log.ErrorFormat("Account is locked out for UserId: {0}", userName);
            throw new FaultException("Your account is locked out. Please try after some time.", new FaultCode("AccountLocked"));
        }
    }
    else
    {
        //todo: check if extra config setting is required for this cache policy
        cacheInvalidLoginPolicy.AbsoluteExpiration = DateTimeOffset.Now.AddMinutes(5);
        cacheInvalidLogins.Add(userName.ToUpper(),
                               new UserLogin {UserName = userName, InvalidLoginTimestamp = new List<DateTime> {DateTime.Now}},
                               cacheInvalidLoginPolicy);
    }
}

If I'm right it's not thread-safe. I suppose two browsers with the same username could login at the same time which means that the AuthenticateUser method runs parallel on two threads.

Suppose the following:

the user already has an invalid login attempt,
two new threads enter AddInvalidLoginToCache the same time,
invalidLoginThresholdFromConfig = 2.

A possible execution order is the following:

// InvalidLoginTimestamp.Count = 1, we already have an invalid login attempt
thread1: enters AddInvalidLoginToCache
thread2: enters AddInvalidLoginToCache
thread1: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 2
thread2: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 3
thread1: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false
thread2: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false

The result is that a lucky attacker will never be locked out.

The second problem is visibility. Since InvalidLoginTimestamp and invalidLogin are shared between threads you need some synchronization to ensure other threads will see modifications (not just stale data or invalid state).

C# multi-threading: Acquire read lock necessary?
How to synchronize the access to a List used in ASP.NET?

private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}

I'd put the comment before the line to avoid horizontal scrolling:

//Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}
private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default;

Furthermore, I'd rename it to invalidLoginAttemptsCache. The cacheInvalidLogins name looks like a method name since it starts with a verb (cache...).

bool validCredentials = false;
... // other statements
try
{
    validCredentials = securityService.Authenticate(userName, password);

The boolean could be declared right before the first use with a smaller scope:

...  // other statements
bool validCredentials = false;
try
{
    validCredentials = securityService.Authenticate(userName, password);

Should I declare variables as close as possible to the scope where they will be used?

private void AddInvalidLoginToCache(string userName)
{
    ...
    cacheInvalidLogins.Set(userName.ToUpper(), invalidLogin, LockOutCachePolicyFromConfig());

The code calls this method with the uppercased username. I'd rename the parameter to upperCasedUserName and remove the .ToUpper() call.
This variable is never user, I guess you could remove it:

string clientId = userName.Split('\\')[0];

Username seems to be one word, I'd not capitalize the n.


Is my C style good? 100 line timer program This code works exactly as the prompt and the code predict.
Is my style good, my implementations, or what should I change, or what? I'm trying to improve my code, and writing more of it helps... Any and all relevant feedback is good.

#include <stdio.h>
#include <time.h>
#include <math.h>

#define SECONDS_PER_MINUTE 60
#define MINUTES_PER_HOUR 60
#define HOURS_PER_DAY 24
#define PROMPT_STRING "Type any key to show time, q to show time then quit: "

#define TIME_TYPE unsigned long

char getChar() ;
void updateTimes() ;
void initTimes() ;
void printPrompt() ;
void printTime (TIME_TYPE timeInit) ;

TIME_TYPE start, end;

int main (const char argc, char* argv[]) {
        char command = 0;
        initTimes();
        while (command != 'q') {
                printPrompt();
                command = getChar();
                updateTimes();
                printTime(end-start);
        }
        return 0;
}

char getChar() {
        char retVal = -1;
        char trashVal = -1;
        retVal = getc(stdin);
        while ( trashVal != '\n' && retVal != '\n') {
                trashVal = getc(stdin);
        }
        return retVal;
}

void updateTimes() {
        end = time(0);
}

void initTimes() {
        start = time(0);
        end = start;
}

void printPrompt() {
        printf(PROMPT_STRING);
}

void printTime (TIME_TYPE timeInit) {
        /* Basic declarations */
        TIME_TYPE time = timeInit;
        int days, hours, minutes, seconds;
        int timeDifferential = 0;
        /* Assigning crap, reassigning remaining seconds to 'time' */
        /* Days */
        timeDifferential = (SECONDS_PER_MINUTE *
                                MINUTES_PER_HOUR *
                                HOURS_PER_DAY);
        days = (time/timeDifferential);
        time -= timeDifferential*days;
        /* Hours */
        timeDifferential = (SECONDS_PER_MINUTE *
                                MINUTES_PER_HOUR);
        hours = (time/timeDifferential);
        time -= timeDifferential*hours;
        /* Minutes */
        timeDifferential = (SECONDS_PER_MINUTE);
        minutes = (time/timeDifferential);
        time -= timeDifferential*minutes;
        /* Seconds */
        seconds = time;
        /* Check for validity of times, then print.
         * Also print error if need be.*/
        if (!(seconds < 60 && seconds >= 0 &&
                        minutes < 60 && minutes >= 0 &&
                        hours < 24 && hours >= 0 )) {
                printf ("ERROR! TIME FORMAT SCREWY!\n");
        }
        printf ("Total time (s): %u\n %2dd %2dh %2dm %2ds\n", timeInit,
                        days, hours, minutes, seconds);
}

Compiler Warnings

Firstly, always enable maximum warning settings for your compiler, it will catch things that you miss. For example, with gcc -Wall -Wextra, the following is produced:

  timer.c:20:5: warning: first argument of 'main' should be 'int' [-Wmain]

  timer.c: In function 'main':
  timer.c:20:22: warning: unused parameter 'argc' [-Wunused-parameter]
  timer.c:20:34: warning: unused parameter 'argv' [-Wunused-parameter]

  timer.c: In function 'printTime':
  timer.c:92:21: warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'long unsigned int' [-Wformat]

Nothing major, which is good, but we can fix some of these up. Firstly, as it says, main should have signature:

int main (int argc, char* argv[])

not the current signature of

int main(const char argc, char* argv[])

The next warnings complain about unused parameters of argc and argv. If you aren't going to utilize command line arguments in your program, you can simply leave them out:

int main()

Fixing the printf warning is a simple matter of replacing the %u\n with %lu\n.

Code Style

Generally, there is too much whitespace. Very few programmers use 8 space indentation - I'd recommend sticking to 4. Also, leaving a space between method declarations and the trailing semicolon is slightly odd - char getChar() ;. None of this is wrong, it's just unorthodox, and will probably be a bit jarring to most C programmers (and programmers are a picky bunch). Pretty much the first thing I did with your code is modify the indentation and remove any whitespace for trailing semicolons.

Never, ever #define a type. Utilize typedef instead. This also means types should not be ALL_CAPS.

typedef unsigned long time_type;

There are very good reasons for this. For example, if we define a pointer type:

#define ptr_int int *

Now, what happens if we do the following:

ptr_int x, y;

The preprocessor will expand this to:

int *x, y; //Uh-oh!

This is declaring a pointer to integer (x) and a normal integer (y). This is sure to introduce annoying and hard to track down bugs. If instead we used a typedef, this will fix the above problem (although it shows you one must be careful when having more than declaration per line).

Comments are sparse (then again, this program probably doesn't really need them). However, you've fallen into the "obvious comment" trap:

/* Basic declarations */
/* Assigning crap, reassigning remaining seconds to 'time' */
/* Days */

and so on. None of these is really helpful. We can see that up the top the basic declarations happen, and that then some assignment takes place. The variable names of days, minutes and seconds are descriptive enough that they make the comments superfluous.

Other Quibbles

getChar actually returns an int. Pretty much anywhere you have a char, you should replace it with an int:

int getChar()
{
    int retVal = -1;
    int trashVal = -1;
    ...
}

int main()
{
    int command = 0;
    ....
}

Also, why getc(stdin) over simply getchar()? Either is fine really, but getchar is slightly more idiomatic when reading from stdin.

With any slightly more modern version of C, prefer const declarations to using #define. It's good that you didn't simply write magic numbers all over the place, but prefer things like:

const int seconds_per_minute = 60;

to their #defined counterparts. (They should probably also be static, but don't get too hung up on this while learning).

There are a number of reasons for this. The biggest reason is the fact that #define is just a dumb text replacement mechanism. Say you have a larger program and you are trying to debug it - since #define simply replaces a textual pattern with a given value, all symbols are lost. It's great in the source code to not have any magic numbers, but in a debugger, you won't have that luxury with #define - it'll be back to magic numbers all over again.

The other more minor reasons are that const guarantees it won't change, and if you need to take the address of any of these variables for any reason, well, you're totally out of luck with #define.

That being said, you cannot always get away with using const instead of #define. There's a big post about this on StackOverflow that is worth reading.

Finally, try to avoid using global variables (variables outside the scope of any function, so TIME_TYPE start, end;). In a small program like this it doesn't matter too much, but it's a good habit to get into.

I've typed a bit of a wall of text, but most of this stuff is pretty minor. Summing up:

Always compile with all warnings enabled.
Prefer 4 space to 8 space indentation.
Don't use #define to introduce a new type or a type alias; use typedef.
Prefer to use const variables to #define'd variables.
Try to avoid global variables if possible.


Concurrency and locking in a chat server I've never done concurrent programming before, but I've written one using go channels and RWMutex, but I'm not sure if my approach is idiomatic. I feel like it could all use channels (Maybe).

The protocol is simple: Messages are delimited by '\n'. The first message client sends is it's name. The rest are normal messages. I used a map because it's add + delete is faster, and because I'm not concerned about the ordering.
I'm mostly concerned with the locks at the last 4 methods. Could I use channels here instead? Am I using locks correctly?

type message struct {
    message string
    client  *client
}

type client struct {
    conn   net.Conn
    reader *bufio.Reader
    name   string
}

type Server struct {
    l           net.Listener
    clients     map[*client]struct{}
    messageChan chan message
    port        uint16
    name        string
    m           sync.RWMutex
}

// NewServer starts listening, and returns an initialized server. If an error occured while listening started, (nil, error) is returned.
func NewServer(name string, port uint16) (*Server, error) {
    p := strconv.FormatUint(uint64(port), 10)
    l, e := net.Listen("tcp", "localhost:"+p)
    if e != nil {
        return nil, e
    }
    return &Server{l, make(map[*client]struct{}), make(chan message), port, name, sync.RWMutex{}}, nil
}

// Start starts accepting clients + managing messages.
func (r *Server) Start() {
    go r.sendMessages()
    for {
        conn, e := r.l.Accept()
        if e != nil {
            continue
        }
        go r.handleConn(conn)
    }
}

func (r *Server) handleConn(conn net.Conn) {
    reader := bufio.NewReader(conn)
    name, e := reader.ReadString('\n')
    name = strings.TrimSpace(name)
    if e != nil {
        conn.Close()
        return
    }

    c := &client{conn, reader, name}
    r.addClient(c)
    r.publishMessage(c.name + " has joined the server\n")
    r.handleClient(c)
}

func (r *Server) handleClient(c *client) {
    for {
        msg, e := c.reader.ReadString('\n')
        if e == io.EOF {
            c.conn.Close()
            r.deleteClient(c)
            r.publishMessage(c.name + " has left the server\n")
            return
        } else if e != nil {
            continue
        }
        r.messageChan <- message{c.name + ": " + msg, c}
    }
}

func (r *Server) sendMessages() {
    for {
        m := <-r.messageChan
        r.m.RLock()
        for k := range r.clients {
            if k != m.client {
                go k.conn.Write([]byte(m.message))
            }
        }
        r.m.RUnlock()
        fmt.Print(m.message)
    }
}

func (r *Server) publishMessage(msg string) {
    r.m.RLock()
    for k := range r.clients {
        go k.conn.Write([]byte(msg))
    }
    r.m.RUnlock()
    fmt.Print(msg)
}

func (r *Server) addClient(c *client) {
    r.m.Lock()
    r.clients[c] = struct{}{}
    r.m.Unlock()
}

func (r *Server) deleteClient(c *client) {
    r.m.Lock()
    delete(r.clients, c)
    r.m.Unlock()
}

I would appreciate if anyone could give some suggestions/feedback.

This is a nice project to learn concurrency on, and you've already identified a few issues that concern you, and you're right in some ways. Go documentation recommends using channels when possible, over mutexes. The documentation in the sync package says: "Other than the Once and WaitGroup types, most are intended for use by low-level library routines. Higher-level synchronization is better done via channels and communication."

Channels are your friend in Go, and you should use them liberally (well, more liberally than mutexes).

There are some special cases with channels, though, and your code has a good example of how they can be used in a broken way. Your concurrency is not nearly as significant as you think it is. I will focus on this code for the moment, and we'll simplify it down, and then get back to the concurrency problem in a bit...

Client handler

This code....

func (r *Server) handleConn(conn net.Conn) {
    reader := bufio.NewReader(conn)
    name, e := reader.ReadString('\n')
    name = strings.TrimSpace(name)
    if e != nil {
        conn.Close()
        return
    }

    c := &client{conn, reader, name}
    r.addClient(c)
    r.publishMessage(c.name + " has joined the server\n")
    r.handleClient(c)
}

func (r *Server) handleClient(c *client) {
    for {
        msg, e := c.reader.ReadString('\n')
        if e == io.EOF {
            c.conn.Close()
            r.deleteClient(c)
            r.publishMessage(c.name + " has left the server\n")
            return
        } else if e != nil {
            continue
        }
        r.messageChan <- message{c.name + ": " + msg, c}
    }
}

... takes a client network connection, pulls the name, and then establishes a loop for reading and distributing messages. It distributes the messages by pushing them on to the r.messageChan channel. Let's simplify that code a whole bunch, and point out some problems as we go.

First up, you need to learn the defer statement, and use it for closing the client connection. If you change the first line of the function to be:

func (r *Server) handleConn(conn net.Conn) {
    defer conn.Close()

now you don't need to worry about closing it in a number of other places. Just returning from the function is enough to close it.

Secondly, you should use a scanner... not a Reader. See: Scanner which has the documentation "Scanner provides a convenient interface for reading data such as a file of newline-delimited lines of text." Scanner defaults to using line-termination (which is what you have) so it's easy to set up. Your code:

    reader := bufio.NewReader(conn)

becomes:

     scanner := bufio.NewScanner(conn)

Note that Scanner never returns an EOF error, it just returns a false scan at EOF (or any other error). The advantage for you of a scanner is that it simplifies your loops, and error handling. This is not obvious immediately in your case as you scan the name, but will become apparent when processing messages. To get the name, you need:

// find the next end-of-line.. (or perhaps EOF)
if !scanner.Scan() {
    // could not scan anything on the first scan... odd... no name.
    // the scanner.Err() may have a value if there's a problem, but we don't care.
    return
}
name := strings.TrimSpace(scanner.Text())

OK, that's how you get the name.... and now we can create the client (using the scanner instead of reader) and we also use a defer function to remove the client instead of shifting that logic in to some other place (keeping the clean-up code in the same place as the mess-up code is important for maintainability). Also, we should relocate the "publish" methods in to the addClient and deleteClient methods (where they really should be anyway):

c := &client{conn, scanner, name}
r.addClient(c)
defer r.deleteClient(c)
r.handleClient(c)

OK, so now we have a client that can process incoming messages, let's look at that code, but using a scanner now instead of a reader, and it does not need to do the deleteClient or the related "publish" of the deletion. It also does not need to close the connection:

func (r *Server) handleClient(c *client) {
    for c.scanner.Scan() {
        msg := c.scanner.Text()
        r.messageChan <- message{c.name + ": " + msg, c}
    }
    // we could check scanner.Err() here, but we are ignoring all non-EOF errors anyway
}

Now, that looks neat... so small now that it's hardly worth having it's own function, let's pull it back in to the handleConn function (which we will rename to handleClient because I prefer client... and we'll remove the scanner from the client struct because no-one else uses it in any other place... our handleClient method is now:

func (r *Server) handleClient(conn net.Conn) {
    defer conn.Close()

    scanner := bufio.NewScanner(conn)

    // read off the first line as the "name"

    // find the next end-of-line.. (or perhaps EOF)
    if !scanner.Scan() {
        // could not scan anything on the first scan... odd... no name.
        // the scanner.Err() may have a value if there's a problem, but we don't care.
        return
    }
    name := strings.TrimSpace(scanner.Text())

    // register our client in to the control systems.
    c := &client{conn, name}
    r.addClient(c)
    defer r.deleteClient(c)

    // send messages in to the channel.
    for scanner.Scan() {
        r.messageChan <- message{name + ": " + scanner.Text(), c}
    }
}

Now, that's a bunch simpler, especially the reduced error handling, and the reduced content in the client.

There were a few things to hammer out here. We shifted the printing of messages like X has joined the server and X has left the server in to the addClient and deleteClient methods respectively. This reduces the mixed logic of handling a client and also reporting activity in this function, and shifts it to the more logical place where we are actually adding and deleting things. We used defer functions to handle cleanup of resources at the same time/place that the resources were created. We used a scanner instead of a reader.

messageChan

Let's cover that concurrency issue I mentioned earlier. It comes down to the messageChan. The problem is only one client is busy at a time. Let me explain... this code here:

r.messageChan <- message{name + ": " + scanner.Text(), c}

pushes messages on to the channel, and this code pulls them off:

for {
    m := <-r.messageChan
    .... distribute the messages to other clients
}

You would think that multiple clients could all be pushing messages, and the for-loop is pulling them and distributing them at the same time. Unfortunately, all the clients "block" at the point where they push the message, and they all wait there, until the for-loop starts, and it will pull just one message. That one message will be processed, and the client that happened to have its message pulled, will then go off and wait for another message - all the other clients are still blocked. Once the for loop has sent the message off, it will come back and read another from the channel, and one other client will be lucky enough to have its message delivered.

This is because you have created an unbuffered channel. The documentation has this to say: "If the capacity is zero or absent, the channel is unbuffered and communication succeeds only when both a sender and receiver are ready." Since the receiver is only ready once each loop, the receiver essentially serializes all the clients.

You need to declare your channel with some buffer size if you want to avoid this issue. Even a size of 1 would be beneficial. A larger buffer may result in caching of messages but that should not be a problem.

Communication instead of Mutexes

Communication in Go is an odd concept to explain. The purpose of mutexes is to allow safe access to resources from multiple go-routines. Communication is sort of the opposite - communication basically implies only one go-routine ever accesses a resource, so mutexes are not needed, but we can communicate the changes (normally via a channel) to that go-routine.

If only one routine accesses a resource, no mutex is needed. Let's see how we can apply that to your code. Your "server" code really has three things going on...

clients get added
messages get distributed
clients get removed.

You've already got a channel for the messages, so let's look at the other parts....

func (r *Server) addClient(c *client) {
    r.m.Lock()
    r.clients[c] = struct{}{}
    r.m.Unlock()
}

func (r *Server) deleteClient(c *client) {
    r.m.Lock()
    delete(r.clients, c)
    r.m.Unlock()
}

Let's turn those in to channels instead:

func (r *Server) addClient(c *client) {
    r.added <- c
}

func (r *Server) deleteClient(c *client) {
    r.deleted <- c
}

We will have to create those channels (with some buffering) on the Server... but, that's how we communicate the add/remove from the client.

How do we handle that on the server? Currently the server code looks like:

func (r *Server) sendMessages() {
    for {
        m := <-r.messageChan
        r.m.RLock()
        for k := range r.clients {
            if k != m.client {
                go k.conn.Write([]byte(m.message))
            }
        }
        r.m.RUnlock()
        fmt.Print(m.message)
    }
}

If we listen for communication from all channels, not just the messageChan, we can do it all in that routine (and no need for mutexes because we're the only ones to manipulate the client map.

func (r *Server) sendMessages() {
    for {
        select {
        case m := <-r.messageChan:
            for k := range r.clients {
                if k != m.client {
                    go k.conn.Write([]byte(m.message))
                }
            }
            fmt.Print(m.message)
        case c := <-r.added:
            r.publishMessage(c.name + " has joined the server\n")
            r.clients[c] = struct{}{}
        case c := <-r.deleted:
            r.publishMessage(c.name + " has left the server\n")
            delete(r.clients, c)
        }
    }
}

Now we can even remove the client map from the Server completely, and keep it local inside thesendMessages` go-routine.

Read up on the select statement which is how communication from multiple sources is handled: https://golang.org/ref/spec#Select_statements

Conclusion

This post has been long enough already... hopefully you'll get something out of it. I hope that you at least get how the unbuffered channels can impact things, and that communication with select-statements allows you to localize resources to just one routine, meaning mutexes are not needed.


Async download of files The following code is an async beginner exploration into asynchronous file downloads, to try and improve the download time of files from a specific website.

Tasks:
The tasks are as follows:

Visit https://digital.nhs.uk/data-and-information/publications/statistical/mental-health-services-monthly-statistics

Extract the latest publication link. It is the top link under Latest Statistics and is the first match returned by css selector .cta__button.

N.B. Each month this link updates so the next monthly publication e.g. 8 Apr 2021 the link will update to that associated with Mental Health Services Monthly Statistics Performance January, Provisional February 2021.

Visit the extracted link: https://digital.nhs.uk/data-and-information/publications/statistical/mental-health-services-monthly-statistics/performance-december-2020-provisional-january-2021 and, from there, extract the download links for all the files listed under Resources; currently 17 files.

Finally, download all those files, using the retrieved urls, and save them to the location specified by folder variable.

Set-up:
Python 3.9.0 64-bit Windows 10

Request:
I would appreciate any suggested improvements to this code e.g. should I have re-factored the co-routine fetch_download_links into two co-routines, each with a ClientSession, where one co-routine gets the initial link to where to get the resources, and the second co-routine to retrieve the actual resource links?

mhsmsAsynDownloads.py
import time
import os
from bs4 import BeautifulSoup as bs
import aiohttp
import aiofiles
import asyncio
import urllib

async def fetch_download_links(url:str) -> list:
    async with aiohttp.ClientSession() as session:
        r = await session.get(url, ssl = False)
        html = await r.text()
        soup = bs(html, 'lxml')
        link = 'https://digital.nhs.uk' + soup.select_one('.cta__button')['href']
        r = await session.get(link, ssl = False)
        html = await r.text()
        soup = bs(html, 'lxml')
        files = [i['href'] for i in soup.select('.attachment a')]

        return files

async def place_file(source: str) -> None:
    async with aiohttp.ClientSession() as session:
        file_name = source.split('/')[-1]
        file_name = urllib.parse.unquote(file_name)
        r = await session.get(source, ssl = False)
        content = await r.read()

    async with aiofiles.open(folder + file_name, 'wb') as f:
        await f.write(content)

async def main():
    tasks = []
    urls = await fetch_download_links('https://digital.nhs.uk/data-and-information/publications/statistical/mental-health-services-monthly-statistics')

    for url in urls:

        tasks.append(place_file(url))

    await asyncio.gather(*tasks)

folder = 'C:/Users/<User>/OneDrive/Desktop/testing/'

if __name__ == '__main__':

    t1 = time.perf_counter()
    print("process started...")
    asyncio.get_event_loop().run_until_complete(main())
    os.startfile(folder[:-1])
    t2 = time.perf_counter()
    print(f'Completed in {t2-t1} seconds.')

References/Notes:

I wrote this after watching an async tutorial on YouTube by Andrei Dumitrescu.
The example above is my own
https://docs.aiohttp.org/en/v0.20.0/client.html
https://docs.aiohttp.org/en/stable/client_advanced.html
Data is publicly available

Looks fine to me, you could potentially shave off some repetition, but it's not like it matters that much for a small script.
That said, I'd reuse the session, inline a few variables, fix the naming (constants should be uppercase) and use some standard library tools like os.path to make the script more error-proof.  The list return type could also be more concrete and mention that it's a list of strings for example.
Also, and that's a bit more important, I'd rather not have a script slurp in the whole HTTP response like that!  Who knows how big the files are, plus it's wasteful ... better to stream the response directly to disk!  Thankfully the APIs do exist, using .content.iter_any (or _chunked if you prefer to have a fixed chunk size) you can just iterate over each read data block and write them to the file like so:
import time
import os
import os.path
from bs4 import BeautifulSoup as bs
import aiohttp
import aiofiles
import asyncio
import urllib
import typing

STATISTICS_URL = 'https://digital.nhs.uk/data-and-information/publications/statistical/mental-health-services-monthly-statistics'
OUTPUT_FOLDER = 'C:/Users/<User>/OneDrive/Desktop/testing/'

async def fetch_download_links(session: aiohttp.ClientSession, url: str) -> typing.List[str]:
    r = await session.get(url, ssl=False)
    soup = bs(await r.text(), 'lxml')
    link = 'https://digital.nhs.uk' + soup.select_one('.cta__button')['href']

    r = await session.get(link, ssl=False)
    soup = bs(await r.text(), 'lxml')
    return [i['href'] for i in soup.select('.attachment a')]

async def place_file(session: aiohttp.ClientSession, source: str) -> None:
    r = await session.get(source, ssl=False)

    file_name = urllib.parse.unquote(source.split('/')[-1])
    async with aiofiles.open(os.path.join(OUTPUT_FOLDER, file_name), 'wb') as f:
        async for data in r.content.iter_any():
            await f.write(data)

async def main():
    async with aiohttp.ClientSession() as session:
        urls = await fetch_download_links(session, STATISTICS_URL)
        await asyncio.gather(*[place_file(session, url) for url in urls])

if __name__ == '__main__':
    t1 = time.perf_counter()
    print('process started...')
    asyncio.get_event_loop().run_until_complete(main())
    os.startfile(OUTPUT_FOLDER[:-1])
    t2 = time.perf_counter()
    print(f'Completed in {t2-t1} seconds.')


Confirming safety of SQL injection I believe with everyone's help on Stack Overflow, I got my code safe guarded from SQL injection. I'm trying to confirm that is correct, just in case I misinterpreted the help and advice I received.

<?php
$username = "xxxx";
$password = "xxxx";
?>

xxxx

<form name="form" action="xxxx.php" method="get">
<input type="text" name="q" size="60" />
<input type="submit" name="Submit" value="Search">
</form>

<?

$var = $_GET['q'];

try {
$conn = new PDO('mysql:host=localhost;dbname=xxxx', $username, $password);
$stmt = $conn->prepare("SELECT * FROM xxxx WHERE xxxx LIKE :search");
$stmt->execute(array(':search' => '%'.$var.'%'));

$result = $stmt->fetchAll();

if ( count($result) ) {
foreach($result as $row) {
echo "
"
. $row['xxxx'] .
"	"
. $row['xxxx'] .
"	"
. $row['xxxx'] .
"	"
. $row['xxxx'] .
"	" .
"	http://www.xxx.com/" . $row['image'] . "

<form action=xxx.php method=POST>
<input type=hidden value=" . $row['id'] . " name=edit>
<input type=image src=http://www.xxx.com/images/edit.png height=30>
</form>
	      <form action=xxx.php
method=POST>
<input type=hidden value=" . $row['id'] . " name=delete>
<input type=image src=http://www.xxx.com/images/delete.png height=30>
</form>

";
}
}
else {
echo "No results found.";
}
}
catch(PDOException $e) {
echo 'ERROR: ' . $e->getMessage();
}
?>

You should not mix PHP logic and HTML. All logic should be placed on top of the document and HTML (+ some minor PHP loops/echos/ifs) below it;
$var = $_GET['q']; is unneeded and you loose memory;
never use the shorttag (<?);
You forgot to set the errormode for PDO, that means it doesn't throw exceptions. To set it, use PDO::setAttribute:

$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

I do not recommend to use * in your queries. The query will be faster and easier to use if you specify which columns you want;
You should specify the fetch style of PDOStatement::fetchAll. Otherwise it defaults to PDO::FETCH_BOTH, which means your array get to big and your script slower. You can specify it with a parameter in the function:

$stmt->fetchAll(PDO::FETCH_ASSOC);

Or specify it for the PDOstatement:

$stmt->setFetchMode(PDO::FETCH_ASSOC);

A small tip: Indent your code within each block with a tab or 4 spaces, that makes your code much easier to read. Also, choose a coding standard and use it everywhere. Consistent scripts are a pleasure to write, read and use!

Some none PDO/PHP things:

The <font> element is outdated. You should do this with CSS;
You should not use inline CSS (like style=, align=, width=), put it in a CSS stylesheet instead;
When you are using 2 <br> next to eachother, you almost always know you do something wrong and you should take a look at how to solve it with CSS margins/paddings.


PHP Custom $_GET checker function performance I'm trying to follow the DRY programming philosophy and I am tired of running isset() and an if not empty statements for each time I want to check a $_GET[] variable.

I am working on a page that receives up to 4-5 GET parameters in the URL, so I wrote the following function to reduce repetitive code:

function getset( $get, $default, $empty=FALSE ) {
   // checks if a GET parameter is set
   if ( isset($_GET[$get]) ) {
      // optionally allows empty values
      if ($empty) { return $_GET[$get]; }
      // check if value is not empty
      else {
         if ( $_GET[$get] != '' ) { return $_GET[$get]; }
         // otherwise return default value
         else { return $default; }
      }
   }
   // if param not set then return default value
   else { return $default; }
}

// set the variables
$foo = getset('foo', 'abc');
$bar = getset('bar', 'xyz', TRUE)

// localhost/?foo    => returns 'abc'
// localhost/?foo=x  => returns 'x'
// localhost/?bar    => returns TRUE / ''

I am wondering if my function is proper, efficient and safe compared to the alternative:

// default values
$foo = 'abc';
$bar = '';

// set $foo from $_GET value
if ( isset($_GET['foo']) && $_GET['foo'] != '' ) {
    $foo = $_GET['foo'];
}
// set $bar from $_GET value
if ( isset($_GET['bar']) ) {
    $bar = $_GET['bar'];
}

I am open to suggestions for improving/refining my code.

If your aim is to go DRY, I would consider taking it one step further. You are writing a function for a common use case indeed, but one could wonder if this is only used for $_GET variables. What about $_POST or $_SESSION or some none magic array your application uses...

I'm used to working with Laravel, and this framework offers a very basic support method called array_get (docs here). I use it so often, it should really be part of core php.

Allow me to rewrite your function to this API. (Note that I'll leave . syntax for nested arrays out, though it has proven useful to me it would take us to far.)

function array_get($array, $key, $default = null) {
  if (! array_key_exists($key, $array)) {
    return $default;
  }

  if (is_null($array[$key])) {
    return $default;
  }

  return $array[$key];
}

// usage
$get = array_get($_GET, 'myKey', 'default');
$post = array_get($_POST, 'myKey', 'default');
$session = array_get($_SESION, 'myKey', 'default');
$value = array_get($myArray, 'myKey', 'default');

I'm not sure about you $empty parameter. It doesn't feel entirely right to me to have it in there, but I may be wrong.

If you decide to stick with your function, try to clean it up a bit.

I feel the comments are obsolete when you are performing such trivial, perfectly readable tasks.
All the else statements are obsolete, they just clutter the code.
Nesting control structures is rarely a good idea. It makes the code harder to read. You'll be surprised how rarely you have to nest control structures if you take this rule into account. If you have to, you are probably doing something else wrong (single responsibility? extract another function perhaps)
Nice job on the early returns!

Have a look at this:

function getset( $get, $default, $allowEmpty=FALSE ) {
    if (! isset($_GET[$get]) ) {
        return $default;
    }

    if ($allowEmpty) {
        return $_GET[$get];
    }

    if ( $_GET[$get] === '' ) {
        return $default;
    }

    return $_GET[$get];
}

update:
Concerning your question about the $allowEmpty, I think you are approaching the problem from the wrong angle. Either you want a variable to have a value, or you want to default a missing variable.

// default
$foo = array_get($_GET, 'foo', 'default');

// required
$bar = array_get($_GET, 'bar');
if (is_null($bar)) {
  // required var missing
}

I feel cramming that $allowEmpty into the function violates he SRP somewhat. You are not just getting a value from an array anymore, you are validating it as well.

Do feel free to add it however, it is your code after all. It should be as simple as adding a fourth parameter to the array_get function and checking it similar to the way I provided in the refactored getset function.


SQL query to select most recent version I have the following table definition:

CREATE TABLE ProductPointValues (
    id          INT,
    effective   DATE,
    minPoints   INT,
    maxPoints   INT,

    CONSTRAINT PK PRIMARY KEY (id, effective),
    CONSTRAINT FK FOREIGN KEY (id) REFERENCES Products(id)
)
;

INSERT INTO ProductPointValues
VALUES
    (1, '1/1/2015', 100, 200),
    (1, '2/1/2015',  50, 250),
    (1, '3/1/2015',   0, 300),
    (1, '1/1/2016', 500, 900)

And am running this query to select values for the most recent version of an id:

SELECT P.id, P.minPoints, P.maxPoints
FROM ProductPointValues P
INNER JOIN
    (
        SELECT id, MAX(effective) effective
        FROM ProductPointValues
        WHERE effective <= GETDATE()
        GROUP BY id
    ) T
ON P.id = T.id AND P.effective = T.effective

returns

id          minPoints    maxPoints
----------- -----------  -----------
1           0            300

is there an equivalent, more elegant way to write this using aggregate functions?

You can use a sub-query:

declare @tbl table (
    id          INT,
    effective   DATE,
    points      INT)

INSERT INTO @tbl
VALUES
    (1, '1/1/2015', 123),
    (1, '2/1/2015', 234),
    (1, '3/1/2015', 345),
    (2, '4/1/2015', 123),
    (2, '5/1/2015', 234),
    (2, '6/1/2015', 345)

select * from @tbl t1
where effective = (select max(effective) from @tbl t2 where t1.id = t2.id)


Dynamic Multi-threading lock with while loop I have already asked a question about a dynamic multi threaded lock, and the reviewer found a problem.

Here is the description of the problem that I need to solve:

  I need to create a structure to control dynamically created locks. For example, I have 100 threads running, and each thread will "process" a "key". Sometimes two threads could be processing the same "key", and if that occurs, a dynamic lock needs to be created and used by both, so only one thread can process the key at a time.

I rewrote my code again with a new idea, and I would like you to review it for possible bugs/leaks caused by multithreaded processing.

//Implementation of the class who will block to two process don't execute if they have the same key.
public class DynamicLockGenerator
{
    private static Dictionary<string, object> _locksByKeys = new Dictionary<string, object>();

    private static object _lock = new object();

    public static void ExecuteLock(string key, Action action)
    {
        try
        {
            bool isFree = true;

            do
            {
                lock (_lock)
                {
                    if (!_locksByKeys.ContainsKey(key))
                        _locksByKeys.Add(key, new GuidLock(Guid.NewGuid()));

                    isFree = Monitor.TryEnter(_locksByKeys[key]);
                }

                if (!isFree)
                {
                    Thread.Sleep(50);
                }

            } while (!isFree);

            action.Invoke();
        }
        catch (Exception ex)
        {
            throw ex;
        }
        finally
        {
            Monitor.Exit(_locksByKeys[key]);
        }
    }
}

//This is the method call.
DynamicLockGenerator.ExecuteLock(key, () => SomeMethodToExecuteAlone());

Just one question : when do you release the key ?
Suppose twice the same key is called almost simultaneously, so that one of the two threads involved will go through the

  Thread.Sleep(50) ;

line ..
The first thread enters the lock, the key is added to the dictionary, fine.
At almost the same time the other thread enters the lock, find the key already there and go sleeping.
When the first thread exit, if you don't release the key, the other thread will wait a long long time ..
May be something like

_lockByKeys.Remove(key)

is enough to solve this one.

After this, reading again this code, I found a dictionary, a lock and a monitor. I'm quite sure that a ConcurrentDictionary could make the same job, with a cleaner code if not faster.

Lastly, 100 threads with some of them blocked could become a production nightmare if too much keys have the same value.

Hope this help !


Print worksheets depending on checkbox. Slower than before I have an userform that has check boxes where user can decide which worksheets to print.

Code in userform:

Sub PrintOK_Click()

PrintSelection = Summary.value + _
CCSummary.value + _
DivSalaries.value + _
CCSalaries.value + _
PrintAll.value _

' Displays warning box if no report is selected
If PrintSelection = 0 Then
    MsgBox "No Report(s) selected. Please select Report(s) to print.", 0 + vbExclamation, "Incomplete Request"
End If

' Check to see which worksheets have been selected and
' Run print modules for selected worksheets
    If Summary.value = -1 Then
        Call setup("Summary")
    End If

    If CCSummary.value = -1 Then
        Call setup("Monthly CC Sum")
    End If

    If DivSalaries.value = -1 Then
        Call setup("Div Sal Sum")
    End If

    If CCSalaries.value = -1 Then
        Call setup("CC Sal Sum")
    End If

    If PrintAll.value = -1 Then
        Call setup("PrintAll")
    End If

' Close Form box
End
End Sub

Userform selection is passed into the following. Takes the worksheet(s) selected and stores into array.

Sub setup(Optional tabs As String)
'store sheets into array

Dim arr() As Variant

'called from printer icon on activesheet
If tabs = "" Then
    arr = Array(ActiveSheet.Name)

'called via checkbox from print menu
ElseIf tabs = "PrintAll" Then
    arr = Array("Summary", "Monthly CC Sum", "Div Sal Sum", "CC Sal Sum")
Else
    arr = Array(tabs)
End If

Call goPrint(arr)

End Sub

Which then passes the array to another sub to do the printing

Sub goPrint(tabArr As Variant)

Dim ws As Worksheet
Dim area As String
Dim title As String

'loop through array to print each sheet
For i = 0 To UBound(tabArr)

    Set ws = Worksheets(tabArr(i))

    With ws

        If .Name = "Summary" Then
            area = "PRNSUMMARY"
            title = "$1:$9"
        ElseIf .Name = "Monthly CC Sum" Then
            area = "PRNMONTHLYCCSUM"
            title = "$2:$16"
        ElseIf .Name = "Div Sal Sum" Then
            area = "PRNDIVSALSUM"
            title = ""
        ElseIf .Name = "CC Sal Sum" Then
            area = "PRNCCSALSUM"
            title = "$2:$16"
        End If

        With .PageSetup
            .PrintArea = area
            .PrintTitleRows = title
            .PrintTitleColumns = ""
            .LeftHeader = ""
            .CenterHeader = ""
            .RightHeader = ""
            .LeftFooter = Year(Now()) & " Budget:   &F"
            .CenterFooter = "Page &P of &N"
            .RightFooter = "&D:   &T"
            .LeftMargin = Application.InchesToPoints(0.25)
            .RightMargin = Application.InchesToPoints(0.25)
            .TopMargin = Application.InchesToPoints(0.5)
            .BottomMargin = Application.InchesToPoints(0.5)
            .HeaderMargin = Application.InchesToPoints(0)
            .FooterMargin = Application.InchesToPoints(0.25)
            .PrintHeadings = False
            .PrintGridlines = False
            .PrintComments = xlPrintNoComments
            '.PrintQuality = 600
            .CenterHorizontally = False
            .CenterVertically = False
            .Orientation = xlLandscape
            .Draft = False
            .PaperSize = xlPaperLetter
            .FirstPageNumber = xlAutomatic
            .Order = xlDownThenOver
            .BlackAndWhite = False
            .Zoom = False
            .FitToPagesWide = 1
            .FitToPagesTall = False
        End With

        'Debug.Print tabArr(i)
        'Debug.Print area
        .PrintOut Copies:=1, Collate:=True

    End With

Next

End Sub

There's a delay of maybe two seconds after pressing OK before the printer starts to print. The previous (much longer) code had no discernible delay. How to improve speed?

(Previous code had a different sub for each worksheet thus repeating the pagesetup code block multiple times.)

(Previous code had a different sub for each worksheet thus repeating the pagesetup code block multiple times.)

Was the previous code also working directly with Worksheet objects by any chance?

You're working with an array of strings / sheet names...

'called from printer icon on activesheet
If tabs = "" Then
    arr = Array(ActiveSheet.Name)

'called via checkbox from print menu
ElseIf tabs = "PrintAll" Then
    arr = Array("Summary", "Monthly CC Sum", "Div Sal Sum", "CC Sal Sum")
Else
    arr = Array(tabs)
End If

...and then creating worksheet objects as you iterate the array:

'loop through array to print each sheet
For i = 0 To UBound(tabArr)

    Set ws = Worksheets(tabArr(i))

Why not work with an array of Worksheet objects instead, and simply iterate that array?

If tabs = vbNullString then
    arr = Array(ActiveSheet)
ElseIf tabs = "PrintAll" then
    arr = Array(SummarySheet, MonthlySummary, DivSalSumSheet, CCSalSumSheet)
Else
    sheetNames = Split(tabs, ",")
    arrLength = UBound(sheetNames)
    ReDim arr(0 To arrLength)
    For i = 0 To arrLength
        arr(i) = ThisWorkbook.Worksheets(sheetNames(i))
    Next
End If

That's still brittle though - ideally you'd have the worksheet references themselves handy, ready to be passed to that goPrint method.

The goPrint method could then simply iterate the object array with a simple For Each:

For Each ws In tabArr

This code isn't ideal either; your previous code was also a little faster because it didn't need to check this condition:

    If .Name = "Summary" Then
        area = "PRNSUMMARY"
        title = "$1:$9"
    ElseIf .Name = "Monthly CC Sum" Then
        area = "PRNMONTHLYCCSUM"
        title = "$2:$16"
    ElseIf .Name = "Div Sal Sum" Then
        area = "PRNDIVSALSUM"
        title = ""
    ElseIf .Name = "CC Sal Sum" Then
        area = "PRNCCSALSUM"
        title = "$2:$16"
    End If

If the sheet name is CC Sal Sum then you accessed and compared the worksheet's name 4 times already. And if it's something else, ...you get whatever you get.

This would be a nice case for a Select Case statement:

Select Case .Name
    Case "Summary"
        '...
    Case "Monthly CC Sum"
        '...
    Case "Div Sal Sum"
        '...
    Case "CC Sal Sum"
        '...
    Case Else
        'handle unexpected input?
 End Select

At runtime, a Select Case block pretty much amounts to a conditional GoTo jump, which means no matter how many cases you have, you'll only ever evaluate the condition once.

A note about this:

' Close Form box
End

That's the big red "NUKE 'EM ALL" button. Sure, End will close the form box. But it will also end the macro. It's very very very rare that you actually need to do this.

Compare to:

'Close form box
Me.Hide

Is that comment even needed then?

Structurally, the main issue I'm seeing is that the code is relying on side-effects: the form calls setup, and then closes. What's setup doing then? Shouldn't it set things up for printing?

Oh, it does... but it's also calling goPrint - which is a side-effect that makes it much harder to write a test for the method and see if it works as intended. Everything is coupled, and the UI is running the show.

What if you flipped things around instead, and controlled things from the outside?

Sub PromptAndPrint()

    Dim model As New SelectionModel
    model.Selection = "PrintAll"

    Dim view As New SelectionForm
    Set view.ViewModel = model
    view.Show vbModal

    If view.IsCancelled Then Exit Sub

    ConfigurePageSetup model.SelectedWorksheets
    PrintSheets model.SelectedWorksheets

End Sub

That's right: the UI becomes nothing more than a presentation tool; the execution flow is controlled by the macro, not by the UI, and then you have some SelectionModel class responsible for encapsulating the data you want to pass around, and abstract away the sheet names -> worksheet objects transformation; you can test ConfigurePageSetup without wasting a sheet of paper, and you can test the whole SelectionModel functionality without even showing the UI, without configuring page setup and without wasting a sheet of paper: VBA is much more object-oriented than people generally want to admit.

Nitpicks:

Indentation isn't always consistent. Consider using VBE add-ins like the latest MZ-Tools or Rubberduck, which offer automatic indentation (disclaimer: I'm heavily involved in Rubberduck).
Member names don't follow common naming conventions. Use PascalCase, not camelCase, for public members; that's consistent with every single type library and object model available to VBA.

Also:

Consider using explicit access modifiers. Procedures are implicitly Public by default.
Consider using explicit parameter modifiers. Parameters are passed ByRef unless specified otherwise.
Consider extracting the margins to a user-defined type, or at least a few constants.
Consider removing the Call keyword, which is obsolete/useless (and then, remove the parentheses).
Consider using the zero-footprint vbNullString built-in constant, instead of an empty string literal that eats up a whole 2 bytes for no reason.


FizzBuzz encapsulation In Ruby (identifying and separating concerns) I'm working on an article about encapsulation in OOP, and I'm using the FizzBuzz problem to demonstrate what I think is so great about it. I'm hoping to share this with developers that might also be intermediate, as a way to start building a bridge for understanding common OO principles.

To this end, any feedback, criticism, or questions are greatly appreciated.

The concerns I'm trying to extract and encapsulate are:

The printing of the numbers
The iteration of the collection
The logic for determining what should be returned

class FizzNumber

  FIZZ_MAP = {
    "Fizzbuzz" => :fizzbuzz?,
    "Fizz" => :fizz?,
    "Buzz" => :buzz?
  }

  def initialize(number)
    @number = number
  end

  def fizz?
    @number % 3 == 0
  end

  def buzz?
    @number % 5 == 0
  end

  def fizzbuzz?
    fizz? && buzz?
  end

  def fizz_value
    value = FIZZ_MAP.find { |key, value| send(value) }
    value&.first || @number.to_s
  end
end

class FizzBuzzer
  include Enumerable

  def initialize(collection)
    @collection = collection
  end

  def each
    @collection.each do |item|
      yield FizzNumber.new(item)
    end
  end
end

fizzer = FizzBuzzer.new((1..100).to_a)
puts fizzer.map(&:fizz_value).join("\n")

To this end, any feedback, criticism, or questions are greatly appreciated.

Here are some ideas to improve the code.
Split printing and logic
Might be debatable but from my POV the

The printing of the numbers

and

The logic for determining what should be returned

are not split.
One of the goals of object oriented design is to make the code easier to change. So imagine we want to implement now to print Boom if a number number % 10 == 0. How would we do this? We would need to implement a BoomNumber class and change FizzBuzzer and might even change the main method because fizz_value does not make sense anymore. Not good!
In this example we can just swap out the builder and it would work.
class PrintableNumber
  def initialize(name)
    @name = name
  end

  def to_s
    @name
  end
end

class BoomNumberBuilder
  def initialize(number)
    @number = number
  end

  def build
    PrintableNumber.new(name)
  end

  private

  attr_reader :number

  def name
    if boom?
      "Boom"
    else
      number
    end
  end

  def boom?
    number % 10 == 0
  end
end

class FizzNumberBuilder
  def initialize(number)
    @number = number
  end

  def build
    PrintableNumber.new(name)
  end

  private

  attr_reader :number

  def name
    if fizzbuzz?
      "FizzBuzz"
    elsif fizz?
      "Fizz"
    elsif buzz?
      "Buzz"
    else
      number
    end
  end

  def fizzbuzz?
    fizz? && buzz?
  end

  def fizz?
    number % 3 == 0
  end

  def buzz?
    number % 5 == 0
  end
end

class PrintableNumberCollector
  include Enumerable

  def initialize(collection, builder = FizzNumberBuilder)
    @collection = collection
    @builder = builder
  end

  def each
    collection.each do |item|
      yield builder.new(item).build
    end
  end

  private

  attr_reader :collection, :builder
end

puts "== Fizzer =="
fizzer = PrintableNumberCollector.new((1..10).to_a)
puts fizzer.map(&:to_s).join("\n")

puts "== Boomer =="
boomer = PrintableNumberCollector.new((1..10).to_a, BoomNumberBuilder)
puts boomer.map(&:to_s).join("\n")

 Implement common interface
In your FizzNumber class you implement a fizz_value method which you then use in your main fizzer.map(&:fizz_value).join("\n"). This creates an unnecessary coupling. If you look into the method, you can already see what the method name should be number.to_s
  def fizz_value
    value = FIZZ_MAP.find { |key, value| send(value) }
    value&.first || @number.to_s
  end
end

so a better function name would be
def to_s
  value&.first || @number.to_s
end

private

def value
  FIZZ_MAP.find { |key, value| send(value) }
end

Avoid meta programming
Try to avoid meta programming (send) if not absolutely necessary. It makes the code a lot harder to read. You only have 4 different cases which can easily reflected with an if else.
  def to_s
    if fizzbuzz?
      "FizzBuzz"
    elsif fizz?
      "Fizz"
    elsif buzz?
      "Buzz"
    else
      number
    end
  end

Use getter and setter
Instead of using instance variables @collection you should try to use getter / setter methods. If you need to do e.g. validations or some processing of the variable, you only need to change one place.
class FizzBuzzer
  attr_reader :collection

  def each
    collection.each do |item|
      yield FizzNumber.new(item)
    end
  end
end

Use simple delegator
Instead of implementing the each method you could also use the SimpleDelegator module.
https://ruby-doc.org/stdlib-2.5.1/libdoc/delegate/rdoc/SimpleDelegator.html


Check if the given string has duplicates Description:

Given a string find if it contains duplicate characters. The string contains only ASCII characters.

Code:

import java.util.Arrays;

class Main {
  /**
   * Given a string check if it duplicate characters.
   * []        -> true
   * [f, o, o] -> false
   * [b, a, r] -> true
   *
   * @param {String} str
   *
   * @return false if string contains duplicate else true
   */
  public static boolean isUnique(String str) {
    if (str.length() == 0) {
      return true;
    }
    boolean[] seen = new boolean[25];
    for (int i = 0; i < str.length(); i++) {
      int index = Character.toLowerCase(str.charAt(i)) - 'a';
      if (seen[index]) {
        return false;
      }
      else {
        seen[index] = true;
      }
    }
    // invariant
    return true;
  }

  public static void main(String[] args) {
    System.out.println(isUnique(""));       // true
    System.out.println(isUnique("a"));      // true
    System.out.println(isUnique("foo"));    // false
    System.out.println(isUnique("bar"));    // true
    System.out.println(isUnique("hello"));  // false
    System.out.println(isUnique("Hello"));  // false
  }
}

The above algorithms runs in O(n), the alternate solution is to sort the string and the check the adjacent elements but it would be O(nlogn). I need to understand if I am using Java correctly, any other styles related comments are most welcome.

PS: I would like to know if I need to include more tests to cover all cases.

Documentation mismatch

The documentation and the functionality do not fit. Your function is supposed to "check if [a string] has duplicates". From the documentation, I expect a function hasDuplicates that returns true if I have duplicate characters and false otherwise.

However, you provide isUnique, which does the opposite: return true if there are no duplicates and false if there are duplicates. The internal documentation fits, but I would say "checks whether it contains no duplicate characters" or "contains unique characters only".

Possible bugs

You checked your string only on alphabetic strings. What happens on "12345678"? You will index -48 due to '1' - 'a'. That's a bug waiting to happen. Also, what happens if your string contains a 'z'? What happens if str is null? Maybe str shouldn't be null, but that should get mentioned in the documentation (see above).

Also, you should mention that your function is case-insensitive. But why does "Aa" count as a duplicate to begin with?

Both cases (non-alpha character and same letters in different cases) should get added to your tests.

Algorithm

Your seen algorithm is fine if you know exactly how many buckets you need. But you often don't know that. A Map<char,bool> can happen here. If you use a TreeMap you end up with the mentioned \$\mathcal O(n \log n)\$, but if you use a HashMap the asymptotic complexity is \$\mathcal O(n)\$ again. Alternatively, use a set.

But that's asymptotic complexity. If you don't consider any non-alpha strings (see section above), then you can immediately return false if the string is longer than 26 characters due to the pigeon hole principle.


Python snippet for finding word in string I wrote a simple function that finds a word in a string without use of the re module.

My function looks like this:

def find_in_string(string, word):
    Indices = ()
    wordlength = len(word)
    for i in range(0, len(string)):
        if string[i:i + wordlength] == word:
            Indices += (i,)
    return Indices

Is there any way to improve it?

I don't like the name of your function. Find what in string? Something like find_all_substring_indices() better describes what the function does
You aren't finding words, you're finding substrings. Your code would find the word "car" twice in "carcar" even though we would consider "carcar" a single word. You mention the re package. Presumably, you're talking about word boundaries. They behave a little differently (punctuation can be a word separator). But, since your original question doesn't behave that way, it is not a refactoring for me to suggest how to do that (nor is it on topic, "how do I replicate regex word boundaries?" is a question for StackOverflow)
Indices should be snake_case. Only class names are UpperCamelCase
Don't use a tuple for indices! Every time you append to it you have to create a new object because tuples are immutable. In fact, you are creating two: one for the (i,) and then another one when you do indices + (i,). Use a list: indices = [] then indices.append(i)! That's what they're meant for!
range(0, len(string)) can just be range(len(string))
string[i:i+wordlength] == word is the same thing as (and much less clear than) string[i:].beginswith(word)
All of this slicing is really inefficient, especially given there is also a string method that does what you want: index
You should unittest your code! I suspect you have a bug in that if your "word" is "FooFoo" it will be found twice in "Little bunny FooFooFoo" (once at the beginning of "FooFooFoo" and once three characters into "FooFoo"). Surely, words shouldn't be able to overlap like this.
Add """Docstrings.""" to document what your function does.

You should just be repeatedly using index until you read the end of the string. This is more efficient than doing slicing by yourself or sliding the substring character by character along your string. Stealing mostly from SO:

def find_all_substrings(string, sub):
    start = 0

    while True:
        try:
            start = string.index(sub, start)
        except ValueError:
            break

        yield start
        start += len(sub)

This is a generator, if you still want a tuple returned you can just build one up instead (but at a list first!):

def find_all_substrings(string, sub):
    indices = []
    start = 0

    try:
        while True:
            start = string.find(sub, start)
            indices.append(start)
            start += len(sub)
    except ValueError:
        pass

    return tuple(indices)


Formatting as a table in Scala I'm trying to get my head in the Scala game, and would welcome feedback on the following code, which produces a table from data.

class TabulatorTest extends FunSuite with ShouldMatchers {

  test("format") {
    Tabulator.format(List(
      List("head1", "head2", "head3"),
      List("one", "two", "three"),
      List("four", "five", "six"))) should be ("""
+-----+-----+-----+
|head1|head2|head3|
+-----+-----+-----+
|  one|  two|three|
| four| five|  six|
+-----+-----+-----+
""".trim)
  }

  test("format empty") {
    Tabulator.format(List()) should be ("")
    Tabulator.format(List(List())) should be ("""
++
||
++
++
""".trim)
  }

  test("uneven rows") {
    try {
      Tabulator.format(List(
        List("head1", "head2", "head3"),
        List("one", "two")))
      fail()
    } catch {
      case e: IllegalArgumentException =>
    }
  }
}

object Tabulator {

  def format(table: Seq[Seq[Any]]) = table match {
    case Seq() => ""
    case _ =>
      val cellSizes = for (row <- table) yield
        (for (cell <- row) yield
          if (cell == null) 0 else cell.toString.length)
      val colSizes = for (col <- cellSizes.transpose) yield col.max
      val rows = for (row <- table) yield formatRow(row, colSizes)
      formatRows(rowSeparator(colSizes), rows)
  }

  def formatRow(row: Seq[Any], colSizes: Seq[Int]) = {
    val cells = (for ((item, size) <- row.zip(colSizes)) yield
      if (size == 0) "" else ("%" + size + "s").format(item))
    cells.mkString("|", "|", "|")
  }

  def formatRows(rowSeparator: String, rows: Seq[String]): String = (
    rowSeparator ::
    rows.head ::
    rowSeparator ::
    rows.tail.toList :::
    rowSeparator ::
    List()).mkString("\n")

  private def rowSeparator(colSizes: Seq[Int]) =
    colSizes map { "-" * _ } mkString("+", "+", "+")
}

I can't help a lot. Only some syntactic hints:

object Tabulator {

  def format(table: Seq[Seq[Any]]) =
    if (table.isEmpty) ""
    else {
      def cellSize(cell: Any) =
        cell.toString.length

      val cellSizes =
        table map { _ map cellSize }
      val colSizes =
        cellSizes.transpose map { _.max }
      val rows =
        table map formatRow(colSizes)

      formatRows(rowSeparator(colSizes), rows)
    }

  def formatRow(colSizes: Seq[Int])(row: Seq[Any]) = {
    val cells =
      for ((item, size) <- row zip colSizes) yield
        if (size == 0) "" else "%"+size+"s" format item
    cells mkString ("|", "|", "|")
  }

  def formatRows(rowSeparator: String, rows: Seq[String]): String = (
       rowSeparator
    :: rows.head
    :: rowSeparator
    :: rows.tail.toList
    ::: rowSeparator
    :: Nil
  ) mkString "\n"

  private def rowSeparator(colSizes: Seq[Int]) =
    colSizes map { "-"*_ } mkString ("+", "+", "+")
}

I deleted the null-check in cellSize because null is not familiar in Scala. If you definitely need null then restore the check.


Bash script that checks for package dependencies and installs them if necessary I'm new to writing bash scripts and was wondering if I could get someone's advice on a part of the script I'm working on.

Intended purpose of code

Check if a package exists using dpkg, and if it doesn't, offer to install it for the user. This snippet is part of a larger script that installs a particular Conky configuration along with all of its dependencies with minimal effort from the user.

Concerns

I feel as though there is a more elegant way to check if a package is installed using dpkg (code was found on Stack Overflow).
Is there a better way of handling the (y/n) response?

Here is the code that I am using:

declare -a packages=("conky-all" "lm-sensors");

for i in "${packages[@]}"; do
    if [ $(dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -c "ok installed") -eq 0 ]; then
        echo "$i is not installed, would you like to install it now? (Y/N)";
        read response
        if [ "$response" == "y" ] || [ "$response" == "Y" ]; then
            sudo apt-get install "$i";
        else
            echo "Skipping the installation of $i...";
            echo "Please note that this Conky configuration will not work without the $i package.";
        fi
    else
        echo "The $i package has already been installed.";
    fi
done

Instead of this:

    if [ $(dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -c "ok installed") -eq 0 ]; then

A better way to write the same thing:

    if ! dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -q "ok installed"; then

Instead of this:

        if [ "$response" == "y" ] || [ "$response" == "Y" ]; then

A simpler way to write is:

        if [[ $response == [yY]* ]]; then

This is not example the same. It will match anything that starts with "y" or "Y". If you want to match strictly only those letters, just drop the * from the pattern:

        if [[ $response == [yY] ]]; then

Finally,
all the ; at line endings are unnecessary.
The purpose of ; is to separate multiple statements on the same line.
Line breaks naturally serve as statement separators.


Best way to split array to two sub-arrays, with maximum absolute difference between them Given array a, we want to find an index k in range [0, a.length-1] such that:

Math.abs( (a[0]+a[1]+...a[k-1]) - (a[k] + a[k+1] + ... + a[a.length-1]) )

is maximal (in case k == 0, the result above is the sum of a elements).

Below is the code I wrote.
Please tell me your opinion about it - run time, correctness and coding conventions.

public static int sumArray(int[] a) {
    int sum = 0;
    int len = a.length;

    for (int i = 0; i < len; i++) {
        sum += a[i];
    }

    return sum;
}

public static int findMaxDif(int[] a) {
    int left = 0;
    int right = sumArray(a);
    int max = Math.abs(right);
    int maxIndex = 0;
    int difI;
    int len = a.length - 1;

    for (int i = 0; i < len; i++) {
        left += a[i];
        right -= a[i];
        difI = Math.abs(left - right);

        if (difI > max) {
            max = difI;
            maxIndex = i + 1;
        }
    }

    return maxIndex;
}

Simple tests that I wrote and work fine:

public static void main (String[] args) {
    int[] a1 = {1,2};
    if (findMaxDif(a1) == 0) {
        System.out.println("test1 pass");
    }

    int[] a2 = {-1,2};
    if (findMaxDif(a2) == 1) {
        System.out.println("test2 pass");
    }

    int[] a3 = {2,-1};
    if (findMaxDif(a3) == 1) {
        System.out.println("test3 pass");
    }

    int[] a4 = {-5,-4,4,3};
    if (findMaxDif(a4) == 2) {
        System.out.println("test4 pass");
    }
}

Some ideas:

You can replace sumArray with IntStream.of(a).sum();.
Tests should be written using JUnit or another testing framework and named according to what they are testing.
Try to write the absolutely most simple test case you can think of rather than following the writer's instinct to vary the input. You want the mental arithmetic to verify your test results to be as simple as absolutely possible. Your tests should be easy to rewrite using just -1, 0 and 1.
shouldPartitionSingleElementListAtStart and shouldPartitionListWithMultipleIdenticalValuesAtStart tests would be useful.
What happens if the input is an empty list? It's up to you whether that should fail like (I think) it does now, but the case should be tested. You might even want to raise a custom NonPartitionableListException or EmptyListException in this case.
"Difference" is usually spelled out fully or, if shortened, written as "Diff" (two "f"s).
Variable names should be understandable at the point they are defined - it should not be necessary to see all their usages to understand what they do. So for example left could be called leftPartitionSum.
On a related note, findMaxDif might be clearer as getMaxPartitionDifferenceIndex. "find" is usually used when you want to return a "non-answer" in case no answer is possible, while "get" is usually used when you want the code to throw an exception if there is no answer.

Other than that my opinion is that this code is short, readable and performant.


YAML Configuration file I have recently started working on a server software in Java and wanted to add a configuration option to the program.  I have gone through a few revisions of this class because I never liked what I came up with.  I finally made something that I'm somewhat happy about and it works better than past versions.  I'm just curious if anything can be done better or if this class should be rewritten again in a different?

FYI: some methods are not done yet. I'm not worried about those methods, I'm asking about what is there so far.

import java.io.File;
import java.util.HashMap;
import java.util.Map;

import org.yaml.snakeyaml.DumperOptions;
import org.yaml.snakeyaml.Yaml;

public class Config
{
public static final String SERVER_NAME = "serverName";
public static final String MOTD = "motd";
public static final String SERVER_PORT = "serverPort";

private static final File file = new File("config.yaml");
private Map<String, Object> config = new HashMap<String, Object>();

private DumperOptions yamlOptions = new DumperOptions();
private Yaml yaml;

private void setDefaults()
{
    config.clear();

    config.put(SERVER_NAME,     "Server");
    config.put(MOTD,            "Welcome to the server!");
    config.put(SERVER_PORT,     21020);
}

public void generate()
{
    setDefaults();
    save();
}

public void load()
{
        //To-do
}

public void save()
{
    //To-do
}

public Object getObject(String key)
{
    return config.get(key);
}

public String getString(String key)
{
    return (String) getObject(key);
}

public int getInt(String key)
{
    return (int) getObject(key);
}

public double getDouble(String key)
{
    return (double) getObject(key);
}
}

Example of usage:

Config.getInt(Config.SERVER_PORT);

So far it's looking quite good, but I have a few comments:

Which file to load?

private static final File file = new File("config.yaml");

In my opinion it would be better to pass the File to load in to the constructor of this Config class, for flexibility. Now perhaps you only need to load a specific file, but in the future you might want to support multiple files. Who says that you always want to load config.yaml? Sometimes you might want to try different configurations, and instead of making backups of your config file and renaming files like crazy, you could instead load config-withsomespecials.yaml

private final File file;
public Config(File file) {
    this.file = file;
}

Speaking of flexibility, your setDefaults class works fine for your server, but you could make your Config class an abstract class, setDefaults as an abstract method and use a ServerConfig as a subclass in which you override this method:

// In your `Config` class:
protected abstract void setDefaults();

// In your `ServerConfig` class:
@Override
protected void setDefaults() {
    config.clear();

    config.put(SERVER_NAME,     "Server");
    config.put(MOTD,            "Welcome to the server!");
    config.put(SERVER_PORT,     21020);
}

These are only suggestions, if you don't want to make it that flexible, that's up to you.

<personal-opinion> As for the choice of using YAML.... it isn't exactly my favorite format. Let me guess, you're a Minecraft / Bukkit player? The Minecraft / Bukkit world is about the only place I've come across this format. If you have the ability to choose, I would personally recommend using Jackson, which is primarily used for JSON, although it has support for XML and even support for YAML. Jackson is wildly used, very flexible, and I think it is more well-documented than Snakeyaml.</personal-opinion>


Minimum Sum Path of Pyramid with Data Structures I have a program that uses ArrayLists to store a pyramid that has been read by a txt file and calculates the minimum sum value of travelling from the top of the pyramid to the bottom.

My code's main method is like this:

File file1 = new File(args[0]);
Scanner reader = new Scanner(file1); // The Scanner that is going to read file1.
try {
    int pyramidSize = reader.nextInt();
    System.out.println("Pyramid Size = " + pyramidSize);
    ArrayList<ArrayList <Integer>> solution1List = new ArrayList<ArrayList <Integer>>(pyramidSize);

    // Reads all integers to an array list.
    for(int i = 0; i < pyramidSize; i++) {
        solution1List.add(new ArrayList<Integer>());
    }
    int row = 0;
    int ctr = 0;
    int rowCounter = 1;

    while(reader.hasNext()) {

        if(reader.hasNextInt()) {

            while(row < rowCounter) {
                int temporaryForSolution1Integer = reader.nextInt();
                solution1List.get(ctr).add(temporaryForSolution1Integer);
                row++;
            }
            row = 0;
            ctr++;
            rowCounter++;
        }
        else {
            reader.next();
            row++;
        }
    }

    System.out.println("Triangle size is = " + solution1List.size());
    System.out.println("The minimum sum path is = " + minimumSumPath(solution1List));

And my method for calculating the sum is like this:

public static int minimumSumPath(ArrayList<ArrayList<Integer>> triangle) {

    if (triangle.size() == 0)
        return 0;

    int size = triangle.size();
    int min = Integer.MAX_VALUE;

    int[] sum = new int[size];
    sum[0] = triangle.get(0).get(0);

    for(int current = 1; current <= size - 1; current++) {
        int next_size = triangle.get(current).size();

        for(int next = next_size - 1; next >= 0; next--) {
            if (next == 0) { // Sum[0] gets done by walking the leftmost direct way.
                sum[0] = sum[0] + triangle.get(current).get(next);
            }
            else if (next == (next_size - 1)) { // Reaches to the rightmost element of that iteration
                sum[next] = sum[next-1] + triangle.get(current).get(next);
            }
            else { // Provides sum[next] to be the minimal sum that can come there
                sum[next] = Math.min(sum[next-1], sum[next]) + triangle.get(current).get(next);
            }
        }
    }

    for(int i = 0; i < size; i++) {
        if(sum[i] < min) {
           min = sum[i];
        }
    }
    return min;
}

My code is working properly. But I wonder if I can do this by implementing with a kind of Tree Data Structure (or other data structure but I thought maybe it could be done with a Tree but I couldn't figure which kind of Tree?). Any advice? I would appreciate any help and recommendation.

I have solved it by creating my own Tree. Here is how it is:

import java.util.LinkedList;
import java.util.Queue;
public class SolutionTree {

    SolutionNode root;

    public SolutionTree() {
        root = null;
    }

    public void addRoot(int rootValue) {
        root = new SolutionNode(rootValue);
    }

    // New Node Adder
    public void addSolutionNode(int newNodeValue) {
        SolutionNode newNode = new SolutionNode(newNodeValue);
        SolutionNode newNodeRoot = breadth(root);
        if(newNodeRoot.getChildLeft() == null) {
            newNodeRoot.setChildLeft(newNode);
            newNode.setParentLeft(newNodeRoot);
        }
        else if(newNodeRoot.getChildRight() == null) {
            newNodeRoot.setChildRight(newNode);
            newNode.setParentLeft(newNodeRoot);
            if(newNodeRoot != root) {
                    if(newNodeRoot.getParentLeft().getChildRight().getChildLeft() == null) {
                        newNodeRoot.getParentLeft().getChildRight().setChildLeft(newNode);
                        newNode.setParentRight(newNodeRoot.getParentLeft().getChildRight());
                    }
            }
        }
    }

    // Node Class of Solution Tree
    protected class SolutionNode {

        private int value;
        private SolutionNode parentLeft;
        private SolutionNode parentRight;
        private SolutionNode childLeft;
        private SolutionNode childRight;

        // Constructor
        public SolutionNode() {
            value = 0;
            parentLeft = null;
            parentRight = null;
            childLeft = null;
            childRight = null;
        }

        // Constructor
        public SolutionNode(int v) {
            value = v;
            parentLeft = null;
            parentRight = null;
            childLeft = null;
            childRight = null;
        }

        // MODIFIERS

        public void setValue(int val) {
            value = val;
        }

        public void setParentLeft(SolutionNode leftParent) {
            parentLeft = leftParent;
        }

    public void setParentRight(SolutionNode rightParent) {
        parentLeft = rightParent;
    }

        public void setChildLeft(SolutionNode leftChild) {
            childLeft = leftChild;
        }

        public void setChildRight(SolutionNode rightChild) {
            childRight = rightChild;
        }

        //ACCESSORS

        public int getValue() {
            return value;
        }

        public SolutionNode getParentLeft() {
            return parentLeft;
        }

        public SolutionNode getParentRight() {
            return parentRight;
        }

        public SolutionNode getChildLeft() {
            return childLeft;
        }

        public SolutionNode getChildRight() {
            return childRight;
        }

    }

    // function to compute the minimum sum path
    // It only returns the sum of the values of nodes on the min sum path
    int minSumPath(SolutionNode current) {
        if(current == null)
            return 0;

        int sum = current.getValue();

        int left_sum = minSumPath(current.childLeft);
        int right_sum = minSumPath(current.childRight);

        if(left_sum <= right_sum) {
            sum += minSumPath(current.childLeft);
        }
        else {
            sum += minSumPath(current.childRight);
        }

        return sum;
    }

    // Breadth First Traversal
    public static SolutionNode breadth(SolutionNode root) {
        Queue<SolutionNode> queue = new LinkedList<SolutionNode>() ;
        if (root == null)
            return null;
        queue.clear();
        queue.add(root);
       while(!queue.isEmpty()){
            SolutionNode node = queue.remove();
            if(node.childLeft != null)
                queue.add(node.childLeft);
            if(node.childRight != null)
                queue.add(node.childRight);
            if(node.childLeft == null || node.childRight == null)
                return node;
        }
        return null;
    }

}

I am invoking minSumPath at the main of my file reading program.

Have a nice day.


Time spent by product in process represented by clockin and clock out This is the code I wrote to find out the time spent by product in process represented by clockin and clock out. Time stamps X and Y (for example, 7/23/2013 7:00 AM and 7/23/2013 8:00 AM) reflect the time period where in I am calculating the time.

X, Y, CLOCK_IN and CLOCK_OUT are all timestamps.

I wrote this code, but I am not a programmer. Is there a better way to do it? Please explain step by step.

It runs against a huge set of data. How can I reduce its runtime?

Here is a calculation I'm running in MS Access 2010:

Sum(

     IIf([CLOCK_IN]<[X] And [X]<[CLOCK_OUT] And [CLOCK_OUT]<[Y],([CLOCK_OUT]-[X])*24*60,0)+
     IIf([CLOCK_IN]<[X] And [Y]=[CLOCK_OUT],60,0)+
     IIf([CLOCK_IN]=[X] And [Y]<[CLOCK_OUT],60,0)+
     IIf([X]<[CLOCK_IN] And [CLOCK_IN]<[Y] And [Y]<[CLOCK_OUT],([Y]-[CLOCK_IN])*24*60,0)+
     IIf([X]<[CLOCK_IN] And [CLOCK_IN]<[CLOCK_OUT] And [CLOCK_OUT]<[Y],([Y]-   [CLOCK_IN])*24*60,0)+
     IIf([CLOCK_IN]<[X] And [X]<[Y] And [Y]<[CLOCK_OUT],60,0)+
     IIf([X]=[CLOCK_IN] And [Y]=[CLOCK_OUT],60,0)+
     IIf([X]=[CLOCK_IN] And [CLOCK_OUT]<[Y],24*60*([CLOCK_OUT]-[X]),0)+
     IIf([X]<[CLOCK_IN] And [CLOCK_OUT]=[Y],24*60*([CLOCK_OUT]-[X]),0))/60)

)

I'll attempt to provide something helpful, even though this is old.

To  start with, just syntax-wise, [brackets] are only required if your column name contains a reserved character, such as a space or apostrophe. It looks less cluttered if you don't use them unless needed, and I don't see any need for it in your columns.
Line breaks and tabs make code much easier to read. I know Access is quite limited in the amount of formatting you can do and doesn't allow comments, but every bit helps.
I notice a lot of redundancy in your Iif statements. 3 statements result in ([CLOCK_OUT]-[X])*24*60. 2 statements result in ([Y]-[CLOCK_IN])*24*60. And 4 statements all result in 60 if true. My thought is to group these together as 3 subqueries and use WHERE instead of Iif to sort out the results.

Here is how I would write this. Note I used Your_Table_Name since your code doesn't say, make sure you replace this with the actual table name.

SUM(
    (
    SELECT ((CLOCK_OUT - X) * 24) * 60
    FROM Your_Table_Name
    WHERE CLOCK_IN < X AND X < CLOCK_OUT AND CLOCK_OUT < Y
    OR X < CLOCK_IN AND CLOCK_OUT = Y
    OR X = CLOCK_IN AND CLOCK_OUT < Y
    )
    +
    (
    SELECT ((Y - CLOCK_IN) * 24) * 60
    FROM Your_Table_Name
    WHERE X < CLOCK_IN AND CLOCK_IN < Y AND Y < CLOCK_OUT
    OR  X < CLOCK_IN AND CLOCK_IN < CLOCK_OUT AND CLOCK_OUT < Y
    )
    +
    (
    SELECT 60
    FROM Your_Table_Name
    WHERE CLOCK_IN < X AND Y = CLOCK_OUT
    OR CLOCK_IN = X AND Y < CLOCK_OUT
    OR CLOCK_IN < X AND X < Y AND Y < CLOCK_OUT
    OR X = CLOCK_IN AND Y = CLOCK_OUT
    )
)