Groovy method optimization to build > tokens I have a string array:

def invoices = [LEDES98BI V2,
LINE|INVOICE_DATE|INVOICE_NUMBER|INVOICE_TOTAL,
1|20150301|INV-Error_Test1|22,
2|20150301|INV-Error_Test1|24,
3|20150301|INV-Error_Test2|26,
4|20150301|INV-Error_Test2|28,]

I tried to convert it to HasMap<String, List<List>> with key as invoice numbers(INV-Error_Test1, INV-Error_Test2) and the values are each invoice line:

[
INV-Error_Test2:[[3,20150301, INV-Error_Test2,26], [4,20150301, INV-Error_Test2,28]],
INV-Error_Test1:[[1,20150301, INV-Error_Test1,22], [2,20150301, INV-Error_Test1,24]]
]

And this is the method which converts the string array into <String, List<List>> tokens:

def extractInvoiceLineItems(def invoices) {
    Map invLineItems = new HashMap<String, ArrayList<ArrayList>>();
    def lineItems = []
    for(int i = 2; i<invoices.length; i++){
       def tokens =  invoices[i].split('\\|') as List
       if(tokens.size != 1) {
        lineItems.add(tokens)
      }
    }

    for (int i=0; i< lineItems.size; i++) {
        invNumber = lineItems.get(i).get(1)
         if(invLineItems.keySet().find{it == invNumber}) {
            templineItem = invLineItems.get(invNumber)
            templineItem.add(lineItems.get(i))
            invLineItems.put(invNumber,templineItem)
         }
        else {
            def list = []
            list.add(lineItems.get(i))
            invLineItems.put(invNumber,list)
        }
    }
invLineItems
}

I am using lots of traditional for loops and am wondering whether it can be simplified further (using closures or any other way).

Yes, it can be simplified further. I divided the process into three steps and added comments and assertions to make it easier to see how it works.

// Simulate converting a CSV file into a list.
def csv =
'''LEDES98BI V2
LINE|INVOICE_DATE|INVOICE_NUMBER|INVOICE_TOTAL
1|20150301|INV-Error_Test1|22
2|20150301|INV-Error_Test1|24
3|20150301|INV-Error_Test2|26
4|20150301|INV-Error_Test2|28'''.split(/\n/)

assert csv == [
    'LEDES98BI V2',
    'LINE|INVOICE_DATE|INVOICE_NUMBER|INVOICE_TOTAL',
    '1|20150301|INV-Error_Test1|22',
    '2|20150301|INV-Error_Test1|24',
    '3|20150301|INV-Error_Test2|26',
    '4|20150301|INV-Error_Test2|28']

/*
 * Ignore the first two csv lines and create a list of lists.
 * The inner lists represent the fields of each record.
 */
def invoices = csv[2..-1].collect { it.tokenize('|') }

assert invoices == [
    ['1', '20150301', 'INV-Error_Test1', '22'],
    ['2', '20150301', 'INV-Error_Test1', '24'],
    ['3', '20150301', 'INV-Error_Test2', '26'],
    ['4', '20150301', 'INV-Error_Test2', '28']]

/*
 * Create a Map by grouping each record by its invoice number.
 */
invoices = invoices.groupBy { it[2] }

assert invoices == [
    'INV-Error_Test1':[
        ['1', '20150301', 'INV-Error_Test1', '22'],
        ['2', '20150301', 'INV-Error_Test1', '24']
    ],
    'INV-Error_Test2':[
        ['3', '20150301', 'INV-Error_Test2', '26'],
        ['4', '20150301', 'INV-Error_Test2', '28']
    ]
]

Here's the same program in a condensed form to give you an idea of just how concisely this can be done:

'''LEDES98BI V2
LINE|INVOICE_DATE|INVOICE_NUMBER|INVOICE_TOTAL
1|20150301|INV-Error_Test1|22
2|20150301|INV-Error_Test1|24
3|20150301|INV-Error_Test2|26
4|20150301|INV-Error_Test2|28'''
    .split(/\n/)[2..-1]
    .collect { it.tokenize('|') }
    .groupBy { it[2] }


Encrypt/decrypt PHP function I have to protect really sensitive information and I have to do it both ways: encryption and decryption. How safe is it?

function encrypt($mprhase) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     $crypted_value = mcrypt_generic($td, $mprhase);
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return base64_encode($crypted_value);
}

function decrypt($mprhase) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     $decrypted_value = mdecrypt_generic($td, base64_decode($mprhase));
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return $decrypted_value;
}

In all reality the safety of the Data can be affected by any part of the transportation process, So just seeing how you encrypt and decrypt the information doesn't really help us help you in determining the safety of the data.

Nothing to do with the Security of the Functions

what you should do is create one function that will accept a parameter of encrypt or decrypt most of the information that you are using in both functions is duplicated.

function encryptOrDecrypt($mprhase, $crypt) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     if ($crypt == 'encrypt')
     {
         $return_value = base64_encode(mcrypt_generic($td, $mprhase));
     }
     else
     {
         $return_value = mdecrypt_generic($td, base64_decode($mprhase));
     }
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return $return_value;
}

obviously some of the variables have been changed to protect the innocent, (in other words you should change the variable $return_value to something more meaningful)

(another note: you might want to fiddle around with the input parameter that I added as well.)

Additional Important Information

As Corbin Pointed out, this should be a class/object that has methods for encrypting and deciphering and the whole works that go along with a class/object, unfortunately I don't know PHP syntax well enough to write it out.

a Function can do this, but it is sloppy coding, and I should have said this from the get go, I apologize.

I will look into the syntax when I have time or you can Visit this Fun page Or you can use ol' reliable


Order and visibility tracking algorithm (e.g. scope visibility) I have a tree of objects.

Object id:0 Object type:0
  Object id:1 Object type:0
    Object id:3 Object type:0
  Object id:2 Object type:0
    Object id:4 Object type:0
    Object id:5 Object type:0

For each object I have a method that returns its "index" in the tree down to the root item.

Indices:

0 - 0
1 - 0 0
2 - 0 1
3 - 0 0 0
4 - 0 1 0
5 - 0 1 1

I need to be able to check for "visibility" from one object to another object, with the rules being pretty much identical to those of C/C++ scopes. E.g. the root item is like a global, so it should be visible to every other items, and items are only visible to items on their level or higher as long as they are in the same branch.

In short:

0 - visible to all
1 - visible to 2, 3, 4, 5
2 - visible to 4, 5
3 - visible to none
4 - visible to 5
5 - visible to none

After some trial and error I came up with this code, and it does seem to work as expected given the above tree hierarchy.

bool isVisibleTo(Node * accessor) {
    QList<uint> accessedI = getIndex();
    QList<uint> accessorI = accessor->getIndex();
    if (accessedI.size() > accessorI.size()) return false; // item is deeper than its accessor
    else if (accessedI.size() == accessorI.size()) { // same size - check if all but the last are compatible
        for (int i = 0; i < accessedI.size() - 1; ++i) if (accessedI.at(i) != accessorI.at(i)) return false; // indecies not identical
        if (accessedI.last() > accessorI.last()) return false; // identical, but accessor index is larger
    }
    for (int i = 0; i < accessorI.size() - (accessorI.size() - accessedI.size()); ++i) {
        if (accessedI.at(i) > accessorI.at(i)) return false; // wrong subtree
    }
    return true;
}

test output:

node  0 is visible to node  0
node  0 is visible to node  1
node  0 is visible to node  2
node  0 is visible to node  3
node  0 is visible to node  4
node  0 is visible to node  5
node  1 is visible to node  1
node  1 is visible to node  2
node  1 is visible to node  3
node  1 is visible to node  4
node  1 is visible to node  5
node  2 is visible to node  2
node  2 is visible to node  4
node  2 is visible to node  5
node  3 is visible to node  3
node  4 is visible to node  4
node  4 is visible to node  5
node  5 is visible to node  5

But still, I am fairly new and inexperienced to programming, so any notes, corrections and recommendations are welcome.

I came up with a cleaner looking solution:

bool isVisibleTo(Node * accessor) {
    Node * node = accessor;
    while (node) {
        if (node == this) return true;
        if (node->_parent) {
            uint i = node->_parent->_children.indexOf(node);
            while (i) if (node->_parent->_children.at(--i) == this) return true;
        } node = node->_parent;
    } return false;
}

But it will be inevitably slower in a scenario with lots of children. While the first "messy" solution uses the relative tree indices and only traverses the object tree from child to parent, this cleaner solution goes through all children nodes before going to the parent. I will have to do some profiling with a bigger tree to verify if that is the case.

EDIT: I take it back, despite what appeared as an obvious performance penalty this method is actually about 4 times faster consistently. I tested with a whooping 10 million nodes


Tokenizing a large document I'm currently trying to process a corpus of a million patent text files, which contain about 10k non-unique words on average. My current data pipeline works as follows:

Load the patent texts as strings from a local sqlite database table
Tokenize¹ each document and save the results in a new table
Create a dictionary containing all unique words in the corpus
Train an tfidf model with the tokenized documents

¹Tokenization means taking a document text (string) as input and returning a list containing every word in the document (duplicates allowed). Words tend to be separated by spaces, special characters, numbers etc., the regex in my code has served quite well for this purpose.

In my data pipeline, I identified the tokenize function as my bottleneck, the relevant part is provided in my MWE below:

import re
import urllib.request
import time

url='https://raw.githubusercontent.com/mxw/grmr/master/src/finaltests/bible.txt'
doc=urllib.request.urlopen(url).read().decode('utf-8')

PAT_ALPHABETIC = re.compile(r'[^\W\d]+')

def tokenize(text):
    matches=PAT_ALPHABETIC.finditer(text)
    for match in matches:
        yield match.group()

def preprocessing(doc):
    tokens = [token for token in tokenize(doc)]
    return tokens

start_time = time.time()
preprocessing(doc)
print("--- %s seconds ---" % (time.time() - start_time))

I would guess that you're never going to take another 50% off. You could consider pulling the problem down into C or Haskell or Awk or whatever and then binding that more tightly optimized implementation back up into python; IDK.

As for more incremental improvements, it depends a bit how the thing is going to be used.

You flagged the question for multi-threading. That certainly might help, but you'll have to figure out how to divy up the job. Maybe there are natural split-off points that can be identified without actually reading the subject data, in which case any parallelization system will probably work. Maybe you can figure out the size of the subject on disk, and open multiple readers against that file starting at evenly spaced offsets; in this case you'll need to figure out the fence-posting when you combine the results of each of the workers.

