Refactoring the Primitive Obsession code smell

The Primitive Obsession code smell refers to code that is using primitive types (ex: int, bool) instead of classes and enums. This defeats one of the benefits of object-oriented programming: encapsulation.

Code Smell: Primitive Obsession.

Definition: Using primitives instead of encapsulating them with a class.

Solution:

  • Encapsulate the primitive fields:
    • Move them to a new class.
    • Create new methods for any operations being done on the fields.
  • Replace type code primitives with enums.

Primitive Obsession code smell example

Here’s an example of the Primitive Obsession code smell. This is a snippet from the Yatzy Refactoring Kata. It’s only two methods from the class, including their tests, instead of the full class.

Yatzy class

public class Yatzy { public static int Ones(int d1, int d2, int d3, int d4, int d5) { var sum = 0; if (d1 == 1) sum++; if (d2 == 1) sum++; if (d3 == 1) sum++; if (d4 == 1) sum++; if (d5 == 1) sum++; return sum; } public static int LargeStraight(int d1, int d2, int d3, int d4, int d5) { int[] tallies; tallies = new int[6]; tallies[d1 - 1] += 1; tallies[d2 - 1] += 1; tallies[d3 - 1] += 1; tallies[d4 - 1] += 1; tallies[d5 - 1] += 1; if (tallies[1] == 1 && tallies[2] == 1 && tallies[3] == 1 && tallies[4] == 1 && tallies[5] == 1) return 20; return 0; } }

Yatzy tests

public class YatzyTest { [Fact] public void Fact_1s() { Assert.True(Yatzy.Ones(1, 2, 3, 4, 5) == 1); Assert.Equal(2, Yatzy.Ones(1, 2, 1, 4, 5)); Assert.Equal(0, Yatzy.Ones(6, 2, 2, 4, 5)); Assert.Equal(4, Yatzy.Ones(1, 2, 1, 1, 1)); } [Fact] public void largeStraight() { Assert.Equal(20, Yatzy.LargeStraight(6, 2, 3, 4, 5)); Assert.Equal(20, Yatzy.LargeStraight(2, 3, 4, 5, 6)); Assert.Equal(0, Yatzy.LargeStraight(1, 2, 2, 4, 5)); } }

Before we begin

Refactoring rule #1: Always make sure you have tests covering the code you’re about to refactor. Run the tests after each small step.

Introduce Parameter Object to the Ones() method

The Ones() method is being passed five primitives – d1, d2, d3, d4, d5. We’ll apply the Introduce Parameter Object refactoring by creating a new class called Dice, moving the primitives to that class, and passing in the Dice class to Ones() instead of the primitives.

1 – Create Dice class

namespace Yatzy { public class Dice { } }

2 – Add d1, d2, d3, d4, and d5 int fields to the Dice class

We’re copying the parameters from the Ones() method as fields in the Dice class. We need to initialize these fields in the Dice() constructor.

public class Dice { public readonly int d1, d2, d3, d4, d5; public Dice(int d1, int d2, int d3, int d4, int d5) { this.d1 = d1; this.d2 = d2; this.d3 = d3; this.d4 = d4; this.d5 = d5; } }

3 – Replace the int parameters with the Dice class

We need to pass in the Dice class and then use the five int fields we just created.

public static int Ones(Dice dice) { var sum = 0; if (dice.d1 == 1) sum++; if (dice.d2 == 1) sum++; if (dice.d3 == 1) sum++; if (dice.d4 == 1) sum++; if (dice.d5 == 1) sum++; return sum; }

4 – Fix compiler errors in the unit test

Because we changed the signature of the Ones() method, we’ll get a bunch of compile errors. We can lean on the compiler to find all the places we have to update. In this case, it’s simple, we just need to update the Fact_1s() test to pass in Dice objects.

[Fact] public void Fact_1s() { Assert.True(Yatzy.Ones(new Dice(1, 2, 3, 4, 5)) == 1); Assert.Equal(2, Yatzy.Ones(new Dice(1, 2, 1, 4, 5))); Assert.Equal(0, Yatzy.Ones(new Dice(6, 2, 2, 4, 5))); Assert.Equal(4, Yatzy.Ones(new Dice(1, 2, 1, 1, 1))); }

