Simple Q&A game

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;

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

{
this.question = question;
}

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

{
}

}

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

public class GameManager : MonoBehaviour {

Question currentQuestion;
[SerializeField]
float buttonMoveSpeed;

int points = 0;

bool isPlayerCorrect;

bool moveComplete = false;

int currentQuestionNumber;

[SerializeField]
float waitBetweenQuestions;

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

//Variables
[SerializeField]
GameObject questionText;

[SerializeField]
GameObject falseButton;

[SerializeField]
GameObject trueButton;

[SerializeField]

[SerializeField]

[SerializeField]
Text pointsText;

{
{
} else
{
}
}

void NewRound()
{
{
Debug.Log("Error! No more questions to answer!");
throw new NoMoreQuestionsException();
return;
}
//Remove old question
//Check if there are any more questions to answer
Debug.Log("New Round!");
//Generate new question number
System.Random ran = new System.Random();
//Set current question
//Set question text to the new question
Text txt = questionText.GetComponent<Text>();
txt.text = currentQuestion.question;
//Reset players result
//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);
}

void Start () {

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

//Se the level

//Set unaswered questions to the main questions
//Check if there are any questions to answer
{
}
Debug.Log("New Round!");
//Generate new question number
System.Random ran = new System.Random();
//Set current question
//Set question text to the new question
Text txt = questionText.GetComponent<Text>();
txt.text = currentQuestion.question;
//Reset players result
//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);
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
{
{
points += 1;
pointsText.text = points.ToString();
return true;
}
return false;
}

{
{
} else
{
}
}

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
{
return;
}
//Check is user has pressed the true button.
{
//Check if player is correct and change the answer text to show.
{
if (isCorrect(true))
{
}
else
{
}
}
//Move the false button
moveFalse();
if (!startedTimer)
{
Debug.Log("STARTED WA8T");
startedTimer = true;
StartCoroutine(StartWait());
}
}
//Check is user has pressed the false button.
{
//Check if player is correct and change the answer text to show.
{
if (isCorrect(false))
{
}
else
{
}
}
//Move the true button
moveTrue();
if (!startedTimer)
{
Debug.Log("STARTED WA8T");
startedTimer = true;
StartCoroutine(StartWait());
}
}

}
}


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))
{
}
else
{
}
}
//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.

• Ok, yup just clean up my code a bit :) Will do thanks :) – DarkZek Apr 30 '16 at 3:33

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;

private string question;

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

{
}


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.

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.
• Thanks :D Yes, that allready happened once forgot what they were for. I'll do that thanks – DarkZek Apr 30 '16 at 3:32