Memory text based game

Question

Would anyone be so kind as to review my code for Memory?

import java.util.*;

public class Cards {

    public boolean gameInProgress = true;

    public void shuffleCards() {

        Scanner userInput = new Scanner(System.in);
        List cardList = new LinkedList();
        List matchedCards = new LinkedList();

        //this is the list of cards available to pick from
        String[] cards = {"a", "b", "c", "d", "e", "a", "b", "c", "d", "e"};

        //this shuffles the cards each game
        Collections.shuffle(Arrays.asList(cards));

        //this moves the cards array into a LinkedList
        Collections.addAll(cardList, cards);

        //main game loop. stops when all cards are matched
        while (gameInProgress) {

            //Stores the users card picks in variables card1 and card2
            System.out.println("pick a card (0-9)\n");

            int card1 = userInput.nextInt();

            System.out.println("First card picked is the letter " + cardList.get(card1));

            System.out.println("pick a second card.\n");

            int card2 = userInput.nextInt();

while(card1 == card2){
            System.out.println("You cannot pick the same card twice. Pick a different card.");
            card2 = userInput.nextInt();
        }

            System.out.println("second card picked is the letter " + cardList.get(card2));


            //checks if a card has been picked already
            while(matchedCards.contains(cardList.get(card1))) {
                System.out.println("First card already picked. Pick again: ");
                card1 = userInput.nextInt();
            }

            while(matchedCards.contains(cardList.get(card2))){
                System.out.println("Second card already picked. Pick again: ");
                card2 = userInput.nextInt();
            }

                //copies the matched cards to a new linked list
                if (cardList.get(card1) == cardList.get(card2)) {
                    System.out.println("You got a match!\n");

                    matchedCards.add(cardList.get(card1));
                    matchedCards.add(cardList.get(card2));

                    System.out.println("You have collected " + matchedCards.size() + "/10 cards");

                    System.out.println(matchedCards + " are in the matched pile\n");

                    //stops the game once all cards have been matched
                    if (matchedCards.size() == 10) {
                        System.out.println("You Win!!!");
                        gameInProgress = false;
                    }
                }
                else {
                    System.out.println("Not a match\n");

                }
        }
    }
}



    public class Game {


    public static void main(String[] args) {


        Cards card = new Cards();

        card.shuffleCards();



    }
}

Answer 1

Review:

public void shuffleCards()

Should only shuffle cards, because that's what it says it does. Create new methods void playGame() int getFirstPick(), int getSecondPick(), boolean isAlreadyMatched(int card1, int card2), boolean areAllCardsMatched() and possibly others.

Assign these methods to the class that they should belong to.

Game
    void playGame()        
    int getFirstPick()
    int getSecondPick()
    boolean verifyFirstPick(int firstPick)
    boolean verifySecondPick(int firstPick, int secondPick)

Cards
    void shuffleCards()
    boolean isAlreadyMatched(int card)
    boolean areAllCardsMatched()

Why?

Because right now you've got one long piece of code that does everything, and it's confusing you and causing bugs. By splitting it up, it'll be easier to identify what you need to do and whether the code is doing it.

If you do it right, you'll end up with code like this:

public void playGame(){
    //printInstructions(); //an idea, perhaps?
    setupGame();
    while(!isGameWon()){
        doTurn();
    }
    printGameWon();
}

That's awfully abstract, Pim.

Okay, lets dive a level deeper, in setupGame:

public void setupGame(){
    cards = new Cards();
    cards.shuffle();
    userInput = new Scanner(System.in);
}

That sets up the playing field and the input, it seems.

What's the next method? isGameWon()...

public boolean isGameWon(){
    return (cards != null && cards.areAllCardsMatched());
}

It's just a check for whether all the cards are matched. (There's a sneaky shortcut here; with cards != null I check whether the game has been initialized yet.)

Okay, what about doTurn() then? That one is a bit bigger.

