Suggestions for working with poor code

[Ovid]

I am currently working on adding a fair amount of functionality to a Web site whose programs have been designed very poorly. Amongst other things, taint checking and strict have not been used. Code has been thrown together without regard to side effects, massive Here docs are used to output HTML, etc. Since I am getting a fair amount of experience with these issues, I thought I would offer some of my observations for fellow monks. Some of these pertain to the existing code and concentrates on 'quick fixes'. Some pertains to new code that's added.

Quick (?) Fixes

• Security comes first.

  Personally, I believe we have an obligation to ensure that our client's code is as secure as possible. Check out Kevin Meltzer's untaint module for a quick and easy way of untainting data. Untainting is necessary, but does not have to be difficult. This fix isn't so quick, but it's mandatory.

• Security: use the multiple argument form of system.

  If the code uses system calls, using the multiple argument form of system reduces the chance that unsafe data will be passed to the shell and it's often a quick and easy change.

• Try putting 'use strict' at the top of the code.

  Usually, this will break it, but recently I did this to a program that was 2,000 lines long. I then ran it and the error log had an extra 130 lines in it. While many of the issues were not quick fixes, there were many that were simply a failure to declare variables.

• Make sure you check the return on all system calls.

  I've discovered that bad code often fails to check to see if open, read, flock, and other system calls were successful. At least add an or die "$!" after them. Nothing is worse than tracking down a bug caused by a silent failure on an open 50 lines earlier.

• If using DBI, try to convert all SQL statements to use placeholders, if possible.

  When using placeholders, the DBI module will automatically quote your data for you. Otherwise, putting the variables directly into the SQL statement could be dangerous. A user entering a single quote mark into a field can be sufficient to crash the program.

  # This is bad
  my $sql = qq{ INSERT INTO ECinterface..CustomContent (contentType, con
  +tentDate, question, answer)
                VALUES ('tileInfo', $date, $question, $answer)};
  my $sth = $dbh->prepare($sql);
  $sth->execute;
  
  
  # This is good
  my $sql = qq{ INSERT INTO ECinterface..CustomContent (contentType, con
  +tentDate, question, answer)
                VALUES ( ?,?,?,? )};
  my $sth = $dbh->prepare($sql);
  $sth->execute( 'tileInfo', $date, $question, $answer );
  

Adding new functionality

• Remember that 'use strict' is lexically scoped.

  If you can't get the code to run under strict, make sure that when you build new functionality, that you at least use strict on the code you have created.

• Don't reuse bad code.

  Code reuse is good, but not if the original code is junk. If you are writing a sub similar to one that already exists, consider not updating the existing code. Rewrite the function from scratch, allowing calls to the original subroutine, method, or whatever, to be routed to and handled by your code. Then, delete the original code, if possible.

• Track how long it takes you to fix bugs.

  I recently spent half an hour not seeing a misspelled variable because "use strict" was not in place. It would have taken me a couple of seconds to find the bug, otherwise. By pointing out how much extra time I spend maintaining code as a result of poor design, I find that my boss is much more willing to give me leeway on deadlines.

• Comment the new code liberally.

  Often, poor code is not commented well. Do not add to the problem. Further, if some programmer comes behind you and is trying to figure out why your code is structured so differently, they'll appreciate the heads up. Sometimes I need to do some strange tricks to add my sauce to the spaghetti. Further, add a "to do" list comment at the top of the code so that you and others won't forget.

Any and all tips that others wish to add are welcome!

Cheers,
Ovid

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

[dws]

Bad formatting can hide a number of sins.

If necessary, the first thing I do when taking on bad code is reformat it. It doesn't matter whether it's Perl, Java, C, or HTML. A surprising number of problems (like mangled boolean conditions in branches and loops) fall right out when the code is tidied up so that you can actually see what it's doing.

Then it's a lot easier to get on with the fixes Ovid suggests.

[tinman]

I've found that taking a deep breath and a step back from the turmoil of badly written code can help immensely.. quite a few instances where you can see places where code can be consolidated into a single reusable library..

With this in mind, trying to understand the basic intent of the code is really important to me.. I write down a small note describing what each section of code tries to do... this allows me to focus on reuse as well as consolidate several segments together..

