Compare two files and save the result into a pickle file My code should compare two vectors saved as dictionary (two pickle files) and save the result into a pickle file too. This works but very slowly. For one compare result I'm waiting about 7:20 min. Because I have a lot of videos (exactly 2033) this prog will run about 10 days. This is too long. How can I speed up my code for Python 2.7?

import math
import csv
import pickle
from itertools import izip

global_ddc_file = 'E:/global_ddc.p'
io = 'E:/AV-Datensatz'
v_source = ''

def dot_product(v1, v2):
    return sum(map(lambda x: x[0] * x[1], izip(v1, v2))) # izip('ABCD', 'xy') --> Ax By

def cosine_measure(v1, v2):
    prod = dot_product(v1, v2)
    len1 = math.sqrt(dot_product(v1, v1))
    len2 = math.sqrt(dot_product(v2, v2))
    if (len1 * len2) <> 0:
        out = prod / (len1 * len2)
    else: out = 0
    return out

def findSource(v):
    v_id = "/"+v[0].lstrip("<http://av.tib.eu/resource/video").rstrip(">")
    v_source = io + v_id
    v_file = v_source + '/vector.p'
    source = [v_id, v_source, v_file]
    return source

def getVector(v, vectorCol):
    with open (v, 'rb') as f:
        try:
            vector_v = pickle.load(f)
        except: print 'file couldnt be loaded'
        tf_idf = []
        tf_idf = [vec[1][vectorCol] for vec in vector_v]
    return tf_idf

def compareVectors(v1, v2, vectorCol):
    v1_source = findSource(v1)
    v2_source = findSource(v2)
    V1 = getVector(v1_source[2], vectorCol)
    V2 = getVector(v2_source[2], vectorCol)
    sim = [v1_source[0], v2_source[0], cosine_measure(V1, V2)]
    return sim

#with open('videos_av_portal_cc_3.0_nur2bspStanford.csv', 'rb') as dataIn:
with open('videos_av_portal_cc_3.0_vollstaendig.csv', 'rb') as dataIn:
#with open('videos_av_portal_cc_3.0.csv', 'rb') as dataIn:
    try:
        reader = csv.reader(dataIn)

        v_source = []
        for row in reader:
            v_source.append(findSource(row))
        #print v_source

        for one in v_source:
            print one[1]
            compVec = []
            for another in v_source:
                if one <> another:
                    compVec.append(compareVectors(one, another, 3))
            compVec_sort = sorted(compVec, key=lambda cosim: cosim[2], reverse = True)

            # save vector file for each video
            with open (one[1] + '/compare.p','wb') as f:
                pickle.dump(compVec_sort,f)

    finally:
        dataIn.close()

One suggestion is to use numpy to vectorize your dot product.

>>> timeit.timeit('dot_product(range(10000),range(10000))', setup='from itertools import izip\ndef dot_product(v1, v2):\n    return sum(map(lambda x: x[0] * x[1], izip(v1, v2)))', number=1000)
2.666857957839966
>>> timeit.timeit('np.dot(range(10000),range(10000))', setup='import numpy as np', number=1000)
0.9193508625030518

Another suggestion is to use multiple threads or processes to run multiple compare results at the same time. The libraries threading or multiprocessing would be useful here.


Inheritance and small number of parameters Uncle Bob in his Clean Code says:

  So all the same rules apply. Functions that take variable arguments
  can be monads, dyads, or even triads. But it would be a mistake to
  give them more arguments than that.

void monad(Integer... args); void
dyad(String name, Integer... args);
void triad(String name, int count, Integer... args);

So Uncle Bob suggests that no more than 3 arguments should a function get, right?

But what about CTOR arguments in class inheritance hierarchy? What if each class in hierarchy adds a new field and you should initialize them in CTOR. See an example below:

class Person
{
private:
    std::string m_name;
    int m_age;

public:
    Person(const std::string& name, const int age);
    std::string getName() const { return m_name; }
    int getAge() const { return m_age; }
    ~Person();
};

#include "Person.h"
class Student : public Person
{
private:
    std::string m_university;
    int m_grade;

public:
    Student(const std::string& name, const int age, const std::string& university, const int grade);
    std::string getUniversity() const { return m_university; }
    int getGrade() const { return m_grade; }
    ~Student();
};

See how student gets 4 arguments, while Person gets only 2 and Student adds two more. So how we should handle this?

So Uncle Bob suggests that no more than 3 arguments should a function get, right?

No. The guy you're referring to suggests that more than two arguments to a function which takes "variable arguments" is bad.

The key to understanding lies in distinguishing between a single argument (int count) and argument pack (Integer ... args). The function which takes the latter is what is called variable number arguments function, compared to fixed number argument function. The three dots here are actually part of the C++ syntax going back to C called ellipsis. Google variadic to impress people)

So if Student ctor takes 4 args (or 10 args) its ok. But if amongst the args Student Ctor takes is an argument pack, then don't overcomplicate things, by passing too many single arguments. Separate the handling of the argument packs (yes, there can be more than one pack) from the rest of them.


Extension methods to save and load form size, position, and state I wrote these extension methods to provide a generic/re-usable method for saving a form's size, position, and window state. What I would primarily like to know is if there is a better way to handle storing the WindowState property. I would also like to know if there are any bugs I might have overlooked, unnecessary code, or ways to make it more readable.

public static void SaveFormSizeAndLocation( this Form form )
{
    try
    {
        using( RegistryKey key = Application.UserAppDataRegistry.CreateSubKey(form.Name) )
        {
            if( key != null )
            {
                if( form.WindowState == FormWindowState.Normal )
                {
                    key.SetValue("Left",form.Left);
                    key.SetValue("Top",form.Top);
                    key.SetValue("Width",form.Width);
                    key.SetValue("Height",form.Height);
                }
                if( form.ShowInTaskbar )
                {
                    string windowState = Enum.GetName(typeof(FormWindowState),form.WindowState);
                    key.SetValue("WindowState",form.WindowState);
                }
            }
        }
    }
    catch
    {
        // Party on, Garth!
    }
}
public static void LoadFormSizeAndLocation( this Form form )
{
    try
    {
        using( RegistryKey key = Application.UserAppDataRegistry.OpenSubKey(form.Name) )
        {
            if( key != null )
            {
                form.Left = (int)key.GetValue("Left",form.Left);
                form.Top = (int)key.GetValue("Top",form.Top);
                form.Width = (int)key.GetValue("Width",form.Width);
                form.Height = (int)key.GetValue("Height",form.Height);
                // Move window into visible screen bounds if outside screen bounds (prevent off-screen hidden windows)
                Rectangle screenRect = SystemInformation.VirtualScreen;
                if( form.Left < screenRect.Left )
                    form.Left = screenRect.Left;
                if( form.Top < screenRect.Top )
                    form.Top = screenRect.Top;
                if( form.Right > screenRect.Right )
                    form.Left = screenRect.Right - form.Width;
                if( form.Bottom > screenRect.Bottom )
                    form.Top = screenRect.Bottom - form.Height;
                if( form.ShowInTaskbar )
                {
                    string windowState = Enum.GetName(typeof(FormWindowState),form.WindowState);
                    windowState = key.GetValue("WindowState",windowState).ToString();
                    form.WindowState = (FormWindowState)Enum.Parse(typeof(FormWindowState),windowState);
                }
            }
        }
    }
    catch
    {
        // Party on, Wayne!
    }
}

Unnecessary code

The windowState variable isn't used in the SaveFormSizeAndLocation method. And I believe you don't need such variable at all, because form.WindowState.ToString() returns exactly the same string.

Suggestions

The SaveFormSizeAndLocation method.
You check the key for null value, and if it is null nothing happens. Maybe it makes sense to omit that check and to handle (or raise) the exception in order to inform user or to save the reason in the application log.

The LoadFormSizeAndLocation method.
You can reduce nesting by inverting the condition:

using (RegistryKey key = Application.UserAppDataRegistry.OpenSubKey(form.Name))
{
    if (key == null)
    {
        return;
    }

    form.Left = (int)key.GetValue("Left",form.Left);
    form.Top = (int)key.GetValue("Top",form.Top);
    ...
}

Naming

I believe that extension methods for the Form class shouldn't contain the word 'Form' in their names. Consider to shorten names: SaveSizeAndLocation and LoadSizeAndLocation.

Styling

It looks unnatural for me:

Spaces after ( and before ). I'd prefer to insert spaces before the ( in operators:

if (x != y)

and to omit spaces in method declarations and calls:

someMethod(int a, float b, string c);
someMethod(a, b, c);

No spaces between method arguments. I'd prefer to add a space after each comma:

SomeMethod(firstArg, secondArg, thirdArg);

This should improve readability.
Personally, I cannot easily distinguish between

SomeMethod(abc,def,ghi); // 3 arguments

and

SomeMethod(abc.def.ghi); // 1 argument


Photogallery First project in OOP review Please take it easy on my as this is my first OOP web application I plan on converting it to some deeper degree but not sure yet.

The basic functionality is there I think, but I'm looking for some feedback on how I can improve this in terms of OOP and simplicity. Sorry for any simple mistakes only been doing php for a few months and its my first language :)

Many Thanks

<?php

class Image{

/*
*list of properties
*image filepath gets pasted in $imageFilepath
*/
public $imageFilepath = "";
public $imageTitle;
public $imageSize;

/*
*list of properties
*image filepath gets pasted in $imageFilepath
*/
public function __construct() {
     echo 'Welcome to Dannys awesome Gallery';
}

/*
*function to get the image filepath, runs through the files the outputs then in an array
*
*/

public function getimage_Filepath() {
    $image_files = get_files($imageFilepath);
    if (count($image_files)) {
        $index = 0;
        foreach($image_files as $index=>$file)
        $index++;
    }
    return;
}

/*
*Outputs the file images found
*
*/

public function setimage_Filepath() {
    echo $this->getimage_Filepath();
}

/*
*Generates the image name, from the actual filename
*
*/

public function getimage_title($imageTitle) {
//gets the image title using basename
    $imageTitle = $this->imageFilepath;
    basename($imageTitle);
    return $imageTitle;
}

/*
*Outputs the image name
*
*/

public function setimage_title($imageTitle) {
    //displays the image title
    echo $this->imageTitle;
}

}
$image = new Image;
?>

The code below is good. I only changed the comments a bit to make them a bit clearer and more concise. However,
one glaring issue is that it seems you have your setters and getters mixed up. Note the getimage_*()
and setimage_*() functions you have in your class. Usually, setter and getter functions do exactly that:
they set a value and get a value. What you seem to be doing is using the set* functions to get the
value returned by another function. This is +wrong+.

<?php

class Image{
    public $imageFilepath = "";
    public $imageTitle;
    public $imageSize;

    /**
     * Construct
     */
    public function __construct() {
         echo 'Welcome to Dannys awesome Gallery';
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getimage_Filepath() {
        $image_files = get_files($imageFilepath);
        if (count($image_files)) {
            $index = 0;
            foreach($image_files as $index=>$file)
            $index++;
        }
        return;
    }

    /**
     * Output tile image found
     *
     * @return array
     */
    public function setimage_Filepath() {
        echo $this->getimage_Filepath();
    }

    /**
     * Generate image name derived from filename
     *
     * @return string
     */
    public function getimage_title($imageTitle) {
        //gets the image title using basename
        $imageTitle = $this->imageFilepath;
        basename($imageTitle);
        return $imageTitle;
    }

    /**
     * Output the image name
     *
     * @return string
     */
    public function setimage_title($imageTitle) {
        //displays the image title
        echo $this->imageTitle;
    }
}
?>

Now, let's get rid of the set* functions:

<?php

class Image{
    public $imageFilepath = "";
    public $imageTitle;
    public $imageSize;

