5
\$\begingroup\$

I'll start with giving a bit of background as there are many classes involved and I can't attach all of them because that would make this question wildly too long. For the curios they can be found at my github repo.

I have an immutable structure called a Chassis which consists of 8 immutable Components. Each Component is named by a Location such as Location.LeftArm or Location.RightTorso.

A Loadout is a particular configuration of Items that are equipped on a Chassis. For each Component on the Chassis, the Loadout has a matching ConfiguredComponent that contains the Items equipped on that component of the Chassis. In addition to this, each Component can have a number of fixed Items and some Items are kind of fixed but can be toggled on and off (not my decision, that's the game rules). So the toggle state is a part of the ConfiguredComponent which is why ConfiguredComponent has a getItemsFixed() method which will return the truly fixed items + any items which have their toggle state set to enabled.

In many places in the code base I need to iterate over all items on the Loadout (fixed and removable). Even more frequently I'm only interested in certain types of items on the loadout. For example those that are a subclass of Weapon which is an Item or even those items that implement a particular interface not necessarily inheriting from Item. In other words I may want to filter on a class that is not descendant from Item. However all items on the loadout have Item as a base class.

To this end I decided upon letting Loadout have methods to return an Iterable of approriate type which I can use to iterate over Items of a particular type. Then I implement a form of MultiIterator that can iterate over all the Items on the Loadout.

Relevant excerpt from Loadout:

public abstract class LoadoutBase<T extends ConfiguredComponentBase> {
   public Iterable<Item> items(){
      return new LoadoutIterable<Item>(this, null);
   }

   public <X> Iterable<X> items(Class<X> aClass){
      return new LoadoutIterable<X>(this, aClass);
   }
}

Implementation of glue class LoadoutIterable:

public class LoadoutIterable<T> implements Iterable<T>{
   private final LoadoutBase<?> loadout;
   private final Class<T> filter;

   public LoadoutIterable(LoadoutBase<?> aLoadout, Class<T> aFilter){
      loadout = aLoadout;
      filter = aFilter;
   }

   @Override
   public Iterator<T> iterator(){
      return new LoadoutItemIterator<T>(loadout, filter);
   }
}

And finally the Iterator itself (remember, T might not extend Item at all but there might be some instance of a subclass of Item that implements T):

public class LoadoutItemIterator<T> implements Iterator<T>{
   private static enum IterationState{
      Fixed, Equipped
   }

   private final static Location[] LOCATION_ORDER;
   private List<Item>              items;
   private IterationState          state           = IterationState.Fixed;
   private int                     index           = 0;
   private final LoadoutBase<?>    loadout;
   private final Class<T>          filter;
   private Location                currentLocation = LOCATION_ORDER[0];

   static{
      LOCATION_ORDER = new Location[Location.values().length];
      for(Location location : Location.values()){
         LOCATION_ORDER[location.ordinal()] = location;
      }
   }

   LoadoutItemIterator(LoadoutBase<?> aLoadout, Class<T> aFilter){
      loadout = aLoadout;
      filter = aFilter;
      items = loadout.getComponent(currentLocation).getItemsFixed();
   }

   LoadoutItemIterator(LoadoutBase<?> aLoadout){
      this(aLoadout, null);
   }

   @Override
   public boolean hasNext(){
      return null != getNextNonFiltered();
   }

   @Override
   public T next(){
      T ans = getNextNonFiltered();
      index++;
      return ans;
   }

   @Override
   public void remove(){
      throw new UnsupportedOperationException();
   }

   @SuppressWarnings("unchecked")
   private T getNextNonFiltered(){
      while( true ){
         if( index < items.size() ){
            Item item = items.get(index);
            if( filter == null || filter.isAssignableFrom(item.getClass()) ){
               return (T)item; // This cast is checked
            }
            index++;
         }
         else{
            index = 0;
            if( state == IterationState.Fixed ){
               state = IterationState.Equipped;
               items = loadout.getComponent(currentLocation).getItemsEquipped();
            }
            else{
               if( currentLocation.ordinal() == LOCATION_ORDER.length - 1 ){
                  return null; // End of items
               }
               currentLocation = LOCATION_ORDER[currentLocation.ordinal() + 1];
               state = IterationState.Fixed;
               items = loadout.getComponent(currentLocation).getItemsFixed();
            }
         }
      }
   }
}

Example usage:

LoadoutBase<?> loadout = ...;
for(Weapon weapon : loadout.items(Weapon.class)){
   ...;
}

The code works as is and I have achieved my sought for syntax for iterating over specific types of items on the loadout. However there are a few things nagging me, most prominently the unchecked cast in LoadoutItemIterator.getNextNonFiltered.

I would like help with:

  • How can I (if I can) get rid of the unchecked cast?
  • Better naming?
  • Performance of LoadoutItemIterator. I tried implementing using Iterators to each sub list individually but that proved to be slower than this implementation. Without filtering this is about 20% faster than just slamming all the sublists into a new list and returning that.
  • Other suggestions for design or solutions to the problem.

And I'm sorry for being unable to provide a runnable example, but I feel like that would be too much work for the reviewer. If you do wish to run the code you can check it out from the github repository at the top and run the gradle setup script to get the dependencies.

\$\endgroup\$
2
\$\begingroup\$

Do you really need your private static enum IterationState? If it is simply used in a flag-like manner to toggle betweed Fixed and Equipped values, then may I suggest replacing it entirely by a boolean value instead?

Going with my assumption above, I can then think about using a method to standardize how I assign the items variable:

private void setItems() {
    // not sure Component is the same as yours or not, I'm just going by naming convention
    final Component component = loadout.getComponent(currentLocation);
    items = isItemsFixed ? component.getItemsFixed() : component.getItemsEquipped();
}

The start of LoadoutItemIterator will look something like this (incorporating @maaartinus's suggestion):

public class LoadoutItemIterator<T> implements Iterator<T> {

    private final static Location[] LOCATION_ORDER = Location.values();
    // ...
    private boolean isItemsFixed = true;

    LoadoutItemIterator(LoadoutBase<?> aLoadout, Class<T> aFilter) {
        loadout = aLoadout;
        filter = aFilter;
        setItems();
    }

    // ...
}

The body of your while(true) loop can be changed significantly too:

// body of while-loop
if (index < items.size()) {
    Item item = items.get(index);
    if (filter == null || filter.isAssignableFrom(item.getClass())) {
        return (T) item; // This cast is checked
    }
    index++;
} else if (prepareNextItemsToLoad() && (currentLocation = currentLocation.next()) == null) {
    return null; // actually, do you want to throw NoSuchElementException? see below
} else {
    setItems();
}

// method implementation
private boolean prepareNextItemsToLoad() {
    index = 0;
    isItemsFixed = !isItemsFixed;
    return isItemsFixed;
}

I am employing a tiny shortcut/trick for the else if clause. You want to move to the next Location (again, using @maaartinus's suggestion for a Location.next()) when you have finished iterating through the equipped items for the currentLocation. Thus, currentLocation.next() is assigned to currentLocation when !isItemsFixed. It is compared with null to see whether the result of calling next() exhausts the Location values. If so, we return null, just like what you have. Otherwise, all we are left to do is to setItems(), since we have conveniently reset index = 0 and toggled isItemsFixed when we call prepareNextItemsToLoad().

One more thing to take note, LoadoutItemIterator.next() should throw NoSuchElementException when your Locations are fully exhausted, which you can also determine by comparing currentLocation. In full:

@Override
public T next() {
    if (currentLocation == null) {
        throw new NoSuchElementException();
    }
    T ans = nextItem();
    index++;
    return ans;
}

I have taken a final liberty to rename getNextNonFiltered as nextItem, because personally for me, seeing a method with a get prefix inside anything remotely Iterable sounds like we have already moved on and incremented our index when the method returns. This could be just me though.

One more edit

D'oh, now that I have completed my review, I also realized my uses of isItemsFixed are already encapsulated within methods... So feel free to stick with your IterationState enum, and replace the boolean comparisons with the appropriate if ( state == IterationState.Fixed )... clauses. :)

\$\endgroup\$
2
\$\begingroup\$

It's too long for the time I can spend now, so just some random comments:

Assuming Location is an enum, you can replace

private final static Location[] LOCATION_ORDER;
LOCATION_ORDER = new Location[Location.values().length];
for(Location location : Location.values()){
    LOCATION_ORDER[location.ordinal()] = location;
}

by

private final static Location[] LOCATION_ORDER = Location.values();

How can I (if I can) get rid of the unchecked cast?

This is strange:

if( filter == null || filter.isAssignableFrom(item.getClass()) ){
    return (T)item; // This cast is checked
}

Firstly, I'd replace null by Object.class for filter. You want everything and everything is an Object. Secondly, the condition can be expressed simpler. Thirdly, there is Class#cast.

if (filter.isInstance(item) return filter.cast(item);

But even this sounds too complicated, given that there's Guava's Iterables#filter doing exactly this.


I can't see how getNextNonFiltered can be non-filtered when it uses filter.


Consider implementing Location#next() to move the functionality to where it belongs.


I don't understand the loop, so I can't tell more. Consider using AbstractIterator or extracting methods to make it clearer.

\$\endgroup\$
  • \$\begingroup\$ I did some benchmarking and having the null check is faster for the non-filter case. As this kind of iteration is done quite frequently I will prefer the null check on performance grounds. But otherwise I agree. \$\endgroup\$ – Emily L. Aug 29 '14 at 7:41
  • \$\begingroup\$ @EmilyL. Then you can't get rid of the unchecked cast. But you can place @SuppressWarnings on a local variable and so limit its scope. \$\endgroup\$ – maaartinus Aug 29 '14 at 9:54
  • \$\begingroup\$ In order to add a next() I would need a first() and hasNext() and in that case I might as well use an Iterator to Arrays.asList(Location.values()). I do not have Guava as a dependency and I'm trying to keep my jar size down for (near) future android port. \$\endgroup\$ – Emily L. Aug 29 '14 at 11:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.