Parsing XML commands and parameters

Question

I'm C# programmer and started Python recently for a new project.

This is a part of my project, and I want your opinion about my code and if it's clean or dirty, too long, has duplicate code, or it's good and standard.

(cmd= command ... at the end of file is just for class testing)

import xml.etree.ElementTree as ET

#TODO: processing attributes should be case insencitive

class command:
    def __init__ (self,htmlcommand):
        self.preview=False
        self.members=[]
        self.__parse__(htmlcommand)        



    def __parse__(self,htmlcommand):
        root = ET.fromstring(htmlcommand)
        self.service=root.attrib['service']
        self.name=root.attrib['name']
        self.source=root.attrib['source']        
        self.preview=bool(root.attrib['preview'])
        self.procedurename=root.attrib['procedurename']  

        mems=root.findall('member')
        if(len(mems)==0):
            raise Exception('command has no member')
        for m in mems:            
            self.members.append(member(m))

class member:
    def __init__ (self,member):
        self.params=[]
        self.__parse__(member)

    def __parse__(self,member):                            
        self.name=member.attrib['name']
        self.method=member.attrib['method']        
        try: 
            self.order=member.attrib['order']
        except:
            pass
        try:
            self.sort=member.attrib['sort']        
        except:
            pass
        self.__parseparams__(member)

    def __parseparams__(self,member):
        params=member.findall('./params/param')        
        for p in params:
            self.params.append(param(p))    

    #def __getparams(member):

class param:
    def __init__(self,param):        
        self.name=param.attrib['name']
        self.value=param.attrib['value']

cmd= command("""<basis core="external.ws.ws" service="bimepasargad" content="33,444" name="Internalissuance" source="cmsDbService" procedurename="sbserviceprocedure2" preview="true">
            <member name="bimepasargadInternalIssuance" method="internalissuance" sort="random">
                <params>
                    <param name="cms.query.token" value="111"/>
                    <param name="duration" value="2222"/>
                </params>
            </member>
            <member name="2" method="2" sort="random">
                <params>
                    <param name="cms.query.token" value="3"/>       
                </params>
            </member>
        </basis> """)

Answer 1

Python naming conventions

• module_name: name of a Python module = name of the file module_name.py without extension;
• package_name: name of a Python package (a directory with a __init__.py file) = name of the directory;
• global_variable/local_variable/attribute_name: name of a global variable (module-level variable), local variable (function variable), class variable (ie: ClassName.attribute_name), or instance variable (ie: self.attribute_name)
• A_CONSTANT: name of a "constant" (read-only by convention), also used for class, function and instance.
• function_name/method_name: name of a function (or method);
• ClassName: name of a class.

In Python, there is no access restriction, everything is public, but we use to have the following rules:

• _protected_name: we add a single underscore to mark a module/package/variable/class/function/method/attribute protected,
• __private_name: we add a double underscore to mark a module/package/variable/class/function/method/attribute private.

note: Internally, Python will rename the private methods and attributes to make them a little harder to used from outside the class. We call that "name mangling". Consult the Python documentation: 9.6. Private Variables

Example:

class MyClass:
    def __init__(self):
        self.hidden = "Me"

    def __method(self):
        print(self.hidden)

    def show(self):
        self.__method()


a = MyClass()
a.show()
# -> Me

a._MyClass__method()  # name mangling
# -> Me

a.__method()
# -> AttributeError: 'MyClass' object has no attribute '__method'

Private variables are not very popular in Python.

Magic methods

Magic methods, in Python, are methods which name is surrounded by double underscores ("dunder"). There are used to define operators (__eq__, __add__, ...), special methods (__init__, __new__, ...), protocols (__enter__ / __exit__), etc.

There is a great article on GitHub Magic Methods by Gabriel NIEBLER and ali.

Code Style Review

