Code reviews establish one and one thing: Is what you are looking at 'good'? New code, old code, code from last month, is any of it worth anything. The biggest gain you earn is akin to the opensource idea of "many eyes". You can witness, if you haven't, many solutions being posted on pm that have small mistakes, but have been caught by someone else.

You have a few argumenets usable for management if you need to convince them that should you make the slightest change for the better, it is for the better. Better code means easier maintenance, simpler design, faster updates after refactoring etc ec.. there is a return on acting upon fixing bad code. I suggest the refactoring text by fowler on this. He has a good 10 pages on interacting with manglement on this issue.

As for the best way to review? I'm guessing there is no design document. Start from the biz straight down to the code. A task needs to get done.. (use case) This task involves doing these N steps with certain conditionals in a certain direction. (activity diagram for the use case). The data is stored as such (class diagrams and entity relationship digrams) using these bits of logic. (in code documentation, class diagrams, sequence diagrams etc..). As you cover case by case, you'll slowly envelop everything. It's a bit of systems analysis and design with an existing project/product.

Being a tech person, your biz cases need not be the best MBA (Master of Business Administration degree) speak in the world. If your biz team helps you, you'll have the definitive manual on your system. And when you make changes, anyone can browse the manual and figure these things out. More importantly, you'll know the relationships and dependencies of how everything works.

Update: MBA cleared up (tnx davis)

1. Document the assumptions you find. At least review the documentation. We're going through this right now with some old VB code. We have no idea what language the app will end up being in, but at least we have some idea of what's going on, in case the primary maintainer gets hit by a bus. Twiki is good for this, and easy to set up. Plus, management can read it, if they care.
2. Start adding unit tests. Add tests for everything you touch, plus one additional test. The tests may start out being stupid, but at least you'll have something. Plus, you're only testing those items you change. Stuff that isn't broken and isn't being touched, theoretically, doesn't need to be looked at ... at first.
3. Eventually, you'll end up with a critical mass of documentation and unit tests. At that point (3-9 months down the road), you'll be able to make better decisions about what to do with the code. You might have accidentally rewritten some of it by that time. At the very least, you'll be able to make better cases to management for whatever decision you choose to make.

Basically, you need to triage based on need for new functionality. Most applications tend to have between 50% and 90% of the code touched every 18 months, or so. This is a rule of thumb, based on my experience.

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]

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

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.

So management is not entirely happy with code review idea. They worry about what the returns will be for the time invested. They also claim that the reviewers will be overwhelmed by all the faults in the old code each time they have to review changed code.

Would they be willing to ride in a train that had foregone preventative maintenance because "it's too hard to fix things up when nothing has gone wrong before"?

Greeting again Monks!

Thanks for all the very useful responses and links! Have a lot of ammunition for the next meeting.

Let me try to answer some of the questions that were asked in the responses. First of all it really still is safe to travel! The software to control traffic consists of several layers. The bottom layer designed under very strict procedures and safety standards. It is designed by one team, reviewed by another, and tested by yet another. It contains the logic, some hardwired, to make sure no two trains can ever make use of the same track and collide.

The software we make sits on top of this layer. Since the bottom layer is secure the guidelines for our software are less strict. If our software fails things just stop and are delayed.

We do use revision control, a 20 year old one, but we do have a nice GUI in Perl/Tk on top of it. We do do a lot of testing, but only after all code is written.

To give an idea of the problems we have in our code:

• We have a coding standard but not everybody followed the rules.
• We have functions that are over 800 lines long. They did not started out that way of course. A few years ago a function would be 60 lines long. Then the requirements changed, and again, and again; each developer added a few lines, and it turned into a 200 line function. The next developer did not have time to split it into several functions because only 8 hours were available to implement the seamingly simple change in the requirements. So the function grew and grew. Now we have 800 line monster functions that are hard to maintain and understand.
• We have software architectures that worked well a few years ago, but since they were not adapted to the evolving requirements, they do not work that well today. In response to that all kind of hacks are implemented to get around this quikcly, worsening the situation.
• We have the same functionallity implemented in different ways in different applications. (Since there was no time or budget to turn it into a library.)

A lot of this stuff, I hope, can be prevented if we get some budget for code review. Going to try to get them to adapt the approach suggested by dragonchild and etcshadow; only fix and refactor the stuff in functions that were changed,and add unittests in the process.

Management is now atleast entertaining the idea of code reviews and with this method and all the arguments supplied by the others that replied there is some light at the horizon.

Have the feeling that we are not the only shop with this kind of problem though :).

Many thanks again.

You might enjoy reading about the Big Ball of Mud, and how to attack it.

Makeshifts last the longest.

To give an idea of the problems we have in our code...

I'd recommend you take a look at Peter Scott's excellent Perl Medic, which has lots of very good advice on maintaining legacy Perl.

I am dismayed that this is an issue at all.

Are they liable if trains crash due to a software bug? Would they rather invest in insurance over code reviews? Are they willing to risk injuries, even casualties?

If I was in your position I'd try to sow some paranoia along these lines.

Perhaps it's like that scene in fightclub where it was more expensive to recall cars than deal with the isnurance claims. :\

----
Then B.I. said, "Hov' remind yourself nobody built like you, you designed yourself"

Actually I'll propose that we lobby the congress to pass a law that software such as this one and those for airline traffic controls, cruize ships, cars must be open source. Each passener can review the code, bring his/her own test suite in a PDA, and only board it when the test passes. I predict that's going to save the software industry.