1
\$\begingroup\$

still new to java and i am trying to improve my understanding and use of OOP concepts.here i am supposed to print the name of a randomly selected card from a deck of 52 cards.

Program:

/*
 * RandomCard.java
 * -------------------
 * Displays the name of a card randomly chosen from a complete deck of 52 
 * playing cards.
 */

 import java.util.Random;

 public class RandomCard {
    /* Instance Variables */
    private Random rgen = new Random(); /* random generator */

    /**
     * Display the name of a randomly selected card from a deck of 52 cards
     */
    public static void main(String [] args) {
        System.out.println("This program displays a randomly chosen card");
        Card randomCard = selectCardFromDeckRandomly();
        System.out.println(randomCard);
    }

    /*
     * Randomly select a card from a card deck
     * @ return The selected card
     */
    private Card selectCardFromDeckRandomly() {
        String rank = selectRanomCardRank();
        String suit = selectRandomCardSuit();
        return (new Card(rank, suit));  
    }

    /*
     * Randomly select a card rank
     * @ return the selected card rank
     */
    private String selectRanomCardRank() {
        String cardRank;
        int r = 1 + rgen.nextInt(13);
        switch (r) {
        case 1:
            cardRank = "Ace";
            break;
        case 2:
            cardRank = "2";
            break;
        case 3:
            cardRank = "3";
            break;
        case 4:
            cardRank = "4";
            break;
        case 5:
            cardRank = "5";
            break;          
        case 6:
            cardRank = "6";
            break;
        case 7:
            cardRank = "7";
            break;
        case 8:
            cardRank = "8";
            break;
        case 9:
            cardRank = "9";
            break;
        case 10:
            cardRank = "10";
            break;
        case 11:
            cardRank = "Jack";
            break;
        case 12:
            cardRank = "Queen";
            break;          
        default:
            cardRank = "King";
            break;
        }
        return cardRank;
    }

    /*
     * Randomly select a card suit
     * @ return The selected card suit
     */
    private String selectRandomCardSuit() {
        String cardSuit;
        int s = 1 + rgen.nextInt(4);
        switch (s) {
        case 1:
            cardSuit = "Clubs";
            break;
        case 2:
            cardSuit = "Diamonds";
            break;
        case 3:
            cardSuit = "Hearts";
            break;
        default:
            cardSuit = "Spades";
            break;
        }
        return cardSuit;
    }
}

class Card:

/*
 * Card.java
 * -------------
 * Creates a playing card of certain Rank and Suit.
 * This class is part of exercises 1 solution.
 * This program is intended as a practice on programming methodologies.
 */


 public class Card {
    /* Instance variables */
    private String rank;   /* The card rank : Ace,2,3,4,5,6,7,8,9,10,Jack,Queen,King */
    private String suit;   /* The card suit : Clubs, Diamonds, Hearts, Spades */

    /* Constructor */
    /**
     * Creates an instance object of class Card with the specified rank and suit
     * @param rnk The rank of the card
     * @param sut The suit of the card
     */
    public Card(String rnk, String sut) {
        rank = rnk;
        suit = sut;
    }

    public String toString() {
        return (rank + " of " + suit);
    }
}

Note: still not in a position to use arrays.

Evaluation points:

  1. Encapsulation
  2. Top-Down design (refactoring)
  3. Readability and complexity reduction
  4. Object oriented
  5. Documentation
\$\endgroup\$
3
  • \$\begingroup\$ fixed indentation. which parentheses? you mean at return statement? i deliberately included those parentheses to make it clear what is returned. not sure if that is bad or just unnecessary. appreciate any advice on that. \$\endgroup\$
    – ahmed dahy
    Commented Jul 18, 2018 at 19:58
  • \$\begingroup\$ The parenthesis that you added in Rev 3. \$\endgroup\$ Commented Jul 18, 2018 at 20:36
  • \$\begingroup\$ Your code is not selecting randomly from a Deck. It can result in duplicate choices. Instead Create a deck of the pre-generated cards. Then select and remove a card from the deck. Code is broadly OK, but the #1 review criteria is always meeting the requirements. You'll need at least three Class, Card (present) Deck (missing) and a Dealer. \$\endgroup\$ Commented Jul 19, 2018 at 18:21

1 Answer 1

2
\$\begingroup\$

You did many things right. Great! =)

Readability and complexity reduction

Your

private String rank;

is nice and readable, but what is a rnk in

public Card(String rnk, String sut)

?

I would not suggest to compromise on the readability of publicly visible variable names. (And I would neither suggest doing so on any other variable of any access level.)

If there's no good alternative name for parameter names, use this to differentiate the readable (and equal) variable and parameter names:

public Card(String rank, String suit) {
    this.rank = rank;
    this.suit = suit;
}

A common alternative solution is to add an underscore character to the variable name like so:

private String _rank;
private String _suit;  

Speaking of those two variables, it might make sense to make them final. After all, the suit of a card should never be changed. This reduces complexity, because you can only ever set the value once.

Top-Down design (refactoring)

a randomly selected card from a deck of 52 cards.

You are not doing that at all. You pick some random rank and some other random suit and construct a card from that. The concept of a "deck" doesn't exist at all in your code. You merely have it implicitely defined just by how your random number generation works.

From top-down design code, I'd expect to see some object representing the deck of cards and some mechanism to get a random card from it. E.g.

Deck cardDeck = new Deck();
Card randomCard = cardDeck.getRandomCard();

Documentation

The public documentation of the constructor parameters

* @param rnk The rank of the card
* @param sut The suit of the card

is not adding any new information, while the hidden comments of the variables do:

/* The card rank : Ace,2,3,4,5,6,7,8,9,10,Jack,Queen,King */
/* The card suit : Clubs, Diamonds, Hearts, Spades */

I wish I could see those when being clueless about how to invoke the constructor.

My personal opinion is that comments like

/* Instance variables */

and

/* Constructor */

do not add any value. If you want them or if you are forced to add them, you should be consistent though and also add

/* Public methods*/

to your classes.

Object oriented

rank and suit should be Enum Types. One part of object oriented philosophy is to create types in a way that they are helpful. A string is a string, but a Suit is not a Rank. Among many others, this has the benefit of preventing mistakes like the following from even compiling:

private Card selectCardFromDeckRandomly() {
    String rank = selectRandomCardSuit();
    String suit = selectRanomCardRank();
    return (new Card(rank, suit));  
}

Note: still not in a position to use arrays.

You actually are in exactly that position. In you case, limiting your code to not use arrays is what reduces your ability to improve it the most.

The use of enums leads to the use of their values() method (which returns an array), which allows you to get rid of the switches, resulting in more generic code.

Oh, and btw. isn't String [] args an array?

\$\endgroup\$
1
  • \$\begingroup\$ I can not thank you enough. your answer is very valuable and rich. \$\endgroup\$
    – ahmed dahy
    Commented Jul 20, 2018 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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