Uploaded image for project: 'Sakai'
  1. Sakai
  2. SAK-17647 Clean up code via static code review sweep for 2.7
  3. SAK-17652

Static code sweep of sites for 2.7 release finds issues

    Details

    • Type: Sub-task
    • Status: CLOSED
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.0
    • Fix Version/s: 2.7.0, 2.8.0
    • Component/s: Site Browser
    • Labels:
      None
    • Environment:
      Static code review

      Description

      13 issues found that look straight forward to clean up
      Please can you do this quickly

      [1] org.sakaiproject.site.tool.helper.managegroup.impl.SiteManageGroupHandler
      NPE if IdUnusedException on line 213

      try

      { site = siteService.getSite(siteId); } catch (IdUnusedException e) {SiteAssociationBean // The siteId we were given was bogus e.printStackTrace(); }
      }
      update = siteService.allowUpdateSite(site.getId());

      [2] org.sakaiproject.site.tool.AdminSitesAction
      Site can be null (checked on line 462) so potential NPE on line 474

      alias = getSiteAlias(site!=null?site.getReference():"");
      if (alias != null) { String urlAliasFull = aliasBaseUrl + alias; context.put(FORM_URL_ALIAS_FULL, urlAliasFull); }
      context.put(FORM_URL_BASE, aliasBaseUrl);
      context.put(FORM_URL_ALIAS, alias);
      }

      // build the menu
      // we need the form fields for the remove...
      Menu bar = new MenuImpl();
      if (SiteService.allowRemoveSite(site.getId()))

      [3] org.sakaiproject.site.tool.SiteAction
      NPE if siteType==null
      2323, 2328

      if (siteType != null && siteType.equalsIgnoreCase((String) state.getAttribute(STATE_COURSE_SITE_TYPE))) { context.put("isCourseSite", Boolean.TRUE); context.put("isProjectSite", Boolean.FALSE); } else {
      context.put("isCourseSite", Boolean.FALSE);
      if (siteType.equalsIgnoreCase("project")) { context.put("isProjectSite", Boolean.TRUE); }
      }

      [4] SiteAction
      NPE if type==null
      1453,1458
      if (type != null && type.equalsIgnoreCase((String) state.getAttribute(STATE_COURSE_SITE_TYPE))) { context.put("isCourseSite", Boolean.TRUE); context.put("isProjectSite", Boolean.FALSE); } else {
      context.put("isCourseSite", Boolean.FALSE);
      if (type.equalsIgnoreCase("project")) { context.put("isProjectSite", Boolean.TRUE); }

      [5] SiteAction
      3043,3060
      NPE if providerSectionList == null

      if (providerSectionList != null)
      {
      for (String providerSectionId : providerSectionList)
      {
      try
      { Section s = cms.getSection(providerSectionId); providerSectionListTitles.put(s.getEid(), s.getTitle()); }
      catch (Exception e)
      { providerSectionListTitles.put(providerSectionId, providerSectionId); M_log.warn(this + ".putSelectedProviderCourseIntoContext " + e.getMessage() + " sectionId=" + providerSectionId, e); }
      }
      }
      context.put("selectedProviderCourseTitles", providerSectionListTitles);
      context.put("size", new Integer(providerSectionList.size() - 1));


      [6] SiteAction
      7729
      for (int j = 0; j < courseSectionList.size() {
      sections = sections
      + (String) courseSectionList.get(j);
      j++;
      if (j < courseSectionList.size()) { sections = sections + "+"; }

      Method concatenates strings using + in a loop
      The method seems to be building a String using concatenation in a loop. In each iteration, the String is converted to a StringBuffer/StringBuilder, appended to, and converted back to a String. This can lead to a cost quadratic in the number of iterations, as the growing string is recopied in each iteration.
      Better performance can be obtained by using a StringBuffer (or StringBuilder in Java 1.5) explicitly.
      For example:
      // This is bad
      String s = "";
      for (int i = 0; i < field.length; ++i) { s = s + field[i]; }

      // This is better
      StringBuffer buf = new StringBuffer();
      for (int i = 0; i < field.length; ++i) { buf.append(field[i]); }
      String s = buf.toString();

      [7] Please verify line 9065 (& is not &&)

      if (userTypes != null & userTypes.size() > 0)

      Potentially dangerous use of non-short-circuit logic
      This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||). In addition, it seem possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects, could cause an exception or could be expensive.
      Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.
      See the Java Language Specification for details

      [8] org.sakaiproject.site.tool.helper.managegroupsectionrole.impl.SiteManageGroupSectionRoleHandler

      Please review line 381

      try { siteId = sessionManager.getCurrentToolSession() .getAttribute(HELPER_ID + ".siteId").toString(); }
      catch (java.lang.NullPointerException npe) { // Site ID wasn't set in the helper call!! }


      [9] org.sakaiproject.site.tool.helper.managegroupsectionrole.impl.SiteManageGroupSectionRoleHandler
      NPE if IduUnusedException 396

      try { site = siteService.getSite(siteId); }

      catch (IdUnusedException e)

      { // The siteId we were given was bogus e.printStackTrace(); }
      }
      update = siteService.allowUpdaSiteAssociationBeanteSite(site.getId());


      [10] org.sakaiproject.site.tool.helper.managegroupsectionrole.rsf.GroupEditProducer

      NPE at if groupMembers is null line 229,259
      if (groupProviderRoles != null)
      {
      for (String groupProviderRole:groupProviderRoles)
      { groupMemberLabels[i] = ROLE_PREFIX + groupProviderRole; groupMemberValues[i] = groupProviderRole; i++; }
      }
      // add the members last
      Iterator<Member> gIterator = new SortedIterator(groupMembers.iterator(), new SiteComparator(SiteConstants.SORTED_BY_MEMBER_NAME, Boolean.TRUE.toString()));

      [11] org.sakaiproject.site.tool.helper.order.impl.SitePageEditHandler
      NPE at line 140 if IdUnusedException

      try { site = siteService.getSite(siteId); } catch (IdUnusedException e) { // The siteId we were given was bogus e.printStackTrace(); }

      }
      update = siteService.allowUpdateSite(site.getId());

      [12] org.sakaiproject.site.tool.helper.order.impl.Tool

      Nullcheck of tr which would already of been an NPE on line 208

      Properties config = tr.getRegisteredConfig();
      String allowMultiple = config.getProperty(TOOL_CFG_MULTI);

      if (tr != null) {

      [13] org.sakaiproject.siteassociation.tool.helper.siteAssoc.SiteAssociationBean
      i=i ???? line 580

      for(i = i; i >= first; i--){
      Site removedS = assocSites.get;
      assocSites.remove;
      if(searchSitesCompleteCopy.contains(removedS))

      { searchSites.add(removedS); }

      }

        Gliffy Diagrams

          Zeplin

            Attachments

              Activity

                People

                Assignee:
                zqian Zhen Qian
                Reporter:
                a.m.berg@uva.nl Alan Berg
                Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                  Dates

                  Created:
                  Updated:
                  Resolved:

                    Git Integration