tags:

views:

164

answers:

5

I would like to hear opinions on small amounts of code replication within methods that check for the same condition e.g.

While(condition){
...... do x 

}

Normally if there was any of this kind of replication I would refactor the code as it can make versioning a nightmare, what if the condition changes for example you have to change every instance, not a nice job to do. However what if the condition is relatively simple and is only used within say 3 methods is it wise to refactor,

So in summary where do people draw the line at refactoring code?

+2  A: 

Refactoring is not free. Any change to the code can introduce bugs. So every concious developer thinks about whether he has time to carefully examine the changes and in many cases decides not to refactor.

sharptooth
The OP doesn't explicitly mention whether the project was written from scratch using TDD (although the question suggests maybe not). But while a project with good unit test coverage is at risk to bugs introduced by refactoring as you say, that risk is significantly mitigated by good tests.
JeffH
Sure, but maintaining good tests in an obligation too. It's not free at all.
sharptooth
+2  A: 

It depends on 2 things, IMO - the size of the duplication (how many lines are involved), and the locality of the duplications - how 'close' are they in terms of context.

If you have duplicated code in several methods in the same class then I would consider extracting even duplicates of a single line into a seperate method (assuming that the line in question was an easly identifiable, easy to isolate, fairly uncoupled piece of code).

Alternatively, if I had some code in one part of a project, and an almost identical piece of code in an (almost) unrelated area then I wouldnt factor that out, as the scope for future divergence would seem quitre high....

Visage
+2  A: 

The key to refactoring is to do it when you need to. In the above example if you have 3 different while loops with the same condition, who is to say in the future you might want different conditions, if you've already refactored then you've introduced a potential error situation.

It's a matter of judgement, the same condition three times seems ok, the same condition 10 times is an obvious refactor, but where is the tipping point?

MrTelly
+1  A: 

I am personally usually quite aggressive with refactorings. I believe that if you clean your code regularly you mostly need to do small and simple refactorings. If you leave it for a while, it gets so messy and difficult to maintain.

In your particular case, I would definitely refactor if the condition has a reasonable business meaning, because it will make the code more readable. But even if this is a technicality, I would consider refactoring provided that its just the matter of extracting a function or property.

Ideally you should have unit tests that will make sure that your refactoring is still correct, so the cost of doing it should be really a few minutes, often smaller than writing this response.

Grzenio
+1  A: 

A common rule of thumb is the Rule of Three introduced by Martin Fowler in the seminal work Refactoring. The rule says that two things that are basically the same can stay, but once you add a third you should refactor them.

Besides making future changes easier as you mention, refactoring helps with readability and can make intention more obvious.

JeffH