Simple Deferred / Promise pattern in JavaScript I've wrote a very simple implementation of Deferred / Promise pattern (inspired by jQuery's $.deferred) for my tiny project (which does not use jQuery)

It can handle multiply onSuccess an onFail callbacks with result caching. Example in comments above the code.

/**
 * defer() returns methods:
 *      resolve(data) - request is successfully resolved (returns nothing)
 *      reject(msg) - request if failed (returns nothing)
 *      promise() - returns promise function
 *
 * promise() returns methods:
 *      done(successfulCallback) - add on successful callback (returns nothing)
 *      fail(failCallback) - add on fail callback (returns nothing)
 *
 * Example:
 *
 * function someHeavyRequest(url) {
 *      var d = defer();
 *      doHeavyRequest(url,
 *          function (data) {
 *              // on success
 *              // ...do something
 *              d.resolve(data);
 *          },
 *          function (data) {
 *              // on fail
 *              // ...do something
 *              d.reject(data);
 *          });
 *      return d.promise();
 * }
 *
 * var req = someHeavyRequest("http://some.url/of/request/");
 *
 * req.done(function (data) {
 *      console.log("We got it!");
 *      console.log(data);
 * });
 *
 * req.done(function (data) {
 *      console.log("We got it again without new request!");
 *      console.log(data);
 * });
 *
 * req.fail(function (data) {
 *      console.error("Something wrong");
 *      console.error(data);
 * });
 *
 */
function defer () {
    var status = 0, // 0 = in progress / 1 = successful / -1 = fail
        callbacks = { done: [], fail: [] },
        args = { resolve: [], reject: [] },
        execute = function (callbacks, args) {
            var f;
            while (callbacks.length) {
                f = callbacks.shift();
                if (f) f.apply(this, args);
            }
        };

    return {
        promise: function () {
            return {
                done: function (callback) {
                    callbacks.done.push(callback);
                    if (status === 1) {
                        execute(callbacks.done, args.resolve);
                    }
                },

                fail: function (callback) {
                    callbacks.fail.push(callback);
                    if (status === -1) {
                        execute(callbacks.fail,args.reject);
                    }
                }
            };
        },

        resolve: function () {
            status = 1;
            args.resolve = arguments;
            execute(callbacks.done, args.resolve);
        },

        reject: function () {
            status = -1;
            args.reject = arguments;
            execute(callbacks.fail, args.reject);
        }
    };
}

A few things caught my eye:

You can call resolve and later call reject too - or the other way around, or call either one multiple times. It should only allow the first resolve/reject call to have an effect.
status might as well be a string (pending, resolved, rejected) which obviate the need for a comment explaining the integer values, and would improve readability
The if(f) check in execute should probably be a full if(typeof f === 'function') check; lots of values are truthy without being functions. You could also do this check in done/fail and simply ignore/complain about non-function arguments.
There's some duplication between done/fail and resolve/reject.

For that last one, if the status becomes a string, it's also relative easy use that as a key for organizing the callbacks and arguments.

function defer() {
  var currentState = 'pending',
      callbacks    = { resolved: [], rejected: [] },
      args         = []; // only need 1 args array

  function execute(state) {
    var cb;
    while(callbacks[state].length) {
      cb = callbacks[state].shift();
      if( typeof cb === 'function' ) cb.apply(this, args);
    }
  }

  // generic function factory for done/fail functions
  function hook(state) {
    return function (cb) {
      callbacks[state].push(cb);
      if(currentState === state) execute(state);
    }
  }

  // generic function factory for resolve/reject functions
  function complete(state) {
    return function () {
      if(currentState !== 'pending') return;
      args = Array.prototype.slice.call(arguments, 0);
      currentState = state;
      execute(state);
    }
  }

  return {
    promise: function () {
      return { done: hook('resolved'), fail: hook('rejected') };
    },
    resolve: complete('resolved'),
    reject:  complete('rejected')
  };
}

You may want to add then, always and isPending (or just state) functions. And once you've resolved or rejected, you might as well get rid of the callbacks you're not going to ever use (i.e. if resolved, clear out the reject-callbacks,and vice-versa).

Lastly, if you want to emulate jQuery, make it chainable, so you can call x.done(...).fail(...).fail(...).always(...) etc.


Calling a method only once in an updateScore function in SpriteKit This works fine, but obviously it's messy and I'm thinking there has to be a better way to do this. Having to constantly remember to flip the bool back is a pain, is there anything else I can do here?  I take it SpriteKit itself doesn't have any convenience method for calling something only once for example.

func updateScore() {

        // MARK: Increase Difficulty
        switch score {
        case 0:
            if !levelToggle {
                showLevelGraphic(1)
                levelToggle = true
            }
        case 1:
            levelToggle = false
        case 5:
            if !lighteningToggle {
                GameEffects.lightening(self, masterNode: worldNode, backgroundStringName: "background")
                GameEffects.lightening(self, masterNode: worldNode, backgroundStringName: "foreground")
                GameEffects.shakeScreen(worldNode)
                lighteningToggle = true
            }

            if !rainToggle {
                makeItRain()
                rainToggle = true
            }
            break
        case 6:
            lighteningToggle = false
        case 10:
            if !rotateToggle {
                GameEffects.rotateScreenAndHold(worldNode, finishAngle: 0.06, totalEffectDuration: 5.0)
                rotateToggle = true
            }
        case 15:
            rotateToggle = false
        case 16:
            if !rotateToggle {
                GameEffects.rotateScreenAndHold(worldNode, finishAngle: -0.06, totalEffectDuration: 5.0)
                rotateToggle = true
            }
             break
        case 17:
            rotateToggle = false
        case 20:
            stopRaining()
            rainToggle = false
        case 25:
            if !rotateToggle {
                GameEffects.rotateScreenAndHold(worldNode, finishAngle: -0.06, totalEffectDuration: 10.0)
                rotateToggle = true
            }
        case 26:
            rotateToggle = false
        case 45:
            break
        case 50:
            if !levelToggle {
                showLevelGraphic(2)
                levelToggle = true
            }
            kGroundSpeed = 200
            break
        case 55:
            if !zoomToggle {
                GameEffects.zoomIn(worldNode)
            }
        case 56:
            zoomToggle = false
        case 70:
            if !rotateToggle {
                GameEffects.rotateScreenAndHold(worldNode, finishAngle: 0.06, totalEffectDuration: 10.0)
                rotateToggle = true
            }

First,  use properties,  they are your best friend

var rotateToggle = false
{
    didSet
    {
        if(rotateToggle)
        {
            GameEffects.rotateScreenAndHold(worldNode, finishAngle: -0.06, totalEffectDuration: 5.0)
        }
        else
        {
            //if you need something on false, do it here
        }
    }
}

do this for all of your other toggles.

Update for score property:

var score : UInt = 0
{
    didSet
    {
        updateScore()
    }
}

Now we have your case statement, chain them together

switch (score)
{
    case 0:
      levelToggle = true
    case 1:
      levelToggle = false
    case 10,16,25:
      rotateToggle = true
//...etc
}

of course, how you have toggle set up, this requires score increases by 1, and updateScore gets fired afterwards before score can increase by 1 again.


Dictionary GetValueOrDefault I'm pretty sure there's no better way to do this but I thought a consensus on here might be nice. Let me know what you think.

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
    TValue value;
    return dictionary.TryGetValue(key, out value) ? value : defaultValue;
}

I'm pretty sure there's no better way to do this but I thought a consensus on here might be nice. Let me know what you think.

At first glance it seems to be ok but one could assume that he/she is always getting either the value or the default value. If one would get the default value for e.g if the dictionary is empty, why shouldn't he/she get the default value if the dictionary is null or the key is null ?

If this isn't the way the method should work, then you need some documentation which cleary states the purpose and any expected exceptions.

Now, what happens if the method is called either like so

Dictionary<string, int> dict = null;
dict.GetValueOrDefault("someKey");

or (assuming the method lives in the "DictionaryExtensions" class)

Dictionary<string, int> dict = null;
DictionaryExtensions.GetValueOrDefault(dict, "someKey");

sure you get a NullReferenceException but is this the type of exception one would expect ? If I call a method with an parameter being null I would expect to get either no exception or to get an ArgumentNullException.

What about if the key is null ? Sure you get an ArgumentNullException but it takes some time because it is thrown at the FindEntry() method of the Dictionary and this is also shown in the StackTrace.

IMO it would be better to check this cases inside the GetValueOrDefault() method and throw the exceptions there.

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
    if (dictionary == null) { throw new ArgumentNullException(nameof(dictionary)); } // using C# 6
    if (key == null) { throw new ArgumentNullException(nameof(key)); } //  using C# 6

    TValue value;
    return dictionary.TryGetValue(key, out value) ? value : defaultValue;
}


Simple Affine cipher The affine cipher is a simple mathematical substitution cipher. If you're interested in the details behind how it works, this page goes further into detail.

After writing a program to encrypt and decrypt text using the affine cipher, I felt that it seemed needlessly cluttered. I have also noticed that there are no questions related to the affine cipher on Code Review, at least not for C++.

Here's the code:

#include <iostream>
#include <algorithm>
#include <numeric>
#include <cmath>

int gcd(int a, int b);
int modInverse(int a, int b); // from https://rosettacode.org/wiki/Modular_inverse#C.2B.2B

int main(){
    std::string choice;
    do{
        std::cout << "Encrypt or Decrypt? [e/d] = ";
        std::getline(std::cin, choice);
        std::transform(choice.begin(), choice.end(), choice.begin(), ::tolower);
    } while(choice.length() > 1 || choice != "e" && choice != "d");

    std::cout << "\nInput string: ";
    std::string input;
    std::getline(std::cin, input);

    int a, b;
    do{
        std::cout << "\na and b must be coprime\na = ";
        std::cin >> a;
        std::cout << "b = ";
        std::cin >> b;
    } while(std::cin.fail() || gcd(a,b) != 1);

    std::cout << '\n';

    if(choice == "e"){
        for(int i = 0; i < input.length(); ++i){
            if(input[i] >= 'a' && input[i] <= 'z'){
                std::cout << (char)((a * (input[i] - 'a') + b) % 26 + 'a');
            } else if(input[i] >= 'A' && input[i] <= 'Z'){
                std::cout << (char)((a * (input[i] - 'A') + b) % 26 + 'A');
            } else{
                std::cout << input[i];
            }
        }
    } else{
        for(int i = 0; i < input.length(); ++i){
            if(input[i] >= 'a' && input[i] <= 'z'){
                std::cout << (char)(modInverse(a, 26) * (26 + input[i] - 'a' - b) % 26 + 'a');
            } else if(input[i] >= 'A' && input[i] <= 'Z'){
                std::cout << (char)(modInverse(a, 26) * (26 + input[i] - 'A' - b) % 26 + 'A');
            } else{
                std::cout << input[i];
            }
        }
    }

    std::cout << '\n';
    return 0;
}

int gcd(int a, int b){
    return b == 0 ? a : gcd(b, a % b);
}

