2つのオーバーロードされたメソッド、同じアクション、重複したコードの原因

Question

CircleまたはSceneの2つの方法があり、N msごとにfillの値を変更すると、背景が赤緑青になります。

二つの方法：明らかに

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

これらの私は両方含むそれらのスーパークラスを呼び出すのアプローチを使用することはできませんがCircleとSceneと同じ継承ツリーに含まれていない、非常に似ています.setFill()/.getFill()メソッド。

ここでコードの重複を削除するにはどうすればよいですか？

Answer 1

一般に、共通コードをファンクション/メソッド/クラスに因数分解し、変化する部分をパラメータ化することによって、重複コードを削除します。この場合、現在の塗りつぶしを取得する方法と、新しい塗りを設定する方法が異なります。 java.util.functionパッケージには、これらのパラメータ化のための適切な種類を提供し、あなたが行うことができます。

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

注意、しかし、あなたがバックグラウンドスレッドからUIを変更するべきではないこと。あなたは本当に定期的に物事を行うために

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

または、（良い）、use a Timelineを行う必要があります。

コメントに記載されているように、各色を後に来る色にマップするMapを指定することもできます。このすべてが与える組み合わせ：

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