Automatic Code Reviews

1
Automatic Code Reviews in 30 minutes (faster than
the speed of light)
Universiteit van Amsterdam
Central Computing servicesAlan Berg Education
and Research Services Group
2
What we are talking about

• A tool that looks for bugs in code,
• that runs once a night against
• the Sakai code base
• and generates a report Website

3
Agenda

• Context
• What is Static Code Analysis?
• Why Static Code Analysis?
• Why Static Code Analysis NOT?
• The Report wrapper
• Roadmap

4
Context

• Sakai - Large code base
• trunkscorm2004 tool 18th August
• 3131 classes
• 721396 lines of code (including comments and
  white lines)
• 440,000 code.
• 52 sub projects (only in trunk)
• Rapid velocity of change
• API change between 2.1 --gt 2.2
• Code base stored in subversion and thus easily
  accessible
• UvA wishes to deploy OSP as soon as possible.
  Need to know where we are.

5
What is Static Code Analysis?

• Looks at code for bug and style patterns
• Offline
• Also looks for duplicate code
• Generates metrics such as where there is too much
  complexity.
• Findbugs http//findbugs.sourceforge.net/
• QJPro http//qjpro.sourceforge.net/
• PMD http//pmd.sourceforge.net/
• A potential pool of roughly 550 checks and
  rising

6
Static analysis / Functional tests

• Functional tests apply only to specific parts of
  the code.
• Functional tests test the most important parts of
  the code under specific conditions
• Static analysis tests almost all code
• Static analysis pushes up average code quality
• If coupled with Jira may help support debugging

7
RANDOM EXAMPLES --gt Pushing up base quality
8
System.out (2096 examples found) (moving to
log4j)
88 protected String getPassword(String
username) 89 90
System.out.println("DavRealm.getPassword("
username ")") 91 return (null) 92

Ignoring exceptions (772 examples
found) State Feedback to user or debugger
120 String title siteId 121
try 122 123 Site site
SiteService.getSite(siteId) 124
title site.getTitle() 125 126
catch (Exception ignore) 127 128

9
Junk DNA (1418 examples found) remove or expand
553 ResourceProperties props
edit.getProperties() 554 rv2
StringUtil.trimToZero(((BaseAliasEdit)
edit).m_createdUserId) 555 rv3
StringUtil.trimToZero(((BaseAliasEdit)
edit).m_lastModifiedUserId) 556
rv4 edit.getCreatedTime() 557
rv5 edit.getModifiedTime() 558
559 560 return rv 561
Boolean instancing () (performance) Avoid
instantiating Boolean objects you can reference
Boolean.TRUE, Boolean.FALSE, or call
Boolean.valueOf() instead.
context.put("allowSubmit", new Boolean(allowSubmit
))
10
PositionLiteralsFirstInComparisons (959)
(Subtle)Position literals first in String
comparisons - that way if the String is null you
won't get a NullPointerException, it'll just
return false.
if (option.equals("post"))
ComparedObjectsWithEquals (46)
String first"one" String second new
String("one") if (first!second) System.out.pri
ntln("Strange Captain")
683 public void setTempSubject(String
tempSubject) 684 685 // if there's
a change 686 if (tempSubject !
m_tempSubject) 687 688 //
remember the new 689 m_tempSubject
tempSubject 690 691 692 //
setTempSubject()
PMD with pretty default set of rules 66 unique
rules triggered
11
Complexity

• Whoops too long to show examples (2521 pmd)!
• Methods ,Classes
• Imports (coupling)
• Duplication of code
• 107 classes gt 1000 lines in length (Perl code)
• Longest 14,358 lines long (Wow) org.sakaiproj
  ect.content.tool

Bugs
Size
12

• Show us the money

• http//mercator.ic.uva.nl/sakai_review/nightly/bet
  a/trunk/www/

13

• Why Static Code Analysis?

• Findbugs quote most software contains many bugs
  that are easy to find. From experience I find
  this to be true.
• Can be automated and fits snugly in nightly build
  structure
• 50 Objective
• False positives and nags
• Good for training purposes for common programming
  faults
• Hints at defect clustering and code complexity
  Refactoring
• At a gross level can give an indicator of
  quality

14

• Why Static Code Analysis?

• Computer power still doubling every 18 months
• Static Analysis tools improving rapidly
• Unlike Functional tests checks all code thus
  improves general quality

15

• Why Static Code Analysis - NOT?

• False positives
• Complains over minor infringements
• Prioritising is an issue
• Developers need to be motivated to read the
  reports
• Bugs patterns can be at times obscure
• Wrong part of the development cycle

16
The report wrapper Ad hoc solution
Alan Use whiteboard if time allows
Static analysis via PMD,QJPro Findbugs
Import results into Database
Generate entries in fact table
Generate Static website
Which classes belong to which project?
HTMLize Source for navigation
Warning One assumption needed to make all the
code work
17
Features

• Graphical report
• Top suspicious code
• Duplicate code
• Sorted on line number
• Information sits in a database so is easy to
  manipulate
• Uses three tools
• Spots complexity
• Runs nightly against trunk
• Tools have Eclipse PlugIns for training purposes

18

• Roadmap

• Better contact with developers
• Better contact with decision makers
• From prototype to a quality service
• More integration of tools
• Look at qalab for charting?http//qalab.sourcefor
  ge.net/
• Use service for other Educational projects

19

• DETAILED TOUR AND A QUESTION

• http//mercator.ic.uva.nl/sakai_review/nightly/bet
  a/trunk/www/
• How do we motivate developers to use this tool?
• Examples
• Email reports
• Search functionality
• Improved reports
• Coupling with Jira
• Generic background
• Confluence
• Magazine articles

20
QUESTIONS
21
Why three tools? Evolution of the eye
metaphore

• Measures different rules
• Have overlap
• Uses different methods of expanding ruleset
• Stronger at some issues than others
• Different saved data structures
• All have Eclipse plugins
• All can be run from Ant
• Prototype service so may discard weaker tools
  later
• Well known in Market and open source