int modInverse(int a, int b){
    int b0 = b, t, q;
    int x0 = 0, x1 = 1;
    if (b == 1) return 1;
    while (a > 1) {
        q = a / b;
        t = b, b = a % b, a = t;
        t = x0, x0 = x1 - q * x0, x1 = t;
    }
    if (x1 < 0) x1 += b0;
    return x1;
}

Here is an example output for the encryption side:

Encrypt or Decrypt? [e/d] = e

Input string: Hello World!

a and b must be coprime
a = 5
b = 9

Sdmmb Pbqmy!

And here is an example output for the decryption side:

Encrypt or Decrypt? [e/d] = d

Input string: Sdmmb Pbqmy!

a and b must be coprime
a = 5
b = 9

Hello World!

I am using the modInverse() function from Rosetta Code. This is quasi-related to my ongoing series of classical ciphers. So far I have done a simple Caesar cipher and an Atbash cipher.

How can I improve this code, both for readability and for efficiency?

It's very easy to understand each step in your program, even without understanding
the actual algorithm you're implementing, which is really nice. Here's how I would improve things.

Use Functions

Each part of your main() function looks to me like it should be a separate named function. For example, it's clear that you're asking for whether the user wants to encrypt or decrypt, then getting the input string, then asking for a and b, then doing the encryption or decryption. Each of those things is a separate task and as such should be its own function. Your main() should look more like this:

int main() {
    std::string choice = getTaskFromUser(); // returns either 'e' or 'd'
    std::string input = getInputFromUser(); // return string to encrypt or decrypt
    int a, b;
    getAAndB(a, b); // Gets a and b
    if (choice == TASK_ENCRYPT)
    {
        displayEncryptedText(input, a, b);
    }
    else
    {
        displayDecryptedText(input, a, b);
    }

    std::cout << "\n";
    return 0;
}

Usability

There are a few things in your program that are named confusingly. First, I had to look up what "coprime" meant. It's not a hard concept, but I'd never heard it before. It might be worth printing out a one or two-sentence description of what it means when asking the user to input coprime numbers.

The variables a and b may also be poorly named. What function do they serve in the algorithm? Looking at the linked page, it looks like the cypher consists of a linear equation of the form y = ax + b where a is the slope and b is the intercept. That might be a good set of names, or it might not depending on how a typical user of such an algorithm thinks about it. a and b aren't terrible given the abstractness of the algorithm, but if you can improve those names, I recommend it. Certainly, as a user, I wouldn't understand what they represent.

Avoid Magic Numbers

Looking at the code, I see this mysterious number "26" repeated in several places. What does it represent? As an English speaker, I can guess that it's the number of letters in the alphabet, but what if I want to include numbers and symbols? What if I want to implement this algorithm in a different language with more or fewer letters? At the very least, there should be a named constant for the value 26. I recommend something like:

const int kAlphabetSize = 26;

That way you can change it later. Also, the magic values a, A, z, and Z should be handled better. Do you want to preserve the case of the original text? It seems to me like it makes the cypher weaker if you do. It's additional context that someone trying to decrypt it can use. I recommend allowing at least all the (printable) ASCII characters to be used as one large alphabet. It simplifies the code as you don't end up special-casing as many things.


Create a person if no duplicate exists I am new to promises and although I am enjoying the simplicity after dealing with callback hell I am having some issues trying to chain them together as you can see by the mess below. So basically what I am trying to achieve is to run through the chain and if there are no duplicate people found then hit the catch and create the person. If not then go down the chain through the subsequent promises and it works but i feel like i am going about this in the wrong manner.
I think it is because in each promise i am resolve to true or false.

self.getPotentialPersonDuplicates()
    .then(function (result) {
        return result.length > 0
            ? self.promptUserToPickFromDuplicatePersons(result)
            : Promise.reject(new Error("There where no duplicates found"));
    })
    .then(function (result) { return result ? self.selectPersonFromDuplicatePersonLookup() : Promise.resolve(); })
    .then(function (result) { return result ? self.addAsParticipant(redirect) : Promise.resolve(); })
    .catch(function (e) {
        console.log(e.stack);
        return self.createOdsPerson()
        .then(self.addAsParticipant.bind(self, redirect));
    })
    .then($ui.stopWaiting());

The promise methods in there contain a lot of code and I don't think there are any code review related issues so I have not included them.

To me it looks like you should move the if (result) into each one of  promptUserToPickFromDuplicatePersons, selectPersonFromDuplicatePersonLookup and addAsParticipant and add a method:

function handleError(e( {
    console.log(e.stack);
    return self.createOdsPerson()
           .then(self.addAsParticipant.bind(self, redirect));
}

Then you can rewrite the code as:

self.getPotentialPersonDuplicates()
  .then(promptUserToPickFromDuplicatePersons)
  .then(selectPersonFromDuplicatePersonLookup)
  .then(addAsParticipant )
  .catch(handleError)
  .then($ui.stopWaiting);

You could also look at the Bluebird promise library


Project Euler #1-3 in VBA Self explanatory, and all the functions contain descriptions of their specific problem statements.

Since this is project Euler, I would be particularly interested in any analysis of the functions' computational efficiency.

Option Explicit

Public Function ProjectEuler1() As Long

    '/ Problem statement: sum all multiples of 3 and 5 from 1 to 999

    Dim total As Long
    Dim i As Long

        For i = 1 To 999

            If i Mod 3 = 0 Or i Mod 5 = 0 Then

                total = total + i

            End If

        Next i

    ProjectEuler1 = total

End Function

Public Function ProjectEuler2() As Long

    '/ Problem statement: sum all even Fibonacci numbers below 4 Million

    Dim fiboA As Long, fiboB As Long, currentFibo As Long '/ alternating fibonacci numbers
    Dim bIsHigher As Boolean

        fiboA = 1
        fiboB = 1
        currentFibo = 2

    Dim total As Long

        Do While currentFibo < 4000000

            If currentFibo Mod 2 = 0 Then total = total + currentFibo

            If bIsHigher Then
                    fiboA = currentFibo
                Else
                    fiboB = currentFibo
            End If

            bIsHigher = Not bIsHigher

            currentFibo = fiboA + fiboB

        Loop

    ProjectEuler2 = total

End Function

Public Function ProjectEuler3(ByVal numToFactorise As Long) As Long

    '/ Problem statement: find the largest prime factor of <big number>
    '/ I decided to generalise it

    Dim i As Long
    Dim sqrtLimit As Long
    Dim partiallyFactorisedNum As Long
    Dim largestPrimeDivisor As Long

        sqrtLimit = Application.Floor(numToFactorise ^ 0.5, 1)
        partiallyFactorisedNum = numToFactorise

        i = 2
        If partiallyFactorisedNum Mod i = 0 Then
            largestPrimeDivisor = i
            partiallyFactorisedNum = KeepDividingWhileInt(partiallyFactorisedNum, i)
        End If

        i = 3
        Do While i <= sqrtLimit

            If isPrime(i) Then

                If partiallyFactorisedNum Mod i = 0 Then
                    largestPrimeDivisor = i
                    partiallyFactorisedNum = KeepDividingWhileInt(partiallyFactorisedNum, i)
                End If

            End If

            i = i + 2
            sqrtLimit = Application.Floor(partiallyFactorisedNum ^ 0.5, 1)
        Loop

    If partiallyFactorisedNum > largestPrimeDivisor Then largestPrimeDivisor = partiallyFactorisedNum
    ProjectEuler3 = largestPrimeDivisor

End Function

Public Function isPrime(ByVal numTocheck As Long)

    Dim i As Long
    Dim sqrtLimit As Long
        sqrtLimit = Application.Floor(numTocheck ^ 0.5, 1)

        isPrime = True

        If numTocheck Mod 2 = 0 And numTocheck <> 2 Then isPrime = False

        i = 3
        Do While i <= sqrtLimit And isPrime
            If numTocheck Mod i = 0 Then isPrime = False
            i = i + 2
        Loop

End Function

Public Function KeepDividingWhileInt(ByVal numToDivide As Long, ByVal divisor As Long) As Long

    Dim isDivisible As Boolean

        isDivisible = True
        Do While isDivisible
            If numToDivide Mod divisor = 0 Then
                    numToDivide = numToDivide / divisor
                Else
                    isDivisible = False
            End If
        Loop

    KeepDividingWhileInt = numToDivide

End Function

While quickly running over your code, my eyes stopped on your function KeepDividingWhileInt.

It seemed over-complicated.

Let's write what it does:

Creates isDivisible
Sets isDivisible to true
Loops while isDividible is true:

If numToDivide Mod divisor is 0

Divides numToDivide by divisor

Else

Sets isDivisible to false

Returns numToDivide

Well, there is a subtle bug on step 2: You assume that the number is divisible by divisor, which may not be true (e.g.: KeepDividingWhileInt(3,2)).

Also, the whole code could be extremely simplified, by moving the condition inside that if into the while.

Also, you could try to use While ... Wend instead of Do While ... Loop, as suggested by @Mat's Mug. This point actually applies everywhere on your code.

The final result:

Public Function KeepDividingWhileInt(ByVal numToDivide As Long, ByVal divisor As Long) As Long

    While numToDivide Mod divisor = 0
        numToDivide = numToDivide / divisor
    Wend

    KeepDividingWhileInt = numToDivide

End Function

Oh my! The function is so simple that you can read it in a single sentence:

  Divide numToDivide by divisor while the reminder is 0, returning the final result.

So simple, right?


Figure code and uniform code merger Below I have a simple method that will merge a figure code with a uniform code, let me talk more about that and how it works so you guys have a neat understanding.

Each character on my gaming emulator has a unique figure code, unless their dressed the exact same of course but unlikely, here is an example of a few figure codes.

lg-285-76.hr-155-31.sh-300-92.ca-1819-63.hd-209-2.ch-3030-76
he-3149-1331.sh-3035-110.hr-170-61.fa-3276-72.ch-255-110.hd-209-2.lg-280-1331
ch-210-110.hd-209-7.hr-828-1407.he-3082-63.lg-280-1408.cp-3309-77.sh-290-92

Above is 3 examples of a figure string, I wont go as far as explaining each part as its not relevant in this question (as of now).

So, each character has a figure code and then there is a few uniform codes. Now the uniform codes usually only contain the ch and lg parts for chest and leg, which is usually all a uniform needs, except I have coded this method to replace any parts of the uniform into the figure code with the uniform part. Here is a few examples of the uniform figure codes..

(Legs, Shoes and Chest)
lg-285-76.sh-300-92.ca-1819-63.ch-3030-76
(Legs and Chest)
lg-280-110.ch-1004-110

So what am I asking?
I am asking for some ways to improve the performance of this method.

Edit: I've added functionality to choose which parts you want to replace, with a list string as the second parameter, which is default empty (replaces all parts).

Here is the method:

public static string ReturnUniform(string uniform, string figure, List<string> partsToReplace = new List<string>())
{
    List<string> uniformParts = uniform.Split('.').ToList();
    List<string> figureParts = uniform.Split('.').ToList();

    foreach (string uniformPart in uniformParts)
    {
        string[] parts = uniformPart.Split('-');
        string part = parts[0];

        if (partsToReplace.Any() && !partsToReplace.Contains(part))
        {
            continue;
        }

        if (figure.Contains(part + "-"))
        {
            var indexOfItem = figureParts.ToList().FindIndex(value => value.StartsWith(part));
            figureParts[indexOfItem] = uniformPart;
        }
        else
        {
            figureParts.Add(uniformPart);
        }
    }

    return string.Join(".", figureParts);
}

If I understood you correctly what you need is to make this method faster.
Firstly, you need to answer one simple question is that really what you NEED, this method does not look that time consuming at all (unless you have hundreads of elements in that string). Have you benchmarked it and saw that it consumes a lot of time?

EDIT: it came to me that you might be asking about performance of your idea, if so - for me it looks good enough.

If you really need to make this method faster here is what I can see:

1. Do not use foreach loop. If you decided to make uniform.Split('.') a list take advantage of it. Change your loop to for

2. Do not use LINQ at all, it is known for being time consuming (since it enumerates everythingcall enumerator in every function).

partsToReplace.Any() && !partsToReplace.Contains(part)

can be changed to for loop when you do Any by List.Count method and Contains by simply checking every element. Exactly the same thing is applicable to if (figure.Contains(part + "-")) and var indexOfItem = figureParts.ToList().FindIndex(value => value.StartsWith(part));

in the latter I think you do not need to make ToList() but I have no compiler at hand to check it.

Note
Those changes are micro optimalisations, if your application is slow it is probably because of some other part. Plase do propper benchmark to check this.

Also : I think there should be List<string> figureParts = figure.Split('.').ToList();


Calculating numbers in the Collatz sequence I have found two ways in which I can generate the Collatz sequence given a start number. I have looked briefly into their performance, but I'd like a more in depth/solid review into the difference in the two methods.

The main difference really is, one of them doesn't have if statements. But the down side to this method is there's a fair bit of math involved, which is where I'm concerned with performance.

The main questions I have are:

Do computers get slower when they have to make (slightly) complex calculations?
Do if statements actually have a significant affect on performance?
If we were generating the numbers in the Collatz sequence for large numbers (for example numbers > 1,000,000,000), would the performance difference actually be noticeable?

Here are the two methods I have created:

private static ArrayList<Integer> generateCollatz1(int n) {
    ArrayList<Integer> results = new ArrayList<>();
    while(n != 1) {
        if(n % 2 == 0) {
            n = n / 2;
            results.add(n);
        }else {
            n = ((n * 3) + 1);
            results.add(n);
        }
    }
    return results;
}

private static ArrayList<Integer> generateCollatz2(int n) {
    ArrayList<Integer> results = new ArrayList<>();
    while(n != 1) {
        n = (int) ( (7 * n + 2) - (Math.pow(-1, n)) * (5 * n + 2) ) / 4;
        results.add(n);
    }
    return results;
}

The math in the second method (generateCollatz2) is using the following formula:

$$ f(n) = \frac{7n + 2 - (-1)^n (5n + 2)}{4}$$

Test it

You are asking which one is faster, but did you actually test it yourself?  I'm sure if you did you would find out.

Math.pow()

In your second solution, just using Math.pow() is slower than the entire first solution, probably by an order of magnitude.  I'm not sure why you think using a single if statement would be slower than calling an extremely complicated function (which probably contains lots of if statements).

Avoiding Math.pow

You can compute \$(-1)^n\$ by using the 1 bit, like this: -(2*(n & 1)-1).  So you can change your second solution to:

n = ( (7 * n + 2) + (2*(n & 1)-1) * (5 * n + 2) ) / 4;

This version would be very similar in runtime to the first solution.  I couldn't tell you which would be faster.  I'm also not sure if the division by 4 here will be optimized away.  You may want to convert that into a shift by 2 instead:

n = ( (7 * n + 2) + (2*(n & 1)-1) * (5 * n + 2) ) >> 2;


Creating an insert at front function for a circular doubly linked list I have only been working with C for a couple of months and I think I have a pretty good function to  insert a node in the front of the circular doubly linked list.  I have tested in adding several nodes to it, but would like some feedback.

struct used in functions:

typedef struct TCB_t
{
    struct TCB_t * next;
    struct TCB_t * previous;
    ucontext_t context;
    int val;
}TCB_t;

Here is the function that calls the add_to_list() function:

void start_thread(void (*function)(void), int *arg)
{
    printf("In main: creating thread\n");
    struct stack * stackP = (struct stack*)malloc(8192);
    tcb = (struct TCB_t *)malloc(sizeof(struct TCB_t));
    init_TCB (tcb, function, stackP,  8192);
    tcb->val = iter;
    iter++;
    add_to_list( &ptr, &tcb);
}

This is the function in which I am not sure if I am adding the nodes correctly:

void add_to_list( struct TCB_t **ptrBlock, struct TCB_t **addQ)
{
    (*addQ)->val = iter;
    iter++;

    if(!*ptrBlock)
    {
        *ptrBlock = *addQ;
        (*ptrBlock)->next= *ptrBlock;
        (*ptrBlock)->previous = *ptrBlock;
    }
    else
    {
        (*ptrBlock)->previous->next = *addQ;
        (*addQ)->previous = (*ptrBlock)->previous;
        (*addQ)->next = *ptrBlock;
        (*ptrBlock)->previous = *addQ;
    }
}

Here are some things that will help you improve your code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

Consider how your code is used

When somebody uses this code, they will probably want to insert a value into a data structure, and have the data structure handle all of the messy details, such as memory management. However, this code only seems to store successive integers (from a global variable) and requires the caller to allocate memory for the entire structure.  So instead of an interface like this:

void add_to_list( struct TCB_t **ptrBlock, struct TCB_t **addQ)

a better interface would probably be this:

void add_to_list(struct TCB_t *node, int value)

This interface would allocate space for an additional TCB_t, add the passed value to it and link it appropriately to the existing node.

Provide convenience functions for testing

In order to test your function, it would be useful to be able to do things like print out the contents of the list.  One way to do that is to provide a convenience functions such as this one:

void print_list(const struct TCB_t *n)
{
    if (n == NULL) {
        printf("empty list\n");
        return;
    }
    printf("%d", n->val);
    for (struct TCB_t *p=n->next; p!=n; p = p->next)
        printf(", %d", p->val);
    printf("\n");
}

Consider providing a whole interface

Instead of providing just one function, it is often more useful to provide a couple of related functions to deal with a particular structure.  For example, in the case of a circular doubly-linked list, node insertion, node deletion, printing, initialization and destruction are all reasonable things to expect from an interface.  Providing only one fails to provide a sufficient spectrum that makes the data structure actually usable.


Changing the id field to an _id field for the lowest tweet id I am collecting tweets using a Python script and I want to store them in a MongoDB database.

I want to find out the lowest tweet id and for each tweet, I want to change the id field to an _id field.

Is it possible not having to iterate over all the tweets with a for loop in order to do these 2 things.

search_result = #search result from Twitter API

min_id = float('inf')

result = []
for tweet in search_result:
    if tweet['id'] < min_id:
        min_id = tweet['id']
    tweet['_id'] = tweet.pop('id')
    result.append(tweet)

Three parts to your questions: 1) find lowest tweet ID and 2) assign id: to _id 3) can (1) and (2) be done without iterating over the list.

To answer (3) first, (1) definitely can be done without iteration, but I am unaware of how to do (2) without iteration.

(1) Lowest Tweet ID
The sort function is great for this. This question has more on sorting. To create a new list with the lowest id in the first position:

results = sorted(search_results, key=lambda x: x.id)

To do the same thing replacing the list:

search_results.sort(search_results, key=lambda x: x.id)

Simply access the "0"th result in the list and you have your lowest tweet id.

2) Assign id: to _id

Perhaps someone can answer this better, but the best way I know of is how you've done it:

for tweet in search_results:
    tweet['_id'] = tweet.pop('id')

Your final code would look something like:

search_result = #search result from Twitter API
results = sorted(search_results, key=lambda x: x.id)
min_id = results[0]['id']
for tweet in search_results:
    tweet['_id'] = tweet.pop('id')


Efficient implementation of comparison operators for cards in a poker simulation Just as a learning exercise, I am implementing a poker game simulator. I am purposefully not searching for ready made tutorials showing how to do this, relying mostly on the python docs - that way I hope to learn in a more memorable, albeit slower, way. I'm looking for advice on how to code the comparison between cards, so that e.g. Five beats Four.
My current implementation works, but I feel it is a little hacky and very inefficient in one particular detail and there must be a better way. In the Card class init, I am setting an attribute order which is just a list of the members of the CardValue enum, in the order they are declared.
While I could just use enum.auto(), or manually set the values as integers and hence simply use those for comparisons (still with eq etc), that would lose me the ability to make use of the enum values to form the human readable card value names.
Is there a better way to go about this than the below code? I feel like the comparison operators should be in the enum itself, but so far have not worked out how to get that to work with my meaningful values for the members of the enum.
from enum import Enum
from functools import total_ordering

class CardValue(Enum):
    TWO = "2"
    THREE = "3"
    ...
    ...
    KING = "King"
    ACE = "Ace"

@total_ordering
class Card:
    def __init__(self, value, suit):
        self.value = value
        self.suit = suit
        self.order = [en for en in CardValue] ### This seems wrong and very inefficient!

    def __eq__(self, other):
        if self.__class__ is other.__class__:
            return self.order.index(self.value) == self.order.index(other.value)

    def __lt__(self, other):
        if self.__class__ is other.__class__:
            return self.order.index(self.value) < self.order.index(other.value)

There are a couple choices, but the best is to record the value's index on Card creation:
self.order = value._member_names_.index(value.name)

Then your comparison can use self.order directly:
return self.order < other.order

_member_names_ is a supported attribute and won't be going away.


Picking from a list of individuals based on their fitness values I have this method that takes a list of my individuals (class has public field fitnessValue). I sum all fitnessValue and then draw a random value between 0 and sum of fitnessValue. In the end, I check for each individual where cumulative sum of fitnessValue passes the point I counted before and return the one which passed first.

public Individual pickRandomIndividual(List<Individual> pool) {
    Double totalScore = 0d;
    Double runningScore = 0d;
    for (Individual ind : pool) {
        totalScore += ind.fitnessValue;
    }

    Double rnd = Math.random() * totalScore;

    for (Individual ind : pool) {
        if (rnd >= runningScore &&
                rnd <= runningScore + ind.fitnessValue) {
            return ind;
        }
        runningScore += ind.fitnessValue;
    }
    return null;
}

I want to refactor this function with Java 8 streams. Any idea how can I achieve that?

I changed it a bit, into:

Double fitnessSum = genePool.stream()
                .mapToDouble(individual -> individual.fitnessValue)
                .sum();
Double runningScore = 0d;

Double rnd = random() * fitnessSum;

for (Individual g : genePool) {
    if (rnd >= runningScore && rnd <= runningScore + g.fitnessValue) {
            return g;
    }
    runningScore += g.fitnessValue;
}
return null;

Still, I do not know how to change cumulative sum part into a stream. Maybe entire logic is poorly implemented? I just want to draw one Individual from the list depending on how large the Individual's fitnessValue is.

