Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

I know that you said that you are aware of the recursive call to main being a bad thing, but I thought I should suggest an alternative anyway. The [game loop pattern][1]game loop pattern, where you have a initialize() then just a update(), can apply here, in a simple form. You can make Update() be in a while(notQuit) loop, which can contain all of your logic.

I'd definitely make Hand an ArrayList. They get more then 2 cards in blackjack, and the cards can have different values depending on how the hand is (Ace can be high or low). The rest of my issues with this class is summarised with make hand an arrayList. Resizing arrays is expensive and adding 1 element to a list is less so. [1]: http://gameprogrammingpatterns.com/game-loop.html

I know that you said that you are aware of the recursive call to main being a bad thing, but I thought I should suggest an alternative anyway. The [game loop pattern][1], where you have a initialize() then just a update(), can apply here, in a simple form. You can make Update() be in a while(notQuit) loop, which can contain all of your logic.

I'd definitely make Hand an ArrayList. They get more then 2 cards in blackjack, and the cards can have different values depending on how the hand is (Ace can be high or low). The rest of my issues with this class is summarised with make hand an arrayList. Resizing arrays is expensive and adding 1 element to a list is less so. [1]: http://gameprogrammingpatterns.com/game-loop.html

I know that you said that you are aware of the recursive call to main being a bad thing, but I thought I should suggest an alternative anyway. The game loop pattern, where you have a initialize() then just a update(), can apply here, in a simple form. You can make Update() be in a while(notQuit) loop, which can contain all of your logic.

I'd definitely make Hand an ArrayList. They get more then 2 cards in blackjack, and the cards can have different values depending on how the hand is (Ace can be high or low). The rest of my issues with this class is summarised with make hand an arrayList. Resizing arrays is expensive and adding 1 element to a list is less so.

deleted 137 characters in body
Source Link
Yann
  • 2.2k
  • 1
  • 17
  • 36

I would make the dealer in location 0 of the players array, for reasonsas this means that will hopefully become clear lateryou can pass around your array of players, and rely on the fact that no matter how large the array is, player[0] is the dealer. As well as this, you're duplicating data by both creating a Player user and a Dealer dealer as well as storing that in the array. I'd just store it in the array.

As well as this, moving all of your initialization logic to a single method might be a good idea. This can include your welcome text.

Since all that you are doing with the output of your whoWon method is printing out the name, returning a whole Player object is a little wasteful, and you might want to consider returning just a string containing their name.

I would make the dealer in location 0 of the players array, for reasons that will hopefully become clear later. As well as this, you're duplicating data by both creating a Player user and a Dealer dealer as well as storing that in the array. I'd just store it in the array.

As well as this, moving all of your initialization logic to a single method might be a good idea. This can include your welcome text.

Since all that you are doing with the output of your whoWon method is printing out the name, returning a whole Player object is a little wasteful, and you might want to consider returning just a string containing their name.

I would make the dealer in location 0 of the players array, as this means that you can pass around your array of players, and rely on the fact that no matter how large the array is, player[0] is the dealer. As well as this, you're duplicating data by both creating a Player user and a Dealer dealer as well as storing that in the array. I'd just store it in the array.

As well as this, moving all of your initialization logic to a single method might be a good idea. This can include your welcome text.

Source Link
Yann
  • 2.2k
  • 1
  • 17
  • 36

Right, I'll take things in the order that they appear, as they occur to me.

BlackJackGame class

I would make the dealer in location 0 of the players array, for reasons that will hopefully become clear later. As well as this, you're duplicating data by both creating a Player user and a Dealer dealer as well as storing that in the array. I'd just store it in the array.

As a slight aside, I'd make it an ArrayList, but if you're trying to just use simple data types, I understand why not, but it's something to consider if you're planning to allow more than 1 player in the future.

As well as this, moving all of your initialization logic to a single method might be a good idea. This can include your welcome text.

Since all that you are doing with the output of your whoWon method is printing out the name, returning a whole Player object is a little wasteful, and you might want to consider returning just a string containing their name.

I know that you said that you are aware of the recursive call to main being a bad thing, but I thought I should suggest an alternative anyway. The [game loop pattern][1], where you have a initialize() then just a update(), can apply here, in a simple form. You can make Update() be in a while(notQuit) loop, which can contain all of your logic.

PlayerTurn() has quite a few responsibilites, which is something that you don't really want out of a function. I'd pass it just one player, the one who's turn that it is, and return without printing out anything other than what it needs to (the score after the player has taken a card), as it shouldn't have any responsibility over reporting who won. Checking that should be under your whoWon method, and printing it should be controlled by another method.

In goAgain, you're checking the boolean against null. Don't do that, you've got a lovely true/false that you can check against, use it! This is how I'd write that method:

private boolean goAgain(Scanner sc){
 while(true){
 System.out.println("Would you like to play another round? \"yes\"/\"no\": ");
 String input = sc.nextLine();
 String answer = input.toLowerCase();
 switch(answer){
 case "y":
 case "yes":
 return true;
 case "n":
 case "no":
 return false;
 default:
 System.out.println("Invalid answer. Please answer only \"yes\" or \"no\"");
 break;
 }
 System.out.println();
 }
}

Or something similar.

Card class

It's functional, it works, and if you're going for simple, this is it. However, it's not very flexible, and it doesn't involve the notion of a deck, so it's possible to get 5 2s in a row. Whether you want to do that is up to you, but there's plenty of card implementations on this site that'll give you pointers.

Player interface

I'd possibly have a getName(). I know that you've overridden toString(), but that's not the first thing that I'd look for if I were using this interface.

Dealer and Player class

I'd definitely make Hand an ArrayList. They get more then 2 cards in blackjack, and the cards can have different values depending on how the hand is (Ace can be high or low). The rest of my issues with this class is summarised with make hand an arrayList. Resizing arrays is expensive and adding 1 element to a list is less so. [1]: http://gameprogrammingpatterns.com/game-loop.html

lang-java

AltStyle によって変換されたページ (->オリジナル) /