1
\$\begingroup\$

I'm making the very common example of Factory design pattern which creates a factory of shapes and return an instance of the type Shape. I wonder which is the best way to replace that bunch of nested if-else statements so that my code is more readable and maintainable.

I think there should exist a suitable design pattern to solve this issue but since I'm a beginner using design patterns I don't know where to start.

This is the code snippet for the getShape() method:

public Shape getShape(String type, String scope) {

    if (type == null) {
        return null;
    }
    if (type.equalsIgnoreCase("circle")) {
        if (scope.equalsIgnoreCase("singleton")) {
            return SingletonCircle.getInstance();
        }
        return new PrototypeCircle();
    } else if (type.equalsIgnoreCase("rectangle")) {
        if (scope.equalsIgnoreCase("singleton")) {
            return SingletonRectangle.getInstance();
        }
        return new PrototypeRectangle();
    } else if (type.equalsIgnoreCase("square")) {
        if (scope.equalsIgnoreCase("singleton")) {
            return SingletonSquare.getInstance();
        }
        return new PrototypeSquare();
    }
    return null;
}
\$\endgroup\$
  • \$\begingroup\$ One addition to the other answers: I'd provide enum's for type and scope, so nothing wrong - except null - can be passed. And the caller of the method knows what he can pass and does not have to check the actual implementation. \$\endgroup\$ – slowy May 2 '17 at 14:03
1
\$\begingroup\$

Parameter checks

First of all, I'd be very strict about the check of the parameters: I do not see any use in gracefully trying to work around unset or illegal parameters instead of throwing an exception.

Thus, I recommend you start out with:

    Objects.requireNonNull(type);
    Objects.requireNonNull(scope);

These throw NullPointerException if the respective parameter is null. This is the clearest way to tell your user that this was unexpected. (If you use some kind of validation framework like javax.validation, you might even annotate the parameters as @NotNull).

Furthermore, I recommend IllegalArgumentException for illegal types instead of returning null.

DRY

Then, you repeat yourself with the check scope.equalsIgnoreCase("singleton") - you could refactor that into a variable.

switch

String comparisons can be done via a switch statement. To be case-insensitive, you can switch over toLowerCase of the string and use lowercase constants.

delegate to specific methods

Last advice: move the concrete object creation to specific methods for objects, and simply call the sub-method from the main method.

Putting it all together

public Shape getShape(String type, String scope) {
    Objects.requireNonNull(type);
    Objects.requireNonNull(scope);
    boolean isSingleton = scope.equalsIgnoreCase("singleton");
    switch(type.toLowerCase()) {
    case "circle":    return getCircle(isSingleton);
    case "rectangle": return getRectangle(isSingleton);
    case "square":    return getSquare(isSingleton);
    default: throw new IllegalArgumentException("unknown type");
    }
}

private Shape getCircle(boolean inSingletonScope) {
    return inSingletonScope
        ? SingletonCircle.getInstance()
        : new PrototypeCircle();
}

...
\$\endgroup\$
0
\$\begingroup\$

I would do something like:

    public Shape getShape(String type, String scope) {
         Object toRe;
         Objects.requireNonNull(type);
         Objects.requireNonNull(scope);
         switch (type.toLowerCase()) {
             case "circle":
                 toRe = scope.equalsIgnoreCase("singleton")?SingletonCircle.getInstance():new PrototypeCircle();
                 break;
             case "rectangle":
                toRe = scope.equalsIgnoreCase("singleton")?SingletonRectangle.getInstance():new PrototypeRectangle();
                break;
            case "square":
                toRe = scope.equalsIgnoreCase("singleton")?SingletonSquare.getInstance():new PrototypeSquare();
                break;         
             default:
                 toRe = null;
                 break;     
        }

        return toRe;
    }

Note:

I wrote the code directly here so it may have as syntax errors, if so just comment on my post so i can fix them!

\$\endgroup\$
  • \$\begingroup\$ I'd suggest do return the instances directly, you save all the breaks, you can remove the default case, when you return null at the end, you save the Object toRe declaration on top. \$\endgroup\$ – slowy May 2 '17 at 13:53
  • \$\begingroup\$ @slowy Assigning a value to a local variable and then returning that at the end is considered a good practice. Methods having multiple exits are harder to debug and can be difficult to read. \$\endgroup\$ – Feconiz May 2 '17 at 14:17
  • \$\begingroup\$ Well, that's just your opinion, man. Did you have an assembly / c programmer as teacher in school? Because in assembly or c, it's really important, to follow the 'one exit / one entry' rule (one exit actually means to one place, not only one return statement), because they used gotos and alternate entry points - and well, memory leaks. But in java? I don't accept 'harder to debug' with our tools we have today, where we can flick the 'on exit' checkbox on the breakpoint. Harder to read? If I compare your code with mtj's code, I can't accept that argument either ;-) \$\endgroup\$ – slowy May 3 '17 at 9:20
  • \$\begingroup\$ @slowy I'm self-taught, but I remember someone telling me that it's better to have one exit only :/ I may be wrong, but I haven't studied programming (yet) ! \$\endgroup\$ – Feconiz May 3 '17 at 9:32

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.