Simple Q&A game

Question

I just wanted to get some criticism on my programming to make me better. I made a game in Unity, just a simple question and answer game.

Demo

using UnityEngine;
using UnityEngine.UI;
using System.Collections.Generic;
using System.Collections;
using System.Linq;
using UnityEditor.VersionControl;
using System.Threading;
using System;

[System.Serializable]
class Question
{
    public string question;

    public bool answer;

    public Question(string question, bool answer)
    {
        this.answer = answer;
        this.question = question;
    }

    public string getQuestion()
    {
        return question;
    }
    public void setQuestion(string msg)
    {
        question = msg;
    }

    public bool getAnswer()
    {
        return answer;
    }


}

class NoMoreQuestionsException : Exception
{
    public NoMoreQuestionsException()
    {
    }
} 


public class GameManager : MonoBehaviour {

    Question currentQuestion;
    [SerializeField]
    float buttonMoveSpeed;

    int points = 0;

    bool isPlayerCorrect;

    bool changedAfterAnswer = false;

    bool moveComplete = false;

    int answer = 0;

    int currentQuestionNumber;

    [SerializeField]
    float waitBetweenQuestions;

    //List of questions
    [SerializeField]
    Question[] questions;

    List<Question> unansweredQuestions = new List<Question>();

    //Variables
    [SerializeField]
    GameObject questionText;

    [SerializeField]
    GameObject falseButton;

    [SerializeField]
    GameObject trueButton;

    [SerializeField]
    Text trueAnswerText;

    [SerializeField]
    Text falseAnswerText;

    [SerializeField]
    Text pointsText;

    public void Answered (bool playerAnswer)
    {
        if (playerAnswer)
        {
            answer = 1;
        } else
        {
            answer = -1;
        }
    }

    void NewRound()
    {
        if (unansweredQuestions.Count < 2)
        {
            Debug.Log("Error! No more questions to answer!");
            throw new NoMoreQuestionsException();
            return;
        }
        //Remove old question
        unansweredQuestions.RemoveAt(currentQuestionNumber);
        //Check if there are any more questions to answer
        Debug.Log("New Round!");
        //Generate new question number
        System.Random ran = new System.Random();
        currentQuestionNumber = (ran.Next(unansweredQuestions.Count - 1));
        //Set current question
        currentQuestion = unansweredQuestions[currentQuestionNumber];
        //Set question text to the new question
        Text txt = questionText.GetComponent<Text>();
        txt.text = currentQuestion.question;
        //Reset players result
        answer = 0;
        //Set the buttons back to centre
        falseButton.transform.position = new Vector3(1012.5f, 218.5f, -107.9f);
        trueButton.transform.position = new Vector3(283.5f, 218.5f, -107.9f);
        changedAfterAnswer = false;
    }

    void Start () {

        //Load points
        System.Random ran1 = new System.Random();
        points = ran1.Next(100);
        pointsText.text = points.ToString();

        //Se the level

        //Set unaswered questions to the main questions
        unansweredQuestions = questions.ToList();
        //Check if there are any questions to answer
        if (unansweredQuestions.Count < 1)
        {
            Debug.Log("Error! No questions to answer!");
        }
        Debug.Log("New Round!");
        //Generate new question number
        System.Random ran = new System.Random();
        currentQuestionNumber = (ran.Next(unansweredQuestions.Count - 1));
        //Set current question
        currentQuestion = unansweredQuestions[currentQuestionNumber];
        //Set question text to the new question
        Text txt = questionText.GetComponent<Text>();
        txt.text = currentQuestion.question;
        //Reset players result
        answer = 0;
        //Set the buttons back to centre
        falseButton.transform.position = new Vector3(1012.5f, 218.5f, -107.9f);
        trueButton.transform.position = new Vector3(283.5f, 218.5f, -107.9f);
        changedAfterAnswer = false;
        Debug.Log("Question Number: " + currentQuestionNumber);
    }

    void moveFalse()
    {
        if (falseButton.transform.position.x > 1330)
        {
            moveComplete = true;
            return;
        }

        Vector3 pos = falseButton.transform.position;

        pos.x += buttonMoveSpeed;

        falseButton.transform.position = pos;
        moveComplete = false;
    }

    void moveTrue()
    {
        if (trueButton.transform.position.x < -25)
        {
            moveComplete = true;
            return;
        }

        Vector3 pos = trueButton.transform.position;

        pos.x -= buttonMoveSpeed;

        trueButton.transform.position = pos;
        moveComplete = false;
    }

    //Check if users answer is correct
    bool isCorrect(bool playerAnswer)
    {
        if (currentQuestion.getAnswer() == playerAnswer)
        {
            points += 1;
            pointsText.text = points.ToString();
            return true;
        }
        return false;
    }

    void setAnswerText(bool answer, string text)
    {
        if (!answer)
        {
            trueAnswerText.text = text;
        } else
        {
            falseAnswerText.text = text;
        }
    }

