3
\$\begingroup\$

I have written this to practice Test Driven Development. I would like to know if the process is right and how it could be improved.

Problem Statement : A wizard can cast spell on goblin and make his size big, normal or small. He can also make the goblin visible or invisible. In addition to this, the wizard can keep track of his spells and choose to undo or redo them. Initially, the goblin is normal and visible.

Test cases (Unit tests):

public class WizardSpec {

    private Goblin goblin;
    private Wizard wizard;

    @Before
    public void setUp() {
        goblin = new Goblin();
        wizard = new Wizard();
    }

    @Test
    public void wizardCastsSpellToMakeGoblinSmall() {
        wizard.castsSpell(goblin, new ShrinkSizeSpell());
        assertEquals(Size.SMALL, goblin.getSize());
    }

    @Test
    public void wizardCastsSpellToMakeGoblinBig() {
        wizard.castsSpell(goblin, new EnlargeSizeSpell());
        assertEquals(Size.BIG, goblin.getSize());
    }

    @Test
    public void wizardCastsSpellToMakeGoblinInvisible() {
        wizard.castsSpell(goblin, new InvisibleSpell());
        assertEquals(Visibility.INVISIBLE, goblin.getVisibility());
    }

    @Test
    public void wizardCastsSpellToMakeGoblinVisible() {
        wizard.castsSpell(goblin, new VisibleSpell());
        assertEquals(Visibility.VISIBLE, goblin.getVisibility());
    }

    @Test
    public void whenWizardUndoesSpellGoblinGoesBackToPreviousState() {
        wizard.castsSpell(goblin, new VisibleSpell());
        wizard.castsSpell(goblin, new InvisibleSpell());
        wizard.castsSpell(goblin, new VisibleSpell());
        wizard.castsSpell(goblin, new VisibleSpell());
        wizard.castsSpell(goblin, new InvisibleSpell());
        wizard.castsSpell(goblin, new InvisibleSpell());
        for(int i = 0; i < 4; i++)
            wizard.undoSpell();
        assertEquals(Visibility.INVISIBLE, goblin.getVisibility());
    }

    @Test
    public void whenWizardRedoesSpellGoblinGoesToTheSameStateAgain() {
        wizard.castsSpell(goblin, new ShrinkSizeSpell());
        wizard.castsSpell(goblin, new NormalSizeSpell());
        wizard.castsSpell(goblin, new NormalSizeSpell());
        wizard.castsSpell(goblin, new EnlargeSizeSpell());
        wizard.undoSpell();
        wizard.undoSpell();
        wizard.redoSpell();
        wizard.redoSpell();
        assertEquals(Size.BIG, goblin.getSize());
    }
}

Wizard class: Wizard casts spell, undoes the spell and redoes it.

public class Wizard {

    private Stack<Spell> undoSpellStack = new Stack<Spell>();
    private Stack<Spell> redoSpellStack = new Stack<Spell>();

    public void castsSpell(Goblin goblin, Spell spell) {
        spell.execute(goblin);
        undoSpellStack.add(spell);
    }

    public void undoSpell() {
        if(!undoSpellStack.isEmpty()) {
            Spell spell = undoSpellStack.pop();
            redoSpellStack.add(spell);
            spell.undo();
        }
    }

    public void redoSpell() {
        if(!redoSpellStack.isEmpty()) {
            Spell spell = redoSpellStack.pop();
            undoSpellStack.add(spell);
            spell.redo();
        }
    }
}

Goblin class:

public class Goblin {

    private Size size;
    private Visibility visibility;

    public Goblin() {
        setSize(Size.NORMAL);
        setVisibility(Visibility.VISIBLE);
    }

    public Size getSize() {
        return this.size;
    }

    public void setSize(Size size) {
        this.size = size;
    }

    public Visibility getVisibility() {
        return this.visibility;
    }

    public void setVisibility(Visibility visibility) {
        this.visibility = visibility;
    }
}

Size: One of the attributes of Goblin. Initially, I implemented this as a string and then thought this was primitive obsession code smell and changed it.

public enum Size {
    SMALL("Small"), BIG("Big"), NORMAL("Normal");

    private String size;

    Size(String size) {
        this.size = size;
    }

    @Override
    public String toString() {
        return this.size;
    }
}

Visibility : Another attribute of Goblin

public enum Visibility {
    VISIBLE("Visible"), INVISIBLE("Invisible");

    private String visibility;

    Visibility(String visibility) {
        this.visibility = visibility;
    }

    @Override
    public String toString() {
        return this.visibility;
    }
}

Spell: I have used the command pattern here. Spell is the command. Redo method was duplicated in all the classes that extend Spell. So I extracted it to the base class.

public abstract class Spell {

    public abstract void execute(Goblin goblin);

    public abstract void undo();

    public void redo() {
        undo();
    }
}

ShrinkSizeSpell: Spell to shrink the goblin.

public class ShrinkSizeSpell extends Spell{

    private Goblin goblin;
    private Size size;

    @Override
    public void execute(Goblin goblin) {
        this.size = goblin.getSize();
        goblin.setSize(Size.SMALL);
        this.goblin = goblin;
    }

    @Override
    public void undo() {
        if(size != null && goblin != null) {
            Size previousSize = this.goblin.getSize();
            this.goblin.setSize(this.size);
            this.size = previousSize;
        }
    }
}

EnlargeSizeSpell: Spell to make the Goblin big.

public class EnlargeSizeSpell extends Spell{

    private Goblin goblin;
    private Size size;

    @Override
    public void execute(Goblin goblin) {
        this.size = goblin.getSize();
        goblin.setSize(Size.BIG);
        this.goblin = goblin;
    }

