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

[Chris Corbyn]

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 <[e-mail]>
 */
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:

<jsp:useBean id="color" class="beans.Rotator">
  <jsp:setProperty name="color" property="item" value="green" />
  <jsp:setProperty name="color" property="item" value="orange" />
  <jsp:setProperty name="color" property="item" value="yellow" />
  <jsp:setProperty name="color" property="item" value="pink" />
  <jsp:setProperty name="color" property="item" value="blue" />
</jsp:useBean>
 
// ... snip ...
 
<%= color %> //green
<%= color %> //orange
<%= color %> //yellow
<%= color %> //pink
<%= color %> //blue
<%= color %> //green
...

[Deepak Bala]

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 ]

[Chris Corbyn]

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.

[Chris Corbyn]

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 <[e-mail]>
 */
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:

        <div id="navigation_links">
          <jsp:useBean id="color" class="beans.Rotator">
            <jsp:setProperty name="color" property="items"
                value="${fn:split('green;orange;yellow;pink;blue', ';')}" />
          </jsp:useBean>
          <ul>
            <jsp:useBean id="menu" class="beans.NavigationMenu" />
            <c:forEach items="${menu.items}" var="item">
              <c:choose>
                <c:when test="${item.active}">
                  <li><div class="nav_item ${color.next} on">
                    <div class="nav_link">
                      <mayo:link to="${item.url}">${item.text}</mayo:link>
                    </div>
                    <div class="nav_blank"></div>
                  </div></li>
                </c:when>
                <c :-o	 therwise>
                  <li><div class="nav_item ${color.next}">
                    <div class="nav_link">
                      <mayo:link to="${item.url}">${item.text}</mayo:link>
                    </div>
                    <div class="nav_blank"></div>
                  </div></li>
                </c :-o	 therwise>
              </c:choose>
            </c:forEach>
          </ul>
        </div>

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