Caesar Cipher Encryption / Decryption in Python I'm trying to write a Python program to predict the most likely substitution for a Caesar cipher. I managed to get it working, but it just seems very clunky and slow, especially because I need to preserve cases and non-alphabetical characters.

Would it be cleaner / more pythonic to use the chr() and ord() functions instead of the ALPHABET list?
Any suggestions on how I could improve it overall (while also maintaining relative readability)?

Thank you so much!
ALPHABET = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
FREQUENCIES = [.0817, .0149, .0278, .0425, .1270, .0223, .0202, .0609, .0697, .0015, .0077, .0402, .0241, .0675, .0751, .0193, .0009, .0599, .0633, .0906, .0276, .0098, .0236, .0015, .0197, .0007]

def getGoodness(msg):
    msg_length = sum([1 for i in msg if i.isalpha()])
    msg_frqs = [round(msg.count(a) / msg_length, 4) for a in ALPHABET]
    diffs = [abs(a - b) for a, b in zip(msg_frqs, FREQUENCIES)]
    avg_diff = round(sum(diffs) / len(diffs), 4)
    return avg_diff

def shift(string, n):
    shifted = [i for i in string]
    for char in range(len(shifted)):
        if shifted[char].isalpha():
            new_char = ALPHABET[(ALPHABET.index(shifted[char].lower()) + n) % len(ALPHABET)]

            if shifted[char].isupper():
                new_char = new_char.upper()
            shifted[char] = new_char
    return ''.join(shifted)

#########################################

msg = "Oqqcfrwbu hc ozz ybckb zokg ct ojwohwcb, hvsfs wg bc kom o pss gvcizr ps opzs hc tzm. Whg kwbug ofs hcc gaozz hc ush whg toh zwhhzs pcrm ctt hvs ufcibr. Hvs pss, ct qcifgs, tzwsg obmkom psqoigs pssg rcb'h qofs kvoh viaobg hvwby wg wadcggwpzs. Mszzck, pzoqy. Mszzck, pzoqy. Mszzck, pzoqy. Mszzck, pzoqy. Ccv, pzoqy obr mszzck! Zsh'g gvoys wh id o zwhhzs. Poffm! Pfsoytogh wg fsorm!"

goodness = []
shifted = []
for i in range(26):
    goodness.append(getGoodness(shift(msg, i)))
    shifted.append(shift(msg, i))
gdict = dict(zip(goodness, shifted))
print(gdict.get(min(gdict.keys())))

The shift() function was fairly easy to understand, in no small part because
you've chosen reasonable and clear variable names. Here are a few relatively
minor suggestions regarding the function. (1) Strings are iterable, so you can
convert to characters directly. (2) Python makes iteration so easy that looping
over data via an index is usually not needed. Instead, just iterate directly.
And for those cases when an index is needed, enumerate() is often the best
option, because it provides both index and value -- which helps your code by
reducing its visual weight and thus aiding readability. (3) The assignment to
new_char is a moderately complex expression. One way to improve code
readability is to such computations apart. (4) A dict to lookup a letter's
index would eliminate the need to scan ALPHABET repeatedly.
# 4
ALPHA_IDXS = {a:i for i, a in enumerate(ALPHABET)}

def shift(string, n):
    # 1
    shifted = list(string)
    # 2
    for i, char in enumerate(shifted):
        if char.isalpha():
            # 3 and 4
            j = (ALPHA_IDXS[char.lower()] + n) % len(ALPHABET)
            new_char = ALPHABET[j]
            if char.isupper():
                new_char = new_char.upper()
            shifted[i] = new_char # The only place we use index.
    return ''.join(shifted)

Another technique to consider is converting shift() into a generator function.
This simplifies the code even more because the function no longer has to
assemble a return data collection -- although at a small cost to the caller,
which must do the assembling. Here's what that might look like.
def shift(string, n):
    for i, char in enumerate(string):
        if char.isalpha():
            j = (ALPHA_IDXS[char.lower()] + n) % len(ALPHABET)
            new = ALPHABET[j]
            char = new if char.islower() else new.upper()
        yield char

You did a nice job in getGoodness() with keeping the steps of the algorithm
very clear. Here are a few suggestions: (1) Because bool is a subclass of
int, you can treat True and False as integers -- just add up the Trues.
Also, functions like sum() take any iterable, so you don't need an
intermediate list. (2) msg_length seems a bit misleading: it is not the
length of msg; it is the number of alpha characters. (3) A Counter can
compute letter frequencies for you. (4)
Shouldn't this calculation be done on a lowercase version of the text?
from collections import Counter

def getGoodness(msg):
    # 1 and 2
    n_alphas = sum(i.isalpha() for i in msg)
    # 3 and 4
    freqs = Counter(msg.lower())
    msg_frqs = [round(freqs[a] / n_alphas, 4) for a in ALPHABET]
    diffs = [abs(a - b) for a, b in zip(msg_frqs, FREQUENCIES)]
    avg_diff = round(sum(diffs) / len(diffs), 4)
    return avg_diff

Finally, some comments about the orchestration code. (1) Put your code in
functions. (2) Set yourself up for testing. It helps a lot during the
development and debugging lifecycle. (3) In the loop to compute goodnesses for
the shifted strings, a tuple seems like a handier data structure than a
dict. It simplifies the code and is better suited for min(). (4) I
recommend the habit of setting up all scripts so they can take arguments --
again, useful during development and sometimes usage.
# 1
def main(args):
    # 2
    TESTS = (
        (
            "Oqqcfrwbu hc ozz ybckb zokg ct ojwohwcb, hvsfs wg bc kom o pss gvcizr ps opzs hc tzm. Whg kwbug ofs hcc gaozz hc ush whg toh zwhhzs pcrm ctt hvs ufcibr. Hvs pss, ct qcifgs, tzwsg obmkom psqoigs pssg rcb'h qofs kvoh viaobg hvwby wg wadcggwpzs. Mszzck, pzoqy. Mszzck, pzoqy. Mszzck, pzoqy. Mszzck, pzoqy. Ccv, pzoqy obr mszzck! Zsh'g gvoys wh id o zwhhzs. Poffm! Pfsoytogh wg fsorm!",
            "According to all known laws of aviation, there is no way a bee should be able to fly. Its wings are too small to get its fat little body off the ground. The bee, of course, flies anyway because bees don't care what humans think is impossible. Yellow, black. Yellow, black. Yellow, black. Yellow, black. Ooh, black and yellow! Let's shake it up a little. Barry! Breakfast is ready!",
        ),
        (
            "Ebiil, tloia!",
            "Hello, world!",
        ),
    )
    for msg, exp in TESTS:
        # 3
        scores = []
        for i in range(26):
            txt = ''.join(shift(msg, i))
            g = (getGoodness(txt), txt)
            scores.append(g)
        got = min(scores)[1]
        # 2 Always be testing.
        print(exp == got, '=>', got[0:30])

# 4
if __name__ == '__main__':
    main(sys.argv[1:])


Append Tuple to Two `Maybe []` The following function appends a tuple to two Maybe [a]'s.

tupleToMaybeList :: (a, a) -> Maybe [a] -> Maybe [a] -> Maybe [a]
tupleToMaybeList (a, b) cs ds = do
    x  <- return a
    y  <- return b
    xs <- cs
    ys <- ds
    return ((x : y : xs) ++ ys)

Please critique it for idiomatic Haskell-ness.

There's a bit of redundancy in your implementation. b <- return a is effectively the same as let b = a. There's no need to “inject” pure values into a monadic context with return, a pure value in scope is a pure value in scope so just use a and b in place of your x and y.

The parentheses around x : y : xs are also technically redundant. Regardless of the ordering of the application of x : y : and xs ++ ys you'll still end up with the same list. This is always the case for list cons and append, if you think about it you should see why. Note that the fixity precedence for both functions is infixr 5.

prependTupleConcatMaybeLists :: (a, a) -> Maybe [a] -> Maybe [a] -> Maybe [a]
prependTupleConcatMaybeLists (a, b) cs ds =
  do xs <- cs
     ys <- ds
     return $ a : b : xs ++ ys

I do question the necessity of writing this particular function. Concatenating cs and ds should probably be taken care of separately to prepending a tuple to a list. E.g.,

prependTupleMaybe :: (a, a) -> Maybe [a] -> Maybe [a]
prependTupleMaybe (a, b) xs = fmap (a:b:) xs

-- call site: let zs = liftA2 (++) (prependTupleMaybe pair xs) ys
-- or       : let zs = prependTupleMaybe pair (liftA2 (++) xs ys)


Brainf**k to Ruby interpreter -- v3 Previous iteration.

I think this is about as good as it's gonna get, but just for kicks, let's do one more round of iterations.

As before, it's a very simple substitution from Brainfuck to Ruby, with fancy things like joining duplicate lines into one and indentation.

Some things that bug me:

The regex (/\++|-+|<+|>+|[.,\[\]]/) feels like it can be simplified, but I can't figure out how.
I feel like the map can be simplified, but I can't figure out how.
The constant repetition of #{'  ' * indent_level} bugs me, but I can't see how to get rid of it.
indent_level being declared outside of the scope of each feels like an anti-pattern, but AFAICT there's no better way to do it.

input_file, output_file = ARGV
code = IO.read(input_file).delete('^+-<>.,[]')

