Quick Sort: Mutable or ST Array This is the most Monadic code I have written to date :-) and I'd welcome comments.

I'm also struck how much faster this mutable approach is than the immutable version I first wrote (see end). OK, so QuickSort is pretty much on mutability, but most algorithms seems to involve significant manipulation of elements in the data structure and immutability leads to slowness due to the endless copying of e.g. Arrays.  I'm left wondering whether immutability is practical in the real world?

-- qsort :: Array -> beginning of subsection -> end of subsection
qsort :: (IOArray Int Int) -> Int -> Int -> IO ()
qsort arr min mx =
    if mx - min < 1 then do
        return ()

    else do
        p <- readArray arr min
        --foldM :: Monad m => (a -> b -> m a) -> a -> [b] -> m a
        final_i <- foldM (partitioner p) (min+1) [(min+1)..mx]
        swap min $ final_i - 1
        qsort arr min     (final_i-2)
        qsort arr final_i mx

    where
        swap i j = do
            arr_i <- readArray arr i
            arr_j <- readArray arr j
            writeArray arr i arr_j
            writeArray arr j arr_i

        partitioner p i idx = do
            arr_idx <- readArray arr idx
            if arr_idx > p then
                return i
            else do
                swap i idx
                return $ i+1

I have since made the minor modifications to use an ST Monad (Gist). Not sure which is to be considered better Haskell though.

qsort :: (V.Vector Int) -> Int -> Int -> (V.Vector Int)
qsort v i j
    | len < 2 = v
    | j > len - 1 =
        let
            -- should really switch pivot with last point in < group
            --swap 0 (i-1)
            (less, greater) = V.splitAt i v
            less_sorted = qsort (V.tail less) 1 1
            great_sorted = qsort greater 1 1
        in less_sorted V.++ cons (V.head less) great_sorted
    | vector_j > p =
        qsort v i (j + 1)
    | otherwise =
        qsort (swap i j) (i+1) (j+1)
    where
        len = V.length v
        p = v ! 0
        vector_i = v ! i
        vector_j = v ! j
        swap i' j' = update v $ fromList [(i', v!j),(j', v!i)]

Mutability, ST and pure code

Whenever you can use ST instead of IO use ST. ST limits the effects that can happen, and runST (or its variants) make sure that no STRefs (or variants) leave the ST. That is, you can always embed a ST s a where you want to use a. You cannot do the same for IO a.

Unfortunately, update has \$\mathcal O(n+m) \$, and therefore every call of swap is \$\mathcal O(n) \$. That's not a problem when you have a single Vector, e.g.

