How can I improve my code?

[Ashley Bye]

I am trying to learn Java and have put a small text based game together. The source is uploaded to github (see link below) and I'd appreciate it if members could provide comments here on how I could improve the code so that it is neater, more efficient, where I could refactor, etc. There is one bug I've found that I can't fix - the menu options are written to the terminal in reverse order.

As I'm just getting started with the forum I'd appreciate it if a moderator could point me in the right direction for posting this type of request.

Thanks for your help.

https://github.com/basicdesigns/DragonSlayer

[Vivek SharmaJi]

For code quality, you can download PMD, checkstyle, findbug code review tools in your eclipse.

[Ashley Bye]

Thanks Vivek. Are there similar tools for NetBeans?

[Junilu Lacar]

Quite impressive for a "beginner" actually. I take it you have some previous programming experience? Your methods could definitely use a good helping of breaking down into smaller chunks of work by "Extract Method" refactoring. Since you are a bit advanced, I also suggest that you start writing unit tests to make refactoring safer and help drive further changes. I can help you through if you don't mind making this a very long discussion (there's a lot you can do to make your code better and more object oriented)

[Junilu Lacar]

We could also have a good discussion on some of your design decisions such as those you made for Character and its children.

[Vivek SharmaJi]

Ashley, it seems findbugs works with NetBeans

https://netbeans.org/kb/docs/java/code-inspect.html

[Matthew Brown]

I'd echo Junilu - that's a pretty good starting point to build upon. Here are a couple of concrete suggestions to start.

Firstly, the error you're seeing with the order in Game.menu()? You've got the options stored in a HashMap. A HashMap doesn't have a defined order - in particular, you can't expect it to iterate in the same order that you added items. So that is likely to be the cause (if they are coming out in reverse that's probably a coincidence - they could be any order). There is, however, another class - LinkedHashMap - that behaves exactly like a HashMap except that it does maintain order. So you could see if that helps.

Now, let's look at this bit of code that checks to see if the player picked up a diamond:

boolean hasDiamond = false;
ListIterator<Gift> iterator = inventory.getGifts().listIterator();
while (iterator.hasNext()) {
    Gift gift = iterator.next();
    if (gift.getName().equals("Diamond")) {
        hasDiamond = true;
    }
}

Firstly, that could be made more readable by using the extended for loop (the same applies for most other cases where you are iterating a collection):

boolean hasDiamond = false;
for(Gift gift: inventory.getGifts()) {
    if (gift.getName().equals("Diamond")) {
        hasDiamond = true;
    }
}

But we can improve it further in terms of the design. Here's a question: which object is best placed to work out whether an Inventory contains a particular item? Answer: the Inventory itself. It's a design concept known as an "information expert" - it's usually better to give responsibility for something to a class that already has the information needed to do it.

To apply that in this case, you can move the code into a method in the Inventory class. Then this bit of code here, in the Game class, is reduced to:

boolean hasDiamond = inventory.hasGift("Diamond");

See how that makes things clearer?

[Ashley Bye]

Thank you all for your comments. I am beginning to learn Java yes, however I do have some experience programming in the past, mostly some college level bits in Pascal and VB 10 year ago and I've played around on and off with php frameworks (and trying to figure out unsuccessfully how they work). I've taken up the Java mantle because I like the enterprise nature of the language and want to challenge myself to really learn it and how to actually program in depth rather than just how the language works.

Vivek, I found findbugs for the latest version of NetBeans but they don't have a plugin available for the latest version. With PMD it tells me in the InputHelper.class that the method should only have one exit point an that should be the last line in the method. If it is a try/catch and I want to return either the input or the error, why is it bad to do it with I return in both the try and catch blocks?

Matthew, thanks. So what I should actually be doing is making the classes that hold the information responsible for the logic that decides if it has a property or not? Is property the right word for a Diamond? I didn't know that about the HashMap, that would explain a lot. I'll give LinkedHashMap a try.

Junilu, I'd be more than willing to get into a discussion about how to make the code better and more OO. What is extract Method refactoring (I'll also have a google). Where is a good place to learn about writing unit tests? Would that be using JUnit?

