Memory text based game

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.ou

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...

StackExchange Code Review Q#62695, answer score: 8