Brainfuck interpreter in Haskell Okay, so I just started learning Haskell around a week ago and this is my first real program that I worked on all of yesterday with a lot of help from IRC. I know that using indicies and arrays is not very "Haskellish" but I found constantly manipulating lists and traversing them was extremely slow and sometimes the program took over 10 minutes to execute while this version is instant.

Afterwards I found you can do it with some Zipper package in 10 lines of code trivially but I didn't want to use anything too fancy.

I find that when I'm writing Haskell because there is no state I find myself simulating state with function parameters (saving variables and mutating them in a recursive call). I'm pretty sure I took it too far because most of these functions take four parameters and rarely change them but I'm not sure about many alternatives.

I haven't gotten to monads, functors, or applicatives yet, so while I'm sure they could solve this quite elegantly, they're still just magic to me. Unless the explanation is quite simple, I'd prefer if replies didn't mention them. I'm mostly looking for refactorings, style-advice, and better ways of implementing some things.

import qualified Data.Sequence as S
import Data.Char (chr, ord)
import Data.Array

-- Current Index, Indentation Depth, Program Array -> Bracket Index
prevBracketIndex :: Int -> Int -> Array Int Char -> Int
prevBracketIndex i depth cs
    | cs ! i == '[' = if (depth - 1) == 0 then i else prevBracketIndex (i - 1) (depth - 1) cs
    | cs ! i == ']' = prevBracketIndex (i - 1) (depth + 1) cs
    | otherwise = prevBracketIndex (i - 1) depth cs

nextBracketIndex :: Int -> Int -> Array Int Char -> Int
nextBracketIndex i depth cs
    | cs ! i == '[' = nextBracketIndex (i + 1) (depth + 1) cs
    | cs ! i == ']' = if (depth - 1) == 0 then i else nextBracketIndex (i + 1) (depth - 1) cs
    | otherwise = nextBracketIndex (i + 1) depth cs

execCode :: Int -> S.Seq Int -> Int -> Array Int Char -> IO ()
execCode tapePos ts codePos cs
    | codePos == (snd . bounds $ cs) = return ()
    | cmd == '+' = execCode tapePos (S.update tapePos (value + 1) ts) nextPos cs
    | cmd == '-' = execCode tapePos (S.update tapePos (value - 1) ts) nextPos cs
    | cmd == '>' = execCode (tapePos + 1) ts nextPos cs
    | cmd == '<' = execCode (tapePos - 1) ts nextPos cs
    | cmd == '[' && value == 0 = execCode tapePos ts (nextBracketIndex codePos 0 cs + 1) cs
    | cmd == ']' && value /= 0 = execCode tapePos ts (prevBracketIndex codePos 0 cs + 1) cs
    | cmd == '.' = putStr [chr $ S.index ts tapePos] >> execCode tapePos ts nextPos cs
    | cmd == ',' = do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos cs }
    | otherwise = execCode tapePos ts nextPos cs
    where value = S.index ts tapePos
          cmd = cs ! codePos
          nextPos = codePos + 1

tape = S.fromList $ replicate 30000 0

main = do
    file <- readFile "example.bf"
    execCode 0 tape 0 (listArray (0, length file - 1) file)

Use case instead of == and guards everywhere:

prevBracketIndex :: Int -> Int -> Array Int Char -> Int
prevBracketIndex i depth cs = case cs ! i of
    '[' -> if (depth - 1) == 0 then i else prevBracketIndex (i - 1) (depth - 1) cs
    ']' -> prevBracketIndex (i - 1) (depth + 1) cs
    _ -> prevBracketIndex (i - 1) depth cs

Use State and lens to carry state around instead of manual threading.

Use monad-loops to spin the loop instead of manual tail calls.

prevBracketIndex should avoid recursion too by using lists of indices.

Initial depth is always 0 in prevBracketIndex so it should be made local to improve readability. Also, cs is not changed across recursive calls so there is no need to pass it across. Applying both ideas:

prevBracketIndex :: Int -> Array Int Char -> Int
prevBracketIndex i cs = pbi i 0 where
    pbi i depth = case cs ! i of
        '[' -> if (depth - 1) == 0 then i else pbi (i - 1) (depth - 1)
        ']' -> pbi (i - 1) (depth + 1)
        _ -> pbi (i - 1) depth

For execCode we can do the same transformation: cs is invariant across loops, and initial positions are always 0.

Note also that prevBracketIndex can be completely precalculated (replaced by a single array lookup), as cs doesn't change.

Applying everything above but case we get:

import qualified Data.Sequence as S
import Data.Char (chr, ord)
import Data.Array
import Data.List

cachePrev cs = listArray (bounds cs) $ snd $ mapAccumL f [] $ assocs cs where
    f l (i, c) = case c of
        '[' -> (i : l, Nothing)
        ']' -> (tail l, Just $ head l)
        _ -> (l, Nothing)

cacheNext cs = listArray (bounds cs) $ snd $ mapAccumR f [] $ assocs cs where
    f l (i, c) = case c of
        ']' -> (i : l, Nothing)
        '[' -> (tail l, Just $ head l)
        _ -> (l, Just i)

cache arr i = case arr ! i of
    Nothing -> error "oops!"
    Just idx -> idx

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = execCode 0 ts 0 where
    prev = cachePrev cs
    next = cacheNext cs

    execCode tapePos ts codePos
        | codePos == (snd . bounds $ cs) = return ()
        | cmd == '+' = execCode tapePos (S.update tapePos (value + 1) ts) nextPos
        | cmd == '-' = execCode tapePos (S.update tapePos (value - 1) ts) nextPos
        | cmd == '>' = execCode (tapePos + 1) ts nextPos
        | cmd == '<' = execCode (tapePos - 1) ts nextPos
        | cmd == '[' && value == 0 = execCode tapePos ts (cache next codePos + 1)
        | cmd == ']' && value /= 0 = execCode tapePos ts (cache prev codePos + 1)
        | cmd == '.' = putStr [chr $ S.index ts tapePos] >> execCode tapePos ts nextPos
        | cmd == ',' = do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos }
        | otherwise = execCode tapePos ts nextPos
        where
            value = S.index ts tapePos
            cmd = cs ! codePos
            nextPos = codePos + 1

For tests I found csFromString to be a convenient helper:

csFromString file = listArray (0, length file - 1) file

And it helps to write main in a more compact way:

main = readFile "example.bf" >>= execCode' tape . csFromString

Now let's apply the case proposal:

execCode _ _ codePos | codePos == (snd . bounds $ cs) = return ()
execCode tapePos ts codePos = case cs ! codePos of
    '+' -> execCode tapePos (S.update tapePos (value + 1) ts) nextPos
    '-' -> execCode tapePos (S.update tapePos (value - 1) ts) nextPos
    '>' -> execCode (tapePos + 1) ts nextPos
    '<' -> execCode (tapePos - 1) ts nextPos
    '[' -> if value == 0 then execCode tapePos ts (cache next codePos + 1) else execNext
    ']' -> if value /= 0 then execCode tapePos ts (cache prev codePos + 1) else execNext
    '.' -> putStr [chr $ S.index ts tapePos] >> execNext
    ',' -> do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos }
    _ -> execNext
    where
        value = S.index ts tapePos
        nextPos = codePos + 1
        execNext = execCode tapePos ts nextPos

Now cmd is not needed anymore, and [ and ] required some additional plumbing.

Now let's remove duplication in 3 symmetrical pairs of instructions - updatePos, updateVal and branch:

execCode _ _ codePos | codePos == (snd . bounds $ cs) = return ()
execCode tapePos ts codePos = case cs ! codePos of
    '+' -> updatePos succ
    '-' -> updatePos pred
    '>' -> updateVal succ
    '<' -> updateVal pred
    '[' -> branch (== 0) next
    ']' -> branch (/= 0) prev
    '.' -> putStr [chr $ S.index ts tapePos] >> execNext
    ',' -> do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos }
    _ -> execNext
    where
        value = S.index ts tapePos
        nextPos = codePos + 1
        execNext = execCode tapePos ts nextPos

        updatePos f = execCode tapePos (S.update tapePos (f value) ts) nextPos
        updateVal f = execCode (f tapePos) ts nextPos
        branch cond dir = if cond value then execCode tapePos ts (cache dir codePos + 1) else execNext

Now it's time to remove duplication between cachePrev and cacheNext:

mkCache cs mapAccumX bracketPush bracketPop = listArray (bounds cs) $ snd $ mapAccumX f [] $ assocs cs where
    f l (i, c)
        | c == bracketPush = (i : l, Nothing)
        | c == bracketPop = (tail l, Just $ head l)
        | otherwise = (l, Nothing)

cache arr i = case arr ! i of
    Nothing -> error "oops!"
    Just idx -> idx

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = execCode 0 ts 0 where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode _ _ codePos | codePos == (snd . bounds $ cs) = return ()
    execCode tapePos ts codePos = case cs ! codePos of
        '+' -> updatePos succ
        '-' -> updatePos pred
        '>' -> updateVal succ
        '<' -> updateVal pred
        '[' -> branch (== 0) next
        ']' -> branch (/= 0) prev
        '.' -> putStr [chr $ S.index ts tapePos] >> execNext
        ',' -> do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos }
        _ -> execNext
        where
            value = S.index ts tapePos
            nextPos = codePos + 1
            execNext = execCode tapePos ts nextPos

            updatePos f = execCode tapePos (S.update tapePos (f value) ts) nextPos
            updateVal f = execCode (f tapePos) ts nextPos
            branch cond dir = if cond value then execCode tapePos ts (cache dir codePos + 1) else execNext

Here is complete final source:

import qualified Data.Sequence as S
import Data.Char (chr, ord)
import Data.Array
import Data.List

mkCache cs mapAccumX bracketPush bracketPop = listArray (bounds cs) $ snd $ mapAccumX f [] $ assocs cs where
    f l (i, c)
        | c == bracketPush = (i : l, Nothing)
        | c == bracketPop = (tail l, Just $ head l)
        | otherwise = (l, Nothing)

cache arr i = case arr ! i of
    Nothing -> error "oops!"
    Just idx -> idx

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = execCode 0 ts 0 where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode _ _ codePos | codePos == (snd . bounds $ cs) = return ()
    execCode tapePos ts codePos = case cs ! codePos of
        '+' -> updatePos succ
        '-' -> updatePos pred
        '>' -> updateVal succ
        '<' -> updateVal pred
        '[' -> branch (== 0) next
        ']' -> branch (/= 0) prev
        '.' -> putStr [chr $ S.index ts tapePos] >> execNext
        ',' -> do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode tapePos newTape nextPos }
        _ -> execNext
        where
            value = S.index ts tapePos
            nextPos = codePos + 1
            execNext = execCode tapePos ts nextPos

            updatePos f = execCode tapePos (S.update tapePos (f value) ts) nextPos
            updateVal f = execCode (f tapePos) ts nextPos
            branch cond dir = if cond value then execCode tapePos ts (cache dir codePos + 1) else execNext

