How can I improve my code?

[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 answered you previously in the immediate context of the question and code involved. However, in the context of that first TDD cycle and the 6 lines of code we wrote, space_at_position_0_0_has_two_neighbours is more abstract/general than getNeighboursOf_space_at_0_0_returns_2. The problem with the latter name is that it already hints at a method that we don't even have yet at this point. All we know is we are going to test some behavior of the World class, or document a design aspect of the World class. We haven't decided to create a getNeighbours(space) method yet.

The point is to try to keep implementation details from leaking out into code that should be more abstract/high-level. A test name should be as abstract/high-level as possible because it summarizes an idea -- we want to convey intent more than detail. The test code will give the details. Later on, we'll be writing unit tests that do focus on implementation details but they will have to be preceded by tests that are more abstract/general in nature. If you think about it, the "position_0_0" part of the name also leaks some implementation details so sometimes it can't be totally avoided. Also, we aren't locked in to these names. We can always change them as we learn more about what the code wants to do.

Note: "What the code wants to do." -- may sound like a strange way of thinking about what's happening. I've heard of sculptors who say things like "I let the stone tell me what's inside. My job is to chip away what's not needed." In our case, the tests will help us hear what the code wants to do. Without tests, all we hear is the cacophony of all the things we're thinking of doing. I don't know if this makes sense or if it sounds like I'm smoking something I shouldn't. Anyway, that's kind of how I look at the TDD mindset.

[Junilu Lacar]

At some point, we should also discuss the decision to throw away existing code vs. trying to refactor and salvage as much of it as we can. Throwing away a lot of code is sometimes unavoidable but it should be the last resort. In real-world programming, you usually don't have the luxury of starting over from scratch. You have to try to move forward with what you already have in place. "The only way to go fast is to go well" still applies though and TDD/Refactoring is still the best way to give you a good chance to successfully and consistently keep moving forward with a legacy codebase.

[Junilu Lacar]

N Sam wrote:Writing test code eats up valuable design / development time.

It has been proven in the Agile community by many people on many projects that in fact, writing test code is a valuable part of design / development. Testing actually saves you much more time than what you think you're saving by not doing it. Conversely, not testing eats up much more time later than what you think you're saving now.

And nobody can put a price on the amount of ANGER, FRUSTRATION, PAIN, and SUFFERING that you avoid when you have well-tested code vs. poorly tested code.

[Eduardo Bueno]

It's been already a long reading but worth every letter. Thank you Junilu and Ashley and keep up the good work.

I'm also in the urge to code this myself but as Junilu I'll try to just stick around and add or absorb whatever I can.

Junilu Lacar wrote:If you think about it, the "position_0_0" part of the name also leaks some implementation details so sometimes it can't be totally avoided. Also, we aren't locked in to these names. We can always change them as we learn more about what the code wants to do.

I've been thinking about it since Ashley posted and isn't it correct to name it space_in_corner instead of space_at_0_0? since the test can also be applied for other corner spaces as well.

[Ashley Bye]

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?

- No, probably not. I'd have just plower on with how I first thought it should be done and then scratched my head when something didn't work, go back and b*st*rdise what I had already written and made a complete mess.
- Same answer as above. Then I'd be really confused about what I'm trying to achieve with the methods and tests.
- In fact, I think all those questions can be answered with my first answer.

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

- So basically, write something and see if it works. Refactor it. See if it still makes sense. Go back a step or 2 if need be. Repeat?
- It makes it hard when you're learning alone to program. I suppose asking questions here is a good start but it just makes the going slower than if I were in a classroom (I knew I should have taken computing at university).
- Although they drive the design, you still end up having to experiment right? Only that as you gain more experience, you know what does and doesn't work in certain situations and the experimentations become more complex and larger in scale as projects increase in size?

Back to the code, I now have:

// WorldTest.java

public void corner_space_should_have_2_neighbours() {
    World world = new World(4, 7);
    world.getNeighboursOf(new Space(0, 0));

    assertThat(numberOfSpaces, is(2));
}

// World.java
public World(int width, int height) {
    this.width = width;
    this.height = height;
}

public ArrayList<Space> getNeighboursOf(Space space) {

}

Which leads me to ask - is an array list of spaces to return the results? Is there a better way I can achieve this? But I also know that I have yet to actually instantiate the world - I have a world object but no spaces in it. So I'm also asking how to I store the information about spaces in the world object? Should I store the spaces in a multidimensional array of Space[width][height]? What other ways could this be done? Can I use an array list of spaces again? Is there another collection that would be better suited? Answering these questions will need to be done before I can then look at further implementing the getNeighboursOf method, but will probably require smaller individual tests to be written to make sure the implementation up to that point is sound.

[Ashley Bye]

The point is to try to keep implementation details from leaking out into code that should be more abstract/high-level. A test name should be as abstract/high-level as possible because it summarizes an idea -- we want to convey intent more than detail. The test code will give the details. Later on, we'll be writing unit tests that do focus on implementation details but they will have to be preceded by tests that are more abstract/general in nature.

Ah, so we're not specifically writing unit tests yet, rather tests to make sure the concept works and to guide us towards a decision on how to implement the concept?

[Ashley Bye]

Junilu Lacar wrote:At some point, we should also discuss the decision to throw away existing code vs. trying to refactor and salvage as much of it as we can. Throwing away a lot of code is sometimes unavoidable but it should be the last resort.

Concur it should be a last resort. However, in the interests of getting good code and learning, I'm happy to throw away code in this project as needed, rather than trying to make the best of a bad situation. Hopefully as I get more into TDD and get better at decoupling classes this is less likely to happen.

[Junilu Lacar]

Eduardo Bueno wrote:I've been thinking about it since Ashley posted and isn't it correct to name it space_in_corner instead of space_at_0_0? since the test can also be applied for other corner spaces as well.

"correct" is too definitive. I'd go with "appropriate" instead. Your suggestion is a good one because it would inspire more conversation. In a real-time setting, we would take a minute or two to discuss whether this would lead us to having more than one assertion in the test or not. A general rule of thumb is to keep your tests focused by having only one assertion per test. I try to follow this rule of thumb most of the time but it's not uncommon for me to add a couple more assertions to support the main assertion, kind of like corollary assertions. The assertions all have to be related though. So the question would be:

Do we want to go with space_in_corner with multiple assertions (one for each corner) or do we want four tests, one test / one assertion per corner. If we go with one test / four assertions, do we want to rename to spaces_in_corners_have_2_neighbors? If we go with four tests, one test / one assertion, do we want to use space_in_southwest_corner, space_in_southeast_corner, space_in_northwest_corner, space_in_northeast_corner instead?

I hope you guys are beginning to see that a lot of value is coming from stopping to ask these questions. They really make you think about clarity of your design and code, right? Once you start thinking about making the code and design easy to understand, it somehow makes the design a lot easier to work with. That's why I said that "Good designs are flavored with lots of discussions" -- they are easier to digest the more you discuss them, learn from your discussions, and apply what you learned to the code.

[Junilu Lacar]

Ashley Bye wrote:Ah, so we're not specifically writing unit tests yet, rather tests to make sure the concept works and to guide us towards a decision on how to implement the concept?

Yes. I get a good sense that you're starting to get in line with this kind of thinking

[Junilu Lacar]

Ashley Bye wrote:Although they drive the design, you still end up having to experiment right? Only that as you gain more experience, you know what does and doesn't work in certain situations and the experimentations become more complex and larger in scale as projects increase in size?

Yes, you have to experiment a lot. Each test increment is a small experiment. Each line of code written to make a failing test pass is an experiment. Each time you refactor, you are applying what you learned from those experiments back into the code so that, as Ward Cunningham says, "the code reflects your new understanding of the system." The more TDD cycles you go through, the more experiments you end up running.

The more experiments you run, the more you learn about your system. The more you refactor, the more goodness you put back into the system. This is a "Righteous Cycle" and it goes in the opposite direction from the "Vicious Cycle" that you get caught up in when you don't do a lot of experimentation and refactoring.

[Junilu Lacar]

Ashley Bye wrote:Which leads me to ask - is an array list of spaces to return the results? Is there a better way I can achieve this? But I also know that I have yet to actually instantiate the world - I have a world object but no spaces in it. So I'm also asking how to I store the information about spaces in the world object? Should I store the spaces in a multidimensional array of Space[width][height]? What other ways could this be done? Can I use an array list of spaces again? Is there another collection that would be better suited? Answering these questions will need to be done before I can then look at further implementing the getNeighboursOf method, but will probably require smaller individual tests to be written to make sure the implementation up to that point is sound.

All great questions. Here's the answer that I was hinting at before: Let the tests guide you.

If you need to initialize the World with Spaces then write a test that fails because there is no code that initializes the World with Spaces. TDD Rule #1, right? A failing test gives you permission to write production code. Without a failing test, you're not supposed to write anything besides a little more test code (TDD Rule #2).

[Ashley Bye]

I've been thinking about how to initialise the world with spaces. It needs a few lines of code in the constructor to do so. I can either put all of the code in the constructor or write a couple of private methods to make the constructor easier to read. Do I need to write a test for each of the private methods (and how do I test they work correctly) or does testing the overall result suffice?

I'm still umming about the best way to store the spaces, either in a multidimensional array or an array list. Does each space need to know its location in the overall grid? I think it makes sense because otherwise I need to iterate through the entire array each time I want to find where the space is and compare the objects until I find a match. But I'm concerned that this makes space too dependent on the world. What if I want to change the grid system in the future? That then means I have to change the space also, so the 2 are not currently very loosely coupled. What are your thoughts?

Thoughts aside for now, this is what I have:

// WorldTest.java

@Test
public void world_is_initialised_with_a_space_in_each_grid_square() {
    World world = new World(2, 2);

    assertThat(world.getSpaceAtGrid(0,0).getClass().getSimpleName(), is("Space"));
    assertThat(world.getSpaceAtGrid(1,0).getClass().getSimpleName(), is("Space"));
    assertThat(world.getSpaceAtGrid(0,1).getClass().getSimpleName(), is("Space"));
    assertThat(world.getSpaceAtGrid(1,1).getClass().getSimpleName(), is("Space"));
}

@Test(expected=IndexOutOfBoundsException.class)
public void getSpaceAtGrid_2_2_throws_exception_for_world_size_2_by_2() {
    World world = new World(2, 2);
    world.getSpaceAtGrid(2, 2);
}

@Test
public void corner_space_should_have_2_neighbours() {
    World world = new World(3, 3);

    assertThat(world.getNeighboursOf(new Space(0, 0)).size(), is(2));
    assertThat(world.getNeighboursOf(new Space(0, 2)).size(), is(2));
    assertThat(world.getNeighboursOf(new Space(2, 0)).size(), is(2));
    assertThat(world.getNeighboursOf(new Space(2, 2)).size(), is(2));
}

@Test
public void middle_space_should_have_4_neighbours() {
    World world = new World(3, 3);

    assertThat(world.getNeighboursOf(new Space(1, 1)).size(), is(4));
}

@Test
public void side_space_should_have_3_neighbours() {
    World world = new World(3, 3);

    assertThat(world.getNeighboursOf(new Space(0, 1)).size(), is(3));
    assertThat(world.getNeighboursOf(new Space(2, 1)).size(), is(3));
    assertThat(world.getNeighboursOf(new Space(1, 0)).size(), is(3));
    assertThat(world.getNeighboursOf(new Space(1, 2)).size(), is(3));
}

// World.java

public class World {

    private Space[][] theWorld;
    private int width;
    private int height;

    /* Constructors */
    public World(int width, int height) {
        this.width = width;
        this.height = height;

        createWorldGrid();
        populateWorldGrid();
    }

    /* Getters and setters */
    /* Instance methods */
    public Space getSpaceAtGrid(int x, int y) {
        return theWorld[x][y];
    }
    
    public ArrayList<Space> getNeighboursOf(Space space) {
        int currentX = space.getXCoord();
        int currentY = space.getYCoord();
        ArrayList<Space> neighbours = new ArrayList<>();
        
        // North = (x, y+1); East = (x+1, y); South = (x, y-1); West = (x-1, y)
        if (currentY+1 < height) { neighbours.add(getSpaceAtGrid(currentX, currentY+1)); }
        if (currentX+1 < width) { neighbours.add(getSpaceAtGrid(currentX+1, currentY)); }
        if (currentY-1 >= 0) { neighbours.add(getSpaceAtGrid(currentX, currentY-1)); }
        if (currentX-1 >= 0) { neighbours.add(getSpaceAtGrid(currentX-1, currentY)); }
        
        return neighbours;
    }

    private void createWorldGrid() {
        theWorld = new Space[width][height];
    }

    private void populateWorldGrid() {
        for (int i = 0; i < width; i++) {
            for (int j = 0; j < height; j++) {
                theWorld[i][j] = new Space(i, j);
            }
        }
    }

}

Some of the tests were written after the code but were very useful as I had missed '>=0' for the lower limits. It's also good to see that the code all works. Should all of these tests have been conceived before I wrote the code? Or is it okay to write one test, implement the code, write the rest of the tests to make sure the code functions as expected with varying input?

[Ashley Bye]

I'm at the point where I need to write my menu and use it to return a space to move into. However, I'm stuck as to what test to write. All the menu does is prints some stuff to the screen, gets an input value from the user and returns it. The game logic will then determine what that equates to. Is this a good design or should the menu be returning the next space?

[Junilu Lacar]

I would be inclined to follow Model View Controller (MVC). The menu is the View and is responsible only for display and input. The input gets passed to something that can translate character input to method calls. That something can be a method at first, then as I add more translations, it might become a full-blown class. If I have some time later today, I'll try to post some code snippets that illustrate the step-by-step process I might go through as I incrementally add tests and code. This way you have something to compare with your solution path. In this medium, it's quite a challenge to keep all these small bits of conversation on the same track and focused on one goal so I'm going to try working with bigger chunks but still try to walk you through the thought process involved with all the smaller parts. Got a day job to keep though so I'll post later tonight.

[Ashley Bye]

Junilu, thanks. Yeah, I know that feeling about a day job; unfortunately my it,em off has come to an end so the frequency with which I can work on my programming has reduced significantly as I've had to go back to the real world! Seeing an example of how you do your testing would be a massive help. It might help me piece together all the little fragments I've currently got floating around in my head!

Regarding using MVC I guess I'd use some kind of collection as the data? Looking a bit further out at the wider project, would this be a good pattern to use for getting world spaces? They should be stored separately to the bulk of the application, right? How do I deal with tests that did pass but now fail because they were written with a very simple Space class in mind, and now that it has evolved and all my spaces are being created from a class called GameSpaces with their coordinates encoded as part of the construction the initial tests to make sure World had spaces in the correct place, returned the correct number of move options etc all fail. I could rewrite them to take account of my worlds size but these would then fail again if I changed the world dimensions. What's frustrating as that the code runs as expected. Is this a common problem or does it just indicate that I've not been flexible enough in my design?