Feedback requested on simple trivia

Question

I'm a newbie self-learned programmer, I've started poking around a bit at C# around two months ago, since I will be starting formal programming studies in about 7 months. I heard the best way to learn how to code is to code so I've written this trivia application and I would love some constructive feedback, what are the good things? And what are the things I could improve on / have done better.

Progam.cs

namespace EasyTrivia
{
    class Program
    {

        const bool DEBUG_MODE = false; 

        public static void DebugPrint(string printOut) {

            if (DEBUG_MODE) { Console.WriteLine("[DEBUG]: {0}", printOut); }
        }
        static void Main(string[] args)
        {
            //PLACEHOLDER, check for command line arguments
            for (int y = 0; y < args.Length; y++)
            {
                switch (args[y])
                {
                    case "debug1":
                        Console.WriteLine("DEBUG {0} ARGUMENT", y);
                        break;
                    case "debug2":
                        Console.WriteLine("DEBUG {0} ARGUMENT", y);
                        break;
                    case "debug3":
                        Console.WriteLine("DEBUG {0} ARGUMENT", y);
                        break;
                }
            }

            const int MAX_PLAYERS = 4; // TODO: Add more freedom here, so you can have any number of players

            string[] pNames = new string[4];
            Player[] player = new Player[4];

            Console.WriteLine("How many players wish to play? (max 4)");
            string input = Console.ReadLine();
            int noOfPlayers;
            bool parseInput = Int32.TryParse(input, out noOfPlayers); // We're doing the easy check

            while (noOfPlayers > MAX_PLAYERS || noOfPlayers < 1)
            {
                Console.WriteLine("Incorrect input. Please enter a valid number of players inbetween 1 and 4");
                input = Console.ReadLine();
                parseInput = Int32.TryParse(input, out noOfPlayers);
            } Console.Clear();

            for (int i = 0; i < noOfPlayers; i++)
            {
                Console.WriteLine("Please enter the name for player {0}", i + 1);
                pNames[i] = Console.ReadLine();
                player[i] = new Player();
                player[i].Name = pNames[i];
                player[i].Score = 0;

            } Console.Clear();

            GameLoop(player, noOfPlayers);
        }
        static void GameLoop(Player[] player, int noOfPlayers)
        {
           bool GameState = true;
           List<int> blacklist = new List<int>();
           Random RNG = new Random();
           const int WINNING_SCORE = 3; // TODO: Custom winning score should be configurable through commandline argument.
           TriviaHandler trivia = new TriviaHandler();
           trivia.InitializeQuestion();


            //Begin Game Loop
            while (GameState)
            {
                DebugPrint("Game loop initializing...");

                string questionString = trivia.FetchQuestion(blacklist, RNG);

                //If it's a duplicate entry FetchQuestion() will return null
                while (questionString == null) { questionString = trivia.FetchQuestion(blacklist, RNG); }

                Console.WriteLine(questionString);

                for (int i = 0; i < noOfPlayers; i++)
                {
                    Console.WriteLine("Player {0}, type in your answer:", i+1);
                    string pInput = Console.ReadLine();
                    if (trivia.CheckAnswer(pInput))
                    {
                        Console.WriteLine("Correct! You have been awarded 1 point");
                        player[i].Score += 1;
                        // If they get enough pts to win we gotta end the game here too.
                        if (player[i].Score == WINNING_SCORE) 
                        { 
                            Console.WriteLine("Congratulations player {0} has won the game! Would you like to play another one? (Y/N)", player[i].Name);
                            string choice = Console.ReadLine().ToUpper();
                            while (choice != "Y" && choice != "N")
                            {
                                Console.WriteLine("Incorrect input! Please press Y for yes or N for no");
                                choice = Console.ReadLine().ToUpper();
                            }
                            if (choice == "Y")
                            {
                                //Reset player score first
                                for (int z = 0; i < player.Length; i++)
                                {
                                    player[z].Score = 0;
                                }
                                GameLoop(player, noOfPlayers);
                            }
                            else { Environment.Exit(0); }
                        }
                        //If the answer is correct break the loop
                        break;
                    }
                    else { Console.WriteLine("Incorrect Answer, question is passed on to next player"); }
                } 
            }
        }
    }
}

Player.cs

namespace EasyTrivia
{
    class Player
    {
        private string name;
        public string Name { get; set; }

        private int score;
        public int Score { get; set; }
    }
}

TriviaHandler

namespace EasyTrivia
{
    class TriviaHandler
    {
        Question[] question = new Question[Question.GetMaxQuestions(true)];

        private int qID;
        public int QID { get; set; }

