9
\$\begingroup\$

I made a simple text-based UNO game in Java. It lacks some features of the original game, but it's good enough for now. I'm new to Java, so I would like to receive as much feedback and suggestions as I can on my code.

The Unocard class:

import java.util.Random;

public class Unocard
{
    public String color;
    public int value;
    private Random rand;
    private String face;

    public Unocard(int v, String c)
    {
        value = v;
        color = c; 
    }

    // Creates a random card
    public Unocard()
    {
        rand = new Random();
        value = rand.nextInt(28); // 108 cards in a standard Uno deck. Can be reduced to 27 (disregarding colors)
        // Assigning value
        if (value >= 14) // Some cards are more common than others
            value -= 14;
        // Assigning color
        rand = new Random();
        switch(rand.nextInt(4) )
        {
            case 0: color = "Red"; 
                break;
            case 1: color = "Green"; 
                break;
            case 2: color = "Blue"; 
                break;
            case 3: color = "Yellow"; 
                break;
        }
        // If the card is a wild card
        if (value >= 13)
            color = "none";
    }

    public String getFace()
    {
        /* Returns the face of the card (what the player sees)
         * Ex. [Red 5]
         */
        face = "[";
        if (color != "none")
        {
            face += this.color + " ";
        }

        switch(this.value)
        {
            default: face += String.valueOf(this.value); 
                break;
            case 10: face += "Skip"; 
                break;
            case 11: face += "Reverse"; 
                break;
            case 12: face += "Draw 2"; 
                break;
            case 13: face += "Wild"; 
                break;
            case 14: face += "Wild Draw 4"; 
                break;
        }
        face += "]";
        return face;
    }

    // Determines if you can place this card on top of a given card
    // The color needs to be specified because if a wild card was chosen in the previous round, the color would have changed, but the card staying the same
    public boolean canPlace(Unocard o, String c)
    {
        if (this.color == c)
            return true;
        else if (this.value == o.value)
            return true;
        else if (this.color == "none") // Wild cards
            return true;
        return false;
    }
}

Main:

import java.util.ArrayList;
import java.util.Scanner;

public class Uno
{
    public static void main(String[] args)
    {
        ArrayList<Unocard> playerdeck = new ArrayList<Unocard>();
        ArrayList<Unocard> compdeck = new ArrayList<Unocard>();
        int win; // 0 - no result; 1 - win; -1 - loss. 
        Scanner input;
        Unocard topCard; // card on top of the "pile"
        int choiceIndex; // Index of chosen card for both player and computer
        String currentColor; // Mainly used for wild cards

        gameLoop:
        while (true)
        {
            playerdeck.clear();
            compdeck.clear();
            win = 0;
            topCard = new Unocard();
            currentColor = topCard.color;

            System.out.println("\nWelcome to Uno! Initialising decks...");
            draw(7, playerdeck);
            draw(7, compdeck);

            /*****************Turns*****************/
            for (boolean playersTurn = true; win == 0; playersTurn ^= true)
            {
                choiceIndex = 0;
                System.out.println("\nThe top card is: " + topCard.getFace());

                if (playersTurn) /*****Player's turn******/
                {
                    // Displaying user's deck
                    System.out.println("Your turn! Your choices:");
                    for (int i = 0; i < playerdeck.size(); i++)
                    {
                        System.out.print(String.valueOf(i + 1) + ". " + 
                        ((Unocard) playerdeck.get(i) ).getFace() + "\n");
                    }
                    System.out.println(String.valueOf(playerdeck.size() + 1 ) + ". " + "Draw card" + "\n" + 
                                       String.valueOf(playerdeck.size() + 2) + ". " + "Quit");
                    // Repeats every time the user doesn't input a number
                    do
                    {
                        System.out.print("\nPlaease input the number of your choice: ");
                        input = new Scanner(System.in);
                    } while (!input.hasNextInt() );
                    // The choices were incremented to make them seem more natural (i.e not starting with 0)
                    choiceIndex = input.nextInt() - 1;

                    // Taking action
                    if (choiceIndex == playerdeck.size() )
                        draw(1, playerdeck);
                    else if (choiceIndex == playerdeck.size() + 1)
                        break gameLoop;
                    else if ( ((Unocard) playerdeck.get(choiceIndex)).canPlace(topCard, currentColor) )
                    {
                        topCard = (Unocard) playerdeck.get(choiceIndex);
                        playerdeck.remove(choiceIndex);
                        currentColor = topCard.color;
                        // Producing the action of special cards                        
                        if (topCard.value >= 10)
                        {
                            playersTurn = false; // Skipping turn

                            switch (topCard.value)
                            {
                                case 12: // Draw 2
                                System.out.println("Drawing 2 cards...");
                                draw(2,compdeck);
                                break;

                                case 13: case 14: // Wild cards                         
                                do // Repeats every time the user doesn't input a valid color
                                {
                                    System.out.print("\nEnter the color you want: ");
                                    input = new Scanner(System.in);
                                } while (!input.hasNext("R..|r..|G....|g....|B...|b...|Y.....|y.....") ); //Something I learned recently
                                if (input.hasNext("R..|r..") )
                                    currentColor = "Red";
                                else if (input.hasNext("G....|g....") )
                                    currentColor = "Green";
                                else if (input.hasNext("B...|b...") )
                                    currentColor = "Blue";
                                else if (input.hasNext("Y.....|y.....") )
                                    currentColor = "Yellow";

                                System.out.println("You chose " + currentColor);
                                if (topCard.value == 14) // Wild draw 4
                                {
                                    System.out.println("Drawing 4 cards...");
                                    draw(4,compdeck);
                                }
                                break;
                            }
                        }
                    } else System.out.println("Invalid choice. Turn skipped.");


                } else /************ computer's turn **************/
                {
                    System.out.println("My turn! I have " + String.valueOf(compdeck.size() ) 
                                        + " cards left!" + ((compdeck.size() == 1) ? "...Uno!":"") );
                    // Finding a card to place
                    for (choiceIndex = 0; choiceIndex < compdeck.size(); choiceIndex++)
                    {
                        if ( ((Unocard) compdeck.get(choiceIndex)).canPlace(topCard, currentColor) ) // Searching for playable cards
                            break; 
                    }

                    if (choiceIndex == compdeck.size() )
                    {
                         System.out.println("I've got nothing! Drawing cards...");
                         draw(1,compdeck);
                    } else 
                    {
                         topCard = (Unocard) compdeck.get(choiceIndex);
                         compdeck.remove(choiceIndex);
                         currentColor = topCard.color;
                         System.out.println("I choose " + topCard.getFace() + " !");

                         // Must do as part of each turn because topCard can stay the same through a round
                         if (topCard.value >= 10)
                         {
                             playersTurn = true; // Skipping turn

                             switch (topCard.value)
                             {
                                 case 12: // Draw 2
                                 System.out.println("Drawing 2 cards for you...");
                                 draw(2,playerdeck);
                                 break;

                                 case 13: case 14: // Wild cards                         
                                 do // Picking a random color that's not none
                                 {
                                     currentColor = new Unocard().color;
                                 } while (currentColor == "none");

                                 System.out.println("New color is " + currentColor);
                                 if (topCard.value == 14) // Wild draw 4
                                 {
                                     System.out.println("Drawing 4 cards for you...");
                                     draw(4,playerdeck);
                                 }
                                 break;
                             }
                         }
                    }

                    // If decks are empty
                    if (playerdeck.size() == 0)
                        win = 1;
                    else if (compdeck.size() == 0)
                        win = -1;
                }

            } // turns loop end

            /*************Results**************/
            if (win == 1)
                System.out.println("You win :)");
            else 
                System.out.println("You lose :(");

            System.out.print("\nPlay again? ");
            input = new Scanner(System.in);

            if (input.next().toLowerCase().contains("n") )
                break;
        } // game loop end

        System.out.println("Bye bye");
    }
    // For drawing cards
    public static void draw(int cards, ArrayList<Unocard> deck)
    {
        for (int i = 0; i < cards; i++)
            deck.add(new Unocard() );
    }
}
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Apr 4 '16 at 12:58
  • \$\begingroup\$ What version of Java are you targeting? \$\endgroup\$ – Robert Snyder Apr 4 '16 at 13:14
  • \$\begingroup\$ @RobertSnyder No particular version right now. \$\endgroup\$ – Okoyos Apr 4 '16 at 13:23
  • \$\begingroup\$ Can you provide a flow chart?. Do you have the plus card stacking feature? \$\endgroup\$ – Christopher Perez Dec 12 '19 at 17:40
