Parsing XML file containing some events

Question

I'm a beginner and have written my first Python module that parses a XML file containing some events and prints them out to stdout as JSON. I would appreciate some feedback about design, code layout, best practices, what can be optimized and so on.

from __future__ import with_statement import json import xml.etree.ElementTree as etree def _sanitize_string(string): """docstring for _clean_string""" string = string.replace('\n', '').split() return ' '.join(string) class Event(object): """docstring for Event""" def __init__(self, date, event, organizer, time, venue): self.date = date self.event = event self.organizer = organizer self.time = time self.venue = venue def provide_data(self): """docstring for provide_data""" data = {} data['date'] = self.date data['event'] = self.event data['organizer'] = self.organizer data['time'] = self.time data['venue'] = self.venue return data class EventCollection(list): """docstring for EventCollection""" def __init__(self): super(EventCollection, self).__init__([]) def as_json(self): """docstring for as_json""" print json.dumps([event.provide_data() for event in self], sort_keys=True, indent=2) def read_file(event_collection, filename='events.xml'): """docstring for read_file""" try: with open(filename) as f: tree = etree.parse(f) for row in tree.findall('.//row'): # event = [_sanitize_string(node.text) for node in row] # if event[0] and event[0][0].isdigit(): # print event tmpl = [_sanitize_string(node.text) for node in row] if tmpl[0] and tmpl[0][0].isdigit(): event = Event(tmpl.pop(0), tmpl.pop(0), tmpl.pop(0), tmpl.pop(0), tmpl.pop(0)) event_collection.append(event) except IOError as ioerr: print "File Error: %s" % str(ioerr) def main(): """docstring for main""" event_collection = EventCollection() read_file(event_collection) event_collection.as_json() # event_collection.as_csv() if __name__ == '__main__': main() 

Answer 1

from __future__ import with_statement import json import xml.etree.ElementTree as etree def _sanitize_string(string): """docstring for _clean_string""" 

Docstrings should contain a description of the function, not some old name for the function.

 string = string.replace('\n', '').split() 

Do you really want "green\ngrass' to come through as 'greengrass'?

 return ' '.join(string) class Event(object): """docstring for Event""" def __init__(self, date, event, organizer, time, venue): self.date = date self.event = event self.organizer = organizer self.time = time self.venue = venue def provide_data(self): """docstring for provide_data""" data = {} data['date'] = self.date data['event'] = self.event data['organizer'] = self.organizer data['time'] = self.time data['venue'] = self.venue return data 

The class as it stands doesn't really do anything. If this is all its ever going to do, just use a dictionary instead.

class EventCollection(list): """docstring for EventCollection""" def __init__(self): super(EventCollection, self).__init__([]) 

Passing the [] is wasteful, because the empty argument constructor will have the same effect without building a temporary list. There is no need to override the constructor here. The default behavior will build the list just fine.

 def as_json(self): """docstring for as_json""" print json.dumps([event.provide_data() for event in self], sort_keys=True, indent=2) 

This would really be better as a seperate function rather then an entire class. Its misplaced in a list class because its not really dealing with the list per se.

def read_file(event_collection, filename='events.xml'): """docstring for read_file""" try: with open(filename) as f: tree = etree.parse(f) for row in tree.findall('.//row'): # event = [_sanitize_string(node.text) for node in row] # if event[0] and event[0][0].isdigit(): # print event 

Don't keep dead code in comments, delete it.

 tmpl = [_sanitize_string(node.text) for node in row] 

I can't guess what tmpl is supposed to mean. This being xml, does these nodes have tags that you should be checking for correctness?

 if tmpl[0] and tmpl[0][0].isdigit(): 

I have no idea what this test is doing. It should probably be commented to explain the signficance of that first character being a digit.

 event = Event(tmpl.pop(0), tmpl.pop(0), tmpl.pop(0), tmpl.pop(0), tmpl.pop(0)) 

Use: date, event, time, organizer, venue = tmpl event = Event(date, event, time, organizer, venue)

It makes is easier to see what is going on

 event_collection.append(event) except IOError as ioerr: print "File Error: %s" % str(ioerr) 

If you catch an error, simply printing it out is insufficient. You should really either recover or abort. You could call sys.exit() here to shutdown the program. I'd probably rethrow the exception and catch it in my main() function. If the script is supposed to be quick and dirty, just don't catch the exception and let the default handling show the error when it dies.

def main(): """docstring for main""" event_collection = EventCollection() read_file(event_collection) 

Avoid functions which modify arguments. Use a return value. Have read_file create and return event_collection.

 event_collection.as_json() # event_collection.as_csv() if __name__ == '__main__': main() 

** EDIT **

Ways of doing exception handling:

1. Don't. Just take your try and except out. If something goes wrong, your program will die with a stack trace. Not very professional, but in some cases that's just fine

2. Use sys.exit()

   try: do_stuff() except IOError: print "Went wrong" sys.exit() # program will exit here

3. Use your own exception class (my preffered method)

Code:

class UserError(Exception): """ This error is thrown when it the user's fault Or at least the user should see the error """ # later try: do_stuff() except IOError as error: raise UserError(error) # later def main(): try: do_program() except UserError as error: print error 

Its a bit bulky for a simple script like this, but for bigger programs I think its the best solution.

Two other issues:

1. You should really print >> sys.stderr, "your error message" so that your errors go to standard error and not standard out
2. You should also end by calling sys.exit(some non-zero number) when an error happens to indicate failure.