PEP 8 coding style violation (see the Style Guide for Python Code

• No white space before '(' or after ')'
• Missing white space after ','
• missing whitespace around operator: "=", "+", "-", "/", "*", "%", etc.
• Expecting 1 blank lines between methods
• Expecting 2 blank lines between classes and functions

Also, you can fix spelling of "htmlcommand" => "html_command" for better maintenance.

Your constructor could be re-written like this:

class Command(object):
    def __init__(self, html_command):
        self.preview = False
        self.members = []
        self._parse(html_command)

Note: If you are using Python 2.7, you should inherit the object class to used new style classes/object model.

Best practices

It is a usually (but not always) considered a best practice to define all your attributes in the constructor:

def __init__(self, html_command):
    self.service = None
    self.name = None
    self.source = None
    self.preview = None
    self.procedure_name = None
    self.members = None
    self._parse(html_command)

As I told you, it's better to used your own exception class, for instance:

class InvalidCommandException(Exception):
    pass

That way, the user of your library can write it's own exception handler:

html_command = "..."
try:
    command = Command(html_command)
except InvalidCommandException as exc:
    print("Error: {0}".format(exc))

This avoid catching all Exception...

For that, you could define the _parse() method like this:

def _parse(self, html_command):
    root = ET.fromstring(html_command)
    self.service = root.attrib['service']
    self.name = root.attrib['name']
    self.source = root.attrib['source']
    self.preview = root.attrib['preview'] == "true"
    self.procedure_name = root.attrib['procedurename']
    members = root.findall('member')
    if not members:
        raise InvalidCommandException('command has no member')
    self.members = [Member(m) for m in members]

You can notice the used of a comprehension list to fill the self.members attribute. It generally more efficient that a loop with append().

note: if an attribute is missing, the KeyError exception will be raised. If you want a different behavior an raise your own exception, you need to wrap the parse function into a exception manager:

    try:
        self.service = root.attrib['service']
        self.name = root.attrib['name']
        self.source = root.attrib['source']
        self.preview = root.attrib['preview'] == "true"
        self.procedure_name = root.attrib['procedurename']
    except KeyError as exc:
        raise InvalidCommandException("Missing attribute: {key}".format(key=str(exc)))

For Member class, we can simplify a lot. Since the _parse() method is only used in the constructor, why not use it's implementation in it?

class Member(object):
    def __init__(self, member):
        self.name = member.attrib['name']
        self.method = member.attrib['method']
        self.order = member.attrib.get('order')
        self.sort = member.attrib.get('sort')
        params = member.findall('./params/param')
        self.params = [Param(p) for p in params]

Of course, we can do that for the other classes too.

You can use get() to retrieve optional attributes.

Other remarks

I noticed that all classes except Command use a etree node as constructor parameter. So, I think, it's better to change the signature of the Command constructor to accept a etree node too:

You could also add a __repr__ method to help debugging:

def __repr__(self):
    cls = self.__class__.__name__
    fmt = "{cls}(name={name!r})"
    return fmt.format(cls=cls, name=self.name)

The result

import xml.etree.ElementTree as ET


class InvalidCommandException(Exception):
    pass


class Command(object):
    def __init__(self, command):
        try:
            self.service = command.attrib['service']
            self.name = command.attrib['name']
            self.source = command.attrib['source']
            self.preview = command.attrib['preview'] == "true"
            self.procedure_name = command.attrib['procedurename']
        except KeyError as exc:
            raise InvalidCommandException("Missing attribute: {key}".format(key=str(exc)))
        members = command.findall('member')
        if not members:
            raise InvalidCommandException('Command has no member')
        self.members = [Member(m) for m in members]

    def __repr__(self):
        cls = self.__class__.__name__
        fmt = "{cls}(name={name!r})"
        return fmt.format(cls=cls, name=self.name)


class Member(object):
    def __init__(self, member):
        try:
            self.name = member.attrib['name']
            self.method = member.attrib['method']
            self.order = member.attrib.get('order')
            self.sort = member.attrib.get('sort')
        except KeyError as exc:
            raise InvalidCommandException("Missing attribute: {key}".format(key=str(exc)))
        params = member.findall('./params/param')
        self.params = [Param(p) for p in params]

    def __repr__(self):
        cls = self.__class__.__name__
        fmt = "{cls}(name={name!r})"
        return fmt.format(cls=cls, name=self.name)


class Param(object):
    def __init__(self, param):
        try:
            self.name = param.attrib['name']
            self.value = param.attrib['value']
        except KeyError as exc:
            raise InvalidCommandException("Missing attribute: {key}".format(key=str(exc)))

    def __repr__(self):
        cls = self.__class__.__name__
        fmt = "{cls}(name={name!r}, value={value!r})"
        return fmt.format(cls=cls, name=self.name, value=self.value)


def main():
    html_command = """\
    <basis core="external.ws.ws" service="bimepasargad"
           content="33,444" name="Internalissuance"
           source="cmsDbService" procedurename="sbserviceprocedure2"
           preview="true">
        <member name="bimepasargadInternalIssuance"
        method="internalissuance" sort="random">
            <params>
                <param name="cms.query.token" value="111"/>
                <param name="duration" value="2222"/>
            </params>
        </member>
        <member name="2" method="2" sort="random">
            <params>
                <param name="cms.query.token" value="3"/>
            </params>
        </member>
    </basis>"""
    root = ET.fromstring(html_command)

    try:
        command = Command(root)
        print(command)
        print(command.members)
        print(command.members[0].params)
    except InvalidCommandException as exc:
        print("Error: {0}".format(exc))


if __name__ == '__main__':
    main()

output

Command(name='Internalissuance')
[Member(name='bimepasargadInternalIssuance'), Member(name='2')]
[Param(name='cms.query.token', value='111'), Param(name='duration', value='2222')]

Answer 2

Multiple things to fix:

• variable naming - class names should be in camel-case and start with a capital letter (reference); when there are multiple words in a variable, you should put underscore between them, e.g. in your case procedurename would become procedure_name

• missing spaces - there should be spaces around operators, like around the = here:

  self.name = param.attrib['name']
  

• incorrect private method naming - when you put double underscores around a method name (this is called "dunder"), you are implying that it is a "magic method" which may confuse future readers of your code (which can also be you); while, I think you meant to create a private method. In other words, naming the method __parse, not __parse__

• if(len(mems)==0): can be simplified as if not mems:

• bare except clause should be avoided, either replace with handling a more specific error type:

  try: 
      self.order=member.attrib['order']
  except:
      pass
  

  with:

  try: 
      self.order = member.attrib['order']
  except KeyError:
      pass
  

  Or, follow the @LaurentLAPORTE's advice and use the .get() method to set the self.order to None if the "order" attribute does not exist:

  self.order = member.attrib.get('order')