Home > .NET, C# > Are Extension Methods a Code Smell

Are Extension Methods a Code Smell

Are Extension Methods a Code Smell?

December 10th | 2009

Are Extension Methods a Code Smell?

The extension method is a handy feature that came in C# 3.0/VB.NET 9.0 and .NET Framework 3.5. Quite simply, it allows the appearance of extending a class and giving it additional functionality without actually having to modify that class. Here’s an example:

class OtherClass
{
    public void Foo()
    {
        User user = new User();
        user.DisplayUserName();
    }
}

public class User
{
    public string UserName { get; set; }
}

public static class Extensions
{
    public static void DisplayUserName(this User user)
    {
        string userName = user.UserName;
        //Display userName
    }
}

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

In the “Foo” method we are calling a method “DisplayUserName” which appears to belong to the User class, but is actually a static method in a completely different location.

Extension methods are currently present in the .NET Framework. Their most notable use is in LINQ. All of LINQ’s operators are implemented as an extension method on the IEnumerable<T> interface.

But can they be a code smell? In most cases I would say yes, for several reasons.

Firstly, if it is something simple, why not just implement it in the object? In the case of the User, it may not be his responsibility to display to the interface. Therefore, to avoid violating the Single Responsibility Principle, it would be better to keep the functionality out of the User class. However, this turns something that might have been better as a service into a concrete static implementation, and this could be difficult to test by mocking. By creating a service and adding it to a container, we have a more appropriate solution.

Secondly, new developers—or even experienced ones—might be confused. It’s impossible to tell just by looking at the code if it’s an extension method or an actual method on the object. If you have multiple extension methods the code can be extremely difficult to read. It may even introduce bugs or make code reviews labor intensive.

A third scenario where I see extension methods as problematic is in the fact that they seem to be instance-based. That is, they have the appearance of accessing a method on an instance of an object. In actual fact, the compiler is just translating it to a normal static method. A developer might be tempted to do something like this:

public static class Extensions
{
    private static string _userName;
    public static void DisplayUserName(this User user)
    {
        _userName = user.UserName;
        DoDisplay();
    }

    public static void DoDisplay()
    {
        //Do something with _userName
    }
}

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

This is perfectly legal and working code, but by introducing something outside of the scope that is static, and not tied to an instance of User, we’ve introduced thread safety issues. I believe this is one of the reasons Extension Properties were not introduced—it’s an invitation for this sort of code.

Extension methods can be a slippery slope. While they may have been introduced for a perfectly valid and specific reason, there’s a broken window effect: if there’s one broken window in the neighborhood, it’s an invitation for other windows to be broken too. There’s an obvious invitation for extension methods to be written in such a way that if one extension method is written, then you’re inviting other people to use them in possibly careless ways.

Are they useful in any context at all? Yes, I believe they are. Usually for extending sealed classes for which you don’t have the source. A common pattern is to make string helpers as extension methods. Here’s an example:

class Program
{
    static void Main(string[] args)
    {
        string sample = "the quick brown fox jumped over the lazy dog";
        string titled = sample.ToTitleCase();
        //titled will be "The Quick Brown Fox Jumped Over The Lazy Dog"
    }
}

public static class StringExtensions
{
    public static string ToTitleCase(this string s)
    {
        return System.Globalization.CultureInfo.CurrentUICulture.TextInfo.ToTitleCase(s);
    }
}

 

This might be an acceptable use of an extension method. We are extending a sealed and well-known class “string” and giving it additional functionality.

I could also see it working, on owned-code, with helper methods in unit test projects.

As far as organization goes, try keep extension methods down to a minimum, or none if possible. If you must use them, organize them in way that all the extensions are organized in <ClassBeingExtended>Extensions static class. Or to take it step further, append the extension method with “Extension” so it is easily identified as one.

I think they are cool – but ultimately the possible negatives outweigh the positives.

What say you readers? Are extension methods a quick way to shoot yourself in the foot?

Kevin Jones is a Team Lead at Thycotic Software, an agile software services and product development company based in Washington DC. Secret Server is our flagship password management software product. On Twitter? Follow Kevin

