7
\$\begingroup\$

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());
    }
}
}
\$\endgroup\$
10
  • 1
    \$\begingroup\$ We cannot help with adding additional implementation. It must be complete and working. \$\endgroup\$
    – Jamal
    Aug 13, 2014 at 22:33
  • \$\begingroup\$ @Jamal hey, i have edited the code to full, please take a look. \$\endgroup\$ Aug 13, 2014 at 22:42
  • 1
    \$\begingroup\$ If it still doesn't do the things you were asking us to implement, you can remove those requests and the question can be reopened. That aspect can still be asked on Stack Overflow. \$\endgroup\$
    – Jamal
    Aug 13, 2014 at 22:44
  • \$\begingroup\$ @Jamal ok, i think i understand now. I have removed the requests for implement and only for a better code organization. \$\endgroup\$ Aug 13, 2014 at 22:46
  • 1
    \$\begingroup\$ To me this looks like it's reviewable in the state it's in currently. But I would suggest that maybe you'd get a better review if you waited until it was completely finished and posted that instead. As it is, we can probably give you pointers in the right direction, but not much in the way of concrete examples like "look how much better it would be to do X rather than Y" \$\endgroup\$ Aug 13, 2014 at 23:20

3 Answers 3

10
\$\begingroup\$

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.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ You could also mention that playerAmount should be renamed to playerNumber (same thing with the input prompt). It's awkward to say "amount" in this context. \$\endgroup\$
    – Jamal
    Aug 14, 2014 at 0:35
  • \$\begingroup\$ @Jamal indeed! Phone post, will edit ;) \$\endgroup\$ Aug 14, 2014 at 0:44
  • \$\begingroup\$ Thanks for your answer, I'm starting over and try rewriting them correctly. \$\endgroup\$ Aug 14, 2014 at 1:20
4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Underscore is not an official coding convention in C#. It is highly debatable, with at most a 50% usage rate. I will now go about proving this to be true by saying not to use it. \$\endgroup\$
    – Magus
    Aug 14, 2014 at 23:15
  • \$\begingroup\$ I might be mistaken, I learned it that way and always thought it was a standard \$\endgroup\$
    – IEatBagels
    Aug 15, 2014 at 1:29
  • 2
    \$\begingroup\$ Don't worry, it's common enough. Some official .NET libraries were written that way, and some weren't. If you use Resharper (and probably other, similar tools), you can make color do the job of an underscore as well. \$\endgroup\$
    – Magus
    Aug 15, 2014 at 14:35
1
\$\begingroup\$

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());
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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