CODEWITHSUNDEEP
javarefactoring
Sunil
Sunil

Reputation: 864

Refactor by removing if and else conditions

Below snippet in my method uses lot of if and else condtitions. I would like to avoid it using some way. How can I achieve the below functionality by having a config using HashMap for each type?

        boolean success = true;
        if (source.isTypeA()) {
            if (!model.getPermission || !source.hasAccess) {
                success = false;
            }
        } else if (source.isTypeB()) {
            if (source.condition() && (!model.getPermission() || !source.hasAccess)) {
                success = false;
            }
        } else if (source.isTypeC() && (!source.isMobile || !model.getPermission)) {
            success = false;
        } else if ((source.isTypeD() && source.condition) && (!source.isMobile || !model.getPermission)) {
            success = false;
        }
        return success;

Upvotes: 2

Views: 111

Answers (4)

Juniver Hazoic
Juniver Hazoic

Reputation: 127

You shall use strategy design pattern to create different strategy class based on your source type, and to determine the success based on different criteria of different strategy class. By such way are you be able to conforming to open/closed principle.

A code sample is as below. You could take a look at a wikibook of strategy pattern.

interface SuccessInterface{
    boolean IsSuccess(Object model);
}

interface SuccessFactoryInterface{
    SuccessInterface Create(Object source) throws Exception;
}

class SuccessTypeFactory implements SuccessFactoryInterface{

    @Override
    public SuccessInterface Create(Object source) throws Exception {
        switch (source.getType()) {
            case typeA:
                return new TypeA();
            case typeB:
                return new TypeB();
            case typeC:
                return new TypeC();
            case typeD:
                return new TypeD();
            default:
                throw new Exception();
        }
    }
}

class TypeA implements SuccessInterface {
    private Object _source;

    public TypeA(Object source) {
        _source = source;
    }

    @Override
    public boolean IsSuccess(Object model) {
        return model.getPermission() && _source.hasAccess();
    }
}

class TypeB implements SuccessInterface {
    private Object _source;

    public TypeB(Object source) {
        _source = source;
    }

    @Override
    public boolean IsSuccess(Object model) {
//        add your criteria here
        return false;
    }
}

class TypeC implements SuccessInterface {...}

class TypeD implements SuccessInterface {...}

Upvotes: 0

displayName
displayName

Reputation: 14379

First Way:

private boolean codeUsingALotOfIfElse(Source source, Model model) {
    if (source.isTypeA() && (!model.getPermission || !source.hasAccess)) return false;
    if (source.isTypeB() && (source.condition() && (!model.getPermission() || !source.hasAccess))) return false;
    if (source.isTypeC() && (!source.isMobile || !model.getPermission)) return false;
    if ((source.isTypeD() && source.condition) && (!source.isMobile || !model.getPermission)) return false;
    return true;
}

Second Way:

private boolean codeUsingALotOfIfElse(Source source, Model model) {
    int caseInt = getIntMappingFromSourceType(source); //This function returns a value that can be used for switching.
    switch(caseInt) {
    case *intA*: 
        return (model.getPermission && source.hasAccess));
    case *intB*:
        return !(source.condition() && (!model.getPermission() || !source.hasAccess));
    case *intC*:
        return source.isMobile && model.getPermission;
    case *intD*:
        return !(source.condition && (!source.isMobile || !model.getPermission));
    default:
        return true;
}

Upvotes: 0

Balázs Nemes
Balázs Nemes

Reputation: 729

You can try to use some more polymorphic solution. Make source to be an interface, with an isSuccess method. TypeA, TypeB, TypeC and TypeD are implementations of that interface, all implementing its own version of isSuccess. Then you won't need that whole if statement at all.

return source.isSuccess(model);

class TypeA {
    public boolean isSuccess(Model model) {
        return model.getPermission() && hasAccess();
    }
 ....
}

Upvotes: 1

Carl Manaster
Carl Manaster

Reputation: 40336

Assuming the Types are non-overlapping, a switch statement combined with Jon Skeet's suggestion of mid-function returns gives you this:

switch (source.getType()) {
    case TYPE_A:
        return model.getPermission() && source.hasAccess;
    case TYPE_B:
        return !source.condition() || (model.getPermission() && source.hasAccess);
    case TYPE_C:
        return model.getPermission() && source.isMobile;
    case TYPE_D:
        return !source.condition() || (model.getPermission() && source.isMobile);
    default:
        return false;
}

Upvotes: 0

Related Questions