Two-player number guessing game

Question

I created a 2-player number guessing game. Each player picks a number between 1 and 10 and closest one wins.

I'm just looking for best practices suggestions and improvements.

Console program.cs

using ClassLibrary1;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace GameConsole
{
 class Program
 {
 static int[] _guesses = new int[2] { 0, 0 };
 const int ERROR_INPUT = 1000;
 const int ERROR_SAME = 1001;
 static int _numberOfRounds = 2;
 static Class1 _newGame;
 static List<string> _status = new List<string>();
 static void Main(string[] args)
 {
 _newGame = new Class1();
 do
 {
 Console.Clear();
 PrintScores();
 PromptPlayer(Player.One);
 PromptPlayer(Player.Two);
 //both guesses are valid at this point
 //Console.Clear();
 //_status.ForEach(x => Console.WriteLine(x));
 Player winner = _newGame.Play(_guesses);
 Console.WriteLine("And the winner of this round was Player {0}. The number was: {1}", Enum.GetName(typeof(Player), winner), _newGame.TargetNumber);
 Console.ReadKey();
 //Setup new round
 _guesses = new int[] { 0, 0 };
 _status = new List<string>();
 _newGame.CreateNewTargetNumber();
 } while (_newGame.PlayerOneScore < _numberOfRounds && _newGame.PlayerTwoScore < _numberOfRounds);
 Console.Clear();
 PrintScores();
 _status.ForEach(x => Console.WriteLine(x));
 if (_newGame.PlayerOneScore == _numberOfRounds)
 Console.WriteLine("Player One wins!");
 else
 Console.WriteLine("Player Two wins!");
 }
 private static void PrintScores()
 {
 _status.Add(string.Format("Player One: {0}", _newGame.PlayerOneScore));
 _status.Add(string.Format("Player Two: {0}", _newGame.PlayerTwoScore));
 _status.Add("---------");
 }
 private static void PromptPlayer(Player player)
 {
 bool isValid = false;
 do
 {
 Console.Clear();
 _status.ForEach(x => Console.WriteLine(x));
 if (_guesses[(int)player] == ERROR_INPUT)
 {
 Console.ForegroundColor = ConsoleColor.Red;
 Console.Write("Player {0}, enter valid number between 1 and 10: ", Enum.GetName(typeof(Player), player));
 Console.ForegroundColor = ConsoleColor.Gray;
 isValid = GetPlayerGuess(player);
 }
 else if (_guesses[(int)player] == 0)
 {
 Console.Write("Player {0} enter your guess? ", Enum.GetName(typeof(Player), player));
 isValid = GetPlayerGuess(player);
 }
 else if (_guesses[(int)player] == ERROR_SAME)
 {
 Console.ForegroundColor = ConsoleColor.Red;
 Console.Write("Player {0}, can't use same number: ", Enum.GetName(typeof(Player), player));
 Console.ForegroundColor = ConsoleColor.Gray;
 isValid = GetPlayerGuess(player);
 }
 if (isValid)
 {
 _status.Add(string.Format("Player {0} guess was: {1}", Enum.GetName(typeof(Player), player), _guesses[(int)player]));
 isValid = true;
 }
 } while (!isValid);
 }
 private static bool GetPlayerGuess(Player player)
 {
 string playerGuess = string.Empty;
 playerGuess = Console.ReadLine();
 int validGuess = 0;
 int.TryParse(playerGuess, out validGuess);
 if (validGuess > 0 && _guesses.Contains(validGuess))
 {
 _guesses[(int)player] = ERROR_SAME;
 return false;
 }
 else if (validGuess > 0 && validGuess <= 10)
 {
 _guesses[(int)player] = validGuess;
 return true;
 }
 else
 {
 _guesses[(int)player] = ERROR_INPUT;
 return false;
 }
 }
 }
}