Related to this: in addition to liberal comments, updating documentation or in some cases, writing some document that describes the structure and function of a code block is very helpful to any person maintaining the code.. you don't have to wonder "what was that guy thinking" or "why did he do *that* ?".. its all there in a document.. and also provides a cursory overview of what has been going on without jumping straight into the code (I'm a big fan of the saying that goes "the less time you spend planning, the more time you spend coding")...
Caveat: Docs that aren't updated are worse than useless, though...

[clemburg]

Track how long it takes you to fix bugs.

I agree enthusiastically. It will be your only argument when somebody comes and asks you where all the hours have gone. For this kind of job (take responsibility for badly written code, fixing bugs, etc.) this is an absolute must.

For these purposes, two little forms (or spreadsheets, or editor modes/templates, or whatever) will be very helpful (pedantically detailed discussion of these can be found in An Introduction to the Personal Software Process, electronic materials are available at The PSP Resource Page, including time tracking tools, emacs modes, forms, etc.):

• Time recording log
• Defect recording log

These are the essentials of both (header columns, add date, person, project, client, etc. as you need):

Time recording log:

• Start Time
• Stop Time
• Interruption Time
• Delta Time
• Activity Category (coding, testing, reading docs - make up your own)
• Comments (more detailed description of task)

Defect recording log:

• Defect ID (e.g., sequential number)
• Type (one of: documentation, syntax, build/package, assignment, interface, checking, data, function, system, environment - your own are welcome)
• Inject Phase (when was the defect put into the program - estimate - design, coding, testing, linking, etc.)
• Remove Phase (when was the defect found - compile time, testing, etc.)
• Fix Time (how long did it take to fix)
• Description (description of defect)

Contrary to what you may think, it does *not* take much time to use these forms (or similar means to record the information). But it will give you all the data you need to be sure you did the Right Thing, and the confidence and evidence to convince your boss or client that what you did was worth the time and the money.

Christian Lemburg
Brainbench MVP for Perl
http://www.brainbench.com

[coreolyn]

You mean these logs haven't been automated into CPAN module yet??

coreolyn Still looking for time to record time usage

[r.joseph]

Wonderful post Ovid - just added to my favs list. For someone who had the great misfortune a while back of inheriting a large, ill-maintained and astrociously coded website, I know what you mean and this post really highlights some of the main points that go into fixing it.

I also have to agree heartily with the replies, although I would like to add something. I find sometimes that it actually helps, with particularily insubordinate code, to take part of it out of the main file (say, a sub) and put it into another script that has major error-checking, lots of warnings and what not, and then test it from there. Sometimes this will yield a solution very quickly, and other times it has quickly allowed me to see what was wrong and what needed to be recoded.

Just thought I'd offer a quick idea...great job again!

r. j o s e p h
"Violence is a last resort of the incompetent" - Salvor Hardin, Foundation by Issac AsimovW

[knobunc]

Very cool node.

With regard to the To Do list, I scatter them throughout my code if there is a place I need to do further work. However, I have a make rule for todo that searches for all of the lines with TODO in them and prints them out. So a usage of a TODO:


if ($whatever) {
    # TODO - Finish code to take over the world
}

Becomes:


To Do List
Dir/file.pl 132: Finish code to take over the world

When run through the following (ugly, suboptimal, but working) code in Tools/todo.sh:


#/bin/sh

echo 'To Do List'
find . -type f | xargs grep -n TODO | perl -ne '($file, $line, $rest) 
+= split /:/, $_, 3; $file =~ s|^./||; $rest =~ s|.*?TODO.*?[-\s:]+||;
+ $rest =~ s|"[.;,]\s*$||; $rest =~ s|\\n||g; print "$file $line: \u$r
+est\n"' | sort | uniq | grep -v '.#' | grep -v Makefile | grep -v CVS
+/

Which I call from my Makefile:


todo:
        Tools/todo.sh

Kinda ugly, but it lets me put the TODO statements where I actually need to do the work. So I can proof out a block of code by writing narrative comments with TODO at the start of the line (behind comment characters of course). Then fill in the code later and not worry about missing a piece. Also since the TODOs are where the stuff needs to be filled in, I have lots of context around the issue and don't need to write as much as I would if they were at the top of the file. Plus anyone without something to do in the group can just type make todo and add some code. Finally, it is easier to add a TODO right where you need it, than bop up to the top of the file and then have to find where you were back in the code.

-ben