7
\$\begingroup\$

This is my first by-self written code. Looking for advice on what not to or to do so I could deal with any bad habits now. It's a guessing game and works perfectly fine. I just wanted to know if more experienced programmers would see any huge mistakes in the coding style.

Basically it generates a number and then asks for the user input. If the two match you win. I tried playing around with arrays, multiple classes and methods and I was curious if there is anything wrong here.

Class: GuessGame

package base;

import java.util.Scanner;

public class GuessGame {

    public static Scanner scanKeyboard = new Scanner(System.in);


    public static void main(String[] args) {

        Handler gameHandler = new Handler();

        System.out.println("Start guessing! You have " + gameHandler.guessLeft + " guesses left.");
        System.out.println(gameHandler.goal);

        while(!gameHandler.gameOver){
            if (gameHandler.guessLeft == 0) {
                gameHandler.gameOver = true;
                gameHandler.gameOver();
            } else if(gameHandler.isGuessRight()){
                gameHandler.gameWin();
                gameHandler.gameOver = true;
            } else{

                gameHandler.guessCount++;
                System.out.println("Give me your " + gameHandler.guessCount + ". guess!");
                gameHandler.guesses[gameHandler.guessCount] = scanKeyboard.nextInt();
                gameHandler.guessLeft--;
                }
            }

        scanKeyboard.close();

        }

Class: Handler

package base;

import java.util.Random;

public class Handler {

    public int guessLeft = 5;
    public int guessCount = 0;
    public int[] guesses = new int[guessLeft + 1];

    public boolean gameOver = false;

    public Random rand = new Random();
    public int goal = rand.nextInt(100);


    public Handler(){
    }

    public String PrintGuesses(){
        String str = " ";
        for(int i = 1; i < guessCount; i++){
            str += guesses[i] + " ";
        }
        return str;
    }

    public void gameOver(){
        System.out.println("You suck!");
        System.out.println("The answer was: " + goal);
        System.out.println("Your miserable guesses were:" + PrintGuesses()+ guesses[guessCount]);

    }

    public void gameWin(){
        System.out.println("You won!");
        System.out.println("You guessed it on your " + guessCount + ". guess. With the guess " + guesses[guessCount] + ".");
        System.out.println("Your glorious guesses were:" + PrintGuesses() + guesses[guessCount]);
    }

    public boolean isGuessRight(){
        if (guessCount != 0){
            if (guesses[guessCount] == goal){
                return true;
            } else if(guesses[guessCount] < goal){
                System.out.println("Too low.");
            } else if(guesses[guessCount] > goal){
            System.out.println("Too high.");
            }
        }
        return false;

    }
\$\endgroup\$
  • \$\begingroup\$ If there is one thing, I don't really understand why you are making the array public int[] guesses = new int[guessLeft + 1]; and iterate it with starting index 1 (in Java, an N length array is indexed from 0 to N-1), and why you need + guesses[guessCount] when you print the guesses. \$\endgroup\$ – Zhuinden Dec 13 '14 at 11:57
  • \$\begingroup\$ Basically i found it easier to start the array with index of 1. So i made it that whatever i choose for guessLeft. The array would have the index for the same count.For example guessLeft = 5. The guesses array would also have an index of 5. And i would just ignore guesses[0]. So i guess i shouldn't do that then. And i add + guesses[guessCount] because without it the method wouldn't print the last number. \$\endgroup\$ – borderlinecool Dec 13 '14 at 12:15
  • \$\begingroup\$ Well, that's pretty much the indicator that something is wrong ^.^ you should use the zero indexing in Java, because then you can just say for(int i = 0; i < array.length; i++) { ... } for every element, rather than messing with <= and starting from 1 and allocation size+1 and the like. \$\endgroup\$ – Zhuinden Dec 13 '14 at 12:23
  • \$\begingroup\$ Makes sense :). \$\endgroup\$ – borderlinecool Dec 13 '14 at 12:31
  • \$\begingroup\$ A good rule of thumb is to keep all variables private in OOP. Just in case you are going to add more features in the future, other classes should not be able to see the variables. \$\endgroup\$ – ajsgolf27 Dec 22 '14 at 7:11
7
\$\begingroup\$

Naming

Handler is too generic and doesn't explain what this class is about. GameHandler would be better. Or maybe GameManager even better.

Methods should be named camelCase, so this violates the convention: PrintGuesses

The gameOver and gameWin methods' main job is to print messages. So it would be better to name them as such, for example printGameOverMessage, printGameWinMessage.

Visibility

A good general rule of thumb: you shouldn't manipulate the fields of objects directly like this: gameHandler.gameOver = true

All the fields in Handler should be private:

  • They should not be writable by other classes
  • Other classes should not need to see these implementation details

This is related to good encapsulation and information hiding: other classes should not need to know how the game handler works, they should be able to interact with it using public interface methods.

General coding style

  • The formatting is a bit inconsistent. Use your IDE's function to reformat the code nicely and consistently.
  • The empty public constructor of Handler is unnecessary, you can delete it, the compiler will add it automatically
| improve this answer | |
\$\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.