I notice that the function preprocessing just wraps the iterable results up in a list. Is that necessary? If you can avoid ever actualizing the whole 10k-items list in memory that will likely help some.

Similarly, you're reading the whole file (http payload) as a single string. Both file handles and HTTPResponse objects will let you read just a chunk at a time; this would let you handle the subject data like a lazy iterable (similar to the way you're yielding from .finditer()). Of course how much of an advantage this is for you will depend on the underlying implementation, and you'll have to be careful about fence-posting again. This may also be a good way to break off jobs for parallelization.


Count the number of duplicated chars in a string It's an exercise, which I found on Codewars.

Instructions:

Write a function which returns the count of distinct case-insensitive alphabetic characters and numeric digits which occur more then once in a given string. The given string can be assumed to contain only alphabets (lower-, uppercase) and numerics.

Examples:

"abcde" results in 0, because no characters repeats more than once.
"aabbcde" results in 2, because of 'a' and 'b'.
"aabBcde" => 2, because 'a' occurs twice and 'b' occurs twice (b and B).
"indivisibility" => 1, because 'i' occurs six times.
"Indivisibilities" => 2, because 'i' seven times & 's' twice.
"aA11" => 2, because 'a' and '1'.
"ABBA" -> 2, because 'A' and 'B' each twice.

My (valid*) solution:

fun duplicateCount(text: String): Int {
    var count = 0
    var invalid = ArrayList<Char>()
    var i = 0

    while (i < text.length) {
        if (invalid.contains(text[i].toLowerCase())) {
            i++
            continue
        }

        var j = i + 1

        while (j < text.length) {
            if (text[i].toLowerCase() == text[j].toLowerCase()) {
                invalid.add(text[i].toLowerCase())
                count++
                break
            }
            j++
        }

        i++
    }

    return count
}

*It has passed the unit-tests.

What are your thoughts about my implementation?

How could it be improved? What would you have done differently and why?

Looking forward to reading your comments and answers.

A more straight forward solution would be to count each character and then to count the characters that have a count larger than 1. This can easily be done using the functional methods available in Kotlin's standard library:

fun duplicateCount(text: String): Int =
  text.toLowerCase()
    .groupingBy { it }.eachCount()
    .count { it.value > 1 }

.groupingBy { it }.eachCount() creates a map (Map<Char, Int>) that assigns each character in the string to its count.

.count { it.value > 1 } then counts all entries in the map where the count is more than one.

EDIT: Inspired by @SimonForsberg here in comparision a procedural implementation of the same algorithm. I also modified the version above to use the better count(predicate) method, which I originally forgot about.

fun duplicateCount(text: String): Int {
    val lcText = text.toLowerCase()
    val characterCount = mutableMapOf<Char, Int>()

    for (i in 0 until lcText.length) {
        val char = lcText[i]
        characterCount.put(char, 1 + characterCount.getOrDefault(char, 0))
    }

    var count = 0

    for (entry in characterCount) {
        if (entry.value > 1) {
            count++
        }
    }

    return count
}


JS create array by comparing 2 dictionaries I have 2 dictionaries that I have to compare, if an element of one dictionary exists in the other dictionary, then add the element to an array but only one time...
here the working code
const getFavouriteShows = (graphql, favouriteShows) => {
    const arraya = [];

    //iterate trough favouriteShows
    for (let item in favouriteShows) {
      //iterate trough graphql
      for (let itemGQL in graphql) {
        //if same slug, add the item to new array JUST ONE TIME
          if(favouriteShows[item].slug === graphql[itemGQL].show?.slug){
            arraya.push(graphql[itemGQL])
            break
          }
      }
    }
       return Object.assign(arraya)
  }

So, how to express this in a more inline functional way? using map, filter.
Thanks!
Edit, examples for the data coming in and out https://gist.github.com/mako34/44301d3c96249de9a07aa2550ca1172a

Review

Use semicolons. Why? If you don't know why you should use them, then you should use them. Not using them can result in very hard to track down bugs that will waste hours of your valuable time.

Use const wherever and whenever possible.

The return expression Object.assign(arraya) does nothing and is not required ( Object.assign returns the first argument unchanged if there is only one argument). return arraya; will do the same.

