Simple foreach adding to a list I am having a simple foreach where I add objects to a list:

    List<SoonestDrawDateModel> soonestDrawDateModel = new List<SoonestDrawDateModel>();

    foreach (var item in drawDates)
    {
        SoonestDrawDateModel sdModel = new SoonestDrawDateModel();
        sdModel.DrawDay = item;
        sdModel.DrawDayId = item.DayId;
        sdModel.CutOffDayId = item.CutOffDayId;

        soonestDrawDateModel.Add(sdModel);

        sdModel = new SoonestDrawDateModel();
        sdModel.DrawDayId = item.DayId + 7;
        sdModel.CutOffDayId = item.CutOffDayId;
        sdModel.DrawDay = item;
        soonestDrawDateModel.Add(sdModel);
    }

    var realDrawDates = soonestDrawDateModel.OrderBy(x => x.DrawDayId);

Is there any way to do this with Linq with fewer lines?

What you're doing here, essentially, is creating two Model items for each item in your source list, and storing them in a list. Here's a somewhat contrived usage of the SelectMany LINQ function which flattens a hierarchical list:

drawDates
  .Select(dd => new SoonestDrawDateModel[2]  // create a 2-item array for each item.
                 {
                   new SoonestDrawDateModel
                    {
                      DrawDay = item,
                      DrawDayId = item.drawDayId,
                      CutOffDayId = item.CutOffDayId
                    },
                   new SoonestDrawDateModel
                    {
                      DrawDay = item,
                      DrawDayId = item.drawDayId + 7,
                      CutOffDayId = item.CutOffDayId
                    }
                }) // end of Select method.
 .SelectMany(items => items) // flatten collection of arrays into one collection.
 .OrderBy(x => x.DrawDayId); // order by.
 .ToList() //convert to a List<SoonestDrawDateModel>.

What we're doing here is creating a 2-item array or each Draw item, meaning we have an IEnumerable<SoonestDrawDateModel[]>, then use SelectMany to take all the members of each SoonestDrawDateModel[] and flatten them into one big IEnuemrable<SoonestDrawDateModel>, which we then sort and return.

You can simplify the syntax by extracting the main creation block into a method:

private SoonestDrawDateModel[] CreateDateModelPair(DrawItem item)
{
    return new SoonestDrawDateModel[2]
    {
     new SoonestDrawDateModel
     {
        DrawDay = item,
        DrawDayId = item.drawDayId,
        CutOffDayId = item.CutOffDayId
     },
     new SoonestDrawDateModel
     {
        DrawDay = item,
        DrawDayId = item.drawDayId + 7,
        CutOffDayId = item.CutOffDayId
     }
  }
}

then call it with this LINQ call that conveys the intent rather well:

drawDates
  .Select(CreateDateModelPair)
  .SelectMany (dateModels => dateModels)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();

Or, as @anaximander suggests, an even terser (but less explicit) version that combines the Select and SelectMany calls. It saves a step, but it's less clear that we have two things going on - one to convert the DrawDate to two SoonestDrawDateModels, and another to flatten that list of lists.

drawDates
  .SelectMany (CreateDateModelPair)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();


Prices and contracts query The query below is quite slow (taking about half a second). Any obvious things I could improve?

The PRICES table has about 21 million rows and 30 columns while CONTRACTS has 130 k rows and 10 columns.

SELECT
 CONTRACTS.EXT_ID,
 PRICES.REFERENCE_DATE,
 PRICES.PRICE_DATE,
 CONTRACTS.PERIOD,
 PRICES.TIME_TO_MATURITY,
 PRICES.CLOSE_PRICE,
 PRICES.ORIGINAL_PRICE_ID
FROM
 PRICES INNER JOIN CONTRACTS ON PRICES.CONTRACT_ID = CONTRACTS.ID
WHERE
 (((CONTRACTS.EXT_ID)=@CONTRACTID)
 AND ((PRICES.REFERENCE_DATE)=@REFDATE)
 AND ((PRICES.STATUS_TYPE) LIKE 'CAL%'));

if you less care about the "Dirty read" then you can use the "(nolock)" after the table name.
And you already have the indexing.


Finding the intersection of elements in two sets Is there another better way to implement those two methods in 1 and 2?

  Implement the method contains.
  contains returns true if the given element
  is a member of the set, and false otherwise.
  Implement the method intersection.
  intersection returns a set which contains the elements
  in the set that are also members of the given set

  output:

{9, 1, 4, 3, 7, 5}

{4, 2, 8, 5, 7}

{4, 7, 5}

public class IntSet {

private int[] elements = null;

public IntSet(int[] elements) {
    this.elements = new int[elements.length];
    for (int pos = 0; pos < elements.length; pos++) {
        this.elements[pos] = elements[pos];
    }
}

// toString returns a string representation of the set
public String toString() {
    String s = "{";
    if (elements.length == 0)
        s = s + "}";
    else {
        for (int pos = 0; pos < elements.length - 1; pos++)
            s = s + elements[pos] + ", ";
        s = s + elements[elements.length - 1] + "}";
    }

    return s;
}

// contains returns true if the given element
// is a member of the set, and false otherwise.
public boolean contains(int element) {

    for (int i = 0; i < this.elements.length; i++) {
        if (this.elements[i] == element)
            return true;
    }
    return false;
}

// intersection returns a set which contains the elements
// in the set that are also members of the given set
public IntSet intersection(IntSet set) {
    // use the method contains
    int members = 0;
    for (int i = 0; i < elements.length; i++) {
        if (set.contains(elements[i]))
            members++;
    }

    int[] newMembers = new int[members];
    int position = 0;
    for (int i = 0; i < elements.length; i++) {
        if (set.contains(elements[i]))
            newMembers[position++] = this.elements[i];

    }
    return new IntSet(newMembers);
}

//Some instances of the class IntSet are created and used like this:
public static void main(String[] args) {
    int[] numbers1 = {9, 1, 4, 3, 7, 5};
    IntSet set1 = new IntSet(numbers1);
    int[] numbers2 = {4, 2, 8, 5, 7};
    IntSet set2 = new IntSet(numbers2);
    System.out.println(set1);
    System.out.println(set2);

    IntSet set = set1.intersection(set2);
    System.out.println(set);
    //When this code fragment is executed, the following printout is produced:
    //{9, 1, 4, 3, 7, 5}
    //{4, 2, 8, 5, 7}
    //{4, 7, 5}
  }
}

Is there another better way to implement those two methods in 1 and 2?

Of course there is.

My advice is to think about the problem as though you were solving the problem without a computer.

Here, I'm going to give you a list of a thousand unsorted integers between 0 and 999999.

Now I'm going to ask you one thousand times whether a particular number I've chosen at random is on the list.

Would you really try to answer every question by reading down the list from top to bottom every time?

Give some thought as to how you might solve that problem before you read on.

.

.

.

.

.

No, really, think about it.

.

.

.

You probably thought something like

the first time I go down the list, I could make ten smaller lists:  numbers I've seen from 0-99999, numbers I've seen from 100000-199999, from 200000-299999, and so on.  That will take a thousand steps to make ten buckets of roughly 100 items each.
Then the second time I'm asked the question, I can first figure out what bucket the number might be in. Then I can search that bucket, and I know I will only have around 100 things to check instead of 1000.
And hey, while I'm doing that, I might as well divide that bucket up into ten buckets of roughly ten items each!

Every time you do a search that makes ten more buckets, you make subsequent searches that hit that bucket ten times faster.

All right, that should get you started; can you implement something like this bucketing algorithm in code?

Now, think the same way about intersection. Can you use the same bucketing trick to make intersection vastly faster? Or something similar?  Again, think about how you would solve the problem if I asked you to do it by hand.


Simple CodeReview Questions Notifications for OSX Some days ago, I figured out that I'd like to get a notification every time a new question is asked here. So, I've made a really simple Python script, which uses pync (only OSX compatible) to do that for me.

I'd like to get some feedback regarding anything that comes to your mind, including good practices. That's not a complicated piece of code, but I'd like some feedback on it anyway.

import stackexchange
import time

from pync import Notifier

def get_notifications():
    """This method uses the SE API and pops up a notification everytime a new question is asked on CR"""
    cr = stackexchange.Site(stackexchange.CodeReview)

    old = []
    while True:
        questions = cr.recent_questions(filter='_b')
        for question in questions[:1]:
            if question.title not in old:
                old.append(question.title)
                Notifier.notify('Votes: {0} | Answers: {1} | Views: {2}'.format(
                    question.score, len(question.answers), question.view_count),
                    title='CodeReview: {0}'.format(question.title),
                    subtitle='Tags: {0}'.format(', '.join(question.tags)),
                    sound='default',
                    open=question.url)
            else:
                continue

        time.sleep(2)

if __name__ == '__main__':
    get_notifications()

Nitpicks

Maximum line length should be 72 characters for docstrings.
For your old variable, you may wan to use a set instead of a list to check for existence (not in) as it will be \$O(1)\$ instead of \$O(n)\$.

Reusability

As it stand, I can't use this script because it is too much tied to your OSX only notifier. I'd rather yield questions from get_notifications (which will not be greatly named after that) and format them elsewhere, letting other users handle it with other notifications systems (or even email, for that matter).

Something like:

import stackexchange
import time

def pync_notify(question):
    from pync import Notifier
    Notifier.notify('Votes: {0} | Answers: {1} | Views: {2}'.format(
        question.score, len(question.answers), question.view_count),
        title='CodeReview: {0}'.format(question.title),
        subtitle='Tags: {0}'.format(', '.join(question.tags)),
        sound='default', open=question.url)

def gtk_notify(question):
    from gi.repository import Notify
    Notify.init('stackexchange')
    notification = Notify.Notification.new(
        'CodeReview: {}'.format(question.title),
        'Votes: {} | Answers: {} | Views: {}'.format(
            question.score, len(question.answers), question.view_count),
        'dialog-information')
    notification.show()

def get_questions():
    """Fetch recent questions from CodeReview

    This function uses the SE API and yield each
    new question that is asked on CR.
    """

    cr = stackexchange.Site(stackexchange.CodeReview)

    old = {}
    while True:
        questions = cr.recent_questions(filter='_b')
        for question in questions[:1]:
            if question.title not in old:
                old.add(question.title)
                yield question

        time.sleep(2)

if __name__ == '__main__':
    for question in get_questions():
         pync_notify(question)

Should be enough. Note that I import modules each time a new question appears, which is not ideal, but at least it let you easily deal with modules that are not present on the system. And the few extra time spent realizing the module is already loaded is negligible compared to your time.sleep(2) anyway. You may want to use classes with a __call__ method if you really want to import and initialize only once.

On the reusability side, it could also be good to be able to parametrize the generator with the site the user whish to fetch:

def get_questions(site=stackexchange.CodeReview):
    se = stackexchange.Site(site)
    …

Better usage of the stackexchange API

The stackexchange module let you directly query information using "site classes" (such as stackexchange.CodeReview) without necessarily relying on a stackexchange.Site object. So:

CR = stackexchange.CodeReview()
CR.questions()

will be a direct call to the SE API on questions.

From there, two things worth noting:

The sort method;
The fromdate parameter.

Sorting results

The advantage of using sort='creation' in the query parameters is that you only need to store the last asked question in your old variable. You can then iterate over the result of the query and stop when q.title == old.title without storing previous questions anymore.

