patterncsharpMinor
Simple Text Adventure: Cleaning up after the party
Viewed 0 times
aftersimplethetextpartyadventurecleaning
Problem
This week's Code Review Weekend Challenge is about implementing a simple console game.
Here's my game loop:
So I have a
The player has an
So the role of the
The game's mechanics are tucked inside a
```
public abstract class GameScreen
{
protected IDictionary> MenuItems;
public abstract GameScreen Run();
protected void Write(string text)
{
Console.Write(text);
Console.WriteLine();
Console.WriteLine("[ENTER]");
Console.ReadLine();
}
protected string Prompt(string text)
Here's my game loop:
class Program
{
static void Main(string[] args)
{
var player = new Player();
var intro = new IntroScreen(player);
var nextScreen = intro.Run();
while (nextScreen != null)
{
nextScreen = nextScreen.Run();
}
}
}So I have a
Player class:public class Player
{
public string Name { get; set; }
public ObservableCollection Inventory { get; private set; }
public Player()
{
Inventory = new ObservableCollection();
Inventory.CollectionChanged += Inventory_CollectionChanged;
}
private void Inventory_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
{
if (e.Action != NotifyCollectionChangedAction.Add) return;
foreach (InventoryItem item in e.NewItems)
{
Console.WriteLine("Received '{0}'!\n({1})", item.Name, item.Description);
}
Console.WriteLine();
Console.ReadLine();
}
public bool HasItem(string name)
{
return Inventory.Any(item => item.Name.ToLower() == name.ToLower());
}
}The player has an
Inventory that's made up of InventoryItem instances:public struct InventoryItem
{
public string Name { get; set; }
public string Description { get; set; }
}So the role of the
Player is really to carry an inventory, whatever that is; whenever an item is added to the player's inventory, there's an output to the console.The game's mechanics are tucked inside a
GameScreen abstract class:```
public abstract class GameScreen
{
protected IDictionary> MenuItems;
public abstract GameScreen Run();
protected void Write(string text)
{
Console.Write(text);
Console.WriteLine();
Console.WriteLine("[ENTER]");
Console.ReadLine();
}
protected string Prompt(string text)
Solution
Well, i am new to this weekend-challenge thing so I am not sure, from which stand point should i review such question. I'll give it a try.
-
I like your game loop. Its simple and makes sense. It can be simlified further though:
-
I don't like your intro screen. It feels like it consists of two separate screens (1 - where you try to recall your name, 2 where you clean up). As it is - it is somewhat difficult to follow the execution path.
-
I think instead of being a protected method
-
I dont like the use of an observable collection, i think it is an overkill. And it exposes the methods it should not. A simple wrapper around List which only exposes
-
-
-
In general, i do not like implementation which uses hardcoded strings. I think that proper implementation should use scripts or (if the implementation should be C#-only) XML parsing. But i guess its a bit too much work for a two-hour challenge, right? :)
-
I like your game loop. Its simple and makes sense. It can be simlified further though:
var activeScreen = new IntroScreen(player);
while (activeScreen != null) // do-while, if you are not a hater ;)
{
activeScreen = activeScreen.Run();
}-
I don't like your intro screen. It feels like it consists of two separate screens (1 - where you try to recall your name, 2 where you clean up). As it is - it is somewhat difficult to follow the execution path.
-
I think instead of being a protected method
Menu should be an actual screen class which accepts options as constructor parameter. That would simplify reading.-
I dont like the use of an observable collection, i think it is an overkill. And it exposes the methods it should not. A simple wrapper around List which only exposes
AddItem and HasItem methods would be much batter. At very least - collection should not be public.-
Player should probably be a protected property of your base screen.-
return Inventory.Any(item => item.Name.ToLower() == name.ToLower()); I think ToLower() is confusing. Imho you should not manipulate properties which you use as an ID. Its better to make sure that all the green bags are called "GREEN BAG" instead (by saving it to constant field, for example)-
In general, i do not like implementation which uses hardcoded strings. I think that proper implementation should use scripts or (if the implementation should be C#-only) XML parsing. But i guess its a bit too much work for a two-hour challenge, right? :)
Code Snippets
var activeScreen = new IntroScreen(player);
while (activeScreen != null) // do-while, if you are not a hater ;)
{
activeScreen = activeScreen.Run();
}Context
StackExchange Code Review Q#38203, answer score: 5
Revisions (0)
No revisions yet.