A Pragmatic Approach to Automatic Code Reviewing

1
A Pragmatic Approach to Automatic Code Reviewing

• Alan Berg Gilgamesh Nootebos

2
IMPORTANT MESSAGE

• HANDS UP THOSE THAT CAN COMMIT

3
Agenda

• Who are Gilgamesh and Alan?
• What is automatic static code review
• How is it implementated for Sakai?
• History of errors
• Lets attack some bugs
• Recommended reading

4
Who are we?

• Developers, University of Amsterdam
• Central Computer Services / Educational Services
• Responsible for building or supporting large
  scale deployments for all staff and students
  (30,000) maybe 60,000 later
• Small team
• We like going on holidaysTo go on holidays
  systems need to be
• Stable
• Scalable
• Known quality.

5
Automatic static code reviews

• Spots bug patterns without running the code
• Open source, more than one tool, wrapped in Perl
  to extract information
• Findbugs
• PMD
• QJ Pro, JLint, checkstyle etc. Ignore because
  of time constraints

6
Notes on bug patterns

• http//bugs.sakaiproject.org/confluence/display/QA
  /RealbugsfoundinSakaiandrelatedbugpattern

7
PMD / Findbugs

• Sourcecode vs. Bytecode
• Lots of rules(PMDFindbugs 500)
• Well documented
• Java Puzzlers sometimes hard to understand
• PMD mostly belly fluth, but priority 1 2 spot
  some significant issues
• Use XPath to make custom PMD rules
• Findbugs is very accurate but hard to customize
• no_finalizers_David_Haines
• http//pmd.sourceforge.net/rules/

8
PMD XPath example
no_finalizers_David_Haines
ltrule name"no_finalizers_David_Haines"
message"An easy method to leak system
resources" class"net.sourceforge.pmd.rules.XPat
hRule"gt ltdescriptiongtfinalizers
arelt/descriptiongt ltpropertiesgt ltproperty
name"xpath"gt ltvaluegtlt!CDATA //MethodDeclara
tor_at_Image'finalize' gt lt/valuegt
lt/propertygt lt/propertiesgt ltprioritygt3lt/priorit
ygt ltexamplegt lt!CDATA public class Example
protected void finalize() gt
lt/examplegt lt/rulegt
http//www.freesoftwaremagazine.com/columns/destro
y_annoying_bugs_part_4
9
Example report
10
IMPORTANT MESSAGE

• HANDS UP THOSE THAT CAN COMMIT

Sudoku Lego bricks Code Reviewer Reads Java
Puzzlers in his spare time
11
Example Lost in time

• UnsynchronizedStaticDateFormatter
• http//qa1-nl.sakaiproject.org/codereview/trunk/ea
  sy_UnsynchronizedStaticDateFormatter.html

• Possible solutions
• Synchronize access to every call
• Create local objects on each call
• ThreadLocal prevents shared access

12
Please choose one of the following

• Obvious Easy Wins
• Literals firsthttp//qa1-nl.sakaiproject.org/code
  review/bug_dashboard/pmd_literals_first.html
• Unusedhttp//qa1-nl.sakaiproject.org/codereview/b
  ug_dashboard/pmd_unused.html
• Broken if statementhttp//qa1-nl.sakaiproject.org
  /codereview/bug_dashboard/pmd_brokennullcheck.html
  

13
Its a mentality thingJust to get you going.

• Pick a rule any rulehttp//qa1-nl.sakaiproject.or
  g/codereview/trunk/vote.html
• Examples
• Empty if statementshttp//qa1-nl.sakaiproject.org
  /codereview/trunk/easy_EmptyIfStmt.htm
• EmptyTryBlockhttp//qa1-nl.sakaiproject.org/coder
  eview/trunk/easy_EmptyTryBlock.html
• OverrideBothEqualsAndHashcode http//qa1-nl.sakai
  project.org/codereview/trunk/easy_OverrideBothEqua
  lsAndHashcode.html
• MisplacedNullCheckhttp//qa1-nl.sakaiproject.org/
  codereview/trunk/easy_MisplacedNullCheck.html
• UseCollectionIsEmptyhttp//qa1-nl.sakaiproject.or
  g/codereview/trunk/easy_UseCollectionIsEmpty.html
  

14
More, give me mooooreee

• NonStaticInitializerhttp//qa1-nl.sakaiproject.or
  g/codereview/trunk/easy_NonStaticInitializer.html
• OptimizableToArrayCallhttp//qa1-nl.sakaiproject.
  org/codereview/trunk/easy_OptimizableToArrayCall.h
  tml
• UnsynchronizedStaticDateFormatterhttp//qa1-nl.sa
  kaiproject.org/codereview/trunk/easy_Unsynchronize
  dStaticDateFormatter.html
• FinalFieldCouldBeStatichttp//qa1-nl.sakaiproject
  .org/codereview/trunk/easy_FinalFieldCouldBeStatic
  .html

15
Give me more

• Preserve Stack Tracehttp//qa1-nl.sakaiproject.or
  g/codereview/trunk/easy_PreserveStackTrace.html

16
CHEATS

• Some hints for easy to find stuffhttp//qa1-nl.sa
  kaiproject.org/codereview/bug_dashboard/wins.txt
• But does not remove unnecessary null checksif
  (ua null ("".equals(ua)
  !"generic".equals(devID))) if (ua null
  (ua.equals("") !devID.equals("generic")))
• All literalshttp//qa1-nl.sakaiproject.org/codere
  view/bug_dashboard/literals.txt

17
FindBugs

• Accurate
• Non trivial
• Categories
• Multithreaded
• Bad practice
• Performance
• Malicious code
• Style

18
Lets always be correct

• Low false positives, designed to be the first to
  tackle.http//qa1-nl.sakaiproject.org/codereview/
  bug_dashboard/findbugs_CORRECTNESS.html
• EC_UNRELATED_TYPES, EC_UNRELATED_CLASS_AND_INTERFA
  CE
• Null pointershttp//qa1-nl.sakaiproject.org/coder
  eview/bug_dashboard/findbugs_generic_null.html
• Infinite loopshttp//qa1-nl.sakaiproject.org/code
  review/bug_dashboard/findbugs_generic_infinite.htm
  l
• Replaceallhttp//qa1-nl.sakaiproject.org/coderevi
  ew/bug_dashboard/findbugs_generic_return.html

19
More

• Bad Practice
• DE_MIGHT_IGNORE
• GC_UNRELATED_TYPES
• ODR_OPEN_DATABASE_RESOURCE
• RR_NOT_CHECKED
• Multithreaded Correctness
• STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE
• ML_SYNC_ON_FIELD_TO_GUARD_CHANGING_THAT_FIELD
• Correctness
• FE_TEST_IF_EQUAL_TO_NOT_A_NUMBER
• http//qa1-nl.sakaiproject.org/codereview/bug_dash
  board/findbugs_generic_nan.html
• ICAST_INT_CAST_TO_FLOAT_PASSED_TO_ROUND
• UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS
• UPM_UNCALLED_PRIVATE_METHOD

20
Recommended Reading

• Books
• Effective Java Second Edition
• Java Concurrency in Practice
• Java Puzzlers Traps, Pitfalls, and Corner Cases
• Sites
• FindBugs
• PMD