How can I improve my code?

[Ashley Bye]

My code is back up on github if you want to look at it so far. Thanks for the suggestion about test method names, can I use assertThat for an equals, i.e. `assertThat((numberOfItems == 2), is(true))`? I tried it and couldn't make it work so went back to assertEquals for that. I'll have a google in a bit and see what I can find.

I see your point about the story - yes, the player does pickUp treasure but the way my code reads the space is collecting the treasure, which will be a hinderance to understanding when it comes to reading the code back in a few months time. So looking at your suggestion, player.moveInto(space) I can use a containsTreasure() method of the space in the moveInto() method of player and return a boolean. Testing the boolean, the player can pickUpTreasure but I'm not sure how to do that - I've not passed the space variable to pickUpTreasure. Is it a good idea to do it that way? Perhaps I could always call pickUpTreasure(space), test in the method if the space has a treasure and then update my inventory. Seems a bit clunky in either case.

player.moveInto(space);
if (space.containsTreasure()) {
  player.pickUpTreasure(space);
}

But then I need to do this for enemies and friends too. I should be informed by the space that it has treasure, enemies, friends, etc.

Is there a way that I am not considering that will achieve this?

Edit: I have an idea, I'll be back...

[Junilu Lacar]

We used to have a lot of methods that started with "should" but quickly recognized that it added as much value to clarity as "test" did. Now we just ask what's the intent of the test and start writing down the answer as the method name. With a bit of discussion we can usually distill it down to a few essential words. I've tried Given-When-Then but it seems a bit stilted so I hardly ever use it. Besides, well-chosen names in the conversational style usually end up implying the "Given-When-Then" parts anyway, although not always in that order.

For example, in SpaceTest.java, the test containsTreasure_is_true_when_initialized_with_weapon has the Given and Then parts implied: (given a Space, then) containsTreasure_is_true_when_initialized_with_weapon. If "initialized" doesn't work for us, we might change it to "constructed" instead. Then that change would spur a discussion on whether we want to limit it to constructor initialization or do we need a setter as well? "ABCD - Always be coding AND designing!"

We spend a fair amount of time discussing names because we want to have ones that get as close to our intent as possible.

[Junilu Lacar]

Ashley Bye wrote:can I use assertThat for an equals, i.e. `assertThat((numberOfItems == 2), is(true))`?

It's simpler than you think:

assertThat(numberOfItems, is(2));

[Junilu Lacar]

Once a player moves into a Space, he should "know" that. So, player.pickupTreasure() implies something: that the player is picking up the treasure from the current space he is occupying. This leads to having to document, with tests and code, the following "rules": A Player must always be occupying a Space while the game is in play.

Here are a few more questions I ask when reviewing code: Do you really need this parameter? Can this parameter be eliminated? Can these parameters be combined?

[Ashley Bye]

My tests for the rewritten version post deletion.

In SpaceTest:

    @Test
    public void containsTreasure_is_false_after_collectTreasure() {
        Treasure treasure = new Weapon("Sword", 20);
        Space space = new Space(treasure);
        space.collectTreasure();
        
        assertThat(space.containsTreasure(), is(false));
    }
    
    @Test
    public void containsTreasure_is_true_when_weapon_passed_to_constructor() {
        Treasure treasure = new Weapon("Sword", 20);
        Space space = new Space(treasure);
        
        assertThat(space.containsTreasure(), is(true));
    }
    
    @Test
    public void collectTreasure_has_2_weapons_when_created_with_2_weapons() {
        Treasure t1 = new Weapon("Sword1", 20);
        Treasure t2 = new Weapon("Sword2", 40);
        ArrayList<Treasure> treasures = new ArrayList<>();
        treasures.add(t1);
        treasures.add(t2);
        Space space = new Space(treasures);
        
        assertEquals(space.collectTreasure().size(), 2);
    }

And in InventoryTest:

    @Test
    public void listWeapons_has_a_count_of_2_when_2_weapons_in_the_inventory() {
        Treasure t1 = new Weapon("Sword1", 20);
        Treasure t2 = new Weapon("Sword2", 40);
        ArrayList<Treasure> treasures = new ArrayList<>();
        treasures.add(t1);
        treasures.add(t2);
        Inventory inventory = new Inventory();
        inventory.add(treasures);
        
        assertEquals(inventory.listWeapons().size(), 2);
    }

