Best way for Cyclomatic complexity reduction

Question

Below Method has cyclomatic complexity of 13 i follow some approach using ternary operator for small if else, but for long code what is best approach what design pattern use for resuce if else condition or any other way to reduce cyclomatic complexity as sonarQube Shows Cyclomatic Complexity error. I am newbie help me for undersatnding the process

  @RequestMapping(value = "/submitUser", method=RequestMethod.POST)
            public String submitUser(@ModelAttribute User userBean,Locale locale, RedirectAttributes redirectAttributes, HttpSession session, Model model) {
        boolean status = false, isAdd = false;
        String imagePath = "", task = "";
        isAdd = userBean.getAdd();
        /*if(isAdd) {
                    task = "add";
                } else {          // replace this by task=isAdd?"add":"edit";
                    task = "edit"; //
                }*/
                GlobalLogger.logApplicationDebugLog("Received request to " + task + " user ", LOGGER);
        if (session != null && session.getAttribute("UserImagePath") != null) {
            imagePath = session.getAttribute("UserImagePath").toString();
                }
                int currentUserId = Integer.parseInt(session.getAttribute(SessionKeyConstants.USER_ID).toString());
                try {
                    status = iuser.submitUser(userBean, imagePath,currentUserId);
                    if(isAdd) {
                        if (status) {
                            GlobalLogger.logInfoLog("User "+userBean.getUserName()+" has been " + task + "ed successfully", LOGGER);
                            redirectAttributes.addFlashAttribute("successMsg", messageSource.getMessage("operationMsg.addUserSuccess", new String[] {userBean.getEmpEmail()}, locale));
                            return "redirect:/users.action";
                        } else {
                            GlobalLogger.logApplicationDebugLog("Error in adding user "+userBean.getUserName(), LOGGER);
                            redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage("operationMsg.addUserFailure", new String[] {}, locale));
                            redirectAttributes.addFlashAttribute("isAdd", isAdd);
                            redirectAttributes.addFlashAttribute("userBean", userBean);
                            return "redirect:/addUser.action?isAdd=true";
                        }
                    } else {
                        if (status) {
                            GlobalLogger.logInfoLog("User "+userBean.getUserName()+" has been " + task + "ed successfully", LOGGER);
                            redirectAttributes.addFlashAttribute("successMsg", messageSource.getMessage("label.addSuccessMsg", new String[] {userBean.getFirstName()}, locale));
                            return "redirect:/users.action";
                        } else {
                            GlobalLogger.logApplicationDebugLog("Error in adding user "+userBean.getUserName(), LOGGER);
                            redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage("label.addFailedMsg", new String[] {userBean.getFirstName()}, locale));
                            redirectAttributes.addFlashAttribute("isAdd", isAdd);
                            redirectAttributes.addFlashAttribute("userBean", userBean);
                            return "redirect:/addUser.action?isAdd=false";
                        }
                    }
                } catch (UserException e) {
                    GlobalLogger.logApplicationDebugLog("User Name or Employee Id exists for user "+userBean.getUserName(), LOGGER);
                    redirectAttributes.addFlashAttribute("userBean", userBean);
                    redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage(e.getErrorcode(), new String[] {}, locale));
                    return "redirect:/addUser.action?isAdd=true";
                }
            }

@assylias i couldn't understand could you help me by seeing the above code that i posted — vaib, Mar 04 '15 at 07:19

pkalinow · Accepted Answer · 2015-03-04T08:21:16.197

The most straightforward resolution of a problem with too long methods is breaking them into pieces. Extract parts of the method into separate methods called from the original method. Usually it can be done automatically with IDE, e.g. Eclipse (menu Refactor/Extract method).

In your method you have almost the same code for isAdd==true and for false. You can extract this code to a separate method, eg. String redirectUser(boolean isAdd, boolean status, <other attribs>) {...}. Your method will then be much simpler.

The first change is simple - extract method (this can be done automatically):

    @RequestMapping(value = "/submitUser", method=RequestMethod.POST)
public String submitUser(@ModelAttribute User userBean,Locale locale, RedirectAttributes redirectAttributes, HttpSession session, Model model) {
    boolean status = false, isAdd = false;
    String imagePath = "", task = "";
    isAdd = userBean.getAdd();
    GlobalLogger.logApplicationDebugLog("Received request to " + task + " user ", LOGGER);
    if (session != null && session.getAttribute("UserImagePath") != null) {
        imagePath = session.getAttribute("UserImagePath").toString();
    }
    int currentUserId = Integer.parseInt(session.getAttribute(SessionKeyConstants.USER_ID).toString());
    try {
        status = iuser.submitUser(userBean, imagePath,currentUserId);
        return redirectUser(userBean, locale, redirectAttributes, status,
                isAdd, task);
    } catch (UserException e) {
        GlobalLogger.logApplicationDebugLog("User Name or Employee Id exists for user "+userBean.getUserName(), LOGGER);
        redirectAttributes.addFlashAttribute("userBean", userBean);
        redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage(e.getErrorcode(), new String[] {}, locale));
        return "redirect:/addUser.action?isAdd=true";
    }
}

private String redirectUser(User userBean, Locale locale,
        RedirectAttributes redirectAttributes, boolean status,
        boolean isAdd, String task) {
    if(isAdd) {
        if (status) {
            GlobalLogger.logInfoLog("User "+userBean.getUserName()+" has been " + task + "ed successfully", LOGGER);
            redirectAttributes.addFlashAttribute("successMsg", messageSource.getMessage("operationMsg.addUserSuccess", new String[] {userBean.getEmpEmail()}, locale));
            return "redirect:/users.action";
        } else {
            GlobalLogger.logApplicationDebugLog("Error in adding user "+userBean.getUserName(), LOGGER);
            redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage("operationMsg.addUserFailure", new String[] {}, locale));
            redirectAttributes.addFlashAttribute("isAdd", isAdd);
            redirectAttributes.addFlashAttribute("userBean", userBean);
            return "redirect:/addUser.action?isAdd=true";
        }
    } else {
        if (status) {
            GlobalLogger.logInfoLog("User "+userBean.getUserName()+" has been " + task + "ed successfully", LOGGER);
            redirectAttributes.addFlashAttribute("successMsg", messageSource.getMessage("label.addSuccessMsg", new String[] {userBean.getFirstName()}, locale));
            return "redirect:/users.action";
        } else {
            GlobalLogger.logApplicationDebugLog("Error in adding user "+userBean.getUserName(), LOGGER);
            redirectAttributes.addFlashAttribute("errorMsg", messageSource.getMessage("label.addFailedMsg", new String[] {userBean.getFirstName()}, locale));
            redirectAttributes.addFlashAttribute("isAdd", isAdd);
            redirectAttributes.addFlashAttribute("userBean", userBean);
            return "redirect:/addUser.action?isAdd=false";
        }
    }
}

Now, you can compare the two cases of the if, because they are very similar. Maybe you can rewrite them to avoid duplication. For example, instead of return "redirect:/addUser.action?isAdd=true"; you can write return "redirect:/addUser.action?isAdd=" + isAdd.

Refactoring is an iterative process. You should make small changes, run unit tests to ensure that nothing has broken, and repeat until the result is satisfying.

Now you have an example. I won't work more on this code - it's your job, not mine :) — pkalinow, Mar 04 '15 at 08:23
i am using spring tool suite it doesn't show code (refactor/extract method) — vaib, Mar 04 '15 at 08:50
STS is bases on Eclipse. It contains the "Refactor" menu with "Extract Method..." option. — pkalinow, Mar 04 '15 at 09:16

Best way for Cyclomatic complexity reduction

1 Answers1