Download the JSON from API and parse the data like Unique Voting Station Name and Councillors Name Please give me some pointers on how I can improve this project with Python 2.7.

import json
import urllib
urls = ["https://data.calgary.ca/resource/kqmd-3dsq.json","https://data.calgary.ca/resource/4wni-k3sg.json"]
print " \nThis is Json Data Parser Program. \nThis program will print the Unique Voting Station Name from 2013 or 2017 City Elections from City of Calgary OpenData Portal"
CRED = '\033[91m'
CEND = '\033[0m'

try:
    year= int (raw_input("\nElection Year to Search [2013 or 2017]:"))

    if ( year not in range(2013,2018,4) ):
        year=int(raw_input("Invalid Year ! \nPlease type again Year to Search [2013 or 2017]:"))\

except (EOFError, KeyboardInterrupt, ValueError):
    year= int (raw_input("\nThe exception has occured !!\nPlease type again Year to  Search [2013 or 2017]:"))
else:
    print "You choose: {} year for Elections Results".format(year)

try:
    ward=int(raw_input("\nWard to Search:"))

    if ( ward not in range(0,14)):
        ward=int(raw_input("Ward does not exist in City of Calgary !\nPlease type again Ward to Search:"))

except (EOFError,ValueError,KeyboardInterrupt):
    ward=int(raw_input("\nThe exception has occured !!\nPlease type again Ward to Search:"))
else:
    print "\nYou choose ward: {} for Elections Results".format(ward)

def jsonUrl(year):
    global url
    if (year == 2017):
        url = urls[0]
        return url
    elif (year == 2013):
        url = urls[1]
        return url
    else:
        raise Exception('The City of Calgary only has data for 2013 and 2017')

jsonUrl(year)

response = urllib.urlopen(url)
data= json.loads(response.read())

def wardstnname(ward):
    wardwithscore = "{}".format(ward)
    wardstnresult = []
    for i in data:
        try:
            if (i["ward"] == wardwithscore and  i["voting_station_name"] not in wardstnresult):
                        wardstnresult.append(i["voting_station_name"])
        except KeyError:
            continue
    if len(wardstnresult) > 0:
        print "\nThere were total", len(wardstnresult),"unique voting stations in your ward"
        print "\nThe list of the Voting Station were as follows"
        for votingstnname,b in enumerate(wardstnresult,1):
            print '{} {}'.format(votingstnname,b)
    else:
        print CRED,"\nCity of Calgary has no Data on this Ward for Voting Station Name on Year", year,CEND

wardstnname(ward)

def wardOffice(ward):
    wardwithscore= "{}".format(ward)
    officeResult = []
    for i in data:
        if (i["ward"] == wardwithscore and i["office"] == "COUNCILLOR" and i["ballot_name"] not in officeResult):
            officeResult.append(i["ballot_name"])
    if len(officeResult) > 0:
        print "\nThere were total", len(officeResult),"councillors in your ward"
        print "\nThe list of the Councillors were as follows"
        for councillorname,b in enumerate(officeResult,1):
            print '{} {}'.format(councillorname,b)
    else:
        print CRED,"\nCity of Calgary has no Data on this Ward for Councillors on Year", year,CEND

wardOffice(ward)

First, you should stop using Python 2, if possible. It will stop being supported in a bit over a year. Switch to Python 3, it is better in many ways.

Now, let's start with year not in range(2013,2018,4). This is a very inefficient way to do this, because in Python 2, range just generates a list and in lookup in lists is \$\mathcal{O}(n)\$. Instead use a collection for which in is \$\mathcal{O}(1)\$, like set or `dict. This is a good habit to form, even though here this list has only two elements.

As a side note, this was such a common beginner's mistake that they made sure that the Python 3 implementation of range (which is a generator on steroids), is actually \$\mathcal{O}(1)\$.

Now, to come to the underlying problems of the code. It is currently a mix of global variables, global code, function definitions, which sometimes take arguments, but also use global variables, and all of which modify global state (even though some of hem also return something). And these functions are then called right away.

Instead, try to define pure functions, that is functions that take all they need as arguments and return their result, without modifying any global state. You can then use these functions in a main method that does all the work.

import json
import requests
CRED = '\033[91m'
CEND = '\033[0m'

URLS = {2013: "https://data.calgary.ca/resource/kqmd-3dsq.json",
        2017: "https://data.calgary.ca/resource/4wni-k3sg.json"}

WARDS = set(range(0, 14))

def ask_year():
    while True:
        try:
            year = int(raw_input("\nElection Year to Search {}:".format(sorted(URLS.keys()))))
            if year not in URLS:
                print "No data for that year!"
                continue
        except ValueError:
            print "Please enter a valid year!"
            continue
        return year

def ask_ward():
    while True:
        try:
            ward = int(raw_input("\nWard to Search:"))
            if ward not in WARDS:
                print "Ward does not exist in City of Calgary !"
                continue
        except ValueError:
            print "Invalid ward number!"
            continue
        return ward

def get_voting_stations(ward, data):
    for station in data:
        try:
            if station["wards"] == ward:
                yield station["voting_station_name"]
        except KeyError:
            continue

def print_ward_voting_stations(ward, data):
    vorting_stations = set(get_voting_stations(ward, data))
    if vorting_stations:
        print "\nThere were in total", len(vorting_stations), "unique voting stations in your ward"
        print "\nThe list of the Voting Station were as follows"
        for i, name in enumerate(vorting_stations, 1):
            print '{} {}'.format(i, name)
    else:
        print CRED, "\nCity of Calgary has no Data on voting stations for ward {} in year {}".format(ward, year), CEND

def get_councillors(ward, data):
    for ballot in data:
        if ballot["ward"] == ward and ballot["office"] == "COUNCILLOR":
            yield ballot["ballot_name"]

def print_ward_councillors(ward, data):
    councillors = set(get_councillors(ward, data))
    if councillors:
        print "\nThere were in total", len(councillors), "councillors in your ward"
        print "\nThe list of the Councillors were as follows"
        for i, name in enumerate(councillors, 1):
            print '{} {}'.format(i, name)
    else:
        print CRED, "\nCity of Calgary has no Data on this Ward for Councillors in year", year, CEND

if __name__ == "__main__":
    print " \nThis is Json Data Parser Program. \nThis program will print the Unique Voting Station Name from 2013 or 2017 City Elections from City of Calgary OpenData Portal"

    year = ask_year()
    print "You chose: {} year for Elections Results".format(year)
    ward = str(ask_ward())
    print "\nYou chose ward: {} for Elections Results".format(ward)

    data = requests.get(URLS[year]).json()
    print_ward_voting_stations(ward, data)
    print_ward_councillors(ward, data)

The major differences are: I used a dict to map from the year to the url of the json file. This automatically raises a KeyError if an undefined year is given. You also already ensure that what the user entered is a valid year, here I re-used the keys of that dictionary.

I used a set for the valid wards. Both the urls and the wards are constant, and therefore written in ALL_CAPS, as recommended by Python's official style-guide, PEP8.

I separated the filtering of the data for what you need and making it unique. For this I defined each one function for outputting a stream of the voting stations/the office the ballot was for. This is then consumed by set, which makes it unique (but removes the order, which I think is not important here). This once again removes the necessity to do e.g. i["voting_station_name"] not in wardstnresult, which is again \$\mathcal{O}(n)\$.

I also removed catching KeyboardInterrupt and EOFError. Catching these errors should be well motivated. The first one appears when the user presses Ctrl+C, which means they want to abort whatever you are doing. Ignoring that and telling them to please enter a valid year is almost passive aggressive. The same goes for the latter, which happens if the user presses Ctrl+D. This is used to denote that the input is finished (especially important for multi line input). I don't see any reason whay you would want to catch that here.

Instead I replaced these with an infinite while True loop, which continues asking the user until they enter a valid year/ward or press Ctrl+C, which kills the whole program.


