What Is Feature Envy?

Photo by James Jeremy Beckers on Unsplash

Today’s topic is ‘Feature Envy’, a code smell and an indicator of deeper problems with the software. Feature Envy means that one class is ‘envious’ of, and contains, behaviour that rightly belongs to another. The code is in the wrong place: One type is doing too much and the other, not enough.

In extreme cases, we can end up with God-classes that fail to delegate work and do everything themselves. In my experience, much procedural code, written in object-oriented languages, is plagued by Feature Envy. 

By removing instances of Feature Envy, we get better encapsulation, code reuse and balancing of responsibilities between classes. 

Let’s look a couple of examples of Feature Envy: 

Example 1:

  private async Task Validate(CustomerRegistration registration)
  {
     if (registration == null)
        throw new MissingCustomerRegistration();

     if (string.IsNullOrWhiteSpace(registration.FirstName))
        throw new MissingFirstName();
     if (string.IsNullOrWhiteSpace(registration.LastName))
        throw new MissingLastName();
     if (string.IsNullOrWhiteSpace(registration.EmailAddress))
        throw new MissingEmailAddress();

     var existCust = await Repository.GetCustomer(registration.EmailAddress);
     if (existCust != null)
        throw new DuplicateCustomerEmailAddress(registration.EmailAddress);
  }

Validate() is doing more than it should. Can you identify the code that we could move to a better home?

That’s right; it’s the detailed validation of the CustomerRegistration parameter. The code is performing multiple validation checks that would make more sense in the CustomerRegistration class. Let’s move the code over into a new public Validate() method on CustomerRegistration:

  public class CustomerRegistration
  {
     // ...
     
     public void Validate()
     {
        if (string.IsNullOrWhiteSpace(FirstName))
           throw new MissingFirstName();
        if (string.IsNullOrWhiteSpace(LastName))
           throw new MissingLastName();
        if (string.IsNullOrWhiteSpace(EmailAddress))
           throw new MissingEmailAddress();
     }
  }

Now we are in a position to replace the detailed validation in the original Validate() method with a call to CustomerRegistration.Validate():

  private async Task Validate(CustomerRegistration registration)
  {
     if (registration == null)
        throw new MissingCustomerRegistration();
     registration.Validate();
     var existCust = await Repository.GetCustomer(registration.EmailAddress);
     if (existCust != null)
        throw new DuplicateCustomerEmailAddress(registration.EmailAddress);
  }

Much simpler and shorter than before! 

Example 2:

A private helper method IsPublicSchool() is defined as: 

  private bool IsPublicSchool(School school)
  {
     return school.Type == 2;
  }

, and called several times in conditional statements:

  if (IsPublicSchool(school))

The Type property is part of the School class. So why do we determine outside of class School whether a school is public or not? That does not feel right. Let’s fix this by adding an IsPublic property to School:

  public bool IsPublic => Type == 2;

The conditional statements simplify to:

  if (school.IsPublic)

Much better! 

How do we identify Feature Envy?

I think a decent indicator is when one class does an extended amount of work with the instance of another class. The registration validation in our first example was like that.

In our second example, we were building behaviour in another class that we could entirely determine inside the School type.

Whenever I remove some Feature Envy, the code feels more natural and balanced. It’s great.

Incorrect Use Of Partial Classes

Photo by Rob Necciai on Unsplash

Today’s tip deals with a feature of .NET: Partial classes. 

Is see partial classes abused from time to time. If you are not aware, .NET allows us to split a class definition across multiple source files. We decorate each of the class fragment definitions with the ‘partial’ keyword. At build time, the compiler takes the various partials and fuses them back into a single class. 

Why is this useful? 

Generally, this is a framework feature that is handy when we are working with autogenerated code. I’ll get into the details of that next time.

Today I would like to point out a misuse of partial classes.

For example, imagine an enormous AccountController class split across five files:

          AccountController.cs

          AdminAccountController.cs

          UserAccountController.cs

          SuperAdminAccountController.cs

          OrganisationAccountController.cs

In each of these files, AccountController is defined as a partial class:

  public partial class AccountController

With about 2,000 lines per file, AccountController contains approximately 10,000 lines of code. Each of the partials is already bloated, but the whole AccountController class is enormous. Huge classes like this can be a real challenge to work with. 

Unfortunately, breaking AccountController up over multiple source code files obscures the actual size of the class. Programming is communication with other developers. We do not want to disguise problems – we want to highlight them, so everyone’s aware of them.

A developer who is aware that a class is already 10,000 lines long may avoid adding more behaviour to it. However, if a class appears to be only 2,000 lines long, they may not be quite so hesitant. 

Splitting up a huge class by breaking it up over several files is an anti-pattern. It does not shrink the size of the class – it only appears to reduce it.

A better way is to break a large class into several smaller ones. If necessary, we can use patterns like Aggregation or Composition to reassemble the functionality into a larger whole again. 

In our example, the names of the AccountController files already indicate a breakdown by responsibilities. AdminAccountController.cs is probably the place where Admin-related AccountController behaviour resides. Similarly for OrganisationAccountController.cs and the other files. 

The fix might be as simple as renaming the AccountController class fragments to align with the file names. In the AdminAccountController.cs file, we change the declaration from 

  public partial class AccountController 

to  

  public class AdminAccountController 

Notice that we have removed the partial keyword. AdminAccountController is no longer a partial class – it’s now a separate class.  

Also, breaking down a large class might not always be this simple.

So, when is it appropriate to use partial classes?

There is an excellent reason to use partial classes. We’ll discover that next time.

Today’s takeaway is to avoid using partial classes to spread large classes over several files. It does not reduce class size and complexity but merely obscures it.

How To Write Good Unit Tests

Photo by Christina @ wocintechchat.com on Unsplash

