K&R Exercise 5-15: sorting with extra options As I'm learning to program in C, I'm making my way through K&R.

The goal of the exercise is to add a option to the program so that the sorting does not depend on upper or lower-case letters.

What are your opinions on my use of function pointers in this exercise?
Do you see any obvious beginner traps in the code?

#include "main.h"
char *pointersToLines[MAX_NO_OF_LINES];
int (*compareFunctionPointers[3])(void *, void *);
int main(int argc, char *argv[]) {

    // Store pointers to the diffrent types of
    // compare functions in a array of
    // function pointers.
    compareFunctionPointers[COMPARE_DEFAULT] = &strcmp;
    compareFunctionPointers[COMPARE_FOLDED] = &strcmpFold;
    compareFunctionPointers[COMPARE_NUMERICLY] = &numcmp;

    int numberOfLines;
    int sortDecreasing  = 0; // 1 if the input is to be sorted in a decreasing order
    int compareFunction = COMPARE_DEFAULT; // Index for the compare-function
    // Parse the command line arguments
    if (argc > 1) {
        for (int i = 1; *(argv + i); i++) {
            char *c = *(argv + i);
            if(strcmp(c, "-n") == 0)
                compareFunction = COMPARE_NUMERICLY;
            if(strcmp(c, "-r") == 0)
                sortDecreseingly = 1;
            if(strcmp(c, "-f"))
                compareFunction = COMPARE_FOLDED;

        }
    }

    if ((numberOfLines = readLines(pointersToLines, MAX_NO_OF_LINES)) >= 0) {
        qsort1((void **)pointersToLines, 0, numberOfLines - 1,
           (int (*)(void *, void *))compareFunctionPointers[compareFunction],
            sortDecreasing);

        writeLines(pointersToLines, numberOfLines);
        return 0;
    }
}

Sorting Function

void qsort1(void *v[], int left, int right, int (*comp)(void *, void *),
           int increesingOrDecresing) {
    int last, compare;
    //Base case
    if (left >= right) {
    return;
    }

    swap(v, left, (right + left) / 2);
    last = left;
    for (int i = left + 1; i <= right; i++) {
        compare = (increesingOrDecresing) ? -(*comp)(v[i], v[left])
                                    : (*comp)(v[i], v[left]);
    if (compare < 0)
        swap(v, ++last, i);
    }
    swap(v, last, left);
    qsort1(v, left, last - 1, comp, increesingOrDecresing);
    qsort1(v, last + 1, right, comp, increesingOrDecresing);
}

Compare functions

int strcmpFold(char *s1, char *s2) {
    while (toupper(*s1) == toupper(*s2)) {
        s1++;
        s2++;
    }
    if (toupper(*s1) < toupper(*s2))
        return -1;
    else if (toupper(*s1) > toupper(*s2))
        return 1;
    else
        return 0;
}

int numcmp(char *s1, char *s2) {
    double v1, v2;
    v1 = atof(s1);
    v2 = atof(s2);

    if (v1 < v2)
        return -1;
    else if (v1 > v2)
        return 1;
    else
        return 0;
}

I'd use a typedef for a pointer to your compare functions:

typedef int (*cmpfnc)(void *,void *);

This simplifies things and eliminates the ugly cast used in main's call to qsort1

I'd remove the & from &strcmp et. al. It is less idiomatic than strcmp. Both say "I want the address of ...". This allows the address of a function and a function pointer of the same type to be used interchangeably syntactically.

That is, you can say:

typedef int (*cmpfnc)(void *,void *);

cmpfnc myptr;

myptr = (cmpfnc) strcmp;

strcmp(x,y);
myptr(x,y);

And, they're all interchangeable. Also, note that (*myptr)(x,y) is simplified.

