Re: [Cdk-devel] last call for review: 3519748 A new flag

[Egon W. <ego...@gm...>]

Hi John,

just some random comments, not meant as arguments...

On Fri, Jun 15, 2012 at 9:13 AM, John May <joh...@gm...> wrote:
> My biggest gripe with this is setting the order to null. I'm of the opinion that you should never use null to represent the absence of a value.

This is actually a deliberate design in the CDK: to use null for unset
values. This was chosen over the alternative of things having UNSET
for all sorts of enums etc.

Not using null for 'unset' everywhere would make the interface complexer...

> Setting the order to null and using a flag would provide more null pointer exceptions and require more defensive and cluttered coding with (!= null) statements. I'm aware the bond order can be null already but this would amplify the problem.

At the same time, to surfaces problems... dedicated UNSET values can
more easily be ignored by code, leading to unsuspected behavior...

> Here's my suggestions.
>
> Set this as a new order, we already do this with the stereo (UP_OR_DOWN) and would make for a more consistent API. We could also have a new UNKNOWN enumeration that would remove all null checking
> on bond order.

This is a very significant rewrite of the CDK, and many algorithms
would need to be updated...

> public enum Order {
>        UNKNOWN,                        <- new
>        SINGLE,
>        SINGLE_OR_DOUBLE,               <- new
>        DOUBLE,
>        TRIPLE,
>        QUADRUPLE
> }

IBond.Order is for fixed bond orders... how should algorithms handle
this. That makes your second proposal more attractive to me...

> another options is to make the order interface and have two types of order?
>
> public interface IOrder;
>
> public enum BasicOrder implements IOrder {
>        SINGLE,
>        DOUBLE,
>        TRIPLE,
>        QUADRUPLE
> }
>
> public enum QueryOrder implements IOrder {
>        SINGLE_OR_DOUBLE
> }

Because in this proposal you clearly have an indication of the query
concept, much like in the IQueryBond...

The patch by Klas takes a middle road, that does not break the current
design, but does introduce the concept. Please review the patch as
such.

Egon

-- 
Dr E.L. Willighagen
Postdoctoral Researcher
Department of Bioinformatics - BiGCaT
Maastricht University (http://www.bigcat.unimaas.nl/)
Homepage: http://egonw.github.com/
LinkedIn: http://se.linkedin.com/in/egonw
Blog: http://chem-bla-ics.blogspot.com/
PubList: http://www.citeulike.org/user/egonw/tag/papers