Simple treasure hunter I have made a program that reads 3 numbers:

number of treasure chests
treasure hunter's position
number of moving instructions

It then reads a string (which symbolizes the treasure chests), being each character either T (chest has a treasure) or V (chest is empty). After that, it reads the moving instructions in the format %c %d, being the first character (direction) either E(left) or D(right) and then the number of steps.

When the treasure hunter moves, he loots the treasures which he stands in, and in the end the program tells how many he got.

The requirements are the following:

  1 ≤ number of treasure chests ≤ 100 000
  1 ≤ treasure hunter's position ≤ number of treasure chests
  1 ≤ number of moving instructions ≤ 100 000
  the number of steps is always higher than 0
  the treasure hunter never goes beyond the treasure chests

My working code which I wish to improve in every way possible is the following:

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

int main() {
  char e;
  int a, b, c, f, g = 0, i, j;
  scanf("%d %d %d", &a, &b, &c);
  char *d = malloc(a * sizeof(char));
  scanf("%s", d);
  for(i = 0; i < c; i++) {
    scanf(" %c %d", &e, &f);
    for(j = 0; j <= f; j++) {
      if(d[b - 1] != 'V') {
        d[b - 1] = 'X';
      }
      if(e == 'E') {
        b++;
      } else {
        b--;
      }
    }
  }
  for(i = 0; i < a; i++) {
    if(d[i] == 'X') {
      g++;
    }
  }
  printf("%d\n", g);
  return 0;
}

Example of input and output:

I:
5 3 3
TVTTT
D 1
E 2
D 3

O:
3

Variable names
Variables such as a, b, c, d are meaningless.  Use names with meaning such as position, numChests, chestValues, etc.  Sure, this is just a small program with 8 total variables, but you should get in the habit of producing readable code and not just correct code.
Bug
You don't allocate enough space for your string here:

char *d = malloc(a * sizeof(char));
scanf("%s", d);

Suppose a had value 5, as in the example.  The string "TVTTT" has 5 characters plus a terminating null character, for a total of 6 characters.  So you need to allocate an extra byte for the null character when you call malloc().
Also, it is unsafe to just call scanf() because if the input string happened to be wrong and contain more than 5 characters, you would overflow your buffer.  To be safe, you can use fgets(), which limits the number of characters it writes to your buffer:
char *chestValues = malloc(numChests+1);
fgets(chestValues, numChests+1, stdin);

Note also that sizeof(char) is always equal to 1, so you can omit that.
No need for second loop
You have a second loop to count the number of X in your chest array.  But you could have counted the number of chests found that were T in your first loop and then you wouldn't need the second loop at all.
Sample rewrite
Here is a rewrite of your program which fixes all of the above:
#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    int   numChests   = 0;
    int   position    = 0;
    int   numMoves    = 0;
    int   numFound    = 0;
    char *chestValues = NULL;

    scanf("%d %d %d\n", &numChests, &position, &numMoves);
    chestValues = malloc(numChests+1);
    fgets(chestValues, numChests+1, stdin);

    for (int i = 0; i < numMoves; i++) {
        char direction = 0;
        int  distance  = 0;
        scanf(" %c %d", &direction, &distance);
        for (int j = 0; j <= distance; j++) {
            if (chestValues[position - 1] == 'T') {
                chestValues[position - 1] = 'X';
                numFound++;
            }
            position += (direction == 'E' ? 1 : -1);
        }
    }
    printf("%d\n", numFound);
    return 0;
}


JARJAR challenge on SPOJ I tried this problem and I saw that my solutions was way too slow compared to others. I want to know how I can improve time complexity.

  A job has been assigned to Jar Jar Binks, it goes as follows: There
  are N spaceships parts, each with a weight of Wi kg. Given a weight W,
  he has to show how many parts can be used in order to make a ship with
  a weight of exactly W kg. He has to show all possible solutions, of
  course if possible. Everybody knows Jar Jar Binks particularly because
  of his clumsiness, so you have to help him. Write a program that
  solves his problem!

  Input

  There will be several cases, each beginning with two integers N, Q
  (1<=N<=60, 0<=Q<=10000). Next there will be N positive integers
  representing the weights of the N spaceship parts (1<=Wi<=1000). Q
  lines will follow, each one with only one integer W, the total weight
  of the spaceship. End of input will be denoted with N = 0 and Q = 0.
  This case should not be processed

  Output

  Print a line with K integers per query in ascending order. They must
  represent the amount of pieces that can be used to make a spaceship
  with weight W. If there is no way to make a spaceship with weight W,
  output a line with the string “That's impossible!” (quotes to clarify)

#include <bits/stdc++.h> // I don't think this would have any impact on run-time , right?

using namespace std;

#define pb push_back

#define FOR(i,a,b) for(int i=a;i<=b;++i)

int a[60];
int dp[61][61][60001]; // dp[number of weights considered][no. of weights selected from earlier][if this sum is possible]
int main()
{
    ios_base::sync_with_stdio(false);cin.tie(0);cout.tie(0); // for speed improvement

    int n,q;

    while(1)
    {
        cin>>n>>q;

        if(n==0&&q==0)break;

        int sum=0;
        FOR(i,1,n){cin>>a[i];sum+=a[i];}

        memset(dp,0,sizeof(dp));

        dp[0][0][0]=1;
        // I know that a 3d array is not needed but this would affect only memory, right?
        FOR(i,1,n)
        {
            dp[i][0][0]=1;
            FOR(j,1,i)
            {
                FOR(k,0,sum)
                {
                    if(k>=a[i])
                    dp[i][j][k]=dp[i-1][j-1][k-a[i]]; // typical dp. either a[i] is taken into account

                    dp[i][j][k]|=dp[i-1][j][k];       // or it is not
                }
            }
        }

        while(q--)    // handling queries
        {
            int x;
            cin>>x;
            vector<int> v;   // we will store answers here

            if(x>=1&&x<=sum)
            {
                FOR(j,1,n)
                if(dp[n][j][x])v.pb(j);

                if(v.empty())cout<<"That's impossible!";

                else for(auto it :v)cout<<it<<" ";

                cout<<"\n";
            }

            else cout<<"That's impossible!\n";
        }
    }
    return 0;
}

At first glance:

The code is a bit difficult to read.  The macros and variable names do not improve readability (using the same names than the problem would help a lot).
If I understand correctly (and I'm honestly not sure) you compute all possible solutions, then use this huge table on the input.
This would be a good idea if the result set was small and the number of 'Q' benefiting from it (much) bigger. (I'd say the order of 10E+7 'Q', give or take an order of magnitude)

A bit deeper:

The sum of weights as a quick elimination is a good idea.

But your algorithm involves this brutal O(m * n²) loop.  This does not inspire confidence in the execution speed.
Also, your main buffer takes 850MB of memory (assuming an int is 32 bits).  That's a LOT of cache misses : another downer.

The way I imagine the algorithm, only 2 arrays of 60 integers are needed (plus bonuses). That's 240 bytes or ~4 to ~8 lines of L0 cache : fast.  As for the work for a given total weight, it should be broadly O(n*log(n))

Reading the problem, a few obvious algorithm optimization steps are presenting themselves.

First, there is no precision that say that pieces weights are different (and if the test is interesting, there should be a fair amount of pieces with identical weights in the input, if only to make room for subtlety).

The weight pieces should be sorted in reverse order and the number of different weights counted.
Note that by starting with the bigger values on each iteration, the test of overflow you will do after each addition will trigger earlier.

Now that you know how many times a weight is used, you only start from it once and multiply the amount of solutions for that starting weight by U. (Repeat for all different weights)

There is also no precision that the total weight cannot be repeated.
I'm assuming that's what your O³ was meant for.
But instead of computing what you don't need, just store the results by total weight in a map (that you will reset for each new set of pieces weights).

And at last, since it's C++11.  You could take advantage of the amount of CPUs and dispatch the test. This part is actually tricky because knowing if you have to divide by (N+Qs) (and have a printer dispatch, the main thread, that centralizes the printing to keep order) or to divide for each Q of an N requires a bit of experiment and tuning.

An example to be clear, with [] being a thread.

[N0 Q01 Q02 Q03 ...] [N1 Q10 ...] per thread, plus [printer dispatch].

or

[N0 Q00] [N0 Q01] [N0 Q02] [N0 Q03] [N1 Q10], plus [printer dispatch].

These are the first ideas but the result should be quite fast.


Pretty print directory structure in a JSON recursively I am trying to pretty print the directory structure defined in a JSON. I do have a solution using recursion but I'm looking for feedback on how this solution can be improved or if there's any better approach.
Problem Statement
A deeply nested directory contains following information:

Name
Data (can be an array of sub directories or file content)

Desired output: Print the directory structure in below format:
root_dir
|__dir_a
|____file_00.txt
|____dir_b
|______dir_c
|________file_01.txt
|________file_02.txt
|________file_03.txt
|________file_04.txt
|______file_05.txt
|______file_06.txt
|____dir_d
|______file_07.txt
|______file_08.txt

Solution:
My idea is to traverse data recursively till a file is not found (because beyond it there is no further nesting) and keep pushing directories and files in an array which would then be pretty printed with help of template literals.
P.S. I don't want to reverse the arrays twice, how can I fix that?

const indentation = 2;
const flatDirStructure = [];

const dirStructure = [
  {
    name: 'root_dir',
    data: [
      {
        name: 'dir_a',
        data: [
          { name: 'file_00.txt', data: 'file_00 content' },
          {
            name: 'dir_b',
            data: [
              {
                name: 'dir_c',
                data: [
                  { name: 'file_01.txt', data: 'file_01 content' },
                  { name: 'file_02.txt', data: 'file_02 content' },
                  { name: 'file_03.txt', data: 'file_03 content' },
                  { name: 'file_04.txt', data: 'file_04 content' }
                ]
              },
              { name: 'file_05.txt', data: 'file_05 content' },
              { name: 'file_06.txt', data: 'file_06 content' }
            ]
          },
          {
            name: 'dir_d',
            data: [
              { name: 'file_07.txt', data: 'file_07 content' },
              { name: 'file_08.txt', data: 'file_08 content' }
            ]
          }
        ]
      }
    ]
  }
];

const flattenDirStructure = (data, depth) => {
  if (typeof data === 'string') {
    return true;
  } else {
    data.reverse().forEach(({ name, data }) => {
      const isFile = flattenDirStructure(data, depth + indentation);

      flatDirStructure.push({
        name,
        depth,
        isFile,
      });
    });

    return false;
  }
};

const prettyPrint = () => {
  const output = [];

  flatDirStructure.reverse().forEach(({ name, depth }) => {
    const indentation = new Array(depth).fill('_').join('');
    output.push(`${indentation ? '|' : ''}${indentation}${name}`);
  });

  console.log(output.join(`\n`));
}

flattenDirStructure(dirStructure, 0);
prettyPrint();

You could take a single loop without reversing the array and hand over the prefix for the next level indentation.

const
    print = (data, indentation, prefix = '') => data
        .map(({ name, data }) => Array.isArray(data)
            ? prefix + name + '\n' + print(data, indentation, (prefix || '|') + '_'.repeat(indentation))
            : prefix + name
        )
        .join('\n'),
    dirStructure = [{ name: 'root_dir', data: [{ name: 'dir_a', data: [{ name: 'file_00.txt', data: 'file_00 content' }, { name: 'dir_b', data: [{ name: 'dir_c', data: [{ name: 'file_01.txt', data: 'file_01 content' }, { name: 'file_02.txt', data: 'file_02 content' }, { name: 'file_03.txt', data: 'file_03 content' }, { name: 'file_04.txt', data: 'file_04 content' }] }, { name: 'file_05.txt', data: 'file_05 content' }, { name: 'file_06.txt', data: 'file_06 content' }] }, { name: 'dir_d', data: [{ name: 'file_07.txt', data: 'file_07 content' }, { name: 'file_08.txt', data: 'file_08 content' }] }] }] }],
    result = print(dirStructure, 2);

   console.log(result);
.as-console-wrapper { max-height: 100% !important; top: 0; }


Fetching settings for batches of wafers using loops I have this code below that needs to query multiple database tables.

import mysql.connector
import itertools, time
cnx = mysql.connector.connect(host="host", port=3306, user="user", passwd="pass", db="factory")

cursor = cnx.cursor()

cursor.execute("select distinct(batch) from batchdata where equipment = 'DUV001'")
batch = cursor.fetchall()
cursor.execute("select distinct(settingname) from runsettings where runnumber = 1 and equipment = 'DUV001'")
settings = cursor.fetchall()

start =  time.time()

settings = list(itertools.chain(*settings))
batches = list(itertools.chain(*batch))

lstoflst = []
for batch in batches:
    cursor.execute("select distinct(wafer) from batchdata where batch=%s and equipment = 'DUV001'",(batch,))
    wafers = cursor.fetchall()
    wafers = list(itertools.chain(*wafers))

for wafer in wafers:
    waflist = {}
    for i in settings:
        cursor.execute("select float8value from runsettings where settingname = %s and wafer = %s",(i,wafer))
        vals = cursor.fetchall()
        waflist[i] = vals[0][0]

    lstoflst.append(waflist)

print time.time() - start

I know there are a lot of things to improve like:

Error handling
Database connection
prepared statements on the query

But I want to focus first on the performance of this code. It works fine, but its taking at least 5 to 10 seconds minimum before it can finish running the loop.

In what way can I improve the performance of the code above?

Your program reminds me of a Java applet that I wrote during my first summer internship.  I spent a significant chunk of that summer writing code to query several database tables and match values from one table with values from another table.  The performance sucked, but it was tolerable.  Then the applet was deployed to an overseas office — and it took minutes to display one screen.

The lesson is, any time you execute queries in a loop, and the number of those queries depends on how much data you have, you're going to have a bad time.  The overhead of issuing a query and retrieving the results is significant.  What you should do instead is issue one query (or a small fixed number of queries) that gets all of the information you want.

I later learned that all of those troubles could have been avoided by writing a proper SQL query in the first place, namely a join query.

Here, it looks like you are interested in constructing lstoflst, which contains the runsettings used for run 1 of each wafer that was in a batch that went through equipment DUV001.  The program should look more or less like this:

cnx = …
cursor = cnx.cursor()
cursor.execute("""
    SELECT batchdata.wafer
         , runsettings.settingname
         , runsettings.float8value
        FROM runsettings
            INNER JOIN batchdata
                ON batchdata.equipment = runsettings.equipment
                AND batchdata.wafer = runsettings.wafer
        WHERE
            runsettings.equipment = 'DUV001' AND
            runsettings.runnumber = 1
        ORDER BY batchdata.wafer;
""")
wafer_groups = itertools.groupby(cursor.fetchall(), key=operator.itemgetter(0))
lstoflst = [dict(row[1:] for row in group[1]) for group in wafer_groups]


Handlers for checkboxes to select various map features I am developing a plugin for QGIS (a Geographic Information Systems) software which uses Python. I have several functions where each of them is connected to a QCheckbox. So when a checkbox is checked, the associated function then does the following:

It reads the text of a QComboBox which contains a list of layers.
Identifies the layer by its name.
Sets an expression to be used.
Selects all features which satisfies the expression.

And when the checkbox is unchecked:

Deselects all features which satisfies the expression.

Is there a way to merge these functions into one or make it more consise?

Here is the code:

def score_0():
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = NULL
    expr = QgsExpression( """ "Score" IS NULL """ )
    it = opp_sel_layer.getFeatures( QgsFeatureRequest( expr ) )
    ids = [i.id() for i in it]
    area = 0
    if self.dockwidget.score0_checkbox.isChecked():
        opp_sel_layer.select( ids )
    else:
        opp_sel_layer.deselect( ids)

def score_1():
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = 1
    expr = QgsExpression( """ "Score" = 1 OR "Category" = 'Possible' OR "Category" = 'Favourable'  """ )
    it = opp_sel_layer.getFeatures( QgsFeatureRequest( expr ) )
    ids = [i.id() for i in it]
    area = 0
    if self.dockwidget.score1_checkbox.isChecked():
        opp_sel_layer.select( ids )
    else:
        opp_sel_layer.deselect( ids)

def score_2():
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = 2
    expr = QgsExpression( """ "Score" = 2 OR "Category" = 'Intermediate' OR "Category" = 'Likely' """ )
    it = opp_sel_layer.getFeatures( QgsFeatureRequest( expr ) )
    ids = [i.id() for i in it]
    area = 0
    if self.dockwidget.score2_checkbox.isChecked():
        opp_sel_layer.select( ids )
    else:
        opp_sel_layer.deselect( ids)

def score_3():
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = 3
    expr = QgsExpression( """ "Score" = 3 OR "Category" = 'Sensitive' OR "Category" = 'Unlikely' """ )
    it = opp_sel_layer.getFeatures( QgsFeatureRequest( expr ) )
    ids = [i.id() for i in it]
    area = 0
    if self.dockwidget.score3_checkbox.isChecked():
        opp_sel_layer.select( ids )
    else:
        opp_sel_layer.deselect( ids)

def score_4():
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = 4
    expr = QgsExpression( """ "Score" = 4 OR "Category" = 'Showstopper' """ )
    it = opp_sel_layer.getFeatures( QgsFeatureRequest( expr ) )
    ids = [i.id() for i in it]
    area = 0
    if self.dockwidget.score4_checkbox.isChecked():
        opp_sel_layer.select( ids )
    else:
        opp_sel_layer.deselect( ids)

# Connect checkboxes with their associated functions
self.dockwidget.score0_checkbox.stateChanged.connect(score_0)
self.dockwidget.score1_checkbox.stateChanged.connect(score_1)
self.dockwidget.score2_checkbox.stateChanged.connect(score_2)
self.dockwidget.score3_checkbox.stateChanged.connect(score_3)
self.dockwidget.score4_checkbox.stateChanged.connect(score_4)

you only have a difference in two places: query and attribute of dockwidget you are going to check against. so you can pass them as parameters to your score function.

def score(self, query, check_attr):
    opp_selectedLayerIndex = self.dockwidget.areaOpporunityMap_combo.currentText()
    opp_sel_layer = QgsMapLayerRegistry.instance().mapLayersByName(str(opp_selectedLayerIndex))[0]
    # Select all features where "Score" = NULL
    expr = QgsExpression(query)
    it = opp_sel_layer.getFeatures(QgsFeatureRequest(expr))
    ids = [i.id() for i in it]

    checkbox = self.dockwidget.getattr(check_attr)

    if checkbox.isChecked():
        opp_sel_layer.select(ids)
    else:
        opp_sel_layer.deselect(ids)


Converting input from a FileReader to JSON and outputting it again Below is my code which I use to read data from a remote URL (which is GZipped), convert it to a Map, process the map (remove various unwanted fields, etc), the write it back to a file in JSON format.

Unfortunately, it's ugly. I'm doing multiple things in the same method, but can't think of a good way to break them apart, as the input files can have hundreds of thousands of lines, so they will cause me to run out of memory quickly if I try to read in the whole thing, then process it, then output it.

Can anyone offer any assistance/suggestions?

private void importTdatFile(String fileURL) {

    String filename = getFilename(fileURL) + ".gz";

    try {
        URL url = new URL(fileURL);

        // set up input
        GZIPInputStream gzis;
        if (new File(filename).isFile()) {
            InputStream is = ClassLoader.getSystemResourceAsStream(fileURL);
            gzis = new GZIPInputStream(is);
            System.out.println("Using tdat header from classes directory");
        } else {
            gzis = new GZIPInputStream(url.openStream());
        }
        BufferedReader reader = new BufferedReader(new InputStreamReader(gzis));

        // set up output
        BufferedWriter writer = new BufferedWriter(new FileWriter(catalog.getName() + ".json"));

        // create a template so I only have to create a map once
        Map<String, String> template = new LinkedHashMap<String, String>(catalog.getFieldData().size());
        for (String fieldName : catalog.getFieldData().keySet()) {
            template.put(fieldName, null);
        }

        // start processing
        while (reader.ready()) {
            String line = reader.readLine();
            if (line.matches("^(.*?\\|)*$")) {
                Map<String, String> result = new HashMap<String, String>();
                String[] fieldNames = catalog.getFieldData().keySet().toArray(new String[]{});
                String[] fieldValues = line.split("\\|");

                for (int i = 0; i < fieldValues.length; i++) {
                    FieldData fd = catalog.getFieldData().get(fieldNames[i]);
                    if (catalog.getFieldDataSet().contains(fd)) {
                        result.put(fieldNames[i], fieldValues[i]);
                    }
                }

                result = removeNulls(result);
                result = removeUnwantedFields(result, catalog);
                result = fixFieldPrefixes(result, catalog);
                result = fixFieldNames(result, catalog);

                writer.write(getJsonLine(result));

            }
        }

        writer.close();
        reader.close();
        gzis.close();
    } catch (MalformedURLException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

An idea:

private BufferedReader getReader(final String fileUrl) throws IOException {
    final String filename = getFilename(fileUrl) + ".gz";
    final URL url = new URL(fileUrl);
    final InputStream stream;
    if (new File(filename).isFile()) {
        stream = ClassLoader.getSystemResourceAsStream(fileUrl);
        System.out.println("Using tdat header from classes directory");
    } else {
        stream = url.openStream();
    }
    final GZIPInputStream gzipStream = new GZIPInputStream(stream);
    final InputStreamReader gzipStreamReader =
        new InputStreamReader(gzipStream, "UTF-8");
    final BufferedReader reader = new BufferedReader(gzipStreamReader);
    return reader;
}

You don't have to close the GZIPInputStream, Reader.close() does it.

I'd invert the condition inside the while loop:

// start processing
while (reader.ready()) {
    final String line = reader.readLine();
    if (!line.matches("^(.*?\\|)*$")) {
        continue;
    }
    Map<String, String> result = new HashMap<String, String>();
    ...
}

It makes the code flatten which is easier to read.

This code maybe unnecessary, since no one uses the template˛object:

// create a template so I only have to create a map once
final Map<String, String> template =
    new LinkedHashMap<String, String>(catalog.getFieldData().size());
for (final String fieldName : catalog.getFieldData().keySet()) {
    template.put(fieldName, null);
}

You should specify the charset when you call the constructor of the InputStreamReader.

final InputStreamReader gzipStreamReader =
    new InputStreamReader(gzipStream, "UTF-8");

Omitting it could lead to 'interesting' surprises, since it will use the default charset which varies from system to system.

Here is the code after a few more method extractions. Check the comments please and feel free to ask if something isn't clear.

public void importTdatFile(final String fileUrl) throws MyAppException {
    try {
        doImport(fileUrl);
    } catch (final IOException e) {
        // MalformedURLException is a subclass of IOException
        throw new MyAppException("Cannot import", e);
    }
}

private void doImport(final String fileUrl) throws IOException {
    BufferedReader reader = null;
    BufferedWriter writer = null;
    try {
        reader = getReader(fileUrl);
        writer = getWriter();

        while (reader.ready()) {
            final String line = reader.readLine();
            final Map<String, String> results = processLine(line);
            filterResults(results);
            final String jsonLine = getJsonLine(results);
            writer.write(jsonLine);
        }
    } finally {
        closeQuietly(reader);
        writer.close(); // do NOT ignore output errors
    }
}

private BufferedReader getReader(final String fileUrl) throws IOException {
    final InputStream stream = getStream(fileUrl);
    final BufferedReader reader = createGzipReader(stream);
    return reader;
}

private InputStream getStream(final String fileUrl)
    throws MalformedURLException, IOException {
    final InputStream stream;
    // I don't really like this 'if' here
    if (isGzipFile(fileUrl)) {
        // I'm not sure that the condition is correct for classpath loading
        // or not
        stream = ClassLoader.getSystemResourceAsStream(fileUrl);
        // I would put this println to somewhere else (after refactoring the 'if')
        System.out.println("Using tdat header from classes directory");
    } else {
        final URL url = new URL(fileUrl);
        stream = url.openStream();
    }
    return stream;
}

private BufferedReader createGzipReader(final InputStream stream)
        throws IOException, UnsupportedEncodingException {
    final GZIPInputStream gzipStream = new GZIPInputStream(stream);
    final InputStreamReader gzipStreamReader =
        new InputStreamReader(gzipStream, "UTF-8");
    final BufferedReader reader = new BufferedReader(gzipStreamReader);
    return reader;
}

private boolean isGzipFile(final String fileUrl) {
    final String filename = getFilename(fileUrl) + ".gz";
    return new File(filename).isFile();
}

private BufferedWriter getWriter() throws IOException {
    // TODO: FileWriter use the default character encoding (see javadoc),
    // maybe you should use a FileOutputStream with a specified encoding.
    final String outputFilename = getOutputFilename();
    final FileWriter fileWriter = new FileWriter(outputFilename);
    return new BufferedWriter(fileWriter);
}

private String getOutputFilename() {
    return catalog.getName() + ".json";
}

private Map<String, String> processLine(final String line) {
    final Map<String, String> result = new HashMap<String, String>();
    if (!isProcessableLine(line)) {
        return result;
    }
    // It's hard to refactor without the internals of catalog.
    final String[] fieldValues = line.split("\\|");

    for (int i = 0; i < fieldValues.length; i++) {
        // TODO: possible ArrayIndexOutOfBoundsException here?
        final String fieldName = catalog.getFieldName(i);
        final FieldData fieldData = catalog.getFieldData(fieldName);
        if (catalog.fieldDataSetContains(fieldData)) {
            final String fieldValue = fieldValues[i];
            result.put(fieldName, fieldValue);
        }
    }

    return result;
}

private void filterResults(final Map<String, String> results) {
    removeNulls(results);
    // TODO: catalog probably a field, so it should be visible inside these
    // methods without passing them as a parameter
    removeUnwantedFields(results, catalog);
    fixFieldPrefixes(results, catalog);
    fixFieldNames(results, catalog);
}

private boolean isProcessableLine(final String line) {
    // TODO: A precompiled regexp maybe faster but it would be premature
    // optimization, so don't do that without profiling
    return line.matches("^(.*?\\|)*$");
}

private void closeQuietly(final Closeable closeable) {
    if (closeable == null) {
        return;
    }
    try {
        closeable.close();
    } catch (final IOException e) {
        // TODO: log the exception with a logger instead of the
        // printStackTrace();
        e.printStackTrace();
    }
}

Anyway, your streaming approach is fine, you shouldn't read the whole file into the memory.


Function to get current timestamp in node I have this code to get the current timestamp so that I can save it as a part of the key in leveldb database.

export default function timestamp() {
  var [seconds, nanoseconds] = process.hrtime()
  return seconds * 1000000000 + nanoseconds
}

I am wondering are there any downsides to use this function. Or are there any better alternatives to get current timestamp in node.

Your code does not work - process.hrtime() does not return with a timestamp of any useful form unless you compare it to another process.hrtime() in the same running program (typically used for measuring performance).

From the documentation:

  These times are relative to an arbitrary time in the past, and not
  related to the time of day and therefore not subject to clock drift.
  The primary use is for measuring performance between intervals

Bottom line is that the arbitrary time in the past could be anything, and not the same "epoch" as new Date().

There is no way in native JavaScript or Node (without making OS system calls through libraries, etc.) that I know of to get a more granular resolution than the millisecond times from Date.


Graph Implementation in Java using Set and Map There is a Graph implementation. I'm using Set to store unique Edges and Map to store Vertices and their associated edges. Assume Vertex and Edge class has been defined somewhere else implementing hashcode() and equals() working fine.

public class Graph
{
    static class Vertex {}

    static class Edge {}

    private Map<Vertex, Set<Edge>> vertexSetMap;
    private Set<Edge> edgeSet;

    public Graph()
    {
        edgeSet = new LinkedHashSet<>();
        vertexSetMap = new LinkedHashMap<>();
    }

    public boolean addVertex(Vertex vertex)
    {
        if(!vertexSetMap.containsKey(vertex))
        {
            vertexSetMap.put(vertex, new LinkedHashSet <>());
            return true;
        }
        return false;
    }

    public boolean addEdge(Vertex v1, Vertex v2, Edge edge)
    {
        if(edgeSet.add(edge))
        {
            vertexSetMap.get(v1).add(edge);
            vertexSetMap.get(v2).add(edge);
            return true;
        }
        return false;
    }

    public Set<Edge> getEdges(Vertex vertex)
    {
        return vertexSetMap.get(vertex);
    }

    public Set<Vertex> getNeighborsOf(Vertex vertex)
    {
        Set<Vertex> neighbors = new LinkedHashSet <>();
        for(Edge edge : vertexSetMap.get(vertex))
        {
            Vertex v1 = edge.getVertex1();
            Vertex v2 = edge.getVertex2();
            if(v1.equals(vertex)) neighbors.add(v2);
            else neighbors.add(v1);
        }
        return neighbors;
    }

    public void removeVertex(Vertex vertex)
    {
        Set<Edge> deleteEdges = getEdges(vertex);
        vertexSetMap.remove(vertex);
        edgeSet.removeAll(deleteEdges);
        vertexSetMap.keySet().forEach(vertex1 ->
        vertexSetMap.get(vertex1).removeAll(deleteEdges));
    }

    public void removeEdge(Edge edge)
    {
        edgeSet.remove(edge);
        Vertex v1 = edge.getVertex1();
        Vertex v2 = edge.getVertex2();
        vertexSetMap.get(v1).remove(edge);
        vertexSetMap.get(v2).remove(edge);
    }

    public boolean containsVertex(Vertex vertex)
    {
        return vertexSetMap.containsKey(vertex);
    }

    public boolean containsEdge(Vertex v1, Vertex v2, Edge edge)
    {
        return (edgeSet.contains(edge) && vertexSetMap.get(v1).contains(edge)
            && vertexSetMap.get(v2).contains(edge));
    }

    public boolean containsEdge(Vertex v1, Vertex v2)
    {
        for(Edge e : vertexSetMap.get(v1))
            if(vertexSetMap.get(v2).contains(e)) return true;
        return false;
    }

    public int totalVertices()
    {
        return vertexSetMap.keySet().size();
    }

    public int totalEdges()
    {
        return edgeSet.size();
    }

    public Set<Vertex> getAllVertices()
    {
        return new LinkedHashSet <>(vertexSetMap.keySet());
    }

    public Set<Edge> getAllEdges()
    {
        return edgeSet;
    }

    public void clear()
    {
        edgeSet = null;
        edgeSet = new LinkedHashSet <>();
        vertexSetMap = null;
        vertexSetMap = new LinkedHashMap <>();
    }
}

As of now, you only use LinkedHashSet and LinkedHashMap, thus declaring edgeSet and vertexMap as HashSet and HashMap is pointless. You could deploy Inversion of Control in order to demand some Set and Map at construction.

Method getEdges(...) could possibly return null, so you may want to wrap the return value in an Optional. Using Optional, you can also unify the behaviour of Map#get(Object) (which may or may not throw a NPE if a null is passed as argument):

public Optional<Set<Edge>> getEdges(Vertex vertex)
{
    Optional<Set<Edge>> result = Optional.empty();
    try
    {
        result = Optional.ofNullable(vertex);
    }
    catch (NullPointerException e)
    {
        // No action needed, we return an empty Optional
    }
    return result;
}

Your remove...(...)-methods throw a NPE if the Vertex/Edge provided is not contained within the graph or is null. As with Map#get(Object), Set#remove(Object) may or may not throw a NPE. You can rewrite them to not throw:

public void removeVertex(Vertex vertex)
{
    Optional<Set<Edge>> deleteEdges = getEdges(vertex);
    if (deleteEdges.isPresent())
    {
        vertexSetMap.remove(vertex);
        edgeSet.removeAll(deleteEdges.get());
        vertexSetMap.keySet().forEach(vertex1 ->
                vertexSetMap.get(vertex1).removeAll(deleteEdges.get()));
    }
}

public void removeEdge(Edge edge)
{
    try
    {
        if (edgeSet.remove(edge))
        {
            Vertex v1 = edge.getVertex1();
            Vertex v2 = edge.getVertex2();
            vertexSetMap.get(v1).remove(edge);
            vertexSetMap.get(v2).remove(edge);
        }
    }
    catch (NullPoionterException e)
    {
        // No action needed, there is nothing to remove.
    }
}

When you add methods

public boolean containsVertex(Vertex vertex)
public boolean getOther(Vertex vertex)

To your Edge-class, you can rewrite getNeighborsOf(...) to use Streams

public Set<Vertex> getNeighborsOf(Vertex vertex) {
    return vertexSetMap.get(vertex).stream()
          .filter(edge -> edge.contains(vertex))
          .map(edge -> edge.getOther(vertex))
          .collect(Collectors.toSet());
}

What is missing is a Set<Vertex> getVertices() method (you have one for Edges, so I would expect one for Vertexs as well). This would probably require creating an additional field Set<Vertex> vertices in your Graph class.


2D Collision Detection I wrote this a while ago but wanted to see if I went about it the right way and if my brain is working correctly. I was thinking about these two projects the other day and I recently released the source. This game was originally for a Windows Form Game Jam so you have full context.

This code worked but I always felt there were inaccuracies in how it handled it. For example, there had been times a rock would clip through a ship.

private void CollisionDetection()
{
    foreach(PictureBox s in rocks)
    {
        //These tell the difference between, I'm not sure if this is all correct, its just what I came up with.
        int Xdifference = (s.Location.X + (s.Width / 2)) - (player.Location.X + (player.Width / 2));
        int Ydifference = (s.Location.Y + (s.Height/ 2)) - (player.Location.Y + (player.Height/ 2));

        //get the actual distance apart
        double dist = Math.Sqrt((Xdifference * Xdifference) + (Ydifference * Ydifference));

        //And finally, does the real comparison
        if(dist < ((s.Width / 2) + (player.Width / 2)) && damagable)
        {
            health -= 10;
            damagable = false;
            impactHit.Play();
        }
    }
}

Full file is found here and full source, here.

Collision detection

You can calculate the collision much easier with the Rectangle structure.

Use the Contains method if you are interested in single points that

  Determines if the specified point is contained within this Rectangle structure.

var pictureRect = new Rectangle(
    pictureBox.Location.X,
    pictureBox.Location.Y,
    pictureBox.Width,
    pictureBox.Height
);
var isCollision = pictureRect.Contains(player.Location.X, player.Location.Y);

If you want to know whether the rectangles have a collision then use the  IntersectsWith method that

  Determines if this rectangle intersects with rect.

var isCollision = pictureRect.IntersectsWith(player.Rectangle());

Create and an extension to easier get the rectangle from a PictureBox.

public static Rectangle Rectangle(this PictureBox pictureBox)
{
    return new Rectangle(
        pictureBox.Location.X,
        pictureBox.Location.Y,
        pictureBox.Width,
        pictureBox.Height
    );
}

Naming

foreach(PictureBox s in rocks)

You should use more meaningful names than just s. A variable like pictureBox or pb would be much better because it's derived from the collection. The s does not have any meaning. Although in this case a rock would be perfect.

Health & Damage

health -= 10;

This operation is not clear at first and maybe you have other places where you need to decrease health. Consider using constants and a helper method for this.

private static class Damage
{
    public const int WhenHitRock = 10;
}

void DecreaseHealth(int value)
{
     health -= value;
}

Everything together

The result of applying all suggestions and proper names would be a self-documenting code that no longer has to be commented.

When you write comments you only should explain why you are doing something because the code already shows what.

foreach (var rock in rocks)
{
    var isPlayerRockCollision = rock.Rectangle().IntesectsWith(player.Rectangle());
    if (isPlayerRockCollision)
    {
        DecreaseHealthBy(Damage.WhenHitRock);
        damagable = false;
        impactHit.Play();
        return;
    }
}

Notice that there is a return inside the if. You set the demagable to false so I think it's not necessary to test other rocks for a hit. You can stop at the first one.


Create annotation to validate JWT in Spring controller instead of filter I developed a Spring RESTful service that uses a JWT for authorization.
To the validity of this JWT, i used two different implementations.

Create a filter to intercept every request and validate request, if invalid then reject it.
Create a annotation to be used in controller methods, this approach is explained below.

The JWT contains user id which is used to identify authenticated user, but if i used a filter, i have to decode this JWT again in controller method, because of this problem i used second implementation.

The question is about annotation approach.

Is this a good implementation to JWT validation ?
How can i improve this code ?

Here's the code,

Annotation

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface Authorize {
    String[] value();
}

Implementation of the annotation

public class MethodAccessResolver implements HandlerMethodArgumentResolver {

    @Override
    public boolean supportsParameter(MethodParameter parameter) {
        return (parameter.getMethodAnnotation(Authorize.class) != null) && (parameter.getParameterName().equals("jwt"));
    }

    @Override
    public Object resolveArgument(
        MethodParameter parameter,
        ModelAndViewContainer mavContainer,
        NativeWebRequest webRequest,
        WebDataBinderFactory binderFactory
    ) throws Exception {
        String[] allowedUsers = parameter.getMethodAnnotation(Authorize.class).value();
        JwtProperties jwt = JwtUtil.validate(webRequest.getHeader("Authorization"));
        String currentUser = jwt.getScope().get(0);

        int no = 0;
        for (String allowedUser : allowedUsers) {
            if (!allowedUser.equals(currentUser)) {
                no++;
            }
        }

        if (no == allowedUsers.length) {
            throw new UnauthorizedAccessException("Un-authorized access", 0xd1561);
        } else {
            return jwt;
        }
    }
}

Controller method - where the annotation is used

@GetMapping(
        path = "/api/event/post",
        produces = MediaType.APPLICATION_JSON_VALUE
)
@Authorize(value = {UserSupport.USER, UserSupport.ADMIN, UserSupport.END_USER})
public ResponseEntity<?> getLiveLocations(
        HttpServletRequest request,
        JwtProperties jwt
) throws
        MongoException,
        ParseException,
        JwtException,
        UnsupportedEncodingException,
        ResourceNotFoundException,
        UnauthorizedAccessException {
    //do something
}

An Exception is thrown if token is invalid or the user role contains in token is not allowed to access particular URL.

If token is validated then user details will be injected to method parameter JwtProperties jwt.

I'd prefer the filter, just because then you don't have to worry about a missed annotation leaving your API wide open. I never found decoding JWTs to be expensive, and I suspect you're performing premature optimization.
value isn't especially descriptive. Perhaps @Authorize(users= would be better.
Your code can be simplified by returning early if a match is found, rather than counting failures and comparing them:

final JwtProperties jwt = JwtUtil.validate(webRequest.getHeader("Authorization"));
final String currentUser = jwt.getScope().get(0);

for (final String allowedUser : parameter.getMethodAnnotation(Authorize.class).value()) {
    if (allowedUser.equals(currentUser)) {
        return jwt;
    }
}

throw new UnauthorizedAccessException("Un-authorized access", 0xd1561);


Best way to get day-of-week occurance I need to determine a meeting date which convenes on the second Thursday of every month.

I've written the following code to do this:

//Determine next monthly meeting (occurs on 2nd thursday of each month)
define('MAX_POSSIBLE_SECOND_THURS', 14);
define('JANUARY', 1);
define('DECEMBER', 12);
define('THURSDAY', 4);

$current_month  =   intval(date('n'));
$next_month     =   $current_month == DECEMBER ? 1 : $current_month + 1;
$current_day    =   intval(date('j'));

$iThursdayCount =   0;
$month_to_use   =   $current_day <= MAX_POSSIBLE_SECOND_THURS ? $current_month : $next_month;
$year_to_use    =   $month_to_use <> JANUARY ? intval(date('Y')) : intval(date('Y')) + 1;
for ($iDay = 1; $iDay <= MAX_POSSIBLE_SECOND_THURS; $iDay++) {
        $date_to_check  =   strtotime($year_to_use.'-'.$month_to_use.'-'.$iDay);
        if(date('N', $date_to_check)== THURSDAY) {
        $iThursdayCount++;
        if ($iThursdayCount == 2)
           break;
    }
}

$next_meeting_date = $month_to_use.'/'.$iDay.'/'.$year_to_use;

echo $next_meeting_date;

Although the loop is small, it seems like a lot of work to determine the second Thursday.  Are there any improvements that can be made?  Or would you do something totally different?

Try the DateTime class.  With this you can use relative formats:

new DateTime('second thu of next month');
new DateTime('second thu of jul');

That should make the code a lot easier.


Google News scraper to fetch links with similar stories The following code takes either a URL or the title to an existing news article.

Searches Google News using the title.
Collects all links from search results.

import urllib2
from lxml import html
import requests

def get_page_tree(url=None):
    page = requests.get(url=url, verify=False)
    return html.fromstring(page.text)

def get_title(url=None):
    tree = get_page_tree(url=url)
    return tree.xpath('//title//text()')[0].strip().split(' -')[0]

def find_other_news_sources(url=None, title=None):
    # Google forwards the url using <google_domain>/url?q=    <actual_link>. This might change over time
    forwarding_identifier = '/url?q='
    if not title:
        title = get_title(url=url)
    google_news_search_url = 'http://www.google.com/search?q=' + urllib2.quote(title) + '&tbm=nws'
    google_news_search_tree = get_page_tree(url=google_news_search_url)
    other_news_sources_links = [a_link.replace(forwarding_identifier, '').split('&')[0] for a_link in
                            google_news_search_tree.xpath('//a//@href') if forwarding_identifier in a_link]
    return other_news_sources_links

Instead of constructing the google_news_search_url with two string concatenations, use string formatting.
The other_news_sources_links line is very dense. Please split it up.
In addition to the defaults for all your args being None, you're calling all of your functions with keyword arguments, which seems unnecessary.
But before you fix that, consider why you need these 2 line functions in the first place. They don't seem to do anything complicated enough to warrant having to jump around in the code.


Computing sums in some CSV files horizontally and in other CSV files vertically I'd like to eliminate the code duplication in the following two methods by moving the common code into a separate method callable by both. The comments indicate the blocks of code that have a differing implementation in each method.

def compute_totals_h(self, size, bad_codes):
    with open(self._in_file_path, self._FILE_READ_MODE) as f:
        # initialize reader
        reader = csv.reader(f)
        field_names = reader.next()
        for i, code in enumerate(field_names):
            code = code.strip(string.punctuation).upper()
            field_names[i] = code
        for code in field_names:
            if (len(code) <= size) and (code not in bad_codes):
                self._totals[code] = 0

        for row in reader:
            # get totals
            for i, val in enumerate(row):
                code = field_names[i]
                if (code in self._totals):
                    self._totals[code] += string_utils.to_int(val)

    self._write_totals()

def compute_totals_v(self, code_field, est_field):
    with open(self._in_file_path, self._FILE_READ_MODE) as f:
        # initialize reader
        reader = csv.DictReader(f)

        for row in reader:
            # get totals
            code = row[code_field].strip(string.punctuation).upper()
            est = string_utils.to_int(row[est_field])
            if code in self._totals:
                self._totals[code] += est
            else:
                self._totals[code] = est

    self._write_totals()

I'm thinking of a solution that has a common abstract method that can be called from compute_totals_h and compute_totals_v with each method passing functions to handle its implementation. I can't figure out how to do this while passing the arguments for each implementation correctly. It would look something like:

def compute_totals(self, initialize_reader, get_totals):
    with open(self._in_file_path, self._FILE_READ_MODE) as f:
        reader = initialize_reader(f)

        for row in reader:
            get_totals(row)

        self._write_totals()

I'll also appreciate suggestions on a better way of handling this type of code refactoring to eliminate this general class of code duplication problems.

Where we improve the existing code
First of, let's start by talking a bit about some issues in your code. We'll see if we can factorize some bits latter.
The first thing to note is that you iterate twice over your field_names to construct its final form and initialize self._totals. There is no need for it:
field_names = reader.next()
for i, code in enumerate(field_names):
    code = code.strip(string.punctuation).upper()
    field_names[i] = code
    if (len(code) <= size) and (code not in bad_codes):
        self._totals[code] = 0

Next, we see that you differentiate the behaviours of your sums whether self._totals[code] has been initalized or not. One way to simplify it is to turn it into a collections.defaultdict. To be more precise, you will have to define self._totals = defaultdict(int) instead of self._totals = dict(). This will allow you to write, in compute_totals_v:
for row in reader:
    # get totals
    code = row[code_field].strip(string.punctuation).upper()
    self._totals[code] += string_utils.to_int(row[est_field])

This will also allow you to write, in compute_totals_h:
for row in reader:
    # get totals
    for code, val in zip(field_names, row):
        if (len(code) <= size) and (code not in bad_codes):
            self._totals[code] += string_utils.to_int(val)

eliminating the need to initialize self._totals fields. Also, using zip here is more idiomatic. And since you are using Python 2 (I guess reader.next() would have been next(reader) in Python 3) you can use itertools.izip to avoid building a new list in memory.
We can see here that we're moving the test that initialized self._totals[code] into the “get totals” loop. But it is not efficient since we are re-doing it for each row of data. Time to improve that filtering by getting something closer to the if code in self._totals we had. The easiest way is to turn invalid codes into None and check if code is not None instead:
def compute_totals_h(self, size, bad_codes):
    with open(self._in_file_path, self._FILE_READ_MODE) as f:
        # initialize reader
        reader = csv.reader(f)
        field_names = [c if (len(c) <= size) and (c not in bad_code)
                       else None
                       for c in (
                           code.strip(string.punctuation).upper()
                           for code in reader.next())]

        for row in reader:
            # get totals
            for code, val in zip(field_names, row):
                if code is not None:
                    self._totals[code] += string_utils.to_int(val)

    self._write_totals()

We now have something closer to compute_totals_v and can start to work on factorizing.
Where we get to do what you ask for
The general layout you extracted for your compute_totals method is quite close to what compute_totals_v and compute_totals_h has become. You should note, however, that compute_totals_h needs to return both reader and field_names for its initialization. So we need to return a dummy value (let's say None) in the initialization function for compute_totals_v as well. And thus, we need to accept a second parameter (the dummy value or field_names) in get_totals for both methods.
We also need to define the helper functions as inner functions of the method so we can capture the parameters of the method once and for all without having to pass them around.
# Somewhere, where you define self._totals:
# self._totals = defaultdict(int)

def compute_totals_h(self, size, bad_codes):
    def _init_reader(file_handler):
        reader = csv.reader(file_handler)
        field_names = [c if (len(c) <= size) and (c not in bad_code)
                       else None
                       for c in (
                           code.strip(string.punctuation).upper()
                           for code in reader.next())]
        return reader, field_names

    def _get_totals(row, field_names):
        for code, val in zip(field_names, row):
            if code is not None:
                self._totals[code] += string_utils.to_int(val)

    self.compute_totals(_init_reader, _get_totals)

def compute_totals_v(self, code_field, est_field):
    def _get_totals(row, *args):
        code = row[code_field].strip(string.punctuation).upper()
        self._totals[code] += string_utils.to_int(row[est_field])

    self.compute_totals(lambda f: (csv.DictReader(f), None), _get_totals)

def compute_totals(self, initialize_reader, get_totals):
    with open(self._in_file_path, self._FILE_READ_MODE) as f:
        reader, field_names = initialize_reader(f)

        for row in reader:
            get_totals(row, field_names)

    self._write_totals()

I'm not convinced, however, of the improvement of this change. Readability seems worse to me. And calling the get_totals function for each row has its slight overhead.


Maintaining a single collection subdivided by a value's property I'm working on a networked game. In this game, the base entity is called an actor. An actor can either be replicated or local, with each instance having a "replicates" field that represents this.

When managing a scene, a collection of actor must be maintained. My problem is finding a suitable solution to this. Right now I store a collection of references to local actors, replicated actors and all actors. This means data is duplicated which results in code like this:

        for( int i = 0; i < actorsToRemove.Count; ++i )
        {
            if( actorsToRemove[i].Replicates )
            {
                replicatedActors.Remove( actorsToRemove[i].Id );
            }
            else
            {
                localActors.Remove( actorsToRemove[i] );
            }
            allActors.Remove( actorsToRemove[i] );
        }
        actorsToRemove.Clear();

Instead of just removing from a single collection.

I could have one single collection that holds all actors and use properties that return another collection. For example:

public List<Actor> ReplicatedActors
{
    get
    {
        // loop through all actors and return a new list that holds all replicating actors.
    }
}

But this seems like too much unnecessary copying.

What would be the best way to go about this?

What you suggested is absolutely fine. There isn't any unnecessary copying. Although, to make things simpler, I suggest using LINQ extension methods for filtering.

using System.Linq;

public IEnumerable<Actor> ReplicatedActors
{
    get { return allActors.Where(actor => actor.Replicates); }
}

public IEnumerable<Actor> LocalActors
{
    get { return allActors.Where(actor => !actor.Replicates); }
}

You'll still have to remove actors from allActors, these two are separate read-only lists of actors derived from the list of all actors.

Edit: I also suggest changing your actor removal loop to be a foreach loop for simplicity.

foreach (var actor in actorsToRemove)
{
    allActors.Remove(actor);
}
actorsToRemove.Clear();


Append an annual total to all Excel files I am new to all this and figured, after doing some tutorials, the best way to get some proper experience is to simply start a little project.

I have yearly folders with Excel files in it, those are roughly the same, only last year I made a change to the structure of them.
My aim was to cycle through them all and add one value of them to a list, and then compare those results.
So far, this all works out as I hoped.
I have not figured out yet however, how to define the part that reads the excel files into a callable object ((?)hope that's the correct term).

My main question is, how efficient, or inefficient is this script I wrote?
I assume, one could reduce the length of this code a lot.

import os
import openpyxl

my_list_2015 = []
my_list_2016 = []
my_list_2017 = []
year_show = 2015

def brutto_total(netto):
    brutto = netto + netto * 0.2
    return '%.2f' %  brutto

def show_result(year):
    global year_show
    print("-" * 14 + str(year_show) + "-" * 14)
    print("Total Sum Netto 2015:   " + str('%.2f' % year))
    print("Total Sum Brutto 2015:  " + str(brutto_total(year)))
    year_show = year_show + 1
    return year_show

for i in os.listdir(os.chdir('path_to_excel_files')):
    if i.endswith(".xlsx"):
        workbook = openpyxl.load_workbook(i, data_only=True)
        sheet = workbook['Blatt1']
        c=sheet['L46'].value
        my_list_2015.append(c)
        my_list_total_2015 = sum(my_list_2015)

for i in os.listdir(os.chdir('path_to_excel_files')):
    if i.endswith(".xlsx"):
        workbook = openpyxl.load_workbook(i, data_only=True)
        sheet = workbook['Blatt1']
        c=sheet['L46'].value
        my_list_2016.append(c)
        my_list_total_2016 = sum(my_list_2016)

for i in os.listdir(os.chdir('path_to_excel_files')):
    if i.endswith(".xlsx"):
        workbook = openpyxl.load_workbook(i, data_only=True)
        sheet = workbook['Zusammen']
        c=sheet['C11'].value
        my_list_2017.append(c)
        my_list_total_2017 = sum(my_list_2017)

show_result(my_list_total_2015)
show_result(my_list_total_2016)
show_result(my_list_total_2017)

You are onto a good start, since you already use some functions already; yes they do look like callable objects, but functions in general are referred as such to disambiguate from that more involved construct.

As you noted, you have a number of duplicate code that can be shortened into a single function that take different arguments.  Before doing that, let's take one of the for loops and find out what could be improved first.

The show_results function should take in a parameter, but we can fix this later

Calling os.chdir haphazardly just to list some files is a bit overkill.  I am guessing you want to allow the singular i variable be opened without having to use absolute paths, but doing so can be dangerous if this code is executed in a multi-threaded context (say you want to process multiple files at once), as a thread changing the working directory will change them for all, thus potentially breaking code running on other threads.

Fortunately, Python offers a much better way to glob these files from a path, using a module incidentally named glob.  Now the loop is simply

# insert these into the imports
from os.path import join
from glob import glob

for path in glob(join('path_to_excel_files'. '*.xlsx')):

The reason why we use os.path.join is for cross-platform compatibility.  Also try to avoid single letter names if there is a more expressive name available.

Lastly, the sum call in each loop could be done once after all the values have been gathered rather than every loop as the result would not matter at all.

To simplify code, look for parts that are common, and from that parts that take values that define the specific usage.  From the three for loops we can see that it's just simply sheet identifier within the workbook, the cell identifier from the sheet.  This is what it might look like (please note that I don't have the library installed, nor your Excel files, so I can't exactly test the following)

def process_workbooks_in_dir(excel_root, sheet_id, cell_id):
    values = []
    for path in glob(join(excel_root, '*.xlsx')):
        workbook = openpyxl.load_workbook(path, data_only=True)
        sheet = workbook[sheet_id]
        value = sheet[cell_id].value
        values.append(value)

    return values

Note that sum can be called later to sum up the list of values returned, this function now simply process the list of excel files within the provided base path, and the load_workbook function will be given the absolute path of each excel files for the processing to be done.  To use this, the following may be done.

my_list_2015 = process_workbooks_in_dir('path_to_excel_files', 'Blatt1', 'L46')
my_list_2016 = process_workbooks_in_dir('path_to_excel_files', 'Blatt1', 'L46')
my_list_2017 = process_workbooks_in_dir('path_to_excel_files', 'Zusammen', 'C11')

my_list_total_2015 = sum(my_list_2015)
my_list_total_2016 = sum(my_list_2016)
my_list_total_2017 = sum(my_list_2017)

Of course, verify that all this still working as intended.  You may note that the 2015 and 2016 lists are using the same parameters - this is now much more visible with the repeated code moved out of the way to show how these parameters can become accidentally (or intentionally) repeated in the remaining code.

While this is basically done, we can still do better with more advanced techniques.

As there is significant overhead with opening and closing files (especially for a complicated file format such as excel spreadsheets), it would be great if this is done only once for all the operations required.  Let's explore for way to streamline the flow better so that this can be done.  Note that the actual act of opening/closing files can be decoupled from the usage of the workbook, and that the workbook can be passed as a parameter.  Let's move that workbook usage logic to a separate function:

def read_value_from_workbook(workbook, sheet_id, cell_id):
    sheet = workbook[sheet_id]
    return sheet[cell_id].value

Now this is a much more simple function, but we still need the other bits such as opening the files, specify how to get the data through this function and to store it.

def process_workbooks_by_spec(excel_root, specification):
    results = {}
    # predefine the results with the year lists as required
    for year, sheet_id, cell_id in specification:
        results[year] = []

    # for the actual work
    for path in glob(join(excel_root, '*.xlsx')):
        workbook = openpyxl.load_workbook(path, data_only=True)
        for year, sheet_id, cell_id in specification:
            value = read_value_from_workbook(workbook, sheet_id, cell_id)
            results[year].append(value)

    return results

Note that Python can unpack tuples into multiple variables so we can have a list of tuples put into friendly variable names within the for loop to assign the year, sheet_id and cell_id for usage.  Also another thing that can simplify/eliminate the initial assignment loop is to use collections.defaultdict, which would reduce the code to simply this:

from collections import defaultdict

def process_workbooks_by_spec(excel_root, specification):
    results = defaultdict(list)
    for path in glob(join(excel_root, '*.xlsx')):
        workbook = openpyxl.load_workbook(path, data_only=True)
        for year, sheet_id, cell_id in specification:
            value = read_value_from_workbook(workbook, sheet_id, cell_id)
            results[year].append(value)

    return results

This is now much more efficient in terms of overall execution.  All the required operation needed to extract data can be passed in as a single list that might look something like this:

spec = [
    ('2015', 'Blatt1', 'L46'),
    ('2016', 'Blatt1', 'L46'),
    ('2017', 'Zusammen', 'C11'),
]

Note that the top level loop still provide the path to the files, which then open the workbook, then the inner loop unpacks the specification and pass the ids along with the workbook to the inner function, extract the required data and append that to results dictionary under the respective key (by year).

As this is relatively simple, the inner loop remain as is within that function, however for much more complicated business logic, it is wise to consider moving the inner loop outside to a separate function so that the logic is contained in a more concise way.

Now to use it:

results = process_workbooks_by_spec('path_to_excel_files', spec)

Accessing results['2015'] should now return the list of values extracted, and one can simply pass the results into a function that will process this.  Consider modifying show_results to this:

def show_results(results):
    for year, values in sorted(results.items()):
        result = sum(values)
        print("-" * 14 + str(year) + "-" * 14)
        print("Total Sum Netto %s: %.2f" % (year, result))
        print("Total Sum Brutto %s: %s" % (year, brutto_total(result)))

Now rather than relying on some global value outside of the function and some hard-coded value defined at the module, this only take in the results dictionary produced by the process_workbooks_by_spec function and iterate through every item and produce an output.  Of course, the sum is done at the time of calculation and if this is undesirable, it can be moved as part of the results as another dictionary, but this is up to you.

I would also not prematurely have some math processing function to return something that is not a number, so brutto_total should return the number as is, and format it later to keep the summary string consistent.  Putting it all together

from os.path import join
from glob import glob
from collections import defaultdict
import openpyxl

def brutto_total(netto):
    return netto + netto * 0.2

def read_value_from_workbook(workbook, sheet_id, cell_id):
    sheet = workbook[sheet_id]
    return sheet[cell_id].value

def process_workbooks_by_spec(excel_root, specification):
    results = defaultdict(list)
    for path in glob(join(excel_root, '*.xlsx')):
        workbook = openpyxl.load_workbook(path, data_only=True)
        for year, sheet_id, cell_id in specification:
            value = read_value_from_workbook(workbook, sheet_id, cell_id)
            results[year].append(value)

    return results

def show_results(results):
    for year, values in sorted(results.items()):
        result = sum(values)
        print("-" * 14 + year + "-" * 14)
        print("Total Sum Netto %s: %.2f" % (year, result))
        print("Total Sum Brutto %s: %.2f" % (year, brutto_total(result)))

spec = [
    ('2015', 'Blatt1', 'L46'),
    ('2016', 'Blatt1', 'L46'),
    ('2017', 'Zusammen', 'C11'),
]

results = process_workbooks_by_spec('path_to_excel_files', spec)
show_results(results)

This is how I might have done this (also reminder: I don't have your files and library installed, but this complete version at least pass the syntax check); hopefully this shows how you might be able to make use of the rich standard library functions that are available in Python to simplify certain tasks, and to also think about how one might break a program down and restructure it into a more clear, reusable form.


Java to Beanshell: validate an attribute for each record of a spreadsheet Suppose this Java code is converted to Beanshell and called by an application which reads a spreadsheet and validates the department of each employee at a university.

My primary concern is if there is a more efficient, standard, or otherwise better way to go about validating an employee's department rather than how I have done so below.  Null should be returned if the department is invalid.

import java.util.HashSet;
import java.util.Arrays;

public class Test{

    public static final String VALID_DEPARTMENTS[] = {
            "Computer Science", "Biology"
    };

    public static final HashSet<String> set = new HashSet<String>( Arrays.asList(VALID_DEPARTMENTS) );

    public String validate(String dep) {
        if( set.contains(dep) )
            return dep;
        return null;
    }

    public static void main(String[] args){
        Test t = new Test();
        System.out.println( t.validate("CS") );
    }
}

A few things come to mind:

In Java 9 one can use Set.of("X", "Y", ...).
Or could simplify to private static final Set<String> VALID_DEPARTMENTS = new HashSet<>(Arrays.asList("A", "B"));
Could modify validate(dep) to return set.contains(dep) ? dep : null;
It would be nice to return "" instead of null if at all possible.  This is so that a project won't end up with tons of if (x != null) all over the place.
Recommend renaming Test to something more descriptive.  Maybe DepartmentValidator might be a step in the right direction.
Probably refactor dep to department.  Eventually the meaning may become less obvious as time passes and the code base grows larger.
Note: the validation is currently quite strict (exact match), so I'm assuming the incoming data is pretty exact (no misspellings or different capitalization, etc.)


Reverse Polish Notation in F# In my question to learn F#, I've decided to get one step closer to creating a programming language, and implement this simple Reverse Polish Notation "interpreter" of sorts.

It does not allow for parentheses in input ( ), and only accepts expressions containing the following valid tokens:

0 1 2 3 4 5 6 7 8 9 + - * /

Here's a small list of sample inputs and outputs for reference:

2 2 + -> 4
10 10 + 5 * -> 100
2 2 * 4 + 2 * -> 16

I have a few concerns here:

Is this written in a proper "functional" way?
Is there any way to shorten evaluate_expr?
Are there any glaring issues that I missed?

open System
open System.Collections.Generic
open System.Text.RegularExpressions

/// <summary>
/// Evaluate an expression pair, like '2 2 +'.
/// <summary>
/// <param name="operand">The operand to use.</param>
let evaluate_expr_pair (a: string) (b: string) (operand: string) =
    match operand with
    | "+" -> (Int64.Parse(a) + Int64.Parse(b)).ToString()
    | "-" -> (Int64.Parse(a) - Int64.Parse(b)).ToString()
    | "*" -> (Int64.Parse(a) * Int64.Parse(b)).ToString()
    | "/" -> (Int64.Parse(a) / Int64.Parse(b)).ToString()
    | _ -> ""

/// <summary>
/// Evaluate a tokenized expression, such as '[| "2"; "2"; "+" |]',
/// and return a result, in this case, '2'.
/// </summary>
/// <param name="expr">The expression to evaluate.</param>
let evaluate_expr (expr: string[]) =
    let program_stack = new Stack<string>()

    for token in expr do
        program_stack.Push(token)

        match token with
        | "+" -> let operand = program_stack.Pop()
                 let b = program_stack.Pop()
                 let a = program_stack.Pop()
                 program_stack.Push(evaluate_expr_pair a b operand)

        | "-" -> let operand = program_stack.Pop()
                 let b = program_stack.Pop()
                 let a = program_stack.Pop()
                 program_stack.Push(evaluate_expr_pair a b operand)

        | "*" -> let operand = program_stack.Pop()
                 let b = program_stack.Pop()
                 let a = program_stack.Pop()
                 program_stack.Push(evaluate_expr_pair a b operand)

        | "/" -> let operand = program_stack.Pop()
                 let b = program_stack.Pop()
                 let a = program_stack.Pop()
                 program_stack.Push(evaluate_expr_pair a b operand)

        | _ -> ()

    program_stack.Pop()

/// <summary>
/// Tokenize an input expression, such as '2 2 + 5 *'.
/// </summary>
/// <param name="expr">The expression to tokenize.</param>
let tokenize_expr (expr: string) =
    let split_pattern = "\s*(\+|\-|\*|\/)\s*|\s+"
    let split_regex = new Regex(split_pattern)
    let result = split_regex.Split(expr)
    result.[0..result.Length - 2]

/// <summary>
/// Check all the tokens of an expression to make sure
/// that they are all legal.
/// </summary>
/// <param name="expr">The expression to check.</param>
let check_expr (expr: string) =
    let valid_token_pattern = "[\s0-9\+\-\*\/]"
    let valid_token_regex = new Regex(valid_token_pattern)

    for token in expr do
        if valid_token_regex.Match(token.ToString()).Success then
            ()
        else
            Console.WriteLine(String.Format("Invalid token \"{0}\".", token))

[<EntryPoint>]
let main argv =
    while true do
        Console.Write(": ")
        let input_expr = Console.ReadLine()

        input_expr |> check_expr
        let tokenized_expr = input_expr |> tokenize_expr
        let result = tokenized_expr |> evaluate_expr

        Console.Write("\n ")
        Console.Write(result)
        Console.Write("\n\n")
    0

evaluate_expr_pair

Your calculator performs integer division, which is a bit surprising.  To fix it to do floating-point or decimal arithmetic, though, you would have to do a search-and-replace of Int64.Parse, which has been written an unreasonable number of times in evaluate_expr_pair.

evaluate_expr_pair also does a lot of Parse and ToString round trips.  Not only is that inefficient, it could also cost you some loss of precision if you were doing floating-point arithmetic.

operand should really be called operator.  (The operands are a and b.)  Ignoring unrecognized operators is a bad idea, even if you have validated the input in check_expr.  If not doing exception handling, I'd rather leave out the default case and let it crash.  I'd also prefer to detect errors while attempting to evaluate the expression than pre-validate, because there are some errors that you can't reasonably detect without evaluating the expression (such as stack underflow).

evaluate_expr

Here, with your use of the program_stack, you are venturing outside the realm of functional programming, because the .Push() and .Pop() operations cause mutation.  A more FP approach would be to use a List as an immutable stack.

Instead of blindly pushing tokens, some of which are operators, onto a stack of strings, you should only put numeric data in the stack.

You would be better off treating the operators as functions that manipulate the entire stack directly, instead of as functions that take two operands and return one result.  Otherwise, you would have to implement an operation like swap as a special case, and distinguish between binary operations and unary operations such as negate and ex.

tokenize_expr

The simple strategy would be to use new Regex("\s+") as the delimiter pattern.  You're treating the operators as captured delimiters — presumably to make spaces optional in an expression like 1 2+ — and discarding the last element of the resulting array.  That works, as long as the expression ends with an operator.  If the expression is just 5, for example, you'll discard a number.

The regex could be written more succinctly using a [-+*/] character class.  I wouldn't bother naming split_pattern and split_regex as variables.

main

printf is preferred over System.Console.Write.

If you going to set up a |> pipeline, don't interrupt it by defining let tokenized_expr — that just gets in the way.

Summary

RPN calculators are much simpler when the operators work directly on the stack, rather than having a controller feed the operands to them.  In fact, every function except main has traces of the operator definitions.

Here's an implementation that I came up with:

open System
open System.Text.RegularExpressions

exception InputError of string

let tokenize (expr: string) =
    expr |> (new Regex("\s+|\s*([-+*/])\s*")).Split
         |> Array.toList
         |> List.filter(fun s -> s.Length > 0)

let perform (op: string) (stack: decimal list) =
    match (op, stack) with
    | ("+",     a :: b :: cs) -> (b + a) :: cs
    | ("-",     a :: b :: cs) -> (b - a) :: cs
    | ("*",     a :: b :: cs) -> (b * a) :: cs
    | ("/",     a :: b :: cs) -> (b / a) :: cs
    | ("swap",  a :: b :: cs) -> b :: a  :: cs
    | ("drop",  a :: cs)      -> cs
    | ("roll",  a :: cs)      -> cs @ [a]
    | (n,       cs)           ->
        try
            decimal n :: cs
        with
            | :? System.FormatException -> raise (InputError(n))

let evaluate (expr: string list) =
    let rec evaluate' (expr: string list) (stack: decimal list) =
        match expr with
        | []        -> stack
        | op :: exp -> evaluate' exp (perform op stack)
    evaluate' expr []

[<EntryPoint>]
let main argv =
    while true do
        printf ": "
        try
            match Console.ReadLine() |> tokenize |> evaluate with
            | num :: []  -> num   |> printfn "%g"       // Single answer
            | stack      -> stack |> printfn "%O"       // Junk left on stack
        with
        | InputError(str) -> printfn "Bad input: %s" str
    0


Divide and conquer algorithm for max binary gaps The codility.com free test is as follows:

  A binary gap within a positive integer N is any maximal sequence of
  consecutive zeros that is surrounded by ones at both ends in the
  binary representation of N.
       For example, number 9 has binary representation 1001 and contains a binary gap of length 2. The number 529 has binary representation
  1000010001 and contains two binary gaps: one of length 4 and one of
  length 3. The number 20 has binary representation 10100 and contains
  one binary gap of length 1. The number 15 has binary representation
  1111 and has no binary gaps.

  Write a function def solution(N) that, given a positive integer N,
  returns the length of its longest binary gap. The function should
  return 0 if N doesn't contain a binary gap.

  For example, given N = 1041 the function should return 5, because N
  has binary representation 10000010001 and so its longest binary gap is
  of length 5.

My implementation is as follows:

def merge_step(x, y, result):

    if x[0] == 1:
        if y[0] == 0 and len(result) == 0:
            result.append(1)

    if x[0] == 0 and y[0] == 0:
        if len(result) > 0:
            result[len(result) -1] = result[len(result) -1] + 1

def divide_and_conquer(array, result):
    # [1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1]

    if len(array) == 1:
        return merge_step([array], [array], result)

    mid = len(array) // 2
    x = array[:mid]
    y = array[mid:]

    divide_and_conquer(x, result)
    divide_and_conquer(y, result)
    merge_step(x, y, result)

    if len(result) == 0:
        return 0
    return max(result)

def solution(N):
    # write your code in Python 2.7
    array = [int(x) for x in "{0:b}".format(N)]
    return divide_and_conquer(array, [])

print(solution(1041))
print(solution(15))

The online interpreter says it is correct, but I wanted to check whether there's a better way to do this.

Also, I wanted to verify that my running time is n log(n).  The recursion is \$O(log n)\$ and the max is an \$O(n)\$ operation, correct?

Bug

What will be the value of merge_step([array], [array], result)? Nothing.
The solution will be incorrect for 0 and 1.

Algorithm

Converting an integer to binary string, and then converting that to a list of integers (zeros and ones), and then slicing dicing lists is very inefficient.

A much more simple and efficient approach is to count the sequences of unset bits:

longest = 0
current_len = 0

while num > 0:
    if num & 1:
        if current_len > longest:
            longest = current_len
        current_len = 0
    else:
        current_len += 1
    num >>= 1

return longest

The reason why the time complexity of this is \$O(\log n)\$ is that we're counting bits, which grows much slower than the value of \$n\$.

Doctests

As I pointed out in the previous review, I strongly recommend to use doctests to verify your implementation.


WebAPI Handling fire and forget exceptions This is a fire and forget task from within an existing WebAPI service. The code below simply sends a log event (might be that a client has connected to the service to perform some task) to another service and if it fails should not return an error to the client.

I simply want the exception logged and forgotten about, meanwhile the service will continue to carrying out the original task.

I've used ContinueWith and am wondering if this is the correct way to do it.

Task.Factory.StartNew(() =>
{
    var client = (HttpWebRequest) WebRequest.Create(_url);

    client.Method = "POST";
    client.ContentType = "application/json";

    var bytes = Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(log));

    var requestStream = client.GetRequestStream();

    requestStream.Write(bytes, 0, bytes.Length);

    requestStream.Close();

    var response = client.GetResponse();

    requestStream.Close();
    response.Close();

}).ContinueWith((ca) =>
{
    if (ca.Exception != null)
    {
        _logger.LogException(LogLevel.Error, ca.Exception.Message, ca.Exception);
    }

}, TaskContinuationOptions.OnlyOnFaulted);

ContinueWith() using TaskContinuationOptions.OnlyOnFaulted is right way to do it, especially when you do not have control over first task code (for example because it comes from an unknown method as Task).

However in your example you do not need a continuation task to handle errors, you may (and should) enclose your code inside a try/catch block and handle exceptions in-place:

Task.Factory.StartNew(() =>
{
    try
    {
        var client = (HttpWebRequest)WebRequest.Create(_url);

        // ...
    }
    catch (Exception e)
    {
         _logger.LogException(LogLevel.Error, e.Message, e);
    }
});

Note that this is just first step. First of all you're catching a generic Exception, if it's just for logging purposes it may be fine but you should consider if you have better options (error recovering? retrying?) handling more specific exceptions. For example you  may want to handle WebException and check if it's a timeout, there may be a network connection issue and you may want to wait and retry like in System.Net.WebException: The remote name could not be resolved.

Now you should correctly dispose resources you're using. If an error occurs, for example, after you acquired request stream but before you close it then connection will be immediately released. To simplify this you have a nice C# statement, using:

using (var requestStream = client.GetRequestStream())
{
    requestStream.Write(bytes, 0, bytes.Length);
    // ...
}

Also note that you should close request stream before asking for response stream, even if it's not an issue it's easier to do it right if you have using in-place.


Decrypting single byte XOR encryption The code along with the utils module is also at github. If you'd also have any advice on how the whole crate is structured I'd love to hear it. :)

use std::ascii::AsciiExt;
use std::str;

use utils::hex_to_bytes;
use s1c2::fixed_xor;

/// Scores ASCII test represented by byte array. The higher the score, the more common
/// English characters the text contains. Letter frequencies are taken from
/// https://en.wikipedia.org/wiki/Letter_frequency.
fn score_text(text: &[u8]) -> f32 {
    let frequencies = "xzqkjupnlgeyihrmfsdcbwaot";
    let text = str::from_utf8(text).unwrap();

    let score: usize = text.chars().map(|letter| {
        frequencies.find(letter.to_ascii_lowercase()).unwrap_or(0)
    }).sum();

    score as f32/text.len() as f32
}

/// Tries to decrypt text encrypted with a single character XOR
/// encryption.
pub fn decrypt_xor(ciphertext: &str) -> Option<(char, String)> {
    let cipherbytes = hex_to_bytes(ciphertext);
    let mut max = 0.0;
    let mut best_solution = None;

    // 32 to 127 should cover printable ASCII characters
    for character in 32..128 {
        let cipher = vec![character; cipherbytes.len()];
        let plaintext = fixed_xor(&cipherbytes, &cipher);
        let score = score_text(&plaintext);
        if score > max {
            max = score;
            best_solution = Some((character as char, String::from_utf8(plaintext).unwrap()));
        }
    }

    best_solution
}

#[test]
fn test_score_text() {
    assert_eq!(score_text(b"x"), 0.0);
    assert_eq!(score_text(b"Z"), 1.0);
    assert_eq!(score_text(b"$"), 0.0);
    assert_eq!(score_text(b"zZz"), 1.0);
}

#[test]
fn test_decrypt_xor() {
    assert_eq!(decrypt_xor("1b37373331363f78151b7f2b783431333d78397828372d363c78373e783a393b3736"),
               Some(('X', "Cooking MC's like a pound of bacon".to_string())));
}

Perform the UTF-8 conversion outside of score_text. With a name like _text, it should probably accept a &str anyway. Note that types are a kind of static assertion about the data; &str is a way of saying "UTF-8 encoded bytes". This also helps highlight that the UTF-8 check was being done twice.
Use expect instead of unwrap. When the code fails, you will be thankful.
The tracking of the maximum is annoying. Ideally, you'd be able to just say max_by_key on the iterator, but f32s don't implement Ord. There's the possibility of creating a wrapper type that ensures it is never NaN, but that feels like overkill here. You could use max_by, but that's not currently stable, and would still involve an unwrap. Instead, I'd note that all of the strings are the same length here, so dividing them by the length is unnecessary. Adjust the score function to return the previous numerator as a usize and then you can use max_by_key.
Making this change shows that you didn't just want "max", you were relying on finding the first one. Evidently, the max_by_key algorithm returns the last-most equal value. This causes your tests to fail and pick x instead of X as the key. You would have seen this problem previously if the key had been x. You should probably include spaces in the scoring function. "In English, the space is slightly more frequent than the top letter (e)".
Speaking of the scoring function, it appears to only have 25 characters? And the order doesn't make any sense to me. The highly-recognizable "etaoin" is missing. You may wish to double-check your work there.
I'd adjust your tests to assert on the properties of the scoring function that you care about. You don't care about a specific value, you care that:

a more-common letter has a higher score than a less-common one
ASCII case differences do not change the score
unknown letters don't crash

This makes me realize that unknown letters will have the same score as 'z', which seems incorrect.

use std::ascii::AsciiExt;
use std::str;

use utils::hex_to_bytes;
use s1c2::fixed_xor;

/// Scores ASCII test represented by byte array. The higher the score, the more common
/// English characters the text contains. Letter frequencies are taken from
/// https://en.wikipedia.org/wiki/Letter_frequency.
fn score_text(text: &str) -> usize {
    let frequencies = "zqxjkvbpygfwmucldrhsnioate ";
    text.chars().map(|letter| {
        frequencies.find(letter.to_ascii_lowercase()).map_or(0, |score| score + 1)
    }).sum()
}

/// Tries to decrypt text encrypted with a single character XOR
/// encryption.
pub fn decrypt_xor(ciphertext: &str) -> Option<(char, String)> {
    let cipherbytes = hex_to_bytes(ciphertext);

    // 32 to 127 should cover printable ASCII characters
    (32..128).map(|character| {
        let cipher = vec![character; cipherbytes.len()];
        let plaintext = fixed_xor(&cipherbytes, &cipher);
        (character as char, String::from_utf8(plaintext).expect("Wasn't UTF-8"))
    }).max_by_key(|a| score_text(&a.1))
}

#[test]
fn test_score_text() {
    assert!(score_text("e") > score_text("x"));
    assert_eq!(score_text("e"), score_text("E"));
    assert!(score_text("$") < score_text("a"));
}

#[test]
fn test_decrypt_xor() {
    assert_eq!(decrypt_xor("1b37373331363f78151b7f2b783431333d78397828372d363c78373e783a393b3736"),
               Some(('X', "Cooking MC's like a pound of bacon".to_string())));
}

The scoring idea is interesting, but I'm not in love with it. Something with two X shouldn't count the same as one Z. You could probably make better use of the relative frequencies.


Graceful shutdown of applications/servers I am trying to find out if there is any difference between two implementations that currently do the same thing - gracefully shutdown applications/servers, for example, when Ctrl+C is hit. Both work fine and are based on the documentation.

What a friend of mine says, is that Example 2 handles shutdown at the application level which shuts down all contexts throughout the application. However, Example 1 does it at the HTTP server level which doesn't necessarily shut down all contexts throughout the application. Since I am a beginner I cannot argue back and need your input on this, please.

Example 1

The signals are handled in the http.go file so the whole graceful shutdown has been handled in a single file.

cmd/app/main.go

package main

import (
    "context"
    "internal/http"
    "os"
    "os/signal"
    "syscall"
)

func main() {
    // Bootstrap config, logger, etc

    http.Start()
}

internal/http/server.go

package http

import (
    "context"
    "github.com/prometheus/common/log"
    "net/http"
    "os"
    "os/signal"
    "syscall"
    "time"
)

func Start() {
    log.Infof("starting HTTP server")

    srv := &http.Server{Addr: ":8080", Handler: nil}

    idle := make(chan struct{})

    go shutdown(srv, idle)

    if err := srv.ListenAndServe(); err != http.ErrServerClosed {
        log.Fatalf("failed to start/close HTTP server [%v]", err)
    }

    <-idle

    log.Info("gracefully shutdown HTTP server")
}

func shutdown(srv *http.Server, idle chan<- struct{}) {
    sig := make(chan os.Signal, 1)

    signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)

    <-sig

    ctx, cancel := context.WithTimeout(context.Background(), time.Duration(10)*time.Second)
    defer cancel()

    if err := srv.Shutdown(ctx); err != nil {
        log.Fatalf("shutdown HTTP server by interrupting idle connections [%v]", err)
    }

    close(idle)
}

Example 2

The signals are handled in the application's main.go file so the whole graceful shutdown has been split within two files. The only addition is this example uses the WithCancel context.

cmd/app/main.go

package main

import (
    "context"
    "internal/http"
    "os"
    "os/signal"
    "syscall"
)

func main() {
    // Bootstrap config, logger, etc

    backgroundCtx := context.Background()
    withCancelCtx, cancel := context.WithCancel(backgroundCtx)
    go shutdown(cancel)

    http.Start(withCancelCtx)
}

func shutdown(cancel context.CancelFunc) {
    sig := make(chan os.Signal, 1)

    signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)

    <-sig

    cancel()
}

internal/http/server.go

package http

import (
    "context"
    "github.com/prometheus/common/log"
    "net/http"
    "time"
)

func Start(ctx context.Context) {
    log.Infof("starting HTTP server")

    srv := &http.Server{Addr: ":8080", Handler: nil}

    idle := make(chan struct{})

    go shutdown(ctx, srv, idle)

    if err := srv.ListenAndServe(); err != http.ErrServerClosed {
        log.Fatalf("failed to start/close HTTP server [%v]", err)
    }

    <-idle

    log.Info("gracefully shutdown HTTP server")
}

func shutdown(ctx context.Context, srv *http.Server, idle chan struct{}) {
    <-ctx.Done()

    ctx, cancel := context.WithTimeout(context.Background(), time.Duration(10)*time.Second)
    defer cancel()

    if err := srv.Shutdown(ctx); err != nil {
        log.Fatalf("shutdown HTTP server by interrupting idle connections [%v]", err)
    }

    close(idle)
}

So to be helpful to others who might be interested in the final outcome after many R&D days and suggestions from other forum users so on., this is what I ended up with. I hope it helps and comments are welcome.

cmd/myself/main.go

package main

import (
    "context"
    "net/http"
    "log"
    "os"
    "os/signal"
    "syscall"

    "myself/internal/app"
)

func main() {
    srv := &http.Server{
        Addr:    ":8080",
        Handler: router.New(),
    }

    ctx, cancel := context.WithCancel(context.Background())

    signalChan := make(chan os.Signal, 1)

    go handleSignal(signalChan, cancel)

    if err := app.New(srv).Start(ctx); err != nil {
        log.Fatal(err)
    }

    log.Info("shutdown complete")
}

func handleSignal(signalChan chan os.Signal, cancel context.CancelFunc) {
    // os.Interrupt: Ctrl-C
    // syscall.SIGTERM: kill PID, docker stop, docker down
    signal.Notify(signalChan, os.Interrupt, syscall.SIGTERM)

    sig := <-signalChan

    log.Infof("shutdown started with %v signal", sig)

    cancel()
}

internal/app/mysql.go

package app

import (
    "context"
    "fmt"
    "log"
    "net/http"
    "time"
)

type App struct {
    server  *http.Server
}

func New(srv *http.Server) App {
    return App{
        server:  srv,
    }
}

func (a App) Start(ctx context.Context) error {
    shutdownChan := make(chan struct{})

    go handleShutdown(ctx, shutdownChan, a)

    if err := a.server.ListenAndServe(); err != http.ErrServerClosed {
        return fmt.Errorf("failed to start [%v]", err)
    }

    <-shutdownChan

    return nil
}

func handleShutdown(ctx context.Context, shutdownChan chan<- struct{}, a App) {
    <-ctx.Done()

    ctx, cancel := context.WithTimeout(context.Background(), 10 * time.Second)
    defer cancel()

    if err := a.server.Shutdown(ctx); err != nil {
        log.Infof("interrupted active connections [%v]", err)
    } else {
        log.Infof("served all active connections")
    }

    close(shutdownChan)
}

```


Reading from file and storing as Linked List This is the code I have:

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

typedef struct island{
    char *name;
    struct island *nextIsland;
} island;

void printIslands(island *i){
    printf("%s\n",i->name);
    if(i->nextIsland != NULL){
        printIslands(i->nextIsland);
    }
}

island* fileReader(FILE *file){
    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);
    if(fileRead != EOF){
        i->name = strdup(islandName);
        i->nextIsland = fileReader(file);
    }
    if(fileRead == EOF) {
        return NULL;
    }
    return i;
}

int main(){
    FILE *x = fopen("islands.txt","r");
    island *i = fileReader(x);
    printIslands(i);
    fclose(x);
}

Input file:

 islandone
 islandtwo
 islandthree
 islandfour
 islandfive

The output of the program will be exactly same with the input file, which is desired.

My question is, do you think fileReader is too complicated and fragile? How would you implement fileReader? Would you create a createIsland method and somehow use it in the fileReader?

Any comments welcome on the code.

I think you can simplify this. You test for EOF twice.

island* fileReader(FILE *file){
    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);
    if(fileRead != EOF){
        i->name = strdup(islandName);
        i->nextIsland = fileReader(file);
    }
    if(fileRead == EOF) {
        return NULL;
    }
    return i;
}

If you re-arrange this so you only test once:

island* fileReader(FILE *file){

    island *i = malloc(sizeof(island));
    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);

    if(fileRead == EOF) {
        return NULL;
    }

    i->name = strdup(islandName);
    i->nextIsland = fileReader(file);
    return i;
}

Now that we have re-arranged it. It becomes more obvious that you are leaking i. You create i in the first half but don't use it till the second half (and there is an early return in there).

island* fileReader(FILE *file){

    char islandName[20];
    int fileRead = fscanf(file,"%s",islandName);

    if(fileRead == EOF) {
        return NULL;
    }

    island *i = malloc(sizeof(island));
    i->name = strdup(islandName);
    i->nextIsland = fileReader(file);

    return i;
}

After this re-arrange. Two things stick out.

islandName[20] has a max of 19 characters. This may be OK but you may want to consider not makeing this a limitation. But you should also change your scan statement to make sure you don't exceed this limit.
You don't check the return value of malloc

Code

island* fileReader(FILE *file){

    char islandName[20] = {};

    // Note: %*c is there to read the first space character
    //       after the island name. If the island name is
    //       too long then the read is abandoned (because we
    //       limit it to 19 characters) and only the %19s will
    //       have input and thus the returned value will be 1
    //       (rather than 2 on a successful read)
    int fileRead = fscanf(file,"%19s%*c",islandName);

    // If file name exceeds 20 characters then read (but ignore)
    // any remaining characters in the island name
    if (fileRead == 1) {
        fscanf(file,"%*s");  // Reads upto the next space
    }                        // The * means ignore the input.

    if(fileRead == EOF) {
        return NULL;
    }

    island *i = malloc(sizeof(island));
    // Check to make sure we allocated space.
    if (i != NULL) {
        i->name = strdup(islandName);
        if (i->name != NULL) {
            i->nextIsland = fileReader(file);
        }
        else {
            // If the strdup() failed.
            // Then your object is invalid so release and
            // set i to NULL so it becomes the last one.
            free(i);
            i = NULL;
        }
    }

    return i;
}

The other thing you need to watch out for is the recursion here. This may work well for your test cases. But a huge list of millions of islands is going to eat up your memory. Look to using a loop instead of recursion.

island* fileReader(FILE *file){

    island* head = getNextIsland(file);
    island* last = head;

    while(last != NULL) {
        last->next = getNextIsland(file);
        last = last->next;
    }
    return head;
}

Don't assume that the list is not empty!

void printIslands(island *i){
    printf("%s\n",i->name);          // breaks id i is NULL on first call
    if(i->nextIsland != NULL){
        printIslands(i->nextIsland);
    }
}

When writing recursive routines it is usual to check if you have reached the end as the first thing in the recursive function. Then do the body of the action. You don't need to check the next one as soon as you recurse the next call will then check.

void printIslands(island *i){

    if (i == NULL) {
        return;
    }
    printf("%s\n",i->name);
    printIslands(i->nextIsland);
}

Again do not be lulled into false sense of security by recursion prefer to use a loop.

void printIslands(island *i){

    for(;i != NULL; i = i->next) {
        printf("%s\n",i->name);
    }
}