If you compile with [more] warning (e.g. -Wall), setting compareFunctionPointers needs a cast for strcmp. Using strcmp directly here is a slight blemish (because its signature uses const char * instead of void *, but it works in practice with the cast.

You could wrap strcmp with a wrapper function: int mystrcmp(void *left,void *right) { return strcmp(left,right); } as an alternative to using the cast. This would allow the compiler to do more type checking. But, using strcmp directly would probably run faster.

For most of your functions, you're breaking type safety of the arguments, so you have to be careful when doing that to be sure that things match up when the program runs.

Side note: You use sortDecreasing in main, but rename it in qsort1. Note that the the name in main is much more descriptive of its meaning. Also, you could replace the value with 1 or -1 and simplify qsort1 with a multiply (see my cleaned up code below).

Here is your code annotated with what I think could be cleaned up a bit [Note that it's based on the code in your original post, so I had to guess about some missing functions]:

#include <string.h>

#define MAX_NO_OF_LINES     100

// NOTE/BUG: this should be a typedef
int (*compareFunctionPointers[3]) (void *, void *);

void
swap(void **v,int left,int right)
{
    void *tmp;

    tmp = v[left];
    v[left] = v[right];
    v[right] = tmp;
}

int
numcmp(void *left,void *right)
{
    return *(int *) right - *(int *) left;
}

// NOTE/BUG: this should use a typedef
void
qsort1(void *v[], int left, int right, int (*comp) (void *, void *), int increesingOrDecresing)
{
    int last,
     compare;

    // Base case
    if (left >= right) {
        return;
    }

    swap(v, left, (right + left) / 2);
    last = left;
    for (int i = left + 1; i <= right; i++) {
        compare = (increesingOrDecresing) ? -(*comp) (v[i], v[left])
            : (*comp) (v[i], v[left]);
        if (compare < 0)
            swap(v, ++last, i);
    }
    swap(v, last, left);
    qsort1(v, left, last - 1, comp, increesingOrDecresing);
    qsort1(v, last + 1, right, comp, increesingOrDecresing);
}

enum {
    COMPARE_DEFAULT,
    COMPARE_FOLDED,
    COMPARE_NUMERICLY
};

char *pointersToLines[MAX_NO_OF_LINES];

int
main(int argc, char *argv[])
{

    // Store pointers to the diffrent types of
    // compare functions in a array of
    // function pointers.
    // NOTE/BUG: these require a cast
    // NOTE/BUG: leave off the "&" as it is more idiomatic
    compareFunctionPointers[COMPARE_DEFAULT] = &strcmp;
#if 0
    compareFunctionPointers[COMPARE_FOLDED] = &strcmpFold;
#else
    compareFunctionPointers[COMPARE_FOLDED] = &strcasecmp;
#endif
    compareFunctionPointers[COMPARE_NUMERICLY] = &numcmp;

    int numberOfLines;
    int sortDecreasing = 0;             // 1 if the input is to be sorted in a decreasing order
    int compareFunction = COMPARE_DEFAULT;  // Index for the compare-function

    // Parse the command line arguments
    if (argc > 1) {
        for (int i = 1; *(argv + i); i++) {
            char *c = *(argv + i);

            if (strcmp(c, "-n") == 0)
                compareFunction = COMPARE_NUMERICLY;
            if (strcmp(c, "-r") == 0)
                sortDecreasing = 1;
            if (strcmp(c, "-f"))
                compareFunction = COMPARE_FOLDED;

        }
    }

#if 0
    if ((numberOfLines = readLines(pointersToLines, MAX_NO_OF_LINES)) >= 0) {
#endif

    // NOTE/BUG: too much casting, particularly on compareFunctionPointers
        qsort1((void **) pointersToLines, 0, numberOfLines - 1, (int (*)(void *, void *)) compareFunctionPointers[compareFunction], sortDecreasing);

#if 0
        writeLines(pointersToLines, numberOfLines);
    }
#endif

    return 0;
}

Here is a more cleaned up version incorporating the ideas I've expressed above:

#include <string.h>

#define MAX_NO_OF_LINES     100

typedef int (*cmpfnc)(void *,void *);

cmpfnc compareFunctionPointers[3];

void
swap(void **v,int left,int right)
{
    void *tmp;

    tmp = v[left];
    v[left] = v[right];
    v[right] = tmp;
}

int
numcmp(void *left,void *right)
{
    return *(int *) right - *(int *) left;
}

void
qsort1(void *v[], int left, int right, cmpfnc comp, int sortdir)
{
    int last,
     compare;

    // Base case
    if (left >= right) {
        return;
    }

    swap(v, left, (right + left) / 2);
    last = left;
    for (int i = left + 1; i <= right; i++) {
        compare = sortdir * comp(v[i], v[left]);
        if (compare < 0)
            swap(v, ++last, i);
    }
    swap(v, last, left);
    qsort1(v, left, last - 1, comp, sortdir);
    qsort1(v, last + 1, right, comp, sortdir);
}

enum {
    COMPARE_DEFAULT,
    COMPARE_FOLDED,
    COMPARE_NUMERICLY
};

char *pointersToLines[MAX_NO_OF_LINES];

int
main(int argc, char *argv[])
{

    // Store pointers to the diffrent types of
    // compare functions in a array of
    // function pointers.
    compareFunctionPointers[COMPARE_DEFAULT] = (cmpfnc) strcmp;
    compareFunctionPointers[COMPARE_FOLDED] = (cmpfnc) strcasecmp;
    compareFunctionPointers[COMPARE_NUMERICLY] = numcmp;

    int numberOfLines;
    int sortDecreasing = 1;             // 1=increasing, -1=decreasing
    int compareFunction = COMPARE_DEFAULT;  // Index for the compare-function

    // Parse the command line arguments
    if (argc > 1) {
        for (int i = 1; *(argv + i); i++) {
            char *c = *(argv + i);

            if (strcmp(c, "-n") == 0)
                compareFunction = COMPARE_NUMERICLY;
            if (strcmp(c, "-r") == 0)
                sortDecreasing = -1;
            if (strcmp(c, "-f"))
                compareFunction = COMPARE_FOLDED;

        }
    }

#if 0
    if ((numberOfLines = readLines(pointersToLines, MAX_NO_OF_LINES)) >= 0) {
#endif

    // NOTE/BUG: too much casting, particularly on compareFunctionPointers
        qsort1((void **) pointersToLines, 0, numberOfLines - 1,
        compareFunctionPointers[compareFunction], sortDecreasing);

#if 0
        writeLines(pointersToLines, numberOfLines);
    }
#endif

    return 0;
}


Iteration over "zipped" tuples (for_each_in_tuples) I want to implement for_each_in_tuples that takes a functor and one or more tuples of the same size and applies this functor to i'th elements of each tuple for i = 0, ..., size - 1.

Example:

#include <iostream>
#include <tuple>

std::tuple t1(1, 2.2, false);
std::tuple t2(3.3, 'a', 888);

std::cout << std::boolalpha;
for_each_in_tuples(
    [](auto a1, auto a2)
    {
        std::cout << a1 << ' ' << a2 << '\n';
    }, t1, t2);

// Outputs:
// 1 3.3
// 2.2 a
// false 888

Implementation:

#include <cstddef>
#include <tuple>
#include <type_traits>

namespace impl
{
template<typename T, typename... Ts>
struct First { using Type = T; };

template<typename... Ts>
using First_t = typename First<Ts...>::Type;

template<auto value, auto... values>
inline constexpr auto all_same = (... && (value == values));

template<class Tuple>
inline constexpr auto tuple_size = std::tuple_size_v<std::remove_reference_t<Tuple>>;

template<std::size_t index = 0, class Function, class... Tuples>
constexpr void for_each_in_tuples(Function func, Tuples&&... tuples)
{
    constexpr auto size = tuple_size<First_t<Tuples...>>;

    func(std::get<index>(std::forward<Tuples>(tuples))...);
    if constexpr (index + 1 < size)
        for_each_in_tuples<index + 1>(func, std::forward<Tuples>(tuples)...);
}
}

template<class Function, class... Tuples>
constexpr void for_each_in_tuples(Function func, Tuples&&... tuples)
{
    static_assert(sizeof...(Tuples) > 0);
    static_assert(impl::all_same<impl::tuple_size<Tuples>...>);

    impl::for_each_in_tuples(func, std::forward<Tuples>(tuples)...);
}

At Compiler explorer: https://godbolt.org/g/cYknQT

Main questions:

Is this implementation correct and can it be simplified?
Which name is better, for_each_in_tuple or for_each_in_tuples (or ...)?

template<typename T, typename... Ts>
struct First { using Type = T; };

template<typename... Ts>
using First_t = typename First<Ts...>::Type;

This can be done using std::tuple_element in conjunction with std::tuple.

template <typename... Ts>
using First_t = std::tuple_element_t<0, std::tuple<Ts...>>;

template<auto value, auto... values>
inline constexpr auto all_same = (... && (value == values));

For an empty pack, do you really want the value to be undefined?  The default behavior with empty packs are && is considered true and || is considered false.

static_assert(sizeof...(Tuples) > 0);

Instead of failing, maybe try to call the function with 0 arguments and see what happens?

static_assert(impl::all_same<impl::tuple_size<Tuples>...>);

If you want zipped-like behavior, you'll want to zip tuples until one of the tuples has exhausted its elements (min size instead first size).

  Is this implementation correct and can it be simplified?

Yes. Use an iterative approach, like sequential expansion, over recursion.

  Which name is better, for_each_in_tuple or for_each_in_tuples (or ...)?

Maybe for_each_zipped.  Tuples can be gathered from the signature of the function.

If you want to avoid the recursion, use fold expressions, std::index_sequence, and std::make_index_sequence.

Start with a simple helper to just invoke the function with elements across all the tuples at a specific index.

template <std::size_t Index, typename Function, typename... Tuples>
constexpr void invoke_at(Function&& func, Tuples&&... tuples) {
    func(std::get<Index>(std::forward<Tuples>(tuples))...);
}

Now we need a way to sequentially call it (invoke_at<0>(args), invoke_at<1>(args), ..., invoke<N>(args)).  Use fold expressions like you did for all_same, but with the comma operator and unary right fold ((invoke_at<N>(args), ...)).  To generate the Ns that gets expanded, we use std::index_sequence.

template <std::size_t... Indices, typename Function, typename... Tuples>
constexpr void apply_sequence(Function&& func,  std::index_sequence<Indices...>, Tuples&&... tuples) {
    (((void)invoke_at<Indices>(std::forward<Function>(func), std::forward<Tuples>(tuples)...), ...));
}

Finally, write your function that does checks for preconditions, creates the index sequence, and forwards the arguments to the above helper.

template <typename Function, typename... Tuples>
constexpr void tuple_for_each(Function&& func, Tuples&&... tuples) {
    static_assert(sizeof...(tuples) > 0, "Must be called with at least one tuple argument");

    constexpr auto min_length = std::min({std::tuple_size_v<std::remove_reference_t<Tuples>>...});
    if constexpr (min_length != 0) {
        impl::apply_sequence(std::forward<Function>(func),
                             std::make_index_sequence<min_length>{},
                             std::forward<Tuples>(tuples)...);
    }
    else {
        func();
    }
}

Note - The expansion has a cast to void that disables any overloaded shenanigans abusing the comma operator.


Find Element in Tree Given a List of Directions Looking at Learn You a Haskell, I implemented a function that, given a binary tree, i.e. left and right branches, give a list of Directions and give the element at that location.

Here's the Tree:

data Tree a = Empty | Node a (Tree a) (Tree a) deriving (Show)

And then the functions:

elemAt :: Directions -> Tree a -> a
elemAt []     (Node x _ _) = x
elemAt (R:ds) (Node _ _ r) = elemAt ds r
elemAt (L:ds) (Node _ l _) = elemAt ds l

But, I re-wrote it to be safe (maybe not)

maybeElemAt :: Directions -> Tree a -> Maybe a
maybeElemAt []     (Node x _ _)     = Just x
maybeElemAt (R:ds) (Node _ _ Empty) = Nothing
maybeElemAt (R:ds) (Node _ _ r)     = maybeElemAt ds r
maybeElemAt (L:ds) (Node _ Empty _) = Nothing
maybeElemAt (L:ds) (Node _ l _)     = maybeElemAt ds l

Testing

anotherTree :: Tree Char
anotherTree = Node 'A' (Node 'B' Empty Empty) Empty

ghci> maybeElemAt [R,R,R] anotherTree
Nothing
ghci> maybeElemAt [R] anotherTree
Nothing
ghci> maybeElemAt [L] anotherTree
Just 'B'

Please critique my implementations of either function.

It's not that safe.

*Main> maybeElemAt [] Empty
*** Exception: tree.hs:(7,1)-(11,54): Non-exhaustive patterns in function maybeElemAt

So let's add a pattern

maybeElemAt :: Directions -> Tree a -> Maybe a
maybeElemAt []     (Node x _ _)     = Just x
maybeElemAt (R:ds) (Node _ _ Empty) = Nothing
maybeElemAt (R:ds) (Node _ _ r)     = maybeElemAt ds r
maybeElemAt (L:ds) (Node _ Empty _) = Nothing
maybeElemAt (L:ds) (Node _ l _)     = maybeElemAt ds l
maybeElemAt _ Empty = Nothing

And now we can see that two of the patterns can be removed:

maybeElemAt []     (Node x _ _) = Just x
maybeElemAt (R:ds) (Node _ _ r) = maybeElemAt ds r
maybeElemAt (L:ds) (Node _ l _) = maybeElemAt ds l
maybeElemAt _ Empty = Nothing

GHC has an option -fwarn-incomplete-patterns, or you can use -W to get that plus other warnings. See Warnings and sanity-checking in the GHC User's Guide.


How do I avoid multiple chain calls? Imagine code that reads from a file, cleans the data and finally writes the cleaned data to a new file.

Let's briefly define these methods as Ruby stubs:

def write(filename, data)
   File.open(filename, "w") do |f|
      f.write(data.to_yaml)
   end
end

def read(filename)
   YAML.load_file(filename)
end

def clean(data)
   data.map do |line|
      line.strip!
   end
end

We can also have an orchestrator method that triggers the whole logic:

def read_clean_write(filename)
    write(filename, clean(read(filename)))
end

I think that while the isolated read, write and clean methods are modular and apply the single responsibility principle, read_clean_write is quite the opposite (and looks ugly). What is the best way to do that while keeping SRP?

One way would be to wrap it in an object with each method returning self so that methods can be chained.

class DataObject
  def initialize(data)
    @data = data
  end

  def clean
     @data = @data.data.map(&::strip)
     self
  end

  def write(filename)
    File.open(filename, "w") do |f|
      f.write(@data.to_yaml)
    end
    self
  end

end

def read(filename)
  data = YAML.load_file(filename)
  DataObject.new(data)
end

Another way would be to use mixins.

module DataMethods

  def clean
    # need to use the destructive methods to modify self
    map!(&::strip)
    # or each(&:strip!)
    self
  end

  def write(filename)
    File.write(filename, to_yaml)
    self
  end

end

def read(filename)
  YAML.load_file(filename).extend(DataMethods)
end


Making a Pong ball reflect off the paddle I am currently making Pong in C++/SDL2 and after a while of tinkering, I have come up with a solution!

This function gets the hit position and returns the necessary direction change:

MoveDirection Ball::getHitPos(const Player& player) const
{
    MoveDirection dir; //Direction of movement

    //If ball is moving left
    if (m_dir == MoveDirection::LEFT)
    {
        std::cout << "Yes";
        int hitPos = m_pos.y - player.getPos().y; //Get hit position
        int playerSize = player.getPos().h / 2;
        int offSet = 25; //Enables to split up positions into three seperate parts
        //Calculate dir change
        if (hitPos <= playerSize - offSet) {
            dir = MoveDirection::RIGHTUP;
        }
        else if (hitPos >= playerSize + offSet) {
            dir = MoveDirection::RIGHTDOWN;
        }
        else {
            dir = MoveDirection::RIGHT;
        }
    }

    //If Ball is moving right
    if (m_dir == MoveDirection::RIGHT)
    {
        int hitPos = m_pos.y - player.getPos().y; //Get hit position
        int playerSize = player.getPos().h / 2;
        int offSet = 25; //Enables to split up positions into three seperate parts
        //Calculate dir change
        if (hitPos <= playerSize - offSet) {
            dir = MoveDirection::LEFTUP;
        }
        else if (hitPos >= playerSize + offSet) {
            dir = MoveDirection::LEFTDOWN;
        }
        else {
            dir = MoveDirection::LEFT;
        }
    }

    return dir;
}

Here are all of the possible movement directions:

enum class MoveDirection
{
    UP,
    DOWN,
    LEFT,
    RIGHT,
    LEFTUP, //Ball diagonal movement
    LEFTDOWN, //Ball diagonal movement
    RIGHTUP, //Ball diagonal movement
    RIGHTDOWN //Ball diagonal movement
};

And here is the height of the racket:

static constexpr int SIZE_HEIGHT = 75;

I must also note that m_pos is SDL_RECT and player.getPos().y is getting the y element of SDL_RECT.

Your ball can only move in the eight (semi)cardinal directions? It can't move at, say, a 30-degree angle?

Well, even so, it will simplify your code to treat the ball as if it can move in any direction. Either:

struct MoveDirection {
    double angle;
    double speed;
};

or more likely:

struct MoveDirection {
    double vx;
    double vy;
};

And then rewrite your getHitPos function as something more like

MoveDirection Ball::getHitPos(const Player& player) const
{
    MoveDirection dir; //Direction of movement

    //If ball is moving left
    if (m_dir.vx < 0)
    {
        std::cout << "Yes";
        int hitPos = m_pos.y - player.getPos().y; //Get hit position
        int playerSize = player.getPos().h / 2;
        int offSet = 25; //Enables to split up positions into three seperate parts
        //Calculate dir change
        dir.vx = +1;
        if (hitPos <= playerSize - offSet) {
            dir.vy = +1;
        }
        else if (hitPos >= playerSize + offSet) {
            dir.vy = -1;
        }
        else {
            dir.vy = 0;
        }
    }
    else if (m_dir.vx > 0)  // ball is moving right
...

Then just keep refactoring and turning branches into arithmetic expressions until you're happy. It might end up looking something like

MoveDirection result;
result.vx = -m_dir.vx;
const double hitPos = m_pos.y - player.getPos().y; //Get hit position
const double playerSize = player.getPos().h / 2;
result.vy = (hitPos > playerSize + 25) - (hitPos < playerSize - 25);
return result;


A Public and Private Key generator for RSA I made an RSA encryption public and private key generator and I would like some feedback. In particular, as this is cryptography related I want to know if I have done anything that might be considered insecure.

FYI, the Crypto.Random module is from the crypto-api package.

(Disclaimer, I am not using this for anything serious, just to help understand how the process works.)

import System.Random
import Crypto.Random
import Crypto.Random.DRBG
import Control.Monad.CryptoRandom
import Data.Bits
import qualified Data.ByteString as BS

data PublicKey  = PublicKey Integer Integer deriving (Show,Eq)
data PrivateKey = PrivateKey Integer Integer deriving (Show,Eq)

defaultExp = 65537

modExp :: Integer -> Integer -> Integer -> Integer
modExp b 0 m = 1
modExp b e m = t * modExp ((b * b) `mod` m) (shiftR e 1) m `mod` m
  where t = if testBit e 0 then b `mod` m else 1

fermatPrimeTest :: StdGen -> Int -> Integer -> Bool
fermatPrimeTest g k n = all (\a -> modExp a (n-1) n == 1) as
  where
    as = take k $ randomRs (2,n-2) g

totient :: Integer -> Integer -> Integer
totient p q = lcm (p-1) (q-1)

-- Find GCD of two numbers plus the Coefficients of Bezouts Identity.
-- Used to find modular inverse.
euclideanAlg :: Integer -> Integer -> (Integer, Integer, Integer)
euclideanAlg a b
  | b > a     = tripFlip $ euclideanAlg2 b 1 0 a 0 1
  | otherwise = euclideanAlg2 a 1 0 b 0 1
  where
    tripFlip (a,b,c) = (a,c,b)
    euclideanAlg2 rk0 sk0 tk0 0 sk1 tk1 = (rk0,sk0,tk0)
    euclideanAlg2 rk0 sk0 tk0 rk1 sk1 tk1 =
        let qi = rk0 `div` rk1 in
        euclideanAlg2 rk1 sk1 tk1 (rk0 - qi*rk1) (sk0 - qi*sk1) (tk0 - qi*tk1)

-- Modular inverse, d, of a such that a.d = 1 mod m
modMultInv :: Integer -> Integer -> Integer
modMultInv m a = let (r,_,d) = euclideanAlg m a
                 in d `mod` m

-----------------
-- Prime Number Generator using Secure RNG
-----------------

genPrime :: CryptoRandomGen g => g -> Int -> Integer -> Integer -> Integer
genPrime g k minPrime maxPrime = head $ filter (fermatPrimeTest g' k) ns
  where
    Right (i,g'') = crandom g
    g'            = mkStdGen i
    Right (n,_)   = crandomR (minPrime, maxPrime) g''
    ns            = iterate ((+) 2) (n .|. 1)

genPQ :: CryptoRandomGen g => g -> Int -> Integer -> Integer -> (Integer,Integer)
genPQ g k minPrime maxPrime = (p,q)
  where
    Right (g1,g2) = splitGen g
    p = genPrime g1 k minPrime maxPrime
    q = genPrime g2 k minPrime maxPrime

-- genRSAKeys e g k minPrime maxPrime
-- e is public exponent, g is random seed,
-- k is number of iterations to run Rabin-Miller test.
-- minPrime, maxPrime is range to search for primes.
genRSAKeys :: CryptoRandomGen g => Integer -> g
           -> Int -> Integer -> Integer
           -> (PublicKey, PrivateKey)
genRSAKeys e g k minPrime maxPrime = let
                                        (p,q) = genPQ g k minPrime maxPrime
                                        n    = p*q
                                        t    = lcm (p-1) (q-1)
                                        d    = modMultInv t e
                                     in (PublicKey n e, PrivateKey n d)

I can't really comment on the RSA key generation itself, since I only know the basics. From a layman's perspective it seems fine, but I cannot touch on details. However, there are some other parts that we can focus on.

Good code is hard to write, especially cryptographic one. Whenever we read cryptographic code, it's usually a good idea to have the algorithm or the original paper at hand to follow the steps. Since we're already occupied with the state machine and the mathematics behind the scenes, the code should be easy to read.

Let us have a look at modExp:

modExp :: Integer -> Integer -> Integer -> Integer
modExp b 0 m = 1
modExp b e m = t * modExp ((b * b) `mod` m) (shiftR e 1) m `mod` m
  where t = if testBit e 0 then b `mod` m else 1

That's the usual double-and-add method for b ^ e `mod` m, and it's completely fine. However, testBit and shiftR are misleading. We're not using some intricate bit patterns here and xor values later, nor do we use the number of bits as a seed. We just want to check whether e is currently odd and divide by two:

-- | Returns (b ^ e) `mod` m.
modExp :: Integer -> Integer -> Integer -> Integer
modExp b 0 m = 1
modExp b e m = t * modExp ((b * b) `mod` m) (e `div` 2) m `mod` m
  where t = if odd e then b `mod` m else 1

Note that you want to replace div by quot and mod by rem if all your values are guaranteed to be positive if you focus on performance, since quot/rem is slightly faster than div/mod. However, their result differs for negative values.

Speaking of positive, defaultExp is missing its type signature. Apart from that, it's great that every function has a type signature, although additional documentation wouldn't harm, e.g.

-- | Generates 'k' random numbers to check whether `n` is prime.
fermatPrimeTest :: StdGen -> Int -> Integer -> Bool
fermatPrimeTest g k n = all (\a -> modExp a (n-1) n == 1) as
  where
    as = take k $ randomRs (2,n-2) g

Similarly some bindings are never used. euclideanAlg2 is the greatest offender here:

euclideanAlg2 rk0 sk0 tk0 0 sk1 tk1 = (rk0,sk0,tk0) -- <- sk1 tk1 unused

Note that the worker is usually called go or similar:

euclideanAlg :: Integer -> Integer -> (Integer, Integer, Integer)
euclideanAlg a b
  | b > a     = tripFlip $ go b 1 0 a 0 1
  | otherwise =            go a 1 0 b 0 1
  where
    tripFlip (a,b,c) = (a,c,b)

    go rk0 sk0 tk0 0   _   _   = (rk0,sk0,tk0)
    go rk0 sk0 tk0 rk1 sk1 tk1 =
        let qi = rk0 `div` rk1
        in go rk1 sk1 tk1 (rk0 - qi*rk1) (sk0 - qi*sk1) (tk0 - qi*tk1)

It's easy to get rid of those bindings if we enable all warnings with -Wall. By the way, you defined but never used totient, nor did you use anything from ByteString. You probably intended to use totient it in genRSAKeys but automatically inlined it.

Apart from those nitpicks, your code seems fine. As I said, I cannot comment about the cryptographic details and whether your implementation is prone to side channel attacks or similar. Also, the quality of the random numbers depends heavily on CryptoRandomGen and therefore is out of scope for this review.

Personally, I would use a top-down approach instead of a bottom-up approach in the code, though. That makes it easier to know our goal:

data PublicKey  = PublicKey Integer Integer deriving (Show,Eq)
data PrivateKey = PrivateKey Integer Integer deriving (Show,Eq)

-- genRSAKeys e g k minPrime maxPrime
-- e is public exponent, g is random seed,
-- k is number of iterations to run Rabin-Miller test.
-- minPrime, maxPrime is range to search for primes.
genRSAKeys :: CryptoRandomGen g => Integer -> g
           -> Int -> Integer -> Integer
           -> (PublicKey, PrivateKey)
genRSAKeys e g k minPrime maxPrime =
  let  (p,q) = genPQ g k minPrime maxPrime
       n    = p*q
       t    = totient p q
       d    = modMultInv t e
  in (PublicKey n e, PrivateKey n d)

-- | Generates a pair of primes in the given range.
genPQ :: CryptoRandomGen g => g -> Int -> Integer -> Integer -> (Integer,Integer)
genPQ g k minPrime maxPrime = if p /= q
                                 then (p,q)
                                 else ...
  where
    Right (g1,g2) = splitGen g
    p = genPrime g1 k minPrime maxPrime
    q = genPrime g2 k minPrime maxPrime

...

That's a matter of preference though.


"Lonely Integer" Python implementation Problem Statement

  There are N integers in an array A. All but one integer occur in pairs. Your task is to find the number that occurs only once.

Input Format

  The first line of the input contains an integer N, indicating the number of integers. The next line contains N space-separated integers that form the array A.

Constraints

  \$1≤N<100\$

  \$N % 2 = 1\$ (N is an odd number)

  \$0≤A_{i}≤100,∀i∈[1,N]\$

Output Format

  Output S, the number that occurs only once.

Solution

#!/usr/bin/py
from collections import Counter

def histogram(ary):
    """ Creates a histogram of the given array.
    Args:
        ary: The array.
    Returns:
        The dictionary with key as number and value as count.
    """
    return Counter(ary)

def lonelyinteger(ary):
    """ Finds the unique element in the array.
    Args:
        ary: The input array.
    Returns:
        Number or None
    """
    for frequency in histogram(ary).items():
        if frequency[1] == 1:
            return frequency[0]
    return None

if __name__ == '__main__':
    a = int(raw_input())
    b = map(int, raw_input().strip().split(" "))
    print lonelyinteger(b)

The solution works perfectly fine. But in the process of learning problem-solving, I am interested in a few things:

I don't think that the information like N is odd or maximum limit of 100 is related to my solution. Is there some optimizations I am missing?
What about the space complexity? I think it's \$O(N)\$ + size of the histogram.
Runtime seems linear i.e \$O(N)\$.

Solution 2

It includes the factors like why N should be odd.

Example: 1 ^ 1 ^ 2 ^ 2 ^ 3

#!/usr/bin/py
def lonelyinteger(a):
    res = 0
    for each in a:
        res ^= each
    return res

if __name__ == '__main__':
    a = input()
    b = map(int, raw_input().strip().split(" "))
    print lonelyinteger(b)

A few quick comments:

Indeed, your solution doesn’t require \$N\$ to be odd. You’ve solved the slightly more general problem of finding a lonely integer in an array where every other element occurs at least twice, but could occur more often than that.

Likewise, the restriction that \$N \leq 100\$ isn’t used, but I can’t think of how that could be used in a solution.
I don’t know why you’ve defined a histogram() function, when you could swap it out for Counter(). That will make your code a little easier to read, because when I read Counter(), I immediately know what it does; if I see histogram() then I have to check I’ve understood the definition.
When you iterate over the histogram, you should use .iteritems() instead of .items(); in Python 2.7 the latter is slower and more memory-expensive.
You don’t need an explicit return None; Python will automatically return None if it drops out the end of a function.
Rather than iterating over the tuples in Counter(array).items() and picking out the element/frequency by numeric index, it would be better to use tuple unpacking in the for loop:

for elem, frequency in histogram(ary).items():
    if frequency == 1:
        return elem

You could also just use the most_common() method on Counter(), and take the last element of that – but at that point you’re barely doing any work, so that might not be in the spirit of the problem.


Client Server code in OCaml style I have a piece of code that works but, as I start learning OCaml, I am not confortable with the "functionnal style" to write code.

The following code create two threads. One for a tcp server that accepts one client and sends him the "test" string. In the other thread, there is a client that reads in a socket after 2 seconds of wait.

open Sys
open Unix
open Lwt

let host = Unix.inet_addr_loopback (* 127.0.0.1 *)
let port = 6600
let max_pending_request = 10

let () = Lwt_log.add_rule "*" Lwt_log.Info

let send_message oc =
  Lwt_io.write_line oc "test"

let accept_connection conn =
  let fd, _ = conn in
    let oc = Lwt_io.of_fd Lwt_io.Output fd in
    Lwt.on_failure (send_message oc ) (fun e -> Lwt_log.ign_error (Printexc.to_string e));
    Lwt_log.info "New connection" >>= return

let create_server sock =
  Lwt_unix.setsockopt sock SO_REUSEADDR true;
  Lwt_unix.bind sock @@ ADDR_INET(host, port);
  Lwt_unix.listen sock max_pending_request;
  Lwt_unix.accept sock >>= fun conn ->
    accept_connection conn >>= fun () ->
      Lwt.return (Lwt_unix.shutdown sock SHUTDOWN_SEND)

let sock_recv sock maxlen =
  let str = Bytes.create maxlen in
  Lwt_unix.recv sock str 0 maxlen [] >>= fun recvlen ->
    return (String.sub str 0 recvlen)

let sock_read sock =
    Lwt_unix.sleep 2.0 >>= fun () ->
     ignore(Lwt_unix.connect sock @@ ADDR_INET(host, port));
      sock_recv sock 1024 >>= fun answer ->
        Lwt_io.write_line Lwt_io.stdout answer

let () =
  let create_socket () = Lwt_unix.socket PF_INET SOCK_STREAM 0 in
    let server_sock = create_socket () in
    let client_sock = create_socket () in
      let threads = Lwt.join [create_server server_sock;
                              sock_read client_sock] in
        Lwt_main.run threads

I use a lot of ";" and I have the feeling that it indicates that my code is not good (based on the code of others people I have read).

Is there anything in my code that I should avoid or some parts that could be improved?

Using semicolons is OK. The IO is imperative and side-effectfull by default, so there is nothing to do here. By saying Lwt_unix.listen sock max_pending_request you're just setting a property inside the kernel. It is not blocking, so there is no need to create a thread here.

However, you should never use ignore function, to cast away the Lwt.t type. This is an error. The returned value is a thread, and the effect of connection will not happen, until the thread is determined. So, when you call sock_recv on the sock it might fail with ENOTCONN error. In case, when you really don't need the result of the computation, from a thread, and you don't need to sync with it, you can use Lwt.ignore_result or Lwt.async function, to create a background thread, that will be handled by the system. Note, however, that any exception thrown from the ignored computation will terminate the program, that basically means, that the the computation should never throw exceptions.

And few stylistic notes:

Your indentation is incorrect. Use ocp-indent for indentation, it handles monadic operands (that shouldn't be nested).
You can deconstruct values basically in any context, where you can bind, e.g.,

let accept_connection conn =
  let fd, _ = conn in

can be written as

let accept_connection (fd,_) =

Instead of using expr >>= return value pattern, you can use the map operator >|=. For example, the following

  Lwt_unix.recv sock str 0 maxlen [] >>= fun recvlen ->
  return (String.sub str 0 recvlen)

Can be rewritten as,

  Lwt_unix.recv sock str 0 maxlen [] >|= String.sub str 0


Remapping and interpolating 255x1024 array I have a function that accepts a \$255\times1024\$ array and remaps it to another array in order to account for some hardware related distortion (lines that should be straight are curved by the lens). At the moment it does exactly what I want, but slowly (roughly 30 second runtime).

Specifically, I'm looking at the nested for loops that take 18 seconds to run, and the interpolation that takes 10s. Is there any way to optimize/speed up this process?

EDIT: Nested for loops have been optimized as per vps' answer. Am now only interested in optimizing the interpolation function (if that's even possible).

def smile(Z):
    p2p = np.poly1d([ -3.08049538e-07,   3.61724996e-04,  -7.78775408e-02, 3.36876203e+00])
    Y = np.flipud(np.rot90(np.tile(np.linspace(1,255,255),(1024,1))))
    X = np.tile(np.linspace(1,1024,1024),(255,1))
    for m in range(0,255):
        for n in range(0,1024):
            X[m,n] = X[m,n] - p2p(m+1)
    x = X.flatten()
    y = Y.flatten()
    z = Z.flatten()
    xy = np.vstack((x,y)).T
    grid_x, grid_y = np.mgrid[1:1024:1024j, 1:255:255j]
    newgrid = interpolate.griddata(xy, z,(grid_x,grid_y), method = 'linear',fill_value = 0).T
    return newgrid

p2p(m + 1) does not depend on n. You may safely extract its calculation from the inner loop:

for m in range(0,255):
    p2p_value = p2p(m + 1)
    for n in range(0,1024):
        X[m,n] = X[m,n] - p2p_value

If you call smile() multiple times, it is worthwhile to precompute p2p once.

Some further savings can be achieved by accounting for the sequentiality of p2p arguments. Notice that p2p(m+1) - p2p(m) is a polynomial of lesser degree; you may
calculate it incrementally:

    p2p_value += p2p_delta(m)

Edit:

Some math:

Let \$P(x) = ax^3 + bx^2 + cx + d\$. You may see that \$P(x+1) - P(x) = 3ax^2 + 3ax + a + 2bx + b + c = 3ax^2 + (3a + 2b)x +(a+b+c)\$ is a second degree polynomial, a bit easier to calculate than the original third degree one. Which leads to the code (fill up the list of coefficients according to the above formula):

    p2p_delta = np.poly1d([...])
    p2p_value = p2p_delta(0)
    for m in range(0, 255)
        for n in range(0, 1024)
            X[m,n] -= p2p_value
        p2p_value += p2p_delta(m)


Fast draw wxPython OnPaint I have the following wx.Window:

class SketchWindow(wx.Window):

  def __init__(self, parent):
    wx.Window.__init__(self, parent, -1)
    self.SetBackgroundColour('White')
    # Window event binding
    self.Bind(wx.EVT_PAINT, self.OnPaint)
    self.Bind(wx.EVT_IDLE, self.OnIdle)
    # run
    self.Run()

  def OnPaint(self, evt):
    self.DrawEntities(wx.PaintDC(self))

  def DrawEntities(self, dc):
    dc.SetPen(wx.Pen('Black', 1, wx.SOLID))
    dc.SetBrush(wx.Brush('Green', wx.SOLID))
    # draw all
    for e in self.entities:
      x, y = e.location
      dc.DrawCircle(x, y, 4)

  def OnIdle(self, event):
    self.Refresh(False)

  def Run(self):
    # update self.entities ...
    # call this method again later
    wx.CallLater(50, self.Run)

I need to draw on my window a number of circles [0, 2000] every N milliseconds (50 in my example), where at each step these circles may update their location.

The method I wrote works (the circles are anti-aliased too) but it's quite slow.
Is there a way to improve my solution?

Generally, the Python style is to indent by one tab, or four spaces. Functions and variables should be named in snake_case, and classes should be in PascalCase. See PEP8 for more style information.

You should also use docstrings to describe what your functions do. Here's an example of how docstrings are used.

def my_func( ... ):
    """
    Describe what your function does and
    it's arguments in this docstring.
    """
    ...

Other than that, your code looks pretty nice! Good work!


Pizza maker with Bloch builder I tried to implement the builder pattern of GoF. After searching for almost every related posts/examples on the Internet, I'm still confused. But I found that there are two kinds of patterns, which are both called builder:

Bloch Builder : A famous post, but a comment pointed out it's actually a fluent interface, still useful, though. Also notice that it's not the same as Method Chaining, see the differences.
Builder Pattern @thejavageek.com : The author gives a very clear structure of Builder Pattern: Director, Builder, ConcreteBuilder, Product.

Finally I decide to make my own version of "Builder Pattern", which follows the first link above, with some modifications:

The static method Pizza.makePizza() acts as the Director.
For simplicity, I didn't make the Factory a factory method or abstract factory, but it can be without problem.
Before you want to make a pizza, you have to override some methods, which you can put anything must be done before the pizza is created.

Main.java

public class Main {
    public static void main(String[] args) throws InterruptedException {
        Factory myFactory = new Factory();

        Pizza myPizza = Pizza.makePizza(new Pizza.Builder(myFactory) {
            @Override
            public void prepareDough() {
                myFactory.prepareDough();
            }

            @Override
            public void prepareToppings() {
                myFactory.prepareToppings();
            }
        }.withSize(20).withBacon().withPepperoni());

        System.out.println(myPizza);
    }
}

Pizza.java

public class Pizza {
    private final int size;
    private final boolean cheese;
    private final boolean pepperoni;
    private final boolean bacon;

    private Pizza(Builder builder) {
        size = builder.size;
        cheese = builder.cheese;
        pepperoni = builder.pepperoni;
        bacon = builder.bacon;
    }

    public static Pizza makePizza(Builder builder) throws InterruptedException {
        builder.prepareDough();
        Thread.sleep(2000);
        builder.prepareToppings();
        Thread.sleep(2000);
        return new Pizza(builder);
    }

    public static abstract class Builder {
        private int size;
        private boolean cheese = false;
        private boolean pepperoni = false;
        private boolean bacon = false;

        public Builder(Factory factory) {
        }

        public Builder withSize(int size) {
            this.size = size;
            return this;
        }

        public Builder withCheese() {
            cheese = true;
            return this;
        }

        public Builder withPepperoni() {
            pepperoni = true;
            return this;
        }

        public Builder withBacon() {
            bacon = true;
            return this;
        }

        protected abstract void prepareDough();
        protected abstract void prepareToppings();
    }

    public String toString() {
        return String.format("pizza={size=%d, cheese=%s, pepperoni=%s, bacon=%s}",
            size,
            cheese,
            pepperoni,
            bacon);
    }
}

Factory.java

public class Factory {
    public Factory() {}

    public void prepareDough() {
        System.out.println("Preparing dough...");
    }

    public void prepareToppings() {
        System.out.println("Preparing toppings...");
    }
}

Result:

Preparing dough...
Preparing toppings...
pizza={size=20, cheese=false, pepperoni=true, bacon=true}

Your main() code feels cumbersome, and the modelling seems unnatural to me.  I also find that .withSize(20).withBacon().withPepperoni() is hard to read, perhaps due to its placement after the definition of the anonymous subclass of Pizza.Builder.

public class Main {
    public static void main(String[] args) throws InterruptedException {
        Factory myFactory = new Factory();

        Pizza myPizza = Pizza.makePizza(new Pizza.Builder(myFactory) {
            @Override
            public void prepareDough() {
                myFactory.prepareDough();
            }

            @Override
            public void prepareToppings() {
                myFactory.prepareToppings();
            }
        }.withSize(20).withBacon().withPepperoni());

        System.out.println(myPizza);
    }
}

Assuming that you are interested in constructing an immutable Pizza, I would prefer to see:

public class Main {
    public static void main(String[] args) throws InterruptedException {
        Pizza myPizza = new PizzaBase(20).addTopping(Pizza.Topping.BACON)
                                         .addTopping(Pizza.Topping.PEPPERONI)
                                         .bake();
        System.out.println(myPizza);
    }
}

In particular:

This interface mimics the process of making a pizza.
The base is mandatory, and you must specify its size.  The toppings are optional.
Offering an .addTopping(…) method that accepts a parameter is more flexible.  You might even be able to .addTopping(CHEESE) twice to get extra cheese.
I'm not convinced that dependency injection for the Factory is worthwhile.


Pass Triangle challenge In order to solve this challenge:

  Challenge Description

  By starting at the top of the triangle and moving to adjacent numbers on the row below, the maximum total from top to bottom is 27.

   5

  9 6

 4 6 8

0 7 1 5

  5 + 9 + 6 + 7 = 27

  Input sample

  Your program should accept as its first argument a path to a filename. Input example is the following:

5
9 6
4 6 8
0 7 1 5

  You make also check full input file which will be used for your code evaluation.

  Output sample

  The correct output is the maximum sum for the triangle. So for the given example the correct answer would be

27

I came up with the following code:

static int GetMaxSum(int[][] numbers)
{
    int firstCandidate = 0;
    int secondCandidate = 0;
    int max = 0;

    for(int i = 1; i < numbers.Length; i++)
    {
        for(int j = 0; j < numbers[i].Length; j++)
        {
            firstCandidate = 0;
            secondCandidate = 0;

            if (j - 1 >= 0)
            {
                firstCandidate = numbers[i][j] + numbers[i - 1][j - 1];
            }

            if (j < numbers[i - 1].Length)
            {
                secondCandidate = numbers[i][j] + numbers[i - 1][j];
            }

            numbers[i][j] = firstCandidate > secondCandidate ? firstCandidate : secondCandidate;
        }
    }

    int lastIndex = numbers.Length - 1;
    var lastLine = numbers[lastIndex];
    for(int i = 0; i < lastLine.Length; i++)
    {
        if(lastLine[i] > max)
        {
            max = lastLine[i];
        }
    }

    return max;
}

static int[] ParseLine(string line)
{
    string[] numbers = line.Trim().Split(' ');
    int numbersLength = numbers.Length;
    var result = new int[numbersLength];

    for(int i = 0; i < numbersLength; i++)
    {
        result[i] = int.Parse(numbers[i]);
    }

    return result;
}

static void Main(string[] args)
{
    var list = new List<int[]>();
    int max = 0;

    using (StreamReader reader = File.OpenText(args[0]))
    {
        while (!reader.EndOfStream)
        {
            string line = reader.ReadLine();

            if (null == line)
            {
                continue;
            }

            list.Add(ParseLine(line));
        }
    }

    max = GetMaxSum(list.ToArray());

    Console.WriteLine(max);
}

and wanted some feedback on it.

The basic idea is:

Read all the file
Transform the content in a Lower Triangle Matrix structure
Calculate max sums directly in the matrix (being that it's used only internally and it can be thrown once it's been processed)

This was the most efficient - with regard to space and time - algorithm I could think of.

Everything you can find - readability problems, efficiency problems, design problems, etc... - is fine.

int firstCandidate = 0;
int secondCandidate = 0;
int max = 0;

None of these need as wide a scope as they have. In particular, firstCandidate and secondCandidate are only used inside the nested loop, and are initialised every time they're used. They should be pushed in (if not eliminated...)

        firstCandidate = 0;
        secondCandidate = 0;

        if (j - 1 >= 0)
        {
            firstCandidate = numbers[i][j] + numbers[i - 1][j - 1];
        }

        if (j < numbers[i - 1].Length)
        {
            secondCandidate = numbers[i][j] + numbers[i - 1][j];
        }

        numbers[i][j] = firstCandidate > secondCandidate ? firstCandidate : secondCandidate;

This can be simplified slightly with a simple algebraic rearrangement to factor out the numbers[i][j] from the two candidates, since we know that at least one candidate must exist. (Note that by using int.MinValue we also fix something which could be argued to be a bug, unless the spec clearly states that the triangle will never contain negative numbers).

I agree with TarkaDaal that Math.Max is slightly more readable. And I would also find it more readable with ternary operators. Bearing in mind the scope change mentioned above, I get:

        int firstCandidate = j > 0 ? numbers[i - 1][j - 1] : int.MinValue;
        int secondCandidate = j < numbers[i - 1].Length ? numbers[i - 1][j] : int.MinValue;
        numbers[i][j] += Math.Max(firstCandidate, secondCandidate);

To find the maximum of an array, Linq is more readable and you won't notice any speed difference.

Summarising, I would rewrite the d.p. method as

static int GetMaxSum(int[][] numbers)
{
    for(int i = 1; i < numbers.Length; i++)
    {
        for(int j = 0; j < numbers[i].Length; j++)
        {
            int firstCandidate = j > 0 ? numbers[i - 1][j - 1] : int.MinValue;
            int secondCandidate = j < numbers[i - 1].Length ? numbers[i - 1][j] : int.MinValue;
            numbers[i][j] += Math.Max(firstCandidate, secondCandidate);
        }
    }

    return numbers[numbers.Length - 1].Max();
}


PHP recreate array if it contains certain values I have $fruits_arr:

Array
(
    [0] => Array
        (
            [id] => 213
            [fruit] => banana
        )

    [1] => Array
        (
            [id] => 438
            [fruit] => apple
        )

    [2] => Array
        (
            [id] => 154
            [fruit] => peach
        )
)

And $ids_arr:

Array (
  [0] => 213
  [1] => 154
)

I want to recreate $fruits_arr to have only array items where id is equal to a value from $ids_arr. I also want to maintain the index/array order of $fruits_arr.

I'm using the following:

$selected_fruits = array();

foreach( $fruits_arr as $fruit ) :
    if ( in_array( $fruit['id'], $ids_arr ) ) :
        $selected_fruits[] = $fruit;
    endif;
endforeach;

print_r( $selected_fruits );

It seems to work but I am wondering if there is a shorter, better way to accomplish this in the latest PHP version.

Only thing you can use besides the foreach loop, it is array_filter:

$selected_fruits = array_filter($fruits_arr, function($fruit) use($ids_arr) {
    return in_array( $fruit['id'], $ids_arr );
});

looks shorter and uses native functions


Character range generator I am working on trying to create a range function in JavaScript that imitates PERL's 'a'..'zz'.

which expands to:

a->z
aa->az
ba->bz
...
za->zz

And so on for every extra character you add to the end of the range( e.g zzz .... ).

I have a working implementation, However, I think that there is a more efficient - and perhaps more idiomatic - solution.

There are two things I would like answers to address:

I would like to reduce the code size - this seems like too many lines for the problem that it solves.
I would like improve the performance - the implementation that I have is very slow for any end string > 5 characters long.

function charRange( start, end ) {
  var current = start;
  var range   = [];

  range.push( current ); //Put the start directly into the range.
  if ( typeof start === typeof end &&
     ( typeof start === "string" )) {
    if ( typeof end !== "string" ) {
        console.log( "ERROR: start and end must both be Strings!" );
    }
  }

  while ( !stringEqual(current.split(""), end.split("")) ) {
    current = next(current);
    range.push( current );
    // process.exit();
  }

  return range;

  function stringEqual( chars, testChars ) {
    var isEqual = 1;
    var sum = 0;
    var testSum = 0;
    var i = 0;
    while ( i < testChars.length ) {
        // console.log(chars);
        // console.log(testChars);
        // console.log(i);

        if ( isEqual && chars[i] !== testChars[i] ) {
            isEqual = 0;
        };
        if ( chars[i] ) {
            sum     += chars[i].charCodeAt(0);
        };
        testSum += testChars[i].charCodeAt(0);
        i++;
    }

    // console.log( sum + " " + testSum );
    return isEqual || (sum < testSum ? 0 : 1);
  }

  function allAreZ( string ) {
    var found = 1;
    var i     = 0;
    while ( i < string.length ) {
        if ( string[i] !== "z" ) {
            found = 0;
            break;
        }
        i++;
    }

    return found;
  }

  function findIncrementIndex( item ) {
    var index = item.indexOf("z");
    var i     = 0;

    while (index === 0) {
        i++;
        index = item.substr(i).indexOf("z");
    }

    index += i;

    return index;
  }

  function next( item ) {
    if ( typeof item === "number" ) {
        return item++;
    } else if ( typeof item === "string" ) {
        var lastCharCode = item.charCodeAt( item.length -1 );
        item = item.toLowerCase();

        if ( lastCharCode === "z".charCodeAt( 0 ) ) {
            if ( allAreZ(item) ) {
                // console.log( "Reached the end, adding a letter");
                return Array(item.length +2).join("a");
            } else {
                var index = findIncrementIndex(item);
                var charCode = item.charCodeAt( index -1 );
                var newChar = String.fromCharCode( ++charCode );
                item = item.replace(/[a-y]z/, newChar + "a").replace(/z+$/, "a");
                // console.log( "Reached the end, next set is: " + item);
                return item;
            }

        } else {
            var newChar = String.fromCharCode( ++lastCharCode );
            // console.log( "Next Character is: " + newChar + " Code: " + lastCharCode + " Input: " + item);
            return item.replace( /\w$/, newChar );
        }
    }
  }
};

From a once over:

As a caller I would rather whether start or end was not a string instead of getting ERROR: start and end must both be Strings!
Indentation! Please consider using something like jsbeautifier
You can merge these

var range = [];
range.push(current); //Put the start directly into the range.

into

var range = [start]; //Begin with the start <- Perhaps too zen a comment

Do not keep commented out code
I cannot understand why you would need

while(!stringEqual(current.split(""), end.split(""))) {

why can't you simply

while(current != end) {

Also, this, I cant see when you would need this:

if ( typeof item === "number" ) {
    return item++;

Since you verify that both start and end are string, this should never happen?
allAreZ is written in a convoluted way, in this type of function ( small, and dedicated to 1 task), it is appropriate to return immediately. You could try something like

function allZ( string ) {
    var i = 0;
    while ( i < string.length ) {
        if ( string[i] !== "z" ) {
            return false;
        }
        i++;
    }
    return true;
}

or, you something fun like this

function allZ( string ) {
    return string.split('z').length == string.length + 1;
}

I am mulling about writing something far shorter with .toString(26) To be sure zzzzzz equals 308915775, that is a lot of entries to store in array, in fact at this point it is silly to store this into an array. You would be better of writing an iterator class which provides a next() function for ranges in that size.

function charRange(start, end)
{
  if(typeof start !=  "string" ){
    throw 'start must be a string';
  }

  if(typeof end !=  "string" ){
    throw 'end must be a string';
  }

  var range = [],
      current,
      mapRadix = {},
      mapAlphabet = {},
      a = 'a'.charCodeAt(0);

  for( var i = 0 ; i < 26 ; i++ ){
    mapRadix[i.toString(26)] = String.fromCharCode(a+i);
    mapAlphabet[String.fromCharCode(a+i)] = i.toString(26);
  }

  function string2Number( s ){
    var i26 = '';
    for( var i = 0 ; i < s.length ; i++){
      i26 += mapAlphabet[ s[i] ];
    }
    return parseInt(i26,26);
  }

  function number2String( int ){
    var n = int.toString(26),
        s = '';
    for( var i = 0 ; i < n.length ; i++){
      s += mapRadix[ n[i] ];
    }
    return s;
  }

  current = string2Number( start );
  end = string2Number( end ) + 1;
  if( current > end ){
    throw 'start must be smaller than end';
  }

  while(current != end ) {
    range.push(number2String(current++));
  }

  return range;
}


Specializing std::hash for std::array I want to have an std::unordered_map which uses arrays as keys. As long as a type has an operator== and an std::hash it can be used as a key.

Here is my std::hash<std::array>

template<class T, size_t N>
struct std::hash<std::array<T, N>> {
    auto operator() (const std::array<T, N>& key) const {
        std::hash<T> hasher;
        size_t result = 0;
        for(size_t i = 0; i < N; ++i) {
            result = result * 31 + hasher(key[i]); // ??
        }
        return result;
    }
};

I copied the multiply by 31 from somewhere. An example here uses << and ^. I don't know which is better or how to apply the cppreference example to an array.

// cppreference example
template<> struct hash<S>
    {
        typedef S argument_type;
        typedef std::size_t result_type;
        result_type operator()(argument_type const& s) const
        {
            result_type const h1 ( std::hash<std::string>{}(s.first_name) );
            result_type const h2 ( std::hash<std::string>{}(s.last_name) );
            return h1 ^ (h2 << 1); // or use boost::hash_combine (see Discussion)
        }
    };

It seems to work. All of the code compiles and the return values for std::hash<key_type> seem ok to me.

using key_type = std::array<int, 2>;
using hash_type = std::hash<key_type>;
using map_type = std::unordered_map<key_type, int>;

map_type map; // compiles
hash_type hasher; // compiles

int main() {
    // test return values of std::hash<key_type>
    for(int y = -2; y < 3; ++y) {
        for(int x = -2; x < 3; ++x) {
            std::cout << hasher({x, y}) << " ";
        }
        std::cout << std::endl;
    }
}

The values printed from this code are:

18446744073709551552 18446744073709551583 18446744073709551614 29 60
18446744073709551553 18446744073709551584 18446744073709551615 30 61
18446744073709551554 18446744073709551585 0 31 62
18446744073709551555 18446744073709551586 1 32 63
18446744073709551556 18446744073709551587 2 33 64

I'm not sure what makes a good hash function for std::unordered_map, so I'd appreciate any feedback on how to make this hash function better.

What is a "good" hash function?

The requirement of a hash function is that it always produces the same value for a given key (in the same instance of the program) and a good hash function should produce each output with equal probability.

Your function achieves this (multiplying by a prime such as 31 is a well-known way of combining hash values to reduce collisions caused by different permutations of the same values).

However, it's up to you to decide whether the hash inputs in your application have a random distribution, or whether they have some pattern that may perturb the output distribution from the hash function.

Can you use your mental reasoning to force a collision?  {1, 0} and {0, 31} might do it, for example.  Can you think of any reason why your code might produce collisions more often than by independent chance?

Style points

The code is neat and clear.  It might be an idea to allow an alternative to std::hash<T> to be supplied by the caller, just as std::unordered_map itself allows a user-supplied hasher.  This may be useful with char* strings, for example.

You could simplify the loop, as std::array supports range-based for:

    for (const auto& element: key) {
        result = result * 31 + hasher(element);
    }

More tests

It's good that you've provided a test suite.  Consider whether you could add more tests - at least one test with an element type other than int and at least least one with different array lengths (you should probably include a test for std::array<char, 1> as a boundary case, for example).

Alternatives

Boost provides a hash() function that accepts built-in (i.e. C-style) arrays, so you might be able to reinterpret_cast the std::array.  I've never tried this, but I don't see why it wouldn't work...


Formatting database results to be consumed as multiple tables by the view In the code below, $rows is the result of database query. In the view that consumes this, I need to break the results into a separate table for each vendor.

The below creates a structure that allows this in the template, but it seems like a clunky way of doing it. Advice on how to improve it?

    $byVendor = [];
    if (sizeof($rows) < 1)
        return $byVendor;
    $vendor   = $rows[0]->AMVEND;
    $i = 0;
    foreach ($rows as $row ) {
        $currentVendor = $row->AMVEND;
        if ($currentVendor != $vendor)
            $i++;
        $byVendor[$i][] = $row;
    }
    return $byVendor;

Code used in the view:

@section('content')
@foreach($vendors as $vendor)

{{$vendor[0]->AMNAME}}

            Date/Time
            PO Number
            Item
            Qty

    @foreach($vendor as $item)

            {{$item->CREATED_AT}}
            {{$item->SOURCE}}
            {{$item->ITEM}}
            {{$item->QTY}}

    @endforeach

@endforeach
@endsection

If you add a group by clause to your query on the AMVEND column, you won't have to do any looping before you pass the data to the view. You can simply detect when the vendor changes in the view's foreach loop.


Australian income tax calculator I'm looking for some feedback on my first Javascript app. I've built an app that calculates tax for Australian tax payers. Specific feedback on areas that could be object orientated and any errors I have made would be great.

//Inputs
var income = 250000;
var superAnnuationPercentage = 9.25;
var superAnnuationTaxRate = 15;

//Defines the tax brackets
var taxBracket = [
    {from: 0, to: 18200, percentage: 0, amount: 0},
    {from: 18201, to: 37000, percentage: 19, over: 18200, amount: 0},
    {from: 37001, to: 80000, percentage: 32.5, over: 37000, amount: 3752},
    {from: 80001, to: 180000, percentage: 37, over: 80000, amount: 17547},
    {from: 180001, to: Infinity, percentage: 45, over: 180000, amount: 54547}
];

//Defines HECS/HELP repayment brackets
var hecsHelp = [
{from:0,     to:53345, percentage:4},
{from:53344, to:59421, percentage:4},
{from:59422, to:65497, percentage:4.5},
{from:65498, to:68939, percentage:5.0},
{from:68940, to:74105, percentage:5.5},
{from:74106, to:80527, percentage:6.0},
{from:80258, to:84481, percentage:6.5},
{from:84482, to:92970, percentage:7.0},
{from:99070, to:99069, percentage:7.5},
{from:99070, to:Infinity, percentage:8.0},
];

//Calculates superannuation based on a percentage
var superannuationCalc = function (int) {
    var percent = superAnnuationPercentage/100;
    return  int * percent;
};

//define income after removing superannuation element. Superannuation is taxed at a lower rate.
var grossIncome = income - superannuationCalc(income);

//The end result of alll the code.
var workoutTax = taxCalc(grossIncome);

//bringing it all together
function workOutSuperTax(){
var a = superannuationCalc(income);
var b = superAnnuationTaxRate;
var c = superannuationTaxCalc(a,b);
return c;
}

var superTax = workOutSuperTax();

var workOutHECS = hecsHelpCalc(income);

totalTax = workoutTax + superTax + workOutHECS;

//The end result!
console.log(totalTax);

//Loops through taxBracket object and finds the tax bracket for the post deduction gross income amount
function taxCalc(grossSuperIncome){
    for(var x = 0; x < taxBracket.length; x++){
        if(grossSuperIncome <= taxBracket[x].to){
            var amountOver = grossSuperIncome - taxBracket[x].over;
            var percent = taxBracket[x].percentage / 100;
            return taxBracket[x].amount + (amountOver * percent);

        }

    };
};

//calculates super tax amount by usuing superTaxRate and superAnnuation variables
function superannuationTaxCalc(superAmount,superTaxRate){
    var percent = superTaxRate / 100;
    return superAmount * percent;
};

//calculates HECS bracket
function hecsHelpCalc(hecsIncome){
        for (var x =0; x < hecsHelp.length; x++){
            if(hecsIncome <= hecsHelp[x].to){
            var percentage = hecsHelp[x].percentage / 100;
            return hecsIncome * percentage;
            }
        };
};

Ranges

You use

var hecsHelp = [
{from:0,     to:53345, percentage:4},
{from:53344, to:59421, percentage:4},
{from:59422, to:65497, percentage:4.5},
{from:65498, to:68939, percentage:5.0},
{from:68940, to:74105, percentage:5.5},
{from:74106, to:80527, percentage:6.0},
{from:80258, to:84481, percentage:6.5},
{from:84482, to:92970, percentage:7.0},
{from:99070, to:99069, percentage:7.5},
{from:99070, to:Infinity, percentage:8.0},
];

to define ranges with different percentages. A few things to say about this :

it does not respect the DRY principle : this makes things easier to get wrong (and I believe that either the first or the second range is indeed wrong as it does not follow the same pattern where range[n].to + 1 == range[n+1].from)
it does not define what happens when the number you want to handle happens to be between range[n].to and range[n+1].from which can happen if you are handling non-integers values.

I think things would be easier if you were to define it like :

var hecsHelp = [
{from:0,     percentage:4},
{from:53344, percentage:4}, # By the way, is this line useful ?
{from:59422, percentage:4.5},
{from:65498, percentage:5.0},
{from:68940, percentage:5.5},
{from:74106, percentage:6.0},
{from:80258, percentage:6.5},
{from:84482, percentage:7.0},
{from:99070, percentage:7.5}, # This one seems pretty weird too.
{from:99070, percentage:8.0},
];

Then, iterating backward (or forward if you define your array as the reverse of what I have written), everything should be easier to maintain.


Automate the Boring Stuff with Python - The Collatz sequence project I am new to programming and started learning with Automate the Boring Stuff with Python.  I just completed chapter 3's "Collatz sequence" project.

I was wondering what would you do differently and why? Trying to learn how to think like a programmer and become more efficient.

def collatz(number):
    global nextNumber
    if number % 2 == 0:
        nextNumber = (number//2)
        print(nextNumber)
    else:
        nextNumber = (3*number+1)
        print(nextNumber)

print('Type in an integer.')
integer=input()

if integer.isdigit() ==True:
    collatz(int(integer))
    while nextNumber !=1:
        collatz(nextNumber)

while integer.isdigit() != True :
        print('Please try again.')
        integer=input()
        collatz(int(integer))
        while nextNumber !=1:
            collatz(nextNumber)

print('Thank you!')

One thing you’ll want to learn is not to repeat yourself, when coding.

Consider:

def collatz(number):
    global nextNumber
    if number % 2 == 0:
        nextNumber = number//2
        print(nextNumber)
    else:
        nextNumber = 3*number+1
        print(nextNumber)

You have two identical print statements at the end of both the if and the else clauses.  These can be combined and moved out of the if-else:

def collatz(number):
    global nextNumber
    if number % 2 == 0:
        nextNumber = number//2
    else:
        nextNumber = 3*number+1
    print(nextNumber)

You are printing in your collatz generator function.  If you want the longest sequence starting under 10000000, you’re going to do a lot of printing!  Instead, move the printing responsibility to the caller.  They know whether they want to print it, or just find the length.

Don’t use global to return one value from a function.  (As Aaron states in the comments: "don't use global ever if you can avoid it (which is always)"). Just return the value,

def collatz(number):
    if number % 2 == 0:
        nextNumber = number//2
    else:
        nextNumber = 3*number+1
    return nextNumber

and let the caller assign it to whatever variable they want.

nextNumber = collatz( int(number) )
print(nextNumber)
while nextNumber != 1:
    nextNumber = collatz(nextNumber)
    print(nextNumber)

Reordering, to remove one of the calls to collatz:

number = int(number)
while number != 1:
    number = collatz( number )
    print(number)

Here, you are try to validate the user input is a number:

if integer.isdigit() ==True:
    collatz(int(integer))
    # ... print Collatz sequence ...

while integer.isdigit() != True :
        print('Please try again.')
        integer=input()
        # ... print Collatz sequence ...

Multiple issues here.

You are using an if <expr> followed by a while <not expr> to perform a sort of if ... else ... statement.  If the first input is valid, you print the Collatz sequence in the "then" clause, and the while loop never starts because the condition is immediately False.  If the if <expr> doesn't pass, the while <not expr> catches control and prompts the user for valid input, in a loop.
You are repeating the code to print that Collatz sequence.  And as I said above, (omitted, because *I* am *not* going to repeat myself).
The while loop will fail with a ValueError exception at collatz(int(integer)) if the user enters an invalid input the second time.
.isdigit() tests if the string consists only of "digit" characters, including odd things like the superscript 2 (²), which are not valid for the int(...) method.  So you could still get a ValueError exception even when .isdigit() returns True!  You would need to use .isdecimal() instead.

These issues can be avoiding by using Python's exceptions.  Try to convert the input to an integer, and use the exception to detect invalid input.  Python’s exception handling is exceptional! Learn it. Rely on it:

number = None
while number is None:
    try:
        number = int( input("Enter a number: ") )
    except ValueError:
        print("Try again")

 # ... print collatz sequence here


C# Code to Find all Divisors of an Integer I made this so I can feed it integers and return an array with all the divisors of that integer.  I put checks in in case the integer is less than 2.  Order of integers in the array must be smallest to largest.  The code works.  What I need is to optimize this code to be as fast as possible.  Right now on an online IDE I am getting around 40ms combined against the given test.  I need to trim this down as much as possible.

using System.Collections.Generic;

public class Divisors
{
    public static bool IsPrime(int n)
    {
        if (n == 2) return true;
        if (n % 2 == 0) return false;

        for (int x = 3; x * x <= n; x += 2)
            if (n % x == 0)
                return false;

        return true;
    }

    public static int[] GetDivisors(int n)
    {
        List<int> divisors = new List<int>();

        if (n < 2)
        {
            return null;
        }
        else if (IsPrime(n))
        {
            return null;
        }
        else
        {
            for (int i = 2; i < n; i++)
                if (n % i == 0)
                    divisors.Add(i);
        }

        return divisors.ToArray();
    }
}

namespace Solution
{
  using NUnit.Framework;
  using System;

  [TestFixture]
  public class SolutionTest
  {
    [Test]
    public void SampleTest()
    {
      Assert.AreEqual(new int[] {3, 5}, Divisors.Divisors(15));
      Assert.AreEqual(new int[] {2, 4, 8}, Divisors.Divisors(16));
      Assert.AreEqual(new int[] {11, 23}, Divisors.Divisors(253));
      Assert.AreEqual(new int[] {2, 3, 4, 6, 8, 12}, Divisors.Divisors(24));
    }
  }
}

Style

As far as style goes, your code looks clean and readable, and follows the conventions, so good job for that.

You may want to include documentation comments with \\\ four your class and methods. Although the method names are descriptive enough in this rather simple case, it is a good habit to take on.

Tests

It's nice you use a test framework to test your class. However, the methods name don't match (Divisors vs GetDivisors) and comparing arrays doesn't work that way.

You could also benefit from including more tests for edge cases . What if the given argument is prime (since you make it a special case)? What if it is int.MaxValue? What if it is 0? What if it is negative?

Make your class static

Your Divisors class only has static methods. As such, it should be a static class. I might help the compiler with optimizations and can improve performance a little.

Unexpected behavior

As far as I know, 1 and n are always divisors of n, yet they are omitted from the returned array. You should probably include them, or at least document that they are omitted.

If the number is negative, you return null. While in theory negative numbers have divisors (and positive numbers have negative divisors), I understand why this you chose this approach, as well as why you only return positive divisors. I would suggest you either document this behavior, or enforce it by using the uint datatype. I would chose the former approach, as intis much more prevalent, and using uint would most likely imply a lot of casting down the line.

Optimizing the algorithm

You check if the number is prime before looking for divisors. First of all, your algorithm for primality checking is rather naive and can be optimized in various ways. More importantly, this is an optimization only if the argument is prime, but is counterproductive in the vast majority of cases when it isn't prime. I suggest to simply get rid of that check.

Furthermore, you check if every number between 2 and n is a divisor of n; however, you know that if i is a divisor, so is n / i. Therefore, you can loop only on values between 1 and sqrt(n) and add two divisors for every match.

My attempt

    public static class Divisors
    {
        /// <summary>
        /// Finds all the divisors of any positive integer passed as argument.
        /// Returns an array of int with all the divisors of the argument.
        /// Returns null if the argument is zero or negative.
        /// </summary>
        public static int[] GetDivisorsMe(int n)
        {
            if (n <= 0)
            {
                return null;
            }
            List<int> divisors = new List<int>();
            for (int i = 1; i <= Math.Sqrt(n); i++)
            {
                if (n % i == 0)
                {
                    divisors.Add(i);
                    if (i != n / i)
                    {
                        divisors.Add(n / i);
                    }
                }
            }
            divisors.Sort();
            return divisors.ToArray();
        }
    }

As for performance, finding all divisors for every integer between 0 and 10,000 takes around 130ms with your solution on my machine vs 12ms with mine, so a performance gain of around 10x.
Finding divisors for int.MaxValue takes around 9s your solution vs 5ms with mine, a performance gain greater than 1000x!
Finally, finding divisors for 2095133040 – the largest highly composite number that fits in the int datatype, with a total of 1600 divisors – takes around 5s with your solution, vs 13ms with my solution, again a performance gain of around 400x.

Performance can probably be improved further by estimating how many divisors has a given input and passing that estimate to the List<int> constructor, and thus limiting how much memory reallocating is done as the list grows. In fact, the upper bound of the number divisors is known: 1600. You could simply allocate the list as:

List<int> divisors = new List<int>(1600);

This brings the execution time down to 5ms for the highest composite number, but feels like a waste of memory in most cases.


Z^m number system in Rust version 2 A \$Z^m\$ number system includes integers in the interval \$[0, m)\$ when \$m > 0\$ or \$(m, 0]\$ when \$m < 0\$. The code defines a trait Mod to represent numbers in this system and the +, -, and * operator for it.

use std::ops::Add;
use std::ops::Mul;
use std::ops::Sub;
use std::ops::Rem;

struct Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    modulo: T,
    i: T,
}

trait Modulo<T>
    where T: Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn modulo(self, n: T) -> T;
}

impl<T> Modulo<T> for T
    where T: Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn modulo(self, n: T) -> T {
        ((self % n) + n) % n
    }
}

impl<T> Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn new(modulo: T, i: T) -> Mod<T> {
        let n = i.modulo(modulo);
        Mod {
            modulo: modulo,
            i: n,
        }
    }
}

impl<T> Add for Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn add(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i + other.i)
    }
}

impl<T> Sub for Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn sub(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i - other.i)
    }
}