Thinking more about the player / space and treasure problem, I was thinking about making the spaces an observer of player, then whenever a player method moveInto(space) is run, the space with that ID knows to tell the player there is treasure, enemy etc. I'm not sure if this would work because the player would then need to tell the player to pick up the treasure and to fight an enemy. Also, would that mean needing to load all the spaces from the outset of the game? Not really an issue with it this size, but if it was a massive game with graphics etc, I can foresee it would have memory issues.

[Junilu Lacar]

Also, remember that these are only snippets that are meant to drive your design thinking. They're not necessarily meant to tell parts of the *actual* story but rather provide hints of how story elements might work together. The actual "gametime" story might be:

Player moves into Space. The PLAYER (the person playing the game) is informed of the options available to the Player. PLAYER chooses an option to for the next Player action - say "press T to pick up Treasure". Player is sent message to pickupTreasure: player.pickupTreasure().

BTW, "send a message to object" is another way of saying "call a method of object" -- I don't think it's in common use any more but the two mean the same thing.

[Junilu Lacar]

Rethink the thought behind collectTreasure_has_2_weapons_when_created_with_2_weapons, both the name of the test and the code in the test.

What I "hear" the code in this test saying to me when I read it, assuming this is in SpaceTest.java:

Tell Space to collect treasure. Space will give back a collection of treasures. If Space has 2 treasures in it, then the collection it gives back will include those 2 treasures

Doesn't quite make sense, does it? How about this:

If Space has two treasures and the Player occupying it is told to collect any treasures available, then the Player should end up with two treasures collected and there will be no more available treasures in the Space.

In code, I might try:

Space space = new Space();
space.add(new Weapon("Sword", 20));
space.add(new Gift("Bread", 5));

...
player.moveInto(space);
player.collectTreasures();

// assert Space has no more available treasures
// assert player now has two more Treasures in his Inventory

[Junilu Lacar]

At this point, I'd probably say something like this:

Wait a minute! Before we follow this line of thought into the deep end, let's consider backing up a bit. How about we work on World, Player, Spaces, and getting the player to move around a bit? This way, we have the most basic functionality of the game done and we could use it to help us work out details for interactions with things that are in the Spaces as the Player moves around in the World.

Wouldn't it be a great first step to see that the Player can actually move around in the World of Spaces, even if the Spaces are all empty?

[Ashley Bye]

Okay, so here are my tests now:

public class SpaceTest {
    
    public SpaceTest() {
    }

    @Test
    public void containsTreasure_is_false_after_getTreasure() {
        Space space = new Space();
        space.add(new Weapon("Sword", 20));
        space.getTreasure();
        
        assertThat(space.containsTreasure(), is(false));
    }
    
    @Test
    public void containsTreasure_is_true_when_weapon_passed_to_constructor() {
        Space space = new Space();
        space.add(new Weapon("Sword", 20));
        
        assertThat(space.containsTreasure(), is(true));
    }
    
}

public class InventoryTest {
    
    @Test
    public void listWeapons_has_a_count_of_2_when_2_weapons_in_the_inventory() {
        Treasure t1 = new Weapon("Sword1", 20);
        Treasure t2 = new Weapon("Sword2", 40);
        ArrayList<Treasure> treasures = new ArrayList<>();
        treasures.add(t1);
        treasures.add(t2);
        Inventory inventory = new Inventory();
        inventory.add(treasures);
        
        assertThat(inventory.listWeapons().size(), is(2));
    }
    
}

public class PlayerTest {
    
    @Test
    public void moveInto_space_returns_correct_space_name() {
        Space space = new Space();
        space.setDescription("Test Space");
        Player player = new Player("Ashley");
        String desc = player.moveInto(space).getDescription();
        
        assertThat(desc.equals("Test Space"), is(true));
    }
    
    @Test
    public void pickUpTreasure_adds_2_weapons_to_inventory_when_2_weapons_found_in_currentSpace() {
        Space space = new Space();
        space.add(new Weapon("Sword1", 20));
        space.add(new Weapon("Sword2", 40));
        
        Player player = new Player("Ashley");
        player.moveInto(space);

        assertThat(player.getInventory().listWeapons().size(), is(2));
    }
    
}

Yes, it would be great and I was starting to wonder if I should be adding more functionality to get a basic program together. Question: how much of a program should be written before TDD begins in ernest? According to the information I have read the first thing written should be a test, but that means writing tests for constructors, setters, etc and taking a long time to getting round to the basic functionality which is where I see TDD then starting to be beneficial.

