Reduzierung der Anzahl der Rückgabeanweisungen in einer Methode

Question

Ich habe einen Java-Code, in dem mehrere Return-Anweisungen in einer einzigen Methode sind. Aber zum Zweck der Code-Reinigung kann ich nur eine Return-Anweisung pro Methode haben. Was kann getan werden, um dies zu überwinden?

ist hier ein Verfahren von meinem 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

ich eine Aktion nach vorne Variable zu Beginn des Verfahrens festgelegt würde.

ActionForward actionForwardToReturn = null; 

Dann jeder dieser beiden Leitungen ersetzen

return mapping.getInputForward(); 

return mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

mit diesen zwei Zeilen:

actionForwardToReturn = mapping.getInputForward() 

actionForwardToReturn = mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

schließlich die Variable zurück.

return actionForwardToReturn; 

Dies sollte nicht allzu schwierig sein :)

On a side note ... (eigentlich die ursprüngliche Antwort auf die Frage):

mehr return-Anweisungen können es schwer zu debuggen Code.

Ich persönlich hätte nur ein Aktionsobjekt, das Sie am Ende der Methode zurückgeben. Der Vorteil davon ist, dass ich einen Breakpoint direkt auf die Return-Anweisung setzen kann und genau sehe, was dieses Objekt ist.

Jede Protokollierung oder andere Querschneiderei, die ich später hinzufügen möchte, müsste nur an einem Punkt erfolgen. Andernfalls müsste ich jeder Zeile, in die Sie zurückkehren, eine Protokollanweisung hinzufügen.

Answer 2

Die Komplexität, die einer Methode hinzugefügt wird, um mehrfache Rücksendeanweisungen zu entfernen, ist viele Male nicht wert, besonders in einer Methode wie der Ihren.
Es ist nichts falsch daran, sie in diesem Fall zu verwenden.

Answer 3

Es lohnt sich nicht, mehrfache Rückgaben zu einer einzelnen Rücksendeanweisung pro Methode zu ändern. Tatsächlich erhöhen, das wird unnötig die Last der das Ergebnis in einer lokalen Variablen speichern und dann machen die Rückkehr schließlich

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

hoffe, das hilft, aber ist es nicht machen viel Sinn für mich

Answer 4

Wie user3580294 es gibt nichts falsch mit mehreren Return-Anweisungen. Allerdings könnten Sie die letzten zwei if-Anweisungen kombinieren, da sie im Wesentlichen dasselbe zurückgeben.

Verwenden @Octopus ‚s Methode, wenn Sie unbedingt haben eine return-Anweisung von anderen

Answer 5

Wie bereits ausgeführt haben, wird eine einzige return-Anweisung, die nicht unbedingt den Code sauberer machen. In diesem Fall macht das Aufteilen der Methode in kleinere Teile den Code wahrscheinlich lesbarer.

Zum Beispiel dieser Teil:

// 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); 
    } 

kann durch die Einführung von zwei Methoden ersetzt werden, und diejenigen zu schreiben, mit:

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

dies die Struktur des Algorithmus mehr auf mehreren Orten Dadurch wird klar und geben Ihnen möglicherweise mehr Einblick, wie dieser Code umstrukturiert werden könnte, um Ihre Kodierungsrichtlinien einzuhalten.