Simulating the radioactive decay of atoms using a random number generator I've found two ways of writing the same program (one that only uses local variables, but no methods other than the main one) and other that uses one instance variable that is used in two methods.

This program aims to simulate the radioactive decay of atoms and uses a random number generator.

Here are the two versions of the code:

/*
 * File: HalfLife.java
 *
 * This program simulates the decaying process of radioactive atoms that
 * have a half life of one year. The program will show the amount of atoms
 * remaining at the end of each year until all atoms has decayed
 */

import acm.program.*;
import acm.util.RandomGenerator;

public class HalfLife extends ConsoleProgram {

    /* Number of atoms */
    private static final int NATOMS = 10000;

    public void run() {
        println("There are " + NATOMS + " atoms initially.");
        int remainingAtoms = NATOMS;
        int year = 0;
        while (remainingAtoms > 0) {
            /* Simulate decaying process for each atom. Each atom has a 50% chance of decay. */
            for (int i = remainingAtoms; i > 0; i--) {
                if (rgen.nextBoolean()) {
                    remainingAtoms--;
                }
            }
            year++;
            println("There are " + remainingAtoms + " atoms at the end of year " + year);
        }
    }

    /* Create an instance variable for the random number generator */
    private RandomGenerator rgen = RandomGenerator.getInstance();
}

