Converting an input string to a list of sub lists with numbers I am learning Python 3. One thing I am doing to get some repetition is going through checkio. It is a website with coding exercises. One of the exercises gives an input string as follows:

str_to_nbrs("(2,2),(6,2),(2,6)")

str_to_nbrs("(3,7),(6,10),(9,7)")

I need the output of this code to produce a list of sub lists with the pairs of numbers so I can put them into variables to use for calculations.

This exercise actually has to do with x and y coordinates and circles. I am not sure why this input was in the form of a string. But, it gives me a chance to learn how to use the tools to convert a string like this to numbers that I can use in mathematical processes.

I figured out how to accomplish what I need. I am relatively new to Python 3. But, from what I have seen already, it is a well-thought-out and efficient language. I would bet there is a better way to do it with less code.

Please note that I added the big obvious comments to explain what the code is doing. They are not part of my regular coding style.

One more thing: I am still trying to come up with a convention for variable names. I have started lists with "l_". I try to give things meaningful names, but also try to keep them relatively short. I know "l_work" isn't a good name. But, I just wanted to finish that section of code, and used that...err...unimaginative name.

import re

# str_to_nbrs("(2,2),(6,2),(2,6)")

def str_to_nbrs(data):
    print("String input:")
    print(data)
    print()
    new_data = re.sub(r"\(","",data)
    new_data = new_data + ","

    print("Remove left parenthesis and add a comma a the end for split method:")
    print(new_data)
    print()

    l_data = new_data.split("),")
    l_data.pop()
    print("Reformatted string data in a list:")
    print(l_data)
    print()

    l_work = []
    l_nbrs = []
    l_all = []

    for i in l_data:
        l_work = i.split(",")
        for j in l_work:
            l_nbrs.append(int(j))
        l_all.append(l_nbrs)
        l_nbrs = []
    print("Integers in sub lists:")
    print(l_all)
    print()
    print("---------------------")
    print()

str_to_nbrs("(2,2),(6,2),(2,6)")
str_to_nbrs("(3,7),(6,10),(9,7)")

re.sub(r"\(","",data) is not a very good use of regular expressions. You could have more easily achieved the same result with data.replace("(", "").

But since you are already using regular expressions, why not use them to do more:

>>> re.findall(r'\d+,\d+', "(2,2),(6,2),(2,6)")
['2,2', '6,2', '2,6']

This regular expression finds all digits (\d) (allows also multiple digits using +), which are separated only by a comma. Since different pairs also have parenthesis around them, this just gives you a list of the pairs.

By adding parenthesis around the digits, this captures each of them and splits each pair:

>>> re.findall(r'(\d+),(\d+)', "(2,2),(6,2),(2,6)")
[('2', '2'), ('6', '2'), ('2', '6')]

Now all that is left to do is convert them to integers and put it in a function:

def str_to_nbrs(data):
    return [list(map(int, p)) for p in re.findall(r'(\d+),(\d+)', data)]


Prime factorisation in java I came across this question in a coding contest (Java-restricted) and I got TLE. I am unable to provide the link of the question as the contest is closed now. Can I know how can I optimise this?

Four Functions
Given four functions

A(x) = Number of i where 1 <= i <= x and gcd(i,x) = 1
B(x) = Sum of A(d) where d divides the number x
C(x) = Sum of exponents of each prime in the prime factorization of B(x)
D(x) = Sum of C(i) where 1 <= i <= x

If $$B(x) = {p_1} ^ {a1} {p_2} ^ {a2} {p_3} ^ {a3}$$
where p1,p2,p3 are prime factors of B(x), then C(x) is a1 + a2
Find the value of D(N) Constraints:
$$1 \le testcases \le 1000000$$
$$1 \le N \le 1000000$$
Example :
Input :
1
4
Output :
4
For n = 4
$$C(1) = 0$$
$$C(2) = 1$$
$$C(3) = 1$$
$$C(4) = 2$$
$$D(4) = C(1) + C(2) + C(3) + C(4) = 4$$

This is my approach
public class Main {
    static long commonDivisor(long num1,long num2) {
        if(num1 == 0) {
            return num2;
        }
        return commonDivisor(num2 % num1, num1);
    }

    static long funcA(long numb) {
        long answer = numb;
        for (long itr = 2; itr * itr <= numb; itr++) {
            if(numb % itr == 0) {
                while(numb % itr == 0) {
                    numb /= itr;
                }
                answer -= answer / itr;
            }
        }
        if(numb > 1) {
            answer -= answer / numb;
        }
        return answer;
    }
    static long funcB(long numb) {
        long sum = 0;
        for (long itr = 1; itr <= Math.sqrt(numb); itr++){
            if (numb % itr == 0){
                if(numb/itr == itr){
                    sum = sum + funcA(itr);
                } else {
                    sum = sum + funcA(itr) + funcA(numb/itr);
                }
            }
        }
        return sum;
    }
    static boolean check_prime(long numb) {
        BigInteger bigint = new BigInteger(String.valueOf(numb));
        return bigint.isProbablePrime(1);
    }
    static long funcC(long numb) {
        if (numb <= 1) {
            return 0;
        }
        long remNum = numb;
        long expo = 0;
        for (long itr = 2; itr <= Math.sqrt(numb); itr++) {
             if(check_prime(itr)) {
                while (remNum % itr == 0) {
                    expo++;
                    remNum /= itr;
                }
             }
        }
        if(check_prime(remNum)) {
            if (remNum > 1) {
                expo++;
            }
        }
        return expo;
    }
    static long funcD(long numb) {
         long result = 0;
         for(long itr = 1; itr <= numb; itr++){
             result += funcC(funcB(itr));
         }
         return result;
    }

    public static void main(String[] args) {
        try {
        BufferedReader buffer = new BufferedReader(new InputStreamReader(System.in));
        long testCases = Long.parseLong(buffer.readLine());
        for(long itr = 1; itr <= testCases; itr++) {
            long number = Long.parseLong(buffer.readLine());
            System.out.println(funcD(number));
        }
    } catch(Exception e) {
        System.out.println("Error");
    }
}

For a coding contest, the answer is almost never to implement the problem the same way as it is given.
For example here, functions A and B are actually intentionally misleading. Function A is also known as Euler's Totient, and function B computes the sum of totients of divisors which is the same as the original number. Detailed review of them is therefore a waste, functions A and B don't need to be implemented at all.
Function C can be improved by noting that the lowest divisor of a number is necessarily a prime, so calling check_prime is unnecessary. If itr was a * b then a and b would have been found first and divided out of remNum already, making itr not a divisor of remNum.
The square root is also unnecessary, and relatively expensive. It can be avoided by testing itr * itr <= numb instead. There is no risk of overflow given that numb <= 1000000. Further refinements are possible.
Function D can be improved by caching the partial sums. In isolation that would not help, but there can be multiple test cases during a run of the program, and they can reuse the previously computed sums. For example as an extreme case, after the test case 1000000 has been used, any further test cases could be essentially free, because all possible cases would have been calculated at that point. Also, after funcD(10) has been evaluated, funcD(20) could start with that result instead of starting over from scratch.


Slow projection maths in C++ code I'm optimising our codebase and currently, the slowest part of the code is a method called calculateResourceZ which is used in projections. I'm looking for suggestions on areas of improvement. It needs the accuracy so it has to use doubles.

double calculateResourceZ(const double resource0, const double fwAtWicket, const double decimalOversRemaining, const double lambdaLocal, const double power) const {
    if (fwAtWicket == 0) {
        return 0;
    }

    const double resourceZPart1 = resource0 * fwAtWicket * pow(lambdaLocal, (power + 1.0));
    const double fg = calculateFunctionG(lambdaLocal, decimalOversRemaining);

    const double resourceZPart2 = (1.0 - exp(-((decimalOversRemaining * expDecay * fg * pow(lambdaLocal, -power)) / fwAtWicket)));
    return resourceZPart1 * resourceZPart2;
}

double calculateFunctionG(const double lambda, const double oversRemaining) const {
    if (version < Version::OCT2014) {
        return 1;
    }

    if (oversRemaining == 0 || lambda == 0) {
        return 0;
    }

    const double alphaLambda = calculateAlphaLambda(lambda);
    const double betaLambda = calculateBetaLambda(lambda);

    return pow((oversRemaining / 50.0), -(1.0 + alphaLambda + betaLambda * oversRemaining));
}

double calculateAlphaLambda(const double lambda) const {
    return -1.0 / (1.0 + c1 * (lambda - 1) * exp(-c2 * (lambda - 1.0)));
}

double calculateBetaLambda(const double lambda) const {
    return -c3 * (lambda - 1.0) * exp(-c4 * (lambda - 1.0));
}

This doesn't do much, so if it's the bottleneck it must be because you're calling it a lot. The first thing to do is to look at the calling pattern. If there are lots of calls with the same value of lambda then you may be able to calculate alpha and beta once rather than each time; you may even be able to cache fg and share it between lots of calls with the same lambda and oversRemaining.

On the level of microoptimisation, pow(lambdaLocal, power + 1.0) is mathematically equal to lambdaLocal * pow(lambdaLocal, power) and * pow(lambdaLocal, -power) is mathematically equal to / pow(lambdaLocal, power); I don't see a numerical reason that pulling that pow out would hurt, and it might shave a couple of cycles off. Then (... / lambdaPower) / fwAtWicket is (...) / (fwAtWicket * lambdaPower) which is another common subexpression.

There's potentially another algebraic tweak: decimalOversRemaining * fg could be refactored by including that extra product in the method:

double calculateFunctionGPrime(const double lambda, const double oversRemaining) const {
    if (version < Version::OCT2014) {
        return oversRemaining;
    }

    if (oversRemaining == 0 || lambda == 0) {
        return 0;
    }

    const double alphaLambda = calculateAlphaLambda(lambda);
    const double betaLambda = calculateBetaLambda(lambda);

    return 50 * pow((oversRemaining / 50.0), -(alphaLambda + betaLambda * oversRemaining));
}

which saves a multiplication in the exceptional cases and an addition in the primary case. Then since the calculations of both alpha and beta have a leading negation those can be removed along with the negation in that last pow:

double calculateFunctionGPrime(const double lambda, const double oversRemaining) const {
    if (version < Version::OCT2014) {
        return oversRemaining;
    }

    if (oversRemaining == 0 || lambda == 0) {
        return 0;
    }

    const double alphaLambda = calculateAlphaLambda(lambda);
    const double betaLambda = calculateBetaLambda(lambda);

    return 50 * pow((oversRemaining / 50.0), alphaLambda + betaLambda * oversRemaining);
}

double calculateAlphaLambda(const double lambda) const {
    return 1.0 / (1.0 + c1 * (lambda - 1) * exp(-c2 * (lambda - 1.0)));
}

double calculateBetaLambda(const double lambda) const {
    return c3 * (lambda - 1.0) * exp(-c4 * (lambda - 1.0));
}


applyFactors refactoring I have inherited the Java function below, and it works the way it should, but you have to look at it for a minute to figure out exactly what is going on. Is there a more succinct or elegant way to encode this logic?

private Float applyFactors(Float originalValue, Float localFactor, Float globalFactor){
    if (globalFactor == null || globalFactor == 0){
        if (localFactor == null || localFactor == 0){
            return null;
        } else {
            return localFactor * originalValue;
        }
    } else {
        if (localFactor == null || localFactor == 0){
            return globalFactor * originalValue;
        } else {
            return localFactor * originalValue * globalFactor;
        }
    }
}

What I like to do in this sort of complicated conditional situation is to do the comparisons upfront and give them names. This makes it easier to reason about the code, and doesn't have any drawback if the conditional expressions are cheap.

I then go ahead and replace the conditional expressions with the named booleans, and it usually becomes more obvious how to simplify the code. Here's what I ended up with:

private Float applyFactors(Float originalValue, Float localFactor, Float globalFactor) {
    Float result = null;

    boolean hasGlobalFactor = ((globalFactor != null) && (globalFactor != 0));
    boolean hasLocalFactor =  ((localFactor != null) && (localFactor != 0));

    if (hasGlobalFactor && hasLocalFactor) {
        result = localFactor * globalFactor * originalValue;
    }
    else if (hasGlobalFactor) {
        result = globalFactor * originalValue;
    }
    else if (hasLocalFactor) {
        result = localFactor * originalValue;
    }

    return result;
}

This approach also makes the code more readable as well.


Can phrase be made from whole words in another string? I am working to solve this HackerRank puzzle: Hash Tables: Ransom Note
The premise is that if I am given a string like this:

give one grand today

I can find a match for each of the words in the following string, but I cannot use the same word more than once:

give me one grand today night

My code looks like this:
  var ransom = "give one grand today".split(' ');
  var magazine = "give me one grand today night".split(' ');
  var result = ransom.reduce(function(words, word) {
    var index = magazine.indexOf(word);

    if(index !== -1) {
      words.push(word);
      magazine.splice(index, 1);
    }

    return words;
  }, []);

