Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Remove code duplication in specific example

I have three methods with duplicated code. The first two methods are nearly completely duplicated. The third one differs a bit as for fires more information should be drawn.

I want to remove this duplicated code and thought about the template method pattern using inner classes. Is this the right way to go or is there a better solution?

private void drawWaterSupplies(Graphics g) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

    int imageOffsetX = waterSupplyImage.getWidth() / 2;
    int imageOffsetY = waterSupplyImage.getHeight() / 2;
    for (Location l : groundMap.getWaterSupplyLocations()) {
        int x = (int) (l.getX() * hScale);
        int y = (int) (l.getY() * vScale);

        g.drawImage(waterSupplyImage, x - imageOffsetX, y - imageOffsetY,
                null);
    }
}

private void drawEnergySupplies(Graphics g) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

    int imageOffsetX = energySupplyImage.getWidth() / 2;
    int imageOffsetY = energySupplyImage.getHeight() / 2;
    for (Location l : groundMap.getEnergySupplyLocations()) {
        int x = (int) (l.getX() * hScale);
        int y = (int) (l.getY() * vScale);

        g.drawImage(energySupplyImage, x - imageOffsetX, y - imageOffsetY,
                null);
    }
}

private void drawFires(Graphics g) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

    int imageOffsetX = fireImage.getWidth() / 2;
    int imageOffsetY = fireImage.getHeight() / 2;
    for (Fire fire : groundMap.getFires()) {
        Location l = fire.getLocation();
        int x = (int) (l.getX() * hScale);
        int y = (int) (l.getY() * vScale);

        g.drawImage(fireImage, x - imageOffsetX, y - imageOffsetY, null);
        // TODO: draw status bar showing state of fire below
    }
}
like image 262
kobo Avatar asked Mar 15 '26 01:03

kobo


1 Answers

It seems to me your collection of objects (Fire, WaterSupply etc.) aren't as clever as they should be. Ideally you should be able to say:

for (Fire f : groundMap.getFires()) {
   f.draw(g);
}

and each object would be able to locate itself (using its location), size itself (since a Fire knows it's going to use a FireImage etc.) and draw itself onto the provided Graphics object.

To take this further, I would expect to pass a Graphics object to your groundMap thus:

groundMap.drawFires(g);

The idea is that in OO you shouldn't ask objects for their details and then make decisions. Instead you should tell objects to do things for you.

like image 53
Brian Agnew Avatar answered Mar 17 '26 15:03

Brian Agnew



Donate For Us

If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!