3
\$\begingroup\$

I wrote a little console game that is supposed to take some dice and some players and determine the winner of the game. I wrote it for the sole purpose of figuring out why i'm doing the things i'm doing and to better my skills. This way when I move onto learning a new language I can use this game as a guidance tool. I'd love some feedback on what could be done to improve this/make it more readable/if you see any errors/any standards that i'm missing/ misusing anything/ bad OOP especially. Even if something isn't done wrong but there is a better way to do it that would be great.

I didn't comment on purpose because it's not incredibly complicated and that would add a hundred lines.

Program.cs

class Program
{
    static int Main(string[] args)
    {
        Console.WriteLine("Welcome to Dice Game..." + Environment.NewLine);

        return Game.Setup();

    }
}

Game.cs

public static class Game
{

    public static List<Player> Players;
    public static List<Die> Dice;
    public static List<string> ComputerNames = new List<string> { "Herold", "Juan", "Jack", "Dodson", "Watson", "Tom", "Jillian", "Sarah", "Leonardo", "Jane" };

    public static int Setup()
    {
        Console.WriteLine("Press any key to start. Press Q to Quit.");

        string start = string.Empty;
        while (string.IsNullOrEmpty(start))
        {
            start = Console.ReadLine();
        }

        if (start != "q" && start != "Q")
        {
            Start();
        }

        return Quit();
    }

    public static void Start()
    {
        Dice = new List<Die>();
        Settings.GetSettingsFromUser();
        GetPlayers();
        DisplayPlayers();
        Begin(Players);
        Setup();
    }

    public static int Quit()
    {
        return 0;
    }

    private static void GetPlayers()
    {
        Players = new List<Player>();

        for(int i = 1; i <= Settings.NumberOfHumanPlayers; i++)
        {
            Player player = new Player(i);
            Players.Add(player);
        }

        for (int i = 1; i <= Settings.NumberOfAIPlayers; i++)
        {
            Player player = new Player(i + Settings.NumberOfHumanPlayers, isAi: true);
            Players.Add(player);
        }
    }

    public static void DisplayPlayers()
    {
        Debug.Assert(Players != null && Players.Count >= 2);

        Console.WriteLine("Players in the game...");

        foreach(Player p in Players)
        {
            Console.WriteLine(p.PlayerNumber + ". " + p.Name);
        }
    }

    public static void Begin(List<Player> players)
    {
        Debug.Assert(Players != null && Players.Count >= 2);
        Debug.Assert(Dice != null && Dice.Count > 0);

        for (int i = 1; i <= Settings.NumberOfRounds; i++)
        {
            foreach (Player p in players)
            {
                Console.WriteLine("Player " + p.PlayerNumber + "(" + p.Name + ") " + "roll... (Press enter to roll)");
                Console.ReadLine();
                Console.WriteLine("Player " + p.PlayerNumber + "(" + p.Name + ") " + "rolled a " + p.RollDice() + Environment.NewLine);
            }
            GetRoundWinner(players);
        }
        GetWinner();
    }

    private static void GetRoundWinner(List<Player> players)
    {
        if (Settings.HighScoreWins)
        {
            GetHighScoreWinner(players);
        }
        else
        {
            GetLowScoreWinner(players);
        }
    }

    private static void GetHighScoreWinner(List<Player> players)
    {
        int maxScore = 0;
        List<Player> winners = new List<Player>();
        foreach (Player p in players)
        {
            if (p.RoundScore >= maxScore)
            {
                maxScore = p.RoundScore;
            }
        }

        foreach (Player p in players)
        {
            if (p.RoundScore == maxScore)
            {
                winners.Add(p);
            }
        }

        if (winners.Count > 1)
        {
            Begin(winners);
        }
        else
        {
            foreach (Player p in Game.Players)
            {
                if (winners.First().PlayerNumber == p.PlayerNumber)
                {
                    p.AddWin();
                }
            }
        }
    }

    private static void GetLowScoreWinner(List<Player> players)
    {
        int minScore = int.MaxValue;
        List<Player> winners = new List<Player>();
        foreach (Player p in players)
        {
            if (p.RoundScore <= minScore)
            {
                minScore = p.RoundScore;
            }
        }

        foreach (Player p in players)
        {
            if (p.RoundScore == minScore)
            {
                winners.Add(p);
            }
        }

        if (winners.Count > 1)
        {
            Begin(winners);
        }
        else
        {
            foreach (Player p in Game.Players)
            {
                if (winners.First().PlayerNumber == p.PlayerNumber)
                {
                    p.AddWin();
                }
            }
        }
    }

