Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've written some code to:

  1. Parse an XML file.
  2. Extract some strings.
  3. Make copies of the original XML whose names are suffixed with the extracted strings.

Please review this.

from lxml import etree as ET
import re

def create_paramsuffix_copies(xmlfile):

    NSMAP = {"c": "http://www.copasi.org/static/schema"}

    parsed = ET.parse(xmlfile)

    list_of_params = []

    for a in parsed.xpath("//c:Reaction", namespaces=NSMAP):
        for b in a.xpath(".//c:Constant", namespaces=NSMAP):
            list_of_params.append((a.attrib['name'])+'_'+(b.attrib['name']))

    return list_of_params


def add_suffix(xmlfile, name_suffix):

    parsed = ET.parse(xmlfile) #parse xml

    xmlfile = re.sub('.cps', '', xmlfile) #remove suffix

    parsed.write(xmlfile+'_'+name_suffix+'.cps', 
                 xml_declaration = True,
                 encoding = 'utf-8',
                 method = 'xml')


modelfile = 'ct.cps'

for a in create_paramsuffix_copies(modelfile):
    add_suffix(modelfile,a)
share|improve this question
up vote 7 down vote accepted

Some style changes can happen, most are described here.

  • Using single letter variable names can be hard to understand, and don't explain what the data is well. It is good not to use them.
  • str.format is the best way to put variables into strings. E.G. '{}_{}.cps'.format(xmlfile, name_suffix)
  • CONSTANTS should be all caps. MODEL_FILE = 'ct.cps'
  • When passing 'kwargs' don't put spaces around the = operator. E.G. encoding='utf-8'
  • Use new-lines sparingly.
  • In-line comments are harder to read then comments before the code.
  • Comments should have a space between the '#' and the comment.

It may be good to change create_paramsuffix_copies to a generator, in this case it should be faster than making a list.

def create_paramsuffix_copies(xmlfile):
    NSMAP = {"c": "http://www.copasi.org/static/schema"}
    parsed = ET.parse(xmlfile)
    for a in parsed.xpath("//c:Reaction", namespaces=NSMAP):
        for b in a.xpath(".//c:Constant", namespaces=NSMAP):
            yield '{}_{}'.format(a.attrib['name'], b.attrib['name'])

You could add docstrings to your program so people know what the functions do at a later date.

But it looks really good!

share|improve this answer
    
Thanks for the suggestions - they all sound good - I will implement them! – Charon Jul 7 '15 at 8:56
    
@Charon I'm glad I could help! Also I didn't link to the style guide: python.org/dev/peps/pep-0008 – Joe Wallis Jul 7 '15 at 12:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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