Asking for questions asked after a given date

The fromdate parameter takes a timestamp and make the query returns only questions asked after that point in time. There is no real need for sorting anymore, all you need to remember is the date at when the last query was made.

Proposed improvements

import stackexchange
import datetime
import time

def pync_notify(question):
    from pync import Notifier
    Notifier.notify('Votes: {0} | Answers: {1} | Views: {2}'.format(
        question.score, len(question.answers), question.view_count),
        title='CodeReview: {0}'.format(question.title),
        subtitle='Tags: {0}'.format(', '.join(question.tags)),
        sound='default', open=question.url)

def gtk_notify(question):
    from gi.repository import Notify
    Notify.init('stackexchange')
    notification = Notify.Notification.new(
        'CodeReview: {}'.format(question.title),
        'Votes: {} | Answers: {} | Views: {}'.format(
            question.score, len(question.answers), question.view_count),
        'dialog-information')
    notification.show()

def now():
    """Return the current timestamp"""
    date = datetime.datetime.now()
    return int(date.timestamp())

def get_questions(site=stackexchange.CodeReview, query_delay=2):
    """Fetch recent questions from the Stack Exchange network

    This function uses the SE API and yield each
    new question that is asked on the given site.
    """

    se = site()
    last_query = now()

    while True:
        fromdate, last_query = last_query, now()
        questions = se.questions(fromdate=fromdate, sort='creation')
        yield from questions
        time.sleep(query_delay)

if __name__ == '__main__':
    for question in get_questions():
         pync_notify(question)

Last minute reading

On the discussion about throttles of the StackExchange API, one can read:

  While not strictly a throttle, the Stack Exchange API employs heavy caching and as such no application should make semantically identical requests more than once a minute. This is generally a waste of bandwidth as, more often than not, the exact same result will be returned.

This means that you probably want to increase your time.sleep delay from 2 to at least 60.


Sum two integers, unless they are the same, in which case return double their sum I was doing a simple problem on codingbat and testing out my python. The problem was:

  Given two int values, return their sum. Unless the two values are the same, then return double their sum.

sum_double(1, 2) → 3
sum_double(3, 2) → 5
sum_double(2, 2) → 8

My solution was:

def sum_double(a, b):
  if a is b:
    return 2 * (a + b)
  else:
    return a+b

and it worked just fine. However, when I checked the solution the answer they had was:

def sum_double(a, b):
  # Store the sum in a local variable
  sum = a + b

  # Double it if a and b are the same
  if a == b:
    sum = sum * 2
  return sum

But this seems to Java-y to me. Am I right to believe that my code is more pythonic? Or was their answer a better way to write code in python?

The existing answers mostly have just said is is wrong here. Which it undoubtedly is.
The reason it's bad to use here is if you pass 257 and above, or -6 and below, you may get incorrect output.
The best way to show how it's bad is to ask you what the output of the following are:

sum_double(2, 2)
sum_double(1 + 1, 2)
sum_double(257, 257)
sum_double(256 + 1, 257)

Simple! Surely it's 8, 8, 1028 and 1028, right?
No, it's actually, 8, 8, 1028 and 514.
This is known and expected behaviour:

  The current implementation keeps an array of integer objects for all integers between -5 and 256,
  when you create an int in that range you actually just get back a reference to the existing object.

Other than that both are fine, and I wouldn't really fret over which is 'better', this is as both are clear and understandable.
Which is all your code needs to be.
A function that would be un-pythonic, as it's not clear could be:

def sum_double(a, b):
    return (a + b) * ((a == b) + 1)


Multiple dependent subqueries This query currently takes approximately 250 seconds to run and I'd like to reduce it by as much as I can. I have tried using left joins instead of subqueries and it increases run time.

SELECT `items`.`id`,

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 7 DAY) group by `itemID`), '0') as `sold7`,
ifnull((SELECT round(`supply`) FROM `supply` WHERE `itemID` = `items`.`id` AND `date` > date_sub(now(), interval 7 DAY) group by `itemID`), '0') as `listed7`,

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 30 DAY) group by `itemID`), '0') as `sold30`,
ifnull((SELECT round(`supply`) FROM `supply` WHERE `itemID` = `items`.`id` AND `date` > date_sub(now(), interval 30 DAY) group by `itemID`), '0') as `listed30`,

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 60 DAY) group by `itemID`), '0') as `sold60`,
ifnull((SELECT round(`supply`) FROM `supply` WHERE `itemID` = `items`.`id` AND `date` > date_sub(now(), interval 60 DAY) group by `itemID`), '0') as `listed60`,

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 90 DAY) group by `itemID`), '0') as `sold90`,
ifnull((SELECT round(`supply`) FROM `supply` WHERE `itemID` = `items`.`id` AND `date` > date_sub(now(), interval 90 DAY) group by `itemID`), '0') as `listed90`,

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 180 DAY) group by `itemID`), '0') as `sold180`,
ifnull((SELECT round(`supply`) FROM `supply` WHERE `itemID` = `items`.`id` AND `date` > date_sub(now(), interval 180 DAY) group by `itemID`), '0') as `listed180`

FROM `items`

group by `items`.`id`

Formatting

First, the way you wrote this makes it difficult to understand what you are trying to do. Here are a few points to improve it:

Use indentation and line breaks that illustrates your nesting. I provided an example below.
Use all caps for keywords (or all lower case, just be consistent).
Back ticks are not required unless you have a space or reserved operator in your code. It makes the code easier to follow without them.

Here is an example, this:

ifnull((SELECT sum(`history`.`quantity`) FROM `history` where `history`.`itemID` = `items`.`id` and `history`.`date` > date_sub(now(), interval 7 DAY) group by `itemID`), '0') as `sold7`,

Could be:

IFNULL(
    (SELECT SUM(history.quantity)
    FROM history
    WHERE history.itemID = items.id
    AND history.date > DATE_SUB(NOW(), INTERVAL 7 DAY)
    GROUP BY itemID
    ), 0
) AS sold7,

Performance

The first problem I see is the excessive use of IFNULL() and subqueries within them. I'm not sure as to why you are replacing NULL with 0 as your aggregate functions will factor NULLs out anyways. But I suppose you have a reason to do this that I don't understand.

I have at least three recommendations:

If your dataset is very large, you may want to break it down into smaller steps using a temporary table.
You would likely want to use a LEFT JOIN instead of subqueries.
I would strongly suggest to create a stored procedure as this looks like the type of query that is run regularly. That way the execution plan will saved with your query.

Here you go:

DELIMITER $$
    CREATE OR REPLACE PROCEDURE proc_MyProcedure() AS -- pick a meaningful name
    BEGIN
    INSERT INTO TEMPORARY TABLE temp_MyTable -- pick a meaningful name
     SELECT items.id AS itemID,
      history.quantity AS historyQty,
      history.date AS historyDate,
      supply.supply AS supplyQty,
      supply.date AS supplyDate
     FROM items
     LEFT JOIN supply ON items.id = supply.itemID
     LEFT JOIN history ON items.id = history.itemID
     WHERE history.date > DATE_SUB(NOW(), INTERVAL 180 DAY)
     OR supply.date  > DATE_SUB(NOW(), INTERVAL 180 DAY)
    ;
    -- here we set nulls to 0
    UPDATE temp_MyTable
    SET historyQty = 0
    WHERE historyQty IS NULL;
    UPDATE temp_MyTable
    SET supplyQty = 0
    WHERE supplyQty IS NULL;
    -- finally we aggregate on the temp table
    SELECT  (SELECT SUM(historyQty) FROM temp_Mytable WHERE history.date > DATE_SUB(NOW(), INTERVAL 7 DAY)) AS sold7,
     (SELECT ROUND(supply) FROM temp_MyTable WHERE supplyDate > DATE_SUB(NOW(), INTERVAL 7 DAY)) AS listed7,
    -- etc.
    FROM temp_MyTable
    GROUP BY itemID;
    END$$
DELIMITER ;

Then just CALL proc_MyProcedure(). You will notice the first time takes longer, but any subsequent calls should be much faster.


Cracking Caesar Cipher I'm new to Haskell, so I wrote a program to crack Caesar Ciphered text for practice. The program calculates the frequency of each letter in the input, and then checks to see which shift minimizes the Euclidean norm of the element-wise difference between the text's frequencies and pre-measured frequencies in the English language. The program prints the shift to stdio and writes the decrypted text to {input file name}_DECRYPTED.

While this isn't actually a great (let alone mediocre) decryption method, I'd appreciate any input about how to improve my code to make it more "Haskell-like" or idiomatic. Syntax and readability suggestions are also more than welcome.

Thank you!

import System.Environment (getArgs)
import System.IO
import Data.List (elemIndex)
import Data.Char (ord, chr, isUpper, isAlpha, toLower)
import Data.Bool (bool)
import Data.Map (fromListWith, toList)

-- English letter frequencies from A to Z
frequencies :: (Fractional a) => [a]
frequencies = [0.0812,
               0.0149,
               0.0271,
               0.0432,
               0.1202,
               0.023,
               0.0203,
               0.0592,
               0.0731,
               0.001,
               0.0069,
               0.0398,
               0.0261,
               0.0695,
               0.0768,
               0.0182,
               0.0011,
               0.0602,
               0.0628,
               0.091,
               0.0288,
               0.0111,
               0.0209,
               0.0017,
               0.0211,
               0.0007]

main :: IO()
main = do
  (filename: _) <- getArgs
  handle <- openFile filename ReadMode
  encrypted <- hGetContents handle
  let shift_size = decrypt (map toLower $ filter isAlpha encrypted) in
    do
    print shift_size
    writeFile (filename ++ "_DECRYPTED") (shift (negate shift_size) encrypted)
  hClose handle

shift :: Int -> String -> String
shift s input = map (shiftLetter s) input

shiftLetter :: Int -> Char -> Char
shiftLetter offset c
  | isAlpha c =
    let i = ord $ bool 'a' 'A' (isUpper c)
    in chr $ i + mod ((ord c - i) + offset) 26
  | otherwise = c

decrypt :: String -> Int
decrypt text = let len = fromIntegral $ length text
                   (_, freq) = unzip $ toList $ fromListWith (+) [(c, 1.0) | c <- text]
                   in minShift frequencies $ map (flip (/) len) freq

minShift :: (Fractional a) => (Ord a) => [a] -> [a] -> Int
minShift freq1 freq2 = let norms = map (\x -> norm freq1 (shiftList freq2 x)) [0..25]
                           (Just num) = elemIndex (minimum norms) norms in num

shiftList :: (Fractional a) => [a] -> Int -> [a]
shiftList freq x = iterate oneCycle freq !! x

oneCycle :: (Fractional a) => [a] -> [a]
oneCycle (x:xs) = xs ++ [x]

norm :: (Fractional a) => [a] -> [a] -> a
norm xs ys = foldl (\acc -> \(x, y) -> acc + (x - y)^2) 0 $ zip xs ys

Let's start with the trivial stuff...

A few minor stylistic points that are pretty much universal:

The type IO () is always written with a space between IO and (), never IO().
Similarly, multiple constraints are written as a comma-separated list:

(Fractional a, Ord a) => ...

rather than a chained list (Fractional a) => (Ord a) => ...
For do-blocks, even though the syntax permits:

do
firstLine
secondLine