  console.log(result.length === ransom.length ? 'Yes' : 'No' );

My code works for most of the test cases but times out for the large inputs.
How can I make my code faster?

Well, the challenge is named "Hash Tables: ..." so that might be a clue to use JS objects (which are basically hashes) instead of arrays. The array-searching (indexOf) is rather slow.

So it might work to instead treat the dictionary/vocabulary (the magazine words) as keys/property names.

function check(ransomString, magazineString) {
  const vocabulary = magazineString
    .split(" ")
    .reduce((hash, word) => {
      hash[word] = (hash[word] || 0) + 1;
      return hash;
    }, {});

  return ransomString
    .split(" ")
    .every(word => {
      vocabulary[word] -= 1;
      return vocabulary[word] >= 0;
    });
}

Here, the vocabulary object is built with the magazine words as keys, and a numeric count as its value, in case a word occurs more than once in the magazine. Keys in JS objects are automatically strings and case-sensitive.

To match it against the ransom note, we decrement the count for each of the words in the note, as we match them. If the word isn't in the vocabulary at all vocabulary[word] -= 1 will result in NaN, and NaN >= 0 is false.

Note: I don't actually see in the challenge description that words can only be used once, although it makes a lot of sense for the "story" they're telling (although the kidnapper could have bought multiple magazines). If the once-only constraint isn't there, you can do:

function check(ransomString, magazineString) {
  const vocabulary = magazineString
    .split(" ")
    .reduce((hash, word) => {
      hash[word] = true;
      return hash;
    }, {});

  return ransomString
    .split(" ")
    .every(word => vocabulary[word] === true);
}

Alternatively, if the words in magazine are unique already, but can only be used once, you could also do although the above are probably as fast/faster:

function check(ransomString, magazineString) {
  const vocabulary = magazineString
    .split(" ")
    .reduce((hash, word) => {
      hash[word] = true;
      return hash;
    }, {});

  return ransomString
    .split(" ")
    .every(word => {
      if(vocabulary[word] !== true) {
        return false;
      }
      delete vocabulary[word];
      return true;
    });
}


Extracting large tar.bz2 folders in parallel The bash below utilizes parallel to extract tar.bz2 folders with 9 files in them. The total size of each is around 60GB. Below are some specs of my system. My question is the below the best/fastest (currently takes ~ 1 hour) way to extract these large tar.bz2 folders? I do not want to slow my system down or damage it so I wanted to ask more knowledgable users.

OS = Ubuntu 14.04, 64 bit
Memory = 128 GB
Processor = Intel® Xeon(R) CPU E5-2630 v3 @ 2.40GHz × 16

#!/bin/bash

# parallel untar of tar.bz2

maxproc=3 # Max number of threads.
i=0

# Count files
set -- /path/to/folders/*.tar.bz2
FILES="$#"

# Blank $1 $2 ...
set --

let i=1
for FILE in /path/to/folders/*.tar.bz2
do
    printf "(%2d/%2d)\tProcessing %s\n" "$i" "$FILES" "$FILE"
    let i=i+1

    tar -xvjf "$FILE" -C /path/to/folders >/dev/null &

    # Turn $1=pida $2=pidb $3=pidc $4=pidd, into
    #      $1=pida $2=pidb $3=pidc $4=pidd $5=pide
    set -- "$@" $!

    # Shift removes $1 and moves the rest down, so you get
    # $1=pidb $2=pidc $3=pidd $3=pide
    # $# is the number of arguments.
    if [ "$#" -ge $maxproc ]  ; then wait "$1" ; shift; fi
done

wait

Re-purposing the argument list this way is not idiomatic Bash, and it took me some time to realise that (firstly) the supplied arguments are ignored and replaced with a list from filesystem and that (secondly) those arguments are replaced with the process IDs of the child tar processes.  It might be better to use distinctly-named array variables instead.

There's very little point in passing the -v argument to tar and then redirecting it to /dev/null.  If you don't pass -v, then tar will quietly get on with its job:

    tar -xjf "$FILE" -C /path/to/folders &

Why is maxproc set to 3?  Is that an empirical value based on your observed disk (and filesystem) throughput?  If so, provide a means to overwrite it (e.g. allow it to be passed as an environment variable).

If you want to spawn one child for each processor (core) on your system, you might choose maxproc=$(getconf _NPROCESSORS_ONLN); I'll leave it as an exercise to think of other possible defaults.

The wait command here waits for a specific process to exit.  If job 2 finishes before job 1, then the script won't start a new process to replace it until after job 1 completes, resulting in lower overall utilisation, especially if the jobs are of unequal size or proceeding at unequal rates.

Instead, replacing wait "$1" with wait -n will wait for any one child process to exit.  This is more likely to be what we want.

The script always returns a success value, even if one or more extractions fail (e.g. due to a full filesystem).  It's possible to remember that an error occurred:

status=0
for FILE in ...
do
    ...
    wait -n || status=1
done

wait || status=1
exit $status

Consider using an existing implementation of parallel or xjobs rather than reinventing the wheel.  That turns this script into a simple one-liner.


Are there mistakes in reading file? I've recently made a test. There were four tasks. One of them was to improve following code:

public class FooFileProcessor
{
    public void ProcessFile(string filename)
    {
       Stream fileStream = File.OpenRead(filename);
       Console.WriteLine(ReadAllContent(fileStream));
       fileStream.Close();
    }

    public string ReadAllContent(Stream stream)
    {
       StreamReader streamReader = new StreamReader(stream);
       return streamReader.ReadToEnd();
    }
}

What I've suggested:

try
{
    using (StreamReader sr = new StreamReader("TestFile.txt"))
    {
       String line = sr.ReadToEnd();
       Console.WriteLine(line);
    }
}
catch (Exception e)
{
   Console.WriteLine("The file could not be read:");
   Console.WriteLine(e.Message);
}

please, review my code and show all mistakes you've seen.

The first thing that springs to mind is that it would probably be better to not create the Stream and StreamReader instances when it can be avoided simply by using the File.ReadAllText method to accomplish the same goal:

public void ProcessFile(string filename)
{
   Console.WriteLine(File.ReadAllText(filename));
}

In other situations you might need to avoid this, especially for very large files.  For small files and simple operations the File.Read... methods should suffice.

Main advantage is that you relegate the object management issues to the framework, reducing the complexity of your code.  This should result in fewer bugs and greater readability.

For larger files, especially where you are wanting to process individual lines (a common use case) it is often useful to use the File.ReadLines method to process the content.  This should read the lines as needed with some buffering, reducing memory consumption while maintaining code simplicity.

On the question of exception handling... I prefer to only put exception handling in when it is required by the specific situation, and I didn't see a specific need here.  If the idea of an exception breaking out of this code concerns you then by all means add some in.  Personally I'd prefer to have the exception bubble out than have to write all the extra code in here to not only handle the exception but also change what we do with a null return, etc.

In general I don't like the idea of always trying to catch exceptions as soon as possible.  I prefer to catch as soon as I need to and no sooner.


General TicTacToe in Haskell I decided to take a more general approach while implementing the TicTacToe game in Haskell.

Util

module Data.List.Utils where

import Data.List (intersperse)

surround :: a -> [a] -> [a]
surround x ys = x : intersperse x ys ++ [x]

nth :: Int -> (a -> a) -> [a] -> [a]
nth _ _ [] = []
nth 0 f (x:xs) = f x : xs
nth n f (x:xs) = x : nth (n - 1) f xs

TicTacToe

module TicTacToe where

import Data.List (transpose)
import Data.Foldable (asum)
import Data.List.Utils (surround, nth)

data Tile = O | X | Empty deriving (Eq)

instance Show Tile where
    show Empty = " "
    show O     = "O"
    show X     = "X"

-- ======================
-- Board Helper Functions
-- ======================
type Board = [[ Tile ]]

showBoard :: Board -> String
showBoard xss = unlines
             . surround vtx
             . map (concat . surround mid . map show)
             $ xss

    where mid = "|"
          vtx = surround '+' $ replicate (length xss) '-'

whoWon :: Board -> Maybe Player
whoWon xss = asum
           . map winner
           $ diag : anti : cols ++ xss

    where cols = transpose xss
          diag = zipWith (!!) xss           [0..]
          anti = zipWith (!!) (reverse xss) [0..]

          winner (x:xs) = if all (==x) xs && x /= Empty
                              then Just (toPlayer x)
                              else Nothing

getCoords :: Int -> Board -> (Int, Int)
getCoords n = divMod (n - 1) . length

fillTile :: Board -> Int -> Tile -> Board
fillTile xss n tile = nth row (nth col (const tile)) xss

    where (row, col) = getCoords n xss

isOver :: Board -> Bool
isOver = all (notElem Empty)

-- ========================
-- Player related functions
-- ========================
data Player = Player1 | Player2 deriving (Eq)

instance Show Player where
    show Player1 = "1st Player"
    show Player2 = "2nd Player"

toPlayer :: Tile -> Player
toPlayer X = Player1
toPlayer O = Player2

fromPlayer :: Player -> Tile
fromPlayer Player1 = X
fromPlayer Player2 = O

changePlayer :: Player -> Player
changePlayer Player1 = Player2
changePlayer Player2 = Player1

-- ====================
-- Game logic functions
-- ====================
validateInput :: Board -> String -> Either String Int
validateInput xss s  = case reads s of

    [(n, "")] -> check n
    _         -> Left "Only integers allowed"

    where check n

            | n < 1 || n > length xss ^ 2 = Left "Out of range"
            | xss !! row !! col /= Empty  = Left "Already filled"
            | otherwise                   = Right n

            where (row, col) = getCoords n xss

askInput :: Player -> Board -> IO ()
askInput player board = do

    putStrLn $ showBoard board
    putStrLn $ "Your turn, " ++ show player ++ "!"

    putStr $ "Tile number (1 - " ++ show (length board ^ 2) ++ "): "
    number <- getLine

    case validateInput board number of

        Left s  -> putStrLn ("Invalid input: " ++ s) >> gameStep player board

        Right n -> let tile = fromPlayer player
                       next = changePlayer player

                   in gameStep next (fillTile board n tile)

gameStep :: Player -> Board -> IO ()
gameStep player board = case whoWon board of

    Just winner -> do

        putStrLn $ showBoard board
        putStrLn $ show winner ++ " won!"

    Nothing | isOver board -> putStrLn "Game ended in a draw"
            | otherwise    -> askInput player board

startBoard :: Int -> Board
startBoard x = replicate x (replicate x Empty)

main :: IO ()
main = gameStep Player1 (startBoard 3) -- Change NxN

Do you use the '-Wall' options of GHC ? It is quite handy, especially when there are non-exhaustive pattern matches in your (non-exhaustive pattern matches are bad ;-) )

TicTacToe.hs:84:41: Warning:
    Defaulting the following constraint(s) to type ‘Integer’
      (Num b0) arising from the literal ‘2’ at TicTacToe.hs:84:41
      (Integral b0) arising from a use of ‘^’ at TicTacToe.hs:84:39
    In the second argument of ‘(^)’, namely ‘2’
    In the second argument of ‘(>)’, namely ‘length xss ^ 2’
    In the second argument of ‘(||)’, namely ‘n > length xss ^ 2’

TicTacToe.hs:96:58: Warning:
    Defaulting the following constraint(s) to type ‘Integer’
      (Num b0) arising from the literal ‘2’ at TicTacToe.hs:96:58
      (Integral b0) arising from a use of ‘^’ at TicTacToe.hs:96:56
    In the second argument of ‘(^)’, namely ‘2’
    In the first argument of ‘show’, namely ‘(length board ^ 2)’
    In the first argument of ‘(++)’, namely ‘show (length board ^ 2)’

TicTacToe.hs:37:11: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘winner’: Patterns not matched: []

TicTacToe.hs:62:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘toPlayer’: Patterns not matched: Empty
Linking dist/build/TicTacToe/TicTacToe ...

Note: you can also feed your source code into hlint. It will sometimes be able to find some smarter ways to write code (though it won’t give you anything special in your case).

Util module

Concerning the Util module, unless you want to distribute it apart from your project, it should not be placed in Data.Utils.

The nth function can be written using splitAt:

nth :: Int -> (a -> a) -> [a] -> [a]
nth i f l = case splitAt i l of
                (start, elm:end) -> start ++ [f elm] ++ end
                _                -> []

That way, just reading the code makes it clear the nth function applies a function to the nth element of a list.

TicTacToe

You still have problems with putStr and the buffering of ouput. You should put a hFlush stdout right after your putStr call (imported from System.IO). Here’s my terminal:

Turn: Player 1
1
Tile number (1-9): +-+-+-+
|X| | |
+-+-+-+
| | | |
+-+-+-+
| | | |
+-+-+-+

Turn: Player 2
2
Tile number (1-9): +-+-+-+
|X|O| |
+-+-+-+
| | | |
+-+-+-+
| | | |
+-+-+-+

You have placed validateInput and askInput in the "Game logic functions" block though they obviously are UI related.

In validateInput:

"Only integers allowed" is a message that must be generated by the user interface since it converts a String to an Int (this has no direct link to the game and could be reused in other programs)
"Out of range" and "Already filled" are messages that must be generated by the game itself

Note: as the error messages themselves are related to the user interface, the functions should not return them in plain english. One way to do this is to create a type : data ValidateError = OutOfRange | AlreadyFilled | OnlyInteger deriving (Eq) (or a class of types for more advanced error handling). With this, your functions do not deal with translation problems or with representation (an error could also be represented by a sound, an animation or a graphic).

You have a mutual recursion between askInput and gameStep. askInput should not call gameStep. askInput should just do one thing: ask the player a value and return it, nothing else. askInput also decides which player will play. Again, it is not its role. Try to apply the Single responsibility principle.


Avoid using C Strings on C++ code to trim leading whitespace I'm learning C++ and wrote a function to remove all the spaces and tabs at the beginning of the input string. It removes them until it find a character different of space and tabs.

In the first version I have mixed C and C++ code and someone has told me that this is a bad coding style, but in the second version I have tried to use only C++ code.

Did I do well?

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

std::string trimLeft(const std::string& input) {

    if ((input.empty()) ||
        ((input.at(0) != ' ') && (input.at(0) != '\t')))
        return input;
    else {
        char * tab2 = new char[input.length() + 1];
        char *trimmed = new char[input.length() + 1];

        strcpy(tab2, input.c_str());

        bool skip = true;
        size_t pos = 0;

        for (size_t i = 0; i < (input.length() + 1); i++) {
            if (skip) {
                if ((tab2[i] == ' ') || (tab2[i] == '\t'))
                    continue;
                else {
                    skip = false;

                    trimmed[pos] = tab2[i];
                    pos++;
                }
            }
            else {
                trimmed[pos] = tab2[i];

                if (tab2[i] == '\0')
                    break;
                else
                    pos++;
            }
        }

        std::string stringTrimmed(trimmed);

        return stringTrimmed;
}

So, I have tried to implement it using only C++ strings:

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

std::string trimLeft(const std::string& input) {

    if ((input.empty()) ||
        ((input.at(0) != ' ') && (input.at(0) != '\t'))) {
        return input;
    }
    else {
        size_t pos = 0;

        for (size_t i = 0; i < input.length(); i++) {
            if ((input.at(i) == ' ') || (input.at(i) == '\t'))
                continue;
            else {
                pos = i;
                break;
            }
        }

        return input.substr(pos, (input.length() - pos));
    }
}

Suggestions on your code:

Your code currently returns the input string unmodified if it contains nothing but spaces and tabs.  I'd say it should return an empty string.
You always use input.at(x) instead of input[x].  The former checks at runtime for out of range errors and in which case throws an exception, while the latter does not.  Your code ensures that you are calling them with valid indexes and no additional check is needed.
Your code begins with an if statement.

if ((input.empty()) ||
    ((input.at(0) != ' ') && (input.at(0) != '\t'))) {
    return input;
}

This special case looks redundant because your else branch already handles input that is empty or does not start with ' ' or '\t'.
Parentheses are welcome to clarify operator precedence, but you are using too many parentheses IMHO.  I would rewrite it like this:

if (input.empty() || (input[0] != ' ' && input[0] != '\t'))

I removed the parentheses around input.empty() (which is a postfix-expression), and input[0] != ' ' and input[0] != '\t' (which are equality-expressions).  I don't think people will take it wrong.
Your use of size_t instead of int is great.  In fact, std::string::size_type is guaranteed to be size_t.  However, you should use std::size_t instead of the unqualified size_t.  (See When using C headers in C++, should we use functions from std or the global namespace?)
s.substr(i) is equivalent to s.substr(i, s.size() - i), so instead of

return input.substr(pos, (input.length() - pos));

It suffices to write

return input.substr(pos);

You store the value of i to pos, break the loop, and then construct the return value based on pos.  This looks unnecessary to me.  Why don't you just directly return inside the loop when the desired i is found?

With these applied, your code is already only three lines:

for (std::size_t i = 0; i < input.length(); i++)
    if (input[i] != ' ' && input[i] != '\t')
        return input.substr(i);

(You still need to fix the trim("    ") == "    " bug.)

In fact, you can simply make use of the C++ standard library facilities and simplify your code even further:

std::string trim_left(const std::string& input)
{
    if (auto p = input.find_first_not_of(" \t"); p != std::string::npos)
        return input.substr(p);
    else
        return "";
}

If you cannot use C++17, move the declaration of p out of the if statement.


Find a positive integer within range using only elementary operations Background
The platform I'm coding for is an esoteric scripting engine coded some time ago that only has limited capabilities. Because I cannot produce a version of the engine that can be run on its own (it needs to attach to a running instance of a game), I have translated the code into C# with limitations to mimic the scripting environment.
Task
Given a blackbox function, find the secret in the blackbox using the following limitations:

You can only declare variables of the decimal type
You have access to the elementary operators (+, -, /, *, ==, != ...etc) except for modulo (%)
You cannot use library functions (Math.Floor() ...etc)

The function is as follows:
//the function returns the result of the operation between the secret and the input
//available operations are: >=, >, <=, <, ==, !=
public static bool BlackBox(decimal num, string operation)
{
    //the secret is always an integer greater than or equal to 1
    //you can assume a maximum value the secret cannot exceed, if it helps
    //in this example, the maximum is 400000
    decimal secret = 317810;

    if (operation == ">=" && secret >= num)
        return true;
    else if (operation == ">" && secret > num)
        return true;
    else if (operation == "<=" && secret <= num)
        return true;
    else if (operation == "<" && secret < num)
        return true;
    else if (operation == "==" && secret == num)
        return true;
    else if (operation == "!=" && secret != num)
        return true;
    else
        return false;
}

Implementation
My first thought was to try a binary search but was stumped because I can't use floor/ceil. I settled on using a Fibonacci search to get to the ballpark and follow with a linear search to find the exact value:
public static void Main()
{
    decimal num1 = 0;
    decimal num2 = 1;

    //use fibonacci sequence to get close to ballpark
    while (BlackBox(num2, ">="))
    {
        decimal temp = num1;
        temp += num2;
        num1 = num2;
        num2 = temp;
    }

    //use linear search for the last mile
    while (BlackBox(num1, "!="))
    {
        num1++;
    }

    Console.WriteLine("The number is " + num1.ToString());
}

This works well for small numbers but as the range of the secret grows the linear portion can get quite time consuming (I chose a worst-case example for this implementation for this reason) because the scripting engine is no where near as efficient as C#. My question is how can I improve this algorithm in terms of performance? Or, is there another algorithm that can perform better than Fibonacci search (maybe a binary search implementation is possible)?

My first thought was to try a binary search but was stumped because I can't use floor/ceil.

I'm not sure why you thought that would matter.  Once you have a maximum number, all you need is to divide the difference between max and min in half and add it to min on each iteration.  Here's a simple example that works for your test case.  It might need tweaking for some edge cases.
const int MAX_SECRET = 400000;
public static decimal Find()
{
    decimal max = MAX_SECRET;
    decimal min = 0;
    while(max - min > 0)
    {
        decimal temp = ((max - min) / 2) + min;
        if(BlackBox(temp,"=="))
        {
            return temp;
        }
        if(BlackBox(temp, "<"))
        {
            max = temp;
        }
        else
        {
            min = temp;
        }
    }
    return max;
}

EDIT
Did some more thinking on the efficiency of this algorithm.  The problem mainly stems from using a decimal to represent an int.  A decimal doesn't want to do integer math.  One way around this is, to use the rounding functionality when the maximum precision is exceeded, to make a floor function.  This greatly reduces the iterations to find the target number.
Here's one way that uses the max precision of C#:
const int MAX_SECRET = 400000;
public static decimal Find()
{
    decimal max = MAX_SECRET;
    decimal min = 0;
    decimal count = 0;
    while(max - min > 0)
    {
        decimal temp = floor((max - min) / 2) + min;
        if(BlackBox(temp,"=="))
        {
            return temp;
        }
        if(BlackBox(temp, "<"))
        {
            max = temp;
        }
        else
        {
            min = temp;
        }
    }
    return max;
}
const decimal MAX_PRECISION = 10000000000000000000000000000M;
static decimal floor(decimal num)
{
    decimal temp = (num / MAX_PRECISION) * MAX_PRECISION;
    if(temp > num)
    {
        --temp;
    }
    return temp;
}

This cuts the iterations down to the same level as using int's instead of decimal's.  To use this in a different language the MAX_PRECISION constant may need to adjusted.


Floating-Point data input in Bash and C I wrote a function that dynamically stores statistical data. I tried to clone the functionality of how I had originally written my solution in Bash. Is there perhaps something I could improve, or do better?

Bash Solution

#!/bin/bash

data_set=()
while IFS= read -r -p 'Enter statistical data (empty line to quit):' input; do
  [[ $input ]] || break
  data_set+=("$input")
done

C Solution

double* get_data(int* data_size)
{
    double input;
    char line[4096];
    double* data_set = NULL;
    double* new_data = NULL;
    int size = 0;

    puts("Enter data (end w/empty line): ");
    while(fgets(line, sizeof(line), stdin))
    {
        if(line[0] == '\n') break;

        new_data = realloc(data_set, (size+1) * sizeof(double));

        if(sscanf(line, "%lf", &input))
        {
            if(new_data == NULL)
            {
                free(data_set);
                puts("Error (re)allocating memory");
                exit(1);
            }
           data_set = new_data;
           data_set[size++] = input;
       }
    }

    *data_size = size;

    return data_set;
}

From your bash script, I assume that you expect a proper (no junk) input. In that case, you can just do

while ((c = getchar()) != '\n') {
    ungetc(c, stdin);
    scanf("%lf%*c", &x);
    printf("x = %g\n", x);
}

Of course, you'll replace printf with memory (re)allocation.

If you stick to your solution, I think that

if (sscanf(line, "%lf", &input))
{
    new_data = realloc(data_set, (size+1) * sizeof(double));
    ...

makes a bit more sense. The way you're doing it now, you (re)allocate the memory even if the input is not a real number. This can lose you all your old data, if realloc has to move the block (so new_data != data_set) in the same step in which input is not a real number.

Edit: As requested in the comments, I'll elaborate why and how the original code could lose the data. So, I'm talking about this code:

while(fgets(line, sizeof(line), stdin))
{
    if(line[0] == '\n') break;

    new_data = realloc(data_set, (size+1) * sizeof(double));

    if(sscanf(line, "%lf", &input))
    {
        if(new_data == NULL)
        {
            free(data_set);
            puts("Error (re)allocating memory");
            exit(1);
        }
       data_set = new_data;
       data_set[size++] = input;
   }
}

There are two things to consider:

The sole purpose of the line

if(sscanf(line, "%lf", &input))

is to make sure that line contains a real number (and to put it in input, as a number). If we were sure that line contains a double, we wouldn't use if.
The line

new_data = realloc(data_set, (size+1) * sizeof(double));

tries to reallocate the memory pointed to by data_set so that it would fit size+1 doubles. However, this may not be possible if the memory right behind the block pointed to by data_set is occupied.

In that case, another memory block of the required size will be allocated, the data will be copied there, and its address will be returned as the return value of realloc, which we save in new_data. The old memory is freed

So, what happens if both of these things happen, i.e., if we don't input a number and the occupation of the memory is such that realloc cannot simply expand the occupied memory?

First, we do realloc:

new_data = realloc(data_set, (size+1) * sizeof(double));

Now, our memory old memory (the one pointed to by data_set) is freed, a new memory is allocated and pointed to by new_data, and the data is copied to this new memory.

But, since we didn't input the number, this if will have a false condition:

if(sscanf(line, "%lf", &input))

So, we do not enter this block and, among other things, we don't do

data_set = new_data;

Hence, for the next step, our data_set points to some freed memory (where our data used to be), while the pointer new_data will be lost, either with the next new_data = realloc(...) command, or when we exit the function. In both cases, our data is unreachable, and any action on data_set will have unpredictable (most probably wrong) results.

It is not very probable that this will happen, but it might, and the solution is simply to reallocate the memory if and only if we're going to work with it. Hence, my suggestion above.


Project Euler Problem 5 Performance I'm hoping for suggestions of a concise way to write my list comprehension filter in the first solution.
I would also appreciate insight on the underlying reasons for some performance differences in my solutions for Problem 5 (https://projecteuler.net/problem=5):

2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.
What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

This first solution takes around 1.5 seconds on my laptop and the other takes 7.5 seconds.
#Project Euler problem 5
import time
start = time.time()
def smallestFactor(start):
#list comprehension filters out numbers not divisible by 11-20
    options = [i for i in range(start, start + 1000000, 20)\
    if i % 20 == 0 and i % 19 == 0 \
    and i % 18 == 0 and i % 17 == 0 \
    and i % 16 == 0 and i % 15 == 0 \
    and i % 14 == 0 and i % 13 == 0 \
    and i % 12 == 0 and i % 11 == 0]
#return if LCM found
    if len(options) > 0:
        print(options[0])
        return options[0]
#recursion with larger numbers if not found
    else:
        smallestFactor(start + 1000000)
smallestFactor(2520)
end = time.time()
print("Speed: " + str(end - start))

My second solution instead uses a function as the list filter. The speed difference may be trivial, but I'm curious about why it takes longer to invoke a function which otherwise performs a similar filtering task as a series of conditionals written directly into the list comprehension.
#Project Euler problem 5
import time
start = time.time()
factors = [i for i in range(20,9,-1)]
def check(num):
#loop 20-11
    for n in factors:
#check modulus, break or return at end of list
        if num % n == 0 and n != factors[len(factors) - 1]:
            continue
        if num % n == 0 and n == factors[len(factors) - 1]:
            return num
        else:
            break
def smallestFactor(start):
#list comprehension filters out numbers not divisible by 11-20
    options = [i for i in range(start, start + 1000000, 20) if check(i) == i]
#return if LCM found
    if len(options) > 0:
        print(options[0])
        return options[0]
#recursion with larger numbers if not found
    else:
        smallestFactor(start + 1000000)
smallestFactor(2520)
end = time.time()
print("Speed: " + str(end - start))

You should care about prime factors. Take the prime factors of one to ten, and 2520:

1: none
2: 2
3: 3
4: 2, 2
5: 5
6: 2, 3
7: 7
8: 2, 2, 2
9: 3, 3
10: 2, 5
2520: 2, 2, 2, 3, 3, 5, 7

You should notice that each prime factor appears in all the divisors. So, 2520 is divisible by 8, as it contains the factors 2, 2, and 2.
The remainder will be the product of 3, 3, 5, and 7.

And so you should make a naive prime factors function. After that, you want to get the prime factors of the divisors, and make a dictionary of prime factors for the smallest multiple of the divisors.

Where a simple solution such as the following, is 20000 times faster than the code in your question.

import collections
from operator import mul
from functools import reduce

def prime_factors(num):
    div = 2
    while num > 1:
        if num % div:
            div += 1
            continue
        yield div
        num /= div

def gen_smallest_divisable(divisors):
    factors = collections.defaultdict(int)
    Counter = collections.Counter
    for divisor in divisors:
        div_factors = Counter(prime_factors(divisor))
        for number, amount in div_factors.items():
            factors[number] = max(factors[number], amount)
    return reduce(mul, (n ** a for n, a in factors.items()))

def smallest_multiple(multiple):
    return gen_smallest_divisable(range(1, multiple + 1))

  but I'm curious about why it takes longer to invoke a function which otherwise performs a similar filtering task as a series of conditionals written directly into the list comprehension.

Just take it at face value, calling functions is slow. Also comprehensions do some magic when passed to the AST, which may speed them up. But 6 seconds for the creation of 11650000 stack frames doesn't seem unreasonable to me.

If you wanted to stick with your method, I'd highly recommend you use itertools.count, as then you don't need your recursion. And you won't test a couple thousand numbers after finding your match.

I'd also make a divisors list, to simplify things. Which has a performance cost, due to using any.

import itertools

def smallest_factor(start):
    divisors = range(11, 21)
    options = (i for i in itertools.count(start, 20)
               if not any(i % d for d in divisors))
    return next(options)


C++: Hiding class implementation detail generic It always bugged me that you would have to write your private class members inside the header files in C++. This means that everytime you change you implementation, client code has to be rebuild.

I found thise site which describes how to hide implementation detail in C++: http://www.octopull.demon.co.uk/c++/implementation_hiding.html

However I also wanted the code to be generic, so that I didn't have to rewrite the same code for each and every class I create. Here is what I got:

#define TL_UTILS_IMPL_DECL(member) \
    private: \
        class impl; \
        impl* member;

#define TL_UTILS_IMPL_START(t_class, parent) \
    class t_class::impl \
    { \
        public: \
            t_class* parent; \
            impl(t_class* self) \
                : parent(self) \
            {}

#define TL_UTILS_IMPL_END() \
    };

#define TL_UTILS_IMPL_INIT(member) \
    member = new impl(this);

#define TL_UTILS_IMPL_TERM(member) \
    delete member;

#define TL_UTILS_IMPL_PREFIX(t_class) \
    t_class::impl::

Example usage:

// Header file:
    class foo
    {
            TL_UTILS_IMPL_DECL(m)

        public:
            foo();
            ~foo();
    };

// Source file:
    TL_UTILS_IMPL_START(foo, p)
        void bar();
    TL_UTILS_IMPL_END()

    foo::foo()
    {
        TL_UTILS_IMPL_INIT(m)

        // from here on we can use the private
        // data and methods stored in m
        m->bar();
    }

    foo::~for()
    {
        TL_UTILS_IMPL_TERM(m)
    }

    void TL_UTILS_IMPL_PREFIX(foo) bar()
    {
        // do something useful
        // use p as a pointer to the parent class instance
    }

I always hear that preprocessor macros are dirty and should not be used in C++, and I agree to some degree. However I could not realize what I wanted using templates.

So what do you think?

Is the use of preprocessor macros justified here?
Is the code safe?
Any improvements you can think of?

Is the use of preprocessor macros justified here?

To me it seems like overkill. The only non-trivial macro is TL_UTILS_IMPL_START, and that will almost certainly never be useful - usually, the impl class won't need a back-pointer to its wrapper class, but will need other constructor arguments, so you'll usually have to write your own class-specific constructor and initialiser anyway. The other macros are just as verbose as, and much less self-explanatory than, the code they replace.

  Is the code safe?

You need to apply the "Rule of Three" - either implement, or remove, the copy constructor and copy assignment operator. I generally use boost::scoped_ptr for the implementation pointer, which sorts this out for me (as well as removing the need for a destructor body).


Angular 2 clock with RxJS Observable I'm fairly new to Angular 2 and I started off with creating a clock for my app. I tried to stick to the official documentation tutorial.

Folder structure:

The CSS file is still empty and the HTML is not very spectacular:

<p>{{time | date:'HH:mm' }}</p>

The clock component itself looks like this:

import {Component} from "@angular/core";
import {ClockService} from "./clock.service";

@Component({
  selector: 'clock',
  templateUrl: './clock.component.html',
  styleUrls: ['./clock.component.css']
})
export class Clock {

  time: Date;

  constructor(private clockService: ClockService) {
  }

  ngOnInit() {
    this.clockService.getClock().subscribe(time => this.time = time);
  }

}

The most important part, however, is the service, which looks like this:

import {Injectable} from "@angular/core";
import {Observable} from "rxjs";

@Injectable()
export class ClockService {

  private clock: Observable<Date>;

  constructor() {
    this.clock = Observable.interval(1000).map(tick => new Date()).share();
  }

  getClock(): Observable<Date> {
    return this.clock;
  }

}

Basically, I need a general review since this is my very first approach. However, what I'm really curious about is if I get the usage of the Observable right. Is it correct to store it as a private field like shown above? Am I doing the .share() correctly?

Your service looks perfectly fine to me. As far as the Clock component goes, I can only recommend one thing.

When getClock().subscribe(...) is executed, a reference to the subscription is being returned back. Ideally, the reference should be kept as long as the component is alive. On component destruction, the subscription should be released by invoking subscription.unsubscribe().

@Component({
  selector: 'clock',
  templateUrl: './clock.component.html',
  styleUrls: ['./clock.component.css']
})
export class Clock implements OnInit, OnDestroy {

  private _clockSubscription: Subscription;
  time: Date;

  constructor(private clockService: ClockService) { }

  ngOnInit(): void {
    this._clockSubscription = this.clockService.getClock().subscribe(time => this.time = time);
  }

  ngOnDestroy(): void {
    this._clockSubscription.unsubscribe();
  }

}

P.S. As a minor side note, I want to mention that some devs like to explicitly mention the OnInit/OnDestroy/... interfaces in the class' implements clause. I am not sure whether it's very helpful or a big deal, but at least the compiler will complain if the ngOnInit is missing while class declares that OnInit is implemented.


Creating a common interface I have a sort of database with a many-to-many association between tags and files. For various reasons, I decided to forgo a junction table in favor of having the left and right tables store the associated values in the tables themselves. However I've ended up writing code like this:

GHashTable *tagdb_get_tag_files (tagdb *db, int tag_code)
{
    return g_hash_table_lookup(db->reverse, GINT_TO_POINTER(tag_code));
}

GHashTable *tagdb_get_file_tags (tagdb *db, int file_id)
{
    return g_hash_table_lookup(db->forward, GINT_TO_POINTER(file_id));
}

void tagdb_remove_tag (tagdb *db, int id)
{
    GHashTable *its_files = g_hash_table_lookup(db->reverse, GINT_TO_POINTER(id));
    if (its_files == NULL)
    {
        return;
    }

    GHashTableIter it;
    gpointer key, value;
    g_hash_table_iter_init(&it, its_files);
    if (g_hash_table_iter_next(&it, &key, &value))
    {
        tagdb_remove_tag_from_file(db, id, key);
    }
    g_hash_table_remove(db->reverse, GINT_TO_POINTER(id));
}

void tagdb_remove_file(tagdb *db, int id)
{
    GHashTable *its_tags = g_hash_table_lookup(db->forward, GINT_TO_POINTER(id));
    if (its_tags == NULL)
    {
        return;
    }

    GHashTableIter it;
    gpointer key, value;
    g_hash_table_iter_init(&it, its_tags);
    if (g_hash_table_iter_next(&it, &key, &value))
    {
        tagdb_remove_file_from_tag(db, id, key);
    }
    g_hash_table_remove(db->forward, GINT_TO_POINTER(id));
}

Where only a few identifiers change, but basically the same thing happens, just on different sides. Is there a simpler, less error prone way to do this in c? Maybe with macros?

You can pass a function that gets the correct object from tagdb.
AKA Service Locator pattern

typedef HashTable* (*ITEM_GETTER)(tagdb*);

HashTable* getTag(tagdb *db)  { return db->reverse;}
HashTable* getFile(tagdb *db) { return db->forward;}

GHashTable *tagdb_get_item_files (tagdb *db, int itemId, ITEM_GETTER getter)
{
    return g_hash_table_lookup(getter(db), GINT_TO_POINTER(itemId));
}

void tagdb_remove_item (tagdb *db, int id, ITEM_GETTER getter)
{
    GHashTable *its_files = g_hash_table_lookup(getter(db), GINT_TO_POINTER(id));
    if (its_files == NULL)
    {
        return;
    }

    GHashTableIter it;
    gpointer key, value;
    g_hash_table_iter_init(&it, its_files);
    if (g_hash_table_iter_next(&it, &key, &value))
    {
        tagdb_remove_tag_from_file(db, id, key);
    }
    g_hash_table_remove(getter(db), GINT_TO_POINTER(id));
}

Edit for clarity:

The old functions can now be implanted like this:

GHashTable *tagdb_get_tag_files (tagdb *db, int tag_code)
{
    return tagdb_get_item_files(db, tag_code, getTag);
}

GHashTable *tagdb_get_file_tags (tagdb *db, int file_id)
{
    return return tagdb_get_item_files(db, file_id, getFile);
}

void tagdb_remove_tag (tagdb *db, int id)
{
    tagdb_remove_item(db, id, getTag);
}

void tagdb_remove_file(tagdb *db, int id)
{
    tagdb_remove_item(db, id, getFile);
}


Cleaning up a three-factor if statement A method that I'm working on in a C# project includes the following:

if (isDailyRateRequired)
{
    CalculatedDailyRate = dailyRate;
}

if (isWeeklyRateRequired)
{
    CalculatedDailyRate = weeklyRate / 7;
}

if (isMonthlyRateRequired)
{
    CalculatedDailyRate = monthlyRate / 30;
}

There has to be a cleaner implementation than this. I could compress it to one line with a set of nested ternary operators, but that would be pretty much unreadable.

Other suggestions?

By looking at your code it appears to me that your basic problem is that isDailyRateRequired, isWeeklyRateRequired and isMonthlyRateRequired are distinct boolean variables instead of an enum.  If you just turned them all into an enum things would be a lot more simple, there would be no possibility of inconsistencies between their values, and you would not even have this multiple-choice issue.


Producing ordinal numbers Is it possible to write this in fewer lines of code?

If you input an integer it will output it as an ordinal number if it is less than 100. The below code works perfectly, but I'm wondering if it could be written more succinctly.

def ordinal(self, num):
    """
      Returns ordinal number string from int, e.g. 1, 2, 3 becomes 1st, 2nd, 3rd, etc.
    """
    self.num = num
    n = int(self.num)
    if 4 <= n <= 20:
      suffix = 'th'
    elif n == 1 or (n % 10) == 1:
      suffix = 'st'
    elif n == 2 or (n % 10) == 2:
      suffix = 'nd'
    elif n == 3 or (n % 10) == 3:
      suffix = 'rd'
    elif n < 100:
      suffix = 'th'
    ord_num = str(n) + suffix
    return ord_num

def ordinal(self, num):
    """
      Returns ordinal number string from int, e.g. 1, 2, 3 becomes 1st, 2nd, 3rd, etc.
    """

Its suspicious that this seems to be a method rather than a free standing function.

    self.num = num

Why are you storing the input here? Given the purpose of this function that seems odd.

    n = int(self.num)

Its doubtful that this is a good idea. What are you converting from? Converting to int should be really be done closer to whether this number came from.

    if 4 <= n <= 20:

You've made this case larger than necessary, many of those would be correct even with out this test, and its not clear what so special about the range 4-20.

      suffix = 'th'
    elif n == 1 or (n % 10) == 1:

You don't need the or. If n == 1, then that the second condition will be true anyways.

      suffix = 'st'
    elif n == 2 or (n % 10) == 2:
      suffix = 'nd'
    elif n == 3 or (n % 10) == 3:
      suffix = 'rd'
    elif n < 100:
      suffix = 'th'

What happens if suffix is >= 100? You'll get an error.

    ord_num = str(n) + suffix
    return ord_num

You don't need to split this across two lines.

Here is my version:

# much code can be improved by using a datastructe.
SUFFIXES = {1: 'st', 2: 'nd', 3: 'rd'}
def ordinal(num):
    # I'm checking for 10-20 because those are the digits that
    # don't follow the normal counting scheme.
    if 10 <= num % 100 <= 20:
        suffix = 'th'
    else:
        # the second parameter is a default.
        suffix = SUFFIXES.get(num % 10, 'th')
    return str(num) + suffix


Summarizing data from multiple sheets I have 9 sheets with 1668 rows and 34 cols. The code collects all data from every sheet into one Summary sheet in the following format:

service 1 supplier 1 price etc..
service 1 supplier 2 price etc..

Unfortunately excel freezes around the 7k row and I receive 'out of memory' error. Any suggestions how to solve improve to code to run on large data?

Sub goEasy()

Dim wsText As Variant
Dim sht As Worksheet
Dim wSum As Worksheet
Dim service As String
Dim supplier As String
Dim priceRange As String
Dim price As String
Dim Lrow As Long, LastRow As Long
Dim a As Long, b As Long

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False

Set sht = ThisWorkbook.Worksheets(4)
Set wSum = ThisWorkbook.Worksheets("Summary")

wsText = Array("<25K", "25K <100K", "100K <250K", "250K <500K", "500K <1M", "1M <5M", "5M <15M", "15M <30M", "30M <50M")

LastRow = sht.Cells(sht.Rows.Count, "A").End(xlUp).Row

    For Each element In wsText
        'For i = 5 To LastRow
        a = 4
        b = 12
        Do While a < LastRow
            'For j = 13 To 47

            If a = LastRow Then
            a = 4
            Exit Do
            End If
            a = a + 1

            Do While b <= 47

            If b = 47 Then
            b = 12
            Exit Do
            End If

                b = b + 1
                Lrow = wSum.UsedRange.Rows(wSum.UsedRange.Rows.Count).Row + 1

                service = ThisWorkbook.Worksheets(element).Cells(a, 1).Text
                supplier = ThisWorkbook.Worksheets(element).Cells(4, b).Text
                priceRange = ThisWorkbook.Worksheets(element).Cells(2, 1).Text
                price = ThisWorkbook.Worksheets(element).Cells(a, b).Text

                wSum.Cells(Lrow, 1) = service
                wSum.Cells(Lrow, 2) = supplier
                wSum.Cells(Lrow, 3) = priceRange
                wSum.Cells(Lrow, 4) = price
            'Next j
            Loop
        'Next i
        Loop
    Next element

Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
Application.EnableEvents = True

MsgBox ("Complete")

End Sub

Naming

In order for code to be useful, it has to be understood. This applies equally to you now, you in 6 months and anybody else who eventually has to work with it. As such, code should be written for other people to read and understand.

Documentation helps with this. Writing down what your program is doing/why. But the easiest way is just to name things descriptively and unambiguously.

Dim service As String
Dim supplier As String
Dim priceRange As String
Dim price As String

These are good names. Somewhat ambiguous, I'd prefer something like serviceName, supplierName, priceText etc. to be completely unambiguous, but I can look anywhere in your code, see those variables and know precisely what they are.

Dim wsText As Variant
Dim wSum As Worksheet
Dim Lrow As Long
Dim a As Long, b As Long

These are not good names.

If I see something called wsText I'm going to parse that as Worksheet Text which means... some kind of text, in a worksheet? A worksheet called text? Oh, it's a list of worksheet names.

Yeah, that was completely non-obvious.
Just call it worksheetNames or maybe targetWorksheetNames.

wSum is similarly ambiguous and not-obvious about what it is. Just call it summarySheet.

a, b are generic, and hence useless. Here, they refer to Row and Column indexes, so why not call them currentRow, currentcolumn?

Good naming just makes code a hell of a lot easier to work with. Like so:

Sub AggregateSheetData()

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False

    Dim sheetNames As Variant
    sheetNames = Array("<25K", "25K <100K", "100K <250K", "250K <500K", "500K <1M", "1M <5M", "5M <15M", "15M <30M", "30M <50M")

    Dim summarySheet As Worksheet
    Set summarySheet = ThisWorkbook.Worksheets("Summary")

    Dim currentSummaryRow As Long
    currentSummaryRow = summarySheet.Cells(summarySheet.Cells.Count, 1).End(xlUp).Row

    Dim service As String
    Dim supplier As String
    Dim priceRange As String
    Dim price As String

    Dim currentSheet As Worksheet
    Dim currentSheetName As String
    Dim currentRow As Long, currentColumn As Long
    Dim lastRow As Long

    Dim sheetCounter As Long
    For sheetCounter = LBound(sheetNames) To UBound(sheetNames)

        currentSheetName = sheetNames(sheetCounter)
        Set currentSheet = ThisWorkbook.Worksheets(currentSheetName)

        lastRow = currentSheet.Cells(currentSheet.Rows.Count, 1).End(xlUp).Row

        For currentRow = 5 To lastRow
            currentSummaryRow = currentSummaryRow + 1

            For currentColumn = 12 To 47

                priceRange = currentSheet.Cells(2, 1).Text
                service = currentSheet.Cells(currentRow, 1).Text
                supplier = currentSheet.Cells(4, currentColumn).Text
                price = currentSheet.Cells(currentRow, currentColumn).Text

                summarySheet.Cells(currentSummaryRow, 1) = service
                summarySheet.Cells(currentSummaryRow, 2) = supplier
                summarySheet.Cells(currentSummaryRow, 3) = priceRange
                summarySheet.Cells(currentSummaryRow, 4) = price

            Next currentColumn
        Next currentRow
    Next sheetCounter

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True

    MsgBox ("Complete")

End Sub

Now things are becoming a lot clearer, and we can move on to the next stage of making this code useful. Namely, explaining and documenting important information.

Magic Numbers

A magic number is any hard-coded value which appears in your code.

Why does a start at 5?

Why does b only go from 12 to 47?

Why is supplier always pulled from row 5?

Why is service always pulled from column 1?

Why are they laid out in columns 1-4 of the summary sheet, and in that order?

How do we know that our worksheets haven't been renamed?

For each of these questions, you should either re-structure your code so they don't have to be hard-coded, or you leave a note explaining why they have the values they have and put them in appropriate variables.

In this case, I recommend constants. Like so:

'/ Each sheet is laid out with Suppliers on row 4, Service in column 1, and then price values in a grid.
Const SUPPLIER_ROW As Long = 4
Const SERVICE_COLUMN As Long = 1

...

...

For currentRow = SUPPLIER_ROW + 1 to finalRow
    For currentColumn = SERVICE_COLUMN + 1 to finalColumn

    ...

    ...

And now, if your data ever moves around, you only have to go and change that value in one place. And everywhere else in your code, you can refer to your constant by name, rather than trusting that you'll remember why the numbers are what they are.

Arrays

This is where we're going to give you a serious performance tune-up. I expect it will solve all of your speed and memory problems.

Doing anything to a Worksheet is a huge operation. If you write

priceRange = currentSheet.Cells(2, 1).Text

it doesn't feel like a big operation, but you have to query the Worksheet Object, which queries the Cells object, which searches through a couple billion range objects to find the one you're after, which then gets queried to determine the text value it's currently displaying. This also triggers Worksheet Events (query handlers, events, calculations, screenUpdating, validation, etc.) which fire off their own chains of cascading events and so on and so forth.

And you're doing this roughly 2 Million times.

(N.B. the above is for illustration only. The actual chain of events is much more complex and convoluted).

When working with data, what you want is an Array. An Array is just a grid of values laid out in memory, so querying a value from an Array is literally a trillion times faster than querying it from a Worksheet object.

VBA makes this incredibly easy. Just create a Range that encompasses all your data, and do the following:

Dim dataArray As Variant
dataArray = dataRange

And now, whatever was in the topLeft Cell of your Range is in dataArray(1, 1). Next row down dataArray(2, 1) and so forth. you can then read this data back to a worksheet by doing the same in reverse:

pasteRange = dataArray

The following is your code, re-written to use Arrays, and it will be quite literally a thousand times faster:

Sub AggregateSheetData()

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False

    '/ row/column positions on data sheets
    Const SUPPLIER_ROW As Long = 4
    Const SERVICE_COLUMN As Long = 1
    Const START_COLUMN As Long = 12

    Const SUMMARY_SHEET_NAME As String = "Summary"

    Dim sheetNames As Variant
    sheetNames = Array("<25K", "25K <100K", "100K <250K", "250K <500K", "500K <1M", "1M <5M", "5M <15M", "15M <30M", "30M <50M")

    Dim summaryData As Variant
    ReDim summaryData(1 To 4, 1 To 1)
    Dim summaryCounter As Long

    Dim service As String
    Dim supplier As String
    Dim priceRange As String
    Dim price As String

    Dim dataRange As Range
    Dim sheetData As Variant
    Dim finalRow As Long, finalColumn As Long
    Dim iRow As Long, iColumn As Long

    Dim currentSheet As Worksheet
    Dim currentSheetName As String

    Dim sheetCounter As Long
    For sheetCounter = LBound(sheetNames) To UBound(sheetNames)

        currentSheetName = sheetNames(sheetCounter)
        Set currentSheet = ThisWorkbook.Worksheets(currentSheetName)

        With currentSheet

            priceRange = .Cells(2, 1).Text

            finalRow = .Cells(.Rows.Count, SERVICE_COLUMN).End(xlUp).Row
            finalColumn = .Cells(SUPPLIER_ROW, .Columns.Count).End(xlToLeft).Column

            Set dataRange = .Range(.Cells(SUPPLIER_ROW, SERVICE_COLUMN), .Cells(finalRow, finalColumn))

        End With

        sheetData = dataRange

        Dim LB1 As Long, UB1 As Long
        Dim LB2 As Long, UB2 As Long
        LB1 = LBound(sheetData, 1)
        UB1 = UBound(sheetData, 1)
        LB2 = LBound(sheetData, 2)
        UB2 = UBound(sheetData, 2)

        For iRow = LB1 To UB1
            service = sheetData(iRow, LB2)

            For iColumn = (START_COLUMN - SERVICE_COLUMN + 1) To UB2

                supplier = sheetData(LB1, iColumn)
                price = sheetData(iRow, iColumn)

                summaryCounter = summaryCounter + 1
                ReDim Preserve summaryData(1 To 4, 1 To summaryCounter)

                summaryData(1, summaryCounter) = service
                summaryData(2, summaryCounter) = supplier
                summaryData(3, summaryCounter) = priceRange
                summaryData(4, summaryCounter) = price

            Next iColumn
        Next iRow
    Next sheetCounter

    Dim summarySheet As Workbook
    Set summarySheet = ThisWorkbook.Worksheets(SUMMARY_SHEET_NAME)

    Dim pasteRange As Range
    With summarySheet

        finalRow = .Cells(.Rows.Count, 1).End(xlUp).Row

        Set pasteRange = .Range(.Cells(finalRow + 1, 1), .Cells(finalRow + summaryCounter, 4))
        pasteRange = summaryData

    End With

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True

    MsgBox ("Complete")

End Sub


Python program to calculate the area of two-dimensional shapes This is the first mini-project I have completed by myself with Python 2.7. I wrote a simple program that calculates the area of different two-dimensional shapes and it works. I want to know what you think of it and also want to know how it can be improved and if I did something wrong, so this is my code:

def circle(pi,radius):
    area_circle = pi * (radius ** 2)
    print " the area of your circle equals %s" % (area_circle)
def rectangle(length, width):
    area = length * width
    print "The area of your rectangle equals %s" %(area)
def triangle(height, base):
    area_triangle = height * base * 0.5
    print "the area of your triangle equals %s" % (area_triangle)
def square(length):
    area_square = length ** 2
    print "the area of your square equals %s" % (area_square)
shapes = ["square", "rectangle", "triangle", "circle"]
print "Area calculator!"
print "Made by Ahmed Fawzy"
print "-------------------------------"
def calculation():
    print "please type the name of your shape"
    print "(Square, Rectangle, Triangle, Circle)"
    user_shape = raw_input()
    if user_shape == shapes[0]:
        square(length = float(raw_input("please type the length of the square.")))
    elif user_shape == shapes[1]:
        rectangle(length = float(raw_input("please type the length of the rectangle.")), width = float(raw_input("please type the width of the rectangle.")))
    elif user_shape == shapes[2]:
        triangle(height = float(raw_input("please type the height of the triangle.")), base = float(raw_input("please type the base length of the triangle.")))
    elif user_shape == shapes[3]:
        circle(3.14, radius = float(raw_input("please type the radius of the circle.")))
    else:
        print "That's not in the choices!, Try again."
        calculation()
calculation()
choice = raw_input("Would you like to calculate the area of a different shape?(yes/no)")
while choice == "yes":
    print "---------------------"
    calculation()
    choice = raw_input("Wanna go again?")

There's one more thing I'd like to add but don't know how, I want to make it infinite loop when the user types an answer different than 'yes' or 'no' to the "would you like to calculate the area of a different shape?" , until the user types the correct answer which is yes or no then the loop breaks and it calculates again, any tips?

Generally looks good so far (I especially like how you set length and width to an inline call to raw_input() -- less code's better).

One suggestion is to only take radius as a parameter to the circle function and instead use the math.pi constant and import math at the top of the program, making the top of your program the following:

import math
def circle(radius):
   area_circle = math.pi * (radius ** 2)
   print " the area of your circle equals %s" % (area_circle)

To add the thing where you keep prompting the user for yes or no until they enter on of them, although I suppose I can't give you code (think that's what StackOverflow is for), think about initializing the response string to something that isn't "yes" or "no" outside a while loop where you keep checking if it's a valid response and asking the user for input...


Adding credentialsID node to SVN location nodes I'm new to ruby. Recently I wrote a script to add a credentialsID node to each SVN location node in each config.xml file for all of our Jenkins jobs (We have many).

It works, but I'm guessing it could be done more elegantly. How would you fix this to be more succinct, clean, etc.

require 'nokogiri'

Dir["somepathhere/.jenkins/jobs/*/config.xml"].each do |filename|

