Reviewing code quality of Apache Sling using Sonar

by freddy mallet|

    A few weeks ago Michael Marth, who runs dev.day.com (Day's developer portal), asked us if we could put together our impressions on the code quality of Apache Sling using Sonar. We thought it would be valuable to share the result of this exercise with the community.

    Apache Sling in a few words



    "Apache Sling is an innovative web framework that is intended to bring back the fun to web development. It uses all those nice cool and new technologies that make up a state-of-the-art framework. This is Apache Sling in five bullets:

    • REST based web framework
    • Content-driven, using a JCR content repository
    • Powered by OSGi
    • Scripting inside, multiple languages
    • Apache Open Source project




    Some size indications of the project


    • 40 Maven modules
    • 70,707 lines of code
    • 731 Java classes
    • and 23,043 lines of Javadoc


    The strengths in terms of quality


    • A project that you get and compile with no difficulty by running two commands :
           1. svn checkout https://svn.apache.org/repos/asf/sling/trunk/
           2. mvn clean install
      This sounds like an evidence but is not always the case :-)
    • Amongst 130,172 physical lines, only 0.9% are involved in a duplication
    • 46.4% of public API are commented with a Javadoc block


    The weaknesses


    • Only 9% of the source code is covered by 338 unit tests
    • Average cyclomatic complexity by method (excluding getters and setters) is greater than 3 (3.2).
      That is kind of a warning saying "your methods are taking too much responsibilities and should be re-factored". This warning is confirmed by others metrics : 394 methods have a complexity greater than 7 and 86 methods have more than 50 statements. What is true at method level gets also partially confirmed at class level as 60 classes have a Fan Out Complexity greater than 20 (The number of other classes referenced by a class)



    Bad programming practices that should be improved


    • 198 times, method parameters are reassigned in the core of the method
    • 68 times, local variables are defined and hide class fields
    • 28 times, NullPointerException are thrown when an IllegalParameterException would be more suitable



    Potential bugs that should be quickly analyzed


    • Correctness - An apparent infinite recursive loop : there is an apparent infinite recursive loop in org.apache.sling.scripting.jsp.jasper.runtime.JspContextWrapper.include(String, boolean)
    • Multithreaded correctness - Unsynchronized get method, synchronized set method : org.apache.sling.scripting.jsp.jasper.compiler.JspRuntimeContext.getJspReloadCount() is unsynchronized, org.apache.sling.scripting.jsp.jasper.compiler.JspRuntimeContext.setJspReloadCount(int) is synchronized
    • Multithreaded correctness - Method calls Thread.sleep() with a lock held : org.apache.sling.event.impl.JobEventHandler.runJobQueue(String, JobBlockingQueue) calls Thread.sleep() with a lock held
    • Malicious code vulnerability - Field is a mutable array : org.apache.sling.jcr.webdav.impl.servlets.SlingWebDavServlet.COLLECTION_TYPES_DEFAULT is a mutable array


    This analysis was done with the intention of giving a synthetic overview of the current state of the project. Where should you start from if tomorrow you wake up with a single idea in mind : "Improving quality of the Apache Sling project !" ?

    • With respectively a cyclomatic complexity of 428, 385 and 343, classes Generator, Parser and XMLEndoginDetector should be first refactored. With no surprise, the Generator.java file has the greatest number of duplicated lines (154) and rules violations (109)
    • With its 43 cyclomatic complexity and no unit tests, the method ModifyAceServlet.handleOperation(..) is what we call "a crappy method" :-)


    More information on the code quality of the project is available on Nemo.