Bad practise? JavaBean where setX() actually should be addX()?

I've written a little JavaBean I used in JSP for alternating colors in navigation menus and table rows etc:

/*
 A bean which wraps an Iterator to keep cycling around rather than ending.
 */
 
package beans;
 
import java.util.Collection;
import java.util.LinkedHashSet;
import java.util.Iterator;
 
/**
 * A bean which wraps an Iterator to keep cycling around rather than ending.
 * @author Chris Corbyn <chris@w3style.co.uk>
 */
public class Rotator <T extends String> {
  
  /** The Collection which is being wrapped */
  protected Collection<T> collection;
  
  /** The Iterator for the collection */
  protected Iterator<T> it = null;
  
  /**
   * No argument constructor for bean.
   */
  public Rotator() {
    this(new LinkedHashSet<T>());
  }
  
  /**
   * General use constructor allowing Collection to be passed.
   * @param Collection<T> collection
   */
  public Rotator(Collection<T> collection) {
    setCollection(collection);
  }
  
  /**
   * Set the Collection being wrapped.
   * @param Collection<T> collection
   */
  public void setCollection(Collection<T> collection) {
    this.collection = collection;
  }
  
  /**
   * Get the Collection being wrapped.
   * @return Collection<T>
   */
  public Collection<T> getCollection() {
    return collection;
  }
  
  /**
   * Add a new item.
   * @param T item
   */
  public void addItem(T item) {
    collection.add(item);
  }
  
  /**
   * Just adds a new item.
   * @param T item
   */
  public void setItem(T item) {
    addItem(item);
  }
  
  /**
   * Get the next item, or the first item if already at the end.
   * @return T
   */
  public T getNext() {
    if (it == null || !it.hasNext()) {
      it = collection.iterator();
    }
    return it.next();
  }
  
  /**
   * Convert this bean to a String (whatever the next item is).
   * @return String
   */
  public String toString() {
    T next = getNext();
    return next.toString();
  }
 
}

The setItem() method doesn't fit it's description and is only there so i can interact with it via JSP's setPoperty tag. Would this be considered a bad practise? Also, the toString() method returns a different value each time.... is that probably not a good idea?

I use it like this:

The setItem() method doesn't fit it's description and is only there so i can interact with it via JSP's setPoperty tag. Would this be considered a bad practise? Also, the toString() method returns a different value each time.... is that probably not a good idea?

Yes to both. Use setItem to set the collection itself and replace addItem with an indexed getter and setter setItem(int index, Value value) to set/get an entity. In the toString method you might want to iterate over all the items and concatenate them into a single string. Just my thoughts. Why do you want this cyclic iterator ?
[ September 08, 2007: Message edited by: John Meyers ]

Both points taken on board and agreed with

I'll rewrite the bean. I think I need to implement a getNext() method because I do need some way of making it iterate cyclically.

The reason it's needed is for alternating colors (but more than just two colors). I wanted to avoid too much complex bitwise logic inside the JSP itself.

Rather than the typical boolean negation or modulus operations I can just keep using ${color.next} and let the bean deal with that logic.

I updated it to use an indexed setter system like so:

/*
 A bean which wraps an Iterator to keep cycling around rather than ending.
 */
 
package beans;
 
import java.util.List;
import java.util.ArrayList;
import java.util.Iterator;
 
/**
 * A bean which wraps an Iterator to keep cycling around rather than ending.
 * @author Chris Corbyn <chris@w3style.co.uk>
 */
public class Rotator <T extends String> {
  
  /** The List which is being wrapped */
  protected List<T> list;
  
  /** The Iterator for the list */
  protected Iterator<T> it = null;
  
  /**
   * No argument constructor for bean.
   */
  public Rotator() {
    setList(new ArrayList<T>());
  }
  
  /**
   * General use constructor allowing the list to be passed.
   * @param T[] collection
   */
  public Rotator(T[] list) {
    setItems(list);
  }
  
  /**
   * Set the List being wrapped.
   * @param List<T> list
   */
  public void setList(List<T> list) {
    this.list = list;
  }
  
  /**
   * Get the List being wrapped.
   * @return List<T>
   */
  public List<T> getList() {
    return list;
  }
  
  /**
   * Add a new item.
   * @param T item
   */
  public void addItem(T item) {
    list.add(item);
  }
  
  /**
   * Get the item at index i.
   * @param int i
   * @return T
   */
  public T getItem(int i) {
    return list.get(i);
  }
  
  /**
   * Add an indexed item.
   * @param int i
   * @param T item
   */
  public void setItem(int i, T item) {
    list.add(i, item);
  }
  
  /**
   * Set the entire list of items from an array.
   * @param T[] items
   */
  public void setItems(T[] items) {
    setList(new ArrayList<T>());
    for (int i = 0; i < items.length; i++) {
      setItem(i, items[i]);
    }
  }
  
  /**
   * Get the entire list of items as an array.
   * @return String[]
   */
  public String[] getItems() {
    Iterator<T> it = list.iterator();
    String[] items = new String[list.size()];
    int i = 0;
    while (it.hasNext()) {
      items[i] = it.next();
      i++;
    }
    return items;
  }
  
  /**
   * Get the next item, or the first item if already at the end.
   * @return T
   */
  public T getNext() {
    if (it == null || !it.hasNext()) {
      it = list.iterator();
    }
    return it.next();
  }
 
}

It's implemented like this:

I have dynamic menu options which are displayed and they are colorful buttons on a dark background. I wanted to avoid the crazy logic determining what color to use in the JSP