    file = File.open(filename, 'r')
    doc = Nokogiri::XML(file)
    file.close
    locations = doc.xpath('/project/scm/locations/hudson.scm.SubversionSCM_-ModuleLocation')
    locations.each do |location|
        crednode = location.at_xpath('credentialsId')
        if crednode
            crednode.content = 'somevalue'
        else
            location.first_element_child.before("<credentialsId>somevalue</credentialsId>\n")
        end
    end
    File.open(filename, 'w') { |file| file.print(doc.to_xml) }
end

You can take a few shortcuts here and there, but overall it's not bad.

My comments:

You don't need both open and close when you just want to read everything; File.read(filename) will work just fine
I'd like to see some constants or command line arguments used in place of the many hardcoded strings. E.g. tag names and xpaths could be constants, while the project directory's path and credentials string could be provided from the command line.
Either one would make the script more maintainable and reusable.
Don't insert the new credentials tag as a raw string; use doc.create_element instead
The if...else isn't strictly necessary; you can just look for and remove any existing credentialsId tags, and then insert your own (though that may mess with the file's whitespace a little)
The Ruby convention is to use 2 spaces of indentation, not 4 (and not tabs)

My Nokogiri is a bit rusty (no pun intended1), but I imagine this should work too.
I've left the project path and the credentials string hard-coded because I don't know if you'll want those from ARGV or something.