impl<T> Mul for Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn mul(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i * other.i)
    }
}

fn main() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x + y).i);
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x - y).i);
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x * y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x + y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x - y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x * y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x + y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x - y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x * y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x + y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x - y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x * y).i);
}

I tried to achieve the goals laid out in the previous version:

Changed the modulo method for better performance.
Generalized over a bunch of types other than i32.

All suggestions are still welcome. In particular, requiring T to implement Copy and Clone might be a bit too restrictive, I would like to relax that.

Place multiple imports from the same module on one line.
I would heartily recommend against interleaving a trait definition (Modulo) and a struct definition (Mod).
I prefer to not put trait bounds on the trait definition. Those usually want to be a supertrait or just on a trait implementation.
Similarly, I prefer not put trait bounds on a struct definition; just on  on implementation.
There's no need for the Clone bound — it's never used.
I'd make your new "operator" trait match others: Parameterize the right-hand side (Rhs) and the Output as an associated type.
Then you can DRY up your duplicated trait bounds by creating a new trait that has all the other traits as a supertrait.
While you are at it, remove the type parameter T  because you always use it in the same fashion; it might as well be hardcoded.
Place a space around = when restricting associated types in traits.
There's no need for the temp var in the Mod constructor.
Tests. TESTS. TESTS You are so close to already having tests! Your main function can be basically transformed into a sequence of tests. Then the computer can verify them, instead of having to read the output each time. Please give them useful names that describe what each tests, not my stupid sequential names.

