1

How can I properly design what a method or function should do?

To clarify the question I am going to use an example from a simple (non GUI) hangman game I created. The design is simple, it has a HangmanGame class which has the word the user needs to guess and other such things as well as the methods that control the game. It also has a HangmanDriver class that controls the flow of the game and methods calls and such.

1 of the methods is to get the users guess and process it and it does this by calling 2 other private (helper) methods inside the class. The following is the code (after which is my actual quetion).

/**
 * This method gets and processes a users guess.
 */
public void getAndProcessGuess()
{
    this.processGuess(this.getGuess());
}

/**
 * Asks the user for their guess and validates that their
 * guess is legal and has not already been made.
 * 
 * @return char The letter the user guessed
 */
private char getGuess()
{
    java.util.Scanner input = new java.util.Scanner(System.in);
    String guess = "";
    char letter = '\0';

    //get and validate the user input
    while (true)
    {
        System.out.print("Please enter your guess: ");
        try
        {
            guess = input.nextLine().toUpperCase();
        }
        catch (InputMismatchException e)
        {}

        System.out.println();

        if (guess.length() >= 1)
            letter = guess.charAt(0);

        if (!(guess.matches("[A-Z]{1}")))
            System.out.println("Please enter a single letter between A and Z\n");
        else if(isLetterGuessed((letter)))
            System.out.println("\nThat letter has already been guessed\n");
        else
            return letter;

        printSeparator(); //if the guess isn't valid print out a line separator
    }   
}   

/**
 * Processes the guess from user
 * 
 * @param letter the letter the user guessed
 */
private void processGuess(char letter)
{

    if (secretWord.indexOf(letter) >= 0)
    {
        int count = secretWord.length() - secretWord.replaceAll(Character.toString(letter), "").length();
        System.out.println("The letter " + letter + " appears " + count + (count == 1 ? " time." : " times.") + "\n");
        setLetterAsGuessed(letter);
        printSecretWordWithGuessedLettersShown();
    }
    else
    {
        System.out.println("The letter " + letter + " does not appear in the hidden word\n");
        decrementNumOfGuessesLeft();
        System.out.println("You have " + numOfGuessesLeft + " incorrect guesses left\n");
    }

    printSeparator();

}

What should be done inside each of these methods and whether they are doing too much?

The issue I have with the way it is now is that the methods print things out. It works in the current way the program works but it feels like these methods are doing more than they should and that perhaps the printing should be done by the Driver class not the instance of the class. Also if this were to ever be changed into a GUI the method wouldn't really make sense.. The issue with not having the method print out a line is that the validation would then also need to be done in the driver class.

The way I see it is there are three possible paths:

Inheritance

The Hangman game can be made into a superclass and it would then have 2 children one a HangmanConsoleGame and another HangmanGUIGame. This would solve the issue with how the methods function in each class (but the question still remains whether the methods should be printing things out).

Keep As Is

It works for how the game runs now. If any changes needed to be made or if the game will be changed to a GUI version the code can be refactored later so it works for the changes. This assumes that the methods doing the validation and printing things out is ok.

Refactor Now

Change the code now so that the validation is done within the Driver class and the methods simply return values with indications if the value is not valid. With the Driver class doing all the printing and the methods for getting the users guess only returning values it can then easily be changed into a GUI later on and minimizes what the methods are doing.

Which of these makes the most sense if any? Is there a better way to be doing this?

3

The main issue I have is that you are printing to console from all your methods. If possible, see if you could refactor your code to return significant results, and then print (or read) to/from console from 1 place, such as the driver.

This would come in handy should you want to make a GUI program, and now your logic will need to be rewired to handle actual windows and panels, and that might not be a fun ride. If you shield your logic from what means you are using to communicate with the user, rewiring it should be easier.

Secondly (my major concern) is that you are creating a new Scanner: java.util.Scanner input = new java.util.Scanner(System.in);, but you never actually release it, so your application has a leak.