public void doTurn(){
    int firstPick = -1;        
    do {
        firstPick = getFirstPick();
    } while(!verifyFirstPick(firstPick));
    printFirstPick(firstPick);

    int secondPick = -1;
    do {
        secondPick = getSecondPick();
    } while(!verifySecondPick(firstPick, secondPick));
    printSecondPick(secondPick);

    checkMatch(firstPick, secondPick);
}

It handles getting the first picked card and the second picked card, then passes them along to checking for a match.

Let's go look at the checkMatch function.

public void checkMatch(int firstPick, int secondPick){
    if(cards.match(firstPick, secondPick)){
        printMatch();
        printRemainingMatches();
    } else {
        printNoMatch();
    }
}

We STILL haven't seen any real code doing anything. All I have shown you right now that actually does anything is the setupGame method. The rest just structures the game flow.

Function lists right now:

Game
    void setupGame();
    void playGame();
    void doTurn();
    void checkMatch(int firstPick, int secondPick);     
    int getFirstPick();
    int getSecondPick();
    boolean verifyFirstPick(int firstPick);
    boolean verifySecondPick(int firstPick, int secondPick);
    boolean isGameWon();

    void printFirstPick(int firstPick);
    void printSecondPick(int secondPick);
    void printMatch();
    void printNoMatch();
    void printRemainingMatches();
    void printGameWon();

Cards
    void shuffleCards();
    boolean match(int card1, int card2);
    boolean isAlreadyMatched(int card);
    boolean areAllCardsMatched();

It'll be your task to implement the ones I haven't shown yet. You'll also have to move some of the variables so they're class members, not just declared in a function (hint: cards and userInput are two of these, I don't know if there are more).

Given the name of the functions, it should be easy to guess what they do.
shuffleCards shuffles the cards. match tests if two cards are a match. And so on...

Answer 2

Bug:

Match a pair (lets assume they're at 0 and 1). Pick 0 as your first card, pick 2 as your second card. Repick 2 as your first card. We've circumvented the double pick check. And then we have a match.

I'd fix this by reordering the statements:

        System.out.println("pick a card (0-9)\n");

        int card1 = userInput.nextInt();

        //checks if a card has been picked already
        while(matchedCards.contains(cardList.get(card1))) {
            System.out.println("First card already picked. Pick again: ");
            card1 = userInput.nextInt();
        }

        System.out.println("First card picked is the letter " + cardList.get(card1));

        System.out.println("pick a second card.\n");

        int card2 = userInput.nextInt();

        while(matchedCards.contains(cardList.get(card2) || card1 == card2)){
            System.out.println("Second card already picked. Pick again: ");
            card2 = userInput.nextInt();
        }

        System.out.println("second card picked is the letter " + cardList.get(card2));

I merged the "second card already picked" and "can't pick same card" messages because technically you're picking a card that's already picked.

Answer 3

Besides what Pimgd has pointed out, I have some more things to say:

First of all, your code is a bit of a mess as the indentation is quite far off, and the spacing likewise. If you're using an IDE, which I hope that you are, use your IDE to automatically correct these things. Or look at Pimgd's code to see how it's done.

---

In your setup, you are first creating a list, then an array, then a list from the array, then add your array to the first list. This can be done simpler.

Additionally, your array is not maintainable as it is very easy to accidentally switch one of the letters to a completely different letter, so that there are no pairs anymore.

Also, you're not using generics for your lists, and I would recommend using ArrayList instead of LinkedList, because it will be faster with regards to the remove method.

A better setup would be:

List<String> cards = Arrays.asList(new String[] { "a", "b", "c", "d", "e" });
List<String> cardList = new ArrayList<String>();
cardList.addAll(cards);
cardList.addAll(cards);
Collections.shuffle(cards);

---

In these two lines:

System.out.println("You have collected " + matchedCards.size() + "/10 cards");

if (matchedCards.size() == 10) {

You are referring to the number 10? But why are you referring to the number 10? Really, why the number 10? Ah, because that's the number of cards you have? Then why don't you refer to the number of cards you have!? What if you would add some cards?

System.out.println("You have collected " + matchedCards.size() + "/" + cards.size() + " cards");

if (matchedCards.size() == cards.size()) {