tape = S.fromList $ replicate 30000 0

csFromString file = listArray (0, length file - 1) file

main = readFile "example.bf" >>= execCode' tape . csFromString

Note that my suggestions about monad-loops, lens and State are still to be applied.

The first step is to declare a datatype for our future state.

data M = M
    { _tapePos :: Int
    , _tape :: S.Seq Int
    , _codePos :: Int
    }

and make inner execCode accept a single parameter.

    execCode :: M -> IO ()
    execCode (M _ _ codePos) | codePos == (snd . bounds $ cs) = return ()
    execCode (M tapePos ts codePos) = case cs ! codePos of
        '+' -> updatePos succ
        '-' -> updatePos pred
        '>' -> updateVal succ
        '<' -> updateVal pred
        '[' -> branch (== 0) next
        ']' -> branch (/= 0) prev
        '.' -> putStr [chr $ S.index ts tapePos] >> execNext
        ',' -> do { c <- getChar; let newTape = S.update tapePos (ord c) ts in execCode (M tapePos newTape nextPos) }
        _ -> execNext
        where
            value = S.index ts tapePos
            nextPos = codePos + 1
            execNext = execCode (M tapePos ts nextPos)

            updatePos f = execCode (M tapePos (S.update tapePos (f value) ts) nextPos)
            updateVal f = execCode (M (f tapePos) ts nextPos)
            branch cond dir = if cond value then execCode (M tapePos ts (cache dir codePos + 1)) else execNext

M stands for machine state :) and underscores are signals to Data.Lens.TH template Haskell code we will start using a bit later.

We will need a state monad transformer ran on top of IO monad. Let's declare the type of our monad stack:

type ExecT a = StateT M IO a