You mentioned before that you thought there was a better way of organising my spaces in the world without each space knowing what spaces were adjacent to it and what were valid moves. I've hit a stump when it comes to figuring out how to do that. Surely a space needs to know which ones are next to it otherwise it could just load any old space? Other than an array of spaces by ID and valid moves in the space object, how would you tackle the problem?

[Junilu Lacar]

Ashley Bye wrote:I was thinking about making the spaces an observer of player, then whenever a player method moveInto(space) is run, the space with that ID knows to tell the player there is treasure, enemy etc. I'm not sure if this would work because the player would then need to tell the player to pick up the treasure and to fight an enemy.

I don't know if it would work. Best way to answer that is to write some tests and code, then read it to see if it makes sense. My gut says that having a Space "observe a Player" is not going to make a lot of sense to a reader of the code. However, making the Player aware of the current Space he's occupying seems to make sense. I can relate to that thought, the same way I can relate to the similar thought that "When I enter a room, I am aware of things that are in the room." I can't really relate to "When I enter a room, the room tells me what things are in it."

Also, would that mean needing to load all the spaces from the outset of the game? Not really an issue with it this size, but if it was a massive game with graphics etc, I can foresee it would have memory issues.

"Premature optimization is the root of all evil." -- Donald Knuth

Don't worry about those things just yet. Good designs usually make it easy to optimize if you really need to do it. Conversely, speculative optimization often leads to poor design decisions.

[Junilu Lacar]

Ashley Bye wrote:Question: how much of a program should be written before TDD begins in ernest? According to the information I have read the first thing written should be a test, but that means writing tests for constructors, setters, etc and taking a long time to getting round to the basic functionality which is where I see TDD then starting to be beneficial.

If we were programming side-by-side, we could have a World, Spaces, and Player moving around in a World of empty Spaces in 30 minutes tops. Maybe even less time than that. With tests. And we'd be doing TDD right from the very first keystroke.

You mentioned before that you thought there was a better way of organising my spaces in the world without each space knowing what spaces were adjacent to it and what were valid moves. I've hit a stump when it comes to figuring out how to do that. Surely a space needs to know which ones are next to it otherwise it could just load any old space? Other than an array of spaces by ID and valid moves in the space object, how would you tackle the problem?

I was thinking of having World be made up of a grid of spaces. A square grid can be represented with arrays and the math to figure out where one square on the grid is in relation to other squares is relatively simple and straightforward. Given the right methods and interactions between Space and World, a Space can easily figure out what other Spaces are adjacent to it. It all depends on where you end up assigning "knowledge" and responsibilities. The World knows things about the area that is occupied by Spaces. A Space "knows" that it can "ask" the World for some information. Etc.

To translate a few of these:

(conceptually) "The World knows about the area occupied by Spaces" translates to (implementation-wise) "World has a reference to a grid of Spaces. The grid is some kind of data structure, possibly nested arrays."

(conceptually) "A Space knows that it can ask the World for some information" translates to (implementation-wise) "a Space has a reference to the World that it's in and can call a method on World to get some information about adjacent Spaces."

[N Sam]

Great job Ashley. As a beginner, i am more than impressed with your first java program. I would like to go through this thread more closely, to see the feedback from the master to the student. As far as i see, the source has evolved as follows (in GIT):
1. DragonSlayer -> nbproject
2. DragonSlayer -> src
3. DragonSlayer2 -> src

Is this correct order of src ? It would be hard to associate the feedback without seeing the intermediate evolutions of the first draft.
--------------------
OOps. I just now took a closer look at DragonSlayer ->nbproject and there is only .xml and .properties files in here. Are you using Netbeans as your IDE ? (perhaps the .xml files are IDE stuff ?) Obviously the initial source is in DragonSlayer->src (perhaps with some modifications of original draft). BTW, can you describe your thinking process, when you took upon this project ? How did you translate the objective of the game and rules of game into the initial draft of code and classes ? You seem to be a very good programmer, and perhaps i can learn something from you.

[Ashley Bye]

I now have a basic level of the game setup. The way the player is controlled definitely needs some work but it gets me moving throughout the world and makes sure I can access the spaces. I now need to find a way to ask the world for what spaces are available for moving into.

Code as follows:

public class WorldCoordinate {
    public int x;
    public int y;
    
