3
\$\begingroup\$

This is a hangman game. The only word is 'word' and I am not planning on changing this since I am not going to publish this game or anything.

If there is any unnecessary/bad code I need to get rid of, please tell me.

Here is my code:

using System;
using System.Threading;

namespace Hangman
{
    class Program
    {
        static void Restart()
        {
            try
            {
                Console.Write("restart? (y/n): ");

                char restart_input = Convert.ToChar(Console.ReadLine().ToLower());

                if(restart_input == 'y')
                {
                    Console.WriteLine("----------");
                    Main();
                }
                
                else if (restart_input == 'n')
                {
                    Console.Write("\nPress any key to exit");
                    Console.ReadKey();
                }
                
                else if (restart_input != 'y' && restart_input != 'n')
                {
                    Console.WriteLine("\ninvalid input");
                    Restart();
                }

            }

            catch(Exception e)
            {
                Console.Write("\n" + e);
                Restart();
            }

        }

        static void Main()
        {
            Console.Title = "Hangman";

            int i = 0;
            int j = 1;

            bool has_guessed_the_letter = false;

            string word = "word";

            int failed_attempts = 0;
            int score = 0;

            Random random = new Random();
            int random_int = random.Next(0, 2);

            char[] char_array = word.ToCharArray();
            char?[] hidden_letters = new char?[i];

            foreach (char letter in char_array)
            {
                switch (random_int)
                {
                    case 1:
                        Console.Write(letter);
                        Thread.Sleep(200);
                        random_int = random.Next(0, 2);
                        break;

                    case 0:
                        Console.Write("_");
                        Thread.Sleep(200);
                        random_int = random.Next(0, 2);
                        Array.Resize(ref hidden_letters, j);
                        hidden_letters[i] = letter;
                        i++;
                        j++;
                        break;
                }
            }

            if (hidden_letters.Length == 0)
            {
                Console.Write("\nthere are no hidden letters, lucky :)");
            }
            
            else if (hidden_letters.Length == word.Length)
            {
                Console.Write("\nall letters are hidden, no luck :(");
            }
            
            else
            {           
                try
                {
                    label:

                    i = 0;
                    j = 1;

                    has_guessed_the_letter = false;

                    Console.Write("\n\ninput: ");

                    char user_input = Convert.ToChar(Console.ReadLine());

                    while (has_guessed_the_letter == false && j <= hidden_letters.Length)
                    {
                        if (user_input == hidden_letters[i])
                        {
                            has_guessed_the_letter = true;
                            hidden_letters[i] = null;
                        }

                        else
                        {
                            i++; j++;
                        }

                    }

                    if (has_guessed_the_letter == true)
                    {
                        Console.WriteLine("Congratulations! " + j + ". hidden letter was " + user_input);
                        score++;                             
                        if (score == hidden_letters.Length)
                        {
                            Console.Write("\nCongratulations! the hidden word was: " + word);
                        }
                        else
                        {
                            Console.WriteLine("score: " + score + " (must reach: " + hidden_letters.Length + ")");
                            Console.Write("failed attempts: " + failed_attempts + " (failed attempts left: " + ((word.Length * 2) - failed_attempts) + ")");
                            goto label;
                        }
                    }
                    else
                    {
                        Console.WriteLine("failed attempt: " + user_input);
                        failed_attempts++;
                        if (failed_attempts == (word.Length * 2))
                        {
                            Console.Write("\ngame over. the hidden word was: " + word);
                        }
                        else
                        {
                            Console.WriteLine("score: " + score + " (must reach: " + hidden_letters.Length + ")");
                            Console.Write("failed attempts: " + failed_attempts + " (failed attempts left: " + ((word.Length * 2) - failed_attempts) + ")");
                            goto label;
                        }
                    }
                }

                catch(Exception e)
                {
                    Console.Write("\n" + e);
                }                
            }
            Console.WriteLine("\n----------");
            Restart();
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

There are a few more things than I want to go into at this time - but there are two Big Ones™ I want to address. One is the goto and the other is the recursion in the Restart method. Eventually, the recursion will blow the stack. And the goto is just bad form past 1985. Here's a reworked version that addresses both those items (plus a little thread-safety around Random for good measure):

namespace Hangman
{
    using System;
    using System.Threading;

    internal static class Program
    {
        private static readonly Random _Random = new ();

        private enum ShouldRestart
        {
            Invalid,

            No,

            Yes
        }

        private static ShouldRestart Restart()
        {
            Console.Write("restart? (y/n): ");

            char restartInput = Convert.ToChar(Console.ReadLine()?.ToLower() ?? string.Empty);

            if (restartInput == 'y')
            {
                Console.WriteLine("----------");
                return ShouldRestart.Yes;
            }

            if (restartInput == 'n')
            {
                Console.Write("\nPress any key to exit");
                Console.ReadKey();
                return ShouldRestart.No;
            }

            Console.WriteLine("\ninvalid input");
            return ShouldRestart.Invalid;
        }

        private static void Main()
        {
            Console.Title = "Hangman";

            while (true)
            {
                int i = 0;
                int j = 1;

                const string Word = "word";

                int failedAttempts = 0;
                int score = 0;
                int randomInt;

                lock (_Random)
                {
                    randomInt = _Random.Next(0, 2);
                }

                char[] charArray = Word.ToCharArray();
                char?[] hiddenLetters = new char?[i];

                foreach (char letter in charArray)
                {
                    switch (randomInt)
                    {
                        case 1:
                            Console.Write(letter);
                            Thread.Sleep(200);
                            lock (_Random)
                            {
                                randomInt = _Random.Next(0, 2);
                            }

                            break;

                        case 0:
                            Console.Write("_");
                            Thread.Sleep(200);
                            lock (_Random)
                            {
                                randomInt = _Random.Next(0, 2);
                            }

                            Array.Resize(ref hiddenLetters, j);
                            hiddenLetters[i] = letter;
                            i++;
                            j++;
                            break;
                    }
                }

                if (hiddenLetters.Length == 0)
                {
                    Console.Write("\nthere are no hidden letters, lucky :)");
                }
                else if (hiddenLetters.Length == Word.Length)
                {
                    Console.Write("\nall letters are hidden, no luck :(");
                }
                else
                {
                    while (true)
                    {
                        i = 0;
                        j = 1;

                        bool hasGuessedTheLetter = false;

                        Console.Write("\n\ninput: ");

                        char userInput = Convert.ToChar(Console.ReadLine() ?? string.Empty);

                        while (!hasGuessedTheLetter && j <= hiddenLetters.Length)
                        {
                            if (userInput == hiddenLetters[i])
                            {
                                hasGuessedTheLetter = true;
                                hiddenLetters[i] = null;
                            }
                            else
                            {
                                i++;
                                j++;
                            }
                        }

                        if (hasGuessedTheLetter)
                        {
                            Console.WriteLine("Congratulations! " + j + ". hidden letter was " + userInput);
                            score++;
                            if (score == hiddenLetters.Length)
                            {
                                Console.Write("\nCongratulations! the hidden word was: " + Word);
                                break;
                            }

                            Console.WriteLine("score: " + score + " (must reach: " + hiddenLetters.Length + ")");
                            Console.Write("failed attempts: " + failedAttempts + " (failed attempts left: " + ((Word.Length * 2) - failedAttempts) + ")");
                        }
                        else
                        {
                            Console.WriteLine("failed attempt: " + userInput);
                            failedAttempts++;
                            if (failedAttempts == (Word.Length * 2))
                            {
                                Console.Write("\ngame over. the hidden word was: " + Word);
                                break;
                            }

                            Console.WriteLine("score: " + score + " (must reach: " + hiddenLetters.Length + ")");
                            Console.Write("failed attempts: " + failedAttempts + " (failed attempts left: " + ((Word.Length * 2) - failedAttempts) + ")");
                        }
                    }
                }

                Console.WriteLine("\n----------");

                ShouldRestart shouldRestart = ShouldRestart.Invalid;

                while (shouldRestart == ShouldRestart.Invalid)
                {
                    shouldRestart = Restart();
                }

                if (shouldRestart == ShouldRestart.No)
                {
                    break;
                }
            }
        }
    }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ ConsoleKey restartInput = Console.ReadKey().Key; Console.WriteLine(); if (restartInput == ConsoleKey.Y) { restart } else { exit } - can work more friendly to user. \$\endgroup\$
    – aepot
    Jul 14 at 18:06
  • 2
    \$\begingroup\$ lock for Random is a kind of redundancy on a single thread, and it looks weird. Btw, if you need a cool thread-safety, try [ThreadStatic] above Random definition line instead of lock. \$\endgroup\$
    – aepot
    Jul 14 at 18:09
  • \$\begingroup\$ sir. this is the most confusing C# code I have readen in my life \$\endgroup\$ Jul 15 at 13:51
  • \$\begingroup\$ @user13988674 the changes from your code are so very minimal, I'm genuinely surprised by that comment. \$\endgroup\$ Jul 15 at 14:46
  • \$\begingroup\$ but I am just a beginner \$\endgroup\$ Jul 16 at 22:38
1
\$\begingroup\$

Right out of the gate, I would introduce OOP to this monolithic code, but I am unsure whether you are at the right stage of learning to develop to be ready for OOP, because it notably changes how you structure your code. It would also require an entire explanation of OOP principles, which is out of scope for the question at hand and is much better serviced by looking up OOP tutorials.

Instead, for this answer I'm going to focus on smaller things in your code, in no particular order.

If you're looking for overall advice on your code, starting to use OOP is a BIG improvement you could make, and eventually you will have to learn it if you want to develop in C#.


Recursion

What you've used here is a recursive pattern. Main calls Restart, which calls Main, which calls Restart, which ...

There is a time and a place to use recursion, but this is not one of them. The problem with recursion is that the call stack is finite, and with each recursive step, you drill deeper into the call stack. Eventually, if you player keeps replaying the game, your code will crash because the call stack has grown too large, leading to a StackOverflowException.

This would've been better expressed using an iteration, because this is not susceptible to stack overflows. There are three main iteration types: for, while and do while.
You don't know in advance how often you'll repeat, so for is not right. Because you want to run your game at least once, a do while makes more sense than a while here.

Your game logic can be repeated using:

bool shouldRestart;

do
{
    shouldRestart = false;

    // Play the game

    Console.Write("restart? (y/n): ");
    var restartInput = Console.ReadKey().KeyChar;

    shouldRestart = (restartInput.ToLower() == 'y');
} while(shouldRestart);

Game logic

You've used way too much if/else nesting in your logic for the code to be easily readable. Rather than tackle each individual thing you did, which requires a back and forth in understanding why you did it this way, I'm going to opt to rebuilt the logic from scratch, to give you an idea how you could've approached this without letting it devolve into such a branched logic.

Random start

I'm going to skip this because it's not part of Hangman. The only real random here would be the word selection, which you're not doing. This can be added later, when the base logic has been established.

Score

Similarly, I'm omitting the concept of a score. The goal in Hangman is to guess the word in as many tries as possible, so the score can be directly derived from the remaining attempts left when you guess the whole word. I would argue that the remaining attempt count is the score.

Game state

To store the state of the game, we need a few different things. Your approach wasn't bad, but I want to make some adjustments because they will make it easier to update the game state during the game.

const int MaximumAttempts = 7;            // How many attempts a player has.
const string word = "word";               // The word to guess

var guessedLetters = new List<char>();    // Guesses the user has made
int remainingAttempts = MaximumAttempts;  // How many attempts left in this game?

For the sake of simplicity, we'll assume that the configured word is in lowercase.

Printing the hidden word

We need to obfuscate the word. We should replace all characters with * unless it is a character that the player has already guessed. We'll make a nice method for this:

public static string Obfuscate(string word, List<char> visibleLetters)
{
    var characters = word.Distinct();
    string result = word;

    foreach(var character in characters)
    {
        bool isVisible = visibleLetters.Contains(character);

        if(!isVisible)
        {
            result = result.Replace(character, '*');
        }
    }
    
    return result;
}

Guessing

Now we want the player to guess a letter. For now, let's assume the player nicely inputs only lowercase letters. Validation can be written at a later stage.

When a guess is made, we add this guess to the list of guessed characters (no matter if correct). If the word does not contain the guessed character, we decrease the remaining attempts. If the word has been guessed or the remaining attempts are 0, the game ends. Otherwise, we repeat this process.

This, too, is an iterative process, and it also needs to run at least once, so another do while is warranted here.

bool gameWon  = false;
bool gameLost = false;
string obfuscatedWord = Obfuscate(word, guessedLetters);

do {
    Console.WriteLine(obfuscatedWord);
    Console.WriteLine($"{remainingAttempts} attempts remaining!");
    Console.Write("Guess a letter: ");

    char guess = Console.ReadKey().KeyChar;
    
    guessedLetters.Add(guess);

    if(!word.Contains(guessedLetter)) 
        remainingAttempts--;

    // Generate a freshly obfuscated word
    obfuscatedWord = Obfuscate(word, guessedLetters);

    // The game is won when there is no more obfuscation,
    // i.e. the obfuscated word is equal to the original word
    gameWon = obfuscatedWord.Equals(word);

    // The game is lost when there are no remaining attempts
    gameLost = (remainingAttempts == 0);
} while(!gameWon && !gameLost);

We can only break out of this loop by either winning or losing the game. In other words, the subsequent code can trust for a fact that one of the booleans is true and will indicate whether the game was won or lost.

if(gameWon)
    Console.WriteLine($"Victory! The word to guess was {word}!"); 

if(gameLost)
    Console.WriteLine($"Defeat! The word to guess was {word}!"); 

You could use an else here, because both booleans should never be true at the same time. However, I already know that there is no way for this logic to set both booleans to true at the same time, and I'd prefer to save myself the nesting logic because it detracts from code readability.

It's perfectly okay to add the else if you prefer, but you went overboard with the nesting in your original code, so I want to urge you to avoid nesting where you can. Nested ifs become really hard to follow.
I'll admit that I never even bothered to decode the branching logic you wrote, simply because it's just so hard to follow by reading it in a single pass. Don't worry about this, by the way. All developers learn to write working code (which you did) before they learn to write working and readable code. You'll get there :)

Bells and whistles

Note that I skipped a few bells and whistles, such as printing a list of guesses the player has already made, printing a list of available guesses, and printing the hangman himself. Those are all flourishes you can add yourself, which don't impact the core game logic.


Monolithic code

Your methods contained too much logic, and should be broken up into smaller submethods to make things easier to understand.

I already broke up your code somewhat in the two section above, but I intentionally left some things that can still be improved. I already mentioned it: we should really be validating the user's input, and force them to retry when they input something bad.

If we added this to the code directly, it would start distracting us from the core game logic. This is why we are going to put this logic in a submethod, so that we don't conflate the game logic with some nice-to-have validation.

Coincidentally enough, this again warrants an iteration, and do while is again the appropriate choice as you want the player to input a character at least once.

public const string AllowedCharacters = "abcdefghijklmnopqrstuvwxyz";

public static char MakeGuess(List<char> alreadyGuessed)
{
    char guess;
    bool characterIsInvalid = false;

    do {
        // Reset validation flag
        characterIsInvalid = false;
        
        Console.Write("Guess a letter: ");
        char guess = Console.ReadKey().KeyChar;

        // Ensure lower case
        guess = guess.ToLower();

        // Ensure it's a letter, otherwise repeat
        if(!AllowedCharacters.Contains(guess))
        {
            Console.WriteLine("Input is not a letter!");
            characterIsInvalid = true;
        }

        // Ensure this has not been guessed yet
        if(alreadyGuessed.Contains(guess))
        {
            Console.WriteLine("You already guessed this before!");
            characterIsInvalid = true;
        }

    } while(characterIsInvalid);

    return guess;        
}

We can be sure that the value returned by MakeGuess is valid. Our core game logic doesn't need to validate this input any further. All we need to do is change our game logic to rely on this method, instead of reading the input directly:

// ...

Console.WriteLine(obfuscatedWord);
Console.WriteLine($"{remainingAttempts} attempts remaining!");

char guess = MakeGuess(guessedLetters);
    
guessedLetters.Add(guess);

// ...

There are other submethods ripe for developing, but I will leave these as an exercise to you. To help you identify some of these:

  • The comment I made in the first snippet (// Play the game) should not be replaced by the game logic I wrote in a later snippet. Instead, the game logic should be in its own method, and // Play the game should be replaced with a call to that submethod.
  • string ChooseWord() (can return fixed value, or randomly chosen value from a list of words)
  • void PrintHangmanImage(int remainingAttempts) {}
  • void PrintVictory(string word, List<char> guessesMade, int remainingAttempts) {}
  • void PrintDefeat(string word, List<char> guessesMade) {}
  • void PrintAvailableGuesses(List<char> guessesMade) {}

Smaller feedback points

String concatenation

Console.WriteLine("score: " + score + " (must reach: " + hidden_letters.Length + ")");

Avoid + when concatenating strings. One + doesn't break your code but it's inefficient and will bring your application to its knees if you concatenate many values.

Better approaches include String.Format(); StringBuilder, and IMO the best default approach is string interpolation ($"score: {score} (must reach: {hidden_letters.Length})").

Goto

goto label;
                        

In short, never use goto. It's an archaic concept and only used as a crutch for badly designed logical flow. The better solution is to redesign your flow so that you don't need sudden jumps from one place to another.

I often leave room for exceptions for most (if not all) coding guidelines, but not for goto. It is that bad.

Naming

has_guessed_the_letter = false;
int failed_attempts = 0;
int random_int = random.Next(0, 2);
char[] char_array = word.ToCharArray();
char?[] hidden_letters = new char?[i];

Generally speaking, avoid underscores-as-spaces, and skip articles such as "the".

hasGuessedLetter = false;
int failedAttempts = 0;
char?[] hiddenLetters = new char?[i];

Additionally, avoid putting the type in the variable name (note that this advice becomes less relevant in OOP code, for different reasons).

int randomValue = random.Next(0, 2);
char[] characters = word.ToCharArray();

Resizing arrays

Array.Resize(ref hidden_letters, j);

Ever since the advent of LINQ, arrays and lists have both become rather easy to use, even interchangeably. However, lists have one clear advantage over arrays: they are dynamically sized. Arrays are fixed size.

When you notice you want to resize your array, instead rewrite your code to use a List<T> instead of a T[]. Hardly any of your code will need to be changed (lists also allow for indexed access), and lists have nice and easy methods to add/remove members.

Newlines

Console.Write("\nthere are no hidden letters, lucky :)");

This is a bit of a nitpick, but favor using Environment.NewLine over '\n', as a matter of readability.

Console.Write($"{Environment.NewLine}there are no hidden letters, lucky :)");

Alternatively, you could also using an empty WriteLine call:

Console.WriteLine();
Console.Write("there are no hidden letters, lucky :)");

In either case, it becomes slightly easier to notice that the message you're writing doesn't begin with nthere but rather there.

As I said, it'd a bit of a nitpick, but if you consistently use newline characters everywhere, it starts being a bit annoying.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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