RPG Character Builder

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", resu

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 this qualifiers 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.

StackExchange Code Review Q#100754, answer score: 20