Techniques to re-factor garbage and maintain sanity?

Question

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,

score 32 · Accepted Answer · answered Apr 08 '11 at 14:09

There are three books I would recommend :

Refactoring: Improving the Design of Existing Code by Martin Fowler. That's the theory book. It`s worth reading as there are general recipes that can be applied to any code. But you may not find explicit examples of what you will encounter in c#.
Working effectively with legacy code by Michael Feathers. It can feel a bit dated sometime, but the general idea of how to deal with code that is hard to maintain is well explained. (Michael Feathers actually recognizes the need for an update to this book, called "Brutal Refactoring")
Professional Refactoring in C# & ASP.NET by Danijel Arsenovski. It is a really good specialized .net book, and has tons of pratical examples of refactoring. There is also a vb.net version that is as useful for anyone working with vb.net. It will guide you about tools that are available for automatic refactoring as well (Resharper and DevExpress CodeRush Express).

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.

Nicole · Answer 2 · 2011-04-08T14:41:43.823

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.

score 6 · Answer 3 · answered Dec 15 '11 at 16:40

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.

VirtuosiMedia · Answer 4 · 2011-12-15T17:14:42.950

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.

score 4 · Answer 5 · answered Apr 08 '11 at 13:57

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.

score 3 · Answer 6 · answered Dec 15 '11 at 17:05

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.

score 3 · Answer 7 · answered Apr 08 '11 at 14:07

3

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.

answered Apr 08 '11 at 14:07

S.Lott

45,522
6
93
155

score 1 · Answer 8 · answered Dec 15 '11 at 16:41

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.

score 1 · Answer 9 · answered Apr 08 '11 at 14:39

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).

score 1 · Answer 10 · answered Apr 08 '11 at 14:43

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.

score 1 · Answer 11 · answered Apr 08 '11 at 16:39

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.

Techniques to re-factor garbage and maintain sanity?

11 Answers11

Linked

Related