current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This code does the following:

  1. Read CSV file and determine sub_type based on series_desc.

  2. Turn row into dictionaries new_person and new_subscriptions.

  3. Check list people for the new_person and add the person if it is actually new.

  4. Check each person in the list people for the new_subscription and add it if it is not there.

My desired output is a list of dictionaries called people. A dictionary in people might look like this:

{'id': group_id, 'subscriptions': [sub_dict1, sub_dict2]}

where a sub_dict looks like this:

{'id': sub_id, 'season': season_desc, 'sub_type': sub_type}

The code:

import csv

people = []

#open csv file
csv_file = csv.reader(open(filename, 'rb'))

#read CSV file into a list of people
for record in csv_file:
    sub_id = record[0]
    group_id = record[6]
    season_desc = record[11]
    series_desc = record[12]
    price = record[13]

    if "MW" in series_desc:
        sub_type = "Masterworks"
    elif "Pops" in series_desc:
        sub_type = "Pops"
    elif "Chamber" in series_desc:
        sub_type = "Chamber"

    new_person = {'id': group_id}
    new_subscription = {'id': sub_id, 'season': season_desc, 'sub_type': sub_type}

    #if this is the first time through the loop
    if len(people) == 0:
        new_person['subscriptions'] = []
        new_person['subscriptions'].append(new_subscription)
        people.append(new_person)

    #if this is not the first time through the loop
    else:
        #check if this person has already been recorded
        if any(person['id'] == new_person['id'] for person in people) == True:
            #check if this subscription has already been added to this person
            #use enumerate to get a key to search people
            for i in enumerate(people):
                if i[1]['id'] == new_person['id']:
                    key = i[0]

                    if any(sub['id'] == new_subscription['id'] for sub in people[key]['subscriptions']) == True:
                        pass
                else:
                    people[key]['subscriptions'].append(new_subscription)

        #if not, add the subscription to them and save the person
        else:
            new_person['subscriptions'] = []
            new_person['subscriptions'].append(new_subscription)
            people.append(new_person)

The challenge: It is REALLY slow.

I have for/if/else loops nested inside for/if/else loops, so I am sure that is the cause. What can I do differently?

share|improve this question
    
How much data are you going through that this is noticeably slow? –  ferada Jun 18 at 8:44
    
About 55,000 records, currently. We add about 3-4K annually, also. –  Brandon Dorris Jun 19 at 15:23

2 Answers 2

up vote 3 down vote accepted
  • The opened file is never closed, a better habit is to use with open(filename, 'rb') as foo:.
  • foo == True is rarely needed; unless you really need to differentiate between True/False and the various other truthy values it's easier just to test for the variable instead. Same goes for the empty list for that matter.
  • The pass doesn't do anything; price is unused.
  • Instead of linear search you could keep a set or dict of already recorded people around.
  • With enumerate it's easier to destructure the result directly in the loop; that said, the enumerate is also not necessary here.
  • The use of key looks buggy: In the else branch it has never been set (except maybe because of Pythons moronic scoping rules). In any case it'd better to make clear how that works.

That would be something the lines of the following then:

import csv

people = []
people_ids = set()

def add_person(group_id, new_subscription):
    people.append({
        'id': group_id,
        'subscriptions': [new_subscription]
    })
    people_ids.add(group_id)

#open csv file
with open(filename, 'rb') as file:
    csv_file = csv.reader(file)

    #read CSV file into a list of people
    for record in csv_file:
        sub_id = record[0]
        group_id = record[6]
        season_desc = record[11]
        series_desc = record[12]

        if "MW" in series_desc:
            sub_type = "Masterworks"
        elif "Pops" in series_desc:
            sub_type = "Pops"
        elif "Chamber" in series_desc:
            sub_type = "Chamber"

        new_subscription = {
            'id': sub_id,
            'season': season_desc,
            'sub_type': sub_type
        }

        #if this is the first time through the loop
        if len(people) == 0:
            add_person(group_id, new_subscription)
            continue

        #check if this person has already been recorded
        #if not, add the subscription to them and save the person
        if group_id not in people_ids:
            add_person(new_subscription)
            continue

        #check if this subscription has already been added to this person
        #use enumerate to get a key to search people
        for person in people:
            if person['id'] != group_id:
                person['subscriptions'].append(new_subscription)

I also shuffled the branches around, which together with continue makes for less indented blocks.

If you're going to extend this, I'd recommend more factoring into functions, a main function, etc.

share|improve this answer
    
Thank you so much for a great response. I had already corrected a couple things you noticed. When I get back in front of my machine I will test this out and hopefully mark as an answer. –  Brandon Dorris Jun 17 at 22:18
    
I haven't had time to debug, but it looks like this is adding some people to the list multiple times. It also seems to get stuck in a loop, and uses approx. 35X the memory as the original script. It does appear to be working faster until it gets stuck. EDIT - it is not counting people multiple times. I am looking for the memory use and will post a result. Definitely moving faster, so I am marking this as the answer. –  Brandon Dorris Jun 18 at 6:07
up vote 1 down vote

Use a loop:

if "MW" in series_desc:
    sub_type = "Masterworks"
elif "Pops" in series_desc:
    sub_type = "Pops"
elif "Chamber" in series_desc:
    sub_type = "Chamber"

becomes:

for type_ in ("MW", "Pops", "Chamber"):
    if type_ in series_disc:
        sub_type = type_

If as you say:

The series description might have "MW" or "mw" or "Masterworks" or "masterworks." I need the type for all of those to be "Masterworks."

just use some tuples:

for types, to_write in ( ( ("MW","mw","Masterworks","masterworks"), "Masterworks"))):
    if any(t in series_disc for t in types):
         sub_type = to_write
share|improve this answer
    
My code is simplified. The series description might have "MW" or "mw" or "Masterworks" or "masterworks." I need the type for all of those to be "Masterworks." –  Brandon Dorris Jun 17 at 21:21
    
@BrandonDorris Use a dict to map those to the same thing. –  o11c Jun 17 at 23:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.