What makes a good unit test?

Meaningful Naming

Are these helpful unit test names?

• Test_6
• Test_Something
• Accounts_Test

No, they are not. These names are devoid of meaning. Readers ought to be able to comprehend the purpose of a unit test immediately; 

A technique that I like is using Given/When/Then to name tests. Here are a couple of examples:

• Given_No_Matching_UseCase_When_Try_GetUseCase_Then_Throw_UnknownUseCase
• Given_No_AccountGroupName_When_Call_BuildQuery_Then_Throw_ArgumentNullException

But it doesn’t have to be Given/When/Then. For example, in the Bowling Game Kata, Roll_Guttergame is an excellent contextual unit test name for a rolling all zeros.

The reader should immediately get the idea of what a test is all about. There should be no head-scratching.

Easy to Understand

I have seen unit tests that were two pages long. There were many lines of setup and verification all on display. With tests like that, it’s difficult to discern which parts are relevant to a particular test and which are just mechanics.

When we have to brace ourselves before we dig into a unit test to try and understand what is going on, that is not a useful unit test. It should not be like we are trying to decipher the Dead Sea Scrolls.

What happens to tests that are hard to understand and maintain? When they start failing, they will get commented out or deleted.

The body of a unit test should be short and understandable. We must use all our programming prowess to make unit tests intuitive to understand.

Following is an example where I did not fully achieve this:

  [Theory]
  [InlineData(9876.5432, 9876.54)]
  [InlineData(5432.9876, 5432.99)]
  public async Task Given_4DP_Budget_When_Call_UpdateBudget_Then_Save_Budget_As_Rounded_To_2DP(
     decimal budget4DP, 
     decimal roundedBudget2DP)
  {
     var useCase = SetupUseCase();
     await useCase.UpdateBudget(new BudgetChange(AccountGroupName, budget4DP));
     VerifySaveNewBudget(useCase, roundedBudget2DP);
  }

As the name indicates, the test ensures that the UpdateBudget() method rounds incoming 4 decimal place budget figures to 2 decimal places.

The test has only three lines, which is nice and short. However, when we read the test code, there is at least one aspect that is a little bit off. The test code exposes an unnecessary detail which may cause minor confusion. Do you know what I mean? Maybe reread the test.

The test uses AccountGroupName. Why is this here? How is it involved in budget rounding? It’s not. AccountGroupName is needed to initialise BudgetChange. It’s a detail that is detracting from the meat of the unit test. 

What should we do? Easy. Hide it:

  [Theory]
  [InlineData(9876.5432, 9876.54)]
  [InlineData(5432.9876, 5432.99)]
  public async Task Given_4DP_Budget_When_Call_UpdateBudget_Then_Save_Budget_As_Rounded_To_2DP(
     decimal budget4DP, 
     decimal roundedBudget2DP)
  {
     var useCase = SetupUseCase();
     await useCase.UpdateBudget(SetupBudgetChange(budget4DP));
     VerifySaveNewBudget(useCase, roundedBudget2DP);
  }

We have relegated the instantiation of class BudgetChange to method SetupBudgetChange(). We have thus hidden AccountGroupName from the high-level test code.

So, the only moving parts the test is exposing are the ones that we are interested in: 

• the useCase instance that does the work,
• the incoming 4DP budget figure, and 
• the rounded 2DP budget. 

All other detail still exists but has been conveniently hidden away in helper methods.

What Is A Unit Test?

Photo by Nina Mercado on Unsplash 

In his highly recommended book, ‘The Art Of Unit Testing’, Roy Osherove describes a unit test as ‘an automated piece of code that invokes a unit of work in the system and then checks a single assumption about the behaviour of that unit of work.‘

It’s a good explanation. Let’s pick it apart. In places, I will layer my interpretation on top of Roy’s definition.

‘an automated piece of code…‘

It’s Automated; not manual. Manually testing a system is not automatic and therefore never a unit test. A computer runs a suite of unit tests. And Yes, it’s more code for developers to write. It’s up to us to produce the code for the unit tests and the implementation. Unit tests are meant to be meaningful and understandable by the developers who maintain the implementation code. To that end, a unit test should be written in the same computer language as the implementation. Imagine how awkward and difficult it would be to switch languages between unit tests, written in Python, but the system code written in C#. 

‘…invokes a unit of work in the system…‘

Real-world systems do many things. They carry out many functions. For a unit test, we want to focus on one such system function and exercise it. For the sake of clarity, this does not mean we are restricted to calling only the highest-level functionality in the system, say, like creating a new customer. Systems perform lower-level, detail-oriented functions, say, hashing a password, and they too are units of work. Anything a system can do is somewhere exposed as a public function. Each unit test focusses on exercising one ‘thing’ a system does, or one unit of work, at a time. 

‘…checks a single assumption about the behaviour of that unit of work.‘

Whenever we test a system, whether manually or automatically, in order of it to be a useful test, we must have certain expectations as to how the system should respond. For example, when we try to log in with invalid credentials, then the system will deny us access. A given input for an action will produce an expected result. It’s how predictable systems work. What Roy is saying here is that we should pick one output, one assumption, and check that. For example, a call of a high-level workflow method might perform these steps:

1. Try and get some information from a repository (e.g. database), and 
2. If there was no data, then exit early, or
3. If there was data, make some changes to that data, and
4. Try and save that data back to the repository.

Each of these steps is an assumption and ought to be separately tested via a unit test:

1. Do we try and get data from the repository?
2. If we didn’t get data, do we exit early?
3. If we did get data, do we make the correct changes to it?
4. Do we try and save the altered data back to the repository?

These are independent steps and so deserve at least one unit test each. 

I feel that there is a bit more to unit tests which we will examine next time.