    private static void GetWinner()
    {
        int maxWins = 0;
        List<Player> winners = new List<Player>();

        foreach(Player p in Game.Players)
        {
            if(p.Wins >= maxWins)
            {
                maxWins = p.Wins;
            }
        }

        foreach (Player p in Game.Players)
        {
            if (p.Wins == maxWins)
            {
                winners.Add(p);
            }
        }

        if(winners.Count == 1)
        {
            Console.WriteLine("Player " + winners.First().PlayerNumber + "(" + winners.First().Name + ") won!!!");
        }
        else
        {
            Console.WriteLine("Tie!");
            Console.Write("Players ");
            foreach(Player p in winners)
            {
                if(winners.Last() == p)
                {
                    Console.Write(p.PlayerNumber + "(" + p.Name + ") ");
                }
                else
                {
                    Console.Write(p.PlayerNumber + "(" + p.Name + "), ");
                }

            }
            Console.Write("Tied!");
        }
        Console.Write(Environment.NewLine + Environment.NewLine);
    }
}

Settings.cs

public static class Settings
{
    public static bool HighScoreWins { get; set; } = true;
    public static int NumberOfRounds { get; set; } = 1;
    public static int NumberOfHumanPlayers { get; set; }
    public static int NumberOfAIPlayers { get; set; }

    public static void GetSettingsFromUser()
    {
        GetGameType();
        GetDice();
        GetNumberOfRounds();
        GetNumberOfHumanPlayers();
        GetNumberOfAIPlayers();            
    }

    private static void GetGameType()
    {
        string gameType = string.Empty;

        while (gameType != "H" && gameType != "h" && gameType != "L" && gameType != "l")
        {
            Console.WriteLine("High(H) or Low(L)?");
            gameType = Console.ReadLine();
        }

        if(gameType == "H" || gameType == "h")
        {
            HighScoreWins = true;
        }
        else
        {
            HighScoreWins = false;
        }
    }

    private static void GetDice()
    {
        int numberOfDice = 0;
        while (numberOfDice <= 0)
        {
            Console.WriteLine("How many dice?");
            int.TryParse(Console.ReadLine(), out numberOfDice);
        }

        bool playStandard = false;

        Console.WriteLine("Play with standard 6 sided dice? (y)");
        string stdDiceInput = Console.ReadLine();
        if(stdDiceInput == "y" || stdDiceInput == "Y")
        {
            playStandard = true;
        }

        for(int i = 1; i <= numberOfDice; i++)
        {
            Die die;
            if (playStandard)
            {
                die = new Die();
            }
            else
            {
                int numberOfSides = 0;
                while (numberOfSides <= 0)
                {
                    Console.WriteLine("How many sides on die " + i + "?");
                    int.TryParse(Console.ReadLine(), out numberOfSides);
                }
                die = new Die(numberOfSides);
            }
            Game.Dice.Add(die);
        }
    }

    private static void GetNumberOfRounds()
    {
        int numberOfRounds = 0;
        while (numberOfRounds <= 0)
        {
            Console.WriteLine("How many rounds?");
            if (int.TryParse(Console.ReadLine(), out numberOfRounds))
            {
                NumberOfRounds = numberOfRounds;
            }
        }
    }

    private static void GetNumberOfHumanPlayers()
    {
        int numberOfHumanPlayers = 0;
        while (numberOfHumanPlayers <= 0)
        {
            Console.WriteLine("How many human players?");
            if (int.TryParse(Console.ReadLine(), out numberOfHumanPlayers))
            {
                NumberOfHumanPlayers = numberOfHumanPlayers;
            }
        }
    }

    private static void GetNumberOfAIPlayers()
    {
        int numberOfAIPlayers = 0;
        int minNumberOfAI = 0;

        if(NumberOfHumanPlayers == 1)
        {
            minNumberOfAI = 1;
        }
        if(NumberOfHumanPlayers == 0)
        {
            minNumberOfAI = 2;
        }

        do
        {
            Console.WriteLine("How many computer opponents?");
            if (int.TryParse(Console.ReadLine(), out numberOfAIPlayers))
            {
                NumberOfAIPlayers = numberOfAIPlayers;
            }

        }
        while (numberOfAIPlayers < minNumberOfAI || numberOfAIPlayers > Game.ComputerNames.Count);
    }
}

Player.cs

public class Player
{
    public Player(int playerNumber, bool isAi = false)
    {
        PlayerNumber = playerNumber;
        if (isAi)
        {
            IsAI = true;
            GenerateAIName();
        }
        else
        {
            RequestPlayerName();
        }
    }

