PHP dynamic insert - mysql. Is there a better way than this? I have a dynamic form that users can add extra fields too. The group of fields are added and the input name stays the same but gets a incremental number attached to it. So for my insert php file I just check to see if the next value is set and if so it inserts all the required info. But this code doesn't look very 'streamlined' to me. Is there a better way?

mysql_select_db("inventory", $con);

if( !isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]')";
}

if( isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]')";
}

if( isset($_POST['productid3']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]'),
('$_POST[productid3]','$_POST[type3]','$_POST[client3]','$_POST[product3]','$_POST[color3]','$_POST[quantity3]')";
}

    // And I have like this 5 more times, the code gets longer each time!

Array based approach

Instead of appending a number to the field, use an array based approach:

<form action="" method="POST">
    Product 1<br
    Product Id <input type="text" name="products[0][id]">

    Product Type <input type="text" name="products[0][type]">

    Product 2

    Product Id <input type="text" name="products[1][id]">

    Product Type <input type="text" name="products[1][type]">
</form>

Now if you were to submit this to PHP and do print_r($_POST['products']);, you would get something like the following:

array(
    0 => array(
        'id' => '...',
        'type' => '...',
    ),
    1 => array(
        'id' => '...',
        'type' => '...',
    )
)

This allows you to do the logic for the insertion once, and then use a loop to do all of the fields.

The (pseudo) code would be like:

$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();

//Note that I've assumed the id column is an int, and the type column is a string.
//In real schema, I would expect type to also be an int, but I want to illustrate proper escaping.
//(There is more on this in a later section)
$query = "INSERT INTO products (id, type) VALUES (%d, '%s')";

foreach ($products as $product) {
    //Ensure that $product contains a valid entry, and if it does not, store errors somewhere
    if (valid entry) {
        $res = mysql_query(sprintf($query, (int) $product['id'], mysql_real_escape_string($product['type'])));
        if (!$res) {
            //Log an error about the query failing
        }
    }
}

SQL Injection

Put all the validation on the client side you want, but at the end of the day, it's bypassable.  In fact, a user doesn't even have to use a browser, much less use JavaScript.  A user can send your server anything he wants.  After all, at the end of the day, HTTP is a text based protocol.

Also, properly escaping isn't just about security.  It's also about correctness.

$string = "The other day, someone said, "Hello my name is Fred!""; //Syntax error!

Not escaping SQL is the same concept as this.  This isn't a security issue; it's just wrong.  Escaping SQL is the same concept as doing:

$string = "The other day, someone said, \"Hello my name is Fred!\"";

The escaping is necessary so that the SQL server can parse the query correctly in the same way that escaping in PHP is necessary so that the PHP engine can parse the script correctly.

The reason that SQL injection is a security concern is user input.  If you don't properly escape, users can change your code.

It's hard to make a brief PHP-injection example, but hopefully the concept is there.  An attempt at an example:

eval("\$x = 5 + {$_POST['input']};");

That's all good and fine as long as $_POST['input'] === "5"; or some other kind of innocent value.  However, consider if $_POST['input'] === "5; evilFunction()".  Suddenly the eval is:

eval("\$x = 5 + 5; evilFunction();");

This is the broad concept of SQL injection.  (Various articles on the internet will explain it much better than me though :p.)

(eval should very rarely [arguably, never] actually be used -- this is just to illustrate a point)

Anyway, the solution is very simple.  Anything that is going to be a string needs to be run through mysql_real_escape_string before being interpolated into a query.  Likewise, anything that is not a string should be handled as the proper type:

$query = "INSERT INTO tbl (stringField, intField) VALUES ('%s', '%d')";
$qry = sprintf($query,
        mysql_real_escape_string($someString), //make sure the string field is properly escaped
        (int) $someInt //make sure the int field is an int
       );

PDO

I would advise everyone to move away from the old mysql extension.  The mysql_* functions are a very thin wrapper around the C API, and though there is not anything particularly wrong with them, PDO is just better.

In particular, PDO:

Offers a cleaner API
Offers better error handling (the ability to have failed queries throw exceptions is wonderful)
Handles transactions SQL-dialect agnostically (for the most part)
Different RDBMS can be changed out relatively easily (switching from mysql_* to Postresql is extremely painful.  Switching from PDO with MySQL to PDO with Postresql isn't pleasant, but is care is taken to stay as dialect-agnostic as possible, it's reasonably easy)
Prepared statements

Prepared statements deserve their own sublist:

100% safe from SQL injection (if used correctly, of course)
Better performance for batched queries
Various other minor things

Prepared statements work vaguely like:
* Create a parameterized statement
* Let the SQL server handle putting the variables into place

The second point is the important one.  Since the server is putting the paramters into place, it's impossbile for something to be misinterpretted.

An example will be a lot more useful:

//create the connection
$db = new PDO("mysql:host=localhost;dbname=database_name", "username", "password");
//I like for failed queries to throw exceptions.
//If you're not comfortable with exception handling, you would want to omit the following line.
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();

//Create the prepared statement -- in a way this is like having SQL create a temporary function
$insertStmt = $db->prepare("INSERT INTO products (id, type) VALUES (:id, :type)");

foreach ($products as $product) {
    //Ensure that $product contains a valid entry, and if it does not, store errors somewhere
    if (valid entry) {
        //Execute the statement -- this is similar to calling the temporary function
        //Note that a new array is created in case $product contains an extra key.  For example,
        //if there were a $product['extra'] field, then using $insertStmt->execute($product) would
        //cause an error since there would be an extra paramater that MySQL would not know what
        //to do with.
        $insertStmt->execute(array(
            'id' => $product['id'],
            'type' => $product['type']
        ));
    }
}

Don't assume user input is there

Never assume anything from the client side.  What I mean in this particular instance is to never assume a key exists in $_POST, $_GET, $_COOKIE, etcetra.

Just because isset($_POST['productid2']) is true does not mean that the type2 key for $_POST is set.  This goes back to the note earlier about people being able to send your client whatever they want.

I could make your script issue all kinds of index undefined and mysql_query() errors if I wanted by making a form like the following:

<form action="http://yoursite.tld/somepage.php" method="POST">
    <input type="text" name="productid2" value="let there be errors!">
    <input type="submit">
</form>

My approach is to be paranoid on user provided values.  Something intersting to note is that $_GET/$_POST/$_COOKIE/$_REQUEST values will always be either a string or an array (unless somewhere in your script you have something like: $_POST['int'] = 5;).  This means that if you're not expecting an array, you should make sure you're getting a string.

$type = (isset($_POST['type']) && is_string($_POST['type'])) ? $_POST['type'] : null;
if ($type === null) {
    echo 'Please provide a type';
} else {
    //do some other validations on $type
}

If you want to be a bit more flexible, you could use is_scalar instead of is_string.

The broad idea is that you never want to access an array by key $k unless you are certain that array_key_exists($k, $arr) is true.  When $arr is something user provided, you can never be certain of that unless you explicitly check it.


Converting an already-uploaded file and saving it to a model's FileField Please review this:

from os import path, remove

try:
    video = Video.objects.get(id=original_video_id)
except ObjectDoesNotExist:
    return False

convert_command = ['ffmpeg', '-i', input_file, '-acodec',
       'libmp3lame', '-y', '-ac', '2', '-ar',
       '44100', '-aq', '5', '-qscale', '10',
       '%s.flv' % output_file]

convert_system_call = subprocess.Popen(
    convert_command,
    stderr=subprocess.STDOUT,
    stdout=subprocess.PIPE
)

logger.debug(convert_system_call.stdout.read())

try:

    f = open('%s.flv' % output_file, 'r')
    filecontent = ContentFile(f.read())
    video.converted_video.save('%s.flv' % output_file,
                     filecontent,
                     save=True)
    f.close()

    remove('%s.flv' % output_file)

    video.save()

    return True
except:
    return False

Clean, easy to understand code. However:

Never hide errors with a bare except. Change

try:
    ...
except:
    return False

into

try:
    ...
except (IOError, AnyOther, ExceptionThat, YouExpect):
    logging.exception("File conversion failed")

Also, unless you need to support Python 2.4 or earlier, you want to use the files context manager support:

with open('%s.flv' % output_file, 'r') as f:
    filecontent = ContentFile(f.read())
    video.converted_video.save('%s.flv' % output_file,
                               filecontent,
                               save=True)
remove('%s.flv' % output_file)

That way the file will be closed immediately after exiting the with-block, even if there is an error. For Python 2.5 you would have to from __future__ import with_statement as well.

You might also want to look at using a temporary file from tempfile for the output file.


Finding average of eight immediate neighbors of a matrix I have this basic Java code to find average of eight immediate neighbors of a matrix.

Is there any way to simplify or merge any part of it, or can I refactor it?

I'm a beginner in Java programming and am trying to improve myself.

// CODE STSRT
import java.util.Scanner;
import java.lang.*;

class Matrixer
{

   double[][] matrix, computedMatrix;
    final int rows, cols;

    public Matrixer(int N, int M, double[][] imatrix)
    {
        rows = N;
        cols = M;
        matrix = imatrix;
        computedMatrix = new double[N][M];
    }

    public void computeAverages()
    {
        for (int i = 1; i < rows - 1; i++)
        {
            for (int j = 1; j < cols - 1; j++)
            {
                computedMatrix[i][j] = cellNeighborsAverage(i, j);
            }
        }
    }

    private double cellNeighborsAverage(int row, int col)
    {
        // Ignore center cell And tack Neighbors
        double sum = matrix[row - 1][col - 1] + matrix[row - 1][col]
            + matrix[row - 1][col + 1] + matrix[row][col - 1]
            + matrix[row][col + 1] + matrix[row + 1][col - 1]
            + matrix[row + 1][col] + matrix[row + 1][col + 1];
        return sum / 8;
    }

    public void printComputedMatrix()
    {
        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < cols; j++)
            {
                System.out.printf("%.0f", computedMatrix[i][j]);
                System.out.print(" ");
            }
            System.out.println();
        }
    }