You have to keep in mind that Streams are not the solution to everything. In this case for example, you need to keep track of multiple values, i.e. the current individual, and a cumulative sum (and you have to return a a value as soon as a certain condition is met). Yes, there are ways to do this with Streams, but I think in this case it is best to implement it with a simple loop like you have.

A couple of side notes:

Use the primitive type double instead of the wrapper class.
Declare variables as late as possible.
Perhaps allow the user to pass a Random instance.
I think your implementation can be simplified by subtracting, which would then mean you only have to check for < 0, e.g.:

double fitnessSum = genePool.stream()
        .mapToDouble(individual -> individual.fitnessValue)
        .sum();

double remainder = Math.random() * fitnessSum;
for (Individual gene : genePool) {
    remainder -= gene.fitnessValue;
    if (remainder <= 0.0) {
        return gene;
    }
}
return genePool.get(genePool.size() - 1);


Select recordIDs for Bill of Materials I have a hierarchy of parts in my database.  For example, one part (call it 0010102-0112-315) is a resistor.  It's used in a billion different top-level assemblies.  My final goal is to get all the top level product IDs related to the low level IDs (e.g. 0010102-0112-315).

My current approach is to left join the table onto itself 8 times (one past the max product depth), and when the recordId after is NULL then it must be a top level recordId.

However, the query takes almost 7 minutes to run, and I have a feeling I'm doing something wrong.

I can't use CTEs (to my knowledge) because we're using SQL Server 2003 (before CTEs).  Is there a better way of doing this?

Here's my current code:

SELECT I1.IMA_ItemID as ItemID1,
    I1.IMA_ProdFam as ProdFam1
    ,I1.IMA_RecordID as RecordId1 -- same as PSH.PSH_IMA_RecordID
    ,I2.IMA_RecordID as RecordId2
    ,I3.IMA_RecordID as RecordId3
    ,I4.IMA_RecordID as RecordId4
    ,I5.IMA_RecordID as RecordId5
    ,I6.IMA_RecordID as RecordId6
    ,I7.IMA_RecordID as RecordId7
    ,I8.IMA_RecordID as RecordId8
    ,I9.IMA_RecordID as RecordId9
FROM   Item AS I1
    left join ProductStructureHeader AS PSH1 ON PSH1.PSH_IMA_RecordID = I1.IMA_RecordID
    left join ProductStructure AS PS1 on PS1.PST_PSH_RecordID = PSH1.PSH_RecordID
    left join Item AS I2 ON I2.IMA_RecordID = PS1.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH2 ON PSH2.PSH_IMA_RecordID = I2.IMA_RecordID
    left join ProductStructure AS PS2 on PS2.PST_PSH_RecordID = PSH2.PSH_RecordID
    left join Item AS I3 ON I3.IMA_RecordID = PS2.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH3 ON PSH3.PSH_IMA_RecordID = I3.IMA_RecordID
    left join ProductStructure AS PS3 on PS3.PST_PSH_RecordID = PSH3.PSH_RecordID
    left join Item AS I4 ON I4.IMA_RecordID = PS3.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH4 ON PSH4.PSH_IMA_RecordID = I4.IMA_RecordID
    left join ProductStructure AS PS4 on PS4.PST_PSH_RecordID = PSH4.PSH_RecordID
    left join Item AS I5 ON I5.IMA_RecordID = PS4.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH5 ON PSH5.PSH_IMA_RecordID = I5.IMA_RecordID
    left join ProductStructure AS PS5 on PS5.PST_PSH_RecordID = PSH5.PSH_RecordID
    left join Item AS I6 ON I6.IMA_RecordID = PS5.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH6 ON PSH6.PSH_IMA_RecordID = I6.IMA_RecordID
    left join ProductStructure AS PS6 on PS6.PST_PSH_RecordID = PSH6.PSH_RecordID
    left join Item AS I7 ON I7.IMA_RecordID = PS6.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH7 ON PSH7.PSH_IMA_RecordID = I7.IMA_RecordID
    left join ProductStructure AS PS7 on PS7.PST_PSH_RecordID = PSH7.PSH_RecordID
    left join Item AS I8 ON I8.IMA_RecordID = PS7.PST_IMA_RecordID

    left join ProductStructureHeader AS PSH8 ON PSH8.PSH_IMA_RecordID = I8.IMA_RecordID
    left join ProductStructure AS PS8 on PS8.PST_PSH_RecordID = PSH8.PSH_RecordID
    left join Item AS I9 ON I9.IMA_RecordID = PS8.PST_IMA_RecordID

Some other obscure facts that might be useful:

RecordId1 above is the highest level RecordId
Selecting * where IMA_ItemStatusCode = 'Active' and IMA_ItemTypeCode = 'Purchased Item' from the Item table gives the lowest level items
Can't think of anything else, but feel free to ask as there might be something.

Here are a couple of Questions:

Have you tried reverse order in your joins?  I.e. start with PS=>PSH=>Item.
Have you tried inner joins versus left join?  Could see speed improvement (less data for sure)
Is there any way to recurse only on item?  In other words, can you get PS1.PST_IMA_RecordID from your Item row?  If so, then you would only recurse on one table.


Creating Pascal's Triangle when specified number of rows Does anyone have any tips on how I could improve my code

function pascal(row){
 for(var k = 0; k<=row; k++){
  var pascalRow = [1];
  for(var i = 1; i<=k; i++){
   pascalRow.push(pascalRow[i-1]*(k+1-i)/i)
  }
  console.log(pascalRow)
 }
}
pascal(/*Insert # of rows here */)

Nine line code review.

Apart from some style and layout issues your code is good.

Style

If you are to use var to declare variable you should put them at the top of the function. Or you can use let which defines block scoped variables and you put them where you need them. Block scope means that the variable is only available inside the block it is declared for.
Put spaces between operators +, -, =, /, *, ===, <= etc. as it becomes very hard to read code when numbers and variables are all one continuous string. Making code more readable makes it easier to spot bugs.
Don't repeat, and use contextual information to reduce name size. For example, you had pascalRow which is a little too much information in a function that is called pascal, row holds the same information regarding what the variable references.
The function name however does not give much information about what it does and is a rather bad name. Maybe pascalsTriangle would be clearer.
The argument row is singular, the custom is a plural for counts though rows would imply an array so something like numberRows or rowCount would be more fitting
The custom for loop variable names are i then j, then k with i as the outer loop and then inner. Loop variable names and position is not a biggy in the book of buggy, bad, code; consistency is very important. Using the idiomatic styles helps you stay consistent.
Javascript has automatic semicolon insertion (ASI). The rule is only use ASI when you can list off the top of your head every way it can break your code. Add the semicolon- it will save you a ton of WTF bug hunting.
I assume you are a beginner, and to help improve your code you should always add the compiler/interpreter directive "use strict"; as the first line of any JS file or script tag. It will catch may common mistakes with an error and save you from some of the hardest to catch bugs. It will also run the code faster.

Rewrite.

So with the above in mind, here are two rewrites. The first uses var

"use strict";
function pascalsTriangle(rowCount) {
    var i, j, row;
    for (i = 0; i <= rowCount; i++) {
        row = [1];
        for (j = 1; j <= i; j++) {
            row.push(row[j - 1] * (i + 1 - j) / j);
        }
        console.log(row);
    }
}

The version using let and const:

"use strict";
function pascalsTriangle(rowCount) {
    for (let i = 0; i <= rowCount; i++) {
        const row = [1];
        for (let j = 1; j <= i; j++) {
            row.push(row[j - 1] * (i + 1 - j) / j);
        }
        console.log(row);
    }
}

Improve?

Good coders are always trying to find ways to improve the code and the app. Here are some simple improvements to both the algorithm and output. This is meant just as an example and to introduce some new syntax.

Optimise

You may notice that each row is mirrored so you can save some computation by only doing half the row. Reversing an array (simple in Javascript) is not always the best option. Using two arrays for the left and right sides reduces the work done under the hood. For even rows you need to remove the duplicate center number.

Safeguard

If abused, your function can block a page and likely crash it when memory runs out.

The argument for the number of rows can have have any value. Generally you would vet such a value, making sure its a number, and that the number is not too big. Takes a long time and a lot of memory to compute pascalsTriangle(Infinity)

So a simple argument check at the start check for a number and sets an upper triangle size. For bad numbers the count defaults to 10, and the max count is 40.

User friendly

Console is not a nice output, its slow and the formatting is not always what you want (And its is even worse when the in-page console is used). It would be better to create the triangle as a string and output the result after the computation is complete so that you can use the complete triangle to help set out the numbers.

"use strict";

function pascalsTriangle(rowCount) {
    const tri = []; // Hold all rows
    rowCount = Math.min(40, isNaN(rowCount) ? 10 : rowCount);
    for (let i = 0; i <= rowCount; i++) {

        // arrays for left and right sides
        const left = [1];
        const right = [1];
        for (let j = 1; j <= i / 2; j++) {  // only do half a row
           const val = left[j - 1] * (i + 1 - j) / j;
           left.push(val);
           right.unshift(val);
        }
        if (i % 2 === 0) { right.shift() } // remove extra num for even rows

        // concat the two arrays and join the result as a string push to the tri
        tri.push([...left, ...right].join(" : "));
   }

  // Display as triangle
  const maxLen = tri[tri.length - 1].length;
  tri.forEach(row => {
      const pad = ((maxLen - row.length) / 2);
      display.innerHTML += row.padStart(row.length + pad, " ") + "
";
  });

}

pascalsTriangle(12)


Increment values in a dictionary In my continued venture to teach myself C# I came across an exercise using Dictionary.  I created a class that has a method.  In that method it creates a dictionary with keys/values.  A string is passed into that method.  If any matching character in that string correlates with a key we increment the value of that key, else we throw an ArgumentException.  This is my code an my first real attempt and working with a dictionary in C#.

I am interested to see if there is a better, concise, an/or faster way to do the same task.  Following is my code and following that is the test.  Test can not be altered.  I tried turning foreach into linq without luck so that would be interesting to see that as well if you know how or recommend it.

For more information My Github link to this exercise.

using System;
using System.Collections.Generic;

public static class NucleotideCount
{
    public static IDictionary<char, int> Count(string sequence)
    {
        Dictionary<char, int> Dna = new Dictionary<char, int>()
        {
            {'A', 0},
            {'C', 0},
            {'G', 0},
            {'T', 0}
        };

        foreach (char c in sequence)
        {
            switch (c)
            {
                case 'A': Dna[c]++; break;
                case 'C': Dna[c]++; break;
                case 'G': Dna[c]++; break;
                case 'T': Dna[c]++; break;
                default : throw new ArgumentException("Strand has invalid nucleotides.");
            }
        }

        return Dna;
    }
}

// This file was auto-generated based on version 1.3.0 of the canonical data.

using System;
using System.Collections.Generic;
using Xunit;

