Coding the Python way

Question

I've just spent the last half semester at Uni learning python. I've really enjoyed it, and was hoping for a few tips on how to write more 'pythonic' code.

This is the __init__ class from a recent assignment I did. At the time I wrote it, I was trying to work out how I could re-write this using lambdas, or in a neater, more efficient way, but ran out of time.

def __init__(self, dir):

    def _read_files(_, dir, files):

        for file in files:

            if file == "classes.txt":
                class_list = readtable(dir+"/"+file)
                for item in class_list:
                    Enrol.class_info_dict[item[0]] = item[1:]
                    if item[1] in Enrol.classes_dict:
                        Enrol.classes_dict[item[1]].append(item[0])
                    else:
                        Enrol.classes_dict[item[1]] = [item[0]]

            elif file == "subjects.txt":
                subject_list = readtable(dir+"/"+file)
                for item in subject_list:
                    Enrol.subjects_dict[item[0]] = item[1]

            elif file == "venues.txt":
                venue_list = readtable(dir+"/"+file)
                for item in venue_list:
                    Enrol.venues_dict[item[0]] = item[1:]

            elif file.endswith('.roll'):
                roll_list = readlines(dir+"/"+file)
                file = os.path.splitext(file)[0]
                Enrol.class_roll_dict[file] = roll_list
                for item in roll_list:
                    if item in Enrol.enrolled_dict:
                        Enrol.enrolled_dict[item].append(file)
                    else:
                        Enrol.enrolled_dict[item] = [file]


    try:
        os.path.walk(dir, _read_files, None)
    except:
        print "There was a problem reading the directory"

As you can see, it's a little bulky. If anyone has the time or inclination, I'd really appreciate a few tips on some python best-practices.

Thanks.

Note: this was part of a module called enrol, containing the Enrol class for which this is the __init__ — Aaron Moodie
– Aaron Moodie, Commented May 31, 2010 at 3:49
looks like you got a classmate: stackoverflow.com/questions/2943396/python-need-some-help — Xavier Ho
– Xavier Ho, Commented May 31, 2010 at 12:17
This looks remarkably like stackoverflow.com/questions/2943396/python-need-some-help — johnsyweb
– johnsyweb, Commented May 31, 2010 at 12:19
@Xavier @Johnsyweb, thanks for the heads up. The assignment was due on sunday (30th), but have pulled the code until after marking and notified the lecturer. — Aaron Moodie
– Aaron Moodie, Commented Jun 1, 2010 at 4:47

Donald Miner · Accepted Answer · 2010-05-31 04:00:57Z

5

Couple things that can clean up your code a bit:

Use the dictionary's setdefault. If the key is missing, then it sets it to the default you provide it with, then returns it. Otherwise, it just ignores the 2nd parameter and returns what was in the dictionary. This avoids the clunky if-statements.

Enrol.venues_dict.setdefault(key, []).append(file)

>>> x = {}
>>> x.setdefault(99, []).append(5) 
>>> x.setdefault(99, []).append(6)
>>> x
{99: [5, 6]}
>>> x.setdefault(100, []).append(1)
>>> x
{99: [5, 6], 100: [1]}

The other possibility is to use os.path.join to make file paths. This is safer than just doing string concatenation.

os.path.join(dir, file)

Other than that, looks good in terms of style, IMO.

answered May 31, 2010 at 4:00

Donald Miner

40.1k10 gold badges99 silver badges118 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Aaron Moodie Over a year ago

Thanks, that's a much neater way of implementing if/elif.

Richard Levasseur · Accepted Answer · 2010-05-31 08:53:51Z

3

In addition to orangeoctopus's suggestions to use setdefault, you can refactor the if-else into a dispatcher (the typical replacement for big if-else and switch statements):

# list of 2-tuples: (bool func(string filename), handler_function)
handlers = [
  ((lambda fn: fn == "classes.txt"), HandleClasses),
  ((lambda fn: fn == "subjects.txt"), HandleSubjects),
  ((lambda fn: fn.endswith(".roll")), HandleRoll)
]

then do

for filename in files:
  for matcher, handler in handlers:
    if matcher(filename):
      handler(filename)
      break

edited May 31, 2010 at 8:53

answered May 31, 2010 at 7:18

Richard Levasseur

15k7 gold badges53 silver badges66 bronze badges

3 Comments

Aaron Moodie Over a year ago

Wow, that great! I was looking for a large if-else and switch alternative, and found a few, but nothing this clean. Python just keeps getting better and better ;) Thanks Richard!

Aaron Moodie Over a year ago

just to clarify, HandleClasses, HandleSubjects etc are functions with a filename parameter? And I'd still need them defined in the _read_files function in order to access the dir var?

Richard Levasseur Over a year ago

Yes, correct. You'd need them defined someplace where those symbols are defined. You could do it in __init__ as self.HandlerSubjects, too.

joaquin · Accepted Answer · 2010-05-31 07:05:07Z

2

Another important point if you want to use you script for long (some would say very long) is to not use deprecated functions in new code:

os.path.walk dissapeared in python 3.x. Now you can use os.walk instead. However os.walk is different to os.path.walk : it does not accept a processing function in its signature. So refactoring your code will imply a little bit more than changing names.

answered May 31, 2010 at 7:05

joaquin

86k31 gold badges146 silver badges155 bronze badges

1 Comment

Aaron Moodie Over a year ago

thanks joaquin. Someone else pointed that out to me in another post. Again, I didn't have time to change it before submitting it, but I;ll have a look at that now.

Collectives™ on Stack Overflow

Coding the Python way

3 Answers 3

1 Comment

3 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related