5
\$\begingroup\$

Think small, Think one

What I mean is that breaking up things into smaller chunks that all do one thing and do it well helps in so many ways. Now granted it can be taken too far, but for the most part if you break things down into something sort of vague and build from there. For instance if you think of uno as a few things such as "Game Rules", "Cards", "Players", "Deck" then you would at a bare minimum start with 4 classes. Those 4 things would do one thing and do it well. Like lets take Game Rules. You could have started with it calling each player in order and asking them to take their turn. It could have kept track of the deck and gave a player a card when it was requested. It could have also shuffled the deck of predefined cards (like it sorta would have been if you buy the game). After each player it could have checked if said player won or not. This would have made the class kind of big, but now maybe you could have seen that the user input is what is bloating the class. So you could have pulled it out in a user input class of sorts.. so on and so forth. The point is that if you think of the individual moving parts of a system and start there and work your way down while keeping in the back of your mind that you want things to stay small, and they should be responsible for one thing and one thing only then your code would look very different than it does now and potentially easier to understand at first glance

Magic Numbers

Magic numbers are numbers or string literals (for example: "none") make a person wonder. "What does 12 mean?" or "What does 13/14 do?". You asked that question to your self at one time because you put a comment in to tell yourself that 12 means that the card is a Draw 2. So instead of putting 12 why not make a class (abstract class if you want, or maybe an enum) that contains some these magic numbers it takes the guess work and the comments out of your code. I imagine it looking somethign like this in one case

switch (topCard.value)
{
    case CardValues.DrawTwo
        System.out.println("Drawing 2 cards...");
        draw(2,compdeck);
        break;

    case CardValues.Wild:
    case CardValues.WildDrawFour:
    //...code revmoed for clarity

    System.out.println("You chose " + currentColor);
    if (topCard.value == CardValues.WildDrawFour)
    {
        System.out.println("Drawing 4 cards...");
        draw(4,compdeck);
    }
    break;
}

It's not much, but I think the concept of what i'm saying will make a big difference. Good luck.

\$\endgroup\$
1
\$\begingroup\$

You shouldn't have rand and face as class fields. You only use face inside the getFace() method to build the string so it should be a local variable in that method instead. It's the same with rand and the constructor. Additionally you shouldn't create a new Random object every time you need a random number. Instead you should have one single instance of Random for the whole application.

Generating a random card in the card object doesn't seem to be a good idea anyway. This way you'll have duplicate cards and not a regular deck. It would be better to create a complete deck of all 108 cards, shuffle it and draw cards from it.

It's a bad idea to use Strings to represent the colors or numbers to repesent to card functions. This can go wrong if you happen to mistype a color name. Use Enums instead.

\$\endgroup\$

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