views:

121

answers:

4

I have some code that I want to refactor. I have lots of methods that take multiple arguments of the same type, for example:

public void foo(String name, String street, boolean b1, boolean b2) { ... }

and so on. Because the different objects can only be distinguished by name I would like to wrap them in Objects (Enums) so I can make use of the typesystem of the language (Java in this case).

public class Name {
    private String value;
    public String getValue() { return value; }
    // ...
}

Like this I could force calling code to pass in Objects of a certain type. That would assure that it would not accidentally mix up the order of the method parameters and thus not produce unexpected behaviour at runtime:

foo(new Name("John"), new Street("Broadway"), new B1(true), new B2(false);

This makes refactoring a lot safer, you can carry the object through the system as long as you want, the data inside it, the string is safe at all times. Only when you need it, you get it by calling getValue().

Now, for the Objects that wrap strings, it is pretty straightforward, as there are lots of states instances can be in.

But how about the boolean wrappers? These are either TRUE or FALSE. The implementation just looks, well, a bit funny:

public enum Quanto {

    YES() {
        protected boolean isQuanto() {
            return true;
        }
    },
    NO() {
        protected boolean isQuanto() {
            return false;
        }
    };

    protected abstract boolean isQuanto();

}

Even stranger I find what the calling code looks like:

public void doStuff(Quanto quanto) {
    if(quanto.isQuanto()) {
        // ...
    }
}

Technically it does not matter of course, but it just doesn't feel right... Have you found "better" ways of dealing with this?

EDIT: What also displeases me is the fact that there are more values thinkable than YES and NO in the above example, say MAYBE...?!

Thanks!

+9  A: 

I would wrap booleans into semantic enums instead. In other words:

public void doStuff(boolean isActive, boolean wrapResult);

becomes

public enum State {ACTIVE, INACTIVE};
public enum ResultMode {WRAPPED, UNWRAPPED};

public void doStuff(State state, ResultMode resultsMode);
ChssPly76
This. Clean, easy, succinct.
CPerkins
Yes. But that is exactly what I proposed, isn't it? You just leave the isQuanto method away and have client code do a stright if(q == Quanto.YES) ...
raoulsson
IMO `doStuff(State.ACTIVE, Mode.UNWRAPPED)` looks more natural and is easier to comprehend than `doStuff(Active.YES, Wrapped.NO)`.
ChssPly76
+1  A: 

Instead of having enums for the booleans, why not have encapsulation objects for the different values you are passing into the method? This way the values are on getters and setters and have the information you want:

public class Encapsulator {

    private boolean isSelected;
    private boolean isAwake;


    public boolean isAwake() {
        return isAwake;
    }

    public void setIsAwake(boolean isAwake) {
        this.isAwake = isAwake;
    }

    public boolean isSelected() {
        return isSelected;
    }

    public void setIsSelected(boolean isSelected) {
        this.isSelected = isSelected;
    }
}

This way, when you are accessing the data it is very clear that a particular element in the object does one thing or another. It also reduces the parameter set for methods, which is a code smell according to Martin Fowler.

aperkins
I personally like parametric coupling. Of course you can turn that into a mess as well, but preferring the extra param over some other construction (especially if it involves adding an extra class) often yields the better solution IMHO.
eljenso
The problem is when you have lots of parameters that are being passed around - that is what the pattern is designed to solve. If there are only 2 or 3 (maybe even 4), or they are only being passed once, that is a different situation entirely. :)
aperkins
+4  A: 

Basically what you're doing is named parameters, so I'd suggest one class per function, with members for each parameter.

The call site would then look something like this:

foo(new foo_arguments().Name("John").Street("Broadway").B1(true).B2(false));

And then you'd use arguments.Name() and such inside the function.

This has the additional advantage of letting you give default arguments without massive numbers of overloads, and lets the arguments be specified in any order, so this works:

foo(new foo_arguments().Street("Sesame").Name("Monster"));

The required class:

public class foo_arguments {
    private string _name = "John Doe";
    public foo_arguments Name(string name) { _name = name; return this; }
    public string Name() { return _name; }

    private string _street = "Pennsylvania Ave NW";
    public foo_arguments Street(string street) { _street = street; return this; }
    public string Street() { return _street; }

    private string _b1 = false;
    public foo_arguments B1(string b1) { _b1 = b1; return this; }
    public boolean B1() { return _b1; }

    private string _b2 = true;
    public foo_arguments B2(string b2) { _b2 = b2; return this; }
    public boolean B2() { return _b2; }
}

As an aside, in C# you can do this really elegantly with auto-properties and object initializers.

me22
Depending on what the sourcecode looks like it is hopefully possible to identify one or several objects that makes sense in a more profound way than, say foo_arguments. I don't know what the booleans do in the above example, but perhaps you could have a Person class containing name, street, b1 and b2. Try to identify what Objects you want to have, instead of passing primitives.
Buhb
+2  A: 

Do the methods always take the same set of arguments? If they do maybe you want to create a data object that stores all the data and you only provide this object to the functions. Maybe you can even move the functions into the object at a later point. Having methods with many input arguments sometimes is a hint that there is a object for all this arguments missing.

I would not bother to encapsulate one Boolean in one object. That doesn't seems right for me.

Janusz