4
\$\begingroup\$

This is a number guesser that guesses a random number between the values that you set. The user has to use the up or down arrow keys to say if the number that they thought of between the number generated. This continues until the number that the user is thinking is generated and then the user pressed enter to say that their number has been guessed.

How can I make my code shorter? I feel that like I have a lot of unnecessary code. I mainly want to shorten the Update() area but any and all code shortcuts that could help me would be great.

NOTE: I had to use the newMax and newMin because the max and min values weren't updating correctly when I called them on and arrow presses.

using UnityEngine;
using System.Collections;

public class NumberWizard : MonoBehaviour {
    int max;
    int min;
    int guess;
    bool canGuess = false;
    bool setMax = false;
    bool setMin = false;

    // Use this for initialization
    void Start () {
        print ("Welcome to Number Wizard");
        print("Set max number!");
        setMax = true;
    }

    void StartGame () {
        print ("Pick a number in your head, but don't tell me!");

        print ("The highest number you can pick is " + max);
        print ("The lowest number you can pick is " + min);

        max = max +1;
        guess = Random.Range (min, max);

        print ("Is the number higher or lower than " + guess + "?");
    }

    // Update is called once per frame
    void Update () {        
        if (min >= max && !canGuess) min = max - 1;
        if (max > 0 && min < 0 && !canGuess) min = 0;
        if (max < 0) max = 0;

        int newMax = max;
        int newMin = min;

        if (Input.GetKeyDown(KeyCode.UpArrow)){
            if (canGuess){
                min = guess;
                NextGuess ();
            } else if (!canGuess && setMax){
                max += 100;
                newMax += 100;
                print ("Max: " + newMax);
            } else if (!canGuess && setMin){
                min += 1;
                newMin += 1;
                print ("Min: " + newMin);
            }
        } else if (Input.GetKeyDown(KeyCode.DownArrow)){
            if (canGuess){
                max = guess;
                NextGuess ();
            } else if (!canGuess && setMax){
                max -= 100;
                newMax -= 100;
                if (newMax < 0) newMax = 0;
                print ("Max: " + newMax);
            } else if (!canGuess && setMin){
                min -= 1;
                newMin -= 1;
                if (newMin < 0) newMin = 0;
                print ("Min: " + newMin);
            }
        } else if (Input.GetKeyDown(KeyCode.Return)){
            if (canGuess){
                print ("I guessed correct!");
                Start ();
                min = 0;
                max = 0;
                canGuess = false;
            } else if (!canGuess && setMax){
                if (max > 0){
                    print("Set min number!");
                    setMax = false;
                    setMin = true;
                } else if (max == 0)print("Choose a max number!");
            }else if (!canGuess && setMin){
                StartGame ();
                setMin = false;
                canGuess = true;
            }
        }
    }

    void NextGuess () {
        guess = (max + min) / 2;
        print ("Is the number equal to, higher or lower than " + guess + "?");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Don't know about shorter, but using flags and putting everything inside the ifs looks very ugly and the logic is hard to determine from it. \$\endgroup\$ – Sami Kuhmonen May 21 '15 at 10:06
1
\$\begingroup\$

Architecture:

You should really try to abstract out the environment specific aspects (the fact you're using unity) I would suggest creating a stand alone class Game and implementing all the business logic in there. Your class (NumberWizard ) would then become a thin wrapper that is only responsible for instantiation of the Game class and passing commands to it. Doing so would contribute a lot for your solution testability and re-usability.

Business logic and readability:

  1. You could try to give a descriptive name to your logical conditions:

    !canGuess && setMax

    would become var isInSetMinState = !canGuess && setMax which would then be used in the if statements. Doing so would make it easier to see what each if statement is for.

  2. Your game has 2 major types of actions:
    Increase/Decrease
    Confirm(essentially change state)
    It also has 3 states:
    SetMax
    SetMin
    Guess
    I would try to use them more explicitly in your code. It would make your code more readable and easier to understand.

something like the following:

private enum GameState {SetMax,SetMin,Guess};
private enum Action {None, Increase, Decrease, Confirm};
var _state = GameState.Init
var _action = None;

...

public void Update()
{
    _action = GetAction();
    switch(_action)
    {
        case Action .Confirm:
            switch(_state)
            {
                case GameState.SetMax:
                    _state = GameState.SetMin;
                break;
                case GameState.SetMin:
                   _state = GameState.SetMin;
                break;
                case GameState.Guess:
                   _state = GameState.Win;
               break;
        }
        break;
        case Action.Increase:
            switch(_state)
            {
                case GameState.SetMax:
                    //increase max logic
                break;
                case GameState.SetMin:
                   //increase min logic
                break;
                case GameState.Guess:
                   //guess in an upper range of [guess,max]
                break;
            }
        break;
        case Action.Decrease:
           //same as decrease
    }
}
\$\endgroup\$
  • \$\begingroup\$ @jozza710 I just posted my version of the same game. You might want to take a look at it. \$\endgroup\$ – Daniel Sokolov May 24 '15 at 3:55

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.