PMD Applied

Simon P. Chappell writes "It's a fundamentally agreed fact within our industry that code reviews are good. Really good. Sliced bread good. But have you actually tried organizing one? If you can get everyone together that needs to be there at the same time in the same meeting room, then you still have the challenge of trying to keep a roomful of geeks from getting trapped in minutiae and squabbling over details like formatting conventions. Well, what if I told you that you could get your code reviews done in less than five minutes and that there would be no arguing? Enter PMD, an open-source Java static analyzer. Think of it as a code review in a box. As if that weren't wonderful enough, there's even a book, PMD Applied, written by Tom Copeland, the author of PMD." Read on for the rest of Simon's review.

PMD Applied
author	Tom Copeland
pages	221
publisher	Centennial Books
rating	9
reviewer	Simon P. Chappell
ISBN	0976221411
summary	A good book if you use or are considering using PMD for static analysis of Java code

I hope that it's not too much of stretch to suggest that this book is primarily written for users of PMD. That said, I actually purchased it (yup, this one wasn't a freebie) because I was considering a recommendation to use PMD as part of the build process at a previous employer. This is not a slam on the documentation on the PMD website, but Mr. Copeland was offering signed editions of the book if you pre-ordered and I wanted to add to my growing collection of books signed by the author. (No contest; geeky as charged.)

PMD Applied has ten chapters. The first chapter is the mandatory introduction and the second covers installation. Nothing unusual there. Chapter three covers using PMD from within Ant or Maven. I'm glad to see both build systems covered here. Both have a pretty good sized user base and both deserve to be represented. Chapter four covers using PMD within an IDE. The range of IDEs covered is excellent and includes all of the ones that I would have covered and a couple of others that I hadn't heard of.

Chapter five begins the serious job of using PMD for static analysis by examining the ability of PMD to detect duplicate code using the wonderfully named Copy and Paste Detector (CPD). Chapter six is titled Best Practices. It's a wonderful collection of advice from Mr. Copeland, on how to begin applying PMD to your Java development process. As a Team Leader, I understand many of the points he makes. Teams generally don't enjoy having impartial tools dumped on them in the middle of a project that do nothing but nitpick their code and programming style; such things are a fast-track to being unpopular. Mr. Copeland's guidelines will increase the chance of your team accepting PMD, as you start with a subset of helpful rules with low chances of false positives.

For all of its cleverness, PMD is nothing without well written rules and chapter seven has this covered. PMD ships with a good sized rule set of its own, even though not all of these will be suitable for every site. Creating custom rule sets is an important first step in customizing PMD for your team. Once the team is used to PMD, they're likely to want to start adding new custom rules to the mix and the book has you covered there as well, with instructions on writing rules with both Java and XPath expressions.

Chapter eight addresses the matter of customizing PMD. Naturally, making changes to the code and recompiling is the first customization covered, but then it looks at custom report formats, adding new languages for the CPD and advice on making contributions of code to the PMD project. Chapter nine is a look at the internals of PMD. Now, perhaps this won't be of much interest to some of the readers of this book, but I found it fascinating. There's nothing quite like having the internal workings of a useful tool explained clearly by the person who wrote it. If you don't like it, it's only twenty pages, so skip over it and check out chapter ten where the playing field of similar open-source tools is examined. The book wraps up with a large appendix with a very useful explanation of all of the rules that ship in the standard PMD deliverable.

I'm going to let a good part of my personal biases show here, but I absolutely love slim and detailed technical books. PMD Applied is 221 pages, which is about the size of the pre-ANSI version of Kernighan and Ritchie's The C Programming Language; the greatest technical book in the known universe. This is the perfect size for a technical book, allowing the author to introduce their subject, explain everything that needs saying, wrap up and be done before the reader's eyes roll back into their head and information overload descends upon them. This book is perfectly sized.

The level of technical discussion matches the size of the book and is just right. While the first few chapters deal with what is PMD and how do you install and run it, the rest of the book deals with good solid code analysis using PMD. This is why we bought the book. We're geeks ... show us the code! Mr. Copeland doesn't disappoint us and there are many excellent code examples throughout the book.

The appendix is a snapshot of the rules that shipped with PMD at the time of the book's publishing. While new rules have been added since this time, the list of every rule and a portion of example code that would trigger that rule are useful. Even if you don't use PMD, buy the book so you can use the appendix as a comprehensive list of examples of how not to write Java code!

After having been out for a year, the exact level of the software described in the book has been passed by. This may bother you, but I feel that the basic principles of PMD have remained true and so I wouldn't let the version numbers dissuade you from purchasing.

The book is only available from the publishers, Centennial Books. They're a smaller publisher, but I had no problem with purchasing through their website.

If you use or are considering using PMD for static analysis of Java code, then this book should be by your side."

---

Slashdot welcomes readers' book reviews -- to see your own review here, read the book review guidelines, then visit the submission page.

Misses the point

Code review is as much about semantics as syntax. It's great to be able to find duplicated code and the like, but what about more subtle mistakes? The only way you will really be able to benefit from code reviews is if you frequently have experienced eyes going over the code. In addition, there are lots of suggestions around for how to set up a code review to avoid squabbling (reviewers should only be looking for specific problems, code formatting is not one of them).

