Обратная связь по простым мелочам
Я начинающий программист-новичок, я начал немного зацикливаться на C # около двух месяцев назад, так как я начну изучение формального программирования примерно через 7 месяцев. Я слышал, как лучший способ узнать, как кодировать код, чтобы я написал это тривиальное приложение, и я хотел бы получить конструктивную обратную связь, какие хорошие вещи? И какие вещи я мог бы улучшить /сделал лучше.
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>
2 ответа
Первое примечание - не используйте константу DEBUG_MODE, чтобы определить, следует ли печатать отладочное сообщение или нет. Есть лучшие способы сделать это. Вы можете использовать условный атрибут , чтобы проверить, является ли символ компиляции " DEBUG ". Вызовы методов, отмеченных условным атрибутом, полностью удалены из сгенерированного кода, если символ не определен («DEBUG» определяется по умолчанию при запуске приложения в режиме отладки):
[Conditional("DEBUG")]
public static void DebugPrint(string printOut)
{
Console.WriteLine("[DEBUG]: {0}", printOut);
}
Другим вариантом является использование библиотеки журналов (NLog, log4net и т. д.). Вы сможете определить цели ведения журнала (или добавочные файлы), которые будут писать сообщения в консоль, файл, базу данных и т. Д. Вы сможете определять уровни протоколирования сообщений из файла конфигурации без изменения кода приложения. Например. с NLog вы можете получить экземпляр журнала
private static Logger Logger = LogManager.GetCurrentClassLogger();
И используйте его для написания отладочных сообщений, если сообщение об уровне отладки включено в конфигурации (вы можете изменить формат сообщения из конфигурации):
Logger.Debug("Initializing questions...");
Вернуться к приложению. Я предлагаю вам следовать стилям заглавной буквы для типов имен, методов и переменные, предложенные Microsoft. Например. локальные переменные должны иметь имена camelCase. Также не используйте префиксы и сокращения в именах переменных - pNames менее читабельны, чем playerNames. И трудно понять, что такое listA и listB, почему запросы имеют имена pQuery и aQuery и их имена переменных запроса lQ и aQ
Если вы используете автоматически реализованные свойства, вам не нужно определять поля:
class Player
{
public string Name { get; set; } // back-storage will be generated
public int Score { get; set; }
}
Класс Question странный. Первое, что не очевидно, это то, что он инициализируется, когда вы проверяете вопросы max что-то. Я предлагаю вам создать класс Question, который будет представлять данные, которые у вас есть в xml:
public class Question
{
public int Id { get; set; }
public string Pretext { get; set; }
public string Answer { get; set; }
}
Я предлагаю не записывать имена файлов жесткого кода в приложении. Переместить имя файла вопросов для перемещения в настройки приложения:
<appSettings>
<add key="questionsFileName"
value="C:\EasyTrivia\EasyTrivia\Questions.xml"/>
</appSettings>
И переместите логику доступа к данным в отдельный класс (с этим классом Question вы можете использовать атрибуты сериализации Xml для десериализации вопросов из XML-файла), например
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();
}
}
Как я понимаю логику вашего blackList - вы не хотите получать один и тот же вопрос несколько раз, и вы хотите задать вопросы в случайный порядок. Чем можно сделать очень легко без каких-либо черных списков и циклов, а следующий случайный вопрос не в черном списке:
var repository = new QuestionRepositor();
var random = new Random();
var questions = repository.GetAllQuestions().OrderBy(q => random.Next());
Это будет анализировать xml, возвращать вопросы и сортировать их в произвольном порядке. Теперь ваш игровой цикл может выглядеть так:
foreach(var question in questions)
{
Ask(question); // extract all loop code to some method
if (players.Any(IsWinner))
break;
}
Извлеченный код может выглядеть так:
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;
}
У вашей программы есть другие улучшения, но ответ слишком велик для формата вопрос-ответ. Продолжайте и используйте рекомендации выше.
Есть много хороших материалов от Сергея и Андриса, поэтому я сделаю свой короткий:)
- Вам следует избегать повторного кода, как у вас в основном методе, с корпусом коммутатора.
- Вам следует избегать больших методов, таких как GameLoops и GetMaxQuestions. Разбейте их на более мелкие методы.
- Вы можете заменить некоторые из ваших циклов foreach петлями для простоты.
Одна вещь, которая жизненно важна для рефакторинга и тестируемости, заключается в том, что вы понимаете, что происходит внутри метода без фактического разделения содержимого. Чтобы добиться этого, используйте именование, чтобы уточнить и избежать таких переменных, как вопрос /вопросы, если они не находятся в цикле foreach. Переименование вопросов на что-то вроде allAvailableQuestions поможет вам не смешивать их и должно быть более ясным, что этот массив для