Getting position of coordinates and checking if they match I have to compare variables to obtain coordinates.

           l
upleft     l     upright
-----------l--------------
downleft   l     downright
           l

Here is what I am doing now:

protected function check_if_quadrant_is_valid($variable)
    {
        $middle_of_x = 297;
        $middle_of_y = 421;

       $heigth =  $variable["y"] <= $middle_of_y ?   "up" :  "down";

       $side =  $variable["x"] <= $middle_of_x ?  "left" : "right";

            $variable_location = $heigth . $side;

            if($this->item->$variable_location == 1)
            {
                return true;
            }
         return false;
    }

The variable $variable is an array with x and y as key and their coordinates as value.

The variable $item is an object. It contains whether the variable should be in the quadrant. It contains $item->upleft, $item->upright, $item->downleft, $item->downright that are booleans.

The point of this function is to retrieve the quadrant of the $variable and check if it is valid within the $item.

The code works perfectly, however it feels very cheap.

Here is the same that can be copypasted and their values changed to test:

function check_if_quadrant_is_valid()
    {
        #Change at will
        $item["upleft"] = 0;
         $item["upright"] = 1;
          $item["downleft"] = 1;
           $item["downright"] = 1;

        $variable["y"] = 90;
        $variable["x"] = 20;

        $middle_of_x = 297;
        $middle_of_y = 421;

       $heigth =  $variable["y"] <= $middle_of_y ?   "up" :  "down";

       $side =  $variable["x"] <= $middle_of_x ?  "left" : "right";

            $variable_location = $heigth . $side;

            if($item[$variable_location] == 1)
            {
                return true;
            }
         return false;
    }

I find the original code overly complicated.

Here is a revised version of the original test code.

class Test {

    private $item = [ [ false, true ], [ true, true ] ];

    const VERTICAL_MIDDLE = 421;
    const HORIZONTAL_MIDDLE = 297;

    public function is_quadrant_valid($variable) {
        $down = $variable['y'] > self::VERTICAL_MIDDLE;
        $right = $variable['x'] > self::HORIZONTAL_MIDDLE;

        return $this->item[$down][$right];
    }

}

$t = new Test();
echo $t->is_quadrant_valid([ 'y' => 90, 'x' => 20 ]) ? 'Valid' : 'Invalid';

This includes a class, which seems to be consistent with the original code.

This version defines $item as a private property of the class.  Rather than making keys by concatenating strings, I use a two dimensional array system.  This also allows us to arrange $item such that the quadrant organization matches.

Rather than storing 0s and 1s that we have to convert into false and true values, this just stores true and false directly.

Instead of using regular variables to store the coordinates of the middle, this uses constants.

I changed the name of the function to be shorter, but I don't think that it is any less clear.

Since the original takes a function parameter, it seems that the test version should as well.  This way we could run multiple tests just calling with different values.

Since we index the arrays with 0 for false and 1 for true, we can simplify the variables.  I renamed them to better match what they hold.

I find this simpler and easier to read.  You should be able to easily transfer this back to your original code if desired.

As a general rule, if (is_true()) return true; else return false; can be replaced by return is_true(); which is much shorter.


Is this String concatenation creating extra String? Using the eclipse formatting on a class with some @NamedQueries created concatenations.

While I have been doing this for years without paying attention I just realised that may be it
is  not a good usage.

So basically Which one of these Strings inside the queryis better ? Is the second one creating too many Strings? Should I be bothered at all?

My answer would be yes second one is creating extra Strings but I am not sure I should be concerned.

First Query

 @NamedQuery(
      name = SomeConstant,
      query="SELECT person FROM Person person Where person.name = :name ")

Second Query

 @NamedQuery(
      name = SomeConstant,
      query="SELECT person FROM Person person"+" "+
               "Where person.name = :name ")

Enter the mad world of optimization!

The Java compiler and HotSpot just-in-time compiler are programmed with a great many rules for optimizing performance of Java code.

In general, it is NOT worth your time to fret about the performance impact of these kinds of details as they are often taken care of. Rather, just try to write clean readable code that solves the problem. If performance is an issue, profile the code and then optimize the bottlenecks.

Now, to address this specific issue, I am virtually certain that constant expressions are resolved by the Java compiler at compile time. I would be very surprised if the byte code for your two queries wasn't identical.


A simple but useful error handler I am trying to make a very simple error handler that would catch and handle all kinds of errors in both development and production environments. The idea is to keep it as simple as possible yet useful in the production environment.

In order to do so I am converting all errors into exceptions. Then I will need just a single function to handle both errors and exceptions.

Then, according to my principles of error handling

in the dev environment, it should just echo the error information out, and
in the prod environment, the error information has to be logged and a generalized error message has to be shown on-screen.

It am going to distinguish the modes using display_errors php.ini parameter and also consider all CLI scripts to be in the dev mode too.

set_error_handler(function ($level, $message, $file = '', $line = 0)
{
    throw new ErrorException($message, 0, $level, $file, $line);
});

set_exception_handler(function ($e)
{
    if (ini_get('display_errors') || php_sapi_name() === 'cli') {
        echo $e;
    } else {
        error_log($e);
        header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
        echo "
500 Internal Server Error

              An internal server error has been occurred.

              Please try again later.";
    }
});

There is vast room for improvement, like to create distinct exception classes for each error type (such as DeprecatedErrorException, NoticeErrorException etc.) or, based on the severity, to decide whether the script execution has to be terminated or not. Or a more creative/user-friendly error message. But I am not sure if I should include all that in a simple demonstration example. What are essential additions or modifications could be made to this code?

Perhaps a more "friendly" message would be nice and/or a way to customize it. As an example the generic catcher we're using right now reads:

echo '

Sorry but we ran into an unexpected problem.

The problem has been logged and support staff have been notified.

Please try again.

';


Python calculator script Are there any ways to make this calculator script for Python better or simpler?

thelist = ["Add", "add", "Multiply", "multiply", "Divide", "divide","Subtract", "subtract"]

def Multiply(x,y):
z = x * y
print(z)

def Divide(x,y):
x = float(x)
y = float(y)
z = x / y
print(z)

def Add(x,y):
z = x + y
print(z)

def Subtract(x,y):
z = x - y
print(z)

while True:
    operation = input("What would you like to do? Multiply/Divide/Add/Subtract ")
    if operation in thelist:
        break
    else:
        print("That was not an option..")

