Re: [Tm4j-developers] TopicMapObject.equalsByID - is it really needed?

[Xuân B. <xua...@ba...>]

Hi Conal,

nice to read you. :-)

Conal Tuohy wrote:
> I have an old topicmap provider implementation which no longer links 
> against recent builds of TM4j because of the addition of the 
> equalsByID method to the  TopicMapObject interface.
>
> It is certainly an easy method to implement, but I'm concerned because 
> I don't even understand /why/ the method has been added to 
> TopicMapObject. It appears it may have been a "rough idea" that never 
> really developed? In any case, it seems like a needless complication 
> to me, and I am inclined to remove it. First I thought I would ask 
> about it here.
>
> The point of the method *appears* to be to hide the data type of the 
> object's id, presumably so that it could potentially be something 
> other than a String. But this seems pointless to me, since actually 
> these id properties are constrained to be strings (actually, XML 
> names) by the XML Topic Maps specification. There's no way they could 
> be anything other than strings!
>
> When I looked at each provider's implementation of the TopicMapObject 
> interface in the TM4j source tree, they were all exactly the same:
>
>     public boolean equalsByID(TopicMapObject o) {
> return getID().equals(o.getID());
>     }
>
> That fact in itself is a good indicator that the method doesn't 
> properly belong in the TopicMapProvider interface. If there is no need 
> for different providers to implement the method differently (and 
> indeed the JavaDoc for the interface actually /specifies/ the 
> implementation!), then it belongs in a helper class, or as a protected 
> helper method, but not as a public API.
>
> Each of the tm4j topic map providers had an implementation of 
> TopicMapObject which included this method. These methods were used in 
> at most a few places within that provider's package (e.g. 
> org.tm4j.topicmap.hibernate.TopicImpl calls the method as a helper 
> within isOfType(Topic)). In all of those cases the method could just 
> as well have been protected - it didn't need to be part of the public API.
>
> Even if it were just a protected method of TopicMapObjectImpl (and not 
> required by the TopicMapObject interface) it would seem hardly 
> justifiable because it is just a single line of code, and used only a 
> few times (not used at all in the unified provider!).
>
> Incidentally, I'm not sure if the implementation in the "unified" 
> provider is correct - it's subtly inconsistent with equals().
>
> The only use I could see of the method outside of provider classes was 
> in org.tm4j.topicmap.utils.TopicMapWalker, where it was used just 
> once, and could I think have just as well used equals() or at least 
> getID().equals(o.getID())
>
> So I propose to remove the equalsByID method from the interface, and 
> replace its use in TopicMapWalker with equivalent code comparing id 
> values.
>
> Any objections?
If I remember correctly, I think 
http://tm4j.cvs.sourceforge.net/tm4j/tm4j/src/org/tm4j/topicmap/tmdm/tm4j1/TopicMapObjectImpl.java?revision=1.5&view=markup#l_126 
is the reason for equalsByID(). This provider is a bridge between XML 
Topic Maps (in the style of TM4J 0.9.x) and TMDM (in the style of 
TM4J2). TMDM in general, and the TM4J2-implementation of TMDM in 
particular, does not require a TopicMapObject to have exactly one ID. 
Consequently, getID() is not defined, strictly speaking. If getID() is 
called anyways, some compatibility magic auto-creates an ID if no ID 
cannot be found or somehow inferred. However, this is a kludge (as a 
read-only method has a state-changing behaviour), and should be avoided, 
especially if the only purpose of calling getID() is comparing whether 
two TopicMapObjects are equal in some sense (which should be 
read-only-process in turn). You do not want to have an OutOfMemoryError 
just because you called getID().

Thus, equalsByID() avoids all the performance- and other penalties 
involved with calling getID().

I do not want to make any changes to the TM4J 0.9.x public API (because 
of exactly the reasons of incompatibilities on the side of the users or 
implementors of the API). However, because providing compatibility with 
old users of the old API is the very reason for the existence of the 
bridge between XML Topic Maps and TMDM, and because there are much more 
users than implementors for that API, I think that some sacrifice on the 
part of the implementors of the API (like implementing a new method in 3 
lines) is justified in this case. I also did not know that your provider 
implementation existed. If I did (and I had write-access to your 
implementation), I would have changed yours, too, as I did with the 
other implementations I've been aware of.

It would also be nice if there was something like an 
AbstractTopicMapObject which could be mixed into each of the providers 
implementation such that such a change would only be needed to be done 
once. However, Java does not support mixins or multiple inheritance, and 
thus, new methods in public interfaces cannot have a default 
implementation in every case. However, this should not prevent us from 
providing a decent implementation, it just means a little bit more pain 
and more work on each implementation when refactoring becomes necessary, 
including sudden compile errors when some provider, which should have 
been covered by refactoring, actually has been left out.

So, how do you think now regarding this issue?
>
> Con
ciao,
Xuân. :-)