6

I just created the following method in one of my classes

public static bool Assimilate(this List<Card> first, List<Card> second)
{
    // Trivial
    if (first.Count == 0 || second.Count == 0)
    {
        return false;
    }

    // Sort the lists, so I can do a binarySearch
    first.Sort();
    second.Sort();

    // Copia only the new elements
    int index;
    for (int i = 0; i < second.Count; i++)
    {
        index = first.BinarySearch(second[i]);
        if (index < 0)
        {
            first.Insert(~index, second[i]);
        }
    }

    // Edit
    second = null;

    return true;
}

And a friend of mine, reviewing my code, said that I shouldn't create methods that 'extend the List class', since that violates the open/closed principle. If I want to extend the class List I should create a new class that inherits from List and implement my "merge" method in that new class. Is he right? Extending the List class violates the Open/Closed principle?

Trauer
  • 1,981
  • 2
  • 18
  • 40
  • I've never heard the term "Open/Closed principle", but from Wikipedia's definition, I don't see how this violates that. – Cory Nelson Apr 25 '14 at 16:11
  • Changing the behaviour of the class would but i don't see that here. What i would dislike is that it changes the input lists (it sorts them) especially the second list as a side effect . – Ralf Apr 25 '14 at 16:14
  • 6
    You should seriously ask your friend whether he knows "Open/Closed principle" – Sriram Sakthivel Apr 25 '14 at 16:16
  • So, theres nothing wrong with my code? – Trauer Apr 25 '14 at 16:30
  • Ralf pointed out one potential issue with your code, but that's separate from the matter of extension method vs. subclass. Is there a need to sort the second list? I don't see that serving any real purpose here. – JLRishe Apr 25 '14 at 16:34
  • Sorting it will make the search for unique elements faster. Plus I won't use the "second" after this method is called. – Trauer Apr 25 '14 at 16:59
  • How will sorting `second` make the search faster? You are iterating through it one item at a time. Sorting `second` doesn't improve anything. Even if _right now_ you're not using it after calling the method, you should still think about the implications of your design and not have unnecessary side effects (after all, isn't good design exactly what this question is about?). – JLRishe Apr 25 '14 at 17:35

3 Answers3

7

I don't think this violates open/close principle. I think about it in terms of if I have to 'change' existing code to add functionality to an object then I'm violating open/close, but extending an object is exactly what you should do to add functionality.

You can extend the object in different ways in different languages, inheritance is just one way; c# provides you the ability to add extension methods to an existing class.

Remember 'open for extension - close for modification'

Jaime
  • 6,736
  • 1
  • 26
  • 42
  • I said that to my friend. He said that extensions should be made thru inheritance ._. – Trauer Apr 25 '14 at 16:29
  • 1
    Why? There are situation that cannot be solved through inheritance that can be solved with extension methods. For instance, your example now can be used with anything that is a List (you may want to receive IList btw), if you were to implement this with inheritance you would have to __change__ the class of the lists you want to use this functionality with. – Jaime Apr 25 '14 at 16:35
  • 1
    @Trauer Ask your friend how he will add extension to `String` class; for example `ToPascalCase` method. ? and let's see he stands still with the ridiculous statement *that extensions should be made thru inheritance*! – Sriram Sakthivel Apr 25 '14 at 17:16
2

If using an extension method rather than a subclass violates the open/closed principle, then by that logic all extension methods would violate it, but they are a feature that has been intentionally added to C# and widely used throughout the .NET framework itself, to much benefit. (Without extension methods, we wouldn't have LINQ, and that would be a real shame.)

An extension method does not modify the class itself (in terms of modifying its code) or any of its existing functionality, so it does not violate the open/closed principle.

JLRishe
  • 99,490
  • 19
  • 131
  • 169
1

The open / close principle is a principle, not a mandate of what that principle should look like in a particular language.

The underlying principle is that, to create a robust, flexible object hierarchy, the base interface can be extended but should not be arbitrarily modified.

In most languages, inheritance is the only way to do this kind of extension, so the open / closed principle requires using inheritance. C# happens to give you two techniques for extension: inheritance and extension methods. There's nothing wrong with using them both.

Josh Kelley
  • 56,064
  • 19
  • 146
  • 246