Re:Misses the point

           (if the purpose
    of code) {
is             to be }
[    understood,       then
  formatting        standards]
//       can     really     help.

Re:Misses the point

I've felt for many years that source should be stored as syntax trees and that editors should show you the source how every _you_ want to see it so it's most clear to you, not to the author.

PMD defined

WTF is PMD???

Look at the sourceforge definition of PMD [sourceforge.net].

I have

ran many code reviews, most of them very succesfully. They just need someone to run them that isn't attached to the code.

still need human eyes

There are lots of static checkers, and they're valuable to use before a formal review. The real usefulness of human reviewers is to find bugs in the logic, not the coding. Math errors, bad assumptions, missed corner cases, etc.

Static analysis is the sh-t

Sewwiously [homestarrunner.com]. I don't know why Microsoft doesn't (appear to) invest in static analysis in their code... Imagine outputting a to-do list with, say, *every* freaking buffer overflow exploit in the Windows code tree.

everyone's got a different name for it

Think of it as a code review in a box.

Hey, if you want to call it [youtube.com] a "code review", that's your prerogative.

Business logic? Algorithms?

Err... call me old-fashioned, but aren't code reviews supposed to focus on the business logic implementation, potential side effects, and the algorithms used? Don't get me wrong, it's nice that to have an automated tool to check syntax and for duplicated code, but that's not where the savings comes from. Until you get a human level A.I. to do code reviews, automated code review tools are just gravy and not the meat to solving code problems.

Static Analyzer Run != Code Review

Enter PMD, an open-source Java static analyzer. Think of it as a code review in a box.

No, no, no!

Neither of these development tools is a substitute for the other. Static analyzers are very good at catching *certain* *limited* *kinds* of obvious and likely or conceivable bugs in a short span of time, but in general they cannot deduce the intentions of the developer -- let alone the desires of the user. That's where your peers come in: they review your designs before you write the code and review patches prior to merging them into a revision control system. Static analyzers are *debugging tools*: use them while coding and/or when you run regresssion tests.

Why wasn't that obvious to the submitter?

How good is that code?

My friend wrote a syntax analyzer to report gross statistics about code. Calculate the ratio of number of lines of comments to lines of code, average number of lines per function, etc etc. Finally it would come up with a let grade between A and F for code qualtiy based on some heuristics. That was his masters project.

Our diabolical prof fed this analysis code itself to it as data. It spat out a D. He got a D.

Code collaborator is a good option.

I've used code collaborator [smartbearsoftware.com] and I find it pretty good. The interface can be a little limiting sometimes, but overall a good product that allows the reviewers to review from their office when they have time to do it. This can occasionally lead to the review dragging on, but the convenience is worth it.

findbugs is better

I've used both PMD and findbugs and PMD is pretty good, but I find findbugs to be much easier to set up and use. (http://findbugs.sourceforge.net/ [sourceforge.net])

PMD - OK, but...

I've used PMD, and it's quite good, but irritating at times. It complained when I was creating an object within a loop. Now what I had to do was populate a list of objects so the whole point was to create multiple objects, using a loop. But it still complained...

It's like checkstyle - good for catching some things, but sometimes too irritating to take seriously.

Structural analysis for Java

I really thought that IBM was headed somewhere when they cooked up Structural analysis for Java
http://www.alphaworks.ibm.com/tech/sa4j [ibm.com] back in the 04's.

Yes, heuristics can catch probable mistakes in developed code, but the above sort of thing, if sophisticated enough, could warn you about serious architectural mistakes as you are making them.

Finding Time For Code Reviews Is The Problem

I used to hate going into code reviews at this one place I worked. Because nobody had time to prepare for it properly, it often collapsed into a series of "You need to indent 4 spaces instead of 3" kind of criticisms, while ignoring issues like code reuse, poorly commented code, and misuse of basic OOP principles.

I don't believe a machine, no matter how well programmed, can replace a proper review done by real live human beings. But to do that, you need to give people the time to do it right. Unfortunately, the project managers in the companies I've worked for didn't really believe in a structured programming approach. It's a big waste of time, and programmers are being paid to write code,not sit around talking about code.

Oh well, these are the same managers who feel that functional and technical specs are also a waste of time. I believe this is called the Redmond Programming Methodology. AKA "Quality is Job 1.1"
 

What happened to Slashdot?

Where is all the whining about Slashdot linking off to some evil corporate bookseller, when the book can be acquired for cheaper at Amazon?

This software cannot ever work properly.

This was proved to not be viable in 1936 when Alan Turing proved that no machine given a description of a program and a finite input can decide whether the program finishes running or will run forever, given that input.

Thus a general algorithm to solve the halting problem for all possible program-input pairs cannot exist, let alone validate the semantic approach or validitiy of the code itself.

Unfortunately there are enough managers who want to devalue software development from a skilled art into a brain-dead process, so they will buy into this no matter how invalid it really is.

Defence contracting

Used to do this all the time when writing software for defence / weapons systems. (in Oz at least)

