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> """)

• Mast: Could you add (an anonymized version of) the XML file so we can see whether you have a efficient way of finding what you're looking for? While you're at it, please add sufficient context as to the exact function of your program. What made you write it, what problem does it solve?
• Reza Akraminejad: well its just a parser for specific language. I just want to know my coding style in python is good? its a command named basis and should have one or more members and each member can have 0 or more params and all param nodes should be inside params node
• Laurent LAPORTE: The conversion from string to boolean must not be done with the bool() function like this self.preview=bool(root.attrib['preview']) because any non-empty string is evaluated to True even the "false" string. You can replace by: self.preview = root.attrib['preview']) == "true" since the only possible values are "true" and "false".
• Laurent LAPORTE: Raising Exception is generally considered a bad practice. You should create a subclass of it. It is more difficult to design an exception handler when you get a exception of type Exception.

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')
  

• Laurent LAPORTE: The OP could use self.order = member.attrib.get('order'). That way the order attributes will be initialized with None if the attribute is missing instead of no initialization at all.
• Laurent LAPORTE: Note that __parse__ is not rigorously a magic method but a private one. It looks like a magic method, so it can confuse the developer.
• alecxe: @LaurentLAPORTE you are right, the "confusing" naming is what I meant, I've improved the wording. Thanks!
• MKesper: @LaurentLAPORTE IF you need private methods, call them _method() with exactly one underscore. The command class should have at least one public method that you are calling, in this case parse() might be a good name.