6

So, I've fallen into the fad trap, and started replacing a large amount of linq queries with extension methods.

For example:

orders.Where(o => o.Status == ShippedStatus.Shipped).Select(o => o.ID)

has become :

orders.ShippedOrderIds

Extension methods are static with all the cons implied, and I believe the 'most-correct' OO way to handle this kind of refactor would be to wrap it in a 'Orders' object and expose property(ies) that do this instead.

A couple questions:

  1. Is there a third (or more) alternative that makes more sense than either of these approaches?
  2. How much worse is the extension approach than the 'true' OO approach?

Quick clarification of the refactoring contexts - none of these refactors operate on single objects, just collections of objects.

  • 20
    Spoiler alert: LINQ is extension methods. – Jesse C. Slicer Feb 5 at 17:53
  • 1
    Hm. If I had to give a quick stab then, as "business logic", it's just a fairly typical Service method: IEnumerable<int> GetShippedOrderIds() which can be accessible to other Services and Controllers, as well as mockable for unit tests. – Jesse C. Slicer Feb 5 at 18:15
  • 1
    I think that's definitely a solid approach. I think I would label that as the "Orders" approach that mmathis talks about and that I mention when I talk about wrapping it in an "Orders" object. I dislike the idea of having a "OrdersService" because the connotation there (at least for me) is that the data (the orders) and the behavior (getorderIds) is then separated. – Cygnus Feb 5 at 18:44
  • 1
    Don't do things because of a fad. By wrapping LINQ queries you are creating abstractions, but you can't do that by simply following "best practices". Take time to understand if these abstractions actually help the problem your code addresses. Are these abstractions fairly stable in face of changing requirements? Because in some cases they will be, but in others they will not, and if they are not, you gain practically nothing by introducing them, but they may introduce a cost. ShippedOrderIds is not about wrapping LINQ, it's about defining an appropriate interface for the orders collection. – Filip Milovanović Feb 5 at 20:52
  • 4
    "static with all the cons implied" - written correctly, they are pure methods with almost no drawbacks. Conceptually they're really not different than regular class members, just a handy syntax. – whatsisname Feb 6 at 2:43
9

Extension methods offer you an opportunity to reason about problems a totally different way-- functionally. Functional programming is a totally different paradigm from object-oriented and has certain advantages. For example, when you use functional thinking and immutable variables, your program instantly becomes thread-safe without using any locks or concurrency mechanisms at all. In addition, when you write pure functions, unit testing is a snap, and you don't need any mocks or stubs at all. What's more, extension methods allow you to put methods on interfaces-- no class required at all-- and you only need to implement them once and they will operate on any class that implements the interface, even if they don't share a base class.

Here are the rules to make your code work well in this fashion:

  1. Write your extension methods so that they are pure. They should not access any state outside of the method inputs and should not modify any state at all.

  2. Write your extension methods so they always return a new instance, rather than modifying the instance that was handed to it.

  3. Write them generically when possible; this allows type-safe method chaining that can transform types.

  4. If you need the methods to have dependencies, inject them as parameters, e.g. Where accepts a delegate which allows the caller to "inject" a logic engine (exposed as a Func<T,bool>) that determines whether a row will be included.

By the way, all LINQ methods conform to these rules.

So for example, if you have several types of classes that have a "shipped" status, you could implement this:

interface IShippable
{
    ShippedStatus ShippedStatus { get; }
    int Id;
}


static public IEnumerable<T> ShippedOrderIds<T>(this IEnumerable<T> source) where T : IShippable
{
    return source.Where( s => s.ShippedStatus == ShippedState.Shipped ).Select( s => s.Id );
}

This will work with any shippable item without requiring any upcasting or downcasting; it is pure, so you don't need to inject any dependencies and there is nothing to mock or isolate; and it will allow type-safe method chaining if you write other extension methods.

