5

Short version:

I wrote some code that is not done "the right way" because, when I wrote it, I didn't know how to do it. Now that I know how to do it "the right way", how do I decide how to fix it?

Longer version with details:

java.util.DecimalFormat is not Thread safe. I have what's essentially an immutable Description object that is used all over my code over many different threads. To fix this problem, I created a wrapper class SynchronizedFormatter that basically just wraps a lock around the methods of DecimalFormat that I use.

The right way to do this is to use a ThreadLocal. I didn't even know what that was when I wrote the code the first time. Is it worth it to fix? The synchronized version works just fine. How do I decide, about this in specific and about this situation more generally?

Also, I should mention that I do need to make other changes to this class for more important reasons than "I did this bit wrong the first time". That's what got me thinking - if I'm mucking about in this class anyway, maybe I should fix these other things that are on my bucket list. But then I fear slippery slope...


This is not a duplicate of When to refactor because Refactoring means:

Improving a computer program by reorganising its internal structure without altering its external behaviour.

This question talks about actually modifying the behavior of the code (e.g. using synchronized vs. ThreadLocal. Perhaps it should not be tagged .

  • 6
    You should fix it only when you have a full test suite to validate the change didn't break anything. – user53019 Jan 28 '15 at 18:55
  • Relevant – Telastyn Jan 28 '15 at 18:56
  • possible duplicate of When to refactor – gnat Jan 28 '15 at 19:16
  • 3
    In my mind it's still refactoring - perhaps classes or methods are changing their behavior, but the program as a whole (from the user's point of view) isn't changing. Maybe I'm just parting rabbits. – Dan Pichelman Jan 28 '15 at 19:36
  • 1
    I would fix this to work the correct way. That formatting code may run quickly from a human's perspective, but this implementation ends up being a very large critical section of code. Best to use the ThreadLocal idiom instead, even if it is tedious work. – user22815 Jan 28 '15 at 19:36
4

A simple fix would be to replace the internals of your wrapper to use Threadlocal.

So the format() changes from:

private DecimalFormat format = new ...;

private final Object lock = new Object();

public String format(double value){
    synchronized(lock) {
        return format.format(value);
    }
}

to

private ThreadLocal<DecimalFormat> format = new ThreadLocal(){
    public DecimalFormat initialValue(){
        return new ...;
    }
};

public String format(double value){
    return threadLocalFormat.get().format(value);
}

This has the upside that the new implementation is threadsafe without the cost of synchronizing the calls and the interface for all the other classes remains the same. Thus the change is localized to only 1 class.

  • It's worse than that. The class as deployed actually returns a SynchronizedFormatter object :( – durron597 Jan 28 '15 at 19:00
  • @durron597 I meant as the implementation of the synchonizedFormatter – ratchet freak Jan 28 '15 at 19:01
  • 2
    This answer doesn't respond his question – Fabio Marcolini Jan 28 '15 at 19:15
  • 2
    I like this. Just simplify the inner workings of the SynchonizedFormatter wrapper and you don't have to touch any other code. – winkbrace Jan 28 '15 at 19:38
1

It all comes down to risk management and gut feeling. Try to estimate the following:

  • How big are the costs of the mistake as it is in the code right now. Does it cause recurrent costs? Or costs in form of a risk that it might turn into a bug in the future? What is the cost and the probability of that bug?

  • How big are the costs of fixing it right now: Risk of breaking something + the time you (and the rest of the team) need.

Finally do this calculation for NOW and for some time in the future. Some kind of problems are not such a big problem right now, but might turn into an ugly mess in a year from now, but are then also much more expensive to fix. You probably want to fix them now.

In most projects you won't do these with actual numbers, but based on your gut feelings.

I generally try to reserve some time for this kind of code maintenance, and use it for the issues with the highest return per investment, and adjust the total time for this so my code base stays on a constant, high quality.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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