Choose random card from a deck

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
1
down vote

favorite












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






share|improve this question





















  • 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.
    – ahmed dahy
    Jul 18 at 19:58










  • The parenthesis that you added in Rev 3.
    – 200_success
    Jul 18 at 20:36










  • 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.
    – Martin Spamer
    Jul 19 at 18:21

















up vote
1
down vote

favorite












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






share|improve this question





















  • 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.
    – ahmed dahy
    Jul 18 at 19:58










  • The parenthesis that you added in Rev 3.
    – 200_success
    Jul 18 at 20:36










  • 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.
    – Martin Spamer
    Jul 19 at 18:21













up vote
1
down vote

favorite









up vote
1
down vote

favorite











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






share|improve this question













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








share|improve this question












share|improve this question




share|improve this question








edited Jul 18 at 20:37









200_success

123k14143399




123k14143399









asked Jul 18 at 18:06









ahmed dahy

264




264











  • 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.
    – ahmed dahy
    Jul 18 at 19:58










  • The parenthesis that you added in Rev 3.
    – 200_success
    Jul 18 at 20:36










  • 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.
    – Martin Spamer
    Jul 19 at 18:21

















  • 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.
    – ahmed dahy
    Jul 18 at 19:58










  • The parenthesis that you added in Rev 3.
    – 200_success
    Jul 18 at 20:36










  • 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.
    – Martin Spamer
    Jul 19 at 18:21
















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.
– ahmed dahy
Jul 18 at 19:58




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.
– ahmed dahy
Jul 18 at 19:58












The parenthesis that you added in Rev 3.
– 200_success
Jul 18 at 20:36




The parenthesis that you added in Rev 3.
– 200_success
Jul 18 at 20:36












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.
– Martin Spamer
Jul 19 at 18:21





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.
– Martin Spamer
Jul 19 at 18:21











1 Answer
1






active

oldest

votes

















up vote
2
down vote



accepted










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?






share|improve this answer





















  • I can not thank you enough. your answer is very valuable and rich.
    – ahmed dahy
    Jul 20 at 19:39










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f199765%2fchoose-random-card-from-a-deck%23new-answer', 'question_page');

);

Post as a guest






























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
2
down vote



accepted










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?






share|improve this answer





















  • I can not thank you enough. your answer is very valuable and rich.
    – ahmed dahy
    Jul 20 at 19:39














up vote
2
down vote



accepted










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?






share|improve this answer





















  • I can not thank you enough. your answer is very valuable and rich.
    – ahmed dahy
    Jul 20 at 19:39












up vote
2
down vote



accepted







up vote
2
down vote



accepted






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?






share|improve this answer













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?







share|improve this answer













share|improve this answer



share|improve this answer











answered Jul 18 at 21:48









I'll add comments tomorrow

2,010418




2,010418











  • I can not thank you enough. your answer is very valuable and rich.
    – ahmed dahy
    Jul 20 at 19:39
















  • I can not thank you enough. your answer is very valuable and rich.
    – ahmed dahy
    Jul 20 at 19:39















I can not thank you enough. your answer is very valuable and rich.
– ahmed dahy
Jul 20 at 19:39




I can not thank you enough. your answer is very valuable and rich.
– ahmed dahy
Jul 20 at 19:39












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f199765%2fchoose-random-card-from-a-deck%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Python Lists

Aion

JavaScript Array Iteration Methods