I am putting together some guidelines for code reviews. We do not have one formal process yet and are trying to formalize it. And our team is geographically distributed.
We are using TFS for source control (we used it for tasks/bug tracking/project management as well, but we migrated that to JIRA) with Visual Studio 2008 for development.
What are the things you look for when doing a code review?
- These are the things I came up with
- Enforce FxCop rules (we are a Microsoft shop)
- Check for performance (any tools ?) and security (thinking about using
OWASP – code crawler) and thread safety - Adhere to naming conventions
- The code should cover edge cases and boundaries conditions
- Should handle exceptions correctly (do not swallow exceptions)
- Check if the functionality is duplicated elsewhere
- A method body should be small (20-30 lines), and methods should do one
thing and one thing only (no side
effects and avoid temporal coupling -) - Do not pass/return nulls in methods
- Avoid dead code
- Document public and protected methods/properties/variables
What other things should we generally look out for?
I am trying to see if we can quantify the review process (it would produce identical output when reviewed by different persons) Example: Saying "the method body should be no longer than 20-30 lines of code" as opposed to saying "the method body should be small".
Or is code review very subjective (and would differ from one reviewer to another)?
The objective is to have a marking system (say -1 point for each FxCop rule violation, -2 points for not following naming conventions, 2 points for refactoring, etc.) so that developers would be more careful when they check in their code. This way, we can identify developers who are consistently writing good/bad code. The goal is to have the reviewer spend about 30 minutes max, to do a review (I know this is subjective, considering the fact that the changeset/revision might include multiple files/huge changes to the existing architecture, etc., but you get the general idea, the reviewer should not spend days reviewing someone's code).
What other objective/quantifiable system do you follow to identify good/bad code written by developers?
Book reference: Clean Code: A handbook of agile software craftmanship by Robert Martin.
Best Answer
Grading individuals in a review is counter to most successful systems I've worked with, maybe all. But the goal I've been trying to reach for more than 20 years is fewer bugs and increased productivity per-engineer-hour. If grading individuals is a goal, I suppose reviews could be used. I've never seen a situation where it was required, as a worker or as a leader.
Some objective study (Fagan, etc.) and a lot of popular wisdom suggests that peer relationships facilitate code reviews aimed at reducing bugs and increasing productivity. Working managers may participate as workers, but not as managers. Points of discussion are noted, changes to satisfy reviewers are generally a good thing but not required. Hence the peer relationship.
Any automated tools that can be accepted without further analysis or judgment are good - lint in C, C++, Java. Regular compilation. Compilers are REALLY good at findng compiler bugs. Documenting deviations in automated checks sounds like a subtle indictment of the automated checks. Code directives (like Java does) that allow deviations are pretty dangerous, IMHO. Great for debugging, to allow you to get the heart of the matter, quickly. Not so good to find in a poorly documented, 50,000 non-comment-line block of code you've become responsible for.
Some rules are stupid but easy to enforce; defaults for every switch statement even when they're unreachable, for example. Then it's just a check box, and you don't have to spend time and money testing with values which don't match anything. If you have rules, you'll have foolishness, they are inextricably linked. Any rule's benefit should be worth the foolishness it costs, and that relationship should be checked at regular intervals.
On the other hand, "It runs" is no virtue before review, or defense in review. If development followed the waterfall model, you'd like to do the review when coding is 85% complete, before complicated errors were found and worked out, because review is a cheaper way to find them. Since real life isn't the waterfall model, when to review is somewhat of an art and amounts to a social norm. People who will actually read your code and look for problems in it are solid gold. Management that supports this in an on-going way is a pearl above price. Reviews should be like checkins- early and often.
I've found these things beneficial:
1) No style wars. Where open curly braces go should only be subject to a consistency check in a given file. All the same. That's fine then. Ditto indentation depth**s and **tab widths. Most organizations discover they need a common standard for tab, which is used as a large space.
2) `Ragged
text that doesn't
for content.`
BTW, K&R indented five (FIVE) spaces, so appeals to authority are worthless. Just be consistent.
3) A line-numbered, unchanging, publicly available copy of the file to be reviewed should be pointed to for 72 hours or more before the review.
4) No design-on-the-fly. If there's a problem, or an issue, note its location, and keep moving.
5) Testing that goes through all paths in the development environment is a very, very, very, good idea. Testing that requires massive external data, hardware resources, use of the customer's site, etc, etc. is testing that costs a fortune and won't be thorough.
6) A non-ASCII file format is acceptable if creation, display, edit, etc., tools exist or are created early in development. This is a personal bias of mine, but in a world where the dominant OS can't get out of its own way with less than 1 gigabyte of RAM, I can't understand why files less than, say, 10 megabytes should be anything other than ASCII or some other commercially supported format. There are standards for graphics, sound, movies, executable, and tools that go with them. There is no excuse for a file containing a binary representation of some number of objects.
For maintenance, refactoring or development of released code, one group of co-workers I had used review by one other person, sitting at a display and looking at a diff of old and new, as a gateway to branch check-in. I liked it, it was cheap, fast, relatively easy to do. Walk-throughs for people who haven't read the code in advance can be educational for all but seldom improve the developer's code.
If you're geographically distributed, looking at diffs on a screen while talking with someone else looking at the same would be relatively easy. That covers two people looking at changes. For a larger group who have read the code in question, multiple sites isn't a lot harder than all in one room. Multiple rooms linked by shared computer screens and squak boxes work very well, IMHO. The more sites, the more meeting management is needed. A manager as facilitator can earn their keep here. Remember to keep polling the sites you're not at.
At one point, the same organization had automated unit testing which was used as regression testing. That was really nice. Of course we then changed platforms and automated test got left behind. Review is better, as the Agile Manifesto notes, relationships are more important than process or tools. But once you've got review, automated unit tests/regression tests are the next most important help in creating good software.
If you can base the tests on requirements, well, like the lady says in "When Harry Met Sally", I'll have what she's having!
All reviews need to have a parking lot to capture requirements and design issues at the level above coding. Once something is recognized as belonging in the parking lot, discussion should stop in the review.
Sometimes I think code review should be like schematic reviews in hardware design- completely public, thorough, tutorial, the end of a process, a gateway after which it gets built and tested. But schematic reviews are heavyweight because changing physical objects is expensive. Architecture, interface and documentation reviews for software should probably be heavyweight. Code is more fluid. Code review should be lighter weight.
In a lot of ways, I think technology is as much about culture and expectation as it is about a specific tool. Think of all the "Swiss Family Robinson"/Flintstones/McGyver improvisations that delight the heart and challenge the mind. We want our stuff to work. There isn't a single path to that, any more than there was "intelligence" which could somehow be abstracted and automated by 1960s AI programs.