if operation == "Multiply" or operation == "multiply":
    while True:
        try:
            x = int(input("First number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    while True:
        try:
            y = int(input("Second number: "))
            break
        except ValueError:
            print("Make sure to enter a number...")
    Multiply(x,y)
elif operation == "subtract" or operation == "Subtract":
    while True:
        try:
            x = int(input("First number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    while True:
        try:
            y = int(input("Second number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    Subtract(x,y)
elif operation == "Add" or operation == "add":
    while True:
        try:
            x = int(input("First number: "))
            break
        except ValueError:
            print("Make sure to enter a number..")
    while True:
        try:
            y = int(input("Second number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    Add(x,y)
elif operation == "Divide" or operation == "divide":
    while True:
        try:
            x = int(input("First number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    while True:
        try:
            y = int(input("Second number: "))
            break
        except ValueError:
            print("Make sure to enter a number.")
    Divide(x,y)
else:
    pass

A thorough review would very much depend on what you're trying to achieve.

For example, it seams mighty clumsy to have to type multiply instead of just *. Also, a case-insensitive match would be much better. For that, you might want to check method lower or how to use regular expressions in Python.

Judging from your use of print, you're aiming at Python 3. If this is the case, then - unlike in Python 2 - you don't need

x = float(x)
y = float(y)

in function Divide. Of course, if you want your code to work in both Python 2 & 3, it's O.K. to keep that code. However, even in that case, converting only one of these two variables to float is enough.

Like most (all?) interpreted and pseudocompiled languages, Python has eval function. So, you can basically do result = eval(expression), where expression is a string with any Python code. For example, if you have a string "1+2*3", and send it to eval, you'll get 7 (unless my math is off :-)). This might help you significantly, but be careful to not pass on just any (potentially junky) user's input.

One thing seems strange to me. You do

if operation == "Multiply" or operation == "multiply":
    12 lines to input x and y
    Multiply(x,y)
elif operation == "subtract" or operation == "Subtract":
    THE SAME 12 lines to input x and y
    Subtract(x,y)
...

Why don't you first input x and y, and then do if-elif block for the actual computation, or, at least, make a function to input x and y and return them (functions in Python can return multiple values)? You have functions for simple expressions which are called only once (multiplication, subtraction, etc), which seem completely unnecessary, but you don't make one for 12 lines of code which are invoked 4 times in your code.

Regarding your naming of functions, function names should be lowercase, with words separated by underscores as necessary to improve readability, i.e., you shouldn't capitalize the names of your functions.


Removing nodes from queue I had an exercise to create two methods for simple queue class, first to delete last element and second to delete element at k position. Particularly I'm not asking for better implementation. Is it good enough and elegant ? What I should change ? I'll post only mentioned methods, rest should be simple enough.

/**
 * Deletes node at position k.
 */
public void delete(int k) {
    if (k > N || isEmpty()) throw new NoSuchElementException("Cannot delete element at position: " + k);
    Node previous = first;
    int deleteIndex = 1;

    if(k == deleteIndex){               //special case for first element
        if(first.next == null)
            last = null;
        first = first.next;
        N--;
        return;
    }

    for (Node node = first; node != null; node = node.next, deleteIndex++) {
        if (k == deleteIndex) {
            previous.next = node.next;
            if (previous.next == null) last = previous; //if deleted element was the last one
            N--;
            return;
        }
        previous = node;
    }
}

/**
 * Deletes last node from the queue.
 */
public void removeLastNode() {
    if(isEmpty()) throw new NoSuchElementException("Cannot delete last element from queue");
    if (N == 1) {           //special case for only one element in queue
        first = null;
        last = null;
        N--;
        return;
    }

    Node previous = null;
    for (Node node = first; node != null; node = node.next) {
        if (node.next == null) {
            previous.next = null;
            last = previous;
            N--;
        }
        previous = node;
    }
}

Well, your code is correct and mostly ok. Except for two things:

You have 1-based indexing? Follow the principle of least suprise, and use zero-based indexing like all other data structures as well.
Your variable names could be more obvious.

N seems to be the length or size of your list – use this longer name instead. Also, the name N does not adhere to the best practice that variables and members should start with lowercase characters.
k is an index – the traditional abbreviation for that is i.
deleteIndex is not the index which you want to delete. Rather, it is some iteration variable. I'd call it position.

Then there are some minor style issues.

It is advisable to always use if with braces, as it tends to improve readability. If you do not use braces, I would recommend to put the body of the if on the same line. You do this with your isEmpty guards, but not at if (first.next == null).
You have very long lines. I recommend using 80–100 characters. Your longest line seems to be 104 characters long with this intendation, which is too long. (Incidentally, your lines get shorter if you use the if with curlies…).
Instead of an NoSuchElementException you should consider an IndexOutOfBoundsException.

Some parts of your code could be made more obvious, or structured differently:

In delete, you have the test if (first.next == null), where you actually want to test if (size == 1).
Finding a node at a certain position is so useful that it should have a method of its own. You want a Node previous = nodeAt(i - 1); ... instead of the for loops.
If you don't do that, your for loops still have the problem that they do three things between iterations: previous = node; node = node.next; position++. You have put one of these at the end of the loop body, the rest in the increment statement of the for loop. It would have been better to group these together.


Is there a better way to loop through records and call an api? I have a CakePHP function here that gets info for a twitter screen_name from the twitter api and writes it to a mysql db. I'm just looking for feedback about other ways to accomplish this that might be more efficient with fewer loops maybe. The challenge is, there are many screen_names that need to be processed for a $reportid before the "report" is considered done. Any feedback is very much appreciated. thanks.

    function admin_wtfTwitter($reportid, $jobid) {

    $this->autoRender = false;

    //use if the api lags
    //set_time_limit(18000);

    //notify the log
    $this->admin_saveLog($reportid, $jobid, 'parsecrawlresults', 'Twitter wce started.');

    //get the twitter channels from reportdetails
    $channels = $this->Reportdetail->getChannel($reportid, 4);  //channel 4 = twitter
    foreach ($channels as $c) {

        //open reportdetail channel for writting
        $this->Reportdetail->create();
        $this->Reportdetail->set(array('id' => $c['Reportdetail']['id']));

        /*
        *   small_tag is null --> continue to next reportdetail record
        *   small_tag is bad
        *   small_tag is good but no data returned
        */
        if ($c['Reportdetail']['small_tag'] == null) { $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag null')); $this->Reportdetail->save(); continue; }

        $uri = 'http://api.twitter.com/1/users/show.json?screen_name=' . $c['Reportdetail']['small_tag'];
        $data = @file_get_contents($uri);
        if($data === false) { unset($data); $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag bad')); $this->Reportdetail->save(); continue; }
        $content = json_decode($data, true);

        //set fans, title username
        $this->Reportdetail->set(array(
            'fans' => $content['followers_count'],
            'title' => $content['description'],
            'username' => strtolower($content['screen_name'])
        ));

        //proceed with second api call****************************
        unset($uri);
        unset($data);
        unset($content);
        $i = 0;
        $e = 0;
        $postmonths = array();
        //LIMIT SET TO 1000 - CAN BE 3200 - https://dev.twitter.com/docs/api/1.1/get/statuses/user_timeline
        $uri = 'https://api.twitter.com/1/statuses/user_timeline.json?trim_user=true&exclude_replied=true&include_entities=true&include_rts=true&contributor_details=false&count=1000&screen_name=' . $c['Reportdetail']['small_tag'];
        $data = @file_get_contents($uri);
        if($data === false) { unset($data); $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag bad')); $this->Reportdetail->save(); continue; }
        $content = json_decode($data, true);

        //set engagement, posts
        foreach ($content as $t) {
            if (!empty($t['id_str'])) {
                $i++;
                $e = $e + $t['retweet_count'];
                array_push($postmonths, substr($t['created_at'], 4, 3)); //API resutls are: "created_at":"Fri Nov 30 00:38:34 +0000 2012
            }
        }
        $this->Reportdetail->set(array(
            'posts' => $i,
            'engagements' => $e
        ));

        //get and set frequency
        $freq = $this->admin_calcFrequency($i, count(array_unique($postmonths)));
        $this->Reportdetail->set(array('frequency' => $freq));

        //********************************************************

        //we're done with this one
        $this->Reportdetail->set(array('done' => 1));

        //commit the content to reportdetail record
        $this->Reportdetail->save();

    }

    //notify the log
    $this->admin_saveLog($reportid, $jobid, 'parsecrawlresults', 'Twitter wce completed.');
}

There are two fundamental principles I would like to point out: "Don't Repeat Yourself" (DRY) and Single Responsibility. The principles should be self explanatory from their names. The very first thing I notice is that you have a "god" function. In other words, a function that is trying to do everything. This violates both of these principles. DRY because the code cannot be easily reused; Single Responsibility because functions should do just one thing. To avoid this you should separate this one function into multiple smaller ones. The best way to do this is to try and determine each individual task you are trying to accomplish and separate accordingly. I'll leave that bit up to you.

A small thing real quick before continuing. This is actually a method, not a function, and therefore the access modifier should be explicitly defined as public, private, or protected to avoid accidents. By default it resolves to the public scope, but you should not depend upon that.

public function admin_wtfTwitter() {

Internal comments are very detrimental to legibility and code size. You should avoid them except when debugging. If your code is self-documenting then most comments will become unnecessary and you will only need doccomments for the rest. Comments like "notify the log" or "get the twitter channels" should be inherently obvious from the context. If you do nothing else, removing these comments alone will dramatically increase overall legibility.

You should also avoid (un)commenting out a feature to simulate different environments. If you have a feature that you will sometimes need to activate, you should create a switch in your function's arguments to handle it. For instance:

function admin_wtfTwitter( $reportid, $jobid, $lag = FALSE ) {
    if( $lag ) {
        set_time_limit( 180000 );
    }

Though I should caution that you shouldn't override your time limit like that. First of all, it makes your server hang while it waits for the task to finish. Second, it could mask a much larger problem, such as an infinite loop. If your program is doing so much that it can't finish, then that should be a sign that there is something wrong and you should fix it, not mask it. There are a number of different methods you can use: manually limit the initial amount of work, dynamically limit it by breaking it up into chunks, and caching results to name a few. There are probably more, but those are the ones I'm familiar with. The method you chose depends upon your situation.

You should avoid single letter variables except when used in generic throw away values or incrementals. Using $c instead of $channel is rather abstract and can later lead to confusion, especially in larger chunks of code. The same can be said for non-common acronyms, in other words acronyms that are not HTML, PHP, etc...

Whitespace is your friend and PHP never minds a little over-zealousness in its use. Never cram multiple statements onto one line, even if it is legible, but especially avoid it when its not. Having more than one level of indentation wont hurt. That being said you should be aware of the arrow anti-pattern and ensure you aren't violating it.

if( $channel[ 'Reportdetail' ] [ 'small_tag' ] == NULL ) {
    $this->Reportdetail->set( array(
        'done' => -1,
        'note' => 'small_tag null'
    ) );
    $this->Reportdetail->save();
    continue;
}

Lets return to the first principle I mentioned: DRY. There are a few subtle aspects that are sometimes overlooked. Usually this principle is applied by breaking code up into functions or loops and most think that is enough. But it can also be used for something as simple as saving a reused array pointer to a variable. So, instead of continuously typing out $c['Reportdetail']['small_tag'], save that value to a variable, say $small_tag and use that instead. Later, should you decide to change the source or path, the initial value of the variable is the only line you will have to worry about.

Continuing on this line of thought, if you are setting multiple values to a resource, then the best way is to do it all at once. Not only does this follow DRY, it also saves processing time. Currently you are using $this->Reportdetail->set() method repeatedly to set multiple attributes. Always avoid accessing or manipulating resources multiple times when at all possible.

$attributes = array(
    'id' => $channel[ 'Reportdetail' ] [ 'id' ]
);

if( $small_tag == NULL ) {
    $attributes[ 'done' ] = -1;
    $attributes[ 'note' ] = 'small_tag null';

    $this->Reportdetail->set( $attributes );
    $this->Reportdetail->save();
    continue;
}

You can even go one step farther and create a "default" state. For example, if the $small_tag is NULL or bad the "done" attribute is set to "-1" and there is a "note" attribute. So if we set up our $attributes array with the null values as a default, we can manipulate or unset attributes as necessary. This sometimes even allows us to remove an if statement if done right.

$attributes = array(
    'id'   => $channel[ 'Reportdetail' ] [ 'id' ],
    'done' => -1,
    'note' => 'small_tag null'
);

$uri = "http://api.twitter.com/1/users/show.json?screen_name=$small_tag";
$data = file_get_contents( $uri );
if( $data === FALSE ) {
    if( $small_tag ) {
        $attributes[ 'note' ] = 'small_tag bad';
    }

    $this->Reportdetail->set( $attributes );
    $this->Reportdetail->save();
    continue;
}

$attributes[ 'done' ] = 1;
unset( $attributes[ 'note' ] );

There are a few things I want to point out about what I demonstrated above. The most noticeable is the new if statement structure. If we assume that file_get_contents() will return FALSE if $small_tag is NULL then we can reuse the set/save/continue for both "bad" statements, all we have to do is apply the proper "note" depending upon the value of $small_tag. There shouldn't be any noticeable difference in processing time as it should immediately fail, but you may want to check that.

Something a little less noticeable is the removal of the error suppressor @. First of all, you should never use the error suppressor. You should always explicitly check for errors and handle them. However, it isn't even necessary here. file_get_contents() only throws errors if using the maxlength or offset arguments, neither of which you are. The only "bad" return you have to worry about is a FALSE one.

Another not so obvious change is the removal of the unset( $data ); line. Micromanaging memory like this is unnecessary. The benefit is so negligible as to be nearly useless and the added clutter detracts from even that small appeal. The only reason to unset a value is if you may end up using it later and don't want it to have a value first. If you are in a loop and the next iteration of that loop automatically overwrites the previous values, then unsetting is unnecessary. This includes unsetting $uri and $content.

The final thing I wanted to point out is that you are recreating a boolean by using numerical TRUE/FALSE values with your "done" attribute. The difference between using an actual boolean and a pseudo-boolean is small but the abstractness can cause confusion. You should just use a real boolean unless absolutely necessary.

The rest is more of the same. There's only one last thing I noticed, and it goes along with what I said about the arrow anti-pattern earlier: Watch for unnecessary indentation. If you can reverse an if statement to reduce the amount of indented code then do so. For example:

foreach( $content as $t ) {
    if( empty( $t[ 'id_str' ] ) ) {
        continue;
    }

    //etc...
}

Hope this helps


Shifting an 128-bit integer consisting of four 32-bit integers For a little project of mine I have written two versions for shifting a 128-bit unsigned integer consisting of four 32-bit unsigned integers in x86 assembly. I cannot really decide which is better in performance, style, etc..

UPDATE:
I have written another two different functions. The first one doesn't need any conditional jumps, so there is no problem with branch mispredictions. However, it needs 40 bytes (on a 32-bit platform) to store the jump-tables that I have created in static memory.
The second one uses conditional jumps, but in a better way than before, I think.
Both functions doesn't really care about shift-values >=128/=0.

UPDATE 2:
Because I was unsatisfied with the size of the jump-table (especially on a 64-bit platform) I rewrote the first function as a compromise between conditional jumps and jump-table size.

       .data
   JTABLE:
       .long L0,L1,L2,L3
       .text
       .global _shl_128
       .intel_syntax
   _shl_128:
       push ebx
       push esi
       mov edx, [esp+12]            //pointer to array of integers
       mov ecx, [esp+16]            //value of bits to shift
       mov esi, ecx
       shr esi, 5
       mov esi, [JTABLE+esi*4]
       mov eax, [edx]
       mov ebx, [edx+4]
       and ecx, 31
       jmp esi
   L0:
       mov esi, [edx+8]
       shld [edx+12], esi, cl
       shld esi, ebx, cl
       shld ebx, eax, cl
       shl eax, cl
       mov [edx], eax
       mov [edx+4], ebx
       mov [edx+8], esi
       jmp L4
   L1:
       mov esi, [edx+8]
       je L5
       shld esi, ebx, cl
       shld ebx, eax, cl
       shl eax
   L5:
       mov [edx+4], eax
       mov [edx+8], ebx
       mov [edx+12], esi
       jmp L7
   L2:
       je L6
       shld ebx, eax, cl
       shl eax
   L6:
       mov [edx+8], eax
       mov [edx+12], ebx
       jmp L8
   L3:
       shl eax, cl
       mov [edx+12], eax
       mov dword ptr [edx+8], 0
   L8:
       mov dword ptr [edx+4], 0
   L7:
       mov dword ptr [edx], 0
   L4:
       pop esi
       pop ebx
       ret

Function:

    .text
    .global _shl_128
    .intel_syntax
_shl_128:
    push ebx
    push esi
    mov edx, [esp+12]            //pointer to array of integers
    mov ecx, [esp+16]            //value of bits to shift
    mov esi, ecx
    and ecx, 31
    cmp esi, 96
    mov eax, [edx]
    jae L1
    cmp esi, 64
    mov ebx, [edx+4]
    jae L2
    cmp esi, 32
    mov esi, [edx+8]
    jae L3
    shld [edx+12], esi, cl
    shld esi, ebx, cl
    shld ebx, eax, cl
    shl eax, cl
    mov [edx], eax
    mov [edx+4], ebx
    mov [edx+8], esi
    jmp L4
L3:
    je L5
    shld esi, ebx, cl
    shld ebx, eax, cl
    shl eax
L5:
    mov [edx+4], eax
    mov [edx+8], ebx
    mov [edx+12], esi
    jmp L6
L2:
    je L7
    shld ebx, eax, cl
    shl eax
L7:
    mov [edx+8], eax
    mov [edx+12], ebx
    jmp L8
L1:
    je L9
    shl eax, cl
L9:
    mov [edx+8], eax
    mov dword ptr [edx+8], 0
L8:
    mov dword ptr [edx+4], 0
L6:
    mov dword ptr [edx], 0
L4:
    pop esi
    pop ebx
    ret

Well, I don't have a 32 bit build environment available just now.  While I wrote this for 64bit, I'm only processing 32bits at a time.  I haven't timed it, so I can't say if it is faster than yours, but it has no jumps.  At the very least, it might give you some ideas.

If the comments look like copy/paste from some C code, there's a reason for that...

; void Shl(int s, DWORD *m)
; Shift the 4 DWORDs at m left by s bits

Shl proc

; s - ecx
; m - rdx

; rax, rcx, rdx, r8-r11 are scratch under Windows 64bit calling convention

    mov r8d, [rdx + 12]     ; t1 = m3; MSB
    mov r9d, [rdx + 8]      ; t2 = m2;
    mov r10d, [rdx + 4]     ; t3 = m1;
    mov r11d, [rdx]         ; t4 = m0; LSB
    xor eax, eax            ; t0 = 0;

    cmp ecx, 32
    cmovge r8d, r9d       ; t1 = t2;
    cmovge r9d, r10d      ; t2 = t3;
    cmovge r10d, r11d     ; t3 = t4;
    cmovge r11d, eax      ; t4 = t0;

    cmp ecx, 64
    cmovge r8d, r9d       ; t1 = t2;
    cmovge r9d, r10d      ; t2 = t3;
    cmovge r10d, r11d     ; t3 = t4;

    cmp ecx, 96
    cmovge r8d, r9d       ; t1 = t2;
    cmovge r9d, r10d      ; t2 = t3;

    cmp ecx, 128
    cmovge r8d, r9d       ; t1 = t2;

    and ecx, 31

    shld r8d, r9d, cl       ; t1 = (t1 << s) | (t2 >> (32 - s));
    shld r9d, r10d, cl      ; t2 = (t2 << s) | (t3 >> (32 - s));
    shld r10d, r11d, cl     ; t3 = (t3 << s) | (t4 >> (32 - s));
    shl r11d, cl            ; t4 = t4 << s;

    mov [rdx + 12], r8d  ; m[3] = t1;
    mov [rdx + 8], r9d   ; m[2] = t2;
    mov [rdx + 4], r10d  ; m[1] = t3;
    mov [rdx], r11d      ; m[0] = t4;

    ret

Shl endp

Yes, I'm using 7 registers, but if you push everything and use EBP, I think you'll be able to squeeze this in.  Another reason I prefer 64bit.


Canvas color detection I am coding a game, and I need to detect the color of a rectangles on a canvas, by moving a character and  touching them, so that a message will be displayed "this is magenta" and so on. Please find below the game and my coding, so that you will better understand me:

jsFiddle

var canvas = document.getElementById("canvas");
var context = canvas.getContext("2d");

/*moving grey character*/
var xPos = 0;
var yPos = 0;

var bucketWidth = 100;
var bucketHeight = 10;

context.fillStyle = "grey";
context.fillRect(xPos, yPos, bucketWidth, bucketHeight);
context.strokeRect(xPos, yPos, bucketWidth, bucketHeight);

/*static red character*/
var xPos2 =  canvas.width / 2;
var yPos2 =  canvas.height / 2;

var bucketWidth2 = 100;
var bucketHeight2 = 10;

context.fillStyle = "red";
context.fillRect(xPos2, yPos2, bucketWidth2, bucketHeight2);

/*static magenta character*/

var bucketWidth3 = 100;
var bucketHeight3 = 10;

var xPos3 =  canvas.width / 2;
var yPos3 =  - bucketHeight3 - bucketHeight3 + canvas.height / 2;

context.fillStyle = "magenta";
context.fillRect(xPos3, yPos3, bucketWidth3, bucketHeight3);

/*Function to move the grey character from left to right and from top to button*/

function move(e){

if (e.keyCode === 37 && xPos > 0 && xPos <= canvas.width-bucketWidth){
    xPos -= bucketWidth;
} else if (e.keyCode === 39 && xPos >= 0 && xPos < canvas.width-bucketWidth){
    xPos += bucketWidth;
}  else if (e.keyCode === 40 && yPos >= 0 && yPos < canvas.height-bucketHeight){/*down arrow*/
    yPos += bucketHeight;
}  else if (e.keyCode === 38 && yPos > 0 && yPos <= canvas.height-bucketHeight){ /*up arrow*/
    yPos -= bucketHeight;
}

canvas.width= canvas.width;

context.fillStyle = "grey";
context.fillRect(xPos, yPos, bucketWidth, bucketHeight);
context.strokeRect(xPos, yPos, bucketWidth, bucketHeight);

context.fillStyle = "red";
context.fillRect(xPos2, yPos2, bucketWidth2, bucketHeight2);

context.fillStyle = "magenta";
context.fillRect(xPos3, yPos3, bucketWidth3, bucketHeight3);

}

document.onkeydown = move;

By the way, I was told to use the function getImageData(), but I could not apply the function by moving the character. Somebody send me an example, but it is working by using the mouse and jQuery was used, but I am not allowed to use jQuery. Please see this link.

Reading the color on the canvas is not the right way to proceed, for several reasons :

You want the color string (expl : 'red'), not the r,g,b values provided by getImageData
(expl : 255, 0, 0) .
You can't draw the buckets with gradient or color animation with such a method.

Anyway you are the one handling the buckets, so what about keeping them all within a nice Object ?
And what about going Object for the buckets also ? You'll have great flexibility to change anything to your buckets later.

Below I define a BucketGrid class that will handle the buckets, and a Bucket class that holds data relative to a bucket.

http://jsfiddle.net/gamealchemist/ydtkbzoc/6/

BucketGrid Class :

function BucketGrid(columnCount, rowCount, bucketWidth, bucketHeight) {
    var buckets = [];
    this.columnCount = columnCount;
    this.rowCount = rowCount;
    this.bucketWidth = bucketWidth;
    this.bucketHeight = bucketHeight;

    this.getBucket = function (column, row) {
        return buckets[column + row * columnCount];
    }

    this.setBucket = function (column, row, bucket) {
        bucket.column = column;
        bucket.row = row;
        buckets[column + row * columnCount] = bucket;
    }

    this.insertNewBucket = function (column, row, color) {
        this.setBucket(column, row, new Bucket(this, color));
    }

    this.isValidPosition = function (column, row) {
        return ((column >= 0) && (column < this.columnCount) && (row >= 0) && (row < this.rowCount));
    }

    this.draw = function (context) {
        for (var i = 0; i < buckets.length; i++) {
            var thisBucket = buckets[i];
            if (thisBucket) thisBucket.draw(context);
        }
    }

}

Bucket Class :

function Bucket(owner, color) {
    this.owner = owner;
    this.column = 0;
    this.row = 0;
    this.color = color;

    this.draw = function (context) {
        var owner = this.owner,  bw = owner.bucketWidth,  bh = owner.bucketHeight;
        context.fillStyle = this.color;
        context.strokeStyle = '#000';
        context.save();
        context.translate(this.column * bw, this.row * bh);
        context.fillRect(0, 0, bw, bh);
        context.strokeRect(0, 0, bw, bh);
        context.restore();
    }
}

Setup :

var buckets = new BucketGrid(4, 38, 100, 10);

buckets.insertNewBucket(2, 20, 'red');
buckets.insertNewBucket(2, 21, 'magenta');

/*moving grey character*/
var hero = new Bucket(buckets, 'grey');

Handlers :

/*Function to move the grey character from left to right and from top to button*/
function move(e) {
    var keyCode = e.keyCode;
    switch (keyCode) {
        case 37:
            if (buckets.isValidPosition(hero.column - 1, hero.row)) hero.column--;
            break;
        case 39:
            if (buckets.isValidPosition(hero.column + 1, hero.row)) hero.column++;
            break;
        case 40:
            /*down arrow*/
            if (buckets.isValidPosition(hero.column, hero.row + 1)) hero.row++;
            break;
        case 38:
            /*up arrow*/
            if (buckets.isValidPosition(hero.column, hero.row - 1)) hero.row--;
            break;
    }
    var hovered = buckets.getBucket(hero.column, hero.row);
    colorDiv.innerHTML = hovered ? hovered.color : 'none';

    drawScene();
}

function drawScene() {
    context.clearRect(0, 0, context.canvas.width, context.canvas.height);
    buckets.draw(context);
    hero.draw(context);
}

launch the game :

var canvas = document.getElementById("canvas");
var colorDiv = document.getElementById('whichColor');
var context = canvas.getContext("2d");

document.onkeydown = move;
drawScene();


UserInfo information class with a database I am no expert in C#, the opposite really, I just wanted an overall review on my UserInfo class that gets user information from a database using MySQL. I was also wondering if it is a good idea to really process the data in the way I did, getting everything from the database, even the things that aren't needed. But I think this way is a lot more tidy and neat. I was just wondering what you would say about it.

using MySql.Data.MySqlClient;
using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Scratch.Application.Habbo.Users
{
    class UserInfo
    {
        private Dictionary<string, string> _userInfo;

        public UserInfo(int Id)
        {
            this._userInfo = new Dictionary<string, string>();

            using (var DbCon = Scratch.GetEnvironment().GetDatabase().GenerateNewConnection())
            {
                DbCon.AssignQuery("SELECT * FROM `users` WHERE `id` = @id");
                DbCon.RegisterParameter("id", Id);
                DbCon.OpenConnection();

                using (MySqlDataReader Reader = DbCon.ExecuteReader())
                {
                    if (Reader.HasRows)
                    {
                        while (Reader.Read())
                        {
                            for (int index = 0; index < Reader.FieldCount; index++)
                            {
                                //bypass duplicate error, just replaces if duplicate
                                this._userInfo[Reader.GetName(index)] = Reader.GetString(index);
                            }
                        }
                    }
                }
            }
        }

        private void UpdateData(string Key, string NewValue)
        {
            if (this._userInfo.ContainsKey(Key))
                this._userInfo.Add(Key, "");

            this._userInfo[Key] = NewValue;
        }

        public string GetString(string Key)
        {
            return this._userInfo[Key];
        }

        public int GetInt(string Key)
        {
            return int.Parse(this._userInfo[Key]);
        }
    }
}

Why do you do this:

if (this._userInfo.ContainsKey(Key))
    this._userInfo.Add(Key, "");

this._userInfo[Key] = NewValue;

Those first two lines are completely superfluous; moreover if you need to get a value from a Dictionary<T, T> and aren't sure the key is present, you need to use Dictionary<TKey, TValue>.TryGetValue.

Local variables like Reader need to be camelCase. Ditto parameters like Key and NewValue.

Why aren't you using an ORM like Entity Framework instead of this ugly, needlessly generic code? I mean: public string GetString(string Key), public int GetInt(string Key) -- really?


Creating membership list with Parallel.ForEach I had a foreach loop which used to loop Membership users and create list of a custom class. Following was the for loop:

foreach (MembershipUser usr in userlist)
{
    userandGroup usrgp = new userandGroup();
    usrgp.id = ((Guid)usr.ProviderUserKey).ToString() ;
    usrgp.Name = usr.UserName;
    ProfileBase profile = ProfileBase.Create(usr.UserName);
    profile.Initialize(usr.UserName, true);
    // Following line takes approx 40ms to execute once.
    usrgp.DisplayName = profile.GetPropertyValue("FirstName").ToString() + " " + profile.GetPropertyValue("LastName").ToString();
    usrgp.type = "user";
    lst.Add(usrgp);
}

As the commented line used to take approx 40ms to execute, the whole loop for 40 users used to take approx 1600ms to execute.

I got suggestion to use Parallel.ForEach loop to reduce the execution time of the loop from this question

SO I updated the loop as:

    Object obj = new Object();
    Parallel.ForEach(userlist, (usr) =>
    {
        userandGroup usrgp = new userandGroup();
        usrgp.id = ((Guid)usr.ProviderUserKey).ToString();
        usrgp.Name = usr.UserName;
        ProfileBase profile = ProfileBase.Create(usr.UserName);
        profile.Initialize(usr.UserName, true);
        usrgp.type = "user";
        usrgp.DisplayName = profile.GetPropertyValue("FirstName").ToString() + " " + profile.GetPropertyValue("LastName").ToString();
        lock (obj)
        {
            lst.Add(usrgp);
        }
    });

The loop works. Now the performance is way better (Although not best). It now takes only 700ms to execute the loop.

But I am not sure if it is the correct way to write Parallel.ForEach loop as I am new to it. Kindly suggest if the code is right?

Before going parallel I'd first try to understand which part of that code is slow. Maybe there is something you can do which will boost performance even more. That said I have few notes:

userandGroup does not follow naming conventions, types should be PascalCase.
There is no benefit to abbreviate names to usrgp and obj. is usrgrp a...group? Call it group (or userGroup if owner isn't clear from context). obj says what it is (an object), not what it does (the object which is used as synchronisation context). You may rename it (at least) to syncRoot (name used also in the framework itself for this purpose).
If lst is local then you do not need obj and you can directly lock on it.
You do not need to cast ProviderUserKey to Guid to call ToString(): usr.ProviderUserKey.ToString() or (if ProviderUserKey is object and it might be null) maybe Convert.ToString(usr.ProviderUserKey).
Object initialisation may be simplified:

Using Object Initialization syntax:

var group =  new userandGroup
{
    id = usr.ProviderUserKey.ToString(),
    Name = usr.UserName
};

Follow naming convention also here, id should be Id. Also move Profile creation before group and you can initialise everything in this way.

DisplayName initialisation may use string interpolation:

As before:

DisplayName = $"{profile.GetPropertyValue("FirstName")} {profile.GetPropertyValue("LastName")}"

If FirstName or LastName may be empty then you're adding a spurious space. Handle the case (or at least add a call to Trim().)
true boolean parameters are a pain, I see the call and I need to go to check the prototype to know what it means. More than that: if I need another option then I need to add more and more parameters.

Use named parameters if you can't change the prototype (name is fictional in this case):

profile.Initialize(usr.UserName, loadUserProfile: true);

Or, better, use an enum with the [Flags] attribute to make clear - at calling point - what parameter means.

You initialised Profile with the user name as parameter in its ctor, do you need it again here?
If lst is local then you can simplify your code.

Move the logic to create group into a separate function (basically all the code you have inside the parallel loop) then:

var groups = userlist.AsParallel()
    .Select(CreateGroup)
    .ToList();

Where CreateGroup is something like this (note that I moved the hard-coded user string to a constant defined elsewhere and I moved the possibly complex logic for DisplayName to a separate function):

UserAndGroup CreateGroup(MembershipUser user)
{
    var profile = new Profile(user.UserName);
    profile.Initialize(user.UserName, true);

    return new UserAndGroup
    {
        Id = user.ProviderUserKey.ToString(),
        Name = user.UserName,
        Type = UserGroupType,
        DisplayName = ResolveDisplayName()
    };

    string ResolveDisplayName()
        => $"{profile.GetPropertyValue("FirstName")} {profile.GetPropertyValue("LastName")}";
}


Selecting one MySQL date with month reduced by one I was thinking in a way to improve the following query if possible:

SELECT CONCAT(DATE_FORMAT(start_date, '%Y'), ',',
       (MONTH(start_date) -1), ',', DATE_FORMAT(start_date, '%d')) as start_date
FROM PROJECTS

I need start_date in this format: dd/mm-1/yyyy.

Note: I am using an API that use month from 0 to 11, and MySQL uses 1 to 12 – that's why I need month - 1.

If there is no workaround then, bad practice as it may be, what you have is functional. I found this post on SO that may allow you to convert the column's date data type to the 0-11 format. Hope this helps.

EDIT

Removed bad idea. If you really wish to see it just look at the edit history, but it is not a good idea. I'm going back to my original answer.


Preliminary draft of a data structure for a goodies tracker My girlfriend and I are making a goodies tracker to enable us to keep better track of our stamp/gift cards. Right now we've got a super simple Object with some fields of what a stamp card has, and no support for modifying its fields (yet). I want to get some tips on if this is bad or good code, if there are any glaring bugs, and if possible if you'd know any good ways to look at how to integrate similar Objects like gift card and stamp card (inheritance, or composition?)

There are some dependencies which you can choose if you want to download. I'll include the output of the code here:

# -*- coding: utf-8 -*-
from tabulate import tabulate
# TODO: Put the dependencies into a venv so that it's easy to install
# TODO: add the amount of stamps we currently have vs. the stamp_capacity

class StampCard:
    # A stamp card for an establishment which contains a name and a capacity
    # Authors: Cody Antonio Gagnon & Chuan-Li Ojales Chang

    def __init__(self, card_name, card_description, stamp_capacity,
                 stamp_symbol, redeemable_location):
        """
        Initialize the stamp card with a:
            name
            description
            capacity
            symbol for stamping
            location to be redeemed
        """
        self.name = card_name
        self.description = card_description
        self.stamp_capacity = stamp_capacity
        self.stamp_symbol = stamp_symbol
        self.location = redeemable_location

    def get_name(self):
        """
        Return the name of the stampcard
        """
        return self.name

    def get_description(self):
        """
        Return the description of the stampcard
        """
        return self.description

    def get_capacity(self):
        """
        Return the capacity as a string.
        """
        return str(self.stamp_capacity)

    def get_symbol(self):
        """
        Return the capacity as a string.
        """
        return self.stamp_symbol

    def get_location(self):
        """
        Return the redeemable location as a string.
        """
        return self.location

    def print_stamps(self):
        """
        Print the stamps using the typical two-row approach.
        """
        stamps = []
        stamps_per_row = self.stamp_capacity / 2  # hardcoded rows for now...
        for x in range(1, 3):
            stamps.append([self.stamp_symbol] * stamps_per_row)
        print (tabulate(stamps, tablefmt="grid").encode('utf8', 'replace'))  # takes a list of lists or another tabular data type as the first argument, and outputs a nicely formatted table

def main():
    stamp = unicode("", 'utf8')
    oasis = StampCard("Oasis", "Bubble Tea", 10, stamp, "University District")
    print("Name: " + oasis.get_name() + ", Capacity: " + oasis.get_capacity())
    oasis.print_stamps()

main()

It's weird that your get_capacity() returns a string.  I'm guessing that it's to facilitate string concatenation in the print() statement in main(), but that is not a good justification for the weirdness.  A better solution would be to use str.format() rather than string concatenation.

Those get_…() methods look like an awkward translation of Java code.  In Python, it is often customary to just access the member variables directly.  If you did want to write a getter method (to help enforce read-only access, for example), you could write a @property instead.

Going further, it seems like your StampCard is mostly a structure to hold a bunch of read-only properties.  You could just use or inherit from collections.namedtuple.

# -*- coding: utf-8 -*-
from collections import namedtuple
from tabulate import tabulate

class StampCard(namedtuple('StampCard', 'name description capacity symbol redeemable_location')):
    def __str__(self):
        stamps_per_row = self.capacity // 2  # hardcoded rows for now
        stamps = [[self.symbol] * stamps_per_row] * 2
        return tabulate(stamps, tablefmt='grid')

def main():
    oasis = StampCard('Oasis', 'Bubble Tea', 10, u'', 'University District')
    print('Name: {card.name}, Capacity: {card.capacity}'.format(card=oasis))
    print(unicode(oasis))

if __name__ == '__main__':
    main()


Finding the string from the list of array objects TA.ServiceProxy.RoleManagement.GetSubPrivilagesResponses subPrivilagesResponses = GetSubPrivilagesFromPrivilageCodeStatic("Authoring01");
if (subPrivilagesResponses != null && subPrivilagesResponses.SubprivilagesList != null)
{
    for (int j = 0; j < subPrivilagesResponses.SubprivilagesList.Length; j++)
    {
       if (subPrivilagesResponses.SubprivilagesList[j].PrivilageCode == "CREATOR")
       {
           objTemplate.IsTagged = true;
           break;
       }
    }
}

The above code is written as

TA.ServiceProxy.RoleManagement.GetSubPrivilagesResponses subPrivilagesResponses = GetSubPrivilagesFromPrivilageCodeStatic("Authoring01");
if (subPrivilagesResponses != null && subPrivilagesResponses.SubprivilagesList != null)
{
   var item = subPrivilagesResponses.SubprivilagesList.SingleOrDefault(cd => cd.PrivilageCode == "CREATOR");
    if(item != null && item.PrivilageCode == "CREATOR")
        objTemplate.IsTagged = true;
}

In what are the different way it can be modified in order to boost performance and efficiency by making use of Lambda or LINQ

Because the question is about performance and efficiency that's why some sort of benchmarking would be essential to compare different implementations. I have found BenckmarkDotNet really useful for these kind of experiments.

You can define the for loop version as your baseline and the tool will compare the other implementation against that. Here is my simplified example how to setup your environment for measurement:

[HtmlExporter]
[MemoryDiagnoser]
[SimpleJob(BenchmarkDotNet.Engines.RunStrategy.ColdStart, targetCount: 100)]
public class IterationOptimalizationExperiment
{
    private static List<Data> target;
    private const string TheOne = "TheOne";

    [GlobalSetup]
    public void Setup()
    {
        target = Enumerable.Range(0, 10000)
            .Select(i => new Data { Id = i, Type = i % 7777 == 0 ? TheOne : "NotTheOne" })
            .ToList();
    }

    [Benchmark(Baseline = true)]
    public void WithFor()
    {
        bool hasFound = false;
        for (int i = 0; i < target.Count; i++)
            if (string.Equals(target[i].Type, TheOne, StringComparison.OrdinalIgnoreCase))
            {
                hasFound = true;
                break;
            }

    }

    [Benchmark]
    public void WithForEach()
    {
        bool hasFound = false;
        foreach (var t in target)
            if (string.Equals(t.Type, TheOne, StringComparison.OrdinalIgnoreCase))
            {
                hasFound = true;
                break;
            }
    }

    [Benchmark]
    public void WithAny()
    {
        bool hasFound = target.Any(t => string.Equals(t.Type, TheOne, StringComparison.OrdinalIgnoreCase));
    }
}

And you should simply call the following command to start your experiment: BenchmarkRunner.Run<IterationOptimalizationExperiment>();

When you run it in Release mode it will dump something like this:


Inner select performance I have the following code, but I don't know how to make it run faster.

SELECT lot.order as lot_order,
    lot.title as lot_title,
    lot.id as lot_id,
    lot.id_auction,
    IF (
        (SELECT bid
            FROM bid
            WHERE lot.id = bid.id_lot
            ORDER BY bid.date DESC
            LIMIT 0,1) > lot.bid_minimum,
        (SELECT bid
            FROM bid
            WHERE lot.id = bid.id_lot
            ORDER BY bid.date DESC
            LIMIT 0,1),
        lot.bid_minimum
    ) AS bid,
    youtube,
    (SELECT path_image
        FROM lot_album
        WHERE lot_album.id_lot = lot.id
        AND order = 1) as image_featured
FROM lot
WHERE lot.excluded <> 1
AND lot.id_auction = 42
ORDER BY lot.order ASC

As you can see, I have the IF to check if the value of the select is > bid.minimum. The only way to improve this is check it on the programming language (PHP/.NET...) or there is another way?

"lot_album" is a table where you can have lots of images, but also
none. But only one image per lot will have order 1 so that image will
be the "image_featured".

This should be a little better. I believe joins are faster than nested selects, and that if with nested select may or may not select twice or use cached response.  I can't think of a good way of getting rid of the one nested select.  Otherwise, maybe you could write it as a stored procedure or virtual table.  (Note* any spelling mistakes of table or column names are a result of autocorrect)

SELECT lote.ordem as lote_ordem,
lote.titulo as lote_titulo,
lote.codigo as lote_codigo,
lote.codigo_leilao,
GREATEST(
    (SELECT lance
        FROM lance
        WHERE lote.codigo = lance.codigo_lote
        ORDER BY lance.data DESC
        LIMIT 0,1),
    lote.lance_minimo
) AS lance2,
youtube,
youtube,
-- lance.codigo_cadastro,
lote_album.caminho_imagem as imagem_destaque,
FROM lote
LEFT JOIN lote_album
ON lote_album.codigo_lote = lote.codigo
WHERE lote.excluido <> 1
AND lote.codigo_leilao = 42
AND lote_album.ordem = 1
ORDER BY lote.ordem ASC


Check if two nodes are cousins in a Binary Tree Given the binary Tree and the two nodes say ‘a’ and ‘b’, determine whether the two nodes are cousins of each other or not.

Two nodes are cousins of each other if they are at same level and have different parents.

Looking for code review, optimizations and best practices.

public final class CheckCousins {

    private TreeNode root;

    public CheckCousins(List<Integer> items) {
        create(items);
    }

    private void create (List<Integer> items) {
         if (items.size() == 0) {
            throw new IllegalArgumentException("There should atlease be single item in the tree");
        }
        root = new TreeNode(items.get(0));

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode(items.get(left));
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(items.get(right));
                    queue.add(current.right);
                }
            }
        }
    }

    private static class TreeNode {
        private TreeNode left;
        private int item;
        private TreeNode right;

        TreeNode(int item) {
            this.item = item;
        }
    }

    public boolean isCousin(int x, int y) {
        List<NodeData> nodeDatas = new ArrayList<>();
        isCousins(null, root, x, y, 0, nodeDatas);
        if (nodeDatas.size() < 2) return false;
        NodeData nd1 = nodeDatas.get(0);
        NodeData nd2 = nodeDatas.get(1);

        return nd1.depth == nd2.depth && nd1.parent != nd2.parent;
    }

    private static class NodeData {
        private TreeNode parent;
        private int depth;

        public NodeData(TreeNode parent, int depth) {
            this.parent = parent;
            this.depth = depth;
        }
    }

    private void isCousins(TreeNode parent, TreeNode node, int x, int y, int level, List<NodeData> nodeDatas) {
        if (nodeDatas.size() == 2) return;

        if (node == null) {
            return;
        }
        if (node.item == x || node.item == y) {
            nodeDatas.add(new NodeData(parent, level));
            return;
        }

        isCousins(node, node.left,  x, y, level + 1, nodeDatas);
        isCousins(node, node.right, x, y, level + 1, nodeDatas);
    }
}

public class CheckCousinsTest {

    @Test
    public void testIncompleteTree() {
        CheckCousins cs1 = new CheckCousins(Arrays.asList(1, 2, 3, 4, null, null, 7));
        assertTrue(cs1.isCousin(4, 7));
    }

    @Test
    public void testCompleteBinaryTreeSuccess() {
        CheckCousins cs2 = new CheckCousins(Arrays.asList(1, 2, 3, 4, 5, 6, 7));
        assertTrue(cs2.isCousin(4, 7));
        assertTrue(cs2.isCousin(4, 6));
        assertTrue(cs2.isCousin(5, 6));
        assertTrue(cs2.isCousin(5, 7));
    }

    @Test
    public void testCompleteBinaryTreeFailure() {
        CheckCousins cs2 = new CheckCousins(Arrays.asList(1, 2, 3, 4, 5, 6, 7));
        assertFalse(cs2.isCousin(4, 4));
        assertFalse(cs2.isCousin(4, 5));
        assertFalse(cs2.isCousin(6, 7));
        assertFalse(cs2.isCousin(2, 3));
        assertFalse(cs2.isCousin(2, 1));
    }

}

Two nodes are cousins of each other if they are at same level and have different parents.

Are we cousins? Normally, I'd say "no", but according to your definition, it's quite possible. Never mind, nice to meet you.

What I dislike most is that you're using both a TreeNode and an index-based tree embedded in an array. I guess, the array serves the initialization only, but I'm not very sure. Unless the initialization is part of task, move it to a separate class.

Your TreeNode-based structure is pretty inefficient for this kind of test. You could do much better with a

class MyTreeNode {
    private int item;
    private MyTreeNode parent;
    private int depth;
}

where the depth only allows you to say "NO" fast.

The whole program would be like

areCousins(MyTreeNode a, MyTreeNode b) {
    return a.depth == b.depth && a.parent != b.parent;
}

just like you wrote above. Without the parent pointers, things get much more complicated. It looks like you did it right, although I struggled with your void isCousins for a while. The is prefix suggest a boolean and you're using a List where a two-member container would fit better.

Because of this, your program fails for trees containing a given node twice like

CheckCousins cs1 = new CheckCousins(Arrays.asList(1, 2, 3, 4, 5, 4, 5));
assertTrue(cs1.isCousin(4, -1));

Not tested, but I guess it fills nodeDatas with the two 4s and you're out of luck.


Hackerrank "Queues: A Tale of Two Stacks" Javascript Solution Here is the original problem, and below is my solution (passing). It's asking to implement a queue with 2 stacks, which means I cannot simply use Array.shift. I wonder if the solution can be improved. Is there a better way than this to implement a queue with 2 stacks?

function processData(input) {
    var inputs = input.split('\n')
    var cmd;
    var inQueue = [],
        outQueue = []
    var len;

    function inQueueToOutQueue() {
        if (outQueue.length === 0) {
            len = inQueue.length;
            while (len-- > 0) {
                outQueue.push(inQueue.pop())
            }
        }
    }

    for (var i = 1, max = inputs[0]; i <= max; i++) {
        cmd = inputs[i].split(' ');
        switch (cmd[0]) {
            case '1':
                inQueue.push(cmd[1]);
                break;
            case '2':
                inQueueToOutQueue();
                outQueue.pop()
                break;
            case '3':
                inQueueToOutQueue();
                console.log(outQueue[outQueue.length - 1])
                break;
            default:
        }
    }

}

You've pretty much nailed it.
You don't really need the len variable here though,
you could use a condition on inQueue.length directly.

len = inQueue.length;
while (len-- > 0) {
    outQueue.push(inQueue.pop())
}

The other thing I notice is the inconsistent use of semicolons.
Sometimes you add at the end of statements,
sometimes not.
You could choose whichever way, but do it consistently.

Lastly, the empty default statement of the switch is unnecessary, you can drop it.


Implementation of a Singularly LinkedList I'm new to C++ and trying to learn by implementing data structures in it.

Is the code below the most efficient way of implementing the data structure in C++? Does it conform to C++ standards, and how readable is it?

#include <iostream>

class Node
{
public:
    int data;
    Node *next;
    Node(int data)
    {
        this->data = data;
        this->next = nullptr;
    }
};

class LinkedList
{
public:
    Node *head;
    Node *tail;
    int length;
    LinkedList()
    {
        this->head = nullptr;
        this->tail = nullptr;
        this->length = 0;
    }

    void add(int data);
    void add(int data, int position);
    void delete_node(int position);
    int get_length();
    void print();
    int get_first();
};

void LinkedList::add(int data)
{
    Node *node = new Node(data);

    if (this->head == nullptr)
    {
        this->head = node;
    }
    else
    {
        this->tail->next = node;
    }
    this->tail = node;
    this->length = this->length + 1;
}

void LinkedList::add(int data, int position)
{
    Node *temp_head = this->head;
    Node *node = new Node(data);
    int i = 0;
    while (i < this->length)
    {
        if (i == position - 1)
        {
            node->next = this->head->next;
            this->head->next = node;
        }
        this->head = this->head->next;
        i++;
    }
    this->head = temp_head;
    this->length = this->length + 1;
}

void LinkedList::print()
{
    Node *temp_head = this->head;
    while (this->head != nullptr)
    {
        std::cout << this->head->data << '\n';
        this->head = this->head->next;
    }
    this->head = temp_head;
}

int LinkedList::get_length()
{
    return this->length;
}

int LinkedList::get_first()
{
    return this->head->data;
}

int main()
{
    LinkedList *llist = new LinkedList();
    llist->add(0);
    llist->add(1);
    llist->add(3);
    llist->add(4);
    llist->add(2, 2);
    llist->print();
    std::cout << "Length: " << llist->get_length() << '\n';
    std::cout << "First Element : " << llist->get_first() << '\n';
}

C++ is not Java. You may safely get rid of all this->.
The client doesn't need to know about Node. Make it an inner class of the LinkedList.
The ~LinkedList() destructor is missing. You should also consider (or prohibit) the copy constructor, operator=, and move constructor.
add(int data, int position) may silently leak memory. If the position is too large, the created node is not linked into the list, and is lost. Also, there is no way to prepend a node.
I don't like the temp_head approach. Better leave the head alone, and iterate using a cursor variable, e.g.:

void LinkedList::print() {
    Node * cursor = head;
    while (cursor) {
        std::cout << cursor->data << '\n';
        cursor = cursor->next;
    }
}

At least, there is no need to restore head.

Ditto for add.


C++ minimal threadsafe array based on std::deque Here is a minimal example of a threadsafe array I want to build on for a timeseries application, with the following characteristics:

Ever-growing, and the already contained elements remain constant
(Usually) a single writer calling push_back
Multiple dependent readers

Here is the corresponding implementation, or rather an early attempt of it:

template<typename T>
struct threadsafe_array
{
    auto operator[](int i) const
    {
        return deq[i];
    }

    auto size() const
    {
        return deq_size.load(std::memory_order_acquire);
    }

    void push_back(T const& t)
    {
        std::unique_lock<std::mutex> lock(mut);
        deq.push_back(t);
        lock.unlock();
        deq_size.fetch_add(1, std::memory_order_release);
    }

private:
    std::deque<T> deq;
    std::atomic<int> deq_size{0};
    std::mutex mut;
};

My underlying ideas:

reads of the available elements through operator[](int) are carried out lock-free.
a std::deque is used as the underlying container because it does not invalidate concurrent reads when doing a push-back (--in contrast, a std::vector could as it potentially does a reallocation)
the push_back is forwarded to the underlying deque, on which it is applied in an atomic way through locking the std::mutex. Thereafter, the variable deq_size of type std::atomic<int> variable is adjusted using release semantics (so that the previous push_back is not reordered after the fetch_add).
if there are reads occurring in between adding an element to the deque and the adjustment of the size, they have have to get along with a smaller size(), i.e. as if the array had not been updated. Calling operator[size()] therefore does not need to be undefined behvaiour as it is for std::deque (but that's more an inconsistency than a feature).

Questions:

Is this thing already threadsafe and doing what I wrote, or am I missing some points?
Are the memory orders in the atomic operations ok, or are there better choices (e.g. memory_order_relaxed for the load in size())?
Is it preferable doing the update of the size in push_back() under the lock (and thus, if I see it right, limit the size difference between size() and the underlying std::deque::size() to only one)?

Your code isn't thread-safe, as executing a read with .operator[]() and a write with .push_back() isn't synchronized in any way.
Even though no references to elements are invalidated by std::deque<T>::push_back(), it can change the data-structure to retrieve those references.

What you want to look into is called a readers-writer-lock, which is the primitive for allowing either multiple readers or only a single writer access at the same time.
C++17 provides it as std::shared_mutex, so you might have to use boost::shared_mutex unless your library is already there.


Calculating some worst-case values for angles in an interval Sadly enough I have a bit of code that is really slow and being a newbie in programmation, I don't have the faintest clue on how to optimize. Is there any way to make those calculations faster?

This code calculates the worst case of some parameters cycling through AktBeam and FileCounter (those cycles contain a LOT of other stuff, and I'm sorry to say that I cannot include the whole code) and inside a circumference (have a look at the select case block), and stores them in the BT and EC arrays.

AmplCo, AmplCr, PhaseCo and PhaseCr are arrays containing double variables.

I'm looking for all the possibilities here to make my processor perform the least amount of operations (e.g. if x^2 is worse than x * x, I'd use x * x).

Please notice that I cannot split the arrays in any way.

For PhiDrehung = 0 To 2*Sector-1 Step 3
    A = 0
    B = 0
    C = 0
    D = 0
    EC_Worst = 0
    BT_Worst = 0
    For k = 0 To FensterTheta
        For i = PhiDrehung-FensterPhi/2 To PhiDrehung+FensterPhi/2
            Select Case i
                Case Is < 0
                    l = 360+i
                Case Is > 359
                    l = i-360
                Case Else
                    l = i
            End Select
            A = A + AmplCo(FileCounter-1, AktBeam,l,k) * AmplCo(FileCounter, AktBeam,l,k) * Cos(PhaseCo(FileCounter-1, AktBeam,l,k) - PhaseCo(FileCounter, AktBeam,l,k)) + AmplCr(FileCounter-1, AktBeam,l,k) * AmplCr(FileCounter, AktBeam,l,k) * Cos(PhaseCr(FileCounter-1, AktBeam,l,k) - PhaseCr(FileCounter, AktBeam,l,k))
            B = B + AmplCo(FileCounter-1, AktBeam,l,k) * AmplCo(FileCounter, AktBeam,l,k) * Sin(PhaseCo(FileCounter-1, AktBeam,l,k) - PhaseCo(FileCounter, AktBeam,l,k)) + AmplCr(FileCounter-1, AktBeam,l,k) * AmplCr(FileCounter, AktBeam,l,k) * Sin(PhaseCr(FileCounter-1, AktBeam,l,k) - PhaseCr(FileCounter, AktBeam,l,k))
            C = C + (AmplCo(FileCounter-1, AktBeam,l,k)^2 + AmplCr(FileCounter-1, AktBeam,l,k)^2)
            D = D + (AmplCo(FileCounter, AktBeam,l,k)^2 + AmplCr(FileCounter, AktBeam,l,k)^2)
            BT_Worst = BT_Worst + (AmplCo(FileCounter-1, AktBeam,l,k)^2 + AmplCr(FileCounter-1, AktBeam,l,k)^2) / (AmplCo(FileCounter, AktBeam,l,k)^2 + AmplCr(FileCounter, AktBeam,l,k)^2)
        Next
    Next
    EC_Worst = ((A^2 + B^2) / (C*D))^0.5 * 100
    If EC_Worst > EC((FileCounter-1) / 2, AktBeam) Then EC((FileCounter-1) / 2, AktBeam) = EC_Worst

    BT_Worst = 20 * Log( BT_Worst / (((FensterTheta+1)*(FensterPhi+1)) * ((PRad(FileCounter-1, AktBeam) + PRad(FileCounter, AktBeam))/2)) ) / Log(10)
    If BT_Worst > BT((FileCounter-1) / 2, AktBeam) Then BT((FileCounter-1) / 2, AktBeam) = BT_Worst
Next

FensterTheta and FensterPhi are user-inserted values, let Sector = 180.

Your code already seems to do only the steps necessary, slow performance seems to be the limitation of VBA.

You may do some minor improvements however:

Select Case i

You don't need to check conditions here, it's a simple Mod operation:
l = Application.Evaluate("=MOD(" & i & ",360)")

Formulas

Your calculations contains a lot of duplicates, you can use temporary variables to reduce number of calculations:

t1 = AmplCo(FileCounter-1, AktBeam,l,k) * AmplCo(FileCounter, AktBeam,l,k)
t2 = PhaseCo(FileCounter-1, AktBeam,l,k) - PhaseCo(FileCounter, AktBeam,l,k)
t3 = AmplCr(FileCounter-1, AktBeam,l,k) * AmplCr(FileCounter, AktBeam,l,k)
t4 = PhaseCr(FileCounter-1, AktBeam,l,k) - PhaseCr(FileCounter, AktBeam,l,k)

A = A + t1 * Cos(t2) + t3 * Cos(t4)
B = B + t1 * Sin(t2) + t3 * Sin(t4)

t5 = (AmplCo(FileCounter-1, AktBeam,l,k)^2 + AmplCr(FileCounter-1, AktBeam,l,k)^2)
t6 = (AmplCo(FileCounter, AktBeam,l,k)^2 + AmplCr(FileCounter, AktBeam,l,k)^2)

C = C + t5
D = D + t6

BT_Worst = BT_Worst + t5 / t6

This halves the number of array accesses in your code (so best case you can get double speed).

As I've written already, there are limitations in VBA too. Further improvement could be done implementing your code in e.g. Visual Studio and compile as an Excel add-in. (That case you could avoid run time compilation and debugging which are the biggest issue in VBA).


jQuery script to clear input fields I've been trying to write a jQuery script where when someone clicks on the input the space clears and then if they don't type anything in, it reappears.

This is what I came up with. Is there a better way to do this? If so why is it better?

$(document).ready(function() {
            // set all input.text default value according to alt attribute
            $("input.contactStyle").each(function(){
                $(this).val($(this).attr("alt"));
            });

            // clear input.text on focus, if still in default
            $("input.contactStyle").focus(function() {

                if($(this).val()==$(this).attr("alt")) {
                    $(this).val("");
                }
            });

            // if field is empty afterward, add text again
            $("input.contactStyle").blur(function() {
                if($(this).val()=="") {
                    $(this).val($(this).attr("alt"));
                }
            });

            // set all input.text default value according to alt attribute
            $("textarea.contactStyle").each(function(){
                $(this).val($(this).attr("title"));
            });

            // clear input.text on focus, if still in default
            $("textarea.contactStyle").focus(function() {

                if($(this).val()==$(this).attr("title")) {
                    $(this).val("");
                }
            });

            // if field is empty afterward, add text again
            $("textarea.contactStyle").blur(function() {
                if($(this).val()=="") {
                    $(this).val($(this).attr("title"));
                }
            });
});

It seems like you're trying to recreate the functionality of the placeholder attribute that exists in HTML5. For browsers that support it--and that's all modern browsers with the exception of IE9 (I think)--your above code can be replaced with e.g.:

<input type="text" class="contactStyle" placeholder="Your placeholder text">

If you want to also use this functionality in less-modern browsers it'll take some JavaScript, but best not to reinvent the wheel. Several polyfills have been written by other people that let you use the placeholder attribute regardless of browser. There are several listed on this page (do a Ctrl+F for "placeholder").


Performing unit conversions using intermediate conversions Challenge:

  Two input files are given The first contains |N| ratios in the form of
  two labels and a ratio:

  USD, GBP, 0.69
  Meter, Yard, 1.09
  YEN, EUR, 0.0077
  GBP, YEN, 167.75
  Horsepower, Watt, 745.7

  Each line A, B, C means that C is a conversion factor from A to B.  In other words, multiply by C to go from A to B, and 1 A is worth C B's.

  Example: USD, GBP, 0.69 means that 1 USD = 0.69 GBP, so multiplying by
  0.69 converts an amount from USD to GBP.

  And the expected output is a file with the query and the ratio value filled in.

  USD, EUR, 0.89
  Yard, Meter, 0.91

  Write a program that reads both input files and produces the expected output

  USD-EUR can be found by multiplying  0.69*167*0.0077=0.89 (Approx)

My Approach:
These inputs can be represented as a weighted bi-directional graph

     0.69      167        0.0077
USD------->GBP------->YEN------->EUR
   <------    <-------   <-------
     1.449      0.0059    129.87

If we represent this format the problem reduces to finding shortest path between two nodes which can be done using Dijkstra. Instead of writing graph code from scratch i found a library from https://pypi.org/project/Dijkstar/ which reduced my code to

>>> from dijkstar import Graph,find_path
>>> graph=Graph()
>>> graph.add_edge('USD','GBP',{'cost':0.69})
>>> graph.add_edge('GBP','YEN',{'cost':167})
>>> graph.add_edge('YEN','EUR',{'cost':0.0077})
>>> graph.add_edge('EUR','YEN',{'cost':129.87})
>>> graph.add_edge('YEN','GBP',{'cost':0.0059})
>>> graph.add_edge('GBP','USD',{'cost':1.449})
>>> cost_func=lambda u,v,e,prev_e:e['cost']
>>> t=find_path(graph,'USD','EUR',cost_func=cost_func)
>>> print(t.costs)
[0.69,167,0.0077]
>>> from functools import reduce
>>> ans=reduce(lambda x, y: x*y, t.costs)
>>> print(round(ans,2))
0.89

I think the time complexity should be O(|V|+|E|).

Can I optimize this further? Is there a better or another way to solve this problem ?

The post (and the original assignment!) are a bit confused about these concepts:

the ratio between two units of measurement
the exchange rate between two currencies
the weight of an edge in a graph

The confusion is understandable because we sometimes use the word "cost" for all three concepts. But there are some important differences:

When combining ratios or exchange rates, you multiply them, but when combining weights you add them.
When converting units of measurement, it doesn't matter what path you take.

For example, here's a possible set of inputs to your program:

yard, foot, 3
chain, foot, 66
mile, chain, 80
mile, yard, 1760

If you need to use this to convert a mile into feet, it doesn't matter whether you take the path mile → yard → foot or mile → chain → foot, the answer is the same (5280) either way. Different paths can only yield different results if there are errors in the input.

However, when exchanging currency, it might be better value to take one route rather than another. Due to arbitrage the difference between paths is likely to be small in the real world, but it can exist and so ought to be taken into account in any computer model.

This has some consequences:

Converting currencies and converting units of measurement are two different problems, and you shouldn't expect to be able to write one program that handles both.
A general graph is the wrong data structure for units of measurement, because it could have multiple paths between pairs of units, which means you would have to worry about data inconsistency. A better data structure would be a forest, so that there is at most one path between any pair of units and the data cannot be inconsistent.
If you are going to represent a collection of ratios as a weighted graph, then the weights need to be the logarithms of the ratios, so that the total weight of a path (the sum of the weights of the edges) is the log of the product of the ratios, as required.
Logarithms of ratios below 1 are negative, and Dijkstra's algorithm doesn't work with negative edge weights.
A graph with some negative edge weights might have a cycle with a negative sum. This means that there is no path of lowest weight in the graph. For example, suppose that you can exchange 2 marks for a crown, 2 guilders for a mark, and 2 crowns for a guilder. Then you can get arbitrary amounts of money by going around this cycle as often as you like. The program will need to have some way of handling this case—for example, maybe it indicates an error in the input.


Selling shoes of the right size This is a question from HackerRank. I have solved it and want to know if it's the best answer I could have given in Python 3.

Sample code:

>>> from collections import Counter
>>>
>>> myList = [1,1,2,3,4,5,3,2,3,4,2,1,2,3]
>>> print Counter(myList)
Counter({2: 4, 3: 4, 1: 3, 4: 2, 5: 1})
>>>
>>> print Counter(myList).items()
[(1, 3), (2, 4), (3, 4), (4, 2), (5, 1)]
>>>
>>> print Counter(myList).keys()
[1, 2, 3, 4, 5]
>>>
>>> print Counter(myList).values()
[3, 4, 4, 2, 1]

  Task:

  Raghu is a shoe shop owner. His shop has X number of shoes. He has a
  list containing the size of each shoe he has in his shop. There are N
  number of customers who are willing to pay \$X_i\$ amount of money
  only if they get the shoe of their desired size.

  Calculate how much Raghu earned.

  Input format:

  The first line contains the number of shoes.

  The second line contains the space separated list of all the shoe
  sizes in the shop.

  The third line contains N, the number of customers.

  The next N lines contain the space separated values of the shoe size
  desired by the customer and Xi, the price of the shoe.

  Output format:

  Print the amount of money earned by Raghu.

  Sample input:

10
2 3 4 5 6 8 7 6 5 18
6
6 55
6 45
6 55
4 40
18 60
10 50

  Sample output:

200

Is this good? Where can I improve?

from collections import Counter
for _ in range(2):
    shoes = map(int, input().strip().split())
shoes = Counter(shoes)
income = 0
for _ in range(int(input())):
    query = list(map(int, input().split()))
    if query[0] in shoes and shoes[query[0]] > 0:
        shoes[query[0]] -= 1
        income += query[1]
print(income)

Repetition

Except the additional list these lines are identical:

shoes = map(int, input().strip().split())

query = list(map(int, input().split()))

You can write a function:

def read_integers_from_line(line):
    """
    >>> list(read_integers_from_line("12 3 7"))
    [12, 3, 7]
    """
    return map(int, line.strip().split())

That you can use twice.

Tuple unpacking

Instead of query = read_integers_from_line(input()) you can use: shoe, price = read_integers_from_line(input()) so that the code becomes more self-descriptive.

Skipping the first line

The first line contains useless information, you skip it weirdly using a range but I would suggest a more explicit skip by using instead _ = input()


Truncating integer using string manipulation I have a class with a data member that needs to be rounded up to a 2 digit integer, irrespective of the number of the input digits.

For example:

roundUpto2digit(12356463) == 12
roundUpto2digit(12547984) == 13 // The 5 rounds the 12 up to 13.

Currently my code looks like:

int roundUpto2digit(int cents){
    // convert cents to string
    string truncatedValue = to_string(cents);
    // take first two elements corresponding to the Most Sign. Bits
    // convert char to int, by -'0', multiply the first by 10 and sum the second
    int totalsum =  int(truncatedValue[0]-'0')*10 + int(truncatedValue[1]-'0');
    // if the third element greater the five, increment the sum by one
    if (truncatedValue[2]>=5) totalsum++;
    return totalsum;
}

How can this be made less ugly?

If I understood your requirements correctly then it might be like this:

int roundUpto2digit(int cents) {
  if (cents < 100)
    return cents < 10 ? cents * 10 : cents;
  while ((cents + 5) >= 1000)
    cents /= 10;
  return (cents + 5) / 10;
}

The test:

#include <stdio.h>

void
test(int i) {
    printf("%d -> %d\n", i, roundUpto2digit(i));
}

int
main() {
    test(0);
    test(1);
    test(5);
    test(9);
    test(10);
    test(49);
    test(50);
    test(94);
    test(95);
    test(99);
    test(100);
    test(104);
    test(105);
    test(994);
    test(995);
    test(999);
    test(1000);
    test(1040);
    test(1050);
    return 0;
}

The result:

0 -> 0
1 -> 10
5 -> 50
9 -> 90
10 -> 10
49 -> 49
50 -> 50
94 -> 94
95 -> 95
99 -> 99
100 -> 10
104 -> 10
105 -> 11
994 -> 99
995 -> 10
999 -> 10
1000 -> 10
1040 -> 10
1050 -> 11

It is uncertain if [1, 9] range should map into [10, 90] (2 digits) or [1, 9] (1 digit). I could fix it, if the later case is true.


Rock Paper Scissors without arrays and if-statements, how to reduce As working on a little private programming challenge, I tried "Rock Paper Scissors" without arrays or if-statements. It works as intended, though, I am sure this can be done a little bit more optimized. Especially in the context of Math/Programming.

What I try to achieve:

Create 2 Players, one Computer one Human.
They both can make a Gesture (Rock, Paper or Scissors at Random or to Choose for the Human Player).
The value of each Gesture is calculated into a Score
The calculated Scores are calculated into a Winner of the Duel

The Players are two classes that implement an abstract class.
The Winner and Gesture are Enums with specified values

The random Method just sets a random Gesture value for the player like:

 public void Random() => _gesture = (Gesture)_random.Next(0, 3);

The score for a single Hand is created as if it were points on a circle at 0, 120 and 240 degrees

((double)(_winner + 1) * 3 + (double)_gesture * 2) % 6 * Math.PI / 3;

The opposite hand is done the same, however, it starts at 180 degrees ( 180, 300, 60 degrees)

I try to find the position of both hands on the circle and calculate that into a unique value that results in a winner.

(Winner)(Math.Round(Math.Sin(other.Score() - this.Score())) * -(int)_winner);

This all works nice, however I suppose that the latter both rules could be reduced.

double Score() => ((double)(_winner + 1) * 3 + (double)_gesture * 2) % 6 * Math.PI / 3;
public Winner Duel(Player other) => (Winner)(Math.Round(Math.Sin(other.Score() - this.Score())) * -(int)_winner);

Complete code:

using System;

namespace RockPaperScissors
{
    public enum Winner
    {
        Draw = 0,
        Computer = -1,
        Human = 1
    }

    public enum Gesture
    {
        Rock = 0,
        Paper = 1,
        Scissors = 2,
    }

    public abstract class Player
    {
        private static readonly Random _random = new Random(DateTime.Now.Millisecond);
        private readonly Winner _winner;
        protected Gesture _gesture;

        protected Player(Winner winner)
        {
            _winner = winner;
        }

        public Gesture Gesture => _gesture;
        public void Random() => _gesture = (Gesture)_random.Next(0, 3);
        double Score() => ((double)(_winner + 1) * 3 + (double)_gesture * 2) % 6 * Math.PI / 3;
        public Winner Duel(Player other) => (Winner)(Math.Round(Math.Sin(other.Score() - this.Score())) * -(int)_winner);
    }

    public class Computer : Player
    {
        public Computer() : base(Winner.Computer)  { }
    }

    public class Human : Player
    {
        public Human() : base(Winner.Human) { }
        public void Choose(Gesture gesture) => _gesture = gesture;
    }

    class Program
    {

        static void Main(string[] args)
        {
            Computer computer = new Computer();
            Human human = new Human();

            for(int i = 0; i < 10; i ++)
            {
                computer.Random();
                human.Random();

                Console.WriteLine($"Computer does {computer.Gesture}, Human does {human.Gesture}");
                Console.WriteLine($"Winner = {computer.Duel(human)}");
                Console.WriteLine($"Winner = {human.Duel(computer)}");

                Console.ReadLine();
            }
        }
    }
}

This will do

int a = _random.Next(0, 3);
int b = _random.Next(0, 3);
int winner = (a - b + 4) % 3 - 1;

There is no need to subtract angles in radians or degrees as you can simply use integers. Technically a - b is sufficient to determine the winner and + 4 % 3 - 1 wraps it to the -1/0/+1 interval.


Python text appender The aim of it is to be the quickest way to add to existing notes via SSHh.

Is this code in Python style? Am I missing any methods that would make the code simpler or legible?

#! /usr/bin/python3

#Maintain: myemail@mail.com
#Purpose: A simple way to add entries to existing files
#Usage: Write the name of the file you intend to add to.
#Future: A list of editable files when the program opens

from sys import argv
import datetime
import os,errno
import os.path
import contextlib

script, typed = argv
filename = typed+".txt"
savelocation = os.path.join('/home/user/',filename)
print(savelocation)
print("Aditor: a digital typewriter, Type: d when done")

date = str(datetime.datetime.now().date())
with open("/tmp/tempfile.txt","a+") as target:
        target.write("\n")
        target.write(date)
        while True:
                entry=input()
                target.write("\n")
                if entry in ['done','d']:
                        break
                else:
                        target.write(entry)

target = open("/tmp/tempfile.txt","r")
f = open(filename, "a")
f.write(target.read())

f.close()
target.close()
print("Entry saved.")

with contextlib.suppress(FileNotFoundError):
    os.remove("/tmp/tempfile.txt")

If you don't want to write to the file directly and want to "apply" the changes at once, instead of writing to a temp file, you can keep the changes in memory - in a list of strings:

from sys import argv
from datetime import datetime
import os

if __name__ == '__main__':
    _, typed = argv
    filename = typed + ".txt"
    save_location = os.path.join('/home/user/', filename)

    print(save_location)
    print("Aditor: a digital typewriter, Type: d when done")

    date = datetime.now().strftime("%Y/%m/%d")

    cache = ["\n", date]
    while True:
        entry = input()
        cache.append("\n")

        if entry == 'done' or entry == 'd':
            break

        cache.append(entry)

    with open(save_location, "a") as f:
        f.writelines(cache)

Note that I've removed everything related to handling the temp file.

Also there were some other changes applied in the code above:

using 4 spaces for indentation
used _ for a "throwaway" variable (the script name is not used)
fixed the variable naming - words need to be separated with an underscore - save_location instead of savelocation (PEP8 reference)
checking for "done" using simple comparisons with done and d
using strftime() to dump a datetime into string
putting the code to under if __name__ == '__main__': to avoid the code to be executed if imported
organized imports per PEP8 (reference)


Test Driven Development, Mocking & Unit-testing I have the below code which is still under development - but I'm trying to get myself to find the ideal way to move forward.

public interface IMemberSubscriptionService
{
    void SelectSubscription(IMember member, IMemberSubscriptionTypePrice subscription, IOrder linkedOrderPaidWith);
    IMemberSubscriptionRenewal GetCurrentApplicableRenewal(IMember member);

}

public class MemberSubscriptionService : IMemberSubscriptionService
{
    private readonly IMemberSubscriptionRenewalService memberSubscriptionRenewalService;
    public MemberSubscriptionService(IMemberSubscriptionRenewalService memberSubscriptionRenewalService)
    {
        this.memberSubscriptionRenewalService = memberSubscriptionRenewalService;
    }

    public void SelectSubscription(IMember member, IMemberSubscriptionTypePrice subscription, IOrder linkedOrderPaidWith)
    {
        var renewal = GetCurrentApplicableRenewal(member);
        if (renewal == null || !object.Equals(renewal.Subscription, subscription))
        {//if they are different

            if (renewal != null)
            {
                updateRenewalDateToEndAsOfYesterday(renewal);
            }
            var newRenewal = createFirstSubscriptionRenewal(member, subscription, linkedOrderPaidWith);
            memberSubscriptionRenewalService.Renew(newRenewal);
        }

    }

    private IMemberSubscriptionRenewal createFirstSubscriptionRenewal(IMember member, IMemberSubscriptionTypePrice subscription, IOrder linkedOrder)
    {
        IMemberSubscriptionRenewal renewal = member.SubscriptionRenewals.CreateNewItem();
        renewal.StartDate = CS.General_v3.Util.Date.Now.Date;
        renewal.EndDate = CS.General_v3.Util.Date.GetEndOfDay(PayMammoth.Connector.Util.GeneralUtil.GetDateTimeAfterIncrementingBillingPeriod(renewal.StartDate,
            subscription.DurationIntervalType,subscription.DurationInDays).AddDays(-1));
        renewal.LinkedOrder = linkedOrder;
        renewal.Save();
        return renewal;

    }

    private void updateRenewalDateToEndAsOfYesterday(IMemberSubscriptionRenewal renewal)
    {
        renewal.EndDate = CS.General_v3.Util.Date.Now.AddDays(-1);

    }

    #region IMemberSubscriptionService Members

    public IMemberSubscriptionRenewal GetCurrentApplicableRenewal(IMember member)
    {
        throw new NotImplementedException();
    }

    #endregion
}

What this code should do is 'select a subscription' (SelectSubscription) for a particular member.  A member has a list of SubscriptionRenewals which basically contain information for each period of a subscription, e.g taking a monthly subscription would have a SubscriptionRenewal for Jan 2012, another for Feb 2012, etc.

The SelectSubscription should get the current subscription, and if the subscription is to be changed, the existing renewal is updated to end as of yesterday, and the new subscription starts from today, for the total period (e.g changing from monthly to yearly).  If you have a SubscriptionRenewal from 1 - 29 Feb 2012, and today is 15 Feb and you call 'SelectSubscription' with a yearly interval, this should change the existing SubscriptionRenewal to end on 14Feb 2012, and create a new one from 15 Feb 2012 till 14 Feb 2013 for this particular case.

This is just to give you an idea of the functionality.  I need to create the respective unit tests for such a functionality, and came out with several scenarios, as per below:

if subscription is the same as the current, do nothing.
if a current subscription renewal exists, it is updated to end as of yesterday
test that SelectSubscription creates new renewal is created, based on today and the subscription

My main issue is on how to structure the code for unit-testing.  Should you create the GetCurrentApplicableRenewal as part of the same service?  This way, I cannot simply mock that functionality, when I am testing out a portion of the SelectSubscription.  Does it make sense to take that functionality out into a different interface/class, so that it can be mocked?

I see two options:

Test/implement GetCurrentApplicableRenewal first.
Implement the code you need (which will likely go into GetCurrentApplicableRenewal) in your SelectSubscription method now, imeplement/test GetCurrentApplicableRenewal next, and refactor it to use the GetCurrentApplicableRenewal method later.

The latter allows you to implement and define tests for both methods in isolation from one another, only refactoring when you have a full set of tests.  With these tests, you can safely validate whether refactoring SelectSubscription can safely switch to using GetCurrentApplicableRenewal or not.

The former way will save you some time, and as long as you don't skip any tests for SelectSubscription, you will probably be OK.  The downside is that you have to context-switch to GetCurrentApplicableRenewal.


Set openCV basic structures to zero in a unique way For test cases I need a unique way to set any structure to zero. For openCV I wrote this:

  // Fill every basic structure of openCV with zero.
  template < typename Tp >
  void zero( cv::Point_<Tp>& p ) {
    p.x = p.y = 0;
  }
  template < typename Tp >
  void zero( cv::Point3_<Tp>& p ) {
    p.x = p.y = p.z = 0;
  }
  template < typename Tp >
  void zero( cv::Size_<Tp>& s ) {
    s.width = s.height = 0;
  }
  template < typename Tp >
  void zero( cv::Rect_<Tp>& r ) {
    r.x = r.y = r.width = r.height = 0;
  }
  inline void zero( cv::RotatedRect& r ) {
    zero( r.center );
    zero( r.size );
    r.angle = 0;
  }
  inline void zero( cv::TermCriteria& t ) {
    t.type = t.maxCount = 0;
    t.epsilon = 0.0;
  }
  template < typename Tp, int M, int N>
  void zero( cv::Matx<Tp, M, N>& m ) {
    m = 0;
  }
  // Vec<> and Scalar<> are derived from Matx<>
  inline void zero( cv::Range& r ) {
    r.start = r.end = 0;
  }
  inline void zero( cv::Mat& m ) {
    m = 0;
  }
  inline void zero( cv::SparseMat& sm ) {
    sm.clear();
  }
  template < typename Tp >
  void zero( cv::SparseMat_<Tp>& sm ) {
    sm.clear();
  }

Setting cv::Mat to zero by assigning a new zero filled Mat is reallocating. But looping over would be slow (now done by *= 0). Using memset( m.data, ... ) is only possible if m.isContinuous(). Any idea of being safe and fast? Is it possible to join some derived cases?

The template parameters _Tp has been replaced by Tp because starting with _, followed by uppercase letter is reserved. That is something the openCV authors do violate too (I copied it from the openCV core code).

Mat, Matx and derived Vec and Scalar can be set to zero by multiplying with zero. Internal this is a for() over all elements but without the realloc of the zeros() assignment.

If you want to zero a Matx, you can take advantage of its val public data member, which is a good old C array. Then you can rely on the standard library algorithm std::fill to set every element of the matrix to 0; the algorithm generally does static dispatch at compile time to call std::memset whenever possible. Using it should allow you to always be safe but also to be safe when possible:

template < typename Tp, int M, int N>
void zero( cv::Matx<Tp, M, N>& m ) {
  std::fill(std::begin(m.val), std::end(m.val), 0);
}

Since Matx::val is a fixed-size C array, std::begin and std::end should work. If you don't use a C++11 standard library, you can implement it as follows instead:

template < typename Tp, int M, int N>
void zero( cv::Matx<Tp, M, N>& m ) {
  std::fill(m.val, m.val + M*N, 0);
}

For cv::Mat, the documentation says that:

  To set all the matrix elements to the particular value after the construction, use the assignment operator Mat::operator=(const Scalar& value).

It means that you can zero the matrix like this (I couldn't find equivalent overloads for Matx or SparseMat):

inline void zero( cv::Mat& m ) {
  m = 0;
}

Also, as a side note, most modern compilers just don't care about inline, at least they don't take it as a hint for inlining since they already do a great job when it comes to inlining stuff. Nowadays, inline is just a way to solve ODR problems, which don't happen with templates. All of your functions are function templates, so you can safely remove inline.