require 'nokogiri'

CONFIG_FILE_GLOB = ".jenkins/jobs/*/config.xml"
LOCATION_XPATH = "/project/scm/locations/hudson.scm.SubversionSCM_-ModuleLocation"
CREDENTIALS_TAG = "credentialsId"

Dir["somepathhere/#{CONFIG_FILE_GLOB}"].each do |filename|
  doc = Nokogiri::XML(File.read(filename)) # simplified file read

  # create a credentials element ahead of time
  credentials = doc.create_element(CREDENTIALS_TAG, "somevalue")

  doc.xpath(LOCATION_XPATH).each do |location|
    location.xpath(CREDENTIALS_TAG).remove # remove any existing credentials
    location.add_child(credentials.clone) # insert a new one
  end

  File.write(filename, doc.to_xml) # slightly simpler file write
end

Note that in order to insert the new element properly, we must always insert a clone. If you just use add_child(credentials), and there are multiple locations, the element will end up in the last of those locations, because add_child will add or re-parent the element. So the first add_child call would add it, and subsequent ones would move it since it's the same object. Hence, the need for clone.

1) because nokogiri means hand saw


Thread-safe LRU cache Please review my code. I am trying to implement a thread-safe LRU cache using std::list and std::unordered_map. This is a header-only template class which you can use to keep an object cache.