use std::ops::{Add, Mul, Sub, Rem};

trait Modulo<Rhs = Self> {
    type Output;

    fn modulo(self, n: Rhs) -> Self::Output;
}

impl<T> Modulo<T> for T
    where T: Add<Output = T> + Rem<Output = T> + Copy
{
    type Output = T;

    fn modulo(self, n: T) -> T {
        ((self % n) + n) % n
    }
}

trait ZMod: Modulo<Output = Self> + Mul<Output = Self> + Sub<Output = Self> + Add<Output = Self> + Rem<Output = Self> + Copy {}
impl<T> ZMod for T
    where T: Modulo<Output = T> + Mul<Output = T> + Sub<Output = T> + Add<Output = T> + Rem<Output = T> + Copy
{}

struct Mod<T> {
    modulo: T,
    i: T,
}

impl<T> Mod<T>
    where T: ZMod
{
    fn new(modulo: T, i: T) -> Mod<T> {
        Mod {
            modulo: modulo,
            i: i.modulo(modulo),
        }
    }
}

impl<T> Add for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn add(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i + other.i)
    }
}

impl<T> Sub for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn sub(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i - other.i)
    }
}

impl<T> Mul for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn mul(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i * other.i)
    }
}

#[test]
fn t0() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t1() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t2() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t3() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t4() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t5() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t6() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t7() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t8() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t9() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t10() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(1, (x + y).i);
}

