How to avoid duplicated code, field vs local variable

Question

I have a class that, in some scenarios, updates it's internal state permanently, and in other scenarios, calculates a projected value. The process of updating the internal state is the same as the process of calculating the projected value, but because one works with local variables and one works with fields I don't see a way to avoid copy/pasting of code.

I think the problem is because foo and bar are primitives, which means they can't be updated by reference. But for an example as simple as this one does it make sense to wrap foo and bar in an inner class?

public abstract class RatioUpdate {
    private double foo;
    private double bar;

    public Update(double foo, double bar) {
        this.foo = foo;
        this.bar = bar;
    }

    public double getRatio() {
        return foo / bar;
    }

    public double updateData(double input) {
        foo = recalculateFoo(foo, input);
        bar = recalculateBar(bar, input);

        return foo / bar;
    }

    public double getProjectedRatio(double input) {
        double newFoo = recalculateFoo(foo, input);
        double newBar = recalculateBar(bar, input);

        return newFoo / newBar;
    }

    // Abstract because not really relevant, this was the best I could do to avoid duplication
    protected abstract recalculateFoo(double foo, double input);
    protected abstract recalculateBar(double bar, double input);
}

This looks like an opinion question. I probably would wrap them in an inner class, just to make sure that only one place has to be changed if the algorithm changes in the future. If there's duplicated code and I have to change both occurrences, I find that I may miss one even in a small class like this. — ajb, Apr 17 '14 at 17:36
I think that the way you have it is as good as it will get, to be honest. — Boann, Apr 17 '14 at 17:47
@Vitaliy it doesn't ignore the state of the class, the `recalculateFoo` method uses the stateful `foo` field. — durron597, Apr 17 '14 at 19:05
Right, what I meant is that you can update the class with one value and immediately make a query for another value. It seems... strange. — Vitaliy, Apr 17 '14 at 21:09

score 2 · Answer 1 · answered Apr 17 '14 at 18:10

One thing I can think of is 'Extract Class'. If you have the book 'Refactoring - Improving the design of existing code' by Martin Fowler, you'll find data clumps on page 81.

It seems foo and bar are always together. They might be a data clump. A quote from the book: "A good test is to consider deleting one of the data values: if you did this, would the others make sense? If they don't, it's a sure sign that you have an object that's dying to be born."

If you do extract a class, you'll end up with something that looks a lot like what Maurice suggested. However, I'd personally add final tags to foo and bar to make the object immutable.

+1, but my fear is that the performance hit for object creation/destruction is not worth it. These methods (both of them) are called often. — durron597, Apr 17 '14 at 19:13

score -1 · Answer 2 · answered Apr 17 '14 at 17:43

Consider splitting your class in two:

public class Ratio {
    private double foo;
    private double bar;

    public Ratio(double foo, double bar) {
        this.foo = foo;
        this.bar = bar;
    }

    public double getRatio() {
        return foo / bar;
    }
}

public abstract class RatioUpdate {
    Ratio ratio;

    public RatioUpdate(Ratio ratio) {
        this.ratio = ratio;
    }

    public void updateData(Ratio ratio) {
        this.ratio = ratio;
    }

    public Ratio getProjectedRatio(double input) {
        return recalculateRatio(ratio, input);
    }

    // Abstract because not really relevant, this was the best I could do to avoid duplication
    protected abstract Ratio recalculateRatio(Ratio ratio, double input);
}

Nice idea, but your implementation has problems. I wouldn't want `Ratio` to be its own top-level class, because it doesn't make sense out of the `RatioUpdate` context. Also, `foo` and `bar` aren't exposed so `recalculateFoo` cannot actually be defined here. — durron597, Apr 17 '14 at 19:08

How to avoid duplicated code, field vs local variable

2 Answers2