Moving WordPress database and files to new host I've written a script in an attempt to try to automate moving hosts from old hosts to our new docker containers.

All feedback and input appreciated!

#!/bin/bash

# Generate private public ssh-keys
if [ ! -f "$HOME/.ssh/id_rsa" ] && [ ! -f "$HOME/.ssh/id_rsa.pub" ]; then
        ssh-keygen -b 4096
fi

read -p "Enter your ssh host:" ssh_host
read -p "Enter your ssh username:" ssh_user
read -p "Enter your remote ssh port:" ssh_port

# REMOTE SSH

if [ -z "$ssh_user" ]; then
        echo -e "Config: SSH Username missing"
        echo $ssh_user
        exit
fi

if [ -z "$ssh_host" ]; then
        echo -e "Config: SSH Host is missing"
        exit
fi

if [ -z "$ssh_port" ]; then
        echo -e "Config: SSH Port missing"
        exit
fi

ssh-copy-id $ssh_user@$ssh_host -p $ssh_port -i "$HOME/.ssh/id_rsa" &>/dev/null

# REMOTE DATABASE

read -p "Enter path for wp-config.php:" wp_config_path

# INIT
db_details="cat $wp_config_path/wp-config.php"
scp -P $ssh_port -r $ssh_user@$ssh_host:"$wp_config_path/*" .

