numero ridotto di dichiarazioni di reso in un metodo

Question

Ho un codice java in cui esistono più dichiarazioni di reso in un unico metodo. Ma per la pulizia del codice, posso avere solo una dichiarazione di ritorno per metodo. Cosa si può fare per superare questo.

Ecco un metodo dal mio codice: -

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

vorrei impostare una variabile in avanti un'azione all'inizio del metodo.

ActionForward actionForwardToReturn = null; 

quindi sostituire ciascuna di queste due linee

return mapping.getInputForward(); 

return mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

con queste due linee:

actionForwardToReturn = mapping.getInputForward() 

actionForwardToReturn = mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

infine tornare variabile.

return actionForwardToReturn; 

Questo non dovrebbe essere troppo difficile :)

Su un lato nota ... (in realtà la risposta originale alla domanda):

istruzioni return multipli possono rendere difficile il debug codice.

Personalmente avrei un solo oggetto azione che si restituisce alla fine del metodo. Il vantaggio di questo, è che posso mettere un punto di rottura proprio sulla dichiarazione di ritorno e guardare esattamente che cosa è quell'oggetto.

Qualsiasi registrazione o altro problema trasversale che vorrei aggiungere in seguito, dovrebbe essere eseguito solo in un punto. Altrimenti dovrei aggiungere una dichiarazione di registro a ogni riga in cui stai tornando.

Answer 2

La complessità aggiunta a un metodo nel tentativo di rimuovere più dichiarazioni di reso è molte volte non ne vale la pena, specialmente in un metodo come il vostro.
Non c'è niente di sbagliato nell'usarli in questo caso.

Answer 3

Non vale la pena modificare più ritorni a una singola dichiarazione di ritorno per metodo. A dire il vero, che si inutilmente aumentare il carico di memorizzare il risultato in una variabile locale e quindi rendere il ritorno, infine,

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

Spero che questo aiuti, ma, che non ha molto senso per me

Answer 4

Come user3580294 non c'è niente di sbagliato in più dichiarazioni di ritorno. Tuttavia è possibile combinare le ultime due istruzioni if ​​dato che essenzialmente restituiscono la stessa cosa.

metodo Usa @Octopus s' se assolutamente necessario disporre di una dichiarazione di ritorno

Answer 5

Come sottolineato da altri, avendo un unico prospetto di ritorno non significa necessariamente rendere il più pulito il codice. Tuttavia, in questo caso, suddividere il metodo in parti più piccole probabilmente rende il codice più leggibile.

Ad esempio, questa parte:

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

potrebbe essere sostituito con l'introduzione di due metodi e utilizzando quelli a scrivere:

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

In questo modo su più luoghi la struttura dell'algoritmo diventa più chiaro, possibilmente dando più informazioni su come questo codice potrebbe essere refactored per aderire alle linee guida di codifica.