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_statementimport jsonimport xml.etree.ElementTree as etreedef _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 dataclass 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()
Parsing XML file containing some events
python;beginner;parsing;json;xml
from __future__ import with_statementimport jsonimport xml.etree.ElementTree as etreedef _sanitize_string(string):    docstring for _clean_stringDocstrings 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 dataThe 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 eventDon'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: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 fineUse sys.exit()try:     do_stuff()except IOError:     print Went wrong     sys.exit() # program will exit hereUse 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     # latertry:     do_stuff()except IOError as error:     raise UserError(error)# laterdef main():    try:         do_program()    except UserError as error:         print errorIts a bit bulky for a simple script like this, but for bigger programs I think its the best solution.Two other issues:You should really print >> sys.stderr, your error message so that your errors go to standard error and not standard outYou should also end by calling sys.exit(some non-zero number) when an error happens to indicate failure.