public static void main(String[] args) throws NullPointerException {
        //This part for defined the size if matrix
        int R,C;
        Scanner rc = new Scanner(System.in);
        System.out.println("Enter the number of matrix's rows :");
        R = rc.nextInt();
        Scanner cc = new Scanner(System.in);
        System.out.println("Enter the number of matrix's columns :");
        C = cc.nextInt();
        //End of defined part

        // condition part To see if columns and rows not greater than 2 stop execution
  if(R<3){System.out.println("ERROR! \n Columns and rows should be greater than 2");
  }else{
    // condition part To see if columns not equal rows stop execution
    if(R!=C){
    System.out.println("ERROR! \n The number of rows should equal number of columns");
    }
        else{

    Scanner sc = new Scanner(System.in);
    double[][] inputMatrix = new double[R][C];
    try {

        System.out.println("Enter The Elementt of matrix ("+R+"x"+C+")");
        for (int i = 0; i < R; i++) {
            for (int j = 0; j < C; j++) {

                inputMatrix[i][j] = sc.nextInt();
            }
        }
        Matrixer mx = new Matrixer(R, C, inputMatrix);
        System.out.println(" average of eight immediate neighbors is \n");
        mx.computeAverages();
        mx.printComputedMatrix();

    } catch (NullPointerException e) {
        e.printStackTrace();
    }
} } //End of conditions part
}}

//END OF THE CODE

Welcome to CodeReview!

Class naming

Maybe it's a term I'm not familiar with but Matrixer does not seem to ring a bell. Is it supposed to identify a "creator of matrixes"? Is it the name of your project? If you haven't made a conscious decision for this name, I would suggest turning it into something "universally" descriptive like MatrixCalculator (pretty much anything that contains the essence of what your class is supposed to achieve).

Indentation

Several places are indented incorrectly/inconsistently. Use a tab (4 spaces) for each new block level.

I would also layout code like your cellNeighborsAverage like this:

double sum = matrix[row - 1][col - 1] + matrix[row - 1][col]
           + matrix[row - 1][col + 1] + matrix[row][col - 1]
           + matrix[row][col + 1] + matrix[row + 1][col - 1]
           + matrix[row + 1][col] + matrix[row + 1][col + 1];

It's just a bit easier to read.

Likewise for your main method: the indentation is all over the place; maybe something went wrong when copy-pasting it here? Keep your if statements in this format:

if(condition) {
    doSomething();
} elseif (anotherCondition) {
    doAnotherThing();
} else {
    doNothing();
}

Parameter naming

Your constructor takes variables N and M in a scientific-oriented context this is an acceptable practice so it will depend on how you view this code but standard conventions state that variables use the lowerCamelCase convention.

I would also make their meaning clearer: N could become rows and M would be columns.

Likewise your parameter is named imatrix. What does that i signify? You can simply call it matrix and use this.matrix to refer to the instance variable inside the constructor body.

I would also distinguish between matrix and computedMatrix. Computed what exactly? Maybe sourceMatrix and averagedMatrix are more appropriate terms?

Method naming

cellNeighborsAverage doesn't really tell me much about what goes on in there. Something about the neighbours average, that's for sure. But what exactly does it do? Does it calculate it? Return it? Do I have to set it?

Conventions state that your method names are built up like [verb][subject] which would become getAverageFromNeighbours.

NullPointerException

The NPE is something you should always prevent and guard against instead of throwing/catching it at runtime. You know the flow of your program and thus you should be able to determine where a NPE could occur. Guard yourself against this by validating parameters, instantiating fields inline when you can, avoid returning null, etc.

Scanners

You only need one Scanner on your System.in which you re-use to get input.

Returning instead of printing

Right now you're working with void methods that print the information directly to the console. This will bring problems when you decide to publish your library (class) online and let people use it. What if that person now wants to write the results to a textfile instead of the console?

Likewise for unit testing this will become troublesome.

Therefore I suggest that you return the results from your methods instead of printing to the console inside Matrixer itself; let the main class decide what happens with your results.


Finding first match in recursive search I am searching a recursive solution space and would like to return the first match I find. (I am using this to solve sudoku, but I guess this would apply to any recursively defined problem or datastructure).

So, in pseudocode, I need to call a function repeatedly with a set of possible input values, and return the first non-nil result:

for each possible input i:
    r = do-something(i)
    if r <> nil:
        return r
return nil

My first attempt was to just translate this into clojure:

(defn find-first
  [func values]
  (loop [v values]
    (if (empty? v)
      nil
      (if-let [ret (func (first v))]
        ret
        (recur (rest v))))))

But whenever I use (loop) it feels like I am programming C and not clojure, so I tried looking at this as applying a function to a sequence and returning the first non-nil result:

(defn find-first-2
  [do-something range]
  (->> range
       (map do-something)
       (remove nil?)
       first))

This looks more like clojure, and should work the same way because of lazy sequences. However, lazy sequences actually chunk items in sets of 32, so when my ranges are shorter than 32 items this isn't really lazy at all.

I have found the "unchunk"-trick creating really lazy sequences, but this also doesn't really "feel" right...

What would be other ways of implementing this?

If the non-nil results of do-something are guaranteed to be logical true, you can easily use some:

  (some pred coll)

  Returns the first logical true value of (pred x) for any x in coll,
  else nil. One common idiom is to use a set as pred, for example
  this will return :fred if :fred is in the sequence, otherwise nil:
  (some #{:fred} coll)

(defn find-first-3
  [do-something range]
  (some do-something range)
)

If you expect do-something to return false as possible, non-nil value, you could use keep:

  (keep f coll)

  Returns a lazy sequence of the non-nil results of (f item). Note,
  this means false return values will be included. f must be free of
  side-effects.

(defn find-first-4
  [do-something range]
  (first (keep do-something range))
)

As for the chunk-issue: You should not bother if you're not hitting any performance issues or other problems. Otherwise, if you really want to get rid of it, the unchunk-trick you mentioned is totally fine IMHO, until Clojure will offer an official, built-in option (maybe it will someday).


Reverse Vowels of a String The task

is taken from leetcode

  Write a function that takes a string as input and reverse only the
  vowels of a string.

  Example 1:

  Input: "hello"

  Output: "holle"

  Example 2:

  Input: "leetcode"

  Output: "leotcede"

  Note: The vowels does not include the letter "y".

My first solution

/**
 * @param {string} s
 * @return {string}
 */
var reverseVowels = function(s) {
  const LEN = s.length;
  const str = [...s];
  const vowels = ['a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U'];
  const rev = [];
  for (let i = 0; i < LEN; i++) {
    if (vowels.includes(s[i])) { rev.push(s[i]); }
  }
  for (let i = 0; i < LEN; i++) {
    if (vowels.includes(str[i])) { str[i] = rev.pop(); }
  }
  return str.join('');
};

My second solution

/**
 * @param {string} s
 * @return {string}
 */
var reverseVowels = function(s) {
  if (s.length <= 1) { return s; }
  const sForward = [...s];
  const sBackward = [...s].reverse();
  const LEN = s.length - 1;
  let left = -1, right = -1;
  const VOWELS = ['a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U'];
  const findVowel = start => (x, i) => start < i  && VOWELS.includes(x);
  while(true) {
    left = sForward.findIndex(findVowel(left));
    right = sBackward.findIndex(findVowel(right));
    if (left >= LEN - right || left === -1 || right === -1) { return sForward.join(''); }

    [sForward[left], sForward[LEN - right]] = [sBackward[right], sForward[left]];
  }
  return sForward.join('');
};

My third solution

/**
 * @param {string} s
 * @return {string}
 */
var reverseVowels = function(s) {
  if (s.length <= 1) { return s; }
  const str = [...s];
  let left = 0;
  let right = s.length - 1;
  const VOWELS = ['a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U'];
  while(left < right) {
    if (VOWELS.includes(str[left]) && VOWELS.includes(str[right])) {
      [str[right], str[left]] = [str[left], str[right]];
      left++;
      right--;
    } else if (VOWELS.includes(str[left])) {
      right--;
    } else {
      left++;
    }
  }
  return str.join('');
};

My fourth solution

/**
 * @param {string} s
 * @return {string}
 */
var reverseVowels = function(s) {
  if (s.length <= 1) { return s; }
  const str = [...s];
  let left = 0;
  let right = s.length - 1;
  const map = new Map();
  map.set('a', true);
  map.set('e', true);
  map.set('i', true);
  map.set('o', true);
  map.set('u', true);
  map.set('A', true);
  map.set('E', true);
  map.set('I', true);
  map.set('O', true);
  map.set('U', true);
  while(left < right) {
    if (map.get(str[left]) && map.get(str[right])) {
      [str[right], str[left]] = [str[left], str[right]];
      left++;
      right--;
    } else if (map.get(str[left])) {
      right--;
    } else {
      left++;
    }
  }
  return str.join('');
};

My fifth solution

/**
 * @param {string} s
 * @return {string}
 */
var reverseVowels = function(s) {
  if (s.length <= 1) { return s; }
  const str = [...s];
  let left = 0;
  let right = s.length - 1;
  const set = new Set(['a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U']);

  while(left < right) {
    if (set.has(str[left]) && set.has(str[right])) {
      [str[right], str[left]] = [str[left], str[right]];
      left++;
      right--;
    } else if (set.has(str[left])) {
      right--;
    } else {
      left++;
    }
  }
  return str.join('');
};

I recommend using the Array.prototype methods.

These lend themselves to adopting a functional style of programming.

Functional programming is a lot easier to test, as a pure function always returns the same result and never mutate objects.

const vowels = ['a','e','i','o','u'];
function vowelStringReverse(str) {

    //split the string into an array of chars, that includes their index position
    const strArray = str.split('').map((v,i) => ({
        value: v,
        i: i,
        isVowel: vowels.includes(v)
    }));

    //For just the vowels,
    //Create index position lookup, where the index positions are reversed
    const vowelsReverseIndexLookup = strArray.filter(v => v.isVowel)
    .reduce((acc, cur, i, array) => {
        return {
            ...acc,
            [array[array.length-1-i].i]: cur.value
        };

    }, {});

    //Now iterate through the string, and look up the vowel from the lookup.
    const strReversed = strArray.map((v,i) => {
        if (v.isVowel) {
            return vowelsReverseIndexLookup[i]
        }
        else {
            return v.value;
        }
    });

    return strReversed.join('');

}

console.log(vowelStringReverse('hello'));
console.log(vowelStringReverse('leetcode'));

In this example I've given, if you wanted, you could then pull out the anonymous functions I've declared, and write tests for them:

for example:

const vowelsReverseIndexLookup = strArray.filter(v => v.isVowel)
.reduce(reverseIndexReducer, {});

//elsewhere:

const reverseIndexReducer = (acc, cur, i, array) => {
    return {
        ...acc,
        [array[array.length-1-i].i]: cur.value
    };

};


Database output function Please review the following code and list all the coding errors and poor coding practices that you can see in this code.

function output()
{
    // Check authorization
    if(is_authorized())
    {
        $authorized = true;
        include('/path/to/' . $_REQUEST['module'] . '.php');
    }
    echo "
"
    $conn = mysql_connect( "dbserver.com:6789", "dbyuser", "dbpassWd" );
    mysql_select_db( "kum", $conn ); // selects a database
    $q = " SELECT * FROM main WHERE id > " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        echo "
".$row['description']."
";
    }
    echo "

";
    $q = " SELECT * FROM main WHERE id < " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        // Display the status if it is authorized, otherwise display N/A
        echo "
".$row['description']. "(" .
        $authorized ? $row['status'] : "N/A" . ")
";
    }
    echo "
";
}

