directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ersin Er <ersin...@gmail.com>
Subject Re: svn commit: r406888 - /directory/trunks/apacheds/kerberos-shared/src/main/java/org/apache/directory/server/kerberos/shared/store/operations/GetPrincipal.java
Date Tue, 16 May 2006 17:17:22 GMT
Enrique Rodriguez wrote:
> ersiner@apache.org wrote:
> ...
>> +    +    /**
>> +     * TODO: It's better to move this method to a common place.
>> +     */
>> +    private static boolean parseBoolean( String bool )
>> +    {
>> +        if ( bool.equals( "true" ) )
>> +        {
>> +            return true;
>> +        }
>> +
>> +        return false;
>> +    }
>> +     }
I would really not commit this peace of code if knew it would cause this 
much trouble :) While I was reading the user's email, there was just a 
code open in front of me (Safehaus Triplesec code) and I just copied the 
code from there, to fix the build issue in trunks (we do not want this 
to happen, right?), thinking that Enrique would put the method to 
another better place (or inline as he suggested) later. So the main 
point was fixing the trunk build issue. Of course I also looked if 
apacheds-kerberos-shared had a dep to shared-ldap. If it was the 
situation, I would use the BooleanUtils code there (_especially, if I 
need that much flexibility as provided in that Utils code_).
> This entire method is the same as saying:
>
> return bool.equals( "true" );
>
> In which case, you could inline it:
>
> modifier.setLockedOut( val.toLowerCase().equals( "true" ) );
>
> Or, Emmanuel's suggestion:
>
> modifier.setLockedOut( "true".equalsIgnoreCase( val ) );
Agree with inlining. But as I said it was just a temporal copy paste :)
> Now, you have a negligible amount of code that almost added a 
> dependency on shared-ldap.  Besides bloating the kerberos code by 3 X 
> there is also no conceptual link between kerberos and shared-ldap 
> code.  The problem here is not necessary code size, which in this day 
> and age isn't that important, but rather the conceptual integrity of 
> the artifacts we provide and a drastic increase in POM maintenance in 
> product configurations as these sorts of deps trickle down.  I'm sorry 
> to use Ersin's commit as an example, but this sort of "eliminate 
> duplication at all costs" mindset occurs quite a bit on this project 
> and it makes POM maintenance in downstream product assemblies a lot 
> harder.
>
> Moreover, the thing to recognize here is that we have configuration 
> code in apacheds-core and protocol-common and helper methods like the 
> above in many of the protocols and shared-ldap.  In which case, the 
> thing to do is form a configuration package.  What I mean by 
> "conceptual integrity" is that we could then point to a configuration 
> artifact as the place for configuration library and utility code, as 
> opposed to pointing to shared-ldap or apacheds-core or 
> protocol-common, which have nothing to do, conceptually, with boolean 
> text parsing or configuration.
Most of the utilities under shared-ldap has nothing to do with ldap. So 
we may have another lightweight "commons" project.
> Enrique
Cheers,

-- Ersin


Mime
View raw message