6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. \$\endgroup\$ – Jamal Aug 20 '15 at 23:03
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ +1. Extension methods seem to be a shortcut to actually designing classes that conform to a specified inheritance hierarchy, and as such, should be used sparingly, and with some forethought about how you are going to manage the kinds of multiple inheritance problems or brittle structure problems they create. \$\endgroup\$ – Andyz Smith Aug 23 '13 at 14:28
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

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.