Implementation of Conway's Game Of Life on Android I ended up using 2x 2D arrays with a one element border around everything to simplify the kernel logic.

import android.graphics.Canvas;
import android.graphics.Color;
import android.graphics.Paint;

import java.util.Random;

public class WorldSimulation {

    long generation = 0;

    int[][]  next = null;
    int[][] current = null;

    float cellWidth;
    float cellHeight;

    int width;
    int height;

    public WorldSimulation(int width, int height, float w, float h)
    {
        this.width = width;
        this.height = height;

        next = new int[width+2][height+2];
        current = new int[width+2][height+2];

        cellWidth = (w / this.width);
        cellHeight = (h / this.height);
    }

    public void timeStep(Canvas canvas, Paint paint)
    {
        generation ++;

        for (int i = 0; i < next.length; i++) {
            System.arraycopy(next[i], 0, current[i], 0, next[0].length);
        }

        for(int i = 1; i < this.width+1; i++)
        {
            for(int j = 1; j < this.height+1; j++)
            {
                float left = (i * cellWidth);
                float top = (j * cellHeight);
                float right = (left + cellWidth);
                float bottom = (top + cellHeight);

                int status = current[i][j];

                int neighbours = livingNeighbours(i, j);

                if(status == 255) {
                    paint.setColor(Color.rgb(0, 255, 0));

                    canvas.drawRect(left-cellWidth,top-cellHeight,right,bottom,paint);

                    if (neighbours < 2) {
                        next[i][j]=0;
                    }
                    else if(neighbours >3)
                    {
                        next[i][j]=0;
                    }
                }
                else if(status!=255)
                {
                    paint.setColor(Color.rgb(0, status--, 0));
                    canvas.drawRect(left-cellWidth,top-cellHeight,right,bottom,paint);

                    if(neighbours == 3)
                    {
                        next[i][j] =  255;
                    }
                }
            }
        }
    }

    public int livingNeighbours(int i, int j) {
        int alive = 0 ;

        for(int kernelRow = i -1; kernelRow <= i+1; kernelRow ++)
        {
            for(int kernelCol = j -1; kernelCol <= j+1; kernelCol ++)
            {
                if(current[kernelRow][kernelCol]== 255 && !(kernelRow == i && kernelCol == j))
                {
                    alive ++;
                }
            }
        }
        return alive;
    }

    public void setCell(int i, int j, boolean alive)
    {
        next[i][j] =  255;
    }

}

The full implementation can be found here: https://github.com/jacasey/life/ and I've put a version up on the Google Play Store as well.

The whole thing clocks in at around 12 KB which I think is pretty cool when you compare it to the monters apps on the playstore today.

I was thinking of re-implementing this with a 1D array and indexing calculations but I don't think this would really add to much re: size or performance wise.

https://play.google.com/store/apps/details?id=com.jcasey.life

General isues

Your code doesn't follow the single responsibility principle. The class WorldSimulation is responsible for updating the game and creating the graphical representation. If you followed a naming convention where the name of a class describes it's functionality it would have to be WorldSimulationAndPaint.

Instead of passing the canvas and paint to the game engine, you should try to follow the MVC pattern. I have once implemented a Conway's game of life too. I had a timer that was responsible for controlling the updates. The timer sent an event to the game engine telling it "please advance the game state by one frame". When the game engine had done that, the game engine sent an event to the user interface telling it "here's the lates game state, please update the user interface."

Once you have removed the responsibility of drawing from WorldSimulation you can remove the cellWidth and cellHeight fields.

The events can be transmitted by setting up each component as a listener to the relevant source or with an event bus (such as Google Guava).

Field visibility

All the fields in WorldSimulation are visible to other classes in the package. The fields of a class should be as closely guarded as possible so they should be private unless a there is a very good reason to do otherwise. That way you guarantee that the state of the object can only be changed by the object itself. It also forces you to concentrate on the interface your class provides to others.

The livingNeighbours method is not supposed to be accessed from outside the class, so it should be private too.

Magic numbers

What does 255 mean? Instead of using a literal integer every time you should define it as a constant whose name describes the meaning of the value and use the constant instead of the literal 255.

private static final int STATUS_ALIVE = 255;
private static final int STATUS_DEAD = 0;

Duplicated code

The drawRect call is identical in both branches of the if statement. You can take them both out and replace them with a single call after the if statement.

canvas.drawRect(left-cellWidth,top-cellHeight,right,bottom,paint);

Misleading naming

In livingNeighbours you count number of cells, but the value is collected in a variable named alive. In spoken language "alive" has only two values (alive or dead). Thus it sugests that the field would be a boolean (as you have done in the setCell method), while it is in fact an integer. It should be named livingNeighbourCount.

Misleading method signature

The alive parameter suggest that one could set a cell to be either living or dead. The implementation however only sets it to a living state. Either fix the code to handle the parameter or remove it and rename the method to setCellAlive.

public void setCell(int i, int j, boolean alive)
{
    next[i][j] =  255;
}

Rules

You're not really implementing Conway's Game of Life. :)


Generate xyz coordinates of a tube made of multiple rings In this code, I generate the coordinates of a tube made of multiple rings. Finally, I write those x,y,z coordinates in a .csv file. I want to simplify this code (maybe using a for loop?).

import numpy as np
import matplotlib.pyplot as plt

bLength=1.6
numPoints=10
radius = bLength*numPoints / (2 * np.pi)
theta = np.linspace(0,2*np.pi,numPoints,endpoint=False)
dtheta=theta[1]-theta[0]

#creating the x, y coordinates of the first ring
x0,y0=(radius * np.cos(theta)), (radius * np.sin(theta))
#creating the x, y coordinates of the second ring
x1,y1=(radius * np.cos(theta+dtheta/2)) , (radius * np.sin(theta+dtheta/2))
#above first ring and second ring will come alternatively to form a tube.
#plt.plot(x0,y0)
#plt.show()
#plt.plot(x1,y1)
#plt.show()
#now generating the z-coordinate of the first ring, second ring, third ring, etc.
cons0=np.ones(x0.shape)*0
cons1=np.ones(x1.shape)*2
cons2=np.ones(x0.shape)*4
cons3=np.ones(x1.shape)*6
cons4=np.ones(x0.shape)*8
cons5=np.ones(x1.shape)*10
cons6=np.ones(x0.shape)*12
cons7=np.ones(x1.shape)*14
cons8=np.ones(x0.shape)*16
cons9=np.ones(x1.shape)*18

###Now saving the x, y, z coordinates in csv files.

np.savetxt('cooRing00.csv',np.c_[x0,y0,cons0],delimiter=' ',fmt='%10f')
np.savetxt('cooRing01.csv',np.c_[x1,y1,cons1],delimiter=' ',fmt='%10f')
np.savetxt('cooRing02.csv',np.c_[x0,y0,cons2],delimiter=' ',fmt='%10f')
np.savetxt('cooRing03.csv',np.c_[x1,y1,cons3],delimiter=' ',fmt='%10f')
np.savetxt('cooRing04.csv',np.c_[x0,y0,cons4],delimiter=' ',fmt='%10f')
np.savetxt('cooRing05.csv',np.c_[x1,y1,cons5],delimiter=' ',fmt='%10f')
np.savetxt('cooRing06.csv',np.c_[x0,y0,cons6],delimiter=' ',fmt='%10f')
np.savetxt('cooRing07.csv',np.c_[x1,y1,cons7],delimiter=' ',fmt='%10f')
np.savetxt('cooRing08.csv',np.c_[x0,y0,cons8],delimiter=' ',fmt='%10f')
np.savetxt('cooRing09.csv',np.c_[x1,y1,cons9],delimiter=' ',fmt='%10f')

###Now, I have the x, y, z coordinates in many files (based on the length of the tube I want.

Thanks for sharing your code.

For the generation of your z-coordinate you can write something like:

def generate_z_coordinate(n, x0, x1):
    """Generate the z-coordinate for the n first ring.
       :param n: the number of ring
       :param x0: x0 coordinate
       :param x1: x1 coordinate
       :return: A list of the z-coordinate for the n rings
    """
    tmp = 0
    rings = []
    for i in range(0,n):
        if i % 2 == 0:
            x = x0
        else:
            x = x1
        rings.append(np.ones(x.shape) * tmp)
        tmp += 2
    return rings

I used tmp because I lack a better name, but there is probably a better way to name it.

And saving in the csv:

def save(filename, rings, x0, y0, x1, y1):
    """Save rings coordinate into filename
       :param filename: Name of the file to save data in
       :param rings: list of z-coordinate for the rings
       :param x0: x0 coordinate
       :param y0: y0 coordinate
       :param x1: x1 coordinate
       :param y1: y1 coordinate
    """
    for i, elt in enumerate(rings):
        name = filename + str(i) + '.csv'
        if i % 2 == 0:
            x = x0
            y = y0
            z = rings[i]
        else:
            x = x1
            y = y1
            z = rings[i]

        np.savetxt(name, np.c_[x, y, z], delimiter=' ', fmt='%10f')

I am not sure it is more readable than your version but it's more scalable. Depend on what you need.

Also try to be consistant in your code and follow Python style guide(see PEP8):

write bLength = 1.6 instead of bLength=1.6
theta = np.linspace(0, 2 * np.pi, numPoints, endpoint=False)

You can use flake8 or black or PyLint to lint your code


