4
\$\begingroup\$

I am very much new to design pattern. I'm trying to modify the Rock, paper, scissor game from an example book where I try to add more various design pattern. But I am encountering two similar switch statement which I think have duplicates and I don't have any idea on removing those duplicates.

A abstract class:

abstract class UserVsComp {
    protected static Item item;
    abstract protected Item getItem() throws WrongMove;
}

Here are the two switch statement:(First Class is for user movement)

class UserMove extends UserVsComp {
    private String move;
    public UserMove(String move) { this.move = move; }
    public Item getItem() throws WrongMove {
        switch (choiceValue(this.move)) {
            case 0 :  item = new Paper(); break;
            case 1 :  item = new Scissors(); break;
            case 2 :  item = new Rock(); break;
            default:  throw new WrongMove("Wrong move. Game end");
        }
    return item;
}

Second class is for Computer movement:

class ItemGenerator extends UserVsComp {
    public Item getItem() throws WrongMove {
        switch((int) (Math.random() * 2)) {
            case 0: item = new Paper(); break;
            case 1: item = new Scissors(); break;
            case 2: item = new Rock(); break;
            default:  throw new WrongMove("Wrong move. Game end");
        }
    return item;
    }
}

The two switch statement have same case (I guess duplicacy). I'd like to know if there is any way to remove this duplicacy.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Another problem (not related to your question, but this is Code Review): all the subclassing is an abuse of inheritance and probably should be done with composition. ItemGenerator and UserMove are subclasses of UserVsComp, which doesn't make sense to me. Could you please cite the book, so we can have more context? \$\endgroup\$ – Fuhrmanator Feb 4 '15 at 12:39
  • \$\begingroup\$ Thanks for your comment. Yes you are right. This is a mistake I have made. Actually the ItemGenerator class will be CompMove class. Thanks for pointing that out and I will try to use composition instead of inheritance in this example. \$\endgroup\$ – Deb Feb 4 '15 at 12:59
12
\$\begingroup\$

Instead of creating a new Item each time you can just use a enum:

public enum Move{
    Paper{ 
        public boolean canBeat(Move other){
            return other==Rock;
        }
    },
    Scissor{ 
        public boolean canBeat(Move other){
            return other==Paper;
        }
    },
    Rock{ 
        public boolean canBeat(Move other){
            return other==Scissor;
        }
    };

    public abstract boolean canBeat(Move other);


    public boolean losesTo(Move other){
        return other.canBeat(this);
    }
}

Then You can keep them in an array (like what is returned by Move.values()) and index into that.

public Item getItem() throws WrongMove {
    int choice = choiceValue(this.move);
    if(choice<0||choice >= Move.values().length)
        throw new WrongMove("Wrong move. Game end");
    else return  Move.values()[choice];
}
\$\endgroup\$
1
  • \$\begingroup\$ It's a good idea to use enum. Thanks for your reply \$\endgroup\$ – Deb Feb 3 '15 at 12:24
8
\$\begingroup\$

This is a quite easy fix. All you need to do is to extract a method.

Both snippets have the same input: int

Both snippets have the same output: Item

Both snippets can throw the same exception: WrongMove

Put this method somewhere:

public static Item itemForInt(int value) throws WrongItem {
    switch(value) {
        case 0: return new Paper();
        case 1: return new Scissors();
        case 2: return new Rock();
        default:  throw new WrongMove("Wrong move. Game end");
    }
}

And call it from your other methods.

class UserMove extends UserVsComp {
    private String move;
    public UserMove(String move) { this.move = move; }
    public Item getItem() throws WrongMove {
        return SomeClass.itemForInt(choiceValue(this.move)));
    }
}

WrongMove would be better named WrongMoveException

The getItem() method should be marked with @Override in UserMove and ItemGenerator

There is no need to put this on a single line:

public UserMove(String move) { this.move = move; }

This is more readable:

public UserMove(String move) {
    this.move = move;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your response.The answer is very helpful and easy way to do it. Thanks. \$\endgroup\$ – Deb Feb 3 '15 at 12:21

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.