    /**
     * Construct
     */
    public function __construct() {
         echo 'Welcome to Dannys awesome Gallery';
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getimage_Filepath() {
        $image_files = get_files($imageFilepath);
        if (count($image_files)) {
            $index = 0;
            foreach($image_files as $index=>$file)
            $index++;
        }
        return;
    }

    /**
     * Generate image name derived from filename
     *
     * @return string
     */
    public function getimage_title($imageTitle) {
        //gets the image title using basename
        $imageTitle = $this->imageFilepath;
        basename($imageTitle);
        return $imageTitle;
    }
}
?>

Great! Looking much better. Now, remember that your construct should be used to set variables or initialize
functions that the object requires - NOT echoing something out (unless its an Exception, in which case you'd
use throw new Exception('')). So let's move the echo OUT of the class, initialize the $imageFilepath
variable:

<?php

class Image{
    private $imageFilepath;
    private $imageTitle;
    private $imageSize;

    /**
     * Construct
     */
    public function __construct(string $imageFilepath) {
        if(empty($imageFilepath)){
            throw new Exception("Expecting image file path, nothing given.");
        } else {
            $this->imageFilepath = $imageFilepath;
        }
    }

    /**
     * Get image filepath
     *
     * @return array
     */
    public function getImageFilepath() {
        return $this->imageFilepath;
    }

    /**
     * Get the files in a directory pointed by imageFilepath
     *
     * @return array
     */
    public function getFilesInFilepath(){
        $directoryContent = scandir($this->imageFilepath);
        foreach($directoryContent as $key => $value){
            $path = $this->imageFilepath . PATH_SEPARATOR . $value;
            if(is_file($path) && is_readable($path)){
                $files[] = $path;
            }
        }
        return $files;
    }

    /**
     * Generate image name derived from filename
     *
     * @return highlight_string(str)
     */
    public function getImageTitle(string $imageFile) {
        return basename($imageFile);
    }
}

// We initialize the object and set the imageFilepath at the same time
$image = new Image("/home/jsanc623/Pictures/");

// We can get the image filepath
$imageFilepath = $image->getImageFilepath();

// We can get an array of all the files in a directory
$filesInFilePath = $image->getFilesInFilepath();

// We can then get the image titles for each image/
foreach($filesInFilePath as file){
    $imageTitle[] = $image->getImageTitle($file);
}

print_r($imageTitle)
?>


Rational Approximation for e with Matlab I've written a program that calculates the best rational approximation for e with a 1, 2, 3, 4, 5, and 6 digit denominator respectively (on Matlab). For a 5-digit denominator it takes about a minute. For a 6-digit denominator it takes 2 hours. I've tried to improve the code, but I am a beginner. Suggestions are kindly appreciated.

tic
clear
p=1;
q=1;
e=exp(1);
digits=1;
trial=1;
while numel(num2str(q))<=6
    while numel(num2str(q))==digits
        p=p+1;
        if p/q>e
            q=q+1;
        end
        A(trial)=p/q;
        B(trial,1:2)=[p,q];
        trial=trial+1;
    end
    for i=1:trial-1
        error(i)=abs(A(i)-e);
    end
    [b,r]=min(error);
    p_q(digits,1:2)=B(r,1:2);
    digits=digits+1;
end
p_q
toc

This is the output:

p_q =
          19           7
         106          39
        1264         465
       25946        9545
      271801       99990
     1084483      398959

Elapsed time is 7232.153581 seconds.

Edit: Thanks to all who answered. Dennis and Jonas were right. It was the strings and growing matrices that slowed everything down. Besides I got two wrong values for p and q. The new code is this:

tic
p_best=1;
q_best=1;
e=exp(1);
for i=1:6
    for q=10^(i-1):10^i-1
        p=round(q*e);
        if abs(p/q-e)<abs(p_best/q_best-e)
            p_best=p;
            q_best=q;
        end
    end
    disp([p_best,q_best])
end
toc

The output is:

      19           7
     193          71
    1457         536
   25946        9545
  271801       99990
 1084483      398959

Elapsed time is 0.054986 seconds.

It's faster and actually correct. I assumed I knew e like Dennis said. To Joni and hardmath: I can't use continued fractions because the teacher wanted us to use brute force, but thanks.

Continued fractions are an easy way to calculate good rational approximations to real numbers. You can find a disussion of an algorithm to convert a floating point number into a fraction in my blog, with code in JavaScript which should be easy to convert to matlab:

function float2rat(x) {
  var tolerance = 1.0E-6;
  var h1=1; var h2=0;
  var k1=0; var k2=1;
  var b = x;
  do {
     var a = Math.floor(b);
     var aux = h1; h1 = a*h1+h2; h2 = aux;
     aux = k1; k1 = a*k1+k2; k2 = aux;
     b = 1/(b-a);
  } while (Math.abs(x-h1/k1) > x*tolerance);
  return h1+"/"+k1;
}

If you're specifically interested in e you can take a shortcut and use the regularities in its continued fraction representation to produce rational approximations that are far more accurate than the machine epsilon. This method is described here.


Generate dictionary of points on n-sphere This is a long-shot, but my question is to simply optimize this particular function in some code I have written:

import numpy as np
from numpy.core.umath_tests import inner1d

def getSpherePoint(dim,pList,psetLen):
    npgauss = np.random.standard_normal
    pt = abs(np.random.standard_normal(psetLen))
    pt = pt/np.sqrt(inner1d(pt,pt))

