in reply to Difficult code (Resolutions)

Also, it's only work, and it's not worth giving myself an ulcer at an early age.

There'll be plenty of time for that later.

Even if every important variable is named based on some variation of "temp", the important thing is to produce working code in the timeframe given by my employer.

There is a middle ground: 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.

As much as it pains us to pick up a really foul piece of code without stopping to rewrite it, there often isn't a business case for giving it a thorough scrub. But that doesn't mean we can't do any cleanup. Take it one room at a time.

Replies are listed 'Best First'.
Re^2: Difficult code (Resolutions)
by adrianh (Chancellor) on Aug 21, 2003 at 08:27 UTC
    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".

    • If we're adding new functionality write some acceptance tests for it.
    • Find the bit of code that needs updating.
    • Write some unit tests for the existing code if they don't already exist. If we're fixing a bug or a security issue make sure that we have a failing test that illustrates the problem.
    • Test. Code. Refactor. Repeat until done.

    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.

Re: Re: Difficult code (Resolutions)
by xChauncey (Scribe) on Aug 21, 2003 at 05:33 UTC
    Thank you dws.

    I've read many of your posts, and from what I have read, regard your advice highly (especially in regards to the nature my post). The one room at a time approach seems to be an excellent guide with which to approach the refactoring of the code.

    Thanks again,
    C