Simple "arrow-throwing" console game

Question

I'm trying to make a simple console application game. The code is a bit messy and I hope anyone can point out anything wrong.

using System;
using System.Collections.Generic;

namespace CalculatePoints
{
class Program
{
    static void Main(string[] args)
    {
        Game game = new Game();
        game.PlayGame();
    }
}

class Game
{
    private List<Player> player = new List<Player>();

    public void AddPlayer(string name)
    {
        Player person = new Player(name);
        player.Add(person);
    }

    public void PlayGame()
    {
        Console.Write("enter amount of player: ");
        int playerAmount = Convert.ToInt32(Console.ReadLine());

        for (int i = 0; i < playerAmount; i++)
        {
            Console.Write("enter player name: ");
            string playerName = Console.ReadLine();
            AddPlayer(playerName);
        }

        Console.WriteLine("game start");
        foreach (Player name in player)
        {
            Console.WriteLine(name + " turn ");

            Console.WriteLine("enter score 1:");
            int score1 = Int32.Parse(Console.ReadLine());

            Console.WriteLine("enter score 2:");
            int score2 = Int32.Parse(Console.ReadLine());

            Console.WriteLine("enter score 3:");
            int score3 = Int32.Parse(Console.ReadLine());

            Throw result = new Throw(score1, score2, score3);
            Console.WriteLine(result.GetScore());
        }
        Console.ReadKey();
    }
}

class Player
{
    public string Name { get; set; }
    public List<Throw> throwList = new List<Throw>();

    public Player(string name = "")
    {
        Name = name;
    }

    public override string ToString()
    {
        return Name;
    }
}

class Throw
{
    private int throwOne;
    private int throwTwo;
    private int throwThree;

    public Throw(int throwOne = 0, int throwTwo = 0, int throwThree = 0)
    {
        this.throwOne = throwOne;
        this.throwTwo = throwTwo;
        this.throwThree = throwThree;
    }

    public int GetScore()
    {
        return throwOne + throwTwo + throwThree;
    }

    public override string ToString()
    {
        return string.Format("Your total score is {0}", GetScore());
    }
}
}

Answer 1

I like that your Program class is kept to a minimum. That's a very good start.

The player field in the Game class has a bad/misleading name: it's not one player, it's a list of players. players would be better. Also I'd make that field readonly to make it clear that the reference of that list cannot be tampered with. I'd also consider passing that list of players through the constructor as an argument... but that would break your code, because PlayGame is doing too many things.

The first part of PlayGame should be extracted out of the class: it's more of a GetGamePlayers method than a part of the actual game code.

The name Throw, throws me off: throw is a keyword reserved for throwing exceptions; considering the nature and purpose of the class, perhaps PlayerTurn would be a better choice.

---

The Player class is doing something terribly wrong: it's exposing a public field! The throwList can be reassigned and messed with from the outside, which completely ruins encapsulation. Consider exposing it with an IEnumerable<T> property getter instead.

Similarly, the Name setter should be private.

Also, playerAmount is really a playerCount - "amount" is typically better suited for quantities of something, money in particular comes to mind.

---

For flexibility, you shouldn't hard-code the 3 throws in your Throw (/PlayerTurn) class: should the rules change and players are now allowed a 4th dart, or if a player's turn can end with only one or two darts thrown, the hard-coded throwOne, throwTwo and throwThree become a problem. Granted, the optional parameters help a bit, but why store values that you don't need? I'd have gone with a List here too.

Answer 2

I have some minor points to add to everyone's answers.

Your Throw class fields should be prefixed by _ as it is the C# coding convention (check comments below!), doing this would also let you not use this in your constructor

private int _throwOne;
private int _throwTwo;
private int _throwThree;

public Throw(int throwOne = 0, int throwTwo = 0, int throwThree = 0)
{
    _throwOne = throwOne;
    _throwTwo = throwTwo;
    _throwThree = throwThree;
}

In your Player class you expose a public field, which is a List, you shouldn't do that. Instead you should expose a read-only property interface (Like ICollection<> or IEnumerable<>) and initialize this property in your constructor.

public ICollection<Throw> ThrowList {get;private set;}

public Player(string name = "")
{
    Name = name;
    ThrowList = new List<Throw>();
}

Finally, your application will crash if the user enters anything that isn't an Int32 in the prompts for the scores or the number of players. You should validate your input and maybe re-ask the user for the score/number of players until it is a valid input.

Answer 3

I have few bits of code:

Pass the collection together, rather than individually, as you might need to change it.

As you are reading from Console.ReadLine, create a method that takes the value according to the type (up to your judgement).

A few variable are redundant, so you can remove them.

public class Game
{
    private readonly List<Player> _player = new List<Player>();

    private T GetConsoleValue<T>()
    {
        var value = Console.ReadLine();
        return (T)Convert.ChangeType(value, typeof(T), CultureInfo.InvariantCulture);
    }

    public void AddPlayer(string name)
    {
        Player person = new Player(name);
        _player.Add(person);
    }

    public void PlayGame()
    {
        Console.Write("enter number of player: ");
        var playersCount = GetConsoleValue<int>();

        for (var count = 0; count < playersCount; count++)
        {
            Console.Write("enter player name: ");
            string playerName = Console.ReadLine();
            AddPlayer(playerName);
        }

        Console.WriteLine("game start");
        foreach (var name in _player)
        {
            Console.WriteLine(name + " turn ");

            Console.WriteLine("enter score 1:");
            var score1 = GetConsoleValue<int>(); 

            Console.WriteLine("enter score 2:");
            var score2 = GetConsoleValue<int>();

            Console.WriteLine("enter score 3:");
            var score3 =  GetConsoleValue<int>();

            var result = new Throw(new List<int>
            {
                score1,
                score2,
                score3
            });
            Console.WriteLine(result.GetScore());
        }
        Console.ReadKey();
    }
}

public class Player
{
    public string Name { get; set; }

    public Player(string name)
    {
        Name = name;
    }

    public override string ToString()
    {
        return Name;
    }
}

public class Throw
{
    private readonly IEnumerable<int> _throwScores;

    public Throw(IEnumerable<int> throwScores)
    {
        _throwScores = throwScores;
    }

    public int GetScore()
    {
        return _throwScores.Sum();
    }

    public override string ToString()
    {
        return string.Format("Your total score is {0}", GetScore());
    }
}