Encapsulate Fields in Dice class

Take a look at the Ones() method and realize how it’s using the Dice fields. It’s counting the number of dice with the value of 1. It isn’t worried about individual values, just the count. Therefore we can encapsulate these fields by adding a new method – CountWithValue().

1 – Add method CountWithValue() in Dice class

public int CountWithValue(int value) { return 0; }

2 – Make fields private

private readonly int d1, d2, d3, d4, d5;

3 – Update Ones() to use CountWithValue()

By setting the fields to private, the Ones() method can no longer access these fields. We need to change it to call CountWithValue().

public static int Ones(Dice dice) { return dice.CountWithValue(1); }

4 – Verify the Fact_1s test is failing

The test will fail because we haven’t actually implemented CountWithValue().

Yatzy Fact_1s unit test failing as expected

5 – Implement CountWithValue()

Given a value, CountWithValue() needs to return how many of the dice have that value. There are various ways to accomplish this. The client actually doesn’t care how this is implemented. That’s a nice benefit of encapsulation – the client doesn’t dictate how the code is implemented.

Let’s implement this by populating and using a count map.

  • Remove d1-d6 fields from the Dice class.
  • Add a countMap dictionary and initialize 1-6 with counts of 0.
  • Populate countMap in the constructor from the passed in dice values.
  • Update CountWithValue() to return the count for the given value.
using System.Collections.Generic; namespace Yatzy { public class Dice { private Dictionary<int, int> countMap = new Dictionary<int, int>() { {1, 0 }, {2, 0 }, {3, 0 }, {4, 0 }, {5, 0 }, {6, 0 } }; public Dice(int d1, int d2, int d3, int d4, int d5) { countMap[d1]++; countMap[d2]++; countMap[d3]++; countMap[d4]++; countMap[d5]++; } public int CountWithValue(int value) { return countMap[value]; } } }

Replace Type Code with Class to constrain the dice parameters

Because we’re using int parameters, the client can pass in whatever values a 32-bit integer supports. The parameters are unconstrained. Our domain – the Yatzy game – uses six-sided dice. This means the valid range of values is 1-6.

Let’s add a few tests that expose the problem by passing in invalid values.

[Fact] public void bugInRefactoredCode() { Assert.Equal(4, Yatzy.Ones(new Dice(8, 8, 8, 8, 8))); } [Fact] public void bugInExistingCode() { Assert.Equal(0, Yatzy.LargeStraight(-1, -1, -1, -1, -1)); }

Both of these tests will throw exceptions.

When we fixed the Primitive Obsession code smell in the Ones() method, we ended up pushing the smell into the Dice() constructor.

We’ll use the Replace Type Code with Class refactoring to fix the Primitive Obsession code smell in the Dice() constructor.

1 – Add DieValue enum

namespace Yatzy { public enum DieValue { One=1, Two=2, Three=3, Four=4, Five=5, Six=6 } }

Note: Dice is the plural of Die. The DieValue enum represents the value of a single Die.

2 – Replace int parameters in Dice() constructor with DieValue parameters

public Dice(DieValue d1, DieValue d2, DieValue d3, DieValue d4, DieValue d5)

3 – Fix all of the compiler errors

Changing the Dice() constructor parameters to DieValue results in several compiler errors. Once again, we’ll lean on the compiler to help us find all the code that we need to update.

  • Update the countMap to use DieValue as the key.
private Dictionary<DieValue, int> countMap = new Dictionary<DieValue, int>() { {DieValue.One, 0 }, {DieValue.Two, 0 }, {DieValue.Three, 0 }, {DieValue.Four, 0 }, {DieValue.Five, 0 }, {DieValue.Six, 0 } };
  • Change CountWithValue() to use a DieValue parameter.
public int CountWithValue(DieValue value) { return countMap[value]; }
  • Changing CountWithValues()’s parameter breaks the Ones() method. We’ll need to pass in DieValue.One.