    d = dict(zip(pList,pt))
    return d

Note that dim is some integer \$n\$, pList is simply a list of \$2^n-1\$ strings, and psetLen is \$2^n-1\$. This function is designed to return a dictionary keyed  by elements of pList with values determined by a point on the unit \$n\$-hypersphere.

At this point, I need to run this function around a billion times or so to get the results I want, which will take hours. I've done as much optimization as possible here, and my micro-optimization seems to indicate that all three major parts of the function (the random number, the normalization, and the dictionary association) all take approximately a third of the run-time, and I'm not sure how to reduce it further. I've squeezed a bit out of Cythoning it, but I'm not sure how much of an improvement that will give me as compared to actually writing it in C and then importing the C function. Unfortunately, I don't know C (or C++) and haven't written Java in years, so I'm pretty stuck using Python (or extensions thereof).

Is it possible to optimize this further by any significant factor? Would changing languages help dramatically? What improvements could be made in Python?

Style
Following PEP8, the official Python style guide, you should use snake_case for your variables and functions names. You should also put a space after comas.
I would also rename the function sphere_points as the "get" is implied by the fact that you call a function (to get a result).
You also happen to not use the dim parameter and, looking at your description, the last one is just len(pList). Unless this length is common to a lot of calls and there is a real benefit to cache it, you can drop 2 out of 3 parameters.
numpy
numpy has vectorized functions  for your main operations: numpy.fabs (or numpy.absolute if you're dealing with complex numbers) and numpy.linalg.norm. Using these might speed things up. Unfortunately, going back into pure Python realm (dict + zip) can't be speed up that way.
However, as pointed out by @200_success in a comment, these answers suggest using np.sqrt(pt.dot(pt)) or np.sqrt(np.einsum('i,i', pt, pt)) for faster results than np.linalg.norm. As always, profile and choose what's best for your use case.
Cache function access
In Python, local symbols are faster to resolve. Which means np.linalg.norm is kinda slow, but norm is faster. And granted you performed norm = np.linalg.norm somehow before, it will produce the same result.
But caching a complex lookup within the same call you use it does not add much, since the complex lookup still need to be performed.
One way to efficiently do it is to use default values for arguments as the lookup will be performed only once and the speedup will occur at each call.
Proposed improvements
import numpy as np

def sphere_points(keys, random_gen=np.random.standard_normal, absolute=np.fabs, norm=np.linalg.norm):
    pt = absolute(random_gen(len(keys)))
    pt = pt/norm(pt)

    return dict(zip(keys, pt))


Phone List challenge To learn Haskell, I've been going at this challenge, which is to check a list of phone numbers to ensure that no number is a prefix of another. However, my code is way too slow. I am really new to Haskell so I don't know what to look for in performance optimization.

I have generated a sample input which takes 5s (uncompiled via runhaskell) on my machine. What can I do to speed this up?

main = do
    content <- getContents
    let
        allLines = lines content
        caseCount = read (head allLines) :: Int
        cases = tail allLines
    putStr $ unlines $ map toYesNo (processCases cases caseCount)

toYesNo b
    | b = "YES"
    | otherwise = "NO"

processCases :: [[Char]] -> Int -> [Bool]
processCases [] num = []
processCases cases 0 = []
processCases cases remaining =
    let
        caseLines = read (head cases) :: Int
        current = take caseLines (tail cases)
    in (if (checkListConsistency current) then True else False)
        : (processCases (drop (caseLines+1) cases) (remaining-1))

checkListConsistency :: [[Char]] -> Bool
checkListConsistency [] = True
checkListConsistency list =
    let
        number = head list
        rest = tail list
    in if checkNumberConsistency number rest
        then checkListConsistency rest
        else False

checkNumberConsistency :: [Char] -> [[Char]] -> Bool
checkNumberConsistency number [] = True
checkNumberConsistency number list =
    foldr (&&) True (map (checkNumberConsistencyVsNumber number) list)

checkNumberConsistencyVsNumber :: [Char] -> [Char] -> Bool
checkNumberConsistencyVsNumber number number2 =
    let
        numberLength = length number
        number2Length = length number2
    in if numberLength > number2Length
        then notStartsWith number number2
        else notStartsWith number2 number

notStartsWith _ [] = False
notStartsWith [] _ = True
notStartsWith (hl:tl) (hs:ts)
        | hl == hs = notStartsWith tl ts
        | otherwise = True

Performance

Your program is slow because you check each number against all subsequent numbers.  For a fully consistent phone book with n entries, that's O(n2).

A better strategy would be to sort each phone book first.  Then you only need to check consecutive pairs.  If one number is a prefix of another, then the prefix will appear just before the longer number.  That's O(n log n) for the sorting, plus O(n) for the checking, which is O(n log n) in total.

Looping

The infrastructure for handling test cases is clumsy.

One problem stems for the fact that you slurp up all the input into one list right away, with content <- getContents followed by allLines = lines content.  As a result processCases has to take a list of those lines as input, and parcel them out using drop (caseLines+1) cases.  A further complication is that allLines contains non-homogeneous data: the first line specifies the number of test cases, the first line within each test case specifies the size of that test case, and all other lines contain phone numbers.  You're basically doing I/O by dissecting the list, and you would be better off doing real I/O.

processCases performs a countdown using its second parameter.  It would be better to have a processCase function instead, which is called n times.  You can't make that improvement, though, given the way all the input has been slurped up into one list.

Style

For readability, maintainability, and performance, declare the types of all functions.  You missed toYesNo and main.  Instead of [[Char]], use [String], which is easier to understand.
Avoid function names like checkSomething, which sound more imperative than functional.  For example, isListConsistent would be a better name than checkListConsistency.
Take advantage of standard library functions — use Google and Hoogle.  For example, notStartsWith is just ((not.).isPrefixOf).
Simplify predicate tests on a list.  checkNumberConsistency can be

checkNumberConsistency :: String -> [String] -> Bool
checkNumberConsistency n ns = all (checkNumberConsistencyVsNumber n) ns

Avoid if … then … else.  There is usually a better alternative.  For example, (if (checkListConsistency current) then True else False) is just checkListConsistency current.  Your checkListConsistency function can be simplified:

checkListConsistency :: [String] -> Bool
checkListConsistency [] = True
checkListConsistency (n:ns) = (checkNumberConsistency n ns) && (checkListConsistency ns)

Suggested solution

import Control.Monad (replicateM, replicateM_)
import Data.List (isPrefixOf, sort)

isConsistentPhoneList :: [String] -> Bool
isConsistentPhoneList numbers = not $ any (uncurry isPrefixOf) consecutivePairs
  where
    sortedNumbers = sort numbers
    consecutivePairs = zip sortedNumbers $ tail sortedNumbers

yesNo :: Bool -> String
yesNo True = "YES"
yesNo False = "NO"

processCase :: IO ()
processCase = do
  caseSize <- readLn
  inputs <- replicateM caseSize getLine
  putStrLn $ (yesNo . isConsistentPhoneList) inputs

main :: IO ()
main = do
  caseCount <- readLn
  replicateM_ caseCount processCase


A BrainF*ck-ish compiler in C -ish because I removed the input functionality from the compiler. And, the compiler does not support nested loops

I've been recently reading up on compilers and how they work. Although this doesn't use most of the things I've learned about (the lexical and parser things), I thought it would be fun to try create a brainf*ck compiler, rather than an interpreter.

bfc.c

#include <stdio.h>

static int i;
static char *code; // these two as static so get_amt_to_change can easily interact

int get_amt_to_change(char c); // so the compiler doesn't write (ie) add di, 1 ten times

int main(int argc, char **argv) {
    code = argv[1];
    puts("xor di, di\n"
         "setup_loop:\n"
         "mov byte [tape + di], 0\n"
         "add di, 1\n"
         "cmp di, 101\n"
         "jne setup_loop\n"
         "xor di, di"); // sets up the tape with all 0's
    int loop_count = 0; // to keep track of asm subroutines for [ and ]
    for(i = 0; code[i] != '\0'; i++) {
        switch(code[i]) {
            case '+':
                printf("add byte [tape + di], %d\n", get_amt_to_change('+'));
                break;
            case '-':
                printf("sub byte [tape + di], %d\n", get_amt_to_change('-'));
                break;
            case '>':
                printf("add di, %d\n", get_amt_to_change('>'));
                break;
            case '<':
                printf("sub di, %d\n", get_amt_to_change('<'));
                break;
            case '.':
                puts("mov ah, 0Eh\n"
                     "mov al, byte [tape +di]\n"
                     "int 10h");
                break;
            case '[':
                printf("cmp byte [tape + di], 0\n"
                       "je end_loop%d\n"
                       "start_loop%d:\n", loop_count, loop_count);
                break;
            case ']':
                printf("cmp byte [tape + di], 0\n"
                       "jne start_loop%d\n"
                       "end_loop%d:\n", loop_count, loop_count);
                loop_count++; // to not repeat subroutine names
                break;
        }
    }
    puts("jmp $\nsection .bss\ntape resb 100"); // a 100 byte tape
    return 0;
}

int get_amt_to_change(char c) {
    int amt;
    for(amt = 0; code[i] == c; amt++, i++);
    i--; // if it wasn't == c, then go back one character and find out what it was equal to in the next call
    return amt;
}

The above code reads Brainf*ck code passed via command line argument and produces an assembly code, which could then be passed into the NASM assembler.

Examples

Purpose: nothing. brainf*ck doesn't have a purpose logs 'd' to output

Brainf*ck

Note: the backslashes are there so the terminal doesn't read the < and > symbols as it normally would.

\>++++++++++[\<++++++++++\>-]\<.

Assembly

xor di, di
setup_loop:
mov byte [tape + di], 0
add di, 1
cmp di, 101
jne setup_loop
xor di, di
add di, 1
add byte [tape + di], 10
cmp byte [tape + di], 0
je end_loop0
start_loop0:
sub di, 1
add byte [tape + di], 10
add di, 1
sub byte [tape + di], 1
cmp byte [tape + di], 0
jne start_loop0
end_loop0:
sub di, 1
mov ah, 0Eh
mov al, byte [tape +di]
int 10h
jmp $
section .bss
tape resb 100

Purpose: to show the generated assembly code for each of the symbols

Brainf*ck

++--\>\>\<\<.,[]

Assembly

xor di, di
setup_loop:
mov byte [tape + di], 0
add di, 1
cmp di, 101
jne setup_loop
xor di, di
add byte [tape + di], 2
sub byte [tape + di], 2
add di, 2
sub di, 2
mov ah, 0Eh
mov al, byte [tape +di]
int 10h
cmp byte [tape + di], 0
je end_loop0
start_loop0:
cmp byte [tape + di], 0
jne start_loop0
end_loop0:
jmp $
section .bss
tape resb 100

Questions

My C code only has one subroutine. Are there any other logical ones to add?
I did a lot of optimizing on the assembly code output. Could it be further optimized?
My main problem with compilers I've written in the past is that I've over-complicated things. Is that an issue with this (C) code?

You need to enlarge tape because your initialization zeroes 101 bytes.

 section .bss
 tape resb 101

When in graphics mode, the teletype function uses the BL and BH registers as arguments.

 mov  bx,0007h ;Display page 0 and Color 7
 mov  ah, 0Eh
 mov  al, byte [tape +di]
 int  10h

When in text mode, the teletype function uses the BH register as an argument.

 mov  bh,0     ;Display page 0
 mov  ah, 0Eh
 mov  al, byte [tape +di]
 int  10h

You can optimize the setup code by iterating backwards. It shaves off 2 instructions! It too will leave DI=0.

 mov  di, 101
setup_loop:
 sub di,1
 mov  byte [tape + di], 0
 jnz  setup_loop

I don't know if you would care but the code for [ ] by itself produces an infinite loop if the byte at [tape + di] is anything but zero.


Printing Colored, CLId Unicode Chess Board - follow-up Follow up to: Printing Command Line Unicode Chess Board

A couple people from the previous question mentioned looking into coloring the command line with ANSI escape sequences. In Python, colorama seems to handle this quite nicely. Below is my attempt at using it for printing out a chess board.

from enum import Enum
from enum import auto
from colorama import init, Fore, Back, Style

class Color(Enum):
    WHITE = 0
    BLACK = 1

class Piece(Enum):
    EMPTY = auto()
    PAWN = auto()
    ROOK = auto()
    KNIGHT = auto()
    BISHOP = auto()
    KING = auto()
    QUEEN = auto()

ELEMENTS = {
    (Color.WHITE, Piece.EMPTY): Back.WHITE,
    (Color.WHITE, Piece.KING): f'{Fore.LIGHTWHITE_EX}\u265A',
    (Color.WHITE, Piece.QUEEN): f'{Fore.LIGHTWHITE_EX}\u265B',
    (Color.WHITE, Piece.ROOK): f'{Fore.LIGHTWHITE_EX}\u265C',
    (Color.WHITE, Piece.BISHOP): f'{Fore.LIGHTWHITE_EX}\u265D',
    (Color.WHITE, Piece.KNIGHT): f'{Fore.LIGHTWHITE_EX}\u265E',
    (Color.WHITE, Piece.PAWN): f'{Fore.LIGHTWHITE_EX}\u265F',
    (Color.BLACK, Piece.EMPTY): Back.LIGHTBLACK_EX,
    (Color.BLACK, Piece.KING): f'{Fore.BLACK}\u265A',
    (Color.BLACK, Piece.QUEEN): f'{Fore.BLACK}\u265B',
    (Color.BLACK, Piece.ROOK): f'{Fore.BLACK}\u265C',
    (Color.BLACK, Piece.BISHOP): f'{Fore.BLACK}\u265D',
    (Color.BLACK, Piece.KNIGHT): f'{Fore.BLACK}\u265E',
    (Color.BLACK, Piece.PAWN): f'{Fore.BLACK}\u265F'
}

def init_tiles():
    row = [ELEMENTS[(Color(i % 2), Piece.EMPTY)] for i in range(8)]
    return [row if i % 2 == 0 else row[::-1] for i in range(8)]

def init_board():

    def get_army(color):
        return [
            (color, Piece.ROOK),
            (color, Piece.KNIGHT),
            (color, Piece.BISHOP),
            (color, Piece.QUEEN),
            (color, Piece.KING),
            (color, Piece.BISHOP),
            (color, Piece.KNIGHT),
            (color, Piece.ROOK)
        ]

    return (
        [
            get_army(Color.BLACK),
            [(Color.BLACK, Piece.PAWN) for _ in range(8)],
            *[[None] * 8 for _ in range(4)],
            [(Color.WHITE, Piece.PAWN) for _ in range(8)],
            get_army(Color.WHITE)
        ]
    )

def print_board(board, flip=False):

    def flip_board(board):
        return [row[::-1] for row in reversed(board)]

    for i, row in enumerate(board if not flip else flip_board(board)):
        for j, piece in enumerate(row):
            piece = ELEMENTS.get(piece)
            print(f"{tiles[i][j]}{piece if piece else ' '}",
                  Style.RESET_ALL, end='', flush=True)
        print()

if __name__ == '__main__':
    init()
    tiles = init_tiles()
    board = init_board()
    print_board(board)

As always, any optimizations and critique is welcome!

Example Output:

This is some nice polished code. I have just a few remarks.

When initializing ELEMENTS, the piece names get pasted in twice.
Perhaps you'd like to loop over them,
adding a black and a white variant to the dict?
You might even associate the proper unicode code point
with each enum member, rather than calling auto().

        [(Color.BLACK, Piece.PAWN) for _ in range(8)],

You have nice helpers everywhere.
Except for the two lines about pawns.
Maybe you'd like a get_pawns(color) function?

Instead of enumerating row and then doing this:

        piece = ELEMENTS.get(piece)

consider running enumerate over map(ELEMENTS.get, row).


Total number of non overlapping subsets I am trying to solve this question. The essence of the problem is this:

  Given a list of school classes with start and end times, find the total number of non-overlapping subsets.

I've written up a DP solution that I think is optimal in terms of time complexity - \$n * \log(n)\$.

'''
https://www.spoj.com/problems/ACTIV/
'''

from collections import defaultdict

def bin_search(classes, target):
    hi = len(classes)-1
    lo = 0
    while lo <= hi:
        mid = (hi + lo)//2
        if classes[mid][1] > target:
            hi = mid-1
        else:
            lo = mid+1
    return hi

def num_nonoverlapping(classes):
    classes.sort(key=lambda x: x[1])
    dp = [0] * len(classes)
    prefix_sum = defaultdict(int)
    dp[0] = 1
    prefix_sum[0] = 1

    for i in range(1, len(classes)):
        start = classes[i][0]
        best = bin_search(classes, start)
        dp[i] = 1 + prefix_sum[best]
        prefix_sum[i] = dp[i] + prefix_sum[i-1]
    return sum(dp)

def format_ans(ans):
    s = str(ans)
    if len(s) >= 8:
        return s[len(s)-8:]
    else:
        return (8-len(s)) * "0" + s

while True:
    n = int(input())
    classes = []
    if n == - 1:
        break

    for _ in range(n):
        start, end = map(int, input().split())
        classes.append((start, end))

    ans = num_nonoverlapping(classes)
    print(format_ans(ans))

I’m getting a Time Limit Exceeded Error. I'd love some pointers on how I could make this faster.

Disclaimer: I write the review as I go - I haven't performed any optimisation yet but this may be useful to you or other reviewers.

Your code is well organised, having the actual algorithm in a function on its own, away from the logic handling input/ouput. This is a great habit (usually not done properly for programming challenges) and is much appreciated :-)

Adding tests

Before doing any changes to your code, I'd rather start by writing a few tests. This can also be useful later on to use them for benchmarks and check that optimisations do make things faster.

Here, this is an easy task because of the way you've written the code and because examples are provided.

I tend not to bother with unit-tests frameworks for such small tasks but this is a good occasion to give them a try if needed.

def unit_tests_num_nonoverlapping():
    classes, exp = [(1, 3), (3, 5), (5, 7), (2, 4), (4, 6)], 12
    ans = num_nonoverlapping(classes)
    assert ans == exp
    classes, exp = [(500000000, 1000000000), (1, 500000000), (1, 500000000)], 5
    ans = num_nonoverlapping(classes)
    assert ans == exp
    classes, exp = [(999999999, 1000000000)], 1
    ans = num_nonoverlapping(classes)
    assert ans == exp

def unit_tests_format_ans():
    assert "00000000" == format_ans(0)
    assert "00000005" == format_ans(5)
    assert "00000054" == format_ans(54)
    assert "00000540" == format_ans(540)
    assert "00005400" == format_ans(5400)
    assert "00054000" == format_ans(54000)
    assert "00540000" == format_ans(540000)
    assert "05400000" == format_ans(5400000)
    assert "54000000" == format_ans(54000000)
    assert "40000000" == format_ans(540000000)
    assert "00000000" == format_ans(5400000000)

def unit_tests():
    unit_tests_num_nonoverlapping()
    unit_tests_format_ans()

def io_test():
    while True:
        n = int(input())
        classes = []
        if n == - 1:
            break

