Lab 14

Code Inspections

In this lab, you will carry out an inspection of the source code for Ant. Download and uncompress the source from http://ant.apache.org/srcdownload.cgi.

You will work with your buddy. One of you writes up a Track+ issue summarizing your results. (Title: Lab 14 / name1 / name2). One or two groups will be randomly selected to present their result at the end of the class.

Use a development environment of your choice. I will write instructions for NetBeans since I know that's what most people use for their project. But you can equally well use Eclipse. Don't use Notepad.

Make a project from existing sources.

Give the project a name such as AntSource.

Then pick the src/main subdirectory of your Ant source.

You should now have a project tree with the Ant source.

1. Look at org.apache.tools.ant.Main, the entry point of Ant. How does the coding convention differ from the SJSU CS style guide? Consider tab size, braces, placement of variables and fields, indentation, method length, etc. List as many differences as you can find.
2. Is the style consistently applied in this file? Across files? Check out classes such as org.apache.tools.ant.taskdefs.cvslib.CvsTagEntry, org.apache.tools.ant.taskdefs.cvslib.CvsTagDiff, and org.apache.tools.ant.taskdefs.Copy for comparison.
3. What consistent style rules can you see that go beyond the rather minimal SJSU style guide rules?
4. Rate the quality of documentation. Are there consistent Javadoc comments? Other helpful comments? Useless comments?
5. Consider the use of protected instance fields. Some authors view them as an indication of poor style, little better than public fields (which we all know is a no-no). Have a look at the org.apache.tools.ant.taskdefs.Copy class which implements the Ant copy task. It uses lots of protected variables. Should these be private? Is there any subclass of Copy? (Right-click, then Tools->Find Usages in NetBeans) Does it use any of the protected fields? Should it?
6. Consider the use of the Vector class. Vector is like ArrayList, except its methods are synchronized, allowing multiple threads to safely mutate the collection. Of course, there is a cost to synchronization, and the standard advice is not to pay for it unless you need it. Look at Vector propertyFiles in org.apache.tools.ant.Main. Is there any thread safety issue? (Hint: Why is it a field, not a local variable?) What improvements, if any, might you suggest?
7. Consider the code in Main to load a properties file:

               Properties props = new Properties();
               FileInputStream fis = null;
               try {
                   fis = new FileInputStream(filename);
                   props.load(fis);
               } catch (IOException e) {
                   System.out.println("Could not load property file "
                      + filename + ": " + e.getMessage());
               } finally {
                   if (fis != null) {
                       try {
                           fis.close();
                       } catch (IOException e) {
                           // ignore
                       }
                   }
               }

   Many programmers are muddleheaded about try blocks, as evidenced by this discussion. This try{} catch{} finally { try{}catch } structure is a tangled mess.

   The smart advice is to decouple the try/finally, whose job is resource cleanup, from try/catch, whose job is exception handling.

   Rewrite this code segment so that it has the form try { try {} finally{} } catch {}. Explain why this form is superior.

8. If you have extra time, poke around the code and tell us something else that is interesting (good, bad, or just remarkable).