String helper functions in NASM Win16 Assembly Environment

NASM is required to build this program, and DosBox is required to run it. I'd recommend installing these using the Scoop Package Manager. Feel free to ignore install statements for any programs you have installed already.

iwr -useb get.scoop.sh | iex
scoop install git
scoop install dosbox
scoop install nasm

Building

nasm -f bin -o helper.com helper.asm

Running

Load DosBox, then mount the path where helper.com resides to any available drive. For those unfamiliar, it can be any drive in the A-Z range.

mount H: C:\Users\T145\Desktop\
H:
dir
helper.com

helper.asm

bits 16
org 0x100

section .text

_main:
    lea     di, [prompt]
    call    putstring

    lea     di, [string]
    call    getstring

    lea     di, [hello]
    call    putstring

    lea     di, [string]
    call    putstring

    mov     ah, 0x4c     ; standard exit code
    mov     al, 0
    int     0x21

; no parameters
; returns a char in ax
getchar:
    mov     ah, 0        ; call interrupt x16 sub interrupt 0
    int     0x16
    mov     ah, 0
    ret

; takes a char to print in dx
; no return value
putchar:
    mov     ax, dx       ; call interrupt x10 sub interrupt xE
    mov     ah, 0x0E
    mov     cx, 1
    int     0x10
    ret

; takes an address to write to in di
; writes to address until a newline is encountered
; returns nothing
getstring:
    call    getchar      ; read a character
    cmp     ax, 13       ; dos has two ascii characters for new lines 13 then 10
    je      .done        ; its not a 13, whew...
    cmp     ax, 10       ; check for 10 now
    je      .done        ; its not a 10, whew...
    mov     [di], al     ; write the character to the current byte
    inc     di           ; move to the next address
    mov     dx, ax       ; dos doesn't print as it reads like windows, let's fix that
    call    putchar
    jmp     getstring
.done:
    mov     dx, 13       ; write a newline for sanity
    call    putchar
    mov     dx, 10
    call    putchar
    ret

; takes an address to write to in di
; writes to address until a newline is encountered
; returns nothing
putstring:
    cmp     byte [di], 0 ; see if the current byte is a null terminator
    je      .done        ; nope keep printing
.continue:
    mov     dl, [di]     ; grab the next character of the string
    mov     dh, 0        ; print it
    call    putchar
    inc     di           ; move to the next character
    jmp     putstring
.done:
    ret

section .data

    prompt: db "Please enter your first name: ", 0
    string: times 20 db 0
    hello: db "Hello, ", 0

Output

There's plenty to optimize here!

In NASM you get the address simply by writing mov di, prompt. This has a shorter encoding than lea di, [prompt]. (In MASM this would be mov  di, offset prompt giving the same benefit over the lea form).

Instead of writing the pair mov ah, 0x4c mov al, 0, you could combine these in 1 instruction as mov ax, 0x4C00. This shaves off 1 byte from the program.

Your getchar returns a byte in AX and your putchar expects a byte in DX. You would be better off if you used AL and DL. This would avoid those several mov ah, 0 and mov dh, 0 instructions.

Your putchar code uses the BIOS.Teletype function 0x0E. This function does not expect anything in the CX register. What it does require is that you specify the displaypage in the BH register. Simply add mov bh, 0 here. And if it's even possible that your program has to run on the graphical video mode then it would make sense to write mov bx, 0x0007 because then the color for the character is taken from the BL register.

I see that the getstring code also checks for the linefeed code 10. No one does that. If the user presses the Enter key, you'll receive the carriage return code 13 and that's the only code that you need to check. The linefeed code only comes into play when outputting.

The pair of instructions mov [di], al inc di (3 bytes) can be replaced by the 1-byte instruction stosb. Given that your program is in the .COM file format we have DS=ES and the direction flag is almost certainly going to be clear. Ideal for using the string primitive assembly instructions. This also means that your putstring routine could use lodsb if you're willing to trade in DI for SI as the input parameter.

An interesting optimization comes from eliminating a tail call. You wrote call putchar directly followed by ret. This is equivalent to writing jmp putchar. Both shorter and faster this way!

Make it better

Your getstring procedure must not allow the user to input more than 19 characters. Anything more would overflow the 20-byte buffer.
Your getstring procedure should store (in the buffer) a terminating zero when the finishing Enter key arrives. This way the buffer can be used repeatedly and not just this one time.
In assembly we want to avoid all kinds of jumping because those are more time consuming than many other instructions.
Your putstring code uses a je and a jmp instruction on each iteration of the loop. The code below only uses the jne instruction on each iteration.

; IN (di)
putstring:
    jmp     .first
.continue:
    call    putchar
    inc     di           ; move to the next character
.first:
    mov     al, [di]     ; grab the next character of the string
    cmp     al, 0
    jne     .continue
    ret

; IN (al)
putchar:
    mov     ah, 0x0E     ; BIOS.Teletype
    mov     bx, 0x0007
    int     0x10
    ret

Using DX as the input for putchar is a poor choice, not only because DL would be enough, but especially because you need the character in AL anyway. So why not move it there in the first place?

Be consistent

Always write your numbers the same way. You wrote mov ah, 0x4c and also mov ah, 0x0E.
I suggest you use capitals for the hexadecimal digits and always write as many digits as will fit in the destination. So don't write stuff like mov ah, 0xE.
In case you're wondering why I make this suggestion. Using uppercase hexadecimal digits enhances the contrast with the lowercase 0x prefix or lowercase h suffix. Readability is very important in a program.

mov ah, 0x4C
mov ah, 0x0E

or

mov ah, 4Ch
mov ah, 0Eh

For many programmers function numbers are easiest recognized when expressed in hexadecimal.
You could thus write mov ah, 0x00 int 0x16 in your getchar routine.

As a final note, your labels are well chosen and the comments that you've added are all to the point. Congrats...


Please check if these two methods for inheritance and initialization are good or bad I am new to JavaScript.  I know that many inheritance and initialization methods have been written and use some advance features like Object.create:

Simple JavaScript Inheritance

Why Prototypal Inheritance Matters

For practice, I wrote my own methods for inheritance and initialization.  I have no idea if my code is good or if it could be improved.  Could someone help me and give me some advice?

function Class() {};

Class.prototype.create = function () {
    var constr = this.constructor;
    var instance = new constr();

    for (var i = 0; i < arguments.length; i++) {
        var props = arguments[i];
        for (var name in props){
            instance[name] = props[name];
        }
    }

    return instance;
}

Class.prototype.extend = function () {
    var constr = this.constructor;
    var proto = this.constructor.prototype;

    function ProtoClas() {};

    var prototype = ProtoClas.prototype = (function (proto) {
        function F() {};
        F.prototype = proto;
        return new F();
    })(proto);

    ProtoClas.prototype.constructor = ProtoClas;

    for (var i = 0; i < arguments.length; i++) {
        var props = arguments[i];
        for (var name in props){
            prototype[name] = props[name];
        }
    }

    return new ProtoClas();
}

Class.extend = function () {
    var args  = Array.prototype.slice.call(arguments);
    return this.prototype.extend.apply(new this(), args);
}

Class.create = function () {
    var args  = Array.prototype.slice.call(arguments);
    return this.this.prototype.create.apply(this, args);
}

I then generate a new Class and an instance through this Class:

var Person = Class.extend({
    say: function () {
        console.log("Hello");
    },
    walk: function () {
        console.log("walk");
    }
});

var person = Person.create({
    name:'lee'
})

console.log(Person);
console.log(person);

I'm the author of Why Prototypal Inheritance Matters. It's good that you're taking an interest in prototypal inheritance. So let's review your code. I'll start with your use case and then discuss the implementation.
The Use Case
var Person = Class.extend({
    say: function () {
        console.log("Hello");
    },
    walk: function () {
        console.log("walk");
    }
});

var person = Person.create({
    name:'lee'
});

console.log(Person);
console.log(person);

This code looks good. It's very similar to what I wrote in my aforementioned article. I would like to point out one thing though: you don't need two separate methods extend and create. Object creation and extension may be combined into a single function. Hence you could do:
var Person = Class.extend({
    say: function () {
        console.log("Hello");
    },
    walk: function () {
        console.log("walk");
    }
});

var person = Person.extend({
    name: "lee"
});

console.log(Person);
console.log(person);

The Implementation
Since you only require one method for prototypal inheritance I'll primarily focus on your extend function. First, your Class.extend method is needlessly complicated. I would rewrite it as:
function Class() {}

var extend = Class.prototype.extend = function () {
    //...
};

Class.extend = function () {
    return extend.apply(new Class, arguments);
};

As you can see you don't need to call Array.prototype.slice on arguments. It may be directly passed to apply. In addition try keeping things in variables as much as possible. Avoid expressions like this.prototype.extend. Also be explicit: write new Class instead of new this().

Your Class.prototype.extend function is not only confusing but also wrongly implemented. It doesn't extend the given object this. Instead it extends ProtoClas.prototype which is actually just Object.create. I would implement Class as follows:
function Factory() {}

var CLASS = {
    extend: function () {
        Factory.prototype = this;
        var instance = new Factory;
        var length = arguments.length;
        var index = 0;

        while (index < length) {
            var prototype = arguments[index++];

            for (var property in prototype)
                if (instance[property] !== prototype[property])
                    Object.defineProperty(instance, property,
                        Object.getOwnPropertyDescriptor(prototype, property));
        }

        return instance;
    }
};

You may now use CLASS.extend as follows:
var PERSON = CLASS.extend({
    say: function () {
        console.log("Hello");
    },
    walk: function () {
        console.log("walk");
    }
});

var person = PERSON.extend({
    name: "lee"
});

console.log(PERSON);
console.log(person);

Note that instead of doing var instance = Object.create(this) I used a function called Factory instead. The reason for doing this is performance: Object.create creates a new constructor every time. Here I'm simply reusing a single Factory constructor.
In addition for each argument supplied to extend I copy all the properties of the argument to the instance only if it doesn't already exist. This check speeds up the process because every object inherits from Object.prototype and copying is a very expensive operation.
Also instead of copying the properties using instance[property] = prototype[property] I'm using Object.defineProperty and Object.getOwnPropertyDescriptor. The allows you to copy properties as is allowing you to use getters/setters, etc.
The Alternative
On a concluding note I would like to mention that true prototypal inheritance is slow in JavaScript because JavaScript favors the constructor pattern of prototypal inheritance instead. Hence if you wish to write fast code then I suggest you read the following article which describes prototype-class isomorphism: https://stackoverflow.com/a/17893663/783743


Return the largest palindrome from the string Here is the question: find the largest palindrome from a string.

Ex:

ABCBAHELLOHOWRACECARAREYOUILOVEUEVOLIIAMAIDOINGGOOD

Result:

ILOVEUEVOLI

I am not sure of the efficiency of the algorithm.

static void Main(string[] args)
{
    var str = "ABCBAHELLOHOWRACECARAREYOUILOVEUEVOLIIAMAIDOINGGOOD";
    var longestPalindrome = GetLongestPalindrome(str);
    Console.WriteLine(longestPalindrome);
    Console.Read();
}

private static string GetLongestPalindrome(string input)
{
    int rightIndex = 0, leftIndex = 0;
    List<string> paliList = new List<string>();
    string currentPalindrome = string.Empty;
    string longestPalindrome = string.Empty;
    for (int currentIndex = 1; currentIndex < input.Length - 1; currentIndex++)
    {
        leftIndex = currentIndex - 1;
        rightIndex = currentIndex + 1;
        while (leftIndex >= 0 && rightIndex < input.Length)
        {
            if (input[leftIndex] != input[rightIndex])
            {
                break;
            }
            currentPalindrome = input.Substring(leftIndex, rightIndex - leftIndex + 1);
            paliList.Add(currentPalindrome);
            leftIndex--;
            rightIndex++;
        }
    }
    var x = (from c in paliList
             select c).OrderByDescending(w => w.Length).First();
    return x;
}

Your basic algorithm seems pretty efficient, but building a list then sorting it just to find the longest one isn't very efficient.  It would be much more efficient to just keep track of the longest palindrome:

    private static string GetLongestPalindrome(string input)
    {
        int rightIndex = 0, leftIndex = 0;
        var x = "";
        string currentPalindrome = string.Empty;
        string longestPalindrome = string.Empty;
        for(int currentIndex = 1; currentIndex < input.Length - 1; currentIndex++)
        {
            leftIndex = currentIndex - 1;
            rightIndex = currentIndex + 1;
            while(leftIndex >= 0 && rightIndex < input.Length)
            {
                if(input[leftIndex] != input[rightIndex])
                {
                    break;
                }
                currentPalindrome = input.Substring(leftIndex, rightIndex - leftIndex + 1);
                if(currentPalindrome.Length > x.Length)
                    x = currentPalindrome;
                leftIndex--;
                rightIndex++;
            }
        }
        return x;
    }

In my tests this is 3 times faster.

I hate to break this to you, but there is a bug in your code, which will probably mean rewriting your algorithm.  You algorithm assumes that the palindrome will be an odd number of characters.  However, a palindrome can be an even number of characters and your code won't find it if it is.

Here's some code that will find the longest palindrome regardless:

static string LargestPalindrome(string input)
{
    string output = "";
    int minimum = 2;
    for(int i = 0; i < input.Length - minimum; i++)
    {
        for(int j = i + minimum; j < input.Length - minimum; j++)
        {
            string forstr = input.Substring(i, j - i);
            string revstr = new string(forstr.Reverse().ToArray());
            if(forstr == revstr && forstr.Length > minimum)
            {
                output = forstr;
                minimum = forstr.Length;
            }
        }
    }
    return output;
}

EDIT

The above code has a bug.  Here it is reworked:

static string LargestPalindrome(string input)
{
    int longest = 0;
    int limit = input.Length;
    for (int i = 0; i < limit; i++)
    {
        for (int j = limit-1; j > i; j--)
        {
            string forStr = input.Substring(i, j - i);
            string revStr = new string(forStr.Reverse().ToArray());
            if (forStr == revStr && forStr.Length > longest)
            {
                return forStr;
            }
        }
    }
    return "";
}


Nth Index of Char in String I've written a function that searches for the nth occurrence of a character in the given string. A positive value for n will search from left-to-right, while a negative value will search from right-to-left. Zero is not a valid value for n.

Left-to-Right Search

"AABAXYAMN".NthIndexOf('A', 1) -> 0
"AABAXYAMN".NthIndexOf('A', 2) -> 1
"AABAXYAMN".NthIndexOf('A', 3) -> 3

Right-to-Left Search

"AABAXYAMN".NthIndexOf('A', -1) -> 6
"AABAXYAMN".NthIndexOf('A', -2) -> 3
"AABAXYAMN".NthIndexOf('A', -3) -> 1

Code:

/// <summary>
/// Searches for the nth occurrence of a character in the given string. A positive value for n will search from
/// left-to-right while a negative value will search from right-to-left. Zero is not a valid value for n.
/// </summary>
public static int NthIndexOf(this string input, char charToFind, int n) {
    int position;

    switch (Math.Sign(n)) {
        case 1:
            position = 0;
            while (((position = input.IndexOf(charToFind, position)) != -1) && ((--n) > 0)) { position++; }
            break;
        case -1:
            position = input.Length - 1;
            while (((position = input.LastIndexOf(charToFind, position)) != -1) && ((++n) < 0)) { position--; }
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return position;
}

First, your loops are evil to read.  It took me several minutes to figure out what the condition did.  Essentially, you loop N times, where N is the Nth instance of the character in the string you wish to find the index for.  Each time you loop, you find the index of the next occurrence of the character in the string, update the position to search from, and continue the process until the index is either -1 (no Nth instance), or until n > 0 or n < 0, based on which side you are searching from.

A simpler way to write this algorithm is as follows:

public static int NthIndexOfC(this string input, char charToFind, int n)
{
    int position;

    switch (Math.Sign(n))
    {
        case 1:
            position = -1;
            do
            {
                position = input.IndexOf(charToFind, position + 1);
                --n;
            } while (position != -1 && n > 0);
            break;
        case -1:
            position = input.Length;
            do
            {
                position = input.LastIndexOf(charToFind, position - 1);
                ++n;
            } while (position != -1 && n < 0);
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return position;
}

It takes a little bit more room since I wrote the loop body on its own lines, but it is easy to understand, and there are no assignments in the loop condition.

However, this is still not the way I would write this algorithm.  When you think about how IndexOf works, it iterates the string with a for loop from the specified starting index until it reaches the first instance of the requested character, which it returns.  We can write similar behavior, but just keep track of which index we are at:

public static int NthIndexOf(this string input, char charToFind, int n)
{
    int count = 0;
    switch (Math.Sign(n))
    {
        case 1:
            for (var index = 0; index < input.Length; index++)
            {
                if (input[index] == charToFind)
                {
                    count++;
                }

                if (count == n)
                {
                    return index;
                }
            }
            break;
        case -1:
            for (var index = input.Length - 1; index >= 0; index--)
            {
                if (input[index] == charToFind)
                {
                    count++;
                }

                if (count == Math.Abs(n))
                {
                    return index;
                }
            }
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return -1;
}

This is the longest solution yet, but we aren't finished.  Notice all that duplicated code?

public static int NthIndexOf(this string input, char charToFind, int n)
{
    switch (Math.Sign(n))
    {
        case 1:
            return NthIndexOf(input, charToFind, n, 0, input.Length);
        case -1:
            return NthIndexOf(input, charToFind, n, input.Length - 1, -1);
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }
}

public static int NthIndexOf(string input, char charToFind, int n, int searchFrom, int searchTo)
{
    int count = 0;
    var index = searchFrom;

    while (index != searchTo)
    {
        if (input[index] == charToFind)
        {
            count++;
        }

        if (count == Math.Abs(n))
        {
            return index;
        }

        index += Math.Sign(n);
    }

    return -1;
}

There.  Clean, easy to understand and maintain, and lots of options to use.  Notice that the second NthIndexOf needs to have the searchTo one integer larger or smaller than the value you want to stop at to accommodate the multi-directional search.


Ruby: working with arrays I am working on a script that collects all comments from my DB and inserts them into a hash. I then do a collect on the comments hash and split the comments into two separate comment hashes (vid_comments & wall_comments)

w_hash = Hash.new
    w_hash["comments"] = []
    contender.subscriber.videos.collect { |v| w_hash["comments"] << v.comments.where("created_at > DATE_SUB( NOW(), INTERVAL 1 DAY)") }
    w_hash["comments"] << contender.profile.comments.where("created_at > DATE_SUB( NOW(), INTERVAL 1 DAY)")
    w_hash["comments"].delete_if { |x| x.blank? }

    w_hash["vid_comments"] = w_hash["comments"].collect { |c| !c.media_id.nil? }
    w_hash["wall_comments"] = w_hash["comments"].collect { |w| w.parent_id == 0 }
    w_hash.delete("comments")

Is there anyway to shorten the code? I am pretty new to Ruby (PHP import) so excuse my ignorance in things I may be doing wrong.

well, first of all your first query is extremely ineffective

contender.subscriber.videos.collect { |v| w_hash["comments"] << v.comments.where("created_at > DATE_SUB( NOW(), INTERVAL 1 DAY)") }

this will query you db for each video you've got

second, you really shouldn't duplicate you SQL queries, i propose that you add scope to your Comment model (you are using Rails 3, right?)

scope :recent, where("created_at > ?", 1.day.ago)

to make your queries more effective you should use .joins method of ActiveRecord

after all this you code would be something like that

comments = Comment.joins(:videos).where(:videos => { :subscriber => contender.subscriber }).recent
comments << contender.profile.comments.recent

w_hash = {}
w_hash["vid_comments"] = comments.collect { |c| !c.media_id.nil? }
w_hash["wall_comments"] = comments.collect { |c| c.parent_id == 0 }


Flattening multiple nested node readline questions Say I'm creating a simple CLI. I want to use native node readline module to take in some input from user at prompt. I thought of this:

var prompt = chalk.bold.magenta;
var info = {};

rl.question(prompt('Thing One : '), function(args) {
    info.one = args;
    rl.question(prompt('Thing Two : '), function(args) {
        info.two = args;
        rl.question(prompt('Thing Three : '), function(args) {
            info.three = parseInt(args);
            rl.close();
            runSomeOtherModuleNow();
        })
    })
});

This does seem to work in a way I'd like, but this seems like a bad approach. I'd much prefer a flatter code than a pyramid like this.

Also, I was looking for an answer how to flatten (and automate) calls to rl.question(). In my solution I used Promises - chained - to display questions sequentially.

const readline = require('readline');

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});

const QUESTIONS = {
    action1: ['A1: Question 1', 'A1: Question 2', 'A1: Question 3'],
    action2: ['A2: Question 1', 'A2: Question 2', 'A2: Question 3']
}

let askQuestions = (actionKey) => {
    return new Promise( (res, rej) => {
        let questions = QUESTIONS[actionKey];

        if(typeof questions === 'undefined') rej(`Wrong action key: ${actionKey}`);

        let chainQ = Promise.resolve([]); // resolve to active 'then' chaining (empty array for answers)

        questions.forEach(question => {
          chainQ = chainQ.then( answers => new Promise( (resQ, rejQ) => {
                rl.question(`${question}: `, answer => { answers.push(answer); resQ(answers); });
            })
          );
        });

        chainQ.then((answers) => {
            rl.close();
            res(answers);
        })
    });
};

let handleError = (err) => {
    console.log(`ERROR: ${err}`);
}

let doSomethingwithAnswers = (answers) => {
    return new Promise( (res, rej) => {
        console.log('OUTPUT:');
        console.dir(answers);
    });
}

askQuestions('action1')
    .then(doSomethingwithAnswers)
    .catch(handleError);

Output:

A1: Question 1: a
A1: Question 2: b
A1: Question 3: c
OUTPUT:
[ 'a', 'b', 'c' ]

If you want the action to be chosen by the user, add these functions:

let showInterface = () => {
    return new Promise( (res, rej) => {
        console.log('Select action (enter action name):')
        console.log('-'.repeat(30));
        Object.keys(QUESTIONS).forEach(actionKey => {
            console.log(`${actionKey}`);
        });
        console.log('-'.repeat(30));
        res();
    });
};

let askQuestionForActionKey = () => {
    return new Promise( (res, rej) => {
        rl.question('Action key: ', actionKey => res(actionKey));
    });
}

And change main procedure to:

showInterface()
    .then(askQuestionForActionKey)
    .then(askQuestions)
    .then(doSomethingwithAnswers)
    .catch(handleError);

Now output shoud be like:

Select action (enter action name):
------------------------------
action1
action2
------------------------------

Action key: action1
A1: Question 1: a
A1: Question 2: b
A1: Question 3: c
OUTPUT:
[ 'a', 'b', 'c' ]

In case of error (e.g. enter non-existent action 'action3'):

Select action (enter action name):
------------------------------
action1
action2
------------------------------
Action key: action3
ERROR: Wrong action key: action3

It's very easy to apply this solution to your problem. Just define your questions as:

const QUESTIONS = {
    sequence: ['Thing One', 'Thing Two', 'Thing Three']
};

Your callback with answers:

let doSomethingwithAnswers = (answers) => {
    return new Promise( (res, rej) => {
        console.log('Make stuff with answers:');
        console.dir(answers);
    });
}

And the procedure may be unchanged - when a user has the ability to select a set of questions (actions):

showInterface()
    .then(askQuestionForActionKey)
    .then(askQuestions)
    .then(doSomethingwithAnswers)
    .catch(handleError);

Output:

Select action (enter action name):
------------------------------
sequence
------------------------------
Action key: sequence
Thing One: a
Thing Two: b
Thing Three: c
Make stuff with answers:
[ 'a', 'b', 'c' ]

Or if you want to apply to concrete questions set just use:

askQuestions('sequence')
    .then(doSomethingwithAnswers)
    .catch(handleError);

Output:

Thing One: a
Thing Two: b
Thing Three: c
Make stuff with answers:
[ 'a', 'b', 'c' ]

I hope it will be useful :)
Enjoy!


