Slashdot Mirror
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.
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.
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).
Sliced bread good.
Sliced bread good! FIRE BAD!
Wizard Needs Food, Badly
Look at the sourceforge definition of PMD .
ran many code reviews, most of them very succesfully. They just need someone to run them that isn't attached to the code.
The Kruger Dunning explains most post on
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.
Sewwiously. 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.
Take off every 'sig' !!
Think of it as a code review in a box.
Hey, if you want to call it a "code review", that's your prerogative.
Wizard Needs Food, Badly
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.
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?
Our diabolical prof fed this analysis code itself to it as data. It spat out a D. He got a D.
sed -e 's/Chuck Norris/Rajnikant/g' joke > fact
I've used code collaborator 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.
"The crows seemed to be calling his name, thought Caw."
I believe just about every book would cover that.
But: does your foreskin...
run Linux?
Take off every 'sig' !!
> aren't code reviews supposed to focus on the business logic implementation
I think that's exactly right - use tools like PMD to find nickle and dime things like certain null pointer exceptions, unused code, empty try blocks, etc. Let the code reviews be focused on things like "hey, we don't need all these accessors", "we should be using the business rule package here", "this is really more of a Map than a List" and that sort of thing.
The tools do the gruntwork and the people do the thinking... good times.
The Army reading list
SYBASE ASE 15.0 doesnt work with glibc 2.4+ and the "workaround" doesn't help the install package run.
Ya get what ya pay for. Time to upgrade to Yukon! You had your shot, you clusterfuck of hippy bullshit.
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/)
It's called FxCop, and it's been freely available from Microsoft for a _long_ time, having been created internally for use on their own projects. I'm not going to cite, because even a simple Google search would have turned up dozens to hundreds of links about FxCop and teams using it. Since you didn't run said search before posting, I don't see why I should cite.
This has to do with the human element. If you get six people in a room and give them a pile of badly formatted code, even with specific instructions to the contrary they are likely going to at least mentally pick apart all the obvious problems before they even move on to subtle logic flaws and business logic. I think the savings come in because you can be a better code reviewer precisely because you are not being distracted by formatting or stupid little potential errors. I personally have a much easier time reading code if it's formatted in a consistent way, so I like having a corporate code formatting standard even when I disagree on the specific policies in some places.
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.
I really thought that IBM was headed somewhere when they cooked up Structural analysis for Java
http://www.alphaworks.ibm.com/tech/sa4j 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.
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"
"My country, right or wrong; if right, to be kept right; and if wrong, to be set right." --Senator Carl Schurz (1872)
Indeed. It can be quite helpful in reviews of you actually provide a somewhat detailed definition, at the beginning, for what you're talking about, rather than assuming everyone has heard of it. Calling it a "static analyzer" only makes me think of balloons with hair stuck to them. And, "The first chapter is the mandatory introduction... Nothing unusual there," is simply ironic, given its absence here.
Freshmeat says, "PMD is a Java source code analyzer. It finds unused variables, empty catch blocks, unnecessary object creation, and more. It includes CPD, a tool to detect chunks of identical code."
Although I'm not sure if any of that is comparable to what's obtained by peer code review.
Put your code in the box? :)
Where is all the whining about Slashdot linking off to some evil corporate bookseller, when the book can be acquired for cheaper at Amazon?
:q!
(What is "business logic", anyway? Can I assume it's a synonym for "logic", only made more palatable to PHBs?)
I guess that's way of putting it, yes. It's also a tool for people to learn each others parts of the code, and for understanding each others' styles and design choices.
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.
Used to do this all the time when writing software for defence / weapons systems. (in Oz at least)
.. 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.
: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.
... fact is, its the quickest way to finish off a good product.
Its easy - the source is printed out and circulated
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
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
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.
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.
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.
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
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.
http://www.fortifysoftware.com/products/sca/scaHow ItWorks.jsp
I think these guys compile an object that makes it easy to examine every possible logic path in the source.
It's more than just source code pattern matching.
Take off every 'sig' !!