That one had only local variables, now for the other one (I removed comments in this one since it's redundant):

public class AtomicReduction extends ConsoleProgram {

    private static final int BEG_ATOMS = 10000;

    public void run () {
        int nYears = 1;
        println ("There are " + BEG_ATOMS + " atoms in the beginning");
        while (nAtoms > 0) {
            println ("There are " + (atomicRed (nAtoms)) + " atoms at the end of year " + nYears);
            nYears++;
        }
    }

    private int atomicRed (int initialAtoms) {
        for (int i = 0; i < initialAtoms; i++) {
            boolean isReduced = rGen.nextBoolean();
            if (isReduced) nAtoms--;
        }
        return nAtoms;
    }

     private int nAtoms = BEG_ATOMS;

     private RandomGenerator rGen = RandomGenerator.getInstance();
}

Which is "the better" version? And why?  I kind of prefer the first one because it keeps everything local, however the latter has more decomposition and can be potentially easier to understand (with proper comments).

Note: this makes use of ACM libraries so the main method is public void run.

Firstly, some minor nitpicks on your code:

   for (int i = remainingAtoms; i > 0; i--) {

Why are you counting down instead of up? I can see why after careful inspection, but a comment would be useful.

        println ("There are " + (atomicRed (nAtoms)) + " atoms at the end of year " + nYears);

You've violated Command-Query Seperation. atomicRed changes the state of the object, and so it really shouldn't return information about the object. It certainly shouldn't appear in the middle of a print a call.

        boolean isReduced = rGen.nextBoolean();
        if (isReduced) nAtoms--;

Why did you seperate isReduced onto its own line?

As for your actual question: people differ on this point. My general theory is that seperating out functions is good. But don't move previously local variables to the class in order to do that. Doing so only makes your class harder to follow.

In this case, I suggest something like

while(nAtoms > 0)
{
     nAtoms = remainingAtomsAfterYear(nAtoms);
     println("There are " + nAtoms + " atoms at the end of year " + nYears);
     nYears++;
}

This allows you to pull the logic regarding atom decay into a separate function, but avoids storing state on the class. I think this actually gets the best of both approaches.

Alternately, you could do:

while(!atoms.empty())
{
    atoms.simulateYear();
    println("There are " + atoms.count() + " atoms at the end of year " + atoms.years());
}

And move all the logic into another class. That's overkill here, but is helpful if the state gets more complicated.


Check if specific pattern exists in another array and save the index Let's say I have this pattern and a large array:

String [] pattern = new String[]{"i","am","a", "pattern"};
String [] large_array = new String[]{"not","i","am","a", "pattern","pattern","i","am","a", "pattern", "whatever", "x", "y", i","am","a", "pattern",................, i","am","a", "pattern" ....};

As you can see the pattern appears multiple times in the array. The first time at index 1, the second time at index 6, etc...  I want to find out at which positions the pattern begins and return it in a collection (eg list).

In this case the position array is
1,6,13, etc...

Here is my current method:

private ArrayList<Integer> getStartPositionOfPattern(String[] headerArray, String[] pattern) {

    ArrayList<Integer> allPositions = new ArrayList<>();
    int idxP = 0, idxH = 0;
    int startPos = 0;
    while (idxH < headerArray.length) {
        if (pattern.length == 1) {
            if (pattern[0].equals(headerArray[idxH])) {
                allPositions.add(idxH);
            }
        } else {
            if (headerArray[idxH].equals(pattern[idxP])) {
                idxP++;
                if (idxP == pattern.length) { //you reached end of pattern
                    idxP = 0; //start Pattern from begining
                    allPositions.add(startPos); //you can save startPosition because pattern is finished
                }
            } else {
                idxP = 0;
                startPos = idxH + 1;
                if (pattern[idxP].equals(headerArray[idxH])) { //if current arrray.idxH is not pattern.idxP but array.idxH is begining of pattern
                    startPos = idxH;
                    idxP++;
                }
            }
        }

        idxH++;

    }
    return allPositions;
}

Is there a way to make my function more readable and faster? I believe it works correctly, but because the function is complex, I worry I might have an undetected bug.

NOTE: headerArray is the large array.

Naming

NOTE: headerArray is the large array.

If you need to specify this to us, then your variable name isn't good. We should be able to understand your code with only your code because you wouldn't be beside us when we read it.

Don't worry about using full names for your variables. After all, is there a real difference between idxP and indexPosition apart from the fact that it's very clear I'll understand the latter one?

Commenting
Every time you want to write a comment, ask yourself if it is necessary. When you think it is, think again! :p Comments suck (I'm not really talking about XML comments on methods and classes and etc). When you write a comment in a method, it's because there's something weird enough that you'd feel the need to explain why you did something much more than what you did. In your cases, I don't think the comments are necessary.
Readability
In my opinion, there are two great and easy ways to make code more readable.

Extracting methods : Taking small chunks of your code that fills a single purpose and make a method out of it.

Reduce nesting : Remove as much forking as possible (if, while, for, etc.)

When dealing with algorithms, it's often hard to extract methods as many things are dependent.
However, you have lots of forking, so we can work with that.
if (pattern.length == 1) {
    if (pattern[0].equals(headerArray[idxH])) {
        allPositions.add(idxH);
    }
}

Everytime (exceptions might apply lol) you have two if stuck one to another, they can be merged using the AND (&&) operator.
if (pattern.length == 1 && pattern[0].equals(headerArray[idxH])) {
    allPositions.add(idxH);
}

Bam, nesting reduced.
You should try to find other ways to reduce nesting, but again, coding complex algorithms often imply spaghetti-ish code. And in this case, you might want to write a meaningful comment as to why your code is this way.
What's next?
If you really want to reach great performance for a "large" array, you might want to consider looking at String searching algorithms. It's not an exact fit to your problem, but there's something to be done.
Small nitpick : Large has a vague definition. If you want to make your code as correct as possible regarding your input, it can be useful to know an approximation of how much data your working with. For you, maybe an array with 100 elements is large, but for some data scientist at IBM, maybe it's 15000000.


Utopian tree life cycles Simple algorithm based HackerRank challenge.

  The Utopian tree goes through 2 cycles of growth every year. The first
  growth cycle of the tree occurs during the monsoon, when it doubles in
  height. The second growth cycle of the tree occurs during the summer,
  when its height increases by 1 meter.  Now, a new Utopian tree sapling
  is planted at the onset of the monsoon. Its height is 1 meter. Can you
  find the height of the tree after N growth cycles?

  Input Format
  The first line contains an integer, T, the number of
  test cases.  T lines follow. Each line contains an integer, N, that
  denotes the number of cycles for that test case.

  Constraints
  1 <= T <= 10  0 <= N <= 60

  Output Format
  For each test case, print the height of the Utopian
  tree after N cycles.

Test Cases can be found at HackerRank

static int[] LifeCycle(int[] input)
{
    int[] output = new int[input.Length];
    for (int i = 0; i < input.Length; i++)
    {
        int lifeCycleCount = Convert.ToInt32(input[i]);
        if (lifeCycleCount == 0)
        {
            output[i] = 1; //no growth
        }
        else
        {
            var treeHeight = 1;
            for (int j = 0; j < input[i]; j ++)
            {
                if (j % 2 == 0)
                {
                    treeHeight = treeHeight + treeHeight; //Monsoon Growth
                }
                else
                {
                    treeHeight++; //Summer Growth
                }
            }
            output[i] = treeHeight;
        }
    }
    return output;
}
static void Main(String[] args) {
    /* Enter your code here. Read input from STDIN. Print output to STDOUT. Your class should be named Solution */

    int howMany = Convert.ToInt32(Console.ReadLine());
    int[] input = new int[howMany];
    for (int i = 0; i < howMany; i++)
    {
        input[i] = Convert.ToInt32(Console.ReadLine());
    }

    int[] result = LifeCycle(input);
    for (int i = 0; i < result.Length; i++)
    {
        Console.WriteLine(result[i].ToString());
    }
}

Simple enough, but do I really need to use all the if statements and for loops?

I passed all the tests they put the code through (0.08s on each, total of 9 tests)

This is a generally good solution. Pretty good adherence to naming conventions, and generally quite easy to read. It's probably just fine as it is, but if you do want to make it better, there are some improvements that can be made:

Extracting the outermost method

First off, we have:

static int[] LifeCycle(int[] input)

We know right from the start that for this method:

The processing of each individual element is independent from the processing of each other individual element
Operations performed on individual elements (calculating the growth of a tree) are non-trivial
Operations performed on entire collections (input and output) are trivial

So this is screaming to have the individual tree processing extracted out:

static int LifeCycle(int input)

Then the contents of your original method can be nice and concise, using LINQ:

static int[] LifeCycle(int[] input)
{
    return input.Select(i => LifeCycle(i)).ToArray();
}

Aside: Naming

Actually, now that we're here, extracting this method has brought into sharper focus that maybe the naming isn't ideal. input is extremely vague- why not lifeCycleCount, which is what you actually use it as inside the method? Likewise LifeCycle would be better as a description of something the method is doing, like CalculateGrowth.

Initializing the height

One of the uglier pieces of logic is the special case for the number of cycles being 0. Is that necessary? What are we actually getting at with this:

if (lifeCycleCount == 0)
{
    output[i] = 1; //no growth
}

If there aren't any cycles, there's no growth, so we go to the magic number 1. But where's 1 coming from? It's the starting height. Is that encoded somewhere? Yes! Just two lines later:

var treeHeight = 1;

But the starting tree height is 1 no matter whether the cycle count is 0 or not, right? So we can move that outside the if statement. And once we do, immediately it becomes clear the if statement doesn't need to be there at all. If the cycle count is 0, then the for loop will never be executed and you'll stay with the default value of 1, which is what you want.

There's a bit more inconsistency here too. Like you needlessly call Convert.ToInt32 on an integer, and in once place you assign it to a variable with another name (lifeCycleCount), and in another place you keep it as input[i]. This may be left over detritus if you refactored from passing in input as an array of strings.

Extract one more method

At this point, you could happily stop and you'll have a nice piece of code. But there's one more thing that may be an improvement. This:

if (j % 2 == 0)
{
    treeHeight = treeHeight + treeHeight; //Monsoon Growth
}
else
{
    treeHeight++; //Summer Growth
}

Looks to me like its own method. There's quite a lot of information here that the brain has to do some reading/calculation to work out:

The code alternates between executing the two blocks
The first block is always executed first of the two
Both blocks use the tree height as their sole input
The only effect of either block is to change the tree height
From the comments, both blocks represent tree growth

A method lets us encode a lot of that in a way that we can read right away:

static int GrowTree(int initialHeight, bool season)

Then, we can just call it inside the loop like:

treeHeight = GrowTree(treeHeight, j%2==0);

Now this reads much more clearly. The tree height is changing because it is growing, how much it grows depends on its initial height and the season, and the season alternates every time the loop is run.

There are other options with the alternation too which, depending on taste, you may find more readable. Like:

season = j%2==0;
treeHeight = GrowTree(treeHeight, season);

or

treeHeight = GrowTree(treeHeight, season);
season = !season;

(In both cases, season would be a bool initialized outside the loop)

Putting it together

And just so you can see how nice it is with all those changes, here's roughly what it would look like:

static int[] CalculateGrowth(int[] cycleCounts)
{
    return cycleCounts.Select(c => CalculateGrowth(c)).ToArray();
}

static int CalculateGrowth(int cycleCount)
{
    var treeHeight = 1;
    var season = true;

    for(int i=0; i<cycleCount; i++)
    {
        treeHeight = GrowTree(treeHeight, season);
        season = !season;
    }

    return treeHeight;
}

static int GrowTree(int initialHeight, bool season)
{
    return season ? initialHeight * 2 : initialHeight + 1;
}


Is this a good way to handle Web API UriTemplates? I want to be able to do /contacts/Matt%20Phillips and /contacts/1 and have both of them return a contact. The way I did that was to try and parse an int from the captured parameter and call the Id method instead of the byName method like so:

    [WebGet(UriTemplate="{param}"]
    public Contact SingleParam(string param)
    {
        int x;
        if (int.TryParse(param, out x))
        {
            return GetContactById(x);
        }
        else
        {
            return GetContactByName(param);
        }
    }

    private Contact GetContactByName(string param)
    {
        var contact = from c in contacts
                      where c.Name.Equals(param)
                      select c;
        if (!contact.Any())
        {
            return null;
        }
        return contact.Single();
    }

    public Contact GetContactById(int id)
    {
        var contact = from c in contacts
                      where c.ContactId == id
                      select c;
        if (!contact.Any())
        {
            return null;
        }
        return contact.Single();
    }

But it seems clunky to me. Anyone have suggestions for a better way to do this?

There is an easier way. You can use uri templates to help you by making both handling methods operations.

[ServiceContract]
public class ContactsApi
{
    [WebGet(UriTemplate="{id}")]
    public string GetById(int id)
    {
        //return by id
    }

    [WebGet(UriTemplate="{first} {last}")]
    public string GetByName(string first, string last)
    {
        //return by name
    }
}

If you do a GET on /contacts/Matt%20Phillips the second method will get invoked.


Script for analyzing millions of lines I have files with over 100 million lines in each:

...
01-AUG-2012 02:29:44 important data
01-AUG-2012 02:29:44 important data
01-AUG-2012 02:36:02 important data
some unimportant data
blahblah (also unimportant data)
some unimportant data
01-AUG-2012 02:40:15 important data
some unimportant data
...

As you can see, there are important data (starting with date and time) and unimportant data.  Also in each second, there can be many lines of important data.

My goal is to count the number of "important data" in each second (or minute or hour...) and reformat date/time format. My script also lets me count data in each minute, hour etc using options.dlen:

options.dlen = 10 takes YYYY-MM-DDD
options.dlen = 13 takes YYYY-MM-DDD HH
options.dlen = 16 takes YYYY-MM-DDD HH:MM
options.dlen = 20 takes YYYY-MM-DDD HH:MM:SS

I have written the following script (this is the main part - I skip all the file openings, parameters etc).

DATA = {}

# search for DD-MMM-YYYY HH:MM:SS
# e.g. "01-JUL-2012 02:29:36 important data"
pattern = re.compile('^\d{2}-[A-Z]{3}-\d{4} \d{2}:\d{2}:\d{2} important data')

DATA = defaultdict(int)
i = 0
f = open(options.infilename, 'r')
for line in f:
    if re.match(pattern, line):
        if options.verbose:
            i += 1
            # print out every 1000 iterations
            if i % 1000 == 0:
                print str(i) + '\r',

        # converts data date/time format to YYYY-MM-DD HH:MM:SS format (but still keep it as datetime !)
        d = datetime.strptime( line [0:20], '%d-%b-%Y %H:%M:%S')
        # converts d, which is datetime to string again
        day_string = d.strftime('%Y-%m-%d %H:%M:%S')
        DATA [ str(day_string[0:int(options.dlen)]) ] += 1
f.close()
#L2 = sorted(DATA.iteritems(), key=operator.itemgetter(1), reverse=True)
#L2 = sorted(DATA.iteritems(), key=operator.itemgetter(1))
L2 = sorted(DATA.iteritems(), key=operator.itemgetter(0))

It takes about 3 hours to process over 100 million lines. Can you suggest performance improvements for this script?

Update: I have just used PyPy and the same task on the same server took 45 minutes.  I will try to add profile statistics.

1. Make a test case

The first thing to do is to establish the existence of the performance problem, so let's knock up some test data.

def make_test_file(filename, n, t, delta):
    """
    Write `n` lines of test data to `filename`, starting at `t` (a
    datetime object) and stepping by `delta` (a timedelta object) each
    line.
    """
    with open(filename, 'w') as f:
        for _ in xrange(n):
            f.write(t.strftime('%d-%b-%Y %H:%M:%S ').upper())
            f.write('important data\n')
            t += delta

>>> from datetime import datetime, timedelta
>>> make_test_file('data.txt', 10**5, datetime.now(), timedelta(seconds=1))

And then with the OP's code in the function aggregate1(filename, dlen):

>>> import timeit
>>> timeit.timeit(lambda:aggregate1('data.txt', 16), number = 1)
5.786283016204834

So on the real file (1000 times bigger) that would take an hour and a half on my machine (or longer, if the time complexity is worse than linear). So yes, there's a real performance problem.

2. Clean up the code

Let's try a bunch of obvious minor improvements and optimizations (mostly as suggested in other answers):

Convert dlen to an integer once (not every for every line).
Write day_string[:dlen] instead of str(day_string[0:dlen]).
Write pattern.match(line) instead of re.match(pattern, line).
There's no need for key = operator.itemgetter(0) because the sort will proceed on the first element of the pair in any case.
Rename DATA as count and day_string with s (it's really a date-time string, not a day string).
Use with to ensure that the file is closed in the event of an error.
Import the name strptime so it doesn't have to be looked up for every line.

Let's try that:

def aggregate2(filename, dlen):
    strptime = datetime.datetime.strptime
    dlen = int(dlen)
    pattern = re.compile(r'^\d{2}-[A-Z]{3}-\d{4} \d{2}:\d{2}:\d{2} important data')
    count = defaultdict(int)
    with open(filename, 'r') as f:
        for line in f:
            if pattern.match(line):
                d = strptime(line[:20], '%d-%b-%Y %H:%M:%S')
                s = d.strftime('%Y-%m-%d %H:%M:%S')
                count[s[:dlen]] += 1
    return sorted(count.iteritems())

>>> timeit.timeit(lambda:aggregate2('data.txt', 10), number = 1)
5.200263977050781

A small improvement, 10% or so, but clean code makes the next step easier.

3. Profile

>>> import cProfile
>>> cProfile.run("aggregate2('data.txt', 10)")
         2700009 function calls in 6.262 seconds

   Ordered by: standard name
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    6.262    6.262 <string>:1(<module>)
   100000    0.088    0.000    1.020    0.000 _strptime.py:27(_getlang)
   100000    2.098    0.000    4.033    0.000 _strptime.py:295(_strptime)
   100000    0.393    0.000    0.642    0.000 locale.py:339(normalize)
   100000    0.105    0.000    0.747    0.000 locale.py:407(_parse_localename)
   100000    0.119    0.000    0.933    0.000 locale.py:506(getlocale)
   100000    0.067    0.000    0.067    0.000 {_locale.setlocale}
   100000    0.515    0.000    4.548    0.000 {built-in method strptime}
   100000    0.079    0.000    0.079    0.000 {isinstance}
   200000    0.035    0.000    0.035    0.000 {len}
   100000    0.043    0.000    0.043    0.000 {method 'end' of '_sre.SRE_Match' objects}
   300001    0.076    0.000    0.076    0.000 {method 'get' of 'dict' objects}
   100000    0.276    0.000    0.276    0.000 {method 'groupdict' of '_sre.SRE_Match' objects}
   100000    0.090    0.000    0.090    0.000 {method 'index' of 'list' objects}
   100000    0.025    0.000    0.025    0.000 {method 'iterkeys' of 'dict' objects}
   100000    0.046    0.000    0.046    0.000 {method 'lower' of 'str' objects}
   200000    0.553    0.000    0.553    0.000 {method 'match' of '_sre.SRE_Pattern' objects}
   100000    1.144    0.000    1.144    0.000 {method 'strftime' of 'datetime.date' objects}

I've cut some of the output for clarity. It should be clear that the culprits are strptime (73% of runtime), strftime (18%), and match (9%). Everything else is either called by one of those, or negligible.

4. Pluck the low-hanging fruit

We can avoid calling both strptime and strftime if we recognize that the only things we are achieving by calling these two functions are (a) to translate the months from names (AUG) to numbers (08), and (b) to reorder the components into ISO standard order. So let's do that ourselves:

def aggregate3(filename, dlen):
    dlen = int(dlen)
    months = dict(JAN = '01', FEB = '02', MAR = '03', APR = '04',
                  MAY = '05', JUN = '06', JUL = '07', AUG = '08',
                  SEP = '09', OCT = '10', NOV = '11', DEC = '12')
    pattern = re.compile(r'^(\d{2})-([A-Z]{3})-(\d{4}) (\d{2}:\d{2}:\d{2}) '
                         'important data')
    count = defaultdict(int)
    with open(filename, 'r') as f:
        for line in f:
            m = pattern.match(line)
            if m:
                s = '{3}-{0}-{1} {4}'.format(months[m.group(2)], *m.groups())
                count[s[:dlen]] += 1
    return sorted(count.iteritems())

>>> timeit.timeit(lambda:aggregate3('data.txt', 10), number = 1)
0.5073871612548828

There you go: a 90% speedup! That should get you down from three hours to 20 minutes or so. There are a few more things one might try (for example, doing the aggregations for all the different values of dlen in a single pass). But I think this is enough to be going on with.


PDO Login/Register system review I wrote my very first login/register PDO system today.  I know there is still a lot of flaws, but I was wondering what tips and advice you have to help me improve this. I know that PDO is much more secure than MySQl, so would you say my code is secure?  If so, to what extent, since I'm using PDO?  Any tips and advice would be much appreciated!

login.php

Login

<form method="POST">
    <input type="text" name="username">

    <input type="password" name="password">

    <input type="submit">
</form>

<?php
session_start();
    if(isset($_POST['username'], $_POST['password'])){
        require 'core/connect.php';

        $query = dbConnect()->prepare("SELECT username, password FROM users WHERE username=:username AND password=:password");
        $query->bindParam(':username', $_POST['username']);
        $query->bindParam(':password', $_POST['password']);
        $query->execute();

        if($row = $query->fetch()){
            $_SESSION['username'] = $row['username'];
            header("Location: index.php");
        }
    }
?>

register.php

Register

<form method="POST">
    <input type="text" name="username">

    <input type="password" name="password">

    <input type="submit">
</form>

<?php
session_start();

    if(isset($_POST['username'], $_POST['password'])){
        require 'core/connect.php';

        $query = dbConnect()->prepare("INSERT INTO users (username, password) VALUES (:username, :password)");
        $query->bindParam(':username', $_POST['username']);
        $query->bindParam(':password', $_POST['password']);

        if($query->execute()){
            header("Location: index.php");
        } else{
            echo 'ERROR';
        }
    }
?>

index.php

<?php
session_start();

if(isset($_SESSION['username'])){
    echo 'Welcome!', 'Logout';
} else {
    echo 'Login

    Register';
}
?>

core/connect.php

<?php
    function dbConnect(){
        try{
            $username = 'root';
            $password = '';
            $conn = new pdo("mysql:host=localhost;dbname=test;", $username, $password);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            return $conn;

        }   catch(PDOException $e){
            echo 'ERROR', $e->getMessage();
        }
    }
?>

Here are several tips:

Use OOP - this way you could start the session only once
Hash your passwords - this way no one can steal it from the db, or at least the chance is lower
Use MVC - separate your HTML from the PHP code
Move your db connect credentials to the .ini file - this way it could be easily changed and .ini files can be cached by the server

Feel free to check my code for user authorization here in my pet project.


Cardshifter website meta-tags for SEO I have written the following meta-tags for SEO, please let me know if you think this could use any improvement. I have used this article to help me put it together. I have left out the page content on purpose, if you wish to see it I posted it here on CR and on Github (note the following updates have not been committed yet, they are only in my local directory).

<!DOCTYPE html>
<html class="" lang="en" itemscope itemtype="http://schema.org/Organization"> <!-- Schema.org is for Google+ meta-data - See other meta-tags within head section) -->
  <head>
  <meta http-equiv="content-type" content="text/html; charset=UTF-8" />
    <meta charset="utf-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <link rel="icon" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.0/favicon.ico" />

    <!-- META-TAGS -->

      <!-- Standard meta-tags -->
      <title>Cardshifter TCG</title>
      <meta name="description" content="Try Cardshifter trading card game (TCG), an open source multiplayer game!" />
      <meta name="author" content="https://github.com/Cardshifter" />
      <link rel="author" href="https://github.com/Cardshifter" />
      <link rel="publisher" href="https://github.com/Cardshifter" />

      <!-- Google+ Schema.org tags -->
      <meta itemprop="name" content="Cardshifter Trading Care Game">
      <meta itemprop="description" content="Try Cardshifter trading card game (TCG), an open source multiplayer game!">
      <meta itemprop="image" content="/images/cardshifter-logo.png">

      <!-- Facebook OpenGraph tags -->
      <meta property="og:title" content="Cardshifter Trading Care Game" />
      <meta property="og:type" content="website" />
      <meta property="og:image" content="/images/cardshifter-logo.png" />
      <meta property="og:url" content="http://www.CardShifter.com/" />
      <meta property="og:description" content="Try Cardshifter trading card game (TCG), an open source multiplayer game!" />
      <meta property="fb:admins" content="USER_ID" />  <!-- NEED TO MAKE USER ID FOR CARDSHIFTER -->

      <!-- Twitter tags -->
      <meta property="twitter:card" content="summary" />
      <meta property="twitter:url" content="https://twitter.com/Cardshifter" />
      <meta property="twitter:title" content="Cardshifter Trading Care Game" />
      <meta property="twitter:description" content="Try Cardshifter trading card game (TCG), an open source multiplayer game!" />
      <meta property="twitter:image" content ="/images/cardshifter-logo.png" />

    <!-- Bootstrap core CSS -->
    <link href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.0/css/bootstrap.min.css" rel="stylesheet">

    <!-- Custom styles for this template -->
    <link href="cardshifter.css" rel="stylesheet">

    <!-- HTML5 shim and Respond.js for IE8 support of HTML5 elements and media queries -->
    <!--[if lt IE 9]>
      <script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
      <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
    <![endif]-->
  </head>

  <body>
    <!-- page content removed -->
  </body>
</html>

1. Charset

<meta http-equiv="content-type" content="text/html; charset=UTF-8" />

You declare a HTML5 doctype and two charsets. You can remove the first of the two rules and leave this:

  <meta charset="utf-8">

2. IE rendering engine

<meta http-equiv="X-UA-Compatible" content="IE=edge" />

The browser should choose the fitting rendering engine on its own. Setting IE=edge should do nothing in most cases unless the default rendering engine was changed by the user.

That said, it doesn’t do harm.

3. Comments

<!-- META-TAGS -->

Don’t state the obvious. We can see every <meta> thing is a meta tag. Also there are other things under this comment as well. title and link are not meta tags.

I’d also leave out comments like the ones for twitter tags. Separating them by new lines creates enough distinctiveness.

4. HTML Shiv/Shim

<!--[if lt IE 9]>
  <script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
  <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
<![endif]-->

Don’t omit the type attribute for browsers that don’t understand HTML5, because it is not valid HTML 4/XHTML. You include the shiv to ensure IE8 support for HTML5 elements, so you should definitely have it there.

<!--[if lt IE 9]>
  <script type="text/javascript" src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
  <script type="text/javascript" src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
<![endif]-->

4. SEO

Having a good structured document is the one thing which helps. You have description that search engines use, you have a page title, you probably will have fitting headings in your actual content.

However that’s just the foundation. With just that, search engines won’t give you a good ranking. The important part is the content. Make sure you have a good document outline.


Looping to update article codes on one server based on queries on another server I've read somewhere on Stack Overflow that doing queries in a loop is very inefficient. It will hammer your SQL server and make your script very slow.

Sample code:

// Connect to SQL Server 1
$query = "SELECT * FROM
articles
";

$resource = mysql_query($query);
$articles = array();
while $record = mysql_fetch_assoc($resource) {
    $articles[] = $record;
}

// The part that bothers me:
// (Note: this is executed on a different sql server)
foreach ($articles as $article) {
    // Connect to SQL server 2
    $sQuery = "SELECT artcode FROM articles WHERE id='".$article['id']."'";
    $sResource = mysql_query($query);
    if (mysql_num_rows($sResource) == 1) {
        $data = mysql_fetch_assoc($sResource);
        // Connect to SQL Server 1 again ...
        // This will be executed on the first sql server again
        $uQuery = "UPDATE articles SET artcode='".$data['artcode']."' WHERE id='".$article['id']."' LIMIT 1";
        $uResource = mysql_query($query);
    }
}

How would I go and make this code more efficient. (In best case scenario avoiding doing the query in a loop.)

if you can access the second database from the same connection

use this query

UPDATE articles a1 inner join YOURDB.articles a2 on a1.id=a2.id
set a2.artcode = a1.artcode

but if your database is in another server i'm afraid you can't make it in one query


Hash table using linear probing This code is meant to implement a hash table class which uses linear probing.

I'm doing this to improve my style and to improve my knowledge of fundamental algorithms/data structures for an upcoming coding interview.

class Hash:
    def __init__(self):
        self.size = 11
        self.vals = [None] * self.size
        self.keys = [None] * self.size
        self.step = 1

    def put(self, key, val):
        ind = self.hash(key)

        if self.keys[ind] == None:
            self.keys[ind] = key
            self.vals[ind] = val

        elif self.keys[ind] == key:
            self.vals[ind] = val

        else:
            ind = self.rehash(ind)

            while (self.keys[ind] != None and
                   self.keys[ind] != key):
                ind = self.rehash(ind)

            if self.keys[ind] == None:
                self.keys[ind] = key
                self.vals[ind] = val

            elif self.keys[ind] == key:
                self.vals[ind] = val

    def get(self, key):
        ind = self.hash(key)
        start_ind = ind
        val = None
        done = False

        while not done:
            if self.keys[ind] == key:
                val = self.vals[ind]
                done = True

            else:
                ind = self.rehash(ind)

                if ind == start_ind:
                    done = True

                elif self.keys[ind] == None:
                    done = True

        return val

    def hash(self, key):
        return key % self.size

    def rehash(self, oldhash):
        return (oldhash + self.step) % self.size

    def __getitem__(self, key):
        return self.get(key)

    def __setitem__(self, key, val):
        self.put(key, val)

break (or better yet return)

Instead of doing done = False/True. You could just use break and get rid of done altogether. But even better than that, just return.

This:

        if self.keys[ind] == key:
            val = self.vals[ind]
            done = True

Becomes:

        if self.keys[ind] == key:
            return self.vals[ind] # Value found!

This:

            if ind == start_ind:
                done = True

            elif self.keys[ind] == None:
                done = True

Becomes:

            if ind == start_ind: # Not found!
                return

            elif self.keys[ind] == None: # Not found!
                return

(You can return None if you want).

This gets rid of the val variable (and done)


Swift iOS rewrite a function that use Firebase database without updating a global variable I'm learning swift programming and i built a music trivia app for iOS that use firebase real time database.
I have a function that before the game start get the number of record for each category of the game and update a class variable that is a tuple called records.
var records = (classical: 0, mix: 0, casual: 0)

private func getNumberOfRecords(category: String)  {
    var ref: DatabaseReference!
    ref = Database.database().reference().child(category)
    ref.observeSingleEvent(of: .value) { snapshot in
        let enumerator = snapshot.children
        while((enumerator.nextObject() as? DataSnapshot) != nil) {
            switch category {
            case "casual":
                records.casual += 1

            case "classical":
                records.classical += 1

            case "mix":
                records.mix += 1

            default: print("Error loading record from category \(category)")
            }
        }
    }
}

Here is how I call the function and because it's asynchronous the response I receive from firebase and I need to know the number of records before call another function that will load the game data from the database i use DispatchQueue and wait 3 seconds.
getNumberOfRecords(category: "classical")
getNumberOfRecords(category: "casual")
getNumberOfRecords(category: "mix")
DispatchQueue.main.asyncAfter(deadline: .now() + 3) {
  loadDataFromDB(records: records.classical, category: "classical")
  loadDataFromDB(records: records.casual, category: "casual")
  loadDataFromDB(records: records.mix, category: "mix")
}

Now this is working but it's pretty ugly, especially using the function getNumberOfRecords to update the global tuple records. I don't know how can I rewrite this because if i make getNumberOfRecords return a tuple rather than update the global one is always 0 when i pass it as parameter of the function loadDataFromDB inside the DispatchQueue.
Thanks in advance to anyone will have time to read this.

A typical way to write an asynchronous function is to have it take a function as an argument that will be called with the results when they are ready. Such a function is often called a completion handler.
private func getNumberOfRecords(category: String, completion: (Int) -> Void) {
   ...
            default: print("Error loading record from category \(category)")
            }
        }
    }
    completion(records.count)
}

Then at the call site you can write:
getNumberOfRecords(category: "classical", completion: { count in ... })

or better written with trailing-closure syntax:
getNumberOfRecords(category: "classical") { count in ... }

You still have the problem of three asynchronous things, and instead of chaining them together, you can also avoid looping over the data 3 times, you can loop over the data once and remember the counts for each item, using a dictionary:
private func getNumberOfRecords(completion: ([String: Int] -> Void)) {
    var categoryCounts: [String: Int] = [:]

    {
    ... loop over the data here and instead of switch category ...
    categoryCounts[category] = (categoryCounts[category] ?? 0) + 1
    }

    completion(categoryCounts)
}

getNumberOfRecords { categoryCounts in
    for (category, count) in categoryCounts {
       loadDataFromDB(records: count, category: category)
    }
}


Class for exposing information with computer properties (getters) I need some advice on my code structure. I have read that setters and getters break one of the OOP principals, encapsulation, and that we should avoid them if possible but I cannot think of any other way to expose info other than getters.

I have a class which calculates price, savings and taxes based three inputs from the user through the initializer (initialPrice, discount and taxRate).

I'm using three computer properties to exposed the results, is this considered a bad thing? If yes, why and how can I improve my code?

Besides what I described above the class also has the ability to add all of the items in an array, assuming the instances of the CalculatorBrain were placed in an array.

Again, I'm more concern about the structure of my code. Any advice will be appreciated.

class CalculatorBrain {

        private var taxRate:Double = 0
        private var discount:Double = 0
        private var initialPrice:Double = 0

        func setInitialValues(initialPrice:Double, discount:Double, taxRate:Double){
            self.taxRate = taxRate
            self.discount = discount
            self.initialPrice = initialPrice
        }

        private func salePrice()->Double{
            return initialPrice - savings
        }

        var price:Double{
            get{
                return salePrice() + taxes
            }
        }

        var savings:Double{
            get{
                return initialPrice * discount / 100
            }
        }

        var taxes:Double{
            get{
                return salePrice() * taxRate / 100
            }
        }

        internal func grandTotals(list:Array<Item>)->(grandPrice:Double, grandSavings:Double, grandTaxes:Double){
            var grandPrice:Double = 0
            var grandSavings:Double = 0
            var grandTaxes:Double = 0

            for i in 0..<list.count {
                grandPrice = grandPrice + Double(list[i].totalPrice)
                grandSavings =  grandSavings + Double(list[i].savings)
                grandTaxes = grandTaxes + Double(list[i].taxes)
            }
            return (grandPrice, grandSavings, grandTaxes)
        }
    }

Set Initial Values?
private var taxRate:Double = 0
private var discount:Double = 0
private var initialPrice:Double = 0

func setInitialValues(initialPrice:Double, discount:Double, taxRate:Double){
    self.taxRate = taxRate
    self.discount = discount
    self.initialPrice = initialPrice
}

I don't care for this at all.
First, I don't see a good reason for these three properties to necessarily be private.  Why should the user have to guess what they've set their properties to?
Further more, you're not protecting these values from being reset.  The user simply has to call setInitialValues again.  The method name may be an attempted hint at that being the incorrect use of the class, but my mantra is let the compiler do as much for us as possible.
We should expose these properties to the user.  We should not allow the user to modify these properties after initialization.  We should write an init method.
let taxRate: Double
let discount: Double
let basePrice: Double

init(taxRate: Double = 0, discount: Double = 0, basePrice: Double = 0) {
    self.taxRate = taxRate
    self.discount = discount
    self.basePrice = basePrice
}

Why is salePrice() a function?
You have implemented basically everything else as a computed, read-only property.  Why can't salePrice also be a computed, read-only property?  If you want it to be private, that's fine if that makes sense for your class, but it can still be a computed property.
var salePrice: Double {
    return basePrice - savings
}

Also, note what I've done here.  The get { } is omitted.  Computed properties that just return a value can omit the get and are implicitly read-only.  Any code is assumed to be for the get if you have omitted the get & set keywords.

internal is the default access level.
You have marked some things as private.  You have omitted an access specifier on most things, and then your grand total method is specified as internal.  Marking internal is the same as omitting an access specifier.  You should drop this internal keyword here, or consistently use it with all the other properties.  None of your members are public in your current implementation.

About that grandTotals method...
This method almost seems out of place if it weren't for the fact that the class is called CalculatorBrain.  But it doesn't depend on any state, so it should probably be a static method.
Your loop is far from optimal.  But, we don't even need a loop.  We can use reduce.
return list.reduce((0,0,0)) {
    return (
        $0.totalPrice + $1.totalPrice,
        $0.savings + $1.savings,
        $0.taxes + $1.taxes
    )
}

But... I might avoid returning a tuple.  Why not just make a struct?
struct GrandTotal {
    let price: Double
    let savings: Double
    let taxes: Double
}

And now instead of a method that feels like it's in a weird place, we just add an appropriate init:
struct GrandTotal {
    let price: Double
    let savings: Double
    let taxes: Double

    init(list: [Item]) {
        let totals = list.reduce((0,0,0)) {
            return (
                $0.totalPrice + $1.totalPrice,
                $0.savings + $1.savings,
                $0.taxes + $1.taxes
            )
        }

        price = totals.0
        savings = totals.1
        taxes = totals.2
    }
}


Using global variables in my node.js program configuration I know using global variables in JavaScript is bad practice.  Here I am using config, host and port as global variables as I have added a watch function to watch for the changes in the config.json file.  Is the use of global variables in this example a bad practice or is it ok to use them?

config.json file

{
    "host":"127.0.0.1",
    "port":8000
}

server.js file

(function(){

    var fs = require("fs");
    config = JSON.parse(fs.readFileSync("config.json")); //global variable
    host = config.host;  //global variable
    port = config.port;  //global variable
})();

var http = require("http");
var server = http.createServer(function(request,response){
    response.writeHead(200,{"Content-Type":"text/plain"});
    response.end("Hello World");
});

server.listen(port,host,function(){
    console.log("Server listening at:"+host+" on port:"+port);
});

fs.watchFile("config.json",function(){
    config = JSON.parse(fs.readFileSync("config.json"));  //global variable(overwritten)
    host = config.host;  //global variable(overwritten)
    port = config.port;  //global variable(overwritten)

    server.close();
    server.listen(port,host,function(){
          console.log("Server listening at:"+host+" on port:"+port);
       };
    }
});

You say you're using globals because "I have added a watch function to watch for the changes in the config.json file". You don't need globals for this. If you declare the variables at the top level, like you've done with http and server, they'll be accessible everywhere in your script, including inside the anonymous function where you're using them.* You could get rid of the (function() { ... })(); wrapper round the first bit (not sure why you've wrapped that bit) and then declare them there.

