Factory design pattern with Shapes

Question

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;
}

Answer 1

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();
}

...

Answer 2

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!