4
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ 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. \$\endgroup\$
    – holroy
    Oct 24 '15 at 0:15
8
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Awesome! thanks so much for taking the time to share some insights. I really appreciate it. \$\endgroup\$
    – Rod
    Oct 24 '15 at 20:32
0
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ maybe in the Player class as mention in first review? \$\endgroup\$
    – Rod
    Oct 26 '15 at 18:17
  • \$\begingroup\$ @Rod No, that Player class should be used to store data. I can't think of a name right now, however. \$\endgroup\$
    – BCdotWEB
    Oct 26 '15 at 21:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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