search

naturalis / supersmart

Self-Updating Platform for the Estimation of Rates of Speciation, Migration And Relationships of Taxa
MIT License
17 stars 5 forks source link

Move code out of App::Cmd classes #57

Open rvosa opened 9 years ago

rvosa commented 9 years ago

The App::Cmd classes should contain much less code. The only logic they should contain is to translate command line arguments into arguments for the service classes and to write the results out. This is because the service classes can be (re-)used in other contexts, such as standalone scripts, whereas the command classes can only be used within the App::Cmd framework. Also, their methods are currently too long: each method should be no longer than one screenful.

rvosa commented 9 years ago

Putting my money where my mouth is, I have now done this for the smrt-utils plot command, such that the Plot.pm class acts purely as an MVC Controller that converts user input into decorations of the Model (an annotated tree), which is then passed on to the View (a template). This design is implemented as of ec53a73f6b0795505cfc67102efc3d64e63a0195

rvosa commented 9 years ago

Likewise, this is now the case for smrt bbinfer, as of 07aeec27f7d732bed96d364bc30e400e6061bec3

hettling commented 9 years ago

This now also happened for smrt taxize, see 3e9e235ab61ed78d52813357753f64ca8629661e

rvosa commented 9 years ago

The two biggest offenders are now smrt align and smrt orthologize. The run methods have grown to hideous sizes and have become way too fragile and hard to fix. Example: it would be nice if both align and orthologize could take a subfolder name where they will dump their output. This sort of seems to work for align but then orthologize appears to crap out. The code is way to spaghetti-ish to try to fix that.

rvosa commented 9 years ago

As of b6daef1571d70b0f370ae7246206a11d95fb88b9 this is now also done for smrt orthologize. The motivation for this was that the basic logic needs to be re-used anyway, namely for superclustering candidate alignments for smrt bbdecompose

rvosa commented 9 years ago

bbmerge is still scarily large, though.

rvosa commented 9 years ago

I have to re-iterate that run() functions really, really, really, cannot grow beyond a screen full, and ideally much less. For example, BBinfer::run() has again gained about 50 lines over the last few months. This is just not an option: we CANNOT have that much logic in the app classes. It results in bugs, incomplete implementations, and untestable code.

rvosa commented 9 years ago

The following classes still have run() functions that are longer than 100 lines (and 100 is already way beyond "a screen full" on our laptops, which would be closer to 50 LOC): BBdecompose, Cladeinfer, Clademerge, Pipeline. http://programmers.stackexchange.com/questions/133404/what-is-the-ideal-length-of-a-method