Coding: Passing booleans into methods
In a post I wrote a couple of days ago about understanding the context of a piece of code before criticising it, one of the examples that I used of a time when it seems fine to break a rule was passing a boolean into a method to determine whether or not to show an editable version of a control on the page.
Chatting with Nick about this yesterday it became clear to me that I've missed one important reason why you'd not want to pass a boolean into a method.
The first reason I hate passing booleans around is that it usually means we are controlling the path code should take inside a method rather than just calling the appropriate method ourself.
The following type code is not that unusual to see:
public void SomeMethod(bool someBoolean) { if(someBoolean) { // doThis } else { // doThat } }
The client of this method knows what it wants to happen so why not just have two methods, like so:
public void DoThis() { }
public void DoThat() { }
In the specific case I was referring to in the post we had a HtmlHelper (ASP.NET MVC) method called DropDownOrReadOnly which either rendered a drop down with options for a user to select or just displayed the option they had previously selected if they were an existing user.
The boolean in this case was a property on the model which indicated whether or not the user had the ability to change these options or not.
It was therefore a case of doing an if statement in the aspx page or inside the helper. Initially we went for putting it in the aspx page but they started to look so messy we moved it into the helper.
Now what I totally didn't see in this example until Nick pointed it out is that where we are passing in a boolean to this method, what we really want is an object which defines a strategy for how we render the control – we can delegate the decision for whether to display a drop down or read only version of the control.
Instead of passing in a boolean we could end up with something like this:
public abstract class EditMode { public static readonly EditMode Editable = new Editable(); public static readonly EditMode ReadOnly = new ReadOnly(); public abstract void RenderFieldWith(HtmlHelper htmlHelper); }
public class Editable : EditMode { public override void RenderFieldWith(HtmlHelper htmlHelper) { htmlHelper.Label(...); } }
public class ReadOnly : EditMode { public override void RenderFieldWith(HtmlHelper htmlHelper) { htmlHelper.DropDownList(...); } }
We've added the 'Label' method to HtmlHelper as an extension method for the sake of the above example. I'm sure the API for EditMode can be done better but that's the basic idea.
We could then use it like this:
public static class HtmlHelperExtensions { public static void DropDownOrReadOnly(this HtmlHelper htmlHelper, EditMode editMode) { editMode.Render(htmlHelper); } }
Again I've simplified the API to show the idea of delegating responsibility for how we render the control to the EditMode. Nick has written more about this idea in a post about refactoring to the law of demeter.
The final reason that passing booleans around is not a great idea is that when you read the code it's not immediately obvious what's going on – the API is not expressible at all.
If we compare
HtmlHelper.DropDownOrReadOnly(true)
with
HtmlHelper.DropDownOrReadOnly(EditMode.ReadOnly)
I think it's clear that with the second approach it's much easier for someone coming into the code to understand what is going on.
That is all well-and-good unless you make your client code look like the following:
if(someBoolean) o.DoThis();
else o.DoThat();
or, more confusing yet…
HtmlHelper.DropDownOrReadOnly(isReadOnly ? EditMode.ReadOnly : EditMode.Editable);
I get what your saying, I believe in it ( see http://csharptest.net/?p=198 ). It's just worth saying that in NO WAY should you force the logic to the client, rather the goal is to eliminate the condition all together. Provided the caller knows which type of control he is creating, the approach above works very well. When the caller is deriving whether or not the control is read-only by some data (say an access check) the solution gets more troublesome.
Roger
8 Apr 09 at 10:30 am
[...] Coding: Passing booleans into methods – Mark Needham talks about using boolean parameters to methods to change the flow of code, and how having separate methods for each flow is more expressive. [...]
Reflective Perspective - Chris Alcock » The Morning Brew #324
8 Apr 09 at 4:52 pm
I agree that having a single method that does A or B depending on a boolean parameter is less than ideal. However you need to have something somewhere that makes a choice. If you are calling with a constant as in
HtmlHelper.DropDownOrReadOnly(true)
then we can just call the right method directly:
HtmlHelper.ReadOnly()
rather than fussing with an EditMode.
On the other hand, if we're not passing a constant then the type of the passed variable doesn't matter:
HtmlHelper.DropDownOrReadOnly(editMode)
Here it doesn't matter whether editMode is a boolean or an EditMode instance.
The big problem with boolean parameters is that the sense of the parameter is unclear — what does "true" mean? If I set editMode to "true" does that mean I *can* edit, or I *cannot* edit? If we can encode this information as high up the call chain as possible it makes things clearer. If we make our editMode property hold an EditMode instance then in the logic that handles changing the mode we can assign EditMode.ReadOnly or EditMode.DropDown as appropriate. There is now no danger of disagreement over what "true" means — this information is directly encoded into a behaviour choice at the point that the selection is made.
Anthony Williams
8 Apr 09 at 5:36 pm
Coding: Passing booleans into methods at Mark Needham…
Thank you for submitting this cool story – Trackback from DotNetShoutout…
DotNetShoutout
8 Apr 09 at 7:19 pm
The problem with passing a boolean is that it indicates a method that does two things (whatever it does in the true case and whatever it does in the false case).
Your method interface became more expressive because you refactored the code to have a single purpose (RenderFieldWith) and a single purpose is more easily expressed.
So, by focusing on expressability you were able to back into a very old design principle for methods.
Rob Scott
8 Apr 09 at 10:25 pm
The solution you propose is probably over-engineered and all that was needed was 2 distinct methods IMO.
I say over-engineered because as far as I can see you are using double-dispatch where there is only a single dimension of variability. EditMode is the one dimension while HtmlHelper is not (is that right…HtmlHelper is only ever one type (class)?).
The method name looks like it is trying to do 2 things, maybe it should be taking in a field representation object i.e. HtmlHelper.renderField(field) {
field.renderOn(this);
}
field could be an editable or readonly field which is polymorphic. Yes the decision on which type of field that must be created still needs to be decided, in a factory maybe, (much as the boolean check) but at least the solution is general and not as specific as the proposed solution.
I personally like the http://www.seaside.st frameworks way of using a canvas and 'brushes' to display html, javascript etc.
Field>>renderOn: htmlCanvas
^htmlCanvas textInput
value: textInputExample;
callback: [ :value | textInputExample := value ];
beReadOnly.
EditableField>>renderOn: htmlCanvas
^ (super renderOn: htmlCanvas) beEditable.
EditableField would extend Field and the type of object (EditableField component or a readonly field component) would receive the polymorphic call to render itself on a canvas, in this case htmlCanvas.
I know what your post is trying to say but the benefit of going into double dispatch is harder to follow, albeit for the more explicit naming used which helps the user understand what is tryign to be achieved. Isn't simply easier to have a more descriptive intention revealing method or a more 'complete' object that encapsulates the model and can render itself.
Good post…cheers
Carlo
9 Apr 09 at 6:21 pm
@Carlo – I guess ideally it would be cool to get the field to render itself but with the template approach to the view I think it's very difficult/impossible to do it.
A colleague of mine showed me the seaside framework and although I haven't used it the whole way of doing things looks really nice. I think I need to understand smalltalk syntax a bit better to really get what's going on though!
Mark Needham
10 Apr 09 at 1:28 am
[...] Mark Needham's post about how inexpressive an API can be when using booleans in method parameters, another clear [...]
Ivan Sanchez » Dodging booleans
11 Apr 09 at 9:12 pm
[...] to VoteCoding: Passing booleans into methods (4/7/2009)Tuesday, April 07, 2009 from http://www.markhneedham.comIn the specific case I was referring to in the post [...]
ASP.NET MVC Archived Blog Posts, Page 1
13 Apr 09 at 3:05 pm