    public string Name { get; set; }
    public int Wins { get; set; }
    public bool IsAI { get; set; }
    public int RoundScore { get; set; }
    public int PlayerNumber { get; set; }        

    private void RequestPlayerName()
    {
        Console.WriteLine("What is the " + PlayerNumber + " players name?");
        Name = Console.ReadLine();
    }

    private void GenerateAIName()
    {
        Random rand = new Random();
        while (string.IsNullOrEmpty(Name))
        {
            int index = rand.Next(1, Game.ComputerNames.Count);
            if (Game.Players.Where(x => x.Name == Game.ComputerNames[index]).FirstOrDefault() == null)
            {
                Name = Game.ComputerNames[index];
            } 
        }           
    }

    public void AddWin()
    {
        Wins++;
    }

    public int RollDice()
    {
        int rollScore = 0;
        foreach(Die die in Game.Dice)
        {
            rollScore += die.Roll();
        }
        RoundScore = rollScore;
        return rollScore;
    }
}

Die.cs

public class Die
{
    public Die(int numberOfSides = 6)
    {
        this.NumberOfSides = numberOfSides;
    }

    public int NumberOfSides { get; set; }

    public int Roll()
    {
        Random rand = new Random();
        return rand.Next(1, NumberOfSides + 1);
    }
}
\$\endgroup\$
4
\$\begingroup\$

Your static classes Game and Settings contain static state. In general it is not good and should be avoided. I recommend remove static keyword from these classes definitions and add Instance field/property to them. So Game and Settings will be singletons.

You have some difficulties with bools. This code

if(gameType == "H" || gameType == "h")
{
    HighScoreWins = true;
}
else
{
    HighScoreWins = false;
}

can be replaced with

HighScoreWins = gameType == "H" || gameType == "h";

This code

bool playStandard = false;

Console.WriteLine("Play with standard 6 sided dice? (y)");
string stdDiceInput = Console.ReadLine();
if(stdDiceInput == "y" || stdDiceInput == "Y")
{
    playStandard = true;
}

can be replaced with

Console.WriteLine("Play with standard 6 sided dice? (y)");
string stdDiceInput = Console.ReadLine();
var playStandard = stdDiceInput == "y" || stdDiceInput == "Y";

This large code

int maxWins = 0;
List<Player> winners = new List<Player>();

foreach(Player p in Game.Players)
{
    if(p.Wins >= maxWins)
    {
        maxWins = p.Wins;
    }
}

foreach (Player p in Game.Players)
{
    if (p.Wins == maxWins)
    {
        winners.Add(p);
    }
}

can be rewritten as

var players = Game.Players;

var maxWins = players.Select(p => p.Wins).DefaultIfEmpty(0).Max();
var winners = players.Where(p => p.Wins == maxWins).ToList();

This loop

int rollScore = 0;
foreach(Die die in Game.Dice)
{
    rollScore += die.Roll();
}

can be replaced with

var rollScore = Game.Dice.Sum(d => d.Roll());
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ahhh yes, all of the boolean detail is awesome, thank you for that. I was actually curious about making everything static when I started building this though. Because I thought the reason something should be static is that it would be used throughout the entire life while the app ran. I figured game would at least be a good fit for that, or no? \$\endgroup\$ – Marsh Jul 17 '17 at 21:24
1
\$\begingroup\$

Creating a new random in each call is bad. On quick calls could get the same seed and Next.

public class Die
{
    Random rand;
    public Die(int numberOfSides = 6)
    {
        this.NumberOfSides = numberOfSides;
    }    
    public int NumberOfSides { get; set; }
    public int Roll()
    {
        return rand.Next(1, NumberOfSides + 1);
    }
    public Die (Random Rand)
    {
       rand = Rand;
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I didn't know that it would create the same seed and next. Why and how can that happen? \$\endgroup\$ – Marsh Jul 17 '17 at 21:30
  • \$\begingroup\$ Because it can get the same seed. Spin up 100 new Random in a loop and get the first 10 new and see for yourself. \$\endgroup\$ – paparazzo Jul 17 '17 at 21:35
  • 2
    \$\begingroup\$ @Mash See msdn.microsoft.com/en-us/library/system.random.aspx#Multiple \$\endgroup\$ – Rick Davin Jul 17 '17 at 21:42
  • \$\begingroup\$ ahh awesome, it uses the system clock to generate a seed on the default constructor. Thanks a ton for putting me on that! \$\endgroup\$ – Marsh Jul 17 '17 at 22:00
  • \$\begingroup\$ I just earned close vote privilege. After that not really many privileges I care about. \$\endgroup\$ – paparazzo Jul 17 '17 at 22:08

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.