    public WorldCoordinate(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

public class World {
    
    private Space[][] theWorld;
    private LinkedHashMap<Integer, WorldCoordinate> spaceLocations;
    private int width;
    private int height;
    private int startPsn;
    
    public World(int _width, int _height) {
        this.width = _width;
        this.height = _height;
        constructWorld();
        startPsn = 24;
    }
    
    public Space getSpaceByPsn(int xPsn, int yPsn) {
        return theWorld[yPsn][xPsn];
    }
    
    public Space getSpaceById(int id) {
        WorldCoordinate psn = spaceLocations.get(Integer.valueOf(id));
        return getSpaceByPsn(psn.x, psn.y);
    }
    
    public Space getFirstSpace() {
        return getSpaceById(startPsn);
    }
    
    private void constructWorld() {
        theWorld = new Space[height][width];
        spaceLocations = new LinkedHashMap<>();
        
        int spacePsn = 0;
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                Space space = new Space(spacePsn, i, j);
                spaceLocations.put(Integer.valueOf(spacePsn), (new WorldCoordinate(j, i)));
                theWorld[i][j] = space;
                spacePsn++;
            }
        }
    }  
}

public class Space {
    
    private int id;
    private int xPsnInWorld;
    private int yPsnInWorld;
    private String description;
    private ArrayList<Treasure> treasures;
    
    public Space(int id, int xPsn, int yPsn) {
        this.id = id;
        this.xPsnInWorld = yPsn;
        this.yPsnInWorld = xPsn;
        this.treasures = new ArrayList<>();
    }
    
    public int getId() {
        return id;
    }
    
    public int getXPsn() {
        return xPsnInWorld;
    }
    
    public int getYPsn() {
        return yPsnInWorld;
    }
    
    public String getDescription() {
        return description;
    }
    
    public void setDescription(String _desc) {
        this.description = _desc;
    }
    
    public void add(Treasure _treasure) {
        treasures.add(_treasure);
    }
    
    public boolean containsTreasure() {
        return !treasures.isEmpty();
    }
    
    public ArrayList<Treasure> getTreasure() {
        ArrayList<Treasure> treasure = (ArrayList<Treasure>)treasures.clone();
        treasures.clear();
        return treasure;
    }
}

I'm not too sure how I'm going to implement asking the world for available spaces yet, but I can probably get rid of either the x, y position or the id. I've not implemented many unit tests for this, just 2 in the WorldTest:

public class WorldTest {
    
    @Test
    public void getSpaceByPsn_2_3_returns_space_id_14() {
        World world = new World(4, 7);
        
        assertThat(world.getSpaceByPsn(2, 3).getId(), is(14));
    }
    
    @Test
    public void getSpaceByID_14_returns_x_2_y_3() {
        World world = new World(4, 7);
        
        assertThat(world.getSpaceById(14).getXPsn(), is(2));
        assertThat(world.getSpaceById(14).getYPsn(), is(3));
    }
    
}

[Ashley Bye]

N Sam wrote:Great job Ashley. As a beginner, i am more than impressed with your first java program. I would like to go through this thread more closely, to see the feedback from the master to the student. As far as i see, the source has evolved as follows (in GIT):
1. DragonSlayer -> nbproject
2. DragonSlayer -> src
3. DragonSlayer2 -> src

Is this correct order of src ? It would be hard to associate the feedback without seeing the intermediate evolutions of the first draft.
--------------------
OOps. I just now took a closer look at DragonSlayer ->nbproject and there is only .xml and .properties files in here. Are you using Netbeans as your IDE ? (perhaps the .xml files are IDE stuff ?) Obviously the initial source is in DragonSlayer->src (perhaps with some modifications of original draft). BTW, can you describe your thinking process, when you took upon this project ? How did you translate the objective of the game and rules of game into the initial draft of code and classes ? You seem to be a very good programmer, and perhaps i can learn something from you.

Thank you. I decided I wanted to put a project together to help me learn all the different parts of Java that I had worked through in tutorials and books and to develop the rest of my programming skills along the way. (It turns out there is a lot more to OOP than just polymorphism, inheritance and encapsulation.) The idea spawned as a simple text game where I just created a few characters because I wanted to try and use polymorphism and inheritance. I based the storyline on a children's fairytale so the objective was fairly well set and the rules for interactions I came up with by wanting to provide more interaction with other characters than just "you killed it" or "you said hello" (and got the idea from FF7 - which I loved). Overall, I tried to emulate a simplistic version of a text game called castle I played as a kid, but with my own ideas.

