1. 1. Intention in Naming
2. 2. Advanced SRP
3. 3. Strategy for Strategies
4. 4. Over-Engineered?
5. 5. Never Perfect

The other day in mob we were talking about a piece of code. My team was working on what is essentially a quota or throttling service. We want to be able to gently ramp up the number of users. The code ended up looking something like this (as usual, code is simplified for example purposes):

interface ICapacityProvider {
  bool GetAndUpdateCapacity(string customerId);
  void SetSettings(CapacitySettings settings);
  CapacitySettings GetSettings();
}

class CapacityApi {
  bool GetCapacity(string customerId) {
    return this.customerService.IsEnabled(customerId)
        && this.capacityProvider.GetAndUpdateCapacity();
  }
}

class CapacitySettings {
  public Duration Period { get; set; }
  public int UserLimit { get; set; }
}

I stated that I found a few things “weird” or suboptimal and we proceeded to discuss it. This discussion drew out a few ideas regarding code organization that I want to share here.

Intention in Naming

I had an objection to the naming of GetAndUpdateCapacity. Usually when I see a name with “and” in it I immediately check it for a violation of the Single Responsibility Principle. In this case the method checks to see if there is capacity and if so records another usage. Since this behavior is so closely connected it didn’t make sense to split the work into two methods.

The implementation of GetAndUpdateCapacity at its core is:

if (tokens.Count < capacitySettings.UserLimit)
{
  tokens.Add(now);
  return true;
}
return false;

Since clear intention is an important principle in our team we are careful to make sure that names convey what the code is doing. The team felt the name accurately described what was happening and that it was important the name captured the update so that callers wouldn’t be surprised when state was mutated. I agreed.

And also disagreed.

ICapacityProvider is an interface. The team had decided to use the Strategy Pattern and therefore the expectation is that the interface can be implemented by many different algorithms, many of which would not actually update anything, e.g. by querying a tally of events made by some other process. By including “AndUpdate” in the name the interface was signaling an implementation detail that was not true. This is mirrored by the fact that ICacapityApi‘s method is called GetCapacity instead of GetAndUpdateCapacity—the fact that updating was happening was not passed along the hierarchy.

In actuality, the team was really trying to signal that the code was not idempotent. However “AndUpdate” doesn’t really signal that as there are plenty of update implementations that could be idempotent. In reality, the language is missing any sort of contract or signal that a method is idempotent (short of adding our own [Idempotent] attribute) so an attempt was made to signal that intention through naming.

Finally we settled on the name TryRegister. Still not a great name but it better indicates what is happening. The “try” implies a Boolean success/failure result and “register” is a better clue of what is happening—recording the use of the system—that doesn’t necessarily imply an “update” is happening.

Advanced SRP

The next thing I had an issue with was having {Get,Set}Settings on the same interface as TryRegister. For context, the team was in the process of making the settings configurable, in this case CapacityApi was getting matching members that would be called by and REST endpoint to receive the settings. Their justifications for putting settings get/set on the capacity provider were:

• This had to do with capacity.
• The capacity provider needed access to ISerialization anyway.
• The capacity provider was the best class to know about settings.

I asserted that putting the settings there violated the Single Responsibility Principle for a couple of reasons. By choosing the Strategy Pattern it meant that there could be multiple algorithms to determine if we had capacity. Likewise, it’s reasonable to assume that there could be multiple strategies for storing the settings. However, the strategies for capacity and the strategies for settings did not correlate so the coupling was unnecessary. Changing a capacity strategy also meant settings might be fetched from a different place ncessitating a change in behavior unless care was taken to maintain identical settings in each location. Furthermore, some algorithms might not use the settings at all!

Additionally, some strategies for retrieving settings might not have a corresponding method for configuring them. They may be hardcoded, pulled from something like Zookeeper, or calculated based on instance count. This would lead to some providers having SetSettings throw NotSupportedException which would violate the Liskov Substitution Principle.

In fact, we were preparing to change where we stored the settings to a different location that allowed us to update them more easily. That meant that we would be passing in a second ISerialization interface which further showed that settings were orthogonal to the algorithm, not to mention the confusion and difficulty with having two parameters of the same type when dealing with an Inversion of Control container.

Strategy for Strategies

A final part that I felt could be changed is the actual implementation of CapacityApi. One thing I found interesting was this piece of code:

return this.customerService.IsEnabled(customerId)
    && this.capacityProvider.GetAndUpdateCapacity(customerId);

We had chosen a strategy for capacity but were calling into another service to do the initial check on the user. I proposed that the first half of the expression was itself a strategy for determining capacity. It was—to my first point—a strategy that didn’t update and—to my second point—didn’t use the settings.

The first expression was rewritten as:

  class ServiceEnabledCapacityProvider : ICapacityProvider {
    bool GetAndUpdateCapacity(customerId)=> this.customerService.IsEnabled(customerId);
  }

And here is the new implementation of CapacityApi:

public class CapacityApi : ICapacityApi {
  // multiple ICapacityProviders registered with IoC container in order of priority/effort
  readonly IOrderedEnumerable<ICapacityProvider> capacityProviders;

  // Constructor omitted for brevity

  public bool HasCapacity(string tenantId, string userId)
    => this.capacityProviders.All(p => p.TryRegister(tenantId, userId));
}

Simple, succinct, and very testable.

Over-Engineered?

One question came up as to whether this solution was overkill. That’s of course subjective and difficult to answer. We certainly didn’t have plans for additional algorithms so worrying about naming and factoring wasn’t important from a YAGNI point of view. The team initially chose to use a provider, though I made the case that CapacityApi could have easily handled the contents of CapacityProvider without straying too far from the Single Responsibility Principle.

Ultimately, I let principles and the context guide how far I go into engineering. Since we chose the strategy pattern it meant that splitting the algorithm from the settings was necessary. But maybe it wasn’t necessary to move the IsEnabled check into a separate provider.

Never Perfect

There are many ways in which this particular design can fall apart and in other contexts this may be overkill or under-engineered. Having multiple simultaneous providers means that if two providers update usage, the first may update a usage only to have the second declare no capacity in which case the first provider is over-stating its usage potentially denying users later that would otherwise be let in. This is something we have to watch and potentially add a two-phase check/commit process increasing complexity.

We ended up with three separate single-method interfaces. For half-a-dozen lines of “real code” we created six files, plus tests. Part of this can be blamed on the language, but we also could have gotten away with putting those 6 lines in a single class and been done with it. As functionality is split off into separate files it decreases readability as you have to piece together all the pieces of the puzzle to have complete context.

In the end, design is a trade off between conflicting principles an often times it can come down to a coin flip. My goal isn’t perfection but to have a mindfulness of the principles, understand the tradeoffs, and stop when it’s “good enough.”