2
\$\begingroup\$

I decided to polish my skills a little bit after learning about one dimensional arrays, and built my own mini project - hangman game. I'd like to have some criticism, I know it's not perfect and far from it, but please be aware, it's my FIRST EVER real project with more than 20 + lines of code.

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

public class Main {
    static Scanner input = new Scanner(System.in);

    public static void main(String[] args) {

        String[] words = {"java", "hello"};
        int random = (int)(0 + Math.random() * words.length);
        String wordToGuess = words[random];
        boolean isGuessed = false;
        int counter=0;
        int maxGuess = 7;
        char[] chars = new char[wordToGuess.length()];
        for (int i =0; i<chars.length; i++)
            chars[i] = '-';
        while (!isGuessed && counter <7) {

            System.out.println("Enter your guess: ");
            char guess = input.next().charAt(0);
            counter++;
            maxGuess--;
            for (int i = 0; i < wordToGuess.length(); i++) {
                if (wordToGuess.charAt(i) == guess)
                    chars[i] = guess;
            }
            if (doesContain(chars)) {
                for (int j = 0; j < wordToGuess.length(); j++) {
                    System.out.print(chars[j] + " ");


                }
                System.out.println("Guess left: " + maxGuess);

            } else {
                System.out.println("Good job! The word is: ");
                for (int j = 0; j < wordToGuess.length(); j++) {
                    System.out.print(chars[j] + " ");
                }
                System.out.print("Number of tries: ");
                System.out.print(counter);
                isGuessed = true;

            }
        }

    }

    public static boolean doesContain(char[] chars) {
        for (int i =0; i<chars.length; i++) {
            if (chars[i] == '-')
                return true;
        }
        return false;
     }

 }
\$\endgroup\$
2
\$\begingroup\$

It is necessary to have consistent and correct (as per style convention) white space. This includes vertical alignment (such as indentation) and spacing between and around particular syntax.

Convention on braces vary, but often braces are encouraged for single statement blocks. One reason is that a later revision may not realise to add braces when adding another statement to the block.

The following is how I might format the code. Note that I am not afraid to add extra blank lines to separate structures. I am not opposed to having three blank lines between methods, though I did not do that here.

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

public class Main {

  static Scanner input = new Scanner(System.in);

  public static void main(String[] args) {

    String[] words = {"java", "hello"};
    int random = (int)(0 + Math.random() * words.length);
    String wordToGuess = words[random];
    boolean isGuessed = false;
    int counter = 0;
    int maxGuess = 7;
    char[] chars = new char[wordToGuess.length()];

    for (int i = 0; i < chars.length; i++) {
      chars[i] = '-';
    }

    while (!isGuessed && counter < 7) {

      System.out.println("Enter your guess: ");

      char guess = input.next().charAt(0);

      counter++;
      maxGuess--;

      for (int i = 0; i < wordToGuess.length(); i++) {
        if (wordToGuess.charAt(i) == guess) {
          chars[i] = guess;
        }
      }

      if (doesContain(chars)) {

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(chars[j] + " ");
        }

        System.out.println("Guess left: " + maxGuess);

      } else {

        System.out.println("Good job! The word is: ");

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(chars[j] + " ");
        }

        System.out.print("Number of tries: ");
        System.out.print(counter);

        isGuessed = true;

      }

    }

  }

  public static boolean doesContain(char[] chars) {

    for (int i = 0; i < chars.length; i++) {
      if (chars[i] == '-') {
        return true;
      }
    }

    return false;

  }

}

This blob concerns me.

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;
char[] chars = new char[wordToGuess.length()];

It is best to keep separate what can be kept separate. I took a look at "x depends on y" written as x ⇒ y and came up with this.

chars ⇒ wordToGuess
wordToGuess ⇒ words
wordToGuess ⇒ random
random ⇒ words

Therefore we can break and reorder these variable declarations like so:

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
char[] chars = new char[wordToGuess.length()];  

boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;

Then looking down a bit further there is a for-loop which only depends on chars, so we can pull that up.

String[] words = {"java", "hello"};
int random = (int)(0 + Math.random() * words.length);
String wordToGuess = words[random];
char[] chars = new char[wordToGuess.length()];  

for (int i = 0; i < chars.length; i++) {
  chars[i] = '-';
}

boolean isGuessed = false;
int counter = 0;
int maxGuess = 7;

There are seven variables in scope (are accessible) for the rest of main. For each variable one must wonder "does this matter later?". This increases the effort to understand a block of code. Doing a text search I found that words and random are unused later. Therefore, lets more closely contain them.

String wordToGuess;
{
  String[] words = {"java", "hello"};
  int random = (int)(0 + Math.random() * words.length);
  wordToGuess = words[random];
}

char[] chars;
{
  chars = new char[wordToGuess.length()];  
  for (int i = 0; i < chars.length; i++) {
    chars[i] = '-';
  }
}

Now the program is well formatted and just by looking at dependencies we have improved the organisation (we did not have to know what the program does). Further improvements can come from rethinking some of how the program does its task. There seems to be quite a bit happening in the while-loop so I want to consider that first.

Reading the code and doing some string searching, I have determined what the while-loop reads and writes.

Reads & Writes

  • isGuessed
  • counter
  • maxGuess
  • chars
  • input

Reads Only

  • wordToGuess

Writes Only

N/A

