Dijkstra's algorithm Assuming everything else works as expected, is the following a correct - but not the most efficient way - of implementing Dijkstra algorithm? Because I have tested it on several examples and it gives correct result. But I can't imagine Dijkstra is that simple.

  public static void FindShortestPath(Node source, Directed graph)
    {
        if(source==null)
        {
            System.err.println("Source doesn't exist.");
            System.exit(0);
        }
       source.setCost(0); // all nodes are initialized with cost=Double.Max_VALUE when first created.
       PriorityQueue<Node> pq=new PriorityQueue<>(); // Node implements comparable according to cost
       for(Node node:graph.getAdj().keySet()) // get all nodes
          pq.add(node);
        while(!pq.isEmpty())
        {
            Node u=pq.poll(); // the minimum
            List<Node> neighbours=graph.getAdj().get(u); // get the minimum direct successors
            for(Node v:neighbours)
            {
                if(v.getCost()>(u.getCost()+(graph.getCost(u, v)))) // if there is a cheaper one
                {
                    v.setCost(u.getCost()+graph.getCost(u, v)); // change its cost
                    pq.remove(v);  //remove it
                    pq.add(v);  // add it again
                }
            }
        }
    }

Answering your question - yes, this is a correct solution (the algorithm is pretty simple), and yes, it is not the most efficient one.

The efficiency depends on the implementation of the priority queue. In your case, I assume this is PriorityQueue provided in java.util. The more efficient way uses what is called a Fibonacci Heap. Unfortunately, there is no implementation available in java.util, but there are plenty of implementations online (like this one). You can test your solution using one of them.

I will also suggest a couple of things you could implement to improve readability of your code.

Naming

In Java, methods should start with a lowercase character, unlike classes. So it is best practice to name your method findShortestPath.

Also, while frequent in mathematical notation, variable names like u and v can be confusing. These are Nodes in your case, so you can name them accordingly: currentNode, neighbourNode, or at least just current and neighbour.

Formatting

One of your loops contains only one statement, but I would strongly suggest adding brackets anyway or extracting the whole thing to a separate method (see below).

Additionally, consider:

for(Node v:neighbours)

for(Node v : neighbours)

It's a small thing, but most code formatters will do it this way, and it is easier to read.

The same goes for:

Node u=pq.poll();

Node u = pq.poll();

The same applies to mathematical operations, like:

v.getCost() > (u.getCost() + (graph.getCost(u, v))

If you are using an IDE like Eclipse or IntelliJ IDEA, there are features that format your code automatically.

Comments and methods

Comments are good if you are writing complex code that could be hard to understand without them. That's not the case here. Compare:

if(v.getCost()>(u.getCost()+(graph.getCost(u, v)))){ // if there is a cheaper one
    // do stuff
}

with

if(thereIsACheaperOne(u,v)){
    // do stuff
}

private boolean thereIsACheaperOne(Node u, Node v){
   return v.getCost()>(u.getCost()+(graph.getCost(u, v)))
}

When the method names you are using are meaningful, there is no need to comment code - it comments itself. If you extract a lot of statements like this, reading the algorithm becomes really easy. It becomes a short list of easy to understand calls, instead of lots of mathematical operations here and there.

And if you are unsure about what a certain step does, you can just read one specific method body.

A good rule for naming the method could be what I just done - just name the method just like you would phrase the comment.

In general, one method should be doing one thing and one thing only.

pq.remove(v);  //remove it
pq.add(v);  // add it again

Comments like these are pretty much useless. Anyone with a minimum knowledge about programming will know what this code does. These comments make the code messy.

And more...

In your code, you use graph.getAdj() twice. Now, what will happen, if something (e.g. another thread) changes the graph after you added all your nodes, but before you started calculating distances? I suggest that you create a local final variable in your method:

final <WhateverTypeItIs> adjacencyStructure = graph.getAdj()

This way, you are sure that you are working on the same copy of the adjacency structure (be it a list, or matrix) everywhere in your method.

You could also consider adding exception handling to your method instead of checking for source == null. And why are you only checking source? What happens if graph is null?

I would separate this into two parts: first, try to get all the variables needed and then call a separate method that is the algorithm itself. All this in a try-catch block. You could easily catch the NullPointerException and inform the user that either source or graph is not set up properly. Or, better, add a bit more logic to that to ensure which of these is not set up properly.


C struct or pointer to struct? I am currently using this code (and more) to deal with 2D matrices. I am not very proficient in C, so I am not quite sure, if this is a good way of doing it.
I've seen other people in similar situations using pointers to a struct, but I didn't want to complicate things...

typedef struct CMatrix {
    double *m;
    uint32_t width;
    uint32_t height;
} CMatrix;

-

CMatrix newCMatrix(uint32_t width, uint32_t height) {
    CMatrix rtn;
    rtn.m = malloc(width * height * sizeof(double));
    bzero(rtn.m, width * height * sizeof(double));

    rtn.width = width;
    rtn.height = height;

    return rtn;
}

-

CMatrix ma = newCMatrix(2, 2);

All ideas are welcome!

Pointers seem more complicated, but they're usually faster and more correct.

Faster

For example, perhaps you have a function like this:

CMatrix add(CMatrix left, CMatrix right)
{
     ... add the matrices ...
}

In the above you're passing matrix parameters by value instead of by pointer. That means that you're passing a copy of each matrix (instead of a pointer to each matrix).

Assuming you have 32-bit pointers, each matrix is 12 bytes in size (4 bytes for the pointer and 4 bytes for each integer), so you're passing 24 bytes (for two matrices) instead of 8 bytes (for two pointers-to-matrices).

That (24 bytes instead of 8 bytes) is not a big difference (i.e. not much slower); but it's a bigger difference when your structs are larger and more complicated.

More correct

Another problem is that subroutines are operating on a private, local copy of the matrix; for example:

void transpose(CMatrix matrix)
{
    ... height becomes width and vice versa ...
    uint32_t oldWidth = matrix.width;
    uint32_t oldHeight = matrix.height;
    matrix.width = oldHeight;
    matrix.height = oldWidth;
    ... and transpose the data ...
}

This function changes the width and height of its local copy of the matrix, not the width and height of the caller's copy of the matrix.

Summary

Your code happens to work because:

You have a simple structure
You don't alter height and width in a subroutine, after you create the matrix
The data itself within the matrix (i.e. the double *m pointer) is a pointer (and is therefore shared between subroutines)

However it's bad practice: if you're coding in C, you should become familiar with using pointers.

There's nothing wrong with the newCMatrix function you showed in the OP. What I'm criticizing is your statement that pointers complicate things, because they're usually necessary, e.g. for other functions; for example, the 'add' function above should (in order to be idiomatic) be declared more like the following:

CMatrix add(const CMatrix *left, const CMatrix *right)
{
     ... add the matrices ...
}


Function to check whether a path is valid or not needs your critique Here is my code, I use it to verify whether an input path is correct or not. I assume that the input path is encoded in UTF8. Is there any way to make it better?

#define MAX_PATH_LEN MAX_PATH

#define IN
#define OUT

typedef int Bool_T;

static
int GetCharsNumInPath( IN const char * path )
{
    int cnt;

    int i;
    int n = strlen( path );

    for( i = 0; i < n; i++ )
    {
        if( ( path[i] < 0x80 ) || ( path[i] > 0xbf ) )
            cnt++;
    }

    return cnt;
}

Bool_T PathValid( IN const char * path )
{
    if( path != NULL )
    {
        if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )
        {
            int i;
            int n = strlen( path );

            for( i = 0; i < n; i++ )
            {
                switch( path[i] )
                {
                    // ? " / < > * |
                    // these characters can not be used in file or folder names
                    //
                    case '?':
                    case '\"':
                    case '/':
                    case '<':
                    case '>':
                    case '*':
                    case '|':
                        return false;

                    // Can meet only between a local disk letter and full path
                    // for example D:\folder\file.txt
                    //
                    case ':':
                    {
                        if( i != 1 )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }

                    // Space and point can not be the last character of a file or folder names
                    //
                    case ' ':
                    case '.':
                    {
                        if( ( i + 1 == n ) || ( path[i+1] == PATH_SEPERATOR_CHAR ) )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }

                    // two backslashes can not go straight
                    //
                    case PATH_SEPERATOR_CHAR:
                    {
                        if( i > 0 && path[i - 1] == PATH_SEPERATOR_CHAR )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }
                }
            }

            return true;

        }else{ // if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )

            LOG_ERROR( "PathValid FAILURE --> path is too long" );
            return false;
        }

    }else{ // if( path != NULL )

        // wrong argument
        //
        return false;
    }
}

Personally if  the preconditions to the function are not met then I like to return quickly and this not add to the nesting:

Bool_T PathValid( IN const char * path )
{
    if( path != NULL )
    {
        // wrong argument
        return false;
    }

    if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )
    {
        LOG_ERROR( "PathValid FAILURE --> path is too long" );
        return false;
    }

    // Main Function body here.
}

Of course in C you don't want to have too many return points from a function (as at each return point you need to close all open resources and tidy up. So in C (unlike C++) I find my code looking like this:

<type>   function(<Parameters>)
{
      if (!<pre-condition1>)
      {    return <errorStat1>;
      }

      if (!<pre-condition2>)
      {    return <errorStat2>;
      }

      ....

      if (!<pre-condition-N>)
      {    return <errorStat-N>;
      }

      <type>   result = <initial Result>;
      <Set all Resource to NULL>

      // Code Which can generate errors>
      // If it does then set error state. and stop processing.
      // BUT DO NOT RETURN from this code.

      < Close all resource >
      return result;
}

Your check for invalid characters is Windows specific. The OS will usually have a platform or OS specific set of functions for checking valid characters.

Your set of invalid characters is not true for all platforms.
The use of ':' as the second character is Windows only.

If it is there the first character should be 'A' - 'Z'

Limiting the use of space is Windows only
Are you sure about "." not being allowed as last character?
On windows two '\\' can be used as the first two characters to indicate network resource.
Windows has more than one path separator.

The old style '\' (traditional backslash)
The new style '/'  (normal slash).

In your function counting characters in the path. You basically traverse the string twice. Why not combine the two traversals into a single pass:

// int n = strlen( path );
// Not requires (all this does is look for '\0')

for( i = 0; path[i]; i++ )
{       //  ^^^^^^^  Test for '\0'  character

    if( ( path[i] < 0x80 ) || ( path[i] > 0xbf ) )
    {    cnt++;
    }
}


Creating nodes and edges I'm looking for a way to optimize this piece of code in Python.

all_users holds all the Twitter users and friends_ids is an array of IDs.

g = Graph(directed=True)
all_users = users.find({}, { 'id_str' : 1, 'friends_ids' : 1}) # This is from MongoDb