magic strings,
DB connection not closed,
DB errors not handled,
no prepared statements with safe query parameters,
injectable request and query parameters,
mix of UI and logic code,
no HTML encoding,
2 of the 3 comments are very redundant with the line they comment.


Optimize iteration output to text file I've written a piece of code which combines given letters in groups of n letters, and writes all the combinations in a text file. This is my first attempt and I wonder if there is an optimal way to do this.

letter_input = raw_input(' Type in letters: ')

number_input = int(raw_input(' Letters per word: '))

split_letters = list(letter_input)

def function():

    all_combinations_list = list(itertools.permutations(split_letters, number_input))

    output_file = open('/tmp/list.txt', 'w')

    number_of_combinations = len(all_combinations_list)

    for n in range(0,number_of_combinations):
        raw_characters = str(all_combinations_list[n])
        out_1 = raw_characters.translate(string.maketrans('',''), string.punctuation)
        out_2 = out_1.replace(' ','')
        print >>output_file, out_2

    output_file.close()

Don't use global variable

It make things hard to understand and to maintain. In your case, the simple way to do this is to feed your function function() the parameters it needs.

def function(letter_input, number_input, split_letters):

Make your code easier to read

You can ensure you respect PEP 8, the guideline to write Python code. You'll find various tools to help you comply to the usual standards : pep8,  pylint, pyflakes, pychecker.

Also, function is a pretty bad function name, let's pick something better (for instance write_permutation).

Use pythonic loops

Python provides you a clean/safe/concise/way to iterate over pretty much anything : for something in something_iterable. Using the loop index is much more verbose on top of being less efficient.

You can change :

all_combinations_list = list(itertools.permutations(split_letters, number_input))
number_of_combinations = len(all_combinations_list)
for n in range(0,number_of_combinations):
    raw_characters = str(all_combinations_list[n])

to (using the default behavior for range)

all_combinations_list = list(itertools.permutations(split_letters, number_input))
number_of_combinations = len(all_combinations_list)
for n in range(number_of_combinations):
    raw_characters = str(all_combinations_list[n])

then

all_combinations_list = list(itertools.permutations(split_letters, number_input))
for e in all_combinations_list:
    raw_characters = str(e)

then

for e in list(itertools.permutations(split_letters, number_input)):
    raw_characters = str(e)

then (because you don't need to create a temporary list) :

for e in itertools.permutations(split_letters, number_input):
    raw_characters = str(e)

Don't use complicated non-required logic

Converting your tuple in a string you perform logic in in way too complicated on top of not being efficient.
You could just do something like (using what we call "tuple unpacking"):

for e in itertools.permutations(split_letters, number_input):
    a, b, c = e
    out_2 = a+b+c

This can even be more concise by doing the unpacking as part of the loop :

for a, b, c in itertools.permutations(split_letters, number_input):
    print >>output_file, a+b+c

Using with

You can use the with keyword to handle ressources that needs to be "opened" and "closed" such as files. In your case, you can write :

with open('/tmp/list.txt', 'w') as f:
    for a, b, c in itertools.permutations(split_letters, number_input):
        print >>f, a+b+c

and you don't need to close the file manually.

Using the if__name__ guard

It is usually a good habit to put your code behind a guard to have your main code running only if we run your script and not if we import it (because one wants to re-use a function for instance).

No need for list as strings are iterables too

As pointed out in codesparkle's comment, you don't need to build a list.

Once this is done, you code looks like :

import itertools

def main():
    """Main function"""
    split_letters = raw_input(' Type in letters: ')
    number_input = int(raw_input(' Letters per word: '))
    with open('/tmp/list.txt', 'w') as f:
        for a, b, c in itertools.permutations(split_letters, number_input):
            print >>f, a+b+c

if __name__ == "__main__":
    main()

As a final note, your code could be written using a single call to print by using join on the list of permutations but this doesn't bring much so I'll skip that part.


benchmarking requests to localhost I am writing a benchmarking tool from scratch in Python.
However I can't get the performance of other benchmarking tools like wrk or wrk2. Using wr2 I can make 42k requests/s while my code can only create up to 2200 reqs/s. I have tried multiple ways to parallelize the code execution. I have tried using multiprocessing and parallel computing libraries like Dask. But I can't get better performance. I understand wrk and wrk2 are written in C which can be one reason but still 42k vs 2200 seems like a very large difference.
I have tried with different number of workers and number_of_request, but the performance does not change much.
I am trying to understand if I am really hitting the upper limit or I am doing something wrong. The server is running on localhost and written in Java Spring.
This is my code using multiprocessing:
import time
import multiprocessing
from collections import Counter, defaultdict
import requests

# import multiprocessing as mp

num_workers = multiprocessing.cpu_count()

output = multiprocessing.Queue()
def runner(number_of_request):
    output=""
    for i in range(number_of_request):
        try:
            output+=str(requests.get("http://127.0.0.1:8000/").text)
        except:
            pass
        # print(output)

    return output

if __name__ == '__main__':
    number_of_request = 1000
    start = time.time()
    pool = multiprocessing.Pool(processes=num_workers)
    outputs = [pool.apply_async(runner, args = (number_of_request,)) for x in range(num_workers)]
    pool.close()
    pool.join()

    duration = time.time() - start
    req_s = (number_of_request*num_workers)/duration
    print("duration =", time.time() - start)
    print(req_s)

Performance notwithstanding, there's some other cleanup that's worth doing:

Capitalize NUM_WORKERS since it's a global constant
Do not shadow your global output with a local variable of the same name; and ideally don't have a global output at all
After your get() and before your call to .text, you need to check whether the request succeeded - either via .ok and a log entry showing the reason; or by .raise_for_status()
never try / except / pass. This is the broadest and most dangerous form of silent exception-swallowing. If runner is executing and the user attempts to terminate the application with a Ctrl+C, that will be ignored here, and all other error information has become invisible to you. Consider at least except Exception: and logging the exception using the standard logging framework.
Consider annotating runner as def runner(number_of_requests: int) -> str
Do not cast the result of .text to a string - it's already a string
Do not use time.time() here; instead use time.perf_counter() for a sufficiently short duration or time.monotonic() otherwise
The parens in (number_of_request*num_workers)/duration are redundant


Better way to assert correct return values in Groovy I have written a function that returns me the duplicates of a list:

static List<SomeClass> findDuplicateElements(List<SomeClass> myObjects) {
    List<SomeClass> duplicates = myObjects
    .groupBy { it.someAttribute }
    .findAll { it.value.size() > 1 }
    .collect { it.value }
    .flatten()

    assert duplicates.size() != 1
    duplicates
}

The list cannot have the size 1 because if no duplicates are found it must be empty else if there are duplicates it must me at least 2 elements.

Without that check I would do it this way without having to create any new variables:

static List<SomeClass> findDuplicateElements(List<SomeClass> myObjects) {
    myObjects
    .groupBy { it.someAttribute }
    .findAll { it.value.size() > 1 }
    .collect { it.value }
    .flatten()
}

When using assert, is there any way to avoid saving the result in the duplicates variable and returning it at the end?

You can use with to do things inline without declaring another variable:

List list = [
  [first: "foo", last: "bar"],
  [first: "baz", last: "qux"],
  [first: "baz", last: "bar"]
]

List findDuplicateElements(myObjects) {
   myObjects
   .groupBy { it.last }
   .findAll { it.value.size() > 1 }
   .collect { it.value }
   .flatten()
   .with {
     assert it.size() != null
     it
   }
}

assert findDuplicateElements(list).size() == 2


Decimal to binary algorithm Is there a better way to code this specific algorithm using successively division through 2 which converts decimal numbers into binary numbers without calling a built-in function?

print "Please enter a decimal number."
prompt = '>> '
dec_number = input(prompt)
dec_new = dec_number
binstr = ''

'''dec2bin'''
while dec_new != 0:
  binstr += str(dec_new % 2)
  dec_new = dec_new // 2

print binstr[::-1]

In general, your idea is not bad.

The best solution, as far as I read, would be the algorithm Divide by 2 that uses a stack to keep track of the digits for the binary result.

As an intro, this algorithm assumes that we start with an integer greater than 0. A simple iteration then continually divides the decimal number by 2 and keeps track of the remainder.

The first division by 2 gives information as to whether the value is even or odd. An even value will have a remainder of 0. It will have the digit 0 in the ones place. An odd value will have a remainder of 1 and will have the digit 1 in the ones place. We think about building our binary number as a sequence of digits; the first remainder we compute will actually be the last digit in the sequence.

That said, we have:

from pythonds.basic.stack import Stack

def divideBy2(decNumber):
    remstack = Stack()

    while decNumber > 0:
        rem = decNumber % 2
        remstack.push(rem)
        decNumber = decNumber // 2

    binString = ""
    while not remstack.isEmpty():
        binString = binString + str(remstack.pop())

    return binString

print(divideBy2(42))

Comments regarding your code:

Why did you do this: dec_new = dec_number ? You could've just use dec_number. There's no need of assigning its value to another variable.
This: dec_new = dec_new // 2 could be as well as the above line of your code rewritten as: dec_new //= 2
The indentation should contain 4 spaces, not 2.

A version that better (than your solution) utilizes the memory would be:

def dec_to_bin(n):
    bits = []

    bits.append(str(0 if n%2 == 0 else 1))
    while n > 1:
        n = n // 2
        bits.append(str(0 if n%2 == 0 else 1))

    bits.reverse()
    return ''.join(bits)

What I did:

floor divide all the numbers by two repeatedly until we reach 1
going in reverse order, create bits of this array of numbers, if it is even, append a 0 and if it is odd append a 1.

Other ways of doing it:

Using recursion:

def dec_to_bin(x):
    return dec_to_bin(x/2) + [x%2] if x > 1 else [x]

The above solution returns the result as a list which you can later on .join() then apply int() to it.

Another idea that came to my mind is as simple as:

u = format(62, "08b")
>> 00111110


Scala implementation of @rolfl's integer-partitioning algorithm While reviewing @Martijn's Distinct sums of integers (finding all combinations of positive integers that sum to a given number), I proposed the following solution:

/** List of non-increasing positive integers */
type Sum = List[Int]

def sums(n: Int): Iterator[Sum] = {
  /** Prepends the head of the stack repeatedly until a total amount has
    * been added.  The leading element of the result is then increased
    * so that the total added is exactly the specified amount.
    */
  @tailrec
  def topUp(amount: Int, stack: List[Int]): List[Int] = {
    if (amount < stack.head)
      (stack.head + amount) :: stack.tail
    else
      topUp(amount - stack.head, stack.head :: stack)
  }

  /** Returns the next partition by popping the first element, increasing the
    * next element, and topping it up to the same total.  Returns None if
    * the input has only one element.
    */
  def advance(partition: Option[Sum]): Option[Sum] = {
    partition.get match {
      case List(n) => None
      case p       => Some(topUp(p.head - 1, p.tail.head + 1 :: p.tail.tail))
    }
  }

  Iterator.iterate(Some(List.fill(n)(1)): Option[Sum])(advance)
          .takeWhile(! _.isEmpty)
          .map(_.get)
}

Example run:

scala> sums(5).foreach(println)
List(1, 1, 1, 1, 1)
List(2, 1, 1, 1)
List(3, 1, 1)
List(2, 2, 1)
List(4, 1)
List(3, 2)
List(5)

I'm pretty satisfied with the algorithm, as it is based on a Java solution that was devised by @rolfl.

However, I'm not happy about the way I've expressed it in Scala, particularly with regards to the termination of the iterator.

Is there a way to write topUp without explicit recursion, yet still avoiding ::: concatenation?
Is the : Option[Sum] type annotation on Some(List.fill(n)(1)) avoidable?
There seems to be a lot of Option / None / Some noise in advance.  Is there a better way to handle the termination condition?  (Perhaps by using lift?)
Similarly, Iterator.iterate(…)(…).takeWhile(! _.isEmpty).map(_.get) seems cumbersome.  Is there a less verbose way to say "repeatedly apply this function until exhausted" using the standard Scala library?

For the record, instead of advance and Iterator.iterate(…).takeWhile(…).map(…), I had previously written it this way, using explicit recursion:

def makeSums(partition: Sum): Stream[Sum] = {
  partition #:: (partition match {
    case List(n) => Stream.Empty
    case p       => makeSums(topUp(p.head - 1, p.tail.head + 1 :: p.tail.tail))
  })
}

