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?

Answer 1

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();
}