names = []
edges = []
print 'Processing first 1000 batch'
for idx, user in enumerate(all_users):
    if idx % 1000 == 0 and idx != 0:
        print 'Processing ' + str(idx) + ' batch'
    names.append(str(user['id_str']))
    for friend_id in user['friends_ids']:
        edges.append((user['id_str'], str(friend_id)))
        if not str(friend_id) in names:
            names.append(str(friend_id))

print len(names)
print len(edges)

all_users has 4k records but each of the user has at least 10k of friends_ids. In order to create a graph I need a list of nodes which I have in names and list of edges edges.

The format of edges would be [(node1, node2), (node2, node3), ...], which means that node1 connects to node2 and node2 connect to node3. I'm not sure why but this script takes almost full 10 min to run. So, I'm looking for a way to optimize this.

Try this minor variation over your script, that will probably have a dramatic effect on performance:

names = set()
edges = []

for idx, user in enumerate(all_users):
    if idx and not idx % 1000:
        print('Processing {} batch',format(idx))
    user_id = str(user['id_str'])
    names.add(user_id)
    for friend_id in user['friends_ids']:
        friend_id = str(friend_id)
        edges.append((user_id, friend_id))
        names.add(friend_id)

The main modification is making names a set, not a list, Your script is checking membership of every friend_id in names, an operation that takes time linear in the size of names. In contrast, membership in a set is done in constant time.

If you need names to be a list, simply do:

names = list(names)

after the processing is completed.


Searching for songs on an Android application In my application I need to search for some songs on my external storage card every now and then. On startup I read everything, however when it comes to new activities I thought I could just pass the file paths and read the songs I need again.

I've created this function:

public static Song getSong(Context c, String songPath) {
    Cursor audioCursor = c.getContentResolver().query(
            MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
            new String[] { "*" }, MediaStore.Audio.Media.DATA + " = " + songPath, null, Media.TITLE + " ASC");

    if (audioCursor != null) {
        if (audioCursor.moveToFirst()) {

                String path = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.DATA));
                String title = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.TITLE));
                String album = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.ALBUM));
                String artist = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.ARTIST));
                int album_id = audioCursor.getInt(audioCursor.getColumnIndex(MediaStore.Audio.Media.ALBUM_ID));
                int trackid = audioCursor.getInt(audioCursor.getColumnIndex(MediaStore.Audio.Media.TRACK));
                Long duration = audioCursor.getLong(audioCursor.getColumnIndex(MediaStore.Audio.Media.DURATION));

                Song s = new Song();
                s.setPath(path);
                s.setTitle(title);
                s.setArtist(artist);
                s.setAlbum(album);
                s.setAlbumId(album_id);
                s.setTrackId(trackid);
                s.setDuration( new Duration(duration) );

                return s;
        }
    }
    return null;
}

This function would either return null if not found or the song object created from the file's data.

Is it well-written?  Any improvements?
Is it better than using a singleton class?

Situation:

I click on a ListViewItem which represents an Album. It shows me a new Activity with another ListView which holds all the Songs from that Album. So I would just read every Song again via its path.

Intent i = new Intent( this, blabla.class );
i.putStringArrayListExtra( "paths", pathList );

When I use a Singleton class I would just set the selected Album to that Singleton class and read all the Songs from it again in the onCreate() method from the new Activity.

In general the code seems fine and it is not horrible or anything, but I think you could surely have improvements.

High level

Currently your code returns null if audioCursor == null, I think this should be an exception, possibly an existing one, or a custom one. I'm not exactly sure what it does, so I cannot give more advice.

General coding style

I suggest you to change your coding style to more consistent and I think you can enhance readability with some simple changes.

Take a look at:

Cursor audioCursor = c.getContentResolver().query(
        MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
        new String[] { "*" }, MediaStore.Audio.Media.DATA + " = " + songPath, null, Media.TITLE + " ASC");

I understand that CodeReview is quite limited in width and your IDE is not necessarily so limited, but since you decided to split up the arguments, I would prefer it like this:

Cursor audioCursor = c.getContentResolver().query(
    MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
    new String[] { "*" },
    MediaStore.Audio.Media.DATA + " = " + songPath,
    null,
    Media.TITLE + " ASC"
);

This way all arguments are clearly visible and it looks consistent.

Similar arguments hold for:

String path = audioCursor.getString(audioCursor
        .getColumnIndex(MediaStore.Audio.Media.DATA));

Here you are breaking up one argument, I would prefer to break it up outside, like this:

String path = audioCursor
        .getString(audioCursor.getColumnIndex(MediaStore.Audio.Media.DATA));

Then lastly your blanks are off on this line:

s.setDuration( new Duration(duration) );

I'd say there is no reason to add them, it can even create confusion as now I start to think about the reason of the blanks, this is more clear:

s.setDuration(new Duration(duration));

Early exiting

Currently you have all your code in the if (audioCursor != null) statement like this:

if (audioCursor != null) {
    //your code
    return value;
}
return null;

This leads to clutter and less space to actually type code because of the indentation, the preferred way would be to do so:

if (audioCursor == null) {
    return null;
}
//your code
return value;

This way you exit early and the rest of your code is not impacted by this flow.

Variable naming

Most variable names look okay, except Context c and Song s, I see no reason to not use the full name like context and song, it will not harm anybody.
When taking a closer look, I actually noticed that album_id and trackid are also off, they are not consistent and neither in the recommended camelCase, they should be albumId and trackId.

Code safety

I would recommend to make Context context and String songPath final, I would say it is important for context, such that you cannot accidentally reassign the context variable.

Performance note

Currently you create a new String[] { * } on every method call, for maximum performance you could create a private final static String[] STAR_STRING_ARRAY = new String[] { * } and reference that one such that you do not need to create a new string array at every call. It depends on the amount of calls this method receives to see whether it is really worth it.

One more high level look

After analyzing the code I see it can be decomposed into two methods:

public static Song getSong(Context context, String songPath) { ... }
private static Song getSong(Cursor audioCursor) { ... }

Where the latter would be called once you successfully have obtained the audioCursor and that one would do the low level job.

Conclusion

My conclusion still is that the code overall is fine minus the coding style issues, but these improvements would improve it.
I think it is also better than a Singleton class, because this to me simply looks like an utility method, hence warranting a place in a AudioUtils class, but I do not see a reason for a Singleton class for this.


Handling click events on ingredients list I'm learning programming and would appreciate any feedback on my code. I've come up with the below code. The logic works fine but the code itself seems rather confusing to me. Are there some patterns I could use to improve this code?

// This code snippet handles click events on the Ingredients list (see the image below)

$('.items a').click(function (event) {

    // Remove leading letter from the href id (e.g. id="i15" --> 15)
    ingr = parseInt((this.id).substring(1));

    // Here comes the complicated logic for handling click event
    // It works well but can it be improved/simplified?
    if (buy.indexOf(ingr) != -1) {
        if (weHave.indexOf(ingr) != -1) {
            if (weNeed.indexOf(ingr) === -1) {
                weNeed.push(ingr);
                whIndex = weHave.indexOf(ingr);
                weHave.splice(whIndex, 1);
            }
        } else {
            if (weNeed.indexOf(ingr) === -1) {
                weHave.push(ingr);
            } else {
                weHave.push(ingr);
                wnIndex = weNeed.indexOf(ingr);
                weNeed.splice(wnIndex, 1);
            }
        }
    } else {
        if (weHave.indexOf(ingr) != -1) {
            weNeed.push(ingr);
            whIndex = weHave.indexOf(ingr);
            weHave.splice(whIndex, 1);
        } else {
            if (weNeed.indexOf(ingr) != -1) {
                weHave.push(ingr);
                wnIndex = weNeed.indexOf(ingr);
                weNeed.splice(wnIndex, 1);
            } else {
                weNeed.push(ingr);
            }
        }
    }
    shoplist();
    return false; // to prevent the page from scrolling to the top
});

EDIT: Edited the code above and provided more background information below.

Here is a simplified picture of the application I'm trying to create:

Briefly how the application should work:

The idea is to create a shopping list by selecting food and/or ingredients
Clicking on a food item will highlight the food item and all necessary ingredients
Clicking on already highlighted food item will remove the highlighting from the food item and all necessary ingredients
Clicking on an ingredient will highlight the ingredient
Clicking on already highlighted ingredient will remove the highlighting from the ingredient

The above is basic logic but here is where it gets complicated. There are couple of scenarios I'm trying to handle in the code above (these are independent scenarios):

I click on Spaghetti Bolognese. Necessary ingredients, including onion, get highlighted. Then I click on onion, indicating that I already have this ingredient at home. The highlighting is removed from the onion. Then I click on Pork Stew (which requires onion) but onion shouldn't get highlighted because I have already indicated that I do have an onion at home.
I click on Spaghetti Bolognese. Necessary ingredients, including onion, get highlighted. Then I click on Pork Stew. Pork Stew requires onion too so the onion item stays highlighted. But then I decide I don't want to make Spaghetti Bolognese after all so I unclick it. The onion should stay highlighted because it's required for Pork Stew.

So to handle these kind of scenarios I came up with two arrays: weNeed and weHave.

Here is a list of inputs/outputs:

ingr: integer; clicked ingredient
buy: unique list of ingredients, concatenation of weNeed + food (food is an array of ingredients coming from the FOOD list; not in the above code)
weNeed: list of ingredients collected when user clicks on unhighlighted ingredient item
weHave: list of ingredients collected when user clicks on highlighted ingredient item

P.S. Looking back at my question this has grown into a monster. I'm not sure if the extra information I provided is helpful or even more confusing. I could post the whole code if it helps but something is telling me that dealing with these kind of complex issues on the forum is probably not the right way to do things. I'll be happy for any suggestions.

Druciferre seems to have found a way to streamline your current code quite nicely (I would add that an "is-value-x-in-array-y?" helper function would greatly improve readability, as all the indexOf comparisons make me cross-eyed. A "remove-x-from-array-y" function would also help DRY the code), so allow me to suggest a different tack altogether; more rewrite than tweak. Reading your description, here are my suggestions.

1. Objects for each recipe

There are plenty of ways to do this, but the simplest, most direct one I can think of is:

var recipes = [
  {
    name: "Pork stew",
    ingredients: ["pork", "stew", "onion"]
  },
  {
    name: "Spaghetti bolognese",
    ingredients: ["spaghetti", "city of bologna", "onion"]
  },
  // ....
];

