rebeccaroisin/dgparse — pull request #1 by bmcorser (2014-11-21 16:18:40+00:00)
Title: Review

# Introduction

There's now a CLI which can be "installed" with `python setup.py develop` and invoked with `snapgene-json path/to/file.dna`

# Review

## Good things
It works!

### Readability
[Defining the schema](https://github.com/rebeccaroisin/dgparse/blob/9b666598ee148e135ac377540a3e532a30c2c168/dgparse/snapgene.py#L115-L126) of a complex return type at the head of a function is a great idea and a boon to readability. The plain-English definition of these in documentation was also somewhat useful.

### Structure
The structure is generally acceptable; aside from the improper use of `class` (outlined below) and lack of modularity -- but I'm a weeny on that count.

### Tests
It's great to see a comprehensive integration level test suite.

## Bad things
### Readability
Initially glancing over this code, there remain linting and readability issues (hence the suggested reading) which should be addressed. It's probably worth noting at this point that PEP8 compliance is not the same thing as readability; linting code to make the `pep8` program happy doesn't speak to whether or not
humans will find it readable.

### Code smells
The module contains "bare" logic, which in [this case](https://github.com/rebeccaroisin/dgparse/blob/9b666598ee148e135ac377540a3e532a30c2c168/dgparse/snapgene.py#L339-L356) is setting up class constants which is fine, but this is can be a Bad Idea.

There seems to be some misunderstanding of what the `__init__` method of a Python class is for. It's a constructor method which is there to encapsulate any logic relating to the construction of properties from arguments. It shouldn't return anything. Generally, it should only bind values. The [`__call__`](http://stackoverflow.com/a/22879086/3075972) method might be interesting.

- 116a896 This should either be invoked through a [console script entry point](http://calvinx.com/2012/09/09/python-packaging-define-an-entry-point-for-console-commands/) or with the Python interpreter. Either way, it doesn't need to be executable.
- 62ab15b Vague variable names like `data` aren't very helpful.
- e419be7 Unused imports kicking around
- d908a70 A mixture of naming conventions applied

### Structure
The structure of the code is somewhat confusing. This is not helped by the readablity issues, ie. 4c2927e. To take advantage of the structural clarity that comes with using classes in OOP, all the functions defined in this module should be instance methods, which would mitigate bizarre invocations such as [this](https://github.com/rebeccaroisin/dgparse/blob/9b666598ee148e135ac377540a3e532a30c2c168/dgparse/snapgene.py#L377) (even old-style classes had a concept of `self`). However, since the `snapgene` class defined in the `snapgene` module doesn't do anything that couldn't be done in a function and way the class is used makes it not really be a class, I would propose a different approach.

#### 21d8c87
The mapping of byte-values to functions could be done explicitly at module-level. This mapping is never referred to on an instance of the "class" anyway, so this would have no change to the functionality of the code. The same goes for the mapping of byte-values to segment names.

#### bf74fbb
Once the two mappings that were bound to the class namespace are freed, the class is rendered useless and should become a function.

#### acf8f60
Now we're just left with a module full of functions, which doesn't immediately tell us anything about the relationship between them. It makes sense to group functions. I can see two obvious groupings that could be an aid to comprehension of this code: segment parsers and function mappers. This can be done by breaking up the module into a [package](http://stackoverflow.com/a/7948504/3075972) containing smaller [modules](http://stackoverflow.com/a/7948504/3075972).

### Phew!
Any questions, please add comments here or drop an email.