V.foldr (\x a -> x + 2 * a) 0 . V.map sin . (// foo) . fromList

will generate a single vector due to fusion, not three intermediates. Depending on the used vector version and optimization level, you might even end up with a single loop and no generated vector at all. So immutability doesn't prevent optimizations.

Quick Sort on the other hand really needs mutability. So a ST variant (STUArray or MVector) is more appropriate.

Interface

Now that we know that it's fine to use an ST variant, let's have a look at your code. First we look at qsort's type:

qsort :: (IOArray Int Int) -> Int -> Int -> IO ()

An end user has no idea what Int and Int shall be. They want to sort an array, not ponder the right arguments. Therefore, we add a variant that doesn't use that arguments:

import Control.Monad
import Control.Monad.ST
import qualified Data.Vector as V
import qualified Data.Vector.Mutable as M

sort  :: Ord a => V.Vector a -> V.Vector a
sort v = -- left as exercise, should use sort'

sort' :: Ord a => M.STVector s a -> ST s ()
sort' v = qsort v 1 (M.length v)

This interface is easy to use and hard to get wrong. I use the vector variants since I'm more familiar with those, but the same holds for STArray. Note that I relaxed the Int to Ord a => a`.

Internals

Next, we have a look at qsort itself. I'll keep its type for now. First of, instead of

if condition then return () else action

use

when (not condition) action

After all, return does not return prematurely, which can be misleading to Haskell newcomers. Next, try not to shadow Prelude names like min and max. Let's call the arguments lower and upper instead.

Furthermore, you call the value at an index arr_idx. That's misleading. And p is usually used as predicate, so let's call it piv instead. And final_i is just the new midpoint we found with our piv. We end up with.

qsort :: Ord a => (IOArray Int a) -> Int -> Int -> IO ()
qsort arr lower upper =
    when (upper - lower >= 1) $ do
        piv <- readArray arr lower
        midpoint <- foldM (partitioner p) (lower+1) [(lower+1)..upper]
        swap lower $ midpoint - 1
        qsort arr lower   (midpoint - 2)
        qsort arr final_i upper
    where
        swap i j = do
            arr_i <- readArray arr i
            arr_j <- readArray arr j
            writeArray arr i arr_j
            writeArray arr j arr_i

        partitioner piv i idx = do
            value <- readArray arr idx
            if value > piv then
                return i
            else do
                swap i idx
                return $ i+1

Exercise: Try to rewrite qsort with M.Vector or even Data.Vector.Unboxed.Mutable (the latter only needs an additional constraint).

Bottom line

Other than that your mutable code looks fine. I would change swap slightly so that there is only one temporary element, though:

        swap i j = do
            tmp <- readArray arr i
            readArray arr j >>= writeArray arr i
            writeArray arr j tmp

That's personal preference.


Finding all possible combination of alphabets of length 5 The code finds all the combination of alphabet with length 5 and saves it in a list(its ASCII value, minus 97). I want to optimize the code to reduce execution time.

from itertools import product
def permuteKey():

    def pp(chunks):
        return map(''.join, chunks)

    k=pp(product('abcdefghijklmnopqrstuvwxyz', repeat=5))
    key=[]
    for i in range(len(k)):
        temp=k[i]
        temp2=[]
        for j in range(len(temp)):
            temp1=temp[j]
            temp2.append(ord(temp1)-97)
        key.append(temp2)
        print key

permuteKey()

Let me take a stab at this.

from itertools import product
def permuteKey():

    def pp(chunks):
        return map(''.join, chunks)

    k=pp(product('abcdefghijklmnopqrstuvwxyz', repeat=2))
    key=[]
    for i in range(len(k)):
        temp=k[i]
        temp2=[]
        for j in range(len(temp)):
            temp1=temp[j]
            temp2.append(ord(temp1)-97)
        key.append(temp2)
        print key

permuteKey()

Let's start by taking a look at the inner loop:

    temp=k[i]
    temp2=[]
    for j in range(len(temp)):
        temp1=temp[j]
        temp2.append(ord(temp1)-97)

First of all, you're only referring to temp[j] once, and never use j any further. The quick solution would be to iterato over temp instead:

    temp=k[i]
    temp2=[]
    for temp1 in temp:
        temp2.append(ord(temp1)-97)

But now we easily see that temp2 can be written as a list comprehension:

    temp=k[i]
    temp2=[ord(temp1) - 97 for temp1 in temp]

The temp = k[i] can also be removed, leaving

    temp2=[ord(temp1) - 97 for temp1 in k[i]]

Now we end up with

from itertools import product
def permuteKey():

    def pp(chunks):
        return map(''.join, chunks)

    k=pp(product('abcdefghijklmnopqrstuvwxyz', repeat=2))
    key=[]
    for i in range(len(k)):
        temp2=[ord(temp1) - 97 for temp1 in k[i]]
        key.append(temp2)
        print key

permuteKey()

Again, we can apply the trick that k[i] is only used once:

from itertools import product
def permuteKey():

    def pp(chunks):
        return map(''.join, chunks)

    k=pp(product('abcdefghijklmnopqrstuvwxyz', repeat=2))
    key=[]
    for val in k:
        temp2=[ord(temp1) - 97 for temp1 in val]
        key.append(temp2)
        print key

permuteKey()

Another thing I notice, is that you never use the value of one of the items itself, but always ord(c) - 97. Why not just replace abcd...xyz with range(26), and remove the ord(...) - 97 part?

from itertools import product
def permuteKey():

    def pp(chunks):
        return map(''.join, chunks)

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        temp2=[temp1 for temp1 in val]
        key.append(temp2)
        print key

permuteKey()

But now we don't need pp anymore.

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        temp2=[temp1 for temp1 in val]
        key.append(temp2)
        print key

permuteKey()

Also, the definition of temp2 is now equal to val.

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        temp1 = list(val)  # `val` is a tuple, but your code expects a list.
        key.append(temp1)
        print key

permuteKey()

Does it really matter that you want lists instead of tuples? I think not.

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        key.append(val)
        print key

permuteKey()

Now, I have to wonder: why are we actually printing key at every step? I think you might have made a typo, so I'm going to assume you meant the following: (If not, skip the review from here).

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        key.append(val)
    print key

permuteKey()

Now, it's bad practise to print inside a function. Better would it be to return the value.

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    key=[]
    for val in k:
        key.append(val)
    return key

print permuteKey()

Now, notice the similarity between k and key. key = list(k).

from itertools import product
def permuteKey():

    k=product(range(26), repeat=2)
    return list(k)

print permuteKey()

But, now you have an intermediate value, you do not need it, and you can remove it.

from itertools import product
def permuteKey():
    return list(product(range(26, repeat=2)))

print permuteKey()

Now, I don't know how often you need permuteKey. If only once, you can write

from itertools import product
print list(product(range(26), repeat=2))


Object-oriented implementation of a real-world scenario of using a bathroom Is there any object oriented way or design pattern I can use to make this code better? How can I avoid casting in this code? What if I add new Object like WaterTank how will it interact with fitting?

Run.java

package main;

import fittings.Fitting;
import fittings.Tap;
import bathroom.Bathroom;
import user.User;

public class Run {

    public static void main(String[] args) {

        User rutvik = new User("rutvik");

        Bathroom bathroom = new Bathroom(rutvik);

        bathroom.getBathroomFitting(Fitting.TAP).use(rutvik);

        ((Tap)bathroom.getBathroomFitting(Fitting.TAP)).controlSpeed();

    }

}

User.java

package user;

import bathroom.BathroomUser;

public class User implements BathroomUser {

    String userName;

    public User(String userName){
        System.out.println("constructing user... "+userName);
        this.userName=userName;
    }

    @Override
    public String getUserName(){
        return userName;
    }

}

Bathroom.java

package bathroom;

import java.util.ArrayList;
import user.User;
import fittings.Fitting;
import fittings.Tap;

public class Bathroom {

    BathroomUser user;
    ArrayList<Fitting> fittings;

    public Bathroom(User user){
        System.out.println("constructing bathroom...");
        System.out.println("assigning user... " + user.getUserName()+" to bathroom");
        this.user=user;
        fittings=new ArrayList<Fitting>();
        fittings.add(new Tap());
    }

    public Fitting getBathroomFitting(int fittingType){
        for(Fitting fitting:fittings){
            if(fitting.getFittingType()==fittingType){
                return fitting;
            }
        }
        return null;
    }
}

BathroomUser.java

package bathroom;

public interface BathroomUser {

    public String getUserName();

}

Fittings.java

package fittings;

import bathroom.BathroomUser;

public abstract class Fitting {

    public static final int TAP=0;
    public static final int SHOWER=1;
    public static final int SINK=2;

    public abstract void use(BathroomUser user);

    public abstract int getFittingType();

}

Tap.java

package fittings;

import bathroom.BathroomUser;

public class Tap extends Fitting {

    public Tap() {
        System.out.println("constructing TAP fitting...");
    }

    @Override
    public int getFittingType() {
        return Fitting.TAP;
    }

    @Override
    public void use(BathroomUser user) {
        System.out.println("Tap is in use... by " + user.getUserName());
    }

    public void controlSpeed(){
        System.out.println("Tap Speed Changed");
    }
}

You should be programming for interfaces.

In Bathroom instead of

ArrayList<Fitting> fittings;

use

List<Fitting> fittings;

Also you should be using Enums instead of variables. So instead of

public static final int TAP = 0;
public static final int SHOWER = 1;
public static final int SINK = 2;

you should be using

package fittings;

public enum FittingType {
   TAP, SHOWER, SINK;
}

That is a general thing - use enums instead of instance variables. But in your example you don't need that. You can do something like

public Fitting getBathroomFitting(Class<? extends Fitting> fittingType) {

    for (Fitting fitting : fittings) {
        if (fittingType.isInstance(fitting)) {
            return fitting;
        }
    }
    return null;
}

And then in your main method use

bathroom.getBathroomFitting(Tap.class).use(rutvik);

Having a enum which enumerates the sub classes of a class? Not a good idea. You can use the sub classes themselves if you want like above.

BTW in your design, if your bathroom has two taps then how are you going to use the second tap? The method just returns the first instance of that type.

About casting, you will have to cast it somewhere when you are storing as a superclass but using sub class specific things.

  What if I add new Object like WaterTank how will it interact with fitting?

That all depends on what you want to represent in the code.


Calculating the harmonic average In this program I was required to calculate the Harmonic Average using an assembly program with a C driver.

Harmonic mean is defined as:

$$ \frac{n}{\dfrac{1}{x_1} + \dfrac{1}{x_2} + \cdots + \dfrac{1}{x_n}}$$

where \$n = \$ number of inputs and \$x_i = \$ a real number.

I would like to know if:

My code follows common practices.
My code is formatted correctly.
If comments need to be improved.
My code is straightforward; that is, I have achieved the goal with the least amount of work.
There any improvements I should make in my code.

Attached below is my program in Assembly:

;Assembly function that computs the harmonic mean
;of an array of 64-bit floating-point numbers.
;Retrieves input using a C program.
;
;Harmonic mean is defined as Sum(n/((1/x1) + (1/x2) + ... + (1/xn)))
;
;   expects:
;       RDI - address of array
;       RSI - length of the array
;   returns
;       XMMO - the harmonic average of array's values

global harmonicMean

section .data
    Zero dd 0.0
    One dd 1.0

section .text
    harmonicMean:
        push rbp
        mov rbp, rsp              ;C prologue

        movss xmm10, [Zero]       ;Holds tally of denominator
        cvtsi2ss xmm0, rsi        ;Take length and put it into xmm0 register

        .whileLoop:
            cmp rsi, 0            ;Is the length of array 0?
            je .endwhile
            call addDen           ;Compute a denominator value and add it to sum
            add rdi, 4            ;Add size of float to address
            dec rsi               ;Decrease the length
            jmp .whileLoop
        .endwhile:

        divss xmm0, xmm10

        leave
        ret

    ;Calculates a number in the denominator
    addDen:
        push rdi
        movss xmm8, [One]
        movss xmm9, [rdi]
        divss xmm8, xmm9
        addss xmm10, xmm8
        pop rdi
        ret

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

Omit C prolog and epilog if possible

The part you have correctly labeled "C prologue" in your code is used by the compiler to be able to access local variables.  However both inputs and outputs to this assembly routine are in registers, so no stack manipulations are needed.  For that reason, we can eliminate both in this code.

Don't push/pop registers you don't alter

In the addDen routine, the rdi register is pushed and then popped off the stack, but not altered by the routine.  As a result, neither push nor pop are required and both could be omitted.

Avoid subroutine calls

Subroutine calls are not good for fast assembly code.  Your addDen code doesn't need to be a subroutine -- it can be placed inline, avoiding the overhead of a subroutine call.

Reduce jumps per loop iteration

The code as currently structured has both a comparison and conditional jump and also an unconditional jmp .whileLoop that both occur within each loop iteration.  Better practice is to minimize the number of jumps within each loop; ideally one should strive to reduce it to exactly one.

Use direct memory access for instructions

The current code uses this instruction sequence

    movss xmm8, [One]
    movss xmm9, [rdi]
    divss xmm8, xmm9

However, the divss instruction can use memory directly as a source, so we can shorten this to:

    movss xmm8, [One]
    divss xmm8, [rdi]

This saves us a register (xmm9) and some time.

Avoid compare instructions if practical

The current code uses cmp rsi, 0 but that's not really needed since the dec esi at the end of the loop already sets up the zero flag.  Putting all of the previous hints together, a revised routine looks like this:

section .data
    harmonicMean2:
        cvtsi2ss xmm0, rsi        ; xmm0 = len = numerator
        movss xmm10, [Zero]       ; xmm10 = denominator
        inc rsi
        jmp .testzero
    .more:
        movss xmm8, [One]
        divss xmm8, [rdi]
        add rdi, 4
        addss xmm10, xmm8
    .testzero:
        dec rsi
        jnz .more
        divss xmm0, xmm10
        ret

Take advantage of SIMD where practical

SIMD stands for Single Instruction, Multiple Data which allows the CPU to execute multiple operations in parallel, speeding up the routine.  Because your code calculate reciprocals of each data item in the list, we can do that calculation in parallel (4 at a time using SSE) and save some time.  If there aren't 4 data items, we can drop back to a simple serial approach as with what you've already written.  Here's one way to write such a routine:

section .data
    One dd 1.0, 1.0, 1.0, 1.0

section .data
    harmonicMeanP:
        cvtsi2ss xmm0, rsi      ; xmm0 = len = numerator
        xorps xmm10, xmm10      ; xmm10 = denominator = 0
        mov rax, rsi
        shr rax, 2
        jz .single
        movups  xmm9, [One]     ; Loads series of 1s in
    .doquad:
        movaps  xmm8, xmm9      ; copy those ones
        divps   xmm8, [rdi]     ; fetch 4 floats
        add rdi, 16
        addps   xmm10, xmm8

        dec rax
        jnz .doquad
        movaps  xmm8, xmm10         ; copy xmm10
        shufps  xmm8, xmm10, $04E   ; swap low and high halves
        addps   xmm8, xmm10         ; sum now in xmm8
        movaps  xmm10, xmm8         ; copy xmm8
        shufps  xmm10, xmm8, $0B1   ; swap pairs into xmm10
        addps   xmm10, xmm8         ; now have 4 copies of sum in xmm10
        and rsi, 3                  ; less than 4 left
    .single:
        inc rsi
        jmp .testzero
    .more:
        movss xmm8, [One]
        divss xmm8, [rdi]
        add rdi, 4
        addss xmm10, xmm8
    .testzero:
        dec rsi
        jnz .more
        divss xmm0, xmm10
        ret

Test your results

With assembly language code like this, as with much code, the two things one generally looks for are correctness and speed.

Correctness

Although the algorithm is essentially identical in all versions, there is subtle difference that is inherent with the addition of floating point numbers.  Specifically, with floating point numbers, adding a large number to a small number can lead to loss of precision.  In extreme cases, if a is very large and b is very small, we can have a + b = a which is obviously incorrect for non-zero values of b.  (See the oft-cited and still excellent What Every Computer Scientist Should Know About Floating-Point Arithmetic, by David Goldberg for a readable technical discussion for why this is so.)  This can affect the harmonic mean calculations.  For example, if we calculate the harmonic mean of the numbers \$1 \cdots 100000\$, in that order, the correct value is approximately 8271.198621.  However, the original code reports 8270.716797 which is off by a bit.  The reason is that by the time we get to 100000, we're adding a very small number (1e-5) to a relatively much bigger number of about 12.09.  One can use doubles instead of floats here, but while that helps, it only postpones the inevitable when dealing with floating point addition. Numerical analysis is important with these kinds of calculations to assure that the answers are as accurate as they need to be.

Speed

The desire for speed can sometimes conflict with the other goal of correctness, but in this case, we can serve both interests.  When the calculation is parallelized, the effect is that we calculate 4 reciprocals and add 4 results at a time to 4 separate sums.  Because the SSE instructions take about the same amount of time to do one division as to do four in parallel (or 8 in parallel if you can use SSE2 or newer) the results are obtained about 4x as quickly (or 8x with SSE2).  This is where the real speed dividends are, and there's also potentially a small increase in accuracy because each sum is only (roughly) 25% of the total sum (given randomly distributed numbers as input), the loss of precision is slightly reduced.
Here is the C++ program I used to test all variations, including a pure C++ variant that uses doubles for calculation instead of float.

drivemean.cpp

The following code was used to evaluate the various versions for both speed and accuracy.  The stopwatch.h code from this question and was designed for exactly this kind of purpose.

    #include <algorithm>
    #include <iostream>
    #include <iomanip>
    #include "stopwatch.h"

    extern "C" float harmonicMean(float *arr, unsigned len);
    extern "C" float harmonicMean2(float *arr, unsigned len);
    extern "C" float harmonicMeanP(float *arr, unsigned len);
    float hmean(float *arr, unsigned len)
    {
        double sum = 0;
        double n = len;
        for ( ; len; --len, ++arr)
            sum += 1.0/(*arr);
        return n/sum;
    }

    struct MeanTest
    {
        float (*calc)(float *, unsigned);
        std::string name;
    };

    static MeanTest test[]{
        {harmonicMean, "original" },
        {harmonicMean2, "tweaked" },
        {harmonicMeanP, "parallel" },
        {hmean, "C++ double" },
        {nullptr, ""},
    };

    int main()
    {
        const unsigned arrlen{100000};
        const unsigned iterations{1000};
        float nums[arrlen];
        float hm;

        std::iota(nums, nums+arrlen, 1);

        Stopwatch<std::chrono::milliseconds> sw;
        for (MeanTest *t = test; t->calc != nullptr; ++t) {
            sw.start();
            for (unsigned i = iterations; i; --i)
                hm = t->calc(nums, arrlen);
            sw.stop();
            std::cout << std::setprecision(10) << hm << ", "
                << t->name << " time = " << sw.elapsed() << " ms\n";
        }
    }

Results

    8270.716797, original time = 496 ms
    8270.716797, tweaked time = 431 ms
    8271.209961, parallel time = 108 ms
    8271.198242, C++ double time = 675 ms

As you can see, the C++ version was slowest, but most accurate (only off by about 0.0004) but the parallel version was second most accurate but 4x faster than the next fastest.

Using SSE2 instructions would allow one to approach 8x faster and would also allow for some simplification in the use of the shufps instruction.  I'll leave that fun for you.


Designing a Variable Set In my application, I need to allow the user to store variables.  Variables can only be of specific types, but I effectively handle all variables the same way no matter the type.  I would also like to avoid any casting.  Therefore, I have several HashMaps for each variable type, which is causing lots of duplicated code.  Is there a way to simplify my class?

public class VariableSet {

    private final Map<String, PlayerVariable> players;
    private final Map<String, NumberVariable> numbers;
    private final Set<String> notRemovable;
    private final Set<String> usedVariableNames;

    public VariableSet(){
        players = new HashMap<String, PlayerVariable>();
        numbers = new HashMap<String, NumberVariable>();
        notRemovable = new HashSet<String>();
        usedVariableNames = new HashSet<String>();
        addVariable("Current Player", new PlayerVariable(), players, false);
    }

    public PlayerVariable addPlayer(String variableName){
        PlayerVariable var = new PlayerVariable();
        addVariable(variableName, var, players, true);
        return var;
    }

    public NumberVariable addNumber(String variableName){
        NumberVariable var = new NumberVariable();
        addVariable(variableName, var, numbers, true);
        return var;
    }

    private <T extends Variable<?>> void addVariable(String variableName, T variable, Map<String, T> map, boolean removable){
        if (usedVariableNames.contains(variableName)){
            throw new VariableExistsException();
        }
        usedVariableNames.add(variableName);
        map.put(variableName, variable);
        if (!removable){
            notRemovable.add(variableName);
        }
    }

    public void removeNumber(String variableName){
        removeVariable(variableName, numbers);
    }

    public void removePlayer(String variableName){
        removeVariable(variableName, players);
    }

    private <T extends Variable<?>> void removeVariable(String variableName, Map<String, T> map){
        if (notRemovable.contains(variableName)){
            throw new VariableNotRemovableException();
        }
        usedVariableNames.remove(variableName);
        map.remove(variableName);
    }

    public boolean hasPlayerVariable(String variableName){
        return players.containsKey(variableName);
    }

    public boolean hasNumberVariable(String variableName){
        return numbers.containsKey(variableName);
    }

    public Set<String> getVariableNames(){
        return usedVariableNames;
    }

    public void setPlayer(String variableName, PlayerVariable player){
        players.put(variableName, player);
    }

    public void setNumber(String variableName, NumberVariable number){
        numbers.put(variableName, number);
    }
}

All variables extend the same abstract class, so using generics is probably the preferred method, but I can't figure out a way to create a data structure that automatically picks the correct variable type without casting.

The only way to do what you are trying to do, and remove the duplication, is really to cast.

What I would do is to use this kind of map:

private final Map<Class<? extends Variable>, Map<String, Variable>> variables;

To add something to the map, you could do:

variables.get(NumberVariable.class).put("key", value);

And to get:

return (NumberVariable) variables.get(NumberVariable.class).get("key");

You could even write a generic method:

private <T> T getVariable(Class<T> clazz, String key) {
    return clazz.cast(variables.get(clazz).get(key));
}

Note that this shouldn't even give any compiler warnings.

Leaking inner variable

Considering your getVariableNames method, imagine if I would call your code like this:

myVariableSet.getVariableNames.clear();

Whoops! I broke it!

The simple fix is this:

public Set<String> getVariableNames() {
    return new HashSet<>(usedVariableNames);
}

That is, return a copy of the data, then I can manipulate it all I want - I won't break anything!

I wouldn't use a usedVariableNames at all though and just do a map.containsKey(key) directly. If you want to make sure that all variable names are unique though, even if they are of different classes, then go ahead and keep this variable.


Merge two linked lists in Python I have a method which will merger two sorted list together. The function works, but I saw some duplicated code.

Any ideas to make the code more elegant?

class Node(object):

   def __init__(self, data=None, next_node=None):
       self.data = data
       self.next = next_node

def MergeLists(headA, headB):
    head = tail = Node('',None)
    while headA or headB:

        # duplicated code here
        if headA is not None:
            if (headB and headA.data<=headB.data) or (headB is None):
                tail.next = headA
                tail = headA
                headA = headA.next

        if headB is not None:
            if (headA and headB.data <= headA.data) or (headA is None):
                tail.next = headB
                tail = headB
                headB = headB.next

    return head.next

Elegance is in the eye of the beholder; the following (replacing the duplicated code here part) is certainly less redundant:

for i in xrange(2):
    if headA is not None:
        if ( headB and headA.data<=headB.data) or (headB is None):
            tail.next = headA
            tail = headA
            headA = headA.next
    # When i==0, this will flip headA & headB
    # When i==1, it will restore them
    headA,headB = headB,headA


Access case class parameter inside a map with default I have a map whose values are case classes, and I want to access one parameter of that case class with a default value if they map doesn't contain the provided key.

myMap.get(myKey).map(_.valueParam).getOrElse(defaultParam)

intellij-scala suggests that this is a "simplifiable operation on collection” -- is there a more idiomatic way to write this?

*

Edit/Answer: The alternative approach would be to use

myMap.get(myKey).fold(defaultParam)(valueParam)

Map has a getOrElse option:

myMap.getOrElse(myKey,defaultParameter).valueParameter


Mass file renaming via regex I have over 2500 files in a directory tree- some of them are numbered, some of them aren't. eg.

1. That night it rained.epub
The sparrow returns at dawn.epub
001.  Tomorrow is a new day.epub
000.1. We are the children.epub

I created my C# code to iterate through all the directories and rename the numbered files to look neater, but at the same time should leave the unnumbered file unaltered like so:

001.    That night it rained.epub
The sparrow returns at dawn.epub
001.    Tomorrow is a new day.epub
000.1.  We are the children.epub

My coding looks like this:

class Program
{

static string path = @"C:\Users\Anja\Desktop\reekse";

static string name;
static string rpath;
static int numberperiod;
static string number;
static string newname;
static string movepath;

static void Main(string[] args)
    {

    foreach (var file in Directory.GetFiles(path, "*.epub", SearchOption.AllDirectories)) //searches for all the epub files even if there are 20 subdirectories between the file and the main directory
        {

        name = System.IO.Path.GetFileName(file);//gets the filename sans the path
        rpath = System.IO.Path.GetDirectoryName(file);//gets the path sans the  file

        numberperiod = name.IndexOf(".");//find the location of the 1st . (remember-zerobasing is used)

        if (numberperiod == 1)//first period - meaning no 0's is used in numbering
            {
            number = name.Substring(0, numberperiod + 1);//pick up the number as well as the . that follows the numbering

            name = name.TrimStart(number.ToCharArray());//convert the number to char and trim it
            name = name.TrimStart('.', ' ', ' ');// remove any potential blankspaces and . that may be at the start of the filename

            newname = "00" + number + "    " + name; // create a name that has the 0's, the number, adequite spacing, and the name
            }

        if (numberperiod == 2)//same as above but the numbering contains  two digits
            {
            number = name.Substring(0, numberperiod + 1);

            name = name.TrimStart(number.ToCharArray());
            name = name.TrimStart('.', ' ', ' ');

            newname = "0" + number + "    " + name;

            }

        if (numberperiod == 3)//files containing 3 digits
           {
            number = name.Substring(0, numberperiod + 1);

            if (number == "000.")// in case the file has odd numbering as  in the example
                {
                number = name.Substring(0, numberperiod + 3);

                name = name.TrimStart(number.ToCharArray());
                name = name.TrimStart('.', ' ', ' ');

                newname = number + "  " + name;
                }
            else
                {
                name = name.TrimStart(number.ToCharArray());
                name = name.TrimStart('.', ' ', ' ');

                newname = number + "    " + name;
                }

            }

        Console.WriteLine(newname.ToString());

        movepath = Path.Combine(rpath, newname);
        File.Move(file, movepath);
        }
   }
}

The coding works perfectly fine, but it just seems so tedious; I'm positive that there must be a simpler way of doing this like regex or Linq.

Can someone please help? I'm not any good when it comes to either Linq or regex. Can someone please help me rewrite this code to make it simpler?

using Regex.Replace with a help of PadLeft would do this in a single line. so your work can be simplified to :

foreach (var file in Directory.GetFiles(path, "*.epub", SearchOption.AllDirectories))
{
    var fileName = System.IO.Path.GetFileName(file);
    var filePath = System.IO.Path.GetDirectoryName(file);//gets the path sans the  file
    var strName  = Regex.Replace(fileName, @"^((\d+\.){1}(\d\.)?\s+)", m => m.Groups[0].Value.Trim().PadLeft(4, '0').PadRight(8));

    File.Move(file, Path.Combine(filePath, strName));
}

The regex should covers any string with the same pattern in your provided sample.

UPDATE :

To make it more readable for you, you can use the following :

var regex = new Regex(@"^([0-9]+\.?\.\s)");

var fileName = new StringBuilder();

foreach (var file in Directory.GetFiles(path, "*.epub", SearchOption.AllDirectories))
{
    fileName.Append(Path.GetFileName(file));
    var filePath = System.IO.Path.GetDirectoryName(file);

    var match = regex.Match(fileName.ToString());

    if (match.Success)
    {
        fileName.Clear();
        fileName
            .Append(match.Value.Trim().PadLeft(4, '0'))
            .Append("    ")
            .Append(file.Substring(match.Value.Length).Trim());
    }

    fileName.Replace(fileName.ToString(), Path.Combine(Path.GetDirectoryName(file), fileName.ToString()));

    File.Move(filePath, fileName.ToString());

    fileName.Clear();
}

Using StringBuilder is a must in your case, since you're dealing with a list of files name, and since string is immutable, using StringBuilder will avoid creating a new string for each filename which would save your memory and also optimize the performance.


Responsive image solution I just made this responsive image solution, but I'm not sure if it's the best way to do what I'm trying to achieve. Use cases can be found at http://responsimage.com/.

/* responsimage.com v0.2.3 */
(function ($, window, Date) {

    'use strict';

    $(function() {

        var rPrefs = $('meta[name="responsimage"]'),
            rServer = rPrefs.data('server'),
            rStatic = rPrefs.data('static') || 'http://f.cl.ly/items/0M3H0q3n1Z1S1y362d09/spacer.gif',
            rLoading = rPrefs.data('loading') || 'http://f.cl.ly/items/2w2G3N2p0B400Z380J1u/loading.gif',
            rLimit = rPrefs.data('limit') || 100,
            rTimestamp = new Date(),
            rTags = $('[data-responsimage]');

        function responsimage(rInit) {
            rTags.each(function() {
                var rThis = $(this),
                    filename = rThis.data('responsimage'),
                    rWidth = rThis.width(),
                    rHeight = rThis.height(),
                    rAnchor = rThis.data('responsimage-anchor') || 5,
                    rImage;

                if(rInit) {
                    rThis.attr('src', rStatic).css('background', '#fff url(' + rLoading + ') no-repeat center');
                }

                if(rThis.css('font-family') === 'pixel-ratio-2') {
                    rWidth *= 2;
                    rHeight *= 2;
                }

                rImage = rServer.replace('width', rWidth).replace('height', rHeight).replace('anchor', rAnchor).replace('filename', filename);

                if(filename !== 'disabled') {
                    rThis.attr('src', rImage);
                }
            });
        }

        responsimage(1);

        $(window).resize(function () {
            var rNow = new Date();

            if (rNow - rTimestamp >= rLimit) {
                responsimage(false);
            }
        });

        window.onorientationchange = function() {
            setTimeout(responsimage, 0);
        };
    });
}(jQuery, window, Date));

declare the variables inside "rTags.each()" on the outside.  This way there not being declared on each iteration.
The naming of parameters "window" and "Date". I would suggest naming them something different to avoid confusion. This way you can distinguish between the globals from the parameters.
Since you variables are scoped in the functions you can drop the "r" if you wanted too.


Usage of AtomicBoolean I would like to find out if following usage of an AtomicBoolean as a flag variable is correct and good for a multi-threaded application. It is expected that data is going to be loaded only once and when the application is at very low load. It is also expected that the list of SomeObjects is going to be read quite frequently by multiple threads at peak load, probably thousands of time in an hour so that is why I want to avoid hitting the database. The list is not going to be very large so keeping it in memory is not going to be a problem.

public class HibernateDao extends SomeOtherClass implements Dao {

    //Need suggestions for correct usage of following variable
    private AtomicBoolean dataLoaded = new AtomicBoolean();
    private List<SomeObject> someObjects;

    @Override
    public List<SomeObject> getAllSomeObjects() {
        if (dataLoaded.getAndSet(false)) {
            //aim is to atomically load the list only once and avoid calling loadAll
            //every time list is used
            someObjects= (List<SomeObject>) getHibernateTemplate().loadAll(SomeObject.class);
        }
        return someObjects;
    }

    @Override
    public void updateAllSomeObjects(Collection<SomeObject> someObjects) {
            //replaceAll is part of parent class and it removed and reloads
            //data for the entity i.e. SomeObject
        replaceAll(SomeObject.class, someObjects);
            //set this flag atomically so that data is fetched from DB next time
        dataLoaded.getAndSet(true);
    }

}

You are attempting to use the AtomicBoolean to satisfy two requirements:

The first thread to access the data loads it while all others wait until it's been loaded the first time.
Another thread can update the data for future callers.

Neither of these is working perfectly.

While only one thread will load the data, all other threads will receive null without waiting until that first thread completes the loading process.
Because the list of objects is updated after the memory barrier, other threads are not guaranteed to see the new reference immediately.

Solving the first problem is simple enough: use a CountDownLatch to cause all incoming threads to block until the first list of objects is set. Ideally the system would start another thread to specifically load the data rather than letting the first random thread to come along and do it. This isn't necessary (see below), but it's much cleaner.

For the second problem, wrap the list of objects in an AtomicReference to supply a correct memory barrier. You might be thinking, "Oh no! Not two synchronizers for each access!" but Java's synchronization primitives have improved greatly over the years, and the atomic value holders are even cheaper than full synchronization.

public class HibernateDao extends SomeOtherClass implements Dao {
    private CountDownLatch dataLoaded = new CountDownLatch();
    private AtomicReference<List<SomeObject>> data = new AtomicReference<>();

    @Override
    public List<SomeObject> getAllSomeObjects() {
        dataLoaded.await();          // wait until first countDown call
        return data.get();
    }

    @Override
    public void updateAllSomeObjects(Collection<SomeObject> someObjects) {
        replaceAll(SomeObject.class, someObjects);
        data.set(someObjects);
        dataLoaded.countDown();      // release any waiting threads
    }
}

If you really must have the first thread perform the initial loading rather than a separate thread that calls updateAllSomeObjects on its own, you can add back your beloved AtomicBoolean. This would be my last choice, however.

  Note that while you can use if (!ab.getAndSet(true)) it's clearer to use compareAndSet as it indicates that you're performing a tested set rather than allowing any thread to set the new value.

    private AtomicBoolean firstCaller = new AtomicBoolean();

    @Override
    public List<SomeObject> getAllSomeObjects() {
        if (firstCaller.compareAndSet(false, true)) {
            updateAllSomeObjects((List<SomeObject>) getHibernateTemplate().loadAll(SomeObject.class));
        }
        else {
            dataLoaded.await();      // wait until first countDown call
        }
        return data.get();
    }


Printing 100 prime numbers This is a program I wrote to print out 100 prime numbers. How can I improve it?

// Calculating primes
#include <iostream>
#include <iomanip>
#include <vector>
using namespace std;

int main()
{
    const int MAX(100);      // Number of primes to be identified
    long primes[MAX] = {};
    long trial(5);            // Hold a candidate prime
    int count(0);             // Count primes found
    bool found(false);      // Indicates when a prime is found

    vector <long> nonPrimes; //Vector to hold non prime numbers
    vector <long> disqualify; //Vector to hold prime numbers that disqualify the non prime numbers as prime numbers.

    do
    {
        trial += 2;             // Produce next candidate value
        found = false;      // Reset indicator - assume it is not prime

        for (int i = 0; i < count; i++)   // Try division by existing primes
        {
            found = (trial % primes[i]) == 0; // True if no remainder
            if (found) // No remainder means number is not a prime
                break;
            // Terminate the division loop
        }
        // The above loop will exit either due to a break or after trying all
        // existing primes as a divisor. found was initialized to false. If
        // found is false after exiting the loop, a divisor was not found.
        if (!found)             // We have a new prime: not false = true
            primes[count++] = trial;  // Save candidate in next array position
    } while (count < MAX);

    // Main loop has completed - we have found MAX prime numbers.
    // Display the prime numbers, presenting five numbers on one line.
    cout << "Prime numbers found during the program execution:" << endl;
    for (int i = 0; i < MAX; i++)
    {
        if (i % 5 == 0)      // For a new line on first line of output
            cout << endl;      // and on every fifth line that follows
        cout << setw(10) << primes[i];  // Provide space between numbers
    }

    cout << endl;            // All primes displayed - for a new line

    //system("pause");
return 0;
}

STOP. Do not use system("pause"). This is not portable. If you
haven't already, read
this
Stop using using namespace std. Read
this.
I mentioned this in your last review as well.
You don't need to use std::endl for every new line of your output
table. Doing so flushes the output stream needlessly.
You don't need two vectors. Just two for loops and one vector will
suffice. Take a look at the example below.
For beginner code, I think you have good documentation(comments).
Although for some people, it might err on the side of excessive.
You could have initialized the array with the first three primes.
That would have cut down the number of iterations.

 long primes[MAX] = {2,3,5};
 int count(0);             // Count primes found

Example:

#include  <iostream>
#include <vector>
#include <iomanip>

using std::cin;
using std::cout;
using std::endl;
using std::setw;

int  main()
{
    int counter = 0;
    std::vector<int> primes;

    int N, output;
    cin >> N;
    output = N;
    for (int i=2; N > 0; ++i)
    {
        bool  isPrime = true;
        for (int j=0; j < primes.size(); ++j)  //First pass(i==2), will not enter second loop because size() == 0
        {
            if (i % primes[j] ==  0)
            {
                isPrime = false;
                break;
            }
        }

        if (isPrime)
        {
            primes.push_back(i);
            --N;
            cout  <<  i  <<  "\n";
        }
    }

    for(int i=0; i < output; i++)
    {
        if (i % 5 == 0)      // For a new line on first line of output
            cout << "\n";      // and on every fifth line that follows

        cout << setw(10) << primes[i];  // Provide space between numbers
    }
    cout << endl;

    return  0;
}


Drawing a Checked Grid As a senior in Computer Science, have a long way to go towards improving myself. I'd appreciate any and all criticism.

Problem Set

Write a program that can draw a scalable checkered grid (like a chessboard) to any dimension.

#include <iostream>
using std::cout;
using std::cin;

void printSmallSquares(int SquareSize, int width, bool cond) {
bool stars = cond;
    printf("%s", "\n*");
    for (int i = 0; i < width; i++) {
        for (int i = 0; i < SquareSize; i++)
        {
            printf("%s", (stars ? "*" : " "));
        }
    stars = !stars;
}
printf("%s", "*");
}

int main() {
int Height = 8;
int Width = 16;
int SquareSize = 6;

for (int j = 0; j < Height; j++) {
    for (int i = 0; i < Width * SquareSize + 2; i++) {
        printf("%s", "*");
    }

    for (int i = 0; i < SquareSize; i++) {
        (j % 2 == 0 ? printSmallSquares(SquareSize, Width, true) : printSmallSquares(SquareSize, Width, false));
    }
    printf("%s", "\n");
}
for (int i = 0; i < Width * SquareSize + 2; i++) {
    printf("%s", "*");
}
cin >> Width;

}

There are some things to consider:

You already have using std::cout, however you use printf

for (int i = 0; i < Width * SquareSize + 2; i++) {
    std::cout << "*";
}

Edit: See that std::cout takes a std::string, so you should include that header.
C++ mainly works with std::string rather than plain char arrays. Now if you look at std::string there is this constructor string (size_t n, char c) So what you can do now is replace the loop by a constructor:

std::cout << std::string(Width * SquareSize + 2, "*");

You are passing the result of a boolean comparison as a boolean:

for (int i = 0; i < SquareSize; i++) {
    (j % 2 == 0 ? printSmallSquares(SquareSize, Width, true) : printSmallSquares(SquareSize, Width, false));
}

Just pass the comparison directly:

for (int i = 0; i < SquareSize; i++) {
    printSmallSquares(SquareSize, Width, j % 2 == 0);
}

Obviously you should use a appropriate class for your task:

class DrawCheckedGrid {

    DrawCheckedGrid(const int heigth, const int width const int size)
        : Height(height)
        , Width(width)
        , SquareSize(size)
    {}
private:
    int Height;
    int Width;
    int SquareSize;
}

Now that you have a class, you can preallocate all you need

std::string stars = std::string(SquareSize, "*");
std::string spaces = std::string(SquareSize, " ");
std::string separator = std::string(Width * SquareSize + 2, "#") + "\n";

Also you can simply create some helper methods that draw the grid rather than plumbing everything into main():

void drawLines(bool starsFirst) const {
    for (int line = 0; line < Width; ++line) {
        drawLine(starsFirst);
    }
    std::cout << separator;
}

void drawLine(bool starsFirst) const {
    const int offset = starsFirst ? 0 : 1;
    for (int square= offset; square < Width + offset; ++square) {
         std::cout << (square % 2 == 0 ? stars : spaces);
    }
    std::cout << "\n";
}

void drawGrid() const {
    std::cout << separator;
    for (int i = 0; i < Height; ++i) {
        drawLines(i%2 == 0);
    }
}

See that I have created multiple small helpers that do one thing at at a time, rather than one big function.

class DrawCheckedGrid {
    DrawCheckedGrid(const int heigth, const int width const int size)
        : Height(height)
        , Width(width)
        , SquareSize(size)
    {
        stars = std::string(SquareSize, "*");
        spaces = std::string(SquareSize, " ");
        separator = std::string(Width * SquareSize + 2, "#") + "\n";
    }

    void drawGrid() const {
        std::cout << separator;
        for (int i = 0; i < Height; ++i) {
            drawLines(i%2 == 0);
        }
    }
private:

    void drawLines(bool starsFirst) const {
        for (int line = 0; line < Width; ++line) {
            drawLine(starsFirst);
        }
        std::cout << separator;
    }

    void drawLine(bool starsFirst) const {
        const int offset = starsFirst ? 0 : 1;
        for (int square= offset; square < Width + offset; ++square) {
             std::cout << (square % 2 == 0 ? stars : spaces);
        }
        std::cout << "\n";
    }
    int Height;
    int Width;
    int SquareSize;
    std::string stars;
    std::string spaces;
    std::string separator;
}

Here you can also see that all internal functions are private, so we do not leak our internals. You can also add functions that allow you to change the parameters of the grid and pass something to drawGrid().


JavaScript breadth-first search algorithm I have implemented a breadth-first search and I'm more or less looking for suggestions on how I can improve on what I've done here.

How to use:

search(queue, "Musician");

The logs from the search:

Lansana is not a Musician. Lansana is a Software Engineer.
Zack is not a Musician. Zack is a Software Architect.
John is not a Musician. John is a Project Manager.
Francisco is not a Musician. Francisco is a Game Developer.
Ronaldinho is not a Musician. Ronaldinho is a Professional Soccer Player.
Lauren is not a Musician. Lauren is a Mango Seller.
Angelica is not a Musician. Angelica is a Professional Volleyball Player.
Cody is not a Musician. Cody is a Network Programmer.
Match found: Beyonce is a Musician!

I believe it works correctly because if you look at the order in which I enqueue people, and the order in which their friends are enqueued, you'll notice that the logs match the exact order (which of course is crucial in this algorithm in order to get the shortest path).

class Queue {
    constructor() {
        this.count = 0;
        this.data = {};
    }

    enqueue(key, val) {
        if (!this.data.hasOwnProperty(key)) {
            this.count++;
        }
        this.data[key] = val;
    }

    dequeue() {
        let key = Object.keys(this.data)[0],
            memo = this.data[key];
        delete this.data[key];
        this.count--;
        return memo;
    }
}

class Person {
    constructor(args) {
        this.name = args.name;
        this.friends = args.friends || [];
        this.profession = args.profession;
    }
}

let Ronaldinho = new Person({
        name: "Ronaldinho",
        profession: "Professional Soccer Player"
    }),
    Lauren = new Person({
        name: "Lauren",
        profession: "Mango Seller"
    }),
    Angelica = new Person({
        name: "Angelica",
        profession: "Professional Volleyball Player"
    }),
    Cody = new Person({
        name: "Cody",
        profession: "Network Programmer"
    }),
    Beyonce = new Person({
        name: "Beyonce",
        profession: "Musician"
    }),
    Lansana = new Person({
        name: "Lansana",
        friends: [Ronaldinho, Lauren, Angelica],
        profession: "Software Engineer"
    }),
    Zack = new Person({
        name: "Zack",
        friends: [Cody],
        profession: "Software Architect"
    }),
    John = new Person({
        name: "John",
        friends: [Ronaldinho, Angelica],
        profession: "Project Manager"
    }),
    Francisco = new Person({
        name: "Francisco",
        friends: [Beyonce],
        profession: "Game Developer"
    });

let queue = new Queue();

queue.enqueue("Lansana", Lansana);
queue.enqueue("Zack", Zack);
queue.enqueue("John", John);
queue.enqueue("Francisco", Francisco);

function search(queue, profession) {
    let searched = [];

    while (queue.count > 0) {
        let person = queue.dequeue();

        if (!searched[person.name]) {
            if (hasProfession(person, profession)) {
                console.log(`Match found: ${person.name} is  a ${person.profession}!`);
                return true;
            } else {
                console.log(`${person.name} is not a ${profession}. ${person.name} is a ${person.profession}.`);
                for(let i = 0, len = person.friends.length; i < len; i++) {
                    queue.enqueue(person.friends[i].name, person.friends[i]);
                }
            }

            searched.push(person.name);
        }
    }

    return false;
}

function hasProfession(person, profession) {
    return person.profession.toLowerCase() === profession.toLowerCase();
}

Just going to make a few comment on your Queue implementation.

  Should your data be an object this.data = {};?

This is a matter of taste, but I would have opted for an array or a linkedlist data structure if I were you. Using an array, you can easily push items without a key . From my perspective ,you are basically passing the same name for the key/value pair in the enqueue.

  How to retrieve the total number of items enqueued?

I believe your class should  data should only be private to your class. hence the data count should be accessed using a getter function e.g

this.count = function() {
    return this.data.length;
  };

  What should your enqueue look like?

basically, your Queue class is not reusable as it's dependent on having key/value pair has an input. I reckon the enqueue function should take only one value .  In this case John. A pseudocode

    enqueue(value) {
      // created an empty if statement - note if value is undefined this will evaluate to true as well as null ==null
      if (value == null);
      else {
        if ( //data enqueued has exceeded the current array length) {
          let newcapacity = Math.floor(this.count() * this.growfactor / 100);
          // create a new array with the capacity and copy the old array elements to a new one.
        }

        this.size++;
        // assign the item to an empty index
        this.data[index] = value;
      }
    }
    }