makeSums(List.fill(n)(1))

… which returned a Stream[Sum] instead of an Iterator[Sum].  Note that it's not tail-recursive.  (I also tried to write it as a self-referential stream, but couldn't figure out how to terminate it.)

I think that, despite being longer, the Iterator.iterate(…) solution is better because it decouples the "repeated application" aspect from the "produce the next result" aspect.  Your opinion on this matter would be appreciated.

Keep in mind that I likely know less about scala than you and that this is my first review ;)

  Is there a way to write topUp without explicit recursion, yet still avoiding ::: concatenation?

I don't think so, but I may be wrong.

  Is the : Option[Sum] type annotation on Some(List.fill(n)(1)) avoidable?

Yes! You can use the Option type constructor like this:

Iterator.iterate(Option(List.fill(n)(1)))(advance)

  There seems to be a lot of Option / None / Some noise in advance. Is there a better way to handle the termination condition? (Perhaps by using lift?)

I think so, but not with lift. This just uses the standard map and filter (please let me know if this is incorrect logic).

def advance(partition: Option[Sum]): Option[Sum] =
   partition.filter(_.size > 1)
            .map(p => topUp(p.head - 1, p.tail.head + 1 :: p.tail.tail))

  Similarly, Iterator.iterate(…)(…).takeWhile(! _.isEmpty).map(_.get) seems cumbersome. Is there a less verbose way to say "repeatedly apply this function until exhausted" using the standard Scala library?

I really don't know.

One minor point. Here

Iterator.iterate(Some(List.fill(n)(1)): Option[Sum])(advance)
        .takeWhile(! _.isEmpty)
        .map(_.get)

You use ! _.isEmpty, but option has a perfect method for you, isDefined.


Adding items in a list I wrote a function that adds items in a list, but my professor says to try and write it in a simpler way because he says I take too many steps. Is there any other way to write this?

def additems(self, item):
    """Add <item> to this PriorityQueue.

    @type self: PriorityQueue
    @type item: object
    @rtype: None

    >>> pq = PriorityQueue()
    >>> pq.additems("yellow")
    >>> pq.additems("blue")
    >>> pq.additems("red")
    >>> pq.additems("green")
    >>> pq._items
    ['blue', 'green', 'red', 'yellow']
    """
    # TODO
    #only insert if the list isn't already empty
    if not self.is_empty():

        i = 0

        #Look for the lowest priority place to insert the item
        while i < len(self._items):

            if item <= self._items[i]:
                self._items.insert(i, item)

                #exit the loop if it inserts
                i = len(self._items)

            elif item >= self._items[-1]:
                self._items.append(item)

                #exit the loop if it inserts
                i = len(self._items)

            i += 1

    else:
        self._items.append(item)

You don't present the entire code for the class here, but the usage of self and the doctests within the docstring indicates that you are actually using a class. As such my first order would be that in the initialisation of the class you always create the _items list, which would remove the need to check whether there are elements or not when adding elements.

To use i = len(self._items) to terminate the loop is a hackish solution. It would be better to use break instead. But while and for (and actually try ... except) allows for the usage of else which can help out in this particular case. In short, if the loop terminates ordinarily, the else part is executed, if you break out of it, it'll not be executed.

In stead of doing the while i < len(self._items) loop, which requires calculating the length multiple times, and doesn't look to Pythonic, I would suggest using for i, item in enumerate(self._items). This loop construct loops over all the elements (if any) and gives the index of the current element (due to the use of enumerate()).

Lastly, naming the method additems() when you only add one element is misleading and not following standards. A better name would be add_item().

This leads to the following code:

class PriorityQueue(object):

    def __init__(self):
        self._items = []

    def add_item(self, new_item):
        """Add items into queue at correct place."""

        for i, item in enumerate(self._items):
            if new_item < item:
                self._items.insert(i, new_item)
                break
        else:
            self._items.append(new_item)

def main():
    pq = PriorityQueue()
    pq.add_item("yellow")
    pq.add_item("blue")
    pq.add_item("red")
    pq.add_item("green")

    print(pq._items)

if __name__ == '__main__':
    main()

I'm sorry that the environment I tested this in doesn't support doctests, but this does output the correct list of ['blue', 'green', 'red', 'yellow'].


Salted hash generator I've had a go at creating a small class in C# that can generated salted hashes from text. I would like to know how this could be improved (in terms of code style) and whether or not this code is secure enough that it could be used in a professional environment (it won't be).

Hasher.cs

using System.Security.Cryptography;

namespace HashAndSaltTest
{
    public static class Hasher
    {
        private static readonly int MaxSaltLength = 32;

        public static byte[] GenerateSaltedHash(byte[] plainText)
        {
            HashAlgorithm algorithm = new SHA256Managed();
            byte[] salt = GenerateSalt();
            byte[] saltedText = new byte[plainText.Length + salt.Length];

            for (int i = 0; i < plainText.Length; i++)
                saltedText[i] = plainText[i];

            for (int i = 0; i < salt.Length; i++)
                saltedText[plainText.Length + i] = salt[i];

            return algorithm.ComputeHash(saltedText);
        }

        private static byte[] GenerateSalt()
        {
            byte[] salt = new byte[MaxSaltLength];

            using (RandomNumberGenerator random = new RNGCryptoServiceProvider())
                random.GetNonZeroBytes(salt);

            return salt;
        }
    }
}

Because HashAlgorithm implements IDisposable you should enclose the usage of it in an using block as well.
Omitting braces {} although they might be optional can lead to hidden and therfor hard to find bugs. I would like to encourage you to always use braces.
Instead of using a for loop to copy plainText to saltedText and salt to saltedText you may take advantage of the Array.CopyTo() method.
Public methods should validate passed parameters.
Because you don't return the salt you can only generate a hash but you can't verify the hash.

Applying the mentioned points can lead to

public static byte[] GenerateSaltedHash(byte[] plainText)
{
    if (plainText == null) { throw new ArgumentNullException("plainText"); }
    if (plainText.Length == 0) { throw new ArgumentException("Length may not be zero", "plainText"); }

    using (HashAlgorithm algorithm = new SHA256Managed())
    {
        byte[] salt = GenerateSalt();
        byte[] saltedText = new byte[plainText.Length + salt.Length];

        plainText.CopyTo(saltedText, 0);
        salt.CopyTo(saltedText, plainText.Length);

        return algorithm.ComputeHash(saltedText);
    }
}

private static byte[] GenerateSalt()
{
    using (RandomNumberGenerator random = new RNGCryptoServiceProvider())
    {
        byte[] salt = new byte[MaxSaltLength];
        random.GetNonZeroBytes(salt);
        return salt;
    }
}


Creating view rects in loop I don't use constraints or IB to create views so it's easier to create rects this way. Does it wrong totaly?

@interface Client ()

@property (nonatomic, strong) UITextField *name;
@property (nonatomic, strong) UITextField *phoneNumber;
@property (nonatomic, strong) UITextField *address;
@property (nonatomic, strong) UIButton *submit;

@end

@implementation ClientAddVC

- (void)viewDidLoad {
    [super viewDidLoad];
    [self createSubviews];
    [self createViewsRects:@[_name, _phoneNumber, _address, _submit]];

}

- (void)createSubviews{

    _name = [Utility textFieldWithPadding:@"Name" frame:[Utility rectForFormFields]];
    _name.delegate = self;
    _phoneNumber = [Utility textFieldWithPadding:@"Phone Number" frame:[Utility rectForFormFields]];
    _phoneNumber.keyboardType = UIKeyboardTypePhonePad;
    _phoneNumber.delegate = self;
    _address = [Utility textFieldWithPadding:@"Address" frame:[Utility rectForFormFields]];
    _address.delegate = self;

    _submit = [UIButton buttonWithType:UIButtonTypeCustom];
    [_submit setTitle:@"Submit" forState:UIControlStateNormal];
    [_submit addTarget:self action:@selector(saveClient) forControlEvents:UIControlEventTouchUpInside];
    _submit.backgroundColor = [[Cache sharedCache]blackColor];
    _submit.frame = [Utility rectForFormFields];

    [self.view addSubview:_name];
    [self.view addSubview:_phoneNumber];
    [self.view addSubview:_address];
    [self.view addSubview:_specialRequest];
    [self.view addSubview:_averageOrderSize];
    [self.view addSubview:_submit];
}

