Hangman Game in C#

Question

I've created a simple Hangman game (not finished yet).

• What could I improve? (This isn't intended to be object-oriented but suggestions on that are also welcome.)

• What are its flaws?

Here's the code:

public partial class Game : Form
{
    private ReadFromFile readFromFile;
    private Random random;
    private string word;
    private int wrongGuesses;
    private int amountOfLettersRevealed;
    private bool wordRevealed;
    private const int MAX_WRONG_GUESSES = 9;

    public Game()
    {
        InitializeComponent();
    }

    private void Game_Load(object sender, EventArgs e)
    {
        loadWords("words");
        selectRandomWord();
    }

    private void loadWords(string fileLocation)
    {
        readFromFile = new ReadFromFile(fileLocation);
        readFromFile.read();
    }

    private void selectRandomWord()
    {
        random = new Random();

        string[] words = readFromFile.lines;
        word = words[random.Next(words.Length)];
    }

    private void letter_Click(object sender, EventArgs e)
    {
        if (wordRevealed)
            return;

        Button button = (Button)sender;

        char letter = char.Parse(button.Name.Substring(6).ToLower());

        guessLetter(letter);
        buttonClicked(button);
    }

    private void guessLetter(char letter)
    {
        if (!word.Contains(letter))
        {
            // wrong guess
            wrongGuesses++;
        }
        else
        {
            // right guess
            for (int index = 0; index < word.Length; index++)
            {
                if (letter == word[index])
                {
                    revealLetter(index, letter);
                    amountOfLettersRevealed++;
                }
            }
        }
        if (amountOfLettersRevealed == word.Length)
        {
            wordRevealed = true;
            gameWon();
        }
        if (wrongGuesses == MAX_WRONG_GUESSES)
        {
            gameOver();
        }
    }

    private void buttonClicked(Button button)
    {
        button.Enabled = false;
        Image image = loadImage(button.Name.Substring(6) + "_HOV");
        button.BackgroundImage = image;
    }

    private void revealLetter(int index, char letter)
    {

    }

    private void gameWon()
    {
        playSound("youwin");
    }

    private void gameOver()
    {         
        playSound("gameover");
    }

    private Image loadImage(string location)
    {
        object o = Resources.ResourceManager.GetObject(location);
        return (Image) o;
    }

    private void playSound(string sound)
    {
        SoundPlayer sp = new SoundPlayer(sound + ".wav");
        sp.Play();
    }
}

Answer 1

private void loadWords(string fileLocation)

Follow standard naming conventions: private void LoadWords(string fileLocation) - same for the rest of your methods.

In private void guessLetter(char letter) there is the following code:

if (amountOfLettersRevealed == word.Length)
{
    wordRevealed = true;
    gameWon();
}
if (wrongGuesses == MAX_WRONG_GUESSES)
{
    gameOver();
}

Presumably if amountOfLettersRevealed == word.Length then wrongGuesses != MAX_WRONG_GUESSES? In that case it seems to make more sense to use else if, rather than evaluating the second expression even if the first is true. (Why would you call gameWon() and then check if you lost too?)

Also I think that code should not be in that method at all. The method is called guessLetter. It should handle the logic of guessing the letter. Checking whether the player has won or lost is not "guessing the letter".

Also it seems strange to have the letter_Click call a buttonClicked method. Initially I thought that was the button's click event handler due to the similarity between this method's name and the default name for a click event handler for a nameless button. I would maybe call that method UpdateButtonBackground or whatever that describes what that method actually does, since it's not an event handler.

Answer 2

In short methods like this one:

private Image loadImage(string location)
{
    object o = Resources.ResourceManager.GetObject(location);
    return (Image) o;
}

I would just do this:

private Image loadImage(string location)
{
    return (Image)Resources.ResourceManager.GetObject(location);
}

But it looks like you are only calling this method once, so there is really no need to have a separate method for this.

You should probably also implement the RevealLetter method. I also noticed that there is no logic for showing a hidden word to the user (like how many letter are in the word to guess. Is that intentional? it might be nice to have that for the user.