6
\$\begingroup\$

This is a small web application that I have created using the Flask framework.

I am looking for best practices feedback, as well as design pointers. I am not confident that my design choices have been very good, or rather that they were the best they could have been, but I would like some feedback regardless on where I can improve and expand as I'm new to Python.

To begin, I'd like to review where I process my XML configuration file. The file is validated, and then a database is populated with the information. I feel like it could be improved, however, not included is validation and safety-checking of the user-supplied (really admin supplied) HTML from the XML doc.

from lxml import etree
import time, datetime
from time import mktime
from contestassist.database import db_session
from contestassist.contest import Contest
from contestassist.question import Question
from contestassist.config import Config
import sqlalchemy
import sys

class XMLParser:

    def __init__(self, xml_file):
        self.xml_file = xml_file
        self.time_format = "%Y-%m-%d %H:%M:%S"

    def ProcessTemplate(self):

        #parser = etree.XMLParser(dtd_validation=True)
        parser = etree.XMLParser()
        xmldoc = etree.parse(self.xml_file, parser)
        contests = xmldoc.getroot()

        #validate against DTD
        dtd = open(Config.DTDPATH, 'r')
        dtdObj = etree.DTD(dtd)
        if dtdObj.validate(contests) is False:
            print "Supplied XML file is not correctly formatted"
            print(dtdObj.error_log.filter_from_errors()[0])
            sys.exit()
        for contest in contests.getchildren():
            contest_id = self.parseContestInfo(contest)

            self.parseContestQuestions(contest, contest_id)

    def parseContestInfo(self, contestNodes):

        title = contestNodes.xpath('title')
        self.title_data = title[0].text

        description = contestNodes.xpath('description')
        self.description_data = description[0].text

        #Here we carefully extract the time from our xml node into a datetime object
        #in a couple steps. First we get the string from the node, then a time struct using strptime and a format string
        #then we create a datetime from the struct.
        start = contestNodes.xpath('start')
        start_node = start[0].text
        t_struct = time.strptime(start_node, self.time_format)
        self.start_datetime = datetime.datetime.fromtimestamp(mktime(t_struct))

        end = contestNodes.xpath('end')
        end_node = end[0].text
        t_struct = time.strptime(end_node, self.time_format)
        self.end_datetime = datetime.datetime.fromtimestamp(mktime(t_struct))

        #create contest and insert it into the database
        contest = Contest(self.title_data, self.description_data, self.end_datetime, self.start_datetime)
        try:
            db_session.add(contest)
            db_session.commit()
        except sqlalchemy.exc.IntegrityError, exc:
            reason = exc.message
            if reason.endswith('is not unique'):
                print "%s already exists" % exc.params[0]
                db_session.rollback()
        #return the ID of this contest, so we can use it in the next function
        return contest.id

    def parseContestQuestions(self, contestNodes, contest_id):
        #get a list of questions for our contest
        questions = contestNodes.xpath('questions/question')

        #loop through these questions, grab the appropriate data
        #and then create question entries in the database
        for question in questions:
            points_temp = question.xpath('point-value')
            p_value = points_temp[0].text

            text_temp = question.xpath('question-text')
            q_text_value = text_temp[0].text

            answer_temp = question.xpath('question-answer')
            q_answer = answer_temp[0].text

            #create question object, and attach it to the contest with the id contest_id
            question = Question(p_value, q_text_value, q_answer, contest_id)
            try:
                db_session.add(question)
                db_session.commit()
            except sqlalchemy.exc.IntegrityError, exc:
                reason = exc.message
                if reason.endswith('is not unique'):
                    print "%s already exists" % exc.params[0]
                    db_session.rollback()

I have a couple areas where I'd like to focus on:

  1. user.py - Here, I'd like some critique on my class design choices. This class is used to store user information in the database. It provides some utility functions specific to the Flask framework and its plug-ins. It works with SQL Alchemy, so I can retain user info in a database. Am I storing user credentials poorly? How do the authenticate/name_exists/email_exists functions look?
  2. flaskapp.py - this is where I do my app's route handling, build/process forms, etc. Most views are rendered with a list of dictionaries as an argument (containing for example questions, users, etc as part of the application). Is that bad design for a web application? Do I process forms in a decent manner? is there a way I can better organize all these routes? This file got sort of big fast.

Any feedback is appreciated. I would love to hear feedback in any areas, not necessarily what is named in the tags.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Opening files using with

It's not a great idea to open a file just using something like this:

dtd = open(Config.DTDPATH, 'r')

If your program unexpectedly exits, or crashes, resources that are taken up by the opened file aren't freed. This can be bad in some situations. The proper way to open a file would to be using a context manager, or with, like this:

with open(Config.DTDPATH, "r") as dtd:
   ...

By using a context manager, resources will be properly freed even if your program crashes or unexpectedly exits.


Explicit inheritance from object

In addition, classes in pre-Python 3.x should explicitly inherit from object, like this:

class XmlParser(object):
    ...

When your class doesn't you can get weird behavior like this:

>>> class Spam: pass
...
>>> type(Spam())
<type 'instance'>

Versus normal behavior like this:

>>> class Spam(object): pass
...
>>> type(Spam())
<class '__main__.Spam'>

Nitpicks/Style

Python has an official style guide, PEP8. I'd highly recommend that you read it, as it has some very valuable information. You do have a few style violations though, so here's a small list of them:

  • Function names should be in snake_case, not PascalCase, or camelCase.
  • Variable names should be in snake_case as well.
  • Constants should be in UPPER_SNAKE_CASE
  • The XMLParser class should be named XmlParser.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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