search

ostrokach / cinfony

Automatically exported from code.google.com/p/cinfony
1 stars 1 forks source link

[Suggestion] Code de-duplication #9

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
While looking at cinfony's source code, I've seen a lot of duplicated code, 
like the _findbits function and parts and methods from classes which are the 
same in most or all the cinfony modules. 

This approach works, but I think that a common module, imported by all cinfony 
modules and containing all the tiny common bits and base classes would improve 
cinfony's maintainability. I would also make it easier, or more 
straightforward, to implement modules for new backends.

So, with that common base module, all other modules would only have 
backend-specific code, subclassing the base classes from the common module and 
re-defining methods and attributes when needed.

Just an idea, though.

Original issue reported on code.google.com by ssorga...@gmail.com on 7 Jan 2012 at 12:45

GoogleCodeExporter commented 9 years ago 
Today I was playing with the idea. I wrote a template module:
https://github.com/ssorgatem/cinfony/blob/dedup/cinfony/template.py

Which can probably still improve a lot.

Then I adapted rdk.py to use the template module, and the result is:
https://github.com/ssorgatem/cinfony/blob/dedup/cinfony/rdk.py

It spares 59 lines of duplicated code, and it seems to work exactly as before.

If the rest of modules get similar result in decrease of lines, we may be 
talking about removing +600 lines of duplicated code.

Original comment by ssorga...@gmail.com on 9 Jan 2012 at 8:50

GoogleCodeExporter commented 9 years ago 
It's an interesting idea, but don't do too much work on it. There hasn't been 
any issue with Cinfony's maintainability in this respect. Also there is an 
advantage to encapsulating all of the functionality into a single Python file; 
for example, it makes it easier to distribute with other projects (think Pybel 
in Open Babel).

Original comment by baoille...@gmail.com on 10 Jan 2012 at 11:12

GoogleCodeExporter commented 9 years ago

Original comment by baoille...@gmail.com on 10 Jan 2012 at 11:13

GoogleCodeExporter commented 9 years ago 
Yes, I was aware of the pybel case, but I think that including an extra file is 
not very difficult, IMHO.

But i think there's still place for improvement, even without the 'template' 
module: for example merging cdkjpype and cdkjython to get something similar to 
indy. I think this could be interesting for obabel too.

Original comment by ssorga...@gmail.com on 10 Jan 2012 at 11:24

GoogleCodeExporter commented 9 years ago 
Merging I would be more interested in. With indy (and OSCAR) the differences 
were very small. With CDK (and Open Babel) there will be quite a few 
differences, so it'll get a bit ugly. Is this a reasonable price to pay?

Original comment by baoille...@gmail.com on 10 Jan 2012 at 11:46

GoogleCodeExporter commented 9 years ago 
I might experiment with cdk, let's see what happens.

Original comment by ssorga...@gmail.com on 10 Jan 2012 at 11:57

GoogleCodeExporter commented 9 years ago 
And this has happened:
https://github.com/cinfony/cinfony/pull/2

There don't seem to be many differences, most of them are solved at the 
beggining of the module (imports and so), and the other big if'd chunk are 
Molecule.draw() and _Canvas. I made Molecule.draw() accept the usual arguments 
when running in jpype, but just ignoring them, so that the API remains 
compatible.

Also, there seemed to be a lot of legacy imports...

Original comment by ssorga...@gmail.com on 10 Jan 2012 at 10:46