        for _ in range(n):
            start, end = map(int, input().split())
            classes.append((start, end))

        ans = num_nonoverlapping(classes)
        print(format_ans(ans))

unit_tests()
# io_tests()

Improving format_ans

This is most probably not the place where we can get much performance improvements but it is a good place to learn a few additional techniques.

First thing I did was to define the number of digits wanted as a argument with default value 8 so that we do not break already existing behavior but we do get rid of the "8" magic number everywhere.

Using modulo, you can get the last 8 digits without getting into string manipulation.

def format_ans(ans, nb_dig=8):
    s = str(ans % 10**nb_dig)
    return (nb_dig-len(s)) * "0" + s

Python already defines a few builtin functions which you can use for a more concise solution. Here, we could use: str.zfill:

def format_ans(ans, nb_dig=8):
    return str(ans % 10**nb_dig).zfill(nb_dig)

You could probably use other string formatting functions but I think this is explicit enough.

Improving num_nonoverlapping

Due to the limits provided by the problem description, we have to expect a huge numbers of classes. This means that a fast solution will probably come from a great algorithm rather than micro-optimisations.

Your algorithm seems good.

At the moment, I have nothing better to suggest.


How do you "sordid sort" alphanumeric strings? The question is about sorting alphanumeric string in a "more natural" order. Source: Marc LeBrun's "Sordid Sort" Computist Quiz

  Often a system string comparison sorts alphanumeric data “unnaturally”, as with these filenames:

File1
File10 ← ?
File11 ← ?
File12 ← ?
File2
File3
File3X
File4
File5
File6
File7
File8
File999

  Describe an algorithm that will sort alphanumeric strings in a “more natural” order:

File1
File2
File3
File3X
File4
File5
File6
File7
File8
File10
File11
File12
File999

  The algorithm may only rely on comparing characters; to avoid overflow problems with very long numbers it must not convert substrings of digits into their numerical values.

Spoiler alert: If you would like to independently solve the above, please do so now before you let the below spoil it for you.

This implementation has its inspiration from Computist Quiz Answer Guide. Misunderstandings would be mine.

From the Answer Guide: Extra credit: discuss the behavior with respect to leading zeros, as in “007”.

#!/usr/bin/perl -w

# Sordid Sort: http://fxpt.com/wp-content/uploads/2012/01/quiz.pdf
# Solution based on http://fxpt.com/wp-content/uploads/2012/01/QuizAnswers.pdf

use strict;

my $verbose = 0;    # set it to non-zero to get regex matches printed to STDERR
my $ALNUM = qr/(\D*)(\d*)/; # match a pair of alphas-optional and/or num-optional
my @in = ();

while (<>) {
    chomp;
    push @in, $_;
}

sub sordidcmp {
    my @a = $a =~ m/$ALNUM/g;   # always even number of elements since $ALNUM matches in pairs; and always has a sentinel pair
    my @b = $b =~ m/$ALNUM/g;
    my $cmp = 0;
    my $min_ab = (@a < @b) ? @a : @b;
    my $i;
    print STDERR "comparing (<".join("> <", @a).">) with (<".join("> <", @b).">)" if $verbose;
    for ($i = 0; $i < $min_ab; ++$i) {  # advance by 2; once here
        $cmp = $a[$i] cmp $b[$i];   # compare alphas
        last if $cmp;    # end if we found unequals

        ++$i;       # advance once more here
        # compare nums
        my $lenai = length($a[$i]);
        my $lenbi = length($b[$i]);

        # deal with Extra credit: prefix "0" to the shorter num
        if ($lenai < $lenbi) {
            $a[$i] = ("0" x ($lenbi-$lenai)).$a[$i];
        } elsif ($lenbi < $lenai) {
            $b[$i] = ("0" x ($lenai-$lenbi)).$b[$i];
        }

        $cmp = $a[$i] cmp $b[$i]; # note: stringwise compare of nums
        last if $cmp;    # end if we found unequals
    }
    print STDERR " = $cmp\n" if $verbose;
    return $cmp;
}

print join "\n", sort sordidcmp @in;
print "\n" if @in;  # guard against empty input

My questions are:

I observed that the match @a = $a =~ m/$ALNUM/g always produces a sentinel pair of ("", ""). Is the above code relying on undocumented behaviour? I could not find (read: understand) this from the perlop, perlretut, and perlre perldocs.
Can you help simplify the sordidcmp subroutine? Perl golf would be nice, but a more maintainable version is what I would be interested in. Of course, links to other solutions/implementations are welcome.

I observed that the match @a = $a =~ m/$ALNUM/g always produces a sentinel pair. Is the above code relying on undocumented behaviour?

I don't know what you mean by "sentinel pair", but that will always return two values per match since you have two captures.

  Can you help simplify the sordidcmp subroutine?

You could replace sort sordidcmp with Sort::Key::Natural's natsort.


Project Euler #22 - Names Scores I programmed Problem #22 from Project Euler in Python. It works but I want to know if it really is pythonic enough.

  Using names.txt (right click and 'Save Link/Target As...'), a 46K text
  file containing over five-thousand first names, begin by sorting it
  into alphabetical order. Then working out the alphabetical value for
  each name, multiply this value by its alphabetical position in the
  list to obtain a name score.

  For example, when the list is sorted into alphabetical order, COLIN,
  which is worth 3 + 15 + 12 + 9 + 14 = 53, is the 938th name in the
  list. So, COLIN would obtain a score of 938 × 53 = 49714.