- (void)saveClient{
}

- (void)textFieldDidBeginEditing:(UITextField *)textField{
    _currentField = textField;
}

- (void)createViewsRects:(NSArray *)views{
    CGFloat padding = 16;
    CGFloat rowPadding = 8;
    CGFloat totalHeight = padding;

    for (UIView *view in views) {
        CGRect viewRect = view.frame;
        viewRect.origin.x = padding;
        viewRect.origin.y = totalHeight;
        view.frame = viewRect;
        totalHeight += (view.frame.size.height + rowPadding);
    }
    self.bgView.contentSize = CGSizeMake(self.view.frame.size.width, totalHeight);
}
@end

We should be using auto-layout.  Not being as comfortable with auto-layout is not an excuse to not use it.  Manually setting rects is not very good at all.  Manually set rects don't play well with rotation.  Manually set rects mean we probably need a special set of code for iPad versus iPhone.  Manually set rects mean we probably need a special set of code for iPhone 4 vs iPhone 5 vs iPhone 6 vs iPhone 6+.

I'm not really going to touch the createSubviews method, which has some issues that should be address, but I do want to point out the problems with createViewRects: and how we can more easily write this code with auto-layout and using the Visual Format Language quite simply.

The method sets up the position for four views.  The spacing follows two rules:

Each view has an X of 16 pixels from the left edge.
Each view has a Y that leaves a gap of 8 pixels between the other views (plus 8 pixels between the top of the super view and the top of the top view and 8 pixels between the bottom of the super view and the bottom of the bottom view).

For this, I'm going to assume that bgView is a scroll view.  Scroll view's content doesn't really work particularly well with multiple top-level subviews.  Instead, we give the bgView a single subview and add all of the subviews to that subview.  This will allow auto-layout to work well.

Let's define a couple dictionaries to use with VFL.

For starters, so we can put them in a dictionary, let's use NSNumber objects rather than CGFloat for our metrics:

NSNumber *padding = @16.0;
NSNumber *rowPadding = @8.0;

We don't need a total height.  Auto-layout will resolve this for us.  Anyway, creating a dictionary is as simple as this:

NSDictionary *metrics = NSDictionaryOfVariableBindings(padding, rowPadding);

Now, using a mutable string, a mutable dictionary, let's set up these constraints:

int viewIndex = 0;
NSMutableString *vertConstraintsStr = [@"V:|-[rowPadding]-" mutableCopy];
NSMutableDictionary *views = [NSMutableDictionary dictionary];

UIView *backgroundView = [[UIView alloc] init];

for (UIView *view in views) {
    NSString *viewKey = [NSString stringWithFormat:@"view%i", viewIndex];
    ++viewIndex;

    [backgroundView addSubview:view];

    views[viewKey] = view;

    view.translatesAutoresizingMaskIntoConstraints = NO;

    [vertConstraintsStr addFormat:@"[%@]-[rowPadding]-", viewKey];

    NSString *horizConstraintStr =
        [NSString stringWithFormat:@"H:|-[padding]-[%@]", viewKey];

    NSArray *horizontalConstraints =
        [NSLayoutConstraint constraintsWithVisualFormat:horizConstraintStr
                                                options:0
                                                metrics:metrics
                                                  views:views];

    [backgroundView addConstraints:horizontalConstraints];
}

[vertConstraintsStr addString:@"[rowPadding]-|"];

NSArray *verticalConstraints =
    [NSLayoutConstraint constraintsWithVisualFormat:vertConstraintStr
                                            options:0
                                            metrics:metrics
                                              views:views];

[backgroundView addConstraints:verticalConstraints];

[self.bgView addSubview:backgroundView];

Ultimately, this is more lines of code, but this is much more flexible.

Consider what happens if we were to change the size of one of our subviews.  With your code, we must be sure to call createViewsRect: each time any view changes sizes in order to ensure everything still looks right.  With auto layout, the view will automatically adjust to the new size of one of the subviews.  Plus, now you've got some experience working with auto layout so you'll be less intimidated and more comfortable with it in the future, particularly when you want to start doing more complicated layouts.


Determining triangle type from three integer inputs The logic is simple, but I was wondering if anyone could help with rewriting the conditional logic. Also, the only error conditions I can think of are the sides should not be equal to or less than zero. Are there any other error conditions that seem to occur to you?

public class TriangleType {

    static Triangle getType(int a, int b, int c)
    {
        if(a<=0||b<=0||c<=0)
            throw new IllegalArgumentException("Length of sides cannot be equal to or less than zero");

        if(a==b&&b==c&&c==a)
            return Triangle.EQUILATERAL;
        else if((a==b)||(b==c)||(c==a))
            return Triangle.ISOSCELES;
        else if(a!=b&&b!=c&&c!=a)
            return Triangle.SCALENE;
        else
            return Triangle.ERROR;
    }

    public static void main(String[] args)
    {
        System.out.println(TriangleType.getType(13, 13, 0));

    }
}

enum Triangle
{
ISOSCELES(0),
EQUILATERAL(1),
SCALENE(2),
ERROR(3);

private int n;
Triangle(int n)
{this.n = n;}
}

You could encapsulate the conditions in a PotentialTriangle class. Note that the fields are of type long to avoid overflow when calculating whether triangle inequality is violated.

public class PotentialTriangle {
    private final long a, b, c;

    public PotentialTriangle(int sideA, int sideB, int sideC) {
        a = sideA;
        b = sideB;
        c = sideC;
    }

    public boolean isAnySideTooShort() {
        return a <= 0 || b <= 0 || c <= 0;
    }

    public boolean violatesTriangleInequality() {
        return a > b + c || b > a + c || c > a + b;
    }

    public boolean areSidesEqual() {
        return a == b && b == c;
    }

    public boolean areAtLeastTwoSidesEqual() {
        return a == b || b == c || c == a;
    }
}

This would vastly simplify your enum selection code, which I would put directly into the enum:

public enum TriangleType {
    ISOSCELES,
    EQUILATERAL,
    SCALENE;

    public static TriangleType ofPotentialTriangle(PotentialTriangle triangle) {
        throwIf(triangle.isAnySideTooShort(),
                "Length of sides cannot be equal to or less than zero");
        throwIf(triangle.violatesTriangleInequality(),
                "Sum of any two sides must be larger than the remaining side");

        if (triangle.areSidesEqual()) {
            return EQUILATERAL;
        }
        if (triangle.areAtLeastTwoSidesEqual()) {
            return ISOSCELES;
        }
        return SCALENE;
    }

    private static void throwIf(boolean condition, String message) {
        if (condition) {
            throw new IllegalArgumentException(message);
        }
    }
}


Size improvements for cat reimplementation I have written a reimplementation of cat that will be compliant with most scripts, and that is as small and fast as possible. Can anybody suggest some size improvements for it?

#include <stdio.h>              /* we need printf, fprintf, FILE,
                         * fopen, fclose and that sort of
                         * stuff */
#include <stdlib.h>             /* for exit, EXIT_SUCCESS, and
                         * EXIT_FAILURE */

int main(int argc, char *argv[])
{
    FILE *fp;               /* file to cat */
    short int ch;               /* character to print */
    unsigned short int cp;          /* currently printing argv[?] */
    unsigned short int failed = 0;      /* how many failed to print */

    if (argc < 2) {             /* if not enough args act as pipe */
        for (;;) {          /* infinite loop */
            if ((ch = fgetc(stdin)) == EOF) {
                exit(EXIT_SUCCESS);
            } else {
                printf("%c", ch);
            }
        }
    }

    for (cp = 1; cp <= argc - 1; cp++) {    /* for each file in cli args */
        fp = fopen(argv[cp], "r");  /* open the file */

        if(fp == NULL) {
            fprintf(stderr, "%s: cannot open %s\n", argv[0], argv[cp]);
            failed++;       /* increment the number of failed */
            continue;       /* go back to beginning */
        }

        while ((ch = fgetc(fp)) != EOF) {
            printf("%c", ch);
        }

        fclose(fp);         /* close the file */
    }

    exit(failed);               /* exit with the number of files */
}                       /* that failed */

I'm sorry that the indentation is messed up, I use tabs in my code.

In clang version 3.5.0 on Ubuntu the generated binary size is 5200 bytes with these options:

-Oz -fomit-frame-pointer -s

Improvements

I added some more error checking, added -u and - and also wrote a function that writes out a file to avoid repeating myself. Also changed short int to int, printf to putchar, and some other things.

#include <stdio.h>              /* we need printf, fprintf, FILE,
                         * fopen, fclose and that sort of
                         * stuff */
#include <stdlib.h>             /* for exit, EXIT_SUCCESS, and
                         * EXIT_FAILURE */
#include <string.h>             /* need strcmp */

void write_file(char *file, char *progname) /* function to print individual file */
{
    FILE *fp;               /* file to write */
    int ch;                 /* temporary variable */

    if (strcmp(file, "-") == 0) {       /* check if using stdin */
        fp = stdin;         /* use standard input */
    } else {
        fp = fopen(file, "r");      /* open file for writing */
    }

    if (fp == NULL) {           /* error in opening file */
        fprintf(stderr, "%s: cannot open %s\n", progname, file);
        exit(EXIT_FAILURE);
    }

    while ((ch = fgetc(fp)) != EOF) {
        if (putchar(ch) == EOF) {
            fprintf(stderr, "%s: can't read %s\n", progname, file);
            exit(EXIT_FAILURE);
        }
    }

    if (ferror(fp) != 0) {          /* IO error, perhaps? */
        fprintf(stderr, "%s: error reading %s\n", progname, file);
        exit(EXIT_FAILURE);
    }

    if (strcmp(file, "-") != 0) {       /* close the file unless stdin */
        fclose(fp);
    }

    return;
}