(can you tell I don't cook much?)

Anyway, the point is that each recipe "contains" its ingredients. A neat logical encapsulation of data.

2. Use underscore.js (or similar, or roll your own) array functions

Underscore's got some neat array functions. Use 'em. Let's say you have both of the above recipes selected. To get all the ingredients needed, you could do

// concatenate every recipe's ingredients into one array
var requiredIngredients = _.reduce(selectedRecipes, function (ingredients, recipe) {
  return ingredients.concat(recipe.ingredients);
}, []);
requiredIngredients = _.uniq(requiredIngredients); // remove duplicates

Edit Changed _.map to _.reduce which is what I meant to write first time around, but somehow messed up.

Thus, requiredIngredients will be:

["pork", "stew", "onion", "spaghetti", "city of bologna"]

So now you have an array of all the ingredients needed to cook those two dishes.

Now, let's say you also have an array of ingredients that the user has manually selected and one that contains those that he/she already has (i.e. manually unselected):

var ownedIngredients = ["onion", "olive oil"];
var selectedIngredients = ["curry"];

// combine manually selected ingredients with recipe ingredients,
// and remove duplicates
var allIngredients = _.uniq(requiredIngredients.concat(selectedIngredients));

// "subtract" unselected ingredients
var shoppingList = _.difference(allIngredients, ownedIngredients);

And, well, that's pretty much it. When you select a new recipe, re-run that code. When you add/remove an ingredient, re-run that code. With just the above you'd get a shopping list of

["pork", "stew", "spaghetti", "city of bologna", "curry"]

Of course, this is me taking a stab in the dark, not knowing the rest of your app. But this, to me, seems like a better approach than manually trying to manage arrays element-by-element.


Drawing fractals with Turtle So, I have my code here:

import turtle

class fractal:

    """A class containing methods that will replicate
    various fractals.

    Attributes:
        num: An integer representing the number of iterations
        t: A turtle graphics object imported from Turtle.py
    """

    def __init__(self, num, t=turtle.Turtle()):
        self.num = num
        self.t = t

    def kochCurve(self):
        axiom = 'F'
        for _ in range(self.num):
            linSys = 'F+F--F+F'
            axiom = axiom.replace('F',linSys)
        for i in axiom:
            if i == 'F':
                self.t.forward(10)
            elif i == '+':
                self.t.left(60)
            elif i == '-':
                self.t.right(60)

    def kochIsland(self):
        axiom = 'F-F-F-F'
        for _ in range(self.num):
            linSys = 'F+FF-FF-F-F+F+F'
            axiom = axiom.replace('F',linSys)
        for i in axiom:
            if i == 'F':
                self.t.forward(10)
            elif i == '+':
                self.t.left(90)
            elif i == '-':
                self.t.right(90)

    def tree(self):
        axiom = '[X]'
        for _ in range(self.num):
            linSys = 'F-[[X]+X]+F[+FX]-X'
            axiom = axiom.replace('X',linSys)
            axiom = axiom.replace('F','FF')

        self.t.setheading(90)
        self.t.setposition(0,-300)
        stack = []
        for i in axiom:
            if i == 'F':
                self.t.forward(10)
            elif i == '+':
                self.t.right(25)
            elif i == '-':
                self.t.left(25)
            elif i == '[':
                pos = self.t.position()
                head = self.t.heading()
                stack.append((pos,head))
            elif i == ']':
                (pos,head) = stack.pop()
                self.t.penup()
                self.t.setposition(pos)
                self.t.setheading(head)
                self.t.pendown()

fractal(3).kochCurve()

Overall, the code just seems a bit redundant and LONG. I'm creating this class to share on a repository for some others to view. However, I don't particularly enjoy the way it looks.

Essentially, this class makes a turtle from Turtle Graphics and draws fractals with it. So far, I have the three different fractals. All that's needed to call into the object is an integer that will be the number of iterations.

What improvements can I make here?

For each shape, you have this kind of code to generate the movement "recipe":

axiom = ...
for _ in range(self.num):
    replacement = 'some fixed string'
    axiom = axiom.replace(pattern, replacement)
    axiom = axiom.replace(pattern2, replacement2)

This can be generalized with a helper function that takes as parameters axiom and a list of pattern-replacement pairs. All shapes will be able to use this helper and thereby reduce duplication.

The execution of the movement commands is also repetitive. The code for the most complex shape would be able to draw all the simpler shapes, with a single parameter of the turn degrees.


Simple Password Generator Feedback This is my first major project in F#, looking for any critique on how I can make it more standard or more concise.
Some things I feel like I could have done better are the command flag parsing and the actual crypto random generation itself. I do know that crypto generators are slower than pseudo rng but I figure that a crytpo generator is better for generating something that should be secure.

open System.Security.Cryptography
open System.Text
open System

[<EntryPoint>]
let main argv =
    let rec contains item list : bool =
        match list with
        | [] -> false
        | head::tail -> (head = item) || contains item tail

    let strContainsOnlyNumber (s:string) = System.UInt32.TryParse s |> fst

    let rec buildChars chars (args: string list) : char list =
        let numbers = ['0'..'9']
        let lower = ['a'..'z']
        let upper = ['A'..'Z']
        let special = ['!' .. '/'] @ ['@']
        match args with
        | [] -> lower @ upper @ numbers
        | head::_ when args.Length = 1 && strContainsOnlyNumber head -> (chars @ lower @ upper @ numbers)
        | head::tail when head = "-l" || head = "--lower" ->if tail.Length > 0 then buildChars (chars @ lower) tail else chars @ lower
        | head::tail when head = "-u" || head = "--upper" -> if tail.Length > 0 then buildChars (chars @ upper) tail else chars @ upper
        | head::tail when head = "-s" || head = "--special" -> if tail.Length > 0 then buildChars (chars @ special) tail else chars @ special
        | head::tail when head = "-n" || head = "--numeric" -> if tail.Length > 0 then buildChars (chars @ numbers) tail else chars @ numbers
        | _::tail when chars.Length = 1 && tail.Length = 0 -> lower @ upper @ numbers
        | _::tail when chars.Length = 1 -> buildChars chars tail
        | _ -> chars

    let rec buildString  (bytes: byte list) (chars: char list) (builder: StringBuilder) : string =
        match bytes with
        | [] -> builder.ToString()
        | head::tail -> buildString tail chars <| builder.Append chars.[(int) head % chars.Length]

    use rng = new RNGCryptoServiceProvider()
    let bytes : byte array = Array.zeroCreate <| if strContainsOnlyNumber(argv.[0]) then Convert.ToInt32 argv.[0] else 16
    rng.GetBytes(bytes)
    let byteList : byte list = bytes |> Array.toList

    let characters = (buildChars [' '] (argv |> Array.toList)).Tail

    let sb = StringBuilder()
    let pass = buildString byteList characters sb

    printfn "%s" <| pass

    0

As a first attempt in F# you have passed the test, but the overall impression is a little messy, while the algorithm seems to (almost) work as expected.

If calling it with an empty argument list - from this:

match args with
        | [] -> lower @ upper @ numbers

in buildChars, I would expect it to produce a 16 chars password from the default settings. But it fails here:

let bytes : byte array = Array.zeroCreate <| if strContainsOnlyNumber(argv.[0]) then Convert.ToInt32 argv.[0] else 16

with an IndexOutOfRangeException

   let rec contains item list : bool =
        match list with
        | [] -> false
        | head::tail -> (head = item) || contains item tail

This is not used, so delete it. If you need a contains function, the List-module has a predefined such ready to use.

Especially buildChars seems overly messy and complicated, and it is not very efficient that the char lists (numbers, lower, etc) are redefined for each recursion. Instead of having buildChars as a rec function you could have an inner recursive function to the matching and then define the char lists out side of that:
let buildChars chars (args: string list) : char list =
    let numbers = ['0'..'9']
    let lower = ['a'..'z']
    let upper = ['A'..'Z']
    let special = ['!' .. '/'] @ ['@']
    let rec listBuilder chars args =
        match args with
        | [] -> lower @ upper @ numbers
        // ... etc.

    listBuilder chars args

Besides that, I think I would think of another design, if I find my self repeating the almost same code like in this function. List.fold may be a solutions in this case.
Another issue with the function is that if the argument list contains more of the same argument (for instance "-l", "-l") it will be included more than once making the result somewhat biased. Maybe consider to reduce the argument list to a distinct set - unless you want the behavior.

You could consider to print help/information, if the argv has a "-?" entry.

In F#, lists are very convenient because of its operators that makes the code more readable, but in this particular algorithm, I think I would stick to use arrays for everything, because you address the list entries by index, which is not efficient for lists, because chars.[index] is an O(index) operation where the same operation is O(1) for arrays, further List.length is a O(n) operation - adding more inefficiency to the equation.

let rec buildString  (bytes: byte list) (chars: char list) (builder: StringBuilder) : string =
    match bytes with
    | [] -> builder.ToString()
    | head::tail -> buildString tail chars <| builder.Append chars.[(int) head % chars.Length]

This function is not tail recursive and therefore builds up the stack. For a password generator it will probably never be an issue, but there is a hypothetical risk for a stack overflow. Fortunately you can easily make it tail recursive and more efficient, because builder.Append returns the builder itself. So changing the last line to
| head::tail -> buildString tail chars (builder.Append chars.[(int) head % chars.Length])

makes the function tail recursive.

Below is my version with some inline explanation:
let passwordGenerator (argv: string []) =

    // The @ - operator for lists is temporarily redefined to work with arrays
    // in order to make the code more readable
    let inline (@) left right = Array.append left right

    // From the first argument or a possible empty argument list the
    // custom size and if the default settings should be used is determined
    let useDefaults, size =
        match argv.Length with
        | 0 -> true, 16
        | _ ->
            match (Int32.TryParse(argv.[0])) with
            | true, n -> (argv.Length = 1), n
            | false, _ -> false, 16

    // The usable characters are determined from the arguments
    let chars =
        let lower = [| 'a'..'z' |]
        let upper = [| 'A'..'Z' |]
        let numbers = [| '0'..'9' |]
        let special = [| '!' .. '/' |] @ [| '@' |]

        if useDefaults then
            lower @ upper @ numbers
        else
            // This will avoid duplicate chars
            let predicate arg short long (chs: char[]) all =
                (arg = short || arg = long) && not (all |> Array.contains (chs.[0]))

            let folder all arg =
                match arg with
                | a when predicate a "-l" "--lower" lower all -> all @ lower
                | a when predicate a "-u" "--upper" upper all -> all @ upper
                | a when predicate a "-n" "--numerics" numbers all -> all @ numbers
                | a when predicate a "-s" "--special" special all -> all @ special
                | _ -> all

            argv |> Array.fold folder [||]

    // Provides the random bytes
    let bytes =
        use rng = new RNGCryptoServiceProvider()
        let bytes = Array.zeroCreate size
        rng.GetBytes(bytes)
        bytes

    // Generates the password
    let password =
        bytes
        |> Array.map (fun b -> chars.[int b % chars.Length])
        |> fun chs -> new String(chs)

    printfn "%s" password


Writing to file in a thread safe manner I'm writing Stringbuilder to file asynchronously. This code takes control of a file, writes a stream to it and releases it. It deals with requests from asynchronous operations, which may come in at any time.

The FilePath is set per class instances (so the lock Object is per instance), but there is potential for conflict since these classes may share FilePaths. That sort of conflict, as well as all other types from outside the class instance, would be dealt with with retries.

Is this code suitable for its purpose? Is there a better way to handle this that means less (or no) reliance on the catch and retry mechanic?

Also how do I avoid catching exceptions that have occurred for other reasons?

public string Filepath { get; set; }
private Object locker = new Object();

public async Task WriteToFile(StringBuilder text)
    {
        int timeOut = 100;
        Stopwatch stopwatch = new Stopwatch();
        stopwatch.Start();
        while (true)
        {
            try
            {
                //Wait for resource to be free
                lock (locker)
                {
                    using (FileStream file = new FileStream(Filepath, FileMode.Append, FileAccess.Write, FileShare.Read))
                    using (StreamWriter writer = new StreamWriter(file, Encoding.Unicode))
                    {
                        writer.Write(text.ToString());
                    }
                }
                break;
            }
            catch
            {
                //File not available, conflict with other class instances or application
            }
            if (stopwatch.ElapsedMilliseconds > timeOut)
            {
                //Give up.
                break;
            }
            //Wait and Retry
            await Task.Delay(5);
        }
        stopwatch.Stop();
    }

I'm not sure why this is async.

Unless you have a good reason for write to the file asynchronously, this is wasted effort.

If you lock properly, this will work fine.  Your locker variable should be marked static.  You can then get away with something like this:

using System;
using System.IO;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace test
{
    public class Writer
    {
        public string Filepath { get; set; }
        private static object locker = new Object();

        public void WriteToFile(StringBuilder text)
        {
            lock (locker)
            {
                using (FileStream file = new FileStream(Filepath, FileMode.Append, FileAccess.Write, FileShare.Read))
                using (StreamWriter writer = new StreamWriter(file, Encoding.Unicode))
                {
                    writer.Write(text.ToString());
                }
            }

        }
    }
}

This assumes you don't have some other reason for making this async.


An array function that returns an array containing all elements appearing a certain number of times Array.prototype.itemsWithQuantity = function(n) {
    var counts = {};
    var result = [];
    var i = this.length;
    while(i--){
        counts[this[i]] = counts[this[i]] == undefined ? 1 : counts[this[i]] + 1;
    }
    i = this.length;
    var alreadyPushed = {};
    while(i--){
        if(counts[this[i]] == n && !alreadyPushed[this[i]]){
            alreadyPushed[this[i]] = true;
            result.push(this[i]);
        }
    }
    return result;
};

Above is my function that returns all the items in an array with a certain quantity. E.g., [1,2,3,2,3,4].itemsWithQuantity(2) == [2,3].

Not only is the code kinda ugly, but I also have a feeling there's a more efficient way to do it that I'm not spotting.

I guess you could use Array.forEach() to do the counting and laterArray.filter() to get the elements with the desired number of occurences.

It reduces the bookkeeping to the minimum of counts, goodbye alreadyPushed. It also reduces the boilerplateness a little, by giving you local variables implicitely:

You have this[i] everywhere, which can look a little cryptic. You could store it in a local var element = this[i]; which could be considered more readable.
in my suggested code, you get those local variables as a result of using functions that receive the value as a parameter which you can of course name however you want.

I think it depends on how much you are used to seeing certain patterns show up in code. After doing it for a while, you know that

var i = this.length;
while(i--){ //...}

will iterate over all the elements. Array.forEach() calls it by its name, which can improve readability.

I had my first ever go at the built in scripting thingy here on stackexchange, take a look and try the code:

Array.prototype.itemsWithQuantity = function(n) {
  var counts = {};

  // count
  this.forEach(function(element) {
    counts[element] = counts[element] == undefined ? 1 : counts[element] + 1
  });

  // return those elements (which are the keys(=property names) of the object) occuring n times
  return Object.keys(counts).filter(function(element) {
    return counts[element] == n;
  });
};

var input = [1, 2, 3, 3, 4, 1, 2, 5];
var output = input.itemsWithQuantity(2);

document.getElementById("output").innerHTML = output.toString();


Solving a one-dimensional Euler equation for fluid dynamics I just wrote a program to solve one dimensional Euler equation for fluid dynamics. Attached is a snippet of the vtune profile result for one of the function.
There are a few things which looks weird to me. For example, difference in line 72 takes more time than division in line 70. Also line 70 takes more time than 69 which include a sqrt! Can anyone explain the discrepancy? Also suggestion to improve (in terms of performance) this piece of code is welcome.

This particular routine is called many times in the code so I want this to be as fast as possible.

Additional information:

This module is part of a multi-threaded program (using OpenMO).
I am using Intel C++ compiler, icpc, with -O3.
My ultimate aim is to run this code on Xeon-Phi.

Function

__declspec(vector) void roeflux(double rhol, double rhor, double ul, double ur, double pl, double pr,
                double *f_0, double *f_1, double *f_2){
  double el = pl/(GAMMA_M) + 0.5f*rhol*ul*ul;
  double er = pr/(GAMMA_M) + 0.5f*rhor*ur*ur;
  double hl = (el + pl)/rhol;
  double hr = (er + pr)/rhor;

  double sqrtrhol = sqrt(rhol);
  double sqrtrhor = sqrt(rhor);
  double den_inverse = 1/(sqrtrhol + sqrtrhor);
  double uavg = (sqrtrhol*ul + sqrtrhor*ur)*den_inverse;
  double havg = (sqrtrhol*hl + sqrtrhor*hr)*den_inverse;
  double cavg = sqrt(GAMMA_M*(havg - 0.5f*uavg*uavg));
  double cavg_inverse = 1.0f/cavg;

  double d1 = rhor - rhol;
  double d2 = rhor*ur - rhol*ul;
  double d3 = er - el;

  double alpha_2 = GAMMA_M*((havg - uavg*uavg)*d1 + uavg*d2 - d3)*cavg_inverse*cavg_inverse;
  double alpha_3 = 0.5f*(d2 + (cavg - uavg)*d1 - cavg*alpha_2)*cavg_inverse;
  double alpha_1 = d1 - alpha_2 - alpha_3;

  double lambda_1 =  fabs(uavg - cavg);
  double lambda_2 =  fabs(uavg);
  double lambda_3 =  fabs(uavg + cavg);

  double f1 = lambda_1*alpha_1 + lambda_2*alpha_2 + lambda_3*alpha_3;
  double f2 = lambda_1*alpha_1*(uavg-cavg) + lambda_2*alpha_2*uavg + lambda_3*alpha_3*(uavg+cavg);
  double f3 = lambda_1*alpha_1*(havg-cavg*uavg) + 0.5f*lambda_2*alpha_2*uavg*uavg + lambda_3*alpha_3*(havg+cavg*uavg);

  *f_0 = 0.5f*((rhol*ul + rhor*ur) - f1);
  *f_1 = 0.5f*((rhol*ul*ul + pl + rhor*ur*ur + pr) - f2);
  *f_2 = 0.5f*(ul*hl*rhol + ur*hr*rhor - f3);
}

I'm going to assume your target machine is a super scalar CPU with some variation of Tomasulo architecture. I'm also going to assume that there is no significant cache miss ratio at cause for your performance (you should verify this).

When compiling optimised code, the compiler will rearrange operations to reduce stalls due to availability of input data to the instructions. Hence you cannot completely trust the line numbers in the profiler. As the CPU is super scalar some instructions may execute and complete out of order (but are committed in order).

I see that you are computing the reciprocal of cavg and den to save yourself two divisions. It is possible (but doubtful) that this could cause a delay stage as the two dependent operations could end up stalling until the result is available instead of executing in parallel on different execution units.

Contrived example:

double a_inv = 1.0/a;
double b = x*a_inv;
double c = y*a_inv;

If executed as is this would require two execution stages. The two multiplications would have to wait until the division is completed before they can start. The total delay is 1 div + 1 mul (the two mul can execute in parallel on the cpu). Compare this to:

double b = x/a;
double c = y/a;

Execution can begin immediately and the total delay is 1 div as the divs can run in parallel. This is assuming the compiler doesn't do this behind your back already (I'm not sure if it is allowed to by the IEEE rules) . It also depends on if there is enough instructions to fill the execution units while calculating the reciprocal. Any way it's worth trying.

Apart from that I can't think of anything that would make the function as it is any faster.

If you really want speed you need to use SIMD instructions in addition to using many thread. But that requires you to restructure your data from "array of structs" to "structs of arrays".

Edit: I've had an opportunity to take a closer look and there are some things that you can do to improve performance.

This calculates cavg* 1/cavg at two occasions:

  double alpha_3 = 0.5f*(d2 + (cavg - uavg)*d1 - cavg*alpha_2)*cavg_inverse;

which can be re-written as:

double alpha_3 = 0.5f*((d2 - uavg*d1)*cavg_inverse + d1 - alpha_2);

This reduces from 4 mult + 3 add to 3 mult + 3 add.

Here you need to make sure that the compiler actually emits fabs instruction and not cmp+cmov:

double lambda_1 =  fabs(uavg - cavg);
double lambda_2 =  fabs(uavg);
double lambda_3 =  fabs(uavg + cavg);

Here, the compiler will perform CSE so lambda_x*alpha_x products will already be available and we can rewrite:

double f1 = lambda_1*alpha_1 + lambda_2*alpha_2 + lambda_3*alpha_3;
double f2 = lambda_1*alpha_1*(uavg-cavg) + lambda_2*alpha_2*uavg + lambda_3*alpha_3*(uavg+cavg);
double f3 = lambda_1*alpha_1*(havg-cavg*uavg) + 0.5f*lambda_2*alpha_2*uavg*uavg + lambda_3*alpha_3*(havg+cavg*uavg);

as:

double la1 = lambda_1*alpha_1; // For readability, compiler does this anyway
double la2 = lambda_2*alpha_2;
double la3 = lambda_3*alpha_3;
double f1 = la1 + la2 + la3;
double f2 = uavg*f1 - cavg*(la1-la3);
double f3 = la1*(havg-cavg*uavg) + 0.5f*la2*uavg*uavg + la3*(havg+cavg*uavg);

which reduces the expression of f2 from 3 mult + 4 add to 2 mult + 2 add as the results from f1 can be re-used. The compiler should put the calculation of uavg*f1 towards the end of the function and calculate f3 while f1 computes.

There could be more of the same type of optimizations you can do by rearranging operations but unfortunately I don't have time to give a more thorough look.

Disclaimer: The compiler may or may not do these optimizations depending on the FP precision setting. For example -ffast-math on GCC would probably do these, but only if -ffast-math as IEEE is not associative due to round off errors. There is an equivalent option for ICC but I don't know what it's called. But be advised this option can break stuff, like NAN handling/checking.


define_method vs method_missing and CodeClimate scores This may be a better fit for another board, but here goes:

I've made a very simple gem that acts as a days-of-the-week bitmask. To be hip, I registered it on CodeClimate, where it's currently scoring a B/3.0 grade.

Since there's only 1 extremely simple class, the B is bugging me. CodeClimate's reason is that there's too much code outside methods. This is no doubt due to my use of define_method to programmatically add read/write accessor methods like monday,  tuesday and so on to the class.

The methods are currently being added like so:

DAY_NAMES = %w(sunday monday tuesday wednesday thursday friday saturday).map(&:to_sym).freeze
DAY_BITS  = Hash[ DAY_NAMES.zip(Array.new(7) { |i| 2**i }) ].freeze

# ...

# Create the day=, day, and day? methods
DAY_BITS.each do |day, bit|
  define_method(day) do
    get_bit bit
  end
  alias_method :"#{day}?", day

  define_method(:"#{day}=") do |bool|
    set_bit bit, bool
  end
end

Note that the class supports two reader syntaxes (with and without ?).

Now, I could use method_missing instead to make CodeClimate happy, but as I see it, that's only more complex. The method names are all known in advance, so method_missing seems unnecessary, and much less efficient than defining the methods on the class once.

If I were to use method_missing I'd do something like this (get_bit and set_bit already accept both strings and symbols):

DAY_NAME_PATTERN = "((?:sun|mon|tues|wednes|thurs|fri|satur)day)".freeze

# ...

def method_missing(method, *args)
  case method.to_s
  when %r{\A#{DAY_NAME_PATTERN}\??\Z}
    get_bit $1
  when %r{\A#{DAY_NAME_PATTERN}=\Z}
    set_bit $1, args.first
  else
    super
  end
end

I'll still need the DAY_NAMES and DAY_BITS constants, so DAY_NAME_PATTERN would be yet another const - one that seems somewhat like duplication (though I could build it from DAY_NAMES of course, though that would be more expressions outside methods).

So to my mind, using method_missing would only improve the CodeClimate score, yet (ironically) make the code worse. Would you agree? Or do you know some clever 3rd way?

Aside: As Tokland rightly notes in his answer a bitmask is a fairly low-level approach to all of this compared to using sets.

I'm using a bitmask because the class is primarily intended to be used as an ActiveRecord serializer in a Rails app. That's where I extracted it from (partly as an exercise for myself). In the case of my app, the database already had a bitmask column, and this was an encapsulation of that logic. It seemed to me a good way to store the value (even if it is a little arcane for Ruby), as it allows for fast but complex day intersection/union queries at the SQL layer.

All of that could still be accomplished using sets, and only calculating the bitmask when necessary. Regardless of the specifics, however, my question is still valid on its own. Tokland's answer is great, but not quite an answer to the specific question.

Notes:

I am afraid I'd propose a very boring solution: don't use meta-programming. I mean, why add methods and more methods, polluting the class, when you can add just one method and use symbols for the days? Add validations if you're worried about invalid values for days.
The usual trick of using powers of two as a mask is that, a trick, very handy in low-level languages (C, assembler, ...). In a higher-level language you strive for writing code as declarative as possible, with data structures like lists, hashes, trees, or sets, not masks of powers of 2 :-) Here I'd use a set.
I prefer functional solutions even with OOP, just return new instances instead of modifying the existing ones. Referential transparency, idempotence, all the goodies follow.

I'd write (orientative, you get the idea):

class WeekSauce
  DAY_NAMES = [:sunday, :monday, :tuesday, :wednesday, :thursday, :friday, :saturday]

  def initialize(days)
    @days = days.to_set & DAY_NAMES
  end

  def has_day?(name)
    @days.include?(name)
  end

  def add_days(names)
    WeekSauce.new(@days.union(names))
  end
end


Scheduling a response callback I have an Android service which must offload a task to a different thread but schedule the response callback unto the calling thread. After much thinking I came up with this code:

public void testRequestingAScheduleUpdateShouldNotBlockAndDeliverItOnTheCallingThread() throws Exception {
    final StreamSchedule mockSchedule = new StreamSchedule(
        new StreamMetadata("TyPhone", "Tidal Waves 028"),
        new StreamMetadata[] { new StreamMetadata(null, "The Progmatic Radio Show 002 with Ashley Bonsall") },
        new StreamMetadata[] { new StreamMetadata("TEKNO", "Sound Escalation 065 with Xabi") }
    );

    final boolean[] threadBlocked = {true};
    LooperThread looperThread = new LooperThread();
    looperThread.start();

    StreamScheduleProvider mockScheduleProvider = mock(StreamScheduleProvider.class);
    when(mockScheduleProvider.getCurrentStreamSchedule()).then(invocation -> {
        threadBlocked[0] = Thread.currentThread().getId() == looperThread.getId();
        return mockSchedule;
    });
    DataAccessModule.scheduleProviderOverride = mockScheduleProvider;

    final long[] callingThreadId = {-1};

    ScheduleUpdateListener updateListener = mock(ScheduleUpdateListener.class);
    doAnswer(invocation -> {
        callingThreadId[0] = Thread.currentThread().getId();
        looperThread.stopLooper();
        return null;
    }).when(updateListener).onScheduleChanged(any(StreamSchedule.class));

    PlaybackServiceBinder serviceBinder = (PlaybackServiceBinder) bindService(getServiceIntent());
    serviceBinder.subscribeScheduleUpdates(updateListener);
    looperThread.getHandler().post(serviceBinder::updateSchedule);
    looperThread.join();

    verify(updateListener).onScheduleChanged(mockSchedule);
    assertFalse("The thread was blocked, this is unacceptable", threadBlocked[0]);
    assertEquals("The update callback was not called on the requesting thread", callingThreadId[0], looperThread.getId());
}

class LooperThread extends Thread {
    private Handler handler;

    @Override
    public void run() {
        Looper.prepare();
        handler = new Handler(Looper.myLooper());
        Looper.loop();
    }

    public Handler getHandler() {
        return handler;
    }

    public void stopLooper() {
        handler.getLooper().quit();
    }
}

It does work (the test fails if I simply execute the callback on the background thread or if I use no background thread at all) but I have this that intense itch I overlooked something that could make this more readable/elegant. Any ideas?

Disclaimer: I don't know Android. I've wrote two or three programs for it.

Also, I don't have your full code so can't be sure about some of the classes this code relies on.

Test name has an "and", seems like two tests?
Test throws Exception, do you have tests for that?
You don't need to start test name with tests, unless you use JUnit 3.
When LooperThread runs, it calls Looper.prepare() and Looper.loop(). What these do?
LooperThread extends Thread, why not implement Runnable? You only implement run() then?
Further reinforcement to point 1) - you have more than one assertion. If verify buckles, other assertions aren't tested. Is that fine?
Why do you need a final boolean[] threadBlocked = {true}; over just a final boolean? I found no other reference than to that one element, via threadBlocked[0]?
Is this test alone it it's class? If so, I'd extract mocking out of it. Readability / clarity / easier to spot things.
Have you tried structuring the test into classic given - when - then or assess - act - assert?


Add Copyright to PDF File Will you review the syntax, structure, and logic of my code. It can be tested using the main.

using iTextSharp.text.pdf;
using iTextSharp.text;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace AddCopyRightPDF
{
    class Program
    {
        static void Main(string[] args)
        {
            string PDFPath = "D:\\SamplePDF\\SamplePDF.PDF";
            string PDFPathUpdated = "D:\\SamplePDF\\SamplePDFUpdated.PDF";

            byte[] b = AddCopyrighttoPDF(PDFPath);

            System.IO.File.WriteAllBytes(PDFPathUpdated, b);

        }

        public static byte[] AddCopyrighttoPDF(string path)
        {
            byte[] b = null;
            string CopyRighText;
            try
            {

                CopyRighText = "Copyright © " + DateTime.Now.Year + " All Rights Reserved.";
                PdfStamper pdfStamper = null;
                PdfReader pdfReader = null;
                try
                {
                    using (MemoryStream ms = new MemoryStream())
                    {
                        pdfReader = new PdfReader(new RandomAccessFileOrArray(path), System.Text.ASCIIEncoding.UTF8.GetBytes("1111"));
                        pdfStamper = new PdfStamper(pdfReader, ms);
                        for (int i = 1; i <= pdfReader.NumberOfPages; i++)
                        {
                            if (i > 1)
                            {
                                Rectangle pageSizeWithRotation = pdfReader.GetPageSizeWithRotation(i);
                                PdfContentByte overContent = pdfStamper.GetOverContent(i);
                                overContent.BeginText();
                                BaseFont baseFont = BaseFont.CreateFont("Helvetica", "Cp1250", false);
                                overContent.SetFontAndSize(baseFont, 7F);
                                overContent.SetRGBColorFill(0, 0, 0);
                                float n2 = 15F;
                                float n3 = pageSizeWithRotation.Height - 10F;
                                overContent.ShowTextAligned(0, CopyRighText, n2, n3, 0F);
                                overContent.EndText();

                            }
                        }

                        pdfStamper.FormFlattening = true;
                        pdfStamper.Close();
                        b = ms.ToArray();

                        ms.Flush();
                        ms.Close();
                    }
                }
                catch
                {
                    b = null;
                }
                finally
                {
                    if (pdfReader != null) { pdfReader.Close(); }
                }
            }
            catch { b = null; }
            return b;

        }
    }
}

From top to bottom

Since AddCopyrighttoPDF() is a public method you should validate its argument path.

Variables should be named using camelCase casing and should be spelled correctly string CopyRighText; -> string copyrightText;

try
{

    CopyRighText = "Copyright © " + DateTime.Now.Year + " All Rights Reserved.";
    PdfStamper pdfStamper = null;
    PdfReader pdfReader = null;

why are these 3 lines of code inside a try..catch block? Which exception could here happen? The try..cacth block can be removed.

You are using an using statement for the MemoryStream which is a good thing, but both PdfReader and PdfStamper are implementing the IDisposable interface as well hence they should be enclosed in an using block as well.

for (int i = 1; i <= pdfReader.NumberOfPages; i++)
{
    if (i > 1)
    {

If the loop would start at int i = 2 the if block would become superflous which saves one indentation-level of thecode.

BaseFont baseFont should be created outside of the loop and reused.

float n2 = 15F; should be named better and should be a constant.

float n3 should be named better.

overContent.ShowTextAligned(0, CopyRighText, n2, n3, 0F); here 0F should be extracted into a constant as well.

Disposing of the MemoryStream, which happens when the end of the using block is reached will Flush() and Close() it as well.

Implementing the mentioned points will lead to

private const float copyrightFontHeight = 7F;
private const float copyrightHorizontalPosition = 15F;
private const float copyrightVerticalBorder = 10F;
private const float copyrightRotationNone = 0F;
public static byte[] AddCopyrighttoPDF(string path)
{
    if (string.IsNullOrEmpty(path)) { return null; }

    string copyrightText = "Copyright © " + DateTime.Now.Year + " All Rights Reserved.";

    try
    {
        BaseFont baseFont = BaseFont.CreateFont("Helvetica", "Cp1250", false);
        using (MemoryStream ms = new MemoryStream())
        using (PdfReader pdfReader = new PdfReader(new RandomAccessFileOrArray(path), System.Text.ASCIIEncoding.UTF8.GetBytes("1111")))
        using (PdfStamper = new PdfStamper(pdfReader, ms))
        {
            for (int i = 2; i <= pdfReader.NumberOfPages; i++)
            {
                Rectangle pageSizeWithRotation = pdfReader.GetPageSizeWithRotation(i);
                PdfContentByte overContent = pdfStamper.GetOverContent(i);
                overContent.BeginText();
                overContent.SetFontAndSize(baseFont, copyrightFontHeight);
                overContent.SetRGBColorFill(0, 0, 0);
                float copyrightVerticalPosition = pageSizeWithRotation.Height - copyrightVerticalBorder;
                overContent.ShowTextAligned(0, copyrightText, copyrightHorizontalPosition, copyrightVerticalPosition, copyrightRotationNone);
                overContent.EndText();
            }

            pdfStamper.FormFlattening = true;
            pdfStamper.Close();
            return ms.ToArray();
        }
    }
    catch { } //empty because if we just want to swallow the exception

    return null;
}

This should behave exactly like your former code. Thats why its not throwing an ArguemntNullException if path == null and why its not throwing an ArgumentException if path is whitespace only.

I couldn't find the C# documentation of BaseFont but if BaseFont is implementing IDisposable as well, it should be enclosed in an using block as well.


Radix sort implementation I have a problem where I need to sort an extremely large number of numbers but the numbers are in a narrow range (0..r-1) where r is typically around 10, and after googling around a bit it seemed like a good time to use radix sort. I have the following implementation below (I had to extract it out of a much larger program and simplify for illustration). It seems reasonably fast, but is there any other trick I can use to further speed it?

public static void sort(int[] data, int radix) {
    int aux[] = new int[data.length];
    int count[] = new int[radix];
    for (int i = 0; i < data.length; i++) {
        count[data[i]]++;
    }
    int cumulate = 0;
    for (int i = 0; i < radix; i++) {
        for (int j = 0; j < count[i]; j++)
            aux[cumulate + j] = i;
        cumulate = cumulate + count[i];
    }
    for (int i = 0; i < aux.length; i++) {
        data[i] = aux[i];
    }
}

Nice question.... but, you're not doing a Radix Sort, but instead you're doing a Counting Sort

As it happens, a counting sort is probably the best sort algorithm you could choose for this problem, so there's no problem in the basic code, just the name you have given it.

There's a significant optimization you can do though, which will save a bunch of memory.... you don't need the aux array at all.

The "counting" part of your code is great:

int count[] = new int[radix];
for (int i = 0; i < data.length; i++) {
    count[data[i]]++;
}

but the actual sorting is a problem. This code:

int aux[] = new int[data.length];
int cumulate = 0;
for (int i = 0; i < radix; i++) {
    for (int j = 0; j < count[i]; j++)
        aux[cumulate + j] = i;
    cumulate = cumulate + count[i];
}
for (int i = 0; i < aux.length; i++) {
    data[i] = aux[i];
}

has a few problems. First up, the bad 1-liner for-statement:

    for (int j = 0; j < count[i]; j++)
        aux[cumulate + j] = i;

should be (with {} braces):

    for (int j = 0; j < count[i]; j++) {
        aux[cumulate + j] = i;
    }

and just manipulate the data array directly

int cumulate = 0;
for (int i = 0; i < radix; i++) {
    for (int j = 0; j < count[i]; j++) {
        data[cumulate + j] = i;
    }
    cumulate = cumulate + count[i];
}

I would actually restructure that loop to make it more apparent how the indexing is done... I would have:

int rad = 0;
for (i = 0; i < data.length; i++) {
    while (count[rad] == 0) {
        rad++;
    }
    count[rad]--;
    data[i] = rad;
}


depthFirstTraverse fully in C++? Wondering if this is fully in C++, and if not can someone help me tell the differences. This was submitted by me last semester, and received a good grade. I'm currently trying to ensure I can tell C++ and C apart.

#include <fstream>
#include <stack>
#include <stdlib.h>  //Not neccessary in C++
#define swap(type, a, b) {type t = a; a = b; b = t;}
using namespace std;

typedef struct {
    int degree, *adjList, //arraySize = degree
        nextToVisit, //0 <=nextToVisit < degree
        parent, dfLabel, nodeName;
} GraphNode;

int numNodes, startNode = 1;
GraphNode *nodes;

void readGraph();
void depthFirstTraverse(int startNode);

int main() {
    readGraph();
    depthFirstTraverse(startNode);
}

void depthFirstTraverse(int startNode) {
    int nextNode, dfLabels[numNodes], nextToVisit[numNodes], parents[numNodes], lastDFLabel = 0;
    stack<int> stackArray;
    for (int i = 0; i < numNodes; i++) dfLabels[i] = nextToVisit[i] = 0;
    stackArray.push(startNode);
    printf("startNode = %d, dfLabel(%d) = %d\n",startNode,startNode,1);
    while (!stackArray.empty()) {
        if (0 == dfLabels[stackArray.top()]) {
            lastDFLabel++;
            dfLabels[stackArray.top()] = lastDFLabel; }
        if (nextToVisit[stackArray.top()] == nodes[stackArray.top()].degree) {
            if (lastDFLabel < numNodes) {
                printf("backtracking %d", stackArray.top());
                stackArray.pop();
                printf(" -> %d\n", stackArray.top()); }
            else { printf("backtracking %d -> %d\n", stackArray.top(), startNode);
                stackArray.pop(); }
        }
        else { nextNode = nodes[stackArray.top()].adjList[nextToVisit[stackArray.top()]];
            nextToVisit[stackArray.top()]++;
            if (0 == dfLabels[nextNode]) {
                parents[nextNode] = stackArray.top();
                printf("processing (%d, %d): tree-edge, dfLabel(%d) = %d\n",
                    stackArray.top(), nextNode, nextNode, lastDFLabel+1);
                stackArray.push(nextNode); }
            else if (dfLabels[nextNode] < dfLabels[stackArray.top()]) {
                if (nextNode != parents[stackArray.top()])
                    printf("processing (%d, %d): back-edge\n", stackArray.top(), nextNode);
                else printf("processing (%d, %d): tree-edge 2nd-visit\n", stackArray.top(), nextNode); }
            else printf("processing (%d, %d): back-edge 2nd-visit\n", stackArray.top(), nextNode);
        }
    } for (int i = 0; i < 30; i++) printf("-");
}

void readGraph() {
    char ignore;
    ifstream digraph("digraph.data");
    digraph >> numNodes;
    nodes = new GraphNode[numNodes];
    for (int i = 0; i < numNodes; i++) {
        digraph >> nodes[i].nodeName >> ignore >> nodes[i].degree >> ignore;
        nodes[i].adjList = new int[nodes[i].degree];
        for (int j = 0; j < nodes[i].degree; j++) digraph >> nodes[i].adjList[j];
    } digraph.close();
    printf ("Input Graph:\nnumNodes = %d\n", numNodes);
    for (int i = 0; i< numNodes; i++) {
        printf("%d (%d): ", nodes[i].nodeName, nodes[i].degree);
        for (int j = 0; j < nodes[i].degree; j++)
            for (int k = 0; k < j; k++)
                if (nodes[i].adjList[j] < nodes[i].adjList[k])
                    swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);
        for (int j = 0; j < nodes[i].degree; j++) printf("%d ", nodes[i].adjList[j]);
        printf("\n");
    } for (int i = 0; i < 30; i++) printf("-");
    printf ("\n");
}

It's still closer to C than C++ IMO.
First of two non C++ issues:

You don't encapsulate your classes. Your whole interface is public. Thus you are now doomed to forever maintain this interface for eternity. Additionally it allows other people to accidentally modify your structure. Use the C++ class system to encapsulate your objects and at least protect your objects from accidental miss use.

Your tree-traversal actually depends on the node object to track traversal. Now that's a bad design thing. You are basically binding yourself to an implementation and limiting the usability of your code. You should most defiantly look up the Visitor Pattern.

The rest are C++ issues:
You are doing resource management manually:
nodes = new GraphNode[numNodes];
// STUFF
nodes[i].adjList = new int[nodes[i].degree];

It would be better to use a std::vector at least that would be exception safe in controlling the resource:
std::vector<GraphNode>   nodes(numNodes);

You are manually loading nodes. Should a node not know how to stream itself onto and off a stream?
    digraph >> nodes[i].nodeName >> ignore >> nodes[i].degree >> ignore;

I would expect it to look like this:
    digraph >> nodes[i];

Or even better create items and push them into the vector:
Since the only thing that seems to be in the file "digraph.data" seem to be GraphNodes then you could do something like this:
std::copy(std::istream_iterator<GraphNode>(digraph),
          std::istream_iterator<GraphNode>(),
          std::back_inserter(nodes)  // Assuming nodes is a vector described above.
         );

This is not C++
    printf("%d (%d): ", nodes[i].nodeName, nodes[i].degree);

The problem with the printf style is that it is not type safe. The main advantage C++ has over C is the exceptional type safety of the language. Use this to your advantage.
This is so bad I nearly swallowed my tounge.:
                swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);

You have #defined swap.

Use all caps for macros.
The reason is to make sure macros do not clash with any other identifiers.
Macros are not type safe.
If there was a conversion from this object to int the compiler will now happily do it.
There is already a type safe macro as part of the standard library.
If you want to override the default swap behavior you just need to rewrite your type specific version and Koenig lookup will automatically find it.

Not a big deal but prefer pre-increment. The reason is it does not matter for POD types but for user defined types it does (or potentially does) matter. If a maintainer at a latter stage changes the type of the loop variable then you don't have to go and change any of the code the pre-increment is already the correct type.
 for (int i = 0; i < 30; ++i)
                      // ^^^^ Prefer pre-increment here

Don't declare multiple variables on the same line:
int nextNode, dfLabels[numNodes], nextToVisit[numNodes], parents[numNodes], lastDFLabel = 0;

Every coding standard is going to tell you not to do it. So get used to it. Technically there are a few corner cases that will come and hit you and putting each variable on its own line makes it easier to read. Also note it looks like you are initializing all values to zero. That's not happening so all of these (apart from the last one) are undefined.
Also defining arrays with a non cost type is not technically allowed.
dfLabels[numNodes],

Some compilers allow it because it is C90 extension. But its not really portable. Use std::vector (or std::array) instead.
Don't put code on the same line as the close braces.
I dobt anybody is going to like that style even the people that want to save vertical line space.
} for (int i = 0; i < 30; i++) printf("-");

And I prefer to put the statement the for loop is going to run on the next line so it is obvious what is happening (but I am not going to care that much about it).
typedefing a structure is not needed in C++
typedef struct {
   // BLAH
} GraphNode;

This is just
struct GraphNode
{
   // BLAH
};

When you loop over your containers you are using a C style (ie you are using an index). It is usually more conventionally to use an iterator style to index over a container (even a simple array). Thus if you ever choose to move to another style of container the code itself does not need to be changed. The iterator style makes the code container agnostic.
Personally I don't think it is a big deal. In this limited context.
    for (int j = 0; j < nodes[i].degree; j++)

You implement (badly) a sort function:
    for (int j = 0; j < nodes[i].degree; j++)
        for (int k = 0; k < j; k++)
            if (nodes[i].adjList[j] < nodes[i].adjList[k])
                swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);

