[Biojava-l] Nasty CompoundLocation `issue' -- incompatible change

[Michael Dickson]

See below:

Thomas Down wrote:

>On Fri, Jun 01, 2001 at 07:36:04AM -0400, Michael Dickson wrote:
>
>>I'd like to chime in and say that I'm not crazy about the approach 
>>either.  Static methods on singletons that behave as factories is one 
>>thing but in an implementation class I feel that the contstructor (or a 
>>seperate initializer method in complex cases like this one) are a better 
>>choice. 
>>
>>We get alot of cool stuff from BioJava so its not a major issue but I 
>>did want to weigh in with my opinion.
>>
>
>The issue is that, currently, the CompoundLocation constructor
>requires that the following all be true:
>
>  - The list of Locations is sorted into natural order.
>
>  - They're all contiguous.
> 
>  - They're all non-overlapping.
>
Ahh, I'm not sure I fully understoor.  Yes, I see the problem.  There's 
no way to document the implied contract in the method signature and if 
you sorted it out (sic) in the constructor you might end up creating 
something better done a different way.

>
>
>If any of these aren't true then the constructed object will,
>/silently/ fail to meet the full contract specified by the
>Location interface.  Thus it's too dangerous to be a public
>method.  The two options are:
>
>  - The solution I adopted, of having an n-ary union operator 
>    on LocationTools.
>
>  - Make the constructor perform the sort and block merge
>    itself.
>
Again. I apologize, I should have looked at the code first.  I'd thought 
the static was on CompoundLocation  and not a seperate class.

>
>The code is almost identical, either way.  My main concern with
>putting it on the constructor is that, after sorting and mergin
>a list of locations, you may end up with everything collapsing
>down to a single, contiguous, block.  In this case you don't
>really want a CompoundLocation at all, you want a RangeLocation.
>
>There would also be a performance penalty added to the
>(common) binary union operator.
>
>My real feeling is that CompoundLocation shouldn't be a public
>class at all.  If it disappeared, and you just created them via
>the two union methods on LocationTools, would you feel happier.
>
Yes, that would be preferable.  Ultimately you'd get a location object 
of the appropriate type based one the list of locations you supplied.  
The LocationTools class is being the factory for the appropriate Location.

>
>
>All this said, this really seems like a minor issue -- I was
>just fixing a (rather nasty) bug the way which seemed simplest
>to me.  If there's really demand it's easy enough to change
>it back.
>
It is a minor issue and again I'm sorry.  Serves me right for diving in 
before my morning coffee.

Mike

>
>    Thomas.
>