The constructor takes a size as an input parameter and creates a cache of a specified size. Items will be removed based on the least recently used from the cache if the cache goes above the size.

#ifndef LRUCache_h__
#define LRUCache_h__

#pragma once
#include <vector>
#include <list>
#include <unordered_map>
#include <shared_mutex>
#include <iostream>

namespace objcache {

template <class T>
class LRUCache
{
    struct _node {
        int _key;
        T    _value;
    };

    // use map of key to iterator of LRU list
    using ListIterator = typename std::list<_node>::const_iterator;
    using CacheMap = std::unordered_map<int, ListIterator>;

    size_t                m_cacheSize;
    std::list<_node>    m_lruList;
    CacheMap            m_lruMap;
    std::mutex            m_lock;            // reader writer lock, multiple getters are allowed

public:

    /***
     * Method:        objcache::LRUCache::LRUCache
     * Access:        public
     * Description: This creates a cache of given size.
     */
    LRUCache(size_t size) :
        m_cacheSize{ size },
        m_lruList{},
        m_lruMap{},
        m_lock{}
    {
        //m_lruList.resize(m_cacheSize, 0);
        m_lruMap.reserve(m_cacheSize);
    }

    /***
     * Method:    objcache::LRUCache::set
     * Access:    public
     * Description : This API updates or sets the value val to key.
     */
    void Set(int key, const T &val) noexcept
    {
        std::lock_guard<std::mutex> guard(m_lock);

        if (auto mapIter = m_lruMap.find(key); mapIter != m_lruMap.end()) {
            //key already exists, just update the queue
            m_lruList.splice(m_lruList.begin(), m_lruList, mapIter->second);
        }
        else {
            // new item in the cache.
            m_lruList.push_front({ key, val });
            m_lruMap.insert(std::make_pair(key, m_lruList.begin()));
        }

        //remove least recently used element if cache is full.
        if (m_lruList.size() > m_cacheSize) {
            m_lruMap.erase(m_lruList.back()._key);
            m_lruList.pop_back();
        }
    }