        public void InitializeQuestion()
        {
            List<string> questions = new List<string>(Question.ListA);
            //questions.AddRange(Question.ListA);
            List<string> answers = new List<string>();
            answers.AddRange(Question.ListB);

            Program.DebugPrint("Initializing questions...");
            for (int i = 0; i < answers.Count; i++)
            {
                question[i] = new Question();
                question[i].Pretext = questions[i];
                question[i].Answer = answers[i];
                //if (options) { question[i].LoadOptions() }
            }
        }

        public bool CheckAnswer(string input)
        {
            //if (input == "debug") { return true; }
            if (input.ToUpper() ==  question[QID].Answer.ToUpper()) { return true; }
            else { return false;  }
        }
        public string FetchQuestion(List<int> blacklist, Random RNG)
        {
            bool blisted = false;
            int QuestionID = RNG.Next(Question.counter);

            if (blacklist.Contains(QuestionID)) { blisted = true; }

            if (!blisted)
            {
                QID = QuestionID;
                blacklist.Add(QuestionID);
                return question[QuestionID].Pretext;
            }
            else { return null; }
        }
    } 
}

Question.cs

namespace EasyTrivia
{
    class Question
    {
        private string pretext;
        private string answer;
        private string[] options;
        private string[] listA;
        private string[] listB;

        public static string[] ListA { get; set; }
        public static string[] ListB { get; set; }
        public string Pretext { get; set; }
        public string Answer { get; set; }
        public static int counter { get; set; }

        public static int GetMaxQuestions(bool initialize)
        {
            int count = new int();
            List<string> questions = new List<string>();
            List<string> answers = new List<string>();
            Program.DebugPrint("Getting max questions...");
            XElement rawXML = XElement.Load(@"C:\Users\Gabriel\Documents\Visual Studio 2013\Projects\EasyTrivia\EasyTrivia\Questions.xml");
            IEnumerable<XElement> elements = rawXML.Elements();

            //So I've written three ways of fetching the Q/A data

            //Method 1
            //foreach (XElement question in elements)
            //{
            //    if (initialize) { count++; }
            //    questions.Add(question.Element("Pretext").Value);
            //    answers.Add(question.Element("Answer").Value);
            //}
            //if (initialize)
            //{
            //    ListA = questions.ToArray();
            //    ListB = answers.ToArray();
            //}

            //Method 2
            var pQuery = from lQ in elements.Elements()
                         where lQ.Name == "Pretext"
                         select lQ.Value;
            var aQuery = from aQ in elements.Elements()
                         where aQ.Name == "Answer"
                         select aQ.Value;

            foreach (var pretext in pQuery)
            {
                if (initialize) { count++; }
                questions.Add(pretext);
            }
            foreach (var answer in aQuery)
            {
                answers.Add(answer);
            }
            if (initialize)
            {
                ListA = questions.ToArray();
                ListB = answers.ToArray();
            }

            //Method 3
            //string[] lines = File.ReadAllLines("Questions.txt");
            //for (int i = 0; i < lines.Length; i++)
            //{
            //    if (lines[i].Contains("[QUESTION") == true) { count++; }

            //    if (initialize && lines[i].Contains("[QUESTION"))
            //    {
            //        string[] delimiter1 = lines[i + 1].Split('=');
            //        string[] delimiter2 = lines[i + 2].Split('=');
            //        questions.Add(delimiter1[1]);
            //        answers.Add(delimiter2[1]);
            //        ListA = questions.ToArray();
            //        ListB = answers.ToArray();

            //    }
            //}
            if (initialize) { counter = count; }
            return count;

        }
    }
}

Questions.xml

<?xml version="1.0" encoding="utf-8" ?>
<QUESTIONS>
  <Question id="1">
    <Pretext>This is the first test question</Pretext>
    <Answer>test1</Answer>
  </Question>
  <Question id="2">
    <Pretext>This is the second test question</Pretext>
    <Answer>test2</Answer>
  </Question>
  <Question id="3">
    <Pretext>This is the third test question</Pretext>
    <Answer>test3</Answer>
  </Question>
</QUESTIONS>

Answer 1

First note - do not use DEBUG_MODE constant to define whether you should print debug message or not. There are better ways to do it. You can either use conditional attribute to check if compilation symbol "DEBUG" is defined. Calls to methods marked with Conditional attribute completely removed from generated code if symbol is not defined ("DEBUG" is defined by default when you start application in debug mode):

[Conditional("DEBUG")]
public static void DebugPrint(string printOut) 
{
    Console.WriteLine("[DEBUG]: {0}", printOut);
}

