I welcome this discussion because it addresses some of the issues I had when
starting
contributing to the code.
I'm partially responsible of several convenience functions (IsSulfoneOxygen, to
name one
:), and I would be happy to have a guideline to follow to deal with the issue.
The rationale is: I've been schooled by experienced programmers to keep
functions as
simple as possible in order to limit bugs (like that rock band in which they
paint their
faces black and white).
Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding
features made
it grow to several 'pages' of source. Adding convenience functions was an
attempt to
improve code clarity and reduce chances of bugs.
Although, I agree with Noel in terms of not creating ad-hoc functions which are
used only
once. If there's a way to group them together to keep them isolated from the
main code (or
not public?), I'm fine with it.
I'm a big fan of generalizations over special cases.
A similar thing is happening for the OBBond::IsEster function, but in this case
I don't
know how to deal with new features.
I've tried fixing a possible bug and extend the functionalities, so the code
went from 29
to 296 lines (including some documentation/comments).
In order to cover all esters accordingly to IUPAC (and Wikipedia, of course), I
ended up
in writing extra functions that are called by IsEster itself, but can still be
called
independently:
IsEster:
->IsPhosphateEster
->IsCarboxyEster
->IsThioEster
As a consequence, the ABI would change too, and IsEster now could accept
several extra
boolean flags to deal with these extra features (before it accepted none).
I think these extra functions could be considered convenience functions. In
principle, the
whole code could be in a single, larger function, which will have one
well-defined behavior.
Although, I think this would imply the inability to customize the behavior of
IsEster, and
limit the access to new features.
In a possible scenario, a user could call IsEster(), and then write its own
code to get
discriminate between the different ester types. Since the function already did
the job,
why not make it available directly?
I'm aware that this is a complex reply to what was a simple and well-posed
point, but I
would appreciate to have such aspects clarified to prevent bad coding practices
to sneak
in the project.
It would be great if eventually such guidelines would land in a nice and small
"Guidelines" page for OB developers and contributors.
Thanks!
S
On 05/27/2016 08:24 AM, Noel O'Boyle wrote:
> I was just going to make the point that I'm not too keen on the
> majority of the additions to OBAtom and OBMol, which appear to be
> convenience functions. Consider IsSulfoneOxygen. There are a lot of
> elements out there and a lot of functional groups; we should either
> add them all or add none. If in the end, this function is absolutely
> necessary, surely such convenience functions could live elsewhere
> rather than being in the central classes of the library.
>
> I blame IsHydrogen() - it's a magnet for further "Is"s :-)
>
> - Noel
>
> On 23 May 2016 at 19:06, Geoffrey Hutchison <geoff.hutchi...@gmail.com> wrote:
>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html
>>> It makes a few mistakes here and there but is accurate in general.
>>
>> It's a bit weird about adding optional parameters, e.g.
>> bool Build(OBMol &mol);
>> becomes
>> bool Build(OBMol &mol, bool stereoWarnings = true);
>>
>> This isn't an API issue, since a program build to the first will still
>> recompile. This is an ABI problem, but we've never been too concerned about
>> that from 2.x to 2.y.
>>
>>> I have a few comments to make regarding some of the changes, but will do
>>> that separately.
>>
>> I'm obviously interested in the feedback. It's also useful to make sure that
>> source documentation was added.
>>
>> Thanks,
>> -Geoff
>
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are
> consuming the most bandwidth. Provides multi-vendor support for NetFlow,
> J-Flow, sFlow and other flows. Make informed decisions using capacity
> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
> _______________________________________________
> OpenBabel-Devel mailing list
> OpenBabel-Devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>
--
Stefano Forli, PhD
Assistant Professor of Integrative
Structural and Computational Biology,
Molecular Graphics Laboratory
Dept. of Integrative Structural
and Computational Biology, MB-112A
The Scripps Research Institute
10550 North Torrey Pines Road
La Jolla, CA 92037-1000, USA.
tel: +1 (858)784-2055
fax: +1 (858)784-2860
email: fo...@scripps.edu
http://www.scripps.edu/~forli/
------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
_______________________________________________
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
