CODEWITHSUNDEEP
java
Shiladittya Chakraborty
Shiladittya Chakraborty

Reputation: 4418

How to remove The Cyclomatic Complexity

I have below method

public MsgEnum validateUser(String userId, String pwd, UserOperationEnum userOperatioEnum) {
        try {
            MstCredential mstUser = mstUserDAO.validateUser(userId);    

            if (null == mstUser) {
                return MsgEnum.FG40010;
            }   

            if (!pwd.equals(pUtil.decrypt(mstUser.getPassword()))) {
                return MsgEnum.FG40010;
            }

            if (userOperatioEnum.getOprName().equals(mstUser.getOperation()) && mstUser.getStatus() == OperationStatusEnum.ACTIVE.getMsgCode()) {
                return MsgEnum.FG20000;
            }

            return MsgEnum.FG50010;

        }
        catch(Exception e) {
            LOGGER.error("Error occured while validateStoreUser: "+e.getMessage(),e);
            MsgEnum.FG20020.setMsgDesc(MsgEnum.FG20020.getMsgDesc()+ e.getMessage());
            return MsgEnum.FG20020;
        }
    }

I am getting this exception "The Cyclomatic Complexity of this method "validateUser" is 11 which is greater than 10 authorized."

How can I remove this exception?

Upvotes: 2

Views: 8294

Answers (2)

Gerald Mücke
Gerald Mücke

Reputation: 11132

You have to reduce the number of conditional branches of the method. Every condition increases the complexity.

So first, you should bundle the outcomes

if (null == mstUser) {
  return MsgEnum.FG40010;
}   

if (!pwd.equals(pUtil.decrypt(mstUser.getPassword()))) {
    return MsgEnum.FG40010;
}

can be combined to

if (null == mstUser || !pwd.equals(pUtil.decrypt(mstUser.getPassword()))) {
    return MsgEnum.FG40010;
}

but that does not yet remove the complexity, but makes further refactoring more simple.

Next step is refactor the conditions out into separeate method returning boolean

null == mstUser || !pwd.equals(pUtil.decrypt(mstUser.getPassword()))

to 

boolean isPasswordValid(MstCredential mstUser, String pwd){
  return null == mstUser || !pwd.equals(pUtil.decrypt(mstUser.getPassword()));
}

and 

userOperatioEnum.getOprName().equals(mstUser.getOperation()) && mstUser.getStatus() == OperationStatusEnum.ACTIVE.getMsgCode()

to

boolean isOperationValid(MstCredential mstUser, UserOperationEnum userOperatioEnum){
  return userOperatioEnum.getOprName().equals(mstUser.getOperation()) && mstUser.getStatus() == OperationStatusEnum.ACTIVE.getMsgCode();
}

So the final method looks like

public MsgEnum validateUser(String userId, String pwd, UserOperationEnum userOperatioEnum) {
    try {
        MstCredential mstUser = mstUserDAO.validateUser(userId);    

        if (isPasswordValid(mstUser, pwd)) {
            return MsgEnum.FG40010;
        }

        if (isOperationValid(mstUser, userOperatioEnum)) {
            return MsgEnum.FG20000;
        }

        return MsgEnum.FG50010;

    }
    catch(Exception e) {
        LOGGER.error("Error occured while validateStoreUser: "+e.getMessage(),e);
        MsgEnum.FG20020.setMsgDesc(MsgEnum.FG20020.getMsgDesc()+ e.getMessage());
        return MsgEnum.FG20020;
    }
}

if the complexity is still to high, you could further move the contents of the try-block into a separate method, returning a MsgEnum so the only concern of the method becomes to handle the exception.

Upvotes: 6

dinwal
dinwal

Reputation: 467

since I don't have much details on how individual functions are called, you may want to create multiple functions (each for null value, wrong password and such) so that you do not have multiple execution paths in your function. Cyclomatic complexity of max 10 means your if-else or whatever other conditions cannot result in more than 10 ways to return from a function. In your case there are 11.

Upvotes: 0

Related Questions