Lets just say in both C/C++ you should probably use the built-in sorting algorithms:
std::sort(&nodes[i].adjList[0], &nodes[i].adjList[nodes[i].degree]);

Or am I reading your intention here incorrectly? Is it some sort of randomizing feature? Either way you should have probably documented this to explain what you are trying to achieve!
Here is a more C++ like version
#include <vector>
#include <map>
#include <iterator>
#include <stdexcept>
#include <fstream>
#include <iostream>

class GraphVisitor; // We will implement the `Visitor Pattern`
class Graph
{
    public:
    class GraphNode
    {
        public:
            // Allows us to visit each node.
            // Don't need to track progress via modifying the node
            void accept(GraphVisitor& visitor);
        private:
        // The streaming operators are defined so we can freeze and thaw data to a stream
        friend std::istream& operator>>(std::istream& stream, Graph::GraphNode& node);
        friend std::ostream& operator<<(std::ostream& stream, Graph::GraphNode const& node);

        // The data is private nobody actually needs to see this
        int                 nodeName;
        std::vector<int>    link;
    };

    // Graph knows how to implement depth first traversal.
    // External entities can't do this because that requires knowledge of the implementation
    void DepthFirstTraversal(size_t startNode);

    private:
        // The streaming operators are defined so we can freeze and thaw data to a stream
        friend std::istream& operator>>(std::istream& stream, Graph& node);
        friend std::ostream& operator<<(std::ostream& stream, Graph const & node);