[Junilu Lacar]

I have used Pascal and VB in my pre-Java life, too.

Yes, when it comes to Java, JUnit is probably the most popular choice for an automated unit testing framework. There are many online sources for learning about unit tests. However, since you are not exactly a beginner, I suggest you learn about unit tests in the context of doing Test-Driven Development.

The "Extract Method" refactoring involves taking a block of code and making it a separate method. The goal is to make the code more concise and readable, hiding implementation, and revealing intent. Your code already has a lot of flags that hint at opportunities to refactor. In particular, wherever you have written a comment that tries to explain what a block of code does, that's a flag telling me that there's an opportunity to "Extract Method" so that the comment can be replaced with a self-documenting method call.

[Junilu Lacar]

Ashley Bye wrote:I'd be more than willing to get into a discussion about how to make the code better and more OO.

So the first design decision I'd like to explore is the one about creating a hierarchy from Character. Even though it's a small hierarchy, it still involves the same design principles and considerations you'd factor into creating larger hierarchies of classes. Joshua Bloch's book "Effective Java" explains the following:
1. Favor composition over inheritance
2. Prefer interfaces to abstract classes

Bloch also recommends that you consciously design for inheritance if you use it and document the effects of extending a hierarchy and overriding methods. These points are explained at length in the book so I won't try to go into details here.

One thing I like to do is use unit tests as a way to document important aspects of the design and drive design thinking. I hit a few birds with one stone by doing this. The first thing I would do here to try to improve the code is write unit tests that help document the Character hierarchy and the inheritance relationship with Enemy and Companion.

In trying to think of tests to write, I'd ask the following questions:
1. What situations would require me to program to the type Character, with the possibility of dealing with Enemy and/or Companion instances?
2. What situations would require me to program to the type Enemy in particular instead of Character in general?
3. What situations would require me to program to the type Companion in particular instead of Character in general?

[Ashley Bye]

I've been through the code and refactored as best as I can. Does this look better than it was before?

Times when I would want to program directly to character are:
If companions can travel the world with the player, they can get involved in fights too. This would mean having a method to determine if they can fight. Also a method to make them fight (although this would/could be overridden). If I add a health field to character, then there would need to be methods to asses both companion and enemy health levels.

Programming directly to companion:
No reason in the program at the moment. Perhaps if they can give gifts or travel the world with me there would need to be specific methods to drive the interaction.

Programming directly to enemy:
When fighting the enemy.
Determining health level.
Determining what weapon must make the killing blow.

[Junilu Lacar]

I looked at your changes and there's still room to improve clarity and coherence of the code.

But let's focus on understanding the Character hierarchy and the intent of creating one in the first place. When I have doubts about the necessity of creating a class hierarchy, I find it helpful to write unit test code to try out various usage scenarios and actually see how a hierarchy can help me organize data and behavior. The unit tests help me see if I really need a hierarchy or if I can go with some other type of relationship. Also, I usually don't start with a hierarchy. I start with separate classes. When I see duplication, refactoring to eliminate duplication can sometimes lead to the creation of a class hierarchy. It all depends on what the code and tests tell me.

How comfortable are you with JUnit? I'd like to start documenting the intent and design of the Character hierarchy in the form of unit tests. This will allow us to put the discussion in a more concrete context.

[Ashley Bye]

That sounds slightly illogical - possibly write 2 or more classes and then see if there is duplicate code. Although I suppose once you've written one, if you then start another and notice the duplication it's not much of a change to refactor and introduce a superclass. Going back to an earlier comment, why are interfaces better than abstract classes? I'll have to look up the book you mentioned.

I've not used JUnit before - if you can point me in the direction of a good tutorial I'd like to start using it.

[Junilu Lacar]

I'm going to move this thread to the OO and Refactoring forum since this discussion is going to be out of place in Java Beginner

[Junilu Lacar]

