5
\$\begingroup\$

In the C# Player's Guide book there's a challenge in which you have to write the PigDice game in C#:

  1. It's a multiplayer game, every player starts with score 0.
  2. Player X rolls 2 dice.
  3. Player X's score is increased by the sum of the 2 dice' numbers.
  4. If Player X gets 1 on either die, his score becomes 0 again and it's the next player's turn.
  5. Player X can choose between play (roll dice) or hold (lose his turn)
  6. Player X wins if his score is equal or greater than N

Is my approach reasonable? How can I improve my program?

Gist Link

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PigDice
{
    class Program
    {
        static void CreatePlayers(Player[] players)
        {
            for (int index = 0; index < players.Length; ++index)
            {
                string defaultName = $"Player #{index + 1}";

                Console.Write($"{defaultName}'s name: ");
                string playerName = Console.ReadLine();

                players[index] = new Player(playerName == string.Empty ? defaultName : playerName);
            }
        }

        static void ShowGameStatus(Game game) => Console.WriteLine(game.GetScoresTable());

        static void Main(string[] args)
        {
            Console.WriteLine("Welcome to PigDice!");
            Console.Write("Amount of players: ");

            // Number of players that are going to play
            string inputNumber = Console.ReadLine();
            int numberOfPlayers = Convert.ToInt32(inputNumber);

            Game game = new Game(numberOfPlayers);
            Player[] players = game.Players;

            // Ask for and assign players' names
            CreatePlayers(players);

            Console.WriteLine("All set. Let the games begin!");

            // First player starts
            game.PlayingPlayer = players[0];

            // GameLoop returns false if ended
            while (GameLoop(game)) ;

            Console.WriteLine(game.PlayingPlayer + " has won!");
            Console.ReadKey();
        }

        private static bool GameLoop(Game game)
        {
            Player player = game.PlayingPlayer;

            ShowGameStatus(game);

            if (player.hasWon())
                return false;

            Console.Write($"{player}'s turn! Play (no)? ");

            string playersChoice = Console.ReadLine();

            // Handle player's choice to hold
            if (playersChoice.ToLower() == "no")
            {
                Console.WriteLine($"{player} chose to hold!");
                game.NextTurn();
                return true;
            }

            // Handle player's choice to play
            Dice dice = new Dice();

            Console.WriteLine($"{player} is rolling the dice!");

            int number1 = dice.Roll();
            int number2 = dice.Roll();

            Console.WriteLine($"{player} got {number1} and {number2}");

            // If one dice got 1, all score is lost
            if (number1 == 1 || number2 == 1)
            {
                Console.WriteLine($"Bad luck! {player} lost all points!");
                player.OverallScore = 0;
                game.NextTurn();
                return true;
            }

            // Player got a score increase
            int scoreWon = number1 + number2;
            player.OverallScore += scoreWon;

            Console.WriteLine($"{player} got {scoreWon} score and now has {player.OverallScore}");

            return true;
        }
    }
}

Player.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PigDice
{
    class Player
    {
        const int pointsToWin = 50; // instead of 100 which is 
                                    // incredibly hard to get

        public string PlayerName { get; }
        public int OverallScore { get; set; }

        public bool hasWon() => this.OverallScore >= pointsToWin;

        public Player(string playerName)
        {
            this.PlayerName = playerName;
        }

        public override string ToString()
        {
            return this.PlayerName;
        }
    }
}

Game.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PigDice
{
    class Game
    {
        public Player[] Players { get; }
        public int Turn { get; private set; }

        public Player PlayingPlayer { get; set; }

        public Game(int numberOfPlayers)
        {
            Players = new Player[numberOfPlayers];
        }

        public void NextTurn()
        {
            Turn += 1;
            PlayingPlayer = Players[Turn % Players.Length];
        }

        public string GetScoresTable() {
            string table = "----- Scores -----";

            foreach (Player player in Players)
                table += "\n" + player + ": " + player.OverallScore;

            return table;
        }
    }
}

Dice.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PigDice
{
    class Dice
    {
        private Random random = new Random();

        public int Roll() => random.Next(6) + 1;
    }
}
\$\endgroup\$
  • \$\begingroup\$ It may be worth your while to mention in the non-code part of your post that you have adapted the rules to use 50 points to win instead of 100. The description of the problem also says "Player X wins if his score is N" but in your code you made it such that the score has to be "N or more" rather than be exactly N. \$\endgroup\$ – Phrancis Jul 1 '16 at 23:36
  • 2
    \$\begingroup\$ random.Next(6) + 1; same as random.Next(1, 7); \$\endgroup\$ – t3chb0t Jul 2 '16 at 15:32
  • \$\begingroup\$ @t3chb0t Yes it is \$\endgroup\$ – Afonso Matos Jul 3 '16 at 14:23
2
\$\begingroup\$

My initial impression is that there seems to be quite a lot of shared responsibility, particularly between the static methods in Program and the rest of the code. For example:

private static bool GameLoop(Game game)

This really seems like it should belong in your Game class. It's also odd that this function doesn't actually contain the loop, so you have to call it from within one:

while (GameLoop(game)) ;

This seems counter intuitive. Perhaps it should be GetRound or something and called by GameLoop where both of them sit in the Game class...

Player Construction

Game contains an array of players, which it creates on construction, however it isn't responsible for populating the players. This tightly couples the Game class to CreatePlayers in your Program class. If GetScoresTable is called before the players are created the program will crash.

A better approach might be to have CreatePlayers actually create an array of players and then have the array passed into the constructor of Game, rather than the number of players. That way, Game is in a completely constructed state and is ready for other methods on the class to be called.

\$\endgroup\$

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.