public static int Ones(Dice dice) { return dice.CountWithValue(DieValue.One); }
  • Fix the Fact_1s test to pass in DieValue parameters.
[Fact] public void Fact_1s() { Assert.True(Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five)) == 1); Assert.Equal(2, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.Four, DieValue.Five))); Assert.Equal(0, Yatzy.Ones(new Dice(DieValue.Six, DieValue.Two, DieValue.Two, DieValue.Four, DieValue.Five))); Assert.Equal(4, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.One, DieValue.One))); Assert.Equal(4, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.One, DieValue.One))); }

Yes, it’s tedious. That’s the price we pay for having to refactor code smells.

  • Delete the bugInRefactoredCode test. We added this to expose the problem, but now we can’t pass in invalid values, therefore we can delete this test.
[Fact] public void bugInRefactoredCode() { Assert.Equal(4, Yatzy.Ones(new Dice(8, 8, 8, 8, 8))); }

Introduce Parameter Object to the LargeStraight() method

We already did most of the work when we introduced the Dice parameter to the Ones() method. We created and implemented the Dice class. For the LargeStraight() method refactoring, we’ll just pass in and use the existing Dice class.

1 – Replace LargeStraight()’s parameters with Dice class

public static int LargeStraight(Dice dice) { if (dice.CountWithValue(DieValue.Two) == 1 && dice.CountWithValue(DieValue.Three) == 1 && dice.CountWithValue(DieValue.Four) == 1 && dice.CountWithValue(DieValue.Five) == 1 && dice.CountWithValue(DieValue.Six) == 1) return 20; return 0; }

2 – Fix the compiler errors in the largeStraight test

We need to update the test to pass in Dice objects to LargeStraight().

[Fact] public void largeStraight() { Assert.Equal(20, Yatzy.LargeStraight(new Dice(DieValue.Six, DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five))); Assert.Equal(20, Yatzy.LargeStraight(new Dice(DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five, DieValue.Six))); Assert.Equal(0, Yatzy.LargeStraight(new Dice(DieValue.One, DieValue.Two, DieValue.Two, DieValue.Four, DieValue.Five))); }

3 – Delete the bugInExistingCode test

[Fact] public void bugInExistingCode() { Assert.Equal(0, Yatzy.LargeStraight(-1, -1, -1, -1, -1)); }

End results

We took care of the Primitive Obsession code smell by encapsulating the primitive parameters in the Dice class, and passing that to Ones() and LargeStraight() instead of the primitives. We further eliminated the code smell by replacing the primitives with the DieValue enum, therefore constraining the values that could be passed in.

Yatzy class

public class Yatzy { public static int Ones(Dice dice) { return dice.CountWithValue(DieValue.One); } public static int LargeStraight(Dice dice) { if (dice.CountWithValue(DieValue.Two) == 1 && dice.CountWithValue(DieValue.Three) == 1 && dice.CountWithValue(DieValue.Four) == 1 && dice.CountWithValue(DieValue.Five) == 1 && dice.CountWithValue(DieValue.Six) == 1) return 20; return 0; } }

Yatzy tests

public class YatzyTest { [Fact] public void Fact_1s() { Assert.True(Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five)) == 1); Assert.Equal(2, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.Four, DieValue.Five))); Assert.Equal(0, Yatzy.Ones(new Dice(DieValue.Six, DieValue.Two, DieValue.Two, DieValue.Four, DieValue.Five))); Assert.Equal(4, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.One, DieValue.One))); Assert.Equal(4, Yatzy.Ones(new Dice(DieValue.One, DieValue.Two, DieValue.One, DieValue.One, DieValue.One))); } [Fact] public void largeStraight() { Assert.Equal(20, Yatzy.LargeStraight(new Dice(DieValue.Six, DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five))); Assert.Equal(20, Yatzy.LargeStraight(new Dice(DieValue.Two, DieValue.Three, DieValue.Four, DieValue.Five, DieValue.Six))); Assert.Equal(0, Yatzy.LargeStraight(new Dice(DieValue.One, DieValue.Two, DieValue.Two, DieValue.Four, DieValue.Five))); } }

Leave a Comment