reducing number of return statements in a method

Question

I have a java code in which there are multiple return statements in a single method. But for code cleaning purpose, I can have only one return statement per method. What can be done to overcome this.

Here is a method from my code:-

public ActionForward login(ActionMapping mapping, ActionForm form, HttpServletRequest request, HttpServletResponse response) throws Exception {

        // Kill any old sessions
        //request.getSession().invalidate();
        DynaValidatorForm dynaform = (DynaValidatorForm)form;

        // validate the form
        ActionErrors errors = form.validate(mapping, request);
        if(!errors.isEmpty()) {
            this.saveErrors(request, errors);
            return mapping.getInputForward();
        }

        // first check if token is set
        if(!isTokenValid(request, true)) {
            String errmsg="There was a problem with your login. Please close your browser then reopen it and try again. Make sure to click the Login button only ONCE.";
            request.setAttribute("errormessage", errmsg);
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
        }

        // check the form for input errors
        String errmsg = checkInput(form);
        if (errmsg != null) {
            request.setAttribute("errormessage", errmsg);
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
        }

        // no input errors detected
        String resumekey = null;
        // check for valid login
        ObjectFactory objFactory = ObjectFactory.getInstance();
        DataAccessor dataAccessor = objFactory.getDataAccessor();

        request.setCharacterEncoding("UTF-8");
        String testcode = dynaform.getString("testcode").trim();
        String studentname =  dynaform.getString("yourname").trim();


        String password = dynaform.getString("password").trim();

        // 4/3/07 - passwords going forward are ALL lower case
        if (!CaslsUtils.isEmpty(password)) {
            password = password.toLowerCase();
        }

        try{
               resumekey = new String(studentname.getBytes("ISO-8859-1"),"UTF-8");

            } catch (Exception e) {
                log_.error("Error converting item content data to UTF-8 encoding. ",e);
            }

        String hashWord = CaslsUtils.encryptString(password);
        // Make sure this is short enough to fit in the db
        if (hashWord.length() > ConstantLibrary.MAX_PASSWORD_LENGTH) {
            hashWord = hashWord.substring(0, ConstantLibrary.MAX_PASSWORD_LENGTH);
        }

        Login login = dataAccessor.getLogin(testcode, hashWord, false);

        if (login == null || !login.getUsertype().equals(Login.USERTYPE_SUBJECT)) {
            request.setAttribute("errormessage", "Incorrect test code or password.");
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
        }

        // Check if the login has expired
        if (login.getLoginexpires() != null && login.getLoginexpires().before(new Date())) {
            request.setAttribute("errormessage", "Your login has expired.");
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
        }

        // Check if the password has expired
        if (login.getPasswordexpires() != null && login.getPasswordexpires().before(new Date())) {
            request.setAttribute("errormessage", "Your login password has expired.");
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
        }

        HttpSession session = request.getSession();
        session.setAttribute(ConstantLibrary.SESSION_LOGIN, login);
        session.setAttribute(ConstantLibrary.SESSION_STUDENTNAME, studentname);
        List<Testtaker> testtakers = null;
        try {
            //invalidate the old session if the incoming user is already logged in.
            synchronized(this){
            invalidateExistingSessionOfCurrentUser(request, studentname, testcode); 
            testtakers = dataAccessor.getTesttakersByResumeKey(studentname, login);// Adding this code to call getTesttakersByResumeKey instead of getTesttakers to improve the performance of the application during student login
            }
        } catch (Exception e) {
            log.error("Exception when calling getTesttakers");
            CaslsUtils.outputLoggingData(log_, request);
            throw e;
        }
        session = request.getSession();
        if(testtakers!=null)
        {
        if(testtakers.size() == 0) {
            // new student -> start fresh
            log_.debug("starting a fresh test");

            // if this is a demo test, skip the consent pages and dump them directly to the select test page
            if (login.getTestengine().equals(Itemmaster.TESTENGINE_DEMO)) {
                return mapping.findForward("continue-panel");
            }
        }
            // send them to fill out the profile

            // check for custom profiles
            String[] surveynames = new String[4];
            List<Logingroup> logingroups = dataAccessor.getLoginGroupsByLogin(login.getLoginid());
            for(Logingroup logingroup : logingroups) {
                Groupmaster group = logingroup.getGroupmaster();
                log_.debug(String.format("group: {groupid: %d, grouptype: %s, groupname: %s}", new Object[] {group.getGroupid(), group.getGrouptype(), group.getName()}));
                Set<Groupsurvey> surveys = group.getGroupsurveys();
                if(surveys.size() > 0) {
                    // grab the first (and only) one
                    Groupsurvey survey = surveys.toArray(new Groupsurvey[0])[0];
                    if(group.getGrouptype().equalsIgnoreCase(Groupmaster.GROUPTYPE_CLASS)) {
                        surveynames[0] = survey.getSurveyname();
                    } else if (group.getGrouptype().equalsIgnoreCase(Groupmaster.GROUPTYPE_SCHOOL)){
                        surveynames[1] = survey.getSurveyname();
                    } else if (group.getGrouptype().equalsIgnoreCase(Groupmaster.GROUPTYPE_DISTRICT)){
                        surveynames[2] = survey.getSurveyname();
                    } else if (group.getGrouptype().equalsIgnoreCase(Groupmaster.GROUPTYPE_STATE)){
                        surveynames[3] = survey.getSurveyname();
                    }
                }
            }

            // match the most grandular survey
            for(int i=0; i < surveynames.length; ++i) {
                if(surveynames[i] != null) {
                    saveToken(request);
                    return mapping.findForward("student-profile-"+surveynames[i]);
                }
            }
            // no custom profile, send them to the default
            saveToken(request);
            return mapping.findForward("student-profile");
        }

        // get the set of availible panels
        Set<Panel> availiblePanels = dataAccessor.getAvailiblePanels(login, studentname);
        if(availiblePanels.size() == 0) {
            // no panels availible.  send to all done!
            log_.debug(String.format("No panels availible for Login:%s with resumekey:%s", login.toString(), studentname));
            session.setAttribute("logoutpage", true);
            resetToken(request);
            return mapping.findForward("continue-alldone");
        }
        //Eventum #427 - Prevent test takers from retaking a finished test.
        TestSubjectResult testSubjecResult=dataAccessor.getTestSubjectResult(login, resumekey);
        if(testSubjecResult != null){
            if(testSubjecResult.getRdscore() != null && testSubjecResult.getWrscore() != null && testSubjecResult.getLsscore() != null && testSubjecResult.getOlscore() != null){
                if(testSubjecResult.getRdscore().getFinishtime() != null && testSubjecResult.getWrscore().getFinishtime() != null && testSubjecResult.getLsscore().getFinishtime() != null && testSubjecResult.getOlscore().getFinishtime() != null){
                    log_.debug(String.format("Already completed all the Skill Tests.", login.toString(), studentname));
                    session.setAttribute("logoutpage", true);
                    resetToken(request);
                    return mapping.findForward("continue-alldone");
                }
            }
        }
        // get a list of resumeable testtakers
        List<Testtaker> resumeableTesttakers = new ArrayList<Testtaker>();
        for(Testtaker testtaker : testtakers) {
            if(testtaker.getPhase().equals(ConstantLibrary.PHASE_GOODBYE)) {
                // testtaker is done with test.  skip.
                continue;
            }
            if(testtaker.getCurrentpanelid() == null) {
                // testtaker is the profile testtaker
                continue;
            }
            resumeableTesttakers.add(testtaker);
        }
        // sort them from least recent to latest
        Collections.sort(resumeableTesttakers, new Comparator<Testtaker>() {
            @Override
            public int compare(Testtaker o1, Testtaker o2) {
                // TODO Auto-generated method stub
                //return 0;
                return new CompareToBuilder()
                    .append(o1.getLasttouched(), o2.getLasttouched())
                    .toComparison();
            }
        });

        if(resumeableTesttakers.size() == 0 && availiblePanels.size() > 0) {
            // nobody is resumeable but there are panels left to take
            // send them to the panel choice
            // TODO: This is probably a misuse of Struts.
            log_.info("No resumeable testtakers. Sending to panel select");
            saveToken(request);
            ActionForward myForward = (new ActionForward("/do/capstartpanel?capStartPanelAction=retest&lasttesttakerid="
                    + testtakers.get(0).getTesttakerid(), true));
            return myForward;// mapping.findForward(ConstantLibrary.FWD_CONTINUE + "-panel");
        } else {
            // grab the one most recently created and take their test
            log_.info(String.format("Resuming with choice of %d testtakers", resumeableTesttakers.size()));
            // we're forwarding to resume at this point, so we should do the some of the initialization
            // that would have happened if we were still using getTesttaker() instead of getTesttakers() above.

            session.setAttribute(ConstantLibrary.SESSION_LOGIN, login);
            session.setAttribute(ConstantLibrary.SESSION_TESTTAKER, resumeableTesttakers.get(resumeableTesttakers.size()-1));
            saveToken(request);
            return mapping.findForward(ConstantLibrary.FWD_RESUME);
        }

    }

