How secure is my registration code Hello I am a beginner in PHP and I need some opinions. How secure is my registration code ? If I launched this on the web would you feel safe signing up with personal info ?

  <?php

    // shows all errors
    ini_set('display_errors', 1);
    error_reporting(E_ALL);

    //check if form is submitted
    if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register']))
    {
        // looks like a hack, send to index.php
        header('Location: index.php');
        die();
    }

    include("connect.php");

        $errors = [];
    if (empty($_POST["username"])) {
        $errors[] = "Fill in username to sign up";
    }

    if (empty($_POST["pw"])) {
        $errors[] = "Fill in password to sign up";
    }

    if (empty($_POST["pw2"])) {
        $errors[] = "Confirm password to sign up";
    }
    // and so on...
    if ($_POST['pw'] !== $_POST['pw2']) {
        $errors[] = "The passwords do not match.";
    }

    if (!$errors) {
        $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
        $stmt->bind_param("s", $_POST['username']);
        $stmt->execute();
        $row = $stmt->get_result()->fetch_assoc();

        if ($row && $row['username'] == $_POST['username']) {
            $errors[] = "Username exists";
        }
    }
    if (!$errors) {
        $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, array('cost' => 14));

        $stmt = $conn->prepare("INSERT INTO users (username, pw) VALUES(?, ?)");
        $stmt->bind_param("ss", $_POST['username'], $pw );
        $stmt->execute();

        echo "Registration successful Login here
";
    } else {
        foreach ($errors as $error) {
            echo "$error
 \n";
        }
        echo 'Try again
';
    }
    ?>

Some remarks:

You have repetitive code eg:

if (empty($_POST["username"])) {

You might as well define an array of fields to check and run validation in a loop.
Although you have only 3 items here...

There is a major flaw: you are not sanitizing the input fields:

$stmt->bind_param("s", $_POST['username']);

At a minimum you should trim the text because it may contains whitespace (before and after). So if user 'admin' is already taken, I can register as 'admin ' for example (one space). That is not necessarily a direct vulnerability, it depends on the rest of the code, but you have inconsistent data and this will likely cause problems later. Usernames should not contain whitespace leading or trailing whitespace. Because you can have two different accounts the all look the same in print and on screen. That makes impersonation easy.

What you should do is assign the POST fields to variables, and then use variables in the rest of the code, after checking them and sanitizing them. Do not reuse $_POST['whatever'] all across your code.

You should also test what happens if the HTTP request is tainted, for example if
$_POST['username'] is included twice in the POST request. Or if the field contains multiline input (or null characters). How will your code react ? Do you have error handling and logging ?

I note that you are not checking the length of input fields, including the password. What happens if the text is very large, larger than the corresponding table field ?

You should have a sensible password policy and not accept any password like 1234 or same as the username...

What you are showing is the registration form, but what would be interesting to see is the login page. That's the thing that should not be easy to trick or bypass.

If you use a modern development framework you can simplify your life, and your code will very probably be more secure.

In my opinion a page that does not have error handling is not secure, because you are not seeing what's going on and the code could also behave unpredictably.

Conclusion:

  If I launched this on the web would you feel safe signing up with
  personal info ?

No. There are simply not enough checks. The code is not robust. You should test it in adverse conditions. Try automated tools like SQLmap, Nikto and simulate an attack against yourself.

It is good that you are using parameterized queries, that's the least you could do in 2020. But that does not mean user input does not have to be checked and sanitized. You may be opening your site to other vulnerabilities. What is pretty much guaranteed is that users will use very weak passwords since you are accepting anything. Therefore many accounts will be susceptible to brute force attacks. And you will be blamed for facilitating a breach of personal data.


Is this kitchen timer code pythonic? I've been doing bits and bobs with Python for a few years, but I've never took the time to get feedback on what's pythonic and what's not. In the interest of collaborating more with Python programmers, I'd like to make sure my code follows conventions and idioms that the majority of Pythonistas will recognize.

Please review my Kitchen Timer class for:

pythonic idioms and conventions
behavioural correctness (especially around multi-threading and timing precision)
portability
readability
maintainability
efficiency

Also, if the purpose of the class is unclear from its API and comments, please offer your suggestions for improvement there too.

from threading import Timer as TTimer
from time import time
from threading import Lock

class NotRunningError(Exception): pass
class AlreadyRunningError(Exception): pass

class KitchenTimer(object):
    '''
    Loosely models a clockwork kitchen timer with the following differences:
        You can start the timer with arbitrary duration (e.g. 1.2 seconds).
        The timer calls back a given function when time's up.
        Querying the time remaining has 0.1 second accuracy.
    '''

    running = "running"
    stopped = "stopped"
    timeup =  "timeup"

    def __init__(self):
        self.__stateLock = Lock()
        self.state = self.stopped
        self.__timeRemainingLock = Lock()
        self.timeRemaining = 0

    def start(self, duration=1, whenTimeup=None):
        if self.isRunning():
            raise AlreadyRunningError
        else:
            self.state = self.running
            self.timeRemaining = duration
            self._userWhenTimeup = whenTimeup
            self._startTime = self._now()
            self._timer = TTimer(duration, self._whenTimeup)
            self._timer.start()

    def stop(self):
        if self.isRunning():
            self._timer.cancel()
            self.state = self.stopped
            self.timeRemaining -= self._elapsedTime()
        else:
            raise NotRunningError()

    def isRunning(self):
        return self.state == self.running

    def isTimeup(self):
        if self.state == self.timeup:
            return True
        else:
            return False

    @property
    def state(self):
        with self.__stateLock:
            return self._state

    @state.setter
    def state(self, state):
        with self.__stateLock:
            self._state = state

    @property
    def timeRemaining(self):
        with self.__timeRemainingLock:
            if self.isRunning():
                self._timeRemaining -= self._elapsedTime()
            return round(self._timeRemaining, 1)

    @timeRemaining.setter
    def timeRemaining(self, timeRemaining):
        with self.__timeRemainingLock:
            self._timeRemaining = timeRemaining

    def _whenTimeup(self):
        self.state = self.timeup
        self.timeRemaining = 0
        if callable(self._userWhenTimeup):
            self._userWhenTimeup()

    def _now(self):
        return time()

    def _elapsedTime(self):
        return self._now() - self._startTime

1. Bugs

You have two locks, one protecting state, and the other protecting timeRemaining. But these locks operate independently. That means that when you update both of these properties:

self.state = self.timeup
self.timeRemaining = 0

it would be possible for another thread to run in between these two statements and see state == timeup but timeRemaining != 0. You need one lock to protect both properties, so that they are updated atomically. See below for some race conditions that are consequences of this bug.
The timeRemaining property updates the _timeRemaining value by subtracting the elapsed time. But this means that if you keep requesting the timeRemaining property, it will keep subtracting the elapsed time, giving you nonsense like this:

>>> t = KitchenTimer()
>>> t.start(5)
>>> t.timeRemaining
3.2
>>> t.timeRemaining
0.6
>>> t.timeRemaining
-2.6
>>> t.timeRemaining
-6.6
>>> t.timeRemaining
-11.3
>>> t.timeRemaining
0.0

There's a race condition between the start method and the whenTimeup method. In start you have:

if self.isRunning():                  # 1
    raise AlreadyRunningError
else:
    self.state = self.running         # 2
    self.timeRemaining = duration     # 3

and in _whenTimeUp you have:

self.state = self.timeup              # 4
self.timeRemaining = 0                # 5

A possible order of execution is 4–1–2–3–5 which would set state to running and timeRemaining to 0.
There's a race condition between the stop method and the _whenTimeup method. In stop you have:

if self.isRunning():                  # 1
    self._timer.cancel()              # 2
    self.state = self.stopped         # 3

and in _whenTimeUp you have:

self.state = self.timeup              # 4

A possible order of execution is 1–4–2–3 which would leave state as stopped when it ought to be timeup.

(There may well be more race conditions; I haven't looked very hard.)

2. Comments on your code

Why "Kitchen" timer? The class doesn't seem to have anything to do with kitchens.
There doesn't seem to be any documentation for the methods.
Why does the timeRemaining method round the result to one decimal place? There doesn't seem to be any explanation of this. It would be more natural to return the actual time remaining.
Why do you rename threading.Timer as TTimer? The name Timer doesn't seem to conflict with anything in your code.
The Python style guide (PEP8) recommends using lowercase_with_underscores for method and property names. You're not obliged to follow this guide, but it will make it easier for you to collaborate with other Python programmers.
The intended use of private names in Python is to "avoid name clashes of names with names defined by subclasses" but that's not necessary here. So write _state_lock instead of __stateLock and so on.
The _now method seems a bit pointless:

def _now(self):
    return time()

Why not just call time()?


Determinant calculation of a matrix of any degree My program calculates the determinant of a matrix of any degree. If you really can't understand my comments or indentifiers please let me know.

//;;MakeShift make element number 'ind' the head of list:
(defun MakeShift (ind L buf)
    (if (= ind 1) (cons (car L) (append buf (cdr L)))
       (makeReplace (- ind 1) (cdr L) (append buf (list (car L))))
    )
)

//;;Shift call MakeShift:
(defun Shift (ind L) (MakeShift ind L nil))

//;;makeTransp make transposition of two elements list:
(defun makeTransp (L) (cons (cadr L) (cons (car L) nil)))

//;;PushForEach put element elem into a heads of all lists of L:
(defun PushForEach (elem L)
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

//;;MakeTranspositions create a list of all transpositions
//;;using first transposition like '(1 2 3 ...).
//;;transpNum is transpositions  number and numOfElem
//;;is amount of elements in transposition:
(defun MakeTranspositions (transp transpNum numOfElem)
    (cond ((> transpNum numOfElem) nil)
        ((= numOfElem 2) (cons transp (cons (makeTransp transp) nil)))
        (T (append (PushForEach (car transp) (MakeTranspositions (cdr transp) 1 (- numOfElem 1)))
            (MakeTranspositions (Shift (+ transpNum 1) transp) (+ transpNum 1) numOfElem)))
    )
)

//;;MakeFirstTransp make a first transpositiion like '(1 2 3 ... )
//;;which has number of element equal matrix degree:
(defun MakeFirstTransp (matrixDegree transp)
    (if (= matrixDegree 0) transp
        (MakeFirstTransp (- matrixDegree 1) (cons matrixDegree transp))
    )
)

//;;Transpositions make all transpositions of matrix using MakeTranspositions:
(defun Transpositions (matrixDegree)
    (MakeTranspositions (MakeFirstTransp matrixDegree nil) 1 matrixDegree)
)

//;;GetCol return elemnt number col in row (row belong to matrix):
(defun GetCol (col rowVector)
    (if (= col 1) (car rowVector)
        (GetCol (- col 1) (cdr rowVector))
    )
)

//;;GetElem return element a[row][col] which belong to matrix:
(defun GetElem (row col matrix)
    (if (= row 1) (GetCol col (car matrix))
        (GetElem (- row 1) col (cdr matrix))
    )
)

//;;CheckFirst check first element in transposition (cons first transp) for even:
(defun CheckFirst (first transp)
    (cond ((null transp) 1)
        ((< first (car transp)) (CheckFirst first (cdr transp)))
        (T (* -1 (CheckFirst first (cdr transp))))
    )
)

//;;Sign return sign of transposition (1 or -1):
(defun Sign (transp)
    (if (null (cdr transp)) 1
        (* (CheckFirst (car transp) (cdr transp)) (Sign (cdr transp)))
    )
)

//;;Product return product of elements of matrix by transposition transp:
(defun Product (matrix transp) (GetProduct matrix 1 transp))

//;;GetProduct are called by Product:
(defun GetProduct (matrix ind transp)
    (if (null transp) 1
        (* (GetElem ind (car transp) matrix)
            (GetProduct matrix (+ ind 1) (cdr transp))
        )
     )
)

//;;GetSumm return summ of all products by transpositions whith their signs:
(defun GetSumm (matrix transps)
    (if (null transps) 0
        (+ (* (Sign (car transps)) (Product matrix (car transps)))
            (GetSumm matrix (cdr transps))
        )
    )
)

//;;Determinant call GetSumm:
(defun Determinant (matrix matrixDegree)
    (GetSumm matrix (Transpositions matrixDegree))
)

//;;So, programm work fast.

The first thing is to learn proper indentation.

//;;PushForEach put element elem into a heads of all lists of L:
(defun PushForEach (elem L)
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

What's wrong with the code layout?

Common Lisp has built-in documentation features. A documentation string can be placed inside the function and can be retrieved with the DOCUMENTATIONfunction.
trailing parentheses are a big DON'T DO THAT. It just wastes space without adding much information. Your editor will count parentheses or will show the corresponding parentheses.
put constructs, when they are too long on a line, over several lines.
don't use CamelCase, use hyphens

Let's do the editing step by step:

(defun PushForEach (elem L)
  "put element elem into a heads of all lists of L"
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

(defun PushForEach (elem L)
  "put element elem into a heads of all lists of L"
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))))

Now we get the version which is best readable:

(defun push-for-each (elem L)
  "put element elem into a heads of all lists of L"
  (if (null L)
      nil
      (cons (cons elem (car L))
            (push-for-each elem (cdr L)))))

Above function has a documentation string and useful code layout.

Next: can we write it better? You can bet that basic recursion in Lisp has been provided as a higher-order function. Applying a function over all items in a list is called mapping. The basic mapping function is MAPCAR.

(defun push-for-each (element list)
  "adds element as head of all sublists of list"
  (mapcar #'(lambda (item)
              (cons element item))
          list))


Print numbers in a spiral from the center #!/usr/bin/env stack
{- stack
     --install-ghc
     exec ghci
     --package lens
-}

module Main where

import Control.Lens
import Data.List.Lens
import Debug.Trace
import Text.Printf
import Prelude hiding (Left, Right)

data Direction = Up | Left | Down | Right

data Location = Location Int Int

type Grid = [[Int]]

main = printClockwise 25

printClockwise :: Int -> IO ()
printClockwise n = putStrLn $ concatMap printRow grid
  where
    grid = walk directions center 1 emptyGrid
    emptyGrid = replicate sq (take sq (repeat 0))
    printRow row = concatMap (printf "%4s" . show) row ++ "\n"
    -- Ex. for a 5x5 spiral the start location is (2,2)
    center = Location half half
    half = floor $ (fromIntegral sq) / 2
    -- This will return all directions we should walk through the spiral in.
    -- Something like [Right, Down, Left, Left, Up, Up, Right, Right, Right]
    directions = concatMap (\(direction, steps) -> take steps $ repeat direction) (zip directionOrder steps)
    directionOrder = cycle [Right, Down, Left, Up]
    -- Ex. for a spiral up to 25, the steps are 1, 1, 2, 2, 3, 3, 4, 4, 5
    steps = concatMap (take 2 . repeat) [1 .. sq - 1] <> [sq]
    -- Assume the input is a perfect square of an odd number
    sq = ceiling $ sqrt (fromIntegral n)

walk :: [Direction] -> Location -> Int -> Grid -> Grid
walk (dir : dirs) loc@(Location x y) current grid = do
  let grid' = grid & (ix y . ix x) .~ current
  let loc' = moveLocation dir loc
  walk dirs loc' (current + 1) grid'
walk [] _ _ numbers = numbers

moveLocation Down (Location x y) = Location (x) (y + 1)
moveLocation Right (Location x y) = Location (x + 1) (y)
moveLocation Left (Location x y) = Location (x -1) (y)
moveLocation Up (Location x y) = Location (x) (y - 1)

I came across this problem on reddit and thought it'd be fun to try my hand at it in Haskell. The idea is to generate the grid before printing, and the insight to generate that grid was that the movement follows a predictable pattern, of right, down, left, up, and the # of steps taken in a given direction is 1, 1, 2, 2, 3, 3, 4, 4... , n - 1, n -1, n , where n is the square of the input number. So with that you can walk through the grid and fill in the numbers.
The function has no input checking, it assumes the input is a perfect square (of an odd number).

Not too advanced myself - so take with a grain of salt.

Foremost I would separate the logic for constructing the grid and printing. Especially extract the function to construct the grid.  Currently its wrapped in printClockwise  IMHO it should be prominently its own function.
There are a few definitions in printClockwise  (emptyGrid, directionOrder and the printing) that I would consider reusable and would make them top level.
You only use lens in one place. lens is quite an expensive import - there just was an interesting post.  I suppose your intend was to learn lens and therefore it's used - but just for the code I would consider removing the dependency.
the fixed "%4s" in printing - it's easy to estimate the required width for larger numbers. Or just make it that max n is 999.

Some stuff to consider that is really personal choice - no need to follow if you disagree:

changing the data type to strict mutable array or similar.
adding a walk that takes larger strides.
split the walk function into one stream of positions in the spiral, and separately setting the values in a second step.  This could solve the issue that the input needs to be a perfect square - just take the first n positions.

As last remarks:

whenever I see such a spiral I think Ulam spiral. If you want to expand I would consider adding functionality to the printing to allow highlight - in this case for primes.
I also checked whether the coordinates are on OEIS - they are A174344. And your algorithm agrees with the algorithm they post along side (the Julia implementation is almost the same, the other use some shenanigans with sin and cosine).


LRU algorithm implementation In this code:

tmr represents timer array
n represents number of frames in memory
m represents number of pages
rs represents the reference string

There are many functions in this program, but the important one is the timer_confg. This is the function that tests the particular page refereed is least refereed or just refereed. It is used to calculate the timing of the pages. I have designed the function for 3 frames, but if the frames are increased then the cases in the switch statement can be increased. Here 0 means just refereed page and max means maximum from the timer array tmr[] is the least refereed page.

There are other functions for read and display. The max function is used for finding the maximum from the timer which will be used to determine the least refereed page.

This code implements the algorithm using the timer which is maintained by the array tmr[]. The pg[] array denotes the frames in the memory and rs[] denotes the reference string. The rest of the code is about the conditions that will be encountered like:

when frames are empty
when the page is already in the memory
which page needs to be replaced

#include<iostream>
using namespace std;

int max(int r[],int n)
{
    int pos,i,l,max=0;
    for(i=0;i<n;i++)
    {
        if(r[i]>max)
        {
            l=r[i];
            max=r[i];
            pos=i;
        }
    }
    return pos;
}

void read(int a[],int n)
{
    int i;
    for(i=0;i<n;i++)
    {
        cin>>a[i];
    }
}

void display(int b[],int n)
{
    int i;
    for(i=0;i<n;i++)
    {
        cout<<b[i]<<" ";
    }
}

void timer_confg(int tmr[],int pos)
{
    int j=pos;
    switch(pos)
    {
        case 0:
            tmr[j]=0;
            tmr[j+1]=tmr[j+1]++;
            tmr[j+2]=tmr[j+2]++;
            break;
        case 1:
            tmr[j]=0;
            tmr[j-1]=tmr[j-1]++;
            tmr[j+1]=tmr[j+1]++;
            break;
        case 2:
            tmr[j]=0;
            tmr[j-1]=tmr[j-1]++;
            tmr[j-2]=tmr[j-2]++;
            break;
    }
}
int main()
{
    int pg[20],tmr[20],i,j,n,pf=0,rs[20],m,avail=0;
    cout<<"Enter number of frames: ";
    cin>>n;
    for(i=0;i<20;i++)
    {
        pg[i]=-1;
    }
    cout<<"Enter the number of pages:";
    cin>>m;
    cout<<"Enter the reference string: ";
    read(rs,m);
    int pos;
    tmr[0]=2;
    tmr[1]=1;
    tmr[2]=0;
    for(i=0;i<m;i++)
    {
        for(j=0;j<n;j++)
        {
            if(pg[j]==-1)
            {
                pg[j]=rs[i];
                avail=1;
                pf++;
                cout<<"This page "<<rs[i]<<" is PLACED in the frame "<<j<<endl;
                break;
            }
            else
            {
                if(pg[j]==rs[i])
                {
                    pos=j;
                    timer_confg(tmr,pos);
                    avail=1;
                    cout<<"This page "<<rs[i]<<" is already in the frame "<<j<<endl;
                    break;
                }

                else
                {
                    if(avail==0 || j==(n-1))
                    {
                        pos=max(tmr,n);
                        pg[pos]=rs[i];
                        pf++;
                        timer_confg(tmr,pos);
                        cout<<"The page "<<rs[i]<<" is replaced with frame "<<pos<<endl;
                        break;
                    }
                }
            }
        }
    }
    cout<<"The total number of page fault is:"<<pf;
    return 0;
}

I don't know the LRU Algorithm. So I can't criticize the implementation. The most glaring problems are:

Undescriptive variable names, like "tmr", "n" and "m".
Undescriptive procedures, like "max" (max of what? The return value is not the maximum value of the array, but the position).
The use of "magic" numbers. Like the number 20 for example. Store that number in a descriptive constant.
The logic of the main procedure.

For example

 else
 {
     if(...)

could be written as such

else if(...)


DRY the throwing of ArgumentNullException("ParameterName")? I am using this code in a lot of places:

if (ParameterName == null)
{
    throw new ArgumentNullException("ParameterName");
}

But I think this is not DRY. The only difference here is ParameterName. Is it ok to use these lines everywhere, or is there a way to make it DRY?

Google Guava's Precondition class contains good patterns for this problem. For example, the checkNotNull method is the following:

T checkNotNull(T reference, String errorMessageTemplate, Object... errorMessageArgs)

Usage:

checkNotNull(parameter, "parameter cannot be null");

Note its return value. If the value is not null it returns the referencce, so you can use it in the following way:

public MyClass(MyObject dependency) {
    this.dependency = checkNotNull(dependency, "dependency cannot be null");
}

It also supports error message templates with %s.

Although it's a Java library, you can implement the same methods in C# too, they are really simple.

See also: Preconditions Explained


Parsing Wikipedia table with Python I am new to Python and recently started exploring web crawling. The code below parses the S&P 500 List Wikipedia page and writes the data of a specific table into a database.

While this script is hardcoded and I would certainly be interested in some thoughts on performing the same task in a slightly more generic way (perhaps with beautifulsoup), this is not my primary concern. What I really wondered was if there is a less verbose or more "pythonic" way of doing it.

import urllib.request
import re
import pymysql

# Open Website and get only the table on the page with the relevant data. In this hardcoded case
table = urllib.request.urlopen("https://en.wikipedia.org/wiki/List_of_S%26P_500_companies#S.26P_500_Component_Stocks").read().decode("utf-8")
table = table.split("
(.+)<\/a>	'
cigs_grab = '^(.+)	'

ticker, exchange, names, cigs, cigs_sub = ( [] for i in range(5))
match = False

# Parse HTML output and write relevant td data to lists.
# The list is "hardcoded", meaning after each match of either NASDAQ or NYSE ident,
# the matching  as well as the next, the fourth and fifth  after that one get parsed.

for i in range(len(table)):
    if bool(re.search(pattern = tick_ident_nasdaq, string = table[i])):
        ticker.append(re.search(pattern = name_grab, string = table[i]).group(1))
        exchange.append("NASDAQ")
        match = True

    elif bool(re.search(pattern = tick_ident_nyse, string = table[i])):
        ticker.append(re.search(pattern = name_grab, string = table[i]).group(1))
        exchange.append("NYSE")
        match = True

    if match == True:
        names.append(re.search(pattern = name_grab, string = table[i + 1]).group(1))
        names[-1] = re.sub(pattern = "&amp;", repl = "&", string = names[-1])
        cigs.append(re.search(pattern = cigs_grab, string = table[i + 3]).group(1))
        cigs[-1] = re.sub(pattern = "&amp;", repl = "&", string = cigs[-1])
        cigs_sub.append(re.search(pattern = cigs_grab, string = table[i + 4]).group(1))
        cigs_sub[-1] = re.sub(pattern = "&amp;", repl = "&", string = cigs_sub[-1])
        match = False

# Format Data in tuple format for database export
company_data = zip(ticker, exchange, names, cigs, cigs_sub)

# Establish database connection, empty companies table and rewrite list data to table
try:
    conn = pymysql.connect(host = "localhost", user = "root", passwd = "pw", db = "db", charset = "utf8", autocommit = True, cursorclass=pymysql.cursors.DictCursor)
    cur = conn.cursor()
    cur.execute("DELETE FROM companies")
    cur.executemany("INSERT INTO companies (tickersymbol, exchange, name, cigs, cigs_sub) VALUES (\"%s\", \"%s\", \"%s\", \"%s\", \"%s\")", (company_data))
finally:
    cur.close()
    conn.close()

The right tool

As you've said, you are not using the right tool for this task : you can't parse HTML with regexps.

A better approach would be to use an already existing parser like BeautifulSoup.

A simpler container

At the moment, you are putting data in multiple lists to zip them all at the very end. It can be a very nice technique but in our case, you put in different containers things that actually belong together. Also, you have a risk of adding too many elements in a list and having information zipped with information that should be in a different row. An easier option is to have a single list where each elements contain everything you've parsed.

Also, you can take this chance to rewrite in a more straightforward way the parts where you add something a list and then refer to it with my_list[-1].

company_data = []

for i in range(len(table)):
    if bool(re.search(pattern = tick_ident_nasdaq, string = table[i])):
        exchange = "NASDAQ"
    elif bool(re.search(pattern = tick_ident_nyse, string = table[i])):
        exchange = "NYSE"
    else:
        exchange = None
    if exchange:
        ticker = re.search(pattern = name_grab, string = table[i]).group(1)
        name = re.search(pattern = name_grab, string = table[i + 1]).group(1)
        name = re.sub(pattern = "&amp;", repl = "&", string = name)
        cig = re.search(pattern = cigs_grab, string = table[i + 3]).group(1)
        cig = re.sub(pattern = "&amp;", repl = "&", string = cig)
        cig_sub = re.search(pattern = cigs_grab, string = table[i + 4]).group(1)
        cig_sub = re.sub(pattern = "&amp;", repl = "&", string = cig_sub)
        company_data.append((ticker, exchange, name, cig, cig_sub))

Compile your regexp

You can compile regexp if you plan to reuse them many times. It is more efficient and it makes it possible to use them like any Python object.

# Define regex used for parsing
tick_ident_nasdaq = re.compile('href=\"http:\/\/www\.nasdaq\.com\/symbol\/')
tick_ident_nyse = re.compile('href=\"https:\/\/www.nyse.com\/quote\/')
name_grab = re.compile('\">(.+)<\/a>	')
cigs_grab = re.compile('^(.+)	')
amp_re = re.compile("&amp;")

company_data = []

for i in range(len(table)):
    if bool(tick_ident_nasdaq.search(string = table[i])):
        exchange = "NASDAQ"
    elif bool(tick_ident_nyse.search(string = table[i])):
        exchange = "NYSE"
    else:
        exchange = None
    if exchange:
        ticker = name_grab.search(string = table[i]).group(1)
        name = name_grab.search(string = table[i + 1]).group(1)
        name = amp_re.sub(repl = "&", string = name)
        cig = cigs_grab.search(string = table[i + 3]).group(1)
        cig = amp_re.sub(repl = "&", string = cig)
        cig_sub = cigs_grab.search(string = table[i + 4]).group(1)
        cig_sub = amp_re.sub(repl = "&", string = cig_sub)
        company_data.append((ticker, exchange, name, cig, cig_sub))

"&" and "&"

What you are trying to do when substituing "&" with "&":

deserves to but put in a function on its own
actually corresponds to a common problem already solved : HTML entity decoding.


Better way to manipulate this string in sequence? I am working on a small custom Markup script in Java that converts a Markdown/Wiki style markup into HTML.

The below works, but as I add more Markup I can see it becoming unwieldy and hard to maintain. Is there is a better, more elegant, way to do something similar?

private String processString(String t) {
    t = setBoldItal(t);
    t = setBold(t);
    t = setItal(t);
    t = setUnderline(t);
    t = setHeadings(t);
    t = setImages(t);
    t = setOutLinks(t);
    t = setLocalLink(t);

    return t;
}

And on top of it, passing in the string itself and setting it back to the same string just doesn't feel right. But, I just don't know of any other way to go about this.

You could create a StringProcessor interface:

public interface StringProcessor {

    String process(String input);
}

public class BoldProcessor implements StringProcessor {

    public String process(final String input) {
        ...
    }
}

and create a List from the available implementations:

final List<StringProcessor> processors = new ArrayList<StringProcessor>();
processors.add(new ItalicProcessor());
processors.add(new BoldProcessor());
...

and use it:

String result = input;
for (final StringProcessor processor: processors) {
    result = processor.process(result);
}
return result;


A Twitter bot that fetches Forecast.io data and tweets out the current weather with text and emojis This is my first attempt at creating a Ruby script. I set up a Cron job to run this script every hour. I'm requesting constructive critiques and suggestions for making this better.

Sidenote: I've hard coded the location to my hometown. I don't plan on changing it to another location.

Here's an example of the expected output/tweet.

#!/usr/bin/env ruby
require 'twitter'
require 'forecast_io'
require 'gemoji'

client = Twitter::REST::Client.new do |c|
  c.consumer_key        = 'CONSUMER_KEY'
  c.consumer_secret     = 'CONSUMER_SECRET'
  c.access_token        = 'ACCESS_TOKEN'
  c.access_token_secret = 'ACCESS_TOKEN_SECRET'
end

ForecastIO.api_key = 'FORECAST_API_SECRET'
location           = ForecastIO.forecast(27.770, -82.638)
current_weather    = location.currently

icon =
  case current_weather.icon
  when 'clear-day'           then Emoji.find_by_alias('sunny').raw
  when 'clear-night'         then Emoji.find_by_alias('milky-way').raw
  when 'rain'                then Emoji.find_by_alias('umbrella').raw
  when 'snow'                then Emoji.find_by_alias('snowflake').raw
  when 'sleet'               then Emoji.find_by_alias('snowman').raw
  when 'wind'                then Emoji.find_by_alias('cyclone').raw
  when 'fog'                 then Emoji.find_by_alias('foggy').raw
  when 'cloudy'              then Emoji.find_by_alias('cloud').raw
  when 'partly-cloudy-day'   then Emoji.find_by_alias('partly_sunny').raw
  when 'partly-cloudy-night' then Emoji.find_by_alias('partly_sunny').raw
  when 'hail'                then Emoji.find_by_alias('cat').raw
  when 'thunderstorm'        then Emoji.find_by_alias('zap').raw
  when 'tornado'             then Emoji.find_by_alias('bangbang').raw
  else Emoji.find_by_alias('sunny').raw
  end

tweet = "Currently in #StPete it's #{current_weather.temperature.round}° & #{current_weather.summary.downcase} #{icon}"

client.update(tweet)

It looks pretty good to me. Just some minor notes:

Not that it's bad or anything, but in Ruby it's not usual to align code.
You can DRY the case block by creating a hash:

emojis_by_icon = {
  'clear-day' => 'sunny',
  'clear-night' => 'milky-way',
  ...
}

emoji_alias = emojis_by_icon.fetch(current_weather.icon, 'sunny')
icon = Emoji.find_by_alias(emoji_alias).raw

tweet = ... With long lines, I prefer to use the String#% interpolation:

tweet = "Currently it's %{temperature}° & %{summary} %{icon}" % {
  temperature: current_weather.temperature.round,
  summary: current_weather.summary.downcase,
  icon: icon,
}


Extracting lower and upper limit of a range of values based on a multiplier I am trying to find an optimal way to get the min and max limits from a list of lists provided a multiplier. I've tried two methods and I'm wondering if there is a more optimal solution that scales better in terms of performance when executed multiple times.
import timeit

def get_lower_upper_limits(inps):
    ranges, multipliers = inps
    min_l = 0
    max_l = 0
    for idx, multiplier in enumerate(multipliers):
        min_l += multiplier * (min(ranges[idx]) if multiplier > 0 else max(ranges[idx]))
        max_l += multiplier * (max(ranges[idx]) if multiplier > 0 else min(ranges[idx]))
    return min_l, max_l

def wrapper(func, *args, **kwargs):
    def wrapped():
        return func(*args, **kwargs)
    return wrapped

ranges = [[1000, 1400, 1800], [2200, 2400, 2600], [3000, 3100, 3200]]
multiplier = [-1, 2, 3]
assert len(ranges) == len(multiplier)
wrapped = wrapper(get_lower_upper_limits, (ranges, multiplier))
print('get_lower_upper_limits: {}'.format(timeit.timeit(wrapped, number=5000000)))

get_lower_upper_limits: 9.775336363000001
Example:
range = [[1000, 1500, 2000], [2000, 2500, 3000]]
multiplier = [1, 2]
range_after_multiplication = [[1000, 1500, 2000], [4000, 5000, 6000]]
The min/max limit (or number) that can be produced from the sum of each of the elements in the list is:
min_l = 1000 + 4000 = 5000
max_l = 2000 + 6000 = 8000

Function Arguments
def get_lower_upper_limits(inps):
    ranges, multipliers = inps
    ...

This is a curious choice for declaring arguments to the function.  You must package two arguments together as a tuple for the inps argument, without any clear indication to the caller of the requirement.
Why not declare the function as:
def get_lower_upper_limits(ranges, multipliers):
    ...

The caller then has a clue that the function takes two arguments, a ranges and a multipliers.
A """docstring""" could go a long way towards helping the caller understand the requirements for calling the function.  Type-hints would go even further.  I recommend researching both subjects.
Wrapper
With the above change to the function arguments, your wrapped function will need a new definition:
wrapped = wrapper(get_lower_upper_limits, (ranges, multiplier))

This presently provides 1 argument (the tuple created with (ranges, multiplier)) to be passed to the function being wrapped.  With the above change, the function now needs the ranges and multiplier passed as individual arguments:
wrapped = wrapper(get_lower_upper_limits, ranges, multiplier)

Wrapper-free packaging
The wrapper function is not needed.
First, the partial function from functools will perform the wrapping for you (and provides additional capabilities, which are not needed here).  Remove the wrapper function, and replace it with an import:
from functools import partial

wrapped = partial(get_lower_upper_limits, ranges, multiplier)

This may be faster than your hand-written wrapper function, as built-ins can be implemented directly in C.
Again, as this is not using the full capabilities of partial, a simpler alternative exists:
wrapped = lambda: get_lower_upper_limits(ranges, multiplier)

Don't index
In Python, indexing is relatively slow.  If you do not need idx, except for use in variable[idx], it is best to loop over variable directly.
Since in this case, you need to loop over two lists ranges and multipliers simultaneously, you'll need to zip them together first (zip as in zipper, not as in compression).
    for values, multiplier in zip(ranges, multipliers):
        min_l += multiplier * (min(values) if multiplier > 0 else max(values))
        ...

Improved code
import timeit

def get_lower_upper_limits(ranges, multipliers):
    min_l = 0
    max_l = 0
    for values, multiplier in zip(ranges, multipliers):
        min_l += multiplier * (min(values) if multiplier > 0 else max(values))
        max_l += multiplier * (max(values) if multiplier > 0 else min(values))
    return min_l, max_l

ranges = [[1000, 1400, 1800], [2200, 2400, 2600], [3000, 3100, 3200]]
multiplier = [-1, 2, 3]
assert len(ranges) == len(multiplier)
func = lambda: get_lower_upper_limits(ranges, multiplier)

print('get_lower_upper_limits: {}'.format(timeit.timeit(func, number=500000)))

Approximately 5% faster


Bill shortage: Basic Coding I am a beginner in coding. I was doing this question today.

  A new movie has been released. Many people are buying tickets which costs 25 dollars. There are many people wanting a ticket but each person has either one of   25 dollar bill, 50 dollar bill or 100 dollar bill.

  People will only come in the order of queue. You initially have no money. Write a program to see if you can sell ticket to everyone or not.

This is my solution

#include<iostream.h>
#include<conio.h>
#include<stdlib.h>

void main()
{  clrscr();
    int queue;
    int bill[100];
    cout<<"Enter number of people in queue:";
    cin>>queue;
    cout<<"\n(Bill value 25/50/100)";
    for (int i=0;i<queue;i++)
    {
        cout<<"Bill person "<<i+1<<" have:";
        cin>>bill[i];
    }
    int bill25=0;
    int bill50=0;
    int bill100=0;
    for(i=0;i<queue;i++)
    {
        if(bill[i]==25)
        {   bill25++;
        }
        else if(bill[i]==50)
        {
            if(bill25>0)
            {
                bill25--;
                bill50++;
            }
            else
            {
                cout<<"No change";
                exit(0);
            }
        }
        else if(bill[i]==100)
        {
            if(bill50>0&&bill25>0)
            {
                bill50--;
                bill25--;
                bill100++;
            }
            else if(bill25>=3)
            {
                bill25-=3;
                bill100++;
            }
            else
            {
                cout<<"No change";
                exit(0);
            }
        }
        else
        {
            cout<<"Wrong bill";
            exit(0);
        }
    }
    cout<<"Tickets sold";
    getch();
}

My testing has found this to be correct (maybe not the best but correct) solution. But I am not sure about it. Are there any possibilities in which this code fails. Also what would be a better solution at my level?

Okay, first up, I'm a bit rusty with C++, so I apologize if this isn't formatted correctly.

But, the first thing that leaps out is "Single Responsibility Principle".  Aka - each function should be responsible for one thing and have one reason to change.

Or in short: don't write everything in your Main().

What is your Main() doing, at a high level?  It's:

Grabbing inputs for how many people and what bills they have
It's setting up the register (with no bills in it)
It's looping through each person
It's checking how that person impacts the current register
It's outputting the success/failure of the venture.

Awesome!  So... what should your main look like in an abstract sense?

main()
{
    int patronCount = GetPatronCount();
    int[] billAmounts = GetBillAmounts();
    int[] register = SetUpRegister();
    for (i = 0; i < patronCount; i++)
    {
        bool wasAbleToMakeChange = MakeChangeForPatron(register, billAmounts[i]);
        if (!wasAbleToMakeChange)
        {
            // code to return out with failure
        }
    }
    // code to return success
}

So... why would we do this?  Well, because right now, if you need to tweak a line of code in your code?  That could have ramifications all throughout your function.  You can't exactly digest it in small bits, because everything has 'scope' with everything else.  And if you needed to change something, you'd have to consider how that change affects everything else in that function.

This is incredibly important as you grow as a programmer - you're going to be changing code a lot more than you're going to be composing it - and the simpler you can make changing it, the better off you'll be.

Also, it becomes much easier to see what the program does.  Because the function names and variable names document exactly what it's doing at a high level.  It's getting a patron count.  Imagine someone reading your code (and only your code - no explanations)... how quickly would they figure out what it's doing?  Contrast that to my version's main().  So if someone else needs to work with your code (or if you need to work with it, but don't remember it very well) it'll be able to be grok'ed a lot quicker.


sparse vector dot product I'm trying to implement a sparse vector (most elements are zero) dot product calculation. My major idea is to represent each sparse vector as a list (which holds only non-zero dimensions), and each element in the list is a 2-dimensional tuple -- where first dimension is index of vector, and 2nd dimension is its related value.

Any comments about my code improvement, bugs, etc. are appreciated. I also welcome any good advice for in general how to best implement dot product for sparse vector using Python.

def dot_product(v1, v2):
    i1 = 0
    i2 = 0
    result = 0.0
    while i1 < len(v1) and i2 < len(v2):
        if v1[i1][0] == v2[i2][0]:
            result += v1[i1][1] * v2[i2][1]
            i1 += 1
            i2 += 1
        elif v1[i1][0] > v2[i2][0]:
            i2 += 1
        else:
            i1 += 1

    return result
if __name__ == "__main__":
    v1 = [(0, 2), (1, 4), (5, 6)] # (2,4,0,0,0,6)
    v2 = [(1,3),(2,4),(5,7)] #(0,3,4,0,0,7)
    print dot_product(v1, v2)

Valid. But your code is messy. You are taking one step away from the original traditional problem, and that actually becomes a problem form your code.

Your actually better off with the one dimensional arrays then the two dimensional tuples. Tuples may be the wrong data type to use for your problem to resolve itself nicely.

Styling notes:
This:

def dot_product(v1, v2):
    ...

suggest to me that v1 and v2 are vectors, not a list of vectors, but by changing to:

def dot_product1(v1: [()], v2: [()]):
    ...

the obscurity magically disappears.

This method of commenting your code is called annotations and is commonly used in python, I like them a lot.

The

    ...
    return result
if __name__ == "__main__":
    ...

could be more readable if:

    ...
    return result

if __name__ == "__main__":
    ...

An analogue solution which will scale better is

def get_pointers(v: tuple):
    return {key: value for key, value in enumerate(v) if value}

def sparse_vector_dot(a: dict, b: dict):
    result = 0
    for key, value in a.items():
        if key in b:
            result += b[key] * value
    return result

def main():
    a = (2, 4, 0, 0, 0, 6)
    b = (0, 3, 4, 0, 0, 7)
    a, b = get_pointers(a), get_pointers(b)
    print(sparse_vector_dot(a, b))

and the function sparse_vector_dot could be written as:

def sparse_vector_dot_oneliner(a: dict, b: dict):
    return sum(b[key]*value for key, value in a.items() if key in b)

It will scale better since we are never checking what we don't have to check, the only thing we have to check is if there is a corresponding index in the other list (now dict).


Generating a table of contents with collapsible subsections, PhP or JQuery? The code below generates tables of contents with collapsible subsections from HTML headers. I have done it for a WordPress website, so I can use either PHP or JQuery/JavaScript. I have used PHP to generate the HTML of the ToC and JQuery to collapse/un-collapse the sections.

The code works fine, but I was wondering if it could be simpler/more robust.

PHP fiddle is here: https://paiza.io/projects/fyq6rNEs5H6AO77DkDRRXQ . It outputs HTML code that goes in JSFiddle: https://jsfiddle.net/yu51a5/jasngvfz/37/

PHP code:

    $content="  {yu_TOC}
A1
a1
A2
a2
B1
b1
B2
b2
C1
c1
C2
c2
D1
d1
E1
e1
F1
f1";

    $start_text = strpos($content, '}') + 1;

    $toc_title = 'Table Of Contents';
    $id_title = /*sanitize_title*/($toc_title);

    $index = 1;
    $tableOfContents = "";
    $old_level = 0;
    $maybe_button = "";
    // Insert the IDs and create the TOC.
    $content = preg_replace_callback('#<(h[1-9])(.*?)>(.*?)</\1>#si', function ($matches) use (&$index, &$tableOfContents, &$old_level, &$id_title, &$toc_title, &$maybe_button) {
        $tag = $matches[1];
        $toc_entry_title = strip_tags($matches[3]);
        $hasId = preg_match('#id=("|”)(.*?)\1[\s>]#si', $matches[2], $matchedIds);
        $id = $hasId ? $matchedIds[2] : ($id_title . '-' . $index++ . '-' . /*sanitize_title*/($toc_entry_title));

        $new_level = intval($tag[1]);

        //
 is not closed, because we might want to add a button
        $tableOfContents_entry = "
$toc_entry_title";
        if ($new_level > $old_level) { // $new_level = $old_level + 1
            // adding a button + other html from the previous iteration
            $tableOfContents .= $maybe_button . $tableOfContents_entry;
        } else {
            // close the
's
            for ($x = $new_level; $x < $old_level; $x++) {
                $tableOfContents_entry = "
" . $tableOfContents_entry;
            }
            $tableOfContents .= "
" . $tableOfContents_entry;
        }

        // creating html in case it is needed at the next iteration
        $maybe_button = '<input class="toc_button" id="button_toc_' . $id . '"  type="button" value="+" />

';

        $old_level = $new_level;

        return "<$tag $matches[2] id='$id'>$matches[3]</$tag>↑ Back To $toc_title ↑";
     }, $content);

    // make sure all divs are closed
    $nb_opening_divs = preg_match_all('/
/i', $tableOfContents, $matches);
    $nb_closing_divs = preg_match_all('/<\/div>/i', $tableOfContents, $matches);
    for ($x = $nb_closing_divs; $x < $nb_opening_divs; $x++) {
        $tableOfContents .= "
";
    }

    $result = "
$toc_title
" . '
' . $tableOfContents . '
' . trim(substr($content, $start_text));
    echo $result;

JQuery code:

$('.toc_button').click(function() {
  if ($(this).val() === "-") {
    $(this).val("+");
  } else {
    $(this).val("-");
  }
  $('#div_' + this.id).toggle();
});

I find your php script incredibly hard to read.  Maintaining this script would surely involve substantial time investment.

You are conducting far too much preg_ surgery on input that can be prepared to be valid html.  Processing html is far more reliable via a proper DOM parsing tool.  My preferred tool is DOMDocument and I often pair it with XPath when it makes node targeting clearer/simpler.

I don't have the time to completely rewrite your script, but that is precisely what I recommend.

You shouldn't ever need to perform "mopping up" of your generated html by throwing some more </div> in at the end.


Formatting a CAN bus message as a string I am writing a CAN bus logger application and need to format messages as quick as possible in order to prevent buffer overflows on our device. In some cases I am quick enough, but under heavy loads it is still pretty slow. I believe the bulk of the problem happens in my formatting block (I am using TPL to separate the tasks as much as possible). Here is my consumer block which takes a PassThruMsg[] blocking collection, formats those messages, and passes them into a new blocking collection where they are later printed to a file:

private void FormatMessages(BlockingCollection<PassThruMsg[]> messages, BlockingCollection<string[]> formattedMessages)
{
    try
    {
        PassThruMsg msg;
        String[] formatted;
        foreach (var item in messages.GetConsumingEnumerable(_cancellationTokenSource.Token))
        {
            formatted = new string[item.Length];
            for (uint i = 0; i < item.Length; i++)
            {
                msg = item[i];

                if (_firstTime == 0) _firstTime = msg.timestamp;

                formatted[i] = ((msg.timestamp - _firstTime) / 1000000.0).ToString("0.000000 ");

                // Assumed CAN Protocol
                //TODO:
                string canID = BitConverter.ToString(msg.data, 0, 4).Replace("-", "").TrimStart('0');
                string data = msg.dataSize > 4 ? BitConverter.ToString(msg.data, 4, (int)msg.dataSize - 4).Replace("-", " ") : String.Empty;
                formatted[i] += canID + " " + data;
            }
            formattedMessages.Add(formatted);
        }
    }
    catch (OperationCanceledException)
    {
        //TODO:
    }
}

If it is relevant, here is the snippet where I pass into the BlockingCollection

uint numMsgs;
while (!_cancellationTokenSource.IsCancellationRequested)
{
    numMsgs = (uint)msgs.Length;
    status = j2534.PassThruReadMsgs(channelID, msgs, ref numMsgs, 1000);
    PassThruMsg[] copiedMessages = new PassThruMsg[numMsgs];
    Array.Copy(msgs, copiedMessages, numMsgs);
    if (status == Status.ERR_BUFFER_OVERFLOW)
    {
        _fileWriter.WriteLine("DEVICE INDICATED BUFFER OVERFLOW - MESSAGES LOST!");
        logMessages.Add(copiedMessages);
    }
    else if (status == Status.STATUS_NOERROR || status == Status.ERR_TIMEOUT)
    {
        logMessages.Add(copiedMessages);
    }
    else if (status == Status.ERR_BUFFER_EMPTY)
    {
        throw new Exception(String.Format("PassThruReadMsgs Failed(0x{0:X})", status));
    }
}

And the last node:

private void LogMessages(BlockingCollection<string[]> messages)
{
    try
    {
        foreach (var item in messages.GetConsumingEnumerable(_cancellationTokenSource.Token))
        {
            if (item.Length > 0)
            {
                _fileWriter.WriteLine(String.Join(Environment.NewLine, item));
                _fileWriter.Flush();
            }
        }
    }
    catch (OperationCanceledException)
    {
        //TODO:
    }
}

I have looked this over several times and have been working on it for quite a while, but cannot find any places where this is bottle necking in a way that I can fix. I am just hoping a second set of eyes to review this will see performance issues that I don't.

I would start with extracting the message formatting logic to a separate method. It will make profiling and performance testing easier.

foreach (var item in messages.GetConsumingEnumerable(_cancellationTokenSource.Token))
{
    formatted = FormatSingleMessage(item);
    formattedMessages.Add(formatted);
}

Then you can check the FormatSingleMessage method independently to see if the problem is really there.

When you format the messages, you perform several operations on strings. Strings are immutable, so each operation creates a new string on the heap. All these strings have to be garbage collected, which is another expensive operation.

Try to use a StringBuilder instead and compare performance of both approaches. Depending on the size of the messages, you might see a big improvement.


Project Euler #3 in Java Project Euler #3:

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

Here is my solution:
public class PrimeFactorFinder {

    private static final long NUMBER = 600851475143L;

    public static void main(String[] args) {
        long time = System.nanoTime();
        long result = 0;
        for(int i = 2; i < NUMBER; i++) {
            if(NUMBER % i == 0 && isPrime(NUMBER / i)) {
                result = NUMBER / i;
                break;
            }
        }
        System.out.println("Result: " + result + "\nTime required to calculate in nanoseconds: " + (System.nanoTime() - time));
    }

    private static boolean isPrime(long l) {
        for(long num = 2, max = l / 2 ; num < max; num++) {
            if(l % num == 0) {
                return false;
            }
        }
        return true;
    }

}

Output:

 Result: 6857
 Time required to calculate in nanoseconds: 1154999669

Questions:

I am especially annoyed with the isPrime() method. Is there a better way to do this? How?
Any general comments?

Algorithm

Your current method relies on iterating through all numbers and checking if they're 1) a factor 2) prime.

With the new algorithm, not only are we able to skip the expensive primality check, but we're also able to reduce our search space massively (the upper bound is the square root of the number).

public static long maxPrimeFactor(long n) {
    long factor = -1;
    for (int i = 2; i * i <= n; i++) {
        if (n == 1) { break; }
        if (n % i != 0) { continue; }
        factor = i;
        while (n % i == 0) {
            n /= i;
        }
    }
    return n == 1 ? factor : n;
}

This algorithm is a slightly optimized version of Martin R's answer.

We first "trim" down a number by repeatedly dividing it by factors of itself which are smaller or equal to its square root. Here're some examples

[3] (no factors <= sqrt(3))
[4 -> 2 -> 1] (factors: 2)
[200 -> 100 -> 50 -> 25] --> [5 -> 1] (factors: 2, 5)
[7007 -> 1001 -> 143] --> [13] (factors: 7, 11. 13 > sqrt(143), hence it stops)

The square brackets represent groups of "divisions" where a number is repeatedly divided by the factor.

Most composite numbers will be reduced down to 1. This is because all prime factors of a composite number will always be lower than or equal to its square root. However, note that as an optimization, we trim down the upper bound to the square root of the new number upon every iteration. This results in a few cases where we stop at a prime number. In that case, we return that number, which is the largest prime factor.

Note that we only need to iterate through primes. Since a prime sieve is too expensive, iterating through {2, 3, 5, 7, ..., sqrt(n)} works well enough for our scenario. For clarity, my code iterates through all integers from 2 to sqrt(n); I'll leave this to the OP as an exercise.

Benchmarks

In these benchmarks, my algorithm refers to the optimized version of the algorithm, as mentioned in the previous paragraph.

OP's algorithm:

Result: 6857
Time required to calculate in nanoseconds: 870707572

h.j.k.'s algorithm:

Result: 6857
Time required to calculate in nanoseconds: 864074780

Slight improvement there, but not much.

JS1's algorithm:

Result: 6857
Time required to calculate in nanoseconds: 10038730

A definitely massive improvement, but is still a bit slow since the primality tests are too slow.

thepace's algorithm is currently a WIP.

From thepace's algorithm onwards, the benchmarks run too fast to be effective. Using 1000 trials:

Algorithm: wei2912's algorithm
Average time per trial (ns): 15507
Algorithm: Alnitak's algorithm
Average time per trial (ns): 10880

Looks like Alnitak's algorithm is faster.

1000000 random ints

Generating a million random ints and feeding them in:

Algorithm: wei2912's algorithm
Average time per trial (ns): 12337
Algorithm: Alnitak's algorithm
Average time per trial (ns): 7661

Alnitak's algorithm appears to be nearly 2 times faster. Great job to Alnitak! :)

I'd be glad to accept any suggestions for benchmarks.

My original solution in Haskell is as follows:

module Main where

intSqrt :: Integer -> Integer
intSqrt = floor . sqrt . fromIntegral

maxPrimeFact :: Integer -> Integer
maxPrimeFact n = go n 1 (2 : [3, 5..intSqrt n])
  where
    go 1 maxPrime _ = maxPrime
    go n _ [] = n
    go n maxPrime wheel@(w:ws)
        | m == 0 = go d w $ takeWhile (<= intSqrt d) wheel
        | otherwise = go n maxPrime ws
        where
            (d, m) = n `divMod` w

problem3 :: Integer -> Integer
problem3 = maxPrimeFact

main :: IO ()
main = do
    _ <- getLine
    contents <- getContents
    let cases = map read $ lines contents
    let results = map problem3 cases
    mapM_ print results

Credit must go to Martin R for the original solution.


Python Bingo game that stores card in a dictionary I am super proud of this python bingo game I wrote. I am posting it here to make get some help reviewing it to make sure I'm using the best practices. This is for a class, and my professor is a stickler for using best practices. I went above and beyond what the assignment called for so I need a little more helping making sure the code is okay. Any help is appreciated!

Here is the code:

# Author: Helana Brock
# Homework 2, problem 3

import random

random_draw_list = random.sample(range(1, 76), 75)

def generate_card():
    """
    Generates a bingo card and stores the numbers in a dictionary.
    """
    card = {
        "B": [],
        "I": [],
        "N": [],
        "G": [],
        "O": [],
    }
    min = 1
    max = 15
    for letter in card:
        card[letter] = random.sample(range(min, max), 5)
        min += 15
        max += 15
        if letter == "N":
            card[letter][2] = "X" # free space!
    return card

def print_card(card):
    """
    Prints the bingo card.

    Arguments:
        card (dictionary): The card to be printed out.
    """
    for letter in card:
        print(letter, end="\t")
        for number in card[letter]:
            print(number, end="\t")
        print("\n")
    print("\n")

def draw(card, list):
    """
    Pops a number off a list of random numbers. Using the pop method ensures no duplicate
    numbers will be drawn.

    Arguments:
        card (dictionary): The card to to check for the number that was drawn.
        list (list): The list of random numbers to be drawn from.
    """
    number_drawn = random_draw_list.pop()
    for letter in card:
        i = 0
        for number in card[letter]:
            if number == number_drawn:
                card[letter][i] = "X"
            i += 1
    return number_drawn

def check_win(card):
    """
    First checks for diagonal wins, then four-corner, then horizontal, and finally, vertical.

    Arguments:
        card (dictionary): The card to check for a win.
    """
    win = False
    if card["B"][0] == "X" and card["I"][1] == "X" and card["N"][2] == "X" and card["G"][3] == "X" and card["O"][4] == "X":
        win = True
    elif card["O"][0] == "X" and card["G"][1] == "X" and card["N"][2] == "X" and card["I"][3] == "X" and card["B"][4] == "X":
        win = True
    elif card["B"][0] == "X" and card["O"][4] == "X" and card["B"][4] == "X" and card["O"][0] == "X":
        win = True
    for letter in card:
        if(len(set(card[letter]))==1):
            win = True
    for i in range(5):
        cnt = 0
        for letter in card:
            if card[letter][i] == "X":
                cnt += 1
        print(cnt)
        if cnt == 5:
            win = True
            break
    return win

def main():
    """
    Main method for the program. Generates a card, prints it, and loops through drawing
    and printing until the check_win method returns True or the user enters "quit".
    """
    print("Let's play bingo!")
    card = generate_card()

    print("\nHere is your card:\n")
    print_card(card)

    print("\nKeep pressing enter to continue or type quit to exit.\n")
    user_input = input()
    win = check_win(card)
    balls_till_win = 0

    while win == False and user_input != "quit":
        number_drawn = draw(card, random_draw_list)
        balls_till_win += 1

        print(f"\nNumber drawn: {number_drawn}.")
        print(f"Total balls drawn: {balls_till_win}.\n")
        print_card(card)

        win = check_win(card)

        user_input = input()
    if win == True:
        print(f"\nBingo! Total balls to win: {balls_till_win}.")
    else:
        print("Goodbye! Thanks for playing!")

main()

Style

Python comes with an "official" Style Guide for Python Code (aka PEP 8) which is a widely accepted standard for naming conventions and the like. Since you say your professor is keen on using best practices, I'd recommend to have a look at those guidelines. There is also a variety of tools that can help you to check for those best practices automatically. A non-exhaustive list (including Pylint) can be found in this meta-post here on Code Review.

From what I have seen, your code looks quite good in that regard. I especially like your docstrings. The most prevalent issue in your code would be that there are usually two blank lines between different function definitions in order to make their separation more clear.

Code

Idiomatic Python

As @nz_21 already mentioned, if sth == True: and also if sth == False: can be expressed in a more idiomatic way as if sth: and if not sth: (Note: Pylint would also catch this particular "mistake".)

Redefining built-ins

Some of the variable names you have chosen (min, max, list) redefine built-in functions from the Python core. You should avoid that. The simplest way is to append a single trailing underscore, e.g. list_, in order to avoid that issue. Also the parameter list seems to be unused in draw(...).

Random number generation

card[letter] = random.sample(range(min_, max_), 5) could also be rewritten as card[letter] = [random.randrange(min_, max_) for _ in range(5)]. Although I don't think that it will make a big difference in your case. The documentation explicitly list them as alternatives:

  random.randrange(start, stop[, step])

  Return a randomly selected element from range(start, stop, step). This
  is equivalent to choice(range(start, stop, step)), but doesn’t
  actually build a range object.

From that it seems that the later approach would make more sense for a large range of values to choose from.

Edit: removed after hint in the comments with regard to sampling with and without replacement. Thanks to @Wombatz.

Global variables

random_draw_list is used as a global variable, but unfortunately not as a constant as one might expect, but as a mutable global variable. That makes it harder to reuse code. Since draw(...) is the only function using this variable, it makes no sense to have it as a global variable. Instead, it should defined in main() after you have changed draw(...) to accept it as parameter (maybe that's what list was supposed to do in the first place?) and passed when calling draw(...) from main().

Top-level script environment

In order to show what part of the file is actually supposed to be run as a script, Python defines the concept of a top-level script environment. Although the name sounds daunting at first, it's quite likely that you have seen it in someone else's Python code: if __name__ == "__main__": This tells the Python interpreter to only run that part of code if the file is used as a script. In your case all there is to do is to do add the above right before calling the main-function:

if __name__ == "__main__":
    main()

That also helps you to avoid the issue that Python always starts a new bingo game whenever you try to import a function from your file ;-)


Converting a 2D array to a JSON list I am wondering if I could implement this in a cleaner, more elegant, strictly functional way:

const convert2dArrayToJsonList = (array) => {

    if (!is2dArrayParsableToJsonList(array)) {
        throw new Error("The 2D array cannot be converted to a json list" + array);
    }

    const propertyKeys = array[0];

    return array
        .slice(1)
        .map( row => {
            return row.reduce( (accumulatedElement, propertyValue, currentIndex, array) => {

                accumulatedElement[propertyKeys[currentIndex]] = propertyValue;
                return accumulatedElement;
            }, {});
        });
}

The implementation of is2dArrayParsableToJsonList(array) is not relevant in this context, it does what it says.

The 2D array parameter has the property keys in the top row and all other rows represent individual elements in the list.

Here's what I've come up with;

import expect from 'expect';

const is2dArrayParsableToJsonList = () => true;

const convert2dArrayToJsonList = (array) => {

    if (!is2dArrayParsableToJsonList(array)) {
        throw new Error("The 2D array cannot be converted to a json list" + array);
    }

    const [propertyKeys, ...rows] = array;
    const rowToObject = row => Object.assign(... propertyKeys.map( (key,idx) => ({ [key]: row[idx] })));
    return rows.map(rowToObject);
}

const input = [
    ['foo', 'bar', 'baz'],
    [1,2,3],
    [4,5,6]
];

const output = [
    {foo:1, bar:2, baz:3},
    {foo:4, bar:5, baz:6},
];

expect(convert2dArrayToJsonList(input)).toEqual(output);

Core idea is that map gives you both a value and index. So when you map over the keys, we see 'foo/1', 'bar/2', 'baz/3'.


Extract index of first unique element in large array in Swift I'm using the following code to return the first index of a unique character in a large String. It works fine until I get to large strings, where it times out.

Is there a faster way to accomplish the goal of getting a hold of the unique character's index using NSCountedSet?

Update

The string contains 25,000 characters. I refactored the original post to extract the unique chars, then cycle through the array and see if each index is contained within the uniqueChar array. It's a little faster, but not fast enough to pass Leetcode's timer.

func firstUniqChar(_ s: String) -> Int {
    guard Set(s.characters).count > 0 && s.characters.count > 0 else { return -1 }
    let stringArray = s.characters.map({String($0)})

    let countedSet = NSCountedSet(array: stringArray)

    var uniqueChars: [String] = []

    for char in countedSet {
        if countedSet.count(for: char) == 1 {
            uniqueChars.append(String(describing: char))
        }
    }

    for index in 0..<stringArray.count {
        if uniqueChars.contains(stringArray[index]) {
            return index
        }
    }

    return -1
}

Your initial test

guard Set(s.characters).count > 0 && s.characters.count > 0 else { return -1 }

is not needed, the remaining code already handles the case of an
empty string.

Determining the unique characters from countedSet can simpler be done
with a filter operation instead of a for-loop:

let uniqueChars = countedSet.filter {
    countedSet.count(for: $0) == 1
} as! [String]

But actually that list is not needed at all because all you have to do
in the final loop is to find the first character which has a count
of one. The function then looks like this:

func firstUniqChar(_ s: String) -> Int {
    let stringArray = s.characters.map({String($0)})
    let countedSet = NSCountedSet(array: stringArray)
    for index in 0..<stringArray.count {
        if countedSet.count(for: stringArray[index]) == 1 {
            return index
        }
    }
    return -1
}

which is simpler and a bit faster than the original one.

This can further be improved by avoiding the conversion of each
character to a string and the array, and operating on the UTF-16
view of the given string directly:

func firstUniqChar(_ s: String) -> Int {
    let countedSet = NSCountedSet()
    for char in s.utf16 {
        countedSet.add(char)
    }
    for (index, char) in s.utf16.enumerated() {
        if countedSet.count(for: char) == 1 {
            return index
        }
    }
    return -1
}

NSCountedSet is from the Foundation library and works with
NSObject instances. The previous method works because the
UInt16 value is automatically wrapped into an object when
added to the counted set. This conversion can be avoided by
using a native Swift dictionary instead, which makes the
code much faster:

func firstUniqChar(_ s: String) -> Int {
    // Map from character to number of occurrences:
    var counts: [UInt16: Int] = [:]

    for char in s.utf16 {
        if let cnt = counts[char] {
            counts[char] = cnt + 1
        } else {
            counts[char] = 1
        }
    }
    for (index, char) in s.utf16.enumerated() {
        if counts[char]! == 1 {
            return index
        }
    }
    return -1
}

Benchmarks. Test code:

let s = String(repeating: "abcdefghijklmnopqrstuvwxy", count: 1000) + "z" + String(repeating: "abcdefghijklmnopqrstuvwxy", count: 1000)
print(s.characters.count) // 50001

let start = Date()
let i = firstUniqChar(s)
let end = Date()

print(i, end.timeIntervalSince(start))

Results (on a 3.5 GHz Intel Core i5 iMac, compiled in Release
configuration):

Your original function:  0.084 sec
First improvement:       0.058 sec
Second improvement:      0.014 sec
Last function:           0.003 sec

The last method can be more compactly written as

func firstUniqChar(_ s: String) -> Int {
    // Map from character to number of occurrences:
    var counts: [UInt16: Int] = [:]

    for char in s.utf16 {
        counts[char] = (counts[char] ?? 0) + 1
    }

    let index = s.utf16.enumerated()
        .first(where: { counts[$0.element]! == 1 })?
        .offset
    return index ?? -1
}

without changing the performance.


Initializing an array of string constants, and another constant array of the equivalent lengths Ok, a struct would actually be most convenient, but it woudl be easier on teh system to have 2 seperate arrays rather than an array of structs.
#define PAIR(Str) {(Str), sizeof(Str)-1}
static const struct StrStruct {
    const char *Str;
    unsigned int Len;
} Packed4 DefaultStr[] = {
    PAIR("Error"),
    PAIR("An unexpected error occurred"),
    PAIR("while saving data"),
    PAIR("Close some windows or programs and try again"),
    PAIR("Singleplayer"),
    PAIR("Multiplayer"),
    PAIR("Settings"),
    PAIR("Quit"),
    PAIR("Graphics"),
    PAIR("User Interface"),
    PAIR("Sounds"),
    PAIR("Controls"),
    PAIR("Language"),
    PAIR("Compatibility"),
    PAIR("Help"),
    PAIR("About"),
    PAIR("On"),
    PAIR("Off"),
    PAIR("Field of View"),
};

Simply hardcoding them is 'magic numbers'.
static const char *const Strs[] = {
    "Singleplayer",
    "Multiplayer",
    "Settings",
    "Quit",
    "Graphics",
    "User Interface",
    "Sounds",
    "Controls",
    "Language",
    "Compatibility",
    "Help",
    "About",
    "On",
    "Off",
    "Field of View"
};
static const unsigned short Lengths[] = {
    12,
    11,
    8,
    //...
};

So I came up with my current solution, though kind of a pain in the backside, I have this.
#define ERROR "Error"
#define UNEXPECTEDERROR "An unexpected error occurred"
#define WHILESAVINGDATA "while saving data"
#define CLOSEPROGRAMS "Close some windows or programs and try again"
#define SINGLEPLAYER "Singleplayer"
#define MULTIPLAYER "Multiplayer"
#define SETTINGS "Settings"
#define QUIT "Quit"
#define GRAPHICS "Graphics"
#define UI "User Interface"
#define AUDIO "Audio"
#define CONTROLS "Controls"
#define LANGUAGE "Language"
#define COMPATIBILITY "Compatibility"
#define HELP "Help"
#define ABOUT "About"
#define ON "On"
#define OFF "Off"
#define FOV "Field of View"
static const char *const DStr[] = {
    ERROR,
    UNEXPECTEDERROR,
    WHILESAVINGDATA,
    CLOSEPROGRAMS,
    SINGLEPLAYER,
    MULTIPLAYER,
    SETTINGS,
    QUIT,
    GRAPHICS,
    UI,
    AUDIO,
    CONTROLS,
    LANGUAGE,
    COMPATIBILITY,
    HELP,
    ABOUT,
    ON,
    OFF,
    FOV,
};
#define STRLEN(Str) (sizeof(Str)-1)
static const unsigned short DLen[] = {
    STRLEN(ERROR),
    STRLEN(UNEXPECTEDERROR),
    STRLEN(WHILESAVINGDATA),
    STRLEN(CLOSEPROGRAMS),
    STRLEN(SINGLEPLAYER),
    //...
};

Is there a better way to do this? Is there a cleaner way to initialize an constant array of strings, and a constant array of the equivalent lenghts? I'm mainly looking at the last one. How can I improve that one?

To expand on my comment, the X macro technique can be used for this. See here, for example
The idea is that we define the list once, for example
#define STRINGLIST \
X( "alice") \
X( "bob") \
X( "cat")

When we want to use this list, we invoke the above macro, having defined the macro X:
static const char *const DStr[] = {
#define X(S) S,
STRINGLIST
#undef X
};

static const unsigned short DLen[] = {
#define X(S) sizeof( S)-1,
STRINGLIST
#undef X
};

This way we only have to define the strings once, and guarantee that the arrays DStr and DLen are in the same order. The disadvantage is that it looks pretty bizarre first time you see it, and others maintaining your code might be boggled.


Simple Countdown Timer Beginner level script for a countdown timer. Would appreciate if you could point the things I could do to be more efficient.
Basically, I would want to check if the current day is either a "workday"(Mon - Wed) or a "freeday"(Thur-Sun) and achieve the following:

If it's a workday, and the current hour is a working hour (7:00:00 AM - 5:00:00 PM), then count
how many hour(s) remaining till end of the shift (5PM) base on the current time.

If it's a workday, and the current hour is outside of working hours (5:00:01 PM to 6:59:59 AM), say that it is not yet a working hour.

If it's a freeday, count till the next Monday.

I wouldn't want to rely on the if statements if possible, but my understanding level for Python is at this point only. Is there a better way to do it?
Thank you in advance! Happy coding.
import time, datetime

# Add the value to get the next Monday
get_future_monday = {
    0: 1,
    1: 2,
    2: 3,
    3: 4
}

def time_state():
    # Check what day of the week
    # 0 == Monday, 6 == Sunday
    check_day = datetime.datetime.today().weekday()

    # Get future Monday
    future_monday = 6 - check_day

    # Check current time
    check_time_now = datetime.datetime.now()

    # Parse current time
    # Get the year, month and day
    get_year = check_time_now.year
    get_month = check_time_now.month
    get_day = check_time_now.day

    # Define timespan for each routine
    working_hours = datetime.time(7, 0, 0)
    after_hours_am = datetime.time(6, 59, 59)
    after_hours_pm = datetime.time(17, 0, 1)

    if check_day == 0 and check_time_now.time() < after_hours_am: # If Monday and current time not yet 7 AM
        print("It's not time yeeeet!")
        return datetime.datetime(get_year, get_month, get_day, 7, 0, 0)

    elif check_day == 2 and check_time_now.time() > after_hours_pm: # If Wednesday and current time past 5 PM
        print("Time to gooooo!")
        return datetime.datetime(get_year, get_month, (get_day + get_future_monday[future_monday]), 7, 0, 0)

    elif check_day <= 2 and check_time_now.time() < after_hours_am: # If day in Tuesday, Wed and current time not yet 7 AM
        print("Get ready to grind...")
        return datetime.datetime(get_year, get_month, get_day, 7, 0, 0)

    elif check_day <= 2 and check_time_now.time() > after_hours_pm: # If day in Tuesday, Wed and current time not yet 5 PM
        print("Free at last!")
        return datetime.datetime(get_year, get_month, (get_day + 1), 7, 0, 0)

    elif check_day <= 2 and check_time_now.time() >= working_hours and check_time_now.time() <= after_hours_pm: # If day in Mon - Wed and current time not yet 5 PM
        print("Focus focus focus!!!")
        return datetime.datetime(get_year, get_month, get_day, 17, 0, 0)

    else:
        print("Playtime foo UwU")
        return datetime.datetime(get_year, get_month, (get_day + get_future_monday[future_monday]), 7, 0, 0) # Playtime

def countdown(time_state):
    time_state = time_state

    clock_tick = time_state - datetime.datetime.now()

    days = clock_tick.total_seconds() / 86400
    hours, rem = divmod(clock_tick.total_seconds(), 3600)
    mins, secs = divmod(rem, 60)

    print(f"{int(days):02d}:{int(hours):02d}:{int(mins):02d}:{int(secs):02d}")
    time.sleep(1)

def main():
    while True:
        a = time_state()
        countdown(a)

main()
```

Variable names
I find the get_ prefixes unnecessary on these:
get_year = check_time_now.year
get_month = check_time_now.month
get_day = check_time_now.day

That makes them look like functions.
Boundaries
This:
working_hours = datetime.time(7, 0, 0)
after_hours_am = datetime.time(6, 59, 59)

should probably just use 7:00 for after_hours_am. Otherwise, the time 6:59:59 given your current usage will still be considered working hours.
Likewise, this:
check_time_now.time() > after_hours_pm

should use >=.
An easy convention to follow that makes your boundary checking consistent is to always use intervals that are closed at the beginning and open at the end, i.e. 7:00 <= time < 8:00.
Redundant else
This:
    return datetime.datetime(get_year, get_month, get_day, 7, 0, 0)

elif ...

can use an if instead of an elif due to the previous return.
Don't repeat yourself
This return:
    return datetime.datetime(get_year, get_month, get_day, 7, 0, 0)

shares many commonalities with the other returns in its function. Factor out a new_day and new_hour variable assignment, then at the end of your function,
return datetime.datetime(get_year, get_month, new_day, new_hour)

Note that 0 is the default for the remaining parameters.
Timespan division
Trying not to do your own time math, this would be one option that doesn't require knowledge of time position multiples:
days = clock_tick.days
rem = clock_tick - timedelta(days=days)
hours = rem // timedelta(hours=1)
rem -= timedelta(hours=hours)
mins = rem // timedelta(minutes=1)
rem -= timedelta(minutes=mins)
secs = int(rem.total_seconds())

I don't like it, but built-in Python time libraries are kind of bad. Third-party libraries will reduce this to a one-liner format call. This is educational:
https://stackoverflow.com/questions/538666/format-timedelta-to-string


Building a histogram array in PHP I'm visualizing my stored data in an histogram similar to this one:

However, I'm sure that my code smells a lot! So I'd like to have your opinion on how to make it better.

$listings = array(
              array('Price' => 10),
              array('Price' => 10),
              array('Price' => 11)
            ); // so on and so forth, this data is pulled from the database.

// flattening the array so I remove the "Price" key
// the array takes the form of array(10,10,11,...)
foreach($listings as $listing) {
    $flattenedListings[] = $listing['Price'];
}

$widths = range(0, 100, 10); // creates array of the form: array(0, 10, 20, 30, 40, ...)
$bins = array();
$isLast = count($widths);
foreach($widths as $key => $value) {
    if($key < $isLast - 1) {
        $bins[] = array('min' => $value, 'max' => $widths[$key+1]);
    }
}

// creates array of the form:
// $bins = array(
//      array('min'=>0, 'max' => 10),
//      array('min'=>10,'max' => 20),
//      array('min'=>30, 'max'=>40)
//     );

$histogram = array();
        foreach($bins as $bin) {
            $histogram[$bin['min']."-".$bin['max']] = array_filter($flattenedListings, function($element) use ($bin) {
                if( ($element > $bin['min']) && ($element <= $bin['max']) ) {
                    return true;
                }
                return false;
            });
        }

// Last one is a bit complicated, but basically what it does is that it creates an array of ranges as keys, so it generates this:

// array('0-10' => array(1, 2, 3, 4, 5, 6),
//         '10-20' => array(11, 19, 12),
//         '20-30' => array(),
//   );
// Or in other words: foreach range in the histogram, php creates an array containing      values within the allowed limits.

    foreach($histogram as $key => $val) {
        $flotHistogram[$key] = (is_array($val)) ? ( (count($val)) ? count($val) : 0 ) : 0;
    }

// And finally it just counts them, and returns a new array.

As you can see in my code, I'm making an excessive use of foreach loops to basically do a "simple task."

Basically what I have is a list of items along with their prices, and I want to count how many items belong to each range.  My ranges are 10 numbers apart: (0 - 10, 10 - 20, 20 - 30, etc...)

Ideally the output should tell me how many items there are in a given category, like 3 items in the range of $10 - $20, 2 items in the range of $20 - $30 and 1 item in the range of $30 - $40, etc... In the following format:

array('0-10' => 0, '10-20' => 3, '20-30' => 2, '30-40' => 1);

Given the following input:

array(
    array('Price' => 11),
    array('Price' => 12),
    array('Price' => 20),
    array('Price' => 25),
    array('Price' => 20.1),
    array('Price' => 33.5)
);

Additional info:

I'm pulling my prices from a MySQL database using PDO option PDO::FETCH_ASSOC.  How could the flattening of the array be skipped (instead of fetching an array of arrays, just fetch the numbers in a 1 dimensional array)?

Your code isn't exactly terrible, but this is code-review, and good code review has to be harsh, so please keep in mind that I mean this in the nicest of ways.
That said, there are some issues I'd like to address:

Redundancies - DNRY:
That old dead horse: Do Not Repeat Yourself. You have a lot of redundant code. Of course, you can get into code-golfing, and try to write everything as a one-liner. That will result in unmaintainable code, and that's not what we want, but in your case, there is room for some minor improvements that won't harm readability one bit, quite the contrary.

You say you're getting the data (prices, I take it) from a PDO resultset, why, then, don't you fill the $flattenedListings array like so:

$flattenedListings = array();
while($row = $pdoResOrStmt->fetch(PDO::FETCH_ASSOC))
{
    $flattenedListings[] = $row['Price'];
}

That's going to reduce the resources your script requires a tad, and fetches the data while at the same time constructing the flattened array.
Next, the $isLast = count($widths); variable can be ditched all together, either by a simple foreach, very close to to the loop you currently have:

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key +1])) break;//break on the last key
    $bins[] = array(
        'min' => $val,
        'max' => $widths[$key + 1]
    );
}

But, and this might be personal, I dislike this if() break; thing in a loop. Personally, I'd probably write something like:

for($i=0, $j=count($widths) -1; $i<$j;++$i)
{//count -1 = last key, but $i <$j => next to last key
    $bins[] = array(
        'min' => $widths[$i],
        'max' => $widths[$i+1],//can get last key
    );
}

Next, the $histogram. You're using a closure function + array_filter to group your values according to the price range. In itself, it's quite easy to read, and not that inefficient. But I find that closures in PHP (which are objects, instances of Closure) are 9/10 overkill. They're also not a "natural" feature of the language. PHP isn't a functional language, like Lisp, Scheme or JavaScript are. I've compared them to drag queens: A man can wear a dress, stockings, a wig and put on make-up as much as he wants, it still doesn't change the fact that he's a man.
Often, what you end up with is a grotesque, bemusing performance that leaves some feeling ill at ease.

There is a far easier way of determining the range for each price:

$histogram = array();
foreach($flattenedListings as $price)
{//assuming nothing costs 0
    $key = floor(($price -1)/10);
    $range = $bins[$key];
    $key = $range['min'] .'-'. $range['max'];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $histogram[$key][] = $price;
}

If some price can be zero, then just replace:

$key = floor(($price -1)/10);

with

$key = $price ? floor(($price -1)/10) : 0;

And you're good to go.

Simplify:
Now we've got that sorted out, it's time to simplify things a bit. As you can see by the way I've filled $histogram, I don't actually need the min and max values from the $bins array, so instead of filling that array with array('min'=> $x, 'max'=> $x+1), I could easily have written:

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}

This also makes filling the $flotHistogram array a lot easier:

$flotHistogram = array_fill_keys($bins, 0);//set all keys to 0

Then, when filling the $histogram:

$histogram = array();
foreach($flattenedListings as $price)
{
    $key = $bins[floor(($price -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;//add 1 count
    $histogram[$key][] = $price;
}

This leaves us with the following code:

//get prices
$flattenedListings = array();
while($row = $pdoResOrStmt->fetch(PDO::FETCH_ASSOC))
{
    $flattenedListings[] = $row['Price'];
}
//construct range-keys array
$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}
//construct flotHistogram count array
$flotHistogram = array_fill_keys($bins, 0);
//construct array of prices for each key
$histogram = array();
foreach($flattenedListings as $price)
{
    $key = $bins[floor(($price -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;
    $histogram[$key][] = $price;
}

A bridge too far(?)
I may be taking things a bit too far, but that $flattenedListings array seems a bit redundant, now. However I don't know what else you need the db results for, but all in all, you may want to consider doing something like

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}

$flotHistogram = array_fill_keys($bins, 0);

$histogram = array();
//query data:
while($row = $pdoVar->fetch(PDO::FETCH_ASSOC))
{
    $key = $bins[floor(($row['Price'] -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;
    $histogram[$key][] = $row['Price'];
}

Which reduces your code even more. Of course, if you find this too hard to read (imagine, a couple of months from now, will you still be able to make sense of this code), then don't use it, add more comments and don't feel shy to be a bit more verbose or repetitive.


Compute Manhattan distance from origin given a set of left-right-step directions I am solving the problems for Advent of Code 2016 to learn Rust programming. The prompt for the first problem can be summarized:

I start at position 0,0 on a regular grid. I am given a set of directions to get to a location. I can only travel in "steps" on this grid, and I am only given directions in the form (e.g.):

R5, L2, L1, R1, R3, R3, L3, R3, R4, L2, R4, L4, R4

Where the first character is the direction to turn right or left and the following number is the number of steps to take. I need to compute the Manhattan distance between my starting point and the ending point.

The instructions are saved in a text file called "2016-1.txt".

use std::fs;

struct Pos {
    facing: char,
    x: i32,
    y: i32,
}

fn split_dir(dir_str: &str) -> Vec<&str> {
    dir_str.split(", ").collect()
}

fn update_facing(rel_dir: &char, face_char: &char) -> char {
    if *rel_dir == 'L' {
        match face_char {
            'N' => 'W',
            'S' => 'E',
            'E' => 'N',
            'W' => 'S',
            _ => 'I', // Is there a better way to handle the catch-all?
        }
    } else {
        match face_char {
            'N' => 'E',
            'S' => 'W',
            'E' => 'S',
            'W' => 'N',
            _ => 'I',
        }
    }
}

fn update_x(pos_x: i32, face_char: char, move_num: i32) -> i32 {
    match face_char {
        'E' => pos_x + move_num,
        'W' => pos_x - move_num,
        _   => pos_x
    }
}

fn update_y(pos_y: i32, face_char: char, move_num: &i32) -> i32 {
    match face_char {
        'N' => pos_y + move_num,
        'S' => pos_y - move_num,
        _   => pos_y
    }
}

fn get_manhattan_dist(pos_x: i32, pos_y: i32, origin_x: i32, origin_y: i32) -> i32 {
    (pos_x - origin_x).abs() + (pos_y - origin_y).abs()
}

fn main() {
    let s = fs::read_to_string("2016-1.txt")
        .expect("Failed to read file.");

    let split: Vec<&str> = split_dir(&s);

    let mut pos: Pos = Pos {x: 0, y: 0, facing: 'N'};

    for inst in split {
        // Update direction
        let rel_dir = inst.chars().nth(0).unwrap(); // Get first character of the instruction
        pos.facing = update_facing(&rel_dir, &pos.facing);

        // Update position
        let move_num = &inst[1..].parse::<i32>().unwrap();
        pos.x = update_x(pos.x, pos.facing, *move_num);
        pos.y = update_y(pos.y, pos.facing, &move_num);
    }

    let dist = get_manhattan_dist(pos.x, pos.y, 0, 0);
    println!("{}", dist);
}

I am particularly interested in error handling. Particularly in the update_facing function. This is my first Rust program, so all advice is warranted as well.

Your code looks more complicated than necessary for this task.

The first thing I noticed was the directions N, E, S, W. There is no need to explicitly name them. It is simpler to just define a direction as a 2-dimensional tuple:

struct Direction {
    dx: i32,
    dy: i32
}

That's the essence of a direction. As the next step, I remembered that rotating such a direction by 90 degrees is quite simple. It just involves swapping the coordinates and reversing one of them. To get these right, I manually checked all the combinations after writing this code:

impl Direction {
    fn left(&self) -> Direction { Direction { dx: self.dy, dy: -self.dx } }
    fn right(&self) -> Direction { Direction { dx: -self.dy, dy: self.dx } }
}

By these simple definitions, I avoided dealing with N, E, S, W at all.

When I tested the program using the example you provided, my IDE added a trailing newline to the file, as is usual for text files. Then the program crashed because it could not parse an empty string. Therefore I changed split_dir(&s) into split_dir(s.trim()), and it worked.

To understand the main program, I separated it into the part that deals with input and output, and the processing part in between. To do this, I defined this function:

fn manhattan_distance(s: &str) -> i32 {
    ...
}

I inlined the split_dir, update_x, update_y and get_manhattan_dist functions, and in the end my code became:

use std::fs;

struct Pos {
    x: i32,
    y: i32,
    dir: Direction,
}

struct Direction {
    dx: i32,
    dy: i32,
}

impl Direction {
    fn left(&self) -> Direction {
        Direction {
            dx: self.dy,
            dy: -self.dx,
        }
    }

    fn right(&self) -> Direction {
        Direction {
            dx: -self.dy,
            dy: self.dx,
        }
    }
}

fn manhattan_distance(s: &str) -> i32 {
    let steps = s.trim().split(", ");

    let mut pos = Pos {
        x: 0,
        y: 0,
        dir: Direction { dx: 0, dy: -1 },
    };

    for step in steps {
        let (turn, dist) = step.split_at(1);

        // Update direction
        pos.dir = match turn {
            "L" => pos.dir.left(),
            "R" => pos.dir.right(),
            _ => panic!("invalid turn {} in step {}", turn, step),
        };

        // Update position
        let dist = dist.parse::<i32>().unwrap();
        pos.x += pos.dir.dx * dist;
        pos.y += pos.dir.dy * dist;
    }

    pos.x.abs() + pos.y.abs()
}

fn main() {
    let s = fs::read_to_string("2016-1.txt").expect("Failed to read file.");

    println!("{}", manhattan_distance(&s));
}

What's left now are some automatic tests. The function manhattan_distance is well-prepared for that since it has no side-effects, does not need any input or output, gets its parameter as a simple string and just returns its result.

And here are some example tests. You should add some more to explore other interesting cases, like crossing the x or y axis. The current tests might also pass if you omit the calls to abs.

#[cfg(test)]
mod tests {
    use crate::manhattan_distance;

    #[test]
    fn manhattan_distance_example() {
        assert_eq!(11, manhattan_distance("R5, L2, L1, R1, R3, R3, L3, R3, R4, L2, R4, L4, R4"))
    }

    #[test]
    fn manhattan_distance_empty() {
        assert_eq!(0, manhattan_distance(" \t\n"))
    }

    #[test]
    fn manhattan_distance_simple() {
        assert_eq!(13, manhattan_distance("R8, L5"))
    }

    #[test]
    fn manhattan_distance_rectangle() {
        assert_eq!(0, manhattan_distance("R8, L5, L8, L5"))
    }
}

Your code is a good working base, it was just longer than necessary. I also changed most of the variable names to be a little more precise and easier to grasp for a casual reader of the code. For example, since the task talks about "steps", it's only natural to name the corresponding variables in the code also "steps" and "step".

Since this is my first real program in Rust as well, I don't know what the really idiomatic Rust code looks like, I hope I could improve the code nevertheless.

To check whether I made any typical beginner's mistakes, I ran cargo-clippy, and I didn't get any complaints.


Login page using RxSwift in MVVM I am building a simple login page which will check for username before navigating to another screen and here is how I am doing the binding now. I would like to know if I am doing it right and if I am not, what is the recommended way of doing the binding. Moreover, I creating the UI programmatically.

ViewModel.swift

// Inputs
private let usernameSubject = PublishSubject<String>()
private let nextButtonDidTapSubject = PublishSubject<Void>()

// Outputs
private let validUsernameSubject = PublishSubject<Bool>()
private let invalidUsernameSubject = PublishSubject<String>()

// MARK: - Init

init() {

    input = Input(username: usernameSubject.asObserver(),
                  nextButtonDidTap: nextButtonDidTapSubject.asObserver())

    output = Output(validUsername: validUsernameSubject.asObservable(),
                    invalidUsername: invalidUsernameSubject.asObservable())

    nextButtonDidTapSubject
        .withLatestFrom(usernameSubject.asObservable())
        .subscribe(onNext: { [unowned self] text in
            if text.count >= self.minUsernameLength {
                self.validUsernameSubject.onNext(true)
            } else {
                let message = text.count > 0 ?
                    "Please enter a valid username" :
                    "Please enter a username"
                self.invalidUsernameSubject.onNext(message)
            }
        })
        .disposed(by: disposeBag)

}

ViewController.swift

private func configureBinding() {

    loginLandingView.usernameTextField.rx.text.orEmpty
        .subscribe(viewModel.input.username)
        .disposed(by: disposeBag)

    loginLandingView.nextButton.rx.tap
        .subscribe(viewModel.input.nextButtonDidTap)
        .disposed(by: disposeBag)

    viewModel.output.validUsername
        .subscribe(onNext: { [unowned self] _ in
            print("Valid username - Navigate...")
            self.navigate()
        })
        .disposed(by: disposeBag)

    viewModel.output.invalidUsername
        .subscribe(onNext: { [unowned self] message in
            self.showAlert(with: message)
        })
        .disposed(by: disposeBag)

}

I do not recommend this approach. There are entirely too many subjects and they are completely unnecessary. Too much boilerplate.

I recommend a more functional approach:

The view controller would contain something like this:

// assign viewModel before presenting.
var viewModel: (LoginInputs) -> LoginOutputs = { _ in fatalError("assign before view is loaded.") }

// called from viewDidLoad()
private func configureBinding() {

    let inputs = LoginInputs(
        username: loginLandingView.usernameTextField.rx.text.orEmpty.asObservable(),
        loginTrigger: loginLandingView.nextButton.rx.tap.asObservable()
    )

    let outputs = viewModel(inputs)

    outputs.navigateTrigger
        .subscribe(onNext: { [unowned self] in
            self.navigate()
        })
        .disposed(by: disposeBag)

    outputs.invalid
        .subscribe(onNext: { [unowned self] message in
            self.showAlert(with: message)
        })
        .disposed(by: disposeBag)
}

And the view model would look like:

struct LoginInputs {
    let username: Observable<String>
    let loginTrigger: Observable<Void>
}

struct LoginOutputs {
    let navigateTrigger: Observable<Void>
    let invalid: Observable<String>
}

func loginViewModel(minUsernameLength: Int) -> (_ inputs: LoginInputs) -> LoginOutputs {
    return { inputs in
        let usernameEntered = inputs.loginTrigger
            .withLatestFrom(inputs.username)

        let navigateTrigger = usernameEntered
            .filter { minUsernameLength <= $0.count }
            .map { _ in }
        let usernameTooShort = usernameEntered
            .filter { 1 <= $0.count && $0.count < minUsernameLength }
            .map { _ in "Please enter a valid username" }
        let usernameEmpty = usernameEntered
            .filter { $0.isEmpty }
            .map { _ in "Please enter a username" }

        return LoginOutputs(
            navigateTrigger: navigateTrigger,
            invalid: Observable.merge(usernameTooShort, usernameEmpty)
        )
    }
}

The code that presents the view controller would look something like:

let controller = LoginViewController()
controller.viewModel = loginViewModel(minUsernameLength: 8) // or whatever the minimum is.
// show the view controller

The above will maximize the testability of your code. You can test the view model by simply calling the function and pushing data into it. You can test the view controller by assigning a viewModel function that pushes test data.

The above will also establish a strong separation between your logic (in the view model) and your effects (in the view controller.)