Although, the above is more or less a C# equivalent but the main idea is you only enqueue items when they are not null/undefined - hence I used == as opposed to === as undefined== null results in true and null==null results in true . This is equivalent to

if ( typeof(some_variable) !== "undefined" && some_variable !== null ) {}

The newCapacity is simply a strategy to increase the size of an array.

  How to easily delete an item using an array?

Constantly, you need to check if your array's size is zero then throw an exception else call the array's shift() function to delete the first item and update the array.

dequeue() {
  if (size === 0) {
    throw "the queue contains no items";
  }
  this.data.shift();
}

  Note: there are some very useful Queue functions that can be added to your class :

peek() : returns the first item in the queue
contains : Check if an item exist
isempty()- checks if an item is empty

I hope this helps.


Finding a key from values (which are list) in dict I have a dictionary:

x = {'a': [1, 2, 3], 'b': [4, 5, 6]}

and I have to extract a key based on an input. For example assume it is 3, so my output should be 'a'.

Here's my code:

x = {'a': [1, 2, 3], 'b': [4, 5, 6]}

n = 3

output = [k for k, v in x.items() if n in v]

print(output[0])

Can it be done more efficiently?

Elements in the lists are unique, i.e. the value 3 will only be in one list.

Given that you only want the first match, it would be more efficient to stop when you reach it:

output = next(key for key, value in x.items() if n in value)

Note that this will throw a StopIteration exception if there is no matching key, rather than the IndexError your current code throws.

If you need to do this multiple times, you could build a reverse map:

x_reversed = {num: key for key, value in x.items() for num in value}

then the lookup is a trivial x_ reversed[n] However, note the error case changes again, to KeyError.


Translating array pointer access from C++ to Delphi I'd like to know if I translated a piece of code correctly from C++ to Delphi. It looks like it is working, but I have a feeling that I'm reading and writing into memory that I'm not supposed to using Delphi.

Given C++ code:

struct tile_map
{
    int32 CountX;
    int32 CountY;

    uint32 *Tiles;
};

inline uint32
GetTileValueUnchecked(tile_map *TileMap, int32 TileX, int32 TileY)
{
    uint32 TileMapValue = TileMap->Tiles[TileY*TileMap->CountX + TileX];
    return(TileMapValue);
}

uint32 Tiles00[9][17] =
    {
        {1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1},
    };
// More tile map declarations ...
// uint32 Tiles01[9][17] = ...
// uint32 Tiles10[9][17] = ...
// uint32 Tiles11[9][17] = ...

    tile_map TileMaps[2][2];
    TileMaps[0][0].CountX = 17;
    TileMaps[0][0].CountY = 9;
    TileMaps[0][0].Tiles = (uint32 *)Tiles00;

    TileMaps[0][1] = TileMaps[0][0];
    TileMaps[0][1].Tiles = (uint32 *)Tiles01;

    TileMaps[1][0] = TileMaps[0][0];
    TileMaps[1][0].Tiles = (uint32 *)Tiles10;

    TileMaps[1][1] = TileMaps[0][0];
    TileMaps[1][1].Tiles = (uint32 *)Tiles11;