    /***
     * Method:        objcache::LRUCache::get
     * Access:        public
     * Description: This API gets the value of key if it exists in cache,
     *                otherwise, just returns 0
     */
    T Get(int key) noexcept
    {
        std::lock_guard<std::mutex> gaurd(m_lock);
        T val = 0;
        if (auto key_iter = m_lruMap.find(key); key_iter != m_lruMap.end()) {
            m_lruList.splice(m_lruList.begin(), m_lruList, key_iter->second);
            val = (key_iter->second)->_value;
        }
        return val;
    }

    /***
     * Method:    objcache::LRUCache::printCache
     * Access:    public
     * Description : Prints the keys of the cache one-by-one
     */
    void PrintCache() noexcept
    {
        std::lock_guard<std::mutex> guard(m_lock);
        for (const auto &node : m_lruList)
            std::cout << node._key << std::endl;
    }

};

} // namespace objcache

#endif // LRUCache_h__

Not repeating what 1201ProgramAlarm reported.

std::mutex    m_lock;   // reader writer lock, multiple getters are allowed

I see a regular exclusive lock, not a reader/writer lock.
std::lock_guard<std::mutex> gaurd(m_lock);

Not taking advantage of class template argument deduction?  This is a case where I really want it.  (If not, use an alias for the lock type so it can be easily changed.)
std::lock_guard guard(m_lock);

    m_lruList{},
    m_lruMap{},
    m_lock{}

