On Sun, Feb 13, 2011 at 1:23 PM, Emmanuel Lecharny <[email protected]> wrote: > Hi guys, > > as I was writing some doco for the LDAP-API, I checked the DN class and > found some inconsistencies in the methods. For instance, we have various > constructors, that can be split in two sets : > - the Schema Aware DN constructors > - the Schema Agnostic DN constructors > > 1) Schema Aware constructors : > Dn(SchemaManager) > Dn(Dn, SchemaManager) > Dn(SchemaManager, String...) > Dn(String, SchemaManager) > > 1) Schema Agnostic constructors : > Dn() > Dn(Dn) > Dn(Rdn) > Dn(String) > *Dn(String, String, byte[])* --> This constructor is only used by the > deserialization method > Dn(String, String, byte[], List<Rdn>) > Dn(String...) > > As we can see, in the first set, we don't have the SchemaManager parameter > put always at the same position in the arguments list. I would suggest that > the Dn(Dn, SchemaManager) and Dn(String, SchemaManager) constructors be > replaced by > Dn(SchemaManager, Dn) and Dn(SchemaManager, String).
Big +1 > That raise a second issue : the Dn(SchemaManager, String...) and > Dn(SchemaManager, String) constructors will collide, but there is no reason > to have both. I suggest keeping only the Dn(SchemaManager, String...) > method. +1 > For the exact same reason, I would remove the DN(String) constructor to > favor the DN(String...) one. What does Dn(String...) with the array mean? On first sight I cannot make sense of it. OK so you have an array of string arguments. What do they represent? Whereas it's clear what Dn(String) is. I know it's a Dn represented as a String. The array of String arguments makes me ask ... Are these Rdns? See the confusion? > The Dn(String, String, byte[], List<Rdn>) constructor is only used inside > the server, and I don't think we really need it. I suggest to remove it. > > The package protected constructor Dn(String, String, byte[]) is also an > issue. There is no reason to expose it to the end user. > > Last, not least, in order to have some symmetry between both classes of > constructor, I'd like to suggest we create a schema aware constructor : > Dn(SchemaManager, Rdn...) Is Rdn not schema aware already? Or is that just Dn? > and to transform the Dn(Rdn) constructor to Dn(Rdn...). +1 Alex