they are almost never written this way.  It's either:

do firstLine
   secondLine

or else:

do
  firstLine -- w/ your standard choice of indentation, usually 2 or 4
  secondLine

It's often considered acceptable to write:

someother expression $ do
  contentsOf
  theDoBlock

so the contents can be unindented or negatively indented with respect to the do keyword in this case.

For the specific do-block in your main function, because a standalone let statement is permitted in do-notation, you don't need the nesting at all, so this would be more standard:

main = do
  ...
  let shift_size = decrypt (map toLower $ filter isAlpha encrypted)
  print shift_size
  writeFile (filename ++ "_DECRYPTED") (shift (negate shift_size) encrypted)
  hClose handle

(This is despite the fact that shift_size is only needed in the following two lines and not the last.)

I find that idiomatic Haskell tends to use where in preference to let ... in ... statements, so for example:

decrypt :: String -> Int
decrypt text = minShift frequencies $ map (flip (/) len) freq
  where len = fromIntegral $ length text
        (_, freq) = unzip $ toList $ fromListWith (+) [(c, 1.0) | c <- text]

The motivation here is that the definition of decrypt text is given immediately following the =, and if the helper functions like len and freq have sufficiently self-evident names, the reader can mostly ignore the where ... details.

More controversially, some people like to write the most general type signatures possible.  In my opinion, unless you're writing a library or actually need the generality, I don't think there's much point.  The polymorphic type signatures and Fractional constraints clutter up your code, and if you turned on -Wall, which you should be doing anyway, you'd see that GHC is defaulting your type to Double, a default that would be better to make explicit anyway.  Personally, I'd replace most of the Fractional a => ... a ... with Doubles.  (Well, except oneCycle and shiftList, which are probably clearer with unconstrained type signatures.)

Now to the less trivial stuff...

In minShift, consider the definition of norms:

norms = map (\x -> norm freq1 (shiftList freq2 x)) [0..25]

This calculates shiftList freq2 x for every x from 0 to 25, but shiftList works by generating the full list iterate oneCycle freq and then selecting element x, so you would have been better off writing:

norms = map (norm freq1) (take 26 $ iterate oneCycle freq2)

Actually, a more common way of calculating all "cycles" of a list is:

cycles :: [a] -> [[a]]
cycles xs = zipWith (++) (tails xs) (inits xs)

which many Haskellers take great pride in writing using implicit reader monad/applicative:

cycles :: [a] -> [[a]]
cycles = zipWith (++) <$> tails <*> inits

Also, finding the minimum with minimum and then getting its index with elemIndex would be frowned upon because it traverses the list twice (or, on average, one and a half times in the absence of duplicate minimums), and even though it's ridiculous to worry about performance on a 26-item list, I guess folks would be more likely to use a trick like:

minimumIndex :: (Ord a) => [a] -> Int
minimumIndex xs = snd . minimum $ zip xs [0..]

Note that I'm breaking my own rule here about not overgeneralizing functions.  In this case, it just "feels" right.  Anyway, the way this works is by using zip to add an index, so that the list xs = [5,4,6,1,8] becomes:

[(5,0),(4,1),(6,2),(1,3),(8,4)]

Because tuples are sorted lexicographically, finding the minimum will pick up the element (1,3), and we use snd to grab the index "3".

So, now minShift looks like this:

minShift :: [Double] -> [Double] -> Int
minShift freq1 freq2 = minimumIndex $ map (norm freq1) (cycles freq2)

with helpers minimumIndex and cycles as above.

For the top-level norm function, your foldl is really a sum, and you can use zipWith to combine the zip with the calculation of the term:

norm :: [Double] -> [Double] -> Double
norm xs ys = sum $ zipWith (\x y -> (x-y)^2) xs ys

With -Wall on, this warns you that 2 is defaulting to Integer.  I'd probably write:

norm :: [Double] -> [Double] -> Double
norm xs ys = sum $ zipWith (\x y -> (x-y)*(x-y)) xs ys

just to get rid of this warning.

In decrypt, the flip can be replaced with a section:

decrypt text = minShift frequencies $ map (/ len) freq

However, there's a bug in your freq calculation.  The map it builds will only have keys for the letters that actually appear in the input text, so the freq and frequencies lists won't generally line up.  Anyway, I'd pull it out into a separate function:

