Re: Howto best review code that has not been reviewed before?

Well, one way to get past the problem of "the old code is so bad it will clutter the code review" is to only review the changes (except to consider the rest of the code for its contextual value). That is: don't just open up the file and read through it, picking on everything that looks nasty. Rather, open up the diff of the file against the previous revision, and look at the changes. If there's cruft in the change, then bounce it back to be rewritten. If there's cruft outside the change, then live and let live (until you can get a budget for writing unit tests and rewriting it).

Oh... you are using revision control software of some kind, right? If not, I'm never riding a train again.

------------
:Wq
Not an editor command: Wq
[download]

Comment on Re: Howto best review code that has not been reviewed before? Download Code

Replies are listed 'Best First'.
Re^2: Howto best review code that has not been reviewed before? by jbware (Chaplain) on Aug 18, 2004 at 16:08 UTC
This is actually how we're handling the new code review process at my current client, and its worked pretty well. They didn't have any processes before and now are trying to get things in line. We also expand on the "only the stuff we need to touch" philosophy and developers take the initiative to fix up a function/page and get it up to snuff if they're already reworking a majority (like 70%) of the function/page. This ends up being a judgement call depending on the impact of changing the other 30%, but works to expand our "good code" into regions that might not otherwise get it. Its not a perfect solution, but it seems like a reasonable balance that helps to ease everyone, including management, into the process. The nice thing is we're moving forward w/o turning a 15min change into a 3hr burden; which tends to lessen the $$$ impact that management sees. -jbWare	[reply]
Re^3: Howto best review code that has not been reviewed before? by podian (Scribe) on Aug 20, 2004 at 21:38 UTC
This is how we did it at a telecom company as well. We just review the delta. If the delta is messy, then we look at the whole function or file. Now I know about agile methods, I would recommend that you add a test case for each change. Add a test case and: a) run the test case before making the change b) change the code c) run the test case again I will also recommend re-factoring . another approach is to do a code walkthrough. It is different from code review in that you take a use case (a scenario) and go through the code and see if the code will work. This is also faster than code-review.	[reply]


Think about Loose Coupling
	PerlMonks