Simple authentication in ASP.NET MVC 5 I am building an ASP.NET MVC 5 application and, for reasons which are irrelevant at this point, I am attempting to build my own means of authenticating users. I'm still very new to programming, especially to this sort of thing, and I realize that what looks fine to me may be full of problems. Thus, I present my code, simplified for demonstration purposes, for review.

The Contact model:

public class Contact
{
    [Key]
    public int ContactID { get; set; }

    [Required]
    public string Email { get; set; }

    [Required]
    public string Password { get; set; }
}

The Login action:

[HttpPost]
public ActionResult Login(Contact contact)
{
    bool validEmail = db.Contacts.Any(x => x.Email == contact.Email);

    if (!validEmail)
    {
        return RedirectToAction("Login");
    }

    string password = db.Contacts.Where(x => x.Email == loginForm.Email)
                                 .Select(x => x.Password)
                                 .Single();

    bool passwordMatches = Crypto.VerifyHashedPassword(password, loginForm.Password);

    if (!passwordMatches)
    {
        return RedirectToAction("Login");
    }

    string authId = Guid.NewGuid().ToString();

    Session["AuthID"] = authId;

    var cookie = new HttpCookie("AuthID");
    cookie.Value = authId;
    Response.Cookies.Add(cookie);

    return RedirectToAction("Private");
}

Actions that require the user to be logged in will look like this:

public ActionResult Private()
{
    try
    {
        if (Request.Cookies["AuthID"].Value == Session["AuthID"].ToString())
        {
            return View();
        }
        else
        {
            return RedirectToAction("Index");
        }
    }
    catch
    {
        return RedirectToAction("Index");
    }
}

I feel like this is probably really bad. However, every "solution" I find seems overly complex and/or lacking any documentation that would allow me to make sense of it.

All I want is to make sure the user has a valid username and password before they place an order or update their account. Will my code do that securely?

I fear that the credentials could be sniff by some attacker if they are listening to during the connection process. If https is not enabled, the password (if there is no protection) could be seen by any attacker during the HTTP requests.

This point is not directly related as to how you implemented your code, but it will go a long way as to how your code is safe.

string authId = Guid.NewGuid().ToString();

I'm not a security expert, but using GUID for security purpose is something that I would recommend. It seems to be frown upon :

  GUIDs were not designed to be a part of any security system; use of them in a security system is an "off label" use and is therefore a very bad idea. I can't see much at the moment that would break the mechanism.

You should take this advice as-is, and do some more research on it. I would not take a comment on a question as a good reference, but it would make me wonder if it's in fact a good idea.

There is a lot of documentation to help you develop a secure authentication system,but this great question on SO would cover it for most of the important things.

The authentication mechanism looks fine to me. Storing a cookie  and check the value in  the Session is a good way to assure that user is authenticated.

The only drawback is if someone hijack the session and the cookie, he don't need to login with credentials, since the token will assure him that the system see him as an authenticated user. There is not much in your code that you could do to prevent that.

What you will miss most by rolling your own mechanism is some "perk" from using the .Net implementation. From what I've previously done at school and search, you'll miss the [Authorize][3] properties where your code could look :

[Authorize]
public ActionResult Private()
{
    return View();

}

Which would hide all the cookie related code. What that could help you is, if for a specific reason you can't use cookies you won't need to change all the place where you were verifying the identity of your user. This is just for code readability and maintenance, this is not related to security.


My first React custom hook I need to change the page title based on the current state of my store. Right now I came up with
const useTitle = () => {
  const { foo, bar } = useSelector(selectFoorBar);
  const baz = useSelector(selectBaz);

  const fooBarDisplayString = format(foo, bar);
  const barDisplayString = (bar ?? 0).toFixed(0);

  useEffect(() => {
    document.title = `${fooBarString} | ${bazDisplayString}`;
  }, [fooBarDisplayString, bazDisplayString]);
};

export default useTitle;

Is the name useTitle appropriate? It's technically a hook, because it's using other hooks, so it must start with use keyword. That feels weird to somebody coming outside of the React world like me. Furthermore, it doesn't return any title, so it's more of a keepTitleUpToDate(), which should become useKeepTitleUpToDate() which is even weirder. How would you name it?
Is it appropriate to keep this functionality in a hook like I did or is there some other convention in React for stuff that depends on hooks?

I don't see any issue with the code.

It's technically a hook, because it's using other hooks, so it must
start with use keyword. That feels weird to somebody coming outside of
the React world like me. Furthermore, it doesn't return any title...

The use prefix is just a React hook naming convention, and React hooks aren't required to return anything.

How would you name it?

If you are merely asking about what to name it, well, this is rather opinionated, but I suggest:
useDocumentTitleUpdater

Alone the functionality is to update the document title, something like updateDocumentTitle or documentTitleUpdater. The former, to me, implies you are handing the function the value you want it to update something to, while the latter is more of a delegation, telling the function to update the title. Since this is a React hook and there's a hook naming convention, useDocumentTitleUpdater explains the purpose of the hook. It could probably be shortened to useDocTitleUpdater if the previous was too verbose for your taste.

Is it appropriate to keep this functionality in a hook like I did or
is there some other convention in React for stuff that depends on
hooks?

This doesn't appear to be an inappropriate use of a React hook. The alternative would be a utility function that consumes the various arguments from redux, runs the two middle lines to compute the strings, and updates the title. This function would be called from a useEffect hook with the redux state values as dependencies instead.
Your hook has instead encapsulated this into a single line any functional component can use and they won't be responsible for getting the dependencies correct.


connection to mongodb in golang I am building an API server in golang with mongodb. My main.go file looks something like this:

func main() {
    r := mux.NewRouter()

    question := r.PathPrefix("/question").Subrouter()
    question.HandleFunc("", handler.PostQuestion).Methods("POST")
    question.HandleFunc("/{id}", handler.UpdateQuestion).Methods("PUT")
    question.HandleFunc("/{id}", handler.DeleteQuestion).Methods("DELETE")
    question.HandleFunc("/{id}", handler.GetQuestion).Methods("GET")
    r.HandleFunc("/questions", handler.GetAllQuestions).Methods("GET")

    if err := http.ListenAndServe(":8080", r); err != nil {
        log.Fatalf("could not listen on port 8080 %v", err)
    }
}

Every handler has some logic which changes data in MongoDB. Regardless of what I am doing with the question collection, this is common in all the handlers.