I think if you can come up with a basic concept for anything and then flesh it out with a few rules it will give you something to work towards. And it's a bit more challenging and "real-life" than writing the code in tutorials.

Regarding the code on git, just look at src in either of the 2 repo's. The DragonSlayer repo is unlikely to change now.

[Junilu Lacar]

Let's look at this test some more:

    @Test
    public void containsTreasure_is_false_after_getTreasure() {
        Space space = new Space();
        space.add(new Weapon("Sword", 20));
        space.getTreasure();
        
        assertThat(space.containsTreasure(), is(false));
    }

The test method name is perfect. However, space.getTreasure() still doesn't read very well to me. There's no symmetry with space.add(Treasure). A "get" method is usually paired with a "set" method and there are very well-known and accepted semantics associated with these. "add" and "get" don't naturally go together and it's better to not mix terms to avoid confusing the reader.

This is more symmetrical:

    @Test
    public void containsTreasure_is_false_after_getTreasure() {
        Space space = new Space();
        space.addTreasure(new Weapon("Sword", 20));
        space.removeTreasure();
        
        assertThat(space.containsTreasure(), is(false));
    }

"add" is usually paired with "remove". List, for example, has add() and remove() methods. I would take advantage of the semantics generally associated with these names.

I try to avoid making design decisions in anticipation of some future requirement, but in this case I qualified the names with "Treasure" thinking that it will differentiate these later on when we start adding things like Barrier and Character to a Space.

SIDE NOTE: I'm fighting the urge to start programming this myself. I'm not going to do that though so I can avoid tainting the discussion with any biases I may have coming from code I've already written. I have not tried any of these ideas out in code; I'll leave that work to you so you can learn more. These code suggestions are all just off the top of my head and based on what we have discussed so far. This will make our discussion go pretty much the same way a real-time, side-by-side programming episode would go

[Junilu Lacar]

Ashley Bye wrote:I now have a basic level of the game setup. The way the player is controlled definitely needs some work but it gets me moving throughout the world and makes sure I can access the spaces. I now need to find a way to ask the world for what spaces are available for moving into.

Code as follows:
...

(spit-take) ... ... I was kind of hoping we could do that TDD style. That's way too much code than I would write with just 2 tests to back it up. Plus, the tests are telling me that there's way too much information about implementation details getting revealed. That getId() method screams "Noooo!"

Mind if I back you up to scratch again?

[Ashley Bye]

Go ahead. I'm still trying to get my head around this TDD stuff, it doesn't feel natural yet!

[Ashley Bye]

So I've done some refactoring. The World is much neater now and relies only on WorldCoordinate for getting and setting any position information. The grid is still a multidimensional array of type Space. Space only has reference to its position by way of a WorldCoordinate.

I've written the following test which fails so that I can now work on a method to get the moves available for a corner position. It should be trivial to then make sure this works for a position with 4 moves.

@Test
    public void getAdjacentSpaces_returns_0_5_and_1_6_for_space_0_6() {
        World world = new World();
        ArrayList<WorldCoordinate> moves = world.getAdjacentSpaces(new WorldCoordinate(0, 6));
        
        assertThat(moves.get(0).getX(), is(0));
        assertThat(moves.get(0).getY(), is(5));
        assertThat(moves.get(1).getX(), is(1));
        assertThat(moves.get(1).getY(), is(6));
    }

[Junilu Lacar]

I'm going to back you up even more. There's nothing that tells me you need WorldCoordinate. In fact, I think that's over-engineering it a bit.

[Ashley Bye]

Okay. So just go with the below?

world.getAdjacentSpaces(int currentX, int currentY)

Although, I used a WorldCoordinate to make it easier to pass the values as a pair.

Also, the name getAdjacentSpaces bothers me a bit but I can't think of a better way to phrase it. It sounds like space should be making this call...

[Junilu Lacar]

Looking back at that list of Goals (remember, this serves as your Guide Map -- always refer back to this list as you go), I didn't see much about World. So let me suggest this narrative about WORLD:

The DragonSlayer WORLD is a grid of SPACES. The grid is rectangular, with a fixed width and fixed height that can be configured at the start of the game. One SPACE occupies one unit of height and one unit of width on the grid. Each SPACE on the grid can be referenced by its coordinates, starting from (0, 0) being at the Southwest (bottom left) corner up to (WorldWidth - 1, WorldHeight - 1) being at the Northeast (top right) corner of the grid

Candidate tests:

@Test southWest_corner_coordinates
@Test northEast_corner_coordinates
@Test northWest_corner_coordinates
@Test southEast_corner_coordinates

These tests show/document how the World coordinate system works.

[Junilu Lacar]

How about getNeighbors(space)? This returns a list of Spaces that are neighbors with the given space. Neighboring spaces are those on the main directions (North, South, East, West). There may be no neighbors in some directions, depending on the given space's position on the world grid. For example space(0, 0) will only have two neighbors: one to the North and one to the East. space(1,1) will have four neighbors, assuming a World that is at least 3x3 in size.

[Junilu Lacar]

More candidate tests for World:

@Test corner_spaces_have_two_neighbors
@Test non_corner_edge_spaces_have_three_neighbors
@Test other_spaces_have_four_neighbors

These help understand the concept of "neighboring spaces"

[Junilu Lacar]

Consider the difference between the two designs below. Which one is more abstract and which one hints at implementation details?

     // design 1
     List<Space> neighbors = world.getNeighbors(space);

     // design 2
    ArrayList<Space> neighbors = world.getNeighbors(1, 4);

[Ashley Bye]

I thought about testing the corners and for certain things at a grid within the world but couldn't work out how to do it with tests that would be valid once I started properly creating spaces. I did think about inserting a N, E, S, W or C (for central) into each spaces description for first its x then y coord and testing that the four corners came out NE, SE, SW and NW.

I've just been looking at the logic for (x+1, y), (x-1, y), (x, y+1) and (x, y-1) and how to determine if I can actually go in all those directions based on the height and width of the world. My thoughts are also going through the need to know if the space is N, E, S or W of the current space for the movement options. I think that can be computed by the menu on the fly based on the current coords and the returned spaces coords. Anyway, one step at a time.

[Junilu Lacar]

Consider this as well: a POSITION has information about grid coordinates. POSITION can be moved in these directions: North, South, East, West. Only the WORLD has some intimate knowledge about POSITION in that it has an X and Y coordinate. All other objects only know to move POSITION in any of the cardinal directions (N-S-E-W). If a POSITION is successfully changed after sending it the "move" message, it will respond with TRUE. If the move was not successful, it will respond with FALSE.

Edit: We will probably circle back to this one after discussing more design principles.

[Junilu Lacar]

Yes, one step at a time and each step must start by writing a failing test. Red-Green-Refactor, baby!

[Ashley Bye]

Cool - I have 3 tests for the World grid:

@Test
public void findSpaces_returns_2_for_space_at_0_0() {
    World world = new World();
    Space space = new Space(0, 0);
    ArrayList<Space> moves = world.findSpaces(space);

    assertThat(moves.size(), is(2));
}

@Test
public void findSpaces_returns_4_for_space_at_2_2() {
    World world = new World();
    Space space = new Space(2, 2);
    ArrayList<Space> moves = world.findSpaces(space);

    assertThat(moves.size(), is(4));
}

@Test
public void findSpaces_returns_0_1_and_1_0_for_space_at_0_0() {
    World world = new World();
    Space space = new Space(0, 0);
    ArrayList<Space> moves = world.findSpaces(space);

    assertThat(moves.get(0).getXCoord(), is(0));
    assertThat(moves.get(0).getYCoord(), is(1));
    assertThat(moves.get(1).getXCoord(), is(1));
    assertThat(moves.get(1).getYCoord(), is(0));
}

Edit: Thought it best to check the other extreme too...

@Test
public void findSpaces_returns_3_5_and_2_6_for_space_at_3_6() {
    World world = new World();
    Space space = new Space(3, 6);
    ArrayList<Space> moves = world.findSpaces(space);

    assertThat(moves.get(0).getXCoord(), is(3));
    assertThat(moves.get(0).getYCoord(), is(5));
    assertThat(moves.get(1).getXCoord(), is(2));
    assertThat(moves.get(1).getYCoord(), is(6));
}

All are now passing, with the following implementation:

public ArrayList<Space> findSpaces(Space space) {
    int x = space.getXCoord();
    int y = space.getYCoord();
    ArrayList<Space> spaces = new ArrayList<>();

    // N(x, y+1), E(x+1, y), S(x, y-1), W(x-1, y)
    if (y + 1 < height) {
        spaces.add(getSpaceByPsn(x, y + 1));
    }
    if (x + 1 < width) {
        spaces.add(getSpaceByPsn(x + 1, y));
    }
    if (y - 1 >= 0) {
        spaces.add(getSpaceByPsn(x, y - 1));
    }
    if (x - 1 >= 0) {
        spaces.add(getSpaceByPsn(x - 1, y));
    }

    return spaces;
}

