current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
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
up vote 1 down vote favorite

ok spent ages on this simple problem (I'm not good at XML) below code is probably self explanatory and it works, but i think this could be done a lot easier

Sample XML

<data>
        <id name="ch01">
                <channel>test channel 1</channel>
                <gpio>3</gpio>
                <cont>front</cont>
        </id>

        <id name="ch02">
                <channel>test channel 2</channel>
                <gpio>5</gpio>
                <cont>back</cont>
        </id>
</data>

The code used, which I eventually got working after reading a few posts on here:

from xml.dom import minidom
dom = minidom.parse('/usr/local/sbin/logger/chaninfo_test.xml')
id=dom.getElementsByTagName('id')
for node in id:
        chanid=node.getAttribute('name')
        print chanid
        clist=node.getElementsByTagName('channel')
        for c in clist:
                print c.childNodes[0].nodeValue

        glist=node.getElementsByTagName('gpio')
        for g in glist:
                print g.childNodes[0].nodeValue

        colist=node.getElementsByTagName('cont')
        for co in colist:
                print co.childNodes[0].nodeValue
        print

Can this be improved or cleaned up? The XML will increase in size but contents won't change, multiple IDs with one channel, gpis and cont field each.

The first thing that I'm thinking about is combining is and name is channel id='name', etc.

share|improve this question
    
Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. – SuperBiasedMan Mar 9 at 16:42
    
As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. – Jamal♦ Mar 15 at 10:11
up vote 1 down vote

Use for in loops to avoid repetition

These 3 blocks of code are very similar:

1

    clist=node.getElementsByTagName('channel')
    for c in clist:
            print c.childNodes[0].nodeValue

2

    glist=node.getElementsByTagName('gpio')
    for g in glist:
            print g.childNodes[0].nodeValue

3

    colist=node.getElementsByTagName('cont')
    for co in colist:
            print co.childNodes[0].nodeValue

We can clean up by abstracting what is different (the name) into a loop variable:

for name in ('channel', 'gpio', 'cont'):
    for element in node.getElementsByTagName(name):
        print element.childNodes[0].nodeValue

Now also adding another name will lead to only adding it to the list instead of repeating similar code the fourth time.

share|improve this answer
up vote 0 down vote

Yes, it definitely can be improved, therefore I suggest you two libraries

If you want to stick with xml format, try xmltodict library.

It will create a dictionary tree of the elements on your xml which you should be able to access more easily

The second option is using YAML format instead of XML, which will improve readability since you'll end with something like this

data:
  - id: ch01
    channel: test channel 1
    gpio: 3
    cont: front
  - id: ch02
    channel: test channel 2
    gpio: 5
    cont: back

Links to both options

https://github.com/martinblech/xmltodict

http://pyyaml.org/wiki/PyYAMLDocumentation

share|improve this answer

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.