Is That How You Talk To People?
Posted by Davy Brion on July 26th, 2010
I just spotted the following 2 methods in a piece of code:
public void ShowPanelWindow(bool isVisible)
{
Visibility = isVisible ? Visibility.Visible : Visibility.Collapsed;
}
public void ShowBusy(bool isBusy)
{
BusyIndicator.ShowIsBusy = isBusy;
}
And i cringed. Personally, i think it’s weird to read lines of code that say view.ShowPanelWindow(false) or view.ShowBusy(false). Instead, i’d go for something like this:
public void HidePanelWindow()
{
Visibility = Visibility.Collapsed;
}
public void ShowPanelWindow()
{
Visibility = Visibility.Visible;
}
public void LookBusy()
{
BusyIndicator.ShowIsBusy = true;
}
public void StopLookingBusy()
{
BusyIndicator.ShowIsBusy = false;
}
Sure, i’m not gonna win the fewest-lines-of-code contest but then again, we’re not participating in that contest anyway. And while my version is a little bit longer, it doesn’t take more than a few extra seconds to write that extra code, and the improved readability of the consuming code is more than worth it. After all, you do prefer reading view.HidePanelWindow() over view.ShowPanelWindow(false), no?
I’ve always liked the following approach to avoid (or at least, mimimize) bad method names. Just pretend that the classes are people and that the method names are messages between them. There is after all a reason why we referred to it as “sending a message to an object” originally in OO-terms. Give it a shot, and you’ll notice that your code will become more readable with hardly any extra effort.
July 26th, 2010 at 2:45 pm
I think these practices come from the time that refactoring tools were non-existent. If you rename “BusyIndicator”, your code would require 2 changes instead of just the one.
Thankfully it is not 1759 anymore and we can stop doing these things
July 26th, 2010 at 2:48 pm
@Roy
possibly… i think the fact that it’s fewer code is also something that many developers would use as an argument in favor of the first version
and there’s also the fact that some developers still really don’t know how to talk to people
July 26th, 2010 at 4:43 pm
I would still go with first version but call the methods as “ToggleVisibility”. Then create “IsVisible” or “Visible” property and write IsVisible = !IsVisible. And the with Busy Indicator or Busy State I would go for your implementation. Visibility is typically toggled via some UI element but busy state is generally managed through code. By making visibility as Boolean you can always call it via code. Also another fact is Expand/ Collapse is not same as Show/Hide. When you collapse a control, you typically leave a handle to expand it again. Not the same with visibility.
July 26th, 2010 at 4:46 pm
@Hemant
would you still go for ToggleVisibility if it would only be called through code? in this particular case, it was only called from code
July 26th, 2010 at 4:47 pm
@Hemant
btw, in Silverlight, Visibility.Collapsed means => completely hide it… there is no handle to expand again
July 26th, 2010 at 5:03 pm
Well… I was not aware of the SL functions since I haven’t coded in SL yet. If that is the case why blame the poor developer… even Microsoft folks don’t know how to talk to people
July 26th, 2010 at 5:05 pm
@Hemant
oh don’t get me started on the naming conventions/practices of Microsoft code
but that doesn’t mean you shouldn’t use better naming in your own code
July 26th, 2010 at 5:09 pm
Agree. I always tell my team to avoid natural obfuscation. The way they name the code elements and write the logic, it is almost impossible to reverse engineer the real code. Forget REing the IL
July 26th, 2010 at 5:42 pm
I agree with the point of this post. Another thing; if the function in question is actually lengthy and/or complex, and/or can’t easily be refactored itself for some reason, you can always make the current code private and simply create wrapper methods with more descriptive names. After all, not all methods are one-liners like the example, and while refactoring to increase readability is a good thing in most instances, sometimes there isn’t time – and then, a little is clearly better than nothing.
July 26th, 2010 at 8:19 pm
It depends on the context and how those 2 methods are used but… If e.g. visibility comes as data the first version is better. I.e. ShowPanelWindow(someobj.IsVisible) is better than if(someobj.IsVisible) ShowPanelWindow() else HidePanelWindow();
There have been cases when I’ve refactored such code “backwards” to your refactoring
July 26th, 2010 at 9:45 pm
I agree intellectually with you, though I must confess I rather often write this:
private bool dirty = false;
private void SetDirty() { SetDirty(true); }
private void SetDirty(bool isDirty)
{
dirty = isDirty;
}
In my mind, this means that SetDirty actually sets a dirty state given that with no parameter the obvious intention is to ‘set’ the dirty state whereas you must explicitly provide the false value to ‘unset’ it…
As a side note, I hate (could not really explain why) using the ‘this’ keyword when not absolutely necessary. This is why I usually use a different name for the parameter and the member…
July 27th, 2010 at 3:49 am
I always prefer the longer version, it saves my time in the long run.
July 27th, 2010 at 5:31 am
Hi Dave
Whilst I like what you’re suggesting (code readability is becoming top of my list), what about intellisense – most devs calling the code will use intellisense to determine what they can do and being ordered alphabetically will mean the ShowPanelWindow and HidePanelWindow methods will be completely separated (causing the dev to have to hunt through before finding out what there is – of course something like resharper will help)..
What I’m getting at is, beyond outright readability is the issue of the tools we use – should we consider findability [or other tool-related issue] too?
Of another way around this is to muddle the order so the noun comes first (i.e. PanelWindowShow and PanelWindowHide) but that isn’t how we talk to people either..
Thoughts?
July 27th, 2010 at 8:56 am
This example should be quite obvious for everybody – we shouldn’t use the first solution.
Another, more interesting case is: Dispose() and Dispose(bool disposing) . Should we get rid of that syntax? Every time I see this syntax, I need to scratch my head. Does it really need to be like this ?
July 27th, 2010 at 9:17 am
@Matt
interesting point… however, in types that you own i think it’s important to keep the number of members on a class to a respectable number. With that, i mean that it should only have members that make sense for that class, and that should (IMHO) not be a list that is so large that you needs methods to be close to each other alphabetically in order to be easily discovered.
Findability/discoverability begins with a simple, clean and intuitive API. Not with IntelliSense.
@Lars-Erik Roald
Not a big fan of that convention either, but Dispose(bool disposing) is at least not a public member. Not saying that it’s completely ok to give non-public members ‘bad’ names, but there is a good argument to be found to be more lenient about those names.
July 29th, 2010 at 11:13 am
I tend to agree with Matt, having 2 seperate methods is confusing for the developer (especially when the amount of methods is large enough to have to scroll through the suggestions).
When i see HideWindow(), the first thing i wonder is: “is there also a method for hiding the window?” And if there is, what will its name be? UnHideWindow? ShowWindow?
And who guarantees that HideWindow() and ShowWindow() are each others opposite? It’s possible that the author of the code used confusing names for different functionality.
With ShowWindow(bool isVisible) you don’t have these question, you instantly see the method, its possible states and you’re (well, almost) sure true does the opposite of false.
And imho that’s why the boolean-version is easier to read than 2 methods… actually that’s why i’ve written it like that.
July 30th, 2010 at 12:00 pm
Why not have 2 properties (IsPanelWindowVisible and IsBusy) which you can set to true or false?