Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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>
share|improve this question
2  
Consider to ask for review of single class/method instead of asking to review whole application code. Reviewing whole application and writing feedback is time-consuming activity –  Sergey Berezovskiy Jan 31 at 10:03
2  
@SergeyBerezovskiy I hear you (been there!), however remember that you may address only some aspects of the code in your answer - I know it's compelling to review the entire app at once, but you can answer with more than one shorter answers that address different aspects. In an effort to increase the number of per-question answers on CR, we encourage shorter reviews - yours was a great answer nonetheless. Thank you for your contributions, keep answering, and feel free to join us (CR addicts) in chat anytime! ;) –  Mat's Mug Jan 31 at 12:28
    
Sergey I will remember that till next time, nevertheless thanks for taking the time to help me out :). –  Overly Excessive Jan 31 at 12:59
    
I'm not going to add to the excellent feedback already provided. But as far as style e.g. naming conventions are concerned check out Lance Hunt's C# Coding Standards. –  David Jan 31 at 15:49
add comment

3 Answers 3

up vote 14 down vote accepted

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.

share|improve this answer
    
OrderBy and Random might not be the best solution to shuffle a list: stackoverflow.com/questions/1287567/… –  Andris Jan 31 at 11:05
    
@Andris not best for performance, but best for development time and it has good readability. Premature optimization is evil –  Sergey Berezovskiy Jan 31 at 11:11
1  
At first glance the non-deterministic nature or Random seamed to be a problem but after reading about it a bit I find it to be a simple, short and good enough solution. –  Andris Jan 31 at 11:22
1  
First of all. Thanks alot for taking the time to review my code and leave helpful feedback. I'm not so familiar with the naming conventions yet so I'll make sure to check out the MSDN article you mentioned. I'm gonna read through the other things you linked, your code is definitely a lot cleaner. Thanks again! –  Overly Excessive Jan 31 at 12:52
add comment
  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.

share|improve this answer
    
Visual studio was warning me about the unused fields, I thought it was strange but now it all makes sense. Also good catching that bug! :) –  Overly Excessive Jan 31 at 12:58
    
To add to your comment on that snippet, in Java, if(choice == "Y") would simply check for the memory location of "Y", and we should instead be using .Equals() for string equality. IIRC it's the same in C#. Am I right? –  theGreenCabbage Jan 31 at 15:00
1  
In C# comparison operator is overloaded for String. –  Kuba Wyrostek Jan 31 at 15:06
1  
@KubaWyrostek It might be worthwhile to take a look at some gotcha's in connection with the comparison operator: blogs.msdn.com/ericlippert/archive/2009/04/09/… –  Andris Feb 3 at 6:28
    
Very nice article! Thank you! –  Kuba Wyrostek Feb 3 at 16:43
add comment

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

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.