jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tobias Bocanegra <tri...@apache.org>
Subject Re: svn commit: r1532157 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Date Tue, 15 Oct 2013 19:11:42 GMT
Hi,

On Tue, Oct 15, 2013 at 12:05 PM, Jukka Zitting <jukka.zitting@gmail.com> wrote:
> Hi,
>
> On Tue, Oct 15, 2013 at 2:47 PM, Tobias Bocanegra <tripod@apache.org> wrote:
>> Sure, but in this particular case I think that comparing the actual
>> definition is desired. Imagine a NodeType editor that operates on a session
>> and wants to list the nodetypes that were modified.
>
> Makes sense.
>
>> OTOH, in most cases comparing the names should be good enough. If you think
>> the comparing the CND is overhead, please re-open [0] and I'll change it.
>
> CND comparison should be good here.
>
> In fact the main change I'd make to the implementation is to drop the
> caching of the CND string, and simply regenerate it whenever needed.
> That should simplify the equals method to:
>
>     public boolean equals(Object o) {
>         return o instanceof NodeTypeDefinition
>             && getCnd(this).equals(getCnd((NodeTypeDefinition) o));
>     }
>
> I'd value simplicity over performance here as AFAICT no
> performance-sensitive code uses this feature and correctly
> invalidating the cached value would be non-trivial.
good point.

> Also, as a lesser concern, I'd drop the fallback in the "catch
> (IOException e)" clause. Instead of logging the error and continuing
> with different semantics, I think it would be better to just fail fast
> with an IllegalStateException.
ok.

regards, toby

Mime
View raw message