        // The data is just the nodes
        std::vector<GraphNode>  nodes;

};
// A visitor is easy
// When you only have one node type.
// So we just have one visit method.
class GraphVisitor
{
    public:
        virtual ~GraphVisitor() {}
        virtual void visit(Graph::GraphNode& node, int name, std::vector<int> const& links)    = 0;
};

// All the node does with the visitor is tell it that it can visit.
void Graph::GraphNode::accept(GraphVisitor& visitor)
{
    visitor.visit(*this, nodeName, link);
}

// Stream Nodes
std::ostream& operator<<(std::ostream& stream, Graph::GraphNode const& node)
{
    stream << node.nodeName << ":" << node.link.size() << ":";
    std::copy(node.link.begin(), node.link.end(), std::ostream_iterator<int>(stream, " "));
    return stream;
}
std::istream& operator>>(std::istream& stream, Graph::GraphNode& node)
{
    char ignore;
    int  count;

    stream >> node.nodeName >> ignore >> count >> ignore;

    for(std::istream_iterator<int> loop(stream); count != 0; --count, ++loop)
    {   node.link.push_back(*loop);
    }
    // For some reason you sort your nodes so I am sorting them here.
    // Or I am reading your original code incorrectly.
    std::sort(node.link.begin(), node.link.end());
    return stream;
}
// Stream the whole graph.
// This is simply the number of nodes followed by a list of nodes.
std::ostream& operator<<(std::ostream& stream, Graph const & graph)
{
    stream << graph.nodes.size() << " ";
    std::copy(graph.nodes.begin(), graph.nodes.end(), std::ostream_iterator<Graph::GraphNode>(stream, " "));
    return stream;
}
std::istream& operator>>(std::istream& stream, Graph& graph)
{
    int nodeCount;
    stream >> nodeCount;

    // We could implement it like this.
    // As this would match your current file format nicely and if their
    // is anything after the nodes it still works.
    /*
    for(int loop=0;loop < nodeCount; ++loop)    // I implemented it like this becuase
    {
        Graph::GraphNode    node;
        stream >> node;
        graph.nodes.push_back(node);
    }
    */
    // But if there is nothing after the nodes it could be implemented like this:
    std::copy(std::istream_iterator<Graph::GraphNode>(stream),
              std::istream_iterator<Graph::GraphNode>(),
              std::back_inserter(graph.nodes)
             );
}

