Re: FYI: Patch: javax.print.attribute

[Michael Koch <konqueror at gmx dot de>]

On Mon, Dec 22, 2003 at 06:02:45PM +0100, Joerg Brunsmann wrote:
> Hi Michael,
> 
> great to see so many patches. Here's a quick review of your latest.

Thx for the review. I really appreciate your help.

> +  /**
> +   * Creates an empty <code>HashAttributeSet</code> object.
> +   *
> +   * @exception ClassCastException if any element of attributes is not an
> +   * interface of interfaceName
> +   * @exception NullPointerException if attributes or interfaceName is null
> +   */
> +  public HashAttributeSet(AttributeSet attributes, Class interfaceName)
> +  {
> +    this(interfaceName);
> +    
> +    if (attributes != null)
> +      addAll(attributes);
> +  }
> 
> The comment is wrong; if parameter 'attributes' is 'null' no 
> NullPointerException is thrown?

Right, corrected in local code.

> +  /**
> +   * Adds the given attribute to the set.
> +   *
> +   * @param attribute the attribute to add
> +   *
> +   * @return true if the attribute set has changed, false otherwise
> +   */
> +  public boolean add(Attribute attribute)
> +  {
> +    if (attribute == null)
> +      throw new NullPointerException("attribute may not be null");
> +
> +    Object old = attributeMap.put
> +      (attribute.getCategory(), AttributeSetUtilities.verifyAttributeValue
> +                                  (attribute, interfaceName));
> +    return !attribute.equals(old);
> +  }
> 
> Missing @exception tag in comment?

Yes, and some other exceptions too. Fixed in local code.

> +  /**
> +   * Adds the given attributes to the set.
> +   *
> +   * @param attributes the attributes to add
> +   *
> +   * @return true if the attribute set has changed, false otherwise
> +   */
> +  public boolean addAll(AttributeSet attributes)
> +  {
> +    boolean modified = false;
> +    Attribute[] array = attributes.toArray();
> +
> +    for (int index = 0; index < array.length; index++)
> +      if (! add(array[index]))
> 
> Do you need to negate the expression?

Damn, you're right. Fixed in local code.

> +        modified = true;
> +
> +    return modified;
> +  }
> 
> Keep those patches coming ;-)

I would really love if you could look more over the patches ... ;-)
Thanks for helping.

I will commit this with other changes in the next days.


Michael