Clean one room at a time. As you encounter a section of code that you need to extend or do maintenance on, take a few minutes to look it over first. If there are bogus variable names, change them before you change anything else. Then make your changes. By leading with a semantic-preserving refactoring, you eliminate (or greatly reduce) the risk that you'll break things accidentally.

Excellent advice. Also make sure that there are tests in place before you change the code so you can be sure that you're not breaking anything.

My approach when approaching a large nasty lump of Perl code is to do the following:

  1. Write acceptance tests if they don't already exists.
  2. Run it through perltidy so I at least can trust the indentation to not be lying.
  3. Make it compile strict, without warnings and (if appropriate) tainted if it doesn't already.
  4. Review the code making notes of potential bugs, areas of unclear functionality and possible security issues problems.

This, for me, is the bare minimum needed to get business code into a vaguely safe state. Without acceptance tests we don't know what the code should do, and whether it is actually doing it. Without a common code layout style nobody can read the code. Without strict and warnings we can miss many errors.

(Yes, I do know that there can be valid reasons for not using strict/warnings. However the people who are bright enough to do this are not the same group of people who write terrible unmaintainable code.)

In the code review I'm not looking for ugly code - but code that is potentially broken (either functionally or from a security point of view) or that I don't understand. There is an obvious business case for fixing broken code. It should also be obvious that there is a business case for fixing code that is too opaque to be easily understood - because there is a high risk in running things you don't understand.

Once this has been done I prioritise the requests for new functionality and the changes from the code review with the client and then, as dws says, "Clean one room at a time".

There is a huge temptation when presented with a big pile of... unsavory... code to either throw it all away and start from scratch (almost always a really bad idea) or spend months tidying it up (almost always a waste of time that could be better spent elsewhere). Much better to attack it piecemeal.

In reply to Re^2: Difficult code (Resolutions) by adrianh
in thread Difficult code (Resolutions) by xChauncey

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.