// A visitor object used by Graph::DepthFirstTraversal
// It tracks which nodes it has already seen.
// So you do not need to visit them again.
// When a node says it can visit then just visit all the linked nodes.
// If you get a call from a node that has already visited then just exit.
class DepthFirstVisitor: public GraphVisitor
{
    public:
        DepthFirstVisitor(std::vector<Graph::GraphNode>& n)
            : nodes(n)
        {}
    private:
    virtual void visit(Graph::GraphNode& node, int name, std::vector<int> const& links)
    {
        if (visited[&node])
        {   return;
        }

        visited[&node]  = true;
        std::cout << "Visiting Node: " << name << "\n";

        for(std::vector<int>::const_iterator loop = links.begin();loop != links.end();++loop)
        {
            nodes[*loop].accept(*this);
        }
    }
    std::map<Graph::GraphNode const*, bool>     visited;
    std::vector<Graph::GraphNode>&              nodes;
};

// Implementing the Depth First traversal is easy as calling the first node with the
// the correct visitor object
void Graph::DepthFirstTraversal(size_t startNode)
{
    if (startNode >= nodes.size())
    {   throw std::runtime_error("Failed");
    }
    DepthFirstVisitor   visitor(nodes);
    nodes[startNode].accept(visitor);
};

int main()
{
    Graph   graph;

    std::ifstream digraph("digraph.data");
    digraph >> graph;                   // Very easy to load a graph.
    std::cout << graph << "\n\n\n";     // Printing it out is also easy

    graph.DepthFirstTraversal(1); // start at node one instead of zero for some reason!!!!!.
}


