On 27/05/16 20:47, Stefano Forli wrote:
> 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.
>
The simplest way around this which would not change the API more than
once is to have a function called
enum FunctionalGroup OBMol::functionalGroup() {}
with
enum { FunctionalGroup_PhosphateEster,
FunctionalGroup_Amine,
FunctionalGroup_Alcohol };
In this manner support for new functional groups can be added under the
hood without changing the interface.
> 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
>>
>
--
David van der Spoel, Ph.D., Professor of Biology
Dept. of Cell & Molec. Biol., Uppsala University.
Box 596, 75124 Uppsala, Sweden. Phone: +46184714205.
sp...@xray.bmc.uu.se http://folding.bmc.uu.se
------------------------------------------------------------------------------
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