Just adds noise — is that a thing someone actually recommends?  I would say don’t list all the things that are taken care of on their own.
   for (const auto &node : m_lruList)
        std::cout << node._key << std::endl;

The style in C++ is to put the * or & with the type, not the identifier.  This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.
Some people criticize the leading underscore of identifiers as a style.  I point out that it is easy to clash with compiler-supplied identifiers and you must remember to never use a capital letter after the score, so it is a hazard.

Update
#ifndef LRUCache_h__

Any name containing a double underscore is reserved for use by the implementation.

/***
 * Method:        objcache::LRUCache::LRUCache
 * Access:        public
 * Description: This creates a cache of given size.
 */

Banner comments like this just get in the way of reading the code.  Repeating the function name in a comment just before its definition does what exactly?  (Oh, and a point of nomenclature:  C++ does not have “methods”.)  Nobody comments each function as being public or private.  For the description, don’t prefix it with “Description:”; just describe it.
Since I can see name of the argument is size and see how it’s used on the on the following line
LRUCache(size_t size) :
    m_cacheSize{ size },

a comment saying
// This creates a cache of given size

is useless.  It only repeats what is in the signature:  A constructor creates an instance of this class, and this one takes a size.
See (⧺P.1).  You might also find this useful (Comments starts on page 17; material originally published in 1992).


Sorting an array of integers in C++ This is a follow up to my previous post as it contained a lot of errors and wasn't accurate.
I am using the Bubble sort algorithm to sort an array of integers and I would like to know whether I could optimize it in any way.
#include<iostream>

void printarr(int *arr,const int siz){
    for(int i = 0;i < siz;i++) std::cout << arr[i] << " ";
    std::cout << "\n";
}