    IEnumerator StartWait()
    {
        yield return new WaitForSeconds(waitBetweenQuestions);
        WaitForNewQuestion();
    }

    void WaitForNewQuestion()
    {

        NewRound();
        startedTimer = false;
    }

    bool startedTimer = false;

    void Update () {
        //Checking if the player has clicked a button
        if (answer == 0)
        {
            return;
        }
        //Check is user has pressed the true button.
        if (answer == 1)
        {
            //Check if player is correct and change the answer text to show.
            if (!changedAfterAnswer)
            {
                if (isCorrect(true))
                {
                    setAnswerText(true, "Correct!");
                }
                else
                {
                    setAnswerText(true, "Incorrect");
                }
                changedAfterAnswer = true;
            }
            //Move the false button
            moveFalse();
            if (!startedTimer)
            {
                Debug.Log("STARTED WA8T");
                startedTimer = true;
                StartCoroutine(StartWait());
            }
        }
        //Check is user has pressed the false button.
        if (answer == -1)
        {
            //Check if player is correct and change the answer text to show.
            if (!changedAfterAnswer)
            {
                if (isCorrect(false))
                {
                    setAnswerText(false, "Correct!");
                }
                else
                {
                    setAnswerText(false, "Incorrect");
                }
                changedAfterAnswer = true;
            }
            //Move the true button
            moveTrue();
            if (!startedTimer)
            {
                Debug.Log("STARTED WA8T");
                startedTimer = true;
                StartCoroutine(StartWait());
            }
        }

    }
}

Answer 1

You should avoid duplication of code.

e.g.

1. You can extract

    if (falseButton.transform.position.x > 1330)
    {
        moveComplete = true;
        return;
    }

    Vector3 pos = falseButton.transform.position;

    pos.x += buttonMoveSpeed;

    falseButton.transform.position = pos;
    moveComplete = false;

to a method passing 1330 or -25 as a parameter and reuse it instead of having nearly exactly the same code twice.

2. You can extract

        if (!changedAfterAnswer)
        {
            if (isCorrect(true))
            {
                setAnswerText(true, "Correct!");
            }
            else
            {
                setAnswerText(true, "Incorrect");
            }
            changedAfterAnswer = true;
        }
        //Move the false button
        moveFalse();
        if (!startedTimer)
        {
            Debug.Log("STARTED WA8T");
            startedTimer = true;
            StartCoroutine(StartWait());
        }

to a method passing true or false as a parameter and reuse it instead of having nearly exactly the same code twice.

Answer 2

As you are developing in C#, you can use properties instead of attributes with getters/setters.

They do not only look better but are also easier to use. So instead of this

// If you are using attributes, then you should always make them private,
// if you don't there is no sense behind getters and setters.
// public string question;
// public bool answer;

private string question;
private bool answer;

public string getQuestion()
{
    return question;
}
public void setQuestion(string msg)
{
    question = msg;
}

public bool getAnswer()
{
    return answer;
}

You can use the following:

// Note: Properties start with a capital letter
public string Question { get; set; }
public bool Answer { get; }

You can access those with yourObject.Question and yourObject.Answer afterwards.

Otherwise I can only agree to MrSmith's answer, that you may want to create more methods to keep the code simpler and to avoid duplications.

Furthermore I have the feeling that you have not understood the concept of OOP entirely yet, so you may want to read up on it a bit.

Answer 3

Magic Numbers

You have a lot of magic numbers that can be confusing. See examples below.

• new Vector3(1012.5f, 218.5f, -107.9f)
• if (falseButton.transform.position.x > 1330)
• if (trueButton.transform.position.x < -25)

These may make sense now but in the future (say 2 weeks from now when you've been deep in another part of your game's code) it might not be so clear why you chose these numbers.

So in order to make this code a bit more readable and maintainable I would suggest making them constants (or at least variables depending if they're changed or are calculated).

• private static readonly Vector3 FalseButtonOrigin = new Vector3(1012.5f, 218.5f, -107.9f);
• private const int FalseButtonDestination = 1330;
• private const int TrueButtonDestination = -25;

I probably got the purpose of the magic numbers wrong, but I think you get the point. Make them have a meaningful name. Worst case scenario you give them as good of a name as you can think of and place a comment above the const/variable declaration describing its purpose.

Minor Suggestions

• Update() - Perhaps it might be useful to switch those 3 if-statements you have to a switch-statement?
• Start() - You have a declaration of Text txt = questionText.GetComponent<Text>(); but you only appear to use it once. Variables do take memory which can add up. This is mostly something more to think about than me telling you to change it. On one hand, your game might take up a lot of memory and you want to reduce it; On the other hand, you might have to add another call to that variable later making the declaration useful and less maintenance required. So, just pointing that out so you can keep those kinds of things in mind.