Loop with switch vs. different loops

Question

I have a method which loops over user elements, and sets a boolean value according to some given constraint:

public void checkUsers( int constraint ) {
    for(int i=0; i<nodeUsers().size(); i++) {
        UserElement elem = nodeUsers().getUsersElementAt(i);

        switch (constraint) {
          case CHECK_ALL:
              elem.setChecked(true); break;
          case CHECK_NONE:
               elem.setChecked(false); break;
          case CHECK_NO_LANG:
               if (elem.getLanguage() == null)
                   elem.setChecked(true);
               else
                   elem.setChecked(false);
               break;
          // More cases   
        }
    }
}

I'm wondering if this solution is OK. Maybe I could better write different methods like:

public void checkAllUsers() {
    for(int i=0; i<nodeUsers().size(); i++) {
        UserElement elem = nodeUsers().getUsersElementAt(i);
        elem.setChecked(true);  
    }
}

public void checkNoUsers() {
    for(int i=0; i<nodeUsers().size(); i++) {
        UserElement elem = nodeUsers().getUsersElementAt(i);
        elem.setChecked(false); 
    }
}

// Edit: I've added a third case.

Answer 1

The two-methods implementation is better. Your code will be easier for others to understand, since they will only need to read the method names, as opposed to having to understand what your parameter does in the first implementation.

Answer 2

hey,

i don't see why not, as long as you document everything right :)

you might calculate the length of your loop in advance ( saves time & resources )
so

nodeUsers().size() -> make it a variable :)

good luck :)

Answer 3

The second will perform slightly better, but it will vary depending on your JIT settings.

In the worst case scenario, the Constraint will be checked every iteration, which could lead to bad performance. In the best case the constraint is only checked once.

An alternate solution, which has all of the benefits, and none of the potential downsides :

public void checkUsers(bool setTo) {
    for(int i=0; i<nodeUsers().size(); i++) {
        UserElement elem = nodeUsers().getUsersElementAt(i);
        elem.setChecked(setTo); break;  
    }

Answer 4

It seems to me that you could do this more effectively by making your constraint a real enum:

public enum Constraint
{
    CHECK_NONE
    {
        @Override void apply(UserElement element)
        {
            element.setChecked(false);
        }
    },
    CHECK_ALL
    {
        @Override void apply(UserElement element)
        {
            element.setChecked(true);
        }
    };

    public abstract void apply(UserElement element);
}

Then you can have:

public void checkUsers(Constraint constraint) {
    for(int i=0; i<nodeUsers().size(); i++) {
        UserElement elem = nodeUsers().getUsersElementAt(i);
        constraint.apply(elem);
    }
}

Alternatively, have an interface with the same "apply" method, and pass in an instance of the interface into your checkUsers method. It's the same basic pattern - separate the iteration over all user elements from "what to do with the element".

Answer 5

Given that you're stuck in 1.4, I'd suggest extracting the case statement out of the loop, so that while you don't avoid the conditional, you at least isolate it. The loop becomes much simpler, the cyclomatic complexity goes down, and when you do get to switch to a more modern Java, you've got less code to look at for conversion (well, in one function you will be upgrading to exploit foreach; in another, to exploit better enums - but it's a separation of concerns):

public void checkUsers(int constraint) {
    for (int i = 0; i < nodeUsers().size(); i++)
     checkUser(constraint, nodeUsers().getUsersElementAt(i));
}

private void checkUser(int constraint, UserElement elem) {
    switch (constraint) {
    case CHECK_ALL:
     elem.setChecked(true);
     break;
    case CHECK_NONE:
     elem.setChecked(false);
     break;
    case CHECK_NO_LANG:
     if (elem.getLanguage() == null)
      elem.setChecked(true);
     else
      elem.setChecked(false);
     break;
    // More cases
    }
}

This is independent of whether you split the individual cases up into their own functions.

Answer 6

This is a normal variation on the Visitor pattern. Lacking generics support isn't so much of an issue since you're only dispatching one type of value. The interface (or abstract type) implementation seems most correct; this both allows simpler unit testing and opens the door to better extensibility if your deployment case makes dependency injection an attractive option.

Answer 7

Oh, hell. This is better:

private void checkUser(int constraint, UserElement elem) {
    elem.setChecked(shouldCheck(constraint, elem));
}

private boolean shouldCheck(int constraint, UserElement elem) {
    switch (constraint) {
    case CHECK_ALL: return true;
    case CHECK_NONE: return false;
    case CHECK_NO_LANG: return elem.getLanguage() == null;
    // More cases
    }
}

You avoid all the breaks.

Answer 8

You mentioned you are stuck in 1.4 in a comment, so this should get you started on something similar to what Jon Skeet suggested, but as a typesafe enum.

public abstract class Constraint {
  public static final Constraint CHECK_NONE = new Constraint(){
      @Override 
      public void apply(UserElement elem) {
        elem.setChecked(false);
      }
    };
  // etc. for other cases...

  // reduce visibility of ctor
  private Constraint() {};

  public abstract void apply();
}

Usage is the same as in Jon Skeet's answer.

Note that this is only needed if you're stuck on 1.4. If you're using modern Java, use a real enum.