public class NucleotideCountTests
{
    [Fact]
    public void Empty_strand()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 0,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count(""));
    }

    [Fact]
    public void Can_count_one_nucleotide_in_single_character_input()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 1,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count("G"));
    }

    [Fact]
    public void Strand_with_repeated_nucleotide()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 7,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count("GGGGGGG"));
    }

    [Fact]
    public void Strand_with_multiple_nucleotides()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 20,
            ['C'] = 12,
            ['G'] = 17,
            ['T'] = 21
        };
        Assert.Equal(expected, NucleotideCount.Count("AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC"));
    }

    [Fact]
    public void Strand_with_invalid_nucleotides()
    {
        Assert.Throws<ArgumentException>(() => NucleotideCount.Count("AGXXACT"));
    }
}

There is no need for the switch (c). Instead, check to see if c is in the keys of the dictionary using ContainsKey; if not, throw. After that, unconditionally Dna[c]++;.

if (!Dna.ContainsKey(c))
    throw new ArgumentException("Strand has invalid nucleotides.");
Dna[c]++;


Basic calculator that calculates result based on operands using a class Question:

  Create a basic calculator using classes that given 3 inputs - number 1, number 2 and operator -  calculates the result.

class calculator():

    def __init__(self,value1 , value2, operand):
        self.value1 = int(value1)
        self.value2 = int(value2)
        self.operand = operand

    def calculate (self):
        if self.operand == '+' :
            return self.value1 + self.value2

        if self.operand == '*':
            return self.value1 * self.value2

        if self.operand == '-' :
            return self.value1 - self.value2

        if self.operand == '/' :
            if self.value2 == 0:
                raise ZeroDivisionError
            else:
                return self.value1 / self.value2

r = calculator(6,7, '+')
print r.calculate()

r = calculator(3,7, '-')
print r.calculate()

r = calculator(6,0, '*')
print r.calculate()

r = calculator(9,5, '/')
print r.calculate()

How can i make this function better? Is this a correct implementation of a class?

There is no need to manually raise ZeroDivisionError. Python will do it for you anyway.
Rather than inspecting an operand with the bunch of ifs, create a dictionary of operand, lambda:

ops = {
    '+': lambda x,y: x + y,
    ....
}

and calculate the result as

ops[operand](value1, value2)

Such dictionary (initialized in a constructor) is the only justification for making a class. It doesn't feel right to make an operator and values class members. Consider

r = calculator()
r.calculate(1, 2, '+')
r.calculate(3, 4, '*')

etc.


Upload image with secure code I have written [PHP code][ to upload image/s to server.

(To see the result, open the JavaScript console of your browser, look at the Network tab or the Console tab.)

The code is also available and runnable on paste.ofcode.org.

(Upload images)

I would like to know if this code is secure for uploading valid images.

<?php
$IMAGE_SIZE = 2097152; // 2mb ht tp:/ /ww w.convertunits.com/from/MB/to/byte
$MAX_IMAGES = 30;
if(!empty($_FILES['fileToUpload']['name'][0])) {
    foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){}
    if($key<$MAX_IMAGES){
        foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){
            if($imgLocation){//filename exist
                $imageName = $_FILES["fileToUpload"]['name'][$key];
                if(checkMime($imgLocation)&&
                    checkExtension($imageName)&&
                    checkByExif($imgLocation)&&
                    checkSize(filesize($imgLocation))&&
                    checkImageName($imageName)){
                    echo $usr_dir = "img/".changeImageName($imageName).".".getExtension($imageName);
                    if(move_uploaded_file($imgLocation, $usr_dir)){
                        echo "
success full  uploaded
";
                    }else{
                        echo "
 File Not uploaded
";
                    }
                }else{  echo "
Invalid File
";}
            }else{echo " No Image uploaded";}
        }
    }else echo "
 - Only 30 Images are allowed -
";
}else{
    echo "No Image Is selected";
}
function handleError($msg){
    //return json_encode();
}
function checkImageName($imagename){
    $imagename = pathinfo($imagename, PATHINFO_FILENAME);
    if(strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename){
        if (strlen($imagename) >= 1 && strlen($imagename)<= 30){
            return true;
        }else{
            return false;
        //HandleError('Invalid file name');
        }
    }else{
        return false;
    }
}
function issetImage($image_tmpName){
    return (file_exists($image_tmpName) || is_uploaded_file($image_tmpName));//return ture and false
}
function checkMime($image_tmpName){
    if(issetImage($image_tmpName)){
        $whitelist = array("image/gif","image/png","image/jpeg","image/jpg","image/bmp");
        $imageinfos = getimagesize($image_tmpName);
        $imageTypes = $imageinfos['mime'];
        if(in_array($imageTypes, $whitelist)){
            return true;
        }else{
            return false;
        }
    }
    else{
        return false;
    }
}
function checkExtension($imageName){
    $ext = getExtension($imageName);
    if(strtolower($ext) == "jpg" || strtolower($ext) == "png" || strtolower($ext) == "jpeg" || strtolower($ext) == "gif" || strtolower($ext) == "bmp"){
        return true;
    }else{
        return false;
    }
}
function checkByExif($image_tmpName){
    if(issetImage($image_tmpName)){
        $image_type_allowed = array(IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_BMP);
        $filetype =exif_imagetype($image_tmpName);
        if (in_array($filetype, $image_type_allowed)){
            return true;
        }else{
            return false;
        }
    }else{
        return false;
    }
}
function checkSize($imageSize){
    return ($imageSize<= $GLOBALS["IMAGE_SIZE"] && $imageSize>=1) ? true : false;
}
function changeImageName($imgName){
    return md5(pathinfo($imgName, PATHINFO_FILENAME))."_".time();
}
function getExtension($fullImageName){
    if($fullImageName)
        return pathinfo($fullImageName,PATHINFO_EXTENSION);
}
?>

Security

The echo should be vulnerable to XSS via the file extension (but it's not exploitable - at least not easily; Still, I would HTML encode it when echoing).

Otherwise it looks good to me. Regarding the various checks:

checkMime: Can easily be bypassed. Checking the mimetype is generally not very secure, so it doesn't matter that much, but as defense in depth it's not a bad idea. getimagesize isn't a good function for this though, finfo_file generally gives better results. [see eg here]
checkExtension: This is the important check. There are no known vulnerabilities for pathinfo, so this should be secure.
checkByExif: Can very easily be bypassed, as exif_imagetype only checks the first bytes. It's not a security check in any way, but may help with usability (although in this case I don't see the use for it).
checkSize: important check, should be secure.
checkImageName: Not needed as you change the image name anyways. The function itself looks pretty bad though; strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename isn't very readable.

To add additional security as defense in depth, you could move the upload dir outside the web root and check the content of the file (eg that it doesn't contain opening PHP tags).

Returning Early

Your ifs are quite nested. If you return early, you can avoid this, and make it more obvious what condition returns what error. Eg:

if(empty($_FILES['fileToUpload']['name'][0])) {
    return "No Image Is selected";
}
if(too many images){
    return "Only 30 Images are allowed";
}
foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){
    uploadImage($_FILES["fileToUpload"]['tmp_name'][$key], $imgLocation);
}

checkImageName

Instead of this:

function checkImageName($imagename){
    $imagename = pathinfo($imagename, PATHINFO_FILENAME);
    if(strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename){
        if (strlen($imagename) >= 1 && strlen($imagename)<= 30){
            return true;
        }else{
            return false;
        //HandleError('Invalid file name');
        }
    }else{
        return false;
    }
}

You can use this:

function checkImageName($imagename){
    if (preg_match("/[\\\\\/?%*' :|\"<>]/", $imagename)) {
       return false;
    }
    return strlen($imagename) >= 1 && strlen($imagename) <= 30;
}

preg_match is a more fitting function, as you want to see if the string matches certain characters, and this also gets rid of the nested if structure.

Personally, I would prefer a whitelist check on a-zA-Z0-9 here.

Misc

why do you have an empty foreach loop? Seems unnecessary.
I would add a function for the upload of a single image.
Your formatting could use some work. Always use curly brackets, always put statements on their own line, be more consistent with your spacing, etc. Any IDE can fix these issues for you.
be more consistent with your variable names. Eg imagename vs imageName vs image_tmpName, imageName vs imgLocation, etc.


Temperature and currency converters in OOP JS I just started learning OOP from Mosh on YouTube and I looked for easiest exercises to practice. Can I get a feedback on my code please? What I did wrong and what I should change?

In the code there are two exercises: the first one is a temperature converter and the second one is a currency exchange.

let cel = "";
let fah = "";

function Termometer(kel) {
  this.kel = kel;

  this.getCelsius = function() {
    cel = this.kel - 273;
    return cel;
  },

  this.getFahrenheit = function() {
    fah = Math.floor(cel * (9/5) + 32);
    return fah;
  },

  this.init = function() {
    this.getCelsius();
    this.getFahrenheit();
  }
}

// currency exchange
var eur = "";
var usd = "";
var gbp = "";
function Valuta(sum1, sum2, sum3) {
  this.sum1 = sum1;
  this.sum2 = sum2;
  this.sum3 = sum3;

  this.getEur = function() {
    eur = Math.floor(this.sum1 / 4.72);
  };

  this.getUsd = function() {
    usd = Math.floor(this.sum2 / 4.15);
  };

  this.getGbp = function() {
    gbp = Math.floor(this.sum3 / 5.27);
  };

  this.init = function() {
    this.getEur();
    this.getUsd();
    this.getGbp();
  }
}

var valuta = new Valuta(300, 300, 300);
valuta.init();
console.log(eur, usd, gbp)

0. Little Typo

thermometer instead of termometer.

1. Global Variables

  let cel = "";
let fah = "";

Variables inside a global scope can be changed from everywhere.

1.1 Example

Imagine temperature.js is your script with the global variables inside. Now someone adds a table.js to display tables on the html page.

<script src="temperature.js"></script>
<script src="table.js"></script>

One line in table.js is cel = "<td>...</td>". The author of the code has accidentally a spelling mistake and writes cel instead of cell to indicate a table cell and breaks with this line your code in temperature.js.

1.2 Quick fix

function Termometer(kelven) {
    this.kelven = kelven;
    this.cel;
    this.fah;

    /* ... */
}

With this fix we binde the variables to Thermometer. To access them you need to create an instance of Thermometer.

const thermometer = new Thermometer(0)
console.log(thermometer.cel)

This means that inside table.js the author needs to write explicite thermometer.cel = ... to break your code.

2. Intuitive API

  const thermometer= new Thermometer(0)

thermometer.init()

console.log(thermometer.getCelsius())
console.log(thermometer.getFahrenheit())

To call thermometer.init() feels so wrong and it is not intuitive.

2.1 Quick fix

When new Thermometer(0) gets called, in the background a new instance gets build by the javascript runtime and it initializes all variables inside the instance.

To avoid the init method we can store the calculation in variables:

function Thermometer(kelven) {
    this.kelven = kelven;
    this.celsius = this.kelven - 273
    this.fahrenheit = Math.floor(this.celsius * (9 / 5) + 32)

    this.getCelsius = function() { return this.celsius }
    this.getFahrenheit = function() { return this.fahrenheit }
}

The code snipped above looks more intuitive with ES6-Classes:

class Thermometer {
    constructor(kelven) {
        this.kelven = kelven
        this.celsius = this.kelven - 273
        this.fahrenheit = Math.floor(this.celsius * (9 / 5) + 32)
    }

    getCelsius() { return this.celsius }
    getFahrenheit() { return this.fahrenheit }
}

Now, we can can interact with it like:

const thermometer = new Thermometer(0)

console.log(thermometer.getCelsius())
console.log(thermometer.getFahrenheit())

3. Unflexible

  function Thermometer(kel) {
   this.kel = kel

   this.getCelsius = function () {
       cel = this.kel - 273;
       return cel;
   }

   /* ... */
}

Currently the Thermometer only works for kelvin..

It would be great to archive that we can pass in celcius and it still works!

3.1 Common Interface

That this works all Units needs to share a common api that gets called by Thermometer. The Thermometer could call methods or properties like inCelcius, inFahrenheit and inKelvin.

function Thermometer(temperature) {
    this.getCelsius = function () { return temperature.inCelsius }
    this.getFahrenheit = function () { return temperature.inFahrenheit }
    this.getKelvin = function () { return temperature.inKelvin }
}

Based on that we can create our units:

function Kelvin(value) {
    this.inKelvin = value
    this.inCelsius = value - 273
    this.inFahrenheit = Math.floor(this.inCelsius * (9 / 5) + 32)
}

function Fahrenheit(value) {
    this.inKelvin = Math.floor((value + 459) * (9 / 5))
    this.inCelsius = Math.floor((value - 32) / (9 / 5))
    this.inFahrenheit = value
}

function Celsius(value) {
    this.inKelvin = value + 273
    this.inCelsius = value
    this.inFahrenheit = Math.floor(value * (9 / 5) + 32)
}

Now the Thermometer works for all units:

const kelvinMeter = new Thermometer(new Kelvin(0))
const fahrenMeter = new Thermometer(new Fahrenheit(0))
const celsiMeter = new Thermometer(new Celsius(0))

4. Unexpected Behavior

  this.getEur = function() {
   eur = Math.floor(this.sum1 / 4.72);
}

When I would call valuta.getEur() I would expect that the method getEur returns a value. Instead it returns undefined and changes a global variable.

const valuta = new Valuta(1, 2, 3)
const euro = valuta.getEur() // undefined


Correctness of quick sort in C What do you think of this piece of code ? This is an implementation of quick sort in C, but I'm not sure about the quality and correctness.

#include <stdio.h>

void swap(int tab[], int a, int b)
{
  int temp = tab[a];
  tab[a] = tab[b];
  tab[b] = temp;
}

void quickSort(int tab[], int begin, int end)
{
  int left = begin-1;
  int right = end+1;
  const int pivot = tab[begin];

  if(begin >= end)
    return;

  while(1)
    {
      do right--; while(tab[right] > pivot);
      do left++;  while(tab[left] < pivot);

      if(left < right)
        swap(tab, left, right);
      else break;
    }

  quickSort(tab, begin, right);
  quickSort(tab, right+1, end);
}

int main(void)
{
    int tab[5] = {5, 3, 4, 1, 2};
    int i;

    quickSort(tab, 0, 4);

    for(i = 0; i < 5; i++)
    {
        printf("%d ", tab[i]);
    }
    putchar('\n');

    return 0;
}

Is it the standard way of implementing a quick sort in C ?

The code is not a bad implementation of Quicksort but it has a flaw, inherent to the Quicksort algorithm, that you can easily fix.  I tested the code with this main:

const int testsize = 100000;

int main(void)
{
    int tab[testsize];
    int i;

    for(i = 1; i < testsize; i++)
        tab[i] = i;

    tab[0] = testsize+1;
    quickSort(tab, 0, testsize-1);
    return 0;
}

As you can see, this is deliberately the worst case scenario for Quicksort in which it takes \$O(n^2)\$ time to sort.  This code took 11.85 seconds on my machine.  Quicksort works best on randomly arranged data, so paradoxically, you can often actually improve sorting time by randomly shuffling the data before you start.  I wrote this rather simple-minded shuffle routine:

void shuffle(int tab[], int begin, int end)
{
    for (; begin < end/2; ++begin)
        swap(tab, begin, rand()%end);
}

When I inserted a call to shuffle() just before the call to quickSort() in main, the time to sort the same data dropped to 0.026 seconds.  By comparison, the standard qsort() sorts in 0.006 seconds on this same machine.

Also, more generally, I'd be inclined to pass two pointers to swap rather than an array and two indices.  The code may be very slightly faster, but it's certainly more general.

The code could also be made more general by using the same parameters as the qsort() routine in <stdlib.h> but then, that's already written. :)


Finding k largest (or smallest) elements in an array I have written this code for the question:

  Finding k largest (or smallest) elements in an array

My approach uses a temporary array. Please suggest any improvements for readability or better performance.

public static void kLargest(int[] array, int k){

    if(k < 0 || k > array.length){
        return;
    }
    int[] temp = new int[k];

    for(int i = 0; i < k; i++){
        temp[i] = array[i];
    }
    for(int i = k; i < array.length; i++){
        int min = Integer.MAX_VALUE;
        int minIndex = 0;
        for(int j = 0; j < temp.length; j++){
            if(temp[j] < min){
                min = temp[j];
                minIndex = j;
            }
        }
        if(array[i] > min){
            temp[minIndex] = array[i];
        }
    }
    for(int i : temp){
        System.out.print(i + " ");
    }
}

You can write it without extra space and with one less variable. Instead of considering a new array (temp array), do all the swap in the original array; in this case, you will skip O(k) extra space. Also, there is no need to keep track of min value. Keep tracking of min index and update it in every round will be enough.

public static void kLargest2(int[] array, int k){

    int minIndex = 0, i;                            //Find Min

    for (i = k; i < array.length; i++){
        minIndex = 0;
        for (int j = 0; j < k; j++){
            if(array[j] < array[minIndex]){
                minIndex = j;
                array[minIndex] = array[j];
            }
        }
        if (array[minIndex] < array[i]){         //Swap item if min < array[i]

            int temp = array[minIndex];
            array[minIndex] = array[i];
            array[i] = temp;
        }
    }
    for (int q = 0; q < k; q++){                            //Print output
        System.out.print(array[q] + " ");
    }
}


Printing an array in columns I'm trying to define a function which takes an array and prints it to a predefined total width in as many columns as needed. Think of the way ls prints files for example.

Here is the kind of output I am looking for:

$ ./print-columns.py

Files
    /home/stephen/dev/src/cmake-browser/cmakeclient.cpp    /home/stephen/dev/src/cmake-browser/projectsources.cpp
    /home/stephen/dev/src/cmake-browser/main.cpp           /home/stephen/dev/src/cmake-browser/includesmodel.cpp
    /home/stephen/dev/src/cmake-browser/mainwindow.cpp     /home/stephen/dev/src/cmake-browser/definesmodel.cpp
    /home/stephen/dev/src/cmake-browser/debugwidget.cpp    /home/stephen/dev/src/cmake-browser/highlighter.cpp
    /home/stephen/dev/src/cmake-browser/projecttree.cpp    /home/stephen/dev/src/cmake-browser/helpviewer.cpp
    /home/stephen/dev/src/cmake-browser/projectmodel.cpp   /home/stephen/dev/src/cmake-browser/main.cpp
    /home/stephen/dev/src/cmake-browser/projectcode.cpp    /home/stephen/dev/src/cmake-browser/helpviewer.cpp
    /home/stephen/dev/src/cmake-browser/projectdetail.cpp

Targets
    KF5ItemModels                    kdescendantsproxymodel_smoketest klinkitemselectionmodeltest
    krecursivefilterproxymodeltest   kselectionproxymodel_smoketest   kselectionproxymodeltest
    proxymodeltestapp                proxymodeltestsuite              testmodelqueuedconnections