Square grid collision detection var s = player.getSurrounding();
    if (vY > 0) {
        if ((s[0] || (s[7] && xLocal < LOCAL_LEFT) || (s[4] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
            vY = (LOCAL_RIGHT - yLocal) / dt;
        }
    } else if (vY < 0) {
        if ((s[2] || (s[6] && xLocal < LOCAL_LEFT) || (s[5] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) < LOCAL_LEFT) {
            vY = (LOCAL_LEFT - yLocal) / dt;
        }
    }
    if (vX < 0) {
        if ((s[3] || (s[7] && yLocal > LOCAL_RIGHT) || (s[6] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) < LOCAL_LEFT) {
            vX = (LOCAL_LEFT - xLocal) / dt;
        }
    } else if (vX > 0) {
        if ((s[1] || (s[4] && yLocal > LOCAL_RIGHT) || (s[5] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) > LOCAL_RIGHT) {
            vX = (LOCAL_RIGHT - xLocal) / dt;
        }
    }

This might be a long shot, but I threw this together. It seems very inefficient and repetitive. It might be hard to explain, but here I go.

The world is made up of a grid of blocks that is saved in a 40x600 boolean array (true if filled, false if not).

The red block represents the player, a simple block that can fly in all directions. The position is defined by it's center point (orange).

The numbers represent the index of the boolean array s, true if the block is filled and false if not. Filled blocks should be interacted with.

xLocal and yLocal represents the position of the player in it's current grid position.
If the player is all the way to the left: xLocal = (playersize / 2) AKA LOCAL_LEFT.
If the player is all the way to the right: xLocal = blocksize - (playersize / 2) AKA LOCAL_RIGHT.
Same applies for yLocal but for up and down.

dt represents the game's delta time.

If the if-statements go through, the velocity is set so that xLocal will equal LOCAL_LEFT or LOCAL_RIGHT by the next time the position is updated.

Naming

s is a poor name for a boolean array. I have no idea what s stands for. How about surroundingTiles?

Array ordering

Additionally, the order of the tiles seems strange to me. It's ... bottom, right, top, left, bottom right, top right, top left, bottom left. Without the image, I don't think I'd have gotten it. (For those wondering, it's non-diagonal, starting bottom, counter clockwise, then diagonal.) Any particular reason you have this layout within your array?

Magic numbers

Continuing from my previous point, this can be solved by naming the directions in constants:

var BOTTOM_INDEX = 0;
var RIGHT_INDEX = 1;
var TOP_INDEX = 2;
var LEFT_INDEX = 3;
var BOTTOMRIGHT_INDEX = 4;
var TOPRIGHT_INDEX = 5;
var TOPLEFT_INDEX = 6;
var BOTTOMLEFT_INDEX = 7;

I'm calling it Magic Numbers because without the image, I'd be left wondering which index is which. They're magic in that if they change, the code stops working. But why it has to be those numbers exactly is unknown... to the reader, at least.

Difference so far

var surroundingTiles = player.getSurrounding();
if (vY > 0) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }
} else if (vY < 0) {
    if ((surroundingTiles[TOP_INDEX] || (surroundingTiles[TOPLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[TOPRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) < LOCAL_LEFT) {
        vY = (LOCAL_LEFT - yLocal) / dt;
    }
}
if (vX < 0) {
    if ((surroundingTiles[LEFT_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && yLocal > LOCAL_RIGHT) || (surroundingTiles[TOPLEFT_INDEX] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) < LOCAL_LEFT) {
        vX = (LOCAL_LEFT - xLocal) / dt;
    }
} else if (vX > 0) {
    if ((surroundingTiles[RIGHT_INDEX] || (surroundingTiles[BOTTOMRIGHT_INDEX] && yLocal > LOCAL_RIGHT) || (surroundingTiles[TOPRIGHT_INDEX] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) > LOCAL_RIGHT) {
        vX = (LOCAL_RIGHT - xLocal) / dt;
    }
}

(Ckuhn also posted some naming changes, I suggest you add them in here as well.)

But Pim, if I do this, my if statements become very long!

Encapsulate Conditionals

Let's take a look at one of these paths.

var surroundingTiles = player.getSurrounding();
if (vY > 0) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

if the vectorY is greater than 0... I guess that means if you're moving ... down... then...
if the bottom tile is solid or if either the bottom left tile is solid and the ... x something? or the bottom right tile is solid and x something but different...

It's way too confusing!
I can understand it, obviously, as I recognize the pattern (and you've given me an image for explanation). However, if I were to explain this code to someone else, I'd mentally recheck the code before continuing my explanation. I'd even manage to get it wrong at least once during my explanation ("and this checks for the upward movement, ... er, I mean the downward movement").

So, how can we fix that? Let's use English!

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && passingLeftTileBorder) || (surroundingTiles[BOTTOMRIGHT_INDEX] && passingRightTileBorder)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

It's still not clear enough. That's because I have no idea what (yLocal + vY * dt) > LOCAL_RIGHT means! Additionally, we still didn't get it clear enough.

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if ((hittingBottomTile || hittingBottomLeftTile || hittingBottomRightTile) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

Almost.

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if (hittingTileInBottomDirection && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

Nice. It just got wondrously clean.
Sadly, computers are grumpy things that want everything specified, so we'll have to explain our human-readable English to them.

var movingDown = vY > 0;
if (movingDown) {
    var hittingBottomLeftTile = (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT);
    var hittingBottomRightTile = (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT);
    var hittingTileInBottomDirection = (surroundingTiles[BOTTOM_INDEX] || hittingBottomLeftTile || hittingBottomRightTile);
    if (hittingTileInBottomDirection && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

At this point, you could consider shifting certain parts of the logic towards a function which does simple calculations. Your call. I'd prefer that to this inline logic, because if you use the functions, you can write this function on a different level of abstraction. That is, you can write English code consisting entirely of function calls. At that point, if you made a mistake, you'll spot it instantly. Because if you have a function that looks like this...

function checkForCollision(/*arguments omitted*/) {
    checkCollisionInDirection(LEFT);
    checkCollisionInDirection(RIGHT);
    checkCollisionInDirection(TOP);
    checkCollisionInDirection(LEFT);
}

The bug is obvious.

I'll leave the location of variable definitions and the (yLocal + vY * dt) > LOCAL_RIGHT to other answerers.


Hackerrank Gem Stones I am working on some problems on Hackerrank.

  John has discovered various rocks. Each rock is composed of various elements, and each element is represented by a lowercase Latin letter from 'a' to 'z'. An element can be present multiple times in a rock.  An element is called a 'gem-element' if it occurs at least once in each of the rocks.

  Given the list of rocks with their compositions, you have to print how many different kinds of gems-elements he has.

  Input Format

  The first line consists of N, the number of rocks. Each of the next N lines contain rocks’ composition. Each composition consists of small alphabets of English language.

  Output Format

  Print the number of different kinds of gem-elements he has.

  Constraints

  1 ≤ N ≤ 100

  Each composition consists of only small Latin letters ('a'-'z'). 1 ≤ Length of each composition ≤ 100

  Sample Input

3
abcdde
baccd
eeabg

  Sample Output

2

  Explanation Only "a", "b" are the two kind of gem-elements, since these characters occur in each of the rocks’ composition.

I solved the problem but I don't feel that it is fast or pythonic. I was wondering if someone could help me increase the performance and perhaps reduce the amount of memory being used.

numRocks = int(raw_input())
rockList = []
for x in xrange(numRocks):
    rock = raw_input()
    # list of sets()
    rockList.append(set(rock))
gemElement = 0
for x in rockList[0]:
    rocks = len(rockList) - 1
    count = 0
    for y in rockList[1:]:
        if x in y:
            count += 1
            if count == rocks:
                gemElement += 1
print gemElement

Your solution looks pretty good. However, a few details can be changed to make your code more pythonic :

Unused variable

You can use _ to name a variable whose value is not used. In your case, it applies to the first loop variable x.

List comprehension

You can rewrite your initialisaion of rockList abusing list comprehension :

rockList = [set(raw_input())
    for _ in xrange(int(raw_input()))]

Simplifying the logic

At the moment, for each element in the first set, you look in how many other sets it appears to know if it is a gem. You can make things clearer by considering that by default it is a gem except if we don't find it in one of the other sets (and in that case, we can stop looping) :

for x in rockList[0]:
    is_gem = True
    for y in rockList[1:]:
        if x not in y:
            is_gem = False
            break
    if is_gem:
        gemElement += 1

Using Python good stuff

Using Python all builtin, we can write this all(x in y for y in rockList[1:]).

You can extract rockList[1:] to call it only once (note that the cute way to write this in Python 3 would be to use extended iterable unpacking). Your code becomes :

gemElement = 0
other_rocks = rockList[1:]
for x in rockList[0]:
    if all(x in y for y in other_rocks):
        gemElement += 1
print gemElement

Now, you can see that we can easily get the actual list of gems doing :

gems = (x for x in rockList[0]
    if all(x in y for y in other_rocks))

This is not an actual list but a generator expression, you'll need to call list(gems) if you want to see an actual list but we don't really care about the list, we just need the number of elements : len(list(gems)) or sum if you don't want to build an intermediate list in memory.

The code is now :

rockList = [set(raw_input())
    for _ in xrange(int(raw_input()))]
other_rocks = rockList[1:]
print sum(1 for x in rockList[0]
    if all(x in y for y in other_rocks))

One step back from the code

The problem we are trying to solve is linked to a common problem : computing the intersection of multiple sets. It is a generic enough problem so that we can google it and find this answer for instance.

Making the solution as concise as possible, one can write :

print len(set.intersection(*[
    set(raw_input())
    for _ in xrange(int(raw_input()))]))


Parsing DAG from text This module allows to parse indented text file into Map from node to child nodes identified by Coordinate. This module seems way too complex - mainly because of managing stack that allows to "jump back" with decreasing indentation. How can I redesign it?

sample input:

a
 b
 c
  d
   e
 f

produces:

*Parse> parse tree
fromList [("a",["f","c","b"]),("c",["d"]),("d",["e"])]

implementation:

module Parse where
import System.Environment (getArgs)
import Data.List (find, isInfixOf, intercalate)
import Data.Char (isAlpha)
import Control.Lens ((&))
import Data.List.Split (splitWhen)
import Control.Arrow
import qualified Data.Map.Strict as Map
import Data.Maybe
import Debug.Trace
import Control.Applicative

type Coordinate = String

parse :: String -> Map.Map Coordinate [Coordinate]
parse = snd . go
  where
    go = foldl addDependency ([], Map.empty) . filter ((>0) . length) . lines

addDependency (stack, acc) line = if null stack then
                                   (,) [(0, line)] $ Map.singleton line []
                                   else let children = fromMaybe [] . flip Map.lookup acc
                                            lineDepth = countIndent line
                                            indent = signum $ lineDepth - (fst . head $ stack)
                                            coord = toCoordinate line
                                            addChild c = Map.insert c (coord : children c) acc
                                            parent = tail stack
                                        in
                                         case indent of
                                           0 -> (,) ((lineDepth,coord):parent) $ addChild (snd . head $ parent)
                                           1 -> (,) ((lineDepth,coord):stack) (addChild $ snd . head $ stack)
                                           _ -> let newStack = back stack lineDepth
                                                    newHead = snd . head $ newStack
                                                     in (,) ((lineDepth,coord):newStack) (addChild newHead)

back stack@((i,c):ss) indent = if indent > i then stack else back ss indent
toCoordinate = dropWhile (not . special)
special = (||) <$> isAlpha <*> (=='(')
countIndent = length . takeWhile (not . special)

Make things easier on yourself by separating concerns. E.g., each line encodes a position in its amount of leading whitespace followed by an identifier. Parse this representation into a more manipulable form in a single function invocation, don't thread it through your algorithm logic which shouldn't care about serialized representation.

type SerializedGraph = String -- Aliases for pedagogical clarity
type Indentation = Int
type Symbol = String

parse :: SerializedGraph -> [(Indentation, Symbol)]
parse = map (first length . span (== ' ')) . lines
    where first :: (a -> b) -> (a, c) -> (b, c)
          first f (a, c) = (f a, c)

Now consider intermediate representations that can get you closer to your goal. Remember that it's easy to construct a map from an association list with fromList, so let's make that our end goal. Write out the types for a roadmap from where we are to where we want to be.

    [(Indentation, Symbol)] -- [(Hint, Key)]
==> ???
==> [(Symbol, [Symbol])] -- [(Key, [Value])]

If that final representation has only unique keys, then a reasonable intermediary would be one that hasn't had values which share a key accumulated yet. I.e., [(Symbol, Symbol)] -- [(Key, Value)].

associate :: [(Int, String)] -> [(String, String)]
associate []          = []
associate ((i, s):ss) = [(s, t) | (j, t) <- descendants, j == i + 1]
                     ++ associate descendants
                     ++ associate siblings
    where (descendants, siblings) = span ((> i) . fst) ss

Now all that's left to do is create a Map. We could accumulate values on our own, but really that's what fromListWith is for.

toMap :: [(String, String)] -> Map String [String]
toMap = fromListWith (++) . map (second (:[]))
    where second f (a, b) = (a, f b)

All that's left is composing these functions together.

readGraph :: String -> Map String [String]
readGraph = toMap . associate . parse


Remove the object's sibling on click In my program below I am getting the parent node game 1 and then removing one of the two child nodes, depending on which one is clicked. I currently have two event listeners, with two separate functions for removing one child vs the other. It doesn't feel very dry to me, and I think I could benefit from having someone skilled in JS take a look at this and show me how this could be more easily accomplished.

var firstWinner = document.getElementById('game1');

function removefirst() {
    firstWinner.removeChild(firstWinner.childNodes[0]);
}

function removeSecond() {
    firstWinner.removeChild(firstWinner.childNodes[1]);
}

var gameOneNodes = document.getElementById('game1').childNodes;
gameOneNodes[1].addEventListener('click', removefirst, false)
gameOneNodes[0].addEventListener('click', removeSecond, false)

You should define one click handler that removes all siblings of the clicked element.  That is, remove all children of the parent, except the target of the event itself.

document.getElementById('game1').addEventListener('click', function(event) {
    // Remove all siblings of the clicked element
    for (var c = this.childNodes.length - 1; c >= 0; c--) {
        if (this.childNodes[c] != event.target) {
            this.removeChild(this.childNodes[c]);
        }
    }
});
<div id="game1">
  <button>Alpha</button>
  <button>Bravo</button>
  <button>Charlie</button>
  <button>Delta</button>
</div>

You could also iterate this way, but there's an awkward c--:

for (var c = 0; c < this.childNodes.length; c++) {
    if (this.childNodes[c] != event.target) {
        this.removeChild(this.childNodes[c--]);
    }
}


Uploading multiple files to server with PHP and JavaScript I wrote some code to upload multiple files to a server with PHP and JavaScript. I'm happy with the form and JavaScript, below.

<form method="post" enctype="multipart/form-data">
    <input type="file" name="files[]" multiple>
    <input type="submit" value="Upload File" name="submit">
</form>

<script>
    const url = 'process.php';
    const submit = document.querySelector('[type=submit]');

    submit.addEventListener('click', e => {
        e.preventDefault();

        const files = document.querySelector('[type=file]').files;
        const formData = new FormData();

        for (let i = 0; i < files.length; i++) {
            let file = files[i];

            formData.append('files[]', file);
        }

        fetch(url, {
            method: 'POST',
            body: formData
        }).then(response => {
            console.log(response);
        });
    });
</script>

I would appreciate a review of the PHP for handling the $_POST request. I tried to make it as simple as possible, but I'm not sure if there are any major security concerns I'm overlooking.

<?php
if ($_SERVER["REQUEST_METHOD"] === 'POST') {
    if (isset($_FILES['files'])) {
        $errors = [];
        $path = 'uploads/';
        $extensions = ['jpg', 'jpeg', 'png', 'gif', 'html', 'txt', 'pdf', 'doc'];

        $allFiles = count($_FILES['files']['tmp_name']);

        for ($i = 0; $i < $allFiles; $i++) {
            try {
                $file_name = $_FILES['files']['name'][$i];
                $file_tmp = $_FILES['files']['tmp_name'][$i];
                $file_type = $_FILES['files']['type'][$i];
                $file_ext = strtolower(end(explode('.', $_FILES['files']['name'][$i])));

                $file = $path . $file_name;

                if (!in_array($file_ext, $extensions)) {
                    throw new Exception('Extension not allowed: ' . $file_name . ' ' . $file_type . "\n");
                }

                if ($file_size > 2097152) {
                    // 2MB
                    throw new Exception('File size exceeds limit: ' . $file_name . ' ' . $file_type. "\n");
                }
                move_uploaded_file($file_tmp, $file);
            } catch (Exception $e) {
                error_log($e->getMessage(), 3, '/path/to/error_log');
            }
        }
    }
}

The approach
In the beginning of this answer I would like to make a distinct note which is a bit off the actual code review.
You need to change your priorities in writing a code. For the moment you are focused  more on the SEO part, trying to provide PHP users with reference answers to common questions. That's a good goal per se, either for the users and for your own good, but it shouldn't overshadow the main purpose of the code - to perform a certain task. In short, you need more practice with using a code you share with other people. I would even say that your intent should be first to solve the actual problem you have at hand and then share your experience, not just decide to write a reference answer out of the blue.
I took a liberty to make this note because it makes the actual impact on the code you write, particularly in regard of the error handling.
Error handling
There are two kinds of errors, "business logic errors" that aren't actually errors but rather regular cases handled by the application, and "real" errors that occur unexpectedly and have to be fixed by a programmer.
Such errors like 'Extension not allowed' or 'File size exceeds limit' are not real errors. That's just business logic-related conditions, whose results have to be conveyed back to the user in order to let them to fix the issue on their side. But you are bluntly catching them and writing into error log where they hardly help anyone. I assume this logic is the direct consequence of your approach. Were you try to use your own code in a real life application, this flawed error handling would become immediately apparent.
So in order fo fix this issue first create a distinct kind of exception
class FileUploadUserException extends Exception {}

it will let you to catch only this particular kind of exception:
       } catch (FileUploadUserException $e) {
            $errors[] = $e->getMessage();
       }

whereas real errors have to be handled elsewhere, either in the dedicated handler or at least a global try catch around the entire application code as it makes no sense to write a repeated error_log call in every distinct chunk of code.
Another approach would be to avoid exceptions at all and use that $errors array you were intended to use at first.
Security
I would remove htm files from the list as it's a clear XSS attack on your own site.
I would also consider not using the original filenames as they would create a real mess with all sorts of weird characters and possible overwrite each other. The simplest solution would be just md5() from the file contents.
Usability
There is one simple trick that could greatly simplify the handling code. Instead of naming your field name as files[], make them enumerated, like files1, files2, ...
it will let you to handle them the most natural way using foreach, eliminating the need of count() call, a counter and even distinct variables to hold certain values from $_FILES array:
$extensions = ['jpg', 'jpeg', 'png', 'gif', 'html', 'txt', 'pdf', 'doc'];

foreach ($_FILES as $file) {
    $file_ext = strtolower(end(explode('.', $file['name'])));
    $destination = $path . $file['name'];
    ...
    move_uploaded_file($file['tmp_name'], $destination);
}


Compute the square root of a positive integer using binary search The requirement is to find the square root of a positive integer using binary search and the math property that square root of a number n is between 0 and n/2, and the required answer is "floored", meaning mySqrt(8) is to return 2.

Please comment on the efficiency, and if possible, the loop invariants in terms of correctness:

class Solution(object):
    def mySqrt(self, x):
        """
        :type x: int
        :rtype: int

        Loop invariant:

            The answer is always in the range [low, high] inclusive,
            except possibly:

                1) low == high == mid, and mid * mid == x and
                   any of low, high, or mid can be returned as the answer.

                2) if there is no exact answer and the floor is to be
                   returned, then low > high by 1. Since sq != x,
                   so either low or high is set inside the loop.

                   If low gets set and gets pushed up, it is pushed up too much.
                   So when low > high by 1, low - 1 is the answer and it is the same
                   as high, because low > high by 1.

                   If high gets set and gets pushed down, high can be
                   the correct answer. When low > high, it is by 1,
                   and high is the correct floor value to be returned.
                   (since there is no perfect square root and the floor is required)

            0 <= low <= answer <= high <= n//2 + 1

                where answer is floor(sqrt(x)) to be found,
                except if low > high and the loop will exit.

            Each loop iteration always makes the range smaller.

        If the range is empty at the end, low is > high by 1, and high is
            the correct floored value, and low is the ceiling value, so high is returned.
        """

        low = 0;
        high = x//2 + 1;

        while (low <= high):
            mid = low + (high - low) // 2;
            sq = mid * mid;
            if (sq == x):
                return mid;
            elif (sq > x):
                high = mid - 1;  # sq exceeds target, so mid cannot be the answer floored, but when high is set to mid - 1, then it can be the answer
            else:
                low = mid + 1;   # (here sq < x, and mid might be the answer floored, so when low is set to mid + 1, then low might be too big, while high is correct)

        return high;

Your comments on  the elif / else part are too long to be just after the statements
Don't use semicolons (;) in Python.
This is a refactored version of the code

import math

class Solution(object):
    def mySqrt(self, x):
        """
        :type x: int
        :rtype: int

        Returns floor(sqrt(x))
        """
        low = 0
        high = x//2 + 1

        """
        It is proved that 0 <= sqrt(x) <= x/2, so
        we run a dichotomic in [0, x/2] to find floor(sqrt(x))

        Loop analysis:
        * Initialization: low = 0 and high = x/2 + 1
        * Termination: |high-low| is reduced each iteration,
          as shown in lines high = mid - 1 and low = mid + 1.
        * Invariant: low <= floor(sqrt(x)) <= high.
          Let mid be (low + high)/2.
            - If mid^2 <= x < (mid+1)^2,
              then mid is floor(sqrt(x)) and just return it.
            - If mid^2 > x, search for values smaller than mid.
            - Otherwise, if mid^2 < x, search within higher values.
        """
        while (low <= high):
            mid = (low + high) // 2
            sq = mid * mid
            sq_next = (mid+1)*(mid+1)
            if (sq <= x < sq_next):
                return mid
            elif (sq > x):
                high = mid - 1
            else:
                low = mid + 1

for i in range(1, 26):
  assert(math.floor(math.sqrt(i)) == Solution().mySqrt(i))


Selection Sort algorithm in Java I'm relatively young in Java, and am familiar with only the basics of the language, yet. I've started studying data structures, and I've made an attempt to create a solution for the selection sort algorithm. After many attempts with errors and exceptions, I've managed to come up with what looks like a working solution.

I realize that having the swap implemented as a separate method call in the sort function causes some additional overhead, instead of having the swap directly in the loop. But this version helps me follow along the logic clearly, as I'm not that experienced yet.

 package Chap_3;

/**
 * Created by user1 on 8/7/15.
 * my selection sort version
 */

class MySelSort {

    //reference to array
    private int[] arr;
    //num. of elements
    private int n;

    //constructor
    public MySelSort(int max) {
        //create array with size
        arr = new int[max];
        //set current items to 0
        n = 0;
    }

    //--------------------------------------------------
    //insert()
    public void insert(int key) {

        //insert key to relevant place in array
        arr[n] = key;
        //increase size of array
        n++;
    }

    //--------------------------------------------------
    //display()
    public void display() {
        System.out.println("The items in the array are: ");
        for(int i = 0; i < n; i++) {
            System.out.print(arr[i] + " ");
        }
        System.out.println();
    }

    //-------------------------------------------------
    //sort() using selection sort
    public void selSort() {
        int in, out;
        //int min = 0;
        for(out = 0; out < n - 1; out++) {
            int min = out;
            for(in = out+1; in < n; in++) {

                if(arr[min] > arr[in]) {
                    min = in;
                }

            }

            if(arr[out] > arr[min]) {
                swap(out, min);
            }
        }
    }

    //------------------------------------------------
    //swap()
    public void swap(int one, int two) {
        int temp = arr[one];
        arr[one] = arr[two];
        arr[two] = temp;
    }
}

public class MySelSortApp {

    public static void main(String[] args) {
        //create array
        MySelSort sel = new MySelSort(100);

        //insert 8 items
        sel.insert(40);
        sel.insert(60);
        sel.insert(90);
        sel.insert(10);
        sel.insert(80);
        sel.insert(70);
        sel.insert(20);
        sel.insert(50);

        //display items
        sel.display();

        //sort items
        sel.selSort();

        //display again
        sel.display();

    }
}

Your logic seems fine to me, but bear in mind I'm not very experienced in that either, so perhaps someone more experienced with algorithms will find some ways to improve.

Documentation & Naming

It's clear from your code that your intention is to have well-documented code (with is a great habit). It's the first thing that I noticed looking at your code so that's why I am mentioning it specifically.

It feels a bit like your documentation is overcompensating for things that could be just named a bit more clearly. For example:

/**
 * Created by user1 on 8/7/15.
 * my selection sort version
 */

class MySelSort {

    //reference to array
    private int[] arr;
    //num. of elements
    private int n;

I think something like this would be better:

class SelectionSort {

    private int[] arrayToBeSorted;
    private int numberOfElements;

It's a bit more wordy, but completely unambiguous on the other hand. I would also encourage to use Javadoc.

While something like this is OK:

//constructor
public MySelSort(int max) {
    //create array with size
    arr = new int[max];
    //set current items to 0
    n = 0;
}

This would be more like what you see in bigger Java programs:

/**
 * Constructor.
 * <p>
 * Creates an array of integers with a maximum size.
 * The beginning index of the array is initialized to 0.
 *
 * @param maxSize  The maximum size of the array, not null
 */
public SelectionSort(int maxSize) {
    arrayToBeSorted = new int[maxSize];
    numberOfElements = 0;
}

This method's parameter naming is misleading:

//--------------------------------------------------
//insert()
public void insert(int key) {

    //insert key to relevant place in array
    arr[n] = key;
    //increase size of array
    n++;
}

What you call the key is actually rather the value to be inserted at the current key (or index). This would be a bit easier to spot with better naming. Going back to the top, we could name n to elementNumber instead of numberOfElements (which I first named only accordingly to your comment, which again was a bit misleading).

/**
 * Inserts a value at a given key in the array.
 *
 * @param value  The value to be inserted, not null
 */
public void insert(int value) {
    arrayToBeSorted[elementNumber] = value;
    elementNumber++;
}

And so on with documentation and naming. When you get to the end, you get a much clearer picture of what is happening, with no comments needed at all in your main class:

public class SelectionSortApp {

    public static void main(String[] args) {

        SelectionSort arrayToBeSorted = new SelectionSort(100);

        arrayToBeSorted.insert(40);
        arrayToBeSorted.insert(60);
        // ...

        System.out.println("Prior to sorting...");
        arrayToBeSorted.display();

        arrayToBeSorted.selSort();

        System.out.println("After sorting...");
        arrayToBeSorted.display();

    }
}


Stack implementation using linked-list typedef struct Stack {
   char v;
   int x;
   int y;
   struct Stack* next;
  } Stack;

Above is my stack struct.

Stack* helper (char val, int x, int y)
  {
    Stack* node = (Stack*)malloc(sizeof(Stack));
    node->v = val;
    node->x = x;
    node->y = y;
    return node;
  }
  void push (Stack** top,char val,int x,int y)
  {
    Stack* node = helper(val,x,y);
    node->next = *top;
    *top = node;
    printf("%c pushed to stack\n",val);
  }
  void pop (Stack** top,char *val,int *w,int *h)
  {
    if (checkempty(*top))
      return;
    Stack* temp = *top;
    *top = (*top)->next;
    *val = temp->v;
    *w = temp->x;
    *h = temp->y;
    free(temp);
  }
  int checkempty(Stack* top)
  {
    return !top;
  }

Above are my push and pop functions.
I was wondering if there are better ways to do this ? Or is what I am doing the standard way ?

Stack* helper (char val, int x, int y)
  {

Your indentation and whitespace are kind of funky; I recommend looking at what some popular open-source code on GitHub does, and trying to copy them as closely as possible. Or, just run your code through a formatter such as clang-format.

Stack *helper(char val, int x, int y)
{

This function is only ever used in one place, so why do you have it? Right now it's kind of unsafe, because helper returns a Stack * which:

must be freed by the caller to avoid memory leaks, and
has uninitialized garbage in its next field.

If you inlined it into push, you'd have a complete unit of work, without either of those dangerous sharp edges.

Look into proper library organization. You should have a .h file that defines struct Stack and declares push and pop, and a .c file with the implementations.

You should probably name the functions something like stack_push and stack_pop, since push and pop are likely to be common names; e.g. you might later want to write a struct Queue with a queue_push and queue_pop. This kind of "manual namespacing" is common in C libraries.

Of course push shouldn't call printf (there's no "I" in "TEAM" and there's no "printf" in "push data onto a stack").

In pop's argument list, you use w and h where you meant x and y.

pop has interesting behavior on an empty stack: it just returns without initializing *x and *y. This is a problem for the caller, because the caller doesn't have any indication that anything went wrong! You should probably make pop return a bool to indicate whether it was successful.

Compare a pointer for null with (top == NULL), not !top. Clarity counts!

Here's an idea if you want to push yourself harder: Right now you're passing x, y, and value as individual arguments to push, and retrieving them individually from pop. Package them up into a struct!

Here's the struct definitions and function declarations for a different stack implementation. Can you see how to implement these functions?

struct Data { int x, y, value; };

struct DataStack { struct Data data; struct DataStack *next; }

struct DataStack *datastack_push(struct DataStack *, struct Data);
struct DataStack *datastack_pop(struct DataStack *);
struct Data datastack_top(const struct DataStack *);
bool datastack_empty(const struct DataStack *);


Finding median of sub arrays taking too much time I'm trying to solve a programming challenge where I have to find the median of given subarrays of an array. I have used std::vector's n_th element for this. But the evaluator fails some test cases as "time limit exceeded".

#include <iostream>
#include <bits/stdc++.h>

using namespace std;

/*
In
6
2 4 5 3 1 6
3
1 6
2 4
3 3

Out
3
4
5

*/

int get_ceil(int length)
{
    if (length % 2 == 0)
    {
        return length/2;
    }
    else
    {
        return (length+1)/2;
    }

}

int get_median_of_subarray(std::vector<int> & full_array, int lo, int hi)
{

    std::vector<int>::const_iterator begin = full_array.begin() + lo;
    std::vector<int>::const_iterator end = full_array.begin() + hi + 1;
    std::vector<int> sub_array(begin, end);

    int median_pos = get_ceil(hi - lo + 1);
    std::nth_element (sub_array.begin(), sub_array.begin() + median_pos -1, sub_array.end());
    return sub_array[median_pos - 1];
}

int main()
{
    ios_base::sync_with_stdio(false);

    int array_size = 0;
    std::vector<int> input_array;

    cin >> array_size;

    for(int i = 0; i < array_size; i++)
    {
        int x;
        cin >> x;
        input_array.push_back(x);
    }

    int query_size = 0;
    cin >> query_size;

    int lo = 0;
    int hi = 0;

    for(int i = 0; i < query_size; i++)
    {
        cin >> lo >> hi;
        std::cout << get_median_of_subarray(input_array, --lo, --hi) << std::endl;
    }

    return 0;
}

Not related to performance, but never ever do this:

#include <bits/stdc++.h>

using namespace std;

For reference see these Q&A at Stack Overflow:

Why should I not #include <bits/stdc++.h>?
Why is “using namespace std;” considered bad practice?


Computing leading set (compiler design) I have written this program to compute the leading set of the following productions

E->E+T
E->T
T->T*F
T->F
F->(E)
F->#

Here identifier is taken as '#'

#include<stdio.h>
#include<conio.h>
#include<ctype.h>
struct leadT
{
    int n;
    char lead[5];
};
struct leadT leading[7];
char ip[7][7];
int col,cnt=0;
char Sym[5]={ '+','(',')','*','#','i' };
int isSym(char);
void findLeading(char,int);
void leadTabOp(char,int);
void main()
{
    int i,j,c;
    char b[4];
    for(i=0;i<=5;i++)
    {
        scanf("%s",&ip[i]);
    }
    printf("\n");
     for(i=0;i<6;i++)
       {    c=0;
        for(j=0;j<i+1;j++)
        {
            if(ip[i][0] == b[j])
            {
                c=1;
                break;
            }
        }
        if(c !=1)
        {
          b[cnt] = ip[i][0];
          cnt++;
        }

        }
    for(i=cnt-1;i>=0;i--)
    {   leading[i].n=0;
        col=1;
        leading[i].lead[0]=b[i];
        findLeading(b[i],i);
    }

    printf("\n");

    for(i=0;i<cnt;i++)
    {
        for(j=0;j<=leading[i].n;j++)
        {
            if(j==0)
            {
                    printf("leading(%c) = { ",leading[i].lead[j]);
             }
           else if(j == leading[i].n)
           {
               printf("%c ",leading[i].lead[j]);
           }
           else
           {
               printf("%c,",leading[i].lead[j]);
           }

        }
        printf("}");
        printf("\n");
    }

}
int isSym(char ip)
{
    int i;
    for(i=0;i<5;i++)
    {
        if(ip==Sym[i])
        {
            return 1;
        }
    }

    return 0;
}
void findLeading(char p,int row)
{
    int i;
    for(i=5;i>=0;i--)
    {
        if(p == ip[i][0])
        {
            if((ip[i][2] == '>') && (ip[i][4]=='\0'))
            {
                if(isupper(ip[i][3]))
                {
                                leadTabOp(ip[i][3],row);
                }
                else
                {
                                leading[row].lead[col]=ip[i][3];
                                leading[row].n++;
                                col++;
                }
            }
            if( (ip[i][2] == '>') && ( isupper(ip[i][3]) ) && ( isSym( ip[i][4] ) ) )
            {
                            leading[row].lead[col]=ip[i][4];
                            col++;
                            leading[row].n++;
            }
            if( ( ip[i][2] == '>') && ( isSym( ip[i][3] ) ) && ( isupper( ip[i][4] ) ) )
            {
                            leading[row].lead[col]=ip[i][3];
                            col++;
                                leading[row].n++;
            }
        }
    }
}
void leadTabOp(char p,int row)
{   int i,j;
    for(i=0;i<cnt;i++)
    {
        if(p == leading[i].lead[0])
        {
            for(j=1;j<=leading[i].n;j++)
            {
                leading[row].lead[col]=leading[i].lead[j];
                col++;
                leading[row].n++;
            }
        }
    }
}

Please review this.

You should structure your code a bit better and improve your naming conventions.

The first loop in main() apparently scans 6 strings into ip. From that I might deduce that ip is supposed to hold your input. So name it input or even inputProductions. Current IDEs support code completion so not using longer names for the sake of less typing is not a valid argument.

You could also put that code into a separate function readInput(char **result). Later you can adjust how you read your input without changing main.

Then you have a couple of nested loops which compute some values to be stored in b and a value c. Using i and j as loop counter is accepted practice so that's ok but what are b and c supposed to represent? Give them names which express their meaning.

Also your outer loop counter i can go up to 5 and j < i + 1 means j can go up to 5 as well. b is only an array with 4 elements, so the last index you are allowed to access is 3 and b[j] would be out of bounds for any j > 3.

If that nested for loop is supposed to do some pre-processing then put it into a function and give it a sensible name which expresses what it is doing.

You have a variable called cnt - what exactly is it that you are counting? Name the variable accordingly.

Your functions access global variables like cnt or ip. You should pass all data those functions require as parameters. There is not need to create implicit dependencies.

Organize your code so its concerns are separated: input parsing, data processing, output. Your main function should read something like this:

int main()
{
    char input[7][7];
    char b[4];
    int count;

    if (!readInput(input))
    {
         puts("Failed to read input.");
         return -1;
    }

    count = preProcessInput(input, b);
    if (count < 0)
    {
        puts("Failed to pre-process input");
        return -2;
    }

    if (!findLeadingSet(input, b, leadingSet, count))
    {
        puts("Failed to process input");
        return -3;
    }

    printLeadingSet(leadingSet);

    return 0;
}

This makes the structure immediately clear and explains what the steps are without having to read all the code and trying to understand it (apparently preProcessInput should be named as to what it is actually pre-processing).


E-commerce scraper needs shortening How would I go about shortening this code?
I'm sure there's a smarter way to write the code.
It's working fine, it's just ugly.

It is a way to scrape the number of items on an e-commerce site.

import requests
from bs4 import BeautifulSoup
import re

url1 = "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element"
url2 = "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element"
url3 = "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element"
url4 = "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element"
url5 = "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"

#res = requests.get(link.format(url1),headers={"User-Agent":"Mozilla/5.0"})

soup1 = BeautifulSoup(requests.get(url1.format(0)).text, 'lxml')
soup2 = BeautifulSoup(requests.get(url2.format(0)).text, 'lxml')
soup3 = BeautifulSoup(requests.get(url3.format(0)).text, 'lxml')
soup4 = BeautifulSoup(requests.get(url4.format(0)).text, 'lxml')
soup5 = BeautifulSoup(requests.get(url5.format(0)).text, 'lxml')

total_items1 = ''.join(re.findall(r'\d', soup1.select_one('span.products-count').text))
total_items2 = ''.join(re.findall(r'\d', soup2.select_one('span.products-count').text))
total_items3 = ''.join(re.findall(r'\d', soup3.select_one('span.products-count').text))
total_items4 = ''.join(re.findall(r'\d', soup4.select_one('span.products-count').text))
total_items5 = ''.join(re.findall(r'\d', soup5.select_one('span.products-count').text))

#categories = [tag['title'].strip() for tag in soup.select('.refinement-link[title]')

#total_items_sale1 = ''.join(re.findall(r'\d', soup1.select_one('.grid-tile .price-standard')))
#total_items_sale1
#total_items_sale1
#total_items_sale1
#total_items_sale1

#print('Categories:')

#for category in categories:
    #print('\t{}'.format(category))
print('\nTotal Charms: {}'.format(total_items1))
print('\nTotal Bracelets: {}'.format(total_items2))
print('\nTotal Rings: {}'.format(total_items3))
print('\nTotal Necklaces: {}'.format(total_items4))
print('\nTotal Earrings: {}'.format(total_items5))

If you find yourself, doing something over and over again, loops are often the answer.

Your code rewritten with a loop:

import re

import requests
from bs4 import BeautifulSoup

urls = {
    "Charms": "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element",
    "Bracelets": "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element",
    "Rings": "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element",
    "Necklaces": "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element",
    "Earrings": "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"
}

for category, url in urls.items():
    soup = BeautifulSoup(requests.get(url.format(0)).text, 'lxml')
    total_items = ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))
    print("Total {}: {}".format(category, total_items))

So what has changed? The URLs went into a dictionary where the category's name is the key. A simple list would also work here in your case. Now we can iterate over that dictionary. With for category, url in urls.items(): you get the key, i.e. the name of the category and the URL at the same time.

The loop body now does what you had written out by hand, namely getting the page, parse the content with BeautifulSoup and than again with a regex.

Since the URL came bundled with the category, we can no print all the results nicely without writing the same code over and over again.

You can also go a step further and make this into a function:

import re

import requests
from bs4 import BeautifulSoup

def get_total_items(url):
    """Parse the product count from the given pandora URL"""
    soup = BeautifulSoup(requests.get(url).text, 'lxml')
    return ''.join(re.findall(r'\d', soup.select_one('span.products-count').text))

if __name__ == "__main__":
    urls = {
        "Charms": "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element",
        "Bracelets": "https://us.pandora.net/en/bracelets/?sz=30&start={}&format=page-element",
        "Rings": "https://us.pandora.net/en/rings/?sz=30&start={}&format=page-element",
        "Necklaces": "https://us.pandora.net/en/necklaces/?sz=30&start={}&format=page-element",
        "Earrings": "https://us.pandora.net/en/earrings/?sz=30&start={}&format=page-element"
    }

    for category, url in urls.items():
        print("Total {}: {}".format(category, get_total_items(url.format(0))))

Now the intermediate variable soup is inside the function and does not clutter the global namespace. You could also now easily change the implementation of get_total_items(...) without the need to adapt the "application" code.

I also added the infamous if __name__ == "__main__":, which is basically Python's way to tell someone looking at the script: "This piece of code is supposed to be run if you use this as a script." Have a look at the official documentation or this Stack Overflow post if you want to learn more.

A more subtle change: I reorderd the imports to follow the recommendation of the official Style Guide for Python Code (often just PEP8), which is to have imports from the standard libraries first, then a blank line, and then all the third party libraries.

In general PEP8 is often worth a read, especially if you are not yet familiar with Python.


Using Scrapy/Xpath to scrape ESPN for football (soccer) commentaries The class takes an input of a game-id from the ESPN soccer website. The code then has to grab the commentary, process that and also grab the player names/ids and create a small dictionary of those.

I understand the premise of OOP when shown in basic examples but I cannot get my head around using it in actual programs/applications. I have tried here, but to me it seems to not achieve anything extra (so I must be doing it wrong).

I end up with a list of lists containing each of the key events (corners, shots etc) and a dictionary of players, so it works.  But it just seems so inelegant and I would love some tips on improving it (however small/focused they are).

import urllib2
from scrapy.selector import HtmlXPathSelector
import re
import codecs
import timeit
start = timeit.default_timer()

class game:
    def __init__(self,game_id):
        self.game_id = game_id
        self.comms_url = 'http://espnfc.com/uk/en/gamecast/%d/gamecast.html?soccernet=true&cc=5739' % self.game_id
        self.data_text = urllib2.urlopen(self.comms_url).read()
        self.corners = []
        self.shots = []
        self.fouls = []
        self.shots_on_target = []
        self.offsides = []
        self.goals = []
        self.extractCommentaries()
        self.extractPlayers()
        self.extractTeams()
        self.findAction('^Corner', self.corners)
        self.findAction('^Foul by', self.fouls)
        self.findAction('^Attempt', self.shots)
        self.findAction('^Attempt saved', self.shots_on_target)
        self.findAction('^Offside', self.offsides)
        self.findAction('^Goal!', self.goals)
    def extractCommentaries(self):
        self.hxs = HtmlXPathSelector(text=self.data_text)
        self.match_comments = self.hxs.select("//div[@id='convo-window']/ul[@id='convo-list']/li/div[@class='comment']/p/text()")
        self.match_timestamps = self.hxs.select("//div[@id='convo-window']/ul[@id='convo-list']/li/div[@class='timestamp']/p/text()")
        self.events = zip(self.match_timestamps.extract(), self.match_comments.extract())
        self.cleanclean = []
        for time,event in self.events:
            time = re.search('\d+', time)
            if time:
                time = time.group()
                time = int(time)
                self.cleanclean.append((time,event))
        self.dumdum = range(len(self.events))
        self.clean_events = {}
        for key, event in enumerate(self.cleanclean):
            self.clean_events[key] = event
    def extractPlayers(self):
        self.player_names = self.hxs.select("//table[@class='stat-table']/tbody//a/text()")
        self.player_urls = self.hxs.select("//table[@class='stat-table']/tbody//a/@href")
        self.player_ids = []
        for url in self.player_urls.extract():
            digit = []
            for s in url:
                if s.isdigit():
                    digit.append(s)
            self.placeholder = ''.join(digit)
            self.placeholder = int(self.placeholder)
            self.player_ids.append(self.placeholder)
        self.players = dict(zip(self.player_ids, self.player_names.extract()))
    def extractTeams(self):
        self.dummy = ['home','away']
        self.zero = [0,0]
        self.team_names = self.hxs.select("//li[@class='country']/h2/text()")
        self.teams = dict(zip(self.dummy,self.team_names.extract()))
    def findAction(self, keyword, storage):
        for key, value in self.clean_events.iteritems():
            action = re.search(keyword, value[1])
            if action:
                for ha, team in self.teams.iteritems():
                    if keyword == '^Goal!':
                        split_up_action = value[1].split('.')
                        found = re.search(team, split_up_action[1])
                    else:
                        found = re.search(team, value[1])
                    if found:
                        temp_holder = [value[0], team, self.game_id]
                        storage.append(temp_holder)

a = game()
stop = timeit.default_timer()

print stop - start

There are no docstrings, so a user of your class is left wondering which methods could be useful to call. In fact, after studying the code, it looks like all the methods are meant to be called by __init__, in a specific order. Makes me wonder if this should be a class at all.
You set very many instance attributes, even a self.dummy, but use very few outside the method where they are set. Most of them should be local variables instead. Again, I'm thinking that you don't need a class -- all the methods could be simple functions that take one or two parameters instead.
The purpose of the class seems to be to initialize six lists, eg. self.corners. So, to get an overview of the game, one has to access six variables. It would be better to use a single data structure that can be easily looped over and queried. Perhaps a dictionary with keys like "corners" could suit you, but I'm thinking that most useful would be to keep the events in a chronological list and provide a function to filter that list by type of event.


Grouping a CSV by column I've started writing a small CSV parser class as I needed a method to group a CSV file by a given column.

class CSVParser {

    private $FileName;
    private $FileHandle;
    private $HasHeaderRow = false;
    private $RowsToIgnore = array();

    public function __construct ($FileName, $Mode = "r") {
        $this->FileName = $FileName;

        if (($this->FileHandle = fopen($this->FileName, $Mode)) === FALSE)
            return false; // that's all your getting for now.
    }

    public function hasHeaderRow ($Flag = false) {
        $this->HasHeaderRow = (bool) $Flag;
    }

    public function ignoreRows (array $Rows) {
        $this->RowsToIgnore = $Rows;
    }

    public function groupByColumn ($GroupColumn = 0, $RowLength = 1000, $Seperator = ",") {

        $CurrentRow = -1;
        $Classes = array();

        while (($Data = fgetcsv($this->FileHandle, $RowLength, $Seperator)) !== FALSE) {
            $CurrentRow++;

            // does the csv contain a header row?
            if ($this->HasHeaderRow)
                if ($CurrentRow < 1) continue;

            // are there any rows to ignore?
            if (in_array($CurrentRow, $this->RowsToIgnore))
                continue;

            // line 45
            $Classes[$Data[$GroupColumn]][] = array_filter($Data, function ($var) use ($GroupColumn, $Data) {
                return $var !== $Data[$GroupColumn];
            });
        }

        return $Classes;
    }

    public function __destruct() {
        fclose($this->FileHandle);
    }
}

// usage
$csv = new CSVParser("mycsv.csv");
$csv->hasHeaderRow(true);

$classes = $csv->groupByColumn(0));

I'm a little unsure about the removal of the group by column from the array entries on line 45.

Can anyone see a better method of achieving this?

Ok, before I set of, there are 3 things to keep in mind here: Code review has to be tough, to be good, so if this hurts your feelings - sorry, but I'm trying to help.
The second thing is: Please, please, please try to adhere to the coding standards as defined by PHP-FIG (google them). Most, if not all, major players adhere to them, and you should, too. Sure, they're not official (yet), but there are no official standards... the more people adhere to a given set of unofficial rules, the more likely it is they become standard.
And finally: Please, document your code... you add some comments here and there, but not nearly as much as required... the main method of your class will bite you in the bum if you leave it be for a couple of months, and then decide to add something or debug it a little...

Now, to business. As I usually do, I'll be going through your code bit by bit, suggesting and commenting along the way

class CSVParser
{//next line, PHP-FIG style

    /**
     * @var string
     **/
    protected $fileName = null;//camelCase plz
    /**
     * @var resource <file handle>
     **/
    protected $fileHandle = null;
    /**
     * @var bool
     **/
    protected $hasHeaderRow = false;
    /**
     * @var array<mixed>
     **/
    protected $rowsToIgnore = array();

Now, I'd initialize the properties to null, simply because it's a good habit if you're ever in the situation where you find yourself switching from one language to another.
I'd also add doc-blocks per property to document what types these properties will be assigned
I'd also strongly suggest defining the properties as protected, since a class that reads files should be quite generic, and easy to extend from. If you extend, therefore, private is not always very useful...

Now, the methods:

public function __construct ($FileName, $Mode = "r")
{
    $this->FileName = $FileName;
    if (($this->FileHandle = fopen($this->FileName, $Mode)) === FALSE)
        return false;///????
}

This one is a big problem: At no point are you checking the type of any of the arguments that you're receiving. Is $FileName even a string? is it null?
Worse still: $Mode is the mode in which you want to open the file... fine, there's a limited amount of valid strings that can be passed here. For the ease of the user, and to improve readability of the code that uses your class, I'd create constants for the $Mode parameter.
Then: return false? Seriously? From a constructor? A constructor returns an instance of the given class. All other return statements are pointless, they are ignored. If this were to work, this requires the user to write code like this:

$reader = new CSVParser($file, 'r');
if ($reader === false)
{
    //handle error
}

Which is just messy. If you must, throw an exception, but better yet: allow the user to set the file that needs to be read later on. That way, the user can create the instance, and re-use it throughout his code. Here's what I'd write:

const OPEN_READ = 'r';
const OPEN_WRITE = 'w';
const OPEN_APPEND = 'a';
const OPEN_BIN_READ = 'rb';
const OPEN_BIN_WRITE = 'wb';
//and so on
public function __construct($file = null, $mode = self::OPEN_READ)
{
    if ($file)
    {
        if (is_string($file))
        {
            $this->fileName = $file;
            if (!($this->fileHandle = fopen($file, $mode))
                throw new RuntimeException('Opening file '.$file.' failed');
            return $this;//success
        }
        throw new InvalidArgumentException('File argument is not a string');
    }
    //nothing passed => just fine...
}

But this is far from perfect: you should provide support for the SplFileInfo class (users passing an instance of SplFileInfo should work, too) and perhaps allow them to pass a file handle they opened already.
What you've also failed to do is allow the user to set the hasHeaderRow in the constructor. Now, I'm no big fan of constructors requiring too many arguments, so what I'd probably write is something like:

public function __construct($mixed)
{
    if ($mixed instanceof SplFileInfo) return $this->openSplFile($mixed);
    if (is_resource($mixed)) return $this->useFileHandle($mixed);
    //is string, array, some other instance...
}

But that's what I would do, not what you must do.
Onwards:

public function hasHeaderRow ($flag = false)
{
    $this->hasHeaderRow = (bool) $flag;
    return $this;
}

public function ignoreRows (array $Rows)
{
    $this->rowsToIgnore = $Rows;
    return $this;
}

I've added the return $this; statements simply because I like chainable API's. Now the user can write:

$instance->hasHeaderRow(true)
        ->ignoreRows($someArray);

Other than that, the hasHeaderRow is fine, but the ignoreRows method is, to my eye a tad vague, and I don't like how it doesn't allow me to merge in new rows to the ignore array. Perhaps consider writing

public function ignoreRows(array $rows, $merge = false)
{
    if ($merge){
        $this->rowsToIgnore = array_merge(
            $this->rowsToIgnore,
            $rows
        );
        return $this;
    }
    $this->rowsToIgnore = $rows;
    return $this;
}

Now, I'll skip the groupByColumn method for now, because there's a lot of things I to review there, so first, let's look at this:

public function __destruct()
{
    fclose($this->fileHandle);
}

Nice, that's a sensible use of the constructor method. But what about any of the other magic methods? __sleep and __clone to name just 2? All those methods, if a file-handle is concerned should probably be implemented to close the file, or (__clone for example) should return false, or throw an exception.
I've also hinted at this in the beginning of this answer: what if I were to write some app that had to parse several files? Using your class, I'd have to create a new instance for each file. That's just adding pointless overhead. I'd propose you add a openFile method, that works similarly to the constructor (ideally accepting SplFileInfo and the like, too). This openFile method, then could look like this:

public function openFile($mixed)
{
    if ($this->fileHandle !== null && is_resource($this->fileHandle))
    {
        $this->fileName = null; //<<this is never used throughout your code!
        fclose($this->fileHandle);
        //and so on, re-initialize all properties
    }
    //add same if's and else's as in __constructor here
    return $this;
}

Now, I'll post this answer because it's gotten quite lengthy already. Meanwhile, I'll examing the groupByColumn method to actually get to grips with it... it's short, sure, but would you be able to maintain it 6 months from now? If yes: right... ;-P

Now the main function... not only is it a nightmare to read, it again fails to validate any of the params it received, and it contains bugs!
A line by line once-over:

public function groupByColumn ($groupColumn = 0, $rowLength = 1000, $seperator = ",")
{
    $currentRow = -1;//why, you don't need this
    $classes = array();//what do you mean "classes"
    while ($data = fgetcsv($this->fileHandle, $rowLength, $seperator))
    {//no need for the !== false
        $currentRow++;//
        if ($this->hasHeaderRow)//Why check this on each iteration?
            if ($currentRow < 1) continue;//ouch continue
        if (in_array($currentRow, $this->RowsToIgnore))
                continue;//ouch again
        //maintain this... good luck
        $classes[$data[$groupColumn]][] = array_filter(//should issue notice
            $data,
            function ($var) use ($groupColumn, $data)
            {//think Gloves (you're over-complicating)
                return $var !== $data[$groupColumn];
            }
        );
    }
    return $classes;
}

Now without too many words, I'll just re-write the same code to do what -I think- it is you're trying to acchieve

public function groupByColumn ($groupColumn = 0, $rowLength = 1000, $seperator = ",")
{
    $lines = array();
    if ($this->hasHeaderRow)
        fgetcsv($this->fileHandle,
                $rowLength,
                $separator);
    $count = 0;
    while ($data = fgetcsv($this->fileHandle, $rowLength, $seperator))
    {
        if (!in_array(++$count, $this->rowsToIgnore))
        {
            $idx = $data[$groupByColumn];
            unset($data[$groupByColumn]);
            if (!isset($lines[$idx])) $lines[$idx] = array();
            $lines[$idx][] = $data;
        }
    }
    return $lines;
}

Now not only is this code smaller, it's safer, it is also not producing notices, less resource hungry, and it is, IMO, a lot easier to read. It's not difficult to understand the code, but let me back my claims up:

safer: your array_filter was removing duplicate Values but if I were to group by a "column" that contained a number, and my data looked like this: 123,4364, foobar,123, I could be removing 2 values from the array. You only need to remove 1: the value associated with the group-by column, of which you have the offset. Using unset is faster, and safer
No notices: well, for each new $data[$groupByColumn] value, you need to create a new array. You never do so explicitly. $classes[$data[$groupByColumn]][]<-- the latter just assumes there is an array. If it doesn't exist, PHP will create one for you, but it will produce a notice, too, which slows you down.
less resources required: simply because I'm calling less functions and not using a lambda function (which, in PHP is an instance of the Closure class anyway). You create an instance on each iteration... calling a constructor each time... I use unset + the index of an array, which is a simple HashTable lookup internally.
Sure, PHP will probably optimize this, and create a single instance of Closure but this instance will be GC'ed once this method returns. So you're still creating instances each time this method gets called.

Well, that's my review, hope you find a thing or 2 useful in this verbal diarrhea. I've written this all in one go, off the top of my head, and I didn't check any code I posted here, so there might be some syntax errors (and grammar errors) in here, but hey...


Brute force password cracker I wrote this script for a proof of concept JavaScript password cracker:

var charset = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~";
function crack(value){
    function toRadix(N,radix) {
        var HexN = "",
            Q = Math.floor(Math.abs(N)),
            R,
            strv = charset,
            radix = strv.length;
        while (true) {
            R = Q % radix;
            HexN = strv.charAt(R) + HexN;
            Q = (Q - R) / radix;
            if (Q == 0)
                break;
        };
        return ((N < 0) ? "-" + HexN : HexN);
    };
    var start = (new Date()) * 1,
        cracked = false,
        index = 0;
    while(!cracked){
        if(toRadix(index) == value)
            cracked = true;
        else
            index++;
    };
    alert(((new Date()) * 1) - start);
};

This script works fine, but it is slow.

What would be the best way to speed up this algorithm?

Expectation Setting
Your algorithm is an incrementing index, which you then convert in to the radix of your charset.
Your charset is what, 95 characters?
So, there are the following possible permutations for passwords:
1 char -> 95
2 char -> 9025
3 char -> 857375
4 char -> 81450625
5 char -> 7737809375
6 char -> 735091890625
7 char -> 69833729609375
8 char -> 6634204312890625

OK, so, let's assume the person chooses an 8 char password you need to crack, and that it starts about half-way through your alphabet with a 'K'.
That means you will have to calculate about... 3,000,000,000,000,000 passwords before you get to the right one....
Now, let's assume your browser is super fast (like in the scale of a super-computer), and can compute 1 billion passwords each second....
It will need 3,000,000 seconds to get to the right one.... which is.... about 5 weeks (not 2.5 years as originally stated).
Now, your browser, if it is on an amazing PC, will be 1000 times slower.... so, the right way to crack this password this millennium, is to actually wait about 20 years, and then crack it then when computers are a few hundred times faster ;-)
Note, I would not expect even the fastest PC to be able to get to 5char passwords using a single-threaded execution model, in the most optimal way, to check more than 1,000,000 passwords a second, which makes 5 char passwords more than 2 minutes away....
This is one of the joys about brute-force tactics, by the way, is that, in general, the fastest way to crack a brute-force password of reasonable length, is to do nothing.... and wait for technology to get faster... and then start later, and finish sooner.
Alternate algorithm
Using recursion would be a natural way to solve the general problem of checking all solutions, but it has the problem that it checks all solutions in the wrong order (typically it solves the longest passwords first.... which would be counter-intuitive to check a bunch of 8-char passwords before you check all 7 char passwords.
So, you are left with just making the current system faster... and, that's a somewhat fruitless problem, because even halving the time would not be meaningful in most cases.
This leads on to multi-threading, which is the fastest way to accelerate the problem, but I am not sure this is available on your browser.


Moving a clock forward I am taking a Ruby class and our assignment was to write a class that takes two users inputs (1. the time, 2. the amount of minutes to move forward).  Then based on the two inputs forward the time by that amount.  I am new to Ruby so any feedback on how to improve my code would be great.

class Timeshift
  @time
  @timeshift
  @hours
  @minutes
  @additional_hours
  @am_or_pm
  @newTime

  def initialize (time, timeshift)
    @time = time
    @timeshift = timeshift
    @hours = 0
    @minutes = 0
    @additional_hours = 0
    @am_or_pm
    @newTime

  end

  def check_is_valid_time
    /^(1[0-2]|0?[1-9]):([0-5][0-9])(\s[A|P]M)\)?$/.match(@time)
  end
  private :check_is_valid_time

  def get_hours
    temp_array = @time.split(':')
    @hours = temp_array[0].to_i
  end
  private :get_hours

  def get_minutes
    temp_array = @time.split('')
    temp_minutes = temp_array[3].to_i * 10
    temp_minutes += temp_array[4].to_i
    @minutes = temp_minutes
  end
  private :get_minutes

  def get_additional_minutes
    while @timeshift >= 1440
      @timeshift -= 1440
    end
  end
  private :get_additional_minutes

  def get_am_or_pm
    temp_array = @time.split('')
    @am_or_pm = temp_array[6]
  end
  private :get_am_or_pm

  def add_minutes
    @minutes += @timeshift
    while @minutes >= 60
      @additional_hours += 1
      @minutes -= 60
    end
  end
  private :get_additional_minutes

  def add_hours
    @hours += @additional_hours
    if @hours >= 13 && @hours < 25
      @hours -= 12;
      if  @am_or_pm == 'A'
        @am_or_pm = 'P'
      else
        @am_or_pm = 'A'
      end
    elsif @hours >= 25
      @hours -= 24;
    end
  end
  private :add_hours

  def add_time ()
    if !check_is_valid_time
       @newTime = "Not a valid time entered"
    else
        get_hours
        get_minutes
        get_am_or_pm
     if @timeshift > 0
        get_additional_minutes
        add_minutes
        add_hours
      end
      @newTime = "#{@hours}:#{@minutes} #{@am_or_pm}M"
    end
     @newTime
  end
end

# 2 minutes in future
time  = "11:55 AM"
timeshift = 2
puts Timeshift.new(time, timeshift).add_time

Design

The design of this class is confusing.  Perhaps Timeshift shouldn't be a class at all, at least not like this.  This …

Timeshift.new(time, timeshift).add_time

… is problematic, because:

You have used "time shift" both as the duration to add (timeshift) and as the class representing the summation operation (Timeshift).
What does .add_time do?  Why is it called .add_time rather than, say, .add_duration?  How about just .add?
What should be the result of

t = Timeshift.new(time, timeshift)
puts t.add_time
puts t.add_time

Should it print "11:57 AM" twice, or should it print "11:57 AM" then "11:59 AM"?  There is no intuitively correct behaviour, I think.
It's not obvious, other than from thoroughly reading the code, what the inputs and outputs are supposed to be.  (time should be "HH:MM AM" format with a 12-hour clock; timeshift is the number of minutes.)  The inputs and outputs should be documented in a comment.

The class is designed to have a constructor and one public method (.add_time).  Whenever that is the case, chances are that the code shouldn't be a class at all, and should be just a function instead, like add_time("11:55 AM", 2).  (Actually, you have two public methods — .add_minutes was accidentally exposed as a public method due to a copy-and-paste error!)

Another problem is that validation of the constructor's arguments is deferred to .add_time.  Failing earlier would make debugging easier.

Bugs

> Timeshift.new('11:55 AM', 5).add_time
=> "12:0 AM"                               # Expected: "12:00 PM"

> Timeshift.new('11:55 AM', -55).add_time
=> "11:55 AM"                              # Expected: "11:00 AM"

> Timeshift.new('12:00 am', 0).add_time
=> "Not a valid time entered"              # Expected: "12:00 AM"

Implementation

The instance variables at the top (@time, @timeshift, etc.) aren't doing anything.  You don't need to "declare" instance variables like that in Ruby.  Furthermore, you're keeping way more state information than necessary, with so many variables.

It's not really idiomatic to name your methods .get_something in Ruby.  (The naming convention is .something for getters and .something=(value) for setters.)
What's more, using getter methods purely for their side-effects is unorthodox.

Clock arithmetic is much simpler if you convert everything into minutes first, then convert the result back into hours and minutes.

Suggested solutions

As a simple function:

def add_time(hh_mm_ampm, minutes)
  unless parts = /\A(1[0-2]|0[1-9]):([0-5]\d)\s*(AM|PM)\Z/i.match(hh_mm_ampm)
    raise ArgumentError.new("Not a valid time in HH:MM AM/PM format: #{hh_mm_ampm}")
  end
  hh, mm, am_pm = parts.captures

  # Sum, as minutes since midnight
  time_of_day = 60 * hh.to_i + mm.to_i + minutes
  time_of_day += (12 * 60) if am_pm.upcase == 'PM'
  time_of_day %= (24 * 60)

  # Output formatting
  am_pm = (time_of_day < 12 * 60) ? 'AM' : 'PM'
  hh = (time_of_day / 60).to_i % 12
  hh = 12 if hh == 0
  mm = time_of_day % 60
  sprintf('%02d:%02d %s', hh, mm, am_pm)
end

As a class:

class ClockTime
  def initialize(hh_mm_ampm)
    unless parts = /\A(1[0-2]|0[1-9]):([0-5]\d)\s*(AM|PM)\Z/i.match(hh_mm_ampm)
      raise ArgumentError.new("Time not in HH:MM AM/PM format: #{hh_mm_ampm}")
    end
    hh, mm, am_pm = parts.captures
    @time_of_day = 60 * hh.to_i + mm.to_i
    @time_of_day += (12 * 60) if am_pm.upcase == 'PM'
  end

  def add(minutes)
    @time_of_day = (@time_of_day + minutes) % (24 * 60)
    self
  end

  def to_s
    # Left as an exercise for you
    sprintf(...)
  end
end


Python algorithm mimicking mark/sweep garbage collection I don't have much experience with Python and would love some feedback on my implementation of this program.

I am given a file with an integer representing a number of memory-cells, followed by a list of ordered pairs representing links between either a pointer and a memory-cell, or memory-cell and a memory-cell. You could use the list to draw diagram akin to those used in text-book "which cells are marked and which are reclaimed" problems regarding mark/sweep GC.

I was tasked with creating a Python script that takes in such a list, and runs a simulated mark/sweep on it and my solution (below) is fully functional.

Here's an example input file:

6
ptr1,0
0,1
1,2
ptr2,3
3,2
4,5

And here is the script:

import sys
import re

def RepresentsInt(s):
    try:
        int(s)
        return True
    except ValueError:
        return False

#recursive function that traces through a mem-node structure
#setting each node's check_flag to 1 if traversed
def node_trace(pointer, node_pointer_list, nodes_dict):
    if RepresentsInt(pointer[0]):
        if nodes_dict[pointer[0]][1] == 1:
            #returns if the current node has been traversed already
            return
        else:
            #checks the current node as marked and traversed
            nodes_dict[pointer[0]][0] = 1
            nodes_dict[pointer[0]][1] = 1

    for x in node_pointer_list:
    #recurs for each node the current node points to
        if x[0] == pointer[1] and nodes_dict[x[0]][1] == 0:
            node_trace(x, node_pointer_list, nodes_dict)

        #catches the node at the end of a chain
        if RepresentsInt(x[0]):
            if nodes_dict[x[0]][1] == 1:
                nodes_dict[x[1]][0] = 1
                nodes_dict[x[1]][1] = 1
        else:
        #catches a single-link chain from a variable to a mem-block
            nodes_dict[x[1]][0] = 1
            nodes_dict[x[1]][1] = 1

sysArgs = list(sys.argv)
inputFile = sysArgs[1]
inFile = open(inputFile)
node_pointer_list = []
nodes_dict_0 = {}      #contains node records of the format, ['mem-cell': traversed_flag, marked_flag]
                       #traversed/marked _flag is 0 if unchecked, 1 if checked

#read, parse, and store the contents of the input file
node_count = 0
for line in inFile:
    #grabs the number of nodes which is stored in the first line of the file
    if node_count == 0:
        node_count = int(line)
    else:
        #read and parse a line as required
        splitLine = line.rstrip()
        splitLine = re.split(r'[,]', splitLine)

        node_pointer_list.append([splitLine[0], splitLine[1]])

#create a dictionary for all nodes, with flags for "checked" and "traversed"
#initialized to 0
count = 0
while count < node_count:
    nodes_dict_0[str(count)] = [0, 0]
    count += 1

#simulates the mark phase of a mark-sweep algorithm using the
#reference structure described in the input file
for item in node_pointer_list:
    if not RepresentsInt(item[0]):
        #if the link begins with a variable pointer
        #follow the trace marking all nodes along the way
        node_trace(item, node_pointer_list, nodes_dict_0)

print("\nMarked: ")
for item in sorted(nodes_dict_0.items()):
    if item[1][1] == 1:
        print(item[0])

print("\nReclaimed: ")
for item in sorted(nodes_dict_0.items()):
    if item[1][1] == 0:
        print(item[0])

Running the above script with the example input file produces the following output:

Marked:
0
1
2
3

Reclaimed:
4
5

which is correct.

My question simply this: Is the way I implemented this a reasonable solution or are there egregious inefficiencies, needlessly roundabout approaches etc that wouldn't be obvious to an novice that I could improve-on beyond just providing the required functionality?

More specifically: is a recursive algorithm the best approach in python? Is using a dictionary to track the marked status of nodes the most efficient way to do so? Is the way I structured the algorithm as efficient as it could be in terms of big-O?

There are several suboptimal areas of this solution that are interrelated. Please feel free to ask questions if anything I've written here is unclear.

The solution is not "modeling" memory and then performing mark-and-sweep as it probably should be. Instead it is preserving the data-input file in node_pointer_list and repeatedly walking it to find what is needed, because of your for x in node_pointer_list: in node_trace(). This is causing the solution to unnecessarily iterate over the node_pointer_list inside each call to node_trace(), which is making its running time O(N^2) when it should be O(N). It is also making the code 2-3x longer than it should be. Further, the solution is quite hard to read.

I'll try to break down those issues one at a time, though keep in mind they are all interrelated. I'm intentionally explaining rather than providing code, so you can do the learning work to make a better solution. I'll start by talking about code readability.

The way you use if not RepresentsInt(item[0]): to determine if a line is a "pointer" line is very non-intuitive. It would be easier to read if the literal "ptr" appeared somewhere near where you are testing if it is a line containing "ptr", such as:

if item[0].find("ptr") == 0:

Why do you have both "marked" and "traversed" flags? They contain the same information.

A code line like this below, is very hard for anyone to read the meaning of. Someone reading the code is forced to dig around to find out the meaning of the zeroes and ones to understand it.

nodes_dict[x[1]][0] = 1

To make it more readable, you need to get the word "TRAVERSED" in there somewhere, since what you are doing is setting a traversed flag. If you stick with your two element arrays, you can make a TRAVERSED_FLAG symbol to make it more readable, as in:

nodes_dict[x[1]][TRAVERSED_FLAG] = 1

Better style would be to define a class, and give the class a .traversed field. If you also make some of the other changes I highlight below, this access becomes:

current_object.traversed = 1

I don't know if you are comfortable with classes or not, so I'll provide one explanation based on classes, and one based on arrays...

With Classes

You can put the marked field, and the list of an object's references in a single class that represents a memory position in your heap. The class definition would look something like:

class MemoryObject:
    def __init__(self):
        self.marked = 0
        self.references = []  # list of either <int> or <MemoryObject>

First you would instantiate an array of these for the number of memory-spaces according to your data-file, with an additional one as the RootMemoryObject. Then you would walk the file's references, adding to each MemoryObject's references list. You can either store an int reference in the list, and look it up later, or you can look up the MemoryObject now and store that directly in references. When you see a "ptr" entry, you put that in the RootMemoryObject.

The first step of mark-and-sweep is to walk all memory locations and set .marked=0. Then you would call node_trace(RootMemoryObject), which would mark the object visited, and recurse through the objects referenced by that object's .references. Because you have the list of that object's references, you don't need the inefficient loop for x in node_pointer_list:.

Without Classes

Instead of node_pointer_list, you want an array of lists of references. As you walk the datafile, you would insert each reference into the appropriate list for that pointer number. Further, you want an additional "root_references" list to hold references which appear in "ptr" entries.

To start the recursion, you would do for reference in root_references: node_trace(reference), which would mark the reference visited (returning if it was already visited), then look up each child reference and recursively call node_trace(child_reference) on it. Because you have the list of that object's references, you don't need the inefficient loop for x in node_pointer_list:.

Other Factors

The running time you are shooting for is O(N), where N is the number of nodes in your memory. You are currently O(N^2). After you remove the extra loop in node_trace(), you will be properly O(N) because you already have a check to be sure you're not traversing a set of children more than one.

Using recursion is good. An alternative is to use a work-list. To use a work list, you put the root(s) in a list, and then while the work-list is non-empty, you remove an item from the work-list and call node_trace() on it. node_trace puts any child nodes that need to be visited into the work list, and exits.

Both will have the same O(N) running time, but can potentially have different memory consumption. The recursive version's maximum memory consumption is proportional to the depth of object references. The work-list version's maximum memory consumption is hard to calculate, as it can be wildly different based on how you prioritize the work list and what your in-memory objects look like.

If you have 1000 objects in a linked list, each pointing to the next, the work-list will only require 1 entry, whereas the recursive version will require a 1000 depth stack. However, if instead you have 1 object with 999 pointers to leaf objects, then the recursive version will only require a maximum of 1 stack frame, while the work list will have a maximum of 999 entries. So you can see neither is better universally, there are just tradeoffs.


Optimizing paint code I am trying to optimize the following code:

    private void Screen_Paint(object sender, PaintEventArgs e)
    {
        Graphics bmpGraphics = Graphics.FromImage(_screenBitmap);

        int xLoc = 0;
        int yLoc = 0;

        bmpGraphics.Clear(Color.Black);
        _resetInvoked = true;

        for (int i = 0; i < 4000; i += 2)
        {
            var bgBit = (byte) ((_screenMemory[i + 1]) & 112);
            var fgBit = (byte)((_screenMemory[i + 1]) & 15);
            SolidBrush bgBrush = _backgroundBrushesArray[bgBit/16];
            SolidBrush fgBrush = _foregroundBrushesArray[fgBit];

            if (((xLoc % 640) == 0) && (xLoc != 0))
            {
                yLoc += 12;
                xLoc = 0;
            }
            if (_resetInvoked)
            {
                string s = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
                var pf = new PointF(xLoc, yLoc);

                bmpGraphics.FillRectangle(bgBrush, xLoc + 2, yLoc + 3, 8f, 11f);
                bmpGraphics.DrawString(s, _renderFont, fgBrush, pf);

            }
            xLoc += 8;
        }

        _resetInvoked = false;

        e.Graphics.DrawImage(_screenBitmap, new Point(0, 0));
        bmpGraphics.Dispose();
    }

This is a Winforms User Control that mimics a console like screen. Its working just fine, but I'd like to speed it up. Running it through Telerik's JustCode performance monitor, the slowest performance seems to come from the FillRectangle and DrawString methods. Is there anyway to speed this up?

Magic numbers

You have a lot of magic numbers in your code. Consider to express them as meaningful constants. ( 4000, 112, 15, 16, 640, 12, 8f, 11f)

Style

Be consistent with your coding style.

string s = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
var pf = new PointF(xLoc, yLoc);

at the first line you explicitly declare s as string  also it is quite obvious by the right hand side that it is a string. On the second line you use var as expected.

There is nothing wrong with declaring implict by using the type, but here you should have either used var also or instead of var PointF.

Naming

Use meaningful names for variables. Mr.Maintainer will thank you for doing so. s and pf are poor names.

Using statement

Instead of creating and disposing the Graphics you should enclose this inside a using statement, so in case of an exception the Graphics object is properly disposed.

Declaring variables

Variables should be declared as nearly as possible of their use ( here: the brushes ).

Refactoring

This won't do anything regarding speed, if the bottleneck are the FillRectangle() and DrawString() methods.

There is no sense in entering the loop or doing anything if _resetInvoked==true. So using a guard condition to return early is the way to go.

private void Screen_Paint(object sender, PaintEventArgs e)
{

    if (_resetInvoked) { return; }
    _resetInvoked = true;

    using (Graphics bmpGraphics = Graphics.FromImage(_screenBitmap))
    {

        int xLoc = 0;
        int yLoc = 0;

        bmpGraphics.Clear(Color.Black);

        for (int i = 0; i < 4000; i += 2)
        {

            if (((xLoc % 640) == 0) && (xLoc != 0))
            {
                yLoc += 12;
                xLoc = 0;
            }

            var bgBit = (byte) ((_screenMemory[i + 1]) & 112);
            SolidBrush bgBrush = _backgroundBrushesArray[bgBit/16];
            bmpGraphics.FillRectangle(bgBrush, xLoc + 2, yLoc + 3, 8f, 11f);

            var fgBit = (byte)((_screenMemory[i + 1]) & 15);
            SolidBrush fgBrush = _foregroundBrushesArray[fgBit];

            var currentText = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
            var currentPoint = new PointF(xLoc, yLoc);
            bmpGraphics.DrawString(currentText, _renderFont, fgBrush, currentPoint);

            xLoc += 8;
        }

    }

    e.Graphics.DrawImage(_screenBitmap, new Point(0, 0));
    _resetInvoked = false;
}


Binary to ascii mapping in JavaScript This morning I've seen this on Twitter:
http://mynewtechworld.tumblr.com/post/135545896048

I case that somebody can't see the image:

  I
  01101100
  01101111
  01110110
  01100101
  you.

Just for fun I wrote this binary to ascii-converter in JavaScript:

var chars = [
  'I ',
  0b01101100,
  0b01101111,
  0b01110110,
  0b01100101,
  ' you.'
];

var result = chars.reduce(function(previous, current) {

  if (!isNaN(current)) {
    current = String.fromCharCode(current.toString());
  }

  return previous + current;
}, '');

My complete implementation on CodePen

I suggest splitting:

Converting binary to ascii
Concatenating

After defining binaryToNumber and add. the final code will look much simpler:

var result = chars.map(binaryToNumber).reduce(add)