[N Sam]

Ashley - what book(s) did you read to learn Java ? I am about 60% done with the tutorial on oracle site, and am preparing for the OCA exam. Have written tiny programs to test on Netbeans - Netbeans itself seems to have a wide array of choices, but i haven't spent time learning anything other than a simple 'java application' on Netbeans. It is nice to see you both discuss code in detail. Thanks for the thread.

[Ashley Bye]

Most of what I know comes from Head First Java 2nd Edition. It's a great book, although I've probably forgotten as much as I've learned from it!

[Junilu Lacar]

I'm going to back you up again. TDD requires us to write very very small increments of test and code. The reason is that each small increment of code has to be preceded by a small increment of test. Each increment involves quite a bit of design ... you never just write a test. There HAS to be a lot of design thinking and discussion. And quite a bit of retrospecting about the last small decision you made. When you skip the retrospecting and discussing, you lose the opportunity to see other alternatives. That's why TDD is best done with a partner. It requires a lot more discipline but when done properly and effectively it is a very productive and rewarding way to code.

[N Sam]

But in a real world situation, under time pressure for development/release of a product, usually there is no time to do small increments of development. Writing test code eats up valuable design / development time. .....my 2 cents.

[Junilu Lacar]

You pay now or you pay later. What good is it to rush buggy code out the door? It's unprofessional and you spend more time later trying to duct tape, shim, or work around the mistakes that have piled up because you kept moving forward with a poorly thought-out design. This "fear" of getting slowed down by testing has been proven to be the source of many problems in projects. Look at healthcare.gov as a prime example.

"The best way to go fast is to always go well." -- Uncle Bob Martin

[Junilu Lacar]

My version of uncle bob's quote is: Crappy code and crappy designs always come back to bite you in the *ss. Always flush away the crap, every chance you get.

[Ashley Bye]

I agree with you Junilu regarding the importance of good design and spending more time testing and getting it right first time than getting egg on your face and needing to re-write (probably at your own / companies expense in the real world).

Hmm... okay. It's hard to write very small snippets of test code when a method is quite long. What do you have in mind?

[Junilu Lacar]

I know it may seem hard to do but you've already done it a little bit. You just kept going without stopping to retrospect. Let's start with the first test that you wrote. It's as good as any to start.

@Test
public void findSpaces_returns_2_for_space_at_0_0() {  
    World world = new World();  
    Space space = new Space(0, 0);  
    ArrayList<Space> moves = world.findSpaces(space);  
  
    assertThat(moves.size(), is(2));  
} 

