HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Feedback requested on simple trivia

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
triviafeedbacksimplerequested

Problem

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 MAX_PLAYERS || noOfPlayers blacklist = new List();
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 en

Solution

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:


   


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 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.

Code Snippets

[Conditional("DEBUG")]
public static void DebugPrint(string printOut) 
{
    Console.WriteLine("[DEBUG]: {0}", printOut);
}
private static Logger Logger = LogManager.GetCurrentClassLogger();
Logger.Debug("Initializing questions...");
class Player
{        
    public string Name { get; set; } // back-storage will be generated
    public int Score { get; set; }
}
public class Question
{
    public int Id { get; set; }
    public string Pretext { get; set; }
    public string Answer { get; set; }
}

Context

StackExchange Code Review Q#40523, answer score: 15

Revisions (0)

No revisions yet.