// Usage
    int32 PlayerTileX = 2;
    int32 PlayerTileY = 2;
    uint32 TileMapValue = GetTileValueUnchecked(&TileMap[1][1], PlayerTileX, PlayerTileY);

Delphi translation:

program Project1;

{$APPTYPE CONSOLE}

type
    Puint32 = ^uint32;

    tile_map = record
        CountX : int32;
        CountY : int32;

        Tiles : Puint32;
    end;
    Ptile_map = ^tile_map;

{$POINTERMATH ON}
function GetTileValueUnchecked(TileMap : Ptile_map; TileX, TileY : int32) : uint32; inline;
begin
    result := TileMap^.Tiles[TileY * TileMap^.CountX + TileX];
end;

const //in the future these will be read from file, so const for now
    Tiles00:  array [0..8, 0..16] of uint32 =
    (
        (1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1)
    );
    // More tile map declarations ...
    //Tiles01:  array [0..8, 0..16] of uint32 = ...
    //Tiles10:  array [0..8, 0..16] of uint32 = ...
    //Tiles11:  array [0..8, 0..16] of uint32 = ...
var
    TileMaps : array [0..1, 0..1] of  tile_map;
    PlayerTileX, PlayerTileY : int32;
    TileMapValue : uint32;
begin

    TileMaps[0][0].CountX := 17;
    TileMaps[0][0].CountY := 9;
    TileMaps[0][0].Tiles := Addr(Tiles00);

    TileMaps[0][1] := TileMaps[0][0];
    TileMaps[0][1].Tiles := Addr(Tiles01);

    TileMaps[1][0] := TileMaps[0][0];
    TileMaps[1][0].Tiles := Addr(Tiles10);

    TileMaps[1][1] := TileMaps[0][0];
    TileMaps[1][1].Tiles := Addr(Tiles11);

    // Usage
    PlayerTileX := 2;
    PlayerTileY := 2;
    TileMapValue = GetTileValueUnchecked(@TileMaps[1][1], PlayerTileX, PlayerTileY);
end.

Everything there looks right (except for the = on the last line, which should be :=). It's about as close to a literal translation as you can get.

