Extension methods are great. They give so much power and flexibility. They allow developers the opportunity to write clean code. It allows for helpful reuse of code.
Extension methods can also be abused. This leads to noise in the codebase. Usages which weren’t intended. Use cases in places which make no sense etc.
When to write extension methods and when to write “OG” static methods comes down to style, code ownership and quite often “it depends”. There is no one rule which should be applied to everywhere. With other design patterns, while there maybe a couple of different options to solve a problem with their pros and cons, it is likely one is the best of the bunch.
Why am I writing this “soapbox” style post today? Well I saw a post on LinkedIn and on the surface it seemed fine, but as you looked more closely at it and read some of the comments it’s not as good as maybe initially thought. As more junior developers are using these avenues to gain experience the sharing of potential bad or poor advice is not good. Nick Chapsas has a series on his YouTube channel called “Code Cop” which covers posts he’s not a fan of. I would recommend checking it out!
The post in question, which I am not going to link to, was advocating about the use of extension methods but the sample extension method was on string.
public static class StringExtensions
{
public static int WordCount(this string str)
{
if (string.IsNullOrWhiteSpace(str))
{
return 0;
}
return str.Split(new char[] { ' ', '.', '?' }, StringSplitOptions.RemoveEmptyEntries).Length;
}
}
Starting off by looking at the code you can see it doesn’t look too bad. So if we’re going to be picky I would say the parameter should be string? but the user could be working in a codebase where nullability isn’t enabled. The checking for null or whitespace is spot on. I originally learnt you should use string.IsNullOrEmpty, as the whitespace check didn’t originally exist, however I have been using string.IsNullOrWhiteSpace for a long time now. Good choice!
Next up the Split functionality. It has the StringSplitOptions.RemoveEmptyEntries specified which is good. However it does not use all the characters which could indicate a demarkation of a word. There are many ways of showing how to distinguise words, such as ,, so we now have a bug.
Console.WriteLine("Hello,World!".WordCount());
This example returns 1 even though by looking at it you know there are 2 words.
Next up is where it is defined. The user has written business logic on a fundamental part of .NET. This is usually a red flag. Why? Well quite often convention says that you should put the extension method definition in the same namespace as the type you are extending. This allows for discoverability and ease of use. Now in the example the namespace is ommitted however if you were to follow this convention it would be put into the System namespace. Once this is done wherever in your csproj you are working in you will have access to this extension method even when you’re not requiring this business logic or where it doesn’t make sense.
So as you can see in such a small example there are a number of, what I would say are, red flags to the quality of the code. I think the user was probably trying to show the benefits of extension methods however examples are hard and this, in my opinion, was not a good example.
If this needed to be an extension method then I would suggest calling it as a “classic” static method as it would show intent and be explicit.
Console.WriteLine(StringExtensions.WordCount("Hello,World!"));
Now like most of the LinkedIn posts looking for interactions/likes the interesting discussions continue in the comments. There are often good points and divisive opinions. The “top” comment, which arguably was most contriversal at the time, and had the most replies was …
Junior devs, please, don't do this. Create utilities backed by interfaces that can be injected, making them plausible to mock when unit testing the services that implement them. Static and Extension methods are the bane of unit testing.
Personally this comment has more issues than the orginal code on initial reading. But let’s explorer what the commenter is trying to say.
With the blanket statement to say “don’t do this” isn’t a bad way to start. It draws people into the conversation but it also can get people being defensive straight away; not nessessarily ideal!
The next part is a big no from me. As soon as you have a namespace/class named “Utility” or “Common” arguably you’re going down the wrong track and possibly prematurely optimising for scenarios which may never happen. How often have you thought “this could be helpfull to someone in the future, I’ll put it in a utility class” and then it’s never reused? Refactor when required.
If the utility functionality is key to the service you are working on why would you want to mock it? If the function call is “pure” then why mock the result? If you improve the data being passed into the unit test then the expected response will be returned. This is arguably improved testing. Another way of looking at it is “why make it an extension method at all?”, why is it not a private method in your service you are testing so it’s covered by your test cases? But again, like most things, it depends.
And finally the statement which gives the wrong impression “Static and Extension methods are the bane of unit testing.”!
Extension methods are static methods. So straight off it’s almost trying to say that they are different things which is untrue. You can still call extension methods as a “classic” extension method. It’s syntatic sugar at the end of the day.
As static methods, as the name suggests, are static they should be working on the type you are “extending”. The only “state” you have is the instance of the target type you passing in. This can allow for testing the functionality in the static method as you can check the change of state of the instance which has been worked on.
Up until this point I have been implying that the extension/static methods should be on an instance of a class or similar type. Further down the conversation somebody mentions ILogger and the extension methods on this interface. Now if you’ve searching for testing ILogger, or an avid reader of my blog (thanks!), you will know that I have covered this concept before Mocking ILogger with Moq. These types of extension methods on an interface are where I think the original commenter was coming from as these can be a pain to work with although not impossible.
At the end of the day there are a number of extension methods based on interfaces, all the linq based ones for a start, and they have automated tests on them in their owning code base Linq Unit Tests.
What are your thoughts on extension methods? Does your opinion change if they’re based on interfaces? I’d be interested to hear your thoughts. If you want to get in contact please contact me on Twitter @WestDiscGolf