2014-05-19 16 views
7

Tôi có một mã java trong đó có nhiều câu lệnh trả về trong một phương thức duy nhất. Nhưng với mục đích làm sạch mã, tôi chỉ có thể có một câu lệnh trả về cho mỗi phương thức. Có thể làm gì để khắc phục điều này.giảm số lượng câu lệnh trả về theo phương thức

Dưới đây là một phương pháp từ mã của tôi: -

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

    } 
+21

Có thực sự là không có gì sai với nhiều lợi nhuận trong một phương pháp ... Đôi khi nó có ý nghĩa hơn để có nhiều lợi nhuận, đôi khi nó có ý nghĩa hơn để có một trở lại duy nhất. Chọn một trong những quyền cho tình hình. Buộc trả lại một lần cho mỗi phương thức không nhất thiết phải là ý tưởng tốt nhất. – awksp

+5

Xem câu hỏi này (https://programmers.stackexchange.com/questions/118703/where-did-the-notion-of-one-return-only-come-from) để giải thích về "chỉ trả lại một lần "đến từ và tại sao nó không thực sự cần thiết nữa. – awksp

+0

Vâng, duh, tất nhiên sử dụng goto để trở lại duy nhất! – PlasmaHH

Trả lời

3

tôi sẽ thiết lập một biến phía trước một hành động vào lúc bắt đầu của phương pháp.

ActionForward actionForwardToReturn = null; 

Sau đó thay thế mỗi hai dòng sau

return mapping.getInputForward(); 

return mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

với hai dòng sau:

actionForwardToReturn = mapping.getInputForward() 

actionForwardToReturn = mapping.findForward(ConstantLibrary.FWD_CONTINUE); 

cuối cùng trở biến.

return actionForwardToReturn; 

này không nên quá khó khăn :)

Trên một mặt lưu ý ... (thực ra câu trả lời cho câu hỏi ban đầu):

Nhiều báo cáo lợi nhuận có thể làm cho nó khó khăn để gỡ lỗi mã.

Cá nhân tôi sẽ chỉ có một đối tượng hành động mà bạn quay trở lại vào cuối phương thức. Lợi ích của điều này, là tôi có thể đặt một điểm break ngay trên báo cáo trả về và nhìn vào chính xác đối tượng đó là gì.

Bất kỳ sự ghi nhật ký hoặc mối quan tâm cắt chéo nào khác mà tôi muốn thêm sau này, sẽ chỉ phải được thực hiện tại một thời điểm. Nếu không, tôi sẽ phải thêm một thông báo tường trình vào mỗi dòng mà bạn đang quay trở lại.

+0

Bạn vừa giải thích tại sao OP có thể muốn làm điều đó. Vậy cái gì? –

+0

có bạn đã đúng. tôi sẽ thay đổi câu trả lời của tôi –

3

Tính phức tạp được thêm vào phương thức nhằm loại bỏ nhiều câu lệnh trả lại nhiều lần không đáng, đặc biệt là trong một phương thức như của bạn.
Không có gì sai khi sử dụng chúng trong trường hợp này.

10

Nó không phải là một giá trị trả về nhiều thay đổi cho một câu lệnh trả về cho mỗi phương thức. Trên thực tế, điều đó không cần thiết sẽ làm tăng gánh nặng lưu trữ kết quả trong một biến địa phương và sau đó làm cho sự trở lại cuối cùng,

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

Hope this helps, nhưng, nó không có ý nghĩa nhiều đối với tôi

3

Giống như user3580294 không có gì sai với nhiều câu lệnh return. Tuy nhiên bạn có thể kết hợp hai câu lệnh if cuối cùng vì chúng chủ yếu trả về cùng một thứ.

phương pháp sử dụng @Octopus 's nếu bạn hoàn toàn phải có một lệnh return

10

Như đã chỉ ra bởi những người khác, có một tuyên bố trở lại duy nhất không nhất thiết phải làm cho mã sạch của bạn. Tuy nhiên, trong trường hợp này, việc chia nhỏ phương thức thành các phần nhỏ hơn có thể làm cho mã dễ đọc hơn.

Ví dụ, phần này:

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

có thể được thay thế bằng cách giới thiệu hai phương pháp và sử dụng những viết:

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

Bằng cách này trên nhiều nơi cấu trúc của thuật toán ngày càng trở nên rõ ràng, có thể cung cấp cho bạn thông tin chi tiết hơn về cách mã này có thể được tái cấu trúc để tuân thủ nguyên tắc mã hóa của bạn.

+3

Vì vậy, bạn đề xuất thay thế mã của mình bằng các hàm có tác dụng phụ? Trong ví dụ của bạn, hàm "tokenNotSet()" cũng sẽ gọi hàm saveToken()? Thực hành xấu. – pkExec

+1

Điểm tôi cố gắng thực hiện là phương pháp này khá dài, kết hợp cả phác thảo của thuật toán cũng như việc thực hiện các bước khác nhau. Tuy nhiên, tôi đồng ý rằng ví dụ này không phải là tốt nhất vì nó giới thiệu những phương pháp này với các tác dụng phụ. Dường như cuộc gọi tới 'saveToken (yêu cầu)' cần được thực hiện trước khi phương thức này trả về một giá trị, vì vậy, lý tưởng là chúng ta sẽ bọc toàn bộ phương thức này theo một phương thức khác để thực thi hành vi này. – ebo

Các vấn đề liên quan