Categories: .NET, C#
  1. Michael Doyon
    December 11, 2009 at 12:18 am

    Personally, I like extension methods but I have to agree that they are easily abused. I tend to use extension methods a lot for common/simple data validations and conversions. For example, I have created several extension methods to convert an integer into a timespan allowing me to write something like:

    DateTime.Today – 12.Days;

    In this case, it is mere syntactic sugar (which I think is a good usage). But I also use them for adapting business objects to DTOs and vice versa. At first, this seemed like a great idea…and it worked well especially when I only had a handful of adaptations. This pattern quickly grew out of hand though and I have spent a considerable amount of time refactoring it back down to a manageable level. All of the extension methods I write tend to have a common pattern though:

    myclass.Is
    ex: if (myclass.IsEmpty()) { … }

    myclass.As
    ex: Submit(myclass.AsIList());

  2. Florian
    December 11, 2009 at 2:33 am

    c# extensions is what rubyists call “opening a class up” and pythoneers call “monkey patching”.

    It is known to be a fallacy of design. RoR adds dozens of methods to the String class that way, and overrides a bunch of buildin ones. The end result is that RoR does not work with most Ruby libraries that have not been written for RoR, and those libraries that do work with RoR and otherwise, have “if RoR else …” code strewn troughout it.

    Python allows you to extend user defined classes, but there’s no supporting syntax. Extension of buildin classes is not possible. This is a clever design choice of python that prevented Python from running into library incompatibility hell.

    So cudos to C#/.Net for implementing something utterly useless that’s known to be a fallacy of design a couple years after everybody else stopped thinking it’s a good idea.

    • John Smith
      July 1, 2010 at 5:26 am

      Except that in C# the class extensions are only scoped in the current file that has the “using” keyword.

      Ruby/Python monkey-patching breaks libraries because it actually modifies the built-in class with other unexpected functionalities. Extensions are just syntactic sugar (similar to not having to type in this.foo when foo is a class variable) and don’t change the semantics of the class functionality at all.

  3. December 11, 2009 at 3:08 am

    I see another problem in using extension methods. Extension methods really complicate Test Driven Development because they are static methods which are getting the this reference. These methods are difficult to mock with traditional mocking frameworks…

    Daniel

  4. December 11, 2009 at 5:47 am

    I try to use extension methods very sparingly as well, mainly for strings. I like the idea of appending them with ‘Extension’.

  5. December 11, 2009 at 8:40 am

    Another case where they are useful (and the original reason why they were introduced in C# and VB) is to add functionality (implementations) to interfaces. All classes that implement the interface automatically gain functionality without having to add extra methods. Extension methods are at the foundation of LINQ.

  6. December 11, 2009 at 9:09 am

    As with all code types, extension methods can be abused and should be used with caution. However, in the end they are just syntactic sugar and I believe, in small spoonfuls, can improve the readability of code.

    Their use in LINQ is a very useful. The readability of a long chain of commands improves readability vastly compared to the alternative code using static methods. Their use to apparently extend existing but closed classes can be very useful too. I have recently created a large set of extension methods for the CRM DynamicEntity class that has simplified an existing codebase dramatically.

    Using extension methods for your own classes should be approached with further caution but not necessarily disregarded. I can see areas where they can be useful but many others where extending the core classes is better.

    The point about the possible confusion over whether a method call is an instance call or an extension method call is valid to some degree. However, improvements to IDEs are making these mistakes much less easy. There’s always some possibility for errors and we should discourage them when there is no other advantage. In the case of extension methods I am not so sure. Sometimes the benefits outweigh the possibility for reading errors.

    The real problem that you highlight is the one regarding misappropriation of extension methods. Careless use of anything is always careless and, therefore, bad. You could argue though that the static methods that these extensions become can be equally abused.

  7. December 11, 2009 at 10:46 am

    Extension methods solves a specific problem and takes a pragmatic approach to OO rather than a dogmatic one.

    Specifically, it allows you to retrofit a published and therefore immutable interface with new behavior which you did not or could not include originally. If one thing is certain to me, about programming, it is that you never get it right the first time.

    Can they be misused? Absolutely – but show me a language feature which can’t.

  8. December 11, 2009 at 1:24 pm

    Of course they can be abused, just like anything else. Dynamic typing, lazy evaluation, anonymous typing, and a number of other C# features can be abused. However, in my opinion, the benefits do outweigh the risks.

    Of course we’d have no LINQ without extension methods, so that’s a good enough reason in itself. But adding utilities to framework objects like string or DateTime are also very useful – you can create things that the framework left out, like DateTime.IsBetween, or string.GetNumericChars, or other simple functions that are just nice to have. Also you can chain methods that were originally void, like sorting a list:
    List list = /* implementation */ ;
    list
    .SortChained(sorter)
    .ForEach(DoSomething);

    Aside from the little stuff like that, I’ve found a particularly useful pattern – working with state variables in web applications. Instead of:
    object objPersonId = Session[“PERSON_ID”];
    int personId = (objPersonId != null) ? (int)objPersonId : default(int);
    you can put all of the type-unsafe stuff into a central location (static ExtensionMethods class), and your web pages can call them type-safely:
    int id = Session.GetPersonId();

  9. December 11, 2009 at 5:24 pm

    @Michael that’s an interesting idea, but what’s wrong with DateTime.Today.AddDays(-12)?

    @Florian thanks for the input on Ruby and Python. At least in .NET an extension cannot override the class’s own implementation. There is that added negative effect where if you have an extension method called “Foo” on type “X” and the developer of “X” adds a method called Foo, your extension is now broken if they signatures match.

    @Daniel Yes that is definately an issue that I briefly mentioned. That’s why they should be very simple if you use extension methods at all.

    @Tommy Yes, that is a good reason: Like LINQ. However if you own the code to the interface, then it might be better to modify the interface. Though that brings up an interesting point. Extension Methods are useful for API’s to provide an “Opt-Out” functionality the way LINQ does. If you don’t import the namespace, the functionality is gone.

    @BlackWasp – Yes I agree 100%. Though I tend to think Extension methods are more prone to abuse because they don’t [b]look[/b] like static methods, and a junior developer might not even know better. Statics look ugly to me. An extension doesn’t until I figure out it’s an extension.

  10. December 12, 2009 at 1:47 am

    Excellent post. Definitely something to think about here. I am not sure where I sit. They are very nice in certain circumstances, but they are also deceptive. LINQ without them would be unbearable though. Thanks for making me think more about this!

  11. December 13, 2009 at 3:34 pm

    @Joe – I see your point. And in all of your examples you are extending sealed types or well-known types in the .NET Framework. A Fluent Syntax can exist without extension methods. Fluent NHibernate is a good example.

    Using extension methods for a sealed type makes sense, but sometimes encapsulation might be a better idea.

    For well known types (like string), it makes more sense. You can’t go to your developers and say ok, from now on we are going to use the StringWrapper class instead of string. “string” is too popular and replacing it would be odd, thus an extension method would be appropriate.

    I don’t think it’s as black-and-white as “use them or don’t use them”.

  12. James Hare
    December 15, 2009 at 6:32 pm

    Agreed. It’s not black-and-white but it should really be controlled by team standards. We had several instances where developers here on my team developed many extensions off of object (for various reasons) that totally polluted intellisense.

    Anymore I’m more o the mind set that a good extension method should be:

    a) only on sealed classes you have no control over.

    b) must fit the ENTIRE domain of the type being extended.

    If niether a nor b is true, either extend the original class or write a helper method in the correct domain.

  13. December 19, 2009 at 4:13 pm

    I don’t share your fear of extension methods. Extension methods are just syntactic sugar for something we have been able to do all the time. Call static helper methods. All the mistakes you mention a programmer can make, are just as likely in static helper methods.

    The great thing about extension methods is that you can make something that clearer discribes the intent of the code instead of dealing with where what method is in your application. This is great for separating concerns and having clean objects. With your example the User would belong to the business domain and the DisplayUserName() would belong in the UI layer.
    I also use it to implement methods on interfaces. This is great if you don’t your classes to be inherit from our class but you want some method on them anyway.

    @Daniel: I don’t really see the problem with test driven development. Its a static method that takes an object. Can it be much easier than that?

  14. December 19, 2009 at 4:26 pm

    @James – I like your point on B – if it’s a one off extension method created for a very specific purpose then it probably isn’t any good.

  15. April 27, 2010 at 10:03 am

    I agree, though the main problem from my point of view, is that if you want to use an extension method (say you’ve copied some code from somewhere) and you don’t know it’s an extension method then which dependencies do you need to add to make it available? The compiler just says there’s no such method. Rubbish, and I don’t use them.

  1. December 11, 2009 at 4:21 am
  2. March 23, 2010 at 5:27 pm

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: