On 10/14/2016 08:06 PM, Emmanuel Lecharny wrote: > Hi ! > > Stefan has added some TODO in the LdapConnection interface : > > // TODO: all the SASL bind methods are not declared in this interface, but > implemented in LdapNetworkConnection. Is that intended? > // TODO: why does connect() return a boolean? What is the difference > between false and an Exception? > // TODO: think about usage of abbrevisions (Dn/Rdn) vs. spelled out > (relative distinguished name) in javadoc > // TODO: describe better which type of LdapException are thrown in which > case? > // TODO: remove the "we" language in javadoc > // TODO: does method getCodecService() belong into the interface? It > returns a LdapApiService, should it be renamed? > // TODO: does method doesFutureExistFor() belong into the interface? Move > to LdapAsyncConnection? > > I have fixed some of them :
Great, thanks Emmanuel! > - Replaced teh Dn/Rdn abbreviations by the full name > - Removed the 'we' all over the javadoc > - Replaced the doesFutureExistFor() method by a more explicit > isRequestCompleted() method. It's not a pure async method, it's also useful > when one want to abandon a request that takes too long. > - The connect() method returns a boolean to be able to distinguish between > a problem while establishing a connection, and a simple timeout because teh > remote host does not answer. We do have a retry mechanism in this case, and > when we reach the number of possible retries, we simply return 'false'. Of > course, when we have some more problematic problem, an exception is thrown. Hm, but as a user of the API I just want to connect, I have to handle both, the return value and the exception. When I get an Exception I can see the reason (host not found, invalid credetials, etc). But If I just get a "false" I don't know any details. I'd prefer to get an exception. > Regarding the few oher TODOs : > > - We have to add those SASL methods Agreed > - LDAPException : yes, it would be useful to list specific excpetions > instead of generic ones I'd say it's optional to refine them. > - getCodecService() : this is a problem. the LdapApiService means nothing > for those who haven't wrote the API? CodecService is slightly more > significant. This method returns the container that exposes the supported > codec and extended operation. Renaming it to getServerFeatures() would be > better, but we should also rename the LdapApiService (something like > ServerFeatures would probably be clearer ?) CodecService sounds ok to me. But, I wonder if this service needs to be exposed via the LdapConnection interface at all? Does a user of the LDAP API ever need it? I guess only if she want to register new controls or extended operations programatically, but that can also be done by setting system properties Kind Regards, Stefan