8
\$\begingroup\$

I would like some feedback on my code, I'm new to coding with C# but I do have some knowledge on Lua and Python. Is there anything that I need to change/clean up to make it more simplified?

using System;
using System.Linq;

class MainClass {
  public static void Main () {
    int[] cards = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10};

    Console.WriteLine("Welcome to Blackjack. Here are your draws.");
            
    Random drawCard = new Random();
    int draw1 = drawCard.Next(cards.Min(), cards.Max());
    int draw2 = drawCard.Next(cards.Min(), cards.Max());

    Console.WriteLine("You recieved a " + draw1 + " card!");
    Console.WriteLine("You recieved a " + draw2 + " card!");
            
    int sum1 = draw1 + draw2;

    if (sum1 == 21) //Blackjack Ending
    {
      Console.WriteLine("Congratulations! You got " + sum1 + "!");
    }


    else if (sum1 >= 11) //Choice of 3rd draw 
    {
      Console.WriteLine("Is " + sum1 + " enough?");
      bool cont1 = false;  

      drawChoice(cont1); //Call the draw choice function


      if (cont1 == true)
      {
        int draw3 = drawCard.Next(cards.Min(), cards.Max());
        Console.WriteLine("You drawed a " + draw3 + " card!");
        
        int sum2 = draw3 + sum1;
        Console.WriteLine("You have a total of " + sum2 + ".");

        if (sum2 > 21) Console.WriteLine("Game Over!");
      }


      else //NPC's turn starts
      {

      }
    }


    else //Player has less than 11 cards, auto draw
    {
      Console.WriteLine("You have a total of " + sum1 + ".");
      Console.WriteLine("You will be forced to draw another card.");

      int draw3 = drawCard.Next(cards.Min(), cards.Max());
      Console.WriteLine("You drawed a " + draw3 + " card!");

      int sum2 = draw3 + sum1;
      Console.WriteLine("You have a total of " + sum2 + ".");
    }
  }


  static void drawChoice(bool contChoice) //Function for player to choose whether to draw 
  {
    Console.WriteLine("Would you like to draw another card? Y/N");       
    string choice1 = Console.ReadLine();


    if (choice1 == "Y" || choice1 == "y")
    {
      contChoice = true;
      Console.WriteLine(contChoice);
    }


    else if (choice1 == "N" || choice1 == "n")
    {
      contChoice = false;
      Console.WriteLine(contChoice);
    }
  }
}
\$\endgroup\$
4
  • \$\begingroup\$ Sure thing, I'll edit it now. There are some code blocks that are unfinished but that's where I'll stop for now. \$\endgroup\$
    – Willn't
    Commented Aug 18, 2020 at 6:52
  • 5
    \$\begingroup\$ Welcome to Code Review! Don't touch the code now answers are coming in, but be careful with removing lines of code. A lot of questions are closed because they are lacking the much-needed context of the program, since code is unnecessarily hard to review without it and the answers may miss the mark for the actual program if the posted code is a modified version. Just something to keep in mind when posting your next question. For the full guide, please see our FAQ on asking questions if you're interested. \$\endgroup\$
    – Mast
    Commented Aug 18, 2020 at 7:55
  • 2
    \$\begingroup\$ Please do not modify the code after the question has been answered. See our help center. \$\endgroup\$
    – pacmaninbw
    Commented Aug 18, 2020 at 12:48
  • 1
    \$\begingroup\$ You spelled "received" wrong \$\endgroup\$ Commented Aug 19, 2020 at 14:01

3 Answers 3

6
\$\begingroup\$

I've actually made a similar project myself, it's really good fun!

1. Keeping track of scores

First thing I noticed is that you keep track of the sum of the values for the player and presumably also for the dealer, but the score of a hand can actually change dramatically while playing.

1.1. Soft totals in blackjack

Blackjack has this concept called soft totals, which means the total sum can have a different value depending on if there's an ace or not. For example, if a player has an ace (1) and a 7, that actually counts as 18 (11 + 7). But if that same player draws another 7, their total will be 15 (1 + 7 + 7). The value of the ace changes as you draw more cards, so you'll have an easier time keeping scores if you somehow keep the cards separated from each other. This can be done using collections.

1.2. Collections

