CODEWITHSUNDEEP
javacode-duplication
kobo
kobo

Reputation: 155

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

Upvotes: 3

Views: 214

Answers (8)

user902383
user902383

Reputation: 8640

and thats my proposition

enum  Supplies {FIRE(fireImage), WATER(waterImage), ENERGY(energyImage); 
private Bitmap image;
Supplies(Bitmap image)
 {
  this.image = image
 }

 public getImage()
 {
  return image;
 }

} 

private void draw(Graphics g,Supplies supply) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

    int imageOffsetX = supply.getImage.getWidth() / 2;
    int imageOffsetY = supply.getImage.getHeight() / 2;
    for (Location location : groundMap.getLocations(supply)) {

        int x = (int) (location .getX() * hScale);
        int y = (int) (location .getY() * vScale);

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

.... all location for fire, water, energy etc you can hold in Map>, so method getLocations(supply) will be more or less look like that

List<Location> getLocations(Supplies supply)
{
return supplyMap.get(supply);
}

this solution gives you more flexibility if you want to add or remove Supplies and accidents

Upvotes: 0

amukhachov
amukhachov

Reputation: 5900

Not the shortest alternative but it makes code more clean and easier to extend:

enum YourEnum {
    WATER,
    ENERGY,
    FIRE;
}

private void draw(Graphics g, YourEnum type) {
    Bitmap bitmap = getRightBitmap(type);
    double hScale = getWidth() / (double)groundMap.getWidth();
    double vScale = getHeight() / (double)groundMap.getHeight();

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

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



private Bitmap getRightBitmap(YourEnum type) {
    switch (type) {
        case WATER:
            return waterSupplyImage;
        case ENERGY:
            return waterSupplyImage;
        case FIRE:
            return fireImage;
    }
}

private Collection<Location> getLocations(YourEnum type) {
    switch (type) {
        case WATER:
            return groundMap.getWaterSupplyLocations();
        case ENERGY:
            return groundMap.getEnergySupCollections();
        case FIRE:
            Collection<Location> locations = new ArrayList<Location>();
            for (Fire fire : groundMap.getFires()) {
                locations.add(fire.getLocation());
            }
            return locations;
    }
}

Upvotes: 0

RNJ
RNJ

Reputation: 15562

I would delegate to another method and create a super class for Fire, Water and Energy. This super class would ctonain all the common attributes. Eg getLocation()

eg

private void drawEverything(Graphics g, Image im, List<? extends SuperClassOfFireEtc> list, double w, double h) {
    double hScale = getWidth() / w;
    double vScale = getHeight() / h;

   int imageOffsetX = im.getWidth() / 2;
   int imageOffsetY = im.getHeight() / 2;
   for (SuperClassOfFireEtc f : list) {
       Location l = f.getLocation();
       int x = (int) (l.getX() * hScale);
       int y = (int) (l.getY() * vScale);

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

}

Then drawFire can call

 private void drawEverything(g, fireImage, groundMap.getFires(), groundMap.getWidth(), groundMap.getHeight()) {

Upvotes: 1

Wug
Wug

Reputation: 13196

How about:

private void drawImageAtLocations(Graphics g, Image i, Collection<Location> cl) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

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

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

Works right out of the box for the first two:

drawImageAtLocations(g, waterSupplyImage, groundMap.getWaterSupplyLocations());
drawImageAtLocations(g, energySupplyImage, groundMap.getEnergySupplyLocations());

The third one is a bit messier but still shorter than what's originally there:

Set<Location> derp = new HashSet<Location>();
for (Fire fire : groundMap.getFires()) derp.add(fire.getLocation());
drawImageAtLocations(g, fireImage, derp);
// drawImageAtLocations(g, fireStatusBarImage, derp); // TODO blah blah

Upvotes: 1

Brian Agnew
Brian Agnew

Reputation: 272397

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.

Upvotes: 5

Peter Lawrey
Peter Lawrey

Reputation: 533820

You could have a method.

private void drawImageAtLocations(Graphics g, Image image, List<Location> locations) {
    double hScale = getWidth() / (double) groundMap.getWidth();
    double vScale = getHeight() / (double) groundMap.getHeight();

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

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

private void drawWaterSupplies(Graphics g) {
    drawImageAtLocations(g, waterSupplyImage, groundMap.getWaterSupplyLocations());
}

Upvotes: 0

Augusto
Augusto

Reputation: 29997

I think a simple solution would be to have one method and pass Graphics and a collection of Location.

Upvotes: 0

helios
helios

Reputation: 13841

First it seems you could have a common method that receives Graphics, a List<Location> and an image so first and second method could extract that list and call.

The third method could extract the list of Fire, and then create a corresponding List<Location>. Then use the new method.

private void drawImages(Graphics g, List<Location> where, Image imageToDraw) {
...
}

Upvotes: 0

Related Questions