Extension 'Class': Good use of extension methods and increase code readability... or bad smell?

Question

So I've been dealing with several APIs recently provided by different software vendors for their products. Sometimes things are lacking, sometimes I just want to make the code more readable, and I'm trying to avoid a ton of static methods where they don't belong to "get what I need" from the APIs. Thus, I've found myself writing quite a few extension methods.

However, because there are many methods, and in the interest of keeping "my" methods separate from those of the API objects in terms of code readability, I came up with this little tidbit:

public class MyThirdPartyApiExtensionClass {

    public static MyThirdPartyApiExtensionClass MTPAEC(this ThirdPartyApiClass value) {
     return new MyThirdPartyApiExtensionClass(value);
    }

    private ThirdPartyApiClass value;

    public MyThirdPartyApiExtensionClass(ThirdPartyApiClass extendee) {
     value = extendee;
    }

    public string SomeMethod() {
     string foo = value.SomeCrappyMethodTheProviderGaveUs();
     //do some stuff that the third party api can't do that we need
     return foo;
    }

    public int SomeOtherMethod() {
     int bar = value.WowThisAPISucks(null);
     //more stuff
     return bar;
    }

}

Then I can do things like:

string awesome = instanceOfApiObject.MTPAEC.SomeMethod();

and I have a clean separation of my stuff from theirs.

Now my question is.. does this seem like a good practice, improving code readability... or is this a bad idea? Are there any harmful consequences to doing this?

Disclaimer: The code above is just to demonstrate the concept. Obviously there is better sanity checking and usefulness in the real thing.

edit: I suppose the same level of separation could simply be done like this:

public static class MyThirdPartyApiExtensionClass {

    public ThirdPartyApiClass MTPAEC(this ThirdPartyApiClass value) {
     return value;
    }

    public string SomeMethod(this ThirdPartyApiClass value) {
     string foo = value.SomeCrappyMethodTheProviderGaveUs();
     //do some stuff that the third party api can't do that we need
     return foo;
    }

    public int SomeOtherMethod(this ThirdPartyApiClass value) {
     int bar = value.WowThisAPISucks(null);
     //more stuff
     return bar;
    }

}

Answer 1

When dealing with an API that you cannot modify, extension methods are a reasonable way to extend the API's expressiveness while staying relatively decoupled, IMHO.

The biggest issue with extension methods has to do with the fact that they are implicitly inferred to be available based on namespace inclusion (the using statements at the top of the file). As a result, if you simply forget to include a namespace, you can end up scratching your head wondering why there's not available.

I would also add that extension methods are not an obvious construct, and as a result developers don't commonly expect or anticipate them. However, with the increased use of LINQ, I would imagine more and more developers would be getting comfortable with their use.

Answer 2

My opinion is that you're adding an extra level of indirection needlessly. You want the extension methods to be available on the original objects... so why not put them there? Intellisense will let you know that the objects are extensions, for the rare case that you actually care.

Answer 3

To answer your direct question, I think that going through the extra trouble to separate out your functionality from the basic functionality is a bad code smell. Don't worry about having your code separate from their code from a usage perspective. First it makes it that much harder to find what you're looking for since now there's two places to look for the same functionality and secondly the syntax makes it look like your extensions are operating on the MTPAEC property and not the core object (which they are).

My suggestion is to use actual Extension methods which allow you to have that but without having the additional constructor.

public static class ApiExtension
{
    public static string SomeMethod(this ThirdPartyApiClass value)
    {
        string foo = value.SomeCrappyMethodTheProviderGaveUs();
        //do some stuff that the third party api can't do that we need
        return foo;
    }
}

used by

var mine = new ThirdPartyApiClass();
mine.SomeMethod();

C# will do the rest.

Looking at your above suggestion you'll have to split the two classes out I think. One for providing extension groups using the extensions mechanism and another for providing each group of logic.

If you need to separate out yours from a glance then use a naming convention to make your look unique. Though upon hovering and through intellisense it will tell you that it is an extension method.

If you just want the separation of content like you have, then you'll need two classes.

public static class ApiExtensionder
{
    public static MTPAEC(this ThirdPartyApiClass value)
    {
        return new MtpaecExtensionWrapper(value);
    }
}

public class MtpaecExtensionWrapper
{
    private ThirdPartyApiClass wrapped;

    public MtpaecExtensionWrapper(ThirdPartyApiClass wrapped)
    {
        this.wrapped = wrapped;
    }

    public string SomeMethod()
    {
        string foo = this.wrapped.SomeCrappyMethodTheProviderGaveUs();
        //do some stuff that the third party api can't do that we need
        return foo;
    }
}