# Might be dangerous if file contains malicious input in the values?
eval $(awk -F "[()']" '/^define\(/{printf "%s='\''%s'\''\n", $3, $5;}' < wp-config.php | grep DB_*)

if [ -z "$DB_USER" ]; then
        echo -e "Config: RDB user missing"
        exit
fi

if [ -z "$DB_PASSWORD" ]; then
        echo -e "Config: RDB password missing"
        exit
fi

if [ -z "$DB_NAME" ]; then
        echo -e "Config: RDB name missing"
        exit
fi

dump="mysqldump -u $DB_USER --password='$DB_PASSWORD' $DB_NAME"

read -p "Enter your LOCAL database user:" local_db_user
read -p "Enter your LOCAL database name:" local_db_name
read -s -p "Enter your local database password:" local_db_password

printf "\033c"

ssh $ssh_user@$ssh_host -p $ssh_port $dump | mysql -u $local_db_user --password=$local_db_password $local_db_name

sed -i -e "s;\(define([[:space:]]*'DB_USER',[[:space:]]*\)\(.*\)\()\;\);\1'$local_db_user'\3;g" wp-config.php
sed -i -e "s;\(define([[:space:]]*'DB_PASSWORD',[[:space:]]*\)\(.*\)\()\;\);\1'$local_db_password'\3;g" wp-config.php
sed -i -e "s;\(define([[:space:]]*'DB_NAME',[[:space:]]*\)\(.*\)\()\;\);\1'$local_db_name'\3;g" wp-config.php

echo -e "Database imported to: $local_db_name"
echo -e "Cleaning up..."

# CLEANING UP

# This is bad, and should NEVER be used on hosts with active pub/private key authentication.
ssh $ssh_user@$ssh_host -p $ssh_port 'echo "" > $HOME/.ssh/authorized_keys'

ssh_user=
ssh_host=
ssh_port=
DB_USER=
DB_PASSWORD=
DB_NAME=
local_db_name=
local_db_password=
local_db_user=
wp_config_path=

echo -e "Exiting"

Make input validation more user-friendly

You ask to enter host, user and port, and if one of them is empty you exit.
If I entered a host and a user and accidentally pressed enter for port (empty), I would be very unhappy with the script.

It would be better to put the prompting logic in a loop,
and repeat forever until a non-empty value is entered.
Ideally in a function, to reduce code duplication.

Exit with non-zero code on error

When exiting with error,
it's recommended to use a non-zero exit code.

~ is the same as $HOME

I prefer writing ~ because it's shorter.

Use printf instead of echo -e

The flags of echo are not portable.
To get the behavior of echo -e,
I suggest to make it a habit to use printf instead.

Replace awk + grep with just awk

In this command you use a combination of awk and grep:

eval $(awk -F "[()']" '/^define\(/{printf "%s='\''%s'\''\n", $3, $5;}' < wp-config.php | grep DB_*)

You can do what grep does here in awk,
which will be more efficient.
The difference will be negligible in this example,
but it's good to make it a habit to avoid additional processes when easily possible.

Replace multiple sed calls when one is enough

This code rewrites the same file 3 times:

sed -i -e "s;\(define([[:space:]]*'DB_USER',[[:space:]]*\)\(.*\)\()\;\);\1'$local_db_user'\3;g"

  wp-config.php
      sed -i -e "s;(define([[:space:]]'DB_PASSWORD',[[:space:]])(.)()\;);\1'$local_db_password'\3;g" wp-config.php
      sed -i -e "s;(define([[:space:]]'DB_NAME',[[:space:]])(.)()\;);\1'$local_db_name'\3;g"
  wp-config.php

One sed would be enough, using multiple -e parameters.

Pointless cleaning up

Resetting the variables at the end of the script seem pointless.
This is only useful if you source this script, which seems unlikely.
If you run this script (as opposed to sourcing it),
then the variables you defined or modified will not be visible after the script has completed.


Checking the sum of pairs to match a target value I'm new to Python and curious what looks good and what doesn't. The function I have defined looks for pairs in a list of integers and tells the user whether or not any possible pair equals the target number. How can this code be more efficient or fast?

import datetime
from itertools import combinations

def csop(*params):
    t1 = datetime.datetime.now()
    print params[0], params[1]
    for c in combinations(params[0],2):
        print sum(c)
        if sum(c) == params[1]:
            print 'Target number reached'
            t2 = datetime.datetime.now()
            print t2-t1
            exit()
        else:
            pass

To run:

import csop
csop.csop([1,2,3,4],5)

The output should look something like this:

[1, 2, 3, 4] 5
3
4
5
Target number reached
0:00:00.000071

As a style note, this is a bit of an abuse:

def csop(*params):

The point of *params is to allow things to be passed that cannot be known when the code is written. This is not the case here.  There are only two parameters, and you know what they are.  So you should give them names.

This:

exit()

is also bad form.  Exiting the program from the middle of a function is great way to make bugs that are really hard to find.  In this case simply return

this:

else:
    pass

is completely dead code.  pass is used to mark a block that is empty.  It is a by product of everything in Python being indent sensitive.  But the else is optional so just leave it and the pass off.

And performance wise, you perform the sum() twice.  If it matters to you, this result could be saved instead of evaluating it twice.


Ruby Beginner - Fuzzy matching / string manipulation There is no intended end use for this. It is for learning and development only.

I got carried away following an example and ended up with a miniaturised vehicle registration system. You can bulk generate records, each of which is given a unique registration number in keeping with British registration format. I've written a script that compares and lists all registration numbers that fall afoul of the criteria.

It does work, but it is incredibly slow. (over an hour to check 1 mil records).
I am looking for critique on the logic and any optimisation I may have missed.

Example string:    AA99AAA

Example criteria:  A?9?AAA

 def full_search(offensive_list)
    p 'Full check:'
    p "Comparing #{$all_vehicles.count} records against #{offensive_list.count} banned combinations"
    p 'This will take a few minutes'
    vrm_array, example_array = [], []
    vrm_list = $all_vehicles.keys.sort
    vrm_list.each do |vrm|
        vrm_array << vrm.split("") #We split each reg into an array of characters
    end
    offensive_list.each do |example|
        example.strip!
        example_array << example.split("") #and the same with our banned combinations
    end

        vrm_array.each do |vrm|
            example_array.each do |example| #itterate through vrms x examples
                @formatted_vrm = vrm.dup
                if example.length == vrm.length
                    example.each_index do |index|
                        if example[index] == "?" #for each wildcard we add a wildcard to the vrm for comparison
                            @formatted_vrm[index] = "?"
                        end
                    end
                    if @formatted_vrm == example then offensive_found(vrm, example) end
                end
            end
        end
end

def offensive_found(vrm, example)
    built_vrm = ""
    built_example = ""
    if vrm.class == Array #clean up formatting so we can store it
    vrm.each do |character|
            built_vrm << character
        end
        example.each do |character|
            built_example << character
        end
    else
        built_example = example #clearly redundant, but it works so...
        built_vrm = vrm
    end

    if $bad_vrms[built_example] # if we already have a record
        prev_matched = $bad_vrms[built_example] #just add to the array
        prev_matched << built_vrm
        $bad_vrms.store(built_example, prev_matched)
    else
        new_match = [built_vrm]        # or create a new hash key
        $bad_vrms.store(built_example, new_match)
    end
    #p "#{built_vrm} - matched with #{built_example}"
end

If you'd prefer you can clone the full thing on github. https://github.com/Niall47/RubySomDemo

You could consider using Regex to speed up the search. The cleanest way to do so would be to change your full_search.txt to be Regex expressions. For example A?9?AAA would need to be changed to A.9.AAA (in Regex the . means any single character).

Then you could change your full_search method to look like this:

def full_search(offensive_list)
  vrm_list = $all_vehicles.keys.sort
  offensive_examples = offensive_list.map(&:strip)

  offensive_examples.each do |offensive_example|
    vrm_list.grep(/^#{offensive_example}$/).each do |offensive_vrm|
      offensive_found(offensive_vrm, offensive_example)
    end
  end
end

In the Regex the ^ means start of string and the $ means end of string; this basically ensures that substrings are not matched e.g.) abcd matches with the regex .c but not with ^.c$

If you don't want to modify your list, you could so something that dynamically creates the regex in Ruby. For example: offensive_example.gsub("?", ".") this would replace all ? with ..

A couple unrelated pointers...

Ruby has a String#chars method that is (arguably) more readable and might have some minor performance improvements over string.split("").
Instead of assigning an initial empty value and building it in an each, consider using Array#map to reduce the amount of variable reassignments.

# before
vrm_array = []
vrm_list.each do |vrm|
  vrm_array << vrm.split("")
end
vrm_array.each do |vrm|
    # do stuff
end

# after
vrm_array = vrm_list.map { |vrm| vrm.split("") } # this calls `split` on each element in `vrm_list`
vrm_array.each do |vrm|
    # do stuff
end

If you need to take an array of characters and join them together into a single string, consider using Array#join.

irb(main):001:0> ['a', 'b', 'c'].join
=> "abc"

If you have a Hash and you want an empty array if a key doesn't exist, consider using a default value (but be careful with some gotchas related to mutable defaults)

$bad_vrms = Hash.new { |h, k| h[k] = [] } # the argument is the default value which in this case is a block that initializes the key's value to a new instance of an empty array

# now we don't need to check if a key exists and can simply push to it
$bad_vrms[example] << vrm


Count associations in a Ruby model I'm trying to figure out if there's a better way to run the code below.
Basically Course is an association of Student and courses_needed is an array of Course where I have many Students have and need many Courses and I'm trying to figure out the demand.
I've came up with the following code and it runs, which is fine, but when I want to display it on a statistics page with 36 Courses, the same code needs to be run once for each Course.

I've solved it now by having it run once a day, instead of real-time.

  def self.calculate_needed
    Course.all.each do |course|
      neededArray = Array.new
      needed = 0
      Student.all.each do |s|
        unless s.courses_needed.nil?
          s.courses_needed.each do |c|
            if c == course
              needed += 1
            end
          end
        end
      end
      course.needed = needed
      course.save
    end
  end

This should be interesting. The fastest way I can think of is to pluck all the courses_needed arrays out of Student, group them together, and count them:

course_hash = Student.where.not(courses_needed: nil).pluck(:courses_needed).
  flatten.group_by { |c| c }.map { |k, v| [k, v.size] }.to_h

Essentially, I grab all students that actually have courses_needed, I then grab just all the courses needed from students. Flatten the course arrays so that all courses are within the same array, then I group them by course. After that I map again to replace the values (just the list of all same courses) with the number of times each course is listed. Then I do to_h to turn each [course, number] into a course => number hash that you can access easier.

So pluck returns:

[[course, course2, course3], [course, course2]]

Flatten makes this:

[course, course2, course3, course, course2]

Group_by turns it into:

{course => [course, course], course2 => [course2, course2], course3 => [course3]}

Map does:

[[course, 2], [course2, 2] , [course3, 1]]

Then to_h:

{course => 2, course2 => 2, course3 => 1}

At that point you can access the course hash and get the number.

course_hash[course]
# 2

Now that we have the hash we can update the needed column in Course:

Course.find_each do |course|
  course.update_attributes(needed: course_hash[course].to_i)
end

The to_i is used so that if course_hash does not contain the course it returns nil, then to_i will turn nil into 0.


Next palindromic number I wrote a function that takes a number and finds the next palindromic number. Time complexity is currently O(n2), which is really bad. How could one make this function more efficient?

function nextPalindrome(n) {
  // first we create a function to check if a single number is a palindrome
  function isPalindrome(num) {
    const forward = num.toString();
    let backward = [];
    for (let i = forward.length - 1; i >= 0; i--) {
      backward.push(forward[i]);
    }
    backward = backward.join('');
    return forward === backward;
  }
  // now let's create a counter variable to increment our number (n)
  // we'll initialize it as n + 1 since we know we're not going to check the first number
  let countUp = n + 1;
  // we also need a nextPalindrome variable
  let nextP;
  // now we need to begin some sort of loop
  // perhaps a while loop will do
  while (true) {
    if (isPalindrome(countUp)) {
      return countUp;
    } else {
      countUp += 1;
    }
  }

}

Note: Minor change to the algorithm after I found a scenario it didn't cover

This is more of a mathematical problem than a programming one. After some thinking and experimentation, I've come up with an algorithm that can solve this in O(n), where n is the number of digits in the starting number. The basic principle is that we first break down the number into an array of digits, then start at the two ends and work our way inward to the middle, trying to set each pair of digits to the lower one, or as close to the lower one, in as many places as possible.

When examining a pair of digits, we always override the lower-order digit with the higher-order one. If the lower-order digit is greater than the higher-order one, it means the value of our number has reduced as a result of the override, so we set a carry flag. This carry flag will cause the next digit we measured from the lower end of the number (i.e. the lower-order one if we're less than halfway through the number, and the higher-order one if we're more than halfway through) to increment by one before the override is made. If this incrementation causes the digit to "overflow" into 10, we immediately set the digit to 0, set the carry flag to true, and proceed to the next digit.

function nextPalindrome(n) {
    let digits = [];
    let m = n;
    while (m > 0) {
        digits.unshift(m % 10);
        m = Math.trunc(m / 10);
    }

    let carryFlag = false;
    let midPoint = Math.floor(digits.length / 2);

    for (let i = 0; i < digits.length; i++) {
        let copyFrom = digits[i];
        let copyTo = digits[digits.length - 1 - i] + carryFlag;
        if (copyTo === 10) {
            digits[digits.length - 1 - i] = 0;
            carryFlag = true;
        }
        else {
            let digit = (i < midPoint) ? copyFrom : copyTo;
            carryFlag = copyTo > digit;
            digits[i] = digits[digits.length - 1 - i] = digit;
        }
    }

    return digits.reduce((acc, next) => 10 * acc + next, 0);
}

I don't have a pure mathematical proof detailed, but it seems to work. Please feel free to test it (especially with edge cases) and comment if something's wrong.


Review implementation of boolean triple Let a,b,c be a boolean value. I need to print all values of expression a && b && c. I'm writing the class

public class BooleanTriple{
    private boolean a,b,c;
    public BooleanTriple(boolean a, boolean b, boolean c){ this.a =a; this.b=b; this.c=c;}

    /**
    *Increments the value of triple with lexicographical ordering
    */
    public void incr(){
        if (!a) a=true;
        else if(!b){
            a= false;
            b= true;
        else if (!c){
            a= false;
            b= false;
            c=true;
        }
    }
    public boolean logProduct(){ return a && b && c;}
}

And Main class:

public class Main{
    BooleanTriple bTriple = new BooleanTriple(false,false,false);
    public static void main(String[] args){
        for (int i=0; i<8; i++){
            System.out.println(bTriple.logProduct());
            bTriple.incr();
        }
    }
}

But i think, that it's bad implementation. Can you correct me?

if I quite understand your problem, to do a proper incr() method I would do so :

public class BooleanTriple {
    private boolean a,b,c;
    private int i;
    public BooleanTriple(boolean a, boolean b, boolean c){
        this.a =a; this.b=b; this.c=c;
        this.i = (a ? 1 : 0)*4+(b ? 1 : 0)*2+(c ? 1 : 0)*1;  // not sure if this compile, but the idea is here
    }

    public void incr(){
        i = (i + 1) % 8;
        a = (i & 0x4) != 0;
        b = (i & 0x2) != 0;
        c = (i & 0x1) != 0;
    }
    public boolean logProduct(){ return a && b && c;}
}

With i going from 0 (000 binary) to 7 (111 binary) you have all the possible values for a, b and c


Responsive text by JavaScript I'm working on a JavaScript plugin that makes font-sizes flexible (for a responsive layout). The code works well so far, but I'm sure that it's capable of improvement (especially if you look at the global variable).

(function () {
    var extend = function (obj, ext) {
        for (var key in ext) {
            if (ext.hasOwnProperty(key)) {
                obj[key] = ext[key];
            }
        }
        return obj;
    };

    window.txt = function (el, k, c) {
        var s = extend({
            n: Number.NEGATIVE_INFINITY,
            x: Number.POSITIVE_INFINITY
        }, c);

        var fit = function () {
            var a = k;

            var i = function () {
                el.style.fontSize = Math.max(Math.min(el.clientWidth / a, parseFloat(s.x)), parseFloat(s.n)) + "px";
            };
            i();
            window.addEventListener("resize", i);
            window.addEventListener("orientationchange", i);
        };

        if (el.length) {
            for (var i = 0; i < el.length; i++) {
                fit(el[i]);
            }
        } else {
            fit(el);
        }

        return el;
    };
})();

DEMO

Meaning of the used variables:

n = minFontSize
x = maxFontSize
s = settings
c = config
el = element
i → running the function
a or k = fontRatio/compressor

window.txt(document.getElementById("title"), 10);

↑ executes the JavaScript Plugin on an element.

Thoughts:

Variable names: I hate typing unnecessary text as much as the next dev, but variable names are not a good place to skimp. It's one thing when using i as an index variable, for example, but for others, please, please, please use meaningful names. If you have to provide a key after the fact, this should be a good clue that you've not adequately done so.
Don't pollute the global space. If the caller needs access to the function, return it instead. Or use modules (module.exports or the like).
minFontSize less than zero doesn't make much sense. I'd cap it at zero rather than –∞.
maxFontSize being capped at +∞ probably doesn't make sense either.
Your code will re-parse the minimum and maximum font sizes each time the handler is called. Cache these instead.
var a = k: Why? In this case, just use k instead.
i() does not do what you think it will: if el is an array, el.style is going to be undefined, and el.style.fontSize will error.
When iterating arrays using for (;;;), cache the length, like so:

for (let i=0, l=arr.length;i<l;i++) { ... } // for ES5 use "var"

If k = 0, what happens? Division by zero...

This is a very quick rendition of your code (in ES6), with meaningful variable names, and implementing caching and such, but it might be useful:

function fitText(el, ratio, {minFontSize, maxFontSize}) {
  el.style.fontSize = Math.max(Math.min(el.clientWidth / ratio, maxFontSize), minFontSize) + "px";
}

function attachListeners(el, ratio, {minFontSize, maxFontSize} = {}) {
  const boundFitText = fitText.bind(undefined, el, ratio, {minFontSize, maxFontSize}),
        addEventListener = window.addEventListener;
  addEventListener("resize", boundFitText, false);
  addEventListener("orientationchange", boundFitText, false);
}

export function adjustTextSize(el = [], ratio = 1,
  {
    minFontSize=0,
    maxFontSize=Number.POSITIVE_INFINITY
  } = {})
{
  if (ratio === 0) {
    return el; // prevent division by zero later; throwing an exception might be better here, though
  }

  const parsedOptions = {
    minFontSize: parseFloat(minFontSize),
    maxFontSize: parseFloat(maxFontSize)
  }

  const els = (el instanceof Array) ? el : [el];
  for (let i=0, l=els.length; i<l; i++) {
    let actualEl = els[i];
    fitText(actualEl, ratio, parsedOptions);
    attachListeners(actualEl, ratio, parsedOptions);
  }
  return el;
};


Aligning '=' on protocol buffers .proto files Should the equals sign be alined on protocol buffers? I am having a huge discussion with a colleague and we can't get to decide on what's the best practice.

message foobar {
    optional bool var_one_short             = 1;
    optional bool var_two_looooooooong_name = 2;
    optional bool another_var               = 3;
}

vs

message foobar {
    optional bool var_one_short = 1;
    optional bool var_two_looooooooong_name = 2;
    optional bool another_var = 3;
}

I think the second version is much easier to maintain. If you have a new variable with a longer name (like var_two_reaalllllly_looooooooong_name) you don't have to modify three other lines too to keep it nice. The first version also could cause unnecessary patch/merge conflicts.

From Code Complete, 2nd Edition by Steve McConnell, p758:

  Do not align right sides of assignment statements

  [...]

  With the benefit of 10 years’ hindsight, I have found that,
  while this indentation style might look attractive, it becomes
  a headache to maintain the alignment of the equals signs as variable
  names change and code is run through tools that substitute tabs
  for spaces and spaces for tabs. It is also hard to maintain as
  lines are moved among different parts of the program that have
  different levels of indentation.


Quick sort implementation in JavaScript in prototype style I have implemented quickly the quick sort algorithm in JavaScript using prototype based style.

The code is rather short (http://jsfiddle.net/b8mfY/):

<script>

"use strict";

var Sort = function( inputArray ) {
    this.arrayTest = undefined;
    this.init( inputArray );
};
Sort.prototype.init = function( inputArray ) {
    if ( inputArray instanceof Array && inputArray.length > 0 ) {
        this.arrayTest = inputArray;
        this.DumpArray( this.arrayTest );
        this.QuickSort( this.arrayTest, 0, this.arrayTest.length - 1 );
        this.DumpArray( this.arrayTest );
    }
    else console.log( 'Input argument isn\'t an array object or its length equals zero.' );
};
Sort.prototype.QuickSort = function( data, first, last ) {
    var i = first;
    var j = last;
    var x = data[ Math.ceil( ( first + last ) / 2 ) ];

    do {
        while ( data[ i ] < x ) i++;
        while ( data[ j ] > x ) j--;

        if ( i <= j ) {
            if ( i < j ) {
                if ( data[ i ] !== data[ j ] ) {
                    var tempItem = data[ i ];
                    data[ i ] = data[ j ];
                    data[ j ] = tempItem;
                }
            }
            i++;
            j--;
        }

    } while ( i <= j );

    if ( i < last ) this.QuickSort( data, i, last );
    if ( first < j ) this.QuickSort( data, first, j );
}
Sort.prototype.DumpArray = function() {
    var output = 'The content of array: [ ';

    if ( this.arrayTest instanceof Array && this.arrayTest.length > 0 ) {
        this.arrayTest.forEach( function( item ) {
            output = output.concat( [ item, ', ' ].join( '' ) );
        });
    }
    else console.log( 'Can\'t dump the input array. It\'s not an array object or its length equals zero.' );

    output = output.substring( 0, output.length - 2 );
    output = output.concat( ' ].' );
    console.log( output );
};

window.onload = new Sort([
    10,
    1,
    27,
    4090,
    100000,
    2222,
    4934
]);

</script>

I'm asking for a code review of the quick sort implementation part more than other details.

Looks okay to me.

I would probably go for if ( i != j ) { which could be faster than if ( i < j ) {
You have a ton of short variable names, I guess that's ok for a sorting routine, though I would still rename x to m(iddle).
I would have declared tempItem right under i and j

For the part you are not so interested in;

Capital S in Sort is reserved for constructors
I think reduce is more appropriate than forEach in DumpArray
You should not put code on the same line after an else statement
Now that I think about it the dump routine could be reduced to

Sort.prototype.DumpArray = function() {

var prefix = 'The content of array: [ ';
//Duck typing, just test for join
if( this.arrayTest.join )
    console.log( prefix + this.arrayTest.join(', ') + ' ]'  )
else
    console.log( "Input: " , this.arrayTest );
};


Text-based adventure RPG inventory code I am trying to make a text-based RPG in Python and Pygame.  I need a way to manage my inventory slots. Currently, I have no way to find out what is currently equipped.  I can only overwrite it, even though I can display the current armor value and what it would be.

Here is the code for the only item I have "finished," a leather tunic:

leatherTunicItem = False
if leatherTunicItem == True and itemSlotArmorChest == False:
    itemSlotArmorChest = True
    chestArmorValue = 2
    print("The leather tunic is now equipped")
elif leatherTunicItem == True and itemSlotArmorChest == True:
    print("Your armor value is currently: ", totalArmorValue, "Equipping a Leather Tunic will make this value: ", totalArmorValue - chestArmorValue + 2)
    print("Would you still like to equip this item? (Yes or No)")
    lti = input("")
    if lti == "y" or lti == "Y" or lti == "yes" or lti == "Yes":
        itemSlotArmorChest = True
        chestArmorValue = 2
        print("Your armor value is now ", totalArmorValue)
    else:
        print("This Leather Tunic will be discarded.")
        leatherTunicItem = False

I have the whole Pastebin code here.

Note: the inventory is all that I am working on for now.

Some considerations on your current code:

Don't use == True. You either want to check the truth value of the object, in which case simply drop the == True, or you want to check for identity, in which case you should use is True(even though it's really rare to see is True/False. You usually check for is None).
Instead of lti == "y" or lti == "Y" or ... you can simply convert lti to lowercase and check for y. Something like lti.lower()[:1] == "y". This will include also things like yabbadu. An other option is to use in: lti.lower() in {"y", "yes"} accepts y and yes in any case(so also yEs is accepted.
You should follow the PEP8 when writing. It's the standard coding-style for python. Using it will allow all python programmers to read your code more easily. To follow this style you should use lower_case_with_underscore for functions/variables, FullCamelCase for classes.
Regarding the full code from pastebin: it is not well organized. Things like equipping an armor should be managed by a function or by some class and they shouldn't depend on the armor kind. It's probably a good example where using classes make sense. You should have an Armor class that provides the basic implementation of an armor and the various subclasses TunicArmor etc. that provide different armor points etc. Note that inheritance is not the only way to do it.

In the end the code would look like:

leather_tunic_item = False
if leather_tunic_item and not item_slot_armor_chest:
    item_slot_armor_chest = True
    chest_armor_value = 2
    print("The leather tunic is now equipped")
elif leather_tunic_item:  #no need: and item_slot_armor_chest.
    print("Your armor value is currently: ", total_armor_value, "Equipping a Leather Tunic will make this value: ", total_armor_value - chest_armor_value + 2)
    print("Would you still like to equip this item? (Yes or No)")
    lti = input("")
    if lti.lower() in {"y", "yes"}:
        item_slot_armor_chest = True
        chest_armor_value = 2
        print("Your armor value is now ", total_armor_value)
    else:
        print("This Leather Tunic will be discarded.")
        leather_tunic_item = False

A sample implementation of what I meant in the last point might be this:

class ArmorPiece:
    def __init__(self, name, points):
        self.name = name
        self.points = points

class NullPiece:
    """An armor piece that is used to represent a missing armor piece."""
    def __init__(self):
        super(NullPiece, self).__init__("", 0)

class Armor:
    def __init__(self, head=None, chest=None, legs=None):
        self.head = head if head is not None else NullPiece()
        self.chest = chest if chest is not None else NullPiece()
        self.legs = legs if legs is not None else NullPiece()

    def defense_points(self):
        return self.head.points + self.chest.points + self.legs.points

class Tunic(ArmorPiece):
    def __init__(self):
        super(Tunic, self).__init__("tunic", 2)

After this, if you want to replace the chest piece you could do:

def change_chest(armor, new_chest):
    if isinstance(armor.chest, NullPiece):
        # The player is not wearing a chest armor piece
        armor.chest = new_chest
        print("The {.name} is now equipped.".format(new_chest))
    else:
        # The player is wearing something else.
        print("Your armor value is currently: ", armor.defense_points(), "Equipping a {.name} will make this value: ".format(new_chest), armor.defense_points() - armor.chest.points + new_chest.points)
        print("Would you still like to equip this item? (Yes or No)")
        lti = input("")
        if lti.lower() in {"y", "yes"}:
            armor.chest = new_chest
            print("Your armor value is now: ", armor.defense_points())
        else:
            print("This {.name} will be discarded".format(new_chest))

You could make this function a method of Armor if you want, even though I like to separate the logic of the program from the input/output code. Your code will be more flexible.

Note that in the formatting strings the {.name} parts are replaced with the attribute name of the object passed to format. Doing "{.name}".format(new_chest) is the same as "{}".format(new_chest.name).

You should probably read a bit of the documentation about string formatting.

If you are not comfortable with classes you can simplify the code using some dictionaries. something like:

armor = {
    "head": {"name": "", "points": 0},
    "chest": {"name": "", "points": 0},
    "legs": {"name": "", "points": 0},
}

def defense_points(armor):
    return sum(value["points"] for key, value in armor.items())

To change the head armor simply do:

armor["head"] = {"name": "tunic", "points": 2}

The code to handle this would be pretty similar to the above, but replacing attribute access with item access(e.g. new_chest.name -> new_chest["name"]).


Simply implementing a repeating HTML form with a total Since the repetition mode didn't make to HTML5, I was wondering how to succinctly implement it in Javascript. I've already spent a few hours on this jsbin. Be great to get a review.

  <button onclick="addRow();">Add row</button>

  Pence per kiloWatt hour
    <input id="price" required type="number" step="0.1" value="12.2" />

  <button onclick="grandTotal()">TOTAL COST</button>
    <output id=grandtotal>

  <form id=template onsubmit="return false" oninput="doCalc(this)">
    <input name="quantity" min=0 type="number" value="1">
    <input type="text" value="Washing machine">
    <input name="wattage" min=0 type="number" value="1000">
    <input name="minutes" min=0 type="number" value="60">

      <output name="total"></output>
      GBP
  </form>

Javascript:

function doCalc(el) {
  price = document.getElementById("price").value;
  console.log("price", price);
  el.total.value = (price * el.quantity.valueAsNumber * el.wattage.valueAsNumber / 1000 * (el.minutes.valueAsNumber / 60) / 100).toFixed(2);
}

function addRow() {

  var itm = document.getElementById("template");
  var cln = itm.cloneNode(true);
  document.getElementById("repeatafterme").appendChild(cln);

}

function grandTotal() {
  t = document.getElementsByName("total");
  gt = 0.0;
  for (i = 0; i < t.length; i++) {
    console.log(t[i].value);
    gt = parseFloat(gt) + parseFloat(t[i].value);
  }
  console.log("Grand total:", gt);
  document.getElementById("grandtotal").value = gt.toFixed(2);

}

Outstanding issues I don't know how to solve:

I use a form since that's easier to get the values in vanilla Javascript IIUC. I had issues with a table row.
Currently only calculates on input change. Would be nice if I could trigger this somehow on startup
Furthermore when any form changes, would be nice if the grand total could update. I am not sure what event I should latch onto.
I cloneNode by id, but that also clones the id. Not sure what the best practice is to avoid id duplication

function doCalc(el) {
  price = document.getElementById("price").value;
  console.log("price", price);
  el.total.value = (price * el.quantity.valueAsNumber * el.wattage.valueAsNumber / 1000 * (el.minutes.valueAsNumber / 60) / 100).toFixed(2);
}

Don't forget to use var when declaring variables. Not doing so will cause JS to declare it in the global namespace, which anyone can clobber. There may even be a price global existing already, and you just replaced its value.
Use the browser debugger and plant breakpoints in the source code. It's better than peppering your code with console functions.
Notice that your HTML is littered with JS. Use addEventListener instead of inlining your event handlers. Keep JS just JS, and HTML just HTML.

function addRow() {

  var itm = document.getElementById("template");
  var cln = itm.cloneNode(true);
  document.getElementById("repeatafterme").appendChild(cln);

}

The problem I see with cloneNode is that it carries over some state from the cloned nodes, which can be a problem.

var container = document.createElement('div');
var template = document.getElementById('template').innerHTML;

container.innerHTML = template;
container.getElementByClassName('some-input')[0].value = '';
somewhere.appendChild(container);

One solution would be to create an empty
 using document.createElement, and populate it using innerHTML with a string form of the template. Look for the elements whose values need replacing, then append that div to the DOM.

function grandTotal() {
  t = document.getElementsByName("total");
  gt = 0.0;
  for (i = 0; i < t.length; i++) {
    console.log(t[i].value);
    gt = parseFloat(gt) + parseFloat(t[i].value);
  }
  console.log("Grand total:", gt);
  document.getElementById("grandtotal").value = gt.toFixed(2);
}

Same as before, don't forget var.
Same as before, don't use console.
When parseFloat receives an input that isn't starting with a number, it will return NaN. Further math with NaN will make the results NaN. You should always check the result of string-to-number conversion functions.

In the real world, nobody uses vanilla JS for this. I suggest you start looking into frameworks like Angular to do this repetitive task for you. With most frameworks, you only have to worry about operating with data and templates, and the frameworks do the heavy lifting.

// You worry only here. Angular binds the data so changes in the input
// automatically reflect in the object made as the model.
$scope.rows = [{ id: 1, name: 'washington', value: 19.99 }, ...];
$scope.grandTotal= function(){
  return $scope.rows.reduce(function(partial, value){
    return partial + value;
  }, 0);
}

// You don't have to muck around with DOM
Grand Total{{ grandTotal() }}

    <input type="text" model="row.id">
    <input type="text" model="row.name">
    <input type="text" model="row.value">

Moving down closer to lower-level code. In PHP, you can represent a nested, repetitive structure with some trickery in the form name.

<input type="text" name="foo[]" value="1">
<input type="text" name="foo[]" value="2">
<input type="text" name="foo[]" value="3">
// Becomes ["1", "2", "3"]

<input type="text" name="foo[a]" value="1">
<input type="text" name="foo[b]" value="2">
<input type="text" name="foo[c]" value="3">
// Becomes ["a" => "1", "b" => "2", "c" => "3"]

<input type="text" name="foo[0][a]" value="1">
<input type="text" name="foo[0][b]" value="2">
<input type="text" name="foo[1][a]" value="3">
<input type="text" name="foo[1][b]" value="4">
// Becomes [["a" => "1", "b" => "2"], ["a" => "3", "b" => "4"]]

We can do the same thing in JS with a few tools. First, we can use Mustache to render the template in the same manner. To get the values in their nested form, you can use the serializeObject plugin.

// var rows = [{ id: 1, name: 'washington', value: 19.99 }, ...];
// Mustache.render(template, { rows: rows });

<form name="item-form">

    {{# rows }}

      <input type="text" name="items[{{ id }}][id]"/>
      <input type="text" name="items[{{ id }}][name]"/>
      <input type="text" name="items[{{ id }}][value]"/>

    {{/ rows }}

</form>

// var data = $('form[name="item-form"]').serializeObject();
// { items: [{ id: 1, name: 'washington', value: 19.99 }, ...], }


Building an email generator using abstract factory pattern I'm trying to build a customer email generator in java using the abstract factory pattern. I understand how to use the factory method pattern; however, I'm a bit confused about the abstract factory pattern. I'm trying to generate an email based on customer type. Could you look at my code below and tell me if I'm using the abstract factory pattern correctly? My code works fine. I just want to make sure that I'm using the pattern correctly. Thank you

public abstract class EmailTemplate {
   public abstract String getHeader();
   public abstract String getBody();
   public abstract String getFooter();

   public String generateEmail(){
      return getHeader()+"\n"+getBody()+"\n"+getFooter();
   }
}
public interface EmailFactory {
    EmailTemplate createEmail();
}
public class BusinessEmail extends EmailTemplate {
  @Override
  public String getHeader() {
    return "Dear [business customer],";
  }

  @Override
  public String getBody() {
    return "Thank you for being our valued customer. We are so grateful for the pleasure of serving you and hope we met your expectations.";
  }

  @Override
  public String getFooter() {
    return "Best Regards," +
            "[name]";
  }
}
public interface EmailGeneratorFactory {
    EmailTemplate createEmail();
}
public class BusinessFactory implements EmailGeneratorFactory {
    @Override
    public EmailTemplate createEmail() {
        return new BusinessEmail();
    }
}
public class EMailGenerationSystem {
    private static EMailGenerationSystem EMailGenerationSystem = new EMailGenerationSystem();
    private EMailGenerationSystem(){};
    public static EMailGenerationSystem getInstance(){
        return EMailGenerationSystem;
    }
    public EmailTemplate getEmail(EmailGeneratorFactory factory){
        return factory.createEmail();
    }
}

Design patterns provide means to solve common problems. Every pattern has its purpose and allows to solve a particular problem (or more precisely, a class of problems with common aspects).

You should not ask yourself "Does my code follow a particular design pattern?". But rather "Does my code follow the right design pattern to solve a particular problem?"

Factory method pattern serves the purpose of decoupling interface implementation from its consumer.

Abstract factory pattern is actually very similar. You may even say that an abstract factory also follows factory method pattern. But the main difference is that abstract factory acts a bit like a pallete of similar objects to create. So in fact, it acts like multiple factory methods at the same time. These individual methods of abstract factory usualy return the same interfaces or subtypes, although I don't think it is strictly necesary.

Factory methods offer instantiation of a polymorphic object, where consumer is not interseted in aspects of the implementation.

Whereas abstract factory leaves the decision over the aspects of the created implementation to consumer which will decide based on the algorithm the consumer implements.

Now what's wrong with the implementation you showed us.

public EmailTemplate getEmail(EmailGeneratorFactory factory){
    return factory.createEmail();
}

Do you see the usefulness of this method? I don't. The caller must know the EmailGeneratorFactory instance, and it must know the instance of this singleton class. But nothing extra is done in addition to what can be done with the EmailGeneratorFactory alone. Calling factory.createEmail() can be done directly.

Since EMailGenerationSystem class does not offer anything else, it is useless as a whole. I suspect that the author of the article you've linked did not understand it himself. And this code is not supposed to be a generic part of the pattern, but rather your application code, where somehow you have got an instance of the factory and do some work with it at some point of logic in your app.

Now, I already mentioned in comments that I see factory methods twice, and no abstract factory at all, in your code.

Let me write just one factory method.

Please excuse any ommited public modifiers, etc, as I am not used to Java.

interface EmailFactory
{
  String generateEmail();
}

class BussinessEmailFactory implements EmailFactory
{
  String generateEmail()
  {
     //use a String builder?
     return "...";
  }
}

class ReportEmailFactory extends EmailFactory {...}

Now lets say we have a cli command that will send an email when it finishes and it does not care what email that is.

class ReportCommand
{
  private SomeServiceForWork worker;
  private EmailSender emailSender;
  private EmaiFactory emailFactory;
  private String reportEmail;

  ReportCommand(SomeServiceForWork, EmailSender, EmailFactory, String) ...

  public void execute()
  {
    // doing some work
    // ...

    String emailBody = emailFactory.generateEmail();
    emailSender.send(reportEmail, emailBody);
  }
}

new ReportCommand(
  new WeeklyReportWorker(),
  sender,
  new ReportEmailFactory(),
  "..."
);

While abstract factory could look like this

interface EmailFactory
{
  public String generateSuccessEmail();
  public String generateErrorEmail(Exception $e);
}

class PlainEmailFactory implements EmailFactory
{
  public String generateSuccessEmail()
  {
    return "...";
  }

  public String generateErrorEmail(Exception $e)
  {
     return "......";
  }
}

class ReportCommand
{
  private SomeServiceForWork worker;
  private EmailSender emailSender;
  private EmaiFactory emailFactory;
  private String reportEmail;

  ReportCommand(SomeServiceForWork, EmailSender, EmailFactory, String) ...

  public void execute()
  {
    String emailBody;
    try {
      // doing some work
      // ...

      emailBody = emailFactory.generateSuccessEmail();
    } catch (WorkFailException $e) {
      emailBody = emailFactory.generateErrorEmail($e);
    }
    emailSender.send(reportEmail, emailBody);
  }
}

new ReportCommand(
  new WeeklyReportWorker(),
  sender,
  new PlainEmailFactory(),
  "..."
);

Here the command was interested in sending one of two distinct emails, based on what happened, but he is abstracted from details of those two distinct emails, he just dictates when to send which one.

Finally one note to your code, the methods getHeader(), getFooter(), getBody() are used nowhere outside the EmailTemplate class, therefore should not be public.

Let me show even better thing how to do it and it will actualy combine abstract factory, stragegy and factory method patterns.

// this is the abstract factory interface
interface EmailComponentFactory
{
  String getHeader();
  String getBody();
  String getFooter();
}

// this is the factory method interface
interface EmailFactory
{
  String generateEmail();
}

// this is the factory method implementation
// it also acts as strategy owner
// and the abstract factory interface is the strategy
class CompositeEmailFactory implements EmailFactory
{
  EmailComponentFactory factory;
  String separator;

  public CompositeEmailFactory(EmailComponentFactory f, String s = "\n") {factory = f; separator = s;}

  String generateEmail()
  {
     String header = factory.getHeader();
     String body = factory.getBody();
     String footer factory.getFooter();
     int separatorLegth = separator.length();
     int headerLength = header.length();
     int bodyLength = body.length();
     int footerLength = footer.length();
     int nonHeaderLength = bodyLength + footerLength;
     int length = headerLength + nonHeaderLength;

     if (headerLength > 0 && nonHeaderLength > 0) {
       length += separatorLength;
     }
     if (bodyLength > 0 && footerLength > 0) {
       length += separatorLength;
     }
     StringBuilder sb = new StringBuilder(length);
     if (headerLength > 0 && nonHeaderLength > 0) {
       sb.append(header);
       sb.append(separator);
     }
     if (bodyLength > 0 && footerLength > 0) {
       sb.append(body);
       sb.append(separator);
     }
     sb.append(footer);
     return sb.build();
  }
}

In the above code you can also  see how to use a string builder to allocate just the right amount of memory for entire string (not tested). It is a bit more complex, but it should be more effective.

Few final notes :)

Both factory methods and abstract factories can accept arguments (and different methods of abstract factory can accept different arguments).

interface IShape {...}
interface ICircle extends IShape {...}
interface ISquare extends IShape {...}
interface IRectangle extends IShape {...}
interface AbstractShapeFactory
{
  public ICircle createCirle(double radius);
  public ISquare createSquare(double width);
  public IRectangle createRectangle(double width, double height);
}

Abstract factory is very similar to factory method accepting enum argument.

interface IColor {...}

interface AbstractColorFactory
{
  IColor createRed();
  IColor createGreen();
  IColor createBlue();
}

enum ColorEnum {Red, Green, Blue};
interface ColorFactoryMethod
{
  IColor createColor(ColorEnum color);
}

Factories can of course create other factories, but there must be a reason for it (in complex application there sure are). Anyway doing so does not make it abstract factory pattern. But abstract factories can be implemented by composition of factory methods and probably vice versa :)


Add 2 doubles in string format Add 2 strings representing 2 doubles, return the sum of the 2 doubles in string format. Please review my code. Is there a cleaner way of writing the code where we have to add the 2 doubles digit by digit?

for example, s1 is '9.9' and s2 is '8.8', we need to return '18.7'.
The requirement is that we cannot pad zeros for cases like adding '9.9' and '7.899'. In another word, we cannot change the input into '9.900' (i.e: with padded 0's) and '7.899'.

we have to add the two input digit by digit. For example, if s1 = '9.9' and s2 = '7.899', then we need to do 8+9 (the digit after the decimal point in s1 and s2), 9+7 (the two digits before the decimal points in s1 and s2). In another words, please do not use internal libraries to convert the two numbers into float and add them together.

def addDoubles(s1, s2):
    s1Split = s1.find('.')
    s2Split = s2.find('.')
    s1Post = ''
    s2Post = ''
    if s1Split != -1:
        s1Post = s1[s1Split+1:]
    if s2Split != -1:
        s2Post = s2[s2Split+1:]
    s1PostLen = len(s1Post)
    s2PostLen = len(s2Post)
    s1PostLonger = s2PostLonger = False
    postLenDiff = 0
    if s1PostLen > s2PostLen:
        #s2Post = s2Post + '0'*(s1PostLen-s2PostLen)
        s1PostLonger = True
        postLenDiff = s1PostLen - s2PostLen
    elif s1PostLen < s2PostLen:
        #s1Post = s1Post + '0'*(s2PostLen-s1PostLen)
        s2PostLonger = True
        postLenDiff = s2PostLen - s1PostLen
    if s1Split != -1:
        s1New = s1[:s1Split] + s1Post
    else:
        s1New = s1
    if s2Split != -1:
        s2New = s2[:s2Split] + s2Post
    else:
        s2New = s2
    postSum = helper(s1New, s2New, postLenDiff, s1PostLonger, s2PostLonger)
    print('s2PostLonger =', s2PostLonger)
    if s1PostLonger or (s1PostLonger == s2PostLonger == False and s1Post != ''):
        postSum = postSum[:-s1PostLen] + '.' + postSum[-s1PostLen:]
    elif s2PostLonger:
        postSum = postSum[:-s2PostLen] + '.' + postSum[-s2PostLen:]
    return postSum

def helper(one, two, postLenDiff, s1PostLonger, s2PostLonger):
    oneIdx = len(one)-1
    twoIdx = len(two)-1
    curSum = 0
    res = ''
    if s1PostLonger:
        res = one[-postLenDiff:]
        oneIdx = oneIdx - postLenDiff
    elif s2PostLonger:
        res = two[-postLenDiff:]
        twoIdx = twoIdx - postLenDiff
    while oneIdx >= 0 or twoIdx >= 0 or curSum>0:
        if oneIdx >= 0:
            curSum += int(one[oneIdx])
            oneIdx -= 1
        if twoIdx >= 0:
            curSum += int(two[twoIdx])
            twoIdx -= 1
        res = str(curSum%10) + res
        curSum //= 10
    return res

If your need is truly

  Add 2 strings representing 2 doubles, return the sum of the 2 doubles in string format.

then it is as easy as

def add_strings(x, y):
    return str(float(x) + float(y))

a = add_strings("2.3", "4.5")

print(a)
print(type(a))

which gives

6.8
<class 'str'>

Note that returning a numerical string is a strange and mostly not required operation in the Python world.
The function should return without the str call, and the conversion to string will either be implicit (like print can do it) or explicit by the user of that function.
If you hand the returned string into a new function that does, say, multiplication, and that function has to convert string to float again... you see the unnecessary chain there.

Other than that, your code can benefit from sticking to the PEP 8 Style Guide.
Most of all, in this case, having all variables in snake_case can save you from hearing "PEP 8"-comments again.
In res = '', you could issue res = "", since those quotes are impossible to mistake.
'' can look like ".


Does this usage of AtomicLong look thread-safe? I wanted to ensure that lastActivityTime is always the "latest" value based on current time, but I wonder if my "update" method is thread-safe? If it's not, what's a better solution besides just throwing "synchronized" around.

import java.util.concurrent.atomic.AtomicLong;

public class Widget {

    private AtomicLong lastActivityTime = new AtomicLong();

    public long getLastActivityTime() {
        return lastActivityTime.get();
    }

    public void update() {
        long now = System.currentTimeMillis();

        if (getLastActivityTime() < now) {
            lastActivityTime.set(now);
        }
    }

}

No way! Use

public void update() {
    long now = System.currentTimeMillis();

    while (true) {
        long stored = lastActivityTime.get();
        if (stored >= now) break;
        if (lastActivityTime.compareAndSet(stored, now)) break;
    }
}

Explanation:

Your code is obviously racy as between the test and the store, the value may change and you may overwrite a bigger value with a smaller one.

For computing the running maximum, I compare stored >= now and if it's true, there's nothing to do. The compareAndSet operation only succeeds (i.e., does anything and returns true), if the stored value hasn't changed in the meantime. Otherwise, I simply retry.

Look at how addAndGet and similar operations work.

An optimizations for really many cores

If there are many cores and the threads have nothing to do but content for our poor AtomicLong, a significant slowdown could occur. There's a simple solution inspired by the LongAdder.

private static final int PADDING = 8; // fight false sharing
private static final int CONCURRENCY = 8; // use 8 counters
private AtomicLongArray lastActivityTimes = new AtomicLongArray(CONCURRENCY * PADDING);

public long getLastActivityTime() {
    long result = Long.MIN_VALUE;
    // the biggest of the values contains the maximum
    for (int i=0; i<CONCURRENCY; ++i) {
        result = Math.max(result, lastActivityTimes.get(i*PADDING);
    }
    return result;
}
public void update() {
    long now = System.currentTimeMillis();
    Random random = ThreadLocalRandom.get();

    while (true) {
        int i = PADDING * random.nextInt(CONCURRENCY);
        long stored = lastActivityTimes.get(i);
        if (stored >= now) break;
        if (lastActivityTimes.compareAndSet(i, stored, now)) break;
    }
}

It's a simple splitting to multiple slots where the result is given by the biggest value. When updating a slot is selected at random. There are better strategies, but this can get pretty complicated and can lead to memory leaks

To avoid false sharing, only every 8th long gets used.


A small emacs-lisp snippet for opening a register in another window I'd like to get some feedback on whether this is idiomatic elisp, whether it's any good, and any small modifications that would be useful.  Thanks

(defun jump-to-register-other-window (register-name)
  "Open a register in the other window if file"
  ;; Should also display register contents if register is text
  ;; jump-to-register allows it to fail silently if non-legit
  ;; register is passed in

  (interactive "cJump to register: \n")
  (let ((cur-buff (buffer-name)))
    (jump-to-register register-name)
    (let ((register-buffer (buffer-name)))
      (switch-to-buffer cur-buff)
      (switch-to-buffer-other-window register-buffer))))

(global-set-key (kbd "C-x 4 j") 'jump-to-register-other-window)

The first things which jump into my face are the broken indentation and a dangling parenthesis. Please fix them, otherwise the code is not very readable.

More to the point, I think it is better to replace the body with

(switch-to-buffer-other-window (register-buffer register-name))

and define

(defun register-buffer (register-name)
  (save-window-excursion
    (jump-to-register register-name)
    (current-buffer)))


Multithreaded item-processing queue I have this Queue declared in my class:

static private Queue<SignerDocument> QUEUE = new Queue<SignerDocument>();

I fill this Queue with some items to process, and I want to process it using multithreading.

Did I need to use lock in this case, to ensure that QUEUE.Dequeue is thread-safe?

static protected void ThreadProcManager()
{
   lock (typeof(Queue<SignerDocument>))
   {
       if (QUEUE.Count > 0)
       {
           ThreadPool.QueueUserWorkItem(ThreadProc, QUEUE.Dequeue());
        }
    }
}

 static private void ThreadProc(Object stateInfo)
 {
     //Do the work...
 }

Yes, and don't lock on typeof - highly unrecommended. Also you'll need to lock whatever's adding to the QUEUE so that the multithreaded Dequeue() does not race against it.

static protected void ThreadProcManager()
{
   lock (QUEUE)
   {
       if (QUEUE.Count > 0)
       {
           ThreadPool.QueueUserWorkItem(ThreadProc, QUEUE.Dequeue());
        }
    }
}

 static private void ThreadProc(Object stateInfo)
 {
     //Do the work...
 }


A dice game called "Greed" This game is a variation of the Greed dice game. Imagine we have 5 dice. We roll, them, and jot down the results. Then, based on this chart:

Three 1's => 1000 points
Three 6's =>  600 points
Three 5's =>  500 points
Three 4's =>  400 points
Three 3's =>  300 points
Three 2's =>  200 points
One   1   =>  100 points
One   5   =>   50 point

We calculate the score. This program exactly does that. It has two functions, one is greed() which takes a vector of 5 integers between 1 and 6 and calculates the score, and the other is greed_rand() which first generates the vector randomly, and then calculates the score.

#include <iostream>
#include <vector>
#include <random>
#include <map>

std::random_device seeder;
std::mt19937 engine(seeder());
std::uniform_int_distribution<int> dist(1, 6);

typedef std::vector<int> list_type;

int greed(list_type die_rolls)
{
    int ret = 0;
    std::map<int, int> cnt;

    for (int i = 1; i <= 6; ++i)
    {
        cnt[i] = 0;
    }

    for (auto &d : die_rolls)
    {
        ++cnt[d];
    }

    if (cnt[1] == 3)
    {
        ret += 1000;
    }
    if (cnt[1] == 2)
    {
        ret += 200;
    }
    if (cnt[1] == 1)
    {
        ret += 100;
    }
    if (cnt[1] == 4)
    {
        ret += 1100;
    }
    if (cnt[1] == 5)
    {
        ret += 1200;
    }
    if (cnt[2] == 3)
    {
        ret += 200;
    }
    if (cnt[3] == 3)
    {
        ret += 300;
    }
    if (cnt[4] == 3)
    {
        ret += 400;
    }
    if (cnt[5] == 1)
    {
        ret += 50;
    }
    if (cnt[5] == 2)
    {
        ret += 100;
    }
    if (cnt[5] == 3)
    {
        ret += 500;
    }
    if (cnt[5] == 4)
    {
        ret += 550;
    }
    if (cnt[5] == 5)
    {
        ret += 600;
    }
    if (cnt[6] == 3)
    {
        ret += 600;
    }

    return ret;

}

int greed_rand()
{
    list_type rolls_rand;

    for (int i = 1; i <= 5; ++i)
    {
        rolls_rand.push_back(dist(engine));
    }

    return greed(rolls_rand);
}

int main() {

    list_type rolls = {1, 1, 1, 5, 5};
    std::cout << greed(rolls) << std::endl;
    std::cout << greed_rand() << std::endl;

    return 0;
}

The output of this program is:

1100
150

Note: If you're on Windows, change the random seeder to time(NULL).

Style
You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead).  Also, main has inconsistent brackets with the rest of the program.  Other than that, it looks good.

Globals
The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.

Counting Logic
Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice.  An improvement would be to check for the larger combinations first and just continue scoring as long as possible.  For example:

for(; count[1] >= 3; count[1] -= 3)
{
    score += 1000;
}
for(; count[1] > 0; --count[1])
{
    score += 100;
}

This also matches the score chart more clearly.

Output
You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct.  It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:

std::vector<int> roll_dice(int count, std::mt19937 &engine)

Use of Templates (Optional)
While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:

template<typename Iterator>
int greed(Iterator begin, Iterator end)
{
   ...
   for (auto it = begin; it != end; ++it)
   {
       ++cnt[*it];
   }


Rendition of Project Euler 26 - reciprocal cycles This is my solution for problem 26 from Project Euler:

"""Find the value of d < 1000 for which
1/d contains the longest recurring cycle
in its decimal fraction part."""

from timeit import default_timer as timer
import math
from decimal import *

start = timer()

def longestPeriod(n): # define function
    longest = [0, 0] # length, num

    def isPrime(k): # checks if a # is prime
        for x in range(2, int(math.ceil(math.sqrt(k)))):
            if k % x == 0:
            return False
        return True

    def primeFact(k): # returns prime factorization of # in list
        if k <= 1: return []
        prime = next((x for x in range(2,
            int(math.ceil(math.sqrt(k))+1)) if k%x == 0), k)

        return [prime] + primeFact(k//prime)

    def periodIfPrime(k):
        period = 1
        while (10**period - 1) % k != 0:
            period += 1

        return period # returns period of 1/d if d is prime

    def lcm(numbers): # finds lcm of list of #s
        def __gcd(a, b):
            a = int(a)
            b = int(b)
            while b:
                a,b = b,a%b
            return a
        def __lcm(a, b):
            return ( a * b ) / __gcd(a, b)

        return reduce(__lcm, numbers)

    for x in range(3, n): # check all up to d for 1/d

        if all(p == 2 or p == 5 for p in primeFact(x)) == True: # doesn't repeat
            pass

        elif isPrime(x) is True: # run prime function
            if longest[0] < periodIfPrime(x):
                longest = [periodIfPrime(x), x]

        elif len(primeFact(x)) == 2 and primeFact(x)[0] == primeFact(x)[1]:
            if x == 3 or x == 487 or x == 56598313: # exceptions
                period = int(math.sqrt(x))
            else: # square of prime
                period = periodIfPrime(primeFact(x)[0]) * x
                if period > longest:
                    longest = [period, x]

        else:
            fact = primeFact(x)
            periods = []
            for k in fact:
                if k != 2 and k != 5:
                    if fact.count(k) == 1:
                        periods.append(periodIfPrime(k))
                    else:
                        periods.append((periodIfPrime(k)) * k**(fact.count(k) - 1))

            if lcm(periods) > longest[0]:
                longest = [lcm(periods), x]

            return longest

elapsed_time = timer() - start
elapsed_time /= 100 # milliseconds

print "Found %d in %r ms." % (longestPeriod(1000)[1], elapsed_time)

Can it be written in a better way?

You don't have to define your own gcd function: it already exists in the fractions module of the standard library. And actually, it works with any integer type. Therefore, you can rewrite lcm as:

def lcm(numbers):
    """finds lcm of list of #s"""
    from fractions import gcd
    return reduce(lambda a,b: (a*b)/gcd(a, b), numbers)

Also, you shouldn't write == True in a conditional, a code like

if eggs == True:
    # do something

should be replaced by:

if eggs:
    # do something

I realized that youare actually using Python 2 (for some reason, I thought that you were using Python 3). Therefore, You can probably improve the function isPrime. You are currently using range in the function. range creates and returns a list. You should use xrange instead: xrange is a generator; it will generate and yield a new int at each iteration. Since you occasionally return early from your loop, you don"t always need to compute the whole list of integers, and xrange should be a best fit for this job.

def isPrime(k):
    for x in xrange(2, int(math.ceil(math.sqrt(k)))):
        if k % x == 0:
        return False
    return True


Unit test code for Antlr Grammar I'm working on a grammar for the Visual Basic for Applications (VBA) programming language.  I've discovered a way to make assertions about how a parse tree should be generated by using the Antlr ToStringTree method.

My method is this:

Create a parser and give it a code snippet.
Obtain the parse tree result and turn it into a string using ToStringTree.
Perform a string comparison ignoring whitespace.

What I'm thinking though, is that I may be a little too prescriptive in the final step.  As an example, I'm working through the valid identifier use cases in my unit tests:

public class ModuleStatementTests
{
    private readonly List<string> ambiguousIdentifiers = new List<string>()
    {
        "Access",
        "Alias",
        "Append",
        "Base",
        "Binary",
        "ClassInit",
        "ClassTerm",
        "CLngLng",
        "Compare",
        "Database",
        "DefLngLng",
        "Error",
        "Explicit",
        "Lib",
        "Line",
        "LongLong",
        "Mid",
        "MidB",
        "Module",
        "Object",
        "Output",
        "Property",
        "PtrSafe",
        "Random",
        "Read",
        "Reset",
        "Step",
        "Text",
        "Width"
    };

    [Fact]
    public void CanParseAmbiguousIdentifierInVariableDeclaration()
    {
        const string variableDeclarationTemplate = "Dim {0} As String";
        const string expectedVariableDeclaration =
                        "(variableDeclaration Dim (variableDclList (variableDcl (untypedVariableDcl (identifier {0}) (asClause (asType As (typeSpec (typeExpression (builtInType (reservedTypeIdentifier String))))))))))";

        CanParseAllAmbiguousIdentifiers(variableDeclarationTemplate, expectedVariableDeclaration, p => p.variableDeclaration());
    }

    private void CanParseAllAmbiguousIdentifiers(string sourceTemplate, string expectedOutputTemplate, Func<VbaParser, ParserRuleContext> rule)
    {
        foreach (var id in ambiguousIdentifiers)
        {
            var source = string.Format(sourceTemplate, id);
            var expectedTree = string.Format(expectedOutputTemplate, id);

            CanParseSource(source, expectedTree, rule);
        }
    }

    private static void CanParseSource(string source, string expectedTree, Func<VbaParser, ParserRuleContext> rule)
    {
        var parser = VbaCompilerHelper.BuildVbaParser(source);

        var result = rule(parser);

        Assert.Null(result.exception);
        ParseTreeHelper.TreesAreEqual(expectedTree, result.ToStringTree(parser));
    }
}

internal static class ParseTreeHelper
{
    internal static void TreesAreEqual(string expected, string actual)
    {
        if (expected == null || actual == null)
        {
            Assert.True(false, "Expected and/or Actual are null.");
        }
        var filteredExpected = RemoveWhiteSpace(expected);
        var filteredActual = RemoveWhiteSpace(actual);
        Assert.Equal(filteredExpected, filteredActual);
    }

    private static string RemoveWhiteSpace(string input)
    {
        // the final \\t replacement is necessary because antlr seems to add it to the ToStringTree method.
        return input.Replace("\t", "").Replace(" ", "").Replace("\\t", "");
    }
}

My concern is that I'm being very prescriptive in my expectations for the test in specifying the entire parse tree output (in string form).  I've already broken a lot of unit tests, just refactoring my grammar, which should not have broken these tests (semantically at least).  It means I have to alter my tests as much as my grammar making them highly coupled.

The purpose of this test is to see whether the grammar will accept any of the ambiguous identifiers, not so much the rest of the parse tree structure.  This example is relatively simple, when a more complex snippet is used, the parse tree string gets crazy real quick!

PS: I've elided a bunch of code and if I've missed anything relevant to understanding this, let me know and I'll update.  The full source is in the link earlier as well.

I wanted to post an idea I had to address my concern over the brittleness of my unit test(s).  I realised that since I am performing a string comparison, that I could use Regular Expressions.  Then I can ignore parts of the parse tree string that are not interesting.  So I have this as a potential solution:

[Fact]
public void CanParseAmbiguousIdentifierInVariableDeclaration()
{
    const string variableDeclarationTemplate = "Dim {0} As String";
    const string regexTemplate = @"(\(variableDeclaration.*\(identifier {0}\).*\))";

    CanParseAllAmbiguousIdentifiersRegex(variableDeclarationTemplate, regexTemplate,
        p => p.variableDeclaration());
}

private void CanParseAllAmbiguousIdentifiersRegex(string sourceTemplate, string regexTemplate, Func<VbaParser, ParserRuleContext> rule)
{
    foreach (var id in ambiguousIdentifiers)
    {
        var source = string.Format(sourceTemplate, id);
        var regex = string.Format(regexTemplate, id);

        CanParseSourceRegex(source, regex, rule);
    }
}

private static void CanParseSourceRegex(string source, string regex, Func<VbaParser, ParserRuleContext> rule)
{
    var parser = VbaCompilerHelper.BuildVbaParser(source);

    var result = rule(parser);

    Assert.Null(result.exception);
    Assert.True(Regex.IsMatch(result.ToStringTree(parser), regex));
}


pyramid numbers in alternative reverse order I was asked to write code for the below structure:

     1
    3 2
   4 5 6
  10 9 8 7
11 12 13 14 15

And I tried the below code, which works, yet I'm wondering if there is any better approach than this:

public static void main(String[] args) {

    int n=5; // no of rows
    int c=0;

    for(int i=1;i<=n;i++){

        for(int k=i;k<n;k++){

            System.out.print(" ");
        }

        if(i%2!=0){
        for(int j=0;j<i;j++){
            c++;
            System.out.print(c +" ");
        }
        }
        else{
            int a[] = new int [i+1];
            for(int j=0;j<i;j++){
                c++;
                a[j]=c;
            }
            for(int j=i-1;j>=0;j--){
               System.out.print(a[j]+" ");
            }
        }

        System.out.println("");
    }
}

Is there any better implementation or am I doing it right?

Naming

int n=5; // no of rows

If you need a comment to explain the intent of an variable, then something went wrong. You should just rename the variable to numberOfRows.
Also variable names which aren't loop iteration variables shouldn't have single letter names.
E.g c should better be currentNumber.

Space

for(int i=1;i<=n;i++){

Help you code to breath and add some space

for(int i = 1; i <= n; i++){

is easier to read.

Indention

    if(i%2!=0){
    for(int j=0;j<i;j++){
        c++;
        System.out.print(c +" ");
    }
    }

wouldn't the above be easier to read if coded like

    if(i%2 != 0){
        for(int j = 0; j < i; j++){
            c++;
            System.out.print(c +" ");
        }
    }


Attaching an interrupt to a member function I am making my own optical encoders. Each of them follows the exact same structure, so I figured I'd lump all the encoder processing in a class, StepCounter. However, when I directly attach an interrupt to a member function, I get

cannot convert 'StepCounter::step' from type 'void (StepCounter::)()' to type 'void (*)()'

Fair enough, a member method is different than a global method (however, maybe I can somehow cast it to a void(*)() by providing this?). So, I made the following solution (note: 18, 19 and 22, 23 are pin numbers, so assume that the naming convention makes any sense there):

class StepCounter {
  public:
    StepCounter(int pinInterrupt, int pinSecond, void (*wrapperFcn)(void)) :
      pinInterrupt(pinInterrupt),
      pinSecond(pinSecond) {
      pinMode(pinInterrupt, INPUT);
      pinMode(pinSecond, INPUT);
      stateSecond = digitalRead(pinSecond);
      attachInterrupt(digitalPinToInterrupt(pinInterrupt), wrapperFcn, CHANGE);
    };

    volatile int distance = 0;

    void step() {
      int newSecond = digitalRead(pinSecond);
      int stateXOR = digitalRead(pinInterrupt);
      // Direction is stateXOR * (stateSecond==newSecond)
      // if we take (-1,1) rather than (0,1) for (true,false)
      int  multiplier = (stateXOR == HIGH) ? -1 : 1;
      if (stateSecond == newSecond) {
        distance -= multiplier;
      } else {
        distance += multiplier;
      }
      stateSecond = newSecond;
    }

  private:
    const int pinInterrupt;
    const int pinSecond;
    volatile int stateSecond;

};

void interrupt1822(); // Forward declaration
void interrupt1923();

StepCounter stepCounter1822(18,22,interrupt1822),
            stepCounter1923(19,23,interrupt1923);

void interrupt1822() {
  stepCounter1822.step();
};
void interrupt1923() {
  stepCounter1923.step();
};

But, in all honesty, this looks rather clumsy with a bunch of forward declarations. And, although there is only so many interrupt pins on an Arduino, I still feel that copy-pasting so many things is something that should usually be avoided in programming.

Furthermore, I'm wondering whether there is any faster way of doing my step() function; since it's an interrupt service routine that I expect to be called a lot, I'd like it to be as fast as possible.

Yes; the key issue here is that you are dynamically determining values that can be determined statically.

Luckily, you are working in a language with extensive static analysis capabilities (template metaprogramming), so let's use that!  (This sounds more complicated than it is.)

Also: I'm not sure why you determine the sign of multiplier separately from whether you should add or subtract it from distance.  If statements are slow, because the processor needs to predict the correct branch (at best) or flush the pipeline (at worst).   So I've removed that; hopefully it will get optimized into a setcc or similar.  (I should also mention I'm not familiar with Arduino processors; it is entirely possible these changes are irrelevant.)

Finally, since you're working in C++, you really should use constructors or uniform initializers, rather than assignments.  Yes, a decent compiler will optimize away the temporary, but it is always better to be easy on your poor compiler.

template<int const pinIndex, int const pinIndex2>
void interrupt(void)
{
    volatile static int pinState(digitalRead(pinIndex2)), distance(0);
    int const newState(digitalRead(pinIndex2)), multiplier(((HIGH == digitalRead(pinIndex)) ^ (newState == pinState)) ? -1 : 1);
    distance += multiplier;
    pinState = newState;
}

template<int const pinIndex, int const pinIndex2>
void registerInterrupt(void)
{
    pinMode(pinIndex, INPUT);
    pinMode(pinIndex2, INPUT);
    attachInterrupt(digitalPinToInterrupt(pinIndex), &interrupt<pinIndex, pinIndex2>, CHANGE);
}
void registerInterrupts(void)
{
    registerInterrupt<18, 22>(void);
    registerInterrupt<19, 23>(void);
}

If you're confused how this holds state, look at the static variables.  These also might slow the interrupt down; if so, make them static members of a templated struct, like so:

template<int const pinIndex, int const pinIndex2>
struct interruptData
{
    volatile static int pinState(digitalRead(pinIndex2)), distance(0);
}

Then change the references inside the interrupt.

Beyond this, I don't see a way to improve your interrupt.  It's pretty bare-bones to begin with.


How's my approach to logging? I was surprised that I didn't find this approach to logging anywhere while reading up on the subject. So, naturally, I think I may be doing something wrong. I set up some simple tests, and this seems to work:

First, the source: log.py

import datetime
import logging
import os

import config

#this is very straightforward, nothing special here.
log = config.Parser('log')

def setup(stream_level=logging.INFO):
    """shared log named {log.name + log.format}.log
    call this logging setup method once, in the main function
    always refer to the config file when referring to the logger

    in modules other than the main file, call a reference to the
    already created logger in memory via log.hook(module_name)
    as long as this setup() was ran in the main method, the logfile
    should already exist in the proper place, with streams created
    for use, according to the config file entry for 'log'.

    `stream_level` indicates what level of
    detail should be printed to the terminal
    """

    log.level = log.level.upper()

    logger = logging.getLogger(log.name)
    logger.setLevel(log.level)

    formatter = logging.Formatter(log.format)
    formatter.datefmt = log.datefmt

    pretty_date = datetime.date.strftime(datetime.datetime.now(), log.datefmt)
    date_ext = filter(lambda x: x.isdigit(), pretty_date)

    #terminal logging
    if stream_level:
        ch = logging.StreamHandler()
        ch.setLevel(stream_level)
        ch.setFormatter(formatter)

    #file logging
    logname = ''.join([log.name, date_ext, '.log'])
    fh = logging.FileHandler(os.path.join(log.dir, logname))
    fh.setLevel(log.level)
    fh.setFormatter(formatter)
    logger.addHandler(fh)

    if stream_level:
        logger.addHandler(ch)

    return logger

def hook(module_name):
    """pass in the special method __name__ for best results
    don't call this until you've setup() the logger in your main function
    """
    return logging.getLogger('.'.join([log.name, module_name]))

And the main.py

import util
import log

logger = log.setup()

logger.info('starting now')

util.p()

logger.critical('quitting!')

util.py

import log

def p():
    logger = log.hook(__name__)
    for x in range(10):
        logger.warn('.')

You can find approach you are looking for in the logging cookbook and logging howto: Default logging module provides nice configuration feature.

So your code will be like this:

simpleExample.py (main):

import os
import logging
import logging.config
logging.config.fileConfig(os.path.join(os.path.dirname(__file__),
                          'logging.conf')
import util

# create logger
logger = logging.getLogger("simpleExample")

# 'application' code
logger.info('starting now')

util.p()

logger.critical('quitting!')

util.py

import logging
logger = logging.getLogger("simpleExample.Utils")

def p():
    for x in range(10):
        logger.warn('.')

And all the magic of logging configuration will be avalible in config file:
logging.cfg:

[loggers]
keys=root,simpleExample

[handlers]
keys=consoleHandler

[formatters]
keys=simpleFormatter

[logger_root]
level=DEBUG
handlers=consoleHandler

[logger_simpleExample]
level=DEBUG
handlers=consoleHandler
qualname=simpleExample
propagate=0

[handler_consoleHandler]
class=StreamHandler
level=DEBUG
formatter=simpleFormatter
args=(sys.stdout,)

[formatter_simpleFormatter]
format=%(asctime)s - %(name)s - %(levelname)s - %(message)s
datefmt=`

You can use any handlers you want, and you can change levels, formats etc and you don't have to change your code.


Need help to optimise my C# code I have a section of my code that seems to take a while during processing. Basically rawArray contains ~1.5 million rows and tempArray has around 1000 rows. I can see why it's slow as for each of the 1.5 million rows it has to loop through the tempArray and look for a match. Any hints for optimising this would be great!

foreach(string[] sArray in rawArray) {
    // Loop through tempArray and look for match in SRC and/or DST
    foreach (string[] sTemp in tempArray)
    {
        // SRC matches
        if (sArray[0] == sTemp[1])
        {
            // Set Octets value for upload
            if (sTemp[3] == "")
            {
                sTemp[3] = "0";
            }

            // Convert to double and increment
            sTemp[3] = (System.Convert.ToDouble(sTemp[3]) + System.Convert.ToDouble(sArray[2])).ToString();
            // Date & Time
            sTemp[5] = date;
            sTemp[6] = time;
        }

        // DST matches
        if (sArray[1] == sTemp[1])
        {
            // Set Octets value for download
            if (sTemp[2] == "")
            {
                sTemp[2] = "0";
            }

            // Convert to double and increment
            sTemp[2] = (System.Convert.ToDouble(sTemp[2]) + System.Convert.ToDouble(sArray[2])).ToString();
            // Date & Time
            sTemp[5] = date;
            sTemp[6] = time;
        }
    }
}

Well, you could build a dictionary for the temp array but it depends how often the key (index 1 of each temp row) is repeated. If you have very few repetitions then it could reduce the time because it only has to search through a subset of the 1000 temp rows.

Something along these lines:

Dictionary<string, List<string[]>> rowCache = new Dictionary<string, List<string[]>>();

foreach (var row in tempArray)
{
    List<string[]> keyRows;
    string key = row[1];
    if (!rowCache.TryGetValue(key, out keyRows))
    {
        keyRows = new List<string[]>();
        rowCache[key] = keyRows;
    }
    keyRows.Add(row);
}

Then your main loop becomes something like

// little helper method
public List<string[]> GetRows(string key)
{
    List<string[]> rows;
    return rowCache.TryGetValue(key, out rows) ? rows : new List<string[]>();
}

...

foreach (var row in rawArray)
{
    foreach (var srcRow in GetRows(row[0])
    {
       // perform your SRC logic here
    }

    foreach (var dstRow in GetRows(row[1])
    {
       // perform your DST logic here
    }
}

Another major speed improvement can probably be achieved by not using strings for entries which obviously aren't strings. You are doing a lot of double-to-string-and-back conversion which will be slow. If you can convert the input data into double when obtaining it, perform your calculations and then convert it back for output if necessary it should speed things up a lot as well.


How to refactor a verbose datatype? This morning I decided to write this question: https://stackoverflow.com/questions/11727571/are-there-problems-if-i-have-a-verbose-datatype/11727723#11727723

I don't really explain my scenario about how am I accesing through the nested dictionaries. I decided to have the default alias (IDictionary<string, IDictionary<Levels, IList<Problem>>>) but I'm thinking that's not a good option right now.

I'm gonna show some parts of my code:

ProblemBaseFactory

public abstract class ProblemBaseFactory : IProblemFactory
{
    private IDictionary<string, IDictionary<Levels, IList<Problem>>> problemDictionaryPackages;

    protected ProblemBaseFactory()
    {
        problemDictionaryPackages = new Dictionary<string, IDictionary<Levels, IList<Problem>>>();
    }

    protected IDictionary<string, IDictionary<Levels, IList<Problem>>> ProblemDictionaryPackages
    {
        get { return problemDictionaryPackages; }
    }

    public abstract IEnumerable<Levels> AvailableLevels(string packageName);
    public abstract IEnumerable<Problem> LoadProblems(ProblemFactoryProperties properties);
}

RuleProblemFactory

public abstract class RuleProblemFactory : ProblemBaseFactory
{
    private IDictionary<string, IDictionary<Levels, IList<ProblemRule>>> ruleDictionaryPackages;
    private IRuleAlgorithmSelector ruleAlgorithmSelector;
    private IList<Problem> temp;

    protected RuleProblemFactory()
        : this(new Dictionary<string, IDictionary<Levels, IList<ProblemRule>>>(), new SequentialRuleSelector())
    {
    }

    protected RuleProblemFactory(IRuleAlgorithmSelector ruleAlgorithmSelector)
        : this(new Dictionary<string, IDictionary<Levels, IList<ProblemRule>>>(), ruleAlgorithmSelector)
    {
    }

    protected RuleProblemFactory(IDictionary<string, IDictionary<Levels, IList<ProblemRule>>> ruleDictionary,
        IRuleAlgorithmSelector ruleAlgorithmSelector)
    {
        this.ruleDictionaryPackages = ruleDictionary;
        this.ruleAlgorithmSelector = ruleAlgorithmSelector;

        LoadBankOfRules();
    }

    protected IDictionary<string, IDictionary<Levels, IList<ProblemRule>>> RuleDictionaryPackages
    {
        get { return ruleDictionaryPackages; }
    }

    public override IEnumerable<Problem> LoadProblems(ProblemFactoryProperties properties)
    {
        // does exist the package name?
        if (!ProblemDictionaryPackages.ContainsKey(properties.NamePackage))
            throw new InvalidOperationException("namePackage");

        IDictionary<Levels, IList<Problem>> problemPackage = this.ProblemDictionaryPackages[properties.NamePackage];

        // does exist the list
        if (!problemPackage.ContainsKey(properties.Difficulty))
            problemPackage.Add(properties.Difficulty, new List<Problem>());

        temp = problemPackage[properties.Difficulty];

        if (temp.Count >= properties.Quantity)
            return temp;
        else
            return GenerateProblems(properties);
    }

    private IEnumerable<Problem> GenerateProblems(ProblemFactoryProperties properties)
    {

        ruleAlgorithmSelector.Rules = ruleDictionaryPackages[properties.NamePackage][properties.Difficulty];

        ...
    }

    public override IEnumerable<Levels> AvailableLevels(string name)
    {
        return ruleDictionaryPackages[name].Keys;
    }
}

Sometimes I guess is difficult to readability. What do you recomend me to to mantain a better readability?

IDictionary<string, IDictionary<Levels, IList<ProblemRule>>> is difficult to understand and maintain. I don't know enough about your problem domain to give you reasonably sounding names for classes, but I'll try.

In your LoadProblems method, I see you set the IDictionary<Levels, IList<ProblemRule>> to a variable named problemPackage, so I would create a class ProblemPackage, containing a list of ProblemRules per Level (I would drop the 's' in Levels) and move the related logic there.

In ProblemBaseFactory you end up with a IDictionary<string, ProblemPackage>. ProblemPackage will have a IDictionary<Level, ProblemRules> and the class ProblemRules will be an easy wrapper around a IList<ProblemRule>.

If you try to delegate the work to these classes, the readability will almost certainly go up.


Implementing a linked list as practice for a coding interview I am practicing LinkedList problems. How do I make it production level code as is mostly expected in a coding interview? Any other Suggestions?

package LinkedList;

/**
 * @author Amogh Antarkar
 * @email antarkar.amogh@gmail.com
 */
import java.util.*;

public class LinkedList{

    public static void main(String[]args){

        switchCase();
        System.out.println("Successful completion of the LinkedList program");
    }

    /* Note: Here a singleton pattern has been used for the Scanner in
     * as only one instance is created during the entire implementation
     */

    public static void switchCase(){
        int cont;
        Scanner in = new Scanner(System.in);
        LinkedListNode list = new LinkedListNode();

        do{
            System.out.println("Enter the option you want to run");
            System.out.println("1. remove duplicates\n2. Print List\n 3. Make List\n 4.kth to last element");
            int option = in.nextInt();
            switch(option){
            case 1: // remove duplicates
                System.out.println("option chosen:"+option);
                list.removeDuplicates();
                break;

            case 2:
                list.printList();
                break;

            case 3:
                list.makeList(list,in);
                break;

            case 4:
                System.out.println(" Enter k");
                int j = list.kthLastElement(list,in.nextInt());
                break;

            default:
                break;
            }
            System.out.println("Do you want to continue ?");
            cont = in.nextInt();

        }while(cont == 1);

        in.close();
    }
}

class LinkedListNode{

    private int data;
    private LinkedListNode next = null;

    public LinkedListNode(int d){
        data = d;
        next = null;
    }

    public LinkedListNode() {
    }

    // appends data to the List
    void appendData(int d){
        LinkedListNode end = new LinkedListNode(d);
        LinkedListNode n = this;
        while(n.next != null){
            n = n.next;
        }
        n.next = end;
    }

    // creates the LinkedList taking the input
    void makeList(LinkedListNode list, Scanner in){

        System.out.println("Enter the list");
        int len = in.nextInt();
        while(len-- > 0){
            list.appendData(in.nextInt());
        }
    }

    //implementing the recursive solution
    int kthLastElement(LinkedListNode head,int k){

        if(head == null){
            return 0;
        }
        else{
            int i = kthLastElement(head.next,k)+1;
            if(i==k)
                System.out.println(head.data);
            return i;
        }

    }

    // print the entire LinkedList O(n) time
    void printList(){
        LinkedListNode  n = this;
        while(n.next!= null){
            n = n.next;
            System.out.print(n.data+" -> ");
        }
        System.out.print("NULL\n");
    }

    // remove duplicates from the linkedList using Hashtable method
    void removeDuplicates(){

        Hashtable<Integer, Boolean> table = new Hashtable<Integer, Boolean>();
        LinkedListNode current = this, prev = null;

        while(current != null){
            if(table.containsKey(current.data)){
                prev.next = current.next;
            }else{
                table.put(current.data,true);
                prev = current;
            }
            current = current.next;
        }
    }
}

Names

package LinkedList

By Java's naming convention, package names are in lowercase and follows a 'reversed Internet domain name' prefix. Therefore, LinkedList is not a good package name, but something like com.antarkar.amough may be more suitable.

public class LinkedList

It is usually not recommended to use class names that conflict with those from the standard API, as it may cause confusion at best and compilation errors at worst. Since you are indeed doing a linked list implementation, I don't suppose there's a simpler way around this, just bear this in mind in the future for 'production level code'...

Separation of concerns

The biggest issue about this class, going by 'production level code' and 'coding interview' standards, is that you are mixing I/O operations (i.e. user input and display) with the data structure implementation. These two should stay separate for two simple reasons, if I may classify them under the standards you requested:

Production level code

When codes get into 'production' (depending on your definition), you'll want it to do one thing, do it well, and do it in as minimal as possible. If it's a data structure, it shouldn't know how to interact with System.in and System.out. If logging is required, it will use a (preferably widely-adopted and well-tested) logging framework to do so. Lesser lines of code tend to result in less programming bugs introduced. Lesser instructions (at the JVM level) also translate to more efficient memory usage, too.
Coding interview

Intertwining two unrelated features into a single class may indicate a lack of understanding of OOP concepts. What happens if a GUI-based demonstration is required? Also, how will you unit test your code when the class is interfacing directing with System.in and System.out, when it should be done through proper method arguments and (possibly) String outputs?

I'll recommending browsing through the number of linked list implementations in Java we have here on CR, to compare-and-contrast your implementations with the rest.

Modern code

Without even going into Java 8 features, features like try-with-resources should already be adopted around your Scanner so that you do not need an explicit call to close() at the end. Java 7 already has generic type inference aka the 'diamond operator', so you can remove the unnecessary types. Hashtable is a legacy class that is retrofitted to become part of the Collections framework, so you really ought to be using HashMap.

All these pointers matter if this is for a coding interview, as you'll want to show that you are reasonably kept up to date with the newer developments in your programming language of choice.