CODEWITHSUNDEEP
java
matt freake
matt freake

Reputation: 5090

How to refactor to avoid passing "special values" into a Java method?

I'm sure there must be a standard way to do this, but my attempts to search Stackoverflow have failed.

I have a method like:

public void processSomeWidgetsForUser(int userItemId) {
    Iterator<Widgets> iter = allWidgets.values().iterator();
    while(iter.hasNext()) {
        Widget thisWidget = iter.next();
        if (userItemId == -1 || thisWidget.getUsersItemId() == userItemId) {                 
             widget.process();
        }
    }
}

As you can see -1 is a "special value" meaning process all. Doing this saves repeating the loop code in another method called processSomeWidgetsForAllUsers.

But I dislike special values like this because they are easy to misuse or misunderstand, which is exactly the situation what I'm having to fix now (where someone thought -1 meant something else).

I can only think of two ways to improve this.

  1. have a constant, containing -1 called something like Widget.ALLWIDGETS which at least is self-documenting, but doesn't stop code from using a -1 (if someone integrates old code in, for example)
  2. change the method to take a list of all user ids to process, which can be empty, but that doesn't seem great performance-wise (would need to retrieve all user ids first and then loop through removing. Also what happens if the number of widgets in the list changes between retreiving the ids and removing

Is there a better way? I'm sure I'm missing something obvious.

The above code has been changed slightly, so may not compile, but you should get the gist.

Upvotes: 2

Views: 78

Answers (4)

lynks
lynks

Reputation: 5689

Although somewhat redundant, a fairly neat self-documenting approach could be to have 3 methods rather than one;

Make your original method private, and make one small change which would be to add your static final int EXECUTE_ALL = -1 and use that in your original method, then add the two new methods;

public void processWidget(int wID) throws IllegalArgumentException {
    if(wID == EXECUTE_ALL) throw new IllegalArgumentException();
    originalMethod(wID);
}

public void processAllWidgets() {
    originalMethod(EXECUTE_ALL);
}

It makes your class a little more cluttered, but as far as the exposed methods go, it is clearer and hopefully foolproof. You could alter it not to throw an exception and just ignore any invalid ids, that just depends on your situation.

This approach of course has the major downside that it changes how the class appears to other classes, breaking everything that currently uses the, now private, originalMethod().

Upvotes: 4

Jacob Schoen
Jacob Schoen

Reputation: 14192

You can change the method signature to accept a boolean for when you want to process them all.

public void processSomeWidgets(boolean doAll, int userItemId) {
    Iterator<Widgets> iter = allWidgets.values().iterator();
    while(iter.hasNext()) {
        Widget thisWidget = iter.next();
        if (doAll || thisWidget.getUsersItemId() == userItemId) {                 
             widget.process();
        }
    }
}

This makes it more explicit, and easier to read in my opinion as there are no special values.

Upvotes: 1

corsiKa
corsiKa

Reputation: 82559

Have an external method like so:

static boolean idRepresentsAll(int id) {
    return id == -1;
}

In this case, if you decide to replace it with a different mechanism, you only replace your magic number one place in your code.

At the very least, you would want to do something like this:

public static final int ID_REPRESENTING_ALL = -1;

Upvotes: 1

thatidiotguy
thatidiotguy

Reputation: 8981

Number 1 would work very nicely. Be sure to document what the variable is though, so future coders (possibly yourself) know what it means.

/**This is the explanation for the below variable*/
public final static int ALL_WIDGETS = -1;

Upvotes: 1

Related Questions