{-# LANGUAGE TupleSections #-}
import qualified Data.Map.Strict as Map

freq :: String -> [Int]
freq inp
  = Map.elems $ Map.unionWith (+) initMap . Map.fromListWith (+) . map (,1) $ inp
    where initMap = Map.fromList . map (,0) $ ['a'..'z']

This uses Map.unionWith and an all-zeros map initMap to ensure the keys 'a' through 'z' will be in the map.  It also uses Map.elems in place of let (_, freq) = unzip $ Map.toList $ ....

Finally, note that I've used Data.Map.Strict.  This is good practice for "counting" maps, so that large inputs don't cause a memory leak.

My decrypt now looks like:

decrypt :: String -> Int
decrypt text = minShift frequencies $ map (/ len) $ map fromIntegral $ freq text
  where len = fromIntegral $ length text

Also, shiftLetter would probably be clearer to write with separate cases and a helper in place of bool.

shiftLetter :: Int -> Char -> Char
shiftLetter offset c
  | isAsciiLower c = go 'a'
  | isAsciiUpper c = go 'A'
  | otherwise      = c
  where go a = chr $ (ord c - ord a + offset) `mod` 26 + ord a

Note that the isAscii... versions are safer than isLower and isUpper because these allow unicode letters.

For shift, some typical simplifications are possible.  So:

shift s input = map (shiftLetter s) input

can be rewritten (using "eta reduction") as:

shift s = map (shiftLetter s)

Some people might go farther and write:

shift = map . shiftLetter

though this isn't particularlyclear.  Maybe this would be a nice compromise:

shift :: Int -> String -> String
shift offset = map shift1
  where
    shift1 c
      | isAsciiLower c = go 'a' c
      | isAsciiUpper c = go 'A' c
      | otherwise      = c
    go a c = chr $ (ord c - ord a + offset) `mod` 26 + ord a

allowing us to eliminate shiftLetter completely.

In main, for quick-and-dirty argument parsing, you can write:

[filename] <- getArgs

This has the advantage over filename:_ <- getArgs of raising an exception if more than one argument is supplied.

The openFile / hClose pairs is more properly written using a withFile clause.  But, if you're opening a file just to read its contents, it's better to use readFile anyway.

encrypted <- readFile filename

so my final main looks like:

main :: IO ()
main = do
  [filename] <- getArgs
  encrypted <- readFile filename
  let shift_size = decrypt (map toLower $ filter isAlpha encrypted)
  print shift_size
  writeFile (filename ++ "_DECRYPTED") (shift (negate shift_size) encrypted)

The final thing that bothers me is that freq has to pass through the String once to calculate the counts, and then decrypt passes through it again to count the full text length.  I'd like to do it in one pass, so I'd rewrite freq to calculate the full text length, too, and return the fractional frequencies directly.  This also allows us to pull the filtering into freq which is safer, since the above version of freq will break if it gets fed input that isn't restricted to the characters from 'a' to 'z'.

freq :: String -> [Double]
freq str = let (tot', mp') = foldl' step (0::Int, initMap) . getLower $ str
           in  divlist (Map.elems mp') tot'
  where
    -- get ASCII letters, converted to lowercase
    getLower = filter isAsciiLower . map toLower
    -- initial map of all-zero counts for 'a' to 'z'
    initMap = Map.fromList . map (,0::Int) $ ['a'..'z']
    -- for each `c`, add one to `tot` and count a `c`
    step (tot, mp) c = (tot+1, Map.insertWith (+) c 1 mp)
    -- divide each element of xs by n
    divlist xs n = map (/ fromIntegral n) (map fromIntegral xs)

This works with the following versions of main and decrypt:

main :: IO ()
main = do
  [filename] <- getArgs
  encrypted <- readFile filename
  let shift_size = decrypt encrypted
  print shift_size
  writeFile (filename ++ "_DECRYPTED") (shift (-shift_size) encrypted)

decrypt :: String -> Int
decrypt text = minShift frequencies (freq text)
  where len = fromIntegral $ length text

Note that negate can be written - as long as you stick in some parentheses.  Some people hate this because this - is Haskell's only unary operator and looks weird, so they might stick with negate anyway.

Finally, I think I'd rearrange minShift a bit to make things easier to test.  Also, Data.Char and some others (Data.List and Data.Foldable) are commonly imported in full without explicit import lists, and Data.Map.Strict is commonly imported qualified without an explicit import list, so I'd probably write my imports as:

import System.Environment (getArgs)
import Data.List
import Data.Char
import qualified Data.Map.Strict as Map

This gives the final program:

{-# LANGUAGE TupleSections #-}
{-# OPTIONS_GHC -Wall #-}

import System.Environment (getArgs)
import Data.List
import Data.Char
import qualified Data.Map.Strict as Map

main :: IO ()
main = do
  [filename] <- getArgs
  encrypted <- readFile filename
  let shift_size = decrypt encrypted
  print shift_size
  writeFile (filename ++ "_DECRYPTED") (shift (-shift_size) encrypted)

decrypt :: String -> Int
decrypt text = minimumIndex $ norms english (freq text)

-- English letter frequencies from A to Z
english :: [Double]
english = [0.0812, 0.0149, 0.0271, 0.0432, 0.1202, 0.023, 0.0203, 0.0592, 0.0731, 0.001,
           0.0069, 0.0398, 0.0261, 0.0695, 0.0768, 0.0182, 0.0011, 0.0602, 0.0628, 0.091,
           0.0288, 0.0111, 0.0209, 0.0017, 0.0211, 0.0007]

norms :: [Double] -> [Double] -> [Double]
norms freq1 freq2 =  map (norm freq1) (cycles freq2)

norm :: (Fractional a) => [a] -> [a] -> a
norm xs ys = sum $ zipWith (\x y -> (x-y)*(x-y)) xs ys

freq :: String -> [Double]
freq str = let (tot', mp') = foldl' step (0::Int, initMap) . getLower $ str
           in  divlist (Map.elems mp') tot'
  where
    -- get ASCII letters, converted to lowercase
    getLower = filter isAsciiLower . map toLower
    -- initial map of all-zero counts for 'a' to 'z'
    initMap = Map.fromList . map (,0::Int) $ ['a'..'z']
    -- for each `c`, add one to `tot` and count a `c`
    step (tot, mp) c = (tot+1, Map.insertWith (+) c 1 mp)
    -- divide each element of xs by n
    divlist xs n = map (/ fromIntegral n) (map fromIntegral xs)

shift :: Int -> String -> String
shift offset = map shift1
  where
    shift1 c
      | isAsciiLower c = go 'a' c
      | isAsciiUpper c = go 'A' c
      | otherwise      = c
    go a c = chr $ (ord c - ord a + offset) `mod` 26 + ord a

minimumIndex :: (Ord a) => [a] -> Int
minimumIndex xs = snd . minimum $ zip xs [0..]

cycles :: [a] -> [[a]]
cycles = zipWith (++) <$> tails <*> inits

If I run hlint on this, I get one suggestion:

Caesar2.hs:43:20: Suggestion: Use map once
Found:
  map (/ fromIntegral n) (map fromIntegral xs)
Perhaps:
  map ((/ fromIntegral n) . fromIntegral) xs

In this case, I think the way I have it is clearer.

In this form, it's pretty easy to test:

> freq "It's pretty easy to test"
[5.263157894736842e-2,0.0,0.0,0.0,...]
> norms english (freq "It's pretty easy to test")
[8.694623493074792e-2,0.14473570861495846,0.17156728756232686,...]
> minimumIndex $ norms english (freq "It's pretty easy to test")
0
> decrypt "huk hjabhssf dvyrz xbpal dlss lclu vu zovya aleaz."
7
> shift (-7) "huk hjabhssf dvyrz xbpal dlss lclu vu zovya aleaz."
"and actually works quite well even on short texts."


Code Reiview for an PHP PDO Queries? is there a better way to do it? so this is my code for a equipping an item in my game dev't:

try {
        $db = getConnection();

        $db->beginTransaction();

        $sql_chara_gold = $db->query("SELECT chara_gold FROM chara WHERE chara_id = $shop->chara_id");
        $get_chara_gold = $sql_chara_gold->fetchColumn();

        $sql_shop_price = $db->query("SELECT item_price FROM shop WHERE item_id = $shop->item_id");
        $get_shop_price = $sql_shop_price->fetchColumn();

        if($get_chara_gold >= $get_shop_price){
            $sql_put_item = "INSERT INTO bag(chara_id, item_id, item_qty)
            VALUES(:id,:item_id,1)
            ON DUPLICATE KEY
            UPDATE item_qty = item_qty +1";

            $sql_set_gold = "UPDATE chara ch
            INNER JOIN item it
            ON it.item_id = :item_id
            SET ch.chara_gold = ch.chara_gold - it.item_price
            WHERE chara_id = :id";

            $sql_set_qty =  "UPDATE shop
            SET item_qty = item_qty - 1
            WHERE item_id = :item_id";

            $stmt = $db->prepare($sql_put_item);
            $stmt->bindParam("id", $shop->chara_id);
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $stmt = $db->prepare($sql_set_gold);
            $stmt->bindParam("id", $shop->chara_id);
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $stmt = $db->prepare($sql_set_qty);
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $db->commit();

            $db = null;

            echo json_encode($shop);
        }else{
            echo '{"error":{"text":'.json_encode($get_shop_price).'}}';
        }

I don't know if I'd use ORM for something so simple, but it may be something to look into. At the very least, you should get in the practice of escaping query parameters to avoid SQL injection. I often use Aura SQL for this, because it's dead simple: https://github.com/auraphp/Aura.Sql

An example query using Aura would look like this:

$user = $db->fetchOne('SELECT chara_gold FROM chara WHERE chara_id = :id', [
    'id' => $shop->chara_id
]);

Aura will automatically escape any bound parameters, like :id, so you don't have to worry about SQL injection. If you don't use Aura, at least use PDO::quote.

Funds transfer is the textbook example of why you need transactions to meet the ACID requirements, so it's good that you're using one, but I don't see your rollback. I assume this is in your catch clause which wasn't included.


Image batch processing in Ruby I know a lot about Java, PHP and stuff, but I'm fairly new to ruby.

I've written this script to look into the current working directory, get two parameters from the command line and then resize and rename all jpegs it can find.

To all rubyists, what could I have done better to make things ruby-ish?

require "rmagick"
require "readline"

CWD = Dir.pwd
MAX_DIM = ARGV[0]
PREFIX = ARGV[1]

puts "cwd: #{CWD}"
puts "max dimension: #{MAX_DIM}"
puts "prefix: #{PREFIX}"

read = Readline.readline "continue Y/n: "

exit if read.start_with? "n"

i = 1
Dir.foreach(CWD) do |file|
    next if file == "." or file == ".."
    next if !file.end_with? "jpg" and !file.end_with? "JPG"

    img = Magick::Image.read(file).first
    resized = img.resize_to_fit MAX_DIM
    newpath = CWD + "/" + PREFIX.sub("$", i.to_s) + ".jpg"

    resized.write(newpath) do
        self.quality = 80
    end

    File.delete file

    i = i + 1
    img.destroy!
    resized.destroy!
end

I would replace

i = 1
Dir.foreach(CWD) do |file|
    next if file == "." or file == ".."
    next if !file.end_with? "jpg" and !file.end_with? "JPG"

with

Dir[File.join(CWD, '*.png')].each_with_index do |file, i|

or

Dir.glob(File.join(CWD, '*.png')).each_with_index do |file, i|

I prefer DIR[] over Dir.glob, but I think this is a question on your private coding style.
File.join(CWD, '*.png')] creates a path. Advantage: the system may decide, it if use / or \ (or whatever the OS needs).

Another change I would do:
Use the string format options instead of replacing $.
If you want to use the $ in the command line, then you can replace the $ with a format string.
I use in my example below $02i - this means, replace the string with the given integer value, use at least 2 digits and add leading zeros.
This may be a help if you have more then 10 images.

Some more changes I would do:

I wouldn't use constants to fill it with parameters from the command line.
Either use global variables or normal variables.
Instead of Dir.pwd, I would use '.'
I would separate the real action and the user interface.
You have no checks, if the parameters are empty. In my later example I define defaults.
I would't check for n, but for a Yes.

My complete Code:

require "rmagick"
require "readline"

def convert_images(prefix, max_dim, path = Dir.pwd)
  #Use a glob instead of checking the pathes later,
  #Use each_with_index (Attention, i will start with 0!)
  Dir.glob(File.join(path, '*.jpg')).each_with_index do |file, i|
      img = Magick::Image.read(file).first
      resized = img.resize_to_fit(max_dim)
      #Don't use string operations for pathes
      newpath = File.join(path,prefix % [i+1] + ".jpg") #i starts with 0, so add 1.

      resized.write(newpath) do
          self.quality = 80
      end

      puts "%s -> %s" % [ file, newpath ]
      #~ File.delete file

      img.destroy!
      resized.destroy!
    end
  end

#Define parameters with defaults.
max_dim = ARGV[0] || 100
prefix = ARGV[1]     || 'my_pic_version_%i'
prefix.sub!(/\$/, '%02i') #use string format options.

puts "max dimension: #{max_dim}"
puts "prefix: #{prefix}"

read = Readline.readline "continue Y/n: "
if read =~ /^[Yy]/
  convert_images(prefix, max_dim)
end

Some other ideas:

Use the optparse-gem for interfaces
prefix could contain *.jpg (and the call it target_template or similar).


Viewmodel has lots of properties which bind to view I created a configuration View. so it binds a lot of properties in Viewmodel. But I have a another class People and I want to assign properties of viewmodel to class People.

class Viewmodel
{
   public string name {get;set;}
   public int age {get;set;}
   public string nationality {get;set;}
   public string hobby {get;set;}
   …………

}

class People
{
   public string name {get;set;}
   public int age {get;set;}
   public string nationality {get;set;}
   public string hobby {get;set;}
   public People[] friends {get;set;}
   …………
   public void AddFriend(People people)
   {
    //add friend to this
   }
}

I have to write a function:

void Assign()
{
  People.name = Viewmodel.name;
  People.age= Viewmodel.age;
  …………
}

How could I refactor this function? I usually will define more than 10 properties in viewmodel.

This is a very common problem while working with MVVM. I believe the best solution to resolve this is to implement INotifyPropertyChanged in Models; that way you can directly bind the model properties in view and there will be no need to update them explicitly.

Even MS prescribes doing that -

  Typically, the model implements the facilities that make it easy to
  bind to the view. This usually means it supports property and
  collection changed notification through the INotifyPropertyChanged and
  INotifyCollectionChanged interfaces. Models classes that represent
  collections of objects typically derive from the
  ObservableCollection class, which provides an implementation of the
  INotifyCollectionChanged interface.

Taken from - http://msdn.microsoft.com/en-us/library/gg405484(PandP.40).aspx

Although its up to you to decide whether you want that type of implementation or not; and sometimes its not possible to do this too (e.g. you don't have ownership of model classes).

In case you have doubts about implementing INotifyCollectionChanged in Models, have a look at following SO question -

https://stackoverflow.com/questions/6922130/in-mvvm-model-should-the-model-implement-inotifypropertychanged-interface


Decorator to add key parameter support to an existing sort function Preface

sorted built-in function has key keyword-only parameter. Most of the custom implementations like the following for heapsort algorithm

import heapq
import itertools
from typing import (Iterable,
                    TypeVar)

Domain = TypeVar('Domain')

def heapsort(iterable: Iterable[Domain]) -> Iterable[Domain]:
    heap = []
    for element in iterable:
        heapq.heappush(heap, element)
    for _ in itertools.repeat(None, len(heap)):
        yield heapq.heappop(heap)

has no key parameter support. I thought of a decorator that will add key parameter support in the next fashion

from functools import wraps
from operator import itemgetter
from typing import (Callable,
                    Iterable,
                    Optional,
                    TypeVar)

Domain = TypeVar('Domain')
Sortable = TypeVar('Sortable')

def with_key(plain: Callable[[Iterable[Sortable]], Iterable[Sortable]]
             ) -> Callable[..., Iterable[Domain]]:
    @wraps(plain)
    def implementation(iterable: Iterable[Domain],
                       *,
                       key: Optional[Callable[[Domain], Sortable]] = None
                       ) -> Iterable[Domain]:
        if key is None:
            yield from plain(iterable)
            return
        yield from map(itemgetter(2),
                       plain((key(element), index, element)
                             for index, element in enumerate(iterable)))

    return implementation

and after that used like

>>> heapsort_with_key = with_key(heapsort)
>>> list(heapsort_with_key(range(10), key=lambda x: -x))
[9, 8, 7, 6, 5, 4, 3, 2, 1, 0]

Problem

I wonder if there is a better way (e.g. in terms of memory efficiency) of adding key parameter? Does anyone know how it is implemented in sorted?

Python is open-source, and all the source code in the GitHub repository python/cpython. For a great deal of the built-ins, they can be found in cpython/Python/bltinmodule.c
. In the current revision, the beginning of the sorted function definitions is found at line 2175, and the actual function itself is found at line 2201.

However, I'm not sure that knowing the implementation of sorted can help you too much with optimizing heapsort, because sorted uses Timsort.

I can make a suggestion: decorators will always add overhead, and particularly in a case like this, it's probably better to just make a new version of the function that integrates the key parameter with the original function, though this answer might miss the entire point of your question.


Permutations part two This is a follow up from this question. The goal is to make some function that can return the number of possible permutations with repetitions. As Gareth pointed out there is a nice formula for this problem:

$$
\frac{m!}{n_1!\,n_2! \ldots n_k!}
$$

Where \$m\$ is the length of our permutations and \$n_i\$ is the number of times the \$i\$th letter appears in \$m\$. A simple example can be shown with the word Mississippi.

$$
P(\text{Mississippi}) = \frac{11!}{4!\,4!\,2!}
$$

A simple implementation of this can be done in Python as follows:

from numpy import prod
def count_unique_perms(obj):
    obj = str(obj)
    f = [factorial(i) for i in Counter(obj).values()]
    return factorial(len(obj))/prod(f)

I include this code as a basetest for the "improved" version below. Questions were raised whether this approach works for large factorials, and it was said that the running time was at worst \$O(n^3(log n)^2)\$

I used the ideas from my previous question. To try to improve the answer. First I find the number of factors in \$m!\$ then I subtract the factors from \$n_1!\,n_2! \ldots n_k!\$. A small optimization I did was avoiding calculating duplicates.

My question if this is a decent improvement. It seems much slower until really big string. For example, it is slightly faster for str(Mississippi)*500. Any other tips or algorithms are also welcome =)

from collections import Counter
from math import factorial, log
from numpy import prod
from primesieve import generate_primes

def count_unique_perms(obj):
    obj = str(obj)
    f = [factorial(i) for i in Counter(obj).values()]
    return factorial(len(obj))/prod(f)

def fast_unique_perms(obj):
    obj = str(obj)
    factors = factors_factorial(len(obj))

    obj_count = Counter([i for i in Counter(obj).values() if i > 1])
    for p in obj_count:
        count_p = factors_factorial(p, obj_count[p])
        for prime in count_p:
            factors[prime] -= count_p[prime]

    num_perms = 1
    for p in factors:
        num_perms *= p**factors[p]
    return num_perms

def factors_factorial(n, multiplicity=1):
    "Calculates all the facors of n!"
    count = Counter()
    for p in generate_primes(n+1):
        mul = 0
        for k in range(1, int(1+log(n)/log(p))):
            mul += int(n/p**k)
        count[p] = multiplicity*mul
    return count

if __name__ == '__main__':
    import timeit

    lst = 1123
    string = 'mississippi'
    print count_unique_perms(string)
    print fast_unique_perms(string)
    times = 100
    t1 = timeit.timeit("count_unique_perms('mississippi'*1000)",
                       setup="from __main__ import count_unique_perms", number=times)/float(times)
    t2 = timeit.timeit("fast_unique_perms('mississippi'*1000)",
                       setup="from __main__ import fast_unique_perms", number=times)/float(times)

    print "fast_unique_perms: ", 1000*t2, "ms"
    print "count_unique_perms:  ", 1000*t1, "ms"
    print "The count_unique_perms solution was:   ", t2/float(t1), "times faster than fast_unique_perms."

1. Review

There are a bunch of small problems here — I think that if you had given yourself some more time (or looked more carefully at the answers you were given to your previous question) then you would have been able to fix many of them yourself.

Missing docstrings — you've written one for factors_factorial but what do the other functions do?
Vague variable names — what kind of object is obj? What kinds of objects are counted by obj_count? The name i is normally used for indexes but this code uses it for characters, so c would be better. I would expect the name p to refer to a prime, but here it refers to a count or multiplicity, so n or m would be better.
For portability to Python 3 it's a good plan to stick to // for integer division and / for floating-point division. In Python 2 you can use from __future__ import division to enforce the Python 3 semantics. (Note that PEP 238 is from 2001 — fifteen years later you shouldn't be writing new code using the old semantics.)
In the expression:

Counter([i for i in Counter(obj).values() if i > 1])

there's no need to create a list which you are going to throw away again immediately — you can pass a generator expression directly to the Counter constructor.
Instead of looping over the keys of a mapping and then having a separate lookup to find the value, use the items method to loop over the keys and values simultaneously.
Instead of adjusting the values of factors one at a time:

count_p = factors_factorial(n, m)
for prime in count_p:
    factors[prime] -= count_p[prime]

make use of the subtraction operation on Counter objects:

factors -= factors_factorial(n, m)

Each time factors_factorial is called, it has to generate the primes again. It would be better to generate them just once.
Instead of log(n)/log(p), write log(n, p) and save a function call. See the math.log documentation.
This loop requires two calls to log, and computes a power on every iteration:

for k in range(1, int(1+log(n)/log(p))):
    mul += n // p**k

It's better to write the loop like this:

k = n
while k:
    k //= p
    mul += k

Writing it like this is not only simpler and avoids the power, but k gets smaller on each iteration, and so the divisions get cheaper as you go along.

2. Use NumPy

The code in the post imports NumPy in order to use numpy.prod. But if you're going to use NumPy at all, then there's no reason not to use it throughout the code:

import numpy as np

def factorial_factors_powers(primes, n):
    """Return an array containing the power of each prime in the
    factorization of n!.

    """
    p = primes
    n = np.full_like(p, n)
    result = r = np.zeros_like(p)
    while True:
        n //= p
        if n[-1] == 0:       # some primes not contributing any more?
            l = n.argmin()   # number of primes still contributing
            if l == 0:
                break
            n, r, p = n[:l], r[:l], p[:l]
        r += n
    return result

def fast_unique_perms(s):
    """Return the number of unique permutations of the string s."""
    s = str(s)
    primes = generate_primes(len(s) + 1)
    powers = factorial_factors_powers(primes, len(s))
    for n, m in Counter(Counter(s).values()).items():
        if n > 1:
            powers -= factorial_factors_powers(primes, n) * m
    return np.prod(np.array(primes, dtype=int) ** powers)

Note the use of dtype=int to force NumPy to do the final set of multiplications using Python's bignums instead of NumPy's fixed-width numbers.

This is around five times as fast as the code in the post.


Approximate (250 over 100) permutation best fitting certain criteria Given a list of 250 words of 4 letters each, what is the fastest way, in Python, to find a subsample of 100 words (drawn without replacement) so that the distribution of letters across the whole sample is most even?

List of words: http://pastebin.com/2kpiNAcC

Note there are only 18 different characters, and some occur much more frequently than others.

(Currently, "even letter distribution" is operationalized as the smallest difference between max and min letter counts and the smallest standard deviation of letter counts, although I'm open to suggestions.)

I've tried brute forcing it by drawing random samples, and after various refinements have arrived at the code below. But 1. I assume my brute forcing isn't optimal and 2. there probably is a smarter way, but I can't think of any smart approach.

I am interested in finding a better subsample, so either making the brute-forcing more efficient, or using a smarter algorithm could help.
Not sure if this is better suited for Code Review (optimizing the brute forcing) or a site more suited for discussing a smarter algorithm.

import random

import numpy as np
import pandas as pd

# currently unused, as iterating over all combinations seems impossible
from itertools import combinations

from collections import Counter
from joblib import Parallel, delayed

#this just reads the file so "words" is a list of strings
#corresponding to the 250 words
fname = "file.txt"
df = pd.DataFrame.from_csv(fname, sep="\t")
words = list(df.index)

# define function to randomly sample words and rate them
def sampl(r):
    s, mm = 1000000, 1000000  # arbitrary constants
    for w in range(r):
        w = random.sample(words, 100)
        counts = [v for k, v in Counter("".join(w)).items()]
        s_new = np.std(counts)
        mm_new = np.max(counts) - np.min(counts)
        # compare current subsample to benchmark within this run
        if s_new < s:
            w_candidate_1 = w
            s = s_new
        if mm_new < mm:
            w_candidate_2 = w
            mm = mm_new
        if s_new <= s and mm_new <= mm:
            w_candidate_3 = w
    return (w_candidate_1, w_candidate_2, w_candidate_3)

# run the function a bunch (evaluating the results is fairly easy)
runs = 100
n_per_run = 999999
outs = Parallel(n_jobs=-10)(delayed(sampl)(r)
                for r in [n_per_run] * runs)

# to find the best candidate within the output of the parallel job ...
s, mm = 1000000, 1000000
for w in outs[0]:
    counts = [v for k, v in Counter("".join(w)).items()]
    s_new = np.std(counts)
    mm_new = np.max(counts) - np.min(counts)
    if s_new < s:
        w_candidate_1 = w
        s = s_new
    if mm_new < mm:
        w_candidate_2 = w
        mm = mm_new
    if s_new <= s and mm_new <= mm:
        w_candidate_3 = w

for ws in [w_candidate_1, w_candidate_2, w_candidate_3]:
    print([v for k, v in Counter("".join(ws)).items()])

Note: we have 10+ cores and 200+ GB RAM available to burn on this, so embarrassingly parallel approaches are useful.

1. Analysis

In the general case, the problem of finding the sample of words with the "most even" distribution of letters is NP-hard. Here I'm considering a general instance of this problem to be:

  Given an alphabet \$ Σ \$, a set \$ W \$ of 4-letter words over that alphabet, and an integer \$ n ≤ \left|W\right| \$, find the subset \$ W^* ⊆ W \$ with size \$ \left|W^*\right| = n \$ having the smallest variance of letter counts among all subsets of that size.

Here's a proof that this is NP-hard, by a reduction from EXACT COVER BY 3-SETS (that is, EXACT COVER restricted to sets of size 3, also known as "X3C").

Suppose that we have an instance of EXACT COVER BY 3-SETS in the form of a set \$ X \$ and a collection \$ S \$ of 3-element subsets of \$ X \$, and the problem is to determine if there is a subcollection \$ S^* ⊆ S \$ such that each element in \$ X \$ is contained in exactly one subset in \$ S^* \$.

This translates into the word problem as follows. Let \$ Σ = X ∪ S \$; let \$ W \$ be the set of four-letter words \$ abcd \$ such that \$ \{ a, b, c \} = d ∈ S \$, and let \$ n = { \left|X\right| \over 3 } \$. Then solve the word problem to find \$ W^* \$ with the smallest variance of letter counts. There is an exact cover if and only if the variance of the letter counts in \$ W^* \$ is zero. (Because there can only be one instance of any letter from \$ S \$, and so if the variance is zero, there must be exactly one instance of each letter from \$ X \$, and since there are  \$ \left|X\right| \over 3 \$ words, each element of \$ X \$ is covered exactly once.)

So there's no efficient (polynomial time) algorithm that solves your problem. Exhaustive search is out, because \$ {250 \choose 100} \approx 6×10^{71} \$. So the best you can do is to use heuristics to find good approximate solutions. I'll describe one possibility in §4 below.

2. Review

Having code at the top-level of a module makes it hard to test from the interactive interpreter, because whenever you reload the module, the code runs. It's best to guard top-level code with if __name__ == '__main__':.
The comment for sampl doesn't explain the most important points about the behaviour of the function, namely: what arguments does it take? and what does it return?
The number 100 is arbitrary, so it ought to be a parameter.
When finding the minimum or maximum, it's best to avoid a starting point like this, but instead to rearrange the code into a generator that can then be passed to min. That way you can avoid arbitrary starting points like 1000000.
Writing for w in range(r): is misleading, because w is not actually used (it's immediately overwritten by the random sample). It's conventional to write _ for unused loop variables.
The code uses NumPy to do the statistics on the sample. But there are at most 400 letters in the sample, so this doesn't really gain much: NumPy only shows a benefit for large volumes of data. I think that using the standard Python functions statistics.pstdev, max and min would be adequate for this amount of data.
Since the standard deviation is only used for comparison, you'd get the same results if you used the variance instead, and this would have the advantage of avoiding the square root.
Instead of Counter("".join(w)), use itertools.chain.from_iterable and write Counter(chain.from_iterable(w)). This avoids concatenating the words. [Improved by Veedrac in comments.]
Instead of:

counts = [v for k, v in Counter("".join(w)).items()]

write:

counts = list(Counter(chain.from_iterable(w)).values())

since you don't use the keys.
I dislike the logic for selecting w_candidate_3. This is only updated if both scoring functions show an improvement for this sample, and so the "best" sample according to this logic will depend on the order in which samples are examined. This is a bad property for a measure to have. It would be better to find some numeric way to combine the two scores (for example, their sum or product), so that all samples are comparable according to this measure.
The logic in sampl is complicated because you aren't sure of the right way to score the samples. In this kind of situation a better approach would be for sampl to take a scoring function as an argument. Then you could easily experiment with different kinds of score.

3. Revised code

from collections import Counter
from itertools import chain
import random
from statistics import pvariance

def letter_counts(words):
    """Generate the letter counts of the words."""
    return Counter(chain.from_iterable(words)).values()

def score_variance(words):
    """Score words according to variance of letter counts."""
    return pvariance(letter_counts(words))

def score_range(words):
    """Score words according to range of letter counts."""
    counts = list(letter_counts(words))
    return max(counts) - min(counts)

def best_sample(words, score, n=100, r=100):
    """Generate r (default 100) random samples of n (default 100) elements
    from words, and return the sample with the smallest score.

    """
    return min((random.sample(words, n) for _ in range(r)), key=score)

4. Hill climbing

A useful search technique in this kind of global optimization problem is hill climbing. The idea is to pick a random sample to start with, explore its neighbourhood by examining "nearby" samples, pick the best of the neighbours, and continue until no further improvement can be made. For example:

def best_sample_2(words, score, n=100, r=100):
    """Return a sample of n (default 100) elements from words, found by
    starting with a random sample and repeatedly hill climbing by
    generating r (default 100) neighbours and picking the one with
    smallest score, until no more progress is made.

    """
    words = set(words)
    current_sample = set(random.sample(words, n))
    current_score = score(current_sample)

    # Generate neighbouring samples (one word swapped).
    def neighbours():
        nonsample = words - current_sample
        for _ in range(r):
            s = set(current_sample)
            s.difference_update(random.sample(current_sample, 1))
            s.update(random.sample(nonsample, 1))
            yield s

    while True:
        new_sample = min(neighbours(), key=score)
        new_score = score(new_sample)
        if current_score <= new_score:
            # No neighbour is any better, so stop here.
            return current_sample
        current_sample, current_score = new_sample, new_score

Here's an example of how effective hill climbing can be in this problem. First, we'll use the original algorithm to pick the best of 10,000 samples.

>>> score_variance(best_sample(WORDS, score_variance, r=10000))
114.18300653594771

Now, using hill climbing:

>>> score_variance(best_sample_2(WORDS, score_variance))
23.241830065359476

The hill climbing approach reaches a local optimum. So usually it's a good idea to run it several times with different random starting points. For example, if I run it ten times and pick the best:

>>> f = lambda:best_sample_2(WORDS, score_variance)
>>> score_variance(min((f() for _ in range(10)), key=score_variance))
19.712418300653592

For further improvement, take a look at simulated annealing.


Store and manage a fixed amount of things generally I have an enum let's say with four values. Let's call them A, B, C, D.

public enum Categories
{
    A,
    B,
    C,
    D
}

I then have a Thing which stores something and has one of the values from my enum.

public class Thing
{
    public final Categories category;
    public int var;

    public Thing (Categories category) {
        this.category = category;
    }
}

All good. Now I know I will have exactly four things. I can change which things, I can set them to null, but I will always have one of each. I will always have exactly one A,B, C and one D. No more no less. So I store them in my Container. Like this.

public class Container
{
    private final Map<Categories, Thing> things = new HashMap<>();

    public Container () {
        for (int i = 0; i < Categories.values().length; i++) {
            things.put(Categories.values()[i], null);
        }
    }

    public Thing getThing (Categories c) {
        return things.get(c);
    }

    public Thing setThing (Categories c, Thing t) {
        Thing returnThing = null;

        if (things.containsKey(c) && t.category == c) {
            if (things.get (c) != null) {
                returnThing = things.get (c);
            }
            things.put(c, t);
        }

        return returnThing;
    }

    public Thing removeThing (Categories c) {
        Thing returnThing = null;

        if (things.containsKey(c) && things.get(c) != null) {
            returnThing = things.get(c);
            things.put (c, null);
        }

        return returnThing;
    }
}

While this code works, it feels a bit clunky. As I know I will never add another key, nor remove one. Is there any pattern I lack the knowledge of that solves this issue? Or, doubt it, is this the way to accomplish a task such as this?

I would prefer to stay away from external dependencies such as Guava.

Enum names are generally given in the singular form, rather than plural. Hence you may want to call your enum Category.

You should also consider using an EnumMap for your Map implementation:

private final Map<Category, Thing> things = new EnumMap<>(Category.class);

BTW, the return value of Map.put() will be the previously associated value, so you don't need the inner if (things.get (c) != null) { returnThing = things.get(c); } parts of your code (setThing()/removeThing()).

One more observation about your setThing() method: Since you are doing a t.category == c check before the insertion, why not make the method signature just setThing(Thing t)? In that case, the method body simply becomes:

public Thing setThing (Thing t) {
    // t == null check required?
    return things.put(t.category, t);
}

Now, to your question... may I know what's the fixation with exactly four entries in your things object, to the extent of mapping to null values? There is no difference in calling Map.get() for a mapped-to-null key, and an unmapped key: both returns null. The only difference is in Map.containsKey(), but as far as I can tell your class doesn't really expose this to external callers (which I guess is a good design). Is there a more relevant larger context to show how exactly four entries in things are being depended upon?

edit: The code-smell I observe here is that your code is doing/stating the obvious. Consider:

You use null to practically indicate the absence of a mapped Thing.
things is pre-populated with all your enum keys, so things.containsKey(category) will always be true.

In other words, things.containsKey(category) && anotherCondition can be simplified to anotherCondition.

When you are adding or removing mapped Things, you do a null-check to return the previous non-null mapping...
but if the mapping is to null, you return a null anyways.

As mentioned above, the relevant methods in Map already returns the previously associated value or null, so your null checks are pretty much redundant.

Unless your code clearly distinguishes between the absence of a mapping and a null-mapping, which I don't see here, you should simplify your logic to treat both as the same. If you do however, then the difference for Map.containsKey() becomes crucial here, and then all the more should you not have null-mappings...


Path shortener for GUI application I need to display a path within a GUI application. Displaying the whole path would be an overkill, so I made a custom path shortener.

def cut_path(path):
    ''' This function takes a path and outputs the path cut with the ... after the
     the first folder after the first character and completes the name with the filename or the last folder
    Example:
    C:/Users/Mauro/Desktop/DenoiseAvg/DenoiseAverage/test_cut_path.py
    C:/Users/.../test_cut_path.py
     '''
    endidx = 5
    char = path[endidx]
    while char != '/' and endidx < len(path):
        endidx += 1
        char = path[endidx]

    inpath = path[:endidx + 1 ]

    rpath = path[::-1]
    idx = 1
    char = rpath[idx]
    while char != "/" and idx < len(rpath):
        idx += 1
        char = rpath[idx]

    endpath = path[len(path) - idx - 1 : ]

    outpath = inpath + "..." + endpath
    return outpath

Pastebin for syntax highlighting

Is there a shorter, more readable, pythonic way to do it? Is there a way to limit how many characters the path would be long? How would you do it?

Python is 'batteries included', this means offloading code to the core library is a good thing, and often possible. In this case, if you're running Python 3.4+ you could make use of pathlib. All you need is to know the pathlib.PurePath constructor and PurePath.parts.

And so you can change your code to:

import pathlib

def cut_path(path):
    parts = list(pathlib.PurePath(path).parts)
    if len(parts) >= 4:
        parts [2:-1] = ['...']
    return pathlib.PurePath(*parts)

However, as you're doing it on how long the path is, you could dynamically build the new path. And so could use something like:

def cut_path(path, max_size=5):
    if not path:
        return path

    parts = list(pathlib.PurePath(path).parts)

    path = pathlib.PurePath(parts[0])
    for part in parts[1:-1]:
        path /= part
        if len(str(path)) >= max_size:
            path /= '...'
            break
    if len(parts) > 1:
        path /= parts[-1]
    return path

If however you're using Python 2 or Python <3.4 then you can't use the nice new interface, and would have to use the old interface, os.path. Which doesn't support splitting and joining the paths very well. First you may want to use os.path.normcase to normalize the path. Which may destroy the path if you have a Posix path on Windows. After this you'd want to take the drive with os.path.splitdrive, and then split the rest of the parts using os.sep. Then you want to do the same as above, and replace the middle section of the path, and finally os.path.join them all together.

Which could leave you with something like:

def cut_path(path):
    path = os.path.normcase(path)
    drive, path = os.path.splitdrive(path)
    parts = path.split(os.sep)
    parts[0] = drive + parts[0]
    # Join plays funny, so you have to add a separator in these cases.
    if parts[0][-1] == ':':
        parts[0] += os.sep
    if len(parts) >= 4:
        parts [2:-1] = ['...']
    return os.path.join(*parts)


Finding an Eulerian cycle in a graph Given a graph that has an Eulerian cycle, write a function which returns the cycle in tuple form.

I came up with following solution for this problem and am stuck trying to make it faster. Do you have any tips?

def eulerian_cycle_1(data):
    graph, edges_amount = get_graph(data) #graph:: {source:[destination]}
    cycle = deque()

    cur = 0
    while edges_amount > 0:
        choices = graph[cur]

        while choices:
            cycle.append(cur)
            edges_amount -= 1
            cur = choices.pop()
            choices = graph.get(cur, None)

        if edges_amount == 0:
            break

        rotate = 0
        for cur in cycle:
            if graph[cur]:
                break
            rotate += 1

        cycle.rotate(-rotate)

    cycle.rotate(-cycle.index(0))
    cycle.append(0)

    return tuple(cycle)

Pseudocode

EulerianCycle(\$G\$):

form a cycle \$c\$ by randomly walking in graph \$G\$ (don't visit the same edge twice!)
while there are unexplored edges in graph \$G\$

select a node \$n\$ in cycle \$c\$ with still unexplored edges
form cycle \$c′\$ by traversing cycle \$c\$ (starting at node \$n\$) and then randomly walking
\$c ← c′\$

return \$c\$

1. Review

The code returns the wrong result when the graph has no Eulerian cycle. For example, if we give it the graph {0:[1], 1:[]} then the code returns the tuple (0, 0), which does not correspond to any legal path in the graph. It would be better to raise an exception if the graph has no Eulerian cycle.

I know that the problem description says that the graph has an Eulerian cycle, but in real life data is sometimes incorrect and it is a good idea to write code so that it is robust.
Similarly, the code goes into an infinite loop if the graph is disconnected:

>>> eulerian_cycle_1({0:[0], 1:[1]}, 2)
^C
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr188627.py", line 21, in eulerian_cycle_1
    if graph[cur]:
KeyboardInterrupt

It would be more robust to raise an exception in this case.
The code doesn't work if the graph doesn't have a node 0:

>>> eulerian_cycle_1({1:[1]}, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr188627.py", line 8, in eulerian_cycle_1
    choices = graph[cur]
KeyError: 0

It would be more robust to pick a starting node from the graph.
The number of edges can be easily computed from the graph like this:

sum(map(len, graph.values()))

so it would simplify the interface if the code only required the graph, and computed the number of edges itself.
But in fact there is no need to know the number of edges: you can exit the main loop if the search along the cycle for a node with an edge fails.

2. Performance

The problem with the performance is that whenever it reaches a point where the cycle cannot be extended, the code searches along the cycle to try to find a node which still has edges. In the worst case, a graph with \$n\$ nodes and \$O(n)\$ edges might have \$Θ(n)\$ points where the cycle cannot be extended, and at each of these points the code will have to search \$Θ(n)\$ nodes in the cycle. In this case the overall runtime will be \$Θ(n^2)\$, that is quadratic.

The worst case arises in graphs like this:

and we can build these graphs systematically if we define get_graph like this:

def get_graph(n):
    "Return a worst-case graph with n nodes and 2n edges."
    return {i:[min(i + 1, n - 1), max(i - 1, 0)] for i in range(n)}, 2 * n

Now, if we increase the size of the graph by 10 times, it takes 100 times as long to find an Eulerian cycle:

>>> from timeit import timeit
>>> timeit(lambda:eulerian_cycle_1(10**3), number=1)
0.08308156998828053
>>> timeit(lambda:eulerian_cycle_1(10**4), number=1)
8.778133336978499

To make the runtime linear in the number of edges, we have to avoid traversing or rotating the whole cycle, except possibly once at the end. There are two approaches we might take:

Represent the cycle as a linked list (not a deque) so that we can efficiently insert new items at any point, without having to rotate.
Don't try to join the cycles as we go along, but instead keep a collection of cycles and stitch them together at the end using a depth-first search.

I'll show here how to implement the first approach (see this answer for how to implement the second approach).

Python doesn't provide a linked list implementation, but we can easily make one:

class Link:
    "A link in a linked list."
    def __init__(self, node):
        self.node = node
        self.next = self

    def insert(self, link):
        "Insert link after self in the linked list and return link."
        link.next = self.next
        self.next = link
        return link

Now it's just a matter of keeping track of the Link objects corresponding to the nodes in the graph that still have edges, so that we can efficiently find the place to continue extending the cycle.

def eulerian_cycle_2(graph):
    """Return an Eulerian cycle in graph, if there is one, as a list of
    nodes. If there are no Eulerian cycles, raise ValueError.

    """
    # Take a copy of the graph so that we can modify it.
    graph = {k:v[:] for k, v in graph.items()}

    # Start at any node in the graph that has an edge.
    node = next(node for node, neighbours in graph.items() if neighbours)
    choices = graph[node]
    start = cur = Link(node)

    # Map from node we've visited (that still has edges) to a link for
    # that node in the linked list.
    visited = {}

    while True:
        # Extend current cycle until no more edges can be followed.
        cycle_start = node
        while choices:
            visited[node] = cur
            node = choices.pop()
            choices = graph[node]
            cur = cur.insert(Link(node))
        if node != cycle_start:
            raise ValueError("graph has no Eulerian cycle")

        # Find a visited node which still has edges, if any.
        while visited:
            node, cur = visited.popitem()
            choices = graph[node]
            if choices:
                break
        else:
            break

    if any(graph.values()):
        raise ValueError("graph has no Eulerian cycle")

    # Reconstruct cycle by following linked list.
    cycle = []
    cur = start
    while True:
        cycle.append(cur.node)
        cur = cur.next
        if cur == start:
            break
    return cycle

We should check that the performance is linear in the number of edges. Here you can see that when the graph is 10 times as big, the runtime is roughly 10 times as long, as required:

>>> timeit(lambda:eulerian_cycle_2(get_graph(10**3)[0]), number=1)
0.0049093629932031035
>>> timeit(lambda:eulerian_cycle_2(get_graph(10**4)[0]), number=1)
0.045048179978039116
>>> timeit(lambda:eulerian_cycle_2(get_graph(10**5)[0]), number=1)
0.42420267598936334
>>> timeit(lambda:eulerian_cycle_2(get_graph(10**6)[0]), number=1)
4.303449199011084

For comparison, by extrapolating the timings from earlier in this answer, we would have expected eulerian_cycle_1 to take about 24 hours to find a cycle in the worst-case graph with \$10^6\$ notes.


Parsing option symbols I think I have this working, but I'm not sure it's completely accurate.

I'm trying to parse Option Symbols. An Option Symbol is made up of 4 parts:

Root Symbol (up to 6 characters)
Expiration Date (yymmdd)
Option Type (1 character)
Strike price (8 digits)

After parsing the following examples, the results should be as follows:

C020216P00035000

Root Symbol ='C'
Expiration Date = datetime.date(2002, 2, 16)
Option Type = P
Strike Price = int(00035000) x .001 = 35.00

P020216C00040000

Root Symbol = 'P'
Expiration Date = datetime.date(2002, 2, 16)
Option Type = C
Strike Price = int(00040000) x .001 = 40.00

SBC020216C00030000

Root Symbol = 'SBC'
Expiration Date = datetime.date(2002, 2, 16)
Option Type = C
Strike Price = int(00030000) x .001 = 30.00

I'm using the following code:

import re
import datetime as dt

opra_symbol = re.compile(r'(^[^0-9]+)').search(OPRA).group()

opra_expiry = dt.datetime.strptime(re.compile(r'\d{2}\d{2}\d{2}').search(OPRA).group(), '%y%m%d').date()

opra_cp = re.compile(r'([CP])').search(re.compile(r'([CP]\d+$)').search(OPRA).group()).group()

opra_price = int(re.compile(r'(\d+)$').search(OPRA).group()) * .001

Is this the best way of getting my results? I'm mostly concerned with the nested regex expression for the Option Type.

It looks like you can simplify it to the single regex:

    matcher = re.compile(r'^(.+)([0-9]{6})([PC])([0-9]+)$')
    groups = matcher.search(option)

Then symbol, expiry, type and price are in groups[0], groups[1], groups[2] and groups[3] respectively. The expiry is guaranteed to be in yymmdd format, hence the {6} qualifier. You may want to add a {8} length qualifier to the price.


Improving my solution to the Quiz project from Gophercises in Golang I'm new to Go and I completed the first challenge from Gophercises. Any feedback would be highly appreciated.
There are some things that I am unsure of if they are made in an optimal way.
Github repo

Goroutines and channels in the quizList loop

Better error logging for the cli portion

The Shuffle function only works for string, how can I make it work for other types as well?

How should I approach writing test cases?

How should the folder structure be structured?
 package main

 import (
     "encoding/csv"
     "flag"
     "fmt"
     "log"
     "math/rand"
     "os"
     "strings"
     "time"
 )

 type question struct {
     prompt, answer string
 }

 func main() {

     var (
         csvFile = flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
         timeLimit = flag.Int("limit", 3, "the time limit for the quiz in seconds")
         isShuffle = flag.Bool("shuffle", false, "shuffle the quiz order each time it is run")
     )

     flag.Parse()

     if *csvFile == "" {
         fmt.Printf("%s does not exist, please choose another file", *csvFile)
     }

     if *timeLimit == 0 {
         fmt.Printf("Not a valid limit, please choose another limit")
     }

     score := 0

     file, err := os.Open(*csvFile)

     if err != nil {
         log.Fatal(err)
     }

     r := csv.NewReader(file)

     problems, err := r.ReadAll()

     if err != nil {
         log.Fatal(err)
     }

     if *isShuffle {
         Shuffle(problems)
     }

     quizList := populateQuiz(problems)

     timer := time.NewTimer(time.Duration(*timeLimit) * time.Second)

     isTimerDone := true

     ch := make(chan string)

     for _, quiz := range quizList {

         fmt.Printf(quiz.prompt)

         var recipientAnswer string

         go func() {
             fmt.Scanln(&recipientAnswer)
             ch <-recipientAnswer
         }()

         if isTimerDone {
             select {
             case <-ch:
                 if recipientAnswer == quiz.answer {
                     score += 1
                 }
             case <-timer.C:
                 isTimerDone = false
                 fmt.Println()
                 return
             }
         } else {
             break
         }

     }

     fmt.Printf("\nYou scored %d out of %d.\n", score, len(problems))

 }

 func Shuffle(l [][]string) {
     rand.Seed(time.Now().UnixNano())

     rand.Shuffle(len(l), func(i, j int) {
         l[i], l[j] = l[j], l[i]
     })
 }

 func populateQuiz(problems [][]string) []question {

     var questions []question

     for index, record := range problems {

         prompt := fmt.Sprintf("Problem: #%d: %s = ", index + 1, record[0])
         answer := record[1]

         questions = append(questions, question{
             prompt: prompt,
             answer: strings.TrimSpace(answer),
         })

     }

     return questions
 }

from what I saw there are a few changes that could be done to the program

while parsing command-line arguments I think it's better to put them inside a function cause it makes the code look clean and simple and as it is returned as a pointer you would have to keep getting the value as * but you could return it as the specific type and have a simpler handling
 csvFile, timeLimit, isShuffle := parseCMD()

 func parseCMD() (string, int, bool) {
 csvFile := flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
 timeLimit := flag.Int("limit", 3, "the time limit for the quiz in seconds")
 isShuffle := flag.Bool("shuffle", false, "shuffle the quiz order each time it is run")
 flag.Parse()
 return *csvFile, *timeLimit, *isShuffle

}

var (
csvFile = flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
timeLimit = flag.Int("limit", 3, "the time limit for the quiz in seconds")
isShuffle = flag.Bool("shuffle", false, "shuffle the quiz order each time it is run")
)
var can be used on a variable that doesn't have any initialization or if it's gonna be a global score or... if in a case outside an if statement or a for loop (a situation where:= won't work)..  as these are being declared inside a function and have a default value a short variable declaration would be a more golang kinda way. (there is not a lot of difference between using var and:= in your code but in a situation like this:= would be more golang-ish)
https://stackoverflow.com/questions/53404305/when-to-use-var-or-in-go/53404332
https://stackoverflow.com/questions/21657446/var-vs-in-go

 timer := time.NewTimer(time.Duration(*timeLimit) * time.Second)

while defining a ticker add a "defer ticker.stop()" to release any resource related to as it wont be garbage collected (https://golang.org/src/time/tick.go)

if isTimerDone {
select {
case <-ch:
if recipientAnswer == quiz.answer {
score += 1
}
case <-timer.C:
isTimerDone = false
fmt.Println()
return
}
} else {
break
}

In the above case the if check (isTimerDone) won't actually hit as by flow you always go into the select case and if you get an answer you continue the loop and if the timer hits you return.. so a better way to write it would be to remove the IF-ELSE check..
from what I understand yours is a score counting program so assuming you don't answer and the timer hit.. you could continue the loop and score based on the total answer or break the loop if he doesn't answer (this is just a suggestion :) )


Get external IP and update ddns service I've created a cli application that consists of a package with 2 functions. One to get the external ip address, and another function to update a dynamic dns record at dns.he.net. Both are then used in main based on the provided parameters.

Since this is my first "real" go application i would like to know if I'm using structs, interfaces and functions correctly, as well as the overal structure of the application and if stuff should move into the functions or should go into main.

(Since it is tailor-made for my specific case I've hardcoded the remote url's.)

All feedback is welcome.

usage:

dyn-he-updater get --ipv4

dyn-he-updater update --domain=example.com --password=skdffngkele --ipv4

dyn.go

package dyn

import (
    "crypto/tls"
    "github.com/go-resty/resty"
    "log"
    "net"
    "strings"
)

type Dyn interface {
    GetIp() string
    UpdateIp()
}

type He struct {
    Hostname string
    Password string
    Ip       net.IP
}

func GetIp(uri string) net.IP {
    resp, err := resty.R().Get(uri)
    if err != nil {
        log.Fatal("ERROR: %s", err)
    }

    ip := net.ParseIP(strings.TrimSpace(string(resp.Body())))

    if ip == nil {
        log.Fatal("ERROR: Failed to parse the IP.")
    }
    return ip
}

func (h *He) UpdateIp(uri string) {
    log.Println("INFO: updating the ip")
    log.Printf("INFO: URI: %s \nDomain: %s \nPassword: %s \nIP: %v\n", uri, h.Hostname, h.Password, h.Ip)

    resty.SetTLSClientConfig(&tls.Config{InsecureSkipVerify: true})

    resp, err := resty.R().
        SetFormData(map[string]string{
            "hostname": h.Hostname,
            "password": h.Password,
            "myip":     h.Ip.String(),
        }).
        Post(uri)
    if err != nil {
        log.Fatal("ERROR: ", err)
    }

    log.Printf("INFO: resp: %v", string(resp.Body()))
}

main.go

package main

import (
    "fmt"
    "github.com/Adamar/dyn-he-updater/dyn"
    "gopkg.in/alecthomas/kingpin.v2"
    "net"
    "os"
)

var (
    app   = kingpin.New("dyn-he-updater", "A commandline updater for dynamic ip's from dyn.dns.he.net")
    debug = app.Flag("debug", "Enable debug mode.").Bool()

    get   = app.Command("get", "Get the raw ip from an external service.")
    getV4 = get.Flag("ipv4", "Get only the IPv4 ip address.").Bool()
    getV6 = get.Flag("ipv6", "Get only the IPv6 ip address.").Bool()

    update = app.Command("update", "Update the dynamic ip.")
    domain = update.Flag("domain", "Domain you want to update.").Required().String()
    pass   = update.Flag("password", "Password to update the domain.").Required().String()
    v4     = update.Flag("ipv4", "Update only the IPv4 ip address. (default)").Bool()
    v6     = update.Flag("ipv6", "Update only the IPv6 ip address.").Bool()

    CheckIPv4Uri = "http://ipv4.myexternalip.com/raw"
    CheckIPv6Uri = "http://ipv6.myexternalip.com/raw"
    UpdateIpUri  = "https://dyn.dns.he.net/nic/update"
)

func main() {
    kingpin.Version("0.0.1")

    switch kingpin.MustParse(app.Parse(os.Args[1:])) {

    case get.FullCommand():

        if *getV4 == true {
            r := dyn.GetIp(CheckIPv4Uri)
            fmt.Printf("IPv4: %s\n", r)
        } else if *getV6 {
            r := dyn.GetIp(CheckIPv6Uri)
            fmt.Printf("IPv6: %s\n", r)
        } else {
            r := dyn.GetIp(CheckIPv4Uri)
            fmt.Printf("IPv4: %s (default)\n", r)
        }

    case update.FullCommand():
        if *domain != "" && *pass != "" {

            var r net.IP
            if *v4 == true || *v6 == false {
                r = dyn.GetIp(CheckIPv4Uri)
            } else if *v6 == true {
                r = dyn.GetIp(CheckIPv6Uri)
            }

            d := dyn.He{Hostname: *domain, Password: *pass, Ip: r}
            d.UpdateIp(UpdateIpUri)
        }
    }
}

I find it annoying that you can't update both the IPv4 and IPv6 records in just one invocation of your program.

According to the usage guide at https://dns.he.net,

Hurricane Electric also provides a "what is my IP address?" service, at http://checkip.dns.he.net/ .  That way, your program wouldn't be relying on a second service provider.
Better yet, this dynamic DNS service automatically infers that you want to add a DNS record that refers to your current IP address, if you omit the myip parameter.

Autodetect my IPv4/IPv6 address:
% curl -4 "http://dyn.example.com:password@dyn.dns.he.net/nic/update?hostname=dyn.example.com"
% curl -6 "http://dyn.example.com:password@dyn.dns.he.net/nic/update?hostname=dyn.example.com"


Can it be shorter?: DNA Sequence Analyzer from CS50 written in Python This is my first time requesting a code review. This is code I wrote in Python to create a DNA sequence analyzer. This is for the Harvard CS50 course.
I was really hoping to get some constructive criticism on this code I finished for the DNA problem in Problem Set 6. This code passed all tests and is fully functional as far as I am aware.
I was mainly wondering if there were any more succinct ways to rewrite parts of it. I spent several hours writing this and my brain is pretty exhausted, so near the end I just went for whatever worked.
Any input is appreciated! (Also, sorry if this is an excessive amount of comments, but they help me keep everything clear in my head.)
import re
import csv
from sys import argv

# Checks for correct number of command-line arguments
if not len(argv) == 3:
    print("Incorrect number of command-line arguments.")
    exit(1)

# Declares a dictionary containing STR counts
str_counts = {
    "AGATC": "0",
    "TTTTTTCT": "0",
    "AATG": "0",
    "TCTAG": "0",
    "GATA": "0",
    "TATC": "0",
    "GAAA": "0",
    "TCTG": "0"
}

# Opens the csv and txt files to read (and closes them later)
with open(argv[1], "r", newline="") as csv_file, open(argv[2], "r") as sequence:
    # Reads the database into a dict and the sequence into a string
    db = csv.DictReader(csv_file)
    sq = sequence.read()

    # Stores the fieldname of the dictionary and stores the keys in STRS, skipping first line
    keys = db.fieldnames
    key_len = len(keys)
    # Skips first row of headers
    STRs = keys[1:key_len]

    # Bool to signal if match was found
    matched = False

    # Checks for STR matches and length of those matches, then stores those values in str_counts
    for STR in STRs:
        # Executes following code only if there are 1 or more matches
        if re.search(rf"(?:{STR}+)", sq):
            # Creates a list of matches
            matches = re.findall(rf"(?:{STR})+", sq)
            # Finds the longest match
            longest = max(matches, key = len)
            # Stores that value in corresponding dict key
            str_counts[STR] = len(longest) / len(STR)
            # Compares str_counts values to database to find match

    # Compares the match count values to the database values
    for row in db:
        # Declares a counter to later check if all STR values are matched
        # Resets counter to zero every iteration
        match_count = 0
        # Declares a temporary dictionary with only int match values to compare STR counts to
        compare = {}
        # Stores the names field for use later
        match_name = row['name']
        # Deletes the names field so we only have ints in our compare dict
        del row['name']
        # Converts values to integers for easy comparison
        for key, value in row.items():
            compare[key] = int(value)
        # Increments match count
        for STR in STRs:
            if compare[STR] == str_counts[STR]:
                match_count += 1
        # If all fields are matched, print match name, switch bool to true
        if match_count == len(STRs):
            matched = True
            print(match_name)

    if not matched:
        print("No Match")

I would group the code in functions and call them from a central main. With how you have it now, simply loading the file into an interactive console will cause all the code to run, which, if the code is long running or causes side-effects, may be less than ideal. Say I wrote a port-scanner, and it takes 2-3 minutes to run. I may not want it to start simply because I loaded the file into a REPL in Pycharm. Not only would that waste a few minutes each time, port-scanning can get you in trouble if done on a network that you don't have permission to scan.
Code wrapped in functions is simply easier to control the execution of, because it will only run when the function is called.
Grouping code into functions also allows you to easily test each chunk in isolation from the rest of the code. It's difficult to test code when you're dependent on the first half of the code to supply test data to the latter-half. It's much easier however to simply load the script into a REPL, and throw test data at the function (or use proper unit-tests). Then you can test one piece of code in isolation without touching the rest of the code.

I personally believe that STRs and STR should be lower-case. According to PEP8, Python's style and convention guide, regular variables should be lower case, with words separated by underscores. Variables that are considered to be constants however should be upper-case, separated by underscores.
Local variables that remain unchanged have never clicked as "constants" in my head, so it depends on what you decide.
Regardless though, STRs should either be all upper-case, or all lower-case; in which case the STR counterpart will need to be renamed so it doesn't clash with the str built-in. A name like str_ could be used, or you could use a more descriptive name.

compare = {}

. . .

for key, value in row.items():
    compare[key] = int(value)

This can make use of a dictionary comprehension. Whenever you have the pattern of creating a set/list/dictionary before a loop, then populating the structure within the loop, you should consider a comprehension:
compare = {key: int(value) for key, value in row.items()}

Similarly, to ensure that a condition holds for an entire collection (or two in this case), you can combine all with a generator expression (the part passed to all):
if all(compare[STR] == str_counts[STR] for STR in STRs):
    matched = True
    print(match_name)

"If all corresponding values in the two dictionaries match, set the matched flag". It reads quite nicely.
That gets rid of the need for match_count, and the second last loop.

You're looping more than you need to. You appear to only care about if at least one match is found; yet you keep looping even after one is.
I'd break once one is found. If you combine that with for's else clause, you can prevent unnecessary looping and still detect failure:
for row in db:
    . . .
    if all(compare[STR] == str_counts[STR] for STR in STRs):
        print(match_name)
        break

else:
    print("No Match")


Using different instances of an object to lock threads I'd like to use lock objects that are specific to the person I'm updating. In other words, if thread A is updating Person 1, thread B is blocked from also updating Person 1, but thread C is not blocked from updating Person 2.

99% of the time, I don't really need the locks since I'm working with different Person.Id values. In the occasional situations when I am working with the same Person.Id value, I want to lock around some read/write code.

The following code is working as I expect. I'm looking for any "gotcha's" I may have missed, or better ways of accomplishing the same thing. I decided to use the .NET Cache to store the lock objects so I don't need to worry about cleaning them up later (the lock objects are removed from cache when they haven't been used for a certain TimeSpan).

This code provides the object to lock on

public static class PersonLocks
{
    private static readonly object CacheLock = new object();
    private const string KeyPrefix = "LockForPersonID:";

    public static object GetPersonLock(long personId)
    {
        lock (CacheLock)
        {
            string key = BuildCacheKey(personId);
            object cachedItem = HttpRuntime.Cache[key];
            if (cachedItem == null)
            {
                cachedItem = new object();
                HttpRuntime.Cache.Insert(key, cachedItem, null, Cache.NoAbsoluteExpiration, new TimeSpan(0, 5, 0));
            }

            return cachedItem;
        }
    }

    private static string BuildCacheKey(long personId)
    {
        return KeyPrefix + personId.ToString();
    }

}

This is how I'm using the code

object padlock = PersonLocks.GetPersonLock(person.Id);
lock (padlock)
{
    //do read

    //do some data mapping

    //do write
}

Your code would do just fine, in terms of functionality.

However, note that such design holds as long as everybody knows that they should acquire the lock before performing any process on the person object.

Perhaps you should consider re-design your data objects (such as the person class) so that there's an explicit method for any update process. Perhaps something like BeginEdit and EndEdit, or, even better, a method that returns an IDisposable object that acquires the lock and releases it in its Dispose implementation. Example:

public class Person {
    public IDisposable AcquireLock() {
        // ...
    }

    // ...
}

// usage:
var aPerson = GetPerson(1234);
using (aPerson.AcquireLock()) {
    aPerson.LastName = "abcd";
    SavePerson(aPerson);
    // ...
}

This way the locking is made more explicit.

Just a suggestion, though...