Extreme Programming - Refactoring other people's code

Author

Refactoring other people's code

Robert Atkins

2004-05-12, 7:44 pm

I've just started work in a company that is not XP but is "Agile
friendly" and I think would like to be more XP as a whole. So for my
part I'm trying to not write any code without tests and I've begun to
"subtly pair".

There's a lot of code I'd like to refactor. Heaps of duplication across
the codebase, some really nasty tight coupling and some just plain
poorly written code.

How do I do this so it doesn't look like I'm saying, "I think whoever
wrote this is an idiot!". Note that I don't necessarily think that. But
coming in off the street and saying, "All this needs changing!" could
be seen as confronting.

Cheers, Robert.

Phlip

2004-05-12, 7:44 pm

Ilja Preuß

2004-05-12, 7:44 pm

Robert Atkins wrote:

> How do I do this so it doesn't look like I'm saying, "I think whoever
> wrote this is an idiot!". Note that I don't necessarily think that.
> But coming in off the street and saying, "All this needs changing!"
> could be seen as confronting.

Show patience, don't try to remove all the problems at once.

Pair with the original author of the code - "I have some problems
understanding this code - can someone help me, please?"

When you pair on the code, ask questions about it where it isn't fully
clear. Suggest to put the answer into the code, so that you will remember
later.

"What is this fl variable?" "Ah, it's the list of foos - do you mind if we
rename it to fooList or foos?"

Hope this helps, Ilja

Jason Nocks

2004-05-12, 7:44 pm

Robert Atkins wrote:

> I've just started work in a company that is not XP but is "Agile
> friendly" and I think would like to be more XP as a whole. So for my
> part I'm trying to not write any code without tests and I've begun to
> "subtly pair".
>
> There's a lot of code I'd like to refactor. Heaps of duplication across
> the codebase, some really nasty tight coupling and some just plain
> poorly written code.

I can relate to this. First, there's one question: Does the code
predominantly do what people want it to without lots of bugs? If so,
then refactoring and adding tests over time is likely to be worth the
effort. If not, then quite possibly it's not. That's the harsh reality.

> How do I do this so it doesn't look like I'm saying, "I think whoever
> wrote this is an idiot!". Note that I don't necessarily think that. But

You could try: "I need to change this code to get this test to pass."
Shift the focus from the people to the actual problem. This does require
a failing test, though.

You may even need to introduce a small amount of duplication just to
initially write a test and then get it to pass. Then, you need to
eliminate as much of the duplication as possible after getting the test
to pass.

At my company, we've taken on a few projects that have large amounts of
untested, tangled code. One approach that has worked for us at the start
is to start implementing new functionality within the actual test
fixture, then move methods to the most appropriate object after getting
the test to pass. We do this when we can't see an easily testable object
to start with (too much coupling). As we start to tease apart the
coupling, and introduce smaller more testable objects, we probably do
this less often.

> coming in off the street and saying, "All this needs changing!" could
> be seen as confronting.

It also probably wouldn't be recommended. You probably don't want to rip
apart a lot of the code and rewrite it. That's not really refactoring,
that's rewriting. If you rewrite large sections of code, you have an
increased chance of introducing bugs that might not be so easy to find.

IMHO, refactoring works really well as part of TDD, where you work in
small increments, starting with failing tests, and trying to keep the
tests small and focused.

> Cheers, Robert.

Hope this helps. Just my $.02.

Cheers,
Jason Nocks
SourceXtreme, Inc.
http://www.sourcextreme.com/

Robert C. Martin

2004-05-12, 7:44 pm

On 12 May 2004 13:08:57 GMT, Robert Atkins
<ratkins_usenet@yahoo.com.au> wrote:

>I've just started work in a company that is not XP but is "Agile
>friendly" and I think would like to be more XP as a whole. So for my
>part I'm trying to not write any code without tests and I've begun to
>"subtly pair".
>
>There's a lot of code I'd like to refactor. Heaps of duplication across
>the codebase, some really nasty tight coupling and some just plain
>poorly written code.
>
>How do I do this so it doesn't look like I'm saying, "I think whoever
>wrote this is an idiot!". Note that I don't necessarily think that. But
>coming in off the street and saying, "All this needs changing!" could
>be seen as confronting.

1. Don't refactor without tests. That's not refactoring, that's
rewriting. If you need to refactor something that's not under test,
put it under test first. (Read Michael Feather's book "Working
Effectively with Legacy Code)

