I planned to write this post for a couple of weeks now. But recently I saw this discussion post from Ben:
This reminded me, that I should get back to this post and finish it.
It sounded so simple
We use custom NuGet packages and an internal NuGet-server at work to make reusable code easily available. One of those NuGet packages is a "utilities" package, that contains functions and services that come in handy quite often.
A few weeks ago, I wanted to use the EnumUtils. As you can probably guess by now, it provides functions to work with Enums. For example, reading a string representation for an enum-member that is defined in a Description attribute. But let's look at an example. Let's assume we have an enum called TicketStatus.
public enum TicketStatus
{
[Description("open")]
Open,
[Description("in progress")]
InProgress,
[Description("testing")]
Testing,
[Description("approval")]
Approval,
[Description("done")]
Done
}
And somewhere in my code, I want to get the description for a specific value of the enum.
TicketStatus status = GetCurrentTicketStatus();
// now what?
So, now what? I knew that class was called EnumUtils. But EnumUtils.GetDescription(status) did not work. There is no EnumUtils class. But my IDE told me, that there is something called IEnumUtils. OK... I get it. We use dependency injection a lot. Let's get an IEnumUtils thing injected into the constructor.
public class SomeServiceOrController
{
private readonly IEnumUtils _enumUtils;
public SomeServiceOrController(IEnumUtils enumUtils)
{
_enumUtils = enumUtils;
}
public string DoSomething()
{
TicketStatus status = GetCurrentTicketStatus();
return $"Some message including '{_enumUtils.GetDescription(status)}'";
}
/* more methods here ... */
}
That was easy. And let me tell you something. It worked.
Then, what's your problem? ð§
Good question. ð
My problems with this came up when I started writing the tests for my DoSomething() method. Here is what I did:
[Fact]
public void DoSomething_IsWorkingFine()
{
/* Doing stuff to create the test scenario... */
var sut = new SomeServiceOrController(/* what to do here? */);
var result = sut.DoSomething();
result.Should().Be("Some message including 'in progress'")
}
What do I pass to the constructor of SomeServiceOrController? I already found out earlier that I cannot do new EnumUtils() because "EnumUtils" is not found. Sure, I could write a small hand-written mock or just use a mocking framework. This would totally work. But to be honest... I don't want to do that.
I mean... it's just a simple utils function. Why would I mock that? It just reads an attribute from an enum member. I see no need to mock that. That's why I checked the code of the utils package. And I found the following:
internal class EnumUtils : IEnumUtils
{
public string GetDescription(Enum value)
{
/* the implementation is not important here */
}
}
Do you see it? It's internal. That's why I cannot use new EnumUtils() in my project.
And I thought "But what if I don't have dependency injection available? There might be smaller projects, where we do not have DI in place. How would I use it there?". So I looked a little deeper in the code...
public interface IEnumUtilFactory
{
IEnumUtils Create();
}
public class EnumUtilFactory : IEnumUtilFactory
{
public IEnumUtils Create()
{
return new EnumUtils();
}
}
First, the good part: Now I can use the factory in my test.
var sut = new SomeServiceOrController(new EnumUtilFactory().Create());
Now I don't have to mock that stuff. Great. But...
An additional interface (IEnumUtilsFactory) and an additional class (EnumUtilsFactory) just to wrap the new EnumUtils()? Really? Why does this thing need a factory? I don't get that.
Time to calm down
I felt the urge to change the utils package immediately. This implementation is just wrong. I will fix it and the world will be a better place.
After taking a short break and thinking about it a little more, my initial shock was gone. But another shock came. I realized that I did something I can't stand when others do it. I mean the "This is awful code. Why did anyone create it that way? I need to rewrite it to make it good." attitude.
You should always keep in mind, that the developer who created this code most likely had good reasons to do it that way. Maybe there were specific requirements. Maybe there were other constraints. Or they were just learning these concepts and thought it would be a good use-case for it. I mean, we have all been there, right?
Final thoughts
Over-engineering will always happen. Over-engineering is real. Sometimes it is just a few classes that could be reduced to a simple function. Sometimes it is worse and the entire system architecture is excessively over-engineered. But again: There will be over-engineering and we have to deal with it from time to time.
What about my scenario? I will talk to my colleagues about how we will deal with it. Maybe making the EnumUtils public would be a start. Or maybe keep the code as it is for now and create an extension method that does the same and can be used in new projects.
I think my takeaway is: Don't focus on the "bad code" and how you would have done it better. Focus on how to solve the problem at hand.
Latest comments (2)
I suppose if that kind of structure was consistent throughout the codebase for all classes it might be acceptable... Consistency is good lol
Well, consistency is good. I totally agree. But once again, it depends.
Consistency is very important in a lot of cases. Coding guidelines are a classic example. You don't have to love all the rules. But it's better to be consistent than to do your own thing.
I think it's a little different with the techniques I mentioned in the post. With these architectural decisions, there is no "one size fits all" solution. For some parts of the code interfaces, dependency injection and stuff like that are a perfect fit. But there are other parts of the code where this is not a great choice.
As mentioned in the post, sometimes is just a case of "Since I got my new hammer, everything looks like a nail". Ask me how I know. ðĪŠ