What is the worst code you've had to refactor, and how did you deal with it?

Answer 1

The most helpful thing I ever did was buy Michael Feathers' "Working Effectively with Legacy Code".

Answer 2

To get things started, I had code from an overseas consulting code that looked like this:

for(int i=0;i<15;i++) {
    switch(i) {
    case 0:
        do stuff that operates on port 0
        there were like 4 lines of code here
        identical in each case statement except
        for the port number
        break;
    case 1:
        do stuff that operates on port 1
        there were like 4 lines of code here
        identical in each case statement except
        for the port number       
        break;

repeat for cases 2-15.

Answer 3

This is actual code from some in-house Report Tracker software I got stuck maintaining. It is copied and pasted inline in ~40-50 pages (the whole app probably had <20 methods total when I found it, almost everything was inline logic). The code scared the shit out of me, so my solution was just to stick it in a function somewhere and call that. And this is actually how it is formatted.

if(trim($SearchOrder) != "")
    {
    $SearchArray = explode(",",$SearchOrder);
    if($ProductID != "" && !in_array("ProductID",$SearchArray))
     $SearchOrder .= ",ProductID";
    if($TypeID != "" && !in_array("TypeID",$SearchArray))
     $SearchOrder .= ",TypeID";
    if($CustomerID != "" && !in_array("CustomerID",$SearchArray))
     $SearchOrder .= ",CustomerID";
    if($LocationID != "" && !in_array("LocationID",$SearchArray))
     $SearchOrder .= ",LocationID";
    $SearchArray = explode(",",$SearchOrder);
    }
else
    {
    if($ProductID != "")
     $SearchOrder = "ProductID";
    if($TypeID != "")
     $SearchOrder = "TypeID";
    if($CustomerID != "")
     $SearchOrder = "CustomerID";
    if($LocationID != "")
     $SearchOrder = "LocationID";
    $SearchArray = array("$SearchOrder");
    }

    if($SearchOrder != "")
     {
      //Build Customer Products
      if(in_array("CustomerID",$SearchArray))
      {
      $CSArray = array();
      $res = mysql_query("select * from Locations where CustomerID='$CustomerID'");
      while($LRow = mysql_fetch_object($res))
       {
       $LocSQL .= "LocationID = '$LRow->LocationID' || ";
       }
      if($LocSQL != "")
       {
       $LocSQL = "where " . $LocSQL . "1=0";
       }
      $CustomerIDList = BuildArray("select * from Products $LocSQL","ProductID","Serial");
      }
      //Build Location Products
      if(in_array("LocationID",$SearchArray))
      {
      $LocationIDList = BuildArray("select * from Products where LocationID = '$LocationID'","ProductID","Serial");
      }
      //Build ProductType Products
      if(in_array("TypeID",$SearchArray))
      {
      $TypeIDList = BuildArray("select * from Products where TypeID = '$TypeID'","ProductID","Serial");
      }
      //Build Serial Products
      if(in_array("ProductID",$SearchArray))
      {
      $ProductIDList = BuildArray("select * from Products where ProductID = '$ProductID'","ProductID","Serial");
      }

     if(count($SearchArray) > 1)
      {
      $ExecStr = "";
      foreach($SearchArray as $SearchItem)
       {
       $ExecStr .= "array_keys(\$$SearchItem" . "List),";
       }
      $ExecStr = "\$IntList = array_intersect(".(substr($ExecStr,0,strlen($ExecStr) -1)).");";
      }
     else
      {
      $ExecStr = "\$IntList = array_keys(\$".$SearchArray[0]."List);";
      }
     eval($ExecStr);
     }
     else
      $IntList = array_keys(BuildArray("select * from Products","ProductID","Serial"));

//Build Product DropDowns
$ProdSQL = "";
foreach($IntList as $aProduct)
    {
    $ProdSQL .= "ProductID = '".substr($aProduct,2,strlen($aProduct))."' or ";
    }
$ProdSQL = "where $ProdSQL 1=0";
$ProductBox = array();
$TypeBox = array();
$LocationBox = array();
$CustomerBox = array();
$res = mysql_query("select * from Products $ProdSQL");

while($nProductRow = mysql_fetch_object($res))
    {
    ArrayPush($ProductBox,"ID".$nProductRow->ProductID,$nProductRow->Serial);
    if(!in_array("ID".$nProductRow->TypeID,$TypeBox))
     {
     $nTypeRow = DBGetRow("ProductTypes","TypeID",$nProductRow->TypeID);
     ArrayPush($TypeBox,"ID".$nProductRow->TypeID,$nTypeRow->Name);
     }
    if(!in_array("ID".$nProductRow->LocationID,$LocationBox))
     {
     $nLocationRow = DBGetRow("Locations","LocationID",$nProductRow->LocationID);
     ArrayPush($LocationBox,"ID".$nProductRow->LocationID,$nLocationRow->Name);
     if(!in_array("ID".$nLocationRow->LocationID,$CustomerBox))
      {
      $nCustomerRow =  DBGetRow("Customers","CustomerID",$nLocationRow->CustomerID);
      ArrayPush($CustomerBox,"ID".$nLocationRow->CustomerID,$nCustomerRow->Name);
      }
     }
    }

It was after this application that I truly understood why some people despise PHP.

Answer 4

The worst code was a 3-inch stack high printout (yes, we used to print out code then) of a Prolog interpreter written in C.

I wanted something more efficient than the usual AI Lisp query interpreters.

I refactored it into 3 pages of Lisp :) And it run much faster than the code I had been using previously, because it had some very sophisticated backtracking mechanisms.

Answer 5

I started a job some years back and my first task was to refactor a switch statement.

No, I'm not kidding.

The switch statement was 25,000 lines long. Each of the about 40 cases duplicated, with slight variations, a data export process from the master database into a custom B2B transaction format.

Some cases output XML, some Excel, some plain text. Each had its own SELECT statement, each contained similar data-massaging functionality--and there was not a single comment anywhere.

The best part? The switch statement was invoked in a postback from a web page. No, there was no AJAX progress bar.

I still have nightmares. Hold me.

EDITED I forgot to add how I dealt with this: Several base classes and 40 derivations. Not in the same file. DataSets. And some comments. The result was an order of magnitude faster and required half as much code.

Answer 6

I try not to refactor old code just for the sake of it. My general rule is; only change things when there is a bug or feature request (that has been approved). Refactoring old code that you don't fully understand is more likely to break something. If I do decide to refactor something old, it must have a unit test to validate the change.

Defining the difference between old and new code is a bit harder though. We are constantly writing code, and of course I do want to refactor rubbish code that is still current. I try to keep a clear line between projects and refactoring for code readability should only occur in the current project.

Answer 7

It was C++ code that had a number of methods - each method had a very similar switch statement. The code was copied and pasted from each method and slightly modified by the previous developers.

The prior developer in the code fixed a defect in the code and had to touch every function and every case in the switches. Rather than refactor he just made all the edits.

I made just one method with some parameters that distinguished the differences. I think overall I was able to remove about 2000 lines of code. It was amazing.