Haskell FizzBuzz with a type system The core problem is simple enough, but I've developed a type system that I'm not sure about.  The intention is, if the result is a number, calling num $ fizzBuzz 16, for example, lets me retrieve 16 as an Int.  However, it seems awfully repetitive, to have to mention "Fizz" so many times in the code.  Can it be done better?

data NoiseNum = Fizz | Buzz | FizzBuzz | Num Int
  deriving (Eq)

instance Show NoiseNum where
  show FizzBuzz = "FizzBuzz"
  show Fizz     = "Fizz"
  show Buzz     = "Buzz"
  show (Num n)  = show n

num :: NoiseNum -> Int
num (Num n) = n

fizzBuzz :: Int -> NoiseNum
fizzBuzz n
  | n `mod` 15 == 0 = FizzBuzz
  | n `mod`  3 == 0 = Fizz
  | n `mod`  5 == 0 = Buzz
  | otherwise       = Num n

main = do
  mapM_ putStrLn $ map (show . fizzBuzz) [1..100]

In the issue 23 of The Monad.Reader, there is a nice discussion of FizzBuzz and how to solve it efficiently in Haskell. The efficiency being in terms of future extensions such as "output Bazz if it's a multiple of 7, FizzBazz if it's a multiple of 21, (...)". The proposed solution is linear in the number of conditions.

The final answer goes something like this (but I'd advise you to go through the paper: it's a pretty nice read!):

fizzbuzz n = (test 3 "Fizz" . test 5 "Buzz") id (show n)
  where test d s x | n `mod` d == 0 = const (s ++ x "")
                   | otherwise = x


Simple wrapper for PHP mysqli connection I'm creating some web apis to sync some data between desktop/mobile apps.
Working on it, I wrote a simple wrapper for mysqli connection to simplify querying like:

// examples
$connection = new MySQLiConnection();

$connection->execute("INSERT INTO songs VALUES ('God\'s Plan', 'Drake')");
$connection->execute('INSERT INTO songs VALUES (?, ?)', 'Perfect', 'Ed Sheeran');

$result = $connection->getResult('SELECT * FROM songs');
var_dump($result);

$result = $connection->getResult('SELECT * FROM songs WHERE artist = ?', 'Ed Sheeran');
var_dump($result);

Code of the wrapper:

<?php

final class MySQLiConnection
{
    private $connection;

    public function __construct()
    {
        $this->connection = new mysqli('localhost', 'id', 'password', 'database');

        if ($this->connection->connect_errno)
        {
            throw new mysqli_sql_exception('Failed to open mysqli connection: ' . $this->connection->connect_error);
        }

        if (!$this->connection->set_charset('utf8'))
        {
            throw new mysqli_sql_exception('Failed to set utf8 character set: ' . $this->connection->error);
        }
    }

    public function __destruct()
    {
        if (!$this->connection->close())
        {
            throw new mysqli_sql_exception('Failed to close mysqli connection: ' . $this->connection->error);
        }
    }

    private function buildStatement(string $sql, ...$params) : mysqli_stmt
    {
        if ($statement = $this->connection->prepare($sql))
        {
            if (!empty($params))
            {
                $types = '';

                foreach ($params as $param)
                {
                    if (is_int($param))
                    {
                        $types .= 'i';
                    }
                    elseif (is_float($param))
                    {
                        $types .= 'd';
                    }
                    elseif (is_string($param))
                    {
                        $types .= 's';
                    }
                    else
                    {
                        $types .= 'b';
                    }
                }

                $statement->bind_param($types, ...$params);
            }

            return $statement;
        }

        throw new mysqli_sql_exception('Failed to prepare mysqli_stmt: ' . $this->connection->error);
    }

    public function execute(string $sql, ...$params) : void
    {
        $statement = $this->buildStatement($sql, ...$params);

        $success = $statement->execute();
        $statement->close();

        if (!$success)
        {
            throw new mysqli_sql_exception('Failed to execute mysqli_stmt: ' . $this->connection->error);
        }
    }

    public function getResult(string $sql, ...$params) : mysqli_result
    {
        $statement = $this->buildStatement($sql, ...$params);

        $success = $statement->execute();

        if ($success)
        {
            $result = $statement->get_result();

            $statement->close();
            return $result;
        }

        $statement->close();
        throw new mysqli_sql_exception('Failed to execute mysqli_stmt: ' . $this->connection->error);
    }
}

?>

Error reporting
This is much better than your initial attempt, but somehow it become more verbose and I'll tell you why: you are, so to say, abused the error reporting. mysqli_sql_exception is not intended to be thrown manually. Like it's said in the article I gave you link to, mysqli can throw exceptions by itself - so just let mysqli to do it. And therefore there is no reason to check for errors manually, and it will greatly reduce the amount of repeated code.
Automatic binding
Your automated binding function is rather confusing. Apparently you didn't have any clear idea what this code is intended to do:
                else
                {
                    $types .= 'b';
                }