#[test]
fn t11() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t12() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(4, (x * y).i);
}


Validating uploaded images I have attempted to write a method that checks that images being uploaded are valid.

public function checkValidImages(){
            $errors             = array();
            $allowedImages      = array('image/jpg','image/jpeg','image/png','img/gif');
            $maxFileSize        = 2097152;
            $invalidSize        = 0;
            $invalidTypes       = 0;
            $invalidDimension   = 0;
            $minWidth           = 220;
            $minHeight          = 220;

            // Check atleast one image has been selected
            foreach($_FILES['event-images']['tmp_name'] as $tmp_name){

                if($tmp_name == ""){
                    $errors[] = 'Please select atleast one image.'; // This is okay because instance would only occur once
                }
            }

            // Check Images are valid image types
            foreach($_FILES['event-images']['type'] as $type){

                if(!in_array($type, $allowedImages)){
                    $invalidTypes++;
                }
            }

            // Check images are valid sizes
            foreach($_FILES['event-images']['size'] AS $size){
                if($size > $maxFileSize){
                    $invalidSize++;
                }
            }

            // Check images are atleast 220px high and 220px wide
            foreach($_FILES['event-images']['tmp_name'] AS $image){

                // Get image width and height
                $image_dimensions = getimagesize($image); // returns an array of image info [0] = width, [1] = height
                $image_width = $image_dimensions[0]; // Image width
                $image_height = $image_dimensions[1]; // Image height

                if(($image_width < $minWidth) || ($image_height < $minHeight)){
                    $invalidDimension++;
                }
            }

            // Add errors to return array
            if($invalidDimension > 0){
                $errors[] = 'One or more of your images has invalid dimensions. Images must be atleast 220px by 220px.';
            }
            if($invalidTypes > 0){
                $errors[] = 'One or more of your images has an invalid image type. Please only select jpg, png or gif images.';
            }
            if($invalidSize > 0){
                $errors[] = 'One or more of your images is an invalid size. Please only select images less than 2MB in size.';
            }

            // Return errors
            return $errors;
        }