open(output_file, File::CREAT | File::WRONLY) do |output|
  output.puts <<-END.gsub(/^[ \t]*\||\s*#@.*$/, '')
    |#!/usr/bin/env ruby
    |class Mem < Hash #@ Hash because it's more memory-efficient and allows negative values.
    |  def initialize; super(0); end
    |  def []=(i, val); super(i, val & 255); end
    |end
    |data = Mem.new
    |pointer = 0
  END
  indent_level = 0
  code.scan(/\++|-+|<+|>+|[.,\[\]]/)
      .map do |string|
        case string[0]
          when '+' #String of "+"
            next "#{'  ' * indent_level}data[pointer] += #{string.length}"
          when '-' #String of "-"
            next "#{'  ' * indent_level}data[pointer] -= #{string.length}"
          when '<' #String of "<"
            next "#{'  ' * indent_level}pointer -= #{string.length}"
          when '>' #String of ">"
            next "#{'  ' * indent_level}pointer += #{string.length}"
          when '[' #Single "["
            ret = "#{'  ' * indent_level}until data[pointer] == 0"
            indent_level += 1
            next ret #Split it so that it's clear that indent is increased *after* the line
          when ']' #Single "]"
            indent_level -= 1
            next "#{'  ' * indent_level}end"
          when ',' #Single ","
            next "#{'  ' * indent_level}data[pointer] = $stdin.readbyte"
          when '.' #Single "."
            next "#{'  ' * indent_level}putc data[pointer]"
        end
      end.each { |line| output.puts(line) }
end

I'm not 100% sure this works in Ruby, but can you use constants with string interpolation? This string is repeated a lot. I doubt it will ever change, but if it does, it would be nice to only have to change it once.

"#{'  ' * indent_level}data[pointer]"

Could be #{'  ' * indent_level}#{DATA}.


TicTacToe in Haskell Are there any ways which I could improve and simplify this code? Is my code idiomatic Haskell?

Is the use of a lens library to change an element of a two-dimensional array unnecessary? If so, how could I replace it with a more elegant and simpler solution?

Do I need to make use of more types in order to make my code more understandable?

As a Haskell beginner, I am still trying to get used to the functional programming way and would appreciate it if answerers would provide a detailed and easy-to-understand answer (as well as references to materials which I could look up to learn more).

-- Tic Tac Toe implementation in Haskell.

module TicTacToe (
    newGame,
    isWin,
    move
) where

import Control.Lens
import Data.List
import Data.Maybe

-- Size of the grid
n :: Int
n = 3

data Player = X | O deriving (Show, Read, Eq)
type Marking = Maybe Player
type Position = (Int, Int)

type Grid = [[Marking]]

-- An empty grid of size n x n.
emptyGrid :: Grid
emptyGrid = replicate n $ replicate n Nothing

data Game = Game {
    board :: Grid,
    curTurn :: Player
} deriving Show

-- Initial game which board is of size n x n,
-- starting with O as the first player.
newGame :: Game
newGame = Game {
    board = emptyGrid,
    curTurn = X
}

getWinSeqs :: Grid -> [[Marking]]
getWinSeqs grid = horizontal ++ vertical ++ [fDiag, bDiag]
  where horizontal = grid
        vertical = transpose grid
        fDiag = zipWith (\ i x -> i !! (n - x - 1)) grid [0..]
        bDiag = zipWith (!!) grid [0..]

-- Check if a game has been won on a board.
isWin :: Game -> Maybe Player
isWin (Game grid _)
    | isWin' X  = Just X
    | isWin' O  = Just O
    | otherwise = Nothing
    where
        isWin' :: Player -> Bool
        isWin' player = any (all (== Just player)) $ getWinSeqs grid

-- Make the next move.
move :: Game -> Position -> Game
move (Game grid player) (i, j) = Game {
    board = nextGrid,
    curTurn = nextPlayer
} where
    nextGrid =
        if isNothing $ grid !! i !! j
        then set (ix i . ix j) (Just player) grid
        else error "position is occupied"
    nextPlayer
        | player == X = O
        | otherwise   = X

A haskell beginner using Control.Lens... Certainly better than my first program which happened to a tic tac toe solver, so I can compare. We all know Why functional programming matters, don't we?

Make your imports more specific:

import Control.Lens (set,ix)
import Data.List (transpose)
import Data.Maybe (catMaybes)

You will note I replaced your isNothing by pattern matching.
You only use type Position = (Int, Int) once. I would not declare this one.

instead of

    fDiag = zipWith (\ i x -> i !! (n - x - 1)) grid [0..]

I recommend

    fDiag = zipWith (!!) (reverse grid) [0..] -- or
    fDiag = zipWith (!!) grid [n-1,n-2..]

Why force isWin to take Game? Grid would suffice.
I suggest you to write a program that uses this module, then you know better.

OK, now for

move :: Game -> Position -> Game

I'd write

move :: Int -> Int -> Game -> Maybe Game

Instead of throwing an error, return Nothing. Then you can compute all possible moves with

moves :: Game -> [Game]
moves game = catMaybes [move x y game | x<-[0..2], y<-[0..2]]

and I put Game in the last position so you can compose functions like move 2 0 . move 1 1 $ newGame. Actually, that won't work because move now encapsulates in Maybe. But now you can write:

  test = Just newGame >>=
              move 1 1 >>= -- first move in center
              move 2 1 >>=
              move 2 0 >>=
              move 0 2 >>= -- forced
              move 0 0     -- setting up the trap

because Maybe is a monad, and >>= allows forward chaining instead of .! The modified move looks like this:

move :: Int -> Int -> Game -> Maybe Game
move i j (Game grid player) = case grid !! i !! j of
   Just _ -> Nothing
   Nothing -> Just $ Game {
     board = set (ix i . ix j) (Just player) grid,
     curTurn = nextPlayer player
  }

nextPlayer X = O
nextPlayer O = X

I also made nextPlayer a top level function, I'd export it, it looks useful.


Check sum of all primes under two million I'm exploring ruby by writing a code that checks the sum of all prime numbers under two million. As I am still in the process of learning ruby, I'm unfamiliar with all the standards and best practices. What I have so far works, but I am certain it can be improved. Where can I improve my code for both readability and speed?

def is_prime(n)
    range = n
    i = 2

    while i < range do
        return false if (n % i == 0)
        range = n / i
        i += 1
    end
    true
end

sum = 2
(3..2000000).each do |num|
    if (is_prime(num))
        #puts "#{num}"
        sum += num
    end
end

puts "#{sum}"

Ruby favours Functional Programming, let me show you an example.

def prime?(n)
  limit = Math.sqrt(n).to_i + 1
  (2..limit).all? { |i| n % i != 0 }
end

This improves readability as the code makes use of all? to implicitly loop (Instead you looped explicitly) and is faster as the limit is the sqrt of n and not n and all? short-circuits (returns as soon as one condition is false).

Also the second part would benefit from FP:

puts (2..2000000).select{ |n| prime?(n) }.inject(:+)

In general, in Ruby explicit looping is the exception and not the norm, as there are so many built-ins and shortcuts to make use of.

Each function I made use of here is listed at The Ruby Enumerable Documentation and I warmly suggest studying in detail that page, it will take a bit of time, but the time you will save in the future by using it instead of reinventing the wheel each time will be more than worth it.


Making my own programming language This question is really to help me decide on something.  I have started development of my own programming language that I am calling DeliciousWaffle (or maybe samscript).

So far it looks pretty cool.  I built a compiler in Python, and associated .dw files with it.  These files are like .py or .vbs or .java.  They provide the code for the compiler to interpret. I also programmed an IDE for it using wxPython.

Here are a few examples of what it looks like:

The fully functional IDE:

A calculator program that I wrote in DeliciousWaffle:

And here is the code for the compiler.  You can download this if you'd like and try out the syntax.

#deliciouswaffle
import sys
import linecache
varss={}
def Say(vartype,text):
    if vartype=='normal':
        print text
    if vartype=='var':
        print varss.get(str(text))
def Set(var,val):
    global varss
    varss.update({str(var):val})
def Get(var,prmpt):
    val=raw_input(prmpt)
    try:
        val=int(val)
        x=val+1
    except:
        val=str(val)
    Set(var,val)
def Loop():
    global line
    line=0
def IfStr(var1,functlines):
    global line
    var=varss.get(var1)
    try:
        x=var+1
        line+=functlines
    except TypeError:
        pass
def IfInt(var1,functlines):
    global line
    var=varss.get(var1)
    try:
        x=var+1
        pass
    except TypeError:
        line+=functlines
def Convert(var):
    val=varss.get(var)
    if type(val) == type('str'):
        val=int(val)
    if type(val) == type(1):
        val=str(val)
    varss.update({var:val})
def If(var1,compare,var2,functlines):
    global varss
    global line
    var1=varss.get(str(var1))
    var2=varss.get(str(var2))
    if compare=='equ':
        if var1==var2:
            pass
        else:
            line+=functlines
    if compare=='lss':
        if var1<var2:
            pass
        else:
            line+=functlines
    if compare=='gtr':
        if var1>var2:
            pass
        else:
            line+=functlines
def ChangeVar(var,operation,do):
    global varss
    val=varss.get(var)
    if operation=='+':
        val+=do
    if operation=='-':
        val-=do
    if operation=='/':
        val/=do
    if operation=='*':
        val*=do
    varss.update({var:val})
def CombineVars(newvar,var1,var2,operation):
    global varss
    var1=int(varss.get(var1))
    var2=int(varss.get(var2))
    if operation=='+':
        val=var1+var2
    if operation=='-':
        val=var1-var2
    if operation=='/':
        val=var1/var2
    if operation=='*':
        val=var1*var2
    varss.update({newvar:val})
def End():
   global eof
   eof=1
line=0
eof=0

try:
    program = (sys.argv[1])
except:
    program = 'main.dw'

if program == 'main.dw':
    try:
        f=open('main.dw','r')
        f.close()
    except:
        print 'No Delicious Waffle script loaded.'
        print 'Create a script called main.dw, or load one into Delicious Waffle.'
        raw_input()
        sys.exit()

while eof==0:
    try:
        line+=1
        try:
            eval(linecache.getline(program, line))
        except:
            pass
    except:
        eof=1
        f=open('tempvars.file','w')
        f.close()
        sys.exit()

What do you think?  Is this worth completing?  Or should I leave it be?  This question is probably inappropriate for code review, sorry.

I second the recommendation to invent a grammar and use a real parser.  A recursive descent parser is very easy to write, and a great place to start.  You might also have a look at a PEG (Parsing Expression Grammar).  It's almost as easy to use a PEG as it is to write a recursive descent compiler, and a PEG can parse more grammars than can a recursive descent compiler.
Here are two PEGs for Python (I haven't used either of them, so can't vouch for them):

pyPeg
parsimonious

I don't see a good reason to get the Dragon book yet.  Much of what it teaches is how to write the LEXX/YACC type tools, along with language theory.  I think it's too deep and too low level for someone who just wants to try creating a language in order to see how it's done.  If, however, you do want to dive that deep, it is a very good book.

I would, for now, steer clear of a traditional lex/yacc setup.  That's a harder road that can wait until you've had some experience with simpler schemes.  However, if you do want to go there, PLY is a lex/yacc type parser for Python.


Prime generator SPOJ problem in Python 3 I am trying to solve an SPOJ problem: print all prime numbers in a given range (as large as \$10^9\$). I have used the Sieve of Eratosthenes algorithm. But it is still slow when input is in range of \$10^4\$.

import math
no_of_cases = int(input())

for i in range(no_of_cases):
    x = input().split(" ")
    a = int(x[0])
    b = int(x[1])
    lis = set([p*i for p in range(2,b+1) for i in range(2,b+1)])

    print (sorted(set(range(a,b+1))- lis))

I have already asked a similar question over here.

I'll convert your program to a primes_below function. (I don't take into account the lower bound.)
After doing this I got:

def primes_below(n):
    lis = set([p * i for p in range(2, n + 1) for i in range(2, n + 1)])
    return sorted(set(range(2, n + 1)) - lis)

First things first, you need a maximum of \$n ^ 2\$ amount of memory.
Why?
If we look at your algorithm what is \$i_{\text{max}}\$ and \$p_{\text{max}}\$?
Both are \$n\$. And so your current largest number is \$n^2\$.

You said:

  I also tried using square root but that was throwing error.

This also wouldn't work correctly.
Lets go through every combination if we were to do that with 16.

2 * 2 = 4
2 * 3 = 6
2 * 4 = 8
3 * 2 = 6
3 * 3 = 9
3 * 4 = 12
4 * 2 = 8
4 * 3 = 12
4 * 4 = 16

This means that 4, 6, 8, 9, 12, 16 are not prime. We should know that the primes in this range are 2, 3, 5, 7, 11, 13.
And so 10, 14, 15 are all incorrectly said as prime.

But looking at the above you should be able to see that there is no point on having \$3 * 2\$, \$4 * 2\$, \$4 * 3\$.
This is as you've already done those calculations and so you should start the range for \$i\$ at \$p\$.
And this is true for your current solution.

And so we know that we should at least use:

def primes_below(n):
    lis = set([p * i for p in range(2, n + 1) for i in range(p, n + 1)])
    return sorted(set(range(2, n + 1)) - lis)

Now we need to remove the creation of numbers greater than (or equal) \$n\$.
Rather than looking at some complex maths, we'll have a look at range.

  range(stop)
  range(start, stop[, step])

  This is a versatile function to create lists containing arithmetic progressions.
  It is most often used in for loops. The arguments must be plain integers.
  If the step argument is omitted, it defaults to 1.
  If the start argument is omitted, it defaults to 0.
  The full form returns a list of plain integers [start, start + step, start + 2 * step, ...].
  If step is positive, the last element is the largest start + i * step less than stop; if step is negative,
  the last element is the smallest start + i * step greater than stop.
  step must not be zero (or else ValueError is raised).

This means that you can use this instead of your multiplication!

From the above we know that the first number we use is \$p * p\$, this is as \$i\$ starts as \$p\$.
The largest number we want is also \$n\$.
But we don't want to remove all the numbers that are between \$p^2\$ and \$n\$,
and so we need that step.
That step will also be \$p\$.

Or in Python:

def primes_below(n):
    lis = set([i for p in range(2, n) for i in range(p * p, n, p)])
    return sorted(set(range(2, n)) - lis)

Before we say all is good, some maths!
What is the largest \$p\$ that will mean the second range contains a value?

If \$p^2 > n\$ then the second range will be empty, and so the largest \$p\$ follows the equation \$p^2 <= n\$.
And so \$p_{\text{max}} = \sqrt{n}\$.

And adding this to the range significantly improves performance.

def primes_below(n):
    lis = set([i for p in range(2, int(n ** 0.5) + 1) for i in range(p * p, n, p)])
    return sorted(set(range(2, n)) - lis)

This function is roughly ~3800 times faster than the original, at n = 5000.

Now you probably think it's good enough for this challenge, it's pretty fast!
But unfortunately no. Actually I thought I'd broke my machine when I ran it the first time, and I have 32GB of RAM!
I closed every app I had on my machine and the program still broke!
This means that you have to care about the memory and your current method doesn't.

Instead we need to limit the amount of memory to \$n\$.

To do this you can use lis = [True] * n.
And then you want to change them to False if you come across one.
And you'll want to set 0 and 1 to false at the beginning.
The same way you are at the moment.
This should get you something like:

def primes_below(n):
    lis = [True] * n
    lis[0] = lis[1] = False
    for p in range(2, int(n ** 0.5) + 1):
        for i in range(p * p, n, p):
            lis[i] = False
    return ...

Now you want to be able to return the numbers.
Python has enumerate which will give us the index and the value,
this will allow us to make a comprehension.
And so if the value is True add the index to the output.
Which will result in:

def primes_below(n):
    lis = [True] * n
    lis[0] = lis[1] = False
    for p in range(2, int(n ** 0.5) + 1):
        for i in range(p * p, n, p):
            lis[i] = False
    return (i for i, v in enumerate(lis) if v)

Since you don't want to display some numbers you'll want to implement a lower bound, I'd do this out of the function as then if you ever need the Sieve again you'll have a 'pure' one.

This can simply be another comprehension:

numbers = (i for i in primes_below(b + 1) if i < a)

And will make usage simple:

no_of_cases = int(input())
for _ in range(no_of_cases):
    a, b = map(int, input().split(" ")[:2])
    print([i for i in primes_below(b + 1) if i < a)])

This however is still too slow for my liking.
And so to make it 3 times faster, at n = 10 ** 7, you can add a check if p is prime, if it is then do what we were doing before if not go to the next number.

This results in:

def primes_below2(n):
    lis = [True] * n
    lis[0] = lis[1] = False
    for p in range(2, int(n ** 0.5) + 1):
        if not lis[p]:
            continue
        for i in range(p * p, n, p):
            lis[i] = False
    return (i for i, d in enumerate(lis) if d)

This is roughly 11500 times faster than the original, at n = 5000.
And takes about 244 seconds, on my machine, to generate all the primes below 10 ** 9.


Equivalent of Python's dict.update for STL maps in C++ dict objects in Python have an update method that can be used to extend one dictionary with all items from another, possibly overwriting entries that already exist.  I find this function very useful from time to time.

The associative containers from the C++ standard library (std::map and std::unordered_map) do not provide a direct analogue for this functionality so I wrote the following utility functions.

Comments regarding any aspect of the code are welcome.

#include <cassert>
#include <utility>

namespace codereview
{

    template <typename DictT>
    void update(DictT& dst, const DictT& src)
    {
        for (const auto& [key, value] : src) {
            dst.insert_or_assign(key, value);
        }
    }

    template <typename DictT>
    void update(DictT& dst, DictT&& src) noexcept
    {
        // Equal allocators are a precondition for splicing nodes
        assert(src.get_allocator() == dst.get_allocator());
        dst.merge(src);
        while (!src.empty()) {
            auto node = src.extract(src.cbegin());
            dst.extract(node.key());
            dst.insert(std::move(node));
        }
    }

}  // namespace codereview

You're not thinking laterally here.  If you std::swap() your two inputs, then you can merge() the original into the new, and the contents of the new map will prevail.

Here's what I mean:

namespace codereview
{
    template <typename DictT>
    void update(DictT& dst, DictT src)
    {
        std::swap(dst, src);
        dst.merge(src);
    }

}  // namespace codereview

// Test
#include <iostream>
#include <map>

template<typename Map>
void print(const Map& x)
{
    for (auto const& i: x)
        std::cout << i.first << "=" << i.second << " ";
    std::cout << std::endl;
}

int main()
{
    using Map = std::map<int, const char*>;
    Map a { {0, "zero"}, {1, "one"} };
    const Map b { {0, "nothing"}, {2, "two"}};

    print(a);

    // test lvalue argument
    codereview::update(a, b);
    print(a);

    // test rvalue argument
    codereview::update(a, Map{{3, "three"}, {0, "nada"}});
    print(a);
}

Output

0=zero 1=one
0=nothing 1=one 2=two
0=nada 1=one 2=two 3=three

Note that I changed the method signature to accept src by copy - the compiler will move-construct into src if it can, but if not, we'll need a copy so that we don't perturb the caller's version when we swap.


Page Enum with overridden methods for the first and last members I am managing the changing of views in an Android app using this enum and associated methods. I am particularly interested in the Page enum's use of overriding previous() and next() for the first and last elements.

Is this readable?
Is this idiomatic?
Is there a preferable data structure?
Are there general stylistic improvements that could be made?

public enum Page {
  INTRO { @Override public Page previous() {return this; } },
  PUBLIC_PROFILE, PRIVATE_PROFILE, SOCIAL_ACCOUNTS, LOOPS, INVITE,
  FINISHED { @Override public Page next() { return this; }; };

  public Page next() {
    return values()[ordinal()+1];
  }

  public Page previous() {
    return values()[ordinal()-1];
  }
}

private Page page = Page.INTRO;

private void setPage(Page page) { ... }

private void nextPage() {
  page = page.next();
  setPage(page);
}

private void previousPage() {
  page = page.previous();
  setPage(page);
}

Interesting question. Overriding methods in enum instances is not something I have encountered (that I recall). This is not to say it's a bad thing, just not very common. (P.S. having just claimed not to have encountered enum overrides, I see it is used in this answer given shortly before you asked this very question

The concept is something I have solved using different mechanisms before, but I can understand why having the override makes sense to you.

What I don't understand is why you did not just use a regular bound-clipping system. Why do you need the overrides at all?

Consider these methods:

  public Page next() {
    if (ordinal() == values().length - 1) {
        return this;
    }
    return values()[ordinal() + 1];
  }

  public Page previous() {
    if (ordinal() == 0) {
        return this;
    }
    return values()[ordinal() - 1];
  }

There is no reason to override them at all.

On the other hand, if you do override them, then please use a standard indentation for the override methods. Cramming it all on one line is not helpful:

public enum Page {
    INTRO {
        @Override
        public Page previous() {
            return this;
        }
    },
    PUBLIC_PROFILE,
    PRIVATE_PROFILE,
    SOCIAL_ACCOUNTS,
    LOOPS,
    INVITE,
    FINISHED {
        @Override
        public Page next() {
            return this;
        };
    };


Python sum of multiples of some numbers in a range I saw the Project Euler challenges online and I decided to code out a few. The first challenge involved finding the sum of multiples. So I decided to have my code be able to find the sum of multiples for any given range, and for any amount of natural numbers.

My code allows a user to input as many natural numbers as they want and then set the range of multiples that user wants to sum and then sum those multiples.

I just wanted any input on if there's a more efficient way of doing this.

def main():
    # Initialize variables
    multiples = []
    sums = []
    i = 0
    integers = 1

    # Detemines the number of multiples from user
    while True:
        try:
            num_mul = int ( input ( 'How many multiples are you using in total?: ' ) )
            break
        except ValueError as e:
            print ( e )

    # Takes multples and store in list
    while True:
        try:
            for y in range ( num_mul ):
                multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(integers) ) ) )
                integers = integers + 1
            break
        except ValueError as e:
            print ( 'Integer Values ONLY' , e )

    # determine the start and stop of the range from user
    while True:
        try:
            start = int ( input ( 'Start of range: ' ) )
            stop = int ( input ( 'End of range: ' ) ) + 1
            break
        except ValueError as e:
            print ( e )

    # Figures out the multiples of the numbers given between the start and end range
    while i < len ( multiples ):

        for k in range (start, stop ):
            if (k % multiples[i]) == 0:
                sums.append(k)
            else:
                continue

        i = i + 1  # increments through the list values by 1

    x = sum(set(sums))  # Gets rid of duplicated values and sums the list of values

    print ( 'Your SUM total is {}.'.format ( x ) )

if __name__ == "__main__": main ( )

there is some room for improvement here, I tried to adapt a bit your solution without changing it a lot, to make it a bit less repetitive and more understandable
Will explain you some steps :)
Reading input
First of all, you are reading the input from the user on a few places, always in the same way, asking for input again unless an integer is provided. To avoid repetition, this functionality can be extracted into a function
def read_integer(input_message):
    while True:
        try:
            number = int(input(input_message))
            return number
        except ValueError:
            print('Integer Values ONLY')

You just pass the message you want to appear on screen and it will ignore all non integer values, asking for input again
Reuse of variables
Sometimes, you can use same variable for different purposes.
In order to read input in a loop, you can use the same variable that is introduced in the loop as an index, avoiding the creation of the new variable integers only for that
So this code
integers = 1
for y in range ( num_mul ):
    multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(integers) ) ) )
    integers = integers + 1

Can be reduced to
for y in range ( num_mul ):
    multiples.append ( int ( input ( 'Please input your No.{} integer here: '.format(y+1) ) ) )

Avoid unnecessary code
In the end, there is an else condition that simply continues to next iteration. That will also happen even if the else condition does not exist, saving you some unnecessary lines of code
A bit later, you turn all multiples into a set to avoid repetition. That can be handled before, inserting only if they are not already in the list
if (k % multiples[i]) == 0 and k not in sums:
    sums.append(k)

