2
\$\begingroup\$

Although I've been programming a fair while I'm learning Python and would like to know how I can make my code more "Pythonic".

I'm have a CSV file of eBay categories, containing the name and numeric code which I want to import in to my database. Unfortunately the file does not include a 'parent' category ID for each row, instead it uses the ordinal position of the name in row to indicate the nesting level:

L1,L2,L3,L4,L5,L6,Category ID
Antiques,,,,,,20081
,Antique Clocks,,,,,13851
,,Bracket Clocks,,,,100904
,,,Pre-1900,,,66840
,,,Post-1900,,,96762
,,Cuckoo Clocks,,,,13850
,,Longcase/ Grandfather Clocks,,,,100905
,,,Pre-1900,,,60249
,,,Post-1900,,,96763
,Antique Furniture,,,,,20091
,,Armoires/ Wardrobes,,,,98436

I've written a Python script to process the file, which is working correclty and generating the output I need, but I would like to know how it can be improved.

The script below is slightly changed from the one I am using, with simplified output (making it less verbose) but if anyone is interested in the other version which generates a JSON file for importing in to a Django site database, let me know.

I wanted to process the file line-by-line and avoid loading all the data in to memory, in case the file being processed is huge. So it uses a stack to record the parent category ID, other than that very little information is stored.

import csv


class Stack:
    def __init__(self):
        self.items = []

    def push(self, o):
        self.items.append(o)

    def pop(self, n=1):
        while n > 1:
            self.items.pop()
            n -= 1
        return self.items.pop()

    def peek(self, n=1):
        return self.items[-n] if len(self.items) >= n else 'null'


def extract_info(row):
    """
    :param row: data extracted from the csv file
    :return: level (number), the name of the category (string), category ID (number)

    Steps through the csv row until the first populated field (category name)
    and then returns the nesting level, category name and ID (which is always in
    the last field of the row.
    """
    level = 0
    for field in row:
        if field:
            return level, field, row[-1]
        level += 1


id_stack = Stack()

with open('UK_New_Structure_(Oct2015).csv', 'rb') as csvfile:
    rdr = csv.reader(csvfile, delimiter=',', quotechar='"')
    # Skip three header rows
    next(rdr)
    next(rdr)
    next(rdr)
    first_row = next(rdr)
    parent_id = 'null'  # top level categories do not have a parent

    level, name, cat_id = extract_info(first_row)
    print level, name, cat_id, parent_id
    id_stack.push(cat_id)  # Save first parent ID
    last_level = -1

    for row in rdr:
        level, name, cat_id = extract_info(row)

        if level == last_level:  # remove the newly added parent id, last item was not a parent
            id_stack.pop()
            parent_id = id_stack.peek()
            id_stack.push(cat_id)
        elif level < last_level:
            id_stack.pop(last_level - level + 1)
            parent_id = id_stack.peek()
            id_stack.push(cat_id)
            last_level = level
        elif level > last_level:
            parent_id = id_stack.peek()
            id_stack.push(cat_id)
            last_level = level

        print level, '..' * level, name, cat_id, parent_id

I am using Python 2.7.10, but if there are better ways to do this in Python 3, then I'm interested in those as well.

\$\endgroup\$
2
\$\begingroup\$

I don't have many comments regarding your style, as it seems to follow PEP8 (to my knowledge), so lets review the actual code:

  • Implementation of Stack – A straight forward implementation without to much fuzz. You don't have any error checking related to popping something of an empty stack, which might give you a problem at some point
  • Implementation of extract_info – No error handling if faulty input, which could let it return nothing at all if nothing matches on the if field: line
  • rdr is not a good name – It is not clear what next(rdr) actually does, try finding a better name here
  • Simplify if structure – You repeat statements within the if structure, and these could be moved outside of the if structure, which would help readability. In addition you pop/push too many times. Try the following:
if level <= last_level:
    id_stack.pop(last_level - level + 1)

parent_id = id_stack.peek()

id_stack.push(cat_id)
last_level = level
  • Change to newer print – The currently recommended standard for printing seems to be of the form: print('{} {} {} {} {}'.format(level, '..'*level, name, cat_id, parent_id)). In your case it complicates stuff, but in the long run you can pretty print much better using the newer form and variation thereover

Edit: Original code had a little flaw regarding the parent_id. The now corrected code gives the same output when using the provided CSV file as the output from original code.

\$\endgroup\$
  • \$\begingroup\$ I'd suggest reader as better than rdr and it's still a pretty short name. \$\endgroup\$ – SuperBiasedMan Oct 2 '15 at 9:22
  • \$\begingroup\$ Thanks for your reply. Yes, I should include more error checking but you know how it is when you are writing a quick script to mangle some data :) And thanks for mentioning the print alternative syntax, I wasn't aware of it. That will be useful in larger projects. \$\endgroup\$ – Tony Oct 2 '15 at 21:38
  • \$\begingroup\$ I've been looking at your simplification of the if statements and I think it changes the way the code works. As far as I can see the only statement that can be moved outside the blocks is the assignment last_level = level. In your version the new parent ID is not pushed to the stack if level < last_level. You say I pop/push too many times but because I'm processing the file line-by-line I cannot predict the nesting level of the next category. \$\endgroup\$ – Tony Oct 2 '15 at 22:05
  • \$\begingroup\$ @Tony, I've now corrected the logic, and the new version produces the same output when compared to your original output. Note that when last_level == level the expression last_level - level +1 equals 1, aka the ordinary pop... \$\endgroup\$ – holroy Oct 2 '15 at 23:39
  • \$\begingroup\$ Thanks for the update. As to the last_level - level + 1 there are instances when the level jumps back more than one, see the sample data in my question: "Post-1900" (ID:96763, level 4) is followed by "Antique Furniture" (ID:20091, level: 1) so I need to pop three numbers off the stack. \$\endgroup\$ – Tony Oct 3 '15 at 10:43
1
\$\begingroup\$

You can get the index of your iteration using enumerate instead of manually counting:

for i, field in enumerate(row):
    if field:
        return i, field, row[-1]

Repeating the exact same command is silly, just use a for loop instead. If you ever need to change the amount of lines you skip, this makes it far easier and it's easier to count.

# Skip three header rows
for _ in range(3):
    next(rdr)

Note I'm using _ to indicate that the value is throwaway and I really just want to loop three times.

Try to avoid long inline comments like this:

if level == last_level:  # remove the newly added parent id, last item was not a parent

It's hard to read and harder to edit, splitting over two lines is better in these cases.

if level == last_level:
    # Remove the newly added parent id, last item was not a parent

In your if and elif conditions your second elif should be an else so it's obvious that every case will fit into one of these conditions.

    if level == last_level: 
        id_stack.pop()
        parent_id = id_stack.peek()
        id_stack.push(cat_id)
    elif level < last_level:
        id_stack.pop(last_level - level + 1)
        parent_id = id_stack.peek()
        id_stack.push(cat_id)
        last_level = level
    else:
        parent_id = id_stack.peek()
        id_stack.push(cat_id)
        last_level = level
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, the comment about enumerate is exactly the sort of thing I'm interested to learn. Although I'm not sure the last reorganisation of the if statements is correct; assigning last_level = level before the test for if level < last_level will mean the test is always False. \$\endgroup\$ – Tony Oct 2 '15 at 21:35
  • \$\begingroup\$ @Tony You're right, I misread your code and made assumptions about how it works. I edited that now to remove my point about duplication. \$\endgroup\$ – SuperBiasedMan Oct 5 '15 at 8:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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