This is not a bad way to go at all. The only way it could be bad is if you think you are writing object-oriented code. You're not. You're writing functional code, and that can be a good thing.

  • 1
    Most of the answers have a good take on different aspects of what I'm doing, but I liked your emphasis on the functional paradigm and some of the nitty details you included. – Cygnus Feb 7 at 20:09
4

Extension methods are static with all the cons implied,

Because an extension method is just a static method that takes this as its first parameter and doesn't expose the inner workings of this, it encourages the developer down the "pit of success" of pure functions. The major "con" with static methods is when they access, or worse modify, global state. This isn't (or at least shouldn't be, if done correctly) the case with extension methods.

So the only real "con" with a well written extension method is that they upset "OO purists".

and I believe the 'most-correct' OO way to handle this kind of refactor would be to wrap it in a 'Orders' object and expose property(ies) that do this instead.

That's often the problem with trying to take a purist approach. It may well be "correct" from a "pure OO" standpoint, but you've now added the complexity of a wrapper class and needing to create instances of it, rather than dealing with your eg List<Order> that you previously had. And the only reason that complexity has been added is to make it appear more "correct".

As a rule of thumb, for your own classes, use methods to do something to a object (access or change its internal state) and extension methods to do something with an object (typically transform it as per your example). Extension methods shouldn't just be seen as a means of extending others' classes, they are also a neat way of extending your own classes without cluttering the main class file with ancillary functionality.

As a second rule of thumb, If I have need of a function that fits the "pure" requirements (processes, but doesn't modify) its parameters to derive a new value without side effects), then I'd make it a static. It simplifies the code over having to create instance of a new class. If it makes sense, syntactically, to use it as an extension method, then make it that too.

C# isn't an OO language; it's a multi-paradigm language with OO features. So don't worry about the extreme views of "OO purists" when deciding on the best approach for your requirements.

  • 1
    "It's a multi-paradigm language..." - That's the best single-line description of C# I've found. I think you're right to emphasize mixing the OO with the other paradigms, thanks! – Cygnus Feb 9 at 1:23
2

Extension methods really shine when you do not have access to the class being extended - e.g., framework classes like int or string. It is impossible to add them to the class, so you must write a static method, and an extension method is easier to read. If you have access to the Orders class, then you should add methods or properties in that class.

If the orders object is an instance of IEnumerable<Order> (where Order is your own class), then a couple extension methods on IEnumerable<Order> is not a problem, as compared to creating a proper Orders class (which could very well implement IEnumerable). If you are going to write many of these extension methods, then you should instead add them to a proper Orders class. I would take a step back and really examine how much improvement this really gets you though.

If you do continue with this refactoring, consider whether any of these methods can be combined - e.g., orders.GetOrderIdsByStatus(ShippedStatus status), rather than having separate orders.ShippedOrderIds, orders.CancelledOrderIds, orders.ProcessingOrderIds, etc.

  • "If you have access to the Orders class, then you should add methods or properties in that class." On the contrary, see David Arno's answer "As a rule of thumb, for your own classes, use methods to do something to a object (access or change its internal state) and EMs to do something with an object (typically transform it as per your ex). EMs shouldn't just be seen as a means of extending others' classes, they are also a neat way of extending your own classes without cluttering the main class file with ancillary funcs." – Alexander Feb 7 at 23:13
1
  1. LINQ is very readable and standardized. Other people will know exactly what each function does. Extension methods are custom, so you'd be adding extra complexity.
  2. Extension methods should be used for what they were designed - extending stuff you don't have access to. They don't have much advantage apart from this.
  • 2
    "Extension methods should be used for what they were designed - extending stuff you don't have access to". That isn't what they were designed for. They were added as a means of extending interfaces without changing those interfaces and breaking code that implemented them. "extending stuff you don't have access to" is just one of many side benefits that this feature brought to the language. – David Arno Feb 7 at 10:01
  • 2
    User code can't change IEnumerable<Order>, even if it can change Order – Caleth Feb 7 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.