int main(){
    const int siz = 6;
    int arr[siz] = {4,6,3,1,3,8};
    std::cout << "Before sort\n";
    printarr(arr,siz);
    for(int i = 0;i < siz;i++){
        for(int j = i+1;j < siz;j++){
            if(arr[i] > arr[j]){
                int temp = arr[i];
                arr[i] = arr[j];
                arr[j] = temp;
            }
        }
    }
    std::cout << "After sort\n";
    printarr(arr,siz);
}
```

First thing first, make it into a function, such that your main will be
int main(){
    const int siz = 6;
    int arr[siz] = {4,6,3,1,3,8};

    std::cout << "Before sort\n";
    printarr(arr,siz);

    sort(arr, siz);

    std::cout << "After sort\n";
    printarr(arr,siz);
}

Since you've tagged it c++, do not use c-style array. Use std::vector. Use std::swap. Currently, it is plain old c. Nothing wrong with it, except the tag.

Optimization wise, well, bubble sort does not deserve it.


JAVA OOP concept issue Hello guys could you help me please how to render this code as OOP?
I want to improve this code and follow the OOP anyone could help me, please
thank you in advance
this is class Expression
class Expression {
        public int type;
        public int value;
        public Expression leftOp;
        public Expression rightOp;

        public Expression(int type, int value, Expression leftOp, Expression rightOp) {
            this.type = type;
            this.value = value;
            this.leftOp = leftOp;
            this.rightOp = rightOp;
        }
    }

    class Arith {

/** Constantes pour representer les types */
        public static final int TYPE_NUMBER = 1;
        public static final int TYPE_SUM = 2;
        public static final int TYPE_PROD = 3;

    public static void main(String[] args) {

            Expression term = new Expression(TYPE_SUM, 0, new Expression(TYPE_NUMBER, 3, null, null), new Expression(
                    TYPE_PROD, 0, new Expression(TYPE_NUMBER, 2, null, null), new Expression(TYPE_NUMBER, 5, null, null)));
            System.out.println(evaluate(term));
        }

        /** Evalue recursivement the expression */
        public static int evaluate(Expression term) {
            switch (term.type) {
            case TYPE_NUMBER:
                return term.value;
            case TYPE_SUM:
                return evaluate(term.leftOp) + evaluate(term.rightOp);
            case TYPE_PROD:
                return evaluate(term.leftOp) * evaluate(term.rightOp);
            default:
                return 0;
            }
        }
    }

The basic structure isn't bad, but is more structured programming in the C or PASCAL tradition than OOP.
OOP discussion
You currently have three types of expressions that need different evaluation behaviour, but use only one class Expression. You implement this differing behaviour in your main class, using a switch on a type code.
Instead, make Expression an interface (or a super class if you aren't comfortable yet with interfaces) with an evaluate() method, and three classes implementing that interface: NumberExpression, SumExpression, and ProductExpression, each with its own way of doing the evaluate() method, with its own fields, and its own constructor (using only the parameters necessary to that type of expression).
Then, there's no need for a type code any more. The Java OOP system does the switch for you. The evaluation of an expression happens where it should happen, in the expression's own class, and you can eliminate the main evaluate() method. And it's totally easy to create additional expression types like difference, quotient or power. You just add another class.
The main idea of OOP is that an object's behaviour should be implemented in the corresponding class, especially if the behaviour depends on something you'd call a type.
Minor points
The type code you are currently using (if it weren't made obsolete by the OOP refactoring) should not be a list of integer constants, but become an enum.
Avoid public fields, make them private, and if really needed, create getter and setter methods. Fields represent some internal state of an object, and nobody outside should manipulate that state, or even have any assumptions on the inner workings of a class. Communication should only happen by calling public methods. (Fields can be viewed as private sticky notes of some clerk, they're not meant to be seen or modified by anybody else, and even asking this clerk about one of these notes seems like bad habit.)
If you have distinct cases how to create instances of a class, create multiple constructors. Your single Expression constructor is a "one-size-fits-all" one. When creating a constant number, you still have to supply two irrelevant Expressions (given as null in your example), and when creating a sum or a product, you are forced to provide an equally irrelevant value parameter.


wxpython program to clear the Desktop or restore files to the Desktop This is a program that clears the desktop and also undoes the clear. How does the code look? Could I make it look better?

import wx
import os
import shutil
import time

class Window(wx.Frame):
    def __init__(self, parent, id):
        self.Desktop = "C:/Users/Name/Desktop/" and "C:/Users/Public/Desktop/"
        self.Dfiles = "C:/ Desktop_Files/"
        no_caption = wx.DEFAULT_FRAME_STYLE | wx.STAY_ON_TOP
        wx.Frame.__init__(self, parent, id, title="no_caption", size=(300,97), style=no_caption)
        self.panel=wx.Panel(self)
        self.Butt_Clear = wx.Button(self.panel, -1, label="Clear Desktop", pos=(0,0), size=(100,70))
        self.Butt_Clear.Bind(wx.EVT_BUTTON, self.ClearDesk, self.Butt_Clear)
        self.Butt_Undo = wx.Button(self.panel, -1, label="Undo Clear", pos=(185,0), size=(100,70))
        self.Butt_Undo.Bind(wx.EVT_BUTTON, self.UndoClear, self.Butt_Undo)
    def ClearDesk(self, e):
        MainDesktop = os.listdir(self.Desktop)
        MainDesktop.remove('desktop.ini')

        for Desktop_Path_Names in MainDesktop:
            Desktop_Path = os.path.join(self.Desktop, Desktop_Path_Names)
            shutil.move(Desktop_Path, self.Dfiles)

    def UndoClear(self, e):
        Dfolder = os.listdir(self.Dfiles)
        Dfolder.remove('desktop.ini')
        for Desktop_Folder_Path_Names in Dfolder:
            Dfolder_Path = os.path.join(self.Dfiles, Desktop_Folder_Path_Names)
            shutil.move(Dfolder_Path, self.Desktop)

if __name__=='__main__':
    app=wx.App(False)
    frame=Window(parent=None, id=-1)
    frame.Show()
    app.MainLoop()

Your class name is misleading; it says "Window" as if you are trying to draw a window for a GUI purpose, but that is not what that class actually performs. So you should rename it to something else meaningful to enhance the readability of your program.
Parameters and arguments constitute a  level of abstraction which is different from that of the function (or class construction) they are injected it. You should minimize them as much as you can, or avoid them totally if that is possible. In your case, the initializer of your class has a parent parameter which is never used (in your main, you assigned None to it), so it is better you get rid of it as it is useless for your case.
Always in the initializer of your class, you used the id parameter only here:

wx.Frame.__init__(self, parent, id, title="no_caption", size=(300,97), style=no_caption)

So for the same reason as mentioned through the second bullet above, I suggest you to get rid of it. Thus, your initializer should become like this:

def __init__(self):
    ...
    ...

The work is not finished with your __init__(). You should clean it by keeping only what is necessary there, otherwise delegate all the remaining tasks for functions to take care of them. I say your __init__() is not clean because you mix widget creation and settings with some configurations and constants that your application needs. So I suggested you to move this:

self.Desktop = "C:/Users/Name/Desktop/" and "C:/Users/Public/Desktop/"
self.Dfiles = "C:/ Desktop_Files/"

into a different function. The only "constant" you need to keep in your class initializer is the parent  parameter, but you do not use it. In case you used it, it would be the only element that deserves to sit and have its own place in the initializer by running this instruction self.parent = parent
For the same reason, I suggest you to create a separate function to save the settings of your application:

def configure_gui(self):
    no_caption = wx.DEFAULT_FRAME_STYLE | wx.STAY_ON_TOP

Please do not create widgets within your class initializer. Delegate that task, again, for a separate function:

def create_widgets(self):
    wx.Frame.__init__(self, parent, id, title="no_caption", size=(300,97), style=no_caption)
    self.panel=wx.Panel(self)
    self.Butt_Clear = wx.Button(self.panel, -1, label="Clear Desktop", pos=(0,0), size=(100,70))
    self.Butt_Clear.Bind(wx.EVT_BUTTON, self.ClearDesk, self.Butt_Clear)
    self.Butt_Undo = wx.Button(self.panel, -1, label="Undo Clear", pos=(185,0), size=(100,70))
    self.Butt_Undo.Bind(wx.EVT_BUTTON, self.UndoClear, self.Butt_Undo)

In Python, we don't like camelCase and CamelCase naming conventions that you probably used in Java, JavaScript or elsewhere. Please follow the naming conventions as described in PEP8.


Sudoku solutions finder using brute force and backtracking I'm calculating all possible solutions for a given Sudoku board with at least 17 values. My approach is a basic backtracking approach and it works.

protected void calculateSolutions(final SudokuBoard input) {

    if (input.getFilledCells() == 81) {
        addSolution(input);
        return;
    }

    for (int grid = 0; grid < 9; grid++) {
        for (int pos = 0; pos < 9; pos++) {
            if (input.getValue(new Position(grid, pos)).isEmpty()) {
                for (int number = 1; number <= 9; number++) {
                    if (isPossibleAtPosition(grid, pos, number)) {

                        SudokuBoard newInput = input.clone();
                        newInput.setValue(new Position(grid, pos), new Value(number));
                        List<SudokuBoard> results = new SudokuBacktrackSolver().solve(newInput);
                        for (SudokuBoard result : results) {
                            addSolution(result);
                        }
                    }
                }
                return;
            }
        }
    }
}

The function isPossibleAtPosition checks whether the number exists in the cell, row or grid. If it is possible, I create a new SudokuBoard, which ends up in a tree structure, where each solution has a depth of 81.

I also thought about creating a matrix containing all possibilities. This would improve the chance of finding a solution fastest but would produce a big overhead, and as I want to find all possible solutions, it would not be an improvement.

Are there any delimitations to improve the algorithm?

Instead of using a fixed order of filling in cells you can first take the cells with the least amount of options left.

In other words if you know that cell 5,8 can only contain 5 and 6 then try each of them now and reduce the possibilities of the other cells instead of having to come back millions of times just to find out that the cell can't be filled any more.

This requires that you track the number of options in the board.

With a bitset it's doable, though un-setting a square is harder because you need to readjust all affected squares.

class SudokuBoard implements Cloanable{

    private Map<Position, BitSet> grid = new HashMap<Position, BitSet>();
    //fill in constructor

    public void setValue(Position pos, int number){
        number = number - 1;//bits are index 0 based

        BitSet square = grid.get(pos);

        if(!square.get(number))
            throw new IllegalArgumentException("number " + number + " can't be set at position " + pos);

        square.clear();
        square.set(number);

        // for each square in the row, collumn and big square do
        // square.set(number, false);

    }

    public Position getSquareWithLeastNumberOfOptions(){

        Position result = null;
        int count = 10;
        for(Map.Entry<Position, BitSet> e : grid.entrySet()){
            if(e.getValue().cardinality() < count && e.getValue().cardinality() != 1){
                result = e.getKey();
                count = e.getValue().cardinality();
            }
        }
        return result;
    }

    public int getFirstPossibleNumber(Position pos){
        return grid.get(pos).getNextSetBit(0)+1;
    }

    //returns 0 when no more options
    public int getNextPossibleNumber(Position pos, int previous){

        return grid.get(pos).getNextSetBit(previous)+1;

    }

    public boolean isSolved(){

        for(Map.Entry<Position, BitSet> e : grid.entrySet()){
            if(e.getValue().cardinality() != 1){
                return false;
            }
        }
        return true;
    }

    public boolean isSolvable(){

        for(Map.Entry<Position, BitSet> e : grid.entrySet()){
            if(e.getValue().cardinality() == 0){
                return false;
            }
        }
        return true;
    }

    @Override
    public SudokuBoard clone(){
        try{
            SudokuBoard clone = (SudokuBoard)super.clone();
            clone.grid = new HashMap(grid);
            for(Map.Entry<Position, BitSet> e : clone.grid.entrySet()){
                e.setValue((BitSet)e.clone());
            }
            return clone;
        } catch(CloneNotSupportedException e){
             return null;//can't happen
        }
    }

}

Then the algorithm looks something like:

public Set<SudokuBoard> getAllSolutions(SudokuBoard board){
    if(board.isSolved)
        return Collections.singleton(board);
    if(!board.isSolvable())
        return Collections.emptySet();

    Set<SudokuBoard> result = new HashSet<SudokuBoard>();

    Position pos = board.getSquareWithLeastNumberOfOptions();
    for(int num = board.getFirstPossibleNumber(pos); num != 0; num = getNextPossibleNumber(pos, num)){
        SudokuBoard newBoard = board.clone();

        newboard.setValue(pos, num);
        result.addAll(getAllSolutions(newBoard));
    }

    return result;
}

This will still create a lot of new partially solved boards but if you add a unSetValue(Position) to SudokuBoard then you can reuse the original board and only need to clone it when returning it in the singleton: return Collections.singleton(board.clone());.