Uploaded image for project: 'Sakai'
  1. Sakai
  2. SAK-36135

Static code review - 2.8 Samigo

    XMLWordPrintable

    Details

    • Previous Issue Keys:
      SAM-1050

      Description

      1) ToolScoreListener Line 168
      pubAssessment is possibly null at this point causing an NPE
      if (!passAuthz(context, pubAssessment.getCreatedBy())){

      2) ShowAttachmentMediaServlet NPE if media is null at line 126
      Caused by caught Exceptions just before

      ByteArrayInputStream byteArrayInputStream = new ByteArrayInputStream(media);

      Trivia
      3) Readability - Class names should start with Capital - inner class - itemComparator - ItemAuthorBean
      4) Readability - Class names should start with Capital - inner class - itemComparator - SectionBean
      5) Readability - Class names should start with Capital - inner class - titleComparator - QuestionPoolBean
      6) QuestinPoolException is not derived from Exception
      – End Trivia

      7) Dangerous
      Resource leak under IOException please use a try catch finally
      HTMLWorker line 88 dos

      FileOutputStream fos = new FileOutputStream(temp);
      DataOutputStream dos = new DataOutputStream(fos);
      dos.write(img.getContent(), 0, (int)img.getContentLength());
      dos.close();
      fos.close();

      8) Media does not throw an Exception it returns a boolean, therefore requires an if statement for the logging
      DeliveryBean - Line 2024
      try

      { if (SAVETODB) media.delete(); }
      catch(Exception e){ log.warn(e.getMessage()); }

      9) Please review - Failing to check if directory deleted
      XMLImportBean - line 336
      private void deleteDirectory(File directory) {
      if(directory.exists()) {
      File[] files = directory.listFiles();
      for(int i=0; i < files.length; i++) {
      if(files[i].isDirectory()) { deleteDirectory(files[i]); }
      else { files[i].delete(); }
      }
      }
      directory.delete();
      }

      10) XMLImportBean - Line 445
      Method ignores exceptional return value
      This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.

      // remove uploaded file
      try{ //System.out.println("****filename="+fileName); File upload = new File(fileName); upload.delete(); }
      catch(Exception e){ e.printStackTrace(); }
      }


      11) Failed to check that directory is made and log and return early
      UploadAudioMediaServlet - Line 108 - mediaDir.mkdirs();


      12) Media does not throw an Exception it returns a boolean, therefore requires an if statement for the logging
      UploadAudioMediaServlet - Line 435
      try{ if (SAVETODB) media.delete(); }

      catch(Exception e)

      { log.warn(e.getMessage()); }

      return mediaId.toString();
      }

      13,14) Worth a review, not sure of impact.

      Synchronization performed on util.concurrent instance
      This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have there own concurrency control mechanisms that are distinct from and incompatible with the use of the keyword synchronized.

      TimeAssessmentQueue ( queue = new ConcurrentHashMap ()
      Line 61 - synchronized (queue) {
      Line 76 - synchronized (queue) {

      15,16,17) 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();

      15) ExtractionHelper - Line 1586 - score += iter.next();
      16) PDFAssessmentBean - Line 301 - content += "<tr>" + getContentAnswer(item, answer, printSetting) + "</tr>";
      17) SaveAssessmentSettings - Around line 456

      while (releaseToGroupsIter.hasNext()) {
      String group = (String) releaseToGroupsIter.next();
      releaseToGroupsAsString += group;
      if (!group.equals(lastGroup) )

      { releaseToGroupsAsString += ", "; }

      }

      18) document never used, but shadows constructor document. Please remove for maintainability
      XmlStringBuffer - Line 375
      Document document = this.getDocument();

      19,20,21) Result of integer multiplication cast to long (potential overflow)
      This code performs integer multiply and then converts the result to a long, as in:

      long convertDaysToMilliseconds(int days)

      { return 1000*3600*24*days; }


      If the multiplication is done using long arithmetic, you can avoid the possibility that the result will overflow. For example, you could fix the above code to:

      long convertDaysToMilliseconds(int days)

      { return 1000L*3600*24*days; }


      or

      static final long MILLISECONDS_PER_DAY = 24L*3600*1000;
      long convertDaysToMilliseconds(int days)

      { return days * MILLISECONDS_PER_DAY; }

      19) TimedAssessmentGradingModel - line 76 - this.expirationDate = new Date(beginDate.getTime() + timeLeft*1000);
      20) TimedAssessmentGradingModel - line 96 - this.expirationDate = new Date(beginDate.getTime() + timeLeft*1000);
      21) SubmitTimedAssessmentThread - line 113 - if (currentTime > (bufferedExpirationTime + timedAG.getTransactionBuffer()*1000)){

      22) Redundant if statement, you know that builder is null
      XmlStringBuffer - Line 268
      DocumentBuilder builder = null;
      StringReader sr = null;
      org.xml.sax.InputSource is = null;
      try
      {
      if(builder == null)

      23) DeliveryActionListener - Line 1463
      ResourceLoader rb = null;
      if (rb == null) {

      24,25,26,27) Useless control flow
      24) AssessmentFacadeQueries - Line 173
      if (a.getIsTemplate().equals(Boolean.FALSE)) {
      }

      25) HTMLWorker - Line 315
      if (vspace != null && !"".equals(vspace)) {

      }

      26) ItemHelper20Impl - Line 201
      if (itemXml.isMatching())

      { // process matching // return; }

      27) StartCreateItemListener - Line 260

      if (item.getItemType().equals("10"))

      { // do not set section here, sections are set by the editPool form // QuestionPoolBean qpoolBean= (QuestionPoolBean) cu.lookupBean("questionpool"); //qpoolBean.setSelectedSection(item.getSelectedSection()); }

      28) Do you mean equals or do you want to compare object references?
      AssessmentHelperBase - Line 339
      if (timeLimit != null && timeLimit != Integer.valueOf(0))

      29) Do you mean equals or do you want to compare object references?
      ItemAddListener - Line 1364
      if (choiceSequence == matchSequence) {

      30,31,32) Please review

      Certain swing methods needs to be invoked in Swing thread
      (From JDC Tech Tip): The Swing methods show(), setVisible(), and pack() will create the associated peer for the frame. With the creation of the peer, the system creates the event dispatch thread. This makes things problematic because the event dispatch thread could be notifying listeners while pack and validate are still processing. This situation could result in two threads going through the Swing component-based GUI – it's a serious flaw that could result in deadlocks or other related threading issues. A pack call causes components to be realized. As they are being realized (that is, not necessarily visible), they could trigger listener notification on the event dispatch thread.

      30) AudioPanel - Line 296 -f.pack();
      31) AudioRecorder - Line 1032 - f.pack();
      32) AudioRecorderApplet - Line 150 - f.pack();

        Gliffy Diagrams

          Zeplin

            Attachments

              Issue Links

                Activity

                  People

                  Assignee:
                  jthapa Jasmine Thapa (Inactive)
                  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