I created the following script to do that. I'm trying to brush up on my Python, and re-learn how to write pythonic code in the most modern way (latest Python 3), and it doesn't feel very pythonic (and it's not the kind of code I usually write).

#!/usr/bin/env python3

import sys, subprocess, json, math

def col_print(title, array):
  print()
  print()
  print(title)

  indent = " " * 4

  if not array:
    print(indent + "<None>")
    return

  padsize = 1
  # Hardcoded for this example
  termwidth = 150

  maxitemwidth = len(max(array, key=len))

  numCols = int((termwidth + padsize) / (maxitemwidth + padsize))

  numRows = int(math.ceil(len(array) / float(numCols)))

  pad = " " * padsize

  for index in range(numRows):
    print(indent + pad.join(item.ljust(maxitemwidth) for item in array[index::numRows]))

col_print("Files",
          ["/home/stephen/dev/src/cmake-browser/cmakeclient.cpp",
          "/home/stephen/dev/src/cmake-browser/main.cpp",
          "/home/stephen/dev/src/cmake-browser/mainwindow.cpp",
          "/home/stephen/dev/src/cmake-browser/debugwidget.cpp",
          "/home/stephen/dev/src/cmake-browser/projecttree.cpp",
          "/home/stephen/dev/src/cmake-browser/projectmodel.cpp",
          "/home/stephen/dev/src/cmake-browser/projectcode.cpp",
          "/home/stephen/dev/src/cmake-browser/projectdetail.cpp",
          "/home/stephen/dev/src/cmake-browser/projectsources.cpp",
          "/home/stephen/dev/src/cmake-browser/includesmodel.cpp",
          "/home/stephen/dev/src/cmake-browser/definesmodel.cpp",
          "/home/stephen/dev/src/cmake-browser/highlighter.cpp",
          "/home/stephen/dev/src/cmake-browser/helpviewer.cpp",
          "/home/stephen/dev/src/cmake-browser/main.cpp",
          "/home/stephen/dev/src/cmake-browser/helpviewer.cpp"]
)

col_print("Targets",
          ["KF5ItemModels",
          "krecursivefilterproxymodeltest",
          "proxymodeltestapp",
          "kdescendantsproxymodel_smoketest",
          "kselectionproxymodel_smoketest",
          "proxymodeltestsuite",
          "klinkitemselectionmodeltest",
          "kselectionproxymodeltest",
          "testmodelqueuedconnections"]
)

Can you help write a better col_print function?

There are a few style changes in Python.
The main one is Python uses 4 spaces, not 2, as it's standard indent size.
This is important as indent size matters in Python.
Also most variables are snake_case.
So my code examples may have small changes.

I would recommend splitting the displaying and formatting into separate functions.
This is as then you can manipulate the output. Which can be nice.

This means that you would have to change your returns.

Changing this allows you to remove the two empty lines at the beginning,
for having to explicitly call print.
And you can print both files and targets at the same time:

print("\n\n".join([
    col_print("Files",
        [...]
    ),
    col_print("Targets",
        [...]
    )
]))

You could pass termwidth and padsize as default values.
This allows you to change them if you wish at a later point.

def col_print(title, array, pad_size=1, term_width=150):
    ...

You may want to change maxitemwidth so there is only one len.
It makes the code slightly easier to understand.
This is as I originally was a bit confused when I saw the len(max(...)).

max_item_width = max(map(len, array))

Division changed in Python3.

Rather than doing int(a / b) you can do a // b.
Python2 division can be nice at times, and so they kept it in.

Python3 division automatically casts a and b to floats.

In short this means that you don't have to do things like a / float(int(b / c)).
Instead you can do a / (b // c). Which is much more readable.

At this point the code looks quite nice:

def col_print(title, array, term_width=150, pad_size=1):
    indent = " " * 4
    pad = " " * pad_size
    title += "\n"

    if not array:
        return title + indent + "<None>"

    max_item_width = max(map(len, array))
    num_rows = int(math.ceil(len(array) / ((term_width + pad_size) // (max_item_width + pad_size))))

    return title + "\n".join(
        indent + pad.join(item.ljust(max_item_width) for item in array[index::num_rows])
        for index in range(num_rows)
    )


Simple XOR file encryption In https://stackoverflow.com/a/39299060, I wrote the following program.

#include <fstream>
#include <iostream>
#include <string>

static std::string str_xor(const std::string &data, const std::string &key) {
  std::string result(data.size(), '\0');

  for (std::size_t i = 0, i < data.size(); i++) {
    result[i] = data[i] ^ key[i % key.size()];
  }
  return result;
}

Is it idiomatic C++ to evaluate data.size() in every iteration of the loop? Same for key.size().
Is the use of std::size_t ok here, or do I really have to write std::string::size_type?
Can I take advantage of move semantics here? I don't know them yet.

int main(int argc, char **argv) {
  if (argc != 3) {
    std::cerr << "usage: xor <datafile> <keyfile>\n";
    return 1;
  }

  std::ifstream data_in(argv[1]);
  std::string data(
    (std::istreambuf_iterator<char>(data_in)),
    (std::istreambuf_iterator<char>()));
  data_in.close();

The error checking is missing here. I would like the code for error checking to be as brief as possible. Ideally one or two lines that check for all errors at once. Is that still possible after closing the stream?

  std::ifstream key_in(argv[2]);
  std::string key(
    (std::istreambuf_iterator<char>(key_in)),
    (std::istreambuf_iterator<char>()));
  key_in.close();

  std::string result = str_xor(data, key);

  std::ofstream data_out(argv[1]);
  data_out << result;
  data_out.close();

  return 0;
}

Anything else?

This strikes me as the sort of code that's likely to make a lot of old C programmers cringe and think that C++ programmers are wasteful. It reads in the entire input file at once, even though it only ever looks at one byte of that file at any given time. Then it creates an output string the same size as the input file (and writes zero bytes to the whole thing). So, if your input file is, say, 20 megabytes, this allocates a minimum of 40 megabytes of memory to process it.

Along with that, it writes its output over the input file. If (for whatever reason) processing stops in the middle, you've destroyed the data. Your file might have some of the encrypted data, or it might have nothing at all.

So, for a moment, let's consider how that C programmer would probably do the job, and then see if we can't do something that's at least sort of close to the same, but with (perhaps) a little more abstraction.

If I were a C programmer, I'd probably do something like this:

FILE *infile = fopen(argv[1]);
FILE *keyfile = fopen(argv[2]);
FILE *outfile = fopen(argv[3]);

char key[BUFFER_SIZE];

size_t key_size = fread(infile, 1, BUFFER_SIZE, keyfile);

size_t current = 0;

int ch;

while ((ch=getchar(infile)) != EOF) {
    putchar(ch ^ key[current], outfile);
    current = (current + 1) % key_size;
}

This reads and processes one character at a time from the input file. Even if the input file is several gigabytes, it allocates only a small, fixed amount of memory.

At the same time, the C streams do actually do buffered reading and writing, so even though this may not be the most efficient code possible, it's usually going to be reasonably fast nonetheless.

The biggest drawback here is that we've set a fixed maximum size for the key. If the user supplies a short key, we're probably wasting quite a bit of memory. If they supply too long of a key, we use only part of it so we lose some security (though, in fairness, unless the key is huge this has no real security to lose).

So what we'd like would be to read the entire key into a string (so we get the whole thing, regardless of size, without wasting a lot of extra space. Other than that, we'd like to process the data directly from input to output, and let the iostreams handle buffering enough to make that reasonably efficient.

At the same time, we'd like a nice iterator-based model for most of the processing. Since we're taking some input, transforming it, and writing the result to an output, we can use std::transform to do most of the dirty work.

The place iterators break down is in working with our key. Normally iterators go from the beginning of a collection to the end (or vice versa), but in this case we need it to (potentially) iterate through the collection holding the key many times. To do that, we can write ourselves a custom iterator, something like this:

template <class Container>
class mod_iterator {
    using value_type = typename Container::value_type;
    using it = typename Container::const_iterator;

    it begin;
    it end;
    it current;
public:
    mod_iterator(Container const &c) : begin(c.cbegin()), end(c.cend()), current(c.cbegin()) { }
    value_type operator*() { return *current; }
    mod_iterator &operator++() { ++current; if (current == end) current = begin; return *this; }
};

To make it a little more convenient to use that, we can add a little builder function template:

template <class T>
mod_iterator<T> make_mod_iterator(T const &t) { return mod_iterator<T>(t); }

To avoid overwriting the input file, we can open a third file to hold the encrypted data. Once we've done the (boring) work of opening all three files, and reading the key into a string, the processing looks something like this:

auto m = make_mod_iterator(key);

std::transform(std::istreambuf_iterator<char>(data_in), {},
    std::ostreambuf_iterator<char>(data_out),
    [&](char c) { char ret = *m ^ c; ++m; return ret; }
);

One minor addition: you want to specify std::ios::binary when you open the file of the encoded data, because the data you produce may no longer be a proper text file. Since you can use the same code to either encrypt or decrypte, that means you want to specify it for both the input file and the output file:

std::ifstream data_in(argv[1], std::ios::binary);
std::ofstream data_out(argv[3], std::ios::binary);

then the processing looks something like this:

auto m = make_mod_iterator(key);

std::transform(std::istreambuf_iterator<char>(data_in), {},
    std::ostreambuf_iterator<char>(data_out),
    [&](char c) { char ret = *m ^ c; ++m; return ret; }
);


Get properties from all children in a recursive SQL structure In our product, Company is a model that can have many Companies belonging to it; so some Companies are parents, some are children, and many are both.

We have a helper method Company.root that has as its children all Companies that do not actually have a parent; in this way, accessing Company.root and recursively iterating over the children will touch every Company in the database.

For context, 'branches' are just Companies with a special flag set (namely that they cannot have children of their own).

We have a method Company#get_deep_props that makes one SQL call per Company below the queried Company. This has abysmal performance on some pages, and for instance when we need to do Company validation we need to check other companies in the chain for a few different values to ensure pseudo-uniqueness (can not make the column unique).

How can we improve this performance of this method? Can we cache the companies on the first call and use the ActiveRecord::Selection in subsequent calls? Not all properties of Companies are in the database (some are computed) so I don't think we can replace the code with a (sufficiently complex) SQL query in ALL cases, but we might be able to do it for some.

has_many :subdealers, class_name: "Company", foreign_key: "dealer_id"

def children
  root ? Company.where("dealer_id IS NULL") : subdealers
end

def get_deep_props(prop, options = {})
  prop            = prop.to_sym
  default_options = { :with_branches => true }
  options         = default_options.merge(options)
  child_props     = []

  children.each do |child|
    next if (child.branch? && !options[:with_branches])
    child_props.concat(child.get_deep_props(prop, options))
  end

  case prop
  when :dba_tags
    child_props.concat(read_attribute(prop).split)
  when :branches
    child_props.concat(branches)
  when :terminals
    child_props.concat(terminals)
  when :employees
    child_props.concat(employees)
  when :self
    child_props.push(self)
  else
    child_props.push(read_attribute(prop))
  end

  child_props
end

In the database, Company's dealer_id is the id of the parent Company, or null if the company has no parent.

There is more than one way to represent a tree and they have different pro's and con's.

In your case you may want to look at modified preorder tree traveral. You can read more about the algorithm here for example.

By giving all nodes a proper left and right value pair you can load an entire subtree in one query for example, which would improve your situation a lot.


Invoking functions using a function pointer approach I re-used this answer from this StackOverflow answer to simulate a function pointer approach available in high-level languages like C.

While using this approach in Python, the dictionary I am defining to map the functions needs to be defined after all the functions are defined before. It makes the dictionary definition to be the last segment of the code, which doesn't look neat.

Is there a way like in C to export all the functions beforehand, so that I can define the mapping dictionary in the beginning? (See examples below). Or what would be the most pythonic approach to do this?

Below is my sample snippet of the code using the approach.

File-1 - a1.py

#!/usr/bin/env python

from a2 import *

def arithmeticOps():
    doOps('add',6,1)
    doOps('mul',6,4)

if __name__ == '__main__':
    arithmeticOps()

and File-2 - a2.py

#!/usr/bin/env python

def doOps(op, arg1, arg2):
    arithDict[op](arg1,arg2)

def doAdd(arg1, arg2):
    print "Sum =",arg1 + arg2

def doProd(arg1, arg2):
    print "Prod =",arg1 * arg2

arithDict = {
    'add': doAdd,
    'mul': doProd
}

I am able to run this code peacefully in Python 2.7, and it gives me the result I need.

# python a1.py
Sum = 7
Prod = 24

I agree the built-in functions for addition (or) product could be used here, but the problem is for a bigger scale when I have custom functions in place.

You don't have to define your own functions for these arithmetic operations. These functions already exist in the operator module.

from operator import add, mul

operations = {
    'add': add,
    'mul': mul
}

In Python snake-case is preferred over camelCase for variable names.
Don't use import * imports. Import only the items you need. import * based imports can be helpful in shell for debugging and you may sometimes see them in a package's __init__.py to help define a clean public API.

  Is there a way like in C to export all the functions before-hand, so
  that I can define the mapping dictionary in the beginning (See
  examples below). Or what would be the most pythonic approach to do
  this?

There's no way to import only functions. The pythonic way is to import only the functions you need not everything.

  It makes the dictionary definition to be the last segment of the code
  which doesn't look neat.

The dictionary approach is recommended in Python as there's no switch statement in Python and it's better than a bunch of if-elif-else. You could also create the dict in a1.py based on the imported functions. Though I don't find its definition in a2.py okay.

Another option can be to fetch the functions from a2 module object itself if they can be re-named add, mul etc. This way you won't need a dict.

import a2

def arithmeticOps():
    getattr(a2, 'add')(6, 1)
    getattr(a2, 'mul')(6, 4)


Copy paste from one sheet to another I wrote this code and it works fine for my purpose, to look for some values and copy paste the result to another sheet (online for selected column). I want to know the opinion from this community regarding my code.  I just learned the VBA for 1 week.

Option Explicit

Sub Analysis_ClientRating()
    Dim lastrow As Long, i As Long, rowppt As Long, colppt As Long
    Dim rowppt1 As Long, colppt1 As Long, rowppt2 As Long, colppt2 As Long
    Dim rowppt3 As Long, colppt3 As Long

    lastrow = ShNote.Range("C" & Rows.Count).End(xlUp).Row
    rowppt = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    colppt = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    rowppt1 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    colppt1 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    rowppt2 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    colppt2 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    rowppt3 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row
    colppt3 = ShPPT.Cells(Rows.Count, 1).End(xlUp).Row

    Call Entry_Point

    For i = 6 To lastrow
        Select Case ShNote.Cells(i, 5).Value
            Case Is = 20
            ShNote.Cells(i, 3).Copy
            ShPPT.Cells(rowppt + 6, 3).PasteSpecial xlPasteValues
            ShNote.Cells(i, 5).Copy
            ShPPT.Cells(colppt + 6, 4).PasteSpecial xlPasteValues
            rowppt = rowppt + 1
            colppt = colppt + 1

            Case Is >= 17
            ShNote.Cells(i, 3).Copy
            ShPPT.Cells(rowppt1 + 6, 6).PasteSpecial xlPasteValues
            ShNote.Cells(i, 5).Copy
            ShPPT.Cells(colppt1 + 6, 7).PasteSpecial xlPasteValues
            rowppt1 = rowppt1 + 1
            colppt1 = colppt1 + 1

            Case Is >= 15
            ShNote.Cells(i, 3).Copy
            ShPPT.Cells(rowppt2 + 6, 9).PasteSpecial xlPasteValues
            ShNote.Cells(i, 5).Copy
            ShPPT.Cells(colppt2 + 6, 10).PasteSpecial xlPasteValues
            rowppt2 = rowppt2 + 1
            colppt2 = colppt2 + 1

            Case Is >= 11
            ShNote.Cells(i, 3).Copy
            ShPPT.Cells(rowppt3 + 6, 12).PasteSpecial xlPasteValues
            ShNote.Cells(i, 5).Copy
            ShPPT.Cells(colppt3 + 6, 13).PasteSpecial xlPasteValues
            rowppt3 = rowppt3 + 1
            colppt3 = colppt3 + 1
        End Select
    Next i

    Call Exit_Point
End Sub

If you've only learned VBA a week ago, then congratulations and a good job getting a working solution! Hopefully the information and tips here can provide some help in refining not only your solution, but to help guide your code in the future.

You've avoided some basic traps because you're already using Option Explicit and you are assigning a type to each variable on the Dim line. Most professional VBA developers will advise that it's a better habit to declare each variable immediately before it's used, even down lower in the method. This avoids the "wall of declarations" up front.

You have a potential problem on the statement to determine the lastrow value. It's an easy thing to miss, but when you use Rows.Count, you are referring to the number of rows on the active worksheet, NOT the number of rows on ShNote (which is what you want). So the fully accurate line is

lastrow = ShNote.Range("C" & ShNote.Rows.Count).End(xlUp).Row

Of course the same thing goes for similar statements throughout your code.

(I'm skipping around in your code, it will all make sense in the end I promise)

The values that you're using in the Select Case statement are simple numbers, but those numbers are meaningless unless you give them context. What do those numbers mean in your workbook and for your data? Translate that "meaning" into your code. One way can be to create a set of constants. I've defined a set like this:

Const MAX_THRESHOLD As Long = 20
Const HIGH_THRESHOLD As Long = 17
Const MED_THRESHOLD As Long = 15
Const LOW_THRESHOLD As Long = 13

Now you can change the values just be changing the definitions, and your code makes more sense as you (or me) reads it.

Next, I have to question why you're calculating the same last row value on ShPPT eight times in a row. You can calculate it once, then copy the assignment to the other variables. You can also use more meaningful variable names, since we're defining why we're keeping track of the different rows.

Dim lastSourceRow As Long
lastSourceRow = ShNote.Range("C" & ShNote.Rows.Count).End(xlUp).Row

'--- start all the rows at the same value
Dim lastMaxRow As Long
Dim lastHighRow As Long
Dim lastMedRow As Long
Dim lastLowRow As Long
lastMaxRow = lastSourceRow
lastHighRow = lastSourceRow
lastMedRow = lastSourceRow
lastLowRow = lastSourceRow

The separate "counters" you had for the columns of each threshold hold the exact same value as the rows, so they aren't needed.

I see that each value is incremented separately, but there's also a very consistent pattern to the logic within each Case. When you see that, your first thought should be to break out a separate function that can isolate the logic in a single place. That makes it easier to change later. For example, your method could be

Private Function NowCopyPaste2(ByVal fromRow As Long, _
                               ByVal toRow As Long, _
                               ByVal toColumn As Long, _
                               ByRef fromWS As Worksheet, _
                               ByRef toWS As Worksheet) As Long
    fromWS.Cells(fromRow, 3).Copy
    toWS.Cells(toRow + 6, toColumn).PasteSpecial xlPasteValues
    fromWS.Cells(fromRow, 5).Copy
    toWS.Cells(toRow + 6, toColumn + 1).PasteSpecial xlPasteValues
    NowCopyPaste = toRow + 1
End Function

This produces the same logic that exists in your original post. However, you're using copy/paste to transfer a value from one cell to another and there's a much simpler way that uses far less resources. Just simply assign the value of one cell to the other. The improved NowCopyPaste function is

Private Function NowCopyPaste(ByVal fromRow As Long, _
                              ByVal toRow As Long, _
                              ByVal toColumn As Long, _
                              ByRef fromWS As Worksheet, _
                              ByRef toWS As Worksheet) As Long
    toWS.Cells(toRow + 6, toColumn).Value = fromWS.Cells(fromRow, 3).Value
    toWS.Cells(toRow + 6, toColumn + 1).Value = fromWS.Cells(fromRow, 5).Value
    NowCopyPaste = toRow + 1
End Function

Finally, the main logic of your code is more clear for the upfront work you've done and the clear definitions used:

'--- values are copied to the PowerPoint worksheet and sorted by
'    columns according to the threshold tests
Dim i As Long
For i = 6 To lastSourceRow
    Select Case ShNote.Cells(i, 5).Value
        Case Is = MAX_THRESHOLD
            lastMaxRow = NowCopyPaste(i, lastMaxRow, 3, ShNote, ShPPT)

        Case Is >= HIGH_THRESHOLD
            lastHighRow = NowCopyPaste(i, lastHighRow, 6, ShNote, ShPPT)

        Case Is >= MED_THRESHOLD
            lastMedRow = NowCopyPaste(i, lastMedRow, 9, ShNote, ShPPT)

        Case Is >= LOW_THRESHOLD
            lastLowRow = NowCopyPaste(i, lastLowRow, 12, ShNote, ShPPT)

        Case Else
            '--- what will you do here? (could be nothing but it's
            '    still a good idea to document with a comment that
            '    values in this range are not copied
    End Select
Next i

Notice that I also have used comments prior to each logic block that explain what the code does and why. This will always be important for maintaining the code, even if you are the only one to ever look at it again.

Here's the whole module in a single block:

Option Explicit

Public Sub AnalysisClientRating()
    Const MAX_THRESHOLD As Long = 20
    Const HIGH_THRESHOLD As Long = 17
    Const MED_THRESHOLD As Long = 15
    Const LOW_THRESHOLD As Long = 13

    Dim lastSourceRow As Long
    lastSourceRow = ShNote.Range("C" & ShNote.Rows.Count).End(xlUp).Row

    '--- start all the rows at the same value
    Dim lastMaxRow As Long
    Dim lastHighRow As Long
    Dim lastMedRow As Long
    Dim lastLowRow As Long
    lastMaxRow = lastSourceRow
    lastHighRow = lastSourceRow
    lastMedRow = lastSourceRow
    lastLowRow = lastSourceRow

    '--- I don't know what your entry and exit points do, but you should
    '    create a comment that gives the next developer an idea why these
    '    methods exist and why they are here. what do they do?
    Entry_Point

    '--- values are copied to the PowerPoint worksheet and sorted by
    '    columns according to the threshold tests
    Dim i As Long
    For i = 6 To lastSourceRow
        Select Case ShNote.Cells(i, 5).Value
            Case Is = MAX_THRESHOLD
                lastMaxRow = NowCopyPaste(i, lastMaxRow, 3, ShNote, ShPPT)

            Case Is >= HIGH_THRESHOLD
                lastHighRow = NowCopyPaste(i, lastHighRow, 6, ShNote, ShPPT)

            Case Is >= MED_THRESHOLD
                lastMedRow = NowCopyPaste(i, lastMedRow, 9, ShNote, ShPPT)

            Case Is >= LOW_THRESHOLD
                lastLowRow = NowCopyPaste(i, lastLowRow, 12, ShNote, ShPPT)

            Case Else
                '--- what will you do here? (could be nothing but it's
                '    still a good idea to document with a comment that
                '    values in this range are not copied
        End Select
    Next i

    Exit_Point

End Sub

Private Function NowCopyPaste(ByVal fromRow As Long, _
                              ByVal toRow As Long, _
                              ByVal toColumn As Long, _
                              ByRef fromWS As Worksheet, _
                              ByRef toWS As Worksheet) As Long
    toWS.Cells(toRow + 6, toColumn).Value = fromWS.Cells(fromRow, 3).Value
    toWS.Cells(toRow + 6, toColumn + 1).Value = fromWS.Cells(fromRow, 5).Value
    NowCopyPaste = toRow + 1
End Function

Private Sub Entry_Point()
    '--- does something interesting?
End Sub

Private Sub Exit_Point()
    '--- does something interesting?
End Sub


Push, Delete and Print stack elements I've tried asking about the performance on the HackerRank discussion forum, it didn't work out.

The task is to write a program with three operations:
 1 x  Push the element x onto the stack.
 2    Delete the element present at the top of the stack.
 3    Print the maximum element in the stack.

The first input line is the number of lines in the program, all subsequent lines are one of the three instructions.

Sample Input:
 10
 1 97
 2
 1 20
 2
 1 26
 1 20
 2
 3
 1 91
 3

Sample Output:
 26
 91

My Solution:
data = []
for _ in range(int(input())):
    ins = input().split()
    if ins[0] == '1':
        data.append(int(ins[1]))
    elif ins[0] == '2':
        data.pop()
    else:
        print(max(data))

It gets slow on working with input size of 1000 elements or so,
how could I speed this up?

Try tracking the current maximum, otherwise frequent occurrences of 3 will push your run time towards \$\mathcal{O}(n^2)\$.

If you take a closer look at what your input actually means, you will notice that smaller values being pushed onto the stack have actually no significance if a greater value has being pushed previously. So for every fill level of the stack, you already know the corresponding maximum at the time you push onto the stack.

Use that knowledge:

current_max = []
for _ in range(int(input())):
    ins = input().split()
    if ins[0] == '1':
        new_max = int(ins[1])
        if current_max and new_max < current_max[-1]:
            new_max = current_max[-1]
        current_max.append(new_max)
    elif ins[0] == '2':
        current_max.pop()
    elif ins[0] == '3':
        print(current_max[-1])

By storing the maximum instead of the raw value on the stack, you can always access the current maximum directly. Just don't forget to handle the special case when data is empty, so the new value will always be the maximum.