Hi Stefano, Short functions are good - I should really be better at this - but there is no need to add all of your functions to the public API. From what you say, it sounds like this was unintentional. In that case, is it okay if I change things to remove these utility functions from the public API? This will involve moving them from member functions of OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the same.
Regards, - Noel On 27 May 2016 at 19:47, Stefano Forli <fo...@scripps.edu> 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. > > 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
