Reducing the number of return statements in a method

Question

I have Java code in which there are several return statements in one method. But for the purpose of clearing the code, I can only have one return statement for each method. What can be done to overcome this.

Here is 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); } } 

Answer 1

You should not change multiple returns into one return statement for each method. In fact, this will unnecessarily increase the load on storing the result in a local variable, and then finally return.

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

Hope this helps, but it makes no sense to me.

Answer 2

As pointed out by others, having one return statement does not necessarily make your code cleaner. However, in this case, dividing the method into 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); } 

can be replaced in two ways and used for recording:

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

By doing this in several places, the structure of the algorithm becomes more understandable, giving you a better idea of ​​how this code can be reorganized to adhere to your encoding rules.

Answer 3

I would set the direct action variable at the beginning 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; 

It should not be too difficult :)

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

Several return statements can make it difficult to debug code.

I personally will only have one action object, which you will return at the end of the method. The advantage of this is that I can put a breakpoint directly on the return statement and see what exactly this object represents.

Any posting or other cross-cutting concern that I would like to add later would only have to be done at one point. Otherwise, I would have to add a log statement to every line you return to.

Answer 4

The complexity added to the method in an attempt to delete several return statements is not worth it many times, especially in a method like yours.
In this case, there is nothing bad to use.

Answer 5

Like user3580294, there is nothing wrong with multiple return statements. However, you could combine the last two if statements, since they essentially return the same thing.

Use the @Octopus method if you absolutely need to have one return statement