"I know using global variables in JavaScript is bad practice." Using global variables in any language is bad practice if you can avoid it.

*A function like this, which uses variables declared outside the function, is called a "closure".


Searching text files for string patterns that are each defined in their own classes I'm trying to implement interfaces in my design but not sure if this is correct or if there is a better way to do it.

What I need to do is

open a text file
keep searching blocks of text until I find a string that matches against one of the classes that defines that string pattern
process the string I found
continue processing file where I left off

This is the code I'm using so far, but not sure if the design is correct.
Maybe something about creating a new instance of each stringType class just to check if the string matches with the one defined in the class seems wrong to me. I don't need to create the instance of the object until I actually need to do work on it.

EDIT - Forgot to add the process function, not sure if that changes anything

static void Main(string[] args)
{
    //get types that implement IStringType interface
    List<IStringType> types = TypeFactory.GetTypeList();

    //scan text file and check against the types, store any matches found
    var stringMatches =  from t in types
                        where Regex.IsMatch(sourceString, t.stringPattern)
                        select t.getNewInstance(Regex.Match(sourceString,t.stringPattern).Value);

    if (stringMatches.Any())
    {
        //process string matches
    }
}
public class stringType1 : IStringType
{
    public string stringPattern { get { return @"(?m)^TEST1.+[\r\n]"; } set { this.stringPattern = value; } }
    private string _Message { get; set; }