  What is the total of all the name scores in the file?

from itertools import count

FILE = "e022.txt"
CHAR_OFFSET = ord('A') - 1

def parse(file):
    """Return the read in list"""
    with open(file) as fid:
        return fid.read().replace('"',"").split(',')

def score_name(name):
    """Returns the score of a single name"""
    return sum(ord(char) - CHAR_OFFSET for char in list(name))

def score_names(names):
    """Returns the score of the list"""
    return sum(ind*score_name(name) for (ind, name) in zip(count(1),sorted(names)))

if __name__ == "__main__":
    print(score_names(parse(FILE)))

Please feel free to critic me on it. One thing I wasn't sure about: In the file parser, did I need to make the with guard or is this not needed when encapsulated in a function?

Furthermore, the global variable FILE is in a place where it is easy accessible, but let's say we would import it as a module and wouldn't need it. Should I move it down to the main guard and loose a bit of overview?

Your code is split into small functions and looks nice.

A few things could be done in a better way.

zip(count(1),sorted(names) can be rewritten with the enumerate builtin : enumerate(sorted(names), start=1).

You can iterate directly over strings so you don't need to use a list in list(name).

You don't need parenthesis in for (ind, name) in.

To answer your question :

FILE could easily be moved behind the main guard so I'd put it there. Had it been the smallest of the issues, I would have left it where it currently is.
for the with, I consider it to be a good habit to put it. Also, this should answer your solution.


Wiegand 26-Bit Calculator - Transforming a decimal number into an 26-Bit binary number I made a 26-bit binary Wiegand calculator for an ESP32 following this format:

This is "Arduino C" :

unsigned int * getWiegand(unsigned int dec) {
  unsigned int* wiegandNum = new unsigned int[26];

  Serial.println();
  // transform dec number into binary number and store it in binaryNum[]
  int n = 1;
  for(int i = 23; i>=0; i--) {
    int k = dec >> i;
    if (k & 1)
      wiegandNum[n++] = 1;
    else
      wiegandNum[n++] = 0;
  }
  // check for parity of the first 12 bits
  bool even = true;
  for(int i=1;i<13;i++) {
    if(wiegandNum[i] == 1) {
      even = !even;
    }
  }
  // add 0 or 1 as first bit (leading parity bit - even) based on the number of ones in the first 12 bits
  if(even) {
    wiegandNum[0] = 0;
  } else {
    wiegandNum[0] = 1;
  }

  // check for parity of the last 12 bits
  bool odd = false;
  for(int i=13; i<25;i++) {
    if(wiegandNum[i] == 1) {
      odd = !odd;
    }
  }
  // add 0 or 1 as last bit (trailing parity bit - odd) based on the number of ones in the last 12 bits
  if(odd) {
    wiegandNum[25] = 0;
  } else {
    wiegandNum[25] = 1;
  }

  return wiegandNum;
}

void setup() {
  Serial.begin(115200);

  unsigned int* wiegandArray = getWiegand(12679548);
  for(int i=0;i<26;i++) {
    Serial.print(wiegandArray[i]);
  }
}

12679548 will return 01100000101111001011111000 which, based on this online tool is correct.

Execution time for this function is 255 microseconds. I don't know if the time is good or not, but I believe that the code can be improved.

What do you think ?

References:

https://www.arduino.cc/reference/en/language/structure/sketch/setup/
https://www.arduino.cc/reference/en/language/functions/communication/serial/print/

Here are a number of ideas that may help you improve your program.

Separate calculation from I/O

The Serial.println(); call from within getWeigand really shouldn't be there.  It's better if the function does just one thing, and that is to calculate and return the number.

Don't leak memory

The code calls new but there isn't any matching delete which means that the program leaks memory.  It's better to make sure all memory is freed, but better still, see the next suggestion.

Avoid using new and delete

Modern C++ doesn't use new and delete as much as it used to.  In this case, much better than passing around a raw array pointer and hoping the other end knows what the size is, is to use std::array instead.  That is an improvement over a plain array because it works with all of the standard container algorithms and has its own .size() operator. See I.13 for more details on why passing around a single array pointer is not a good idea.

Prefer single bit shifting to multi-bit shifting

Many processors have hardware optimizations for multi-bit shifts, but not all do.  That means on some machines, multi-bit shifts take longer.  For that reason, and because I think it's more readable, I'd suggest changing the first for loop to this:

for (int i = 24; i > 0; --i) {
    wiegandNum[i] = dec & 1;
    dec >>= 1;
}

Simplify your code

Instead of doing things like this:

if(even) {
    wiegandNum[0] = 0;
} else {
    wiegandNum[0] = 1;
}

just use the value directly:

wiegandNum[0] = !even;

Use whitespace for readability

Lines like this one:

for(int i=1;i<13;i++) {

are much easier to read with a little more whitespace:

for (int i = 1; i < 13; i++) {

Create a test routine

With code like this, it's usually good to do extensive testing to make sure it does what you intend.  I am not using an Arduino, but created a version that runs on Linux (or really any computer that supports C++):

#include <string>
#include <iostream>
#include <iomanip>
std::string weiString(unsigned int dec)
{
    std::string str{};
    auto answer = getWiegand(dec);
    for (const auto digit: answer) {
        str.push_back(digit ? '1' : '0');
    }
    return str;
}

int main()
{
    struct {
        unsigned n;
        std::string expected;
        bool operator()() const {
            std::string calculated = weiString(n);
            bool ok{calculated == expected};
            std::cout << std::hex << n
                << std::dec
                << '\t' << expected
                << '\t' << calculated
                << '\t' << (ok ? "OK" : "incorrect!")
                << '\n';
            return ok;
        }
    } tests[]{
        {12679548,  "01100000101111001011111000" },
        {0xffff,    "00000000011111111111111111" },
        {0x1ffff,   "10000000111111111111111111" },
        {0xffffff,  "01111111111111111111111111" },
        {0xdecade,  "01101111011001010110111101" },
        {0xfffffff, "01111111111111111111111111" },  // ignore high bits
    };

    for (const auto& test: tests) {
        test();
    }
}

Results

Here's the alternate version that uses these suggestions.  It also uses the exclusive-or operator ^ to simplify parity calculations:

#include <array>

std::array<unsigned, 26> getWiegand(unsigned int dec)
{
    std::array<unsigned, 26> wiegandNum;
    for (int i = 24; i > 0; --i) {
        wiegandNum[i] = dec & 1;
        dec >>= 1;
    }
    // check for parity of the first 12 bits
    bool even = false;
    for (int i = 1; i < 13; i++) {
        even ^= wiegandNum[i];
    }
    wiegandNum[0] = even;

    // check for parity of the last 12 bits
    bool odd = true;
    for (int i = 13; i < 25; i++) {
        odd ^= wiegandNum[i];
    }
    wiegandNum[25] = odd;

    return wiegandNum;
}

Even shorter is to process one of the parity bits over the entire array as it's generated and then recalculate just the other one and adjust.  It might be less obvious why this works, but it does.  See if you can see why.

std::array<unsigned, 26> getWiegand(unsigned int dec)
{
    bool odd = true;
    std::array<unsigned, 26> wiegandNum;
    for (int i = 24; i > 0; --i) {
        wiegandNum[i] = dec & 1;
        odd ^= wiegandNum[i];
        dec >>= 1;
    }
    // check for parity of the first 12 bits
    bool even = false;
    for (int i = 1; i < 13; i++) {
        even ^= wiegandNum[i];
    }
    wiegandNum[0] = even;
    wiegandNum[25] = odd ^ even;
    return wiegandNum;
}


Swift/iOS: Subclassing UILabel and setting properties I'm writing this subclass to add an icon to a UILabel. It works, but I'm wondering if this is the best/cleanest way to do it. Do you see any improvements? Should maybe text and image properties be set using a init() method?

import UIKit

class IconLabel: UILabel {

    override var text: String? {
        set {
            nameLabel.text = newValue
        }
        get {
            return nameLabel.text
        }
    }

    var image: UIImage? {
        didSet {
            icon.image = image?.withRenderingMode(.alwaysTemplate)
        }
    }

    private lazy var icon: UIImageView = {
        let imageView = UIImageView()
        imageView.tintColor = .white
        imageView.translatesAutoresizingMaskIntoConstraints = false
        return imageView
    }()

    private lazy var nameLabel: UILabel = {
        let label = UILabel()
        label.translatesAutoresizingMaskIntoConstraints = false
        label.font = Theme.regular(size: .tiny)
        return label
    }()

    override init(frame: CGRect) {
        super.init(frame: frame)
        self.backgroundColor = Theme.supportLightGrayColor
        self.layer.cornerRadius = 5.0
        self.clipsToBounds = true
        self.addSubviewsAndConstraints()
    }

    required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    private func addSubviewsAndConstraints() {
        self.addSubview(icon)
        self.addSubview(nameLabel)

        self.directionalLayoutMargins = NSDirectionalEdgeInsets(top: 5.0, leading: 5.0, bottom: 5.0, trailing: 5.0)

        icon.leftAnchor.constraint(equalTo: self.layoutMarginsGuide.leftAnchor).isActive = true
        icon.bottomAnchor.constraint(equalTo: self.layoutMarginsGuide.bottomAnchor).isActive = true
        icon.heightAnchor.constraint(equalTo: self.layoutMarginsGuide.heightAnchor).isActive = true
        icon.widthAnchor.constraint(equalTo: icon.heightAnchor).isActive = true

        nameLabel.leftAnchor.constraint(equalTo: icon.rightAnchor, constant: 5.0).isActive = true
        nameLabel.centerYAnchor.constraint(equalTo: icon.centerYAnchor).isActive = true
        nameLabel.rightAnchor.constraint(equalTo: self.layoutMarginsGuide.rightAnchor).isActive = true
    }
}

You are using IconLabel to act as a container, it contains another UILabel and UIImageView. There is no need that IconLabel should be a UILabel you can just have your label nameLabel and your imageView icon in a UIView container. It means that you need to change :

class IconLabel: UILabel {

to something like

class IconLabelView: UIView {

another note, to keep your custom view generic it's better that you setup the background color outside the class like :

let customView = IconLabelView()
customView.backgroundColor = Theme.supportLightGrayColor

also I think it's better to have two methods for the subview setup and constraints instead of one:

private func addSubviews()

and

private func addConstraints()


LINQ style Sample method I Needed a method to randomly sample an IEnumerable collection without replacement. I'm using this for writing behavioral acceptance tests.

For example, in the test code, I write:
GetElements("#SearchResults > li").Sample(3);

Here's the how I implemented it:

I welcome any feedback or optimizations!

    /// <summary>
    ///  returns a random sample of the elements in an IEnumerable
    /// </summary>
    public static IEnumerable<T> Sample<T>(this IEnumerable<T> population, int sampleSize)
    {
        List<T> localPopulation = population.ToList();
        if (localPopulation.Count() < sample.size) return localPopulation;

        List<T> sample = new List<T>(sampleSize);

        Random random = new Random();
        while(sample.Count < sampleSize)
        {
            int i = random.Next(0, localPopulation.Count);
            sample.Add(localPopulation[i]);
            localPopulation.RemoveAt(i);
        }

        return sample;
    }

Make the Random generator a class-level member so you're not generating a new one with the default seed each time. Also, check your population for null:

/// <summary>
///  random number generator for the enumerable sampler.
/// </summary>
private static readonly Random random = new Random();

/// <summary>
///  returns a random sample of the elements in an IEnumerable
/// </summary>
public static IEnumerable<T> Sample<T>(this IEnumerable<T> population, int sampleSize)
{
    if (population == null)
    {
        return null;
    }

    List<T> localPopulation = population.ToList();
    if (localPopulation.Count < sample.size) return localPopulation;

    List<T> sample = new List<T>(sampleSize);

    while(sample.Count < sampleSize)
    {
        int i = random.Next(0, localPopulation.Count);
        sample.Add(localPopulation[i]);
        localPopulation.RemoveAt(i);
    }

    return sample;
}


Please critique my Enumerable DataReader with yield and log data access pattern We are generating our data access code following this pattern:

public static IEnumerable<SomeDataOutputDTO> GetSomeData(SomeDataInputDTO dto, IDbConnection dbConnection) {
    var queryString = @"Some SQL query";
    using (var command = (OracleCommand)dbConnection.CreateCommand()) {
        command.CommandText = queryString;
        command.BindByName = true;
        command.Parameters.Add("SomeParam", OracleDbType.Int16, dto.SomeParam, ParameterDirection.Input);
        command.Parameters.Add("SomeOtherParam", OracleDbType.Int16, dto.SomeOtherParam, ParameterDirection.Input);
        var success = false;
        try {
            using (var reader = command.ExecuteReader()) {
            success = true;
            while (reader.Read()) {
                object data;
                var newDto = new SomeDataOutputDTO();
                data = reader["first_column"];
                newDto.FirstColumn = (short)Convert.ChangeType(data, typeof(short));
                data = reader["second_column"];
                if (data != DBNull.Value) {
                    newDto.SecondColumn = (short)Convert.ChangeType(data, typeof(short));
                }

                yield return newDto;
            }
        }
    }
    finally {
        if (success) {
            QueryLogger.Debug(queryString, command.Parameters, "TheseQueries.GetSomeData");
        }
        else {
            QueryLogger.Error(queryString, command.Parameters, "DocumentTypeQueries.GetDocumentTypesByApplyType");
        }
    }
}

I want to know if there are any pitfalls regarding this code.
As you can see the control flow is a bit complex since it returns an Enumerable and to do that it contains a

yield return

statement.

Note also that there are two using() blocks and one try block in the middle. The purpose of the try-finally block is to ensure proper logging of the query, in Error level if there was an error, or Debug level if the query ran OK. The purpose of the using blocks is to ensure proper disposal of the command and reader objects.

The QueryLogger helper class uses log4net, not that it should matter much.

Please also remember that this is code generated by a tool, so whether it's readable or easy to maintain is not a concern. My concerns are with proper resource disposal, proper logging, etc.

Also, returning an IEnumerable is very important because it allows to use LINQ when consuming the results. Of particular interest when using LINQ are:

Take / Skip
ToLookup / ToDictionary
ToList
FirstOrDefault

Initially, I shared your concern over resource disposal.  My assumption was that there was the possibility the command could be left hanging around in cases where you did not enumerate over the entire collection (as with something like FirstOrDefault).

However, a few quick tests with a test project reveals that the using statement performs its clean-up as soon as you are done with the enumerator.  A LINQ statement or a foreach loop may only partially traverse the results, but they both still clean up the enumerator when exiting scope.

Keep in mind, though, that there is still potential confusion that might arise from the deferred execution of GetSomeData.  The command doesn't execute until you start enumerating the result.  If anything were to modify the data before between the time you call GetSomeData and when you enumerate the result, you could get different results than you expect.


Directive to create a Google map I have a considerably large directive which is doing many things: First it renders a Google Map, then it adds a listener to check when the bounds of the map change, then it renders points on the map based where the bounds currently are, and finally it has a menu of options which allow the user to turn on and off various settings on the map.

The code snippet below has been heavily modified to show the most important aspects of the directive. I've also hardcoded the template rather than used a templateURL.

This directive works exactly the way I want it to, and my questions are more about best practices for Angular:

Should I be pulling out the Ajax calls and placing them inside a service?
Should I create a Map controller and put some of this configuration logic in there?
When do I know if I need a new controller for that matter?

I'm afraid there is too much going on inside this directive and it should be broken out.

(function(angular) {
 'use strict';

 angular.module('myApp.directives').directive('myAppMap', [function() {

 return {
  template: '
A whole bunch of toggles for the map go here
 \

'
  link: function(scope, element, attrs) {

    var map, markers = [], currentLayer = [];

    function initialize() {
      var mapOptions = {
        zoom: 12,
        center: new google.maps.LatLng(34.05, -118.245),
        mapTypeId: google.maps.MapTypeId.ROADMAP,
        mapTypeControlOptions: {
          mapTypeIds: [google.maps.MapTypeId.ROADMAP, 'myApp_style']
        }
      },
      map = new google.maps.Map(document.getElementById('map-canvas'),mapOptions)
      map.mapTypes.set('myApp_style', styledMap);
      map.setMapTypeId('myApp_style');
      google.maps.event.addListener(map, 'bounds_changed', (function() {
       _getClusters();
      })());
    }

    function _getClusters(filters) {
      $.ajax({
        url: "http://localhost:1625/ocpu/library/imyApp/R/gen_cluster/json",
        type: 'POST',
        dataType:'json',
        data: {
          NE_lng:map.getBounds().getNorthEast(),
          SW_lng:map.getBounds().getSouthWest()
          maxNumber: 100
        },
        success:function(data){
          _setMarkers(map, JSON.parse(data));
        },
        error: function (xhr, ajaxOptions, thrownError) {
          console.log("ajax error - arun", xhr.status, thrownError);
        }
      })
    }

    function _setMarkers(map, clusters) {
      for (var i = 0; i < clusters.length; i++) {
        var thisCluster = clusters[i];
        var myLatLng = new google.maps.LatLng(thisCluster.lat, thisCluster.lon);
        markers[i] = new MarkerWithLabel({
          position: myLatLng,
          map: map,
          icon: "marker_01.png",
          labelClass: "myAppMarker",
          labelAnchor: new google.maps.Point(10, 40),
        })
        google.maps.event.addListener(markers[i], 'click', function() {
          map.setCenter(this.getPosition())
          map.setZoom(map.getZoom()+1);
        })
      }
    }

    function _toggleControls(direction) {
      if (direction=="open") {
        $(".mapControls").height("130");
        $(".mapControls .controls").fadeIn();
      }
      else {
        $(".mapControls .controls").fadeOut();
        $(".mapControls").height("30");
      }
    }

    $(".mapControls .open").click(function(){
      if ($(".mapControls .controls").is(":visible")) {
        _toggleControls("close")
      }
      else {
        _toggleControls("open")
      }
    })

    initialize();

   }
 }
}]);
})(window.angular);

From a once over

You seem to be missing a comma after </div>'
You seem to be missing a comma after SW_lng:map.getBounds().getSouthWest()
This code is reviewable, still, next time make sure there are no syntax errors..
Hard coding (34.05, -118.245) is wrong, get it from your Angular model/controller
Why prefix all your functions with _, except for initialize ?
Hard coding "http://localhost:1625/ocpu/library/imyApp/R/gen_cluster/json" is bad
Production code should not have console.log
You are not using the variable currentLayer
You are not using the parameter filters in _getClusters
You should not create a new listener function for each marker in _setMarkers, you could create 1 function before the loop with a reference to map. This might cause performance problems
You should merge _toggleControls and $(".mapControls .open").click(, it does not look DRY

All in all, I find the Google Maps API unwieldy, and because of that I do not think this is too much. However, you do need to something with the configuration logic, right now it looks akward.


Read gzipped JSON file from URL I am requesting a JSON file that's gzipped. First, I download file:

import urllib.request
testfile = urllib.request.URLopener()
testfile.retrieve("https://xxxxx.auth0.com/1537150574", "file.gz")

After that, I will read the file.gz and get the data.

with gzip.GzipFile("file.gz", 'r') as fin:
    json_bytes = fin.read()
json_str = json_bytes.decode('utf-8')
data = json.loads(json_str)
print(data)

Actually, This above code can work well for me. But I would like to find another way (faster and brief code). Could I have a suggestion?

Your bottleneck is probably that you write the file to disk first and then read it again (I/O).
If the file does not exceed your machines random access memory, decompressing the file on the fly in memory might be a faster option:

from gzip import decompress
from json import loads

from requests import get

def get_gzipped_json(url):
    return loads(decompress(get(url).content))

if __name__ == '__main__':
    print(get_gzipped_json("https://xxxxx.auth0.com/1537150574"))

Also note, that I put the running code into an if __name__ == '__main__': guard.


Python classes to grab HTTP response from website entities I am trying to implement a parent WebContent class and child classes to grab HTTP responses from the actual website entities. There are highlevel codes below and I am wondering what are people's perspectives in terms of the neatest way to implement this in a OOP manner.

import requests
from onelogin.api.client import OneLoginClient

class WebContent(object):

    def __init__(self, client_id, client_secret, login, password, sub_domain, app_id, app_url):

        self.client_id = client_id
        self.client_secret = client_secret
        self.app_id = app_id
        self.app_url = app_url
        self.login = login
        self.password = password
        self.sub_domain = sub_domain

    def _login(self):
        client = OneLoginClient(self.client_id, self.client_secret)
        saml = client.get_saml_assertion(self.login,
                                         self.password,
                                         self.app_id,
                                         self.sub_domain)
        saml_data = saml.saml_response

        session = requests.Session()
        saml_payload = {'SAMLResponse': saml_data}
        session.post("{}/sso/response".format(self.app_url), saml_payload)
        return session

    def get_content(self, endpoint, time_out=30):
        if endpoint:
            session = self._login()
            result = session.get(endpoint, timeout=time_out)
            session.close()
            return result.content

class WebMarketingContent(WebContent):
    def get_endpoint(self, entity_id):
        base_url = "{app_url}/{entity_id}?{query_params}"
        params = '&entity_id={}'.format(entity_id)
        return base_url.format(app_url=self.app_url, query_params=params)

class WebEducationContent(WebContent):
    def get_endpoint(self, entity_id):
        base_url = "{app_url}/category/adhoc_element/{entity_id}?{query_params}"
        params = '&entity_id={}'.format(entity_id)
        return base_url.format(app_url=self.app_url, query_params=params)

if __name__ == '__main__':
    web_marketing_content = WebMarketingContent('client_id',
                                                'client_secret',
                                                'email',
                                                'password',
                                                'sub_domain',
                                                'app_id',
                                                'app_url')

    endpoint = web_marketing_content.get_endpoint(123)
    result = web_marketing_content.get_content(endpoint)

Some suggestions:

if __name__ == '__main__': is usually followed by a call to a main method. The main method then parses arguments using for example argparse to inject all the things which should not be hardcoded in the application - definitely passwords, but in your case probably all of the parameters to WebMarketingContent. This makes the code reusable and scriptable.
WebEducationContent is not used anywhere, so it should be removed.
Take advantage of static analysis and formatting using black, flake8 and mypy with a strict configuration to improve the overall quality.
You shouldn't need to get the endpoint before getting the content. web_marketing_content.get_content(123) should itself work out the endpoint and request it.


A class that represents a key combination For a little game of mine I have created a Key.h file that should allow representation of a key press combination. A key combination is basically some key represented by its virtual key code stored in int _keyand the state of modifier keys - shift, control, alt and windows keys. In case that key combination is used for typing, the key also stores the character that it should produce in _c.

class Key
{
private:
    bool _shift;
    bool _ctrl;
    bool _alt;
    bool _win;
    int _key;
    char _c;

public:
    constexpr Key()
        :
        _shift(),
        _ctrl(),
        _alt(),
        _win(),
        _key(),
        _c()
    {}

    constexpr Key(const Key& key)
        :
        _shift(key._shift),
        _ctrl(key._ctrl),
        _alt(key._alt),
        _win(key._win),
        _key(key._key),
        _c(key._c)
    {}

    constexpr Key(bool shift, bool ctrl, bool alt, bool win, int key, char c)
        :
        _shift(shift),
        _ctrl(ctrl),
        _alt(alt),
        _win(win),
        _key(key),
        _c(c)
    {}

    Key& operator= (Key& key)
    {
        _shift = key._shift;
        _ctrl = key._ctrl;
        _alt = key._alt;
        _win = key._win;
        _key = key._key;
        _c = key._c;
    }

    bool operator== (const Key& key) const
    {
        return (_shift == key._shift && _ctrl == key._ctrl && _alt == key._alt && _win == key._win && _key == key._key, _c == key._c);
    }

    bool operator!= (const Key& key) const
    {
        return !(*this == key);
    }
};

Are there any design flaws or some obvious things that I'm missing?

I would start by fixing the warnings, one of which is an actual bug:

[1]
Operator = not returning a value

main.cpp: In member function 'Key& Key::operator=(Key&)':
main.cpp:50:5: warning: no return statement in function returning non-void [-Wreturn-type]
     }
     ^
main.cpp: In member function 'bool Key::operator==(const Key&) const':

Also: your operator= gets Key& key instead of const Key& key

Wait! don't rush to fix it. You can just eliminate it altogether and rely on the default assignment operator that does the same job ("rule of zero").

[2] using comma in the long list of equality check -- an actual bug

main.cpp:54:100: warning: left operand of comma operator has no effect [-Wunused-value]
         return ( ... && _win == key._win && _key == key._key, _c == key._c);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Wait! don't rush to fix it. We will use std::bitset so the entire operator== would become simpler.

After fixing the warnings above we can proceed for improvements.

[3] Use std::bitset for the bool flags

Instead of:

bool _shift;
bool _ctrl;
bool _alt;
bool _win;

Use:

enum SpecialKeys {SHIFT, CTRL, ALT, WIN, _SIZE_};
std::bitset<SpecialKeys::_SIZE_> specialKeys;

[4] No need for assignment operator and copy ctor (rule of zero).

Just use the defaults.

Proposed fixed code:

#include <bitset>

class Key {
    enum SpecialKeys {SHIFT, CTRL, ALT, WIN, _SIZE_};
    std::bitset<SpecialKeys::_SIZE_> specialKeys;
    int _key = 0;
    char _c = 0;
public:
    constexpr Key() {}
    constexpr Key(bool shift, bool ctrl, bool alt, bool win, int key, char c)
        : _key(key), _c(c)
    {
        specialKeys[SpecialKeys::SHIFT] = shift;
        specialKeys[SpecialKeys::CTRL]  = ctrl;
        specialKeys[SpecialKeys::ALT]   = alt;
        specialKeys[SpecialKeys::WIN]   = win;
    }

    bool operator== (const Key& key) const
    {
        return (specialKeys == key.specialKeys && _key == key._key && _c == key._c);
    }

    bool operator!= (const Key& key) const
    {
        return !(*this == key);
    }
};


CeWL written in Python This is my implementation of CeWL, a ruby application that returns a list of words which can be used for password crackers.

My main concern is performance, as it takes a couple seconds to run. I presume it's mainly within the get_words function. Any and all feedback and criticism is appreciated and considered.

"""
Cewl Implementation (without page depth)

Usage: python3 cewl.py <max word length> <name of file to create> <valid website url>
"""

import string
import sys
import os
from typing import List

import bs4.element
from bs4 import BeautifulSoup
from bs4.element import Comment
import urllib.request

FILE_DIRECTORY = os.path.dirname(os.path.realpath(__file__))

def write_to_file(filename: str, words: List[str]) -> None:
    """
    Creates a file in the same directory as the file.

    :param str file_name: Filename of word file
    :param List[str] words: Words to write to file

    :return None:
    """
    with open(f"{FILE_DIRECTORY}/{filename}", "w") as file:
        for word in words:
            file.write(f"{word}\n")

def filter_words(words: List[str], max_length: int) -> List[str]:
    """
    Filters each of the words by the criteria passed by the user. Also
    ensures that the word is a "word" (no numbers or special characters).

    :param List[str] words: Words to filter
    :param int max_length: Max word length

    :return List[str]: Filtered words
    """
    return [word for word in words if len(word) <= max_length and word.isalpha()]

def visible_tag(element: bs4.element) -> bool:
    """
    Returns if the passed element is visible to scrape words.

    :param bs4.element element: Element to check

    :return bool: True is visible, False otherwise
    """
    return (
        not element.parent.name in ['style', 'script', 'head', 'title', 'meta', '[document]'] and
        not isinstance(element, Comment)
    )

def get_words(url: str) -> List[str]:
    """
    Returns all the words in the website. Steps this function takes:

    1. Get all text from website
    2. Creates a list with words that have punctuation removed
    3. Removes newlines from each word in the list and lowers it
    4. Removes each "" in the list and joins to a string
    5. Returns a list of each word as an element of the list

    :param str url: Url of website

    :return List[str]: List of all text in website
    """
    body = urllib.request.urlopen(url).read()
    soup = BeautifulSoup(body, 'html.parser')
    texts = soup.findAll(text=True)

    # Steps begin #
    visible_texts = filter(visible_tag, texts)
    words = [str(word).translate(str.maketrans('', '', string.punctuation)) for word in visible_texts]
    words = [word.rstrip().lower() for word in words]
    words = " ".join(filter(None, words))
    return words.split()

def main() -> None:
    """
    Main Method for Cewl.

    :return None:
    """
    try:
        max_word_length = int(sys.argv[1])
        filename = sys.argv[2]
        website = sys.argv[3]
    except IndexError:
        sys.exit("Usage: python3 cewl.py <max word length> <name of file to create> <valid website url>")

    words = get_words(website)
    words = filter_words(words, max_word_length)
    words = list(set(words)) # Remove duplicate words and convert back to list #
    write_to_file(filename, words)

if __name__ == "__main__":
    main()

You have quite a few methods that need to run for all words. Currently you always make sure that you output a list again, but you could use generator expressions to avoid these intermediate lists.

from typing import Generator, Iterable

REMOVE_PUNCTUATION = str.maketrans('', '', string.punctuation)

def filter_words(words: Iterable[str], max_length: int) -> Generator[str, None, None]:
    """
    Filters each of the words by the criteria passed by the user. Also
    ensures that the word is a "word" (no numbers or special characters).

    :param Iterable[str] words: Words to filter
    :param int max_length: Max word length

    :return Generator[str, None, None]: Filtered words
    """
    return (word for word in words if len(word) <= max_length and word.isalpha())

...

def get_words(url: str) -> Generator[str, None, None]:
    """
    Returns all the words in the website. Steps this function takes:

    1. Get all text from website
    2. Creates a list with words that have punctuation removed
    3. Removes newlines from each word in the list and lowers it
    4. Removes each "" in the list and joins to a string
    5. Returns a list of each word as an element of the list

    :param str url: Url of website

    :return Generator[str, None, None]: Iterable of all text in website
    """
    body = urllib.request.urlopen(url).read()
    soup = BeautifulSoup(body, 'lxml')
    texts = soup.findAll(text=True)

    # Steps begin #
    visible_texts = filter(visible_tag, texts)
    words = (str(word).translate(REMOVE_PUNCTUATION).rstrip().lower()
             for word in visible_texts)
    return filter(None, words)

Note the () instead of []. I also made the trans a global constant instead of re-creating it every iteration, grouped up some of the methods and used the faster LXML parser. The generator expression will be run by the call set(words) in main.

Your write_to_file can also be slightly improved by using writelines:

def write_to_file(filename: str, words: Iterable[str]) -> None:
    """
    Creates a file in the same directory as the file.

    :param str file_name: Filename of word file
    :param Iterable[str] words: Words to write to file

    :return None:
    """
    with open(f"{FILE_DIRECTORY}/{filename}", "w") as file:
        file.writelines(f"{word}\n" for word in words)

You might want to consider using requests instead of urllib, since it is much more usable (although the difference in this case is negligible). Instead of manually parsing sys.argv, you could use argparse, which allows much more complicated argument parsing and automatic help message generation.

Regarding style, PEP8, the official style-guide, recommends using less than 80 characters per line, which your code does not and mine mostly does (stupid signature for generator type...). However, larger values are also acceptable, if applied consistently. More than 100 characters should be avoided nevertheless.


Writing image pixel data to printer code I've been trying to take image pixel data and write it out into printer code and my results are rather slow.

Here is a simplified version of what I have so far (image is a PIL Image object of 1200 x 1800 pixels):

# ~36 seconds on beaglebone
f.write(pclhead)
pix = image.getdata()
for y in xrange(1800):
  row = '\xff'*72
  ### vvv slow code vvv ###
  for x in xrange(1200):
    (r,g,b) = pix[y*1200+x]
    row += chr(g)+chr(b)+chr(r)
  ### ^^^ slow code ^^^ ###
  row += '\xff'*72
  f.write(row)
f.write(pclfoot)

I know the loop can be optimized way better, but how?

My code is running on a beaglebone so speed is slower than you'd expect, but composing the complex images takes about 5 seconds. I wouldn't expect my printer code function (which just reorders the data) to take much longer than about 2 or 3 seconds. My first attempt (with getpixel) took 90 seconds. Now I have it down to 36 seconds.  Surely I can make this quite a bit faster yet.

For comparison, just so we can all see where the hold up is, this code runs in 0.1 secs (but, of course, is lacking the important data):

# ~0.1 seconds on beaglebone
f.write(pclhead)
pix = image.getdata()
for y in xrange(1800):
  row = '\xff'*72
  ### vvv substituted vvv ###
  row += '\xff'*3600
  ### ^^^ substituted ^^^ ###
  row += '\xff'*72
  f.write(row)
f.write(pclfoot)

I guess a simplified version of this problem is to rewrite something like the following:

[ (1,2,3), (1,2,3) ... 1200 times ]

into

[ 2, 3, 1, 2, 3, 1, etc... ]

but as a string

"\x02\x03\x01\x02\x03\x01 ... "

Start with storing the '\xff' * 72 string as a constant; Python strings are immutable, recreating that string each time is not necessary.

Next, use a list to collect all strings, then join at the end; this is cheaper than constant string concatenation.

Last, avoid attribute lookups in critical sections; assign any attribute lookups done more than once to a local name:

from operator import itemgetter

bgr = itemgetter(1,2,0)

pix = image.getdata()
rowstart = '\xff' * 72
f_write = f.write
empty_join = ''.join
for y in xrange(1800):
    row = [rowstart]
    r_extend = row.extend
    for x in xrange(1200):
        r_extend(map(chr, bgr(pix[y*1200+x])))
    r.append(rowstart)

    f_write(empty_join(row))

You can experiment with joining the whole row (including rowstart) or writing out rowstart values separately; the following version might be faster still depending on write speed versus list concatenation speed:

for y in xrange(1800):
    f_write(rowstart)
    row = []
    r_extend = row.extend
    for x in xrange(1200):
        r_extend(map(chr, bgr(pix[y*1200+x])))
    f_write(empty_join(row))
    f_write(rowstart)


Testing distance between characters in a string Here is another challenge from Coderbyte. I found this one challenging, although not quite as much as the previous two I've posted. Based on the feedback I received on my earlier posts, I structured this solution as a function. It is probably better than my previous solutions but still could be improved.

In particular, I am wondering if there is a way to condense my while loops, perhaps by determining the a and b indices at the same time or maybe by building a helper function? It seems like there is redundancy there. Resetting the index variable strikes me as particular clumsy or inelegant, but it had to happen somehow. I'm also not to happy with splitting the string and then joining the modified array to deal with the spaces. Any feedback on these issues or other suggestions to improve the code would be greatly appreciated.

Here's the challenge, slightly modified:

  Take a str parameter and return true if there are any occurrences of
  characters "a" and "b" separated by exactly 3 places  (i.e. "lane
  borrowed" would result in true because there is exactly three
  characters between a and b). Otherwise return the string false.

(While I was posting this, I realized I had not accounted for spaces. My interpretation of the instructions is that spaces do not count as characters. I have updated my code accordingly.)

var str = prompt("Please enter a string: ").split("");

function test(str){
var index = 0;
var arrayA = [];
var arrayB = [];

while (index >= 0){ //removes spaces
  index = str.indexOf(" ");
  if (index >= 0) {
  str.splice(index, 1);
  }
}
str=str.join("");
index = 0;

while(str.indexOf("a",index) != -1){ //Identifies indices of "a"
  arrayA.push(str.indexOf("a",index));
  index = str.indexOf("a", index)+1;
}

index=0;

while(str.indexOf("b",index) != -1){//Identifies indicies of "b"
  arrayB.push(str.indexOf("b",index));
  index = str.indexOf("b", index)+1;
  }

for(var i=0; i<arrayA.length; i++){//determines if any a's and b's are 3 characters apart
  for(var j=0; j<arrayB.length; j++){
    if (Math.abs(arrayA[i]-arrayB[j]) === 3){
      return true;
    }
  }
}
return false;
}

console.log(test(str));

Here's a revised version:

function ABCheck(str) {
  str = str.split("");
  for (var i = 0; i<str.length; i++){
    if (str[i] === " "){
      str.splice(i,1);
      console.log(str);
    }
  }
  for (var i = 0; i< str.length; i++){
    if (str[i] === "a" && (str[i-3] === "b" || str[i+3] === "b")){
      return true;
    }
  }
  return false;
}

I would recommend using a regular expression.  Regex is designed for solving pattern matching and string manipulation problems. Although it can look esoteric and terse it is well worth the effort to learn since it is very powerful and fairly ubiquitous.  It is very likely your programming language/shell/editor of choice supports a flavor of regex.

var str = prompt("Please enter a string: ");

function test(str){
  return /a\w{3}b/.test(str) || /b\w{3}a/.test(str);
}

console.log(test(str));


Terminating an Active Directory user I'm writing an AD Termination script for work. I'm new to PowerShell and was wondering if multiple commands within PowerShell switch statement is okay to do:

# Switch statement to start here
$switchTitle = "Terminate User"
$confirmMessage = "Are you sure that " + $fullName + " is the user that you want to terminate?"

$yes = New-Object System.Management.Automation.Host.ChoiceDescription "&Yes",
"Starts the termination process for the selected user"

$no = New-Object System.Management.Automation.Host.ChoiceDescription "&No",
"Cancels the termination process and, eventually, will prompt for another user selection"

$options = [System.Management.Automation.Host.ChoiceDescription[]]($yes, $no)

$result = $host.ui.PromptForChoice($switchTitle, $confirmMessage, $options, 0)

switch ($result)
   {
      0 {"You have selected Yes. The Termination Script will now start."
           Copy-Item -Path "Microsoft.PowerShell.Core\FileSystem::\\Path\To\UserFolder" `
           -Destination "Microsoft.PowerShell.Core\FileSystem::\\path\To\TermFolder" -Recurse -Force
        # TODO: Put delete command here for old profile
        # TODO: Put Disable AD Object command here
        }
      1 {"You have selected No. Please choose another user."}
   }

I'm also concerned with PowerShell styling/conventions so if I'm doing anything wrong, please let me know.

Prompting for confirmation

The way you're doing the prompt is overly complicated. Consider this:

[CmdletBinding(SupportsShouldProcess=$true)]
param()

$switchTitle = "Terminate User"
$confirmMessage = "Are you sure that " + $fullName + " is the user that you want to terminate?"

if ($PSCmdlet.ShouldContinue($confirmMessage, $switchTitle)) {
    # She said yes!
} else {
    # nope
}

I sometimes wrap this up into a small function that allows for quicker usage if it's always for a single purpose within a script and I need it a lot:

function Get-Confirmation {
[CmdletBinding(SupportsShouldProcess=$true)]
param(
    [Parameter(Mandatory=$true)]
    [String]
    $User
)

    $confirmMessage = 'Are you sure that {0} is the user that you want to terminate?' -f $User

    $PSCmdlet.ShouldContinue($confirmMessage, 'Terminate User?')
}

Then just use it like so:

# code that populates $fullName
if (Get-Confirmation -User $fullName) {
    # terminate
} else {
    # don't terminate
}

Referencing files

Copy-Item -Path "Microsoft.PowerShell.Core\FileSystem::\\Path\To\UserFolder" `
-Destination "Microsoft.PowerShell.Core\FileSystem::\\path\To\TermFolder" -Recurse -Force

You don't have to use the full provider name to reference a file, even for a UNC path.

It would be fine to just do this:

Copy-Item -Path "\\Path\To\UserFolder" `
-Destination "\\path\To\TermFolder" -Recurse -Force


Better way to write nested loop, if condition and breaking it This is a Java method that takes as input a 2 dimensional array called grid.

It uses the i1 and j1 values from when the corresponding grid[i1][j1] position is true for the first time, and then i2 and j2 from when the corresponding grid[i2][j2] position is true for the second time.

A call is then made to drawLine(i1, j1, i2, j2) to draw line between those two points in the screen.

I wrote the following code to take all stuff I want to plot, but the code works but looks bad, is there any better way to write this. This code works fine, I just want to improve on it.

    for(int x1 = 0; x1 < rows; x1++) {
        for (int y1 = 0; y1 < columns; y1++) {
            if (grid[x1][y1] == true) {
                int x2 = x1 + 1;
                for(int y2 = 0; y2 < 500; y2++) {
                    if (grid[x2][y2] == true) {
                        drawLine(x1, y1, x2, y2);
                        break;
                    }
                }
            }
        }
    }

My implementation:

(Is C#.Net but you'll be able to translate easily).

    public class Point()
    {
        public int X {get; set;}
        public int Y {get; set;}

        public Point (int x, int y)
        {
            this.X=x;
            this.Y=y;
        }
    }

    public void calcLine(bool[][] grid)
    {
        int rows = 500;
        int columns = 500;

        Point p1 = new Point(0, 0);
        // Change this outer 'if' to 'while' if should continue searching for all lines to draw
        if (canFindNextPoint(grid, rows, columns, p1))
        {
            Point p2 = new Point(p1.X + 1, 0);
            if (canFindNextPoint(grid, rows, columns, p2))
            {
                drawline(p1.X, p1.Y, p2.Y, p2.Y);
            }
        }
    }

    private bool canFindNextPoint(bool[][] grid, int rows, int columns, Point p)
    {
        bool found = false;
        for (int x = p.X; x < rows; x++)
            for (int y = p.Y; y < columns; y++)
            if (grid[x][y])
            {
                found = true;
                p.X = x;
                p.Y = y;
                break;
            }
        return found;
    }


Javascript Prototypes that represent HTML elements I've been thinking about making Javascript Prototypes that represent HTML elements.
For example a form prototype with build in ajax requests and form element prototypes. Or a list with list item prototypes.

I think that the biggest benefit of this approach is that it reduces repetitive code.

Here is an example of what I have in mind.

var LightParticle = function() {
    this.setWidth(10);
    this.setHeight(10);
    this.setTop(0);
    this.setLeft(0);
    this.setPosition("absolute");
    this.setBackground("white");
    this.setClassName("LightParticle");
};

LightParticle.prototype = {
    setWidth : function(width) {
        this.width = width;
    },
    getWidth : function() {
        return this.width;
    },
    setHeight : function(height) {
        this.height = height;
    },
    getHeight : function() {
        return this.height;
    },
    setTop : function(top) {
        this.top = top;
    },
    getTop : function() {
        return this.top;
    },
    setLeft : function(left) {
        this.left = left;
    },
    getLeft : function() {
        return this.left;
    },
    setBackground : function(background) {
        this.background = background;
    },
    getBackground : function() {
        return this.background;
    },
    setClassName : function(className) {
        this.className = className;
    },
    getClassName : function() {
        return this.className;
    },
    setElement : function(element) {
        this.element = element;
    },
    getElement : function(element) {
        return this.element;
    },
    setPosition : function(position) {
        this.position = position;
    },
    getPosition : function(position) {
        return this.position;
    },
    setSize : function(size) {
        this.setWidth(size);
        this.setHeight(size);
    },
    getStyle : function() {
        return {
            position: this.getPosition(),
            width : this.getWidth(),
            height : this.getHeight(),
            top : this.getTop(),
            left: this.getLeft(),
            background: this.getBackground()
        }
    },
    getView : function() {
        var element = $("

");
        element
        .addClass(this.getClassName())
        .css(this.getStyle());
        this.setElement(element);
        return element;
    },
    pulsate : function (speed) {
        var height = this.getHeight();
        var width = this.getWidth();
        var top = this.getTop();
        var left = this.getLeft();
        if(this.getElement().height() == height) {
            height = height * 4;
            width = width * 4;
            top = top - (height/2);
            left = left - (width/2);
        }
        $(this.getElement()).animate({
            "height":height,
            "width": width,
            "top": top,
            "left":left
        }, speed);
        var that = this;
        setTimeout(function(){
            that.pulsate(speed);
        }, speed);
    }
}

function addRandomParticle() {
    try {
        var particle = new LightParticle();
        var seed = Math.floor(Math.random() * 70) + 1;
        particle.setBackground("#" + Math.floor((Math.abs(Math.sin(seed) * 16777215)) % 16777215).toString(16));
        particle.setSize(Math.floor(Math.random() * 70) + 10);
        particle.setTop(Math.floor(Math.random() * $(window).height()));
        particle.setLeft(Math.floor(Math.random() * $(window).width()));
        $('#canvas').append(particle.getView());
        particle.pulsate(Math.floor(Math.random() * 2000) + 500);
    } catch(error) {
        console.log(error.message);
    }
}

$(document).ready(function() {
    try {
        for(var i = 0; i < 100; i++) {
            addRandomParticle();
        }
    } catch(error) {
        console.log(error.message);
    }
});

So far I'm not satisfied with the getters and setters since they have no datatype validation.

Does anyone have any idea how I can improve this? Or does someone know a completely better approach to reduce javascript code/events?

As dystroy mentioned, the setters and getters are really not JavaScript style, neither are the many try/catch statements in your functions.

However, if you insist on having all these getters and setters, you should create a utility function that generates a getter/setter  for you. Since you have default values for most properties in your LightParticle I was thinking something along the lines of this :

function addGetterSetter( o , property , defaultValue ){
  var postFix = property[0].toUpperCase() + property.slice(1),
      getter  = 'get' + postFix,
      setter  = 'set' + postFix;
  o[getter] = function(){
    return this[property] || defaultValue;
  }
  o[setter] = function( value ){
    this[property] = value;
    return this;
  }
}

And then you would

var LightParticle = function() {},
    prototype = LightParticle.prototype;

addGetterSetter( prototype , width   , 10 );
addGetterSetter( prototype , height  , 10 );
addGetterSetter( prototype , top     , 0 );
addGetterSetter( prototype , left    , 0 );
addGetterSetter( prototype , position, 'absolute' );
addGetterSetter( prototype , background, 'white' );
addGetterSetter( prototype , className , 'LightParticle' );


Greatest difference between numbers in list I found this problem today and eventually came up with a solution. I'm interested in knowing other ways to solve it. You have a list of unsorted integers, and have to compute the greatest difference between list[j] - list[i] such that i < j.

Here's my code:

#include <stdio.h>

// remember the minimum to the left
static int _cached_max(int min, int n, int const* list)
{
  if(n < 1){
    return 0x80000000;
  }

  int max_index = 0;
  for(int i = 1; i < n; ++i){
    if(list[i] >= list[max_index]){
      max_index = i;
    }
  }

  // I know you can check this condition inside the loop above by keeping
  // some extra variables, keeping it here for readability
  for(int i = 0; i < max_index; ++i){
    if(list[i] < min){
      min = list[i];
    }
  }

  int this = list[max_index] - min;
  int next = _cached_max(min, n - max_index - 1, list + max_index + 1);

  return this > next ? this : next;
}

int custom_max(int n, int const* list)
{
  return _cached_max(list[0], n - 1, list + 1);
}

int main(int argc, char* argv[])
{
  int list[] = {12,21,10,20,9,18};
  int max = custom_max(sizeof list / sizeof list[0], list);

  printf("max = %d\n", max);

  return 0;
}

What is the optimal solution to this problem?

Update:

Here's some sample inputs and outputs:

F( [10;4;5;0;8] ) = 8
F( [10;7;16;5;11;3;9;0] ) = 9
F( [4;5;0;1;2;3;1;8] ) = 8
F( [12;21;10;20;9;18] ) = 10
F( [5;4] ) = -1

Consider that the greatest difference of the following 2 sets exceeds the int range.  For a more robust solution, return a wider type or detect failure.

{INT_MIN, INT_MAX}
{INT_MAX, INT_MIN}

// int custom_max(int n, int const* list)
long long custom_max(int n, int const* list)

Better to use size_t for array indexing.  int may be too narrow.

A solution in liner time, as answered by @janos, is do-able, yet a few changes are needed to handle an ever decreasing list like {10,8,7,4,0}.

For each element arr[i] from 1 to n-1, find if the difference of that element and the prior minimum is a greater difference.

Then update the minimum.

// Detect rare cases when long long and int have similar ranges.
#include <limits.h>
#if LLONG_MIN/2 > INT_MIN || LLONG_MAX/2 < INT_MIN
#error Need wider type
#endif

long long custom_max(size_t n, int const* list) {
   if (n < 2) {
     // Handle pathological case.
     // Avoid magic number
     // return 0x80000000;
     return LLONG_MIN;
   }
   int min = list[0];
   long long maxdiff = LLONG_MIN;
   for (size_t i = 1; i < n; i++) {
     long long diff = (long long) list[i] - min;
     if (diff > maxdiff) {
       maxdiff = diff;
     }
     if (list[i] < min) {
       min = list[i];
     }
   }
   return maxdiff;
}


Finding Largest Integer Palindrome within a Set I've "solved" Project Euler Question 4 and I was wondering if I could make my answer more efficient (I'm using Project Euler to help me learn Haskell).

The problem reads:

  Find the largest palindrome made from the product of two 3-digit numbers

Here is my solution

getMaxPalindrome = maximum[x*y | x<-[100..999], y<-[100..999], reverse(show(x*y)) ==show(x*y)]

All suggestions for improvement are appreciated!

First, since * is commutative, you can save 1/2 of your computation if you restrict yourself to cases where x >= y:

[ x*y | x<-[100..999], y<-[100..x], ... ]

Second, if you could generate all the products in a non-increasing list, you'd just be searching for the first element of such a list satisfying the predicate, which would also speed the search very much. See data-ordlist package, which implements many useful functions on sorted lists, in particular in your case you'll probably need unionBy or unionAllBy