Another option is usage of logging library (NLog, log4net etc). You will be able to define logging targets (or appenders) which will write messages to console, file, database etc. You will be able to define levels of logging messages from configuration file without changing your application code. E.g. with NLog you can get instance of logger

private static Logger Logger = LogManager.GetCurrentClassLogger();

And use it to write debug messages if debug level message are enabled in configuration (you can change message format from config):

Logger.Debug("Initializing questions...");

---

Back to application. I suggest you to follow capitalization styles for naming types, methods and variables suggested by Microsoft. E.g. local variables should have camelCase names. Also do not use prefixes and shortenings in variable names - pNames is less readable than playerNames. And it's hard to understand what is listA and listB, why queries have names pQuery and aQuery, and their query variables names lQ and aQ. Choose names wisely to help others understand your code.

---

If you are using auto-implemented properties, than you don't need to define fields:

class Player
{        
    public string Name { get; set; } // back-storage will be generated
    public int Score { get; set; }
}

---

Your Question class is strange. First thing which is not obvious is that it gets initialized when you check questions max something. I suggest you to create Question class which will represent data you have in xml:

public class Question
{
    public int Id { get; set; }
    public string Pretext { get; set; }
    public string Answer { get; set; }
}

I suggest not to hard-code file names in your application. Move move questions file name to application settings:

<appSettings>
   <add key="questionsFileName" 
        value="C:\EasyTrivia\EasyTrivia\Questions.xml"/>
</appSettings>   

And move data-access logic into separate class (with this Question class you event can use Xml serialization attributes to deserialize questions from xml file), e.g.

public class QuestionRepository
{
    private static readonly string fileName;

    static QuestionRepository()
    {
        fileName = ConfigurationManager.AppSettings["questionsFileName"];
    }

    public List<Question> GetAllQuestions()
    {
        XDocument xdoc = XDocument.Load(fileName);
        var query = from q in xdoc.Root.Elements("Question")
                    select new Question {
                        Id = (int)q.Attribute("id"),
                        Pretext = (string)q.Element("Pretext"),
                        Answer = (string)q..Element("Answer")
                    };

        return query.ToList();
    }
}

As I understand logic of your blackList - you don't want to fetch same question several times and you want to have questions in random order. Than can be done really easy without any blacklists and looping while next random question is not in blacklist:

var repository = new QuestionRepositor();
var random = new Random();
var questions = repository.GetAllQuestions().OrderBy(q => random.Next());

That will parse xml, return questions and sort them in random order. Now your game loop can look like:

foreach(var question in questions)
{
    Ask(question); // extract all loop code to some method
    if (players.Any(IsWinner))
        break;
}

Extracted code can look like:

private void Ask(Question question)
{
     Console.WriteLine(question.Pretext);
     foreach(var player in players) 
     {
         if (question.IsAnswerCorrect(GetAnswer()))
             player.Score++;

         if (IsWinner(player))
             return;
     }
}

private bool IsWinner(Player player)
{
     return player.Score == WinningScore;
}

---

Your program has another parts to improve, but answer is already too big for question-answer format. Keep going and use recommendations above.

Answer 2

1. When using an auto-property like public string Name { get; set; } you don't add a backing field like you do in Player:

   private string name;
   public string Name { get; set; }
   

   The point of an auto-property is to generate the backing field for you. By adding private string name; you have added an additional field to your class that is not used. It should be deleted. The same goes for all the properties in your code.

2. A possible bug in GameLoop after finishing a game:

   if (choice == "Y")
   {
     //Reset player score first
     for (int z = 0; i < player.Length; i++)
     {
       player[z].Score = 0;
     }
     GameLoop(player, noOfPlayers);
   }
   else { Environment.Exit(0); }
   

   In the for loop you use z as a loop variable but increment and check i instead. Also you are calling GameLoop recursively which works in this case but it is an unsuall way of restarting the game. Theoretically after playing thousand of games in a row the call stack could get full and crash the game.

Answer 3

There are a lot of good inputs from both Sergey and Andris, so I'll make mine short :)

• You should avoid repetetive code like you have in your main method with the switch case.
• You should avoid large methods like GameLoops and GetMaxQuestions. Break them up into smaller methods.
• You can replace some of your for loops with foreach loops for simplicity.

One thing that is vital for refactoring and testability is that you understand whats happening inside a method without actually decifering the content. To achieve that use naming to clarify and avoid variables like question/questions unless they are in a foreach loop. Renaming questions to something like allAvailableQuestions would help you not to mix the two and should make it clearer what that array is for