Converting the Delphi code to use dynamic arrays could ease your mind regarding out-of-bounds access, although the initial setup would be more cumbersome. (That might not be a problem if you're ultimately planning on loading the data from a file instead of from constants.) It would look something like this:

program Project1;

{$APPTYPE CONSOLE}

type
    tile_map = array of array of uint32;

function GetTileValueUnchecked(const TileMap: tile_map; TileX, TileY: int32): uint32; inline;
begin
    result := TileMap[TileY][TileX];
end;

const //in the future these will be read from file, so const for now
    Tiles00:  array [0..8, 0..16] of uint32 =
    (
        (1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1)
    );
    // More tile map declarations ...
    //Tiles01:  array [0..8, 0..16] of uint32 = ...
    //Tiles10:  array [0..8, 0..16] of uint32 = ...
    //Tiles11:  array [0..8, 0..16] of uint32 = ...
var
    TileMaps : array [0..1, 0..1] of  tile_map;
    PlayerTileX, PlayerTileY : int32;
    TileMapValue : uint32;
    x, y: Integer;
begin
    SetLength(TileMaps[0][0], Length(Tiles00), Length(Tiles00[0]));
    for y := 0 to High(Tiles00) do
      for x := 0 to High(Tiles00[0]) do
        TileMaps[0][0][y][x] := Tiles00[y][x];

    SetLength(TileMaps[0][1], Length(Tiles01), Length(Tiles01[0]));
    for y := 0 to High(Tiles01) do
      for x := 0 to High(Tiles01[0]) do
        TileMaps[0][1][y][x] := Tiles01[y][x];

    SetLength(TileMaps[1][0], Length(Tiles10), Length(Tiles10[0]));
    for y := 0 to High(Tiles10) do
      for x := 0 to High(Tiles10[0]) do
        TileMaps[1][0][y][x] := Tiles10[y][x];

    SetLength(TileMaps[1][1], Length(Tiles11), Length(Tiles11[0]));
    for y := 0 to High(Tiles11) do
      for x := 0 to High(Tiles11[0]) do
        TileMaps[1][1][y][x] := Tiles11[y][x];

    // Usage
    PlayerTileX := 2;
    PlayerTileY := 2;
    TileMapValue := GetTileValueUnchecked(TileMaps[1][1], PlayerTileX, PlayerTileY);
end.

Test with that code for a little while to convince yourself that the array accesses are right. If they're not, Delphi's range checking will alert you. Once you're confident, you can disable range checking inside GetTileValueUnchecked so that it's true to its name:

function GetTileValueUnchecked(const TileMap: tile_map; TileX, TileY: int32): uint32; inline;
begin
    {$R-}
    result := TileMap[TileY][TileX];
    {$R+}
end;


Longest repeating and non-overlapping substring Given a string find the length of longest repeating non-overlapping substring in it. In other words, find 2 identical substrings of maximum length which do not overlap.

Examples:

Input : str = "aabaabaaba"

Output : 4(aaba)

Input : str = "aaaaaaaaaaa"

Output : 5(aaaaa)

Input : str = "banana"

Output : 2(an or na)

I have written following recursive code.

#include<bits/stdc++.h>
using namespace std;

void f(string str, int i, int j, int len,int& mx_len){
    mx_len = max(mx_len,len);
    if(j>=str.length())
        return;
    if(str[i]==str[j] && (j-i) > len)
        f(str,i+1,j+1,len+1,mx_len);
    f(str,i+1,j+1,0,mx_len);
    f(str,i,j+1,0,mx_len);
}

int main(){
    string s;
    cin>>s;
    int mx_len=0;
    f(s,0,1,0,mx_len);
    cout<<mx_len<<endl;
    return 0;
}

I do understand that this can be done with dynamic programming. My focus here is on learning recursive style.

I am writing 3 statements that are causing recursive calls. Can the number of statements that cause recursive calls be reduced?

What changes,apart from making it dynamic programming (by caching or tabulation), can be done in this code to make it better?

Also do comment on coding style.

Includes and namespaces

First of all, bits/stdc++.h isn't part of the C++ standard. Instead, only include the files you actually need:

#include <iostream>
#include <string>

Next, it's considered bad practice to use using namespace …, so let's get rid of it.

Your function f

There are several things we can improve here. First of all, it's name is ambiguous. What does f do? It calculates the length of the longest repeating and non-overlapping substring. Let's give it a better name and type:

size_t longest_repeating_substring_length(const std::string & str){
    …
}

'But wait', I here you say. 'That's not my function anymore'. It sure isn't. The problem with your function is that it's possible to use it wrong, e.g.

f("123", 100, 0, 0,mx_len);
//       ^^^ oh oh

It's a fine way to implement the dynamic approach, but it shouldn't be available to the user by default. Therefore, let's "hide" it:

namespace detail {
    void longest_repeating_substring_length(
        const std::string & str, size_t i, size_t j, size_t len, size_t & mx_len
    ){
        mx_len = std::max(mx_len,len);

        if(j >= str.length()) {
            return;
        }
        if(str[i] == str[j] && (j - i) > len) {
            longest_repeating_substring_length(str,i+1,j+1,len+1,mx_len);
        }
        longest_repeating_substring_length(str,i+1,j+1,0,mx_len);
        longest_repeating_substring_length(str,i,j+1,0,mx_len);
    }
}

Why is the const std::string& necessary? First of all, const will make sure that we don't accidentially change your string in our function. And & will get rid of additional copies that would happen throughout the execution.

Note that we can get rid of mx_len:

namespace detail {
    size_t longest_repeating_substring_length(
        const std::string & str, size_t i, size_t j, size_t len
    ){
        size_t mx_len = len;

        if(j >= str.length()) {
            return mx_len;
        }
        if(str[i] == str[j] && (j - i) > len) {
            mx_len = longest_repeating_substring_length(str,i+1,j+1,len+1);
        }
        mx_len = std::max(mx_len, longest_repeating_substring_length(str,i+1,j+1,0));
        mx_len = std::max(mx_len, longest_repeating_substring_length(str,i,j+1,0);
        return mx_len;
    }
}

Depending on which variant you'll use, you end up with the following code:

namespace detail {
    void longest_repeating_substring_length(
        const std::string & str, size_t i, size_t j, size_t len, size_t & mx_len
    ){
        mx_len = std::max(mx_len,len);

        if(j >= str.length()) {
            return;
        }
        if(str[i] == str[j] && (j - i) > len) {
            longest_repeating_substring_length(str,i+1,j+1,len+1,mx_len);
        }

        longest_repeating_substring_length(str,i+1,j+1,0,mx_len);
        longest_repeating_substring_length(str,i,j+1,0,mx_len);
    }
}

size_t longest_repeating_substring_length(const std::string & str){
    size_t max_length = 0;
    detail::longest_repeating_substring_length(str, 0, 1, 0, max_length);

    return max_length;
}

Easy-to-use function at global namespace, and function that can possibly get used in a wrong way hidden away in detail.

Which brings us to your new main:

int main(){
    std::string s;
    std::cin >> s;

    std::cout<< longest_repeating_substring_length(s) <<std::endl;

    return 0;
}

Less recursive calls

To get the number of recursive calls down, we have to ask ourselfs "how large can mx_len get in that particular call?"

Well, if we start at position I and J, the maximum we can get is limited by J-I (since we may not overlap both strings) and str.length() - J (since there are only so many characters starting from the second position.

So let's calculate the maximum length we could get for a string:

#include <cassert>
size_t max_length(size_t string_length, size_t first_pos, size_t second_pos) {
    assert(first_pos <= second_pos);
    assert(second_pos <= string_length);

    return std::min(second_pos - first_pos, string_length - second_pos);
}

size_t max_length(const std::string &s, size_t first_pos, size_t second_pos) {
    return max_length(s.size(), first_pos, second_pos);
}

Now we have a method to determine in \$\mathcal O(1)\$ whether we should continue a recursive call:

namespace detail {
    void longest_repeating_substring_length(
        const std::string & str, size_t i, size_t j, size_t len, size_t & mx_len
    ){
        // i is always less than j, otherwise we broke our algorithm
        assert(i <= j);

        // the current max
        mx_len = std::max(mx_len,len);

        if(j >= str.length()) {
            return;
        }

        if(str[i] == str[j] && (j - i) > len && max_length(str,i,j) <= mx_len) {
            longest_repeating_substring_length(str,i+1,j+1,len+1,mx_len);
        }

        if(max_length(str, i + 1, j + 1) > mx_len){
            longest_repeating_substring_length(str,i+1,j+1,0,mx_len);
        }
        if(max_length(str, i, j + 1) > mx_len){
            longest_repeating_substring_length(str,i,j+1,0,mx_len);
        }
    }
}

Further remarks

Your code could get a lot more readable if you used more whitespace. Also, for difficult functions, you really want to add comments and/or documentation.


Improving my bootstrap class (MVC) I need feedback on my Bootstrap for the MVC architecture that I follow.
I load the routes via yaml. Here is an example:

feed:
    path: /{controller}/{action}{slash}
    requirements:
        id: "[1-9][0-9]*"
        slash: "[/]{0,1}"
    methods: [GET,POST]

#Explore Controller
explore:
    path: /{controller}{slash}
    requirements:
        id: "[1-9][0-9]*"
        slash: "[/]{0,1}"
    methods: [GET,POST]

And here is my Bootstrap class:

class Bootstrap
{

    public function __construct()
    {
        $request = Request::createFromGlobals();
        $locator = new FileLocator(__DIR__ . '/../../../config');

        // DI container
        $container = new DependencyInjection\ContainerBuilder;

        $loader = new DependencyInjection\Loader\YamlFileLoader($container, $locator);
        $loader->load('config-development.yml');

        $container->compile();

        // routing
        $loader = new Routing\Loader\YamlFileLoader($locator);
        $context = new Routing\RequestContext();
        $context->fromRequest($request);
        $matcher = new Routing\Matcher\UrlMatcher(
            $loader->load('routing.yml'),
            $context
        );

        try{
            $parameters = $matcher->match($request->getPathInfo());

            foreach ($parameters as $key => $value) {
                $request->attributes->set($key, $value);
            }

            $command = $request->getMethod() . $request->get('action');
            $resource = "controller.{$request->get('controller')}";

            $controller = $container->get($resource);
            $data = $controller->{$command}($request);

        }catch(\Error $e){
            $data = [
                'status'=>404,
                'message'=>'Not found',
                'info'=>$e->getMessage()
            ];
        }catch(\Symfony\Component\Routing\Exception\MethodNotAllowedException $e){
            $data = [
                'status'=>404,
                'message'=>'Not found',
                'info'=>$e->getMessage()
            ];
        }catch(ResourceNotFoundException $e){
            $data = [
                'status'=>404,
                'message'=>'Not found',
                'info'=>$e->getMessage()
            ];
        }

        if(is_array($data)){
            $response = new JsonResponse($data);
        }else{
            $response = new Response($data);
        }

        //Set cors headers
        $response->headers->set('Access-Control-Allow-Origin', '*');
        $response->headers->set('Access-Control-Allow-Methods', 'GET,HEAD,OPTIONS,POST,PUT,DELETE');
        $response->headers->set('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept, Authorization');
        $response->headers->set('Access-Control-Allow-Credentials', 'true');

        $response->send();
    }

I load the controllers and services via yaml. I'm interested if you would change anything and if you see any possible issues that this kind of structure might have.

I'm seeking for improvements in my code.

Disclaimer: I haven't really used YAMLFileLoader but the advice should apply for the sake of improving PHP code.
Catching Exceptions
I would consolidate the exception catching, since apparently the value of $data is always the same (other than the fact that the info value is assigned the exception message). Unless the goal is to only catch \Symfony\Component\Routing\Exception\MethodNotAllowedException and ResourceNotFoundException instances, why not just catch all Exception instances? Something like this:
catch(\Throwable $e){
    $data = [
        'status'=>404,
        'message'=>'Not found',
        'info'=>$e->getMessage()
    ];
}

And for the sake of D.R.Y. code, you could abstract setting $data in those exception handling blocks to a separate method that returns that array. Something like:
function GetFallbackDataForException(Throwable $e) {
    return [
        'status'=>404,
        'message'=>'Not found',
        'info'=>$e->getMessage()
    ];
}

I hadn't learned until testing out this code but an Error can be thrown and caught - I thought that was only possible with instances of Exception but both implement Throwable...ergo...
Method length
The method is quite long - especially considering it is a constructor. It would be advisable to break up the pieces into separate methods - e.g. one to create the request, one to take the return value from the request and send the response, etc. That way the constructor can be shorter as it calls those methods, plus those methods would hopefully lend themselves to unit testing each component of the process.
I considered suggesting that the foreach to set attributes be replaced with a call to array_walk() but that might require flipping the array keys and values, making a Callable out of the method, etc. all to save 1-2 lines...


Hash calculation for array of long values in C# Can the following function be improved in terms of performance?

I am calculating millions of such hashes.
The long array represents a record from data table where all values are encoded as long values. Usually in one calculation stream all arrays are of the same length but in some rare cases this rule does not apply.
Typical length of the array vary from 300 to 1 elements.

I was wondering whether using SIMD (SSE / AVX) instructions in System.Numerics.Vectors library of .NET 4.6 could help?

public static int GetHashCode(long[] array)
{
    if (array == null) return 0;
    unchecked
    {
        int hash = 17;
        for(int i = 0; i < array.Length; i++)
            hash = 31 * hash + array[i].GetHashCode();
        return hash;
    }
}

You could use XOR to combine hashes which is much faster than your calculation:

int hash = 0;
for(int i = 0; i < array.Length; i++)
    hash ^= array[i].GetHashCode();
return hash;


Excel VBA Retrieving a Specific Pattern from String The following VBA code is in response to a SuperUser question which I found interesting.

What I'm looking for in a response?

Code cleanliness. Can I do more to make the code easier to read and potentially debug in the future?
Code reduction. Is there anything else I can do, without changing the logic, to reduce the amount of code written?
The use of variants in code. I've noticed that the use of variants is frowned upon and with good reason. The memory needed for variants seems to be substantially larger than any other data type.
Use of modules. I've found that it's difficult for me to use modules as opposed to hosting the code within the Sheet or ThisWorkbook. Any comments on the use of modules?

Below is a screen shot of how my spreadsheet looks:

In the original question, the user is only asking to scrub 5 records. I duplicated them until I had 5K records. Completion time was 0.66 seconds. Speed doesn't seem like much of a concern in this case.

Below is the code. FYI, this is located in Sheet1:

Sub PatternScrub()

Dim targetRange As Range
Set targetRange = Range("A1", Range("A1").End(xlDown))

Dim Pattern As String
Dim x As Integer

' TO IMPROVE PERFORMANCE

With Application
    .Calculation = xlCalculationManual
    .ScreenUpdating = False
    .DisplayStatusBar = False
    .EnableEvents = False
End With

' MAIN SCRUB
For Each cell In targetRange

    Pattern = Pattering(cell.Value)

    x = PatternIndex(Pattern)

    If x = 0 Then
        GoTo NextIteration
    Else
        cell.Offset(0, 1).Value = Mid(cell.Value, x, 13)
    End If

NextIteration:

Next cell

With Application
    .Calculation = xlCalculationAutomatic
    .ScreenUpdating = True
    .DisplayStatusBar = True
    .EnableEvents = True
End With

End Sub

Private Function Pattering(ByVal target As String) As String

' TURNS THE STRING INTO 1s AND 0s

Dim i As Integer

For i = 1 To Len(target)

   If Mid(target, i, 1) = "." Then
        Mid(target, i, 1) = 0
    Else
        Mid(target, i, 1) = 1
   End If

Next

Pattering = target

End Function

Private Function PatternIndex(ByVal Pattern As String) As Integer

' MATCHES THE PATTERN AND RETURNS THE FIRST INDEX

    On Error GoTo ErrorHandler

    PatternIndex = Application.WorksheetFunction.Search("1101101110111", Pattern)

ErrorHandler:

    Select Case Err.Number
        Case 1004
            PatternIndex = 0
    End Select

End Function

You can remove a goto - unconditional branches are strongly discouraged and in this case the code fall-through renders it unnecessary.

    If x = 0 Then
        GoTo NextIteration
    Else
        cell.Offset(0, 1).Value = Mid(cell.Value, x, 13)
    End If

NextIteration:

becomes

If x <> 0 Then cell.Offset(0, 1).Value = Mid(cell.Value, x, 13)

This code only works for a specific (hard coded) pattern without any variations. Your example (test case) focuses on numbers, but your pattern also matches aa.bb.ccc.ddd.


Preventing SQL Injection in user registration routine At the moment is my code secure for SQL injections and so forth? I still need to hash passwords and make sure fields are valid and so forth.

<?php

try{
    $handler = new PDO('mysql:host=localhost;dbname=s','root', '*');
    $handler->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}

catch(PDOException $e){
    echo $e->getMessage();
    die();
}

$name = $_POST['name'];
$username = $_POST['username'];
$email = $_POST['email'];
$password = $_POST['password'];

$sql = "INSERT INTO userinfo (name ,username, email, password) VALUES (:name,:username,:email,:password)";
$query = $handler->prepare($sql);

$query->execute(array(
    ':name' => $name,
    ':username' => $username,
    ':email' => $email,
    ':password' => $password
));

?>

It is safe.

You can improve your code like this:

no need to use closing ?> in case that you are not outputting any HTML / or something else after your PHP code
no need to use "" to wrap strings in case that you don't have any variables inside a string, you can use '' instead, PHP interpreter does not need to check in that case whether there are any variables in the string or not
you can replace echo $e->getMessage; die() by simplier exit($e->getMessage());
I added salt generation to your code $salt = md5(uniqid(null, true));
I added password hashing to your code by $password = hash('sha256', $password . $salt);

The whole code hereunder:

<?php

try {
    $handler = new PDO('mysql:host=localhost;dbname=s','root', '*');
    $handler->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch (PDOException $e){
    exit($e->getMessage());
}

$name = $_POST['name'];
$username = $_POST['username'];
$email = $_POST['email'];
$password = $_POST['password'];

$salt = md5(uniqid(null, true));
$password = hash('sha256', $password . $salt);

$sql = '
    INSERT INTO userinfo
        (name ,username, email, password, salt)
    VALUES
        (:name,:username,:email,:password, :salt)
';

$query = $handler->prepare($sql);

$query->execute(array(
    ':name' => $name,
    ':username' => $username,
    ':email' => $email,
    ':password' => $password,
    ':salt' => $salt
));


Optimize a Date and Timespan function I have a working function that may be optimizable.

Currently processing 4 objects gives me 0.0459822 for the elapsed time in seconds - used for testing purposes. Once implemented into the app I will be checking > 1000 objects at a time and would like to speed this up if possible. The format of the fd.dataTime and the other TimeSpans are from a database and I have no control over the format. The reason for adding a day to some Dates is due to the time going over midnight and would yield an incorrect minimum timespan as a result.

Required: manually adding a day to the date if the time is less than the date - only case is presented below(before midnight date - after midnight timespans). Only store valid timespans. nullField is a String that = 0:00:00 an empty TimeSpan found in DB.

Here is the Function:

Private Function GetMinTimeSpan(fds As List(Of Field)) As String
 Dim result As New List(Of Date)
 For Each fd In fds
  Dim dt As Date = fd.dateTime
  Dim s1 As TimeSpan = TimeSpan.Parse(dt.ToString("HH:mm:ss"))
  Dim s2 As TimeSpan = TimeSpan.Parse(fd.sg)
  Dim s3 As TimeSpan = TimeSpan.Parse(fd.os)
  If Not fd.sg = nullField Then
    If s1 > s2 Then
      result.Add(Date.Parse(dt.AddDays(1).ToShortDateString & " " & fd.sg))
    Else
      result.Add(Date.Parse(dt.ToShortDateString & " " & fd.sg))
    End If
  End If
  If s1 > s3 Then
    result.Add(Date.Parse(dt.AddDays(1).ToShortDateString & " " & fd.os))
  Else
    result.Add(Date.Parse(dt.ToShortDateString & " " & fd.os))
  End If
 Next
 Return result.Min.ToString("HH:mm:ss")
End Function

Here is a sample testing:

Dim sw As New Stopwatch
sw.Start()
Dim fd1 As New Field With {.dateTime = Date.Now, .sg = "23:59:00", .os = "00:03:00"}
Dim fd2 As New Field With {.dateTime = Date.Now, .sg = "00:01:00", .os = "00:03:00"}
Dim fd3 As New Field With {.dateTime = Date.Now, .sg = "0:00:00", .os = "00:04:00"}
Dim fd4 As New Field With {.dateTime = Date.Now, .sg = "00:12:00", .os = "00:05:00"}
Dim fds As New List(Of Field) From {fd1, fd2, fd3, fd4}
Dim m = GetMinTimeSpan(fds)
Debug.WriteLine(sw.Elapsed.ToString)

I'm not sure how to optimize your code, but let's talk about those variable names.

Naming is hard. It's doubly hard in vb.net, because it's case insensitive. That fact makes it difficult not to step on keywords like Field. I don't like recommending this, but without a being able to use fields and field to differentiate from the keywords Fields and Field, you could de-vowel it to flds and fld and still have a variable name that's easier to understand than fd.

dt I get, but what does that date actually represent?

s1,s2,s3? What do those mean? I vaguely understand what s1 is, but then I hit this.

Dim s2 As TimeSpan = TimeSpan.Parse(fd.sg)
Dim s3 As TimeSpan = TimeSpan.Parse(fd.os)

Field.sg and Field.os? Why do we have a stargate address and an operating system stored in those fields?

TL;DR: Stop over abbreviating variable names. They make sense to you (now), but will they make sense to Mr. Maintainer? Remember, that poor confused maintainer could be a future version of yourself.


Bind variables in PHP-MySQL I am using the below code to execute a MySQL query in PHP.

$cus_id = '1';
$query = new QUERY();
$clause = "SELECT * FROM customers WHERE cus_id=:cus_id AND status='ACTIVE'";
$params = array('cus_id'=>$cus_id);
$result = $query->run($clause, $params)->fetchAll();

Is it secure enough, or do I need to bind the static variable as well as seen below?

$clause = "SELECT * FROM customers WHERE cus_id=:cus_id AND status=:status";
$params = array('cus_id'=>$cus_id, 'status'=>'ACTIVE');

The static variable 'ACTIVE' can not be messed with from the outside in any way, is therefore safe in the first example.

The only reason I can come up with to still warrant a change to the second snippet is for consistency and future refactors. Suppose you want to change the method where this query appears and you want to give the user the freedom to choose between ACTIVE and ARCHIVED? To prevent yourself from making a very quick change and potentially creating a vulnerability, you should possibly make a habit of escaping every variable.

(Though in the specific case of a status fields where only a specific list of constants is valid I'd definitely also validate that the input is a valid input.)


Generating JSON from a query I am trying to generate JSON from a query. All this seems to be too much grinding it out.

string function getJSON() output="false"    {

  var result = "[";
  var myrow = 0;

  for(var row in this.qryLocation)  {
    myrow++;
    result &= "["
             & row.LocationID                     & ','
             & '"' & row.Address & " " & row.City & '",'
             & '"' & row.formatted_CreateDate     & '"'
             & ']'
             & myRow < this.qryLocation.recordcount ? ',' : '';
    } // end for-in query

  return result & "]";
  }

All this seems very hard to read, especially making sure that the last row does not get an extra comma.

I agree with Adam's comment. Generally, you should avoid rolling-your-own JSON. Just create native CF objects as usual (ie CF arrays and structures). Then use SerializeJSON to generate the JSON string.

SerializeJSON eliminates the need for manual quoting, which dramatically improves the readability of the code IMO. It also handles any escaping of object values automatically (the current code does not). Granted, Adobe's SerializeJSON function does have a few .. "quirks". So you may prefer to use a different implementation. However, the basic approach remains the same.

string function getJSON() output="false"    {
  var result = [];

  for(var row in this.qryLocation)  {
    arrayAppend(result, [ row.LocationID, row.Address & " " & row.City, row.formatted_CreateDate ] );
  }

  return serializeJSON(result);
}


Basic function to convert Country name to ISO code using Pycountry I am working on a dataset which contains a column with common country names.

Task: To convert country name into standard ISO names

I have written a basic function which converts country names into country codes using pycountry library.

import pandas as pd
import pycountry
df = pd.DataFrame({"Country":["China","US","Iceland"])

def do_fuzzy_search(country):
    try:
        result = pycountry.countries.search_fuzzy(country)
        return result[0].alpha_3
    except:
        return np.nan

df["country_code"] = df["Country"].apply(lambda country: do_fuzzy_search(country))

But I am facing issues.Its taking too long. My dataset has 17003 rows,which is not even that large a number.

Firstly, suggestions on the code to improve its performance would be welcome.

Secondly, I would like to know if there is completely different way to do this faster.

I always get my code to be as clean as possible before starting work on performance. Here you have a bare except, which can hide errors. Change it to something better, for now I'll change it to Exception. You are also hiding a potential IndexError and AttributeError if the data ever changes shape as they are in the try.

def do_fuzzy_search(country):
    try:
        result = pycountry.countries.search_fuzzy(country)
    except Exception:
        return np.nan
    else:
        return result[0].alpha_3

Since there are roughly 200 sovereign states and you're working with 17003 rows you likely have a lot of the same values hitting a costly function. To resolve this issue you can use functools.lru_cache. Running in amortized \$O(n)\$ time and \$O(n)\$ space.

@functools.lru_cache(None)
def do_fuzzy_search(country):
    ...

Alternately you can sort the data by the provided countries name and then get each country once. Running in \$O(n\log n)\$ time and \$O(1)\$ space.


Using a popup to display messages to the user I am having a popup which is styled to match the page and am using that single popup to display messages to the user. My popup contains one h1 element and an ASP.NET Label.

The h1 element and Label contents and color change according to the operation: if the information was successfully inserted the h1 text becomes Success and Label's text becomes "Your information was saved".

On incorrect input it changes accordingly, and on some unexpected error it changes the same way.

Currently I am calling three different JavaScript functions on appropriate events from my code behind using RegisterClientScriptBlock.

HTML:

 <asp:Label ID="NewCustomerlblSubmitStatusMsg" runat="server" Text=""></asp:Label>

 <input class="NewCustomercmdSubmitPopUp"  type="button" value="Close" />

CSS:

#NewCustomerSubmitStatusPopUp
    {
    display:none;
    position: fixed;
    width:500px;
    height: 150px;
    top: 50%;
    left: 50%;
    margin-left:-255px;
    margin-top:-110px;
    /*background-color:#F7DFDE; */
    padding:10px;
    z-index:102;
    font-family:Verdana;
    font-size:10pt;
    border-radius:10px;
    -webkit-border-radius:20px;
    -moz-border-radius:20px;
    font-weight:bold;
    }
    #NewCustomerSubmitStatusPopUp h1
    {
        /*color:#BD494A;*/
        padding:20px 0;
        text-align:center;
    }

    .NewCustomercmdSubmitPopUp
    {
        height:30px;
        width:50px;
        margin-left:220px;
    }

    .NewCustomerSubmitPopUpImg
    {
        float:right;
        margin-top:-30px;
        margin-right:-25px;
    }

javascript:

function ShowSubmitSuccessPopUp()
 {

    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: 'white' });
    $('#NewCustomerSubmitStatusPopUp').css({ border: '10px solid #9cc3f7'});
    $('#NewCustomerSubmitStatusPopUp h1').html("Success");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#9cc3f7' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#9cc3f7' });
}

function ShowSubmitErrorPopUp()
{

    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: '#F7DFDE' });
    $('#NewCustomerSubmitStatusPopUp').css({ border:'10px solid #BD494A'});
    $('#NewCustomerSubmitStatusPopUp h1').html("Error");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#BD494A' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').html("There are some errors in the page.Please rectify the errors by visiting the Errors tab.");
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#BD494A' });
}

function ShowSubmitUnexpectedErrorPopUp()
{
    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: '#F7DFDE' });
    $('#NewCustomerSubmitStatusPopUp').css({ border: '10px solid #BD494A' });
    $('#NewCustomerSubmitStatusPopUp h1').html("Something Went Wrong");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#BD494A' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').html("There was some error while submitting your information.Please re-enter the values and try again.");
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#BD494A' });
}

As you can see, for every event, I change text text of the h1 and the label element and change the style too.

Can anybody tell me if I am doing it in a good manner or if there is something wrong?

A few ideas:

Your three javascript functions could easily be combined into one with some simple arguments, like headerText, statusMessage, statusType.
In your description you list some success text, but this isn't in the javascript, so I guess this is set in the code-behind. I consolidate how the text is set. If you choose to use javascript, you could simplify things by changing the asp:Label to a span.
It might be nicer to put the style information in the CSS file and then in the javascript just toggle which classes apply.

Here's an example of how it might look:

CSS

#NewCustomerSubmitStatusPopup.successPopup
{
  background-color: white;
  border-color: #9cc3f7; /* width, pattern added to main CSS block */
}

#NewCustomerSubmitStatusPopup.successPopup h1, #NewCustomerSubmitStatusPopup.successPopup span
{
  color: #9cc3f7;
}

#NewCustomerSubmitStatusPopup.errorPopup
{
  background-color: #F7DFDE;
  border-color: #BD494A;
}

#NewCustomerSubmitStatusPopup.errorPopup h1, #NewCustomerSubmitStatusPopup.errorPopup span
{
  color: #BD494A;
}

javascript

function ShowSubmitPopUp(headingText, statusMessage, statusType)
{
    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");

    var popupClass = (statusType == 'error' ? 'errorPopup' : 'successPopup');
    $('#NewCustomerSubmitStatusPopUp').attr('class', popupClass);
    $('#NewCustomerSubmitStatusPopUp h1').html(headingText);
    $('#NewCustomerSubmitStatusPopUp span').html(statusMessage);
}


Calculating running average in Java I have a library which makes HTTP calls to my service. I was trying to calculate running average of how much time my service is taking on an average.

Here is the core logic of how I am calculating "running average":

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.util.LinkedList;
import java.util.Queue;

public class MovingAverage {

    private final Queue<BigDecimal> window = new LinkedList<BigDecimal>();
    private final int period;
    private BigDecimal sum = BigDecimal.ZERO;

    public MovingAverage(int period) {
        this.period = period;
    }

    public void add(BigDecimal num) {
        sum = sum.add(num);
        window.add(num);
        if (window.size() > period) {
            sum = sum.subtract(window.remove());
        }
    }

    public BigDecimal getAverage() {
        if (window.isEmpty()) return BigDecimal.ZERO;
        BigDecimal divisor = BigDecimal.valueOf(window.size());
        return sum.divide(divisor, 2, RoundingMode.HALF_UP);
    }
}

Is there any better/optimized way to do the same thing? I want to make sure this running average calculation is fast since this library runs under very heavy load so this should not increase the overall latency.

I created a ring buffer variant for this answer. It was loosely based upon this question and performs considerably better so it may be beneficial to others; note though that it has not been checked for equivalence (and it uses float instead of BigDecimal):

public class MovingAverage {

    private final float[] window;
    private float sum = 0f;
    private int fill;
    private int position;

    public MovingAverage(int size) {
        this.window=new float[size];
    }

    public void add(float number) {

        if(fill==window.length){
            sum-=window[position];
        }else{
            fill++;
        }

        sum+=number;
        window[position++]=number;

        if(position == window.length){
            position=0;
        }

    }

    public float getAverage() {
        return sum / fill;
    }

}


Calculating the .Sum() in Linq-to-SQL I have the following code where I need to execute 2 Linq-to-SQL statements. If I do not have the Any() in the if statement, then I receive error when the result is no data. However, I am not sure if my approach is efficient enough. Do you have any suggestions?

My goal is to achieve the following with single Linq statement. I need to get the Sum, and get 0 if there are no results.

 if (db.BadgeAssignments.Any(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId))
 {
      currUserScore = db.BadgeAssignments
                        .Where(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId)
                        .Sum(bba => bba.BadgeAssociated.Points);
 }

You have:

if (db.BadgeAssignments.Any(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId))

And later:

.Where(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId)

The two lambdas:

bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId
bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId

Identical. .Any is \$O(n)\$, meaning worst-case, it will have to iterate all elements of the source to find one that matches the predicate; Where is also \$O(n)\$, meaning it will traverse all elements of the source to find all that match the predicate.

currUserScore = db.BadgeAssignments
                  .Where(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId)
                  .Sum(bba => bba.BadgeAssociated.Points);

This does it once, and will return 0 if no match is made while iterating BadgeAssignments... and that leaves you with a single linq statement =)

The predicate could be simplified further if you encapsulated it into a separate method:

currUserScore = db.BadgeAssignments
                  .Where(bba => IsMatch(bba, userId, courseId))
                  .Sum(bba => bba.BadgeAssociated.Points);

The IsMatch function (//todo: find a better name) is the lambda's body:

return bba.UserIdReceiver == userId
    && bba.BadgeAssociated.CourseId == courseId;