Coding style

[floyd-fuh]

Hi there,

I'm teaching Python classes right now and some of those people will use IBM Websphere with Python. I thought I give it a short look. As the first hit was the IBM homepage I took that version of wsadminlib.py. I use the function "compareIntLists" as a worst-practice example of how not to code Python. The version here on github is a lot better (and even works correctly). Why does IBM still serve this very old and broken version of wsadminlib.py? At the moment I'm very concerned about my students who will use this kind of code. A couple of suggestions:

• Have you thought about not using "global" 15 times?
• Introducing classes (object oriented design) and structuring the code as a module would help.
• Why is there an "eval" call? The function "isDefined" is never called. As a security guy I consider "eval" harmful in general. There are other ways of checking if a variable is defined (try and catch a NameError).
• You have commented code, dead code and unused variables. Why not just remove it (you still have the old code in git)? You can use one of the Python debuggers to see variables when debugging. Would make your code much more readable.
• You have inconsistent docstrings
• Doctests would help a lot. You kind of wrote something like that in the "compareIntLists" function, but it is not a real doctest. Doctests could help you a lot to test code.
• The usage of #endif and #endDef and semicolons makes your code very un-pythonic.
• You use camelCase for functions and variables. That's fine as long as your coding guidelines say it has to be like that (contrary to PEP 8).
• You are string parsing quiet a lot, which makes the code pretty hard to read and is not best-practice design.
• You don't use list comprehensions or generator functions at all. Makes your code again un-pythonic.

As a start I would suggest you run pep8 over the script and fix some of the 6879 issues:

$ pip-2.7 install pep8
$ python -m pep8 wsadminlib.py
...snip...
$ python -m pep8 wsadminlib.py | wc -l
6879

Just my suggestions...

cheers,
floyd

[hkp]

Good suggestions. You can always go hero mode and submit a patch to correct
some of the things you find. Just as an example even of how to address some
of the things you've mentioned.

On Wed, Jun 10, 2015 at 8:28 AM, floyd notifications@github.com wrote:

Hi there,

I'm teaching Python classes right now and some of those people will use
IBM Websphere with Python. I thought I give it a short look. As the first
hit was the IBM homepage I took that version of wsadminlib.py. I use the
function "compareIntLists" as a worst-practice example of how not to code
Python. The version here on github is a lot better (and even works
correctly). Why does IBM still serve this very old and broken version of
wsadminlib.py? At the moment I'm very concerned about my students who will
use this kind of code. A couple of suggestions:

• Have you thought about not using "global" 15 times?
• Introducing classes (object oriented design) and structuring the
  code as a module would help.
• Why is there an "eval" call? The function "isDefined" is never
  called. As a security guy I consider "eval" harmful in general. There are
  other ways of checking if a variable is defined (try and catch a NameError).
• You have commented code, dead code and unused variables. Why not
  just remove it (you still have the old code in git)? You can use one of the
  Python debuggers to see variables when debugging. Would make your code much
  more readable.
• You have inconsistent docstrings
• Doctests would help a lot. You kind of wrote something like that in
  the "compareIntLists" function, but it is not a real doctest. Doctests
  could help you a lot to test code.
• The usage of #endif and #endDef and semicolons makes your code very
  un-pythonic.
• You use camelCase for functions and variables. That's fine as long
  as your coding guidelines say it has to be like that (contrary to PEP 8).
• You are string parsing quiet a lot, which makes the code pretty hard
  to read and is not best-practice design.
• You don't use list comprehensions or generator functions at all.
  Makes your code again un-pythonic.

As a start I would suggest you run pep8 over the script and fix some of
the 6879 issues:

$ pip-2.7 install pep8
$ python -m pep8 wsadminlib.py
...snip...
$ python -m pep8 wsadminlib.py | wc -l
6879

Just my suggestions...

cheers,
floyd

—
Reply to this email directly or view it on GitHub
#7.

[gpoul]

Even though you'll find many things in wsadminlib to improve, and I'd invite you and your students to help out with that, I think it makes sense to point out that the real world is never a best-practice and wsadminlib is a library primarily targeted at testers and system admins and will only be used for configuration-time and never at run-time, so easy comprehensible code and clarity of what is being modified are more important than performance.

[floyd-fuh]

The real world is, that people who use Websphere Python will have to read your code. And I hope you didn't just say that system admins don't need nice to read code. More than 10'000 lines of code in one flat file without classes/hierarchy is a little hard to read. If you really don't want to code object oriented you could even do sub-functions:

def abc():
    def inner_function():
        pass
    inner_function() #call it

As far as I remember, when I make mistakes during configuration-time, that can have pretty bad outcome at run-time.

Easy comprehensible code is exactly what Python is aiming and designed for. If you check my list again, there is not a single point where I'm talking about performance. It's all about readability.

The big problem with your code is that it is very inconsistent. My Python heart would bleed if you would write #endIf and #endDef every time, but ok that would be according to your coding style guide. But it is not consitently used.