int main(int argc, char *argv[])
{
    unsigned short int cp = 1;      /* currently printing argv[?] */

    if (argc < 2) {             /* act as pipe */
        write_file("-", argv[0]);
        exit(EXIT_SUCCESS);
    }

    if (strcmp(argv[1], "-u") == 0) {   /* support -u option for SUS compat */
        cp++;               /* increment the argv pointer var */
    }

    for (; cp <= argc - 1; cp++) {      /* for each file in cli args */
        write_file(argv[cp], argv[0]);  /* print the file */
    }

    exit(EXIT_SUCCESS);
}

Do not repeat yourself. Copying stdin doesn't differ from copying any other file. There is no need to special case stdin as you do at lines 15-21.
Avoid naked loops. Every loop does an important job that deserves a name. This is strong enough reason to factor it out as a function:
Expect errors

fgetc may fail. Once fgetc returns EOF, call feof or ferror to see what has actually happened.
printf may fail. Make sure to detect (printf returns -1) and report (analyze errno) errors.

Handle options. At least recognize a standalone - as a fake file name referring to stdin.


Function to find midpoint of two floats where one of the floats might be an empty string There must surely be a nicer way of writing this.

The wrinkle of complexity is that prev_seq or next_seq might be an empty string if the user moves the question to the first or last position, but it could also validly be '0.0' so I can't just rely on 'falsiness'.

I hope it's clear what I'm trying to achieve from the below (with the comments).

How can I improve this?

def update_sequence(self):

    prev_seq = self.request.get('prev_seq')
    next_seq = self.request.get('next_seq')
    question_id = int(self.request.get('id'))
    question = db.get(db.Key.from_path('Questions', question_id))

    try:
        # to slot a question between two numbers we compute the midpoint
        # for, example, to insert a question between 2.1 and 3.4, the
        # question would get the sequence of 2.75
        new_position = (float(prev_seq) + float(next_seq)) / 2
    except ValueError:
        # if we get here, either prev_seq or next_seq was not submitted
        # in the request which means the user pushed the question to the
        # first or last position.
        try:
            float(prev_seq)
        except ValueError:
            # in this case the question has been moved to the first
            # position so we should make:
            # the new position = the next position - 1
            new_position = float(next_seq) - 1
        try:
            float(next_seq)
        except ValueError:
            # in this case the question has been moved to the last position
            # so we should make:
            # the new position = the previous position + 1
            new_position = float(prev_seq) + 1

    question.sequence_float = new_position
    question.put()

def get_new_position(prev_seq, next_seq):

    if next_seq == "":
        new_position = float(prev_seq) + 1
    elif prev_seq == "":
        new_position = float(next_seq) - 1
    else:
        new_position = (float(prev_seq) + float(next_seq)) / 2

    return new_position


Python extended Euclidean algortihm + inverse modulo I programmed the extended Euclidean algorithm together with the inverse modulo because I am making an RSA system from scratch. Any feedback regarding efficiency etc. is welcome :)

    def ext_gcd(a, b):

        a0, a1 = a, b
        x0, x1 = 1, 0
        y0, y1 = 0, 1

        while a1 != 0:
            q = a0//a1
            r, s, t = a1, x1, y1
            a1 = a0 % a1
            x1 = x0 - q*x1
            y1 = y0 - q*y1
            a0, x0, y0 = r, s, t

        return x0, y0, a0

    def inverse_mod(a, mod):
        va, y0, a0 = Math.ext_gcd(a, mod)
        return va % mod

Give your variables more meaningful names reflecting their role in the algorithm.


Nested stream to check the truth condition at first instance Lets say I have  Cars with Features, and I have been given a List of features to look for in the Cars available with me. Though I could have done that using for-loop, what are the suggestions for below? Is this production-worthy?

import java.util.Arrays;
import java.util.List;

import org.apache.commons.collections4.CollectionUtils;

public class LookupTest {

    public static void main(String[] args) {
        Feature ft11 = new Feature(1);
        Feature ft12 = new Feature(2);
        Feature ft13 = new Feature(4);
        Car fer = new Car("Ferrari", ft11, ft12, ft13);

        Feature ft21 = new Feature(1);
        Feature ft22 = new Feature(2);
        Feature ft23 = new Feature(8);
        Car suz = new Car("Suzuki", ft21, ft22, ft23);

        List<Car> cars = Arrays.asList(fer, suz);
        List<Integer> lookForTypes = Arrays.asList(4); // look whether any car has feature-type 4

        if (CollectionUtils.isNotEmpty(lookForTypes)) {
            /* code block in question : start */
            boolean atLeastOneTypeFound = cars
                    .parallelStream()
                    .anyMatch(holding -> holding.getAmounts()
                            .parallelStream()
                            .anyMatch(feature -> {
                                return lookForTypes.contains(feature.type);
                            }));
            /* code block in question : end */
            System.out.println("atLeastOneTypeFound=" + atLeastOneTypeFound);
        }
    }
}

class Car {
    List<Feature> featureList;
    String name;

    public Car(String name, Feature... features) {
        featureList = Arrays.asList(features);
        this.name = name;
    }

    public List<Feature> getAmounts() {
        return featureList;
    }

    public String getName() {
        return name;
    }
}

class Feature {
    int type;

    public Feature(int type) {
        this.type = type;
    }

    public int getType() {
        return type;
    }
}

If you don't need the Car, you can also flatMap to have the features and then distinct them before searching.

cars.stream()
  .flatMap(car -> car.getAmounts().stream())
  .map(Feature::getType)
  .distinct() // As spotted by @Pimgd in the comments this is useless
  .anyMatch(lookForTypes::contains);

But if you want to improve the readability of your code you should better consider to move a part of this "logic" into your objects :