I was wondering if this could be made cleaner or more efficient.

There are 4 foreach loops for the same files, going for a little more complex but more
organaized nested loop would be better, like this

foreach ($_FILES['event-images']['tmp_name'] as $tmp_name) {
    if (!empty($tmp_name)){
        $image = $_FILES["event-images"]["name"];
        $path_info = pathinfo($image);
        if ($path_info['extension'] == 'jpg' || $path_info['extension'] == 'jpeg' || $path_info['extension'] == 'png' || $path_info['extension'] == 'gif'){
            if ($_FILES["image"]["size"] <= $maxFileSize){
                $image_d = getimagesize($image);
                if(($image_d[0] >= $minWidth) && ($image_d[1] >= $minHeight)){
                    $errors[]="valid image";
                }else{
                    $errors[]="invalid dimension";
                }
            }else{
                $errors[]="invalid file size";
            }
        }else{
            $errors[]="format not supported";
        }

    }else{
        $errors[] = 'Please select at least one image.';
    }
}


Adding Length dimension to a Crate Object Consider Crate::addLength() in the following code:

class Crate
{
    protected $length, $width, $height;

    function __construct(float $length, float $width, float $height)
    {
        $this->length = $length;
        $this->width = $width;
        $this->height = $height;
    }

    function addLength(float $length): Crate
    {
        return new self($this->length + $length, $this->width, $this->height);
    }
}

