Two overloaded methods, same actions, causing duplicate code

Question

I have two methods that take either a Circle or a Scene and make the background flash red - green - blue by changing the fill value every N ms.

The two methods:

private void makeRGB(Circle c) {
    Timer t = new Timer();
    t.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            if(c.getFill() == Color.RED) {
                c.setFill(Color.GREEN);
            } else if (c.getFill() == Color.GREEN) {
                c.setFill(Color.BLUE);
            } else {
                c.setFill(Color.RED);
            }
        }
    },0, RGB_CHANGE_PERIOD);
}

private void makeRGB(Scene s) {
    Timer t = new Timer();
    t.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            if(s.getFill() == Color.RED) {
                s.setFill(Color.GREEN);
            } else if (s.getFill() == Color.GREEN) {
                s.setFill(Color.BLUE);
            } else {
                s.setFill(Color.RED);
            }
        }
    },0, RGB_CHANGE_PERIOD);
}

Obviously these are very similar, however as Circle and Scene aren't in the same inheritance tree I can't use the approach of calling a superclass of them both containing the .setFill() / .getFill() methods.

How would I go about removing code duplication here?

James_D · Accepted Answer · 2017-12-21T20:46:06.813

In general, you remove duplicate code by factoring the common code into a function/method/class and parameterizing the parts that vary. In this case, what varies is the way you retrieve the current fill, and the way you set the new fill. The java.util.function package provides the appropriate types for parametrizing these, so you can do:

private void makeRGB(Circle c) {
    makeRGB(c::getFill, c:setFill);
}

private void makeRGB(Scene s) {
    makeRGB(s::getFill, s:setFill);
}

private void makeRGB(Supplier<Paint> currentFill, Consumer<Paint> updater) {
    Timer t = new Timer();
    t.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            if(currentFill.get() == Color.RED) {
                updater.accept(Color.GREEN);
            } else if (currentFill.get() == Color.GREEN) {
                updater.accept(Color.BLUE);
            } else {
                updater.accept(Color.RED);
            }
        }
    },0, RGB_CHANGE_PERIOD);
}

Note, though, that you should not change the UI from a background thread. You should really do

private void makeRGB(Supplier<Paint> currentFill, Consumer<Paint> updater) {
    Timer t = new Timer();
    t.scheduleAtFixedRate(new TimerTask() {
        @Override
        public void run() {
            Platform.runLater(() -> {
                if(currentFill.get() == Color.RED) {
                    updater.accept(Color.GREEN);
                } else if (currentFill.get() == Color.GREEN) {
                    updater.accept(Color.BLUE);
                } else {
                    updater.accept(Color.RED);
                }
            }
        }
    },0, RGB_CHANGE_PERIOD);
}

or, (better), use a Timeline to do things on a periodic basis.

As noted in the comments, you can also provide a Map that maps each color to the color that comes after it. Combining all this gives:

private final Map<Paint, Paint> fills = new HashMap<>();

// ...

    fills.put(Color.RED, Color.GREEN);
    fills.put(Color.GREEN, Color.BLUE);
    fills.put(Color.BLUE, Color.RED);

// ...

private void makeRGB(Circle c) {
    makeRGB(c::getFill, c:setFill);
}

private void makeRGB(Scene s) {
    makeRGB(s::getFill, s:setFill);
}

private void makeRGB(Supplier<Paint> currentFill, Consumer<Paint> updater) {

    Timeline timeline = new Timeline(Duration.millis(RGB_CHANGE_PERIOD), 
        e-> updater.accept(fills.get(currentFill.get())));
    timeline.setCycleCount(Animation.INDEFINITE);
    timeline.play();
}

Also, a `static Map nextColorMap` could replace the multi-storied `if` statement with a single `updater.accept(nextColorMap.get(currentFill.get()));`. — 9000, Dec 21 '17 at 19:44
Thanks for the answer, this is what I was looking for; although I need to look into the `Consumer` class more :) @9000 Thanks for the tip. — Mat Whiteside, Dec 21 '17 at 19:50
Instead of passing `Consumer`/`Supplier` you could simply pass the property, since it allows you to set and get the value... — fabian, Dec 21 '17 at 22:23
@fabian True indeed. (This technique does work in more general cases, though.) — James_D, Dec 21 '17 at 22:39

Two overloaded methods, same actions, causing duplicate code

1 Answers1