Our future execCode'' will be of type ExecT () instead of current M -> IO (). To execute it and discard the state (as we do now) we'll use evalStateT:

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode'' (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode'' :: ExecT ()
    execCode'' = do
        s <- get
        lift (execCode s)

    execCode :: M -> IO ()
    execCode = ... -- unchanged

Now execCode'' is just a wrapper around our old inner execCode. And we fix execCode so it can be called directly. Note that we should do the following:

lift all IO
Replace recursive calls execCode (M ...) with put (M ...) >> execCode
move where statements around so M tapePos ts codePos is in scope
replace old termination guard with when

Here is the result:

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode :: ExecT ()
    execCode = do
        M tapePos ts codePos <- get
        let value = S.index ts tapePos
        let nextPos = codePos + 1
        let execNext = put (M tapePos ts nextPos) >> execCode
        let updatePos f = put (M tapePos (S.update tapePos (f value) ts) nextPos) >> execCode
        let updateVal f = put (M (f tapePos) ts nextPos) >> execCode
        let branch cond dir = if cond value then put (M tapePos ts (cache dir codePos + 1)) >> execCode else execNext

        when (codePos /= (snd . bounds $ cs)) $ case cs ! codePos of
            '+' -> updatePos succ
            '-' -> updatePos pred
            '>' -> updateVal succ
            '<' -> updateVal pred
            '[' -> branch (== 0) next
            ']' -> branch (/= 0) prev
            '.' -> lift (putStr [chr $ S.index ts tapePos]) >> execNext
            ',' -> do { c <- lift getChar; let newTape = S.update tapePos (ord c) ts in put (M tapePos newTape nextPos) >> execCode }
            _ -> execNext

Now note that all branches of case end in >> execCode. So it can be moved around to form a nice while control structure:

execCode :: ExecT ()
execCode = do
    M tapePos ts codePos <- get
    let value = S.index ts tapePos
    let nextPos = codePos + 1
    let execNext = put (M tapePos ts nextPos)
    let updatePos f = put (M tapePos (S.update tapePos (f value) ts) nextPos)
    let updateVal f = put (M (f tapePos) ts nextPos)
    let branch cond dir = if cond value then put (M tapePos ts (cache dir codePos + 1)) else execNext

    when (codePos /= (snd . bounds $ cs)) $ do
        case cs ! codePos of
            '+' -> updatePos succ
            '-' -> updatePos pred
            '>' -> updateVal succ
            '<' -> updateVal pred
            '[' -> branch (== 0) next
            ']' -> branch (/= 0) prev
            '.' -> lift (putStr [chr $ S.index ts tapePos]) >> execNext
            ',' -> do { c <- lift getChar; let newTape = S.update tapePos (ord c) ts in put (M tapePos newTape nextPos) }
            _ -> execNext
        execCode

Now it's finally time for lens to shine.

{-# LANGUAGE TemplateHaskell  #-}
import Control.Lens.TH
import Control.Lens
import Control.Applicative

...

data M = M
    { _mTapePos :: Int
    , _mTape :: S.Seq Int
    , _mCodePos :: Int
    }

$(makeLenses ''M)

type ExecT a = StateT M IO a

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode :: ExecT ()
    execCode = do
        M tapePos ts codePos <- get
        let value = S.index ts tapePos
        let nextPos = codePos + 1
        let execNext = mCodePos += 1
        let updatePos f = mTape %= S.update tapePos (f value) >> execNext
        let updateVal f = mTapePos %= f >> execNext
        let branch cond dir = if cond value then mCodePos %= succ . cache dir else execNext

        when (codePos /= (snd . bounds $ cs)) $ do
            case cs ! codePos of
                '+' -> updatePos succ
                '-' -> updatePos pred
                '>' -> updateVal succ
                '<' -> updateVal pred
                '[' -> branch (== 0) next
                ']' -> branch (/= 0) prev
                '.' -> lift (putStr [chr $ S.index ts tapePos]) >> execNext
                ',' -> do { c <- lift getChar; mTape %= S.update tapePos (ord c) >> execNext }
                _ -> execNext
            execCode

I did the following:

Added imports and TemplateHaskell pragma
Added a template Haskell call to generate definitions for mTapePos from _mTapePos
replaced all put calls with one or many lens-based state modifiers, joined by >>. E.g. if 2 fields of M are modified I chain 2 modifiers. If just 1 - no need to chain. Basically it's just >> execNext instead of nextPos.
removed unused nextPos

Now it turns out that branch has its own hidden execNext (note succ . in the code above):

let branch cond dir = if cond value then mCodePos %= cache dir >> execNext else execNext

So we can transform it to when easily:

let branch cond dir = when (cond value) (mCodePos %= cache dir) >> execNext

And now it turns out that >> execNext is everywhere. We can move it after case and  inline:

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode :: ExecT ()
    execCode = do
        M tapePos ts codePos <- get
        let value = S.index ts tapePos
        let updatePos f = mTape %= S.update tapePos (f value)
        let updateVal f = mTapePos %= f
        let branch cond dir = when (cond value) (mCodePos %= cache dir)

        when (codePos /= (snd . bounds $ cs)) $ do
            case cs ! codePos of
                '+' -> updatePos succ
                '-' -> updatePos pred
                '>' -> updateVal succ
                '<' -> updateVal pred
                '[' -> branch (== 0) next
                ']' -> branch (/= 0) prev
                '.' -> lift (putStr [chr $ S.index ts tapePos])
                ',' -> do { c <- lift getChar; mTape %= S.update tapePos (ord c) }
                _ -> return ()
            mCodePos += 1
            execCode

After some more cleanup we get:

{-# LANGUAGE TemplateHaskell  #-}
import Control.Lens.TH
import Control.Lens
import Control.Applicative

import qualified Data.Sequence as S
import Data.Char (chr, ord)
import Data.Array
import Data.List
import Control.Monad.State

mkCache cs mapAccumX bracketPush bracketPop = listArray (bounds cs) $ snd $ mapAccumX f [] $ assocs cs where
    f l (i, c)
        | c == bracketPush = (i : l, Nothing)
        | c == bracketPop = (tail l, Just $ head l)
        | otherwise = (l, Nothing)

cache arr i = case arr ! i of
    Nothing -> error "oops!"
    Just idx -> idx

data M = M
    { _mTapePos :: Int
    , _mTape :: S.Seq Int
    , _mCodePos :: Int
    }

$(makeLenses ''M)

type ExecT a = StateT M IO a

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode :: ExecT ()
    execCode = do
        M tapePos ts codePos <- get
        let value = S.index ts tapePos
        let tapeAtPos = mTape . ix tapePos
        let branch cond dir = when (cond value) (mCodePos %= cache dir)

        when (codePos /= (snd . bounds $ cs)) $ do
            case cs ! codePos of
                '+' -> tapeAtPos += 1
                '-' -> tapeAtPos -= 1
                '>' -> mTapePos += 1
                '<' -> mTapePos -= 1
                '[' -> branch (== 0) next
                ']' -> branch (/= 0) prev
                '.' -> lift (putChar $ chr value)
                ',' -> do { c <- lift getChar; mTape %= S.update tapePos (ord c) }
                _ -> return ()
            mCodePos += 1
            execCode

tape = S.fromList $ replicate 30000 0

csFromString file = listArray (0, length file - 1) file

main = readFile "example.bf" >>= execCode' tape . csFromString

One more iteration of tuning for execCode' to make code more uniform which is good for readability and maintenance:

tapeAtPos is made a self-contained compound lens, without reliance on tapePos and
tapeAtPos is moved out of execState' body to global scope, and renamed to mTapeAtPos uniformly with other lenses
value is renamed tapeAtPos as it corresponds to mTapeAtPos
mTapeAtPos is used uniformly for both getting and updating the value everywhere including the getChar branch
unsafeUse helper is used to get tapeAtPos uniformly with codePos. It is called unsafe because tapeAtPos may fail if mTapePos is out of range!
lens API is used to read code uniformly with reading tape
extra parenthesis/$ are removed from when condition
M ... <- get is removed as it is not used anymore

The code:

mTapeAtPos f m = (mTape . ix (m ^. mTapePos)) f m

unsafeUse traversal = (^?! traversal) <$> get

execCode' :: S.Seq Int -> Array Int Char -> IO ()
execCode' ts cs = evalStateT execCode (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    execCode :: ExecT ()
    execCode = do
        tapeAtPos <- unsafeUse mTapeAtPos
        codePos <- use mCodePos
        let branch cond dir = when (cond tapeAtPos) (mCodePos %= cache dir)
        when (codePos /= snd (bounds cs)) $ do
            case cs ^?! ix codePos of
                '+' -> mTapeAtPos += 1
                '-' -> mTapeAtPos -= 1
                '>' -> mTapePos += 1
                '<' -> mTapePos -= 1
                '[' -> branch (== 0) next
                ']' -> branch (/= 0) prev
                '.' -> lift (putChar $ chr tapeAtPos)
                ',' -> do { c <- lift getChar; mTapeAtPos .= ord c }
                _ -> return ()
            mCodePos += 1
            execCode

It can be made even more uniform:

at this point tapePos is used only in branch it can be moved into branch
branch then can be moved outside of execCode as it doesn't a closure any more
it seems inner execCode is better named loop, and ExecT is LoopStateT

The code:

branch cond dir = do
    tapeAtPos <- unsafeUse mTapeAtPos
    when (cond tapeAtPos) (mCodePos %= cache dir)

execCode :: S.Seq Int -> Array Int Char -> IO ()
execCode ts cs = evalStateT loop (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    loop :: LoopStateT ()
    loop = do
        codePos <- use mCodePos
        when (codePos /= snd (bounds cs)) $ do
            case cs ^?! ix codePos of
                '+' -> mTapeAtPos += 1
                '-' -> mTapeAtPos -= 1
                '>' -> mTapePos += 1
                '<' -> mTapePos -= 1
                '[' -> branch (== 0) next
                ']' -> branch (/= 0) prev
                '.' -> unsafeUse mTapePos >>= lift . putChar . chr
                ',' -> lift (ord <$> getChar) >>= (mTapeAtPos .=)
                _ -> return ()
            mCodePos += 1
            loop

At this point it seems branch and ./, symmetry aren't worth it, so I inlined branch, reverted getChar branch to use do-notation and moved tapeAtPos <- to the top of loop:

    loop :: LoopStateT ()
    loop = do
        tapeAtPos <- unsafeUse mTapeAtPos
        codePos <- use mCodePos
        when (codePos /= snd (bounds cs)) $ do
            case cs ^?! ix codePos of
                '+' -> mTapeAtPos += 1
                '-' -> mTapeAtPos -= 1
                '>' -> mTapePos += 1
                '<' -> mTapePos -= 1
                '[' -> when (tapeAtPos == 0) (mCodePos %= cache next)
                ']' -> when (tapeAtPos /= 0) (mCodePos %= cache prev)
                '.' -> lift $ putChar (chr tapeAtPos)
                ',' -> do { c <- lift getChar; mTapeAtPos .= ord c }
                _ -> return ()
            mCodePos += 1
            loop

Another improvement is better diagnostics of match failures in cache:

cache arr i = case arr ! i of
    Nothing -> error $ "No matching bracket at offset " ++ show i
    Just idx -> idx

So we get:

{-# LANGUAGE TemplateHaskell  #-}
import Control.Lens.TH
import Control.Lens
import Control.Applicative

import qualified Data.Sequence as S
import Data.Char (chr, ord)
import Data.Array
import Data.List (mapAccumR, mapAccumL)
import Control.Monad.State (lift, get, evalStateT, when, StateT(..))

mkCache cs mapAccumX bracketPush bracketPop = listArray (bounds cs) $ snd $ mapAccumX f [] $ assocs cs where
    f l (i, c)
        | c == bracketPush = (i : l, Nothing)
        | c == bracketPop = (tail l, Just $ head l)
        | otherwise = (l, Nothing)

cache arr i = case arr ! i of
    Nothing -> error $ "No matching bracket at offset " ++ show i
    Just idx -> idx

data M = M
    { _mTapePos :: Int
    , _mTape :: S.Seq Int
    , _mCodePos :: Int
    }

$(makeLenses ''M)

type LoopStateT a = StateT M IO a

mTapeAtPos f m = (mTape . ix (m ^. mTapePos)) f m

unsafeUse traversal = (^?! traversal) <$> get

execCode :: S.Seq Int -> Array Int Char -> IO ()
execCode ts cs = evalStateT loop (M 0 ts 0) where
    prev = mkCache cs mapAccumL '[' ']'
    next = mkCache cs mapAccumR ']' '['

    loop :: LoopStateT ()
    loop = do
        tapeAtPos <- unsafeUse mTapeAtPos
        codePos <- use mCodePos
        when (codePos /= snd (bounds cs)) $ do
            case cs ^?! ix codePos of
                '+' -> mTapeAtPos += 1
                '-' -> mTapeAtPos -= 1
                '>' -> mTapePos += 1
                '<' -> mTapePos -= 1
                '[' -> when (tapeAtPos == 0) (mCodePos %= cache next)
                ']' -> when (tapeAtPos /= 0) (mCodePos %= cache prev)
                '.' -> lift $ putChar $ chr tapeAtPos
                ',' -> do { c <- lift getChar; mTapeAtPos .= ord c }
                _ -> return ()
            mCodePos += 1
            loop

tape = S.fromList $ replicate 30000 0

csFromString file = listArray (0, length file - 1) file

main = readFile "example.bf" >>= execCode tape . csFromString


Bread formula object I designed this object for a "bread formula". They use a concept known as baker's percentages, but you don't need to understand the math to help with the object design.

I designed it as an object (of type Formula) that contains objects (of type Ingredient).

Both classes have nullable properties. C# demarcates nullable types using a ? after the type, like so:

public decimal? Weight { get; set; }

The reason I declare them as nullable types is because I don't know their values (when I create an instance of them) until I calculate them later, as which point I update the null value with a calculated value.

The calculations are just basic methods, like CalculateSum() or CalculateWeight().

Is there a better way to design this object? It works, but I feel like it's the wrong way to do things. I feel like I'm trying to force-fit inline/declarative style programming into an OOP design, but I don't know how else to do it, being so new to OOP.

My thinking is that this object has this property. I won't know its value when I create an instance of it, so I must declare it as nullable. Once I calculate its value using a method that's also internal to the class, I assign/set its value. Then it will no longer be null.

The calculation methods are basic calculations:

public type Method(input,input,...){calc; return calc;}.

 class Ingredient
    {
        public string Name { get; set; }
        public decimal Percentage { get; set; }
        public bool IsFlour { get; set; }
        public decimal? Weight { get; set; }

        public Ingredient(string _Name, decimal _Percent, bool _IsFlour, decimal? _Weight)
        {
            this.Name = _Name;
            this.Percentage = _Percent;
            this.IsFlour = _IsFlour;
            this.Weight = _Weight;
        }
    }

    class Formula
    {
        public decimal Weight_Total_Dough { get; set; }
        public ArrayList Ingredients { get; set; }
        public int NumberOfIngredients { get; set; }
        public decimal? SumPercentages { get; set; }
        public decimal? SumWeights { get; set; }
        public decimal? SumWeightTotalFlour { get; set; }

        public Formula(decimal _Weight_Total_Dough, ArrayList _Ingredients, int _NumberOfIngredients, decimal? _SumPercentages, decimal? _SumWeights, decimal? _SumWeightTotalFlour)
        {
            this.Weight_Total_Dough = _Weight_Total_Dough;
            this.Ingredients = _Ingredients;
            this.NumberOfIngredients = _NumberOfIngredients;
            this.SumPercentages = _SumPercentages;
            this.SumWeights = _SumWeights;
        }
    }

Here's the code for calculating Formula.SumPercentages, which was declared as nullable, but is now assigned a value:

public void SumPercentages(Formula f)
        {
            //Interate through Formula object, which contains an ArrayList of INGREDIENT objects.
            //Extract the precentages for each ingredient.
            //Add them up.
            //Return the summation as SumOfPercentages.

            decimal SumOfPercentages;
            SumOfPercentages = 0;
            foreach (object x in f.Ingredients) //for each INGREDIENT object in the FORMULA object
            {
                Ingredient ig = (Ingredient)x; //get the INGREDIENT.Percentage
                SumOfPercentages += ig.Percentage; //add it to the other INGREDIENT.Percentages
            }

            f.SumPercentages = SumOfPercentages; //SumOfPercentages was null before, but now we assign a value (SumOfPercentages) to it.

Everyone has had good ideas, but since you're asking about OOP, I'd suggest refactoring it altogether.

void Main()
{
    var formula = new Formula(new[] {
        new Ingredient("Flour, AP", 100m, true),
        new Ingredient("Water, Warm", 70.15m, false),
        new Ingredient("Salt", 3.04m, false),
        new Ingredient("Yeast", .24m, false),
        new Ingredient("Pate fermentee", 2.53m, false)
    });
    var weights = formula.GetWeights(10000m);
}

class Ingredient
{
    public string Name { get; private set; }
    public decimal Percentage { get; private set; }
    public bool IsFlour { get; private set; }

    public Ingredient(string _Name, decimal _Percent, bool _IsFlour)
    {
        this.Name = _Name;
        this.Percentage = _Percent;
        this.IsFlour = _IsFlour;
    }
}

class Formula
{
    private IEnumerable<Ingredient> Ingredients { get; set; }
    public decimal SumPercentages { get; private set; }

    public Formula(IEnumerable<Ingredient> _Ingredients)
    {
        this.Ingredients = _Ingredients;
        this.SumPercentages = _Ingredients.Sum(x => x.Percentage);
    }

    public Dictionary<Ingredient, decimal> GetWeights(decimal weightTotalDough)
    {
        return Ingredients.ToDictionary(k => k, v => (v.Percentage / this.SumPercentages) * weightTotalDough);
    }
}

By doing it this way, you separate the formula (which is basically just Ingredient/Percentage pairs) from each application of it.  You can use the same Formula object to calculate the weights for 10000g of dough or 50000g of dough - simply call GetWeights() with a different value.

Notice how many fewer properties you need on each class, and the question of "late setting" of some of them is entirely irrelevant.  Effectively, weight is not an intrinsic property of an ingredient, because it varies based on the total weight, so Weight shouldn't be a property of Ingredient.


Update data in partial view I have partial view which update data on server.
Partial view contains list of configurations (can be check box or text box depends on type).
This code is working, and I would like to know how to implement this better.

Main question is related to "@Html.HiddenFor".
Is this correct way to implement update?
Without "@Html.HiddenFor" my model data is not ok.

Action:

  public async Task<IActionResult> UpdateConfiguration(List<ConfigurationModel> configurations)
        {
            logger.LogInformation("Update configurations");

            using (var client = new HttpClient())
            {
                SetToken(client, this.HttpContext);
                var configUrl = Helper.GetUrlManagerApi(env, "Configuration");
                var response = await client.PutAsync(configUrl, configurations.ToJson());
                HandleResponseError(response, logger);
                return RedirectToAction("Index", "Home");
            }
        }

Partial view:

@using Microsoft.EntityFrameworkCore.Metadata.Internal
@using Bluebox.Common.Models
@using Bluebox.Common.Helper
@using Microsoft.AspNetCore.Mvc.Localization
@using SettingsApp.Models;

@inject IViewLocalizer Localizer
@model List<ConfigurationModel>

@using (Html.BeginForm("UpdateConfiguration", "ManagerSettings"))
{

                @Localizer["Property"]

                    @Localizer["Edit"]

        @for (var i = 0; i < Model.Count(); i++)
        {

                    @Localizer[Model[i].LocalizeId.ToString()]
                    @Html.HiddenFor(model => model[i].Id, @Model[i].Id)
                    @Html.HiddenFor(model => model[i].LocalizeId, @Model[i].LocalizeId)
                    @Html.HiddenFor(model => model[i].Key, @Model[i].Key)
                    @Html.HiddenFor(model => model[i].Type, @Model[i].Type)

                    @if (Model[i].Type == PropertyType.Boolean)
                    {
                        @Html.CheckBoxFor(model => model[i].ValueBool, Model[i].Value == "true")
                    }
                    else if (Model[i].Type == PropertyType.Number)
                    {
                        @Html.TextBoxFor(model => model[i].ValueNum, new { style = "width:50px;", @type = "number" })

                    }
                    else if (Model[i].Type == PropertyType.Text)
                    {
                        @Html.TextBoxFor(model => model[i].Value, new { style = "width:250px;" })

                    }

        }

        <input type="submit" class="button" value='@Localizer["Update"]' />

}

Result:

The most problem in "@Html.HiddenFor" is that users may change this value by means of a browser developer window or CSRF, for avoiding last one you can use

@Html.AntiForgeryToken()

You can protect your application from adverse changes if you will reread all hidden properties such as LocalizeId, Key and Type from your data source before applying user's changes.


Secure password hashing implementation I'm implementing a password hashing method for a website. The code below is part of the User class.

Any pointers on what I could do better?

private string GenerateSalt(int SaltLength) {
    using (var rng = new RNGCryptoServiceProvider()) {
        var salt_bytes = new byte[SaltLength];
        rng.GetNonZeroBytes(salt_bytes);
        return Convert.ToBase64String(salt_bytes);
    }
}

private string GeneratePasswordHash(string Password) {
    //create an hmac hash of the password using the pepper value as the key
    using (var hmacsha = new HMACSHA512(ConvertStringToBytes(ConfigurationManager.AppSettings["PasswordHashPepper"]))) {
        byte[] initial_hash = hmacsha.ComputeHash(ConvertStringToBytes(Password));

        //generate a key value using pbkdf2 that will serve as the password hash
        using (var pbkdf2 = new Rfc2898DeriveBytes(initial_hash, ConvertStringToBytes(this.Salt), int.Parse(ConfigurationManager.AppSettings["PasswordHashWorkFactor"]))) {
            return ConvertBytesToString(pbkdf2.GetBytes(128));
        }
    }
}

private byte[] ConvertStringToBytes(string conversionString) {
    byte[] bytes = new byte[conversionString.Length * sizeof(char)];
    System.Buffer.BlockCopy(conversionString.ToCharArray(), 0, bytes, 0, bytes.Length);
    return bytes;
}

private string ConvertBytesToString(byte[] conversionBytes) {
    char[] chars = new char[conversionBytes.Length / sizeof(char)];
    System.Buffer.BlockCopy(conversionBytes, 0, chars, 0, conversionBytes.Length);
    return new string(chars);
}

public void SetNewPassword(string NewPassword) {
    this.Salt = GenerateSalt(64);
    this.PasswordHash = GeneratePasswordHash(NewPassword);
}

public bool ValidatePassword(string AttemptedPassword) {
    return (GeneratePasswordHash(AttemptedPassword) == this.PasswordHash);
}

I recommend creating a static class named something like Hash, Crypto or Password depending on your project/context and making these methods public static, instead of making them private methods for the user class.
WARNING!
Unless your password encryption management code is sophisticated enough to manage password scheme versions and handle multi-version-migration complications PasswordHashWorkFactor should not be editable (i.e. by editing the relevant key in the config file). In that case, editing it would cause migration efforts or MORE IMPORTANTLY the attacker with R/W access to the config file can crash the system easily by adding an unnoticeable digit to PasswordHashWorkFactor  (add a 0 to 100000). It would take significant time of the admin/dev to figure out the problem in a sophisticated config file while the users cannot log in. You may have it hardcoded as a const at the static class and change it in every major release at the code level. Again this is a matter of convenience vs security trade-off.
Here is my feedback in more detail as an implementation. You may want to check it out.


Multiple anonymous IIFEs inside outer function I have a scrollHandler function which contains all of my scroll event-listener logic.
I've used multiple IIFE's inside to separate the logic:
import skills from './json/skills.json';

let capToggle = false;
let triggerCalculateHeight = false;
let scrollEnd = false;

function headingFadeIn(heading) {
    if (!heading) return;
    const parent = heading.parentNode.parentNode;
    const triggerHeight = parent.offsetTop - window.innerHeight / 1.5;

    if (window.scrollY > triggerHeight) {
        heading.style.opacity = '1';
    } else heading.style.opacity = '0';
}

export default function scrollHandler() {
    // Headings
    (() => {
        headingFadeIn(document.getElementById('portfolio-heading'));
        headingFadeIn(document.getElementById('skills-heading'));
        headingFadeIn(document.getElementById('bio-heading'));
        headingFadeIn(document.getElementById('contact-heading'));
    })();
    // Skills
    (() => {
        const bars = document.querySelectorAll('.skill-bar');
        if (!bars) return;

        bars.forEach((i, index) => {
            const bottom = i.getBoundingClientRect().bottom;

            if (window.scrollY > bottom && !i.dataset.heightCalculated) {
                i.style.transform = `scale3d(1, ${skills[index].height / 10}, 1)`;
                i.setAttribute('data-height-calculated', true);

                capToggle = true;
                triggerCalculateHeight = true;
            }
        });
    })();
    // Reviews
    (() => {
        const el = document.getElementById('reviews-inner');
        if (!el) return;

        const mainBackground = document.getElementById('ul-bg');
        const backgrounds = document.querySelectorAll('.blockquote-bg');
        const texts = document.querySelectorAll('.reviews-toggle');
        const bound = el.getBoundingClientRect().top * 10;

        if (window.scrollY > bound) {
            mainBackground.style.opacity = '1';
            mainBackground.style.transform = 'scale(1) rotate(45deg)';
            backgrounds.forEach(i => (i.style.opacity = '1'));
            backgrounds[0].style.transform = 'translate(-4%, -9%) skew(45deg)';
            backgrounds[1].style.transform = 'translate(-6%, -2%) skew(45deg)';
            backgrounds[2].style.transform = 'translate(-5%, 1%) skew(45deg)';
            texts.forEach(i => (i.style.opacity = '1'));
        }
    })();
    // Bio
    (() => {
        const container = document.getElementById('bio-container');
        const tabSelected = document.getElementById('tab-selected');
        const tabBackground = document.getElementById('tab-background');
        const height = document.getElementById('bio').offsetTop;
        if (!tabBackground || !container || !tabSelected || !height) return;

        if (window.scrollY > height / 1.1) {
            // Background tab styling
            tabBackground.style.left = `${tabSelected.offsetLeft}px`;
            tabBackground.style.maxWidth = `${tabSelected.offsetWidth + 20}px`;

            // Container transition
            container.style.opacity = '1';
            container.style.transform = 'skew(-25deg)';
            container.childNodes[0].style.transform = 'skew(25deg)';
        }
    })();
    // Contact
    (() => {
        const groups = document.querySelectorAll('.form-group');
        if (!groups) return;

        groups.forEach((i, index) => {
            i.childNodes[1].blur();

            if (window.scrollY > (i.getBoundingClientRect().top * 10) / 1.5) {
                i.style.opacity = '1';

                if (index >= groups.length - 1) scrollEnd = true;
            }
        });
    })();

    return { capToggle, triggerCalculateHeight, scrollEnd };
}

Is this considered good practice? Are there any pitfalls that could come from this?

Modules You're already using export syntax, so you're currently in a module. Rather than defining multiple IIFEs, you might consider using separate sub-modules:
// fadeInHeadings.js

// Renaming from headingFadeIn to fadeInHeading for readability
function fadeInHeading(heading) {
    if (!heading) return;
    const parent = heading.parentNode.parentNode;
    const triggerHeight = parent.offsetTop - window.innerHeight / 1.5;

    if (window.scrollY > triggerHeight) {
        heading.style.opacity = '1';
    } else heading.style.opacity = '0';
}

export const fadeInHeadings = () => {
  const headingIds = [
    'portfolio-heading',
    'skills-heading',
    'bio-heading',
    'contact-heading',
  ];
  for (const id of headingIds) {
    fadeInHeading(document.getElementById(id));
  }
};

// scrollHandler.js
import { fadeInHeadings } from './fadeInHeadings';
import { transformBars } from './transformBars';
import { transformReviews } from './transformReviews';

export default function scrollHandler() {
  fadeInHeadings();

  const { capToggled, newTriggerCalculateHeight } = transformBars();
  capToggle ||= capToggled;
  triggerCalculateHeight ||= newTriggerCalculateHeight;

  transformReviews();

  // ...

This way, when, for example, a review isn't scaling properly, you can navigate directly to transformReviews to debug and fix the problem, rather than wander around in a significantly larger file looking for where reviews are referenced.
Variable names and iteration Your two forEaches are a bit confusing:
bars.forEach((i, index) => {

When i is used as a variable name, it's almost always used to reference the index of a collection. To have i not only not refer to an index, but to also have an index variable, is confusing. Instead, consider:
bars.forEach((bar, index) => {

Same sort of thing in Contact:
const groups = document.querySelectorAll('.form-group');
if (!groups) return;
groups.forEach((i, index) => {

i should probably be renamed to formGroup or something similar. Also, querySelectorAll will return an array-like NodeList. Even if it's empty, it'll never be falsey, so you can remove the if (!groups) return; line.
children You have
formGroup.childNodes[1].blur();

childNodes returns a collection that includes text nodes. Text nodes can be empty, and including them in a collection can make logic more tedious than it needs to be. Since you want to select an element with that, better to use .children (which only returns element children nodes), so you can do
formGroup.children[1].blur();
// or
formGroup.children[0].blur();
// or, be more specific and use a CSS selector
formGroup.querySelector(<some-selector>).blur();

Persistent variables? The persistent module variables seem a bit odd. If they really need to be in persistent state, it might make sense to expose a function that returns their current values. If they aren't meant to be persistent, initialize them to false inside scrollHandler instead - or, inside the sub-module which reassigns its value, which is returned to scrollHandler and then returned by scrollHandler. For example, it'd be great if you could do this:
// showFormGroups.js
export const showFormGroups = () => {
    const groups = document.querySelectorAll('.form-group');
    let scrollEnd = false;
    groups.forEach((formGroup, index) => {
        formGroup.children[1].blur();
        if (window.scrollY > (formGroup.getBoundingClientRect().top * 10) / 1.5) {
            formGroup.style.opacity = '1';
            if (index >= groups.length - 1) {
                scrollEnd = true;
            }
        }
    });
    return scrollEnd;
};

// scrollHandler.js
export default function scrollHandler() {
    // ...
    const scrollEnd = showFormGroups();
    // ...
    return { capToggle, triggerCalculateHeight, scrollEnd };

Styles into CSS You have
if (window.scrollY > bound) {
    mainBackground.style.opacity = '1';
    mainBackground.style.transform = 'scale(1) rotate(45deg)';
    backgrounds.forEach(i => (i.style.opacity = '1'));
    backgrounds[0].style.transform = 'translate(-4%, -9%) skew(45deg)';
    backgrounds[1].style.transform = 'translate(-6%, -2%) skew(45deg)';
    backgrounds[2].style.transform = 'translate(-5%, 1%) skew(45deg)';
    texts.forEach(i => (i.style.opacity = '1'));
}

To apply this logic, I think it'd make more sense to put the CSS rules in your CSS file, and then just toggle a class. Something like:
// SCSS for brevity
body.showReviews {
  #ul-bg {
    opacity: 1;
    transform: scale(1) rotate(45deg);
  }
  .blockquote-bg {
    opacity: 1;
  }
  .blockquote-bg:nth-of-type(1) {
    transform: translate(-4%, -9%) skew(45deg);
  }
  .blockquote-bg:nth-of-type(2) {
    transform: translate(-6%, -2%) skew(45deg);
  }
  .blockquote-bg:nth-of-type(3) {
    transform: translate(-5%, 1%) skew(45deg);
  }
  .reviews-toggle {
    opacity: 1;
  }
}

// transformReviews.js
export const transformReviews = () => {
  const el = document.getElementById('reviews-inner');
  if (!el) return;
  const bound = el.getBoundingClientRect().top * 10;
  document.body.classList.toggle('showReviews', window.scrollY > bound);
};

Framework On a larger scale, all of this careful element selecting, iterating, examining, and manual manipulation of the DOM seems a bit tedious. For a professional project that isn't tiny, I would prefer to look into a framework like React to tie element state directly to their respective DOM elements.


Find spiral 2D matrix (List of Lists) of ints in a clockwise spiral I got this question during my interview with Microsoft.

The question called 2D Snake. I'm wondering if my solution 1 passes the test, but if my solution 1 different from solution 2.

  Let's have some fun with 2D Matrices! Write a method find_spiral to
  traverse a 2D matrix (List of Lists) of ints in a clockwise spiral
  order and append the elements to an output List of Integers.

  Example:

  Input Matrix:

[1, 2, 3]
[4, 5, 6]
[7, 8, 9]

  Output List: [1, 2, 3, 6, 9, 8, 7, 4, 5]

Solution 1

def find_spiral(matrix):

    output = []

    while matrix:
        row = matrix.pop(0)
        output.extend(row)
        matrix = zip(*matrix)[::-1]

    return output

Solution 2

def find_spiral(matrix):
    spiral_list = []
    if matrix == None or len(matrix) == 0:
        return list
    m = len(matrix)
    n = len(matrix[0])
    x=0
    y=0

    while(m>0 and n>0):
        if(m==1):
            i = 0
            while i < n:
                spiral_list.append(matrix[x][y])
                i += 1
                y += 1
            break
        elif(n==1):
            i = 0
            while i < m:
                spiral_list.append(matrix[x][y])
                i += 1
                x += 1
            break

        i = 0
        while i < n-1:
            spiral_list.append(matrix[x][y])
            i+=1
            y+=1

        j = 0
        while j < m-1:
            spiral_list.append(matrix[x][y])
            j+=1
            x+=1

        i = 0
        while i < n-1:
            spiral_list.append(matrix[x][y])
            i+=1
            y-=1

        j = 0
        while j < m-1:
            spiral_list.append(matrix[x][y])
            j+=1
            x-=1

        x+=1
        y+=1
        m=m-2
        n=n-2

    return spiral_list

Test cases:

Input
[[1,2],[3,4]]
Expected Result
[1, 2, 4, 3]
Input
[[1,2],[3,4]
Expected Result
[1, 2, 4, 3]

Input
[[1,2,3,4],[5,6,7,8],[9,10,11,12]]
Expected Result
[1, 2, 3, 4, 8, 12, 11, 10, 9, 5, 6, 7]

input
[[1,2],[3,4]
Expected Result
[1, 2, 3, 4, 5, 6, 12, 18, 17, 16, 15, 14, 13, 7, 8, 9, 10, 11]

Input
[[1,0],[0,1]]
Expected Result
[1, 0, 1, 0]

Input
[[1,2,3],[4,5,6],[7,8,9]]
Expected Result
[1, 2, 3, 6, 9, 8, 7, 4, 5]

Just reviewing solution 1.

The solution is clever (repeatedly extract the first row and rotate the remainder by a quarter-turn) and the code terse.
However, the code destructively modifies the input matrix by popping the first row. This would be surprising to the caller and might make it more difficult to use or to test.
The code does not work in Python 3: it fails with the exception:

TypeError: 'zip' object is not subscriptable

In Python 3 you would have to write list(zip(...)). Since Python 2.7 is close to its end of life it is a good idea to write code that would be easy to port to Python 3. In this case you could use

from future_builtins import zip

to get a version of the built-in zip function that's compatible with Python 3.
If the input is an \$n×n\$ matrix, then rotating the remainder takes \$O(n^2)\$ and this has to be done \$O(n)\$ times, leading to a total runtime of \$O(n^3)\$. But intuitively it ought to be possible to output the spiral in \$O(n^2)\$.

We can achieve this by implementing the same approach (repeatedly extract the first row and rotate the remainder by a quarter-turn) but instead of modifying the matrix, we instead modify our system of coordinates. This is probably easiest to see if I give the code first:

def find_spiral_2(matrix):
    "Return list of elements of matrix in clockwise spiral order."
    h, w = len(matrix), len(matrix[0]) # Height and width of remainder
    i, j = 0, -1                # Current position in spiral
    di, dj = 0, 1               # Current direction of movement
    spiral = []
    while h > 0:
        # Output top row
        for _ in range(w):
            i += di
            j += dj
            spiral.append(matrix[i][j])
        # Remove top row and rotate quarter-turn
        h, w = w, h - 1
        di, dj = dj, -di
    return spiral

The way this works is that we maintain the state of the algorithm in the form of the height \$h\$ and width \$w\$ of the remainder of the matrix, the coordinates \$(i, j)\$ of our current position along the spiral, and a vector \$(di, dj)\$ giving the current direction of movement along the spiral path.

To output the top row, we follow the current direction for \$w\$ steps, leaving us in the top-right corner of (the remainder of) the matrix. Then, as in your solution, we remove the top row and rotate the remainder though a quarter-turn—or rather, we modify our coordinates to have this effect. Removing the top row subtracts 1 from \$h\$. Rotating the remainder through a quarter-turn causes \$w\$ and \$h\$ to swap (height becomes width and vice versa) and the current direction of movement to rotate through a quarter-turn. You should check that the line di, dj = dj, -di achieves this.

It should be clear that this does not modify matrix, and runs in \$O(n^2)\$ as required.


How can I speed up my calculations with loops? Python I wrote this code. But it works very slowly.
I'm figuring out how many times I have to run the case generator to find numbers less than or equal to inv, in this case six. I count the number of attempts until a digit <= 6 is generated. I find inv equal to 1 and repeat the loop. Until inv is 0. I will keep trying to generate six digits <= 6.
And I will repeat all this 10 ** 4 degrees again to find the arithmetic mean.
Help me speed up this code. Works extremely slowly. The solution should be without third-party modules. I would be immensely grateful. Thank!
import random

inv = 6

    def math_count(inv):
        n = 10**4
        counter = 0
        while n != 0:
            invers = inv
            count = 0
            while invers > 0:
                count += 1
                random_digit = random.randint(1, 45)
                if random_digit <= invers:
                    invers -= 1
                    counter += count
                    count = 0

            if invers == 0:
                n -= 1
                invers = inv

        return print(counter/10**4)

math_count(inv)

The code is very hard to follow. Here's a possible refactor to make its intent
clearer. Some key ideas in the simplification: (1) use loops to manage counters
rather than manually incrementing/decrementing; (2) use more
meaningful variable names when context isn't clear; and (3) delegate some of
the complexity to a helper function. The third idea is the most impactful: the
helper function is simple because it does so little; and the main function,
having been relieved of a burden, is simpler as well -- so simple, in
fact, that it boils down to computing a sum over two loops.
def math_count(inv, trials = 10000):
    total_attempts = sum(
        attempts_to_get_lte(x)
        for _ in range(trials, 0, -1)
        for x in range(inv, 0, -1)
    )
    return total_attempts / trials

def attempts_to_get_lte(x):
    attempts = 0
    while True:
        attempts += 1
        if random.randint(1, 45) <= x:
            return attempts

But attempts_to_get_lte() is an easily solved probability problem: rather
than have Python simulate the manual flipping of coins, we can just do a little
math. If you prefer that approach, I believe the following is correct
(probability experts should chime in if I've gone astray):
def math_count_exact(inv):
    return sum(45 / x for x in range(inv, 0, -1))


How to make a mutable Range in Ruby? How can I make these methods better?

Range.class_eval do
    def addto_begin(x)
        return self.begin + x..self.end
    end
    def addto_end(x)
        return self.begin..self.end+x
    end
end

Right now, I have to type:

x = 1..10
x = x.addto_begin(3)

to change the begin of the range. What I want to be able to do is:

x = 1..10
x.addto_begin(3)

How do I do this?

Since ranges are immutable you would need to create your own range class that encapsulates the basic range object. This class will give you that and still keep and the methods associated with the basic range object.

class Myrange
  attr_accessor :range
  def initialize(a,b=nil,exc=false)
    if a.is_a? Range
      @range = a
    else
      @range = Range.new(a,b,exc)
    end
  end
  def addto_begin(x)
    Myrange.new(@range.begin + x..@range.end)
  end
  def addto_end(x)
    Myrange.new(@range.begin..@range.end+x)
  end
  def addto_begin!(x)
    @range = @range.begin + x..@range.end
    Myrange.new(@range)
  end
  def addto_end!(x)
    @range = @range.begin..@range.end+x
    Myrange.new(@range)
  end
  def to_s
    @range.to_s
  end
  def inspect
    self.to_s
  end
  def method_missing(*args,&blk)
    @range.send(*args,&blk)
  end
end

x = Myrange.new(1,10)
x = x.addto_begin(3)
p x #=> 4..10
y = x.addto_begin!(3)
p x #=> 7..10
p y.addto_end(3) #=> 7..13
p x.include? 9 #=> true :: Range methods still work


Sequence generator with custom step size, start value and operation I have created a little generator class to generate number sequences.The intended purpose of this class is to be used with generate_n to populate containers. I would like to know whether there is something which i can do to improve performance or usability or to make the code more generic. It should be possible to compile the code with a c++14 conformant compiler at the moment, but i am open to use any number of c++17 features.
My generator class:

#include <functional>

template<typename Number,
         typename AdvanceOperation = std::plus<Number>>
class sequence_generator
{
  public:
    constexpr sequence_generator(Number start_value = Number{},
                                 Number step_size = Number{1},
                                 AdvanceOperation advancer = AdvanceOperation{})
                                 : current_value{start_value},
                                   step_size{step_size},
                                   advancer{advancer}
    {}
    constexpr auto operator()() -> Number
    {
      return next();
    }
    constexpr auto next() -> Number
    {
      auto tmp = current_value;
      current_value = advancer(current_value, step_size);
      return tmp;
    }
  private:
    Number current_value;
    Number step_size;
    AdvanceOperation advancer;
};

template<typename Number, typename AdvanceOperation=std::plus<Number>>
auto make_sequence_generator(Number start_value, Number step_size,
                             AdvanceOperation advancer)
{
    return sequence_generator<Number, AdvanceOperation>{start_value, step_size,
                                                        advancer};
}

Some use cases:

#include <algorithm>
#include <iostream>
#include <vector>
#include <cassert>

#include "generator.hpp"

int main()
{
  std::vector<int> vector(10);
  std::generate(std::begin(vector), std::end(vector),
                sequence_generator<int>{});
  assert(vector == std::vector<int>({0,1,2,3,4,5,6,7,8,9}));
  std::generate(std::begin(vector), std::end(vector),
                make_sequence_generator(1,2,std::multiplies<>{}));
  assert(vector == std::vector<int>({1,2,4,8,16,32,64,128,256,512}));
}

Generally code looks good. Despite that, there are number of places where it could be improved.

There is no need to specify output type explicitly. I'm not a language lawyer, but return type seems explicit to me. Also, ideone compiler is able to compile it without return type hints.

The behavior of the next is surprising. It returns old value and then advances it. I'd expect it to be vice versa. I guess you wanted to output initial value once, so it is probably a design choice. On top of that, it is probably the most performance draining part of the code.

There is no need to write constexpr on every function. It will mean inline, which itself means bloating size of the executable. Cases where it could be really compile time evaluated are very rare. The deal breaker factor here is advancer function, and if it returns different values on each call it is probably impossible to evaluate it at compile time. I'm excluding possibilities of immense template metaprogramming magic.

I would change interface to use binary operation to advance it. It would be much more customizable.

Overall I think std::generate_n is more for randomly generated numbers or something alike. People usually use std::iota to fill with increasing values.

Proposal

I propose a very different solution.

The solution will be a generic function named stepping_iota.

template <typename OutputIterator, typename T, typename UnaryAdavanceOp>
void stepping_iota(OutputIterator first, OutputIterator last, T init_value, UnaryAdavanceOp op)

It will call std::iota under the hood, to avoid surprises. But to do that, it will use proxy pattern. Specifications say that it is not required for decltype(*first) and T to match. So we can implement very small proxy, which will have all the needed functionality.

namespace impl_details
{
    template <typename T, typename UnaryAdvanceOp>
    class proxy
    {
        T value;
        UnaryAdvanceOp op;
    public:
        proxy(T init_value, UnaryAdvanceOp op_) :
            value(init_value),
            op(op_)
        {}

        operator T()
        {
            return value;
        }

        T operator++()
        {
            value = op(value);
            return value;
        }

        T operator++(int)
        {
            T old = value;
            value = op(value);
            return old;
        }
    };
}

I've written post increment for the sake of symmetry.

Then the stepping_iota will construct the proxy and pass it to std::iota:

impl_details::proxy<T, UnaryAdavanceOp> p(init_value, op);
std::iota(first, last, p);

Despite the function has a few parameters, it is customizable and easy to use with lambdas. It uses unary operation, which is common and used in functions such as std::transform.

Complete example:

#include <numeric>

namespace impl_details
{
    template <typename T, typename UnaryAdvanceOp>
    class proxy
    {
        T value;
        UnaryAdvanceOp op;
    public:
        proxy(T init_value, UnaryAdvanceOp op_) :
            value(init_value),
            op(op_)
        {

        }

        operator T()
        {
            return value;
        }

        T operator++()
        {
            value = op(value);
            return value;
        }

        T operator++(int)
        {
            T old = value;
            value = op(value);
            return old;
        }
    };
}

template <typename OutputIterator, typename T, typename UnaryAdavanceOp>
void stepping_iota(OutputIterator first, OutputIterator last, T init_value, UnaryAdavanceOp op)
{
    impl_details::proxy<T, UnaryAdavanceOp> p(init_value, op);
    std::iota(first, last, p);
}

#include <vector>
#include <iostream>

int main()
{
    std::vector<int> v(10'000);

    stepping_iota(v.begin(), v.end(), 0, [](const int& x) { return x + 1; });

    for (const auto& elem : v)
    {
        std::cout << elem << ' ';
    }
}

In C++17 you will be able to write:

impl_details::proxy p( init_value, op );


Linked list implementation of binary heap insertion Is this a correct linked list implementation of binary heap insertion? This is not a homework exercise.

public BinaryHeap insert(int value , BinaryHeap node , int count)
    {

        //1. Insert into a binary tree breadth wise( satisfy complete binary property) (left to right)
        //2. Once inserted ,fix the tree by running heapify every time a new value(non duplicate) is added.
        //3. Count represents the total no of elements in the tree. Calculate the total no of parents for given set of elements.

         int parent = (count - 1)/2;

        if (node == null)
        {
            node = new BinaryHeap();
            node.data = value;

        }

            else
        {
            if (((parent == 0) && (node.left == null)) || (parent % 2 == 1))
            {

                node.left = insert(value, node.left, parent);
                Debug.Assert(node.left != null);
                node.left.parent = node;
                Heapify(node.left);
                if (node.right != null) // if both left and right sub tree are filled
                {
                    Debug.Assert(node.data < node.left.data && node.data < node.right.data);// heapified node  will be minimum
                }
                else // incomplete subtree withh right subtree not filled.
                {
                    Debug.Assert(node.data < node.left.data);
                }
            }

            else
            {
                Debug.Assert(node.left != null);
                node.right = insert(value, node.right, parent);
                Debug.Assert(node.right != null);
                node.right.parent = node;

                Heapify(node.right);
                if (node.left != null) // if both left and right sub tree are filled
                {
                    Debug.Assert(node.data < node.left.data && node.data < node.right.data);// heapified node  will be minimum
                }
                else // incomplete sub tree with right sub tree not filled.
                {
                    Debug.Assert(node.data < node.right.data);
                }

            }
        }// end of else
        // every time you add an element you run heapify to rearrange the nodes.

        return node;
    }

    public void Heapify(BinaryHeap node)

    {
   //Bubble up until you are unable to
        if (node == null)
        {
            return;
        }

        else
        {

            if (node.parent != null && node.parent.data > node.data)
            {
                //swap the value
                int temp = node.parent.data;
                node.parent.data = node.data;
                node.data = temp;
                node = node.parent;
                Heapify(node.parent);
                return;
            }
    }
  }// end of function heapify

It's always worth tidying up indentation and removing random blank lines (e.g. after {).

Naming: in C# it's conventional that method names are in UpperCamelCase, but even if you choose not to follow the convention it's important to be consistent. Here Heapify follows the convention but insert doesn't.

The name Heapify usually refers to taking an unstructured array of data and structuring it as a heap in linear time. The method implemented here should probably have a name like UpHeap.

It's not clear how insert is supposed to be used. Is it called passing the root of the tree? And where does count come from? It seems to me that BinaryHeap should be BinaryHeapNode and should be a private inner class of a public BinaryHeap which exposes public void Insert(int value).

        int parent = (count - 1)/2;

What does this represent? Elsewhere parent is a BinaryHeap, and it's obvious what it means, but as far as I can guess this variable is actually the size of the right subtree.

            if (((parent == 0) && (node.left == null)) || (parent % 2 == 1))

Why? I need some comments somewhere in the code to say how the tree is structured. This looks like an attempt to keep it balanced, but I'm not convinced that it works. (I suspect it should check the parity of count rather than parent). Maybe if I knew precisely what it was trying to do I could be convinced.

I don't understand the necessity of all those asserts, but IMO it would be cleaner to remove the duplication by pulling them out of the cases as

            if (node.left != null)
            {
                Debug.Assert(node.data < node.left.data);
            }
            if (node.right != null)
            {
                Debug.Assert(node.data < node.right.data);
            }

or

            Debug.Assert(node.left == null || node.data < node.left.data);
            Debug.Assert(node.right == null || node.data < node.right.data);

The recursive insert calls Heapify at each level that it touches. Heapify bubbles up. That means there's unnecessary duplication of work here.

Why is Heapify public?

if (condition)
{
    DoStuff();
}
else
{
    if (otherCondition)
    {
        OtherStuff();
    }
}

creates unnecessary indentation and vertical spacing, both of which make it harder to read the code. Rewrite as

if (condition)
{
    DoStuff();
}
else if (otherCondition)
{
    OtherStuff();
}


Enum with associated value as stored property in Swift extension I have a service which connects to server using socket and stays connected,
I used this enum to manage connection state:

enum ConnectionState {
  case none, connecting, failed(Error), connected, disconnected(Error), blocked
}

I wanted to have connectionState as property to know current connection state of service but in extension of service provider class.

Why in extension and not in class itself?
Because ServiceProvider class is not Swift class, its ObjC class and there is no way we I can declare ConnectionState swift enum with associated values in ObjC class.

So I used associative stored property in swift extension, but as ConnectionState has associated values I need to do little more.

First I created extension to enum to make dictionary convertible as:

extension ConnectionState {
  var dictPresentation: [String: Error?] {
    switch self {
    case .none: return ["none": nil]
    case .connecting: return ["connecting": nil]
    case .failed(let error): return ["failed": error]
    case .connected: return ["connected": nil]
    case .disconnected(let error): return ["disconnected": error]
    case .blocked: return ["blocked": nil]
    }
  }

  init(dict: [String: Error?]) {
    assert(!dict.isEmpty, "dict must contain one key")
    let key = dict.keys.first!
    let error: Error? = dict[key] ?? nil
    switch key {
    case "none": self = .none
    case "connecting": self = .connecting
    case "failed": self = .failed(error!)
    case "connected": self = .connected
    case "disconnected": self = .disconnected(error!)
    case "blocked": self = .blocked
    default:
        assert(false, "wrong key in dict")
        self = .none
    }
  }
}

Then used it in service provider class extension as:

private var ConnectionStateHolderKey: UInt8 = 7
extension ServiceProvider {
  private var _connectionStateHolder: NSDictionary {
    get {
        if let connectionStateDict = objc_getAssociatedObject(self, &ConnectionStateHolderKey) as? NSDictionary {
            return connectionStateDict
        }
        //return ["none": nil] gives error "nil is not compatible with expected dictionary value type 'Any'"
        //return ["none": nil as Any] gifs error "Nil is not compatible with 'Any' in coercion"

        let dict = NSDictionary()
        dict.setValue(nil, forKey: "none")
        return dict//default state = none
    }
    set {
        objc_setAssociatedObject(self, &ConnectionStateHolderKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
    }
  }

  var connectionState: ConnectionState {
    get {
        return ConnectionState(dict: _connectionStateHolder as! [String : Error?])
    }
    set {
      _connectionStateHolder = newValue.dictPresentation as NSDictionary
    }
  }
}

Is there anything that can be used to simplify this code and avoid dictionary conversion?
Any suggestions that can improve this code are welcomed!

First note that your var connectionState is  computed property,
not a stored property. It is computed using the get and set
methods. Extensions cannot add stored properties to a type.

Your current implementation crashes if the connection state is retrieved
without having been set before:

let provider = ServiceProvider()
print(provider.connectionState)

because at

let dict = NSDictionary()
dict.setValue(nil, forKey: "none")

dict is an immutable dictionary. Making it mutable does not solve the
problem

let dict = NSMutableDictionary()
dict.setValue(nil, forKey: "none")

because the second statement has no effect and therefore the assertion

assert(!dict.isEmpty, "dict must contain one key")

fails.

You want to create a NSDictionary which – when bridged to [String: Error?] – becomes ["none": nil]. The difficulty here is that the
value type of the Swift dictionary is an optional. But it is possible,
using NSNull():

    get {
        if let connectionStateDict = objc_getAssociatedObject(self, &ConnectionStateHolderKey) as? NSDictionary {
            return connectionStateDict
        }
        return ["none": NSNull()]
    }

(compare SE-0140 Warn when Optional converts to Any, and bridge Optional As Its Payload Or NSNull).

But: All these conversions between enum, Swift dictionary and NSDictionary
are actually not needed, you can achieve the same much simpler.

As stated in Swift Blog: Objective-C id as Swift Any,

  The Objective-C bridge in Swift 3 can in turn present any Swift value as an id-compatible object to Objective-C.

This means that you can pass an enum ConnectionState directly to objc_setAssociatedObject()
and conditionally cast the return value from objc_getAssociatedObject() back to the enum.
The Swift runtime automatically wraps the value into a class.
(AnyObject not working in Xcode8 beta6?
on Stack Overflow contains further information and links.)

So this is everything you need:

enum ConnectionState {
    case none, connecting, failed(Error), connected, disconnected(Error), blocked
}

private var ConnectionStateHolderKey: UInt8 = 7
extension ServiceProvider {

    var connectionState: ConnectionState {
        get {
            if let connectionState = objc_getAssociatedObject(self, &ConnectionStateHolderKey) as? ConnectionState {
                return connectionState
            }
            return .none
        }
        set {
            objc_setAssociatedObject(self, &ConnectionStateHolderKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
        }
    }
}

More suggestions:

Use the nil-coalescing operator ?? to provide a default value in the getter method.
Restrict the scope of the associated objects key by making it a static property inside extension ServiceProvider.

Then the code would look like this:

extension ServiceProvider {

    private struct Keys {
        static var connectionState : UInt8 = 7
    }

    var connectionState: ConnectionState {
        get {
            return (objc_getAssociatedObject(self, &ServiceProvider.Keys.connectionState) as? ConnectionState) ?? .none
        }
        set {
            objc_setAssociatedObject(self, &ServiceProvider.Keys.connectionState, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
        }
    }
}

Finally: Your enum has a case none for "no state". But the Swift way
of representing "no value" is to use an optional:

enum ConnectionState {
    case connecting, failed(Error), connected, disconnected(Error), blocked
}

extension ServiceProvider {

    private struct Keys {
        static var connectionState : Int = 0
    }

    var connectionState: ConnectionState? {
        get {
            return objc_getAssociatedObject(self, &ServiceProvider.Keys.connectionState) as? ConnectionState
        }
        set {
            objc_setAssociatedObject(self, &ServiceProvider.Keys.connectionState, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
        }
    }
}


Loop Through bitarray to retrieve subsets of that bitarray Objective:

I want to loop through a bitarray and store subsets of that bitarray in a table.

Context:

I have a bitarray with 48 elements where each element represent one hour. I want to look back 24 hours from the start of the second day and retrieve the intervals where the last bit is 1.

I was able to achieve this but want to know if someone can provide a better solution :)

I have a table called [Numbers] that has 5000 rows that was created in accordance to this link https://www.mssqltips.com/sqlservertip/4176/the-sql-server-numbers-table-explained--part-1/ .

SCRIPT:

DECLARE @Ba NVARCHAR(48) = '000011110000000001110000000011111111000011110000'
DECLARE @numberOfIntervals INT = 24;
DECLARE @intervals TABLE(
    SequenceId INT,
    [Periods] NVARCHAR(24)
)

INSERT INTO @intervals
SELECT number-1 AS [SequenceId], SUBSTRING(@Ba, number, @numberOfIntervals) AS [Values]
FROM [dbo].[Numbers]
WHERE  number > 1 AND number <= (LEN(@Ba)-(@numberOfIntervals-1)) AND RIGHT(SUBSTRING(@Ba, number, @numberOfIntervals), 1) = '1'

SELECT * FROM @intervals

RESULTS:

[SequenceId]    [Values]
_________________________
5               111000000000111000000001
6               110000000001110000000011
7               100000000011100000000111
8               000000000111000000001111
9               000000001110000000011111
10              000000011100000000111111
11              000000111000000001111111
12              000001110000000011111111
17              111000000001111111100001
18              110000000011111111000011
19              100000000111111110000111
20              000000001111111100001111

You're definitely on the right track by using a "Numbers" table, rather than a loop or cursor. Given the small size and the fact that you know up front that you only need 24 rows, there's no need to calculate it. You can also save a bit my creating the numbers table on the fly.

What you have now should already be reasonably fast but the in-line tally table and hard coded number of rows should beat it... If only by a small margin.

DECLARE @Ba NVARCHAR(48) = '000011110000000001110000000011111111000011110000'
DECLARE @numberOfIntervals INT = 24;
DECLARE @intervals TABLE(
    SequenceId INT,
    [Periods] NVARCHAR(24)
);

WITH
    cte_n (n) AS (SELECT 1 FROM (VALUES (1),(1),(1),(1),(1) ) n (n)),
    cte_Tally (n) AS (
        SELECT TOP (24)
            ROW_NUMBER() OVER (ORDER BY (SELECT NULL))
        FROM
            cte_n a CROSS JOIN cte_n b
        )
INSERT INTO @intervals
SELECT
    t.n - 1 AS [SequenceId],
    SUBSTRING(@Ba, t.n, @numberOfIntervals) AS [Values]
FROM
    cte_Tally t
WHERE
    SUBSTRING(@Ba, (t.n - 1)  + @numberOfIntervals, 1) = '1';

SELECT
    *
FROM
    @intervals;


Activating Lightbox on HTML elements with the has-lightbox class I'm in the process of trying to do "object oriented thinking", as to improve my code skill.

I've the following method that creates a lightbox for every item with the class .has-lightbox. Is this something that would benefit from being a class? A LightboxCollection class for example? That has an array Lightbox?

import * as basicLightbox from 'basiclightbox';

var lightboxableImages = document.getElementsByClassName("has-lightbox");
for (let image of lightboxableImages) {
    image.addEventListener('click', openLightbox);
}

function openLightbox(e) {
    const target = e.target;
    const src = target.dataset['lightbox-src'] ? target.dataset['lightbox-src'] : target.src;

    if (!src) {
        return false;
    }

    const instance = basicLightbox.create(`<img src="${src}">`);
    instance.show();
}

Objects not classes
In JavaScript there are no classes only Objects. The token class is just an alternative syntax (often referred to as syntactical sugar) for defining an Object.
Many ways to create objects
The following shows two very similar objects defined using class syntax and standard syntax
class Foo {
   constructor() { this.bar = "A" }
   poo() {}
}

function Foo() { this.bar = "B" }
Foo.prototype = { poo() {} }

You can also define the object as...
function Foo() {
   this.bar = "C";
   this.poo = function(){};
}

or...
const Foo = {
    bar: "D",
    poo() {},
};

or...
const Foo = () => ({
    bar: "D",
    poo() {},
});

...and there are many more ways to define an object.
Which to use
Which you use is very dependent on how you use the object.

Is there more than one instance?
What is the life time of an instance?
Do you need to extend the object? (polymorphic)
What level of encapsulation is required?
Do you need an inheritance model?
How do you wish to instantiate the object?
What is the legacy support requirement?

To know the language you must be proficient in all its forms, and know what is the appropriate code to use.
Questions

Is this something that would benefit from being a class?

No as there are no classes in JavaScript.

A LightboxCollection class for example? That has an array Lightbox?

You can create a collection from an Array. Assuming you are using only a single instance. It has all the functions that an array has plus what ever you want to add. Eg add
const LightboxCollection = (() => {
    return Object.assign([],{
         add(lightBox){ this.push(lightBox) },
    });
})();

Or encapsulate the array, so you can maintain the array state (vet items and expose only functions that you need)
const LightboxCollection = (() => {
    const items = [];
    return Object.freeze({
         add(lightBox) { lightBox instanceof LightBox && items.push(lightBox) },
         get length() { return items.length },
         set length(len) { items.length = len < items.length ? len : items.length },
         forEach: items.forEach.bind(items),
    });
})();


Dockerfile to install Python and React in Linux container I am learning how to use Docker. I'm creating a container that contains Python and React. This code works, but it seems so heavy when I install it. I only want React, but to get React, I install Node so I can use npm later on. Do you see ways to optimize this? Or are there any red flags?

Dockerfile:

FROM python:3.6-slim

# first two are for python, last five are for node
RUN apt-get update && \
    apt-get install -qq -y build-essential libpq-dev --no-install-recommends && \
    apt-get install -y curl && \
    apt-get -y autoclean && \
    curl -sL https://deb.nodesource.com/setup_5.x | bash && \
    apt-get install -y nodejs && \
    apt-get autoremove -y

# install python components
ENV INSTALL_PATH /brainlint
RUN mkdir -p $INSTALL_PATH
WORKDIR $INSTALL_PATH
COPY requirements.txt requirements.txt
RUN pip3 install -r requirements.txt
COPY . .
RUN pip3 install --editable .
CMD gunicorn -b 0.0.0.0:8000 --access-logfile - "brainlint.app:create_app()"

Requirements.txt:

Flask==0.12.2
gunicorn==19.7.1

Your dockerfile is generally correct. Every RUN instruction in the Dockerfile writes a new layer in the image. Every layer requires extra space on disk. In order to keep the number layers to a minimum, any file manipulation like moving, extracting, removing, etc, should ideally be made under a single RUN instruction. So, you should try to minimize number of RUN command in your file (eg. try to merge installing requirements into one command) You can check size of you layers using cmd:

  docker history

But there is question - Why do you want to have react and python in one container?

From this dockerfile I don't understand why you are adding node/react to your container. If you need python, because you have to prepare some data for react or run some script before start your node app, then you could think about use Multi-Stage Builds. Example:

FROM python:3.6-slim

# here build/do what you want to do
WORKDIR /myapp
COPY myscript.py .
RUN python myscript.py > data

FROM bayesimpact/react-base

# run your react application
WORKDIR /myapp
COPY react_app .
CMD ["./react_app/app", "data"]

So, when you are using Muli-Stage Builds you can:

  You can selectively copy artifacts from one stage to another, leaving
  behind everything you don’t want in the final image.

Also, there is possibility to use prepared earlier image with node and python, it should be smaller, but you have to also check this. (e.g beevelop/docker-nodejs-python)


Binary Search implementation: HackerRank - Climbing the Leaderboard My code passes 11 out of 12 test cases. I was wondering where I can improve my code. NOTE: This code needs performance improvement, as it is working for most of the cases. To mu knowledge it would work for all the test cases where the size of arrays is smaller than 200.
Here is the question:

Alice is playing an arcade game and wants to climb to the top of the leaderboard and wants to track her ranking. The game uses Dense Ranking, so its leaderboard works like this:
The player with the highest score is ranked number 1 on the leaderboard.
Players who have equal scores receive the same ranking number, and the next player(s) receive the immediately following ranking number.
For example, the four players on the leaderboard have high scores of 100, 90, 90, and 80. Those players will have ranks 1, 2, 2, and 3, respectively. If Alice's scores are 70, 80 and 105, her rankings after each game are 4, 3 and 1.

And here is my code:
#!/bin/python3

import math
import os
import random
import re
import sys

# Complete the climbingLeaderboard function below.
def binSearchMod(list1, value, start, end): #implemented for descending order
    mid = (start+end)//2
    #print('Looking for value: ', value, ' in ', start, end, mid , 'list :', list1)
    #conditions for element at start or end or mid
    if value==list1[start]:
        mid = start
    if value == list1[end]:
        mid = end
    if value == list1[mid]:
        return [True, mid]
    if end-start == 1: # if some element lies in between 2 numbers of array
        #print('Found between ', start, end)
        return [False, start]
    if value < list1[mid]:
        return binSearchMod(list1, value, mid, end)
    else:
        return binSearchMod(list1, value, start, mid)

def climbingLeaderboard(scores, alice): # O(log n), not really we have to go through all scores to determine their rank
    res=[]
    rank =1
    rankScores=[scores[0]]
    #ssign ranks to scores
    for score in range(1,len(scores)):
        if scores[score]!=scores[score-1]:
            rank+=1
            rankScores.append(scores[score])

    for ascore in alice:
        if ascore<scores[len(scores)-1]: # alice scores are smaller than all
            res.append(len(set(scores))+1)
        elif ascore > scores[0]: #alice score is greatest
            res.append(1)
        else: #alice score lies somewhere in between
            bsResult = binSearchMod(rankScores, ascore, 0 , len(rankScores)-1)
            #print(ascore, bsResult)
            if bsResult[0]:
                res.append(bsResult[1]+1)
            else:
                res.append(bsResult[1]+2)
    return res

I am guessing I'm trying to improve on test cases with length of arrays containing all scores and alice scores are > 200.

You are on the right track. However, implementing your own bisection algorithm is not a good idea. Python has the built-in ("batteries included") bisect module, containing all the bisection algorithms we need.
They are implemented in Python, but overridden by fast C implementations if those are available, which would then be as fast as we can hope for.

The from bisect import bisect (with the bisect function as an alias for bisect_right) replaces your binSearchMod function.
In the code at the bottom, there is a "manual" bisect implementation without recursion, which is also a step forward.
It is probably best to avoid recursion if (much) simpler means are available.

In your base climbingLeaderboard function, you have

        if ascore<scores[len(scores)-1]: # alice scores are smaller than all
            res.append(len(set(scores))+1)
        elif ascore > scores[0]: #alice score is greatest
            res.append(1)

which handles special cases.
These cases are not special enough to warrant this, and are code smell.
Your basic search algorithm should return correct results to append to res on its own, as we will see shortly.
See also import this: Special cases aren't special enough to break the rules..

As an aside, slicing (using slice objects) makes indexing sequences much easier: scores[len(scores)-1] is just scores[-1].
Further, you return a list using

return [False, start]

This is a bad idea.
You later use it to index into it, but that job can better be done by a tuple.
Simply calling

return False, start

will return a tuple.
This can be unpacked into two variables in one assignment, or indexed into just like lists.
Tuple unpacking is convenient and easy to read.

The distinction between lists and tuples is important: lists are (should be) homogeneous, aka contain a sequence of elements of the same type (think filenames).
Tuples are heterogeneous, aka the position of elements has meaning and they are of different types.
In your example here, this would be bool and int, which have different semantics.

A key aspect to realize is that duplicate scores in the leaderboard can just be tossed, since they do not count towards anything.
This calls for a set implementation.
This also automatically gets rids of your

    #ssign ranks to scores
    for score in range(1,len(scores)):
        if scores[score]!=scores[score-1]:
            rank+=1
            rankScores.append(scores[score])

code block, saving a whole \$ \mathcal{O} (n) \$ iteration.

Since bisect relies on ascending order, while the input is sorted in descending order, a call to sorted is required, which automatically returns a list.

bisect(sequence, item) will return the index where to insert item in sequence while retaining order.
If items compare equal, item is inserted to the right of existing items.
If a list of scores in ascending order is [20, 30, 50], then Alice is really in second place if she scored 30. bisect_left would sort her into third place.

Since ranks are 1-indexed, increment by 1.
Lastly, the below result would be inverted, since the sorting inverted the list.
Therefore, use len to rectify.

#!/bin/python3

import math
import os
import random
import re
import sys
from bisect import bisect

# Complete the climbingLeaderboard function below.
def climbingLeaderboard(scores, alice):
    length = len(scores)
    return [length - bisect(scores, alice_score) + 1 for alice_score in alice]

if __name__ == '__main__':
    fptr = open(os.environ['OUTPUT_PATH'], 'w')

    scores_count = int(input())

    scores = sorted(set(map(int, input().rstrip().split())))

    alice_count = int(input())

    alice = list(map(int, input().rstrip().split()))

    result = climbingLeaderboard(scores, alice)

    fptr.write('\n'.join(map(str, result)))
    fptr.write('\n')

    fptr.close()

This passes all tests. The required sorted step is \$ \mathcal{O}(n\, \log n)\$, see here.

Without sorting the input, a bisect implementation that works on reversed sorted lists is required.
The change compared to the original implementation (link above) is minimal, as seen below.
if a[mid] < x: lo = mid+1 is simply inverted to if a[mid] > x: lo = mid+1 (I also formatted the code more).

Simply calling list((set(sequence)) on the scores will not work.
Duplicates will be purged, but the order will be lost.
Therefore, we simply construct a new list, using a set to block appending already seen elements, see here.

The below approach works, but similarly to yours, fails for long inputs in its naive version.
This is why I added previous_higher_bound.
This counter keeps track of what rank Alice was on in the past.
It could also be named previously_lowest_rank or similar.
This is passed to bisect to drastically tighten the searched range, allowing the tests to pass.
Unfortunately, it also makes the approach more verbose.

# Complete the climbingLeaderboard function below.
def climbingLeaderboard(scores, alice):
    def reverse_bisect_left(sequence, x, lower_bound=0, higher_bound=None):
        """Return the index where to insert item x in list a, assuming a is sorted in reverse.
        """
        if lower_bound < 0:
            raise ValueError("lo must be non-negative")
        if higher_bound is None:
            higher_bound = len(sequence)
        while lower_bound < higher_bound:
            middle = (lower_bound + higher_bound) // 2
            if sequence[middle] > x:
                lower_bound = middle + 1
            else:
                higher_bound = middle
        return lower_bound, higher_bound

    def uniquelify_list(sequence):
        seen = set()
        return [int(x) for x in sequence if not (x in seen or seen.add(x))]

    def leaderboard_rank(scores, score, higher_bound=None):
        result, previous_higher_bound = reverse_bisect_left(scores, int(score), higher_bound=higher_bound)
        return result + 1, previous_higher_bound

    def get_ranks(scores, alice_scores):
        scores = uniquelify_list(scores)
        previous_higher_bound = len(scores)
        ranks = []
        for alice_score in alice_scores:
            result, previous_higher_bound = leaderboard_rank(scores, alice_score, previous_higher_bound)
            ranks.append(result)
        return ranks
    return get_ranks(scores, alice)