patterncsharpModerate
Feedback requested on simple trivia
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
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):
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
And use it to write debug messages if debug level message are enabled in configuration (you can change message format from config):
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 -
If you are using auto-implemented properties, than you don't need to define fields:
Your
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.
As I understand logic of your
That will parse xml, return questions and sort them in random order. Now your game loop can look like:
Extracted code can look like:
Your program has another parts to improve, but answer is already too big for question-answer format. Keep going and use recommendations above.
[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.