2. Don't refactor just to refactor. Only refactor in the area that
you are currently working; and always toward getting new functionality
in place.

3. Don't refactor alone. Always gets someone else's advice and help.

At the XPAU conference last year we had a on-going lab exercise called
"Fit-Fest". We had a small application working and a group of stories
to be implemented against it. We even had acceptance tests written
for the new stories. Folks would pop into the lab for a couple of
hours to work on the project.

By the end of the conference there were no new stories implemented, no
new acceptance tests passing, and yet the code had grown to three
times it's original size. Folks had been coming into the lab to
refactor as opposed to make new stories work. Be wary of this.

-----
Robert C. Martin (Uncle Bob)
Object Mentor Inc.
unclebob @ objectmentor . com
800-338-6716

"The aim of science is not to open the door to infinite wisdom,
but to set a limit to infinite error."
-- Bertolt Brecht, Life of Galileo

Jeff Grigg

2004-05-12, 7:44 pm

Robert Atkins <ratkins_usenet@yahoo.com.au> wrote...
> There's a lot of code I'd like to refactor. Heaps of duplication across
> the codebase, some really nasty tight coupling and some just plain
> poorly written code.
>
> How do I do this so it doesn't look like I'm saying [...]
> "All this needs changing!" could be seen as confronting.

No matter how ugly the code, it doesn't make business sense to go off
on a refactoring cru

e. (...unless someone wants to fund a rewrite
of the software, which is unlikely.)

So,
While making changes requested by the users, look at the code that
you're changing and think about how you can improve it. Are the
comments wrong or misleading? Improve them. Are the variable names
less revealing than they could be? Improve them. Could the method
names and method breakdown be improved? Improve them. Etc.

I like to use this rule, which I made up:
"Don't spend more time refactoring than implementing user requested
functionality."

With particularly nasty legacy code, this might help keep one from
going off on refactoring cru

es that may turn out bad.

Laurent Bossavit

2004-05-12, 7:44 pm

Bob:

> By the end of the conference there were no new stories implemented, no
> new acceptance tests passing, and yet the code had grown to three
> times it's original size. Folks had been coming into the lab to
> refactor as opposed to make new stories work. Be wary of this.

Bring the same code at the next conference, and see if you can get the
reverse to happen. :)

Laurent
http://bossavit.com/thoughts/

Ronald E Jeffries

2004-05-12, 8:30 pm

Robert Atkins

2004-05-17, 9:30 am

In article <y8roc.18234$yH4.9509@newssvr31.news.prodigy.com>, Phlip wrote:
> Robert Atkins wrote:
>
> Never do anything without multiple reasons to do it. R around a bug's site,
> to leverage your new awareness of its control flow. R if something really
> sucks, and if someone asks a question about it. R to introduce a few learner
> tests. R only with a pair. R with the codes author - "I need whoever wrote
> this to help on it.."

I will admit I have slightly suckered myself into a "refactoring
cru

e" after being tasked with adding new functionality to an existing
module. I am *trying* to only refactor around code that I'm adding
functionality to but the feature I'm adding touches a lot of the code.

I'm attempting to refactor only with a pair (the other new guy) but of
course we only got away with that for a day before he was temporarily
re-assigned to fix some bugs unrelated to our new features. Refactoring
with the original code's author is a good idea but I don't think the
original code's author is a "willing pair".

> As code cleans up, add lots of TODO comments, saying where to R more. Fixing

I have been doing this.

> Then, as the team realizes the TODOs are holding them back, start a new
> module. Comment it "TODO-free zone". Use test-first to force all code that

We've sort-of got this. Part of the reason for the duplication is that
the code is spread across a number of different modules and code was
copied between modules if needed. There's now a "common" module which
I'm trying to fold the copies back in to.

Cheers, Robert.

Robert Atkins

2004-05-17, 9:30 am

In article <290ec$40a24c41$cff54506$11971@dcanet.allthenewsgroups.com>,
Jason Nocks wrote:
> Robert Atkins wrote:
>
>
> I can relate to this. First, there's one question: Does the code
> predominantly do what people want it to without lots of bugs? If so,
> then refactoring and adding tests over time is likely to be worth the
> effort. If not, then quite possibly it's not. That's the harsh reality.