Test-Driven Development is *very* counterintuitive but once people get a hang of it, they never want to go back to doing "up front" design and coding. One manager I described this approach to called it "Bass ackwards". This was back in the early 2000s when TDD was not very well-known.

There's a great book "Growing Object Oriented Software Guided by Tests" by Nat Pryce and Steve Freeman that walks you through the process of doing TDD.

The thing about this approach is that it frees you from having to think of *all* the details up front. You start out with a general idea, then use tests and working code to guide you through the process of incrementally refining that idea and adding details. Writing tests first forces you to write testable code and testable code tends to have a better structure than untestable or difficult-to-test code. To borrow from Star Wars: "Use the tests, Ashley. Let the tests guide you."

[Junilu Lacar]

Ashley Bye wrote:I've not used JUnit before - if you can point me in the direction of a good tutorial I'd like to start using it.

Google knows: JUnit tutorial

[Junilu Lacar]

Ashley Bye wrote:Going back to an earlier comment, why are interfaces better than abstract classes? I'll have to look up the book you mentioned.

I don't think preferring interfaces to abstract classes necessarily equates to "interfaces are better than abstract classes." It just reinforces the advice to be very conscious about your motivations for creating a class hierarchy and the consequences of doing so. Class hierarchies are rigid whereas interfaces provide more flexibility. If you are still exploring and fleshing out a design, interfaces are more forgiving and amenable to refactoring than class hierarchies. However, there are cases where a hierarchy is more appropriate. So you just need to know the pros and cons of deciding to go one way or the other. Hopefully, it will become clearer to you as we go through this discussion.

And yes, definitely have a look at Bloch's book. It should be on every Java programmer's bookshelf.

[Junilu Lacar]

Once you have an idea of how to use JUnit, let's try to answer these questions: What tests can I write to show the different ways a Character can be used? What tests can show the different ways an Enemy can be used. What tests can show the different ways a Companion can be used?

[Junilu Lacar]

Just in case you're wondering if this is more of an academic exercise or something that directly relates to improving your code, it's more of the latter. It's kind of a roundabout way of getting you to consider what kind of refactoring you can do to improve the section of code in the Game class that is prefaced by the comment: // Check for characters (line 57)

[Ajitesh Shukla]

Did you check "Single Responsibility Principle" from SOLID set of principles? In brief it says that a class should have just one reason to change. In another words, a class should perform just one functionality OR do just one thing and do it well. The same could as well be applied to method. A method should do just one thing and do it well.

[Ashley Bye]

Okay, I think I have a reasonable grasp of basic JUnit usage now. To answer your questions in relation to the current functionality of my code:

What tests show the different ways a character can be used?
None - All the functionality is contained in the subclasses of Enemy and Companion.

What tests show the different ways an enemy can be used?
Test that the interaction returns the correct string.
Test the fight method (correct weapon kills enemy, incorrect weapon enemy remains alive, enemy remains alive until health is 0).
Looking at the rest of the code in the fight() method dealing with if it was a dragon, I should just test if the enemy killed was a dragon then deal with the outcome in one of the calling classes, probably game.

What tests show the different ways a companion can be used?
Test that the companion interaction says "Hello".

Thinking about refactoring for Game class on line 57, both Enemy and Companion have an inherited interact() method and I think I should move the implementation for that to the Character class. I can then either continue to override that in Enemy, making sure I call the super method too or I can have the fight and interact methods completely separate. The benefit of keeping them separate would be that I can do something along these lines:

// Check for characters
if (there is a character in the game space) {
    assign character to variable // makes code easier to read
    Output notice to screen informing of character
    Call Character superclass interact method
    if (character class is enemy) {
        fight enemy
        remove enemy from space once defeated
        if (all enemies killed) {
            end game
        }
    }
}

I also think it would make the code more readable if I re-wrote the play game method to check there were no more moves to make before showing the menu rather than just returning once all enemies are killed. And the Inventory class should be dealing with determining where to store Treasures, rather than do it in the Game class.

Before I go ahead and make any changes to the code, does this mirror what you're thinning or should I be thinking about it in some other way?

[Junilu Lacar]

