8
\$\begingroup\$

I'm hoping someone can look over my code and tell me if there is anything I can improve. I am not very experienced with Java. I am hoping for some feedback as to avoid developing any bad habits which can turn into bad and slow code.

It can also be found here.

import java.util.Scanner;
import java.io.File;
import java.io.IOException;
import java.io.FileNotFoundException;
import java.io.FileWriter;
import java.util.InputMismatchException;
import java.lang.Runtime;
public class MinOppgave3 {  

static Scanner input = new Scanner(System.in);
static boolean checkLoop = true;
static char[] word;
static char[] temp;

static String hangmanWord = "TemporaryString";
static File f = new File("words.txt");
static String[] wordsFromFile;
static int missCount = 0;   
// Game board for hangman
static char[][] board = {
            {'_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_'},
            {'_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '|'},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '|', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '|', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', ' ', '|', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
            {'|', '_', '|', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_'},
            };  



public static void main(String[] args) throws IOException
{

    fileCreate();
    fileRead();     
    menu();

}

// Menu to choose 1 of 4 option
static void menu() throws IOException
{
    int menuChoice = 0;

    System.out.println("What do you want to do? \n1.Play Hangman\n2.Add a word to the word file\n3.List all the words in the list\n4.Exit");
    try {
        menuChoice = input.nextInt();
    }catch(InputMismatchException e) {
        System.out.println("That is not a valid option");
        menuChoice = 0;
        menu();
    }
    switch(menuChoice) {
        case 1: 
            setHangmanWord();
            playHangman();
            break;
        case 2: 
            fileWrite();
            break;
        case 3: 
            listWords();
            break;
        case 4: 
            System.exit(0);
            break;
        default:
            System.out.println("Wrong input try again!");
            menuChoice = 0;
            menu();
            break;
    }
}

// sets the word for the hangman game
static void setHangmanWord() {
    hangmanWord = wordsFromFile[selectWord()];
    temp = new char[hangmanWord.length()];      
    word = new char[hangmanWord.length()];
    word = hangmanWord.toCharArray();
    for(int x = 0; x < temp.length; x++)
    {
        temp[x] = '_'; 
    }
}

// The actual game, test the input
static void playHangman()
{   
    String inputCharacter = " ";
    char inpChar;
    printboard();
    int countHits = 0;
    System.out.println();
    try {
        inputCharacter = input.next();

        if(inputCharacter.length() < 2 && inputCharacter.length() > 0) 
        {

            if(Character.isLetter(inputCharacter.charAt(0))) 
            {
                inpChar = inputCharacter.charAt(0);

                for(int x = 0; x < temp.length; x++) 
                {
                    if(Character.toLowerCase(inpChar) == Character.toLowerCase(word[x])) 
                    {
                        temp[x] = inpChar;
                        countHits++;
                    }
                }
                if(countHits == 0) 
                {
                    missCount++;
                    boardUpdate(missCount);
                    printboard();
                    playHangman();
                }else {
                    checkWordWithCharacter();
                }                   
            }else {
                System.out.println("What you entered is not a character");
                playHangman();
                inpChar = '\0';
            }
        }
        else 
        {
            checkWordFull(inputCharacter);
        }
    }catch(InputMismatchException e) {
        System.out.println("Det er ikke riktig verditype");
    }
}

// Checks if the word is completed in the game when you enter a single character
static void checkWordWithCharacter() 
{   
    int hits = 0;
    for(int x = 0; x < word.length; x++) 
    {
        if(Character.toLowerCase(temp[x]) == Character.toLowerCase(word[x])) 
        {
            hits++;
        }
    }

    if(hits == word.length) 
    {
        printboard();
        System.out.println("\nYou have won!\n\n");
        try{
        menu();
        }catch(IOException e){
            e.printStackTrace();
        }
    }else {
        System.out.println();
        printboard();
        playHangman();
    }
}

//Checks if the word is completed when you guess using an entire word
static void checkWordFull(String word) 
{
    if(hangmanWord.toLowerCase().equals(word.toLowerCase())) 
    {
        System.out.println("You have won!");
        try{
            menu();
        }catch(IOException e) {
            e.printStackTrace();
        }
    }else {
        System.out.println("That word is incorrect, don't be so hasty and try again!");
        playHangman();
    }

}

// Writes out the gameboard and the characters you have guessed correctly
static void printboard()
{
    for(int x = 0; x < board.length;x++) 
    {
        for(int y = 0; y < board[x].length; y++) 
        {
            System.out.print(board[x][y]);
        }
        System.out.println();
    }

    for(int x = 0; x < temp.length; x++) 
    {
        System.out.print(temp[x]);
    }

}

//Updates the gameboard when you miss a character and starts drawing the man
static void boardUpdate(int missCount) 
{
    switch(missCount) 
    {
        case 1: 
            board[4][11] = '0';
            break;
        case 2:
            board[5][11] = '|';
            break;
        case 3: 
            board[6][11] = '|';
            break;
        case 4: 
            board[7][11] = '|';
            break;
        case 5: 
            board[6][12] = '\\';
            break;
        case 6: 
            board[6][10] = '/';
            break;
        case 7: 
            board[8][11] = '\\';
            break;
        case 8: 
            board[8][9] = '/';
            System.out.println("You have lost!");
            printboard();
            try {
                menu();
            }catch(IOException e) {
                e.printStackTrace();
            }
            break;
        default: break;

    }   
}

// Creates a word file if there isn't one
static void fileCreate() 
{
    FileWriter writer;
    if(!f.exists()) {
        try {
            f.createNewFile();
            writer = new FileWriter(f);
            writer.write("Sindre\n");
            writer.write("Schmidt\n");
            writer.flush();
            writer.close();
        } catch(IOException e) {
            e.printStackTrace();    
        }

    }
}

//Reads the words in the file in to a String Array
static void fileRead() throws FileNotFoundException
{
    Scanner fileInput = new Scanner(f);
    wordsFromFile = new String[lineCount()];
    int count = 0;
    while(fileInput.hasNextLine()) 
    {
        wordsFromFile[count] = fileInput.nextLine();
        count++;
    }
}

// Returns a random value a between 0 and the amount of words in the String array
static int selectWord() {
    int x = (int) (Math.random() * wordsFromFile.length);
    return x;
}

// Allows you to enter new words in to the file that can be used in the game
static void fileWrite() throws IOException
{
    FileWriter writer = new FileWriter(f, true);
    System.out.println("What word do you want to add to the list: ");
    String temporaryString = input.next();
    writer.write(temporaryString + "\n" );
    writer.flush();
    writer.close();
    fileRead();

}

//Counts the lines in the file
static int lineCount() throws FileNotFoundException 
{
    Scanner fileInput = new Scanner(f);
    int lines = 0;

    while(fileInput.hasNextLine()) 
    {       
        lines++;
        fileInput.nextLine();
    }
    return lines;
}


//Lists out all the words read from the file through the string array
static void listWords()  
{
    for(int x = 0; x < wordsFromFile.length; x++) 
    {
        System.out.println(wordsFromFile[x]);
    }
}
}
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Missing Functionality

As I know hangman, the letters that were guessed are written down. You should also save them and print them out each time, otherwise the user has to remember this.

Bug

If I play a game, lose, and then print out all words, the hangman board is printed as well.

Game Loop

Use a game loop instead of your repeated calls to playHangman. Your approach is hard to grasp, especially since you not only call playHangman from playHangman, but also from checkWordWithCharacter and checkWordFull. It would look something like this:

while (!gameOver) {
    char guess getInput();
    processInput(guess);
    printBoard();
}

I would also rather use a while loop for the menu than using your recursive approach.

Single Responsibility

Functions as well as classes should only have one responsibility. You only have one class, so it obviously does everything.

Your playHangman function for example handles the game loop, gets and checks user input, applies the input, updates the game, and prints.

Don't Repeat yourself

You have the functionality of counting how often a character appears in a string twice. Just extract it to a method.

OOP

Java is an object oriented language, so try to make use of this.

Using static is always a sign of bad design. There are situations where it does make sense, but every time you use it, you should think about it.

Try to create more classes. It's better to create too many than too few. At least I would expect a board class, a class handling the word management (reading and writing to a file, etc), a class handling user input, a class handling output, a game class, and maybe even a word class.

Type of countHits

You don't care how often the guessed character is in the word, only if it was in it at least once. So I would make this a boolean.

Variable Names

  • countHits is not only the wrong type, it's also not a good name. guessInWordCount or something would be better.
  • temp might be ok in very limited scope, but not as a class field. Same goes for f.
  • inpChar is cryptic. use inputChar.
  • you use x quite a lot. For loop variables, i would be customary as default (except when you have a good reason), and for return statements something that represents the value, for example selectedWord.

Comments

For method names, use JavaDoc style comments.

Style

  • be consistent with your curly brackets. Either they go all on the same line, or all on a new line. Same goes for else statements.
  • also be consistent with spaces (before and after { and ().

Use any IDE to fix this easily.

Misc

  • inputCharacter.length() < 2 && inputCharacter.length() > 0 is equal to inputCharacter.length() == 1.
  • you can used enhanced for loops: for(int x = 0; x < wordsFromFile.length; x++) -> for (String word : wordsFromFile) {.
  • no need to save return values in temporary variables if they are not used.
  • use private and public for fields and methods.
  • don't assign values that are never used (eg inputCharacter = " ", menuChoice = 0, or inpChar = '\0').
  • instead of toLowerCase().equals, you can use equalsIgnoreCase.
  • You are not properly closing your FileWriter.
\$\endgroup\$
1
\$\begingroup\$

Access modifiers would help. Also, more error handling such as with fileRead() and fileWrite() methods. I would use a try-catch block to deal with the event that the files don't exist. Foreach loops for your arrays would help simplify some of your code as well but It looks pretty good overall.

Example:

  for(Object o: array) //loops through each Object in your array.
   {
     System.out.println(o.toString());
   }
\$\endgroup\$
1
  • \$\begingroup\$ Your advice sounds rather generic. Could you be specific about how you would apply those suggestions to the actual code? \$\endgroup\$ Commented Sep 18, 2014 at 2:16
1
\$\begingroup\$
  • I guess this main() method should throw StackOverflowException, because the menu() is called recursively from menu().
  • Some fields could be final, for example f.
  • This code is not following object oriented paradigm, many static fields, states in static variables etc. So first of all you have to understand what is an object, what means static, and how to avoid using static fields and methods.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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