harry0192 / A01-2023-CMP1903

0 stars 0 forks source link

Code Review: Umeyyan Hawley, 26794347 #2

Closed Umeyyan closed 1 year ago

Umeyyan commented 1 year ago

Your Pack constructor method could use some data validation to ensure that the inputs don't go beyond the scope of the project (ie more than 13 for the value or more than 4 for the suit, and less than 1 for both) this can be done using if statements.

I really like that you've written code to change the values of the card for 1, 11, 12, and 13 into ace, jack, queen, and king respectively. However I feel like this could be done in few lines of code like this:

if (value == 1) { valueStr = "Ace"; } else if (value == 11) { valueStr = "Jack"; } else if (value == 12) { valueStr = "Queen"; } else if (value == 11) { valueStr = "Jack"; } else if (value == 13) { valueStr = "King"; } else { valueStr = value; }

that's just an idea to simplify your code into fewer lines however your code should run perfectly.

The rest of your code all seems okay, however I think it could use more comments to make it easier for external viewers to understand and follow. Your shuffleCardPack method needs some work as it doesn't actually shuffle the deck List. It just states how the cards will be shuffled but doesn't shuffle them.

Your are missing the testing class as specified in the assessment brief.

Overall I think this is a strong attempt at the assessment but definitely needs some work.