11

I'm writing a factory class for a Selenium Web Driver and I came across a choice that I couldn't figure out which is cleaner. Having two methods with the same parameter.

GetWebDriver(string browser)
GetRemoteWebDriver(string browser)

or adding a boolean parameter

GetWebDriver(string browser, bool isRemote)

I would think in this case the first option might be better because there is very likely no reason I need to extend it to more than a boolean of choices and hence two methods. On the other hand, I can see the second one to better for when there is more than two choices or likely many extensions in the second parameter in the future.

Which would you choose for Cleaner Code and why?

craastad
  • 227
  • 1
  • 5
  • I don't always agree with what Nick has to say, but in this case I agree 100%: [Don't use boolean parameters](http://www.nickhodges.com/post/How-Not-To-Code-2-Dont-Use-Boolean-Method-Parameters.aspx). – Marjan Venema Aug 14 '13 at 20:09

6 Answers6

28

Always err on the side of a readable client ...

GetWebDriver("IE", true)

or

GetRemoteWebDriver("IE")

It seems self-evident to me which one is easier to read.

Of course, another option would be

GetWebDriver("IE", DriverType.Remote)

Which is equally readable, but I wouldn't go down that road unless there are likely to be more than two options.

pdr
  • 53,387
  • 14
  • 137
  • 224
  • 1
    In other circustance(e.g. languages that allow keyword arguments) your argument would be invalid. `GetWebDriver("IE", remote=true)` is quite readable in my opinion. – Bakuriu Aug 14 '13 at 14:14
  • @Bakuriu: The argument is only entirely invalid if you are writing the client code or you can force the client developer to use the named parameter (which is essentially what DriverType.Remote does.) – pdr Aug 14 '13 at 14:43
  • I had in mind python3 where you *can* enforce the use of named parameters. – Bakuriu Aug 14 '13 at 14:46
18

Optimizing for readability, I'd choose the first option for the public interface: The intent of GetRemoteWebDriver("IE") is more obvious than GetWebDriver("IE", true).

Of course, from an implementation point of view, it might make sense for both GetWebDriver(string) and GetRemoteWebDriver(string) to call private GetWebDriverImpl(string, bool).

Heinzi
  • 9,646
  • 3
  • 46
  • 59
7

A unit of code (subroutine, procedure, function, module, object, class, component, service, application, …) should do only one thing. How big that thing is, depends: obviously a class should have a bigger responsibility than a method.

A subroutine which takes a boolean flag almost always does two things: one thing when the flag is true, another thing when the flag is false. Therefore, it violates "do one thing" rule and should be split into two subroutines (or possibly three: a private one for the common behavior between the other two).

Jörg W Mittag
  • 101,921
  • 24
  • 218
  • 318
2

I tend to avoid such boolean flags in function interface, because they are a "separation of concern" related smell. So I would rather go for the 2 functions.

However depending on your architecture I would maybe try to have a IDriverStore service with multiple implementations. A configuration object or whatever decides if the store provides a remote driver or not. Then you call myDriverStore.GetWebDriver(browser)

Simon Bergot
  • 7,930
  • 3
  • 35
  • 54
2

I would prefer the first suggestion - to define two different methods.

A developers know immediately what that method is supposed to do (just by reading the name of it), he needs not to bother by the value of parameter of function to know what it will do. As you wrote, when you will need more extensions, the bool type will be not sufficient and maybe you will change it to integer (or Enum to be more readable). But it has some disadvantages:

  • Your method would contain some awful switch or if/else construction to find out what should be the appropriate functionality
  • The orientation in code would be little bit more difficult, you would need to know, what does it mean 0 or 1 as a parameter, or in case of integer it would be even worse

If you define two methods, I recommend to implement the common parts of functionality in one separate (private) method and call it in both methods. Your methods would contain just the unique commands and you would avoid duplicities.

srnka
  • 345
  • 1
  • 2
  • 10
1

In my experience bools as parameters are rarely a good idea as they affect readability in the callers code, so go for two methods rather than one.

Alternatively, use an enum to make the intent more explicit.

Sean
  • 1,124
  • 6
  • 10