Despoliation

There really is nothing wrong with multiple returns in a method... Sometimes it makes more sense to have multiple returns, sometimes it makes more sense to have a single return. Pick the right one for the situation. Forcing single return per method isn't necessarily the best idea. – Lustrate

See this question for an explanation on where "one return only" came from and why it isn't really necessary any more. – Lustrate

Well, duh, of course use goto to a single return! – Raze

Early returns are a nice way to reduce cyclomatic complexity. – Strode

programmers.stackexchange.com/questions/118703/… – Strode

Answer 1

It's not a worth changing multiple returns to a single return statement per method. Actually, that will unnecessarily increase the burden of storing the result in a local variable and then making the return finally,

ActionForward result = null;
//scenario 1 
result = ... 
//scenario 2
result = ...
//scenario 3
result = ...
//finally
return result;

Hope this helps, but, it doesn't make much sense to me

Patton

Answer 2

As pointed out by others, having a single return statement does not necessarily make your code cleaner. However, in this case splitting up the method in smaller pieces probably makes the code more readable.

For example, this part:

    // first check if token is set
    if(!isTokenValid(request, true)) {
        String errmsg="There was a problem with your login. Please close your browser then reopen it and try again. Make sure to click the Login button only ONCE.";
        request.setAttribute("errormessage", errmsg);
        saveToken(request);
        return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
    }

    // check the form for input errors
    String errmsg = checkInput(form);
    if (errmsg != null) {
        request.setAttribute("errormessage", errmsg);
        saveToken(request);
        return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
    }

