Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

So I'm sitting down to a nice bowl of c# spaghetti, and need to add something or remove something... but I have challenges everywhere from functions passing arguments that doesn't make sense, someone who doesn't understand data structures abusing strings, redundant variables, some comments are red-hearings, internationalization is on a per-every-output-level, SQL doesn't use any kind of DBAL, database connections are left open everywhere...

Are there any tools or techniques I can use to at least keep track of the "functional integrity" of the code (meaning my "improvements" don't break it), or a resource online with common "bad patterns" that explains a good way to transition code? I'm basically looking for a guidebook on how to spin straw into gold.

Here's some samples from the same 500 line function:

protected void DoSave(bool cIsPostBack) {
  //ALWAYS  a cPostBack
  cIsPostBack = true;
  SetPostBack("1");
  string inCreate ="~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~";
  parseValues = new string []{"","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","",""};


  if (!cIsPostBack) { //.......
  //....
  //....
  if (!cIsPostBack) {

  } else {

  }
  //....  
  //....
  strHPhone = StringFormat(s1.Trim());
  s1 = parseValues[18].Replace(encStr," ");
  strWPhone = StringFormat(s1.Trim());
  s1 = parseValues[11].Replace(encStr," ");
  strWExt = StringFormat(s1.Trim());
  s1 = parseValues[21].Replace(encStr," ");
  strMPhone = StringFormat(s1.Trim());
  s1 = parseValues[19].Replace(encStr," ");
  //(hundreds of lines of this)
  //....
  //....
  SQL = "...... lots of SQL .... ";

  SqlCommand curCommand;

  curCommand = new SqlCommand();
  curCommand.Connection = conn1;

  curCommand.CommandText = SQL;
  try {
    curCommand.ExecuteNonQuery(); 
  } catch {}
  //....
}

I've never had to refactor something like this before, and I want to know if there's something like a guidebook or knowledgebase on how to do this sort of thing, finding common bad patterns and offering the best solutions to repair them. I don't want to just nuke it from orbit,

share|improve this question

marked as duplicate by gnat, MichaelT, Kilian Foth, BЈовић, Jalayn Jun 5 '13 at 12:10

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

21  
I weep for you my friend. Godspeed to you, I shall see you in Valhalla. –  Daenyth Apr 8 '11 at 13:56
13  
Have you submitted any samples to the DailyWTF? –  FrustratedWithFormsDesigner Apr 8 '11 at 13:59
3  
One file/feature at a time. I'd start, by adding a Data Access Layer. It will be fairly simple, and provide the most bang for the buck. Create some data access classes, then go through your code moving the queries into the appropriate class. Don't yet worry about combining similar queries, do that second. First is to get all the data access into ONE location. Then look for commonalities, and similar queries and refactor them. –  CaffGeek Dec 15 '11 at 16:47
8  
This question has been out for 29 minutes and nobody has mentioned Michael Feathers' Working Effectively with Legacy Code? Consider that rectified. amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/… –  Carl Manaster Dec 15 '11 at 17:01
2  
If this is a hobby project without any paying users then this is an excellent opportunity to feel on your own body what a REAL rewrite actually is about. Just do it. Write it again. You will learn a LOT. –  user1249 Dec 15 '11 at 17:26
show 3 more comments

11 Answers

up vote 23 down vote accepted

There are three books I would recommend :

All of this books emphase on placing a "security nest" before engaging in refactoring when possible. This means having tests that ensure that you don't break the original intent of the code you are refactoring.

And always do one refactoring at a time.

share|improve this answer
3  
+1 for Working effectively with legacy code. Unit tests are great - but 500 lines isn't in any useful way of thinking a "unit" - so it's not really unit-testable. Too damn much goes on inside such a function to be able to write a unit test. So write characterization tests that exercise some paths through your code, and then whittle away at it. –  Carl Manaster Apr 8 '11 at 14:21
1  
Recommend the professional refactoring book. –  JeffO Apr 8 '11 at 15:32
    
Think of a "Unit" as one logical function of the code. That can be error checking, "happy path" validation, etc. Even if there is only one path through the 500 lines (no loops, conditionals, etc.) there are logical groups of functionality. You should have multiple tests per method. –  Berin Loritsch Apr 8 '11 at 16:58
add comment

This is exactly what Martin Fowler talks about in his book Refactoring.

Basic summary:

  • Unit tests! First you want to establish tests for the existing code. Figure out what the inputs and expected outputs are and test them. Don't test to the code. Test to the spec — you don't want to reproduce bugs.
  • Refactor bits at a time, keeping tests passing as you go.
  • Keep in mind that if you have no tests at all or for large portions of code, a rewrite might actually be the most efficient choice.

This interview with Martin Fowler about the topics in the book sheds a lot of light on the subject (on unit tests, payoffs, finding bugs through refactoring, refactoring vs. rewriting).

Keep in mind that bad patterns are bad for a reason — it is a poor solution to the problem. That means that even if you've seen the same kind of bad code, it doesn't mean the problem was the same and the best solution is likely different for each case.

share|improve this answer
    
The odds this code example doesn't have a ton of bugs are slim. Testing to the spec makes sense. It would be interesting to the quality of the specs. –  JeffO Apr 8 '11 at 15:31
7  
@Jeff O: And I'd bet you a nickel that there are no current specs. There may be some original specs, which are probably incomplete and badly written, but code like this doesn't usually come with updated specs. –  David Thornley Apr 8 '11 at 16:10
    
David couldn't be more right: somewhere in the world there may exist a project with code this bad, yet with a solid set of specs, but I'd have to see it before I'd be willing to believe in its existence. –  Carson63000 Apr 8 '11 at 22:18
    
You'd have to create the spec, which is one reason why a rewrite is probably faster anyway. –  NickC Apr 8 '11 at 22:25
add comment

In my past experience, iteratively defining areas of functionality and extracting the relevant code into methods/functions will be the fastest way for you to get started. You can then extract code, put that code into a function, and then test to confirm that you haven't broken anything. Rinse and repeat until you're done or until you have to add new functionality. Refactoring iteratively will provide more flexibility by allowing you to take occasional breaks from refactoring, especially if you still need/want to add new code. Psychologically, an iterative approach will give you the satisfaction of seeing your codebase become more ordered, which will make it easier for you to complete this refactoring task.

share|improve this answer
2  
I would also like to add that you might want to look and see if any of the functionality that was implemented is available in third-party libraries. When looking at my old code I facepalm over the large amount of boilerplate code I would write that was readily available in libraries or frameworks that I was unaware of at the time. –  maple_shaft Dec 15 '11 at 16:48
1  
"We demand rigidly defined areas of doubt and uncertainty!" –  user1249 Dec 15 '11 at 17:28
add comment

It sounds like what you want is to have unit tests. Add test cases that cover the existing behavior, then when you refactor you can rely on your tests to alert you to any failures. Do this on a new branch in your source control as well, so that any accidents don't become catastrophic.

share|improve this answer
    
That's a good start, but are there any resources that talk about dealing with common bad patterns? I know I've seen code like this in other places before... –  Incognito Apr 8 '11 at 14:00
1  
You are looking for something to convert anti-patterns into good patterns, yes? Does this help? antipatterns.com/orig/dev_cat.htm –  FrustratedWithFormsDesigner Apr 8 '11 at 14:08
add comment

Test-Driven Reverse Engineering.

Write unit tests for the "before".

Refactor.

Be sure the tests run after.

Pick small pieces that can be clearly identified as belonging to a single conceptual purpose or component.

share|improve this answer
add comment

I would start by covering as much of your code with automated tests as you can. It might be boring and tedious, but it lets you capture the current behavior of your system. Then, as you iteratively clean things up and rewrite/refactor parts of your codebase, you can feel more confident about not introducing a ton of regressions. You will also hopefully be able to introduce finer grained tests then you could before. This has always helped me in the past.

share|improve this answer
    
Do you know of any testing frameworks that support PHP? –  TheEnigmaMachine Dec 15 '11 at 17:06
    
There are several useful answers here on SO: stackoverflow.com/questions/3908433/automated-testing-with-php, stackoverflow.com/questions/5021254/…, stackoverflow.com/questions/6511337/…. In addition, seleniumhq.org automates testing web pages through browsers. –  Matt H Dec 15 '11 at 17:48
add comment

I would do something like the following:

  • Wrap as much of your code into classes as you can, grouping it by functionality, but leaving it otherwise untouched for now. This will allow you to triage your code.
  • Create a database layer using PDO.
  • Convert your code to the MVC pattern, taking special care to weed out any PHP code that is needlessly generating HTML and transferring that to the view. Your controllers should determine which model and which view to load, and your models should contain everything else.
  • Refactor your remaining classes, one by one. Tackle the most pervasive classes first. Look for classes that do multiple things and split them up. Get rid of repeated code. Make single responsibility methods.
  • Add unit tests as you go along. This will make your code more stable over the long-haul. If you need to refactor it again in the future, you will then have a contract that specifies how the code is supposed to perform. PHPUnit is one test framework that you can use, but there are others as well.
share|improve this answer
add comment

Yous should definitely start with writing unit tests, as others have mentioned. Unfortunately though, this code clearly needs to be nuked from existence. Now if it is working fine the way it is, maybe you do not need to, but any maintinece will quickly become very costly. This code appears to pretty much be the defintion of unmaintanable. But hey, with better design you can probably blow through a rewrite much quicker than the person who stumbled through it (I know cowboys can be quick coders, but this programmer was not cowboy... I think he might have literally been a cow).

share|improve this answer
add comment

If it is a web app, you can test the "output" with a little creativity -- use something like selenium on the browser side to make sure the UI bits still work and then check the database on the back-side to confirm that your update went through. Your black box is big, but you can at least sanity check your changes automatically and repeatably.

share|improve this answer
add comment

As others have said your main weapon here is unit tests. Regardless of what ever else you do you will need these.

As for tools to help refactoring, they exist, but they won't do large scale fixing for you. Things like Resharper will help move methods around, rename variables, extract interfaces etc, but I suspect you are going to have to do a lot of manual refactoring too.

share|improve this answer
add comment

That's not what refactoring is. Refactoring is breaking a piece of code down into its component parts (factors) in a way that improves code reuse and readability.

To improve terrible code is rewriting. So the question is how do you go about rewriting a whole application of bad code. The answer is that you start from scratch. Rewriting piecemeal is far more difficult and in the long run it will actually take longer to do properly.

share|improve this answer
    
If I am to rewrite it, am I better off to just jump in and rewrite it or sit down and plan everything out on paper first? –  TheEnigmaMachine Dec 15 '11 at 16:43
    
Paper. All but the most trivial development tasks should begin with design (planning). And in this case you have the advantage of learning from the mistakes you made the first time around. –  Igby Largeman Dec 15 '11 at 16:47
    
While you are right in the original meaning of those words, code refactoring nowadays is "altering of the code without changing its "external" behavior" and "rewriting" means typically rewriting from scratch (parts or whole code). –  MaR Dec 15 '11 at 16:53
    
@MaR: Yes, I'm perhaps being overly literal. I often see people referring to rewriting as refactoring, and that bugs me. In this case, judging by AgentKC's own account of the state of the code, it sounds like a scratch-rewrite would be advisable. –  Igby Largeman Dec 15 '11 at 16:59
1  
-1 for several reasons. Refactoring may or may not include "breaking a piece of code down into its component parts". Improving terrible code is refactoring (if done properly, one small step at a time, backed by unit tests). Rewriting from scratch is a whole different story, and Joel explains better than me why it is usually a bad idea. –  Péter Török Dec 15 '11 at 17:04
show 2 more comments

Not the answer you're looking for? Browse other questions tagged or ask your own question.