cars.stream()
  .anyMatch(car -> car.hasAtLeastOneFeature(lookForTypes);

// ...

Car {
  boolean hasAtLeastOneFeature(List<Integer> features) {
    // Note that I renamed getAmounts to getFeatures
    return getFeatures().stream()
      .anyMatch(feature -> features.contains(feature.getType());
  }
}


C++ tuple implementation with get as a member If you're familiar with std::tuple, you're aware that accessing its elements is slightly unusual. Instead of using a member function, we're provided with the function std::get instead. I was curious as to why this is, so I conducted a brief search. The few websites that I saw all gave the same reason: the code used to called the method is ugly.

We would expect:

my_tuple.get<0>()

but apparently we would have to write:

my_tuple.template get<0>()

(a source, see footnote 5).

This seemed like a bad case of "I heard that she heard that he heard...", because I was unable to find an implementation of a tuple that exposed this ugliness by including get as a member. I heard that Boost's tuple had get as a member, but after inspecting the source I discovered that it also only accepted 10 elements max. It was time for me to implement it myself!

template <typename... Ts>
class Tuple
{

};

template <std::size_t Index, typename... Rs>
struct TypeHolder;

template <std::size_t Index, typename R, typename... Rs>
struct TypeHolder<Index, Tuple<R, Rs...>>
{
    typedef typename TypeHolder<Index - 1, Tuple<Rs...>>::type type;
};

template <typename R, typename... Rs>
struct TypeHolder<0, Tuple<R, Rs...>>
{
    typedef R type;
};

template <typename T, typename... Ts>
class Tuple<T, Ts...> : public Tuple<Ts...>
{
public:
    Tuple(T obj, Ts... objs)
        : Tuple<Ts...>{ std::forward<Ts>(objs)... }
        , obj{obj}
    {

    }

    template <std::size_t Index>
    typename std::enable_if<
        Index == 0,
        typename TypeHolder<Index, Tuple<T, Ts...>>::type&
    >::type get()
    {
        return obj;
    }

    template <std::size_t Index>
    typename std::enable_if<
        Index != 0,
        typename TypeHolder<Index, Tuple<T, Ts...>>::type&
    >::type get()
    {
        return static_cast<Tuple<Ts...>*>(this)->get<Index - 1>();
    }

protected:
    T obj;
};

This functions as expected, and after some brief testing I found that there were no issues with storing pointers or references. But there was no need to write "template" for every call to get! (My compiler is GCC, by the way.)

Is there a reason why std::tuple was not implemented in this way? I would like to know about any glaring flaws with this code before I move forward with it.

Your source is not very specific as to why/when my_tuple.template get<0>() is required.

If you have code like this:

Tuple<int, double, float> x(1,2,3);
int i = x.get<0>();

That is fine, because all template arguments to Tuple are known at the time of parsing and Tuple<int, double, float> is instantiated immediately, before the call to get is parsed. At that point the compiler already knows that in that specific instantiation of Tuple get is member function template rather than a non-template member and everything is ok.

This however

template<typename T>
auto foo(T t) {
    Tuple<T,int,double> x(t,2,3);
    return x.get<0>();
}

generates a compiler error, because when foo is parsed, the actual type of T is not known and instantiation of Tuple<T,int,double> needs to be delayed until instantiation of foo. This means that when x.get is parsed, the compiler does not yet know what kind of language construct get is in the instatiation of Tuple<T,int,double> and in fact it may very well vary for different T. Therefore it is assumed that get is a non-template member, which doesn't make sense considering the following template argument list.

Your code sadly does not solve this problem and I don't think it is solvable.

Additionally I noticed that your Tuple is an empty class if an empty type list is given. That is a bit inconsistent. I think at least get should be provided as empty function. Calling it should not be allowed, but referring to the member function should be fine.


Using PDO to list and remove updates I started using PDO 2 days ago, but I want to know if the way I'm coding and the way I'm using it is the correct way of doing things with PDO.

Please let me know if you guys notice any security flaws or improvements in general.

Here is a function I wrote that shows updates.

function ShowRemoveUpdates() {
    ob_start();

    try {
      // connectie gegevens
      require_once('connection.php');

      // pagination
      $start  = 0;
      $limiet = 8;

      if(isset($_GET['page'])) {
        $page   = $_GET['page'];
      } else {
        $page   = 1;
      }

      // query
      $stmt = $pdo->prepare('SELECT * FROM updates ORDER BY id DESC LIMIT :start, :limiet');
      $stmt->execute(array(':start' => $start, ':limiet' => $limiet));

      //start van formulier en tabel
      echo "<form method='post' action=''>";
      echo "
";
      echo "Geplaatst door	Titel	Tekst	Tijd	";

      // while loop door resultaten van query
      if($stmt->rowCount() > 0) {
        while ($row = $stmt->fetch()) {
          $idje = $row['id'];

          echo "
";
          echo "".$row['medewerker']."	";
          echo "".$row['titel']."	";
          echo "".$row['tekst']."	";
          echo "".$row['datum']."	";
          echo "<input type=\"checkbox\" name=\"checkvakje\" id=\"checkvakje\" value=\"$idje\">	";
          echo "
";
        }
        echo "
";
        echo "<input type='submit' value='Verwijderen'>";
      } else {
        echo "Geen resultaten gevonden.";
      }

      // Verwijder update functie
      if(isset($_POST['checkvakje'])) {
        $checkvak   = $_POST['checkvakje'];
        $stmt = $pdo->prepare('DELETE FROM updates WHERE id = :checkvak');
        $stmt->execute(array(':checkvak' => $checkvak));
        header("Refresh:0");
        exit();
      }

      echo "</form>";

      // count resultaten per rij en maak pagina nummering
      $stmt = $pdo->prepare('SELECT * FROM updates');
      $stmt->execute();
      $rows = $stmt->RowCount();
      $total=ceil($rows/$limiet);

      if($page>1) {
        echo "Vorige";
      }

      for($i=1;$i<=$total;$i++)
      {
        if($i==$page) {
          echo ".$i.";
          }

        else {
          echo "- ".$i."";
        }
      }

      if($page!=$total) {
        echo " - Volgende";
      }
      // vang errors op
    } catch (PDOException $e) {
        echo "Connection failed" . $e->getMessage() . "";
    }
}

My initial thought is that this function is doing way too much. It is:

reading user input (with no validation)
instantiating a database connection (possibly)
getting a result set from the database
deleting records from the database (possibly)
executing a query to get a row count from the database
generating HTML output
providing end user error messages

This is just too much logic to be put in one function.  I would encourage you to embrace separation of concerns as a key tenant in how you approach programming.

If you properly separate concerns, you move away from having to implement hacky uses of output buffers - something which should typically be reserved for very targeted use cases.  Speaking of your output buffer, I don't see where you are flushing it.  I would think it really poor practice to begin buffering in one area of code and have the buffer flushed/closed in another area of code. You are opening up your code to having to deal with the complexity/fragility of nested buffers if you have this behavior in other areas of code.

Typical convention would have functions/methods in PHP starting with lowercase letters.

Your try-catch block is used here inappropriately.  First, it encompasses way too many lines of code.  There are a number of PDO operations within this code that could throw, yet you only have a single catch block, which seems to consider only that the exception was due to connection failure.  So perhaps any try-catch logic around database connection should be in the include file and not even in this code at all, and you could have try-catch around individual database operations (i.e. query preparations) to give more granular control over how underlying exceptions are handled.

The second problem may make all this moot though, in that you should really only be catching exceptions if you are going to do something meaningful in the recovery operation.  Meaningful might be something like:

logging some aspect of code state before re-throwing the exception to caller;
gracefully recovering from exception such that caller can continue working in an acceptable state;
wrapping/rethrowing the exception to abstract underlying details from calling code.

You are not really doing such a thing here.  You are basically swallowing the exception such that the calling code, which is probably in a better position to understand how to deal with such an exception (i.e. by messaging end user or similar),  has no idea at all that anything went wrong. Don't swallow exceptions.

I appreciate that you are trying to move into using exceptions in your code, but you really need to think of exceptions as a way for one piece of code to communicate to calling code that it was not able to fulfill the contract/promise of what the calling code expects it to do.  If that calling code doesn't know how to recover from or handle such an exception in the code it calls, then it should let that exception continue to "bubble" up the call stack until there is code that is designed to handle the failure.

My next comment is related to the above exception handling comment, in that code that is not designed to message the end user about error conditions, should not do so.  Don't just leak out end user error messaging about DB connections failing with technical exception messages.  For the end user you likely want to clean up the messaging to something more meaningful as well as something that doesn't potentially leak sensitive application state information to the end user.  Typically, code up the call stack from code that is actually interacting with the database is much better positioned to handle this end user messaging.

Consider changing $start to $offset which is much more common terminology with regards to pagination.  Consider changing $limiet to $limit (the correct English word) if you are going to be writing your variable names in English.  Also consider naming your GET/POST parameters in English for consistency (again, that is if your desire is to have your code written in English).  Mixed-language code can be confusing to the reader.

You are not using $page at all in determining your offset for the LIMIT clause, so your pagination is not really working.

I would think your delete use case should be in a separate function altogether.

Consider using SQL_CALC_FOUND_ROWS in your LIMIT query in conjunction with FOUND_ROWS() to optimize this call pattern.

I would encourage you to move away from SELECT * when querying the database in favor of explicitly naming the fields you need in the result set. This may take you a minute or two extra when first writing the query, but it:

makes it clear to the reader of the code as to what fields will be available in the result set - they don't need to go look at the database schema to know what fields are available.
is oftentimes more efficient with regards to bandwidth and memory utilization. Why pull and store fields that you are not going to work with?
can make your application less fragile to schema changes.  For example, if at some point you add a field to this table, if a particular section of code does not need that field, you don't have to change it at all.  With a * select, you could actually break your application if you start introducing new fields that are not expected.

Why are you setting a refresh header here? It doesn't make sense by itself here.  Do you actually want to do a redirect?  If you move the code into different function or before the output from section above, can you get rid of the output buffer?

Again separation of concerns will help you here, as the typical approach one should take in a PHP web application is to do all your PHP back-end logic (dealing with user input, interacting with databases, setting header, performing redirects, preparing data for output, etc.) before you ever get to the point of rendering a view to the browser.

You should validate user input.  Prepared statements are great defense against SQL injection, but they are no defense against useless input.  What if your values for page and checkvakje are not integer representations?  Why would you even attempt a (relatively expensive) query with non-sensical input?

Your spacing around assignment operators is inconsistent.

Is there any reason you are using loose comparison operators (==, !=) instead exact comparisons here?  I would recommend getting in the habit if using exact comparisons by default and only using loose comparisons when there is a true use case for doing so.  this will make your could less fragile to unexpected truthy/falsey condition evaluations.

You have potential XSS vulnerability in that you are working with unvalidated user input in $page. Never trust user input.

You are inconsistent in bracket placement after flow control structures. Sometimes they start on same line, sometimes on following line.  You should strive to be consistent in your styling.  I would recommend a good PHP static code analysis tool like code sniffer or phpMd to help to enforce consistent styling as well as point our other questionable coding practices.

  if(isset($_GET['page'])) {
    $page   = $_GET['page'];
  } else {
    $page   = 1;
  }

Avoid unnecessary code branches (such as the unnecessary else shown in the example above). This could be:

  // here with a bonus validation suggestion
  $page = filter_input(INPUT_GET, 'page', FILTER_VALIDATE_INT)) {
  if(empty($page) || $page < 1) {
      $page = 1;
  }

  // here a suggestion without meaningful validation
  $page = 1;
  if(!empty($_GET['page'])) {
      $page = $_GET['page'];
  }

This may seem trivial, but this is more about getting the right mindset that code branches are to be designed away if possible, as the more code branches you have, the harder your code is to test and the more buggy your code is likely to be.


Fix line breaks, cross-platform So I ran into an issue where a client sent me a sample file and the line breaks were not preserved when reading the file using PHP from linux. So I wrote up a little function that should hopefully fix the issue for all platforms.

This is from a larger File class. The function names are pretty self explanatory.

public function fixLineBreaks() {
    $this->openFile();

    // replace all placeholder with proper \r\n
    $contents = str_replace("[%LINEBREAK%]", "\r\n",

                    // replace all double place holder with single
                    // in case both \r and \n was used.
                    str_replace("[%LINEBREAK%][%LINEBREAK%]", "[%LINEBREAK%]",

                        // replace all \n with place holder
                        str_replace("\n", "[%LINEBREAK%]",

                            // replace all \r with placeholder
                            str_replace("\r", "[%LINEBREAK%]",

                                // First, read the file.
                                fread($this->fp, filesize($this->filename))
                            )
                        )
                    )
                );
    $this->closeFile();

    $this->writeOpen();
    fwrite($this->fp, $contents);
    $this->closeFile();
}

EDIT

Okay, to clarify. I received a CSV file that was supposed to be parsed in PHP. Open it up in notepad and everything is on one line. Opening it with fopen and then reading with fgetcsv would also return the contents as one line. However, if you copy and paste the contents from notepad into NetBeans then all the line breaks are there. This made me realize that there are line breaks but they're not the ones that notepad or PHP recognize.

This was the function that I wrote up which essentially re-writes the file with proper line breaks.

I was posting this here to see if anyone possible had a better solution.

This conversion can be done by simple apps from the Dos2Unix package.
However, if you insist on writing your own converter, I'd suggest making fewer function calls. The function str_replace() can accept arrays, so there is no need for embedded calls.
$contents = str_replace(
    array("\r", "\n", "[%LINEBREAK%][%LINEBREAK%]", "[%LINEBREAK%]"),
    array("[%LINEBREAK%]", "[%LINEBREAK%]", "[%LINEBREAK%]", "\r\n"),
    fread($this->fp, filesize($this->filename))
);

However, you ought to be careful. For example, if your file has two Unix-standard newlines, i.e., \n\n, then your code will replace this with \r\n, which is only one Windows-standard newline.
This regex (see about its syntax here) would probably do the trick:
preg_replace('#(?<!\r)\n|\r(?!\n)#', "\r\n", fread($this->fp, filesize($this->filename)));

But I still think it's better to use already existing and proven software, like the one from the beginning of this answer, or simply turn on the auto_detect_line_endings setting, as suggested in the comments by 200_success.
By the way, if your files are huge, your fread will still (try to) read them whole at once, which is not pleasant for the computer memory and performance.


Displaying all the numbers between 200 and 600 I have to display all the numbers between 200 and 600 (both numbers inclusive) that are divisible by 8:

for($i=200;$i<=600;$i++)
{
    if($i%8==0)
        echo $i.',';
}

It reaches maximum execution time.


SQLite database for a micro/tumble blog application I'm creating a personal website where, among other things, there is a blog, or a "microblog" (I'm still not sure how to define the two), where I will be writing articles and having users, who will create accounts, write comments on the posts. In addition, the blog posts will be tagged to allow for easier searching of specific blog posts. I was also planning on having the users' comments and posts be listed on their profile page, so that it is possible to view all comments or all posts made by a specific user (and reach said comment or post from their profile).

The blog will be built using Python 3, Flask, and SQLAlchemy for the backend.

The structure for the website will be a list of Users, which has a list of Posts that they've created (and are related to in a one-to-many relationship). Each Post will have only one author (one-to-one), but can be tagged a variety of Tags (many-to-many since many posts can be tagged with many tags). Each post will also have a list of comments (multiple comments), but each comment can only be linked to one Post and one User (the author).

Does the models code that I have below accurately describe and implement the design of my database that I'm trying to achieve?

from hashlib import md5
from app import lm, db
from flask.ext.sqlalchemy import SQLAlchemy
from flask.ext.login import LoginManager, UserMixin

class User(UserMixin, db.Model):
    __tablename__ = 'users'
    id            = db.Column(db.Integer, primary_key=True)
    join_date     = db.Column(db.DateTime)
    username      = db.Column(db.String(64), index=True, unique=True, nullable=False)
    realname      = db.Column(db.String(128), index=True)
    email         = db.Column(db.String(128), index=True)
    role          = db.Column(db.String(64))
    about_me      = db.Column(db.String(500))
    last_seen     = db.Column(db.DateTime)

    posts         = db.relationship('Post', backref='author', lazy='dynamic')
    comments      = db.relationship('Comment', backref='author', lazy='dynamic')

    def avatar(self, size):
        gravatar_url = 'http://www.gravatar.com/avatar/%s?d=identicon&s=%d&r=pg'
        return gravatar_url % md5(self.email.encode('utf-8')).hexdigest()

    def __repr__(self):
        _repr = '<models.User instance; ID: {}; username: {}>'
        return _repr.format(self.id, self.username)

@lm.user_loader
def load_user(id):
    return User.query.get(int(id))

class Post(db.Model):
    __tablename__ = 'posts'
    id            = db.Column(db.Integer, primary_key=True)
    author_id     = db.Column(db.Integer, db.ForeignKey('users.id'))
    tag_id        = db.Column(db.Integer, db.ForeignKey('tags.id'))
    title         = db.Column(db.String(128))
    body          = db.Column(db.String(20000))
    create_date   = db.Column(db.DateTime)
    comments      = db.relationship('Comment', backref='tagged_post', lazy='dynamic')

    def __repr__(self):
        _repr = '<models.Post instance; ID: {}; title: {}>'
        return _repr.format(self.id, self.title)

class Tag(db.Model):
    __tablename__ = 'tags'
    id            = db.Column(db.Integer, primary_key=True)
    title         = db.Column(db.String(64))
    tagged        = db.relationship('Post', backref='tag', lazy='dynamic')

    def __repr__(self):
        _repr = '<models.Tag instance; ID: {}; title: {}>'
        return _repr.format(self.id, self.title)

class Comment(db.Model):
    __tablename__ = 'comments'
    id            = db.Column(db.Integer, primary_key=True)
    post_id       = db.Column(db.Integer, db.ForeignKey('posts.id'))
    author_id     = db.Column(db.Integer, db.ForeignKey('users.id'))
    body          = db.Column(db.String(1500))
    create_date   = db.Column(db.DateTime)

    def __repr__(self):
        _repr = '<models.Comment instance; ID: {}; post_title: {}>'
        return _repr.format(self.id, self.post_id.title)

Here is a link to the (current) full source of the website.

Everything is good by this point but I'd change the relation between posts and tags from one-to-many to many-to-many.

You can do this with this code:

tags_to_posts = db.Table(
    "tags_and_posts",
    db.Column( "post_id", db.Integer, db.ForeignKey( "posts.id" ) ),
    db.Column( "tag_id", db.Integer, db.ForeignKey( "tags.id" ) )
)

class Post(db.Model):
    # ...
    tags = db.relationship(
        "Tag",
        secondary=tags_and_posts,
        backref=db.backref( "posts", lazy="dynamic" ),
        passive_deletes=True,
    )

Then tags will be just a list. Assuming my_tag and my_post exist, you'll be able to do something like this:

# Add tag to post
my_post.tags.append(my_tag)
db.session.add(my_post)
db.session.add(my_tag)
db.session.commit()

# Remove tag from post
# You need to be sure that my_tag is in my_post.tags
my_post.tags.remove(my_tag)
db.session.add(my_post)
db.session.add(my_tag)
db.session.commit()

I hope, it helps.


Finding the sum closest to a target number Wallace the Weightlifting Walrus is training for a contest where it will have to lift 1000 kg. Wallace has some weight plates lying around, possibly of different weights, and its goal is to add some of the plates to a bar so that it can train with a weight as close as possible to 1000 kg.

  In case there exist two such numbers which are equally close to 1000 (e.g. 998 and 1002), Wallace will pick the greater one (in this case 1002).

  Help Wallace the Weightlifting Walrus and tell it which weight it will have to lift.

  Input

  The first line of the input contains the number of plates n (1≤ n ≤ 1000). Each of the following n lines contains one positive integer less than or equal to 1000, denoting the weight of each plate.

  Output

  Output one integer, the combined weight closest to 1000.

  https://open.kattis.com/problems/walrusweights

e.g if n = [4, 900, 500, 498, 4], the output will be 1002

var input = [4, 900, 500, 498, 4];
function walrusWeight(input) {
  var target = 1000;
  var optimum = 0;
  var sums = [];
  sums.push(optimum);

  for(var i=0; i < input.length; i++) {
    var newSums = [];
    for(var j=0; j < sums.length; j++) {
      var newSum = sums[j] + input[i];
      if (newSum <= target) {
        newSums.push(newSum);
        if (newSum > optimum) {
          optimum = newSum;
        }
      } else if ((Math.abs(target-newSum) < Math.abs(target-optimum)) || (Math.abs(target-newSum) == Math.abs(target-optimum) && newSum > optimum)) {
    optimum = newSum;
    }
  }
  sums = sums.concat(newSums);
 }
 return optimum;
}

walrusWeight(input);

Style

You code is not properly indented which makes it quite hard to understand.

Calling your function argument input conveys pretty much as much information as calling it argument. I guess weights or weightList would be more meaningful.

It would probably make sense for target to be a parameter of the walrusWeight function.

Bug

Calling your function with var weights = [4, 900, 500, 498, 4, 1000]; and var target = 1000; gives 1002 as a result.

Tests

Before you try to fix anything, I suggest you write a test suite with your function being called with various inputs and the result you are expecting. This will make the developement of a new solution much easier.

Algorithm

Because your current solution is wrong, you'll need to think about a new algorithm.

A simple, yet, efficient enough with small-ish inputs it to iterate over the weight and keep tracks of the various possible sums :

With [4, 900, 500, 498, 4, 1000], you'd get something like :

iteration 0 : possible sums = [0]
iteration 1 : possible sums = [0, 4]
iteration 2 : possible sums = [0, 4, 900, 904]
iteration 3 : possible sums = [0, 4, 900, 904, 500, 504, 1300, 1304]
iteration 4 : possible sums = etc
iteration 5 : possible sums = etc

When you'll have all candidates, it's easy to pick the best.

Then, you'll find various optimisations (get rid of values that won't need to solution early on) but it's a good idea to make it right and then make it fast.

