patterncsharpModerate
Simple "arrow-throwing" console game
Viewed 0 times
simplearrowthrowinggameconsole
Problem
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 = new List();
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 throwList = new List();
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());
}
}
}Solution
I like that your
The
The first part of
The name
The
Similarly, the
Also,
For flexibility, you shouldn't hard-code the 3 throws in 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 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.Context
StackExchange Code Review Q#59956, answer score: 10
Revisions (0)
No revisions yet.