Lastly, this: (!(guess.matches("[A-Z]{1}"))) will check if the string has at least one character (upper case). However, you are taking the first character of input, thus, something like so: 123t should pass the check but break your logic. Using something like: Char.isLetter(guess.charAt(0)) should work better, or else change your pattern to (!(guess.matches("^[A-Za-z]"))).

  • Thank you for your response, all excellent points and suggestions. For the Scanner object is it better to make it a static field in the class so the object is created once even though it is only used in that one method? – yitzih Feb 25 '15 at 16:50
  • 1
    @yitzih: Maybe not a static field, since that might cause issues should you multi thread the application and I have no clue how will the console handle it, so it might be best to declare it as a non static field. – npinti Feb 25 '15 at 16:53
1

It works in the current way the program works but it feels like these methods are doing more than they should

I agree, your methods do too much right now.

I would refactor now. If you extract code to classes, it will make your code more extensible, and also make it easier to find functionality. I would create a structure such as this:

interface Input {
    char getGuess(List<Character> guessedLetters) throws IllegalInputException;
}

interface Output {
    void reportIncorrectInput(String message);
    void reportWrongGuess(String message, char guess);
    void reportRightGuess(String message, char guess);
}

class IllegalInputException extends Exception {
    public IllegalInputException(String s) {
        super(s);
    }
}

class ConsoleInput implements Input {
    @Override
    public char getGuess(List<Character> guessedLetters) throws IllegalInputException {
        // get input. if input is illegal, throw:
        throw new IllegalInputException("message depending on error");
    }
}

class ConsoleOutput implements Output {
    // report (in this case, all methods just print to console)
}

And then use the interfaces in your game controller. That way, you can easily exchange the concrete implementation against a different one (such as a gui).

  • 1
    Not for nothing, but it is just a hangman game. There is such a thing as over-engineering. – Robert Harvey Feb 25 '15 at 16:35
  • 1
    @yitzih: It still had to be said. Many developers use things they don't need. For many smaller applications, you're not going to need this. Part of learning a new technique is also knowing when it is unnecessary. – Robert Harvey Feb 25 '15 at 16:43
  • 2
    @RobertHarvey All big applications begin being small applications. – Tulains Córdova Feb 25 '15 at 17:53
  • 2
    @user61852 - That may be true, but taking a domain that ought to be small and making it large instead leads to foolhardy practices. If someone wants practice at building large applications then they should pick a larger domain; carve off a slice of that; and build from there. Forcing something that's small to become large is a bad idea. – user53019 Feb 25 '15 at 18:05
  • 1
    I agree with @RobertHarvey. The best way to learn a skill is to need it. If you want the skills necessary for a large system, make a large system, let it grow until you realize that it is difficult to work on because of design problems, then devise a solution. That doesn't mean that you have to wait until the system is totally unmaintainable, but you need a problem before you can apply a solution. – abl Feb 25 '15 at 18:56
1

I think it depends on how modular you want your code to be. I learned to make your 'Driver/main' class as minimalistic as possible, put the hard work down as far as you can to make your code as reusable as possible. But it's really a matter of opinion. If you feel that rewriting chunks of your code will be beneficial, do it, but don't waste your time on something that will only give you a meager return.

There is probably always a better way, and absolutely always a different way to do something in code, it's just a matter of whether you think what you did serves its purpose.

If there is a good possibility of moving to a GUI based application, I would recommend doing the super/sub class idea you had mentioned. That would make it easer with plenty of overlap between code, so you don't have to write as much.

There's quite a few options that you could go with, it's just up to your personal preference.

1

In layman's words

  • You have to separate business logic from presentation logic.
  • The game class (business logic) should not read input from console not from an HTML form.
  • The game class (business logic) should only communicate with the world via method calls.
  • A console app (presentation logic) should ask for input and make method calls to the game class, and output info to console
  • A web app (also presentation logic) should ask for input and make method calls to the game class, and output to HMTL
  • A graphic desktop app (yet another presentation logic) should ask for input (maybe a dialog box or mouse events) and make method calls to the game class, and output graphically in windows, dialog boxes, etc.
  • The game class is agnostic. It doesn't worship the console God, the web God or the desktop God.

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.