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')]
bool()
function like thisself.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". \$\endgroup\$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 typeException
. \$\endgroup\$