    public stringType1() { }
    public stringType1(string str)
    {
        _Message = str;
    }

    public string ProcessString(string match)
    {
       //specific code to processes stringType1

    }
    public IStringType getNewInstance(string msg)
    {
        return (IStringType)(new stringType1(msg));
    }
}

public class stringType2 : IStringType
{
    public string stringPattern { get { return @"(?m)^BLAH2.+[\r\n]"; } set { this.stringPattern = value; } }
    private string _Message { get; set; }

    public stringType2() { }
    public stringType2(string str)
    {
        _Message = str;
    }
    public string ProcessString(string match)
    {
       //specific code to processes stringType2

    }
    public IStringType getNewInstance(string msg)
    {
        return (IStringType)(new stringType2(msg));
    }
}

public interface IStringType
{
    string stringPattern { get; set; }
    string ProcessString(string str);
    IStringType getNewInstance(string str);
}

public static class TypeFactory
{
    public static List<IStringType> GetTypeList()
    {
        List<IStringType> types = new List<IStringType>();
        types.AddRange(from assembly in AppDomain.CurrentDomain.GetAssemblies()
                       from t in assembly.GetTypes()
                       where t.IsClass && t.GetInterfaces().Contains(typeof(IStringType))
                       select Activator.CreateInstance(t) as IStringType);
        return types;
    }
}