client, _ := mongo.NewClient(options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
err := client.Connect(ctx)
if err != nil {
    panic(err)
}

defer client.Disconnect(ctx)
collection := client.Database("myapp").Collection("questions")

These functions are in handler/handler.go.

As you can see, I am using the official Mongo driver. What is the best kind of refactor which can be done?

You say this is common in all handlers:

client, _ := mongo.NewClient(options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
err := client.Connect(ctx)
if err != nil {
    panic(err)
}

defer client.Disconnect(ctx)
collection := client.Database("myapp").Collection("questions")

So this means you're connecting and disconnecting to MongoDB for each request. That's incredibly wasteful. The MongoDB client is safe for concurrent use, so you can reuse the connection. Because I see code like this:

question.HandleFunc("", handler.PostQuestion).Methods("POST")

I know your handler functions are methods, so why not add the client to your handler type:

type yourHandler struct {
    coll   *mongo.Collection
}

Then, in your handlers you can simply access:

func (h *yourHandler) PostQuestion(w http.ResponseWriter, r *http.Request) {
    if err := h.coll.InsertOne(r.Context(), ...); err != nil {
        // write error response
        return
    }
}

Now your main function will look a bit different, seeing as we're not disconnecting the mongo client in the handler anymore, and we're not using a context with an arbitrary timeout. I suggest something like this:

func main() {
    // create the base context, the context in which your application runs
    ctx, cfunc := context.WithCancel(ctx.Background())
    defer cfunc()
    client, err := mongo.NewClient(
        options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
    if err != nil {
        log.Fatalf("Error creating mongo client: %+v", err)
    }
    defer client.Disconnect(ctx) // add disconnect call here
    if err := client.Connect(ctx); err != nil {
        log.Fatalf("Failed to connect to MongoDB: %+v", err)
    }
    // set collection on handler
    handler.coll = client.Database("myapp").Collection("questions")
    // rest of the code here
}

So now, whenever the main function returns, the application context is cancelled (which is used to connect the client), and client.Disconnect is called. The handler has access to the collection you're using, removing a lot of duplicate code, and removing the overhead of constantly connecting and disconnecting to MongoDB.

Inside your handler, I'm not using context.WithTimeout(context.Background(), 10*time.Second). Instead I'm using the request context, which means once the request context is cancelled, the query context is. You could still set a time-limit if you want:

func (h *yourHandler) PostQuestion(w http.ResponseWriter, r *http.Request) {
    // context expires if request context does, or times out in 3 seconds
    ctx, cfunc := context.WithTimeout(r.Context(), time.Second * 3)
    defer cfunc() // good form to add this
    if err := h.coll.InsertOne(ctx, ...); err != nil {
        // error response etc...
        return
    }
    // success response
}

Currently our handler requires the field coll to be of the type *mongo.Collection, which makes it harder to test your code. Instead, you might want to change that field to take an interface:

//go:generate go run github.com/golang/mock/mockgen -destination mocks/collection_interface_mock.go -package mocks your.module.io/path/to/handler/package Collection
type Collection interface{
    InsertOne(ctx context.Context, document interface{}, opts ...*options.InsertOneOptions) (*mongo.InsertOneResult, error)
    Name() string
    // all methods you need
}

In your unit tests, you can now inject a mock collection interface, that you can control, allowing you to simulate error returns, etc...

Instead of exposing the coll field on your handler, then, you'll also want to create a constructor function to inject the dependencies of what I have in the main function above (handler.coll = ...):

package service

type Collection interface{} // interface as above, with go generate comment

type handler struct {
    coll Collection
}

func NewHandler(c Collection) *handler {
    return &handler{
        coll: c,
    }
}

I'll leave you with this as a starting point. Just one thing I've not yet mentioned: you're using log.Fatalf when http.ListenAndServe returns an error. That's fine. Why are you using panic(err) when client.Connect(ctx) fails?

Panic is something you should avoid as much as possible. Especially here: it adds very little (to no) value: the only information a panic dump will yield is whereabouts in the mongo package something went awry. You don't control that package, so just log.Fatalf("Mongo error: %+v", err) is to be preferred.

More details

Seeing as your handler functions are actually functions in a package, rather than methods (which I initially thought they were):

I'd refactor the handler package to contain a constructor, and have the actual handler functions as methods on a type. Something like:

package handler

type Collection interface{
    InsertOne(ctx context.Context, document interface{}, opts ...*options.InsertOneOptions) (*mongo.InsertOneResult, error)
    Name() string
    // and so on
}

// main handler type
type Handler struct {
    coll Collection
}

// New returns the handler
func New(c Collection) *Handler {
    return &Handler{
        coll: c,
    }
}

func (h *Handler) PostQuestion(w http.ResponseWriter, r *http.Request) {
    if err := h.coll.InsertOne(r.Context(), ...); err != nil {
        log.Errorf("Insert question error: %+v", err)
        // write response
        return
    }
    // success response
}

From your main package, this will look like this:

func main() {
    ctx, cfunc := context.WithCancel(ctx.Background())
    defer cfunc()
    client, err := mongo.NewClient(
        options.Client().ApplyURI("mongodb://127.0.0.1:27017"))
    if err != nil {
        log.Fatalf("Error creating mongo client: %+v", err)
    }
    defer client.Disconnect(ctx) // add disconnect call here
    if err := client.Connect(ctx); err != nil {
        log.Fatalf("Failed to connect to MongoDB: %+v", err)
    }
    // create handler with collection injected
    myHandler := handler.New(client.Database("myapp").Collection("questions"))

    // configure router to call methods on myHandler as handler functions
    // the handlers will now have access to the collection we've set up here
    r := mux.NewRouter()

    question := r.PathPrefix("/question").Subrouter()
    question.HandleFunc("", myHandler.PostQuestion).Methods("POST")
    // and so on
}


Piccoche - a coordination game Click the divs to remove them. Remove all div's in the shortest time and minimal clicks (to do). Code needs streamlining.

<!DOCTYPE html>

I have a To Do list:

To Do

Add timer,
Add 'missed click counter',
Colours,
Size,
Positions,
Number,
Streamline code into DOM format,
Score,
Play again

Options

Flashers,
Multi-size,
Movers,
Bonuses (double points, static, combos, chains),
Anti-bonuses (move, faster, half points, bombs)

This is cleaner.
added <title> for no HTML Errors W3C Markup Validator.
changed CSS units px to em, for 100% W3C Mobile OK
changed width to 20em (320px) for Mobile Viewport
added mobile meta viewport for Google PageSpeed Insights 100% (less when target divs overlap)
Moved JS below HTML
Made count global
added
 to get misses
added
 to replace alert
replaced CSS div { with .target{
changed div ids to d+index
created target array and init code instead of calling getElementById
added [CDATA[ to JS
added miss counter
added elapsed time
added "Again" button and function
removed function rInt(), was not worth the stack push and pop of calling another function.

While some might say the update() should be added by EventListener. When only one event is required onclick is easy, reliable, and more universally compatible.

W3C HTML Markup Validation Service HTML5: NO ERRORS
W3C HTML Markup Validation Service XHTML Basic 1.1: NO ERRORS
W3C CSS Validation Service: NO ERRORS
W3C mobileOK Checker: Perfect Score: 100%

Google's "Usability Assesment" PageSpeed Insights

Mobile User Experience: Perfect Score: 100%
Desktop User Experience: Perfect Score: 100%
Mobile Speed: Perfect Score: Perfect Score: 100%
Desktop Speed: Perfect Score: 100%

Yahoo's YSlow Grade Perfect Score: 100% (gtmetrix.com)
Page Speed Grade: 99% (gtmetrix.com) 1% loss due to bug in PageSpeed scoring
Page Load time 0.250 seconds (gtmetrix.com)

HTML CSS

<!DOCTYPE html>
<title>Piccoche</title><meta name="viewport" content="width=device-width, initial-scale=1.0" />

All Gone!

<button onclick="playAgain()">Again</button>

JS

New Play Again function

function playAgain(){
  for (var div=0; div<10; div++){
    target[div].style.top = Math.floor(Math.random()*290) + 'px';
    target[div].style.left= Math.floor(Math.random()*290) + 'px';
    target[div].style.display = 'block';
  }
  count = 10;
  misses = -1;
  elapsed = new Date();
  document.getElementById('msg').style.display='none';
}


Implementing Sieve of Eratosthenes faster I wrote a version of sieve of eratosthenes:

number =100000000
def eratos_sieve(n:"find prime below this"=None):
        if not(n):
            n = number
        numbers = [x for x in range(0, n+1)]
        current_prime = 2
        sqrt_limit = int(math.sqrt(n))+1
        while numbers[current_prime] <= sqrt_limit:
            #(len(numbers)-1) is the actual size since
            #since, len([0,...,n])=n+1
            for i in range(current_prime, ((len(numbers)-1)//current_prime)+1):
                numbers[current_prime*i]=-1
            current_prime += 1
            for i in range(current_prime, sqrt_limit):
                if numbers[i]>0:
                    current_prime=i
                    break

        #this part is optional
        j = 0
        for i in range(len(numbers)):
            if numbers[i]>0:
                numbers[j]=numbers[i]
                j += 1
        #uptil here if later primes are to be found using indices
        #else all primes upto n are returned as a single list
        return numbers[1:j]

It seems to perform fast enough for my purposes. I'd like to get some review about it's performance since I'm a novice. Also, I'd like someone with enough memory (if possible) to run the below two tests:

import time
import math
num1 = 100000000 #100 million
num2 = 1000000000 #1 billion
start = time.perf_counter()
eratos_sieve(num1)
total_100_m = time.perf_counter()-start()

start = time.perf_counter()
eratos_sieve(num2)
total_1_b = time.perf_counter()-start()

print("For 100m it takes: ", total_100_m)
print("For 1b   it takes: ", total_1_b)

Four* letters to rule them all

The Sieve of Eratosthenes is an algorithm which heavily relies on loops. Unfortunately, Python's convenient scripty nature comes at a cost: it's not terribly fast when it comes to loops, so it's best to avoid them.

However, this is not always possible, or one, like me in this case, is not so much into algorithms to transform them into another form. Enter numba. numba is a just-in-time compiler for Python code. The just-in-time compiler can transform plain Python code into "native" code for your system. So what are those magical four letters I was talking about? It's @jit.

This simple decorator and no further code changes brought the time for n = 100 000 000 from 36s down to about 3s on my machine. The larger test case did not work out to well with my 16GB RAM and some other things to do apart from Code Review.

from numba import jit

@jit
def eratos_sieve(n:"find prime below this"=100000000):
    # ... the rest of your code here

You can also have a look at this generator based implementation of the algorithm presented on Stack Overflow. The implementation can benefit from just-in-time compilation as well. A quick test using the SO approach and @jit for the previous n brought the execution time down to just below 2s on the same machine as before.

What numba cannot fix for you

There are a few aspects with respect to style and idiomatic language features (often called pythonic code) that you have to/can improve yourself.

While

number =10000000
def eratos_sieve(n:"find prime below this"=None):
    if not(n):
        n = number
    ...

might be working and syntactically valid, I find it very, very uncommon and would not recommend it. A more "pythonic" way to express this would be

def eratos_sieve(n: int = 100000000):
    """Find all primes below a given whole number n"""
    ...

This makes use of Python 3's type hinting and the recommended way to write function documentation using docstrings.

The way you construct your candidate list using numbers = [x for x in range(0, n+1)] is also a little bit complicated, and may be simplified to numbers = list(range(n+1)). 0 is the implicit starting value for range(...) and since range(...) already returns a generator you can simply pass it to a list constructor and work with it.

You should also have a look at the official Style Guide for Python Code (PEP8) and follow it's recommendations regarding whitespace within expressions and statements.

If you intend to supply your testing code together with your function in a single script, wrap it in the well known if __name__ == "__main__": construct to ensure it is only run if your code is used as a script.

* Well not strictly speaking, but it's close.


SQL with SUBSTRING / CHARINDEX I've got a VARCHAR variable of this kind:

@string = "BLABLA10;GOGOGO30;RES777;ANOTHER;"

(several keyword + number separated by ;)

I need to get the INT value after "RES" in a T-SQL query (using SYBASE).

I thought it would be always 3 characters, so I did:

SELECT @val = CONVERT(INT, LTRIM(RTRIM(SUBSTRING(@string, CHARINDEX(";RES", @string)+4, 3))))

But now, it can be 2 characters (or more) for example... so I need to get the value between RES and the next ;

I've done a complicated query:

CONVERT(INT, LTRIM(RTRIM(SUBSTRING(@string, CHARINDEX(";RES", @string)+4, -4 + CHARINDEX(";", SUBSTRING(@string, CHARINDEX(";RES", @string)+1, 70) )))))

It works but it's horrible to read.

Has anyone an idea for a more readable way than this?

I know that the real problem is that we should not use value in concatenated string, but well it was done that way and I can't change it.

You've hit the nail on the head there:

I know that the real problem is that we should not use value in concatenated string, but well it was done that way and I can't change it.

So you are in a situation where you have to over-compensate on the database end to counter a bad design decision. Many of us who write SQL regularly in production environments have been there. So let's see what we can do about it.

I have added new lines and indentations to give a better scope of what your string manipulation query is doing:
DECLARE @string VARCHAR(MAX) = 'BLABLA10;GOGOGO30;RES777;ANOTHER;';
SELECT
    CONVERT(
        INT,
        LTRIM(RTRIM(
            SUBSTRING(
                @string,
                CHARINDEX(
                    ';RES',
                    @string
                )+4,
                -4 + CHARINDEX(
                    ';',
                    SUBSTRING(
                        @string,
                        CHARINDEX(
                            ';RES',
                            @string
                        )
                        +1,
                        70
                    )
                )
            )
        ))
    )
;

Note: I have converted "s to 's so I could run it on SQL Server.
This is quite complicated. You've obviously spent a good amount of time putting this together so it would work. I'd say, to avoid WTFs from Mr. or Ms. Maintainer, you should document it.

Inspired by RegEx verbose
There's a regular expression mode in some programming languages that lets you document your regular expression. Since your SQL does similar things than RegEx does, I figured maybe we could find inspiration there. For example sake, this is a documented verbose RegEx from the Python documentation:

a = re.compile(r"""\d +  # the integral part
                   \.    # the decimal point
                   \d *  # some fractional digits""", re.X)

Let's try and do something similar here, starting with the innermost manipulation:
SUBSTRING(  -- get keyword RES and the chars that follow (up to 70 chars max)
    @string,
    CHARINDEX(      -- find index of
        ';RES',     -- semicolon just before keyword RES
        @string     -- in @string
    )
    +1,         -- +1 to advance past the semicolon
    70          -- length of up to 70

So it starts making a bit of sense for the next person. Let's keep working our way up the chain from there:
DECLARE @string VARCHAR(MAX) = 'BLABLA10;GOGOGO30;RES777;ANOTHER;';
SELECT
    /*
    The purpose of this manipulation is to find
    get the value between RES and the next ;
    in a string of keywords and numbers. (referred to as @string)
    NOTE: Read function documentation from inner to outer to understand how this works.
    */
    CONVERT(
        INT,    -- returned value needs to be an integer
        LTRIM(RTRIM(    -- trim potential white space before or after the value
            SUBSTRING(      -- String of characters between end of RES and following semicolon
                @string,        -- in @string
                CHARINDEX(      -- find index of
                    ';RES',     -- keyword RES and semicolon just before it
                    @string     -- in @string
                )+4,        -- add 4 chars to move index just past RES keyword
                -4 + CHARINDEX( -- find index of
                    ';',        -- semicolon after the value belonging with RES
                    SUBSTRING(      -- String of keyword RES and the chars that follow (up to 70 chars max)
                        @string,
                        CHARINDEX(      -- find index of
                            ';RES',     -- semicolon just before keyword RES
                            @string     -- in @string
                        )
                        +1,         -- +1 to advance past the semicolon
                        70          -- length of up to 70 chars
                    )
                )
            )
        ))
    )
;

Looking at it now, there are already a few things that stand out:

Length 70?
Do you really need to capture RES and up to 70 characters? You are converting the string to a signed 4-byte int which will at most be 2,147,483,647 (10 digits). So, just to be safe, let's factor in the keyword RES, and the following ;, and one extra for good luck. So let's change 70 to 15.
SUBSTRING(@string, CHARINDEX(";RES", @string)+1, 15) )

If you needed to change the outermost CONVERT to say, a BIGINT, then you would need a longer string, but that seems not to be the case based on your code.

LTRIM(RTRIM(
I know that SQL operators often include this "just in case", so I don'tblame you for doing so. But since you're seeking a review, I will point out that perhaps you should think of what are the chances of this actually happening?
To illustrate, a use case of this happening would look perhaps like this:

@string = "BLABLA10;GOGOGO30;RES   777  ;ANOTHER;"

If that is not possible, then you may as well just remove the LTRIM(RTRIM( part altogether. In fact, it looks like converting to int already does that for you, so just remove it.
Demo:

SELECT CONVERT(INT, '   777  ')
-- Result: 777

Note: If you do expect that empty spaces will be present, you may need to adjust the 15 value above (instead of 70) to a higher number to account for this.

Speaking of RegEx...
Did you consider using Regular Expressions instead of this complicated manipulation? Here are a few links for you to consider:

Create a UDF to support Regular Expressions in T-SQL

StackOverflow: Regex Match all characters between two strings

If you supported RegEx it may be as simple as just search for (?<=RES)(.*)(?=;) instead of doing this whole mammoth maneuver.


Calculate sum of all the items in array list? I am setting the sum total of all the items in an array to a text view in android.The values contained in the array are all currency values.

@Override
        public void afterTextChanged(Editable s) {

            int id = 0;

            id = (int) itemtotal.getTag();
            String itemTot = itemtotal.getText().toString();

            if (id >= 0 && id <= totals.size()) {
                totals.set(id - 1, itemTot);

                double[] doubleList = new double[totals.size()];
                double newsum = 0.0d;
                for (int i = 0; i < totals.size(); ++i) {
                    doubleList[i] = Double.parseDouble(totals.get(i));
                    newsum += (double) Math.round((doubleList[i]) * 100.0) / 100.0;
                }
                textViewSum.setText(((double) Math.round((newsum * (0.02) + newsum) * 100.0) / 100.0) + "");//set total text to sum
                textViewVat.setText(((double) Math.round((newsum * (0.02)) * 100.0) / 100.0) + "");
            }
        }

So I am basically calculating the sum total and the vat which is 2%.

The code is pretty straightforward, but I am interested in any possible improvement.

Naming

s is a bad parameter name. If there's nothing better, use editable.

Avoid abbreviations. itemTot might be better as itemTotalValue.

Use camelCase. newsum should be newSum.

It's confusing that textViewSum doesn't actually hold the sum.

Implementation

Don't declare id as 0 and then reassign it right away. Just declare it as what it should be, and make it final because it won't be reassigned.

Guard clauses are very helpful for keeping code from looking like a big > sign. Not an issue now, but it might be if more code gets added to this method.

Don't declare itemtot until it's actually needed. You might leave the method before using the value.

You don't need an entire double[] to hold your computations. You only need one double variable declared inside the for loop.

It might be nice if there were parens to clarify the cast is before the division. Also, you don't need to cast to double. Dividing by 100.0 makes the result a double even though the dividend is a long.

It's ugly to use string concatenation to turn a double into a String. Java provides a method (Double.toString()) for simple conversions, and a class (NumberFormat) for complex conversions. Or you can use String.format(), which is a bit newfangled for me, but I hear the kids love it.

Result

If you were to apply all these suggestions your code might look something like:

@Override
public void afterTextChanged(final Editable editable) {

    final int id = (int) itemtotal.getTag();
    if ((id < 0) || (id > totals.size())) {
        return;
    }

    final String itemTotalValue = itemtotal.getText().toString();
    totals.set(id - 1, itemTotalValue);

    double newSum = 0.0d;
    for (int i = 0; i < totals.size(); ++i) {
        final double value = Double.parseDouble(totals.get(i));
        newSum += Math.round(value * 100) / 100.0;
    }

    final double sum = Math.round((newSum * 0.02 + newSum) * 100.0) / 100.0;
    textViewSum.setText(Double.toString(sum));

    final double vat = Math.round((newSum * 0.02) * 100.0) / 100.0;
    textViewVat.setText(Double.toString(vat));
}


Entity Framework invoice queries I have these queries written in Entity Framework where I try to get:

Name of the customer
Most Recent Invoice Ref
Most Recent Invoice Amount (£)
Number of outstanding invoices (#)
Total of all outstanding invoices (£)

I use these two tables:

CREATE TABLE Customers
(
    CustomerId INT,
    Name NVARCHAR(100),
    Address1 NVARCHAR(100),
    Address2 NVARCHAR(100),
    Postcode NVARCHAR(100),
    Telephone NVARCHAR(15),
    CONSTRAINT Customers_PK PRIMARY KEY (CustomerId)
)

CREATE TABLE Invoices
(
    InvoiceId INT,
    CustomerId INT, -- FK
    Ref NVARCHAR(10),
    InvoiceDate DATETIME,
    IsPaid BIT,

    Value DECIMAL,
    CONSTRAINT Invoices_PK PRIMARY KEY (InvoiceId)
)

And the queries I make are:

public PaginatedList<CustomerListEntity> GetCustomers(int pageIndex, int pageSize, out int totalRecords)
        {
            var customers = _customerRepository.AllIncluding(x => x.Invoices).OrderBy(x => x.CustomerId).Select(x => new
              CustomerListEntity {
                Name = x.Name,
                CustomerID=x.CustomerId,
                RecentInvoiceRef = x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault() != null ? x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault().Ref : string.Empty,
                RecentInvoiceAmount = x.Invoices.OrderByDescending(t=>t.InvoiceDate).FirstOrDefault()!=null? x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault().Value:null,
                UnpaidInvoicesNumber=x.Invoices.Where(t=>t.IsPaid==false).Count(),
                UnpaidInvoicesTotalAmount = x.Invoices.Where(t => t.IsPaid == false).Sum(k=>k.Value)
            });
            totalRecords = customers.Count();
            return customers.ToPaginatedList(pageIndex, pageSize);
        }

Here is the implementation of AllINcluding:

public virtual IQueryable<T> AllIncluding(params Expression<Func<T, object>>[] includeProperties)
    {
        IQueryable<T> query = _entitiesContext.Set<T>();

        foreach (var includeProperty in includeProperties)
        {
            query = query.Include(includeProperty);
        }
        return query;
    }

First off, the Include is ignored because you project the result to a new result (Select(x => new{ ... }). So you may as well remove it.

Then, there is some room for improvement by eliminating the repetitive parts. It's much easier to do this in query syntax:

var customers = from cst in _customerRepository.All() // Assuming this method exists
    let lastInvoice = cst.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault()
    let upaidInvoices = cst.Invoices.Where(t => !t.IsPaid)
    select new
      CustomerListEntity {
        Name = cst.Name,
        CustomerID = cst.CustomerId,
        RecentInvoiceRef = lastInvoice.Ref ?? string.Empty,
        RecentInvoiceAmount = lastInvoice.Value,
        UnpaidInvoicesNumber = upaidInvoices.Count(),
        UnpaidInvoicesTotalAmount = upaidInvoices.Sum(k => k.Value)
    });

The let keyword defines a local variable that can be reused in the LINQ statement. As you see, this greatly improves the readability of the code.

It also slightly improves the generated SQL query, because Ref and Value will now be retrieved in one subquery instead of two. Unfortunately, this doesn't apply to Count and Sum, because aggregates require separate subqueries.

Also, note that the null checks are removed. The LINQ statement is translated into SQL and executed in the database. SQL doesn't have this null reference concept.

One last point is that there is broad consensus on the additional repository layer being totally redundant. Consider removing and query directly on the context.


Creating a thread-safe list using a Lock object Will this code qualify for a truly thread-safe list?  It is using the Lock object.

using System;
using System.Collections.Generic;
using System.Threading;

public class ThreadSafeListWithLock<T> : IList<T>
{
    private List<T> internalList;

    private readonly object lockList = new object();

    private ThreadSafeListWithLock()
    {
        internalList = new List<T>();
    }

    // Other Elements of IList implementation

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

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

    public List<T> Clone()
    {
        ThreadLocal<List<T>> threadClonedList = new ThreadLocal<List<T>>();

        lock (lockList)
        {
           internalList.ForEach(element => { threadClonedList.Value.Add(element); });
        }

        return (threadClonedList.Value);
    }

    public void Add(T item)
    {
        lock (lockList)
        {
           internalList.Add(item);
        }
    }

    public bool Remove(T item)
    {
        bool isRemoved;

        lock (lockList)
        {
            isRemoved = internalList.Remove(item);
        }

        return (isRemoved);
    }

    public void Clear()
    {
        lock (lockList)
        {
            internalList.Clear();
        }
    }

    public bool Contains(T item)
    {
        bool containsItem;

        lock (lockList)
        {
            containsItem = internalList.Contains(item);
        }

        return (containsItem);
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        lock (lockList)
        {
            internalList.CopyTo(array,arrayIndex);
        }
    }

    public int Count
    {
        get
        {
            int count;

            lock ((lockList))
            {
                count = internalList.Count;
            }

            return (count);
        }
    }

    public bool IsReadOnly
    {
        get { return false; }
    }

    public int IndexOf(T item)
    {
        int itemIndex;

        lock ((lockList))
        {
            itemIndex = internalList.IndexOf(item);
        }

        return (itemIndex);
    }

    public void Insert(int index, T item)
    {
        lock ((lockList))
        {
            internalList.Insert(index,item);
        }
    }

    public void RemoveAt(int index)
    {
        lock ((lockList))
        {
            internalList.RemoveAt(index);
        }
    }

    public T this[int index]
    {
        get
        {
            lock ((lockList))
            {
                return internalList[index];
            }
        }
        set
        {
            lock ((lockList))
            {
                internalList[index] = value;
            }
        }
    }
}

Your new approach is much better than the previous one. Instead of deriving from List and using new to hide the base class implementations you create a wrapper around a List implementing IList - this is a much more robust way.

However there are a few oddities:

You don't need to capture the result of a function call in a local variable to return it outside of the lock. You can simply do:

lock (myLock)
{
    return SomeFunction();
}

The Clone implementation is using a ThreadLocal object which is not necessary. For example:

void DoSomething()
{
     var list = new List<int>();
}

If two threads call DoSomething at the same time they will not share list - each will get their own list.  So Clone can be shortened to:

public List<T> Clone()
{
    lock (lockList)
    {
        return new List<T>(internalList);
    }
}

(the LINQ ToList() extension method creates a new copy).

Update: As for your question whether using a ReaderWriterLockSlim would be better - it depends. Using a reader-writer lock is more complex because now you have to make sure you correctly obtain the read/write lock in the correct places. Using a plain lock will be sufficient in 99.99% of all cases and should only be optimized if you actually have proof that it causes a performance problem.

The lock statement also has nice compiler support which will generate the try-catch block around it to make sure it's released when anything throws - something you have to do yourself if you use another lock.

And last but not least I'm suspicious of your need for a thread-safe list. The main feature of IList is the random access capabilities for reading/writing/inserting/removing items at a specific index. However in a multi threaded world these features are less useful than you might think. For example doing this:

if (list.Count > indexImInterestedIn)
{
    DoSomething(list[indexImInterestedIn]);
}

is not thread safe because between the conditional check and the access something could have easily removed elements from the list. So the above code can fail even though the individual operations are thread-safe.

If you check your use-case for the thread-safe list you will probably find that you won't actually need an IList.


Verifying a South African ID I'm verifying South African IDs with jQuery, the code below works but Id like to know if it could be simplified in anyway or if there is a better way of doing things here?

South African IDs are verified as follows:

Using ID Number 8001015009087 as an example:
Add all the digits in the odd positions (excluding last digit).
8 + 0 + 0 + 5 + 0 + 0 = 13  .............................[1]

Move the even positions into a field and multiply the number by 2.
011098 x 2 = 22196  .....................................[2]

Add the digits of the result in [2].
2 + 2 + 1 + 9 + 6 = 20  .................................[3]

Add the answer in [3] to the answer in [1].
13 + 20 = 33  ...........................................[4]

Subtract the second digit of [4](i.e. 3) from 10. The number must tally with
the last number in the ID Number. If the result is 2 digits, the last digit is
used to compare against the last number in the ID Number. If the answer differs,
the ID number is invalid.

And the code:

var me = $("#id");
var odd = new Number();
var even_string = new String();
var even_result = new Number();
var even = new Number();
var result = new Number();

// Check length
if( me.val().length == 13 )
{
    $.each(me.val(), function(p,v){
        if (p%2 == 0 & p != 12)
        {
            odd += Number(v); // --> 1. Add all odd positions except the last one.
        }
        else
        {
            if(p != 12)
            {
                even_string += String(v); // --> 2.1 Join all even positions.
            }
        }
    });

    // 2.2 Multiply even string by two.
    even_result = (even_string * 2);

    // 3. Add the digits of the new even result in two point two.
    $.each(String(even_result), function(p,v){
        even += Number(v);
    });

    // 4. Add answer in three to the answer in one.
    result = odd+even;

    // 5. Subtract the second digit from step 4. from ten.
    result = 10 - Number(String(result).substr(1,1));

    // 6. Make sure we use the very last digit on the result.
    if( (String(result)).length > 1 )
    {
        result = Number( (String(result)).charAt( (String(result)).length-1 ) );
    }

    // 6. The final check
    if( Number(result) != me.val().substr(12,1) )
    {
        // Not a valid South African ID
        alert("Your South African ID is not valid.");
    }
};

jQuery is entirely unnecessary and will just make your solution slower
Don't initialize to new Class() - this is not Java. Instead, initialize to the initial value, or undefined (var x;).
use a function to encapsulate your verification logic, and to separate it from the DOM interface.
don't call me.val() more than once; cache the result
instead of the slow $.each, use a simpler loop
You already know the length of the id string, so you can get rid of the p%2 check and simply operate twice on each loop, once on the current position and once on p + 1, and increment by 2 each time (this also results in half as many iterations, speeding up the loop even further).
Cast using +string (resulting in a number) and '' + number (resulting in a string)
split the string into the first 12 and the last 1 at the beginning, so you don't have to keep excluding the last character and re-extracting it.
'string'.slice(-1) is a more concise way to access the last character in a string
check to make sure that not only is the string 13 characters long, but also that every character is a numeral

With all that and a few stylistic preferences, here's my take on your problem:

function isValidSAID(id) {
    var i, c,
        even = '',
        sum = 0,
        check = id.slice(-1);

    if (id.length != 13 || id.match(/\D/)) {
        return false;
    }
    id = id.substr(0, id.length - 1);
    for (i = 0; c = id.charAt(i); i += 2) {
        sum += +c;
        even += id.charAt(i + 1);
    }
    even = '' + even * 2;
    for (i = 0; c = even.charAt(i); i++) {
        sum += +c;
    }
    sum = 10 - ('' + sum).charAt(1);
    return ('' + sum).slice(-1) == check;
}

if (!isValidSAID(document.getElementById('id').innerHTML)) {
    alert('Your South African ID is not valid.');
}


Using a Boolean method that never returns "false" to check user permissions I need to check that a user is allowed to save/retrieve contacts to/from the database by calling a web service, and return an HTTP403 with an explanation if it is not the case.

So for the sake of factorizing the code, I decided to use a boolean method that "never" returns false and is there only to check this, and throws a corresponding exception.

public boolean canPerformAction(User authenticatedUser) throws ForbiddenActionException{
  if(authenticatedUser == null){
    throw new ForbiddenActionException("There must be a user to perform an action !");
  }

  if(!authenticatedUser.BelongToCompany(company)){
    throw new ForbiddenActionException("The user doesn't belong to the company, therefore he can't perform the action");
  }

  return true;
}

Then in saveContact, I call it as follows:

public Person saveContact(User user, Person contact) throws ForbiddenActionException{

  canPerformAction(user);

  contact= persistContact(contact);

  return contact;
}

I do the same thing in retrieveContact:

public Person retrieveContact(User user, String contactId) throws ForbiddenActionException{

    canPerformAction(user);

    return fetchContact(contactId);
 }

And finally these methods are called in the web service:

@GET
@Path("{id}")
@Produces(MediaType.APPLICATION_JSON)
public Response retrieveContact(@PathParam("contactId") String contactId) {
  try{
    Person contact= retrieveContact(getUser(), contactId);
    return Response.status(Response.Status.OK).entity(contact).build();
  }catch(ForbiddenActionException e){
    return Response.status(Response.Status.FORBIDDEN).entity(e.getMessage()).build();
  }
}

@POST
@Path("{id}")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
public Response saveContact(@PathParam("id") String id, Person contact) {
  try{
    Person contact= saveContact(getUser(), contact);
    return Response.status(Response.Status.OK).entity(contact).build();
  }catch(ForbiddenActionException e){
    return Response.status(Response.Status.FORBIDDEN).entity(e.getMessage()).build();
  }
}

Do you think that this is an acceptable approach of dealing with the problem ?

The Question

The approach is acceptable and it's perfectly valid to use a method like canPerformAction. But it shouldn't return an unused boolean and can be static:

private static void ensureUserAuthorized(User user) throws ForbiddenActionException {
  if (user == null) {
    throw new ForbiddenActionException("message");
  }
  if (!user.belongsToCompany(company)) {
    throw new ForbiddenActionException("message");
  }
}

Other Stuff

The boilerplate response building instructions return Response.status... in service methods can be avoided in two ways:

1) Create an ad-hoc shortcut method like

private static Response buildResponse(Status status, Object entity) {
  return Response.status(status).entity(entity).build();
}

Or, better

2) If you are using Jersey behind your JAX-RS annotations, add an exception mapper class:

@Provider
public class ForbiddenActionMapper implements ExceptionMapper<ForbiddenActionException> {

  @Override
  public Response toResponse(ForbiddenActionException ex) {
    return Response.status(Response.Status.FORBIDDEN)
                   .entity(ex.getMessage())
                   .type(MediaType.APPLICATION_JSON)
                   .build();
  }
}

In this second case the service method can be shortened as follows:

@POST
@annotations...
public Contact saveContact(@PathParam("id") String id, Person contact) throws ForbiddenActionException {
  return saveContact(getUser(), contact);
}


Animal inheritance hierarchy I'm trying to figure out if I'm doing superclasses right.  Also, I welcome critique on Java structure/syntax etc.

Animal.java:

public class Animal{
    private static int counter = 0; // how many animals we have created (to demonstrate a static variable)
    private Boolean isWild = true; // is the animal wild?
    private Boolean hasSharpPointyTeeth = false; // does it have sharp, pointy teeth?!
    private int age = 0; // how old

    Animal(){ // default constructor
        this.isWild = isWild;
        this.hasSharpPointyTeeth = hasSharpPointyTeeth;
        this.age = age;

        counter++;
    }

    Animal(Boolean isWild, Boolean hasSharpPointyTeeth, int age){
        this.isWild = isWild;
        this.hasSharpPointyTeeth = hasSharpPointyTeeth;
        this.age = age;

        counter++;
    }

    //begin getters/setters
    public void setIsWild(Boolean isWild){
        this.isWild = isWild;
    }
    public void setAge(int age){
        this.age = age;
    }
    public void setHasSharpPointyTeeth(Boolean hasSharpPointyTeeth){
        this.hasSharpPointyTeeth = hasSharpPointyTeeth;
    }
    public Boolean getIsWild(){
        return isWild;
    }
    public Boolean getHasSharpPointyTeeth(){
        return hasSharpPointyTeeth;
    }
    public int getAge(){
        return age;
    }
    public static int getCounter(){
        return counter;
    }
    //end getters/setters

    public String speak(){
        return "Grrrr!";
    }

    public String toString(){
        String out =    speak() + "\n" +
                        "Age: " + getAge() + "\n" +
                        "Wild? " + getIsWild() + "\n" +
                        "Has Sharp Pointy Teeth? " + getHasSharpPointyTeeth() + "\n";
        return out;
    }
}

Dog.java

public class Dog extends Animal{
    private String name = "None";
    private String owner = "None";
    private String breed = "Unknown";
    private String sex = "Unknown";

    Dog(){
        super(false, false, 0); // call the constructor from the Animal class
        this.name = name;
        this.owner = owner;
        this.breed = breed;
        this.sex = sex;
    }

    Dog(Boolean isWild, Boolean hasSharpPointyTeeth, int age, String name, String owner, String breed, String sex){
        super(isWild, hasSharpPointyTeeth, age); // call the constructor from the Animal class
        this.name = name;
        this.owner = owner;
        this.breed = breed;
        this.sex = sex;
    }

    //begin getters/setters
    public void setName(String name){
        this.name = name;
    }
    public void setOwner(String owner){
        this.owner = owner;
    }
    public void setBreed(String breed){
        this.breed = breed;
    }
    public void setSex(String sex){
        this.sex = sex;
    }
    public String getName(){
        return name;
    }
    public String getOwner(){
        return owner;
    }
    public String getBreed(){
        return breed;
    }
    public String getSex(){
        return sex;
    }
    //end getters/setters

    @Override
    public String speak(){
        return "Woof!";
    }

    @Override
    public String toString(){
        String out =    super.toString() + // call the toString method of Animal, not the overridden one in Dog
                        "Name: " + getName() + "\n" +
                        "Breed: " + getBreed() + "\n" +
                        "Sex: " + getSex()+ "\n" +
                        "Owner: " + getOwner() + "\n";
        return out;
    }
}

Test.java

public class Test{
    public static void main(String[] args){
        Dog fluffy = new Dog(false,true,3,"Fluffy","Ted","Westie","Male");
        Animal rabbit = new Animal(true,true,8);

        System.out.println("We made " + Animal.getCounter() + " animals.  Here's their info:");
        System.out.println();
        System.out.println(fluffy);
        System.out.println(rabbit);
    }
}

private Boolean isWild = true; // is the animal wild?
private Boolean hasSharpPointyTeeth = false; // does it have sharp, pointy teeth?!

Why are you using the wrapper class? Use the primitive boolean instead.
Also, about comments:
private static int counter = 0; // how many animals we have created (to demonstrate a static variable)
private Boolean isWild = true; // is the animal wild?
private Boolean hasSharpPointyTeeth = false; // does it have sharp, pointy teeth?!
private int age = 0; // how old

All the comments here are unnecessary with better variable names. Comments explain why, not what.
All of the variable names are fine, maybe except counter: could be numberOfAnimals...
NAMING
public Boolean getIsWild()

should be isWild(). Likewise,
public Boolean getHasSharpPointyTeeth()

should be hasSharpPointyTeeth().
But getAge(), for example, should stay getAge().
public static int getCounter()

should be getNumberOfAnimals()
speak() is poorly named. I would've expected it to print what it says, but it just returns what it says. I would either change it to do what I said, even though I would suggest against it, or rename it. I would suggest rename it to getSpeech().
Misc
    this.isWild = isWild;
    this.hasSharpPointyTeeth = hasSharpPointyTeeth;
    this.age = age;

In the default constructor is completely unnecessary: it already has those values, does it need those same values again? Same in the Dog class:
    this.name = name;
    this.owner = owner;
    this.breed = breed;
    this.sex = sex;

The begin and end getters/setters comments are unnecessary: why are those there?
The (){ after each method should be () { according to Java standard conventions.
Constructors should be public.
I think the sex property should be Animal's.
Also you should check the parameters and see if they are valid: you don't want age to be set to -1 or breed to be set to something like hello... do you? I'll leave that up to you, but the general format is this:
if (is not valid) {
    throw new IllegalArgumentException("Why is it not valid?);
}

You shouldn't be testing with main(). Instead use JUnit 4 or any similar testing library.
Final Code
public class Animal {

    private static int numberOfAnimals = 0;
    private boolean isWild = true;
    private boolean hasSharpPointyTeeth = false;
    private int age = 0;
    private String sex = "Unknown";

    public Animal() {
        numberOfAnimals++;
    }

    public Animal(String sex, boolean isWild, boolean hasSharpPointyTeeth, int age) {
        this.sex = sex;
        this.isWild = isWild;
        this.hasSharpPointyTeeth = hasSharpPointyTeeth;
        this.age = age;
        numberOfAnimals++;
    }

    public void setIsWild(Boolean isWild) {
        this.isWild = isWild;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public void setHasSharpPointyTeeth(Boolean hasSharpPointyTeeth) {
        this.hasSharpPointyTeeth = hasSharpPointyTeeth;
    }

    public void setSex(String sex) {
        this.sex = sex;
    }

    public Boolean isWild() {
        return isWild;
    }

    public Boolean hasSharpPointyTeeth() {
        return hasSharpPointyTeeth;
    }

    public int getAge() {
        return age;
    }

    public String getSex() {
        return sex;
    }

    public static int getNumberOfAnimals() {
        return numberOfAnimals;
    }

    public String getSpeech() {
        return "Grrrr!";
    }

    public String toString() {
        String out = getSpeech() + "\n" + "Age: " + getAge() + "\n" + "Wild? "
                + isWild() + "\n" + "Has Sharp Pointy Teeth? "
                + hasSharpPointyTeeth() + "\n";
        return out;
    }

}

public class Dog extends Animal {

    private String name = "None";
    private String owner = "None";
    private String breed = "Unknown";

    public Dog() {
        super("Unknown", false, false, 0);
    }

    public Dog(boolean isWild, boolean hasSharpPointyTeeth, int age, String name,
            String owner, String breed, String sex) {
        super(sex, isWild, hasSharpPointyTeeth, age);
        this.name = name;
        this.owner = owner;
        this.breed = breed;
    }

    public void setName(String name) {
        this.name = name;
    }

    public void setOwner(String owner) {
        this.owner = owner;
    }

    public void setBreed(String breed) {
        this.breed = breed;
    }

    public String getName() {
        return name;
    }

    public String getOwner() {
        return owner;
    }

    public String getBreed() {
        return breed;
    }

    @Override
    public String getSpeech() {
        return "Woof!";
    }

    @Override
    public String toString() {
        String out = super.toString()
                + "Name: " + getName() + "\n" + "Breed: " + getBreed() + "\n"
                + "Sex: " + getSex() + "\n" + "Owner: " + getOwner() + "\n";
        return out;
    }

}


Forming output of lines based on input I have homework, in which I get number of cases and then four numbers.  Each number tells how many rows there are in each column

For example: 2 2 3 3 should be viewed like this:

    _ _
_ _ _ _
_ _ _ _

Wherever there are lines that can be joined in one, I have to do this, and write out minimum number of lines required to make that kind of sketch.

So the above would be:

  __
____
____

And the correct answer is 3.

A more complicated example:

3 1 2 1

Should be viewed as:

_
_   _
_ _ _ _

Joining the lines:

_
_ _
_____

And the answer is: 4.

Here is the code:

#include <iostream>
using namespace std;

int main()
{
    unsigned long long int stPrimerov = 0;
    cin >> stPrimerov;
    unsigned long long int *totalRows = new unsigned long long int[stPrimerov];

    for (unsigned long long int i = 0; i < stPrimerov; i++)
    {
        //totalRows[i] = 0;
        unsigned long long int a, b, c, d;
        cin >> a >> b >> c >> d;
        /*a = rand() % 100000000;
        b= rand() % 100000000;
        c= rand() % 100000000;
        d = rand() % 100000000;*/

        unsigned long long int temp = 0;
        unsigned long long int array[4] = { a, b, c, d };
        for (int k = 0; k<4; k++)
        {
            if (array[k]>temp)
                temp = array[k];
        }

        //totalRows[i] = 0;
         /*int* aArray = new int[temp];
         int* bArray = new int[temp];
         int* cArray = new int[temp];
         int* dArray = new int[temp];*/
        unsigned long long int ttr = 0;
        for (unsigned long long int l = 0; l< temp; l++)
        {
            int aa, bb, cc, dd;
            if (l < a) aa = 1;
            else aa = 0;
            if (l < b) bb = 1;
            else bb = 0;
            if (l < c) cc = 1;
            else cc = 0;
            if (l < d)dd = 1;
            else dd = 0;

            if (aa == 0 && bb == 0 && cc == 0 && dd == 0) {continue; }
            else if (aa == 0 && bb == 1 && cc == 0 && dd == 1) { ttr += 2; continue; }

            else if (aa == 1 && bb == 0 && cc == 0 && dd == 1) { ttr += 2; continue; }
            else if (aa == 1 && bb == 0 && cc == 1 && dd == 0) { ttr += 2; continue; }
            else if (aa == 1 && bb == 0 && cc == 1 && dd == 1) { ttr += 2; continue; }

            else if (aa == 1 && bb == 1 && cc == 0 && dd == 1) { ttr += 2; continue; }
            else{ ttr+= 1; continue; }

        }
        totalRows[i] = ttr;

    }

    for (unsigned long long int a = 0; a < stPrimerov; a++)printf("%d\n",totalRows[a]);
    //system("pause");
    return 0;
}

I have to make it faster.

Example input:

3
2 1 6 4
0 1 1 1
3 9 8 11

Result:

7
1
12

Number of cases can be up to 104 and number of rows up to 108.

Cause of your performance issue

As this is homework, I don't want to give away the whole solution but I'll tell you where the problem is and if you need more help, just comment and I'll see what I can do.

You can have up to \$10^4\$ test cases and the number of rows can go up to \$10^8\$. You're better off assuming your examiner will actually use such a test case. More than likely they will.

Looking at your code you have two for-loops.

This one:

  for (unsigned long long int i = 0; i < stPrimerov; i++)

and this one:

  for (unsigned long long int l = 0; l< temp; l++)

where temp is \$\max_{\forall i}(row_i)\$. So the worst case is when: temp = 10E8, and then the inner loop is executed stPrimerov=10E4 times, resulting in \$10^{12}\$ iterations through the inner loop. Now that's a lot.

Instead of this for (unsigned long long int l = 0; l< temp; l++) you must realize that there are only 4 interesting points that you need to look at, and those are l=array[x]. So by looking at the values in the array and how they differ, you will be able to determine how many lines are necessary without checking each row. So if you can replace the inner for loop with some smart checking, you can reduce from \$10^{12}\$ to \$10^{4}\$ loop iterations which should make even the hardest test cases run in under half a second. :)

Try to think of it for yourself for a while and come back later if you're still stuck and look at the hints below.

Hint #2

   Let's consider an easier problem. Let the input be the same, but all
 columns are either 0 or k high. Like this:

     3
     0 3 3 0
     0 4 0 4
     9 0 9 9

 Result :

     3  (3*1)
     8  (4*2)
     18 (9*2)

 Now this is easy, just count the number of streaks of non zero
 columns. There is one streak that is two wide in the first case.
 There are two streaks that are one column wide in the second set. And
 there are two streaks in the last case, one that is one column wide and
 one that is two columns wide. Then multiply the number of streaks by
 the height of the columns.

Hint #3

   Given four columns of (possibly) unique heights, the rows can be split
 into at most 4 sections where each column in the section is either
 zero or equally tall as the other columns. With these two hints you
 should be able to figure out how to calculate the answer without
 checking every row in the inner loop.


URL Routing Application In a Ruby on Rails application I want to have this routing:

for URL www.example.com/reports go to controller documents and action reports
for URL www.example.com/charts go to controller documents and action charts

In the routes.rb file I currently have:

scope controller: :documents do
  get 'reports' => :reports
  get 'charts'  => :charts
end

The code works as expected, but is there a way so I keep the code dry and don't repeat the action names?

You could do the following. (I assume you already have a resourceful route for your documents right?)

resources :documents do
  collection do
    get 'reports'
    get 'charts'
  end
end

To take advantage of the nested routing provided by rails (see the docs)

Which will generate the following urls:

           Prefix Verb   URI Pattern                                                                    Controller#Action
reports_documents GET    /documents/reports(.:format)                                                   documents#reports
 charts_documents GET    /documents/charts(.:format)                                                    documents#charts

If you don't have any resourceful route, you have to declare it like you already did. The only thing to do would be iterating over an array like this:

scope controller: :documents do
  %w(reports charts).each do |action|
    get action
  end
end

But IMO this hurts the readability of the code, and is only necessary when dealing with a lot of routes.


Extending Exception: avoiding code duplication I'm implementing a REST API in PHP 7.1. It is designed to throw Exceptions to print non-200 responses.

I decided to go with the following:

abstract class APIException extends \Exception
{
    public function __construct($message, $code, Exception $previous = null) {
        parent::__construct($message, $code, $previous);
    }

    public function parseException() {
        return ResponseParser::parseResponse($this->message, $this->code);
    }
}

And then I extended the BaseException with my various personal exceptions, like IncorrectMethodException here.

class IncorrectMethodException extends APIException
{
    public function __construct(String $expectedMethod, Exception $previous = null) {
        parent::__construct("Incorrect method, $expectedMethod expected", 405, $previous);
    }
}

Here is the code to my ResponseParser class:

class ResponseParser
{
    /**
     * @param $data
     * @param int $status
     * @return string
     */
    public static function parseResponse($data, $status = 200)
    {
        header("Access-Control-Allow-Origin: *");
        header("Access-Control-Allow-Methods: *");
        header("Content-Type: application/json");
        header("HTTP/1.1 " . $status . " " . self::_requestStatus($status));
        return json_encode($data);
    }

    /**
     * @param $code
     * @return string status code
     */
    private static function _requestStatus($code)
    {
        $status = array(
            200 => 'OK',
            400 => 'Bad Request',
            401 => 'Unauthorized',
            404 => 'Not Found',
            405 => 'Method Not Allowed',
            500 => 'Internal Server Error',
        );
        return ($status[$code]) ? $status[$code] : $status[500];
    }
}

Is this a correct way to avoid code duplication here?

Naming:

ResponseParser doesn't actually parse a response. Parsing a response would mean taking some response object and extracting data from it. This class does the opposite - given some data, it generates HTTP response.

Separation of concerns:

Exception classes are aware of how to output themselves as HTTP response. Would be better to structure things the other way around: passing the exception object to ResponseParser that generates the HTTP response.

Side-effects:

parseResponse() outputs HTTP headers and returns JSON. It's confusing when function has a return value and it also generates some output. I would it to expect to also output the JSON as the response, or returning both headers data and JSON without causing any side-effects.


Wrapping std::bind call in a function returning std::function Is this code correct?

It appears to work in g++ 4.8.3 and clang (bcc64), however appearing to work is no guarantee of correctness :)

The aim is to be able to replace a call to a member function:

void Master::go()
{
     std::string s;
     func(s);    // func is a member function of Master
}

with a function template that wraps this call (and would indeed work with any class, not just Master):

void Master::go()
{
    std::string s;
    call_function( func, s );    // pseudocode
}

which ends up calling this->func(s).

The code which seems to work:

#include <iostream>
#include <string>
#include <functional>

struct Master
{
    void func(std::string &s) { std::cout << "Master::func(" << s << ")\n"; }
    void go();
    Master() {}

private:
    Master(Master const &);
};

int main()
{
    Master m;
    m.go();
}

template<typename FuncT>
void call_function( std::function<FuncT> fp, std::string s )
{
    fp(s);
}

template<typename FuncT, typename Host>
typename std::function<FuncT> my_binder(Host *h, FuncT Host::*p)
{
    using std::placeholders::_1;
    return std::bind(p, h, _1);
}

void Master::go()
{
    std::string s("hello");
    call_function( my_binder(this, &Master::func), s );
}

If the code is correct, then is my function my_binder a good idea, or am I overlooking a simpler option?

I see a few potential problems that you might want to address.  First, I'll have to say that it's not entirely clear what problem you're trying to solve. That is, it's not clear why one wouldn't just call func(s) as in your very first example.

Handle const functions and objects

The my_binder call doesn't currently handle const functions.  For example, we can easily declare Master::func const.  However, the template fails in this case.  Things only get worse if we also make go const and then declare the instance m of Master to be const.  I'm not just sure how to fix that.

Pass const string reference

A more minor issue is that the second argument to call_function ought to pass the string by reference to avoid creation of a temporary and can be const.

Consider using a lambda instead

Here's an implementation that doesn't use templates and may address your need.

void Master::go() const
{
    const std::string s("hello");
    [&](){func(s);}();
}

That funny looking last line is both declaration and call of a lambda.  It (rather pointlessly in this case) captures all in-scope variables by reference, takes no parameters and invokes func(s).  It then actually calls that lambda with no parameters.  It's a complex way of just saying func(s) but then, so was call_function(my_binder(this, func), s); so I am hoping that wherever it made sense to do the latter, it may be better instead to use a lambda.

To me, this is much simpler and easier to understand, especially since you're using C++11.


Is there any way to improve (shorten) this F# code? I have a very good grasp of the syntax and features of F# as well as some of the concepts that mesh well with the language. However, I do not have enough experience writing it to feel comfortable that the way I am handling the language is appropriate.

Ignoring the fact that I do not provide proper escaping functionality for the file format, is there any way I could write this code better? I know that this is a trivial example, but I fear that I might be making things too hard on myself with this code.

Also, is there a way that I could write this more robustly so that adding in an escape sequence for the ";" character would be easier?

module Comments

open System.IO

type Comment = { Author: string; Body: string }

let parseComment (line:string) =
    match line.Split(';') with
    | [|author; body|] -> Some({Author=author; Body=body})
    | _                -> None

let filterOutNone maybe = match maybe with | Some(_) -> true | _ -> false
let makeSome some = match some with | Some(v) -> v | _ -> failwith "error"

let readAllComments () =
    File.ReadAllLines("comments.txt")
    |> Array.map parseComment
    |> Array.filter filterOutNone
    |> Array.map makeSome

Most of your code can be replaced with a comprehension:

type Comment = { Author: string; Body: string }

let readAllComments () =
  [|for line in System.IO.File.ReadAllLines("comments.txt") do
      match line.Split ';' with
      | [|author; body|] -> yield {Author=author; Body=body}
      | _ -> ()|]