Its easy - the source is printed out and circulated .. peers have 1 week to circulate the code and make markups in coloured pen of their choice, after which a formal code review meeting is held.

It is the responsibility of each peer to be familiar with the requirements spec (even if its not part of their project), and the design that has been signed off. If they dont agree with the design - too bad - the process allows for plenty of time to look at design options, document those options, and document reasons for choosing one approach above another. Dont waste people's time by trying to flog a dead horse at a code review.

The meetings are known in advance, so its never a problem getting people in the same room at the same time.

Code review meeting is informally divided into 4 sections :

1) Intro - if this is NOT the first code review, quickly list the TODO items that came up from the last code review. Good developers will refer to these recurrent issues quickly, and stick to making use of the time by covering new ground. 10 minutes to discuss any code formatting / spelling / naming convention issues. These ARE important for maintainability and consistency, so egos need to be put on hold for the duration of this 10 minute period. Its not hard to do.

2) Critical issues section - no time limit. During this phase of the meeting, we look at :Coding errors, buffer overflows, and other WTFs in general. Requires serious, devious rat-like cunning on the part of the reviewers to think up diabolical scenarios in which the reviewed software might fail. Again, egos are put to one side, since everone eventually ends up on the receiving end of this treatment. OK, its hard to watch 'your baby' being ripped apart like this, but its all for a good cause. A lot of really good things come out of this exersize.

Strangely, when your peers rip the shit out of your code, yes it hurts, but it gives you more respect for those peers. There is nothing better than feeling that you are surrounded by really top-notch people that you CAN bounce ideas off, and get better insights into what you are working on. That is one side-benefit of tight code reviews.

Good, disciplined peers will stick to the agenda, and not wander off into talking about design options or formatting issues during this period.

Document everything - even if the conversation winds around and ends up coming to the conclusion that the code is not at fault after all - document it, so you dont end up going back down the same path later on down the track.

3) Requirements review : quick session to discuss whether the code matches the requirements. Dont bother turning up to a meeting without a thorough understanding of those requirements - you will look like a dick, and nobody will invite you to a pissup on friday arvo. Document any areas where the code falls short of requirements AND ALSO document any areas where the developer has gone over the top and added a pile of 'features' that are outside of the requirements. Keep the code lean and mean.

4) 5 minute wrap up : write up a TODO list of things that need to be done, estimate a timeframe, make sure it gets plugged into the gannt chart, and set a fixed time for the next review.

Time and schedule should be on everyone's mind, but thats a management and sales problem - not a technical one. Good developers will ignore schedule constraints and stick to doing things right. Having this attitude MAY turn some people's hair grey, but thats not our problem ... fact is, its the quickest way to finish off a good product.

And we do build good shit.

Most importantly, everyone should come out of a code review with that smug feeling that they really are part of an elite team of coders, who look at things from every possible angle. It builds morale and raises standards all round.

easy enough

Assuming your manager is competent, what's wrong with 1 developer and 1 senior/manager for a code review?

The key here is that the developer improves his code, or learns more about how the other developers work. That's all.

Lintian checking != peer reviews

So it seems that PMD is something like lint, splint or QAC, but then for Java.

Where I work we use QAC on C code (embedded development), to obtain information and statistics on such things. However, there is also an extensive code review practice set up.

Presubmission reviews are the strongest ways

The classic - sit in a room and review - doesn't work, the engagement factor drops considerably.

The process that I have implemented within my team is quite simple.

    1) Before review, the developers prepare the submission comments as well (low quality checkin comments are as bad as low quality code)
    2) The developers then run a script to prepare a diff for review
    3) The review is set to *all* developers
    4) Any developer can hold the review based on logical, syntactical or stylistic criteria.
    5) When a developer okays the change, they are attached to the checkin comment as a reviewer.

Some retorts that I have received from people outside the group (and my response)

    1) Diffs don't provide enough context ; If you can't interpret a context diff, you don't know your code

Some observations on what happens when the developers get used to it

    1) Developers can do this as part of their day-to-day life
    2) Developers will eventually learn to rely on the code review as a way of understanding
    3) Developers begin to get paranoid about code that has not been reviewed :)
    4) Reviewer developers begin to focus on particular areas of their concern (magic numbers, style, correctness, code familiarity)
    5) It doesn't hold back development,since the half-backed checkin doesn't occur nearly as much, and hence no rework.
    6) The developers begin to strive for a clean review - hence increasing quality
    7) The developers begin to make the process more stringent *on their own*

Surprisingly, the psychological aspect of the review process plays to increase it's strength.

Now the next area for me to get the developers fully engaged on is formal SDLC artefacts :).

Review Tooling

PMD is decent but really not all that useful in the grand scheme of things. Two tools are absolutely critical at my workspace:

checkstyle - enforce a coding style. We integrate this into our SCM system so people can't check in unless their code meets the official style. It's draconian but our codebase would have 12 different styles and be close to unreadable without it.

SmartBear's CodeCollaborator - a tool which makes code reviews easy. This is a necessity when working with junior developers on another continent. I can't speakly highly enough about this program - it's commercial but worth every penny.