Code example

I've tried to implement the suggestions above :

var weights = [2000, 1003, 4, 900, 500, 498, 3, 8];
var target = 1000;

function walrusWeight(weights) {
    var optimum = 0;
    var optimum_distance = target;
    var sums = [0];

    for(var i=0; i < weights.length; i++) {
        var weight = weights[i];
        var newSums = [];
        for(var j=0; j < sums.length; j++) {
            var sum = sums[j];
            if (true) // An optimisation could be added here
            {
                newSums.push(sum)
                var newSum = sum + weight;
                var distance = Math.abs(target - newSum)
                if (newSum <= target)
                {
                    newSums.push(newSum)
                    if (distance < optimum_distance) {
                        optimum = newSum;
                        optimum_distance = distance;
                    }
                }
                else if (distance < optimum_distance || (distance == optimum_distance && newSum > optimum))
                {
                    optimum = newSum;
                    optimum_distance = distance;
                    newSums.push(newSum)
                }
            }
        }
        sums = newSums;
        print(sums)
    }
    return optimum;
}

print(walrusWeight(weights, target));


Multiple indexes over an in memory collection for faster search I have a big in-memory collection of following simplified class:

public class Product
{
    public int Id { get; set; }
    public string UserName { get; set; }
    public int CategoryId { get; set; }
    public string Title { get; set; }
    public string Description { get; set; }
}

I need to search for products based on different properties like UserName or CategoryId.
One way of searching would be using linq to objects like:

var userProducts = products.Where(x => x.UserName == "SomeValue")

This takes some processing when collection is too big and in my case it would be called hundreds of time each second.

What I came up with was to introduce a new collection that supports indexing over different properties:

public class FastCollection<T> : IEnumerable<T>
{
    private IList<T> _items;
    private IList<Expression<Func<T, object>>> _lookups;
    private Dictionary<string, ILookup<object, T>> _indexes;

    public FastCollection(IList<T> data)
    {
        _items = data;
        _lookups = new List<Expression<Func<T, object>>>();
        _indexes = new Dictionary<string, ILookup<object, T>>();
    }

    public void AddIndex(Expression<Func<T, object>> property)
    {
        _lookups.Add(property);
        _indexes.Add(property.ToString(), _items.ToLookup(property.Compile()));
    }

    public void Add(T item)
    {
        _items.Add(item);
        RebuildIndexes();
    }

    public void Remove(T item)
    {
        _items.Remove(item);
        RebuildIndexes();
    }

    public void RebuildIndexes()
    {
        if (_lookups.Count > 0)
        {
            _indexes = new Dictionary<string, ILookup<object, T>>();
            foreach (var lookup in _lookups)
            {
                _indexes.Add(lookup.ToString(), _items.ToLookup(lookup.Compile()));
            }
        }
    }

    public IEnumerable<T> FindValue<TProperty>(Expression<Func<T, TProperty>> property, TProperty value)
    {
        var key = property.ToString();
        if(_indexes.ContainsKey(key))
        {
            return _indexes[key][value];
        }
        else
        {
            var c = property.Compile();
            return _items.Where(x => c(x).Equals(value));
        }
    }

    public IEnumerator<T> GetEnumerator()
    {
        return _items.GetEnumerator();
    }

    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

You can initialize the collection like this:

var fc = new FastCollection<Product>(products);
fc.AddIndex(x => x.Id);
fc.AddIndex(x => x.UserName);
fc.AddIndex(x => x.CategoryId);

And finally you can search the collection like this:

var userProducts = gc.FindValue(x => x.UserName, "SomeValue");

The fast collection makes a big difference when it comes to performance.

My question is if I'm doing it right? I have used delegates and expressions to make it as generic as possible but I have this feeling that there is room for improvement!

Using ToString() to compare expressions for equality might work in simple cases, but:

It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
    lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.