Just to make sure I'm not misreading you: you're saying that if the code
is used and mostly works, it's worth writing tests for. If it's horribly
broken and not used much, don't bother writing tests?

The code is being used in production and mostly working, there's nothing
unsalvable there.

> You could try: "I need to change this code to get this test to pass."
> Shift the focus from the people to the actual problem. This does require
> a failing test, though.

This is a hard bit: the code *is* so tightly coupled in places it's
difficult to write tests which exercise just one method or class at
once. I've had to re-write classes as Interfaces and change constructors
so I can inject mocks and things like that.

>
> It also probably wouldn't be recommended. You probably don't want to rip

Hence why I'm asking for advice ;-)

> IMHO, refactoring works really well as part of TDD, where you work in
> small increments, starting with failing tests, and trying to keep the
> tests small and focused.

As above, I'm trying to do this but it's hard.

Cheers, Robert (and thanks to all in this thread for the advice).

Jason Nocks

2004-05-18, 10:42 am

Robert Atkins wrote:

> In article <290ec$40a24c41$cff54506$11971@dcanet.allthenewsgroups.com>,
> Jason Nocks wrote:
>
>
> Just to make sure I'm not misreading you: you're saying that if the code
> is used and mostly works, it's worth writing tests for. If it's horribly
> broken and not used much, don't bother writing tests?

Thanks for the clarification. No, that's not what I was trying to say. I
meant, if the code is "horribly broken and not used much" start with new
clean test-driven code rather than trying to get there by refactoring.
If the code is worth keeping, then I highly recommend adding tests!

> The code is being used in production and mostly working, there's nothing
> unsalvable there.

My point was along the lines of what's the most effecient way to get to
"clean code that works". If the current codebase is neither clean nor
working, then you don't gain much by preserving what you have. Now that
you've stated that the code is "in production and mostly working", I
would say go ahead and add tests and refactor.

I wouldn't "worry" about the existing code that is not well tested and
that needs refactoring. Don't fix all the code that could be fixed. It's
best to focus only on cleaning up the code that's in the way of getting
your work done. And, whenever a bug is reported, add a test for that
bug. Over time, the code will get better and better. Have patience. If
you finally get to the point where there's a very small amount of
unclean and untested code, you could make a last, final effort to clean
up the rest. But, until that point, I highly recommend small, localized
refactorings to make it easier to add what you need to add.

>
> This is a hard bit: the code *is* so tightly coupled in places it's
> difficult to write tests which exercise just one method or class at
> once. I've had to re-write classes as Interfaces and change constructors
> so I can inject mocks and things like that.

Yes, it can be a bit hard in the scneario you are describing. That's why
I started with the question about whether or not it's worth preserving
the current codebase.

At the risk of violating once-and-only-once, I'm going to re-post one of
my previous comments because it directly addresses trying to use TDD
with existing, tightly coupled code (and as far as I know, it's not a
well documented approach):

"One approach that has worked for us at the start is to start
implementing new functionality within the actual test fixture, then move
methods to the most appropriate object after getting the test to pass.
We do this when we can't see an easily testable object to start with
(too much coupling)."

It looks like I left out a step there (extract method). But, we found
this approach to be tremendously helpful in the exact scenario you are
describing. Also, we did not hesitate to create new, more testable
objects and move tested methods to the testable objects to work around
tight coupling in the earlier stages. I can't imagine that we were the
first ones to come up with this approach, but I'm not aware of anywhere
else that it's documented.

>
> Hence why I'm asking for advice ;-)

Well, I for one, and very glad that you asked. I hope the responses here
have been helpful.

>
> As above, I'm trying to do this but it's hard.

IMHO, this part, at least, gets easier. Whether refactoring existing,
tightly coupled code or not, when things start to get hard, you probably
want to step back and work in smaller steps. Just my $.02.

> Cheers, Robert (and thanks to all in this thread for the advice).

Let us know how it goes and if you run into further areas or examples
that you'd like to discuss.

Cheers,
Jason Nocks
SourceXtreme, Inc.
http://www.sourcextreme.com/

Jason Nocks

2004-05-18, 10:42 am

Robert Atkins wrote:

> Cheers, Robert (and thanks to all in this thread for the advice).

Hey, I almost forgot. Thank you for posting your questions here. I'm
sorry if part of my initial response wasn't very clear.

Cheers,
Jason Nocks
SourceXtreme, Inc.
http://www.sourcextreme.com/

Sponsored Links