Ashley Bye wrote:Before I go ahead and make any changes to the code, does this mirror what you're thinning or should I be thinking about it in some other way?

No, it does not. That's ok though because it'll make the difference that much more revealing.

Let's talk design principles for a minute. Ajitesh mentioned SOLID design principles above. This is a mnemonic that Robert "Uncle Bob" Martin came up with for these design principles:

S - Single Responsibility Principle (SRP)
O - Open-Closed Principle (OCP)
L - Liskov Substitution Principle (LSP)
I - Interface Segregation Principle (ISP)
D - Dependency Inversion Principle (DIP)

Of these, the most pertinent to the current discussion are SRP, OCP, and LSP. I suggest you fire up Google and read some articles on these first. To give you a hint of what I have in mind though, here are a couple of things to think about:

To demonstrate adherence to the LSP, what test can you write that would be valid for the behavior of any Character, regardless of its subtype? If you can't write polymorphic code against Character that works regardless of the specific subtype, then the decision to create a hierarchy becomes more questionable because you are probably violating LSP and probably OCP as well.

Also, when you have to check what the specific class of an object is (line 6 in your pseudocode), it's a red flag or what we call a "code smell". Checking for a specific subtype when you have a reference to a supertype is a symptom of improper or lack of use of polymorphism. In other words, it's a code smell that hints at a violation of LSP.

[Junilu Lacar]

Ashley Bye wrote:What tests show the different ways an enemy can be used?
Test that the interaction returns the correct string.
Test the fight method (correct weapon kills enemy, incorrect weapon enemy remains alive, enemy remains alive until health is 0).
Looking at the rest of the code in the fight() method dealing with if it was a dragon, I should just test if the enemy killed was a dragon then deal with the outcome in one of the calling classes, probably game.

Let's keep this on the back burner for now but this can lead to some interesting discussions around OCP and code modification vs. extension vs. configuration. More on this after we tackle the Character hierarchy questions.

[Junilu Lacar]

Another one for the backburner: WorldMap.createWorld() is a very long method. There is a lot of duplication. IMO, you could manage the possible moves from any given space differently and more effectively by using a different data structure and calculating/evaluating instead of specifying the possible moves.

[Ashley Bye]