And I'll tell you - there is no good scenario for it.
Don't write a code just in case, when you just don't know what to write. In such circumstances just don't write any code at all.
Besides, I am scared by functions like this that sniff a type off a variable automatically. If you will have a query that will compare a string stored in a database to a number, the consequences would be fatal. And PHP is known for converting strings to numbers on its own will. So, although quite small, but there is still a possibility that your binding function will ruin your database.
That's why I prefer either a manual explicit type setting or just blunt setting a string type for all variables. At least it won't harm anyone.
In practice it means that I recommend to use string type for all parameters by default, with a possibility of a fallback with manual type setting. So, taken from ny other article, the mysqli binding function would be like
public function execute(string $sql, $params = [], $types = '') : mysqli_stmt
{
    if (!$params) {
        return $this->connection->query($sql);
    }
    $types = $types ?: str_repeat("s", count($params));
    $statement = $this->connection->prepare($sql);
    $statement->bind_param($types, ...$params);
    $stmt->execute();
    return $stmt;
}

this function does a lot of good stuff:

it lets you to run a query that doesn't support prepared statements

it does no harm automatically sniffing the variable's type

at the same time it lets you to provide a type string manually, like
  $conn->execute('INSERT INTO songs VALUES (?, ?, ?)', ['foo', 'bar', 1], "ssi");

I also removed an argument unpacking operator from the function definition as it makes no sense. Trust me, it takes no effort to add two square brackets around parameters, but it makes your code WAY cleaner and more maintainable.
Closing statements
PHP is incredibly programmer-friendly. Resources don't have to be closed manually, they will be closed automatically when no longer needed.
The code
<?php

final class MySQLiConnection
{
    public $connection;

    public function __construct($config)
    {
        mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
        try {
            $this->connection = new mysqli('localhost', 'id', 'password', 'database');
            $this->connection->set_charset('utf8');
        } catch (\mysqli_sql_exception $e) {
            throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
        }
    }

    public function execute(string $sql, $params = [], $types = '') : mysqli_stmt
    {
        if (!$params) {
            return $this->connection->query($sql);
        }
        $types = $types ?: str_repeat("s", count($params));
        $statement = $this->connection->prepare($sql);
        $statement->bind_param($types, ...$params);
        $statement->execute();
        return $statement;
    }

    public function getResult(string $sql, $params = [], $types = '') : mysqli_result
    {
        return $this->execute($sql, $params, $types)->get_result();
    }
}

As you can see, the final code is much more concise, thanks to code reuse and proper error reporting.
I also made $connection variable public, as there is no more getConnection() function but you will need to access it from outside for sure.
An exception is caught in the constructor because of security concerns. A stack trace for the exception thrown in case of a connection error would contain the database credentials that will end up either in the server logs or - the worst case - displayed on the screen. Once re-thrown this way, credentials are no more exposed. So although as a rule you don't catch an Exception right in place, here it's a special case to take care of.


Ramda - Forming a string based on array of word positions I saw this question and wondered how it'd be best to accomplish this using Ramda.

I'm new to the library so I thought it'd be an interesting learning exercise.

I have a working solution but it seems overly complicated and verbose. I'd be grateful for any suggestions on what other ways it would be possible to achieve this output using Ramda.

I'm aware that this doesn't need a library; as I say, I approached it as a learning exercise.

Here's what I have:

const { addIndex, always, fromPairs, join, map, sort, toPairs, unnest } = R;

const song = {
  "99": [0, 7],
  "bottles": [1, 8],
  "of": [2, 9],
  "beer": [3, 10],
  "on": [4],
  "the": [5],
  "wall": [6]
};

const toSentence = join(' ');

const parsed = toSentence(
    map(
      ([_, word]) => word,
      sort(
        ([a, _], [b, __]) => a < b ? -1 : 1,
        unnest(
          map(
            ([word, indices]) => map(
              index => [index, word],
              indices,
            ),
            toPairs(song)
          )
        )
      )
    )
  )

console.dir(parsed)
<script src="https://cdnjs.cloudflare.com/ajax/libs/ramda/0.26.1/ramda.js"></script>

This is a great question.

I found no easy Ramda functional way to solve this, but you could just use a forEach approach. I find it easier to read.

const {forEachObjIndexed, forEach} = R;

const song = {
  "99": [0, 7],
  "bottles": [1, 8],
  "of": [2, 9],
  "beer": [3, 10],
  "on": [4],
  "the": [5],
  "wall": [6]
};

let words = [];

forEachObjIndexed((indexes, word) => forEach(idx => words[idx] = word, indexes), song);

console.log(words.join(' '));


Refactoring Event Handlers - How to go about this for growing WinForms application I have a class with approximately 140 menu buttons in a nested ribbon-style menu over a canvas type area within a WinForms application.

As a result of this, a large swathe of my codebehind file consists of 140 event handlers for click events.

Many of them however, are remarkable similar, for example:

    private void btnFinancialStatusReport(object sender, EventArgs e)
    {
        SwitchCurrentControl(new FinancialStatusReport());
    }

    private void btnMonthlySalesReport(object sender, EventArgs e)
    {
        SwitchCurrentControl(new MonthlySalesReport());
    }

    private void btnDailySalesReport(object sender, EventArgs e)
    {
        SwitchCurrentControl(new DailySalesReport());
    }

SwitchCurrentControl is a private method with the responsibility of essentially taking the newly constructed UserControl and docking it correctly into the active canvas. This is irrelevant to the question but I thought somebody might ask...

The Question

For this many buttons, (140+) a good 50% of them are practically the same, the only difference being in which Control they create.

Is this acceptable, or is there a way I can refactor this to reduce the amount of handlers?

Note

Before anybody suggests I switch to WPF btw, that is not an option...unless that same person wants to come and redo all 140 associated controls for me ;-)

You can create a Dictionary, where key is a MenuItem, a value us function/delegate that has to be called on click. All 140+ menu items subscribe to the same event hadler and in that event
handler write something like

myMenuItemsDictionary[((MenuItem)sender)]()

this is good, in case when you have no parameters to pass to a delegate, in case when you need to pass them, you can go farther:

define a class

public class MenuItemHandler
{
    delegate MethoddToInvokeOnClick()
    List<object> parametersToPassToDelegate = ..
}

and the instances of this class have in a Value of the dictionary


filesystem create a folder I got a mixture of two languages C and C ++

The code finds the folder my documents and creates additional folders in it when the program starts, I found no other way to implement this better.
I also did not find any ready-made example where all this has already been implemented so as not to make a bicycle, I like when the code looks compact, but it looks bad for me.

template <typename T1, typename T2>
void knownFolder(T1& setKnownFolderID, T2& getDisplayName)
{
    PWSTR pszFolderPath;

    for (;;) // !!!>
    {
        if (FAILED(SHGetKnownFolderPath(setKnownFolderID, 0, nullptr, &pszFolderPath)))
            continue;

        getDisplayName = pszFolderPath;
        CoTaskMemFree(pszFolderPath);

        break;  // <!!
    }
}

/

template <typename T2>
void folderLink(T2& setDisplayName, T2& getDisplayName)
{
    getDisplayName = setDisplayName + L"\\" + getDisplayName;

    for (;;) // !!!>
    {
        if (!filesystem::exists(getDisplayName))
            if (!filesystem::create_directory(getDisplayName))
                continue;

        break; // <!!
    }
}

/

wstring folder_Documents,                    // C:\ ... \Documents
        folder_Microsoft = L"Microsoft",     // C:\ ... \Documents\Microsoft

        folder_XP = L"XP",
        folder_XP_Alpha = L"Alpha",
        folder_XP_Beta = L"Beta",

        folder_Win7 = L"Win7",
        folder_Win7_Alpha = L"Alpha",
        folder_Win7_Beta = L"Beta";

/

void foldersystem()
{
    knownFolder(FOLDERID_Documents, folder_Documents); // Documents

    folderLink(folder_Documents,
        folder_Microsoft); // Microsoft

        folderLink(folder_Microsoft,
            folder_XP); // XP

            folderLink(folder_XP, folder_XP_Alpha);
            folderLink(folder_XP, folder_XP_Beta);

        folderLink(folder_Microsoft,
            folder_Win7); // Win7

            folderLink(folder_Win7, folder_Win7_Alpha);
            folderLink(folder_Win7, folder_Win7_Beta);

}

Use std::filesystem::path for paths

C++17 gives you a canonical type to represent filesystem paths, so use that. This avoids having to create templates of all your functions. It also allows you to combine paths together in a portable way:

void folderLink(std::filesystem::path &setDisplayName, std::filesystem::path &getDisplayName)
{
    getDisplayName = setDisplayName / getDisplayName;
    ...
}

Use std::filesytem::create_directories()

This function will create the desired directory plus all its parent directories if they do not yet exist. Also, it will not report an error if the desired directory already exists. So you could write:

void foldersystem()
{
    std::filesystem::path folder_Documents = ...; // get known folder as a path here

    std::filesystem::create_directories(folder_Documents / "Microsoft" / "XP" / "Alpha");
    std::filesystem::create_directories(folder_Documents / "Microsoft" / "XP" / "Beta");
    std::filesystem::create_directories(folder_Documents / "Microsoft" / "Win7" / "Alpha");
    std::filesystem::create_directories(folder_Documents / "Microsoft" / "Win7" / "Beta");
}

Don't infinitely retry on errors

You are using a very weird pattern with infinite for-loops:

for(;;) {
    if (something_fails())
        continue;

    break;
}

If the thing you are trying to do fails, you are retrying it indefinitely. But if it fails once, why do you expect it will work the next time? The right thing to do at this point is just to return an error.

Prefer returning results instead of using output parameters

If a function returns something, return that instead of passing a reference to an output parameter. Combined with the use of std::filesystem::path, knownFolder() then becomes:

std::filesystem::path knownFolder(KNOWNFOLDERID fodlerID)
{
    PWSTR pszFolderPath;

    if (FAILED(SHGetKnownFolderPath(folderID, 0, nullptr, &pszFolderPath)))
        throw std::runtime_error("Could not get known folder");

    std::filesystem::path result = pszFolderPath;
    CoTaskMemFree(pszFolderPath);
    return result;
}

Use an array of paths if you have many of them

If you have many paths to create, then store them in an array, and use a for-loop to create them all. Here is an example:

static const std::filesystem::path required_subdirectories[] = {
    "XP/Alpha",
    "XP/Beta",
    "Win7/Alpha",
    "Win7/Beta",
};

void foldersystem()
{
    std::filesystem::path folder_Documents = ...; // get known folder as a path here

    for (auto &subdirectory: subdirectories) {
            std::filesystem::create_directories(folder_Documents / "Microsoft" / subdirectory);
    }
}

Note that using the forward slash is perfectly fine here; the std::filesystem functions understand this as a directory separator.