Express a number as sum of 2 primes Problem: Given an even number (greater than 2), return two prime
  numbers whose sum will be equal to given number.

  NOTE 1:  A solution will always exist. (Goldbach's conjecture)

  Note 2: If there are more than one solutions possible, return the
  lexicographically smaller solution.

This solution becomes slower for higher order numbers. How can I optimise this code?

import math
# Function to create a list of all prime numbers uptil given number
def list_prime(A):
    primes = []  # initialise an empty list of primes
    bool_a = [True for i in range(2,A+3)]  # creates boolean list of index
    for  i in range(2, int(math.sqrt(A))+1):
        if bool_a[i]:
            for j in range( i * i, A +1 , i):  # eliminate all multiples of i*i+i, i*i+2i and so on uptil A+1
                if bool_a[j]:
                    bool_a[j] = False;

    for i in range(2,A):  # the index of reamaining bools are prime numbers
        if bool_a[i]:
            primes.append(i)

    return primes

#Function that returns two prime numbers whose sum is equal to the given number
def prime_sum(A):
    solution_set = []

    for i in (list_prime(A)):
        for j in (list_prime(A)[::-1]):
            if i + j == A:
                solution_set.append((i,j))
                break

    return min(solution_set)

Using the Sieve of Eratosthenes seems like a reasonable way to generate a large number of primes.

One thing that you could do is directly return your bitset representation of the primes, rather than copying it to a condensed list. This would save a bit of time in the short term, and be helpful for the next step.

for i in (list_prime(A)):
    for j in (list_prime(A)[::-1]):

A few comments.

First, doing something twice is slower than doing it once! Since list_prime(A) is repeated in both lines, it will have to re-run the Sieve of Eratosthenes: even though you know that the list of prime numbers isn't going to change, Python doesn't! Even worse, because the second line is in a loop, you're running list_prime again for every value of i. So you can speed it up a bit by storing that list in a variable before you get to the loops.

Second, when you have nested loops like this, you make your algorithm quadratic in the size of list_prime(A). The issue is that you're doing the imprecise test for j before the precise test, which is normally the slow way around. That is, you're considering all the values of j which meet the rule "j is a prime number" and then testing whether "j + i == A". Instead consider all the values of j for which "j + i == A" first. There is exactly one of those for each i, which is found by simple subtraction. Then check whether that j is in your list of prime numbers. If so, you have a match. If not, you can immediately check the next i.

(The reason that I suggested returning the bitset representation of the list of primes is that it makes it much faster to check whether a specific number is prime.)

A few less substantial optimisations

solution_set.append((i,j))

It is good to consider the tie-breaking rules for problems like this, and listing candidates first and checking which wins afterwards is a valid way to do so.

However, it is worth thinking about the order of your algorithm before implementing the tie-breaking check. If, as is the case here, the first satisfying value is guaranteed to be the one that wins the tie break because it has the smallest i value, you might as well just return the first one you get to.

if bool_a[j]:
    bool_a[j] = False;

If the end result of these two lines is that bool_a[j] must be False, just set it to false. if statements are surprisingly slow on modern CPUs.

primes.append(i)

Whenever you find yourself appending something to a list one element at a time, consider whether you can rewrite it using list comprehensions. For example something like the following:

primes = [index for (index, is_prime) in enumerate(bool_a) if is_prime]

(However, as above, my actual recommendation is that you completely remove that step and directly return bool_a)


YouTube playlist steganography: hide secret messages inside YouTube playlists I have created this repo on GitHub and I have shown how this software works through a video.

If you have time, please visit the repo and read the files. I'll post some relevant snippets below, but if you can quickly skim over the whole thing, that would be great.

I'm new to Python so I'm sure there are some things in the code below that may not be best code practice. If you spot anything off, please let me know.

file yousecret.py:

I don't see many people writing Tkinter .grid() calls in this fashion so I'm thinking there may be some more practical way to do this. Also, the outcome looks like something from 1995, so I also wish I could give it a modern look.

enc_userkeys_label.grid(row=0,column=0)
enc_userkeys.grid(row=0,column=1)
enc_userkeys_clear.grid(row=0,column=2)
enc_sourcetext_label.grid(row=1,column=0)
enc_sourcetext.grid(row=1,column=1)
enc_sourcetext_clear.grid(row=1,column=2)
enc_button.grid(row=2,column=1)

file ysdecode.py:

What follows below feels like C code, I believe there must be a shorter and faster way to write this function in Python.

def getsum(addrlist):
    alladdr = []
    sumlists = []
    for addr in addrlist:
        final = []
        temp = []
        switch = True
        for url in addr:
            if switch:
                temp.append(url[0])
                switch = False
            elif not switch:
                temp.append(url[-1])
                switch = True
                final.append(tuple(temp))
                temp = []
        alladdr.append(tuple(final))
    for addr in alladdr:
        temp = []
        for pair in addr:
            for bitpair in BITARRAY:
                if pair == bitpair:
                    temp.append(BITARRAY.index(bitpair))
                    continue
        sumlists.append(tuple(temp))
    return sumlists

The addrlist parameter is a list of tuples, like this:

addrlist = [ ('wDzN2SQ8dCs', 'NLQf-Q8zlG0'), ('EhFPMTWSsso', 'wf1laG5Jp34') ]

BITARRAY is a global variable that is also a list of tuples.

for i in '-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz':
    for j in '048AEIMQUYcgkosw':
        BITARRAY.append((i,j))

Style

In any significant project, especially one that you are sharing through GitHub, it is important to write docstrings for each function.  You haven't written any.

It is implied by the PEP 8 official style guide that functions should be named using lower_case_with_underscores and constants should be named using UPPER_CASE_WITH_UNDERSCORES.  I would recommend renaming BITARRAY → BIT_ARRAY and getsum() → get_sum(), because pythonisnotgermanorfinnish.

Implementation

Whenever you build a list by starting with an empty list and appending to it in a loop, you'll almost certainly want to write a list comprehension or a generator expression instead.  That way, you describe the result "all at once" using a single statement.

from string import digits, ascii_uppercase, ascii_lowercase

INIT_CHARS = '-' + digits + ascii_uppercase + '_' + ascii_lowercase
FINAL_CHARS = '048AEIMQUYcgkosw'
BIT_ARRAY = sum(([tuple((a, b)) for a in INIT_CHARS] for b in FINAL_CHARS), [])

The sum() function above is used to concatenate lists.  That expression is a bit nasty.  You can write it better using itertools.product(), which is exactly what we need:

from itertools import product
from string import digits, ascii_uppercase, ascii_lowercase

INIT_CHARS = '-' + digits + ascii_uppercase + '_' + ascii_lowercase
FINAL_CHARS = '048AEIMQUYcgkosw'
BIT_ARRAY = list(product(INIT_CHARS, FINAL_CHARS))

Note, however, that it would be better not to define your BITARRAY at all, if your only goal is to find the index of a particular pair of characters.  Instead of BITARRAY.index(…), it would be more efficient to do a little arithmetic:

def numeric_value(init_char, final_char):
    """
    Given character init_char (one of the characters [-0-9A-Z_a-z]) and
    character final_char (one of the characters [048AEIMQUYcgkosw]),
    deterministically produce a numeric value from 0 to 1023, inclusive.
    """
    return INIT_CHARS.index(init_char) * len(FINAL_CHARS) + \
           FINAL_CHARS.index(final_char)

Based on similar principles, I would write this function:

def get_sum(video_ids):
    """
    Compute a tuple of numbers for the given YouTube video IDs.

    The videos IDs are taken two at a time.  From each pair, the initial
    character of the first ID and the final character of the second ID are
    taken.  Those two characters are mapped a numeric value between 0 and 1023.

    If there are an odd number of video IDs, the last one is ignored.
    """
    return tuple(
        numeric_value(a[0], b[-1]) for a, b in zip(*[iter(video_ids)] * 2)
    )

The zip(*[iter(video_ids)] * 2) expression above is a slick way to split the video_ids into pairs.  It comes from the grouper() example in the itertools recipes.

Note that your original getsum(addrlist) function is actually equivalent1 to

def getsum(addrlist):
    return [get_sum(playlist) for playlist in addrlist]

I recommend splitting the work this way instead, so that you don't end up with an excessively nested list comprehension.

1 Well, it's not exactly equivalent, in that your BITARRAY lookup silently ignores pairs of characters that do not conform to the ([-0-9A-Z_a-z], [048AEIMQUYcgkosw]) scheme, whereas numeric_value(…) would raise a ValueError.


Python Turtle - Olympic flag re-creation I made a program to draw the Olympic flag in Python using the turtle module. I am able to make a basic version of it but i think that there is still room for improvement!

Quick note: I am new to python and my code may not be of the best quality...

Ideally the outcome should look like this...

As you can see there is this 'overlapping' of the circles in the picture which makes the the circles look rather like rings that are on one another. I cannot implement that in my code. Like i said this is a basic version of it.

My code output:

Compare both pictures and you can see the difference.

Here is my code:

import turtle

def c1():
    t = turtle.Pen()
    t.reset()
    t.up()
    t.goto(-165,0)
    t.down()
    t.width(10)
    t.color("blue")
    t.circle(75)

def c2():
    t = turtle.Pen()
    t.reset()
    t.up()
    t.goto(0,0)
    t.down()
    t.width(10)
    t.color("black")
    t.circle(75)

def c3():
    t = turtle.Pen()
    t.reset()
    t.up()
    t.goto(165,0)
    t.down()
    t.width(10)
    t.color("red")
    t.circle(75)

def c4():
    t = turtle.Pen()
    t.reset()
    t.up()
    t.goto(-82.5,-85)
    t.down()
    t.width(10)
    t.color("yellow")
    t.circle(75)

def c5():
    t = turtle.Pen()
    t.reset()
    t.up()
    t.goto(82.5,-85)
    t.down()
    t.width(10)
    t.color("green")
    t.circle(75)

c1()
c2()
c3()
c4()
c5()

It would be awesome if anyone could provide some advice to improve the program in any way.

Your function kind-of works as intended, except the obvious deviation occurring at the crossings of rings. However, you can only draw the logo at one specific size and you need a heck of a lot of code to do it.

Let's start with a little clean-up and I'll explain the how and the why afterwards:

import turtle

OFFSET = 165
HALF_OFFSET = OFFSET/2
BORDER = 10
UPPER_LINE = 0
LOWER_LINE = UPPER_LINE - (HALF_OFFSET + BORDER/4)

def draw_circle(turtle, x=0, y=0, width=10, color="black", size=75):
    """
    Take a turtle, draw a circle.
    Takes 6 arguments, including the turtle.

    turtle: The turtle to use. Save the world, re-use your turtle.
    x: X coordinate offset from 0
    y: Y coordinate offset from 0
    width: width of the border of the ring
    color: color of the ring
    size: size of the ring
    """
    turtle.up()
    turtle.goto(x, y)
    turtle.down()
    turtle.width(width)
    turtle.color(color)
    turtle.circle(size)

def main():
    t = turtle.Pen()
    draw_circle(t, -OFFSET, UPPER_LINE, BORDER, "blue")
    draw_circle(t)
    draw_circle(t, OFFSET, UPPER_LINE, BORDER, "red")
    draw_circle(t, -HALF_OFFSET, LOWER_LINE, BORDER, "yellow")
    draw_circle(t, HALF_OFFSET, LOWER_LINE, BORDER, "green")

    input("Press any key...")

if __name__ == "__main__":
    main()

Which, apart from the input part, does exactly the same thing as your code does. That part halts the program so you can actually enjoy the creation on your screen before the rings disappear again. I've also removed the reset(), since it wasn't necessary.

I apologize for the poor naming, but at least it adheres to PEP8. Consistency is good, re-inventing the wheel unnecessary so just follow the same convention as everyone else. Keeps code readable for most developers at the same time. Good, no?

First, I've reduced the amount of functions. A lot. How? By using arguments. I can even use default arguments to save a lot of duplication. You can see this most of all in the second call to draw_circle, where a black circle is drawn with standard dimensions.

Note that many arguments are read from a value set at the top, which is calculated frome one single value: OFFSET. By changing one value, all other values scale with it.

This improves re-usability and readability by a lot. Your future self will thank you.

Another way to improve re-usability is with the following standard technique:

if __name__ == "__main__":
    main()

If you dive deeper into Python, you'll start encountering special variables like __name__. In this case, __name__ allows us to see whether a piece of code was included from somewhere else or executed directly. Now, everything inside main will only be executed whenever you call the function directly and not when it's included in a different file.

Now, assuming I have 2 scripts, CR_162598.py and foo.py. CR_162598.py is the cleaned-up version of your code. foo.py could look like this:

import turtle
from CR_162498 import draw_circle

t = turtle.Pen()
draw_circle(t, 50, 50)

input("Press any key...")

And all of a sudden, a black ring appears at coordinates 50, 50. Now, you can re-use your code without actually touching your old code. All you need to know is you can use the function draw_circle from the file CR_162498 and what arguments it takes. Noticed the docstring on top of the function? It's there for (among others) exactly that reason. Telling your user (or future self) how to use the function.

On default arguments:
Default arguments (default parameter values) help with setting a default ring. If we 'just want a ring' and don't care about it's size and color, simply calling the function will do. They are the values used if no other values are provided. Basically, giving a default gives you the option of making that parameter optional instead of obligatory. If it's missing, the function will simply take it's default instead.

Note: size is probably your radius, diameter or a value derived from either of those. Renaming it to something more sensible would be beneficial.


Home-grown web service I needed to build a one-way web service for a mobile app I developed. Its only goal is to receive incoming JSON data and insert it into the correct tables. It works, but it looks very "kludgy" and cobbled together (happens a lot with solo development).

I'd like another pair of eyes to see where it could be improved as far as design, robustness, readability, etc. This code is not yet in production, so I can implement suggested changes as needed.

<?php

$body = file_get_contents("php://input");
$json = json_decode($body, true);
//print_r($json);

function extractArray($arr)
{
    foreach ($arr as $k => $v) {
        if (is_array($v)) {
            extractArray($arr[$k]);
            foreach ($v as $kk => $vv) {
                if (is_array($vv)) {
                    unset($arr[$k][$kk]);
                    $arr[$kk] = $vv; // if you want the key to the array to be preserved.
                    // $arr[] = $vv; // would be safer, or check isset($arr[$kk]) first.
                }
            }
        }
    }
    return $arr;
}

function dbInsert(&$data)
{
    $tracks = array_shift($data);

//    //remove unneeded parent array
    $tracks = extractArray($tracks);

    /*
     * Note: user_id/page_id purposely unset.  Why? On the client side, Backbone.js
     * creates a model ID if one is not created manually. So, we create one on
     * the client side, but then remove it on the server side. Saves a lot of hassle otherwise.
     * db creates autoincrement id.
     * */
    unset($tracks['user_id']);

    $trackCols = array_keys($tracks);
    $trackVals = array_values($tracks);

    //connect to db
    $dbh = new PDO('mysql:host=localhost;dbname=kpl', 'root', 'root');
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    //insert tracker data
    try {
        $sth = $dbh->prepare("INSERT INTO tracker (" . implode(", ", $trackCols) . ") VALUES ('" . implode("', '", $trackVals) . "')");

        $sth->execute();

        $tracksPK = $dbh->lastInsertId();
    } catch (PDOException $e) {
        return print_r('Tracker insert failed: ' . $e->getMessage());
    }

    $pages = array_pop($data);

    foreach ($pages as &$page) {
        unset($page['page_id']);
        $pageCols = array_keys($page);

        /*
         * Note: Could not use same query structure as trackers because the foreign key user_id
         * is set dynamically.
         * */
        try {
            $sth = $dbh->prepare("INSERT INTO page (" . implode(", ", $pageCols) . ") VALUES (" . $page['view_count'] . "," . $page['visit_timestamp'] . ",'" . $page['page_viewed'] . "'," . $tracksPK . ",'" . $page['model_type'] . "')");

            $sth->execute();

        } catch (PDOException $e) {
            return print_r('Page insert failed: ' . $e->getMessage());
        }

    }

    return true;
}

if (isset($json)) {

    $info = dbInsert($json);

    print_r($info);

} else {
    die('JSON data fail');
}

There are some big holes in your security

Authenticate the data source. (Is it a legitimate source, username/password/etc)
Sanitize the user data.

Check $trackCols against a list of valid column names you are expecting, to stop me doing things like I have below.

$valid_cols = array('id', 'user', 'name');
foreach ($trackCols as $col) {
    if (!in_array($col, $valid_cols)) {
        die("Alert! Unexpected Column $col");
    }
}

PDO allows you to bind variables to the statement, and it will escape any dangerous data.
This will help stop people from creating attacks as I have below.

$sth = $dbh->prepare('SELECT name, colour, calories
    FROM fruit
    WHERE calories < :calories AND colour = :colour');
$sth->bindParam(':calories', $calories, PDO::PARAM_INT);
$sth->bindParam(':colour', $colour, PDO::PARAM_STR, 12);
$sth->execute();

I have included a test script below (DO NOT RUN AGAINST YOUR LIVE DATABASE)

I have manufactured some bad json data so that it will execute a delete statement on the tracker table in your database.

Obviously it won't work unless I get the table and column names correct, but as your code is now, if I try a dummy column name it will give me back an sql error message, which can help me to narrow it down.

Log the error message instead of returning them. This is an API, all the client end needs to know is that it failed. Don't give the attacker any more information then necessary.

<?php

// WARNING MAKE SURE YOU ONLY RUN THIS AGAINST A TEST DATABASE

$bad_sql = 'id) VALUES (1); DELETE FROM tracker; INSERT INTO tracker(id';
$json = json_encode(array(array($bad_sql => '99999')));

$ch = curl_init();

curl_setopt($ch, CURLOPT_URL,            "http://yourdomain/api.php" );
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1 );
curl_setopt($ch, CURLOPT_POST,           1 );
curl_setopt($ch, CURLOPT_POSTFIELDS,     $json);
curl_setopt($ch, CURLOPT_HTTPHEADER,     array('Content-Type: application/json'));

$result=curl_exec ($ch);

var_dump($result);


Twitter-bootstrap add-on Basically, the code converts a large select list with optgroups into a more user-friendly tabbed Twitter-bootstrap component.

I'm not sure how much into detail I need to get but on GitHub I uploaded a working example. This is the first time I write a jquery plugin and any advise is welcome.

(function($) {
    $.fn.bootstrapSelect = function(options) {

        //Extend default options with those provided.
        var opts = $.extend({}, $.fn.bootstrapSelect.defaults, options);

        var topLevel='';
        var subLevel='';
        var selectedValue ='';
        var sectionActive = false;
        var selected = '';

        // Rewrite the info in the select box
        $('optgroup',this).each(function(x) { //for each optgroup

            //get section label
            var sectionLabel = $(this).attr('label');

            //get section content
            var sectionContent = '';
            $('option',this).each(function(x) {

                /*Check if an option is selected, and set section active*/
                if ($(this).attr('selected')) {
                    selectedValue = $(this).attr('value');
                    sectionActive = true;
                }
                sectionContent += '' + $(this).text() + '\n';
            });

            sectionActive ? selected = true : false;

            topLevel += '
' + sectionLabel + '
\n';

            subLevel += '
\n' + sectionContent + '
\n';

            sectionActive = false;

        });
        //Replacement tabbed select
        var tabbedselect = '
\n';
        if ( opts.position != "tabs-below") tabbedselect += '
\n'+topLevel+'
\n';
        tabbedselect += '
\n'+subLevel+'
\n'
        if ( opts.position == "tabs-below") tabbedselect += '
\n'+topLevel+'
\n';
        tabbedselect += '
\n';

        // Replace the select with the tabbed menu
        this.replaceWith(tabbedselect);

        // Add hidden input field with the selected value
        $("form").append('<input type="hidden" name="'+opts.inputId+'" id="'+opts.inputId+'" value="'+selectedValue+'" />');

        //set first tab and content area active if nothing is selected
        if (selected == false) {
            $("div.tab-content div:first-child").addClass("active");
            $("ul.nav-tabs li:first-child").addClass("active");
        }

        // set input field value on click
        $(".tab-content a").click(function () {
            $("*").removeClass("bsactive");
            $(this).addClass("bsactive");
            $('input#'+opts.inputId).attr('value', $(this).attr('value'));
        })
    };

    //default settings
    $.fn.bootstrapSelect.defaults = {
        'position' : 'tabs-left',
        'inputId'  : 'bootstrap-select'
    };

})(jQuery);

In order to be able to use this plugin on a page with multiple forms, I would recommend identifying the specific form that you are operating on. I can see that you are aware of the issue because you have been using the context parameter in most of the selectors, but since the <form> is an ancestor of this, it would need to be selected differently:

$("form").append(...

should be

$(this).ancestor('form').append(...

Also, I just wanted to mention a personal preference when working with HTML snippets generated by jQuery. Instead of having HTML in JS strings, like

$(this).ancestor("form").append(
    '<input type="hidden" name="'+opts.inputId+'" id="'+opts.inputId+'" value="'+selectedValue+'" />'
);

I like to make hidden template elements, and then clone them, remove the .template class, set the attributes and inner html, and insert the clone into the document. For example:

// stylesheet:
.template { display: none; }

// html, near the end of the <body>:
<input type="hidden" id="hiddenInputTemplate" class="template"/>

// js:
$(this).ancestor('form').append(
    $('#hiddenInputTemplate')
    .clone()
    .removeClass('template')
    .attr('name', opts.inputId)
    .attr('id', opts.inputId)
    .val(selectedValue)
);

For a short example like this, it might not be worth the extra verbosity, but I do have reasons for doing it this way:

My editor can't highlight HTML code, and especially syntax errors, inside of JS strings.
I can read and understand this style a little faster, and I will read it more often than I will write it. Like I said, it's just personal preference.


Bruteforce protector class I'm trying to protect the password login for a website against automated guessing. After too many failed attempts from an IP address, it presents a mandatory CAPTCHA test to the user.

The class is supposed to not forgive the source completely after a successful login attempt, and the more failed attempts the more penalties.

Are there any logic or lock fails here?

public class BruteforceProtector
{
    const int max_log_per_source = 20;
    private ConcurrentDictionary<string, LinkedList<DateTimeOffset>> requests;

    public BruteforceProtector()
    {
        requests = new ConcurrentDictionary<string, LinkedList<DateTimeOffset>>();
    }

    public bool isBlocked(string source, int limit)
    {
        LinkedList<DateTimeOffset> list;
        var haslist = requests.TryGetValue(source, out list);

        if (!haslist)
            return false;

        lock (list)
        {
            removeOld(list);

            return list.Count > limit;
        }
    }

    public void ForgiveSource(string source)
    {
        LinkedList<DateTimeOffset> list;
        if (!requests.TryGetValue(source, out list))
            return;

        lock (list)
        {
            if (list.Count > 0)
                list.RemoveLast();
        }
    }

    public void HandleRequest(string source, int limit, out bool blocked)
    {
        blocked = true;
        bool haslist = true;

        var list = requests.GetOrAdd(source, (x) =>
        {
            var newlist = new LinkedList<DateTimeOffset>();
            newlist.AddLast(DateTimeOffset.Now);
            haslist = false;
            return newlist;
        });

        if (!haslist)
        {
            blocked = false;
            return;
        }
        else
        {
            lock (list)
            {
                list.AddLast(DateTimeOffset.Now);

                removeOld(list);

                if (list.Count > max_log_per_source)
                    list.RemoveFirst();

                blocked = list.Count > limit;
            }
        }
    }

    private void removeOld(LinkedList<DateTimeOffset> list)
    {
        if (list.Count < 1)
            return;

        var oldest = list.First.Value;
        while (oldest.AddHours(list.Count) < DateTimeOffset.Now)
        {
            list.RemoveFirst();

            if (list.Count > 0)
                oldest = list.First.Value;
            else
                break;
        }
    }
}

I'd start with very minor issues.

Usually underscore isn't used in C# names, PascalCase and camelCase are usually preferred (see max_log_per_source). Functions are usually PascalCase (then, for example, isBlocked should be IsBlocked).

Private fields (like requests) do not need any special prefix but you'd better (for clarity) to use a prefix (for example _) or access them like this.requests.

I do not see any extension point in your class then it should be marker sealed.

Fields you initialize only in ctor and that aren't supposed to be changed should be marked readonly: private readonly ConcurrentDictionary<string, LinkedList<DateTimeOffset>> requests;.

In isBlocked() you do not really need haslist local variable. It may be simplified to (you're already doing this in ForgiveSource()):

LinkedList<DateTimeOffset> list;
if (!requests.TryGetValue(source, out list))
    return false;

If you're using C# 7 code might be further simplified:

if (!requests.TryGetValue(source, out var list))
    return false;

ForgiveSource() and HandleRequest() seem to work in concert however I'd change calling logic to avoid to add an entry when login is valid.

This code:

if (condition) {
    // Something
    return;
} else {
    // Something else
}

Can be simplified, you don't need an else here:

if (condition) {
    // Something
    return;
}

// Something else

I find that usually list.Count == 0 is much much clearer than list.Count < 1.

To me a type declared as ConcurrentDictionary<string, LinkedList<DateTimeOffset>> is a missing opportunity to introduce a separate specialized class. More than that you have a thread-safe ConcurrentDictionary and an inner LinkedList which is not. Why don't you introduce a MultiMap<string, DateTimeOffset> type? It's a pretty common data structure to reuse. Also you may wrap it (or extend it) into your own LogInAttemptsCollection (which will be thread-safe). In this way you might separate logic ("list logic" from "is blocked" logic)

Is limit a dynamic parameter which will change for each request? If not then you may consider to move it to a property (or to a readonly field set in ctor).

Why HandleRequest() has an out parameter and a void return type? out parameters should be avoided, when possible, and in this case you just need to change return type to bool.

One note: I feel removeOld() algorithm is way too complex (in general, not for its implementation). Is any reasoning behind this?


Test Scores and Summary Statistics from a file This program takes a file from a user, "example.txt". The file contains only positive integers, and the first interger in the file is stating how many intergers come after that integer (Ex. 3, 45, 98, 102) It is safe to assume everything in the users file is properly formatted and they put .txt at the end. The program is pretty simple for the most part, it was mainly to practice Array's, but I'm curious if I can cut this down a bit, or if I over-complicated anything. This code works by the way.

import java.util.*;
import java.io.*;
import java.lang.*;

public class TestScoresAndSummaryStatistics
{
public static void main(String[] args) throws IOException
{

int i = 0;
Scanner keyboard = new Scanner(System.in);

System.out.println("Enter the name of the file");
String fileName = keyboard.nextLine();
File inputFile = new File(fileName);
Scanner data = new Scanner(inputFile);

//declaring that whatever the first number in the document is is how many indices the document has
int scores[] = new int[data.nextInt()];

while (data.hasNext())
{
  scores[i] = data.nextInt();
  i++;
}

Arrays.sort(scores);

//Highest and Lowest score
System.out.println("Lowest score: " + scores[0]);
System.out.println("Highest score: " + scores[(scores.length - 1)]);

//Calculating Mean
double gradesTotal = 0;
for (i=0; i<scores.length; i++)
{
  gradesTotal = gradesTotal + scores[i];
}
double mean = gradesTotal/scores.length;
System.out.println("Mean score: " + mean);

//Calculating Standard Deviation
double variance = 0;
for (i=0; i<scores.length; i++)
{
  variance = variance + (Math.pow((scores[i] - mean), 2));
}
variance = variance / scores.length;
double standardDeviation = Math.sqrt(variance);
System.out.println("Standard Deviation: " + standardDeviation);

//Finding Median score
double median;
if (scores.length % 2 == 0)
  median = ((scores[(scores.length/2) - 1]) + scores[(scores.length/2)]) / 2;
else
  median = scores[(scores.length/2)];
System.out.println("Median score: " + median);

//Finding how many number of A's, B's, etc
int gradeA = 0;
int gradeB = 0;
int gradeC = 0;
int gradeD = 0;
int gradeF = 0;

for (i=0; i<scores.length; i++)
{
  if (scores[i] >= 90)
  {
    gradeA++;
  }
  else if (scores[i] <= 89 && scores[i] >=80)
  {
    gradeB++;
  }
  else if (scores[i] <= 79 && scores[i] >=70)
  {
    gradeC++;
  }
  else if (scores[i] <= 69 && scores[i] >=60)
  {
    gradeD++;
  }
  else
  {
    gradeF++;
  }
}

System.out.println("You have recieved " + gradeA + " A's, " + gradeB + " B's, " + gradeC + " C's, " + gradeD +
                   " D's, and " + gradeF + " F's");

}
}

Some general issues first:

Indent methods and method blocks. Makes your code more readable
Avoid resource leaks. Close the Scanners in a finally block.

This program has only one method. Split the functionality into many small methods, one for each purpose. For example: reading the scores from the file. This is easy to extract as a method:

    int[] scores = readScoresFromFile(fileName);

...

private static int[] readScoresFromFile(String fileName) throws FileNotFoundException {
    File inputFile = new File(fileName);
    Scanner data = new Scanner(inputFile);
    try {
        //declaring that whatever the first number in the document is is how many indices the document has
        int scores[] = new int[data.nextInt()];

        int i = 0;
        while (data.hasNext())
        {
            scores[i] = data.nextInt();
            i++;
        }

        Arrays.sort(scores);
        return scores;
    }
    finally {
        data.close();
    }
}

I also added the finally block, so the Scanner gets closed. data is a bad name (it could by anything), so let's call it just scanner. Simple and understandable. The method is still too long, so I'd extract one more method, so that the try{} block contains just readScoresFromScanner(scanner);.

As you are using arrays, you need to know the total number of scores in advance. That's not necessary. Collections make your life easier. Assuming you remove the number-of-scores from your input file, you can write:

private static List<Integer> readScoresFromScanner(Scanner scanner) {
    List<Integer> scores = new ArrayList<>();
    while (scanner.hasNext())
    {
        scores.add(scanner.nextInt());
    }
    Collections.sort(scores);
    return scores;
}

Of course, now you need to make some changes to the rest of the code. It will make the code more readable:

    List<Integer> scores = readScoresFromFile(fileName);
    //Highest and Lowest score
    System.out.println("Lowest score: " + Collections.min(scores));
    System.out.println("Highest score: " + Collections.max(scores));

With good refactoring, the comments become unnecessary:

    System.out.println("Mean score: " + calculateMean(scores));

The method becomes very understandable; consider using enhanced loops:

private static double calculateMean(Collection<Integer> scores) {
    double gradesTotal = 0;
    for (int score : scores) {
        gradesTotal = gradesTotal + score;
    }
    return gradesTotal / scores.size();
}

With Java 8, you can also write:

private static double calculateMean(Collection<Integer> scores) {
    return scores
        .stream()
        .mapToInt(Integer::intValue)
        .average()
        .getAsDouble();
}

The code that sums up the grades is very verbose and redundant; try to avoid if-else chains and replace them with, for instance, loops. Use an array or list instead of gradeA, gradeB, etc. A hint:

private static int gradeByScore(int score) {
    int[] scoreForGrade = { 90, 80, 70, 60 };
    for (int grade = 0; grade < scoreForGrade.length; grade++) {
        int requiredScore = scoreForGrade[grade];
        if (score >= requiredScore) {
            return grade;
        }
    }
    return 4; // grade F
}


Populate Drop Down List from SQL Database I am populating a drop down list from my SQL database. List should be able to be displayed with active only, inactive only or both at the same time.

OfficeRepository.cs:

    public static List<OfficeRollups> ListOfficeRollups(bool active, bool inactive)
    {
        List<DataRow> listDataRow = null;

        string whereClause = string.Empty;

        if (active && !inactive)
            whereClause += @" And Active = 1";
        else if (!active && inactive)
            whereClause += @" And Active = 0";

        string srtQry = @"
Select OfficeRollupID, OfficeRollupName, Active
From OfficeRollups
Where 1 = 1 " + whereClause + @"
Order By OfficeRollupName
";

        using (SqlConnection conn = new SqlConnection(Settings.ConnectionString))
        {
            using (SqlCommand objCommand = new SqlCommand(srtQry, conn))
            {
                objCommand.CommandType = CommandType.Text;
                DataTable dt = new DataTable();
                SqlDataAdapter adp = new SqlDataAdapter(objCommand);
                conn.Open();
                adp.Fill(dt);
                if (dt != null)
                {
                    listDataRow = dt.AsEnumerable().ToList();
                }
            }
        }

        var listOfficeRollups = (from o in listDataRow
                           select new OfficeRollups
                           {
                               OfficeRollupID = o.Field<int>("OfficeRollupID"),
                               OfficeRollupName = o.Field<string>("OfficeRollupName"),
                               Active = o.Field<bool>("Active")

                           }).ToList();

        return listOfficeRollups;
    }

    public static IEnumerable<SelectListItem> ListOfficeRollupsForDD(bool active, bool     inactive)
    {
        var listOfficeRollups = OfficeRepository.ListOfficeRollups(active, inactive);

        return from o in listOfficeRollups.ToList()
               where o.OfficeRollupName.Length != 0
               orderby o.OfficeRollupName
               select new SelectListItem
               {
                   Text = o.OfficeRollupName,
                   Value = o.OfficeRollupID.ToString()
               };
    }

HomeController.cs:

    public ActionResult Create()
    {
        Office currentOffice = new Office();

        var listOfficeRollups = OfficeRepository.ListOfficeRollupsForDD(true, false);
        currentOffice.OfficeRollups = listOfficeRollups;

        return View(currentOffice);
    }

Create.cshtml:

@model Office
@using (Html.BeginForm())
{
    @(Html.DropDownList("OfficeRollupID", Model.OfficeRollups))
}

Office.cs:

public class Office
{
    [Required]
    [Display(Name = "Office Rollup")]
    public IEnumerable<SelectListItem> OfficeRollups { get; set; }

    public Int32 OfficeRollupID { get; set; }
}

Here's a simplification of the OfficeRepository methods. Note the other IDisposable types in using blocks:

    public static IEnumerable<OfficeRollups> ListOfficeRollups(bool active, bool inactive)
    {
        var whereClause = active && !inactive
            ? @"Where Active = 1"
            : (!active && inactive ? @"Where Active = 0" : string.Empty);
        var srtQry = @"
Select OfficeRollupID, OfficeRollupName, Active
From OfficeRollups
" + whereClause + @"
Order By OfficeRollupName
";

        using (var conn = new SqlConnection(Settings.ConnectionString))
        using (var objCommand = new SqlCommand(srtQry, conn) { CommandType = CommandType.Text })
        using (var dt = new DataTable())
        using (var adp = new SqlDataAdapter(objCommand))
        {
            conn.Open();
            adp.Fill(dt);
            return dt.AsEnumerable().Select(o => new OfficeRollups
            {
                OfficeRollupID = o.Field<int>("OfficeRollupID"),
                OfficeRollupName = o.Field<string>("OfficeRollupName"),
                Active = o.Field<bool>("Active")
            }).ToList();
        }
    }

    public static IEnumerable<SelectListItem> ListOfficeRollupsForDD(bool active, bool inactive)
    {
        var listOfficeRollups = OfficeRepository.ListOfficeRollups(active, inactive);

        return listOfficeRollups
                .Where(o => !string.IsNullOrEmpty(o.OfficeRollupName))
                .OrderBy(o => o.OfficeRollupName)
                .Select(o => new SelectListItem
                {
                    Text = o.OfficeRollupName,
                    Value = o.OfficeRollupID.ToString()
                })
                .ToList();
    }


Project Euler Problem 12 - triangle number with 500 divisors I've just done Problem 12 of Project Euler:

  What is the value of the first triangle number to have over five hundred divisors?

  The \$N\$'th triangle number is the sum of all natural numbers from \$1\$ to \$N\$, for example, the 5th triangle number is \$1 + 2 + 3 + 4 + 5 = 15\$

I get the correct answer in around 2.5 seconds.

I was wondering if anyone could suggest anything that could improve the performance of the program, or improve the code in general.

public class Problem12 {

public static void main(String[] args) {
    double startTime = System.currentTimeMillis();
    run();
    double endTime = System.currentTimeMillis();
    System.out.println("Took "+((endTime - startTime) / 1000)+" seconds");
}

public static void run() {
    boolean go = true;
    long number = 1;
    long nextNum = 2;
    int maxDivisors = 500;
    while (go) {
        if (countDivisors(number) > maxDivisors) {
            System.out.println("The first triangle number with over "+maxDivisors+" divisors is: "+number);
            go = false;
        }
        else {
            number += nextNum;
            nextNum++;
        }
    }
}

public static long countDivisors(long number) {
    long divisors = 0;
    for (int i = 1; i*i <= number; i++) {
        if (number % i == 0) {
            divisors+=2;
        }
    }
    return divisors;
}
}

There is an error in your countDivisors() function, the result is one too large
for a perfect square, i.e. countDivisors(25) returns 4 instead of 3.
Also the loop variable i should be a long to avoid an integer overflow
when testing i*i <= number.
 A correct version would be

public static long countDivisors(long number) {
    long divisors = 0;
    for (long i = 1; i*i <= number; i++) {
        if (number % i == 0) {
            if (i*i < number) {
                divisors += 2; // i and number/i are (different) divisors
            } else {
                divisors += 1; // i == number/i is a divisor
            }
        }
    }
    return divisors;
}

The run() function can be simplified slightly, the boolean go variable
is not really necessary:

public static void run() {
    long number = 1;
    long nextNum = 2;
    int maxDivisors = 500;
    while (countDivisors(number) <= maxDivisors) {
        number += nextNum;
        nextNum++;
    }
    System.out.println("The first triangle number with over "+maxDivisors+" divisors is: "+number);
}

To improve the performance, use the fact that

triangle numbers are of the form \$ n \, (n+1)/2  \$, and
the "number-of-divisors function" \$ \sigma_0(n) \$ is multiplicative,
i.e. \$ \sigma_0(m \, n) =  \sigma_0(m) \, \sigma_0(n) \$ if \$ m, n \$
are relative prime (see Divisor Function).

If \$ n \$ is even then \$ n/2 \$ and \$ n+1 \$ are relative prime,
and if \$ n \$  is odd then \$ n \$  and \$ (n+1)/2 \$ are relative prime.
This leads to the following implementation

public static void run() {
    long n = 1;
    int maxDivisors = 500;
    while (countDivisors((n+1)/2) * countDivisors(n) <= maxDivisors) {
        n++;
        if (countDivisors(n/2) * countDivisors(n+1) > maxDivisors) {
            break;
        }
        n++;
    }
    long number = n*(n+1)/2;
    System.out.println("The first triangle number with over "+maxDivisors+" divisors is: "+number);
}

which is faster because the countDivisors() function is called with smaller
numbers. On my computer it reduces the time from 0.6 to 0.02 seconds.

A faster implementation of countDivisors() would use the prime
factorisation of the given number, as explained in the above referenced
Wikipedia article.


Generate [min, max] range from strings I need to generate a minimum and maximum value for a range of speeds (slow, medium, fast), the user can specify any combination of the 3 values, and receive a range that encompasses all.

Given any combination of "slow", "medium", "fast", getRange() will return a [min, max] range.

The function can be called in the following ways:

// Any combination of the values
getRange(['slow', 'medium', 'fast']); // [0, 100]
getRange(['medium', 'fast']); // [36, 100]
getRange(['slow']); // [0, 35]

// Or just a single string
getRange('fast'); // [76, 100]

// No string specified
getRange(); // [0, 100]

Is there any way to simplify my code? It seems a bit clunky and there's probably a simpler way of writing this:

function getRange(speed){
    var min, max;

    if(speed && speed.length){
        var obj = {};

        if(Array.isArray(speed)){
            speed.forEach(function(s){
                obj[s] = true;
            });
        }
        else{
            obj[speed] = true;
        }

        if(obj.slow){
            min = 0;
            max = 35;
        }

        if(obj.medium){
            if(typeof min == 'undefined'){
                min = 36;
            }
            max = 75;
        }

        if(obj.fast){
            if(typeof min == 'undefined'){
                min = 76;
            }
            max = 100;
        }
    }

    if(typeof min == 'undefined'){
        min = 0;
    }

    if(typeof max == 'undefined'){
        max = 100;
    }

    return [min, max];
};

I do believe this could be done easier. You could use the || shortcut to set min and max if you could not find a speed range, and you could extract the numbers into a data structure and go from there.

Something like this:

function getRange(speed){
    //Speed range config, in array to preserve order
    var speedRanges = [
            { name: 'slow',   min: 1  , max : 35 },
            { name: 'medium', min: 36 , max : 75 },
            { name: 'fast',   min: 76 , max : 100 }
        ],
        min, max, i, range;
    //Build logic once for arrays, if we are not dealing with an array,
    //then turn speed into an array
    if (!Array.isArray(speed))
        speed = [speed];
    //Check for each speed range, apply if applicable
    for (i = 0; i < speedRanges.length; i++) {
        range = speedRanges[i];
        if (~speed.indexOf(range.name)) {
            min = min || range.min;
            max = range.max;
        }
    }

    min = min || 1;
    max = max || 100;
    return [min, max];
}

If you absolutely need slow to be 0 to 35 then you will have to enhance min = min || range.min; since 0 counts as not set..


ObservablePriorityQueue Implementation I have a requirement in my current project that will need a Prioritised Queue that supports the IObservable interface. Please notify me of any problems with the implementation that I currently have:

ObservablePriorityQueue<T>

public sealed class ObservablePriorityQueue<T> : IQueue<T>, IObservable<T> where T : IPrioritised
{
    #region "IObservable<T> Implementation"
    // A list of the subscribers for the IObservable implementation
    List<IObserver<T>> _subscribers = new List<IObserver<T>>(10);

    Object _observableSyncLock = new Object();

    #region "Interface specific"
    public IDisposable Subscribe(IObserver<T> observer)
    {
        if (observer == null)
        {
            throw new ArgumentNullException("The observer cannot be null.");
        }

        if (!_subscribers.Contains(observer))
        {
            lock (_observableSyncLock)
            {
                _subscribers.Add(observer);
            }
        }

        return new Disposable(() =>
        {
            this.Unsubscribe(observer);
        });
    }
    #endregion

    public void Unsubscribe(IObserver<T> observer)
    {
        if (observer == null)
        {
            throw new ArgumentNullException("The observer cannot be null.");
        }

        observer.OnCompleted();

        if (_subscribers.Contains(observer))
        {
            // remove the entry, but don't dispose it just
            // in case they want to re-subscribe with the same observer later
            lock (_observableSyncLock)
            {
                _subscribers.Remove(observer);
            }
        }
    }
    #endregion

    #region "IQueue<T> Implementation"

    readonly List<T> _data = new List<T>(100);
    Object _queueSyncLock = new Object();

    public void Enqueue(T value)
    {
        if (value == null)
        {
            throw new ArgumentException("The item to be enqueued cannot be null");
        }

        lock (_queueSyncLock)
        {
            _data.Add(value);
        }

        // now that the entry has been added, notify everyone
        Task.Run(() =>
        {
            lock (_observableSyncLock)
            {
                foreach (IObserver<T> subscriber in _subscribers)
                {
                    subscriber.OnNext(value);
                }
            }
        });
    }

    public T Dequeue()
    {
        if (_data.Count > 0)
        {
            lock (_queueSyncLock)
            {
                var result = _data.OrderByDescending(element => element.Priority).ThenBy(element => element.TimeStamp).First();
                _data.Remove(result);
                return result;
            }
        }
        return default(T);
    }

    public T Peek()
    {
        if (_data.Count > 0)
        {
            return _data.OrderByDescending(element => element.Priority).ThenBy(element => element.TimeStamp).First();
        }
        return default(T);
    }

    public Int32 Count { get { return _data.Count; } }
    #endregion
}

IQueue<T>

public interface IQueue<T>
{

    void Enqueue(T value);
    T Dequeue();
    T Peek();

    Int32 Count { get; }
}

IPrioritised<T>

public interface IPrioritised
{
    QueuePriority Priority { get; }
    DateTime TimeStamp { get; }
}

QueuePriority

public enum QueuePriority
{
    None = 0,
    Lowest = 1,
    Low = 2,
    Normal = 3,
    High = 4,
    Highest = 5
}

Disposable

public sealed class Disposable : IDisposable
{
    readonly Action _action;
    public Disposable(Action action)
    {
        _action = action;
    }
    public void Dispose()
    {
        if (_action != null)
        {
            _action();
        }
    }
}

At first, I was confused what exactly did the implementation of IObservable<T> mean. You should properly document that.
You don't need to have a separate lock objects. If you're locking on a specific object, I think you should use that object in the lock.
If you're just calling a single method in your lambda, you can use more succinct syntax: new Disposable(() => this.Unsubscribe(observer)).
If you want to make your code thread-safe, you need to lock all your reads too. In Unsubscribe() you don't do that, which means someone could write to the list while you're reading it. The same problem is in Peek().
Dequeue() shouldn't return default(T) if the queue is empty. This can be problematic especially with value types. Instead, you should have a method like bool TryDequeue(out T result).
You're accessing Count outside of a lock. I wouldn't rely on the fact that doing this is safe, I think you should lock before accessing it too.
Since you're using Rx, you might as well use their Disposable.Create() instead of creating your own.


Prime numbers using wheel factorial I have written a program to find prime numbers up to a given limit using wheel factorization of 2, 3 and 5. I would like it to work above \$10^{10}\$, but its still pretty slow.

What else can I optimize here.
Any way I can parallelize it?
I am pretty sure markNonPrimes's algorithm is less efficient than it should be. Is it?  I feel that I am brute forcing there: it takes ~250ms when limit is \$10^6\$, ~1500ms when limit is \$10^8\$.

#include <iostream>
#include <vector>
#include <algorithm>
#include <ctime>

typedef uint64_t u64;

const uint8_t ONE = 1;
const u64 F30[] = {1,7,11,13,17,19,23,29};

void markNonPrimes(const u64 start, const u64 num, const u64 lcm, const u64 limit, std::vector<uint8_t> &primes)
{
   for (u64 k = start; (lcm*k) < limit; k++)
   {
      u64 K = lcm*k;
      if ((primes[k] & (ONE << 0)) && (K+F30[0]) % num == 0)
         primes[k] &= ~(ONE << 0);
      if ((primes[k] & (ONE << 1)) && (K+F30[1]) % num == 0)
         primes[k] &= ~(ONE << 1);
      if ((primes[k] & (ONE << 2)) && (K+F30[2]) % num == 0)
         primes[k] &= ~(ONE << 2);
      if ((primes[k] & (ONE << 3)) && (K+F30[3]) % num == 0)
         primes[k] &= ~(ONE << 3);
      if ((primes[k] & (ONE << 4)) && (K+F30[4]) % num == 0)
         primes[k] &= ~(ONE << 4);
      if ((primes[k] & (ONE << 5)) && (K+F30[5]) % num == 0)
         primes[k] &= ~(ONE << 5);
      if ((primes[k] & (ONE << 6)) && (K+F30[6]) % num == 0)
         primes[k] &= ~(ONE << 6);
      if ((primes[k] & (ONE << 7)) && (K+F30[7]) % num == 0)
         primes[k] &= ~(ONE << 7);
   }
}

void getPrimesUpto(u64 limit)
{
   const u64 lcm = 30;
   std::vector<uint8_t> primes(limit*0.26 + 1,0xff);

   u64 k = 1;
   u64 n = 0;
   u64 count=3-1;

   markNonPrimes(k, F30[1], lcm, limit, primes);
   markNonPrimes(k, F30[2], lcm, limit, primes);
   markNonPrimes(k, F30[3], lcm, limit, primes);
   markNonPrimes(k, F30[4], lcm, limit, primes);
   markNonPrimes(k, F30[5], lcm, limit, primes);
   markNonPrimes(k, F30[6], lcm, limit, primes);
   markNonPrimes(k, F30[7], lcm, limit, primes);

   while(lcm*k*lcm*k<limit)
   {
      const u64 K = lcm*k;
      if(primes[k] & (ONE<<0))
         markNonPrimes(k+1, K+F30[0], lcm, limit, primes);

      if(primes[k] & (ONE<<1))
         markNonPrimes(k+1, K+F30[1], lcm, limit, primes);

      if(primes[k] & (ONE<<2))
         markNonPrimes(k+1, K+F30[2], lcm, limit, primes);

      if(primes[k] & (ONE<<3))
         markNonPrimes(k+1, K+F30[3], lcm, limit, primes);

      if(primes[k] & (ONE<<4))
         markNonPrimes(k+1, K+F30[4], lcm, limit, primes);

      if(primes[k] & (ONE<<5))
         markNonPrimes(k+1, K+F30[5], lcm, limit, primes);

      if(primes[k] & (ONE<<6))
         markNonPrimes(k+1, K+F30[6], lcm, limit, primes);

      if(primes[k] & (ONE<<7))
         markNonPrimes(k+1, K+F30[7], lcm, limit, primes);

      k++;
   }

   for(u64 k=0; lcm*k < limit; k++ )
   {
      for(u64 i =0; i<8; i++)
         if(primes[k] & ONE<< i && lcm*k+F30[i] < limit) {count++;/*std::cout << lcm*k+F30[i] << " ";*/}
   }

   std::cout  << "\n======\n"<< count << "\n";
}

int main()
{
   clock_t start = clock();
   getPrimesUpto(100);
   std::cout << "\nTime: "<< clock() - start << std::endl;
}

for loops

You are doing the same thing over and over again in many places in the code.

For example,

for (u64 k = start; (lcm*k) < limit; k++){
    u64 K = lcm*k;
    if ((primes[k] & (ONE << 0)) && (K+F30[0]) % num == 0)
        primes[k] &= ~(ONE << 0);
        ......
        if ((primes[k] & (ONE << 7)) && (K+F30[7]) % num == 0)
            primes[k] &= ~(ONE << 7);
}

could become

for(u64 k = start; lcm*k < limit; k++)
{
    u64 K = lcm*k;
    for(int i = 0; i < 8; i++)
    {
        if ((primes[k] & (ONE << i)) && (K+F30[i]) % num == 0)
            primes[k] &= ~(ONE << i);
    }
}

There are many other places you could do this as well. Also, the while loop near the bottom could be transformed into a for loop, as you are basically doing a for loop with while.
u64 vs int

Don't use a u64 when you don't need them; ints save on storage. You can replace the bottom two loops with regular ints.
Comments

Your code is somewhat confusing when you look at it on first glance. Maybe you should add some comments for the reader to see what is happening in your MarkNonPrimes function. Also, in the variable count (why declare it to 3-1 instead of 2?), and how you got that magic constant 0.26 in your vector declaration. The naming of F30 is pretty cryptic too.
Conventions

If you are going to use a convention then stick with it. Don't use ONE << 0 then ONE<<0 later.
Global variables

See here. Pass the variables as a parameter instead of declaring them globally, because as the project grows in size, then it will become harder to not have an ambiguous match for your global names. However, the constant ONE is fine, because not many variables will have the name one, but you should rename it u8_ONE instead because you may be declaring a u16_ONE or something of the sort.

Here is the code with all the modifications (except comments and naming):

#include <iostream>
#include <vector>
#include <algorithm>
#include <ctime>

typedef uint64_t u64;
const uint8_t u8_ONE = 1;
void markNonPrimes(const u64 start, const u64 num, const u64 lcm, const u64 limit, std::vector<uint8_t> &primes, u64 F30[])
{
    for(u64 k = start; lcm*k < limit; k++)
    {
        u64 K = lcm*k;
        for(int i = 0; i < 8; i++)
        {
            if ((primes[k] & (u8_ONE << i)) && (K + F30[i]) % num == 0)
                primes[k] &= ~(u8_ONE << i);
        }
    }
}

void getPrimesUpto(u64 limit)
{
   const u64 F30[] = {1,7,11,13,17,19,23,29};

   const u64 lcm = 30;
   std::vector<uint8_t> primes(limit*0.26 + 1,0xff);
   u64 n = 0;
   u64 count = 2;
   for(int i = 1; i < 8; i++)
   {
       markNonPrimes(1, F30[i], lcm, limit, primes, F30);
   }
   for(int k = 1; lcm * k * lcm * k < limit; k++)
   {
      const u64 K = lcm * k;
      for(int i = 0; i < 8; i++)
          if(primes[k] & (u8_ONE << i))
              markNonPrimes(k+1, K+F30[i], lcm, limit, primes, F30);
   }

   for(int k=0; lcm*k < limit; k++)
   {
      for(int i = 0; i < 8; i++)
         if(primes[k] & u8_ONE << i && lcm * k + F30[i] < limit)
         {
             count++;
             /*std::cout << lcm*k+F30[i] << " ";*/
         }
   }

   std::cout  << "\n======\n"<< count << "\n";
}

int main()
{
   clock_t start = clock();
   getPrimesUpto(100);
   std::cout << "\nTime: "<< clock() - start << std::endl;
}


Speed up script that calculates distribution of every character from user input I have a data set with close to 6 million rows of user input. Specifically, users were supposed to type in their email addresses, but because there was not pattern validation put in place we have a few months worth of interesting input.

I've come up with a script that counts every character, then combines it that so I can see the distribution of all characters. This enables me to do further analysis and get a sense of the most common mistakes so I can begin to clean the data. My question is: how would you optimize the following for speed?

import pandas as pd
import numpy as np
from pandas import Series, DataFrame
from collections import Counter

df = pd.DataFrame({'input': ['Captain Jean-Luc Picard <picard@starfleet.com>','deanna.troi@starfleet.com','geordi @starfleet.com','data@starfleet.com','rik#er@starfleet.com'],
'metric1': np.random.randn(5).cumsum(),
'metric2': np.random.randn(5)})

l = []
for i in range(len(df.index.values)):
    l.append(dict(Counter(df.ix[i,'input'])))
dist = pd.DataFrame(l).fillna(0)
dist = dist.sum(axis=0)
print(dist)

I've run this over ~1/3 of my dataset, and it takes a while; it's still tolerable, I'm just curious if anyone could make it faster.

Since you are using Counter already, it should be faster to do the whole job with it:

c = Counter()
for i in range(len(df.index.values)):
    c.update(df.ix[i,'input'])

for k, v in c.items():
    print(k, v)


Length of the longest sequence of repeating characters I have been going through some CodingBat exercises and I came across this problem. "Given a string, return the length of the largest "block" in the string. A block is a run of adjacent chars that are the same."

Required output:

maxBlock("hoopla") → 2
maxBlock("abbCCCddBBBxx") → 3
maxBlock("") → 0

My code seems to pass all the tests except for the last "other test". Can someone please review my code and tell me where exactly I could have gone wrong?

 public int maxBlock(String str) {
      int charBlock = 0;
      int holder = 1;
      if(str.length() == 0){ //If string is empty return 0
         charBlock = 0;
      } else if(str.length() == 1){ //If string contains only a single char return 1
         charBlock = 1;
      } else {
          for(int i=0; i < str.length()-1; i++){   //loop through each char value
              if((str.length() == 2) && (str.charAt(i) != str.charAt(i+1))){
                  charBlock =1; //return 1 if the length of the string is 2 and non of the two chars match
              }
              else if((str.length() == 3) && (str.charAt(i) != str.charAt(i+1))){
                 charBlock = 1; //return 1 if the length of the string is 3 and non of the three chars match
              }
              else if (str.charAt(i) == str.charAt(i+1)){
                 holder = holder + 1;
                   if(holder > charBlock){
                     charBlock = holder; //update the value of charBlock if a holder is larger current value
                     }
             } else holder = 1;
          }
      }
      return charBlock;
    }

Example test cases:

Expected    Run
maxBlock("hoopla") → 2  2   OK
maxBlock("abbCCCddBBBxx") → 3   3   OK
maxBlock("") → 0    0   OK
maxBlock("xyz") → 1 1   OK
maxBlock("xxyz") → 2    2   OK
maxBlock("xyzz") → 2    2   OK
maxBlock("abbbcbbbxbbbx") → 3   3   OK
maxBlock("XXBBBbbxx") → 3   3   OK
maxBlock("XXBBBBbbxx") → 4  4   OK
maxBlock("XXBBBbbxxXXXX") → 4   4   OK
maxBlock("XX2222BBBbbXX2222") → 4   4   OK
other tests                             X

The missed edge case

The test case you're missing is longer text with non-repeating letters, for example: abcdefghijkl. And it's easy to fix that: just add these lines before the return statement:

    if (holder > charBlock) {
        charBlock = holder;
    }

The biggest clue for figuring out this problem was around these lines:

if((str.length() == 2) && (str.charAt(i) != str.charAt(i+1))){
    charBlock =1; //return 1 if the length of the string is 2 and non of the two chars match
}
else if((str.length() == 3) && (str.charAt(i) != str.charAt(i+1))){
    charBlock = 1; //return 1 if the length of the string is 3 and non of the three chars match
}
else if (str.charAt(i) == str.charAt(i+1)){

That is, the special treatment for length 2 and 3. There doesn't seem to be a logical reason to treat these cases any special, and you could have quessed that this might break for longer strings. And as it turns out, these conditions are completely unnecessary: you can safely remove them, and the code will still pass the tests.

Code review

About this piece:

  int charBlock = 0;
  int holder = 1;
  if(str.length() == 0){ //If string is empty return 0
     charBlock = 0;
  } else if(str.length() == 1){ //If string contains only a single char return 1
     charBlock = 1;
  } else {
     // ...
  }
  return charBlock;

Two things:

It's good to use early returns to reduce the indent level and make the code more readable
It's good to limit variables to the smallest scope possible

These two ideas go hand in hand in this example. Consider this alternative:

    if (str.length() == 0) {
        return 0;
    }
    if (str.length() == 1) {
        return 1;
    }
    int charBlock = 0;
    int holder = 1;
    // ...
    return charBlock;

The guard statements don't need the charBlock or holder variable,
they can simply return,
and now those variables are declared where they are really needed.

When checking if a string is empty, use str.isEmpty() instead of checking on the length.

And in this program, you don't actually need the str.length() == 1 check,
if you omit it the program will still work and pass the tests.

But the single biggest problem with this code, in my opinion,
is the naming of the variables charBlock and holder.
I would name them longest and length, respectively.

Putting the above suggestions together, the implementation becomes this:

    if (str.isEmpty()) {
        return 0;
    }
    int longest = 0;
    int length = 1;
    for (int i = 0; i < str.length() - 1; i++) {
        if (str.charAt(i) == str.charAt(i + 1)) {
            ++length;
            if (length > longest) {
                longest = length;
            }
        } else {
            length = 1;
        }
    }
    if (length > longest) {
        longest = length;
    }
    return longest;

Almost. There is one more important improvement to do: why update the longest length while still counting the same letters? It would be more efficient to move that piece of code to the else block, like this:

        if (str.charAt(i) == str.charAt(i + 1)) {
            ++length;
        } else {
            if (length > longest) {
                longest = length;
            }
            length = 1;
        }

Unit testing

When refactoring non-trivial code like this,
it's good to have unit tests to make it easy to re-validate everything, for example:

@Test
public void test_hoopla() {
    assertEquals(2, maxBlock("hoopla"));
}

@Test
public void test_abbCCCddBBBxx() {
    assertEquals(3, maxBlock("abbCCCddBBBxx"));
}

@Test
public void test_empty() {
    assertEquals(0, maxBlock(""));
}

@Test
public void test_xyz() {
    assertEquals(1, maxBlock("xyz"));
}

@Test
public void test_xxyz() {
    assertEquals(2, maxBlock("xxyz"));
}

@Test
public void test_longer_nonrepeating_sequence() {
    assertEquals(1, maxBlock("abcdefghijkl"));
}

Especially when the contest provides the cases you should pass.
Heck, it's good to write unit tests always.


Templated queue implementation I wrote an implementation for a simple queue in C++ (for practice, since there's an stl queue).  I've been trying to focus on memory management and the rule of three.  Any suggestions/improvements?

(Personally, I feel like there has to be a better way to overload the copy assignment operator).

#ifndef QUEUE_H
#define QUEUE_H

#include "assert.h"

#define NULL 0

template <typename T>
class Queue
{
 public:
  Queue();
  Queue(const Queue& other);
  ~Queue();

  void enqueue(T data);
  T    dequeue();

  bool isEmpty() { return (front_ == back_); }

  Queue& operator=(const Queue& other);

 private:
  struct node
  {
    T     data;
    node* next;
  };

  node* front_;
  node* back_;
};

template <typename T>
Queue<T>::Queue()
{
  front_ = new node();
  back_  = front_;
}

template <typename T>
Queue<T>::Queue(const Queue<T>& other)
{
  front_     = new node();
  back_      = front_;
  node* temp = other.front_;

  while (temp != other.back_)
  {
    enqueue(temp->data);
    temp = temp->next;
  }
}

template <typename T>
Queue<T>::~Queue()
{
  while (!isEmpty()) { dequeue(); }
}

template <typename T>
void Queue<T>::enqueue(T data)
{
  back_->data = data;
  back_->next = new node();
  back_       = back_->next;
}

template <typename T>
T Queue<T>::dequeue()
{
  assert(!isEmpty());

  node* temp = front_;
  T     data = front_->data;
  front_     = front_->next;

  delete temp;
  return data;
}

template <typename T>
Queue<T>& Queue<T>::operator=(const Queue<T>& other)
{
  if (this != &other)
  {
    node* temp = other.front_;

    while (!isEmpty()) { dequeue(); }
    while (temp != other.back_)
    {
      enqueue(temp->data);
      temp  = temp->next;
    }
  }
  return *this;
}

#endif

Use the correct headers

#include "assert.h"

should be

#include <cassert>

since you're writing C++. C++ provides special headers for all C headers, they generally start with a 'c' and have the '.h' removed at the end. Prefer these.

Also system headers, should always use <> rather than "".

Your definition of NULL is unsafe

NULL is typically reserved to use in the context of pointers. Defining NULL as you do means that NULL is not of a pointer type and you can pass NULL as an integer for example this is not desirable.

If you must define NULL you should do it like this:

#ifndef NULL
#define NULL (reinterpret_cast<void*>(0))
#endif

but really most likely it is already included by one of your other headers (<cstddef>, <clocale>, <cstdio>, <cstdlib>, <cstring>, <ctime> and anything that includes those, so pretty much everything).

If you are using C++11 or newer you should use nullptr instead of NULL.

Style

I personally would name node with the same convention as your other classes, Node. I'm also not a big fan of using _ as a suffix for class members but that is personal taste.

Pass template arguments by constant reference

As T can be just about anything, you should pass all Ts by const reference (const T&) to avoid possibly expensive copies.

Const correctness

Methods that do not modify the state of the queue should be declared const.

bool isEmpty()

should be

bool isEmpty() const

being const correct allows you to get compile time errors if you unintentionally modify something you didn't intend to. It's a good habit.

Construct objects as close as possible to their final state

Your node class should have a constructor that initialises the data member. This will reduce some code for you.

Put the sentinel in the class.

Allocating the sentinel node on the heap in your constructor is unnecessary. Prefer to have it be a member variable. It is a good practice to not allocate on the heap when it suffices to use a local or member variable.

Avoid code duplication

The copy constructor and copy assignment operator are typical places where one usually gets code duplication.

I would suggest using the Copy and Swap Idiom for implementing the assignment operator.

And welcome to the site!


The fastest way to find one unique value when all other values are the same I have completed this question and I am wondering what is the fastest way to solve it.
The question is "There is an array with some numbers. All numbers are equal except for one. Try to find it!"
Example:
find_uniq([ 1, 1, 1, 2, 1, 1 ]) == 2
find_uniq([ 0, 0, 0.55, 0, 0 ]) == 0.55

I came up with the solution:
from collections import Counter

def find_uniq(arr):
    nums = list(Counter(arr).items())
    data = [i for i in nums if i[1] == 1]
    return data[0][0]

I decided on using Counter because I felt comfortable using it but when looking at others answers some use sets and others use counter as well.
I am wondering is my code sufficient and which method to solving this question would lead to the fastest time complexity?

Benchmarks!
Benchmarks for lists with a thousand or a million elements, with the unique element in the middle of the array to reflect the "typical"/"average" case. The results are times, so lower=faster.
n=1000
0.90 find_uniq_Jacques
1.18 find_uniq_tinstaafl_1
0.59 find_uniq_tinstaafl_2
0.88 find_uniq_GZ0_1
0.14 find_uniq_GZ0_2
0.88 find_uniq_Peilonrayz
0.22 find_uniq_RootTwo
0.26 find_uniq_HeapOverflow_1
0.28 find_uniq_HeapOverflow_2
0.26 find_uniq_HeapOverflow_3
0.09 find_uniq_HeapOverFlow_Codewars
0.06 find_uniq_HeapOverflow_GZ0
0.57 unique_different_ethiy
0.28 find_uniq_KyleG_1
0.25 find_uniq_KyleG_2

n=1000000
0.94 find_uniq_Jacques
1.36 find_uniq_tinstaafl_1
0.68 find_uniq_tinstaafl_2
0.99 find_uniq_GZ0_1
0.19 find_uniq_GZ0_2
0.98 find_uniq_Peilonrayz
0.19 find_uniq_RootTwo
0.23 find_uniq_HeapOverflow_1
0.26 find_uniq_HeapOverflow_2
0.25 find_uniq_HeapOverflow_3
0.09 find_uniq_HeapOverFlow_Codewars
0.04 find_uniq_HeapOverflow_GZ0
0.57 unique_different_ethiy
0.28 find_uniq_KyleG_1
0.22 find_uniq_KyleG_2

Done with Python 3.8.1 32 bit on Windows 10 64 bit.
Benchmark code:
from timeit import timeit
from collections import Counter
from itertools import groupby

solutions = []
def register(solution):
    solutions.append(solution)
    return solution

@register
def find_uniq_Jacques(arr):
    nums = list(Counter(arr).items())
    data = [i for i in nums if i[1] == 1]
    return data[0][0]

@register
def find_uniq_tinstaafl_1(arr):
    for i in range(len(arr)-1):
        if arr[i] != arr[i+1]:
            if i == 0 and arr[i] != arr[i + 2]:
                return arr[i]
            return arr[i + 1]

@register
def find_uniq_tinstaafl_2(arr):
    for i in range(0,len(arr) - 1, 2):
        if arr[i] != arr[i+1]:
            if i == 0:
                if arr[i] != arr[i + 2]:
                    return arr[i]
                return arr[i + 1]
            else:
                if arr[i] != arr[i-1]:
                    return arr[i]
                return arr[i + 1]
    return arr[-1]

@register
def find_uniq_GZ0_1(arr):
    return next(k for k, freq in Counter(arr).items() if freq == 1)

@register
def find_uniq_GZ0_2(arr):
    group_iter = groupby(arr)
    k1, g1 = next(group_iter)
    c1 = len(list(g1))
    k2, g2 = next(group_iter)
    if c1 > 1:
       # Group g1 has more than one element
       return k2
    try:
       # Group g2 has more than one element
       next(g2)
       next(g2)
       return k1
    except StopIteration:
       # Both g1 and g2 has one element
       return k2 if next(group_iter)[0] == k1 else k1

@register
def find_uniq_Peilonrayz(arr):
    return Counter(arr).most_common()[-1][0]

@register
def find_uniq_RootTwo(arr):
    a, b = set(arr)
    return a if arr[:3].count(a) < 2 else b

@register
def find_uniq_HeapOverflow_1(arr):
    a = arr[0]
    if a not in arr[1:3]:
        return a
    for b in arr:
        if b != a:
            return b

@register
def find_uniq_HeapOverflow_2(arr):
    dupe = sorted(arr[:3])[1]
    for x in arr:
        if x != dupe:
            return x

@register
def find_uniq_HeapOverflow_3(arr):
    a = arr[0]
    for b in arr:
        if b != a:
            return b if a in arr[1:3] else a

@register
def find_uniq_HeapOverFlow_Codewars(arr):
    arr.sort()
    return arr[-(arr[0] == arr[1])]

@register
def find_uniq_HeapOverflow_GZ0(arr):
    group_iter = groupby(arr)
    k1, _ = next(group_iter)
    k2, g2 = next(group_iter)
    next(g2)
    return k1 if k2 in g2 else k2

@register
def unique_different_ethiy(iterable):
    # assert isinstance(iterable, Iterable)
    # assert len(iterable) > 2
    if iterable[0] != iterable[1]:
        return iterable[0] if iterable[1] == iterable[2] else iterable[1]
    else:
        for element in iterable[2:]:
            if element != iterable[1]:
                return element

@register
def find_uniq_KyleG_1(arr):
    common = arr[0]
    if common not in arr[1:3]:
        return common
    for a, b in zip(arr[1::2], arr[2::2]):
        if a != b:
            if a == common:
                return b
            else:
                return a
    return arr[-1]

@register
def find_uniq_KyleG_2(arr):
    iterator = iter(arr)
    common = next(iterator)
    if common not in arr[1:3]:
        return common
    for a, b in zip(iterator, iterator):
        if a != b:
            if a == common:
                return b
            else:
                return a
    return arr[-1]

# Run the benchmarks
for e in 3, 6:
    n = 10**e
    number = 10**(7 - e)  # fewer number of runs for larger n
    print(f'{n=}')
    arr = [0] * n
    arr[n // 2] = 1

    # Repeat round-robin to reduce effects of CPU speed changes etc
    timeses = [[] for _ in solutions]
    for i in range(20):
        for solution, times in zip(solutions, timeses):
            arrs = iter([arr[:] for _ in range(number)])
            t = timeit(lambda: solution(next(arrs)), number=number)
            times.append(t)
        print(i, end=' ')
    print()
    for solution, times in zip(solutions, timeses):
        print('%.2f' % min(times), solution.__name__)
    print()


Calculating the distance squared between all vertex-pairs of a number of 2D polygons I have implemented my code using Cython. It is the current bottleneck in my computations.

There are two non-numpy functions involved:

calculate_2D_dist_squared which calculates the distance squared between two points
calculate_2D_dist_squared_matrix which generates the distances squared between every two distinct vertices. calculate_2D_dist_squared_matrix organizes results so that dist_squared_matrix[1, 2, 3, 4] = "the distance between polygon 1, vertex 2 and polygon 3, vertex 4". All indexing starts from 0.

cdef double calculate_2D_dist_squared(self, np.ndarray[np.float64_t, ndim=1] p1, np.ndarray[np.float64_t, ndim=1] p2):
    cdef np.ndarray[np.float64_t, ndim=1] relative_vector = p1 - p2

    return relative_vector[0]**2 + relative_vector[1]**2

cdef np.ndarray[np.float64_t, ndim=4] calculate_2D_dist_squared_matrix(self, np.ndarray[np.float64_t, ndim=3] polygons_vertex_coords, int num_polygons, int num_vertices):
    cdef:
        int pi_focus
        int vi_focus
        int pi
        int vi
        # at initialization, set all dist_squared values to be -1,
        # indicating that they have been initialized, but not set properly
        # since by definition a dist_squared value has to be >= 0
        np.ndarray[np.float64_t, ndim=4] result  = -1*np.ones((num_polygons, num_vertices, num_polygons, num_vertices), dtype=np.float64)

    for pi_focus in range(num_polygons):
        for vi_focus in range(num_vertices):
            for pi in range(num_polygons):
                for vi in range(num_vertices):
                    # if a dist_squared < 0, then it means that it
                    # it has not been changed since initialization, and
                    # needs to be updated, this way I avoid repeating work
                    if result[pi_focus, vi_focus, pi, vi] < 0:
                        dist_squared = self.calculate_2D_dist_squared(polygons_vertex_coords[pi_focus, vi_focus], polygons_vertex_coords[pi, vi])
                        result[pi_focus, vi_focus, pi, vi] = dist_squared
                        result[pi, vi, pi_focus, vi_focus] = dist_squared

    return result

What are some things I could think about in order to increase the performance of my code?

For the time being, I got a significant improvement in performance by only re-calculating updates to the dist_squared_matrix, rather than always re-calculating the dist_squared_matrix entirely every step.

It would be helpful to have a simple test harness for the code.

In reviewing your code I can see some possible bottlenecks, in Cython there can be quite a large overhead in passing arrays (views) to functions, also the array operations may be convenient but when performing them on very small arrays of only a few values performance will tend to suffer considerably compared with doing the arithmetic directly — Cython can make some optimizations when it comes to calling functions into python-space and a few numpy array operations bypass python-space — but arithmetic on basic types in Cython is guaranteed to be at the speed of c as it translates directly to c code, as does array indexing.

Hence you would probably gain some performance benefits by passing floats directly instead of arrays.

cdef double calculate_2D_dist_squared(self, np.float64_t x1, np.float64_t y1,
                                      np.float64_t x2, np.float64_t y2):
    return (x1 - x2) ** 2 + (y1 - y2) ** 2

As noted previously, indexing is very very fast in Cython, but intermediate views can result in performance losses, hence my instinct would be to call the function in this way:

dist_squared = self.calculate_2D_dist_squared(
    polygons_vertex_coords[pi_focus, vi_focus, 0],
    polygons_vertex_coords[pi_focus, vi_focus, 1],
    polygons_vertex_coords[pi, vi, 0],
    polygons_vertex_coords[pi, vi, 1])

Instead of the other possible way which would involve creating a variable to hold polygons_vertex_coords[pi_focus, vi_focus] (etc).


Number-guessing game with simple GUI This is my second program in Python. I am aware that globals are a bad idea, but that is what I have learned so far in my course.

Right now the program is doing what it's supposed to do, my only concern is that it might be too long for nothing. I'm asking you to help me figure out where I could make it simpler.

import simplegui
import random
import math

#Globals, random guess and player's remaining guesses

num_range = 100
remaining_guesses = 7

# New game, range from 0 - 100 by default
# after the according button is pressed
# Reset the remaining guesses to 7

def range100():
    global num_range
    global remaining_guesses
    remaining_guesses = 7
    print "New game. Range is from 0 to 100"
    print "Number of remaining guesses is %i" %(remaining_guesses)
    print ""
    num_range = random.randrange(0, 101)
    return num_range

# Set game to a range of 0 - 1000
# after the according button is pressed
# Reset the remaining guesses to 10

def range1000():
    global num_range
    global remaining_guesses
    remaining_guesses = 10
    print "New game. Range is from 0 to 1000"
    print "Number of remaining guesses is %i" %(remaining_guesses)
    print ""
    num_range = random.randrange(0, 1001)
    return num_range

# Compare guess to random range
# Remove 1 to remaining guesses
# Display hint on the random range
# Restart the game if player is correct or out of guesses

def input_guess(guess):
    guess = float(guess)
    global remaining_guesses
    global num_range
    remaining_guesses -= 1
    print "Your guess was: %i" %(guess)
    print "Number of remaining guesses is %i" %(remaining_guesses)
    if (remaining_guesses == 0 and guess != num_range):
        print "The correct answer was %i" %(num_range)
        print "Let's try again!"
        print ""
        print ""
        range100()
    elif (remaining_guesses == 0 and guess == num_range):
        print "Correct!"
        print ""
        print ""
        range100()
    elif guess > num_range:
        print "Try a lower number!"
    elif guess < num_range:
        print "Try a higher number!"
    else:
        print "Correct!"
        print ""
        print ""
        range100()
    print ""
    return guess, remaining_guesses

# Create & start frame

f = simplegui.create_frame("Guess the number", 200, 200)

f.add_button("Range is [0, 100]", range100, 200)
f.add_button("Range is [0, 1000]", range1000, 200)
f.add_input('My label', input_guess, 50)
f.start()

# Start a new game

range100()

You have a function called input_guess().  That function inputs the guess, compares the guess, and plays the entire game.  It should only input the guess.  I would create a function to run the program, one to input the number, and one to do the comparison and output prompts about being too large or small.  This way, only the function that plays the game needs to know the number of moves left.  Once the number is correctly guessed, that function just returns to its caller, who can call it again if the user wants to play again.  Also, try not to write functions this long.

As for your range100() and range1000() functions, I would combine those into one function like this:

def get_num(min, max):
    print "New game. Range is from " + min + " to " + max
    num = random.randrange(min, max + 1)
    return num

You can tell the user how many guesses they have left in the function that controls the game.

Finally, you should have the part of the program that always runs in an if __name__ == "__main__": block.  This is for re-usability of your program, and you can read more about it here: What does if __name__ == “__main__”: do?.


Linux wc command in C I'm working on my implementaion of the linux wc command. I finally have something that is working properly (with no options yet) but i think it needs a lot of "cleaning". I mean, i highly disrespect the "do not repeat yourself rule" and i want to hear some ideas on how can i polish this up a bit.

Here 's the code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <getopt.h>
#define BUF_SIZE 1024

typedef struct Total{ //stores the Total count of lines words and bytes
  int lines;
  int words;
  int bytes;
} Total;

Total* new_Total(int lines, int words, int bytes){ //constructor function for Total
  Total* t = (Total*)malloc(sizeof(Total));
  t->lines = lines;
  t->words = words;
  t->bytes = bytes;
  return t;
}

void updateTotal(Total *t, int lines, int words, int bytes){ //updates Total counters
  t->lines += lines;
  t->words += words;
  t->bytes += bytes;
}

void readStdin(Total* t, char c){ //reads from stdin, counts and prints
  int lines = 0, words = 0, bytes = 0, startWord = 0;
  char ch;
  while((ch=fgetc(stdin)) != EOF){//count the bytes, lines and words
    bytes++;
    if (ch == '\n'){
      lines++;
    }
    if (ch != ' ' && ch != '\n' && ch != '\r' && ch != '\t'){ //if there's a visible char, there's a word
      startWord = 1;
    }
    if ((ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t') && startWord == 1){ //when the word ends
      words++; //increment the word coutner
      startWord = 0; //reset word finder
    }
  }
  updateTotal(t, lines, words, bytes);
  if (c == '\0'){ //if the command was called with no arguments
    printf ("%d %d %d\n", lines, words, bytes);
  }
  else{
    printf ("%d %d %d %c\n", lines, words, bytes, c);
  }
}

void readFile(Total *t, char* filename, FILE* fp){
  int lines = 0, words = 0, bytes = 0, startWord = 0;
  char ch;
  if (fp == NULL){
    printf("%s: No such file or drectory\n", filename);
    exit(1);
  }
  while ((ch=fgetc(fp)) != EOF){ //count the bytes, lines and words
    bytes++;
    if (ch == '\n'){
      lines++;
    }
    if (ch != ' ' && ch != '\n' && ch != '\r' && ch != '\t'){
      startWord = 1;
    }
    if ((ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t') && startWord == 1){
      words++;
      startWord = 0;
    }
  }
  updateTotal(t, lines, words, bytes);
  printf("%d %d %d %s\n", lines, words, bytes, filename);
}

void readArgs(Total* t, int argc, char* argv[]){
  FILE* fp;
  for (int i=1; i<argc; i++){
    if (*argv[i] == '-'){//if a '-' is found, read from stdin
      readStdin(t, '-');
      clearerr(stdin);
    }
    else { //tries to read the file in argv[i]
      fp = fopen(argv[i], "r");
      readFile(t, argv[i], fp);
      fclose(fp);
    }
  }
  if (argc > 2){ //if there's more than one file, print the total in the end
    printf("%d %d %d total\n", t->lines, t->words, t->bytes);
  }
  else {
    exit(0);
  }
}

int main(int argc, char* argv[]){
  Total* t = new_Total(0,0,0);
  if (argc<2){ //no arguments
    readStdin(t,'\0'); //pass /0 to readstin because as it is the only time it is called, there's no need to append the name of the argument
    return 0;
  }
  readArgs(t, argc, argv);
  free(t);
  return 0;
}

As you can see, both in readStdin() and in readFile() i have the same piece of code that handles the counting. I've tried to make a function out of this using pointers but i gave up. Any suggestion would be much appreciated. Also, if you have any hint that has nothing to do with the problem that i mentioned, shoot it up!

A standard way to do this is to break the function up into smaller 'private' (static) functions, each with one thing to do. One can pass the default to the counting function in case we are not provided the argument. This allows separation of the code which counts and the UI.

Total is always going to be called with (0, 0, 0), it's unnecessary to provide these arguments. In fact, the storage of Total in dynamic memory is probably unnecessary. No need to updateTotal, use of the counters directly is fine. In fact, there is going to be 1 total, might as well make it static.

fgetc returns an int, char ch is not enough to tell EOF, which is -1. You've defined BUF_SIZE, an excellent optimization, but never use it. I'd go from fgetc to fgets. Note that this assumes that the file is text and doesn't contain a '\0' inside, (eg, modified UTF-8.)

You've included getopt and unistd, making your code POSIX C99, but you're not using those. Removing them makes your code more general C99 (now your code will compile on more compilers.)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <errno.h>

//stores the Total count of lines words and bytes
struct Total {
    size_t lines, words, bytes;
} total;

//count the bytes, lines and words
//gives incorrect results on binary files with embedded 0
//a word is delimited by everything that {isgraph}
//using the unix definition of a text file, {lines = count('\n')}
static int readFile(FILE *fp) {
    char buffer[1024], b;
    int startWord = 1;
    size_t i;
    while(fgets(buffer, (int)sizeof buffer, fp)) {
        i = 0;
        do {
            b = buffer[i++];
            if(b == '\n') {
                /* This will not recognize '\l' in old Mac text files opened
                 with "wb"? Hmm. */
                total.lines++;
                startWord = 1;
            } else if(b == '\0') {
                break;
            } else if(isgraph(b)) {
                //if there's a visible char, there's a word
                if(startWord) total.words++, startWord = 0;
            } else {
                startWord = 1;
            }
        } while(1);
        total.bytes += i;
    }
    return ferror(fp) ? 0 : 1;
}

int main(int argc, char *argv[]) {
    FILE *fp = 0;
    char *fn = "<no file>";
    for(int i = 1; i < argc; i++) {
        fn = argv[i];
        fp = fopen(fn, "r");
        if(!fp || !readFile(fp)) break;
        fclose(fp), fp = 0;
    }
    if(argc <= 1) readFile(stdin);
    if(fp) fclose(fp);
    if(errno) return perror(fn), EXIT_FAILURE;
    /* https://stackoverflow.com/a/2524675/2472827 */
    printf("%zu lines; %zu words; %zu bytes total.\n",
        total.lines, total.words, total.bytes);
    return EXIT_SUCCESS;
}

Some see typedef struct as problematic, and, in my opinion, it's not needed here; see Linux kernel coding style. INT_MAX could be as little as 32,767; it's never going to be less than zero, so I'd prefer unsigned, or size_t in this context. What if there are other chars besides '\n', ' ', '\r', '\t'? I've included <ctype.h> to get rid of the ambiguity. There was a bug counting the word at the end of the line. Now, a line is defined as the number of '\n' (UNIX style.) A word is delimited by isgraph.