Problem
This implements a builder pattern for RPG game characters. Is this a valid builder pattern implementation?
- There are 3 characters: Paladin, Wizard and Elfo.
- There are 3 items: Potion, Sword, Arch.
using System;
using System.Collections.Generic;
using NUnit.Framework;
namespace RPGEngine.Tests
{
[TestFixture]
public class RPGEngineTest
{
[Test]
public void GivenAPaladin_WithSword_Return()
{
//Arrange
var paladin = new
CharacterBuilder()
.Init(new Paladin())
.WithSword()
.Named("Conan")
.Build();
//Act
var result = paladin.Attack();
//Assert
Assert.AreEqual("Conan has splitted the enemy in two", result);
}
[Test]
public void GivenAPaladin_WithArch_Return()
{
//Arrange
var paladin = new
CharacterBuilder()
.Init(new Paladin())
.WithArch()
.Named("Conan")
.Build();
//Act
var result = paladin.Attack();
//Assert
Assert.AreEqual("Conan has hitted the enemy's heart", result);
}
[Test]
public void GivenAnElfo_WithArch_Return()
{
//Arrange
var elfo = new
CharacterBuilder()
.Init(new Elfo())
.WithArch()
.Named("Légolas")
.Build();
//Act
var result = elfo.Attack();
//Assert
Assert.AreEqual("Légolas has hitted the enemy's heart", result);
}
[Test]
public void GivenAnElfo_WithPotion_Return()
{
//Arrange
var elfo = new
CharacterBuilder()
.Init(new Elfo())
.WithPotion()
.Named("Légolas")
.Build();
//Act
var result = elfo.Attack();
//Act
//Assert
Assert.AreEqual("Légolas has converted the enemy in monkey", result);
}
[Test]
public void GivenAWizard_WithPotion_Return()
{
//Arrange
var Wizard = new
CharacterBuilder()
.Init(new Wizard())
.WithPotion()
.Named("Merlín")
.Build();
//Act
var result = Wizard.Attack();
//Assert
Assert.AreEqual("Merlín has converted the enemy in monkey", result);
}
[Test]
public void GivenAWizard_WithSword_Return()
{
//Arrange
var Wizard = new
CharacterBuilder()
.Init(new Wizard())
.WithSword()
.Named("Merlín")
.Build();
//Act
var result = Wizard.Attack();
//Assert
Assert.AreEqual("Merlín has splitted the enemy in two", result);
}
}
public static class Message
{
private const String MessageForSword = "{0} has splitted the enemy in two";
private const String MessageForArch = "{0} has hitted the enemy's heart";
private const String MessageForPotion = "{0} has converted the enemy in monkey";
private static Dictionary<AttackType, String> messageTypes = new Dictionary<AttackType, String>()
{
{ AttackType.Sword , MessageForSword },
{ AttackType.Arch, MessageForArch },
{ AttackType.Potion, MessageForPotion }
};
public static string GetResultOf(AttackType typeOfAttack)
{
return messageTypes[typeOfAttack];
}
}
public enum AttackType
{
Sword,
Arch,
Potion
}
public interface Item
{
string Use();
}
public class Sword : Item
{
public string Use()
{
return Message.GetResultOf(AttackType.Sword);
}
}
public class Arch : Item
{
public string Use()
{
return Message.GetResultOf(AttackType.Arch);
}
}
public class Potion : Item
{
public string Use()
{
return Message.GetResultOf(AttackType.Potion);
}
}
public interface ICharacter
{
String Attack();
Item Item { get; set; }
String Name { get; set; }
}
public class Paladin : ICharacter
{
public String Name { get; set; }
public Item Item { get; set; }
public String Attack()
{
return string.Format(Item.Use(), this.Name);
}
}
public class Elfo : ICharacter
{
public String Name { get; set; }
public Item Item { get; set; }
public String Attack()
{
return string.Format(Item.Use(), this.Name);
}
}
public class Wizard : ICharacter
{
public String Name { get; set; }
public Item Item { get; set; }
public String Attack()
{
return string.Format(Item.Use(), this.Name);
}
}
public class CharacterBuilder
{
private ICharacter _Personaje;
public CharacterBuilder Init(ICharacter personaje)
{
_Personaje = personaje;
return this;
}
public CharacterBuilder WithSword()
{
this._Personaje.Item = new Sword();
return this;
}
public CharacterBuilder WithArch()
{
this._Personaje.Item = new Arch();
return this;
}
public CharacterBuilder WithPotion()
{
this._Personaje.Item = new Potion();
return this;
}
public CharacterBuilder Named(string name)
{
this._Personaje.Name = name;
return this;
}
public ICharacter Build()
{
return this._Personaje;
}
}
Solution
Styling Nitpicks
private ICharacter _Personaje;
I like the underscore prefix for private fields. I like it, because I don’t like plain camelCase fields for two reasons:
- They look just like parameters and local variables
- Disambiguating them from parameters/locals involves sprinkling redundant
thisqualifiers all over the code, which I find amounts to annoying junk.
So I like the underscore prefix you put there. What I don’t like though, is that the field is PascalCase with an underscore. Private fields should be camelCase, or _camelCase.
One more nitpick: string is a C# language alias for the System.String class. Both are completely equivalent, in absolutely every single possible situation. I can live with either being used. I can even live with string being used as a type in declarations and String being used when calling string methods such as String.Format. But inconsistency is annoying:
public interface Item
{
string Use();
}
public interface ICharacter
{
String Attack();
Item Item { get; set; }
String Name { get; set; }
}
public String Attack()
{
return string.Format(Item.Use(), this.Name);
}
I’d just use string everywhere.
Builder Review
public CharacterBuilder Init(ICharacter personaje)
{
_Personaje = personaje;
return this;
}
So wait a minute…
var builder = new CharacterBuilder().WithSword();
This would throw a rather surprising NullReferenceException, because the Init method wasn’t called and the private field isn’t initialized. How about constructor-injecting the ICharacter instance instead?
var character = new Wizard();
var builder = new CharacterBuilder(character).WithSword();
And now the private field can be made readonly, because it’s initialized in the constructor and the CharacterBuilder object is always in a valid state.
There’s a problem though:
var character = new CharacterBuilder(new Wizard())
.WithSword()
.WithPotion()
.WithArch()
.WithPotion()
.WithPotion()
.Build();
What am I expecting here? And what am I getting? An unnamed wizard with 1 potion. And I thought I’d be getting an unnamed wizard archer with a sword and 3 potions.
The problem is that ICharacter can only ever have 1 name and 1 item: this isn’t a very good scenario for a builder pattern.
I would suggest to change the interface a bit:
public interface ICharacter
{
string Name { get; }
Item Item { get; set; }
string Attack();
}
So that you could have:
public class Paladin : ICharacter
{
public Paladin(string name, Item item)
{
Name = name;
Item = item;
}
public string Name { get; }
public Item Item { get; set; }
public string Attack()
{
return string.Format(Item.Use(), this.Name);
}
}
I left Item get/set, because I would think a character’s equipment could change throughout the game (perhaps the Paladdin would use a LongSword or a MythrilSword at one point) – the point is, if there’s an instance of an object, it’s in a valid state. Always.
With your current code, this blows up with a NullReferenceException:
var player = new Paladin();
player.Attack();
Drop that builder, it’s not helping you.
Now, what would be a valid case for a builder? Say you wanted to generate a village:
var village = new VillageBuilder()
.Building(new SmallHouse())
.Building(new SmallHouse())
.Building(new LargeHouse())
.Building(new ArmorShop())
.Building(new WeaponShop())
.Building(new Inn())
.Villager(new WomanInRed())
.Villager(new MadScientist())
.Villager(new PossessedChild())
.Build();
You see the difference? The interface for a Village doesn’t know how many Building or Villager it’s going to have (nor what actual type they might be) – the builder pattern is useful here, because you can build villages with completely different features using the exact same builder code, and the Build method could be called at any given point, the village object would always be in a valid state.
Mat’s Mug made some really good points. I would like to add a few things.
I don’t like the Message class. What will you do when you’ll want to have a spear? A gun? 10 other weapons? 10 more items, which are not weapons? Your class will quickly grow to a point, where it will be real pain to maintain. This looks much better in my opinion:
public class Sword : Item
{
public ICharacter Owner { get; set; }
public string Use()
{
return String.Format("{0} has splitted the enemy in two", Owner);
}
}
If you want to have a place to hold your strings – add them to .resx file. Visual Studio will generate a class to access those strings via properties.
public string Use()
{
return String.Format(Resources.UseSwordMessage, Owner);
}
Also, interface names should have an I prefix, so it should be IItem.
I wouldn’t have defined an action method for ICharacter interface and its child classes because if you need to create new actions for them you would need to copy and paste the same code for each ICharacter type you defined.
Instead I would create a seperate Interface/Class for defining the character action methods and give the class constructor an ICharacter parameter.
Also try to keep in mind that if you want to create a more scaleable program your classses should only have one responsibility. In your case ICharacter classes have the responsibility to keep data. Give doAction kind of responsibilities to another one. And make them talk to each other. This will save you from copy/pasting your code and also you won’t need to remember which paste goes where.
It is the S of SOLID principles:
See here.