Primitive obsession

When reviewing pull requests, the code smell I always encounter is the Primitive obsession. For those of you who don't know, it's a design smell where the code relies too much on using primitive types like bool, int, string, guid, etc. As you'll see, this approach leads to bugs and issues that can hurt your project pretty severely.

Problems

Primitive types are interchangeable

One of the main issues with primitive obsession is that values of the same type can be replaced by one another without breaking the compiler, even if they are two different things. e.g., A phone number and a first name are two separate concepts and shouldn't be both represented as strings.

public class Person
{
  public(string firstName, string phoneNumber)
  {
    this.FirstName = firstName;
    this.PhoneNumber = phoneNumber;
  }
  
  public string FirstName { get; set; }
  public string PhoneNumber { get; set; }
}

var p = new Person("Miguel", "1-123-123-1122");

Problems occur when someone refactors that piece of code and change the order of the parameters.

public class Person
{
  public(string phoneNumber, string firstName)
  {
    this.FirstName = firstName;
    this.PhoneNumber = phoneNumber;
  }
  
  public string FirstName { get; set; }
  public string PhoneNumber { get; set; }
}

var p = new Person("Miguel", "1-123-123-1122");

For the compiler, the code is still valid and compiles, but let me tell you that you'll have some nasty behaviors when you run this in production.

DRY

Like the name says, Primitive types are, well... primitive, which means they contain no logic. The main issue arises when you want to use those values safely. You have no choice but to perform some checks like; not null, not empty, etc. It may seem fine the first few time you have to do it, but over time what tends to happen is that you'll duplicate that logic all over the place in your application. It will lead to a codebase that is very hard to maintain and which is in direct violation of the Don't Repeat Yourself (DRY) principle.

Cognitively complex method signatures

Process(true, false, false, true);

Have you ever seen anything like this? Those kinds of methods are fragile and hard to understand. Even when looking at the method definition, you don't know how you should use all the parameters. Are some parameters exclusive? Do some parameters override the behaviors of others? Moreover, as we all know, developers are terrible at naming stuff, so even if you look at the method signature, you could be misled.

Process(bool enable, bool reverse, bool includeException, bool runSafely)

Solution

The solution to primitive obsession is, in fact, quite simple. You have to leverage the C# compiler type's system by creating a type for everything. It may seem overkill at first, but in the long run, it always pays off. I know... there's a lot a boilerplate code to create a new class in C#, but seriously is it that long? Two minutes maybe? Ask yourself how long it will take to find and fix a bug caused by the potential problems outlined earlier?

C# 9.0 is coming up with improvements regarding the boilerplate code required to create a class. Stay tuned.

public class FirstName
{
  public FirstName(string firstName)
  {
    if(string.IsNullOrWhiteSpace(firstName))
      throw new ArgumentNullException();
    this.Value = firstName;
  }
  public string Value { get; set; }
}

public class PhoneNubmer
{
  public PhoneNubmer(string phoneNubmer)
  {
    if(string.Any(x => Char.IsLetter(x)))
      throw new Exception("Phone number cannot contain letters");
    this.Value = phoneNubmer;
  }
  public string Value { get; set; }
}

public class Person
{
  public(FirstName firstName, PhoneNumber phoneNumber)
  {
    this.FirstName = firstName;
    this.PhoneNumber = phoneNumber;
  }
  
  public FirstName FirstName { get; set; }
  public PhoneNumber PhoneNumber { get; set; }
}

var p = new Person(
            new FirstName("Miguel"), 
            new PhoneNumber("1-123-123-1122"));

As for the Process(...) example, an excellent way to fix it is to create an enum or a class named something like ProcessOptions. That class will be responsible only to allow valid combinations of options. It will make the business rules of all of those different use cases explicit and prevent some of the issues expressed earlier.

Bonus

Separating those concerns in other classes also has another great benefit. It makes the logic super easy to test as you don't have to bootstrap or mock extensively to test that kind of class. Look at the FirstName class, how hard would it be to unit test it in isolation?

Even the services that use those types are easier to test as you can now assume that the objects are always valid if they were constructed successfully. Over time, it will reduce the cyclomatic complexity of your methods and, thus, the code path you need to test.