Code review request for the `gamessTest` branch

[GoogleCodeExporter]

Purpose of code changes on this branch:
First beta version of GAMESS Test Suite is out.

When reviewing my code changes, please focus on:
the overall design, missing features, etc.

After the review, I'll merge this branch into:
/trunk/gc3apps/gamess

Original issue reported on code.google.com by lukasz.m...@gmail.com on 25 Sep 2012 at 11:26

[GoogleCodeExporter]

I'll gladly do this, but I won't have time until next week when the GC3Pie 
training is over.

Original comment by riccardo.murri@gmail.com on 25 Sep 2012 at 7:57

[GoogleCodeExporter]

My first iteration on this:

1) please reduce the number of files and the amount of code to be
   reviewed to the minimum: as far as I can tell, the `testLines.py`
   file is never called so please remove it.

   Also, there might be an opportunity to compact the code: I see no
   reason to have three separate modules for something that is
   basically a glorified "grep".

2) Please revise the code to match the coding style used elsewhere in
   GC3Pie; the GC3Pie developer documentation has guidelines about this.

3) The `ggamess.py` looks mostly OK to me.

4) The `testScenarios.py` and `testLines.py` do not however: I find
   the code to be over-complicated, and it has a few issues here and
   there.  For example:

   * You are using old-style classes, i.e., "class Test:" instead of
     "class Test(object)" -- old-style classes are deprecated and
     should not be used in new Python code.

   * The classes in `testLines.py` are initializing variables as
     class-level variables (that would be `static` in Java), whereas
     you probably meant to have instance-level variables.

   * Do not use "magical constants" like `99` to mean
     "uninitialized/invalid number".  It may well happen that a file
     has 99 or more lines!! If you need that, use Python's `None`
     constant (Python's variables are not typed!)

   * I see no reason to use dynamic dispatch (getattr) with function
     names that do not change (the `label_follow` and `label_analyze`
     are actually constants). It makes the code unnecessary
     complicated.

   * More generally, I see no reason to have two distinct code paths
     for the `grepAndAnalyze` and `grepAndFollow`: the former is just
     a special case of the latter and it should be treated as such.

Some of these issues were already pointed out in earlier code reviews;
please cross-check with those as well before you re-submit for another
review.

Original comment by riccardo.murri@gmail.com on 7 Oct 2012 at 11:14

• Changed title: Code review request for the gamessTest branch
• Changed state: Started
• Added labels: Application-GAMESS, Script-GGamess

[GoogleCodeExporter]

1) please reduce the number of files and the amount of code to be
   reviewed to the minimum: as far as I can tell, the `testLines.py`
   file is never called so please remove it.

I agree. I removed testLines.py. 

   Also, there might be an opportunity to compact the code: I see no
   reason to have three separate modules for something that is
   basically a glorified "grep".

I plan to create a special class grepUtils.py that will support TestSuite as 
well as other cases where extraction of any line & value from GAMESS is 
important. Such module can be utilized elsewhere as well and put perhaps to 
GC3Libs?

2) Please revise the code to match the coding style used elsewhere in
   GC3Pie; the GC3Pie developer documentation has guidelines about this.

3) The `ggamess.py` looks mostly OK to me.

4) The `testScenarios.py` and `testLines.py` do not however: I find
   the code to be over-complicated, and it has a few issues here and
   there.  For example:

LM: testLines.py has been put to Test classes inside testScenarios.py

   * You are using old-style classes, i.e., "class Test:" instead of
     "class Test(object)" -- old-style classes are deprecated and
     should not be used in new Python code.

I fixed it.

   * The classes in `testLines.py` are initializing variables as
     class-level variables (that would be `static` in Java), whereas
     you probably meant to have instance-level variables.
I renamed the function to init if this is what you meant. From 
http://www.leepoint.net/notes-java/data/variables/45local-inst-class.html
I can see I can initialize instance-level variables in such a function, can't I?

   * Do not use "magical constants" like `99` to mean
     "uninitialized/invalid number".  It may well happen that a file
     has 99 or more lines!! If you need that, use Python's `None`
     constant (Python's variables are not typed!)
It's not 'None'. I wanted to encode last with integer. If you know another 
trick just let me know :) this number should not be a positive integer.

   * I see no reason to use dynamic dispatch (getattr) with function
     names that do not change (the `label_follow` and `label_analyze`
     are actually constants). It makes the code unnecessary
     complicated.
I changed it into:
app = TestNextLine(filename_out)
app.init(...)

   * More generally, I see no reason to have two distinct code paths
     for the `grepAndAnalyze` and `grepAndFollow`: the former is just
     a special case of the latter and it should be treated as such.

True, but they do different completely things and should be kept separately 
(OOP principle). We can keep the design that whenever a new test case is 
introduced it gets defined in a separate class. Does it make sense?

Some of these issues were already pointed out in earlier code reviews;
please cross-check with those as well before you re-submit for another
review.

I will. Thanks.

Original comment by lukasz.m...@gmail.com on 23 Oct 2012 at 5:05