I am concerned that Crate object creating a Crate object is a violation of Single Responsibility Principle (SRP).  Crate object duties is to keep dimensions of the crate.  Creating another crate object seems like introducing extra responsibilities to a class that already has responsibilities.

With that in mind, how to I make the code better?

The directions I think I can go are

1)  Is it better to just instead increase the length of the Crate?

function addLength(float $length): void
{
    $this->length += $length;
}

When doing so however, my crate becomes 'stretchy', which is typically not done in real life.  Instead, a new crate is constructed with given measurements.

Thus,

2) Is it better to instead delegate object creation to a separate class?

class CrateService
{

    function addLength(Crate $crate, float $length): Crate
    {
        return new Crate($crate->GetLength() + $length, $crate->getWidth(), $crate->getHeight());
    }
}

That seems to work, even if a little cumbersome - we basically call every crate's dimension individually and add the adder length, and create a new crate.

I see no problem, given your comment, to have a 'stretchy' crate, but in my example code below you can control whether the crate can be stretched or not.

class Crate
{
    private $length, $width, $height;
    private $resizable = true;

    function __construct(float $length, float $width, float $height)
    {
        list($this->length, $this->width, $this->height) = [$length, $width, $height];
    }

    function getDimensions(): array
    {
        return [$this->length, $this->width, $this->height];
    }

