Beefy Boxes and Bandwidth Generously Provided by pair Networks
laziness, impatience, and hubris
 
PerlMonks  

Howto best review code that has not been reviewed before?

by gumpu (Friar)
on Aug 18, 2004 at 09:32 UTC ( #383903=perlmeditation: print w/replies, xml ) Need Help??

Greetings fellow Monks

This is a dillema about codereview.

I work a at company that creates applications for transport. Basically we make the software that controls the flow of the traffic. It is not one application but a whole bunch of applications that all communicate with each other.

These applications grew and evolved over many years and are written in many languages (C, C++, Java, Pascal, Perl). So we have lots of source code. A few milion lines of code. Some parts are of good quality and comply to our coding standard, other parts suffer from software rot or from too tight deadlines.

A lot of the code is constantly modified as the requirements keep on changing. In an effort to improve the quality of the development process we are thinking of having code reviews. The idea is to review every change to code. It would have been better if all code was reviewed and then fixed, but there is no money for this (nor are there any unittest to make sure nothing breaks when things get fixed).

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.

I proposed to have the reviewer add all remarks in header of a sourcefile so the next time it is reviewed these points can be skipped. Also with a little perl program all the points can be extracted from the code and one can create a nice summary of what needs to be done. But that was rejected since the client then sees how good or bad our code really is.

Does anyone has any experience with this? How can one best review changes to heaps of old code?

Have Fun

  • Comment on Howto best review code that has not been reviewed before?

Replies are listed 'Best First'.
Re: Howto best review code that has not been reviewed before?
by exussum0 (Vicar) on Aug 18, 2004 at 11:03 UTC
    It sounds like the process of code reviewing has sorta begun. It hasn't as an official thing, but a rumbling under the feet of your development team. There are two sides of the issue, those who care, and those who don't. Obviously you are one of those who do care.

    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)

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

Re: Howto best review code that has not been reviewed before?
by davis (Vicar) on Aug 18, 2004 at 10:11 UTC
    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.
    If that's really their argument, that's reason enough to have code reviews.

    davis
    It wasn't easy to juggle a pregnant wife and a troubled child, but somehow I managed to fit in eight hours of TV a day.
Re: Howto best review code that has not been reviewed before?
by dragonchild (Archbishop) on Aug 18, 2004 at 14:26 UTC
    This is one version of the "What do I do with this old code?" question. Basically, you're looking at maintaining cruft and how to do that safely. Personally, I would go through the following steps:
    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.

    ------
    We are the carpenters and bricklayers of the Information Age.

    Then there are Damian modules.... *sigh* ... that's not about being less-lazy -- that's about being on some really good drugs -- you know, there is no spoon. - flyingmoose

    I shouldn't have to say this, but any code, unless otherwise stated, is untested

Re: Howto best review code that has not been reviewed before?
by McMahon (Chaplain) on Aug 18, 2004 at 14:14 UTC
    There are any number of studies that have shown pretty clearly that, of all the software development process steps one might choose to implement, code review provides the greatest return on investment.

    I'd recommend surfing this site (probably the foremost source for software QA information not directly related to writing code) for "code review" to get some solid defense of the practice.
Re: Howto best review code that has not been reviewed before?
by etcshadow (Priest) on Aug 18, 2004 at 15:48 UTC
    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
      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.
Re: Howto best review code that has not been reviewed before?
by chromatic (Archbishop) on Aug 18, 2004 at 16:22 UTC
    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"?

Re: Howto best review code that has not been reviewed before?
by gumpu (Friar) on Aug 18, 2004 at 22:16 UTC

    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.

    Have Fun

      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.

Re: Howto best review code that has not been reviewed before?
by Aristotle (Chancellor) on Aug 18, 2004 at 17:08 UTC

    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.

    Makeshifts last the longest.

      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"

Re: Howto best review code that has not been reviewed before?
by johnnywang (Priest) on Aug 18, 2004 at 20:09 UTC
    This really got me worried. please keep us posted, I won't board a train until you announce here the review is all done.

    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.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://383903]
Approved by Arunbear
Front-paged by Old_Gray_Bear
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others romping around the Monastery: (9)
As of 2019-10-21 20:16 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    Notices?