It's very easy: you shouldn't. An interface is used to indicate that a subset of classes present common behaviour or should be considered similar in some way.
In your situation there simply is no reason to make this change because all you do is change a text value in a property, which could just as easily be done by taking an argument in your constructor and keeping a private backing fields which should then be used to populate the property's regex.
This will result in several code optimizations:

No heavy duplication of code in StringType1 and StringType2 (this should have been a red flag)

No reflection necessary to get the different types. Just keep a collection of the different regexes and return a collection of StringType objects, constructed with your regexes.

Additionally, this code will also be obsolete:
public IStringType getNewInstance(string msg)
{
    return (IStringType)(new stringType2(msg));
}

since you should just overwrite that variable with new Stringtype(thisVariable.StringPattern, message).
Ultimately, your code would look something like this:
public class StringType
{
    private IPatternProcessor _processor;
    private string _pattern;
    private string _message { get; set; }

    public StringType(string str, IPatternProcessor processor)
    {
        _message = str;
        _processor = processor;
    }

    public string StringPattern {
        get { return pattern; }
        set { pattern = value; }
    }

    public void Process() {
        _processor.Process(_message);
    }
}

public static class PatternFactory
{
    private static List<String> _patterns = new List<String>();

    static PatternFactory() {
         _patterns.Add("(?m)^TEST1.+[\r\n]";);
         _patterns.Add("(?m)^BLAH2.+[\r\n]";);
    }

    public static List<StringType> GetTypeList()
    {
        return _patterns.Select(x => new StringType(x));
    }
}

public interface IPatternProcessor {
    void Process(string input);
}

public class ProcessMethod1 : IPatternProcessor { }
public class ProcessMethod2 : IPatternProcessor { }

If you want to make it easy for your user, you can hardcode these different methods into properties:
public class PatternProcessor {
    public IPatternProcessor ProcessMethod1 { get { return new ProcessMethod1(); } private set; };
    public IPatternProcessor ProcessMethod2 { get { return new ProcessMethod2(); } private set; };
}

Now you can simply call this with
new StringType(input, PatternProcessor.ProcessMethod1).Process();

If you were really into it, you could just cache the IProcessMethod objects in a collection in your PatternProcessor class (provided each method is stateless), but that might be going a little too far out of the scope of this review.
Style notes

Classes are UpperCamelCase (StringType)
Private fields are less standardized but they are often written as _field
Methods and constructors are always capitalized (StringPattern, StringType), just like the class

Conclusion
This code does not benefit from interfaces at all and as such it is impossible to review it since interfaces are the main problem with the code.
Post-edit
So, with the additional requirement of pattern-specific behaviour the introduction of interfaces and separate types now makes a lot more sense. I see two architecture options off the bat:

Separate StringType classes for each situation (if you name them StringType1, StringType2 etc, I'll chase you with a bat)

Separate strategies (strategy pattern) for each type of processing with a sauce of factory pattern

I will go with the second option because it would require less rewriting. Someone else can write the less complex first option but the second option is more flexible in terms of allowing two different patterns to use the same processing option.
I will change the earlier block of code with some additions to keep everything in one place.


Shell Script Audio Logger I created my first shell script. It's an audio logger that runs on the hour for an hour and restarts. Although if it is started within the hour it will only run until the end of the hour.

I've added it to Startup Applications with the command:

gnome-terminal -e /home/admin/Desktop/audiolog.sh

However when it starts up the text in terminal flickers and is resolved by maximizing the window but works fine. Any suggestions for improvements on my script or anything that is wrong? Thanks

#!/bin/sh

DAY=$(date +%d)
MO=$(date +%m)
YR=$(date +%y)
SE=$(date +%-S)
H=$(date +%H)
M=$(date +%M)

NOW=$(date)

DURATION=$(((60 - M) * 60-SE))

PART=$(find ./Documents/ -type f -name "LoggerAudio
${DAY}-${MO}-${YR}\ ${H}*" | wc -l)

reset

echo "Recording for $DAY $MO $YR - $H 00"
echo "Started at $NOW"
echo "- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
- - - - - - - - - -"

if [ "$PART" -eq 0 ]
then
arecord -f cd -c 1 -d$DURATION -t raw -v -v | lame -b 128 -q2 -mm
-r - ./Documents/"LoggerAudio "$DAY-$MO-$YR\ $H"00".mp3
else
arecord -f cd -c 1 -d$DURATION -t raw -v -v | lame -b 128 -q2 -mm
-r - ./Documents/"LoggerAudio "$DAY-$MO-$YR\ $H"00 - Part - "$PART.mp3
fi

sleep 1s

./Desktop/audiolog.sh;

This looks quite fine, except this part:

if [ "$PART" -eq 0 ]
then
arecord -f cd -c 1 -d$DURATION -t raw -v -v | lame -b 128 -q2 -mm
-r - ./Documents/"LoggerAudio "$DAY-$MO-$YR\ $H"00".mp3
else
arecord -f cd -c 1 -d$DURATION -t raw -v -v | lame -b 128 -q2 -mm
-r - ./Documents/"LoggerAudio "$DAY-$MO-$YR\ $H"00 - Part - "$PART.mp3
fi

One problem here is indentation:
it's a lot easier to read when the branches of conditionals are indented.

A bigger problem is that a long and complicated pipeline is duplicated (looks copy-pasted),
which is a bad practice, because it's prone to errors.
Since only the filename is different,
you can set that in the conditional branches,
and then write the complicated pipeline only once.

if [ "$PART" -eq 0 ]
then
    out="LoggerAudio $DAY-$MO-$YR ${H}00.mp3"
else
    out="LoggerAudio $DAY-$MO-$YR ${H}00 - Part - $PART.mp3"
fi
arecord -f cd -c 1 -d$DURATION -t raw -v -v | lame -b 128 -q2 -mm -r - ./Documents/"$out"

Notice that I also slightly simplified the writing style of the filename.


Simple exercice representing a slidepuzzle I've been helping out a student with an exercice, typed this up rapidly and thought it would be the ideal opportunity to improve my own programming style and approach to problem solving.

In this exercice a grid represents one of those slide puzzles. The patterns have been replaced by numbers. The zero represents the missing piece which allows for movement. There is no gameplay as of yet and the aim is to simply swap the missing piece around as would be done before the start of a game.

How could I make the code more concise?
What would be a good way to cut up the code into functions?
What other things could be improved to approach a decent programming style in Java?

import java.util.Arrays;
import java.util.Random;
import java.util.Scanner;

public class Puzzle {

    public static void main(String[] args) {

        System.out.println("Please enter dimensions of the puzzle.");

        System.out.println("Enter length (>0) ");

        Scanner sc = new Scanner(System.in);
        int width = sc.nextInt();

        System.out.println("Enter height (>0)");

        int height = sc.nextInt();

        int[][] puzzle = new int[width][height];

        populatePuzzle( puzzle );

        System.out.println("Enter position of missing piece");

        int position = sc.nextInt();

        placeMissingPiece( position, puzzle );

        System.out.println( "Missing piece is at " + Arrays.toString( findNumber( puzzle, 0 ) ) );

        displayPuzzle( puzzle );

        int number = 0;

        while( true ){
            System.out.println("Enter the number to be swapped around");

            number = sc.nextInt();

            swapElementValues( puzzle, findNumber( puzzle, 0 ), findNumber( puzzle, number ) );

            displayPuzzle( puzzle );
        }
    }

    public static void swapElementValues( int[][] puzzle, int[] missing, int[] target ){

        int tempValue = puzzle[ missing[0] ][ missing[1] ];

        puzzle[ missing[0] ][ missing[1] ] = puzzle[ target[0] ][ target[1] ];
        puzzle[ target[0] ][ target[1] ] = tempValue;
    }

    public static int[] findNumber( int[][] puzzle, int number ){

        for( int i = 0; i < puzzle.length; i++ ){
            for( int j = 0; j < puzzle[0].length; j++ ){
                if( puzzle[i][j] == number ){
                    int[] foundCoords = new int[2];

                    foundCoords[0] = i;
                    foundCoords[1] = j;

                    return foundCoords;
                }
            }
        }

        return null;
    }

    public static int getRandomInteger(){

        Random randomGenerator = new Random();

        return randomGenerator.nextInt(99) + 1;
    }

    public static void populatePuzzle( int[][] puzzle ){

        for( int i = 0; i < puzzle.length; i++ ){
            for( int j = 0; j < puzzle[0].length; j++ ){
                puzzle[i][j] = getRandomInteger();
            }
        }

    }

    public static void displayPuzzle( int[][] puzzle ){

        for( int i = 0; i < puzzle.length; i++ ){
            for( int j = 0; j < puzzle[0].length; j++ ){
                System.out.print( puzzle[i][j] + "," );
            }

            System.out.println( "" ); // Carriage return
        }

    }

    public static void placeMissingPiece( int elementNumber, int[][] puzzle ){

        int counter = 0;

        for( int i = 0; i < puzzle.length; i++ ){
            for( int j = 0; j < puzzle[0].length; j++ ){
                counter++;

                if( counter == elementNumber ){
                    puzzle[i][j] = 0;
                }
            }
        }

    }
}

System.out.println("Enter height (>0)");

        int height = sc.nextInt();

This pattern repeats itself several times in your code. Turn it into a method, e.g. (assuming the Scanner is turned into a static field):

private static int promptForInt(String message) {
    System.out.println(message);
    return sc.nextInt();
}

-

    public static int getRandomInteger(){

        Random randomGenerator = new Random();

        return randomGenerator.nextInt(99) + 1;
    }

Creating a new Random for each call makes no sense from a performance perspective. The object should be created exactly once.

    public static void populatePuzzle( int[][] puzzle ){

    public static void displayPuzzle( int[][] puzzle ){

    public static void placeMissingPiece( int elementNumber, int[][] puzzle ){

You're not thinking object-oriented here. The above pattern is a really obvious indication that each method should belong to a Puzzle class. That way your main method can concern itself with the core logic of your program instead of the implementation details of the puzzle. E.g.:

Puzzle puzzle = new Puzzle(); // .populate() should probably be called from the constructor
// ...
puzzle.placeMissingPiece(position);
// ...
puzzle.display();
// ...
puzzle.swapElementValues(0, number); // findNumber() called as a private method
// etc...


Python script to execute a command using Paramiko SSH I wrote this to learn exactly how paramiko worked in Py3. Also to keep as a code snippet for future work-related scripts. I know it can be tweaked and I'm hoping you experienced folks can help with that. I left the commented out lines for 'passwd' because not all hosts I connect to will use pubkeys.

import sys, paramiko, getpass

user = input("Username:") #Prompts for username
#passwd = getpass.getpass("Password for " + user + ":") #Prompts for password
key_file = input("Public key full path:") #Prompts for full path to key file
name = input("Target Hostname:") #Prompt for target hostname
command = input("Enter target command:") #Prompt for remote command

#Calling the paramiko ssh function
ssh = paramiko.SSHClient() #Define value 'ssh' as calling the paramiko.sshclient
print('calling paramiko')
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) #Must come before connect line to add hosts to known_hosts
#ssh.connect("name", username="user", password="passwd")
ssh.connect(hostname = name, username = user, key_filename = key_file) # key_filename means to specify the actual file, pkey on the
#other hand means to actually paste in the key text itself
print('trying to connect')
ssh.invoke_shell()
stdin, stdout, stderr = ssh.exec_command (command)
print(stdout.read())
#ssh.close()

All your imports should be below eachother like this:

import sys
import paramiko
import getpass

I think using classes is a bit too much for this, but you should create a few functions to diverse functionality

A Main
A function for the shell commands
A function for connecting

This should be enough for now, you can always add more. Secondly when using comments they should be like this

# A comment
print('do something')

instead of this:

print('do something') # A comment

I do think you have too many comments because the names kind of speak for themselves

def ssh_command(ssh):
    command = input("Command:")
    ssh.invoke_shell()
    stdin, stdout, stderr = ssh.exec_command(command)
    print(stdout.read())

def ssh_connect(host, user, key):
    try:
        ssh = paramiko.SSHClient()
        print('Calling paramiko')
        ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
        ssh.connect(hostname=host, username=user, key_filename=key)

        ssh_command(ssh)
    except Exception as e:
        print('Connection Failed')
        print(e)

if __name__=='__main__':
    user = input("Username:")
    key = input("Public key full path:")
    host = input("Target Hostname:")
    ssh_connect(host, user, key)

I think your program is ok, but could use some improvement, be sure to check out the Paramiko demo's these contain many usefull pointers for working with parmiko


Reorder objects based on provided enumerable order This code feels like it goes through too many conversions to accomplish my goal:

  based on an IEnumerable of ids get those objects from a data store and
  set their DisplayOrder property to the position of the associated
  id.

Is there a more elegant solution that I'm missing?

public void ReorderElements(IEnumerable<int> elementSks)
{
    var elementsToReorder = GetSession().QueryOver<DocumentElement>()
        .WhereRestrictionOn(de => de.DocumentElementSk).IsIn(elementSks.ToArray()).List();

    var elementOrder = elementSks.Select((sk, i) => new { Sk = sk, Order = i })
        .ToDictionary(p => p.Sk, p => p.Order);

    elementsToReorder.ToList()
        .ForEach(e => e.DisplayOrder = elementOrder[e.DocumentElementSk]);

    this.Save(elementsToReorder);
}

If the list on input is going to be short, or if performance doesn't matter to you, you can simplify the code by using IndexOf() instead of the dictionary:

public void ReorderElements(IEnumerable<int> elementSks)
{
    var elementsToReorder = GetSession().QueryOver<DocumentElement>()
        .WhereRestrictionOn(de => de.DocumentElementSk).IsIn(elementSks.ToArray()).List();

    var elementOrder = elementSks.ToList();

    foreach(var element in elementsToReorder)
         element.DisplayOrder = elementOrder.IndexOf(element.DocumentElementSk);

    this.Save(elementsToReorder);
}


Is using return appropriate here? I wanted to get the boolean status from a function. Here is my sample code, wherein I used return in try and catch, to get the status of function.

public static void main(String[] args) {
        System.out.println(getState() ? "successful" : "failed");
}

public static boolean getState() {
        SAXBuilder builder = new SAXBuilder();
        File xmlFile = new File("tile.xml");
        Set<String> set = new TreeSet<String>();
        try {
            Document document = (Document) builder.build(xmlFile);
            Element rootNode = document.getRootElement();
            System.out.println(rootNode);
            return true;
        } catch (IOException io) {
            System.out.println(io.getMessage());
            return false;
        } catch (JDOMException jdomex) {
            System.out.println(jdomex.getMessage());
            return false;
        }
    }

Does it makes sense ? Can I improve it ?
Assuming, that there might be more than 10 catch blocks, then should I use 10 return statements, in each catch block.

For only 2 or 3 instances, I may use multiple return statements, but only in the case of if-else chains or switches. Returns within a try catch block can be done, but also can become complicated when you start mixing in finally with it. In the case of try-catch blocks or several possible 'return' paths, I use a variable and set it as necessary, and return it at the end. If you want an early return to bypass some code, I tend to take that code and wrap it up in a separate method, and use an if statement with my value to return to determine whether to execute that method.

public static void main(String[] args) {
        System.out.println(getState() ? "successful" : "failed");
}

public static boolean getState() {
        boolean success = false;
        SAXBuilder builder = new SAXBuilder();
        File xmlFile = new File("tile.xml");
        Set<String> set = new TreeSet<String>();
        try {
            Document document = (Document) builder.build(xmlFile);
            Element rootNode = document.getRootElement();
            System.out.println(rootNode);
            success = true;
        } catch (IOException io) {
            System.out.println(io.getMessage());
        } catch (JDOMException jdomex) {
            System.out.println(jdomex.getMessage());
        }

        return success;
}


Finding decreasing sequences in an array of numbers Ants

  Requirement

  Given a natural number \$n\$, representing the number of days the study is made, and then a number \$x\$ of \$n\$ natural numbers, representing the number of working ant flies after food, where the \$x_i\$ number of working ants is the day \$i\$. It is required to determine the number of maximum sequences, ordered strictly decreasing after the number of divisors, of minimum length \$2\$.

  Input data

  The input file furnici.in contains two lines. On the first line is the natural number \$n\$, with the meaning of the requirement and on the second line, the elements of the string \$x\$, with the meaning in the requirement.

  Output data

  The output file furnici.out will contain a natural number representing the number of maximum sequences, of minimum length \$2\$, ordered strictly downwards by the number of divisors.

  Restrictions and clarifications

  Time Limit - 0.5 seconds
  Memory limit - 64 MB Total / 8 MB Stack
  \$2 \leq n \leq 100000 \$
  \$10 \leq x_i \leq 1.000.000.000\$

  Example

  furnici.in

10
719 169 4065 813 289 101 123 516 516 1017

  furnici.out

2

  Explanation

  Row number of divisions is \$2\ 3\ 8\ 4\ 3\ 2\ 4\ 12\ 12\ 6\$.
  It is noted that there are two maximum sequences, of at least two, strictly decreasing, \$8\ 4\ 3\ 2\$ and \$12\ 6\$.

My program is given an array of numbers that it has to find the divisors for and then replace the number in the array with its corresponding number of divisors. In the new created array, it will have to find and count all the descending subsequences that occur.

Here is my code:

#include <iostream>
#include <fstream>
#include <cmath>
using namespace std;
int n,i,v[100001],nr,j,s;
int main()
{
    ifstream in("furnici.in");
    ofstream out("furnici.out");
    in>>n;
    for(i=1; i<=n; i++)
    {
        in>>v[i];
        nr=2;
        for(j=2; j<=sqrt(v[i]); j++)
        {
            if(v[i]%j==0)
            {
                nr++;
                if(j!=v[i]/j)
                    nr++;
            }
        }
        v[i]=nr;
        if(v[i]<v[i-1])
        {
            j=i;
            while(v[j]<v[j-1] && j<n)
            {
                j++;
                nr=2;
                in>>v[j];
                for(int k=2; k<=sqrt(v[j]); k++)
                {
                    if(v[j]%k==0)
                    {
                        nr++;
                        if(k!=v[j]/k)
                            nr++;
                    }
                }
                v[j]=nr;
            }
            s++;
            i=j;
        }
    }
    out<<s;
    return 0;
}

furnici.in

10
719  169 4065  813  289  101 123  516  516  1017

furnici.out

2

Explained:

10 represents how many numbers are going to be read from the file;
number 719 has 2 divisors, so 719 gets replaced with 2;
number 169 has 3 divisors, so 169 gets replaced with 3;
number 4065 has 8 divisors, so 4065 gets replaced with 8;
and so on, you kind of get the point. The resulting array will have
descending sequences: 2 3 8 4 3 2 4 12 12 6 and these need to be counted.

The issue with it is that it exceeds the time limit in the last test given to it, but otherwise the answer is correct.

Some general remarks:

Don't use namespace std;, see for example Why is “using namespace std;” considered bad practice?.
Define variables at the narrowest possible scope. In particular, avoid
global variables.
Use better variable names. It is unclear what each variable in

int n,i,v[100001],nr,j,s;

stands for.
return 0; at the end of the main program can be omitted.
The range of integer types is implementation defined, the C++ standard only
guarantees that a (signed) int can hold values from -32767 to 32767,
which is too small for your numbers. Many compilers define int as a 32-bit
integer, but you can use long to be on the safe side, or use fixed-size
types like int32_t.

With respect to readability, I recommend to leave more (horizontal) space,
e.g. around operators and parentheses.

There are two places with identical code to count the divisors
of a number. This should be done in a separate function.

Your code uses a nested loop where the inner loop updates the index of the
outer loop. That is difficult to understand and error-prone. And it is
not necessary: Instead of starting a nested loop when the start of a
decreasing subsequence is found, set a flag instead and continue with the
main loop.

You store all numbers from the input file in an array, which is not necessary:
each loop iteration only needs the previous number to decide if the subsequence
is (still) decreasing. It suffices to store the previously processed number
in a variable.

Summarizing the suggestions so far, the code could look like this:

#include <fstream>
#include <cmath>

long numberOfDivisors(long n) {
    long count = 0;
    for (long j = 2; j <= sqrt(n); j++) {
        if (n % j == 0) {
            count++;
            if (j != n/j)
                count++;
        }
    }
    return count;
}

int main()
{
    std::ifstream inFile("furnici.in");
    std::ofstream outFile("furnici.out");

    long decreasingSequences = 0;
    bool isDescending = false;
    long lastDivisorCount = 0;
    long numDays;
    inFile >> numDays;
    for (long i = 1; i <= numDays; i++) {
        long numAnts;
        inFile >> numAnts;
        long divisorCount = numberOfDivisors(numAnts);
        if (divisorCount >= lastDivisorCount) {
            // No longer decreasing.
            isDescending = false;
        } else if (!isDescending) {
            // A decreasing subsequence started right here.
            isDescending = true;
            decreasingSequences += 1;
        }
        lastDivisorCount = divisorCount;
    }
    outFile << decreasingSequences;
}

Now you can start to improve the performance, and the prime candidate is
of course the numberOfDivisors() function.

An efficient method (and I'm repeating arguments from Getting all divisors from an integer now) is to use the prime factorization:  If
$$
 n = p_1^{e_1} \, p_2^{e_2} \cdots p_k^{e_k}
$$
is the factorization of \$ n \$ into prime numbers \$ p_i \$
with exponents \$ e_i \$, then
$$
 \sigma_0(n) = (e_1+1)(e_2+1) \cdots (e_k+1)
$$
is the number of divisors of \$ n \$, see for example
Wikipedia: Divisor function. Example:
$$
 720 = 2^4 \cdot 3^2 \cdot 5^1 \Longrightarrow
  \sigma_0(720) = (4+1)(2+1)(1+1) = 30 \, .
$$

Here is a possible implementation in C:

long numberOfDivisors(long n){

    long numDivisors = 1;
    long factor = 2; // Candidate for prime factor of `n`

    // If `n` is not a prime number then it must have one factor
    // which is <= `sqrt(n)`, so we try these first:
    while (factor * factor <= n) {
        if (n % factor == 0) {
            // `factor` is a prime factor of `n`, determine the exponent:
            long exponent = 0;
            do {
                n /= factor;
                exponent++;
            } while (n % factor == 0);
                // `factor^exponent` is one term in the prime factorization of n,
                // this contributes as factor `exponent + 1`:
                numDivisors *= exponent + 1;
        }
        // Next possible prime factor:
        factor = factor == 2 ? 3 : factor + 2;
    }

    // Now `n` is either 1 or a prime number. In the latter case,
    // it contributes a factor 2:
    if (n > 1) {
        numDivisors *= 2;
    }

    return numDivisors;
}

As a further improvement you can pre-compute the prime numbers with a
sieving method. Note that it sufficient to pre-compute the primes in the
range \$ 2 \ldots \sqrt{N} \$ where \$ N = 10^9 \$ is the upper bound for the given input.  That should help to stay within the given memory
limit of 64 MB.


Getting multiple keys' values from nested object in JSON (w/out jQuery) I'd like to ask if either my JSON data is ill-conceived and whether how I solved it is particularly inefficient/plain stupid/typical.

Given the following JSON data:

{
    "User01": {
        "dates & times": {
            "date 01": "150",
            "date 02": "165",
            "date 03": "180"
        }
    },

    "User02": {
        "dates & times": {
            "date 01": "150",
            "date 02": "165",
            "date 03": "180"
        }
    },

    "User03": {
        "dates & times": {
            "date 01": "150",
            "date 02": "165",
            "date 03": "180"
        }
    }
}

... and wanting to retrieve the dates & times data so it can be rendered, e.g.:

Date 01: Time 01
Date 02: Time 02
Date 03: Time 03

... and which I've achieved thusly:

for (var key in myObj){
    if (key == selUser){
        for (var date in myObj[key]["dates & times"]){
            console.log("Date: " + date + "; " + "weight: " + myObj[key]["dates & times"][date]);
            var dtTag = "
Date: " + date + "
";
            var ddTag = "
Time: " + myObj[key]["dates & times"][date] + "
";
            output.innerHTML += dtTag;
            output.innerHTML += ddTag;
        }
    } // if
} // loop

So, as to the json data: assuming that there could be hundreds of date & time entries, is this even a legit usage of json or should this kind of data be better formatted using arrays, e.g.:

{ "dates & times":
    {
        "dates": [ "date01", "date02", "date03" ],
        "times": [ "150", "165", "180" ]
    }
}

And in such a case where I had no control of the JSON data, how could I improve on my solution to iterate for selected user and iterate again through nested object's data?

By the way, this is in context of studying JavaScript, not jQuery or any other library/framework.

{ "dates & times":
    {
        "dates": [ "date01", "date02", "date03" ],
        "times": [ "150", "165", "180" ]
    }
}

No, don't do that. You take an explicit relationship in one data structure and make it implicit between two data structures. Instead, do:

{ "dates & times": [
    { "date": "2014-08-24", "time": "150" },
    { "date": "2014-08-24", "time": "165" },
    { "date": "2014-08-24", "time": "180" },
] }

Now the relationship is explicit, and you can iterate over the array more simply and safely than iterating over the keys. (Safely because, if a property is added to Object.prototype, it will show up as a key.)

for (var key in myObj){
    if (key == selUser){
        for (var date in myObj[key]["dates & times"]){
            console.log("Date: " + date + "; " + "weight: " + myObj[key]["dates & times"][date]);
            var dtTag = "
Date: " + date + "
";
            var ddTag = "
Time: " + myObj[key]["dates & times"][date] + "
";
            output.innerHTML += dtTag;
            output.innerHTML += ddTag;
        }
    } // if
} // loop

This has a bit more going on than is totally necessary. The outer loop is superfluous, since we only actually do something for one key. The inner loop changes with our new data structure. Also, we can do a bit of reorganization to help performance a bit. We will also change the if statement into a guard clause to clarify the code path.

// Is this key in the object, and does it have the right property?
if(!myObj[selUser] || !myObj[selUser]["dates & times"]) { // return, throw error, whatever }

output.innerHTML += myObj[selUser]["dates & times"].map(function(el){
    console.log("Date: " + el.date + "; Time: " + el.time);
    var dtTag = "
Date: " + el.date + "
";
    var ddTag = "
Time: " + el.time + "
";

    return dtTag + ddTag;
}).join("");

This relies on a newer ES5 feature, so polyfill or replace with equivalent loops if you need to support older browsers (see below). Also, this code does not exactly give the output you specified, FYI.

Without Array#map:

// Is this key in the object, and does it have the right property?
if(!myObj[selUser] || !myObj[selUser]["dates & times"]) { // return, throw error, whatever }

var i, el, dtTag, ddTag;

var listItems = [];
var datesAndTimes = myObj[selUser]["dates & times"];
var datesAndTimesLength = datesAndTimes.length;

for(i = 0; i < datesAndTimesLength; i++) {
    el = datesAndTimes[i];
    console.log("Date: " + el.date + "; Time: " + el.time);
    dtTag = "
Date: " + el.date + "
";
    ddTag = "
Time: " + el.time + "
";

    listItems.push(dtTag + ddTag);
}

output.innerHTML += listItems.join("");


Map function in modern c++ I have this working map function and I'm wondering if I can improve anything. Should I prefer templates instead of auto in this case?

auto map = [](auto items, auto fun) {
    vector<decltype(fun(items[0]))> result;

    for (auto item : items) {
        result.push_back(fun(item));
    }

    return result;
};

I think one thing that you could do is to call result.reserve(items.size()) (as long as you expect that items can be queried for size()) just after you declare it. That would prevent the vector from allocating new space each time new element is added and/or being copied from time to time as the elements are being added to it, which may result in increased performance, especially when items contains a lot of elements and/or of large size.

It's possible that compiler optimizations would do the same to you, but it won't hurt if you put it anyway just in case. Besides, optimizations might be turned off in debug mode.

Also, since you don't modify items, it's better to make it const auto& for the sake of const correctness and pass it by reference to avoid unnecessary copying as @Deduplicator has pointed out. The same goes for item in the range loop if fun is not to modify it.


How to write a method that takes a general iterator as input argument? Now I have a bunch of these methods ( you can tell how ugly it looks like):

void ConvertUtility::ConvertEdgeIndexList(const list<EdgeIndex> &edgeIndex)
{

     list<EdgeIndex>::const_iterator sit;
     for(sit=edgeIndex.begin(); sit!=edgeIndex.end(); sit++)
     {

     }
}

void ConvertUtility::ConvertEdgeIndexList(const vector<EdgeIndex> &edgeIndex)
{

     vector<EdgeIndex>::const_iterator sit;
     for(sit=edgeIndex.begin(); sit!=edgeIndex.end(); sit++)
     {

     }
}

Is there anyway I can aggregate these two functions into one single function?

How about:

template<typename C>
void ConvertUtility::ConvertEdgeIndexList(C const& edgeIndex)
{
     typename C::const_iterator sit;
     for(sit=edgeIndex.begin(); sit!=edgeIndex.end(); ++sit)
     {                                      //        ^^ Prefer pre-increment on iterators.

     }
}

PS. Stackoverflow may be a better site for this type of question.


Domain object toString null check Following advice to "Make good use of toString() for all Domain classes",  I made what I thought was the most useful toString for my class.  I found that certain operations happen before there are values for the components of the toString.  I wonder if there is a cleaner way to check for nulls or even if I need to approach things differently.  As I'm writing, it occurs to me that depending on other objects in a toString is probably a bad idea.

class AssessmentStep {
    String hours
    Date assStepDate
    Assessment ass
    Step step
    Integer assStepProgress
    static hasMany =[stepNotes:StepNote]

    @Override
    String toString(){
        String stepString = "null"
        String appString = ""
        if (step != null && step.stepName != null){
            stepString = step.stepName
        }
        if (ass !=null && ass.app != null && ass.app.appName != null){
            appString = ass.app.appName
        }

        return appString + " " + stepString
    }

You could do:

String toString(){
    "${ass?.app?.appName ?: ''} ${step?.stepName ?: 'null'}"
}


PBKDF2 C# Implementation This is my first attempt to do any hashing. I wanted to make a class that handled everything I needed to implement PBKDF2. The only question I have is am I ok to store the iterations in the database record or is this a bad practice? I was planning on creating a column to store salt, another for the hashed password, and possibly one for the amount of iterations. My thought with the iterations was that I read that it should increase every so often so when creating new users or they update their passwords it would pull the value to pass in from the database.

    public static byte[] GenerateSalt()
    {
        using (var randomNumberGenerator = new RNGCryptoServiceProvider())
        {
            var salt = new byte[32];
            randomNumberGenerator.GetBytes(salt);

            return salt;
        }
    }

    public static byte[] HashPassword(byte[] password, byte[] salt, int iterations)
    {
        using (var rfc2898 = new Rfc2898DeriveBytes(password, salt, iterations))
        {
            return rfc2898.GetBytes(32);
        }
    }

    public static bool CompareByteArrays(byte[] array1, byte[] array2)
    {
        if (array1.Length != array2.Length)
        {
            return false;
        }

        for (int i = 0; i < array1.Length; i++)
        {
            if (array1[i] != array2[i])
            {
                return false;
            }
        }

        return true;
    }

The code provided might fulfill the functionality however the API is hard to use.

Passwords are usually strings and not binary (byte[]). It would also be nice that the salt could be generated automatically when you hash the password. That way you would call a single method to hash a password instead of calling two methods, in this case that would be GenerateSalt followed by HashPassword.

According to my argument I would suggest you to implement the following API:

// returns hashed password + salt
Tuple<byte[], byte[]> HashPassword(string password, int iterations)
bool ComparePassword(string attempt, byte[] hashedPassword, byte[] salt)

You can always replace the Tuple type by another class that would have the properties byte[] HashedPassword and byte[] Salt. Also often, but not always, the salt and the password are usually base64 encoded. That usually allows you to have a slightly nicer view of the data that is stored while also avoiding storing binary data in your persistence mechanism.

Additionally Rfc2898DeriveBytes outputs 20 bytes. So return rfc2898.GetBytes(32); should be return rfc2898.GetBytes(20);

Answering your additional question about the iteration count, there is nothing wrong with that. You can even use that as a pepper, different users can have slightly different iteration counts. Which makes the job of a hacker harder. In some scenarios people also store the actually algorithm that was used to hash the password. That might allow you to switch between algorithms in a easier fashion.


Lowering speed on my hybride encryption function for large files I have an assignment to encrypt files (+- 1 gb) and speed is an important factor. I am also kinda forced to the C# language since nobody in my group knows C++. The time now recorded for 100 mb is 12 seconds could somebody give me advice if it is possible to improve the speed my encryption function below.

private void buttonSearch_Click(object sender, RoutedEventArgs e)
{
    if (fileSelected && fileDestination)
    {
        String key = AESEncrypt.GenerateKey();
        progressBar.Visibility = Visibility.Visible;
        try
        {
            // AES Encrypt the file
            AESEncrypt.EncryptFile(textBoxSelectFile.Text, textBoxDestinationFile.Text, key,login.PrivateKey, progressBar);

            SecurityDB.Login loginB = (SecurityDB.Login)loginDataGrid.SelectedItem;
            byte[] Storekey = RSAEncrypt.EncryptKey(loginB.PublicKey, key);
            AESEncrypt.CreateKey(Storekey, this.textBoxDestinationFile.Text+"\\");

            // Writing PublicKey in destination Folder
            using (StreamWriter writer = new StreamWriter(this.textBoxDestinationFile.Text + "\\publicKey.txt"))
            {
                writer.WriteLine(loginB.PublicKey);
            }

        }
        catch (Exception ex)
        {
            System.Windows.MessageBox.Show(ex.Message);
        }
    }
    else
    {
        System.Windows.MessageBox.Show("Select Folders");
    }
}

public static async void EncryptFile(string inputFile, string outputFile, string sKey, String privateKey, ProgressBar progress)
{
    String fileName = inputFile;
    fileName = "\\" + fileName.Split('\\').Last();
    var progres = new Progress<int>(value => progress.Value = value);
    UnicodeEncoding UE = new UnicodeEncoding();

    await Task.Run(async () =>
        {
            byte[] key = UE.GetBytes(sKey);

            FileStream fsCrypt = new FileStream(outputFile + fileName, FileMode.Create);

            RijndaelManaged RMCrypto = new RijndaelManaged();

            CryptoStream cs = new CryptoStream(fsCrypt,
                        RMCrypto.CreateEncryptor(key, key),
                        CryptoStreamMode.Write);

            FileStream fsIn = new FileStream(inputFile, FileMode.Open);

            int data;
            byte[] buffer = new byte[0x1000];
            double i = 0;

            while ((data = await fsIn.ReadAsync(buffer, 0, buffer.Length)) > 0)
            {
                cs.Write(buffer, 0, buffer.Length);
                i += buffer.Length;
                number = (int)(i / fsIn.Length * 100);
                ((IProgress<int>)progres).Report(number);
            }
            fsIn.Close();
            cs.Close();
            fsCrypt.Close();

            byte[] hash = Hashing.HashFile(outputFile + fileName, privateKey);
            File.WriteAllBytes(outputFile + "\\Hashing.txt", hash);
        });
}

Multithreaded Approach

Since you are using WinForms you can use BackgroundWorker.  BackgroundWorker will automatically use threads from the ThreadPool, and importantly, they have a notification event for when the thread completes.

There are several ways this could be done, and also keep in mind that I only have moderate experience with multithreading in C#.

You will need to create several BackgroundWorkers.  I would start with 4 and adjust from there. We can also simplify reassembling the blocks by writing them to the output file as they complete.

Here is a high level overview for what you should do:

Put the workers in a list of available threads.
In the main thread, check if there is a free worker in the list.

If Yes, read the next block from the file, pass it and the block index to the worker and start it. Remove the worker from the list
If No, then sleep.

After the worker has completed, it will call the RunWorkerCompleted event, where it will return its result (which should be the encrypted block and the block index.
The event handler should write the encrypted block to the corresponding index in the output file, and then add the worker back to the list.
Repeat until all the blocks have been processed.

Some Notes

The RunWorkerCompleted event may run on the background thread (or you may get errors about it) and we don't want that.  First, any updates to the UI, like updating the progress after a block is written, have to happen on the main UI thread.  Secondly, we don't want multiple threads trying to write to the output file at the same time.  There is a simple trick to get the RunWorkCompleted event to run on the main UI thread.

private void bgworker_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
{
    //if we aren't on the main thread, recursively call on main thread
    if(this.InvokeRequired)
    Invoke( ( MethodInvoker ) delegate {
        bgworker_RunWorkerCompleted(sender, e);
        return;  // don't forget to return or this method will do it twice
    });

   //Write to file
   //Update Progress
   //Add worker back to list
}

The BackgroundWorker.RunWorkerAsync() and RunWorkerCompletedEventArgs only accept/return a single object and you need to pass two (the block index and the block itself).  You will need to either use a tuple/struct/class or something else to hold both objects when passing back and forth.

Finally, You may need special handling for the last block of the file.  Some encryption algorithms (zero) pad a short block to the expected length (1K in this case).  This means that your encrypted file may be slightly longer than your unencrypted file. If this is the case for your chosen algorithm, your allocated output file size may be slightly longer than the original input.  Depending on the file contents, this may or may not be an problem, however you should be aware of it as a potential issue.

Update: What I mean by Block Index

Let's say a file is 10K.  The first block has index 0 and is 1K in length.  The second block has index 1, the third index 2, etc.  The file has 10 blocks with indexes 0-9.

Now lets say that I queue things up and block index 4 finishes first.  In my output file I seek to position 1000*blockIndex, or 4000 and write the 1K bytes.  Then let's say that block 2 finishes.  I seek to position 2000 and write the 1K.  My output file now looks like [0|0|*|0|*|0|0|0|0|0] where '*' denotes valid data and '0' denotes empty/nothing written.

For performance purposes, I'd probably let the UI thread open the output file at the beginning and only close it after the last block is written.  You know the file is done when all of your workers are sitting in the list.  (Workers also have a property to check their status :IsBusy)

Links

Simple tutorial on BackgroundWorker
BackgroundWorker documentation


Sieve of Eratosthenes in Python 3 Currently, the code will generate a range of natural numbers from 1 to N and store it in a list. Then the program will iterate through that list, marking each multiple of a prime as 0 and storing each prime in a secondary list.

This is a translation of a TI-Basic program, shown here, so the same description still applies.

Here's an animation:

One of the main optimizations that I want to see implemented would be the actual removal of prime multiples from the list instead of simply setting them to 0. As it is currently written, this would be a challenge to perform without adding in another if statement. Any suggestions would be helpful.

def sieve(end):
    prime_list = []
    sieve_list = list(range(end+1))
    for each_number in range(2,end+1):
        if sieve_list[each_number]:
            prime_list.append(each_number)
            for every_multiple_of_the_prime in range(each_number*2, end+1, each_number):
                sieve_list[every_multiple_of_the_prime] = 0
    return prime_list

Here's a link to PythonTutor, which will visualize the code in operation.

Just a couple things:

sieve_list = list(range(end+1))

You don't actually need your list to be [0, 1, 2, ... ]. You just need an indicator of whether or not it's true or false. So it's simpler just to start with:

sieve_list = [True] * (end + 1)

That'll likely perform better as well.

When you're iterating over multiples of primes, you're using:

range(each_number*2, end+1, each_number)

But we can do better than each_number*2, we can start at each_number*each_number. Every multiple of that prime between it and its square will already have been marked composite (because it will have a factor smaller than each_number). That'll save a steadily larger increment of time each iteration.

As an optimization, we know up front that 2 and 3 are primes. So we can start our iteration at 5 and ensure that we only consider each_number to not be multiples of 2 or 3. That is, alternate incrementing by 4 and 2. We can write this function:

def candidate_range(n):
    cur = 5
    incr = 2
    while cur < n+1:
        yield cur
        cur += incr
        incr ^= 6 # or incr = 6-incr, or however

Full solution:

def sieve(end):
    prime_list = [2, 3]
    sieve_list = [True] * (end+1)
    for each_number in candidate_range(end):
        if sieve_list[each_number]:
            prime_list.append(each_number)
            for multiple in range(each_number*each_number, end+1, each_number):
                sieve_list[multiple] = False
    return prime_list

Impact of various changes with end at 1 million, run 10 times:

initial solution       6.34s
[True] * n             3.64s (!!)
Square over double     3.01s
candidate_range        2.46s

Also, I would consider every_multiple_of_the_prime as an unnecessary long variable name, but YMMV.