Lets discover the purpose of each variable which is both read and written. Why these particularly? If a variable is only read it is invariant (unchanging) during the loop. If a variable is only written it has no effect on the loop behaviour.

  • input is used to read through stdin a character at a time, once each iteration.
  • counter is increased once per iteration. This is used to print the number of guesses thus far and to terminate the loop if !(counter < 7).
  • maxGuess is decreased once per iteration. This is used to print the number of guesses remaining.
  • chars is updated with the guessed letter once per iteration.
  • isGuessed is set to true only if the guessed word is complete. This is used to terminate the loop if !!isGuessed.

I see a redundancy between counter and maxGuess. counter = 7 - maxGuess or maxGuess = counter + 7. I think counting down to zero from maxGuess makes more sense than counting up to 7, so I will replace counter with maxGuess. Also I rename maxGuess to guessesRemaining and introduce a final variable maxGuesses assigned to 7.

boolean isGuessed = false;
final int maxGuesses = 7;
int guessesRemaining = maxGuesses;

while (!isGuessed && guessesRemaining > 0) {

  System.out.println("Enter your guess: ");

  char guess = input.next().charAt(0);

  guessesRemaining--;

  for (int i = 0; i < wordToGuess.length(); i++) {
    if (wordToGuess.charAt(i) == guess) {
      chars[i] = guess;
    }
  }

  if (doesContain(chars)) {

    for (int j = 0; j < wordToGuess.length(); j++) {
      System.out.print(chars[j] + " ");
    }

    System.out.println("Guess left: " + guessesRemaining);

  } else {

    System.out.println("Good job! The word is: ");

    for (int j = 0; j < wordToGuess.length(); j++) {
      System.out.print(chars[j] + " ");
    }

    System.out.print("Number of tries: ");
    System.out.print(maxGuesses - guessesRemaining);

    isGuessed = true;

  }

}

Final variables cannot be changed, so we know automatically the most the while-loop does with this variable is read it, which it does. Thus we have reduced the number of variables the loop both reads and writes and this can simplify comprehension.

While reading I found the method name doesContain does not suggest its purpose well. Thus I renamed it to isSolved. I also thought chars was too generic of a name so I renamed it to incompleteWord.

I found '-' was used as a placeholder letter in both main and isSolved. We want to avoid magic literals in our program. A "magic literal" is some literal (such as a string, character, or number) which occurs in one or more places and has an non-obvious and unstated reason for being the value it is. When reading code with magic literals we are not sure what the code does. When updating code with magic literals we are not sure where all we need to make the replacement, especially because there may be different reasons for a particular literal. To fix this we can simply name the literal.

static final char placeholderLetter = '-';
...
incompleteWord[i] = placeholderLetter;
...
if (incompleteWord[i] == placeholderLetter) {

There is much from here we can still do. However, I think this is a good place to stop seeing as this is your first substantial program.

I do not think giving advice or answers on how to implement a program from the abstract is on topic. Rather, this Q&A service is for suggesting improvements to code which is already working and understood. If you want to know how to add some ASCII art to your program then I suggest Stack Overflow.

This is my final version of your program after moving a couple more declarations around.

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

public class Main {

  static Scanner input = new Scanner(System.in);

  static final char placeholderLetter = '-';

  static final final int maxGuesses = 7;

  static final String[] words = {"java", "hello"};

  public static void main(String[] args) {

    String wordToGuess;
    { 
      int random = (int)(0 + Math.random() * words.length);
      wordToGuess = words[random];
    }

    char[] incompleteWord;
    {
      incompleteWord = new char[wordToGuess.length()];  
      for (int i = 0; i < incompleteWord.length; i++) {
        incompleteWord[i] = '-';
      }
    }

    boolean isGuessed = false;
    int guessesRemaining = maxGuesses;

    while (!isGuessed && guessesRemaining > 0) {

      System.out.println("Enter your guess: ");

      char guess = input.next().charAt(0);

      guessesRemaining--;

      for (int i = 0; i < wordToGuess.length(); i++) {
        if (wordToGuess.charAt(i) == guess) {
          incompleteWord[i] = guess;
        }
      }

      if (isSolved(incompleteWord)) {

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(incompleteWord[j] + " ");
        }

        System.out.println("Guess left: " + guessesRemaining);

      } else {

        System.out.println("Good job! The word is: ");

        for (int j = 0; j < wordToGuess.length(); j++) {
          System.out.print(incompleteWord[j] + " ");
        }

        System.out.print("Number of tries: ");
        System.out.print(maxGuesses - guessesRemaining);

        isGuessed = true;

      }

    }

  }

  public static boolean isSolved(char[] incompleteWord) {

    for (int i = 0; i < incompleteWord.length; i++) {
      if (incompleteWord[i] == placeholderLetter) {
        return true;
      }
    }

    return false;

  }

}
\$\endgroup\$
  • \$\begingroup\$ WOW, words can't decide how appreciated I am to you, thanks so much for the info! Before reading your comment I have done some changing, as I put the main in 2 code lines and the game is built inside the methods. Meanwhile Ill have a look at what you wrote, \$\endgroup\$ – Eliran Darshan Aug 28 '17 at 20:01
0
\$\begingroup\$

Instead of this:

int random = (int)(0 + Math.random() * words.length);

You could create a Random field

private static final Random random = new Random();

And then get an integer from to n (not including n) like this:

int choice = random.nextInt(n);
\$\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.