    function isResizable()
    {
        return $this->resizable;
    }

    function fixateSize()
    {
        $this->resizable = false;
    }

    function addLength(float $extraLength): float
    {
        return $this->length += $this->isResizable() ? $extraLength : 0;
    }
}

I've added three methods; getDimensions(), isResizable() and fixateSize(). Note that there's no function to undo the fixateSize(). That is intentional: What would be the point of fixating the size if you can undo it?

I've made the dimension and resize fields private. Only the Crate class should be responsible for manipulating these. Child classes should use the methods of Crate to change them, hence the getDimensions() method.

Note how addLength() now returns the new length, and how it doesn't add the $extraLength when the crate size is fixated. Instead of returning the new length you could return a boolean, indicating whether the extra length is accepted. Another reason for not accepting the extra length could be that the crate would become too long.

I wouldn't use the CrateService class, that just overly complicated, and the addLength() method that returns a new object is just weird. The name of the method doesn't suggest this at all. A better name would have been cloneAndStretch().


Get result from form website operation I want to get a result from the website when I submit a form. There are several things that can go wrong.

In a simple scenario I want to return:

true - when form was submitted

false - when form wasn't submitted

null - when we don't know if form was submitted or not

I did that ugly try, because of NullReferenceException (if form is filled correctly, there would be no summary element, and if form is filled incorrectly, there would be no popup element).

How can I make this more elegant?

public bool? GetResult(TimeSpan timeout)
{
    var deadline = DateTime.Now.Add(timeout);

    do
    {
        // check if success
        try
        {
            var success = new WebDriverWait(Driver, TimeSpan.FromSeconds(5))
        .Until(ExpectedConditions.TextToBePresentInElement(Driver.FindElement(By.Id("summary")), "Success!"));
            if (success) return true;
        }
        catch (Exception ex)
        {  }

        // check if not enough data
        try
        {
            var notEnoughData = new WebDriverWait(Driver, TimeSpan.FromSeconds(5))
        .Until(ExpectedConditions.TextToBePresentInElement(Driver.FindElement(By.Id("popup")), "Not enough data"));
            if (notEnoughData) return false;
        }
        catch (Exception ex)
        { }

        // check if too much data
        try
        {
            var tooMuchData = new WebDriverWait(Driver, TimeSpan.FromSeconds(5))
        .Until(ExpectedConditions.TextToBePresentInElement(Driver.FindElement(By.Id("popup")), "Too much data"));
            if (tooMuchData) return false;
        }
        catch (Exception ex)
        { }

        // check if empty data
        try
        {
            var tooMuchData = new WebDriverWait(Driver, TimeSpan.FromSeconds(5))
        .Until(ExpectedConditions.TextToBePresentInElement(Driver.FindElement(By.Id("popup")), "Empty data"));
            if (tooMuchData) return false;
        }
        catch (Exception ex)
        { }

    } while (DateTime.Now > deadline);

    return null;
}

I did that ugly try, because of NullReferenceException (if form is filled correctly, there would be no summary element, and if form is filled incorrectly, there would be no popup element).

This problem can be easyly handled by a null check of the returned WebElement from the call to Driver.FindElement().

A little bit more problematic is IMO the looping condition because the condition only could evaluate to true if the passed TimeSpan is negativ. If this is what you want you really should place a comment there. If you didn't mean to pass a negative timespan then you should reverse the looping condition to DateTime.Now < deadline.

Extracting the repeated call to TimeSpan.FromSeconds(5) to a variable will make the code easier to read.

You know although copy & pasta will save time, you need to check if the pasted code is correct (empty data vs tooMuchData)

    // check if empty data
    try
    {
        var tooMuchData = new WebDriverWait(Driver, TimeSpan.FromSeconds(5))
    .Until(ExpectedConditions.TextToBePresentInElement(Driver.FindElement(By.Id("popup")), "Empty data"));
        if (tooMuchData) return false;

Implementing the mentioned points will look like so

public bool? GetResult(TimeSpan timeout)
{
    var deadline = DateTime.Now.Add(timeout);
    var waitTimeout = TimeSpan.FromSeconds(5);

    do
    {
        WebElement summaryWebElement = Driver.FindElement(By.Id("summary"));
        if (summaryWebElement != null)
        {
            var success = new WebDriverWait(Driver, waitTimeout).Until(ExpectedConditions.TextToBePresentInElement(summaryWebElement , "Success!"));
            if (success) return true;
        }

        WebElement popupWebElement = Driver.FindElement(By.Id("popup"));
        if (popupWebelement != null)
        {
            var notEnoughData = new WebDriverWait(Driver, waitTimeout).Until(ExpectedConditions.TextToBePresentInElement(popupWebElement, "Not enough data"));
            if (notEnoughData) return false;

            var tooMuchData = new WebDriverWait(Driver, waitTimeout).Until(ExpectedConditions.TextToBePresentInElement(popupWebelement, "Too much data"));
            if (tooMuchData) return false;

            var emptyData = new WebDriverWait(Driver, waitTimeout).Until(ExpectedConditions.TextToBePresentInElement(popupWebElement, "Empty data"));
            if (emptyData) return false;
        }

    } while (DateTime.Now > deadline);

    return null;
}

Because the checks for the popup text only differs from the expected text you could think about using a foreach loop over an array containing the expected strings.


Class for typecasting an Activity to an interface In several places, I have some code that looks like the following, the only difference is in the type of listener.

public class CustomFragment extends android.app.Fragment {
    SomeListener listener;

    @Override
    public void onAttach(android.app.Activity activity) {
        super.onAttach(activity);

        // duplication
        //     ||
        //    \||/
        //     \/
        try {
            listener = (SomeListener) activity;
        } catch (ClassCastException e) {
            throw new ClassCastException(activity.toString()
                    + " must implement SomeListener");
        }
    }
}

To avoid duplication, I wrote this simple class:

public class TypeConvertor {
    private final Object whatNeedToCast;

    public static TypeConvertor cast(Object whatNeedToCast) {
        return new TypeConvertor(whatNeedToCast);
    }

    private TypeConvertor(Object whatNeedToCast) {
        this.whatNeedToCast = whatNeedToCast;
    }

    public <OtherType> OtherType to(Class<OtherType> toWhichToCast) {
        try {
            return (OtherType) whatNeedToCast;
        } catch (ClassCastException e) {
            throw new ClassCastException(
                    whatNeedToCast.toString() + " must implement " + toWhichToCast.getName()
            );
        }
    }
}

And the previous code is simplified to the following:

public class CustomFragment extends android.app.Fragment {
    SomeListener listener;

    @Override
    public void onAttach(android.app.Activity activity) {
        super.onAttach(activity);
        listener = TypeConvertor.cast(activity).to(SomeListener.class);
    }
}

But I do not like name TypeConvertor. Help me pick a better name (and help refactor this simple class if needed).

It seems to me like what you have done here is to create an entire class to avoid duplicating one line. (Or OK, 5 lines if counting the try-catch statement)
I don't agree with the need to create a class for this. Especially not a generic one, since this line of code:
return (OtherType) whatNeedToCast;

Does the same thing as:
return OtherType.class.cast(whatNeedToCast);

So all you are doing additionally is to wrap it in another exception which is explained just slightly better. But personally, I think this message...

com.yourpackage.YourActivity@1df59a42 must implement com.yourpackage.SomeListener

...actually says just as much as the default one, if you would not catch the exception in the first place:

java.lang.ClassCastException: Cannot cast com.yourpackage.YourActivity to com.yourpackage.SomeListener

Actually, Cannot cast (class) to (class) would say even more if you would override the toString method in YourActivity.
If you are writing these classes to be used as a library by many other people, then it could be good practice to explain that the activity must implement some interface. Otherwise, I don't see the point of it. But either way, there's no need to create an entire class for this.
Summary:
Get rid of your additional class, just cast your activity to SomeListener and don't catch the exception to throw your own.
So in the end, all that remains is this one line:
listener = (SomeListener) activity;

And it's perfectly OK to duplicate that.