|
Sunil,
Moving the discussion to the developer list
Except for the fact that the diff is backwards, your code should work.
However, if you assume that the most frequently used case is to
add an attribute, then the following is more efficient
as it eliminates a call to toUpperCase.
String name = attribute.getName().toUpperCase();
if( this.map.containskey( name))
return false; else{ this.map.put( name, attribute); return true; } -Steve
>>> "Sunil Kumar" <Sunilk@novell.com> 26-Aug-03 10:25:56 PM >>> Steve, Thanks for correcting me. I just looked at the implementation using HashMap and didn't paid that it implements attention to the java.util.Set contract. As suggested by you the implemenation doesn't look correct. I would suggest following changes to the current implementation: < if( this.map.containskey(attribute.getName().toUpperCase())) < return false; < else{ < this.map.put(attribute.getName().toUpperCase(), < attribute); < return true; < } --- > return (this.map.put(attribute.getName().toUpperCase(), > attribute) != null); let me know your comments about it? Regards, -Sunil. >>> Steve Sonntag 8/26/2003 8:37:59 PM >>> Sunil, I don't agree that the javadoc is incorrect. I believe the implementation is incorrect. See comments inline: > > > > >>> Sunil Kumar 25-Aug-03 11:14:09 PM >>> > > Hi Diego, > > I have verified the added certificate in my configuration. I used Novell > > ConsoleOne to validate it and its showing that the added certificate is a > > valid one. > > > > As far as the seocnd problem reported by you i.e AttributeSet.add > > (LDAPAttribute) returns false. I looked at it and found that the > > Documentation in JavaDoc for attributeSet.add method is wrong. In javaDoc > > they have specifed that: > > add() method will "return true if the attribute was added." > > > > Whereas actually it should be:- > > " return true if there was already a previous value(non-null) for the > > specified attribute in the attribute set ." > > I am not sure where you are quoting from, but ... LDAPAttributeSet implements the interface Set and thus must abide by that contrac! t. The S et.add method states: Returns: true if this set did not already contain the specified element. Or to put it another way, returns true if the element was added. Set further states: Adds the specified element to this set if it is not already present ... If this set already contains the specified element, the call leaves this set unchanged and returns false. So now we must ask the question: Is the implementation correct. The implementation is NOT correct in two ways: 1) The return value is incorrect: The code looks something like: return (this.map.put(name, attribute) != null); This returns true if the attribute exists. It should return false. 2) map.put replaces the value. Quoting from java.util.HashMap "If the map previously contained a mapping for this key, the old value is replaced." If the value exists, the set should be left unchanged. This also violates the contract for Set.add. The code is incorrect, it should be changed. > > Also I have found one more wrong statement in the JavaDoc.They have mentioed : > > * Adds the specified attribute to this set if it is not already present. > > * <p>If an attribute with the same name already exists in the set then the > > * specified attribute will not be added.</p> > > > > Whereas it should be:- > > * Adds the specified attribute to this set if it is not already present. > > * <p>If an attribute with the same name already exists in the set then the > > * old specified value for the attribute will be replaced by the new Value..</p> > > Again as previously stated, the contract for Set.add mandates that the Set not be changed if an attempt is made to add an element already in the set. In this case, only one Attribute with the given attribute name is allowed in the set. LDAPAttributeSet.add should return fal! se if an Attribute by that name already exists in the set and should leave the Set unchanged. -Steve |