How to remove The Cyclomatic Complexity

Question

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?

Did you check this: http://stackoverflow.com/questions/5853343/how-can-i-reduce-the-cyclomatic-complexity-of-this? — PseudoAj, May 06 '16 at 06:35
Let's start with this: why are you blanket-catching `Exception`? — Makoto, May 06 '16 at 06:35
Yes. I have already check this and modified my code as much as possible — Shiladittya Chakraborty, May 06 '16 at 06:35
MsgEnum.FG20020.setMsg also is problematic; could be refactored by returning an interface implemented by MsgEnum and a new class wrapping MsgEnum and exception. Best would be throw your own MsgException or such. — Joop Eggen, May 06 '16 at 06:50

Gerald Mücke · Accepted Answer · 2016-05-06T07:15:42.163

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.

score 0 · Answer 2 · answered May 06 '16 at 06:40

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.

How to remove The Cyclomatic Complexity

2 Answers2