    @Override
    public void undo() {
        if(size != null && goblin != null) {
            Size previousSize = this.goblin.getSize();
            this.goblin.setSize(this.size);
            this.size = previousSize;
        }
    }
}

NormalSizeSpell: Spell to make the goblin normal.

public class NormalSizeSpell extends Spell {

    private Goblin goblin;
    private Size size;

    @Override
    public void execute(Goblin goblin) {
        this.size = goblin.getSize();
        goblin.setSize(Size.NORMAL);
        this.goblin = goblin;
    }

    @Override
    public void undo() {
        if(size != null && goblin != null) {
            Size previousSize = this.goblin.getSize();
            this.goblin.setSize(this.size);
            this.size = previousSize;
        }
    }
}

VisibleSpell: Makes the goblin visible

public class VisibleSpell extends Spell {

    private Visibility visibility;
    private Goblin goblin;

    @Override
    public void execute(Goblin goblin) {
        this.visibility = goblin.getVisibility();
        goblin.setVisibility(Visibility.VISIBLE);
        this.goblin = goblin;
    }

    @Override
    public void undo() {
        if(visibility != null && goblin != null) {
            Visibility previousStateOfVisibility = this.goblin.getVisibility();
            this.goblin.setVisibility(this.visibility);
            this.visibility = previousStateOfVisibility;
        }
    }
}

InvisibleSpell: Makes the goblin invisible

public class InvisibleSpell extends Spell {

    private Visibility visibility;
    private Goblin goblin;

    @Override
    public void execute(Goblin goblin) {
        this.visibility = goblin.getVisibility();
        goblin.setVisibility(Visibility.INVISIBLE);
        this.goblin = goblin;
    }

    @Override
    public void undo() {
        if(visibility != null && goblin != null) {
            Visibility previousStateOfVisibility = this.goblin.getVisibility();
            this.goblin.setVisibility(this.visibility);
            this.visibility = previousStateOfVisibility;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Good approach. I personally would also abstract the Goblin class into e.g. MonsterBase (abstract) or ISpellTargetable (Interface). This gives you freedom to implement other monsters later without changing the other classes. \$\endgroup\$ – Alex B. Jul 10 '16 at 17:24
  • \$\begingroup\$ Thank you for the feedback. That is a good point because when the requirement changes like you said, it would affect all the concrete Spell classes. \$\endgroup\$ – Vimde Jul 11 '16 at 2:38
  • \$\begingroup\$ I don't understand what you're asking when you say you're trying to "practice Test Driven Development" and "would like to know if the process is right and how it could be improved." You didn't really explain the process you used, which is what Test Driven Development is - a process you use to write code, not a design pattern. If your process was to write your tests first then write your code to pass those tests then you did it correctly. \$\endgroup\$ – D.B. Jul 11 '16 at 4:26
  • \$\begingroup\$ I also agree with @D.B. You have to ask yourself: Did I wrote my tests before implementing the logic? E.g. wrote wizard= new Wizard(); and produced a compile error because the class have not yet exist. Satisifed the error by creating an empty class Goblin. Produced another compile error with wizard.castsSpell(null, null) since the method castsSpell does not exist. Also, check for the simplest parameters first (null in this case). Not until then create tests for "real" parameters. Check also learning material. \$\endgroup\$ – Alex B. Jul 11 '16 at 7:38
  • \$\begingroup\$ I agree with both of you. I haven't explained the process. I should have added that too. I did write my tests first but I did not take such small steps. For instance, I wrote wizard.castsSpell(goblin, "Small") and not wizard.castsSpell(null, null). This is how I started. I should perhaps start with small steps. Now here is my question based on that. When I wrote the test first, I had "Small" as a string. When I refactored the code, I changed the test. Is it fine to change the tests after you refactor or should I never change the tests once I have written them in Test Driven Development? \$\endgroup\$ – Vimde Jul 12 '16 at 2:24
1
\$\begingroup\$

On the whole, I like what you've written. There are a few things to think about...

Missing Tests

  • You don't seem to be testing the initial state of Goblin anywhere (it may be that you're testing it somewhere else, or that it's unimportant).
  • You have a spell NormalSizeSpell which you use in your whenWizardRedoesSpellGoblinGoesToTheSameStateAgain, but you don't actually check anywhere that it changes the size of the Goblin to NORMAL.
  • Exception conditions. What happens if you supply a null Goblin to execute, if you redo a spell that hasn't been cast, if you undo a spell that hasn't been cast. I'd expect at least some of these to throw exceptions, currently your code ignores the invalid states and doesn't perform null checks in it's execute methods (although ironically it does check for null in the undo methods).

Duplication

There's a lot of duplication between your spells. You basically have two types of spells, those that change Visibility and those that change Size. If you created two base classes that allowed the target size/visiblity to be passed in, then your Normal/Small/Enlarge spells become simple wrappers that only have to pass the target size down to the base class.

Spell Construction

Consider passing the Goblin into the constructor for your spells (as a target parameter). If you throw an exception if this parameter is null, then you can assume that the goblin exists in your other methods which removes their null checks.

Spell Stack

Whilst the way you've modelled the code makes sense, I'm not entirely convinced by the non-additive nature of your spells. If I started off with a 'Small' goblin, I'd expect Englarge to make it 'Normal', then Enlarge again to make it 'Large'.

\$\endgroup\$
  • \$\begingroup\$ Thank you for reviewing the code. I have missed some scenarios indeed. I have written tests only for the happy path. Regarding the Spell Stack, I thought Small should make the goblin small, Enlarge should make him big and so on. I did not see it the way you did. But that is an interesting point which I would like to implement along with the other suggestions. \$\endgroup\$ – Vimde Jul 12 '16 at 2:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.