Discussion Avoiding bugs with optional setup arguments

I've long be bothered by this particular subtle anti-pattern that can emerge at times when writing code, and have been pondering what different kind of strategies can be employed to minimize the risks of code like this causing bugs in the codebase.

Here's an example demonstrating the troublesome situation (there's a bug in this code):

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public Player() : this(Id.None) { }
4.  
5.     public Player(Id id) : base(id) => PlayerManager.Register(id, this);
6. }
7.  
8. abstract class Entity
9. {
10.     public Id Id { get; }
11.  
12.     protected Entity() : this(Id.None) { }
13.  
14.     protected Entity(Id id) => Id = id != Id.None ? id : Id.NewId();
15. }

The mistake made in the above code was using the id parameter to register the Player entity, instead of the Id property. The prior could have the value of Id.None, while the latter will always hold a valid Id associated with the entity - either an existing one provided as an argument (that was perhaps loaded from disk), or a new one that was generated on-the-fly.

I feel like having this pattern in your code is the most dangerous when inheritance is involved, because it complicates the control flow, and causes the same risk of an error being reintroduced with the implementation of each derived class. But it can happen with any method with optional arguments like this.

I've used many different approaches to tackle this issue when I've encountered it, but I'm curious to hear if others have noticed this, or have any go-to solutions for it?

 

I think this is the problematic statement.

The instance should NOT register itself with something in the outside world. From the Player class perspective, it should not have to know about the existance of a system that manages its instances. This creates a circular dependency after all: PlayerManager can't work without Player, naturally. But Player will also not be functionally complete without also calling into PlayerManager.

Assume you were to declare PlayerManager and all other Entity managers in one assembly, and Player + Entity in another, then this issue would be dead obvious.

Instead, these registry systems should subscribe to an event in Entity:

public event Action<Entity> OnEntityInstantiated;

PlayerManager subscribes to this event and checks if 'entity is Player' and then registers that entity by its Id property.

Of course you can also have a separate event in Player like OnPlayerInstantiated since only a small percentage of Entity instances will be players.

The other obvious alternative is to instantiate Player instances by calling:

Code (CSharp):

1. PlayerManager.Instance.CreatePlayerInstance();

Now PlayerManager creates and registers that instance in one go.

 

That is a valid point This is not actual code from any project, mind you, just a synthetic example I concocted to help demonstrate the parameter-vs-member issue. After getting rid of that two-way dependency, the bug still remains:

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public static event Action<Player> Instantiated;
4.  
5.     public Player() : this(Id.None) { }
6.     public Player(Id id) : base(id) => Instantiated?.Invoke(id, this);
7. }

 

So I can think of a couple of ways to tackle the issue:

Approach 1
Refactor the code, so that Entity only accepts non-empty ids, generating the new ids in higher level code instead. This way the parameter and the property will have identical values, and it doesn't matter which one is used to register the Player entity.

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public Player(Id id) : base(id) => Register(id, this);
4. }
5.  
6. abstract class Entity
7. {
8.     public Id Id { get; }
9.  
10.     protected Entity(Id id)
11.     {
12.         if(id == Id.None) throw new InvalidArgumentException("Entity constructor must be provided a non-empty id.")
13.         Id = id;
14.     }
15. }

Approach 2
Use a data structure like Nullable<Id>, or Optional<Id>, to better highlight the fact that the Id can have a value of "none". With this approach, the compiler can help ensure that no mistakes are made:

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public Player() : this(null) { }
4.  
5.     public Player(Id? id) : base(id) => Register(id, this); // <- compile error
6. }

Approach 3
Use a variable name that better highlights the fact that the Id can have a value of None.

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public Player() : this(Id.None) { }
4.  
5.     public Player(Id idOrNone) : base(idOrNone) => Register(idOrNone, this); // <- more clear now that this is wrong
6. }

My favourite is approach 1. But I think applying both approaches 2 and 3 simultaneously should be enough help to avoid the pitfall in practice as well.

 

Simple fix:

Code (CSharp):

1. sealed class Player : Entity
2. {
3.     public static event Action<Player> Instantiated;
4.  
5.     public Player() : this(Id.None) { }
6.     public Player(Id id) : base(id) => Instantiated?.Invoke(this);
7. }

Do you see it?
It actually fixes the code too because it wouldn't compile.

The event receiver implements:

Code (CSharp):

1. public void OnInstantiated(Player player)
2. {
3.     DoThatRegisterThing(player.Id, player);
4. }

No need to pass that Id along. It's part of the Entity.

 

Good one!

The constructor is clearly the only problematic region of code, where it's possible to mistakenly use the optional parameter, instead of the property. So extracting any logic where the entity's id is used outside of the constructor, in one way or another, I feel is really the best approach.

So basically split the setup into two steps:

1. Only have minimal amount of code in the constructor (or initialization method in unity Objects); optimally only assign arguments to members.
2. Move any additional setup logic into a separate method. E.g. a factory method.

So pretty much just following normal constructor best practices.

To avoid the issue in other kinds of methods, could use method overloading to similarly isolate the code for resolving the default value of an optional argument from all other logic.

Code (CSharp):

1. void Method() => Method(GetDefaultService());
2.  
3. void Method(Service argument)
4. {
5.    // do something with the argument
6. }

 

Yes, this deferred initialization can be a solution in many cases. I once also made a registry class for objects and when you deserialize a complex structure which allows self and cross references you will always run into chicken-and-egg issues. However by using callbacks to actually "query" elements from the registry this problem is a no-brainer. So the registry simply collects delegates of things that want a reference to a certain ID and if that isn't already in the registry, those delegates are simply stored under that ID. Once the object with that ID registers itself, we can inform all the piled up delegates about the object and from now on when another object wants the object reference, the request method can directly invoke the callback rather than stashing it for later. This is essentially lazy initialization.

 

This thread is interesting food for thought. Was in a similar situation needing to ensure certain instance of objects got added to a registry (in this case, runtime created items instances). I opted to use a builder pattern, in which completing the build adds said item to the item instance registry. Code's a bit convoluted though, especially when compared to just using a static delegate to inform of any instances created. Something to look at...