Use for of rather than for in. When you use for in you need to ensure that the key belongs to the object which just makes the code more complex. If you dont check for own property Object.hasOwnProperty then you risk the code not working for reasons unrelated to your code.
To get the keys of an object use Object.keys eg you can then replace the for in with for (let item of Object.keys(favouriteShows)) {
As it is only the value slug you want from each item in the object favouriteShows you can further refine the for of loop to get that value for you. Use Object.values which will get the value you find with favouriteShows[item] and use destructure assignment to pull out slug thus (Note the const) you have for (const {slug} of Object.values(favouriteShows)) {

Avoid naming variables after what they are, rather name them for what they represent (Unless they represent unknowns). Eg arraya would be better as results

Don't use complicated or long names when there is no need. The arguments (graphql, favouriteShows) can be simplified to (graph, shows) without losing any meaning.

Problems
There are a number of problems with this code.

The statement that matches shows by slug  if(favouriteShows[item].slug === graphql[itemGQL].show?.slug){ will match any two shows that do not have slug defined IE slug === undefined
This is not a problem if all objects always have slug defined. However the use of show?. makes it clear that this is at least not true in part.
You can check if slug is defined in the outer loop so you don't waste time searching for undefined in the inner loop.

In your question you state  "...then add the element to an array but only one time." However if favouriteShows contains items that have the same value for slug and there is a match for that value in graphql your function will add the same entry for every occurrence in favouriteShows
To prevent this problem you can use a Set to hold the result rather than an Array, It will ensure only one copy of each item is added.

Improvement
The inner loop repeats, stepping over the same items for every item in favouriteShows. You can use a Map that indexes items via a generated hash value. This reduces the complexity of your inner loop from \$O(n)\$ to \$O(1)\$.
Thus your code ends up as with added comments
// Keep the argument names simple
const favouriteShows = (graph, shows) => {

    // Use set to ensure only a single instance of each item will be in the result
    const result = new Set();

    // Create a map of graph indexed by slug so the inner loop is not needed.
    const bySlug = new Map(Object.values(graph).map(val => [val.show?.slug, val]));

    // Extract value of slug from the item values in shows
    for (const {slug} of Object.values(shows)) {

        // Make sure slug is defined.
        // It is unclear if this value may be falsey so test against undefined
        if (slug !== undefined) {

            // Does the map bySlug have an entry that matches slug?
            if (bySlug.has(slug)) {

                // Yes then get that entry and add it to the result.
                result.add(bySlug.get(slug));
            }
        }
    }

    // To return an array convert the set to an array.
    return [...result.values()];
}

It can also be compacted a little, and making an assumption that slug will be either undefined or some truthy string we get
const favouriteShows = (graph, shows) => {
    const result = new Set();
    const bySlug = new Map(Object.values(graph).map(val => [val.show?.slug, val]));
    for (const {slug} of Object.values(shows)) {
        const show = bySlug.get(slug);
        slug && show && result.add(show);
    }
    return [...result.values()];
}

Note that bySlug.get will find the last entry in the object graph if graph contained duplicates (slug has the same value more than once). As your code uses the first instance you may want to change the second line in the function so that it reverses (via Array.reverse) the order that the map is created. If there are no duplicates then it does not matter.
const favouriteShows = (graph, shows) => {
    const result = new Set();
    const bySlug = new Map(Object.values(graph)
       .map(val => [val.show?.slug, val])
       .reverse()
    );
    for (const {slug} of Object.values(shows)) {
        const show = bySlug.get(slug);
        slug && show && result.add(show);
    }
    return [...result.values()];
}


converting bits binary to bcd I wrote a code that requests user input and then converts binary to bcd then back to binary using bitwise operations.
While I think my code is good I want to know if there is a better way to write the code.
Is there also a way to keep the code mostly the same but getting rid of the arrays for printing out the bit positions?

#include <stdio.h>
#include <limits.h>
#include <stdlib.h>
#include <string.h>

#define UINT_MAXIMUM 4294967294
#define MAX_NUMBER 99999999

void printBits(unsigned long number);
unsigned long convertBinaryToBCD(unsigned long number);
unsigned long convertBCDToBinary(unsigned long number);
unsigned long printNumber(unsigned long input)
{
  printf(" Input: ");
  printf("%10lu", input);
  printBits(input);
  unsigned long output_bcd = convertBinaryToBCD(input);
  unsigned long  output_binary = convertBCDToBinary(output_bcd);
  printf("   BCD: ");

  if(output_bcd != UINT_MAX)
  {
    printf("%10lu", output_bcd);
    printBits(output_bcd);
  }
  printf("Binary: ");
  if(output_binary != UINT_MAX)
  {
    printf("%10lu", output_binary);
    printBits(output_binary);
  }
  return 0;
}
void printBits(unsigned long number)
{
    int bit_position[32];
    printf(" - ");
    int counter = 0;
    for(counter = 32; counter > 0; counter--)
    {
        bit_position[counter] = number % 2;
        number = number / 2;
    }
    for(counter = 1; counter <= 32; counter++)
    {
        if(counter == 9 || counter == 17 || counter == 25)
        {
            printf(" | ");
        }
        if(counter == 5 || counter == 13 || counter == 21 || counter == 29)
        {
            printf(" ");
        }
        printf("%d", bit_position[counter]);
    }
    printf("\n");
}

int main(int argc, char *argv[])
{
  unsigned long number;

  number = strtoul(argv[1], NULL, 10);

  if(number > UINT_MAXIMUM)
  {
    printf("too big\n");
    return 0;
  }
  printNumber(number);

  return 0;
}

unsigned long convertBinaryToBCD(unsigned long number)
{
  if (number > MAX_NUMBER)
  {
    return UINT_MAX;
  }
  if(number < 100)
  {
    return (((number/10)+((number/100)*6))*16)+(number%10);
  }
  unsigned long sub_number_one = ((number / 12) << 5);
  unsigned long sub_number_two = (number % 10) ;
  unsigned long temporary;
  while(sub_number_two != 0)
  {
    temporary = sub_number_one & sub_number_two;
    sub_number_one = sub_number_one ^ sub_number_two;
    sub_number_two = temporary << 1;
  }
  return sub_number_one;
}

unsigned long convertBCDToBinary(unsigned long number)
{
  if (number == UINT_MAX)
  {
    return UINT_MAX;
  }
  if(number < 100)
  {
    return (((number >> 8) * 100) + ((number >> 4) * 10) + (number & 0xF));
  }

  unsigned long sub_number_one = (number >> 8) * 100;
  unsigned long sub_number_two = (number & 0x0F);
  unsigned long temporary;
  while(sub_number_two != 0)
  {
    temporary = sub_number_one & sub_number_two;
    sub_number_one = sub_number_one ^ sub_number_two;
    sub_number_two = temporary << 1;

  }
  return sub_number_one;
}

It's been a long time since I've heard the terms BCD or Binary Coded Decimal. Keep in mind that BCD was primarily a hardware encoding of integers provided by one particular manufacturer of main frame computers. There were computer performance issues when using BCD.
The best way to implement a BCD number would be an array of 4 bit integer values.
One good habit the code demonstrates is that all if statements and loops wraps the code in braces ({ and }). Thank you for that!
Magic Numbers
There are Magic Numbers in the void printBits(unsigned long number) function (32, 9, 17, 25, 5, 13, 21 and 29), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.
Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.
Unused Header Files
The program includes limits.h and string.h but the contents of these files are not used by the code in the program. In the case of string.h it might be better to remove the #include. In the case of limits.h it might be better to use the limits provided. It is better to not include files that aren't being used since this will decrease the size of the code being compiled and might improve build times. The #include directive literally copies the header file into the intermediate file that is compiled after the C pre-processor is run.
Algorithm
It might be better to prompt the user for the number to be converted rather than use argv as the source of the input. Even if argv continues to be the source of the input, it would be better to check argc to see if there are enough arguments and either prompt the user for a number in screen input, or report an error and quit.
Readability
The some of the functions (printBits and convertBinaryToBCD) might be easier to read if there was vertical spacing in the functions.
Off by One Bug
As noted by @1201ProgramAlarm there is an off by one bug in void printBits(unsigned long number), the code is accessing one invalid memory location bit_position[32] and ignoring bit_position[0].
It is also not clear why bit_position is 32 bits rather than 64 bits, since the code is primarily using unsigned long rather than unsigned int.


Checking if the site resolve a DNS or not I've written code to check whether a domain resolves DNS or not like follows. It is working fine, but I doubt whether there is a simple way to check this by using `dns_get_record'

$url = 'http://www.example.com';
$get = get_headers($url); //Fetches all the headers sent by the server in response to a HTTP request
if (preg_match('/^HTTP\/\d\.\d\s+(200|301|302)/', $get[0])){
$parse = parse_url($url); //Parse a URL and return its components
$ip = gethostbyname($parse['host']);  // Get the IPv4 address corresponding to a given Internet host name
//ip2long — Converts a string containing an (IPv4) Internet Protocol dotted address into a proper address
return (ip2long($ip) == true) ? true : false;

Any suggestions would be much appreciated.

You are doing way more than necessary, if all you want to verify is that the hostname resolves to an IP address.

gethostbyname() comes pretty close to doing the job:

  Returns the IPv4 address or a string containing the unmodified hostname on failure.

Actually, that's annoying.  Try gethostbynamel() instead:

  Returns an array of IPv4 addresses or FALSE if hostname could not be resolved.

So, you could just do

$url_parts = parse_url($url);
return gethostbynamel($url_parts['host']) ? true : false;

Note, however, that the gethostbyname() family of functions relies on the operating system to do the name resolution in whatever way it is configured.  Usually, that means using DNS.  However, it might also use a local hosts file, NIS, LDAP, Multicast DNS, etc.  dns_get_record() would actually perform a DNS lookup, independent of the machine's preferred name resolution mechanism.  If you want to go that route, then it looks like checkdnsrr($host, 'A') would be convenient.

The distinction is almost always inconsequential, but I thought I'd mention it since you specifically mentioned DNS resolution in your question.


Finding Duplicates in Java I am trying to figure out a way to make finding duplicates in Java more efficient. I started off with this:

private boolean hasDuplicates(int[] array) {
    List<Integer> list = new ArrayList<Integer>(array.length);
    for(int num : array) {
        list.add(num);
    }
    for(int num : list) {
        if(list.indexOf(num) != list.lastIndexOf(num)) {
            return true;
        }
    }
    return false;
}

But the main problem here is it only can be used if you were to find duplicate numbers. So I made it generic:

private <E> boolean hasDuplicates(E[] array) {
    List<E> list = new ArrayList<E>(array.length);
    for(E e : array) {
        list.add(e);
    }
    for(E e : list) {
        if(list.indexOf(e) != list.lastIndexOf(e)) {
            return true;
        }
    }
    return false;
}

This is what I currently have, but it seems very inefficient, so I tried this:

private static <E> boolean hasDuplicates(E[] array) {
    Arrays.sort(array);
    int length = array.length - 1;
    for(int i = 0; i < length; i++) {
        if(array[i].equals(array[i + 1])) {
            return true;
        }
    }
    return false;
}

But that would throw a ClassCastException if it cannot be cast to java.lang.Comparable during Arrays.sort(). Which returns to the old, inefficient method.

Is there a better solution than this? If so, how?

Use a Set.

private <E> boolean hasDuplicates(E[] array) {
    Set<E> set = new HashSet<E>();
    for(E e : array) {
        if (!set.add(e)) {
            return true;
        }
    }
    return false;
}

The add() method of a set returns false if the object already exists in the set.


Recommendations for stored procedure Just wondering if I could solicit some feedback on a stored procedure I'm running and whether there's a more efficient way of handling the scenario (I'm pretty sure there will be!).

Basically I have a single SP that I call to return a list of records (Jobs) that may have one or more statuses and a sort order (I'm using RowNum for paging). At the moment I'm using WITH RECOMPILE because the variations on the statuses can change all the time (depending on user etc). There's also some filtering going on.

I'm using an IF statement to essentially run the same bit of code with the only change being the sort order.

I guess my questions are: Is there a better way of doing this (maybe different SP's for different statuses)? Am I overcomplicating things due to lack of knowledge (quite likely) Is the SP actually ok, but requires minor tweaks to reduce the number of lines?

I've pasted a portion of the SP below - the only difference to the full code is the additional IF statements for the different sort orders...

I'd appreciate any feedback.

Thanks in advance!

CREATE PROCEDURE [dbo].[sp_Jobs]
(
    @PageNumber int,
    @PageSize int,
    @FilterExpression varchar(500),
    @OrderBy varchar(50),
    @CustomerID int,
    @ShowNotSet bit,
    @ShowPlaced bit,
    @ShowProofed bit,
    @ShowReProofed bit,
    @ShowApproved bit,
    @ShowOnTime bit,
    @ShowLate bit,
    @ShowProblem bit,
    @ShowCompleted bit,
    @ShowDispatched bit,
    @ShowUnapproved bit,
    @ShowClosed bit,
    @ShowReturned bit,
    @UserID int
)
WITH RECOMPILE
AS
    --JobNumber DESC
    if @OrderBy='JobNumberDESC'
    BEGIN

    WITH Keys AS (SELECT TOP (@PageNumber * @PageSize) ROW_NUMBER() OVER (ORDER BY JobNumber DESC) as rn,P1.jobNumber,P1.CustID,P1.DateIn,P1.DateDue,P1.DateOut,p1.client,p1.MasterJobStatusID,p1.MasterJobStatusTimestamp,p1.OwnerID

    FROM
    vw_Jobs_List P1 WITH (NOLOCK)

    WHERE
    (@CustomerID = 0 OR CustID = @CustomerID)
    AND (@UserID = 0 OR OwnerID = @UserID)
    AND ((@ShowNotSet = 1 AND MasterJobStatusID=1) OR (@ShowPlaced = 1 AND MasterJobStatusID=2) OR (@ShowProofed = 1 AND MasterJobStatusID=3) OR (@ShowReProofed = 1 AND MasterJobStatusID=4) OR (@ShowApproved = 1 AND MasterJobStatusID=5) OR (@ShowOnTime = 1 AND MasterJobStatusID=6) OR (@ShowLate = 1 AND MasterJobStatusID=7) OR (@ShowProblem = 1 AND MasterJobStatusID=8) OR (@ShowCompleted = 1 AND MasterJobStatusID=9) OR (@ShowDispatched = 1 AND MasterJobStatusID=10) OR (@ShowUnapproved = 1 AND MasterJobStatusID=11) OR (@ShowClosed = 1 AND MasterJobStatusID=12) OR (@ShowReturned = 1 AND MasterJobStatusID=13)) AND (Search LIKE '%'+@FilterExpression+'%')

    ORDER BY
    P1.JobNumber DESC ),SelectedKeys AS (
    SELECT TOP (@PageSize)SK.rn,SK.JobNumber,SK.CustID,SK.DateIn,SK.DateDue,SK.DateOut

    FROM
    Keys SK

    WHERE
    SK.rn > ((@PageNumber-1) * @PageSize)

    ORDER BY
    SK.JobNumber DESC)

    SELECT SK.rn,J.JobNumber,J.OwnerID,J.Description,J.Client,SK.CustID,OrderNumber, CAST(DateAdd(d, -2, CAST(isnull(SK.DateIn,0) AS DateTime)) AS nvarchar) AS DateIn, CAST(DateAdd(d, -2, CAST(isnull(SK.DateDue,0) AS DateTime)) AS nvarchar) AS DateDue,CAST(DateAdd(d, -2, CAST(isnull(SK.DateOut,0) AS DateTime)) AS nvarchar) AS DateOut, Del_Method,Ticket#, InvoiceEmailed, InvoicePrinted, InvoiceExported, InvoiceComplete, JobStatus,j.MasterJobStatusID,j.MasterJobStatusTimestamp,js.MasterJobStatus

    FROM SelectedKeys SK JOIN vw_Jobs_List J WITH (NOLOCK) ON j.JobNumber=SK.JobNumber JOIN tbl_SYSTEM_MasterJobStatus js WITH (NOLOCK) ON j.MasterJobStatusID=js.MasterJobStatusID

    ORDER BY
    SK.JobNumber DESC
END

--ELSE IF for other column sorting

You can use CASE statements in the ORDER BY clauses to decide which sort order to use:

  SELECT (...)
ORDER BY CASE @OrderBy
           WHEN 'JobNumberDESC' THEN sk.jobnumber
           WHEN (etc.)
         END DESC,
         CASE @OrderBy
           WHEN 'JobNumberASC' THEN sk.jobnumber
           WHEN (etc.)
         END ASC;

(You would have to split up the ascending and descending sort options like that)

One issue with that approach, though, is that in your original statement, I count four places where the ORDER BY statement occurs.  That would mean repeating the long CASE statement a lot.  I think we can fix that, though.

First off, sorting in a subquery usually doesn't make much sense.  I don't see any reason to have keys and selectedkeys sorted, so let's remove those ORDER BY statements.

Now we're left with two ORDER BYs: the one used in the ROW_NUMBER function and the final sort.  No matter what, you need to include the ORDER BY in the ROW_NUMBER function to make sure your paging works correctly, but here's the thing: now you can use that row number to sort the final result (i.e., change the final ORDER BY sk.jobnumber DESC to ORDER BY sk.rn).  Now there's only one place where you need the dynamic ORDER BY clause, which is a lot better than four!

Disclaimer: I haven't tested these ideas; just brainstorming.

Another option would be to use dynamic SQL.  Then you'd have something like this:

DECLARE @OrderByClause varchar(500),
        @sql           varchar(max);

SET @OrderByClause =
    CASE @OrderBy
      WHEN 'JobNumberDESC' THEN 'sk.jobnumber DESC'
      WHEN (etc)
    END;

SET @sql = 'WITH keys
    AS (SELECT TOP (@PageNumber * @PageSize)
        ROW_NUMBER() OVER (ORDER BY ' + @OrderByClause + ') AS rn' +
    (etc.);

EXEC sp_executesql @sql, (etc.);

Just make sure you pay attention to the SQL injection threat.


Discrete event simulation with variable intervals I've built a discrete event simulation system, similar to the bank problem presented on the wikipedia page but with a key difference.

Let's say, that a TELLER can service two CUSTOMERS at the same time, and that the CUSTOMER-DEPARTURE time depends upon the number of CUSTOMERS a TELLER is servicing concurrently; I need to manage the dynamic CUSTOMER-DEPARTURE event scheduling when the number of concurrently served CUSTOMERS changes.

Consider the following example, where the servicing time for a single customer is of 4 time intervals and is doubled when the teller is servicing two customers at the same time:

t=0  CUSTOMER-ARRIVAL (1)
t=1  TELLER-BEGINS-SERVICE (1) --> schedules CUSTOMER-DEPARTURE (1) at t_a=5
...
t=3  CUSTOMER-ARRIVAL (2)
t=4  TELLER-BEGINS-SERVICE (1) --> teller is now servicing two customers:
                                   - reschedule CUSTOMER-DEPARTURE (1) to t_b=6
                                   - schedule CUSTOMER-DEPARTURE (2) at t_c=12
...
t=6  CUSTOMER-DEPARTURE (1)    --> teller is now servicing only one customer:
                                   - reschedule CUSTOMER-DEPARTURE (2) at t_d=9
...
t=9  CUSTOMER-DEPARTURE (2)

The times are calculated using this formula:

t = t_current + (4 - completed) * customer_count

Which gives these results for the values used above:

t_a = 1 + (4 - 0) * 1 = 5
t_b = 4 + (4 - 3) * 2 = 6
t_c = 4 + (4 - 0) * 2 = 12
t_d = 6 + (4 - 1) * 1 = 9

I came up with the following code in Python; is there something you would have done differently?

import heapq

class Simulator(object):

    def __init__(self):
        self.queue = []
        self.time = 0

    def schedule(self, time, callback, *args, **kwargs):
        """
        Schedules an event to be executed at a later point in time.
        ``callback`` is a callable which executed the event-specific behavior;
        the optional ``args`` and ``kwargs`` arguments will be passed to the
        event callback at invocation time.

        Returns an object which can be used to reschedule the event.
        """
        assert time > self.time
        event = [time, True, callback, args, kwargs]
        heapq.heappush(self.queue, event)
        return event

    def reschedule(self, event, time):
        """
        Reschedules ``event`` to take place at a different point in time
        """
        assert time > self.time
        rescheduled = list(event)
        event[1] = False
        rescheduled[0] = time
        heapq.heappush(self.queue, rescheduled)
        return rescheduled

    def run(self):
        """
        Simple simulation function to test the behavior
        """
        while self.queue:
            time, valid, callback, args, kwargs = heapq.heappop(self.queue)
            if not valid:
                continue
            self.time = time
            callback(*args, **kwargs)

def event(id, simulator, scheduled, rescheduled=None):
    if rescheduled is None:
        print "Event #{0}: scheduled at {1} and fired at {2}".format(id,
                scheduled, simulator.time)
    else:
        print "Event #{0}: scheduled at {1}, rescheduled at {2} and fired " \
                "at {3}".format(id, scheduled, rescheduled, simulator.time)

s = Simulator()
e1 = s.schedule(5, event, 1, s, 5)
e2 = s.schedule(7, event, 2, s, 7, 12)
e3 = s.schedule(9, event, 3, s, 9)
s.reschedule(e2, 12)
s.run()

Given the problem scope as I understand it (need to execute events in particular sequence, with ability to rearrange sequence at any point) I think the design seems clean and direct. I caveat that with: I don't know python, and I seem to be missing the part where you are ensuring sequence of your queue by the event's time member. I understand the event.time needs to be the sequencing key for your queue, or else the reschedule will have erratic behavior as self.time goes forward and backward if the queue isn't in order

The design wholesale seems clean though, to my eyes.


String traversal program in C My task was to write a function in C where a string such as {2210090,34566,87234,564676} would be given as input and the function had to find out the count of numbers separated by comma whose average of single digits is greater than or equal to 5.

In the below example, the average of 2210090 is 2, the average of 34566 is 4.8, the average of 87234 is 4.8 and the average of 564676 is 5.6, hence the count is 1.

void GetCount(int n,char* input2)
{
    int sum=0;
    n=0;
    h:
    *input2++;
    if((*input2) && !isdigit(*input2))
    {
        if(sum>n<<2)
            output1++;
        sum=0;
        n=0;
        goto h;
    }
    else if(*input2!='\0')
    {
        sum+=*input2-'0';
        n++;
        goto h;
    }
}

The first parameter n is the total count of number.  The program is working fine and I just wanted to know if anyone could give input on improving the time complexity of the program.  I would also appreciate a better way of solving this problem, reducing the number of ifs and gotos or completely eliminating the need of if or goto.

Before addressing efficiency, some improvements I would recommend:

No need for a goto here.  This is easy to turn into a for loop.
Say const char *input2, to avoid accidentally modifying your input string.
Use whitespace to clarify order of operations:

if(sum > n<<2)  Relational operators and shifts are right next to each other on the operator precedence table.
sum += *input2 - '0';

Take out the asterisk in *input2++;  You aren't using the character.

And a more subtle issue:

isdigit expects an unsigned char casted to an int.  char is usually signed (but on some platforms, it's unsigned).  Thus, if one of your input bytes is not ASCII, it may lead to undefined behavior.

isdigit((unsigned char) *input2)

Here is an editorialized version of your code:

void GetCount(const char* input2)
{
    int sum = 0;    /* Sum of digits in current number */
    int n = 0;      /* Number of digits in current number */

    for (;;)
    {
        input2++;
        if (*input2 == '\0')
            break;

        /*
         * If we have a digit, increment sum and n appropriately.
         *
         * Otherwise (we ran into a brace or comma), tally the current number,
         * and clear the stats for the next number.
         */
        if(isdigit((unsigned char) *input2))
        {
            sum += *input2 - '0';
            n++;
        }
        else
        {
            if(sum > n<<2)
                output1++;
            sum = 0;
            n = 0;
        }
    }
}

Let's look at the sum > n<<2 line, which looks wrong to me.  Here's the condition we want to test (simple, but inefficient):

1. (double)sum / n >= 5.0

We can express it in integer arithmetic as:

2. sum >= n * 5

Now I get what you were trying to do.  Your next steps converted the multiplication to a less expensive shift:

3. sum > n * 4
4. sum > n<<2

Step 3 is wrong.  sum can be greater than n * 4 but less than n * 5.  Example:

34566: sum = 24, n = 5

n * 4 = 20      sum >  n * 4 holds
n * 5 = 25      sum >= n * 5 does not hold

The good news is, we can get rid of the multiply and even the shift!  Just increment n by 5 each iteration:

        if(isdigit((unsigned char) *input2))
        {
            sum += *input2 - '0';
            n += 5;
        }
        else
        {
            if(sum >= n)
                output1++;
            sum = 0;
            n = 0;
        }

Finally, let's get rid of that isdigit.  It's probably pretty fast, but it has to guard against EOF and do a table lookup.  Assuming your input is guaranteed to be in the syntax you describe (non-negative integers delimited by commas and wrapped in braces), you can test for two specific characters instead of having to do a character class lookup.

Here is a final version, with more cleanups:

/*
 * Given a list of numbers in the following syntax:
 *
 *     list ::= '{' '}'
              | '{' number (',' number)* '}'
 *
 *     number ::= [0-9]+
 *
 * Count how many numbers have digits that average to 5 or more.
 */
int GetCount(const char* input)
{
    int sum = 0;                /* Sum of digits in current number */
    int length_times_five = 0;  /* Number of digits in current number, times 5 */
    int ret = 0;

    /*
     * Handle empty list.  Otherwise, the following code
     * would return 1 because 0 >= 0.
     */
    if (input[0] == '{' && input[1] == '}')
        return 0;

    while (*++input != '\0')
    {
        if (*input == ',' || *input == '}')
        {
            if(sum >= length_times_five)
                ret++;
            sum = 0;
            length_times_five = 0;
        }
        else
        {
            sum += *input - '0';
            length_times_five += 5;
        }
    }

    return ret;
}


Calculation of the Distance Matrix in the K-Means Algorithm in MATLAB Purpose of the code :

To assign the corresponding label of the centroids to the points which are close to it. Below is a graphical (2D) example.

Variable X is a matrix, rows represent the points, columns collectively represent the coordinates
idx is the list of centroids assigned to every example

Inefficient MATLAB code to find and assign nearest Centroids
sd2=size(X,2);
t2=[];
for a=1:K
    t1=[];
    for b=1:sd2
        temp=(X(:,b)-centroids(a,b)).^2;
        t1=[t1 temp];
    end
    dmat=sum(t1,2);
    t2=[t2 dmat];
end
[~,idx]=min(t2,[],2);

supposedly faster MATLAB code which worked on a small dataset:
[~,idx]=min(sum(((repmat(X,1,1,K)-reshape(centroids,1,n,K)).^2),2),[],3);

My Question:

Is my second implementation correct? If yes, why doesn't it work on a 300 x 2 matrix X , in the sense that the final array idx produced is not correct. If no, where is the problem (because I tested it with a small sample code just to be sure that I am finding (x-x1)2+ (y-y1)2 correctly).
speculation:

MATLAB is erroneous because of a multidimensional matrix.

You Distance Matrix Calculation
You wrote the code (I summarize it for the Distance Matrix):
K = 3;
n = size(mX, 1);
d = size(mX, 2); %<! Number of dimensions per sample
mC = mX(randperm(n, K), :);

% mD = repmat(mX, 1, 1, K) - reshape(mC, 1, n, K); %<! Your code, Will fail because of reshape
mD = sum((reshape(mX, n, 1, d) - reshape(mC.', 1, K, d)) .^ 2, 3); %<! Correctly with no `repmat()`

The reason it won't work is the reshaping you're doing.
The number of elements in mC is K x d while your reshape() should keep the number of elements.
In the following I gave you a code snippet how to calculate the matrix mD efficiently.
K-Means
To calculate the distance you shouldn't use repmat() which will allocate new memory.
To calculate the Distance Matrix with the 3rd dimension and broadcasting you should do something like:
mD = sum((reshape(mA, numVarA, 1, varDim) - reshape(mB.', 1, numVarB, varDim)) .^ 2, 3);

But a faster way would be:
mD = sum(mA .^ 2).' - (2 * mA.' * mB) + sum(mB .^ 2);

Which utilizes the fast the Matrix Multiplication is probably the most optimized algorithm and that:
$$ {\left\| \boldsymbol{x} - \boldsymbol{y} \right\|}_{2}^{2} = \boldsymbol{x}^{T} \boldsymbol{x} - 2 \boldsymbol{x}^{T} \boldsymbol{y} + \boldsymbol{y}^{T} \boldsymbol{y} $$
For you to have a reference of K-Means algorithm with support for any Distance Function I created a function and ran it on your data:

It will converge very fast for that data:

I also created a small benchmark for various algorithm to compute the Euclidean Squared Distance Matrix.
The first test was between 3 approaches to calculate the distance between a single set of vectors (For example given by a single matrix):

The second test I compared 2 Row / Column variations of the algorithm above:

The Row / Column variations means I test the algorithm in 2 cases:

Row Variation - Each data sample is a row in the data matrix (Like your data).
Column Variation - Each data sample is a column in the data matrix (In MATLAB it means the sample is contiguous in memory).

As can be seen, it is always better to have data in the column variation.
Remark: Pay attention that the Squared Euclidean Distance doesn't obey the Triangle Inequality which is a requirement for a Metric. It is used for computational efficiency as it doesn't change the K-Means algorithm results compared to Euclidean Distance (The L2 Norm based distance).
The MATLAB Code is accessible in my StackExchange Code Review Q254186 GitHub Repository.
Remark: The K-Means algorithm, as a non convex optimization, is sensitive to the initialization. In the above code I used random initialization - randomly choosing samples form data to be the initial centroids. There are better approaches out there (See K-Means++ as an example for a more robust initialization).


Simple surcharge algorithm could use DRY I solved this exercise on jshero.com but I know the solution can be written more cleanly, I just don't know how.
Here are the directions:
Write a function addWithSurcharge that adds two amounts with surcharge. For each amount less than or equal to 10, the surcharge is 1. For each amount greater than 10 and less than or equal to 20, the surcharge is 2. For each amount greater than 20, the surcharge is 3.
This challenge also supposes that you use if...else if...else to solve the exercise. The course also hasn't touched on functions within functions, so I'm mainly concerned with simplification and readability, but also curious about ternaries. Here is my attempt which works,
function addWithSurcharge(num1, num2) {
    let surcharge = 0;
    if (num1 <= 10) {
        surcharge += 1;
    } else if (num1 > 10 && num1 <= 20) {
        surcharge += 2;
    } else {
        surcharge += 3;
    }

    if (num2 <= 10) {
        surcharge += 1;
    } else if (num2 > 10 && num2 <= 20) {
        surcharge += 2;
    } else {
        surcharge += 3;
    }

    return surcharge + num1 + num2;
}

Thank you, much appreciated!

From a short review;

You are dealing with amounts, but you call your variable num1 and num2
Your code now has tons of magic numbers, you should use named constants
I understand you are not yet in to 'functions within functions', but that is how you apply DRY in this case

So the DRY version could look like this;
function addSurcharge(amount){
  const FLOOR = 10;
  const CEILING = 20;

  if (amount <= FLOOR) {
    return amount + 1;
  } else if (amount<= CEILING) {
    return amount + 2;
  } else {
    return amount + 3;
  }
}

function addWithSurcharge(amount1, amount2) {
  return addSurcharge(amount1) + addSurcharge(amount2);
}


Class(es) for animating objects on a canvas I'm building a simple canvas animation engine as an exercise in learning ES6. Coming from C++ and Java, I miss static properties, but I've read that "Prototypes having data properties is generally considered an anti-pattern".

The animation engine simulates \$n\$ river waves (simple lines), flowing from right to left in an HTML5 canvas, using requestAnimationFrame. Waves come in three lengths, based on a baseLength that should be configurable and have a default: 1x, 5x, and 7x. When creating a new wave, I'm picking one of the lengths at random. All waves have a constant flowing velocity on the x axis. You can see this in motion on CodePen (ES5 approach; ignore the boats).

How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method?
Should the velocity (same for all waves) be stored at the Wave, or Waves level?
Is the static config() method a good idea, or should each property be separate? Let's assume for simplicity we won't build an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.
Is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.

Here's the first stab at this in ES6:

'use strict';

class Wave {
  static config() {
    const waveBaseLength = 5;
    return {
      color: '#4F5E5B',
      waveLengths: [waveBaseLength, waveBaseLength * 7, waveBaseLength * 15],
      velocity: 5
    }
  }

  constructor(x, y, velocity) {
    this.x = x;
    this.y = y;
    let waveLengths = Wave.config().waveLengths;
    this.l = waveLengths[Math.floor(Math.random() * waveLengths.length)];
    this.velocity = velocity || 0.5;  // Chrome currently fails at `velocity=0.5` in the constructor declaration
  }

  draw(context) {
    context.beginPath();
    context.moveTo(this.x, this.y);
    context.lineTo(this.x + this.l, this.y);
    context.strokeStyle = Wave.config().color;
    context.stroke();
  }
}

class Waves {
  constructor(n, context) {
    this.waves = [];
    this.context = context;
    for (var i = 1; i <= n; i++) {
      var x = Math.random() * context.canvas.width,
        y = context.canvas.height / n * i;
      this.waves.push(new Wave(x, y));
    }
  }

  step() {
    for (var i = 0; i < this.waves.length; i++) {
      var wave = this.waves[i];

      if (wave.x + wave.l > 0) {
        // if still partially visible in the canvas, draw it
        wave.draw(this.context);
        wave.x -= wave.velocity;
      } else {
        // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
        // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
        wave.x = this.context.canvas.width;
        wave.y = Math.random() * this.context.canvas.height;
      }
    }
  }
}

let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);

window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;

(function loop(timestamp) {
  context.clearRect(0, 0, canvas.width, canvas.height);

  waves.step();

  // keep animating
  window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>

This is really cool! I do have a few small tips though.

First off, rather than creating an entire function, in this case, config, that returns configuration data, I'd store it as a public getter, like this:

class Wave {
  static get CONFIG() {
    const waveBaseLength = 5;
    return {
      color: '#4F5E5B',
      waveLengths: [waveBaseLength, waveBaseLength * 7, waveBaseLength * 15],
      velocity: 5
    }
  }
}

Using this, you can do stuff like Wave.CONFIG.color, and drop the un-needed parentheses.

Finally, you have a few, not so great variable names. A few include l and n. Names like these should be expanded for clarity and readability.


Implementing softmax() in C The Softmax function, or normalized exponential function, is defined as:

$$
\sigma(z)_j = \frac{e^{zj}}{\sum^K_{k=1} e^{zk}} \text{for} \space j \space \text{in} \space [1, \dots, K]
$$

I am trying to implement this in C, starting with an answer here.

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

static void softmax(float *input, int input_len)
{
    assert (input != NULL);
    assert (input_len != 0);
    int i;
    float m;
    /* Find maximum value from input array */
    m = input[0];
    for (i = 1; i < input_len; i++) {
        if (input[i] > m) {
            m = input[i];
        }
    }

    float sum = 0;
    for (i = 0; i < input_len; i++) {
        sum += expf(input[i]-m);
    }

    for (i = 0; i < input_len; i++) {
        input[i] = expf(input[i] - m - log(sum));

    }
}

int main() {
   float arr[7] = {1.0, 2.0, 3.0, 4.0, 1.0, 2.0, 3.0};
   softmax(arr, 7);
   printf("%.3f %.3f %.3f %.3f %.3f %.3f %.3f", arr[0], arr[1], arr[2], arr[3], arr[4], arr[5], arr[6]);
}

Output:

0.024 0.064 0.175 0.475 0.024 0.064 0.175

Any suggestion and comment are welcome.

Prefer double to float for calculations

Unless you're extremely constrained for space, or you have a platform with software floating-point (or very poor FP hardware), there's no reason to use float.

Reduce the scope of index variable

Modern C allows us to declare i local to the loops:

for (int i = 0;  i < input_len;  i++)

Hoist the log(sum) calculation

Your compiler's optimizer will probably spot that log(sum) does not change, but why not help it?

const double log_sum = log(sum);
for (int i = 0;  i < input_len;  i++) {
    input[i] = expf(input[i] - m - log_sum);
}

In fact, since m is also constant, we should hoist that, too:

const double scale = m + log(sum);
for (int i = 0;  i < input_len;  i++) {
    input[i] = expf(input[i] - scale);
}


Simple PHP Router for home grown MVC framework I don't know if you guys get tired of seeing a bunch of requests for review of similar projects, but I was really looking for advice.

My website is designed on the MVC pattern. I have a very simple router that seems to be working just fine! If there is any upgrade I would add to it would be the ability to catch a third parameter (optional ID number, for example).

Any thoughts on the design/execution would be greatly appreciated.

Here is the Router class:

namespace Framework
{
    use App\Controllers;

    class Router
    {
        private $_url = [];

        public function addRoute($route, $dest)
        {
            $this->_url[$route] = $dest;
        }

        public function redirect()
        {
            $url = isset($_GET["url"]) ? $_GET["url"] : "";

            foreach ($this->_url as $route => $dest)
            {
                if("/$url" == $route)
                {
                    $this->parsecommand($dest);
                    break;
                }
            }
        }

        private function parsecommand($dest)
        {
            $goto = explode("@", $dest);

            $controller = $goto[0];
            $action = $goto[1];

            require_once("../app/controllers/$controller.php");

            $class = '\\App\\Controllers\\' . $controller;
            $obj = new $class;

            return $obj->$action();
        }
    }
}

The routes are entered this way:

namespace App
{
    use Framework\Router;

    $routes = new Router;

    $routes->addRoute("/", "HomeController@index");
    $routes->addRoute("/aboutus", "HomeController@aboutus");

    $routes->addRoute("/blog", "BlogController@index");

    return $routes;
}

...which runs as a part of a bootstrap process called from index.

And my controllers look like this:

namespace App\Controllers
{
    use App\Controller as Controller;
    use App\Models\Update as Update;

    class HomeController extends Controller
    {

        public function index()
        {
            $this->view('index', [
                'update' => Update::orderBy('id', 'desc')->first(),
            ]);
        }

        public function aboutus()
        {
            $this->view('aboutus');
        }
    }
}

Rendering the views is handled by Twig in the Controller base class (not really relevant to this question).

Like I said, this works perfectly (for my needs, I wouldn't market it or anything, haha), just wondering if I could get some advice.

EDIT: This does not handle 404 (or any other error for that matter), I realize that would probably the first thing to upgrade.

Welcome to code review!  Don't worry, we don't get tired of these things (or at least I don't).  In retrospect I would have loved to have something like this in my early days - and it's still useful to me now even though I'm well past my early years.

I'll just start from small items and work my way up.  To be fair, I'm a stickler for the little things.  You'd be surprised how often the little things can make a big difference in the long run, especially when you are talking about the core part of a system (such as this).  So let's start with the little stuff:

You don't have to use braces and indent after a namespace.  This is actually the only place I suggest not indenting or including braces.  The reason is because it simply pushes the indentation of your entire file up one level unnecessarily.  Just end your namespace declaration with a semi-colon and leave the use lines and class definition without indentation.
The usage of the redirect method in your router is unclear, and as such I'm not even 100% sure if your redirect method even belongs in the router.  That sounds more like something that should be part of the object that manages the HTTP response (which would also handle things like 404 errors and what not).  Then again it seems like the only part that actually does routing, so maybe the issue is your name.  Perhaps it should be called route()?  That makes sense to me: in your index file you would call $router->route() and expect a controller to be executed.
In the redirect method you use a loop over the routes, which is costly (in terms of performance).  A better option would simply be to do a lookup via isset.  if (isset($this->_url[$url])) {}.
You have no handling of error conditions.  What happens if someone tries to redirect to a route that doesn't exist?  What happens if there is no @ in the route?  It is best to explicitly check for such situations and throw an exception so that you can provide clear details to the developer.
Similar, enforcing some consistency in your data can be helpful.  Your router obviously assumes that routes start with a slash, so you should enforce that in the addRoute method.  This will take care of a whole class of subtle but hard-to-identify bugs down the road.
Your names are a bit inconsistent.  See addRoute vs parsecommand.  Camel case for one but all lowercase for the other.  Pick a convention and stick to it.  If in doubt use PSR.
Your variables names could use a bit more consistency too. Instead of using a private variable named _urls in your router, I think routes would make much more sense.  Also (this could be a matter of preference), you don't really need the leading underscore in the name of private variables.  IMO, that's a convention that primarily belongs in languages that don't have enforced visibility rules.
Writing docblocks will help a lot in the long run.
Your parsecommand method returns whatever the controller returns, but the redirect method ignores the return value.  This makes sense because your controllers don't actually return anything.  If the controllers will always print directly, then there is probably no reason for paresecommand to return anything.

Some larger suggestions:

Dependency Injection:

As I mentioned before, you don't really want to access the $_GET super global (or other super globals) directly.  It is better to use a separate class to get that information out.  It all boils down to separation of concerns.  As a simple example of where this can break down, how would your current setup handle things if you decided to add a command line component?  Perhaps that would have a completely separate router, but perhaps you would rather use the same system you already have in place.  In that case your current setup simply won't work - it is hard-coded to look at $_GET, which doesn't exist in the command line environment.  Instead you could have your router expect to receive a request object in the constructor, and that request would tell it the requested URL.  In the case of a normal web request the request object would simply get the URL out of the $_GET super global, but for a command line request you would perhaps send in a different request object that gets the url out of the command line arguments.

This all boils down to the concept of dependency injection - instead of directly creating/managing your own dependencies, you let the person in charge of you give the necessary dependencies.  This makes the code much more flexible.  It also allows you to create integration and unit tests, which is otherwise impossible when you are directly hooked into the request itself.  For the moment of course I wouldn't suggest switching completely over to managing your system with a full dependency injection container.  However, if you get in the habit of separating out your services a bit more and (in this example), expecting a request object to come in with the constructor, you'll start getting in much better habits.  While you are keeping things simple, now is the time to start learning about dependency injection and test writing.

Autoloading

In your router you require the controller PHP file directly.  This implies that you don't have autoloading setup.  Setting that up in your bootstrap process will save you a lot of trouble.  For instance, your current setup requires your controllers to all live in the same directory.  With an autoload setup that maps namespaces to file directories, you can remove that requirement and make it easier to organize your app.  Then you could do things like:

$routes->addRoute('/shopping/products', '\Shopping\ProductController@index');
$routes->addRoute('/support/contact', '\Support\ContactController@index');

Presumably your files would then be located in (something like) Controllers/Shopping/ProductController.php, Controllers/Support/ContactController.php, etc...  This will help you keep your app organized better if it starts growing larger than a handful of controllers.

Performance

I don't think this is either a surprise or a problem for you, but just to say out loud, this setup will start to cause (performance) trouble if you start to add dozens/hundreds of endpoints.  It doesn't sound like that is your intention, but for something like this to scale to large apps you may need a different routing mechanism or some caching mechanism (or a completely different solution).

That's what I have for now!  Let me know if you have any followup questions/comments.


Waring's prime number conjecture This is from Wikipedia:

  In mathematics, Waring's prime number conjecture is a conjecture in number theory, closely related to Vinogradov's theorem. The conjecture is named after the English mathematician Edward Waring and states that every odd integer exceeding 3 is either a prime number or the sum of three prime numbers. The conjecture is known to follow from the generalized Riemann hypothesis.

My code:

 def combinations(iterable, r):
    # combinations('ABCD', 2) --> AB AC AD BC BD CD
    # combinations(range(4), 3) --> 012 013 023 123
    pool = tuple(iterable)
    n = len(pool)
    if r > n:
        return
    indices = list(range(r))
    yield tuple(pool[i] for i in indices)
    while True:
        for i in reversed(range(r)):
            if indices[i] != i + n - r:
                break
        else:
            return
        indices[i] += 1
        for j in range(i+1, r):
            indices[j] = indices[j-1] + 1
        yield tuple(pool[i] for i in indices)

def prime_number(maximum):
  number_list=(i for i in range(1,maximum+1))
  divisor=1
  divisor_list=[]
  prime_number_list=[]
  for e in number_list:
    while divisor<=e:
      if e%divisor==0:
        divisor_list.append(divisor_list)
        divisor+=1
      else:
        divisor+=1
    if len(divisor_list)==2:
      prime_number_list.append(e)
      prime_number_list.append(e)
      prime_number_list.append(e)
      prime_number_list.append(e)
      divisor=1
      divisor_list=[]
    else:
      divisor=1
      divisor_list=[]
  return prime_number_list

def div_by_two(num):
  if num%2==0:
    return False
  else:
    return True

def Waring_Number(maximum):
  input_list=range(5,maximum+1)
  combination_of_prime_number=combinations(prime_number(maximum),3)
  prime_number_list=prime_number(maximum)
  odd_number=[i for i in input_list if div_by_two(i)]
  for e in odd_number:
    if e in prime_number_list:
      print ("{} is a prime number.".format(e))
    else:
        for combination in combination_of_prime_number:
          if sum(combination)==e:
            print ("{}+{}+{}={}".format(combination[0],combination[1],combination[2],e))
            break

Few Notes:

I just copied and pasted the combination function from itertools documentation, I do not know how the function actually works, only how it works.
prime_number_list.append(e) was repeated four times because the combinations had to include prime numbers being repeated three times too. (For example, (3,3,3) has to be created for Waring Number to work) I know this takes up time.
I am a beginner. I have only been doing Python for few months for hobby and only know the basics

Any and all feedback is welcome.

Naming. Mostly good. You're making the effort to be descriptive with it, which is awesome. I would change odd_number to odd_numbers since it's a list. Also, prime_number really tells me nothing about what the function is actually doing. Not going to suggest an alternative name, because I'll have more to say about this function later. Warring_Number violates PEP 8. (warring_number instead.)

More, descriptive functions. This can make it easier to get a high-level understanding of what's going on when you're reading it. For example, at the start of warring_number, you could do odd_numbers = get_odd_numbers_in_interval(5, maximum). More importantly though, in your prime_number function, you can just do:

for e in number_list:
    if is_prime(e):
        #etc.

This not only makes it more clear what you're doing. You're also making it so that when you want to speed up checking if a number is prime, you don't have to worry about how that logic will affect anything else. It's all right in one place.

Speaking of...

Avoid brute force. If you finding yourself iterating over every single possibility, it's usually worth it to see if there's a clever way to speed things up. Checking if a number is prime has been researched a lot, so I would just go to Wikipedia or StackOverflow and use an algorithm that I'm not smart enough to come up with myself. For checking if a number is a sum of three primes, let's think about it.

Right now, your check for, say, 13 looks something like this:

2 + 2 + 2 = 6
2 + 2 + 3 = 7
2 + 2 + 5 = 9
2 + 2 + 7 = 11
2 + 2 + 11 = 15
etc.

Let's do it manually, and apply a bit of thought.

for i in list_of_primes:
    for j in list_of_primes:

At this point, there is a unique number x such that i + j + x = e. So let's just check that and move on

        if is_prime(e - (i + j)):

More on prime_numbers: I really don't like that it returns 4 values of each prime. It's not an obvious thing to do, and I would prefer to have the calling code manipulate the return value so it looks like it needs to. In any case, you already have a use for the function where you don't want duplicates of every prime. Additionally, it is a rather expensive function, and you call it twice. Instead, you could do:

prime_number_list = prime_number(maximum)
combination_of_prime_number = combinations(prime_number_list, 3)

Style comments: Spaces around operators, please. if e % divisor == 0 is much easier to read than if e%divisor==0.

Instead of

if boolean:
    return False
else:
    return True

You can just do:

return not boolean

Actually, I believe this is a bug in div_by_two. It should just be

def div_by_two(n):
    if n % 2 == 0:
        return True
    else:
        return False

Which would simplify to

def div_by_two(n):
    return n % 2 == 0


Custom C# Dapper ORM wrapper I am I C#.NET developer for a few years now... Last couple of months I have started to study design patterns and enterprise software architecture principles so that I will be able to use those principles in my code... Well here is actual example what I need. Right now I am developing ASP.net web API REST services. My data is stored in MySQL relational database. In data access layer I would like to use Dapper micro ORM, so I would like to create some kind of my own ORM wrapper methods... If I decide in the future to change to some other ORM that I won't need to rewrite my whole DAL layer code... Here is the code how I am planning to do this: What do you think about my approach? (my goal here is not to be technology (dapper) dependent...)

public abstract class BaseORMCommandSettings //SQL command base class
{
    public string CommandText { get; private set; }
    public object Parameters { get; private set; }
    public IDbTransaction Transaction { get; private set; }
    public int? CommandTimeout { get; private set; }
    public CommandType? CommandType { get; private set; }
    public CancellationToken CancellationToken { get; private set; }

    public BaseORMCommandSettings(string commandText, object parameters =
    null,
    IDbTransaction transaction = null, int? commandTimeout = null,
                         CommandType? commandType = null, CancellationToken
    cancellationToken = default(CancellationToken))
    {
        this.CommandText = commandText;
        this.Parameters = parameters;
        this.Transaction = transaction;
        this.CommandTimeout = commandTimeout;
        this.CommandType = commandType;
        this.CancellationToken = cancellationToken;
    }
}

public class DapperCommandSettings : BaseORMCommandSettings//dapper cmd impl
{
    public CommandFlags Flags { get; private set; }

    public DapperCommandSettings(string commandText, object parameters =
    null, IDbTransaction transaction = null, int? commandTimeout = null,
                         CommandType? commandType = null, CancellationToken
    cancellationToken = default(CancellationToken), CommandFlags flags =
    CommandFlags.Buffered)
     :base(commandText, parameters, transaction, commandTimeout,
    commandType, cancellationToken)
    {
        this.Flags = flags;
    }
}

public interface ICustomORM //base interface, for now have only generic Read
                        list method
{
    IEnumerable<T> Read<T>(BaseORMCommandSettings cmd);
}

public class DapperORM : ICustomORM //my own dapper ORM wrapper implentation
{
    private readonly IDbConnection con;

    public DapperORM(IDbConnection con)
    {
        this.con = con;
    }

    public IEnumerable<T> Read<T>(BaseORMCommandSettings cmd)
    {
        var cmdDapper = cmd as DapperCommandSettings;
        var dapperCmd = new CommandDefinition(cmdDapper.CommandText,
        cmdDapper.Parameters, cmdDapper.Transaction,
                                          cmdDapper.CommandTimeout,
        cmdDapper.CommandType, cmdDapper.Flags,
                                          cmdDapper.CancellationToken);

        return con.Query<T>(dapperCmd);
    }
}

To be clear I think Adriano makes a very good point but still I wanted to provide some feedback:

I think you should separate piece of functionality in different classes.

You could abstract the handling of transactions and connections away in e.g

public class SqlHelper
{
    private readonly IDbConnectionFactory _dbConnectionFactory;

    private readonly IExecutionPolicy _executionPolicy;

    public SqlHelper(IDbConnectionFactory dbConnectionFactory, IExecutionPolicy executionPolicy)
    {
        if (dbConnectionFactory == null)
        {
            throw new ArgumentNullException(nameof(dbConnectionFactory));
        }
        if (executionPolicy == null)
        {
            throw new ArgumentNullException(nameof(executionPolicy));
        }
        _dbConnectionFactory = dbConnectionFactory;
        _executionPolicy = executionPolicy;
    }

    public TResult DoInTransaction<TResult>(Func<IDbTransaction, TResult> task,
        IsolationLevel isolation = IsolationLevel.Unspecified)
    {
        using (var conn = CreateConnection())
        {
            conn.Open();
            using (var transaction = conn.BeginTransaction(isolation))
            {
                try
                {
                    var result = task(transaction);
                    transaction.Commit();
                    return result;
                }
                catch (Exception)
                {
                    transaction.Rollback();
                    throw;
                }
                finally
                {
                    conn.Close();
                }
            }
        }
    }
}

this helper is now responsible for only handling connections and transactions and if, in the future, you find a new ORM which uses the IDbConnection like Dapper does, you can re-use it.

Next thing you could do is make use of the "repository pattern" in my opinion this should not be a generic interface with CRUD operations, but It could be that.

using Dapper;

public class DapperAccountTable : IAccountTable
{
    private readonly SqlHelper _sqlHelper;

    public DapperAccountTable(SqlHelper sqlHelper)
    {
        _sqlHelper = sqlHelper;
    }

    public AccountData FindAccountRecordByEmail(Username username)
    {
        if (username == null)
        {
            throw new ArgumentNullException(nameof(username));
        }
        return _sqlHelper.DoWithConnection((conn) =>
        {
            var account =
                conn.Query<AccountData>("SELECT * FROM Account WHERE Email = @Email AND Deleted = 0",
                    new {Email = username.ToUnicodeDbString()}).SingleOrDefault();
            if (account != null)
            {
                account.AdvisorId = QueryPartnerId(conn, account);
            }
            return account;
        });
    }

This repository is now the abstraction you are looking for. If you now want to move a way from dapper and use e.g. NHibernate you could create a new implementation of the IAccountTable interface and let this be injected instead of the DapperAccountTable.

Last but certainly not least is that this is only possible when using  Inversion of Control

I think that this is the maximum amount of abstractions that is possible in a DAL. Other ORM's like EF and NHibernate differ so much from Dapper that you always have to re-write your repositories.


Removing specific items from a slice Given a slice of unknown length (potentially very small or very large), and another slice containing (unique) integers indicating indices to be removed from that slice, the most straight forward way to achieve this would be to sort the indices in reverse order then remove each of them from the slice in turn via:

mySlice = append(mySlice[:i], mySlice[i+1:]...)

Alternatively I could create a new empty slice like:

newSlice = make([]*Whatever, 0, len(mySlice)-len(indices))

And build it up like:

sort.Ints(indices)
nextIndex := 0
for i, thing := range mySlice {
    if nextIndex == len(indices) || i == indices[nextIndex] {
        nextIndex++
    } else {
        newSlice = append(newSlice, mySlice[i])
    }
}

I haven't benchmarked these yet, because I would rather understand why one would be faster than the other (and possibly under what conditions).

In particular, what actually happens in memory to the slice in the first strategy? I understand that slices are simple abstractions over arrays which are contiguous blocks of memory, but does removing the middle item of a slice require that memory to be moved around, such that it would be quicker to manage copying over every second item to a new slice/array than to remove every second item from the original slice?

You are using the builtin append() function, you can read its documentation what it does.

In summary, if the destination slice has enough capacity (cap()) to accomodate the elements you're appending to it, no new array will be allocated, but the destination slice will be resliced and the elements will be copied there.

If the destination slice does not have enough capacity, a new array will be allocated, and the current content of the destination slice will be copied into that, and the elements you're appended will follow. A slice referring to the newly allocated array will be returned.

In your case since you're removing elements, the slice will always have enough capacity, so no new array will be allocated. But still, the elements after the element/index being removed has to be copied forward by 1. In case of big slices this will be relatively much work. And it has to be as many times as many elements/indices you want to remove.

Your implementation does not work though. It causes a run-time panic if the removable indices slice does not contain the last index of the input slice, because your code increments nextIndex beyond len(indices) and indexes it like indices[nextIndex]. To make it work, you should use the following if:

if nextIndex < len(indices) && i == indices[nextIndex] {

Also it is much faster to allocate the new slice with len = len(input) - len(ind) than with len = 0 as you did; and use simple assignment to append 1 element instead of calling the built-in append() function:

s2 := make([]int, len(s)-len(idx))

sort.Ints(idx)
nexti, j := 0, 0
for i := range s {
    if nexti < len(idx) && i == idx[nexti] {
        nexti++
    } else {
        s2[j] = s[i]
        j++
    }
}

Next note that we don't need a new slice, we can do the removal "in-place", in the input slice:

sort.Ints(idx)
nexti, j := 0, 0
for i := range s {
    if nexti < len(idx) && i == idx[nexti] {
        nexti++
    } else {
        s[j] = s[i]
        j++
    }
}

s = s[:len(s)-len(idx)]

And as the final, fastest solution:

We will use the original slice, and we will not copy elements 1-by-1. Elements between the removable indices form a contigous part which we can copy in one step using the built-in copy() function. Note that the first part (before the first removable index) is already in place, we don't even have to copy that.

func remove4(s, idx []int) []int {
    if len(idx) == 0 {
        return s
    }
    sort.Ints(idx)

    prev := idx[0]
    for i := 1; i < len(idx); i++ {
        v := idx[i]
        copy(s[prev-i+1:], s[prev+1:v])
        prev = v
    }
    copy(s[prev-len(idx)+1:], s[prev+1:])

    return s[:len(s)-len(idx)]
}

Try all the examples on the Go Playground.

One final note:

By excluding certain elements from a slice by reslicing it, the underlying array will still contain reference to them, the value will remain in memory. It is recommended that whenever an element removed from a slice, always zero its place in the underlying array (its respective element in the slice) so the value will not remain in memory needlessly. This becomes even more critical if your slice contains pointers to big data structures.

In my example I used int, so no need to zero it.

As an easy example: if you want to remove the last element of a slice:

var s []*SomeType

// Remove last element, but first zero its value:
s[len(s)-1] = nil
s = s[:len(s)-1]


Random song playing app improvements I have a substantial music collection, which will not load entirely in any music program.

So I wrote this quick program, to play a random song in a subfolder of my music directory.

I'm just wondering if anyone can improve on this code? It works pretty well, but the subdirectories are not all equal with respect to size, so it neglects some directories. My random mod might also be a problem. This is just to have a useful shuffle.

import java.io.File;
import java.io.IOException;

public class Pick {
public static void main(String args[]) throws IOException {

    File f = new File("C:\\MUSIC");
    File random = null;
    try {
        random = goDeeper(f);
    } catch (Exception e) {
    }
    while (random == null) {
        try {
            random = goDeeper(f);
        } catch (Exception e) {
        }
    }

    System.out.println(random.getAbsolutePath());
    Process myProcess = Runtime.getRuntime().exec(
            "\"C:\\Program Files (x86)\\foobar2000\\foobar2000.exe\"   \""
                    + random.getAbsolutePath() + "\"  ");

}

public static File goDeeper(File f) {
    File[] children = f.listFiles();

    int rand = (int) ((Math.random() * 123456) % children.length);
    File random = children[rand];
    if (random.isDirectory()) {
        return goDeeper(new File(f.getAbsolutePath() + "/"
                + random.getName()));
    } else if (random.isFile() && random.getName().endsWith("mp3"))
        return random.getAbsoluteFile();

    return null;
}

}

Don't repeat yourself

The try-random-catch block appears two times in your code. The easiest way to fix this would be to delete the first occurrence. You don't need it, as the while check will be true on the first loop, thus executing that block.

Handling Exceptions

There are a lot of ways to handle an exception, but just silently ignoring it is not one of them. It will make it very hard to find bugs.

Alternatives are:

just let the exception bubble up all the way
log the exception to a file (for detailed information use getStackTrace)
print the exception to console (best use printStackTrace, not getMessage)
handle the exception

It is also important to not catch a blanket Exception, but the specific exceptions that may be thrown. With your code, a reader doesn't really know what might be thrown (and thus has no idea at all what might have gone wrong).

Random file selection

Your selection process isn't really random.

Let's say you have structure like this:

/
/dir
/song.mp3
/dir/nonMp3File
/dir/anotherSong.mp3

Now, we enter goDeeper, and randomly select dir. On the next iteration, we randomly select nonMp3File. Now, null is returned and we restart at /. That means if you have a lot of non-mp3 files in you music folder (cover art, etc), the deeper nested the file, the less likely it will be that it will be selected, because the goDeeper returned null first.

Even if you fix 1., your program will still favor songs that are less deeply nested inside subdirectories.

This might not be a bad thing (it depends on the structure of your music directory, and how you want to select) though.

the hardcoded 123456 lets me suspect that you are not really randomly selecting among the children. Something like rand = new Random().nextInt(children.length + 1); might be better.

Infinite loop

If you don't have any MP3 files, the program will loop forever. If the ratio of MP3 to non-MP3 is bad, the program might loop for a long time before playing a song.

Naming

random might be randomFileName (both in main and goDeeper).
f might be baseDirectory (for the variable as well as the parameter).
goDeeper might be getRandomFileName.

Misc

Use curly braces, even for one-line statements to avoid bugs and to increase readability.
Remove unused variables: you are never using myProcess, so just remove it (not the call to exec, just the variable declaration).


Tarjan's strongly connected components algorithm Can anyone suggest to me how I can make this "nicer"? If you can see any glaring details that I could change to speed things up too, that would be great -- though it's not priority.

public class Tarjans {

    private static class Node {
        // -1, -1, our node is unvisited
        // by default.
        public int index = -1, lowLink = -1;
        public String name;

        public Node(String name) {
            this.name = name;
        }

        public String toString() {
            return name;
        }
    }

    // graph representation
    HashMap<String, Node> nodes = new HashMap<>();
    HashMap<String, ArrayList<Node>> graph = new HashMap<>();

    private int index = 0;

    // for a DFS, basically
    private ArrayDeque<Node> visited = new ArrayDeque<>();
    private HashSet<String> stack = new HashSet<>();

    // stores our strongly connected componts
    // from the algorithm pass
    private HashSet<HashSet<Node>> scc = new HashSet<>();

    public HashSet<HashSet<Node>> tarjan() {
        for (Node n : nodes.values()) {
            if (n != null && n.index == -1) {
                strongConnect(n);
            }
        }
        return scc;
    }

    private void strongConnect(Node node) {
        node.index = index;
        node.lowLink = index;
        index += 1;

        visited.push(node);
        stack.add(node.name);

        ArrayList<Node> neighbours = graph.get(node.name);
        if (neighbours != null) {
            neighbours.forEach(n -> {
                if (n.index == -1) {
                    strongConnect(n);
                    node.lowLink = Math.min(node.lowLink, n.lowLink);
                }
                else if (stack.contains(n.name)) {
                    node.lowLink = Math.min(node.lowLink, n.index);
                }
            });
        }

        if (node.lowLink == node.index) {
            HashSet<Node> cycle = new HashSet<>();
            while (true) {
                Node p = visited.pop();
                stack.remove(p.name);
                cycle.add(p);

                if (p == node) {
                    break;
                }
            }
            if (cycle.size() > 1) {
                scc.add(cycle);
            }
        }
    }

    private void foo() {
        nodes.put("bs", new Node("bs"));
        nodes.put("a", new Node("a"));
        nodes.put("b", new Node("b"));
        nodes.put("c", new Node("c"));

        graph.put("bs", new ArrayList<>(Arrays.asList(nodes.get("a"))));
        graph.put("a", new ArrayList<>(Arrays.asList(nodes.get("b"))));
        graph.put("b", new ArrayList<>(Arrays.asList(nodes.get("c"))));
        graph.put("c", new ArrayList<>(Arrays.asList(nodes.get("a"))));

        HashSet<HashSet<Node>> cycles = tarjan();
        for (HashSet<Node> cycle : cycles) {
            System.out.println("[" + cycle.stream().map(Node::toString).collect(Collectors.joining(",")) + "]");
        }
    }

    public static void main(String[] args) {
        new Tarjans().foo();
    }

}

There is no reason to use String as keys in graph. By default an object can be used as a key in a hashmap based on it's uniqueness.

There is no reason that the values have to be ArrayLists instead just use List.

HashMap<Node, List<Node>> graph = new HashMap<>();

This can clean up the init code as you don't have to do new ArrayList<> and can instead just pass Arrays.asList(nodes.get("a")) directly.

Though having said all that you can just put the List of neighbours straight in the Node and not deal with having to look it up in strongConnect.


OOP design for multiple user authentication I'm learning OOPs design principles and design patterns. As an exercise, I came up with this model for multiple user authentication using OOPs. I plan to build this on top of a Flask server, but I've just mocked the behavior for now.

Please let me know your thoughts about the implementation design. Thanks in advance.

  User Model:

class User:
    def __init__(self, name: str = None, ph_no: str = None, username: str = None, password: str = None):
        self.username = username
        self.password = Utils.encrypt(password)
        self.ph_no = ph_no
        self.name = name

        Utils.persist_in_db(self)

    def update_password(self, old_password: str, new_password: str):
        if Utils.encrypt(old_password) == self.password:
            self.password = Utils.encrypt(new_password)
        else:
            raise PasswordNotCorrectException()

    def authenticate(self, password) -> bool:
        return self.password == Utils.encrypt(password)

    def __repr__(self):
        return f'<User: {OrderedDict(self.__dict__)}>'

  Core logic:

import threading
import time
from login_service import LoginService
from models.user import User

class BMSApp:
    def __init__(self):
        self.mock()

    @staticmethod
    def mock():
        user1 = UserThread(name='Rick',
                             username='ricky',
                             ph_no='xxxxx',
                             password='password1234')

        user2 = UserThread(name='Morty',
                             username='morty',
                             ph_no='xxxx',
                             password='lifeisawesome')

        user1.start()
        user2.start()

        user1.join()
        user2.join()

class UserThread(threading.Thread):
    def __init__(self, **kwargs):
        super().__init__()
        self.kwargs = kwargs

    def run(self) -> None:
        BookingStrategy.execute(**self.kwargs)

class BookingStrategy:
    @staticmethod
    def execute(**kwargs):
        login_result = LoginService.login(**kwargs)
        if login_result[0]:
            print(f"Login succeeded for user {login_result[1]}")
        else:
            print(f"Login failed for user {login_result[1]}")

  Mock Login service:

class LoginService:
    @staticmethod
    def login(**kwargs):
        #         TODO: implement proper login functionality
        user = User(**kwargs)
        return random.choice([(True, user), (False, user)])

First of all, I am not at all sure it is good idea to learn OOP by re-implementing security-sensitive feature for a web-framework, which already has support for such things. Please, at least consider using Flask-Security, Flask-Login, Flask-Principal unless you plan a total rewrite of all those. Also, I see plain text passwords in use - most of the time a security risk.

That said, I can find the following problems:

Persistence service is too intrusive and its logic is not clear.
For example, you are creating a user just to login: Why do you want to
persist anything someone tries to enter as a login attempt? Consider
using some ORM, where you can control object's persistence, or come
up with something better than object dealing with its persistence.

Those aspects better be invisible in the model. In Python, it's probably best to use a DTO (data-transfer object, received from the login form, usually a dict) until identity is verified.
I do not really understand why you are using threads. In the Flask environment one can use a session as an abstraction, so dealing at thread level is strange and can make your code too dependent on a specific implementation, preventing porting it to multiprocessing, async/greenlets and the like.

There are some naming problems (what is ph_no?).

In my opinion the best place for OOP in user authentication is (in addition to User class) a possibility to define different authentication strategies (and inject dependencies like Utils.encrypt that way). Then, if a more complex system is in question - role-based authorization, etc, etc. The code you presented above is not geared towards those goals. Instead, it deals with low-level concerns which are already solved in the framework.