Class1.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace ClassLibrary1
{
 public enum Player
 {
 One = 0,
 Two
 }
 public class Class1
 {
 public int TargetNumber { get; private set; }
 public int PlayerOneScore { get { return _scores[0]; } }
 public int PlayerTwoScore { get { return _scores[1]; } }
 private int[] _scores = new int[2] { 0, 0 };
 public Class1()
 {
 TargetNumber = new Random().Next(1, 10);
 }
 public void CreateNewTargetNumber() { TargetNumber = new Random().Next(1, 10); }
 public Player Play(int[] _guesses)
 {
 Player winner;
 int playerOneGuessInt = Convert.ToInt32(_guesses[0]);
 int playerTwoGuessInt = Convert.ToInt32(_guesses[1]);
 int playerOneDiff = Math.Abs(playerOneGuessInt - TargetNumber);
 int playerTwoDiff = Math.Abs(playerTwoGuessInt - TargetNumber);
 if (playerOneDiff == playerTwoDiff)
 {
 //tie
 //who didn't go over
 if (playerOneGuessInt < 0)
 {
 //Console.WriteLine("Player Two wins!");
 _scores[1]++;
 winner = Player.Two;
 }
 else
 {
 //Console.WriteLine("Player One wins!");
 _scores[0]++;
 winner = Player.One;
 }
 }
 else if (playerOneDiff < playerTwoDiff)
 {
 //Console.WriteLine("Player One was the closest. The number was {0}", target);
 _scores[0]++;
 winner = Player.One;
 }
 else
 {
 //Console.WriteLine("Player Two was the closest. The number was {0}", target);
 _scores[1]++;
 winner = Player.Two;
 }
 return winner;
 }
 }
}

• holroy: please don't change code after you've posted a question here at Code Review. That could invalidate answers, both existing (if present) and those in the making.

Answer 1

I see several things that could be changed/improved...

---

You dont't have to initialize array items if they should be zeros so this:

static int[] _guesses = new int[2] { 0, 0 };

is the same as this

 static int[] _guesses = new int[2];

---

You write a comment above those three lines:

//Setup new round
_guesses = new int[] { 0, 0 };
_status = new List<string>();
_newGame.CreateNewTargetNumber();

make it a method and the comment won't be necessary:

private static void SetupNewRound()
{
 _guesses = new int[2];
 _status = new List<string>();
 _newGame.CreateNewTargetNumber();
}

---

I think it would be a good idea to create a new Player class and give it some properties like:

class Player 
{
 public PlayerName Name { get; set; }
 public int Guesses { get; set; }
 public int Score { get; set; }
}

to encapsulate all those fields that hold data related to players like _guesses, PlayerOneScore, PlayerTwoScore, _scores etc. Currently player data is scattered over multiple classes and assemblies. You hold some of it in Class1 and _guesses in Program.

Your enum then gets the name PlayerName and you change _guesses to _players and make it a dictionary of players:

Dictionary<PlayerName, Player> _players = new Players[] 
{
 Player { Name = PlayerName.One },
 Player { Name = PlayerName.Two },
}.ToDictionary(p => p.Name);

This change will allow you to remove lot's of code (several fields) and make your classes simpler and easier to read.

So a line like this one:

else if (_guesses[(int)player] == 0)

would become:

else if (_players[playerName].Guesses == 0)

or instead of hard coding player names or their count in strings

_status.Add(string.Format("Player One: {0}", _newGame.PlayerOneScore));

you'll have

foreach(var player in _game.Players.Values)
{
 status.Add(string.Format("Player {0}: {1}", player.Name, player.Score));
}

and your application will be more flexible.

---

I guess the name of Class1 is just an accident because you gave everything else very good names ;-)

and when I see this line:

static Class1 _newGame;

its name should probably be Game.

• Rod: Awesome! thanks so much for taking the time to share some insights. I really appreciate it.

Answer 2

if (_guesses[(int)player] == ERROR_INPUT) and else if (_guesses[(int)player] == ERROR_SAME) contains four lines which are almost identical, except for the message. Instead of copy-pasting you should have moved them to a method:

private void WarnPlayer(string message)
{
 Console.ForegroundColor = ConsoleColor.Red;
 Console.Write(message, Enum.GetName(typeof(Player), player));
 Console.ForegroundColor = ConsoleColor.Gray;
}

---

I'd move PromptPlayer and GetPlayerGuess to a separate class instead of keeping them in Program.

• Rod: maybe in the Player class as mention in first review?
• BCdotWEB: @Rod No, that Player class should be used to store data. I can't think of a name right now, however.