could be replaced by introducing two methods and using those to write:

If(tokenNotSet() || formHasErrors()){
    return mapping.findForward(ConstantLibrary.FWD_CONTINUE);
}

By doing this on multiple places the structure of the algorithm becomes more clear, possibly giving you more insight in how this code could be refactored to adhere to your coding guidelines.

Diatomic

So you propose to replace his code with functions that have side effects? In your example, the "tokenNotSet()" function would also call saveToken()? Really bad practice. – Sulfapyridine

The point I was trying to make was that the method is rather long, combining both the outline of the algorithm as well as the implementation of various steps. However, I agree that the example is not the best since it introduces these methods with side-effects. It seems though that the call to saveToken(request) needs to be done before this method returns a value, so ideally we would wrap this whole method in a different method that enforces this behavior. – Diatomic

Answer 3

I would set a an action forward variable at the start of the method.

ActionForward actionForwardToReturn = null;

Then replace each of these two lines

return mapping.getInputForward();

return mapping.findForward(ConstantLibrary.FWD_CONTINUE);

with these two lines :

actionForwardToReturn = mapping.getInputForward()

actionForwardToReturn = mapping.findForward(ConstantLibrary.FWD_CONTINUE);

finally return the variable.

return actionForwardToReturn;

This shouldn't be too difficult :)

On a side note... (actually the orginal answer to the question) :

Multiple return statements can make it hard to debug code.

I personally would have just one action object that you return at the end of the method. The benefit of this, is that i can put a break point right on the return statement and look at exactly what that object is.

Any logging or other cross cutting concern I would want to add later, would only have to be done at one point. Otherwise I would have to add a log statement to every line where you are returning.

Infamous

You've just explained why OP may want to do it. So what? – Cimah

Answer 4

The complexity added to a method in an attempt to remove multiple return statements is many a times not worth it, especially in a method such as yours.
There's nothing wrong with using them in this case.

Weyermann

Answer 5

Like user3580294 there's nothing wrong with multiple return statements. However you could combine the last two if statements since they are essentially returning the same thing.

Use @Octopus 's method if you absolutely have to have one return statement

Casualty