11
\$\begingroup\$

Does this code follow standard conventions, not redundant? How I can make it more comprehensible for other people?

It's an exercise from the Java introductory book:

Write a hangman game that randomly generates a word and prompts the user to guess one letter at a time. Each letter in the word is displayed as an asterisk. When the user makes a correct guess, the actual letter is then displayed. When the user finishes a word, display the number of misses.

My program:

import java.util.Arrays;
import java.util.Scanner;

public class Hangman{
    public static void main(String[] args) {
        String[] words = {"writer", "that", "program"};
        // Pick random index of words array
        int randomWordNumber = (int) (Math.random() * words.length);
        // Create an array to store already entered letters
        char[] enteredLetters = new char[words[randomWordNumber].length()];
        int triesCount = 0;
        boolean wordIsGuessed = false;
        do {
        // infinitely iterate through cycle as long as enterLetter returns true
        // if enterLetter returns false that means user guessed all the letters
        // in the word e. g. no asterisks were printed by printWord
        switch (enterLetter(words[randomWordNumber], enteredLetters)) {
            case 0:
                triesCount++;
                break;
            case 1:
                triesCount++;
                break;
            case 2:
                break;
            case 3:
                wordIsGuessed = true;
                break;
        }
        } while (! wordIsGuessed);
        System.out.println("\nThe word is " + words[randomWordNumber] +
            " You missed " + (triesCount -findEmptyPosition(enteredLetters)) +
            " time(s)");
    }

    /* Hint user to enter a guess letter,
    returns 0 if letter entered is not in the word (counts as try),
    returns 1 if letter were entered 1st time (counts as try),
    returns 2 if already guessed letter was REentered,
    returns 3 if all letters were guessed */
    public static int enterLetter(String word, char[] enteredLetters)    {
        System.out.print("(Guess) Enter a letter in word ");
        // If-clause is true if no asterisks were printed so
        // word is successfully guessed
        if (! printWord(word, enteredLetters))
            return 3;
        System.out.print(" > ");
        Scanner input = new Scanner(System.in);
        int emptyPosition = findEmptyPosition(enteredLetters);
        char userInput = input.nextLine().charAt(0);
        if (inEnteredLetters(userInput, enteredLetters)) {
            System.out.println(userInput + " is already in the word");
            return 2;
        }
        else if (word.contains(String.valueOf(userInput))) {
            enteredLetters[emptyPosition] = userInput;
            return 1;
        }
        else {
            System.out.println(userInput + " is not in the word");
            return 0;
            }
    }

    /* Print word with asterisks for hidden letters, returns true if
    asterisks were printed, otherwise return false */
    public static boolean printWord(String word, char[] enteredLetters) {
        // Iterate through all letters in word
        boolean asteriskPrinted = false;
        for (int i = 0; i < word.length(); i++) {
            char letter = word.charAt(i);
            // Check if letter already have been entered bu user before
            if (inEnteredLetters(letter, enteredLetters))
                System.out.print(letter); // If yes - print it
            else {
                System.out.print('*');
                asteriskPrinted = true;
            }
        }
        return asteriskPrinted;
    }

    /* Check if letter is in enteredLetters array */
    public static boolean inEnteredLetters(char letter, char[] enteredLetters) {
        return new String(enteredLetters).contains(String.valueOf(letter));
    }

    /* Find first empty position in array of entered letters (one with code \u0000) */
    public static int findEmptyPosition(char[] enteredLetters) {
        int i = 0;
        while (enteredLetters[i] != '\u0000') i++;
        return i;
    }
}

Log of its work:

(Guess) Enter a letter in word **** > a
(Guess) Enter a letter in word **a* > t
(Guess) Enter a letter in word t*at > q
q is not in the word
(Guess) Enter a letter in word t*at > t
t is already in the word
(Guess) Enter a letter in word t*at > b
b is not in the word
(Guess) Enter a letter in word t*at > h
(Guess) Enter a letter in word that
The word is that You missed 2 time(s)
\$\endgroup\$
4
  • \$\begingroup\$ Would it be good if I attach link on free-image hosting with image of block-scheme illustrating logic of my program, because it's not obvious how it work or it's not necessary? \$\endgroup\$
    – Alexander
    Jul 1, 2015 at 10:18
  • 2
    \$\begingroup\$ Flowcharts and the like would increase the understandability of what you're doing. That's always a good thing. \$\endgroup\$
    – Mast
    Jul 1, 2015 at 10:32
  • 2
    \$\begingroup\$ I think you've done a good job removing redundancies and creating methods to make it more concise. It is a little hard to follow, though. Maybe more descriptive method names would help (the best documentation is readable code!) \$\endgroup\$
    – dev_feed
    Jul 1, 2015 at 11:29
  • \$\begingroup\$ your code is too complicated you need to explain what is going on, as it is difficult for a student to read and know exactly what's happening. Thank you. \$\endgroup\$
    – user120073
    Oct 13, 2016 at 11:32

2 Answers 2

5
\$\begingroup\$

Magic Numbers: It would be better to move your definitions for 0,1,2,3 into an enum or final fields so that you can reference them with a meaningful name. For example final int LETTER_NOT_IN_WORD = 0 or enum HangmanGuess { LETTER_NOT_IN_WORD }

Redundancy: The core logic between findEmptyLetters and inEmptyLetters is the same, you are looking for a char in a char[]. You could refactor like this:

/* Check if letter is in enteredLetters array */
public static boolean inEnteredLetters(char letter, char[] enteredLetters) {
    return indexOf(letter, enteredLetters) >= 0;
}

/* Find first empty position in array of entered letters (one with code \u0000) */
public static int findEmptyPosition(char[] enteredLetters) {
    return indexOf('\u0000', enteredLetters)
}

/* Determine the index in {@code vals} where {@code ch} exists. Returns -1 if {@code ch} is not in {@code vals}. */
public static int indexOf(char ch, char[] vals) { 
    return Arrays.asList(vals).indexOf(Character.valueOf(ch));
}

One thing to point out also is that in the current implementation, findEmptyPosition will return n where n is the enteredLetters.length + 1 if there are no empty positions. I'm not sure if this is the desired functionality.

\$\endgroup\$
4
\$\begingroup\$

First (small) thing: it seems more common to use Random than Math.random(). See this best practices post.

The comments and switch first thing inside the do{ }while() is very confusing for me personally. enterLetter() never returns true or false, and even most new programmers know a do / while iterates until the condition is met. It would be more helpful to describe what 0-3 represent (and if this were production code or more complex, an Enum would make it considerably more understandable).

Inside of enterLetter(), it would be helpful to move the int emptyPosition = findEmptyPosition(enteredLetters); call to the else if where it's used. Not only does it prevent allocating the memory until it's used, but it's also more readable (and you could remove the emptyPosition variable if you wanted and call the function inside the reference, but that's semantic).

\$\endgroup\$

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