A quick thought before I take up the reigns again tomorrow. I thought about changing the design so that the Player is the object doing the interacting (it might be worthwhile implementing this later on if I want to extend functionality, but I'll leave it for now) but actually, Character should do the interacting in this instance.

Character needs to be an abstract class because I don't want it instantiated. It needs to declare a reference to a CharacterInteraction interface which will be implemented in both Enemy and Friend (I think a class rename makes the code more readable). I can then move the fight() method into the CharacterInteraction interact() method of the enemy. I might even want to take this one step further and have classes for types of enemy and friend because at the moment my initialisation repeats itself when declaring them in different spaces.

The problem I have is that enemies have health, friends don't. So I could test whether Character has health remaining = true or false. This would pass for enemy but not friend, which leads me to think Character is not a suitable superclass for both.

I then run into a stumbling block in that I don't know how I would store them in the space if they don't both have a superclass of Character because otherwise I'm creating a lot of variables with null in them, and performing an extra check in the space (hasFriend? hasEnemy?). Whereas, if they are both Character super type, I can do character.interaction.interact(). I could also extend the enemy classes to have a fight method, but I can't call character.interaction.fight() because it's not a method the friend would have.

I also need to access the Players inventory when interacting with an enemy but not a friend (at this point - friends may give gifts in the future), so would need to pass the inventory to the enemy method with interact(Inventory inventory).

So I think I have come to the conclusion that a Character super type is not the correct way to build my class hierarchy, but it leaves me with more questions that I don't have an answer to yet.

[Junilu Lacar]

This is great! You're starting to think about responsibilities and who/what should carry out that responsibility. This is key to good object-oriented design and programming.

But you're still getting bogged down in the details of implementation. Pull yourself up from that level and go a little more abstract for now. There is going to be plenty of opportunity to fiddle around with specifics but that time will be when we actually write tests and "production" code, and then do refactoring. Right now we want to get a high-level conceptual model in our heads or on a whiteboard or a piece of paper. We need that to give us a general sense of purpose and direction.

I've always said that writing software is a lot like story telling (hence, the @author tag in JavaDocs) so let's try to tell the "story" this way first:

The Dragonslayer - a text-based, single player, role playing game (RPG).

The game starts with the Player occupying the first Space of the World/Map/Board (for brevity, let's go with "World" for now). The World is made up of a number of contiguous Spaces. While the game is in play, the Player can move from the Space he currently occupies to another adjacent Space. Sometimes, it may not be possible to move to an adjacent Space because there is something (what do we call this?) that blocks entry from that direction. A Space can be occupied by the Player and one (or two? more than two?) other "Character". The other Character can be friendly (a Companion) or unfriendly (a Foe). If a Companion occupies the Space, (what happens?). If a Foe occupies the Space, the Player can engage in a battle to the death or evade the Foe by moving out of the Space. Avoiding a battle with a Foe is not guaranteed, however, since once the Foe "sees" the Player move into the Space, it may or may not choose to chase the Player. How long a Foe chases the Player depends on the Foe's inherent tracking ability and speed. If Foe has "excellent" tracking ability and can match the Player's speed, the Foe can catch up to the player and immediately engage the Player in battle. ... and so on...

I hope this makes sense. Anyway, the idea is to have a high-level but fairly detailed description of What kind of things happen but is relatively vague and free from preconceptions of How things happen, i.e. the implementation details. When we drop right down to the How of things, we narrow our field of vision and we start limiting our choices for solutions. This way, we have a "Square One" that we can always go back to if we find that a series of "How" decisions has brought us to a dead end. I like to think of this part of the process as equivalent to the "story boarding" process that movie makers go through.

Feel free to expand, contract, or modify the story as I have presented it above. You are, after all, the story teller, not me. Once you have something that you're happy with, we can start dissecting it and mapping out some possible solution paths. Then we can see if any of those paths match your current solution. Or perhaps we can look at your current solution and see how well it aligns with the "What" version of your story.

[Junilu Lacar]

I know that this may seem a bit over-the-top given that all you were really looking for was some advice on how to improve your code. However, this is the thought process I go through when I am faced with a non-trivial refactoring challenge. As I said before, this way lets me get my bearings and have a better sense of purpose and direction for my refactoring efforts. Refactoring is always localized to a small area but it should also be guided by a larger, more holistic vision of the improved software ecosystem. I like to see the forest for the trees before I start clearing out the underbrush. I find it gives me the best overall results.

[Junilu Lacar]

This just occured to me. There is a free tool, GreenFoot, that would be a perfect next step for this program. It will give you more ideas of how to write object-oriented, event-driven Java programs. Let's keep that as a TODO though.

[Ashley Bye]

Okay, so here is my story:

DragonSlayer - A single player, text based, RPG. The object if the game is to control a player on a quest through a magical world to slay the Dragon and rescue the Princess.

The game starts with the Player in his village, the first space of a World. The World is made of a grid of spaces (in a maze like structure) next to each other, and the Player must navigate from one to another on his quest. Not all spaces lead to the next in the sequence due to being blocked by a Barrier (such as a river, lake or impassable mountains, etc.).

The Player can only be in one Space at a time. On the way through the world, the Player meets other Characters who are either a Friend or an Enemy. Friends can give the Player gifts, food and medicine (but may just wish him well on his quest), which the Player stores in an inventory for ease of carrying. Once a Friend has given the Player a gift they will not be able to give them anything further. Friends can talk to the Player and tell him about what they are doing. A type of enemy can appear in different spaces and will always want to fight the Player before the Player can move into the space occupied by the Enemy. The Player can either fight the enemy to try and defeat it or run away to the previous space occupied. Once an enemy has been killed it no longer exists in that space.

Any weapon owned by the Player may be used to strike an Enemy but each enemy has a specific weapon that must be used to carry out the killing blow. Both the Player and the Enemy have a health level (the enemies is set but the players can increase by taking the medicine and eating food). Each weapon the Player uses has a set strength, which will remove that much health from the enemy. The enemy then attacks the player in return, reducing the players health by a percent of the health removed from the enemy (10% of the enemies strength).

If the Player chooses to run away, the enemy can randomly decide whether to give chase. If the enemy chases the Player, they can only chase him as far as the previously occupied space. It is entirely random whether the enemy catches the Player. Once caught, there is no option but to fight the enemy.

One of the spaces the Player enters will contain a treasure, a diamond. Some of the other spaces will contain a weapon. These are also kept in the Players inventory.

The final space of the world contains both the Dragon and the Princess. When the Player defeats the Dragon and rescues the Princess, the overall outcome varies depending on whether the Player has a diamond, which he can choose either to give to the Princess or keep for himself.

[Junilu Lacar]

Awesome!

I think we're almost ready to write a few unit tests and see if they can suggest a direction for taking the design. One more prep step to do though before we go there: Go through the story and draw up a list of nouns. These are your candidate objects/classes. This should be fairly straightforward. Then pick out a few that will have some kind of interaction with each other in some context. The context will involve doing something interesting as part of the game. For example:

Context/Goal: Player is able to navigate from one space to another on his quest.
Nouns involved: Player, Space
Other possible "concepts" that are not so obvious: Move, Direction

Context/Goal: Player is barred from moving into a space because of the presence of a barrier
Nouns involved: Player, Space, Barrier
Other possible "concepts": ... ?

See if you can list down a few more of these -- maybe 5 more goals that you think are very important to consider as you build out your design and tests. Since we've been talking about the Character hierarchy, see what goals and interactions you can come up with that involve Player, Enemy, and Companion. Again, use your "story" to guide you in this exercise. Completing this exercise should get you closer to understanding if you need a Character hierarchy or not. Even though you've already given it some thought and concluded that you don't need it, this may help validate that decision or make you rethink it.

Your eventual objective is to establish a number of Class-Responsibility-Collaborator mappings but don't think in those terms just yet. Just think Goal-Nouns-Interactions for now. These may translate directly to CRC later but we don't know that for sure yet. We'll let the tests and code give us concrete advice later.

[Ashley Bye]

I've put my head together and come up with the following:

Goal: PLAYER is able to NAVIGATE from one SPACE in a WORLD to another.
Nouns: Player, Space, World.
Interactions: Move (Navigate), Direction.

Goal: Prevent PLAYER from moving into a SPACE due to the presence of a BARRIER.
Nouns: Player, Move (Navigate), Barrier.
Interactions:

Goal: PLAYER can find TREASURE and WEAPONS in SPACES.
Nouns: Player, Treasure, Weapon, Space;
Interactions: Collect (Store).

Goal: PLAYER can speak with and collect GIFTS from FRIENDS if they are present in a SPACE.
Nouns: Player, Gift, Friend.
Interactions: Collect (Store), Speak.

Goal: PLAYER can FIGHT or RUN AWAY from ENEMIES if they are present in a SPACE.
Nouns: Player, Enemy, Space.
Interactions: Fight, Run, Kill, Health (of Enemy and Player), Weapon.

Goal: PLAYER must KILL an ENEMY with a specific WEAPON.
Nouns: Player, Enemy, Weapon.
Interactions: Fight, Kill, Health.

Goal: ENEMY may choose to chase PLAYER into the previous SPACE if the PLAYER chooses to run away, where the ENEMY will then either FIGHT the PLAYER or give up the chase.
Nouns: Enemy, Player, Space.
Interactions: Fight, Kill, Health (of Enemy and Player), Weapon.

Goal: PLAYER and ENEMY HEALTH can decrease during a FIGHT.
Nouns: Player, Enemy, Health.
Interactions: Fight.

Goal: PLAYER can increase HEALTH by CONSUMING certain GIFTS given by FRIENDS.
Nouns: Player, Health, Gift, Friend.
Interactions: Health, Consume.

Goal: PLAYER can choose to GIVE the PRINCESS a DIAMOND if it was COLLECTED in one of the SPACES.
Nouns: Player, Princess, Space.
Interactions: Speak, Give.

I can see that these are useful steps - when I first wrote the program I had a concept that I wanted to achieve and then just started writing without any prior thought. I ended up having to re-write a lot of parts as I went through - hopefully this will be a good learning process and the amount of code I need to re-write to make another aspect work will be reduced.

I had a quick look at GreenFoot - it looks interesting!

[Junilu Lacar]

Excellent! Now, fire up that IDE and let's get to writing a test!

Here's how this usually goes when you first start: Open IDE, create a new project, create a new JUnit test ... (crickets) ... (a minute goes by) ... you're still staring blankly at the computer, trying to think of what first test to write.

Go ahead, give it a shot and tell me if you come up with an idea for a test. You just might be able to prove me wrong.

See if answering these helps though: If you were to write a small snippet of code to show what happens when a Player moves into a Space and finds a Weapon, what would that code look like? How can you turn that snippet into a JUnit test case and what would you assert?

[Ashley Bye]

Cool! In the name of going for keeping it as simple as I can to make the test pass I have the following 2 tests (the second is probably redundant because if a method can return a true boolean it can also return one that is false):

public class SpaceTest {
    
    Space space;
    
    @Before
    public void before() {
        space = new Space();
    }
    
    @Test
    public void testWeaponInSpace() {
        space.setContainsWeapon(true);
        assertTrue(space.containsWeapon());
    }
    
    @Test
    public void testWeaponNotInSpace() {
        space.setContainsWeapon(false);
        assertFalse(space.containsWeapon());
    }
    
}

Is that similar to what you were thinking?

[Junilu Lacar]

Not quite. I generally skip testing simple getters and setters. Try testing more interesting behavior, like what I suggested. Set up a Space. Add a Weapon to it. Move a Player into the Space. Assert that something interesting happened. That's kind of where I was going.

[Ashley Bye]

Okay, well I have come up with the following 2 tests for the Space class:

/**
     * Test of collectTreasure method, of class Space.
     */
    @Test
    public void testCollectTreasure() {
        Treasure sword = new Weapon("Sword", 20);
        Space space = new Space(sword);
        space.collectTreasure();
        
        assertFalse(space.containsTreasure());
    }

    /**
     * Test of containsTreasure method, of class Space.
     */
    @Test
    public void testContainsTreasure() {
        Treasure sword = new Weapon("Sword", 20);
        Space space = new Space(sword);
        
        assertTrue(space.containsTreasure());
    }

And for Player:

/**
     * Test of getInventory method, of class Player.
     * Ensures a treasure is correctly returned as a weapon.
     */
    @Test
    public void testWeaponAddedToInventory() {
        Player player = new Player();
        Treasure treasure = new Weapon("Sword", 20);
        Space space = new Space(treasure);
        
        player.getInventory().add(space.collectTreasure());
        assertEquals(player.getInventory().listWeapons().size(), 1);
    }

Although I can't help but think the test for weapon added should go in the InventoryTest class. It feels like I'm not really writing unit tests but functional tests. I may be wrong though.

I've started a new repository on GitHub with my source files in if you want to look at the code.

[Ashley Bye]

Those were my tests - I messed up in git so now have no project files - I'll get back to you.

[Junilu Lacar]

Ashley Bye wrote:It feels like I'm not really writing unit tests but functional tests.

(Star Wars again): Trust your feelings, Ashley... Yes, they are more functional tests than unit tests. That's not a bad thing.

I hope you're starting to see the "top down" approach pattern here. You'll use these first tests to lead you to more tests, which should start to become more focused on a single unit.

Let's talk a little bit about coding conventions. These are just the ones I use on my projects. I have adopted them from a few sources. I don't see many people following the method naming scheme that I adapted from author Neal Ford but I sure wish more did because I just find it reveals intent and design much better. I've refactored your tests to follow the convention.

   // assuming in SpaceTest.java 

   // renamed from testCollectTreasure() 
   @Test public void containsTreasure_is_false_after_collectTreasure {  
        Treasure sword = new Weapon("Sword", 20);  
        Space space = new Space(sword);  
        space.collectTreasure();  
          
        assertThat(space.containsTreasure(), is(false));  // using org.hamcrest matchers  
    }  
 
    // renamed from testContainsTreasure
    @Test public void containsTreasure_is_true_when_constructed_with_Weapon() {  
        Treasure sword = new Weapon("Sword", 20);  
        Space space = new Space(sword);  
          
        assertThat(space.containsTreasure(), is(true));  
    } 

Since you're using JUnit 4, there's no need to prefix each test name with "test". It's redundant with the @Test annotation. The JUnit folks got rid of the "test" prefix requirement for a good reason and you should honor their efforts to free you from that burden by not using it.

Neal Ford suggests giving precedence to clarity over following convention when it comes to naming test methods, hence underscores and relatively long names are used. I find they're more readable and descriptive, especially in JUnit reports. With a bit of attention to consistency, you can have a JUnit test report that reads very nicely and reveals the intent of your tests and designs very clearly. You may notice that there are a few words that are still standard camel case though. These are references to the methods being tested, so I use the actual method names. Yes, this makes them a little brittle to refactoring (rename method), but again, attention to detail is always a good thing to cultivate in yourself as a programmer. I find that it's not a bother to do a file-wide search and replace in test names that reference the old method name. This also makes you choose method names more carefully so you don't have to deal with that little bit of a bother very often.

Lastly, I like to use assertThat and org.hamcrest.* matchers to make the assert statements more DSL-ish. (DSL=Domain Specific Language) As with the Neal Ford convention, I just find it makes the code a bit more readable.

[Junilu Lacar]

After you write a test, you should not be content with just seeing it pass. The TDD cycle is: Red - Green - Refactor. That is, write a test and see it fail (Red). Write some code to make the failing test pass (Green). Refactor to clarify intent, eliminate duplication, etc.

The Refactor step is key to moving forward with a good design. You should read your tests and your code to see if they make sense. Are they consistent with the story that you wanted to tell? You already hinted at this before but I would ask questions along these lines: Does it make sense to make a Space object collect a treasure as suggested by this: space.collectTreasure()? Does that convey the thought we want to convey? Or is there some other way we can say it? Who collects the treasure?

It's said that good programmers only spend 10% of their time actually writing code. The rest of the time is spent *reading* code to make sure that it makes sense to both themselves and to others. So, after writing a test, read and re-read it with this in mind. Then make adjustments to the story you've written as necessary to clarify it.

[Junilu Lacar]

To spur your design thinking, here's an alternative version of the "collect treasure" narrative:

     Space space = new Space(new Weapon("Sword", 20));
     Player player = new Player(..);
     
     player.moveInto(space);
     player.pickUpTreasure();
     
     // assert that the player's inventory now has one more item in it
     // assert that the space no longer has a treasure that can be picked up

I know this will probably raise more questions in your mind but that's good. The thing to ask right now is: Does this tell your story more clearly?

[Matthew Brown]

Junilu Lacar wrote: Idon't see many people following the method naming scheme that I adapted from author Neal Ford but I sure wish more did because I just find it reveals intent and design much better.

I agree. I use a similar style (primarily in C# rather than Java, but the principles are the same) I picked up from a Kevlin Henney talk at a conference, where you have tests named like this:

class Given_that_blah_blah_blah {
    @Test
    public void When_blah_blah_then_blah_blah_blah() {
        ...
    }

    @Test
    public void When_abc_then_xyz() {
        ...
    }

    ...
}

I don't always follow a string Given/When/Then convention, but the idea is that you read your class name and method name together to reveal the intent of the test, and you naturally group together tests applying to the same scenario.

One reason I don't mind breaking the usual method naming convention is that you never actually call these methods in code (since the test runner does that for you), so it doesn't jar the eye in the same way as breaking the convention for a normal method would.