ss_12036337fe569dee7853cdd275808b49c0f7339d.1920x1080

cropped-button Introduction

I’ve recently came across nice, small project in Java, which is an attempt to recreate one of my childhood game – RollerCoaster Tycoon. Project was uploaded on Sourceforge in 2016 and has some basic features of original game. I though it
is a nice occasion to dive into code and learn something – since I always wanted to learn math of 2D isometric games, and this project seems to have good isomethric algorithms.

So I look at the code… And I thought that I need to do some refactor to understand the project.

cropped-button Before we start…

From the very high perspective, original author’s project is fine. Application is ready, easy to run and what is more important – works correctly.
So the project from product perspective is correct. It has basic features of original game like 2D isometric world generation, park design (pavements, trees, benchmarks) and guests with simple logic. Project looks like nice PoC.

For me the most valuable feature of the project was ability to learn rules 2D isometric world. So after I stop playing with running application I’ve look deep into code.

cropped-button Why I’ve decided to start working with legacy code

The code that I found was really hard to read. I was trying to trace some processes, but it was really difficult. Lack of tests also did not make the task easier.

I thought “Ok, it is a good idea for code refactoring training”. Because I have running application I can start to play with legacy code and try to understand it by reading, refactoring and writing the tests. Since application has small set of features I can always finally run it and check if my refactor broke something.

Before I will start with some refactor, here is the original authors project.
My refactored code can be found here.

cropped-button Great, let’s start!

First thing that I’ve noticed was simple src folder in project folder. I’ve decided to introduce Maven build tool and make project as Maven project.
This will help me with dependency managment, and now I can use build process to build target folder. Moreover build process will always launch every tests to ensure, that my future changes won’t broke application logic.

First look at the IDE and I saw not very deep package structure. In src folder we have plain package names without any company, author or project name prefixes.

packages

To avoid future naming collisions, let’s follow java naming package conventions and rename packages to this form:

refactored-packages

Good. Packages names seems to be ok. At the first look they are separated by feature, not layer or technical responsibility. This is important for code understanding. When we first look at this package structure, we can see some application contexts. Maybe not all are obvious, but for me guests, isometric, toolbox, world, and worldGen are understandable.

For those who wants to read more about packaging by feature I suggest to read this article.

cropped-button Detecting the main classes

Since I have quite nice packages structure from the beginning I should easy detect main application classes and understand relationships between them. Let’s look at all classes in project:

packages-opened

First look into classes structure and I can highlight some of them:

  • Guest – class that describe game guest. Guest can be placed only on pavement, and his only logic at this moment is simply taking some path and moving.
  • World – class which represents living game world. Contains Toolbox and list of created Tiles. Can be stored and restored on disc by WorldGen object.
  • Tile – third the most conspicuous class. Represents tile, which can be empty or can contain some object, or pavement with one to many guests.

Among many other classes, these seems to be the most important from game logic perspective. What is striking, each of them seem to have too many responsibilities. Just take a look at the Guest class.
For this moment Guest object is doing a few things:

  1. Has the logic responsible for obtaining specified Guest image
  2. Has the algorithm for drawing itself on map
  3. Has the algorithm which obtains new direction
  4. Has the move algorithm

For me this seems like violation of S.R.P defined by Robert Martin.

cropped-buttonTesting the World

As I mentioned before, World class represents game world. Starting with investigate this class seems to be good idea because it should have a lot of entrypoints to the game context.

Let’s try to understand some method. Here is how we will work:

  1. Read method and try to understand it
  2. Write unit tests based on assumptions
  3. Run tests and make them green
  4. If there is a need, refactor code
  5. Run tests. If any test fails check why and try to fix it

Tests will help us with understanding components logic. They also will be our
guardians when we decide to refactor code. Having a lot of unit tests, which cover code logic gives us confidence for any changes. If we change the logic by accident during refactor, we have very fast feedback on test run. We do not need to run whole application and try to reproduce certain circumstances to test changed logic.

For unit testing I will use Spock Framework and AssertJ. Let’s add them into pom.xml dependencies:

 <dependencies>
        <dependency>
            <groupId>org.spockframework</groupId>
            <artifactId>spock-core</artifactId>
            <version>1.1-groovy-2.4</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.assertj</groupId>
            <artifactId>assertj-core</artifactId>
            <version>3.8.0</version>
            <scope>test</scope>
        </dependency>
    </dependencies>

OK World, here we go. Prepare yourself for some refactoring and testing! For example, we will take addGuestAt(int x, int y) method.

public void addGuestAt(int x, int y) {
        for (int i = 0; i < tiles.length; i++) {
            for (int j = 0; j < tiles[i].length; j++) {
                if (tiles[i][j].getPolygon().contains(x, y)) {
                    if (tiles[i][j].doesHaveNeighbors() && tiles[i][j].isPavement()) {
                        Guest guest = new Guest(tiles[i][j]);
                        tiles[i][j].addNewGuestToList(guest);
                    }
                }
            }
        }
}