Naming
You could use a bit better naming in variables and so, I modified a bit some of them so you can check but things like num_mul can be better understood with longer naming, like multiples_count
Summary
You can check how your solution would look like with this features, I applied the changes that I suggested you:
def read_integer(input_message):
    while True:
        try:
            number = int(input(input_message))
            return number
        except ValueError:
            print('Integer Values ONLY')

def main():
    # Initialize variables
    multiples = []
    sums = []

    # Determines the number of multiples from user
    input_message = 'How many multiples are you using in total?: '
    multiples_count = read_integer(input_message)

    # Takes multiples and store in list
    for y in range(multiples_count):
        input_message = 'Please input your No.{} integer here: '.format(y+1)
        new_multiple = read_integer(input_message)

        multiples.append(new_multiple)

    # determine the start and stop of the range from user
    input_message = 'Start of range: '
    start = read_integer(input_message)

    input_message = 'End of range: '
    stop = read_integer(input_message)

    # Figures out the multiples of the numbers given between the start and end range
    for i in range(multiples_count):
        for k in range(start, stop):
            if (k % multiples[i]) == 0 and k not in sums:
                sums.append(k)

    x = sum(sums)  # Gets rid of duplicated values and sums the list of values

    print('Your SUM total is {}.'.format(x))

if __name__ == "__main__":
    main()


Counting sort in C, revised This is a revised follow-up to this question.

I have implemented most of the suggestions in the accepted answer. I am interested to see what I could improve in my code and what I could do to make it better.

  I have implemented counting sort in C. This program takes its input as integers from command line arguments, sorts the integers with counting sort, then outputs the sorted array.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>

int max(int* arr, size_t len) {
    if (len < 1) {
        return 0;
    }
    int greatest = arr[0];
    for (size_t i = 0; i < len; i++)
        if (arr[i] > greatest)
            greatest = arr[i];
    return greatest;
}

void sort(int* inputs, int* outputs, size_t length) {
    // this is the size of the array of counts
    int greatest = max(inputs, length); // find the greatest number in the array

    // allocate the array of counts
    int* counts = calloc(greatest + 1, sizeof(int));
    if (counts == NULL) {
        fprintf(stderr, "Memory allocation error\n");
        exit(EXIT_FAILURE);
    }

    // count numbers in input array
    for (size_t i = 0; i < length; i++) {
        counts[inputs[i]]++;
    }

    size_t counter = 0; // keep track of where we are in output array

    // loop through all the counts
    for (int i = 0; i <= greatest; i++) {   // for every count in array
        for (int j = 0; j < counts[i]; j++) {   // loop that many times
            outputs[counter++] = i; // add the integer being counted to the output array
        }
    }
    free(counts);
}

int main(int argc, char** argv) {
    int *inputs, *outputs;
    size_t length = argc - 1;   // number of integers to sort

    if (argc < 2) {
        fprintf(stderr, "Not enough arguments given.\n");
        exit(EXIT_FAILURE);
    }

    inputs = calloc(length, sizeof(int));
    outputs = calloc(length, sizeof(int));

    if (inputs == NULL || outputs == NULL) {
        fprintf(stderr, "Memory allocation error\n");
        exit(EXIT_FAILURE);
    }

    for (int i = 1; i < argc; i++) {
        inputs[i - 1] = atoi(argv[i]);  // assign arguments to array
        if (inputs [i - 1] < 0) {
            fprintf(stderr, "Integer %d out of range [0, 2^31)\n", inputs[i - 1]);
            exit(EXIT_FAILURE);
        }
    }

    sort(inputs, outputs, length);

    for (size_t i = 0; i < length; i++) {
        printf("%d ", outputs[i]);  // print space separated sorted numbers
    }
    printf("\n");

    free(inputs);
    free(outputs);

    return EXIT_SUCCESS;
}

Enable more compiler warnings

I get

gcc-8 -std=c11 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds      -Wconversion    199801.c    -o 199801

199801.c:21:35: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
     int* counts = calloc(greatest + 1, sizeof(int));
                          ~~~~~~~~~^~~
199801.c: In function ‘main’:
199801.c:45:21: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
     size_t length = argc - 1;   // number of integers to sort

The second of these can simply be silenced with a cast (since we have confirmed that argc >= 2), but the first probably indicates that greatest should be an unsigned type.  In fact, as we only accept positive values as input, we should probably use that same unsigned type for all the values we accept.

Match allocations and types automatically

You can help your code adapt better to changes in type (and make it easier to review), by allocating memory in terms of the pointer being assigned to:

int* counts = calloc(greatest + 1, sizeof *counts);

inputs = calloc(length, sizeof *inputs);

Now, if you decide to change type of counts is changed (perhaps to unsigned short), then it only needs to be changed in the one place, and there's less risk of inconsistency.  For inputs, it's now clear to reviewers that the type matches, without having to search back for the declaration (that said, in this case, it's probably better to declare it where it's initialized - we're not in C89 any more!).

Don't compare element 0 against itself

This is a trivial change, but will show understanding:

int greatest = arr[0];
for (size_t i = 0; i < len; i++)
    if (arr[i] > greatest)
        greatest = arr[i];

There's no need to compare arr[0] within the loop.  So we can change the for line to:

for (size_t i = 1;  i < len;  i++)

Beware of atoi()

This function doesn't report failures very well - if its input can't be converted, it just returns 0, which is indistinguishable from atoi("0").  I recommend you look at using strtol() and family for conversion:

char *end;
long val = strtol(argv[i], &end, 10);
if (val < 0 || val > INT_MAX || !argv[i][0] || !*end) {
    fprintf(stderr, "Failed to convert input value '%s'\n", argv[i]);
    return EXIT_FAILURE;
}
inputs[i - 1] = val;

Note that my error message doesn't make assumptions about the value of INT_MAX as yours does.

Use malloc() when memory doesn't need to be cleared.

I know you've been advised to use only one of malloc() and calloc() in your program, but I disagree.  Use calloc() when you want the extra behaviour of clearing the contents (or where the product of the two arguments might overflow a size_t), and use malloc() for allocations where you will be writing to the memory yourself.

Use puts() in preference to printf()

We can avoid the complexity of printf() when we output a simple newline-terminated string:

/*printf("\n");*/
puts("");    /* simpler */

Similarly:

    fputs(stderr, "Memory allocation error");


Get first DateTime by given weekdays and a starttime At work there is an input-system where customers are able to specify weekdays and a starting time for an event. The weekdays are integers in the range from 1 (Monday) to 7 (Sunday).
After the customer has given the needed input, I need to calculate the next possible DateTime for the event. If the event is on the same day as today and the current time is not yet over the submitted starting-time, the next possible DateTime would be today.

This is what I have came up with, but it looks awful and complicated. You can obviously shorten the linq-queries a little, but i let the intermediate objects in there for sake of debugging.

public static class Test
{
    public static void Main( string[] args )
    {
        var now = DateTime.Now;
        var startTimes = new List<TimeSpan> {
            now.TimeOfDay ,                                     // get the next possible starting point
            now.TimeOfDay.Add( new TimeSpan( 0 , 1 , 0 ) ) ,    // starting point is today
        };
        var weekdaysList = new List<int[]> {
            new [] { 1 , 4 , 5 } ,
            new [] { 1 , 2 } ,
        };

        foreach( var weekdays in weekdaysList )
        {
            foreach( var startTime in startTimes )
            {
                var dt = GetNext( weekdays , startTime );
            }
        }
    }

    public static DateTime GetNext( int[] weekdays , TimeSpan startTime )
    {
        // convert to german format (Monday:1 - Sunday:7)
        var currentWeekDay = (int)DateTime.Now.DayOfWeek;

        if( currentWeekDay == 0 )
            currentWeekDay = 7;

        // if today, and not yet passed, return todays starting time
        if( weekdays.Contains( currentWeekDay ) )
        {
            var today = DateTime.Now.Date.Add( startTime );

            if( DateTime.Now < today )
                return today;
        }

        // calculate the day index and keep the added days around (better debugging)
        var days = Enumerable.Range( 0 , 7 )
            .Select( addedDays => new { addedDays , dayIndex = ( currentWeekDay + addedDays ) % 7 } )
            .ToArray();
        // only days that are specified
        var validDays = days
            .Where( x => weekdays.Contains( x.dayIndex ) )
            .ToArray();
        // the first day is the one that has the least days added
        var daysToAdd = validDays.First().addedDays;
        // but we already sorted out today, so the next possible day is the second (if exists) or 1 week/ 7 days later
        if( daysToAdd == 0 )
        {
            if( validDays.Count() > 1 )
                daysToAdd = validDays.ElementAt( 1 ).addedDays;
            else
                daysToAdd = 7;
        }

        return DateTime.Now.Date
            .AddDays( daysToAdd )
            .Add( startTime );
    }
}

I feel like I am missing some obvious shortcuts or linq-fu to shorten this code down and make it easier understandable.

I let this code as it is, but it was bugging me the whole day, how could I improve this?

Only the basic algorithm in the GetNext()-Method is relevant, no error-checking etc.

After some fresh air I think I got around my mental blockade. I simply convert to the natural C# DayOfWeek representation beforehand instead of fiddling around with the other one.

public static IEnumerable<DayOfWeek> ConvertDaysOfWeek( int[] daysOfWeekIds )
    => daysOfWeekIds.Select( x => x == 7 ? DayOfWeek.Sunday : (DayOfWeek)x );

public static DateTime GetNext( IEnumerable<DayOfWeek> daysOfWeek , TimeSpan startTime )
{
    var nextDate = DateTime.Now.Date;
    var isToday = daysOfWeek.Contains( nextDate.DayOfWeek ) && DateTime.Now.TimeOfDay < startTime;

    if( !isToday )
        nextDate = Enumerable.Range( 1 , 7 )
            .Select( x => nextDate.AddDays( x ) )
            .First( x => daysOfWeek.Contains( x.DayOfWeek ) );

    return nextDate.Add( startTime );
}


Wrapping single line string to multiple lines with specific length Problem

A single line should be separated by Environment.NewLine so that each row has a length of columns (expect for the last one that may be shorter).

Below is my solution:

    public static string Wrap(this string singleLineString, int columns)
    {
        if (singleLineString == null)
            throw new ArgumentNullException("singleLineString");
        if (columns < 1)
            throw new ArgumentException("'columns' must be greater than 0.");

        var rows = Math.Ceiling((double)singleLineString.Length/columns);
        if (rows < 2) return singleLineString;

        var sb = new StringBuilder();

        for (int i = 0; i < rows; i++)
        {
            if (i > 0) sb.Append(Environment.NewLine);
            var index = i*columns;
            var length = Math.Min(columns, singleLineString.Length - index);
            var line = singleLineString.Substring(index, length);
            sb.Append(line);
        }

        return sb.ToString();
    }

Even if the problem is really simple, there are lots of thinkable approaches to solve it. Is there simpler / more elegant solution than the one above?

I think you shouldn't build a new string there because it's a new responsibility. The method should only split the text and return its parts with the specified length.

The user should decide what to do with the results next. Maybe he does not want to use the Environment.NewLine but some other new-line?

I prefer it to be a split-only method:

public static IEnumerable<string> Split(this string text, int partLength)
{
    if (text == null) { throw new ArgumentNullException("singleLineString"); }

    if (partLength < 1) { throw new ArgumentException("'columns' must be greater than 0."); }

    var partCount = Math.Ceiling((double)text.Length / partLength);
    if (partCount < 2)
    {
        yield return text;
    }

    for (int i = 0; i < partCount; i++)
    {
        var index = i * partLength;
        var lengthLeft = Math.Min(partLength, text.Length - index);
        var line = text.Substring(index, lengthLeft);
        yield return line;
    }
}

I need to complain about the missing {} but I guess you probably know that ;-)