Collections are things like arrays (which you've already used), lists and dictionaries. A list of integers is a good candidate to represent a player's (or the dealer's) current collection of cards, because they can change sizes without complaining. Moreover, lists have built-in functions for getting sum of all the numbers inside them, the minimum and the maximum values of the list and plenty more. You can use those built-in functions to your advantage, as well as the fact that you always know the numbers inside the list to determine if someone's total is soft or not.

2. Game loop

The game of blackjack is played with a finite number of cards in real life, but of course in code you don't have to worry about that. If an (un)lucky player keeps drawing aces, they'll eventually still hit 21 and end the round. However, since you can't predict when a player (or the dealer) is going to lose, you can use something called a game loop. The game loop for blackjack starts executing all the game logic once a player or the dealer confirms that they want to draw a card, and once it's done executing, it'll ask to repeat if necessary.

2.1. Conditions for choosing another card

You'll notice that the player has a lot of freedom in blackjack, they can keep drawing cards until they get 21 or go over, at which point the round ends for them. The dealer, however, doesn't have that freedom. You can find more information on that online.

Either way, if you think about it, both the player and the dealer at some point make the decision to either draw or not to draw another card. You do a check for "Y" or "N" when given an input, which makes sense.

2.2. Translating text input to true or false

Your DrawChoice method alters a bool depending on the player's input, but you could also refactor that method so that it receives a string and returns a bool. That way, you can directly translate the user's input to true (yes, give me another card) or false (no, I don't want another card). It could look something like this:

// One option
public static bool DrawChoice(string input)
{
    if (input == "Y" || input == "y") // You could also use input.ToLower() == "y"
    {
        return true;
    }
    else // If it's not "Y" or "y", it's gonna be "N" or "n"
    {
        return false;
    }

//  else if (input == "N" || input == "n")
//  {
//      return false;
//  }
}

// Alternative one-liner
public static bool DrawChoice2(string input) => input.ToLower() == "y";

Going back to the idea of a game loop, you now have a condition that dictates whether the game loop continues or not. One possible implementation would be this:

string choice = Console.ReadLine();
while (DrawChoice(choice)) // No need to write "== true" or "== false"
{
    // Stuff that happens if a player or the dealer draws another card
    choice = Console.ReadLine() // Ask again once the game logic has executed
}
// Stuff that happens when the loop ends

2.3. Drawing cards

Blackjack is a card game, therefore you'll be drawing cards a lot, be it for a player or for the dealer. If something happens often in the game, it's generally a good idea to make it into a method so you don't have to write the same logic in different places.

Your current implementation draws a random value between the minimum and the maximum of your array of cards. From the documentation, we learn the following:

Next(Int32 minValue, Int32 maxValue)

A 32-bit signed integer greater than or equal to minValue and less than maxValue; that is, the range of return values includes minValue but not maxValue. If minValueequals maxValue, minValueis returned.

So when you write Next(1, 10) (from min and max), you will at most get a 9. Another issue is that even if you fix the implementation to Next(1, 11), you will have equal probabilities to get any value from 1 through 10. But since there's multiple cards in the deck that have the value of 10, they should show up more often than non-10 cards.

Fortunately, your array already has the correct distribution of cards, so instead you could generate a valid random position to get the corresponding value from your array.

At the end of the day, what you'll end up with will look something like this:

public static int DrawCard()
{
   int[] cards = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10 };
   int card;
   // Implement random draw here
   return card;
}

And then you could even do something like this to repeatedly draw cards:

// You could also use a list, which is more flexible
public static int[] DrawCard(int count)
{
    int[] drawn = new int[count];
    for (int i = 0; i < count; i++)
    {
        drawn[i] = DrawCard();
    }
    return drawn;
}

Hope this helps! Good luck and have fun!

\$\endgroup\$
3
  • \$\begingroup\$ This is really high quality, and really detailed. I'll go through it slowly and take my time as I read through it. But one question. For section 2.2, instead of using an else if statement and subbing it with else, wouldn't the user be able to enter anything else other than specifically N? (not that it is necessary, just clarifying.) \$\endgroup\$
    – Willn't
    Commented Aug 18, 2020 at 15:33
  • 1
    \$\begingroup\$ That's certainly a possibility, but even if you allow different inputs, you would only care about the ones that return true to keep the game loop going, right? So in that case I'd suggest you keep adding ||'s in the branch that returns true rather than adding them as else ifs. You could also invert that logic and define which inputs should specifically return false, and return true for any other input. \$\endgroup\$ Commented Aug 18, 2020 at 15:36
  • 1
    \$\begingroup\$ Oh my bad, I had misinterpreted your question. They can still input anything they'd like, because the if/else statement checks what they have typed, not what they might type. The method will only return true if the user typed "Y" or "y". \$\endgroup\$ Commented Aug 18, 2020 at 15:55
9
\$\begingroup\$

Just a few notes so you can improve your code:

int sum1 = draw1 + draw2;

if (sum1 == 21) //Blackjack Ending
{
  Console.WriteLine("Congratulations! You got " + sum1 + "!");
}

This won't ever be true, because

  • you have nowhere in the cards an ace meaning having a possible value of eleven.
  • the maxValue in Random.Next(int minValue, int maxValue) is the exclusive upper bound of the random number returned.

In void drawChoice(bool contChoice) the method argument contChoice is a value type. You can't modify it like you think. After leaving the method regardless wether the user typed y or n the value of cont1 is still false. You should change the method signature to have no arguments but to return a bool.

if (cont1 == true)  

because cont1 already is a bool you won't need to compare it with a bool. You can simply use it as condition like if (cont1) and if you would need to check wether a bool variable is false you would use if (!cont1).

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your answer! 1. I am aware of the if statement not being true, I plan to modify the array variable to define what the Ace card would be in integer values. 2. What I originally planned was to have Main send the argument of cont1 to drawChoice and the variable would be updated and has the data of what the user entered. How shall I execute that, and how can I leave cont1 to be undefined? I will be adding another few more lines of code that would respond to the user if they did not enter Y nor N. \$\endgroup\$
    – Willn't
    Commented Aug 18, 2020 at 12:03
  • 2
    \$\begingroup\$ You wouldn't need cont1 anymore. You would just chang your if condition like so if (drawChoice()) \$\endgroup\$
    – Heslacher
    Commented Aug 18, 2020 at 12:10
3
\$\begingroup\$

I'm not sure how helpful I can be here, but IMHO you need to have a full fledged code review with someone talking you through the code. There's a bunch "wrong" here, but having said that I'm wondering what your aim is with C# - are you just scripting something quick out to learn the syntax, or are you wanting to better understand how to architect things in C#.

If someone came to me with this in a professional setting the first thing I'd tell them: "You need to be able to test this". I would then talk about TDD, and really try to point out how this code is structured to do way too much. There's no abstractions, and everything is very procedural. Every class should do 1 thing. People I'm sure will argue with me on this point. Either way, the fact that you only have 1 class is bad.

Past that, for just general "2 minute quick C# tips". I'd use enumerations for the cards so instead of 10, 10, 10, I'd have 10, Jack, Queen, Ace. Use var instead. if (cont1 == true) is the same as if(cont1). Use better named variables: Just looking at cont1 I have no idea what that means.

JansthcirlU also brought up collections. Looking at this code, it doesn't look like you understand them so I would really focus here as well.

Keep coding :)

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Yeah, I mainly brought up points that seemed useful in the long run, since I've run into plenty of roadblocks while doing my own blackjack project. Obviously you can't expect someone to 100% comprehend a language from a StackExchange answer. \$\endgroup\$ Commented Aug 18, 2020 at 19:14
  • \$\begingroup\$ My aim in C# is to just use my basic knowledge from other coding languages to program a project and to know the syntax of C#. Though I only know the basic stuff like variables and if..else statements, things like enumerations and collections is something I've never heard of before so that can be helpful if I would want to use it in another project. \$\endgroup\$
    – Willn't
    Commented Aug 19, 2020 at 5:30
  • \$\begingroup\$ So my advice to you would be to pick one language, and then actually make something complex with it, like a game or application with a complete UI. Enums are in most and collections (or arrays) are used in every programming language there is. Instead of just learning a language become proficient in using it, don't switch around a bunch until you feel like you really understand what's going on. For me, it's trivial to switch to other languages because I understand the concepts behind everything, I just make a mental map to use whatever syntax the current language I'm in needs. \$\endgroup\$
    – rball
    Commented Aug 20, 2020 at 21:16
  • \$\begingroup\$ I'll go one further with explaining the SRP classes that I would start with off the top of my head: CardScoreCalculator (takes in array of cards, returns score array), DeckManager (what cards have been played, and still in the deck), GameManager (keeps array of all players, who's turn is it, what action did they take, who wins), Card (Friendly name, Values), Player (Current cards, their state, available actions, if this is a dealer). I would probalby employ a strategy pattern to Player as well... :P \$\endgroup\$
    – rball
    Commented Aug 20, 2020 at 21:28

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.