This method takes two integer input parameters, which are some map coordinates. Method start to iterating through every Tile in two dimensional tiles array.
Each Tile has getPolygon() method, which returns Tile polygon. Then method checks if passed coordinates are inside that polygon. If yes, then method checks if tile is pavement Tile and does have neighbors. If yes, then guest is added to tile list.

Checking isPavement() on Tile indicates, that Guest can be added only on Tile with pavement. Moreover, this Tile need to have neighbors (whatever it means, since every Tile has neighbors. Maybe it is about neighbors with pavement? We will check that later).

Let’s write WorldTest with method that creates us Tile with pavement and neighbors (passes itself as neighbor):

class WorldTest extends spock.lang.Specification {
    
    private Tile correctPavementTile(double xPos, double yPos) {
        Tile tile = new Tile(xPos, yPos)
        tile.pavement = true
        tile.x0 = -550
        tile.y0 = 300
        tile.addAsNeighbor(tile, 'N' as char)
        return tile;
    }
}

Then we can add tests and check if out assumptions were correct:

 def "should place guest at chosen pavement tile"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        world.tiles[0][0] = tile

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isNotEmpty()
    }

    def "should not place guest at tile when tile has not pavement"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        tile.pavement = false
        world.tiles[0][0] = tile

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isEmpty()
    }

    def "should not place guest at tile when tile has no neighbours"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        tile.removeNeighbor(0)
        world.tiles[0][0] = tile

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isEmpty()
    }

Let’s run written tests:

tests

After running these tests we can see, that our assumptions were correct. Every test passed.

Now we can start to refactor this method…

cropped-button Refactoring

If we closely look at the addGuestAt(int x, int y) method we will see, that this method takes every Tile and extracts data from them to make sure that Tile is valid for Guest placement. My question is… why World is trying to detect if Tile is valid for take any Guest? This should be responsibility for Tile! Since Tile has all data required for answer the question we should create proper method inside Tile and just call that method from the World.

This method in Tile should look like this:

public boolean canGuestBePlaced(int x, int y) {
        return getPolygon().contains(x, y) && doesHaveNeighbors() && isPavement();
    }

This approach has several advantages. First of all, we encapsulate logic based on Tile data inside Tile. Only Tile knows what is required to place Guest on Tile. If these requirements will change in the future, the only place for change will be this method. Clients which use that method are unaware of that requirements and won’t fall, if the requirements change or Tile data change.
Second of all, logic is not leaking across the application. Logic refered to Tile is kept close to Tile and can be quickly found and noticed.

Applying this method to World method, we have:

  public void addGuestAt(int x, int y) {
        for (int i = 0; i < tiles.length; i++) {
            for (int j = 0; j < tiles[i].length; j++) {
                if (tiles[i][j].canGuestBePlaced(x, y)) {
                    Guest guest = new Guest(tiles[i][j]);
                    tiles[i][j].addNewGuestToList(guest);
                }
            }
        }
    }

Tests passed. Nice. After that change only Tile can tell the World if Tile is good enough to take new Guest.

Let’s go further. I do not see any advantage of keeping tiles in two dimensional array. It is very difficult to read code with two for loops iterating through tiles.
To increase readability I will stick to the List.

private Tile[][] tiles;
List<Tile> tilesList;

I’ve added tilesList along the tiles two dimensional array. Because I do my changes incrementally I can not remove tiles array now. I do not have tests that covers other World methods to make that change. I’m not confident about that changes. So temporary tiles will stay and I will do my changes only in refactored method covered by tests.

List is filled with Tiles on World object creation along with tiles array. No we can change the method:

public void addGuestAt(int x, int y) {
        tilesList.stream()
                .filter(t -> t.canGuestBePlaced(x, y))
                .forEach(t -> {
                    Guest guest = new Guest(t);
                    t.addNewGuestToList(guest);
                });
    }

Whoa! Now this method looks much more cleaner and simpler. There is no more distracting noise from two for loops and three if conditionals. When you look at this method, you see that every Tile from tilesList is being filtered, and then each of Tile that meet requirements gains new Guest.

But what about tests? Since we are no longer using tiles array in this method, we need to change tests a little bit:

def "should place guest at chosen pavement tile"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        world.tilesList.add(tile)

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isNotEmpty()
    }

    def "should not place guest at tile when tile has not pavement"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        tile.pavement = false
        world.tilesList.add(tile)

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isEmpty()
    }

    def "should not place guest at tile when tile has no neighbours"() {
        given:
        World world = new World()
        Tile tile = correctPavementTile(12.5, -2.5)
        tile.removeNeighbor(0)
        world.tilesList.add(tile)

        when:
        world.addGuestAt(810, -80)

        then:
        Assertions.assertThat(tile.getGuestsOnTile()).isEmpty()
    }

After that change tests still pass! Success! We’ve done it! 🙂
This is the final state of our work.

cropped-button Conclusion

This was only small step to make this application more readable and cleaner.
But we could see that every change can be done with confidence, if we have tests that cover changed functionality area.

If I won’t write these tests, I would not know whether my changes broke existing logic or not. I would have to check it by running whole application and clicking, clicking and clicking.

To be continued…