The names of the variables could also be a little bit clearer.

Technically you don't need this if:

    if (partCount < 2)
    {
        yield return text;
    }

The result will be the same with out it because the for would execute only once .


Copy directories while excluding or isolating certain extensions I often find myself wanting to copy directories around with either:

having to copy only specific extensions, or
having to exclude specific extensions.

For this task, I wrote some Python which I am calling from the cmd.

The Python script is humbly called copy_master.py and I have also set an environment variable for it called %Pcopy%.

  copy_master.py

def classify(dirs):
    return {(1, 0): dirs, (0, 1): dirs[::-1], (0, 0): [None, None], (1, 1): [1, 1]}[tuple(map(os.path.isdir, dirs))]

def include(d, files):
    return [f for f in files if not any(f.endswith('.' + x) for x in sys.argv[4].split(',')) and not os.path.isdir(os.path.join(d, f))]

def refuse(d, files):
    return [f for f in files if any(f.endswith('.' + x) for x in sys.argv[4].split(',')) and not os.path.isdir(os.path.join(d, f))]

if __name__ == '__main__':
    import shutil
    import os
    import sys

    if len(sys.argv) < 3 or sys.argv[1].lower() == 'help':
        print('\nParameters:: \n\t > Source and Destination directories: order does not matter; one of the specified '
              'directories MUST NOT exist and this is taken to be the destination. \n\t > Mode: (+) for copying specific'
              ' file extensions only OR (-) for skipping specific extensions. \n\t > Extensions: comma delimited list of'
              ' file extensions to be copied or ignored.\n\nExample Usage:\n\t{0:} {1:} + bdf  -> Copies only bdf files '
              'from {1:} to {0:}\n\t{0:} {1:} - f06,xdb  -> Copies everything but f06 & xbd files from {1:} to {0:}'.format(r'I:\do\not\exist', r'I:\have\the\files\you\want'))
    elif len(sys.argv) > 2:
        src, dst = classify(sys.argv[1:3])
        if src is None:
            raise IOError('The source directory should exist!')
        elif src == 1:
            raise IOError('The destination directory should not exist!')

        if len(sys.argv) == 5:
            mode = sys.argv[3]
            print('\rCopying...', end='')
            shutil.copytree(src, dst, ignore={'+': include, '-': refuse}[mode])
            print('\rCopying completed')
        else:
            q = input('Extensions and execution mode have not been specified. Everything is going to be copied! Proceed (Y/N)?\n::')
            if q.lower() in ['n', 'no']:
                exit()
            else:
                print('Copying...')
                shutil.copytree(src, dst)

Example usages (with the command prompt open, type):

copy only specific extension:

%Pcopy% I:\do\not\exist I:\have\the\files\you\want + txt
#                                                  ^

exclude specific extensions from the copying:

%Pcopy% I:\do\not\exist I:\have\the\files\you\want - xlsx,bat
#                                                  ^

There are a couple of things I am not so happy\sure about:

Have I sacrificed too much readability in the classify() function? Are dicts meant to be (ab)used like that? What should I be returning in the (1, 1) case (if both directories exist)?
Is the use of global sys.argv in include() and refuse() ok?
How can I handle the different ways a user might use the script in the best possible way? Not quite happy about the if statements in the __main__.

Apart from the points discussed above, feel free to suggest other changes or even an entirely different approach. And by all means, use the script if you like it ;)

Bonus Points:
Currently, directories that have to be quoted to be recognized by the cmd are not handled (e.g., C:\users\Example\This&That, note the &)

Is the use of global sys.argv in include() and refuse() ok?

At the moment it's not really readable, since sys.argv[4] doesn't say much... This could be solved by using argparse or naming your sys.argv to variables

  How can I handle the different ways a user might use the script in the best possible way? Not quite happy about the if statements in the __main__.

Use argeparse that will relieve the clutter from the if __name__ == '__main__' and make it easier to add optional parameters in your script.

def parse_arguments():
    """Arguments parser."""

    parser = argparse.ArgumentParser(usage='%(prog)s [optional] <source> <destination>',
                                 description='Copy master',
                                 formatter_class=argparse.RawDescriptionHelpFormatter,
                                 epilog='''<your help string>''')

    # Next you can add arguments like this...
    parser.add_argument('source', type=str, help='source directory')
    parser.add_argument('destination', type=str, help='destination directory')
    parser.add_argument('-t', '--types', type=str, help='included types')
    ...

    # Parse the arguments to the object
    args = parser.parse_args()

    # Check if all args are set or something
    if not args.types:
        parser.error('No typyes are specified')
    return args

# Now you can call this from the main
if __name__ == '__main__':
    import argparse
    args = parse_arguments()
    # And call your arguments like...
    # args.source, etc...

Bonus

imports should be at the top of the file!

At the moment when this script is imported to another module, it will not import all dependencies.

Note that you can import argparse in __main__, since this will on be executed from the cli
(Subjective style) You might consider to chop lines for better readability

return [f for f in files if any(f.endswith('.' + x) for x in sys.argv[4].split(',')) and not os.path.isdir(os.path.join(d, f))]

Could be this:

return [file
        for file in files if any(file.endswith('.' + x)
        for x in sys.argv[4].split(','))
        and not os.path.isdir(os.path.join(d, f))]

At your help string consider using a heredoc string

"""Heredoc string""" Or consider setting it as the docstring __doc__ = """Help"""


Project Euler 3: Getting the largest prime factor of a number I'm looking for some general feedback on my solution to Project Euler challenge 3

  The prime factors of 13195 are 5, 7, 13 and 29. What is the largest
  prime factor of the number 600851475143 ?

let p3 () =
    let rec primeFactors n i primes =
        if i > n/2L then n::primes else
            let quotient, remainder = Math.DivRem(n, i)
            if remainder = 0L then primeFactors quotient 2L (i::primes)
            else primeFactors n (i + 1L) primes
    primeFactors 600851475143L 2L []

There's nothing really wrong with your code but here's a slight rewrite using pattern matching which is arguably a more functional style than if ... then ... else. I also rearranged the args to eliminate the unnecessary n parameter on the outer function.

let primeFactors =
    let rec recPrimeFactors primes i = function
        | n when 2L*i > n -> n::primes
        | n -> match n % i with
               | 0L -> recPrimeFactors (i::primes) 2L (n / i)
               | _ -> recPrimeFactors primes (i + 1L) n
    recPrimeFactors [] 2L

600851475143L |> primeFactors |> List.head |> printfn "%d"


Simple BMI Calculator (Python 3) As my first Python program I have written a simple BMI calculator. I want to know how i can improve this program. Would functions be useful? How can I improve the way that it is presented?

#Introduce BMI Calculator
print ('Welcome to BMI Calculator!')

#Ask if user would like to use Metric or Imperial units
input1 = input ('Would you like to input Metric units or Imperial units? ')

#If Imperial... If Metric... Calculate BMI...
if input1 == 'imperial':
    heightIN = float(input('Input your height in Inches (in): '))
    weightLB = float(input('Input your weight in Pounds (lbs):' ))
    BMI = round(((weightLB/(heightIN*heightIN))*703), 2)

    if BMI >= 19 and BMI <= 24:
        print ('Your BMI is', BMI, 'so, you are healthy weight!')

    elif BMI >= 25 and BMI <= 29:
        print ('Your BMI is', BMI, 'so, you are overweight!')

    elif BMI >= 30 and BMI <= 39:
        print ('Your BMI is', BMI, 'so, you are obese!')

    elif BMI > 39:
        print ('Your BMI is', BMI, 'so, you are extremely obese!')

    elif BMI < 19:
        print ('Your BMI is', BMI, 'so, you are underweight!')

elif input1 == 'metric':
    heightCM = float(input('Input your height in Centimeters (cm): '))
    weightKG = float(input('Input your weight in kilograms (kg): '))
    heightM = heightCM*0.01
    BMI = round((weightKG/(heightM*heightM)), 2)

    if BMI >= 19 and BMI <= 24:
        print ('Your BMI is', BMI, 'so, you are healthy weight!')

    elif BMI >= 25 and BMI <= 29:
        print ('Your BMI is', BMI, 'so, you are overweight!')

    elif BMI >= 30 and BMI <= 39:
        print ('Your BMI is', BMI, 'so, you are obese!')

    elif BMI > 39:
        print ('Your BMI is', BMI, 'so, you are extremely obese!')

    elif BMI < 19:
        print ('Your BMI is', BMI, 'so, you are underweight!')

else:
    print ('There was an error with your input. Please restart the program!')

It's probably best to use a function that uses SI units - you can then convert lbs to kgs and in to cm. Additionally, since the majority of the world either uses Imperial or metric, we can use a Boolean to indicate which one to use:

def BMI(height,weight,is_metric):
    if (is_metric):
        # Height in meters, weight in kg
        return weight / height**2
    else:
        # Height in inches, weight in lbs
        height = height * 0.0254
        weight = weight * 0.453
        return BMI(height,weight,True)


Validating inputs in the constructor Let's take the following example. Even if the number of parameters is acceptable, I find that the code block is too long.

Are these validations at the right place? Is there a better approach in such cases?

public function __construct(
    array $config,
    EntityManager $em,
    EventDispatcherInterface $dispatcher
) {
    $this->em = $em;
    $this->dispatcher = $dispatcher;
    if (isset($config['endPoint'])) {
        $this->endpoint = $config['endPoint'];
    } else {
        throw new ConfigurationException();
    }
    if (isset($config['partnerLogin']) && isset($config['partnerPass'])) {
        $this->partnerLogin = $config['partnerLogin'];
        $this->partnerPass = $config['partnerPass'];
    } else {
        throw new MissingCredentialsException();
    }
    if (isset($config['account_id'])) {
        $this->ids['account_id'] = $config['account_id'];
    } else {
        throw new MissingCredentialsException();
    }
    if (isset($config['user_id'])) {
        $this->ids['account_id'] = $config['account_id'];
    } else {
        throw new MissingCredentialsException();
    }
    $this->apiClient = $this->createApiClient();
}

I would probably create a separate validateConfig() method on the class, such that your code might look like:

public function __construct(
    array $config,
    EntityManager $em,
    EventDispatcherInterface $dispatcher
) {
    $this->validateConfig($config);
    $this->em = $em;
    $this->dispatcher = $dispatcher;
    $this->endpoint = $config['endPoint'];
    // set other config-driven properties here
    $this->apiClient = $this->createApiClient();
}

