Are private extension methods a bad thing to use?

Question

In C# I have the following two extension methods.

    public static void WaitForMilliseconds(this IWebDriver driver, int milliseconds)
    {
        var timeout = new TimeSpan(0, 0, 0, 0, milliseconds);
        WaitForTimeout(driver, timeout);
    }

    public static void WaitForSeconds(this IWebDriver driver, int seconds)
    {
        var timeout = new TimeSpan(0, 0, 0, seconds);
        WaitForTimeout(driver, timeout);
    }

    private static void WaitForTimeout(IWebDriver driver, TimeSpan timeout)
    {
        try
        {
            new WebDriverWait(driver, timeout).Until(x => false);
        }
        catch (Exception){}
    }

But since the class is static, I can write a private extension method to make the syntax more readable like this.

    public static void WaitForMilliseconds(this IWebDriver driver, int milliseconds)
    {
        var timeout = new TimeSpan(0, 0, 0, 0, milliseconds);
        driver.WaitForTimeout(timeout);
    }

    public static void WaitForSeconds(this IWebDriver driver, int seconds)
    {
        var timeout = new TimeSpan(0, 0, 0, seconds);
        driver.WaitForTimeout(timeout);
    }

    private static void WaitForTimeout(this IWebDriver driver, TimeSpan timeout)
    {
        try
        {
            new WebDriverWait(driver, timeout).Until(x => false);
        }
        catch (Exception){}
    }

Is there any inherently bad code smell about private extension methods? Which would is better to use? The only issue I see is that if the code is later refactored into a class the methods will have to be rewritten.

Answer 1

In my opinion, extension methods are only a (pretty nice) syntactic sugar for a static helper method. Under the hood, the IL translates your extension method call to a call on the static method, so really it's just an other way to write methods.

The extension method do not have a private access to the type they are extending either, so changing a static method into an extension one will not break any encapsulation.

As for the private keyword, it is indeed the correct accessibility level since your method will not be used outside of your class.

According to MSDN's Guidelines, the major problem of extension methods is the following :

When using an extension method to extend a type whose source code you cannot change, you run the risk that a change in the implementation of the type will cause your extension method to break.

Also, if someday IWebDriver defines a new method called "WaitForTimeout", your extension method won't be called anymore... But the risk is really pretty low.

So if you think readability is increased by using that extension method, use it - you won't violate any principles or break anything, see it as a nice syntactic sugar.

Answer 2

If it helps make your code more readable and easier to maintain, I see no problem with that at all. Personally, I start with private extension methods and usually wind up promoting them into my company's public libraries when they appear to have enough utility.

So... go for it. Just make sure it's bulletproof (i.e. remove the catch that's hiding exceptions, or recover from any known exceptions that you know how to) in case someone else wants them public someday.

Answer 3

The smell here should not be whether it's private, but rather an extension method called in an extension method.

You might not even need the first two methods to be extension methods. If a fluent syntax is what you're after, you might consider moving the wait methods to your WebDriverWait class and changing their return types to WebDriverWait.

So you would be able to accomplish this: new WebDriverWait(driver).SetTimeout(5).SetWaitSeconds(3).SetWaitMilliseconds(100).Wait(someCondition);

You may also be able to simplify by making a method that takes in a TimeSpan or do everything in milliseconds so you don't have to make methods per unit.