HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Do they have the POWER?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thetheyhavepower

Problem

I'm working on a city building simulation game, and I have reached the point where I need to check whether tiles are connected to a power source. I decided on a flood fill for this, since it seemed simple enough to implement. For reference, I used this flood fill algorithm, and I also found this question on code review about flood fill in java. My code is largely similar to the code in those links, but it is slightly different because I am not filling an image with color but rather marking tiles connected to power plants as isPowered.

I know that recursion is dangerous, and the efficiency of the code may be suspect. With the map mostly filled up, this method gets called a couple of thousand times per cycle. With just a snaky path, about 200 times.

Here is an example to show that the algorithm is working:

The visible tiles are powered.

Here is the code:

private void calculatePower() {
    for (Tile tile : this.tilesForSort) {
        tile.hasBeenSearched = false;
    }
    for (Tile tile : this.tilesForSort) {
        if (tile.getBuildingType() == IsoTileType.POWER) {
            this.powerFill(tile.getPosition().x, tile.getPosition().y);
        }
    }
}
public void powerFill(int x, int y) {

    if (this.isNotInsideGrid(x, y)) {
        return;
    }

    Tile tile = this.levelData[x][y];

    if (tile.hasBeenSearched || tile.zoneType.getLevel() = World.WORLD_WIDTH || y >= World.WORLD_HEIGHT;
}

Solution

It seems that Tile is a key element in your game model.
On the other hand, a field like Tile.hasBeenSearched sounds something technical, not something that belongs to conceptual model.
You probably don't wake up in the morning thinking, "hm, it will be nice to give the Tile class a hasBeenSearched attribute". This field is an implementation detail of the flood-fill algorithm, and I don't think it fits well into Tile. It will be better to use a Set searched.
This field could be encapsulated within a FloodFiller class,
that takes tilesForSort and returns a collection of tiles for which the caller can set isPowered to true.
The advantage of this approach will be that you can reuse FloodFiller for other purposes, say, to actually flood your city.

This call is a bit tedious, with the duplicated tile.getPosition():

powerFill(tile.getPosition().x, tile.getPosition().y);


It would be good to create a helper powerFill(tile.getPosition()) that forwards to powerFill(int, int).

The name isNotInsideGrid is a bit clumsy. How about isOutsideGrid ?

Perhaps something like this might work well:

public class FloodFill {
    interface World {
        Set getEligibleNeighbors(Position position);
    }

    interface Position {
    }

    public static Set floodFill(World world, Position start) {
        Set searched = new HashSet<>();
        floodFill(world, searched, start);
        return searched;
    }

    private static void floodFill(World world, Set searched, Position position) {

        if (searched.contains(position)) {
            return;
        }

        searched.add(position);

        for (Position neighbor : world.getEligibleNeighbors(position)) {
            floodFill(world, searched, neighbor);
        }
    }
}


The idea is:

-
It is the responsibility of the World implementation to return the valid neighbors of a position that are eligible for filling:

  • x +- 1, y +- 1, or whatever coordinate system you use



  • Excluding positions outside of the world's boundary



  • Excluding positions where zoneType.getLevel()



  • Any other custom logic you might need



-
The
FloodFill` class doesn't need to know anything else about the world, and doesn't need to know what "filling" really means

This gives you the flexibility to reuse the flood fill logic to other purposes, without polluting the rest of your game model with specific knowledge catering to the flood fill process.

Code Snippets

powerFill(tile.getPosition().x, tile.getPosition().y);
public class FloodFill {
    interface World {
        Set<Position> getEligibleNeighbors(Position position);
    }

    interface Position {
    }

    public static Set<Position> floodFill(World world, Position start) {
        Set<Position> searched = new HashSet<>();
        floodFill(world, searched, start);
        return searched;
    }

    private static void floodFill(World world, Set<Position> searched, Position position) {

        if (searched.contains(position)) {
            return;
        }

        searched.add(position);

        for (Position neighbor : world.getEligibleNeighbors(position)) {
            floodFill(world, searched, neighbor);
        }
    }
}

Context

StackExchange Code Review Q#92786, answer score: 10

Revisions (0)

No revisions yet.