Command line Hangman game

Question

I was wondering if you can give me advice / and your opinion on the following code. I'm aware there are a few things that it doesn't presently deal with that could be enhanced (I don't have time, this is part of an assignment and the basic functionality is complete).

• Notifying user of their guess count has to be increased, if word chosen from file is greater in length than the guess count (if the guess count is lower than word length, in most cases, user can never win)

• Ignoring punctuation when selecting words from text file

/*
* Game.java
*
* V001
*
* 30.11.2016
*/
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.Random;
import java.io.InputStreamReader;

public class Game {

 // check if args[2] is an int
 public static boolean CheckInt(String input) {
  boolean valid = true;
  for (int i = 0; i < input.length(); i++) {
   String secondarg = input.substring(i, i + 1);
   if (secondarg.compareTo("0") < 0 || secondarg.compareTo("9") > 0) {
    valid = false;
    System.out.println("First argument must be integer");
    System.exit(1);
   }

  }
  return valid;
 }

 public static void main(String[] args) {
  BufferedReader reader = null;
  if (args.length != 2) {
   System.out.println("Please enter two arguments");
   System.exit(2);
  }

  boolean result = CheckInt(args[1]);

  // try to open file
  try {
   reader = new BufferedReader(new FileReader(args[0]));
  } catch (FileNotFoundException fnfe) {
   System.out.println("Error opening file" + args[0]);
   System.exit(3);
  }
  boolean done = false;
  String inputLine = null;
  String[] words = null;
  int length = 0;

  while (!done) {
   try {
    inputLine = reader.readLine();
   } catch (IOException ioe) {
    System.out.println("I/O error");
    System.exit(4);
   }

   //end of file
   if (inputLine == null) {
    done = true;
   } else {
    String line = inputLine;
    String delimter = " ";
    words = line.split(delimter);
    length = words.length;
   }
  }

  //random number generator to select word from array
  Random generator = new Random();
  int num = generator.nextInt(length - 0) + 0;

  //store chosen word
  String chosen = words[num];

  //convert string to char array
  char[] chosenarr = chosen.toCharArray();
  char[] output = new char[chosenarr.length];

  for (int x = 0; x < output.length; x++) {
   output[x] = '*';
  }
  System.out.println(new String(output));

  //user guesses
  int count = 0;
  int guessnum = Integer.parseInt(args[1]);
   
  BufferedReader userguess = new BufferedReader(new InputStreamReader(System.in));
  String guess = null;
  char[] userlinechar = null;
  String userlinestring = null;
  String b = null;
  String c = null;

  for (int a = 0; a < guessnum; a++) {
   
   
   System.out.println("Guess a character..");

   try {

    //convert bufferedreader to chararray
    guess = userguess.readLine();
    userlinechar = guess.toCharArray();
    userlinestring = Character.toString(userlinechar[0]);
    
    //check if user guess is valid
    if (!userlinestring.matches("[A-Za-z]+") || userlinechar.length != 1) {
     System.out.println("Enter valid input");
     a--;
    }
    
    //compare user guess with chosen word
    for (int x = 0; x < chosenarr.length; x++) {
     
    
     for (int j = 0; j < output.length; j++) {
        c = new String(chosenarr);
        b = new String (output);
      
      if (Character.toLowerCase(userlinechar[0]) == Character.toLowerCase(chosenarr[x])) {
        output[x] = Character.toLowerCase(userlinechar[0]);
       
      }
      
     }
    
     
      
     }
     
      if (b.equalsIgnoreCase(c)){
    break;
    }

   } catch (IOException ioe) {
    System.exit(4);
   }
  }
  
  System.out.println(new String(output));

  // check if user has won
  int countLose = 0;
  
  for (int i = 0; i < output.length; i++) {
   if (output[i] == '*') {
    countLose++;
   }
  }

  if (countLose > 0) {
   System.out.println("Hard luck");
  } else {
   System.out.println("Well done");
  }
 }
}

Answer 1

Having a huge main method that does (almost) everything is not a very good idea. It makes your code pretty hard to read. Ideally, a method should do one focused thing.

Let's go through you code:

1. The code parses and validates the command-line arguments. I'd make a separate method for it.

2. It reads something from a file. It's a good candidate for a separate method, too.

3. After that, it generates a random word. I'd make a method getRandomWord to make it more clear and easier to change in the future (if you ever need a change).

4. After that, the game loop starts. I'd recommend to refactor it into a bunch a small and simple methods (for reading user's input, comparing it to the word she should guess, and checking if the game is over).

In fact, it can be a good idea to go even further and split your code into multiple classes (a class should also do one focused thing). A reasonable way to do it would be to, make the following classes:

1. ArgumentParser for parsing and validating command line arguments.
2. WordGenerator for reading the words from a file and emitting random words.
3. UserInputReader for processing user's input.
4. A class that handles the game logic (and just it: no logging, reading input and stuff like that).
5. A class for logging (like displaying errors and showing the output to the user. These two could be two separate classes, too).

There multiple benefits to this approach:

1. The code becomes more readable.
2. It becomes simpler as every method is short and isolated.
3. It's easier to change.
4. It's easier to test. It's a good habit to cover you code with unit-tests. It's nearly impossible to write automatic modular tests for your code right now. Did you just test it manually?

To sum up, I'd recommend to make the code more modular by having a separate classes and methods for each thing that your code needs to do.

Answer 2

since @kraskevich already mentioned making new classes I will take it one small step forward and mention something I think all beginners should know and that is creating and using unit tests. There are a few different test libraries but they test in about the same way. For beginners it is that you would do a simple 3 step process of arrange-act-assert. I'll give examples pertaining to your game in pseudo code to express my point.

Arrange

This is the simple act of making your class ready for a test. I'll use kraskevich's points here. He mentions making an argument parser. so lets assume you created a CommandLineParser and a CommandLineParserTest. You would arrange your test something like this

import org.testng.annotations.Test;

import static org.testng.Assert.*;

public class CommandLineParserTest {

    @Test
    public void testTwoArgumentsAreRequired(){
        CommandLineParser parser = new CommandLineParser();
        String[] arguments = new String[0];

note: I'm using TestNG and made IntelliJ do most of the heavy lifting for me.

So the first 3 lines of code are from the IDE. It is importing all that I need to start testing. The method testTwoArgumentsAreRequired() I created with the @Test annotation. (you can read more about how TestNG finds tests to execute on your own). So the actual arrange for this is simple. I created a new CommandLineParser and a new set of arguments that I want to give it. In this case it is an empty String array.

Act

Now that we have the test arranged like we want we act on it. most of the time this is a simple one line call.

    @Test
    public void testTwoArgumentsAreRequired(){
        CommandLineParser parser = new CommandLineParser();
        String[] arguments = new String[0];

        parser.parse(arguments);

Assert

This is the important part of it all. This is where you check that your code did what you are hoping. In this case I want to assert/show that those arguments were not valid.

    @Test
    public void testTwoArgumentsAreRequired(){
        CommandLineParser parser = new CommandLineParser();
        String[] arguments = new String[0];

        parser.parse(arguments);

        assertFalse(parser.argumentsAreValid());
    }

Edit

Assert is a term associated with many test frameworks. Expect is another similar term. They usually have many methods that are all synonomous with the following Pseudo code

        Object expectedValue = something;
        Object actualValue = somethingElse;

        if(!expectedValue.equals(actualValue))
            throw new Exception("Expected {expectedValue} but found {actualValue}")

in actual code above I said assertFalse(parser.argumentsAreValid()); the assertFalse method expects that you give it a Boolean value (true or false) and the value should equal false. If it is true than the test will fail saying words along the lines of Expected false but found true and give you a line of code where it said that. Hope this clears up this subject a little for you.

Make the test pass

Now I wrote that test first before I wrote anything in CommandLineParser. So it looks like this right now.

public class CommandLineParser {
}

Obviously this has some compile errors which means the test automatically fails. To make it pass I have to add those 2 methods to my class. A few quick "make this method for me" shortcuts later and now I have:

public class CommandLineParser {
    public void parse(String[] arguments) {

    }

    public boolean argumentsAreValid() {
        return false;
    }
}

Now this isn't very useful, but the test does pass.

[TestNG] Running:
  C:\Users\USER\.IdeaIC2016.2\system\temp-testng-customsuite.xml

===============================================
Default Suite
Total tests run: 1, Failures: 0, Skips: 0
===============================================


Process finished with exit code 0
Total time 31ms

so lets make this more useful. I'm not going to show what CommandLineParser looks like to make my new test pass as I'll leave that for you, and this is more for example.

package com.codereview.hangman;

import org.testng.annotations.Test;

import static org.testng.Assert.*;

public class CommandLineParserTest {

    @Test
    public void testTwoArgumentsAreRequired(){
        CommandLineParser parser = new CommandLineParser();

        assertArgumentsAreNotValid(parser);
        assertArgumentsAreNotValid(parser, "one");
        assertArgumentsAreValid(parser, "one", "two");
    }

    private void assertArgumentsAreValid(CommandLineParser parser, String... arguments) {
        parser.parse(arguments);

        assertTrue(parser.argumentsAreValid());
    }

    private void assertArgumentsAreNotValid(CommandLineParser parser, String... arguments) {
        parser.parse(arguments);

        assertFalse(parser.argumentsAreValid());
    }
}

To sum up why I think you should start learning tests since you are new to java is that it gives you a much faster feedback cycle if something works or doesn't. There is a learning curve to it, and it might seem like it is hindering you. However like anything the more you practice it the better and faster you get at it. I'm to the point now where I'm slower if I DONT write tests. Part of it is because if I make a mistake in my logic and have a test showing how it is supposed to work I'll know quickly (usually in less than 1 second) that I made a mistake, and because I run my tests often it is easy to know exactly where the mistake was made. Normally I'd write much more than this, but what I said already if you try it out might take a while to figure out anyway.