protected function validateConfig($config) {
    if (!isset($config['endPoint'])) {
        throw new ConfigurationException();
    }
    if (!isset(
        $config['partnerLogin'],
        $config['partnerPass'],
        $config['account_id'],
        $config['user_id']
    ) {
        throw new MissingCredentialsException();
    }
}

However, that might not be getting you where you ultimately would like to go.  I would suggest that with a config with as many "properties" (your array keys) as you have, that you might consider creating an actual config class to be passed to constructor. This would allow you to type hint against the object type and place logic for validating the config into its own class.  This class would then be 100% assured the dependencies it needs in its constructor are met.

This approach could easily be achieved without much change to the calling code.  Instead of something like:

$object = new YourClass($config, $em, $dispatcher);

You could simply do:

$object = new YourClass(
    new YourClassConfig($array),
    $em,
    $dispatcher
);

Also, if you are going through the trouble of validating dependencies in this manner, is only looking for presence of an array value being set going far enough to make sure your object is set up in proper state? What if these values are set to empty strings or other non-sensical values for your context?


Javascript map/filter/reduce over facebook messenger data Context: I am making a website that allows people to upload their Facebook Messenger data, and see data visualizations for conversations. It is a React front-end, and a nodejs back-end. It doesn't store anything in a database at the moment, but there is a caching layer to deal with different sessions. So once the user uploads their facebook messenger data, the nodejs back-end is supposed to crunch the numbers and aggregate different statistics.

The json looks like:

{
  "participants": [{"name":"Adam"}, {"name":"Bob"}, {"name":"Chris"}],
  "messages": [
    {
      "sender_name": "Adam",
      "timestamp_ms": 1533332999910,
      "content": "I am editing my stackexchange question right now",
      "type": "Generic"
    },
    {
      "sender_name": "Bob",
      "timestamp_ms": 1533332999910,
      "content": "Okay, good luck",
      "type": "Generic"
    }
  ],
  "title": "High School Friends",
  "is_still_participant": true,
  "thread_type": "RegularGroup",
  "thread_path": "inbox/highschoolfriends__aAe4vb"
}

messages is an array of objects, with common text messages having the shape of the above, with other fields that are optional like photos, audio_files, etc.

To compute the set up the data I use:

let jsonMessages = require('../analytics/message_1.json');

let participants = jsonMessages.participants;
let messages = jsonMessages.messages;

let people =  participants.map(function(person) {
    return person.name;
});

So now the data is a javascript object.

So my question: Am I missing something obvious about the map/filter/reduce powers by doing a big for loop of filters? Would this pass code review for an internal analytics tool?

Here is what I have tried:

let people = ['Adam', 'Bob', 'Chris', 'Dave'];

people.forEach(function (person) {
    let obj = {};
    obj.name = person;
    obj.messageCount = messages.filter((obj) => obj.sender_name === person).length;

    obj.photoMessageCount = messages.filter((obj) => ((obj.sender_name === person) && ((obj.photos != null) )).length;

    obj.videoGifMessageCount = messages.filter((obj) => ((obj.sender_name === person) && ((obj.gifs !=null) || (obj.videos!= null))).length;

    obj.audioMessageCount = messages.filter((obj) => ((obj.sender_name === person) && (obj.audio_files != null))).length;

    obj.richContentMessageCount = obj.photoMessageCount + obj.videoGifMessageCount + obj.audioMessageCount;

    stats.push(obj);
});

A big inefficiency of the initial approach is that for each person (which is actually a person's name) messages collection will be traversed 4 times.

To significantly optimize the traversal algorithm the current one is substituted with a single loop with multiple conditions per iteration.
All the needed specific counts are declared beforehand and used as accumulators.The Consolidate conditional expression technique is applied based on a common condition m.sender_name === personName.

The final optimized version:

people.forEach((personName) => {
    let stat = {'name': personName, 'messageCount': 0, 'photoMessageCount': 0,
               'videoGifMessageCount': 0, 'audioMessageCount': 0};
    messages.forEach((m) => {
        if (m.sender_name === personName) {
            stat.messageCount++;
            if (m.photos) stat.photoMessageCount++;
            if (m.gifs || m.videos) stat.videoGifMessageCount++;
            if (m.audio_files) stat.audioMessageCount++;
        }
    });
    stat.richContentMessageCount = stat.photoMessageCount + stat.videoGifMessageCount
                                   + stat.audioMessageCount;
    stats.push(stat);
});

In case if your stats array is intended to be used only for accumulating statistics from people.forEach(...) iterated once - prefer Array.map approach to create stats array with the results at once:

let stats = people.map((personName) => {
    let stat = {'name': personName, 'messageCount': 0, 'photoMessageCount': 0,
               'videoGifMessageCount': 0, 'audioMessageCount': 0};
    // all the logic here
    // ...
    return stat;
});


Multiple similar methods to compute the average student grade I have bunch of similar methods in different classes to average student grades.

In the Student class:

public double getAverageGrade() throws NoGradesForStudentException {
    double sum = 0;
    int totalGrades = 0;
    ArrayList<Subject> studentSubjects = getStudentSubjects();
    for(Subject subject : studentSubjects) {
        try {
            sum += getAverageGradeInSubject(subject);
            totalGrades++;
        }
        catch(NoGradesInSubjectException e) {
            //everything is okey
        }
    }
    if (totalGrades == 0) {
        throw new NoGradesForStudentException("Student does not have any grades yet");
    } else {
        return sum/totalGrades;
    }
}

In the GroupOfCourses class:

public double getAverageGrade() throws NoGradesInCourseException {
    double gradeSum = 0;
    double studentsWithGrades = 0;
    for(Student student : getStudents()) {
        try {
            gradeSum += student.getAverageGrade();
            studentsWithGrades++;
        }
        catch (NoGradesForStudentException e) {
            //viss kartiba, ja izmet so iznemumu
        }
    }
    if (studentsWithGrades != 0) {
        return gradeSum/studentsWithGrades;
    } else {
        throw new NoGradesInCourseException("There are no grades in course.");
    }
}

This one is in the main and user input is mingled with the logic. But the logic is the same as in previous examples.

private static void calculateSubjectAvgGrade() {
    ArrayList<Subject> allSubjects = Main.getSubjectsInAllCourses();

    for(int index = 0; index < allSubjects.size(); index++) {
        System.out.println(index + ". " + allSubjects.get(index).getTitle());
    }
    System.out.print("Izvelieties prieksmetu ievodot ta numuru: ");
    int number = Input.consoleIntIn();

    double sum = 0;
    int subjectsWithGrade = 0;
    for (Course course : Main.courses) {
        try {
            sum += course.getAverageGradeInSubject(allSubjects.get(number));
            subjectsWithGrade++;
        }
        catch(NoGradesInSubjectException e) {
            //viss ok
        }
    }
    double avg = subjectsWithGrade != 0 ? sum/subjectsWithGrade : 0;
    System.out.println("Prieksmeta videja atzime ir: " + avg);
}

I was wondering is there a way I could create one function and call it in those methods? It would look something like this:

public double getAvarageGrade() throws NoGradesException {
     return function(studentSubjects, "Student does not have any grades yet");
}

I'm asking this because I have lot of methods in different classes that duplicate their logic.

I'm using Java 8.

Since you are writing for Java 8, what you want to use is DoubleStream.average().  Java 8 streams are a new and powerful technique that drastically changes how code can be written in Java.

public class Student {

    ...

    public double getAverageGrade() throws NoGradesForStudentException {
        return getStudentSubjects().stream()
                                   .mapToDouble(Subject::getAverageGradeInSubject)
                                   .average()
                                   .orElseThrow(() -> new NoGradesForStudentException("Student does not have any grades yet"));
    }
}

This code is compact enough that near-duplication of code would not be a problem.

However, the NoGradesForStudentException would cause a bit of a problem.  In particular, with a stream-based solution, ignoring cases where a student has no grade in a particular subject would be a tricky exercise in exception handling.  A better approach is to embrace Java's convention for DoubleStream.average(), and change all of these getAverageGrade…() methods to return an OptionalDouble instead of a double.  Then, the code would look like this:

public class Student {

    ...

    public OptionalDouble getAverageGrade() {
        return getStudentSubjects().stream()
                                   .mapToObj(Subject::getAverageGradeInSubject)
                                   .filter(OptionalDouble::isPresent)
                                   .mapToDouble(OptionalDouble::getAsDouble)
                                   .average();
    }
}

Note that taking the average of the average of grades in each subject is not necessarily the same as taking the average of grades.  It may be appropriate to rename the method to make it clear that there is batching by subject.  Similarly, your GroupOfCourses.getAverageGrade() needs to make it clear that it calculates the average of each student's grade.


Is this a conforming implementation of duration_cast? Sadly VS2012's duration_cast is broken, and I actually need the functionality which is broken. So, I wrote my own:

template<typename ToUnit, typename Rep, typename Period>
ToUnit duration_cast_2(const std::chrono::duration<Rep, Period>& right)
{
    typedef std::ratio_divide<Period, typename ToUnit::period>::type ratio;
    typedef std::common_type<std::common_type<typename ToUnit::rep, Rep>::type, intmax_t>::type common_type;
    return ToUnit(static_cast<typename ToUnit::rep>(static_cast<common_type>(right.count()) * static_cast<common_type>(ratio::num) / static_cast<common_type>(ratio::den)));
}

But I'm not entirely confident that it's correct.

I can't say for sure about the internal logic but I can point some problems or things that could be improved in your piece of code:

First of all, typename is missing before the names, making your code non-working with some compilers. I would also use using instead of typedef, but that's a mere matter of taste. Updated version:

using ratio = typename std::ratio_divide<Period, typename ToUnit::period>::type;
using common_type = std::common_type<typename std::common_type<typename ToUnit::rep, Rep>::type, intmax_t>::type;

std::common_type is variadic. You don't have to bother with nested std::common_types:

using common_type = typename std::common_type<typename ToUnit::rep, Rep, intmax_t>::type;

I know that MSVC has not a full support for constexpr, but duration_cast is supposed to be constexpr (it should work with the November 2013 CTP though):

template<typename ToUnit, typename Rep, typename Period>
constexpr ToUnit duration_cast_2(const std::chrono::duration<Rep, Period>& right)
{
    // ...
}


Script for creating custom hosts files I wrote a basic Python script that consolidates and customizes several popular adblocking hosts files.  I was interested in making it more efficient if possible, and I'd like to improve my style and best practices.

import urllib.request
import re
import os

os.chdir('C:/Windows/System32/drivers/etc')

# This contains a list of URLs where popular hosts files are located.
sources = [...]

def listDomains(sources):
    domains = set()
    for s in sources:
        lines = list(urllib.request.urlopen(s))
        for l in lines:
            str = l.decode('utf-8')
            if isValid(str):
                d = getDomain(str)
                if isDomestic(d):
                    domains.add(d)
    return domains

def isValid(line):
    match = re.search('^(0\.0\.0\.0|127\.0\.0\.1)', line)
    if match:
        return True
    return False

def getDomain(str):
    return str.split()[1]

def isDomestic(domain):
    match = re.search('\.(com|net)$', domain)
    if match:
        return True
    return False

def makeHostsFile(domains):
    f = open('hosts', 'w')
    for d in domains:
        f.write('0.0.0.0 ' + d + '\n')
    f.close()

makeHostsFile(listDomains(sources))

Overall this looks pretty good, but there are some places where you can write more idiomatic Python. Some general comments:

More docstrings and comments would be nice. For example: what is a “domestic” URL, and why does it match com/net? Explain why the code is written this way – it will make things much easier if you have to debug/modify a program later.
Using single letter variable names is a bad habit. It’s better to use words, because they’re more descriptive and make the program easier to read. (They’re also easier to grep for later!)
The Python convention for variable names is lowercase_with_underscores, not dromedaryCase. (The exception is class names, which are PascalCase.)

Now some specific comments about your Python style:

Don’t use str as a variable name; this is the name of a builtin function. Overriding the names of the builtin functions is a bad practice, because it will cause all sorts of bugs later.
To avoid excessive indentation, I’d combine the two if statements at the end of listDomains into a single line:

if is_valid(my_str) and is_domestic(get_domain(my_str)):
     domains.add(get_domain(my_str))

Rather than doing if match: return True; else: return False in isValid and isDomestic, you can just do return match. That’s shorter, easier to read and more idiomatic.
In getDomain(), you should explain the format of the line that you’re expecting, so that I know why you’re taking the element in index 1.
In makeHostsFile, rather than having explicit open() and close() calls, the Python idiom is to use a with statement:

with open('hosts', 'w') as f:
    for domain in domains:
        f.write('0.0.0.0 {domain}\n'.format(domain))

Note also that I’ve used new-style string formatting rather than string concatenation; this is the preferred way to do this sort of construction.
I would extend the makeHostsFile function to take an optional path to the hosts file, so that this code is more portable.
Rather than doing your program at top-level, wrap it in a main() function and call as follows:

def main():
    import os
    hosts_file = 'C:/Windows/System32/drivers/etc/hosts'
    make_hosts_file(list_domains(sources), hosts_file)

if __name__ == '__main__':
    main()

The code inside main() is only executed if the script is run directly, but if you import this file from another program, it will be ignored. That makes it easier to reuse this code, because there aren’t any funny side-effects from importing it.

In terms of speed, the regexes seem pretty simple. Is there any reason you couldn’t just do

def is_valid(line):
    return ('0.0.0.0' not in line) and ('127.0.0.1' not in line)

def is_domestic(line):
    return line.endswith('.com') or line.endswith('.net')

which feels like it would probably be a lot faster?


Implementing the merge sort with only arrays, out of place This has been asked a few times here, but I was wondering what I could do to improve the efficiency and readability of my code. My programming skill has gotten rusty, so I would like to elicit all constructive criticism that can make me write code better.

init:

#include <iostream>
using namespace std;

merge()

//The merge function
void merge(int a[], int startIndex, int endIndex)
{

int size = (endIndex - startIndex) + 1;
int *b = new int [size]();

int i = startIndex;
int mid = (startIndex + endIndex)/2;
int k = 0;
int j = mid + 1;

while (k < size)
{
    if((i<=mid) && (a[i] < a[j]))
    {
        b[k++] = a[i++];
    }
    else
    {
        b[k++] = a[j++];
    }

}

for(k=0; k < size; k++)
{
    a[startIndex+k] = b[k];
}

delete []b;

}

merge_sort()

//The recursive merge sort function
void merge_sort(int iArray[], int startIndex, int endIndex)
{
int midIndex;

//Check for base case
if (startIndex >= endIndex)
{
    return;
}

//First, divide in half
midIndex = (startIndex + endIndex)/2;

//First recursive call
merge_sort(iArray, startIndex, midIndex);

//Second recursive call
merge_sort(iArray, midIndex+1, endIndex);

//The merge function
merge(iArray, startIndex, endIndex);

}

main()

//The main function
int main(int argc, char *argv[])
{
int iArray[10] = {2,5,6,4,7,2,8,3,9,10};

merge_sort(iArray, 0, 9);

//Print the sorted array
for(int i=0; i < 10; i++)
{
    cout << iArray[i] << endl;
}

return 0;
}

Try not to use using namespace std.
Code within a function should be indented as well.  You already do this with other code blocks, and the same applies to functions.
Just avoid Hungarian notation:

int iArray;

We already know it's an int array.
Prefer not to pass C arrays to functions in C++.  This causes them to decay to a pointer; it does not actually pass the array itself.

Instead, pass in a storage container, such as an std::vector.  Storage containers will not decay to a pointer, and they're more idiomatic C++.

If you have C++11, initialize the vector using list initialization:

std::vector<int> values { 2, 5, 6, 4, 7, 2, 8, 3, 9, 10 };

If you don't have C++11, declare it and call push_back() for each value:

std::vector<int> values;

values.push_back(2);
values.push_back(5);
// ...

Pass it to a function with such parameters:

void merge_sort(std::vector<int> values, int startIndex, int endIndex) {}

As its implementation is that of an array, you can still access its elements with [].  You can also use its iterators (more preferred).

Storage containers do the memory allocation for you, so you will not need new/delete.  It's best to do as little manual memory allocation as possible in C++.


Search for rows in a 2D array that contain a given sequence I have a 2d array of m * n dimension (m and n can vary from 1 to 100000). The following snippet of code checks if the sequence exists in the row and stores the index if it exists. The time taken by the following code on 10000*10000 matrix is 650 milliseconds. The sequential version of the code also takes the same time.

This on Intel Core i7-7560U CPU @ 2.40GHz × 4

#include <iostream>
#include <vector>
#include <future>

std::vector<int> lps;

void ComputeLPSArray(std::vector<int> const &pattern) {

    std::vector<int> lps(pattern.size());
    int len = 0;

    lps[0] = 0;

    int i = 1;
    while (i < (int) pattern.size()) {
        if (pattern[i] == pattern[len]) {
            len++;
            lps[i] = len;
            i++;
        } else {
            if (len != 0) {
                len = lps[len - 1];
            } else {
                lps[i] = 0;
                i++;
            }
        }
    }
}

int SearchPattern(std::vector<int> const &pattern, std::vector<int> const &row) {
    auto M = (int) pattern.size();
    auto N = (int) row.size();

    int i = 0;
    int j = 0;
    while (i < N) {

        if (pattern[j] == row[i]) {
            j++;
            i++;
        }
        if (j == M) {
            return 1;
        } else if (i < N && pattern[j] != row[i]) {
            if (j != 0)
                j = lps[j - 1];
            else
                i = i + 1;
        }
    }
    return -1;
}

std::vector<int> SearchSequence(std::vector<std::vector<int>> const &matrix, std::vector<int> const &sequence) {

    ComputeLPSArray(sequence);
    std::vector<int> result(matrix.size());
    unsigned int length = 0;
    std::vector<std::future<int>> f(matrix.size());

    for (unsigned int i = 0; i < matrix.size(); i++) {
        std::vector<int> row = matrix[i];
        f[i] = async(std::launch::async, [sequence, row] { return SearchPattern(sequence, row); });
    }

    for (unsigned int i = 0; i < f.size(); ++i) {
        if (f[i].get() == 1)
            result[length++] = i;
    }

    result.resize(length);

    return result;
}

int main() {

    int m = 25, n = 20; // assume `m` and `n` can vary.
    std::vector<std::vector<int >> matrix(m, std::vector<int>(n));

    for (int i = 0; i < m; ++i) {
        for (int j = 0; j < n; ++j) {
            matrix[i][j] = rand() % 1000;
        }

    }

    std::vector<int> sequence = matrix[24]; // This is purely for testing purpose, actual input(matrix&sequence) is read from files.

    std::vector<int> result;
    result = SearchSequence(matrix, sequence); // This is the ONLY function which needs to be optimised.

    return 0;
}

How do I speed up the search time in this scenario?

I rewrote your SearchSequence function to be as absolutely brain-dead as possible:

#include <iostream>
#include <vector>
#include <algorithm>
#include <chrono>

std::vector<int> SearchSequence(std::vector<std::vector<int>> const &matrix, std::vector<int> const &sequence)
{
    std::vector<int> result;

    for (unsigned int i = 0; i < matrix.size(); i++) {
        auto it = std::search(matrix[i].begin(), matrix[i].end(), sequence.begin(), sequence.end());
        if (it != matrix[i].end()) {
            result.emplace_back(i);
        }
    }
    return result;
}

int main() {

    int m = 25000, n = 25000; // assume `m` and `n` can vary.
    std::vector<std::vector<int >> matrix(m, std::vector<int>(n));

    auto start_time = std::chrono::steady_clock::now();

    for (int i = 0; i < m; ++i) {
        for (int j = 0; j < n; ++j) {
            matrix[i][j] = rand() % 1000;
        }
    }

    std::vector<int> sequence = matrix[9999]; // This is purely for testing purpose, actual input(matrix&sequence) is read from files.

    auto initial_time = std::chrono::steady_clock::now();
    std::cout
        << "Initializing the array with rand() took: "
        << std::chrono::duration_cast<std::chrono::milliseconds>(initial_time - start_time).count() << " ms\n";

    std::vector<int> result = SearchSequence(matrix, sequence); // This is the ONLY function which needs to be optimised.

    auto search_time = std::chrono::steady_clock::now();
    std::cout
        << "Executing the actual search took: "
        << std::chrono::duration_cast<std::chrono::milliseconds>(search_time - initial_time).count() << " ms\n";

    return 0;
}

On my laptop, this prints:

$ clang++ -std=c++14 -O3 ./x.cc
$ ./a.out
Initializing the array with rand() took: 5304 ms
Executing the actual search took: 2 ms

$ clang++ -std=c++14 -O0 ./x.cc
$ ./a.out
Initializing the array with rand() took: 7670 ms
Executing the actual search took: 8 ms

This is single-threaded, with absolutely no cleverness to the search at all — I'm just using the $O(n^2)$ std::search algorithm from the standard library.

I suggest that your business with std::vector<std::future<int>> is completely unnecessary, and that if you're measuring any slowness, it's slowness in initializing the matrix with random numbers.


Refactoring same loop logic with different input data I have these two methods:

public static String buildLine(String[] values) {
    StringBuilder lineBuilder = new StringBuilder();
    for (int i = 0; i < values.length - 1; i++) {
        lineBuilder.append(values[i]).append(SEPARATOR);
    }
    lineBuilder.append(values[values.length - 1]);
    return lineBuilder.toString();
}

public static String buildLineFromNodeWorker(String[] values, NodeDecorator nodeWorker) {
    StringBuilder lineBuilder = new StringBuilder();
    for (int i = 0; i < values.length - 1; i++) {
        lineBuilder.append(
                nodeWorker.getItemValueFromAttribute(values[i]))
                .append(SEPARATOR);
    }
    lineBuilder
            .append(nodeWorker
                    .getItemValueFromAttribute(values[values.length - 1]));
    return lineBuilder.toString();
}

they're identical except for what I'm passing to the append method from the StringBuilder.

I'm not able to find a way to write the "business logic" of the loop just once.

One way using the Template method pattern:

abstract class LineBuilder {
    protected abstract String getLine(String value);

    public String buildLine(String[] values) {
        StringBuilder lineBuilder = new StringBuilder();
        for (int i = 0; i < values.length - 1; i++) {
            lineBuilder.append(getLine(values[i])).append(SEPARATOR);
        }
        lineBuilder.append(getLine(values[values.length - 1]));
        return lineBuilder.toString();
    }
}

class StringLineBuilder extends LineBuilder {
    protected String getLine(String value) {
        return value;
    }
}

class NodeWorkerLineBuilder extends LineBuilder {
    private final NodeDecorator nodeWorker;

    public NodeWorkerLineBuilder(NodeDecorator nodeWorker) {
        this.nodeWorker = nodeWorker;
    }

    protected String getLine(String value) {
        return nodeWorker.getItemValueFromAttribute(value);
    }
}

Once you define the above classes you can replace the code inside your methods with:

public static String buildLine(String[] values) {
    return new StringLineBuilder().buildLine(values);
}

public static String buildLineFromNodeWorker(String[] values,
        NodeDecorator nodeWorker) {
    return new NodeWorkerLineBuilder(nodeWorker).buildLine(values);
}


Count number of words in a text file Is this a good approach or is there some other solution that I am not aware of?

//C++ program to count number of words in text file

#include<fstream>
#include<iostream>
#include<string>
using namespace std;

int main()
{
    ifstream inFile; //Declares a file stream object
    string fileName;
    string word;
    int count = 0;

    cout << "Please enter the file name ";
    getline(cin,fileName);

    inFile.open(fileName.c_str());

    while(!inFile.eof())
    {
        inFile >> word;
        count++;
    }

    cout << "Number of words in file is " << count;
    inFile.close();

    cin.get();
    return 0;
}

Your code has a few problems.

You should learn to not use using namespace std;. It's generally frowned upon.
You should never use while(!inFile.eof()). It's pretty much a guaranteed bug.
You should use standard algorithms when they're applicable (as they are here).
Prefer to fully initialize variables at creation (e.g., pass file name when you create a filestream object).
Generally let the destructor handle destruction (e.g., let the filestream object close when it goes out of scope)1.

I'd also strongly prefer to take command line arguments over prompting for input at run time.

I'd write the code using the standard distance algorithm, something like this:

#include <fstream>
#include <algorithm>
#include <iterator>
#include <iostream>
#include <string>
#include <cstdlib>

int main(int argc, char **argv) {
    if (argc < 2) {
        std::cerr << "Usage: count_words <infile>\n";
        return EXIT_FAILURE;
    }

    std::ifstream infile(argv[1]);

    std::istream_iterator<std::string> in{ infile }, end;

    std::cout << "Word count: " << std::distance(in, end);
}

There are a few cases where it makes sense to manually close a file. For example, if you're moving a file between file systems by copying its content, then deleting the original, you want to do everything you can to ensure the copy completed successfully (including successful closing) before you remove the original. Anything that might destroy the user's data calls for extraordinary measures to assure safety. This isn't one of those cases though.


Inserting filename, reading data, inserting regex, and testing if each line matches This program first displays a menu which shows some options that the user can select. Each option will the respective functions which do some things.

The user can insert a filename, read data from it, enter a regex pattern, test if each line of the file matches the given regex pattern.

Please provide feedback on how I can improve my program.

import re

filename = None
readData = None
regex    = None

def test_data():
    '''Checks if each line of data matches the regex'''

    global readData
    global regex

    if regex == None:
        print('Empty regex')
        print('')
        return None

    try:
        pattern = re.compile(regex)
    except:
        print('Invalid regex')
        print('')
        return None
    i = 1
    if readData == None:
        print('Read data is empty')
        print('')
        return None

    for line in readData:
        if pattern.match(line.strip()):
            print(' Line ', i, ':"', line,'"', ' matches current regex', sep='')
        else:
            print(' Line ', i, ':"', line,'"', ' does not match current regex', sep='')
        i += 1
    print('')

def change_regex():
    '''Sets the regex variable'''

    global regex
    print('Current regex is ', end='')
    if regex != None:
        print('"', regex, '"', sep='')
    else:
        print('empty')

    regex = input('Enter new regex: ')
    print('')

def read_file():
    '''Reads data from file if possible'''

    global filename
    global readData
    try:
        file = open(filename, "r")
        readData = file.readlines()
        print('Data read successfully!')
        file.close()
    except TypeError:
        print('filename is empty!')
    except FileNotFoundError:
        print('File not found!')

    print('')

def change_filename():
    '''Sets the filename variable'''

    global filename
    print('Current filename is ', end='')
    if filename != None:
        print('"', filename, '"', sep='')
    else:
        print('empty')

    filename = input('Enter new filename (with extension): ')
    print('')

def show_menu():
    '''Shows the menu'''

    print('Enter 1 to change filename')
    print('Enter 2 to read file')
    print('Enter 3 to change regex')
    print('Enter 4 to test if read data matches given regex')
    print('Enter 5 to exit')

def main():
    while True:
        show_menu()
        try:
            option = int(input('Option: '))
        except:
            print('Invalid input')
            print('Please enter a valid option')
            print('')
            continue
        if option == 1:
            change_filename()
        elif option == 2:
            read_file()
        elif option == 3:
            change_regex()
        elif option == 4:
            test_data()
        elif option == 5:
            break
        else:
            print('Invalid input')
            print('Please enter a valid option')
            print('')

if __name__ == "__main__":
    main()

Some thoughts:

Don't use globals, pass variables as function arguments.
Follow pep8
Don't use == None.  False, 0, 0.0, None, and empty sequences like lists or strings are all considered False, so in your case you can just do something like if not regex.  If you need to test for None specifically, use is None.
Don't do return None, just an empty return will return None automatically.
Don't do a blind except:, you should always check for a specific exception.
For the loop, use enumerate to keep track of the current index.
Use with open(filename, 'r'):.  This will automatically close the file when you leave the with block, even if there is an exception, so you don't need to manually close.
In read_file, don't catch the exception at all.  The default exceptions give the user more information and more useful information than your exceptions, so just let them be handled normally.
It is clearer to do something like print('"%s"' % regex) rather than print('"', regex, '"').  Or better yet regex = '"%s"' % regex if regex else 'empty', then print('Current regex is', regex).
show_menu shouldn't be a separate function.


Random walk with equal probability This method randomly increments or decrements (with equal probability) until either -3 or 3 are reached. I'd like to maintain printing the value of pos on each iteration, and printing the largest positive number max reached at the end of the loop. How can I improve this method using the Random() function? I know this sounds pretty ambiguous, but I'd like to know if I can make rand.nextInt(2) == 0 an easier condition to understand? Any suggestions/improvements are welcome!

public static void randomWalk() {
    Random rand = new Random();
    int pos = 0;
    int max = 0;
    System.out.println("position = " + pos);
    while (pos > -3 && pos < 3) {
        if (rand.nextInt(2) == 0) {
            pos++;
            if (pos > 0) {
                max = pos;
            }
        } else {
            pos--;
        }
        System.out.println("position = " + (int) pos);

    }
    System.out.println("max position = " + max);
}

There's a minor bug there:

        if (pos > 0) {
            max = pos;
        }

should test pos > max instead.

Use

max = Math.max(max, pos)

to make it shorter.

  I'd like to know if I can make rand.nextInt(2) == 0 an easier condition to understand?

Use rand.nextBoolean(). For a probability of exactly 50%, it's perfect.

For other probabilities, you can use

 rand.nextDouble() < probability

Another possibility is simply

pos += 2 * rand.nextInt(2) - 1;
max = Math.max(max, pos);

or

pos += rand.nextBoolean() ? 1 : -1;
max = Math.max(max, pos);

or

private static final int[] DELTAS = {+1, -1};

pos += DELTAS[rand.nextInt(DELTAS.length)];
max = Math.max(max, pos);

without any conditionals. You may consider it tricky, but it isn't. Obviously, my last solution shouldn't be used for such a simple case.

Concerning the computation of max it may be slightly less efficient since it gets executed even if the value decreases. But this doesn't matter unless you need to optimize heavily (your print is many orders of magnitude slower than this).


Implementing views on a std::vector Goal:

Wrap a vector. The wrapper can create view on part of the vector.
Support const_view and view.
view allows modification of elements while const_view does not.
The views are ranges (has begin() and end() methods).

Problem:

Code duplication between the const_view and view.

The actual code implements a 2D sparse array that saves more memory than usual. So I want to learn how to solve this problem.

Simplified Code (implements a dense 2D array)

#include <vector>
class A_csub;
class A_sub;

typedef std::vector<int> V;
typedef typename V::iterator It;
typedef typename V::const_iterator Cit;

class A {
// 2D dense array with _r rows and _c columns.
public:
    A(const int r, const int c);
    A_csub cview(const int n) const;
    A_sub view(const int n); // view nth row
    It sub_begin(const int n); // iterator at begin of nth row
    Cit sub_cbegin(const int n) const;
    int _r;
    int _c;
private:
    V _v;
};

class A_csub {
public:
    A_csub(const A& a, const int n);
    Cit begin() const;
    Cit end() const;
    const int size() const;
private:
    const A& _a;
    const int _n;
};

class A_sub {
public:
    A_sub(A& a, const int n);
    It begin();
    It end();
    const int size() const;
private:
    A& _a;
    const int _n;
};

// -- A -- //
A::A(const int r, const int c) : _r(c), _c(c), _v(r*c) {}
A_csub A::cview(const int n) const { return A_csub(*this, n); }
A_sub A::view(const int n) { return A_sub(*this, n); }
It A::sub_begin(const int n) { return _v.begin() + n*_r; }
Cit A::sub_cbegin(const int n) const { return _v.cbegin() + n*_r; }

// -- A_csub -- //
A_csub::A_csub(const A& a, const int n) : _a(a), _n(n) {}
Cit A_csub::begin() const { return _a.sub_cbegin(_n); }
Cit A_csub::end() const { return begin() + _a._r; }
const int A_csub::size() const { return _a._r; }

// -- A_sub -- //
A_sub::A_sub(A& a, const int n) : _a(a), _n(n) {}
It A_sub::begin() { return _a.sub_begin(_n); }
It A_sub::end() { return begin() + _a._r; }
const int A_sub::size() const { return _a._r; }

int main() {
    A a(10,5);
    a.cview(4);
    A_sub b = a.view(4);
    for (auto && e:b) {e=1;}
}

The variable names you chose are quite bad. I can't seem to figure out what r and c are as constructor input.

Creating such views only for vectors (of ints) is a bit wasteful. You are basically wrapping an iterator pair, so why not make the code generic so that it woks with iterators of any type of container?

Boost, for example, provides Boost.Range for doing just this.

When you implemented a specialized container, you create specialized iterators that know how to traverse it. The view/range stays the same, it's job is still to wrap a pair of iterators.


Beginner's level hovering effect on anchors After a few trials I've written this code where the border of a link expands:

a {
  position: relative;
  text-decoration: none;
margin:0;
}

a::after{
  content: "";
  position: absolute;
//relative to a
  bottom:0;
  left:0;
  width:0%;
  transition: 0.3s width ease-out;
}
a:hover::after {
  width:100%;
  border: 1px solid black;
}
LINKLINKLINK

Would you suggest any beginner-level improvement of the code?
Or any exercise or variant of it?

There's one bug and one tiny improvement, that I can think of:

In my browser (Chrome on Mac), border line on hover animated on top of text rather than under it. It was, because line bottom: 0 was ignored. Previous comment line broke the code. Removing it fixed my problem. There are no // comments in css, always use /* */ (or use scss).
When doing animation where object suddenly appears with border on hover, there might be small glitch if initial object is visible, because content has to adjust to newly gained border or size (depends on bounding-box). For example if initial width was 1%. For these reasons, I set transparent border to original element (in this case I mean a::after and on :hover I just add border-color.