(Reference: http://butunclebob.com/ArticleS.UncleBob.TheThreeRulesOfTdd)

First rule of TDD: You may not write any production code unless it is to make a failing test pass. (A compilation error is a failure)

Ideally, you would have written this:

@Test
public void findSpaces_returns_2_for_space_at_0_0() {  
    World world = new World();  
} 

and stopped with that one line of test code. Then compiled your code. If you did not have a World class written yet, the compiler would complain. First rule of TDD now allows us to write production code because we have a test that's failing (compiler error).

If you were using a smart IDE, then it's a "quick fix" to create a skeleton implementation of World. One keystroke and the IDE creates World.java and provides an empty constructor. Compiler error is fixed. Run the test. It now passes.

Yes, it seems silly but there's a reason for doing this: it's a way to break you out of the habit of just writing code without testing first. Bad habits are painful to break. This is maximizing the pain. Max pain for max gain.

[Junilu Lacar]

With three lines of test code and probably about as much of production code, it seems silly to stop and think about the design. How can you mess up with so few lines of code?

Well, let's see... findSpaces_returns_2_for_space_at_0_0() ... does that name clearly say what the intent of the test is? Or will it be a bit of a head scratcher to someone else coming in for the first time, making it necessary for them to dig in a little deeper to figure out the meaning of the test? Is there a way we can make it clearer?

Good names are an often overlooked part of design. A lot of programmers don't spend much time trying to find good names because they are trying to get the more "important" aspects of the code done. But actually, good names are a *very* important aspect of coding and designing. They're so important that Uncle Bob devotes the entire Chapter 2 of his book "Clean Code" just talking about good, meaningful names. Without good names, your code can mislead the reader. Code with bad names can even mislead YOU, the original author. Bad names can block or derail the cognitive process. They make your code stressful to read.

Have you ever heard of the Stroop Effect? That's what bad names do to your cognitive ability. If your cognitive ability is impaired, you have a tendency to make mistakes. Bugs happen. New mistakes are made on top of old mistakes and your ability to understand the code suffers more and more. After a while, it gets so bad, you are forced to throw the code away and start over. And thus begins another vicious cycle. The only way to go fast is to always go well.

So always spend time reading and re-reading what you've written to make sure it says exactly what you wanted to say. Even if you've only written three simple lines of code.

Back to the test name. Would space_at_0_0_has_two_neighbors be a better name? Does that explain our intent or design more clearly? Remember, we don't even have findSpaces() method at this point yet. By making the name a little more general in nature, we've just broken free of a preconceived idea and have more leeway to think of other alternatives to findSpaces, which frankly, is also a name that should be discussed.

Again, if we were sitting side-by-side, this would have been a three to five minute discussion at most. If my suggestion didn't get a warm reception, we'd toss a few more suggestions back and forth until we can settle on one we could both accept as "good enough for now".

[Junilu Lacar]

So here's the code that we would have after this first small increment:

// WorldTest.java
...
@Test
public void space_0_0_has_two_neighbors() {
    World world = new World(3, 4);
}

// World.java
...
public class World {
    public World(int x, int y) {}
}

Yes, that's all we needed to complete our first TDD cycle.

Are we're good with the name of the test, which is meant to say that we want the space at coordinates (0, 0) on a world to have only two neighbors (one to the north, one to the east)? If not, we can discuss a little bit more before we write another line of test code to make it fail again. This would be the start of the next TDD cycle. In real time, this would have taken two minutes at most.

Edited: the World constructor was missing the parameters before.

I'd like to hear some questions from you at this point. Any comment/question about the World constructor we have right now?

[Ashley Bye]

Regarding the name of the test, I think it's clear what it does. But... having taken on board your point, I suspect that if I put the code down for a week and came back to it I would have trouble remembering exactly what the test does. I like the suggestion of space_at_position_0_0_has_2_neighbours() and think it will be much clearer in the long term. I also think findSpaces could probably be renamed getNeighboursOfSpace(Space space), which means the test name would be getNeighboursOfSpace_returns_2_for_space_at_position_0_0(). What do you think?

I have heard of the Stroop Test but only about colours and the written colour names. I hand't considered how it could be transferred across to programming and probably many other areas of life too. I will definitely strive harder to come up with good names that actually mean what I want the program to do.

Regarding the World constructor we have in the above post, what are the values of int x and in y supposed to do? What is the constructor going to do? Where are we going to get the spaces from? Do we have a data layer that the World constructor needs to access to get this information? How will it know what spaces to put in the world and where will it know to put them?

[Junilu Lacar]

Ashley Bye wrote:I also think findSpaces could probably be renamed getNeighboursOfSpace(Space space), which means the test name would be getNeighboursOfSpace_returns_2_for_space_at_position_0_0(). What do you think?

I think it's good! To make it even more concise, how about we get rid of the "stutter" (read it out loud and you'll hear it) by making it just getNeighboursOf(space)? This is a common naming style, BTW, allowing the statement started by a well-chosen method name to be completed by a well-chosen parameter name. If you read the code out loud, it almost sounds like plain English: "World, get neighbors of space." Notice that the "." between the object and the message being sent to is interpreted as a comma.

I have heard of the Stroop Test but only about colours and the written colour names. I hand't considered how it could be transferred across to programming and probably many other areas of life too. I will definitely strive harder to come up with good names that actually mean what I want the program to do.

Awesome!

Regarding the World constructor we have in the above post, what are the values of int x and in y supposed to do? What is the constructor going to do? Where are we going to get the spaces from? Do we have a data layer that the World constructor needs to access to get this information? How will it know what spaces to put in the world and where will it know to put them?

These are all great questions but I'll hint at my answer with a few more questions:

- Would you have asked these questions if we hadn't stopped to retrospect after writing 6 lines of code?
- Would you have stopped to refactor those test names before proceeding?
- Would you have used your tests to inspire more questions to be asked about your design?
- Or would you have used tests to prove that what you had decided to do was working?

Consider these propositions:
- The code is the design
- Good designs are discovered by experimentation and iteration
- Good designs are flavored with many conversations
- Unit tests are most effective when you use them to drive your design.