| Date: | August 28, 2006 / year-entry #291 |
| Tags: | code |
| Orig Link: | https://blogs.msdn.microsoft.com/oldnewthing/20060828-18/?p=29953 |
| Comments: | 66 |
| Summary: | Generally speaking, I believe that you should try to avoid giving functions a boolean parameter (BOOL, bool, etc.) unless the meaning of that boolean parameter is blatantly obvious. Examples of obvious meaning would be the second parameter to the EnableWindow function (TRUE obviously means the window is being enabled and FALSE means that it's being... |
Generally speaking, I believe that you should try to avoid giving functions a boolean parameter ( But for functions like Even worse is that And the mystery Mind you, this is just my opinion. Others may disagree with me. |
Comments (66)
Comments are closed. |
:-)
Thanks, Raymond! I, too, always struggle with CreateEvent (and others) and say to myself "You stupid prick, when will you remember it!"
Nice to know I’m not alone…
So I suppose an enumeration of legal values would be a much easier thing to read, huh? :)
I hear you on CreateEvent. Every time I was digging through code where I had used it, I always had to look it up to remember which parameter was which. Big waste of time. I had to resort to leaving comments wherever I used to remind me of the parameter ordering.
So many other API functions use enums, like ShowWindow and all the SW_* enums. Why wasn’t this used more often? Or even a single bitmask-type enum where you could pass in something like (EVENTTYPE_AUTORESET | EVENTTYPE_INIT_SIGNALLED) would be an improvement over the current ambiguity.
Hi,
CreateEvent is a good example. At a first glance, the idea with the enum might be overkill but it is easier to read.
You might still end at the documentation to copy & paste the enum’s name, but the BOOLs there…
Isn’t that what the variable names are for in function declarations?
CreateEvent is declared:
HANDLE CreateEvent(LPSECURITY_ATTRIBUTES lpEventAttributes, BOOL bManualReset, BOOL bInitialState, LPCTSTR lpName);
Could those be made more usable with something like enum {RESET_AUTO, RESET_MANUAL} and enum {SIGNALLED, NOTSIGNALLED} ? Yes, but then you still run into the problem of now knowing what those parameters mean, not to mention not knowing the enum types.
With BOOL, you know that there are only two settings and the parameter name should be a giveaway as to the function of the parameter.
Good thing Intellisense provides the formal parameter name and the description from XML comments :)
I’d say that CreateEvent function pains are fixable: if somebody adds EVENT_MANUAL as 1 and EVENT_AUTO as 0 new defines to the windows.h and change the docs then any new code can use them for the second parameter of the calls like CreateEvent( lpsa, TRUE, FALSE, .. ). The third parameter is the state of the event, so it would be readable enough to remain as is (FALSE is obvious enough for non-signaled).
Then when you maintain some older code, and stumble to some TRUE, FALSE code there you can change it once for ever, to the state that
Agreed. I use enums to signify meaning (which could, in fact, be cast to bool in the implementation if only 0 an 1 are uses as enum values…)
Here’s where using named parameters in VB allows you to keep the maintainability without requiring special enums, thus you would have something like : CreateEvent(lpEventAttributes:=attribs, bManualReset:=True, bInitialState:=False …) Naturally, this requires that you take the time to consider your parameter names so that a boolean response makes sense, thus bManualReset could be requireManualReset and bInitialState could be something like initialStateClicked depending on the needs of the application.
[i]Isn’t that what the variable names are for in function declarations?
CreateEvent is declared:
HANDLE CreateEvent(LPSECURITY_ATTRIBUTES lpEventAttributes, BOOL bManualReset, BOOL bInitialState, LPCTSTR lpName); [/i]
Um yes but I think you miss the point. Suppose you’re reading someone’s code where they are [i]calling[/i] the function. All you’ll see is something like
CreateEvent(NULL, TRUE, FALSE, NULL)
And most people can’t remember what the TRUE and FALSE are for without looking it up. Or worse, they think they remember but they actually remembered wrong (double bad if "they" happens to be the person who wrote the code).
Granted, I [i]suppose[/i] you can write something like this:
BOOL bManualReset = TRUE;
BOOL bInitialState = FALSE;
CreateEvent(NULL, bManualReset, bInitialState, NULL)
Right after reading this, I had to add a parameter to a function – a BOOL! Here’s three parameters in a call to it now
… TRUE, FALSE, TRUE, …
sligtly out of control – oh well.
Me, I’m a fan of chars.
Since Enums are typically in a global name space, they rely on verbosity to remain unique, meaning that they are inevitably verbose.
CreateEvent(NULL, EVENTTYPE_AUTORESET, EVENTTYPE_INITIALSTATEISSOMETHING, NULL);
is hard to read because it’s too long, and is hard to write because you have to look up whether it’s EVENTTYPE_AUTORESET or EVENT_TYPE_AUTO_RESET or EVENTTYPE_AUTO every time.
So my proposed alternative,
CreateEvent(NULL, ‘a’, ‘s’, NULL);
fits onto one line, provides some pneumonic assistance, doesn’t require you to remember which direction was supposed to be zero, and doesn’t read like you’re yelling. It’s not perfect either, but I’ve found that it leads to less manual-reading for me than the inevitably verbose enums. Newbies to the function will need to check what ‘a’ means, but after seeing it a few times, it becomes as familiar as the verbose equivalent.
Smart IDEs don’t help with the verbosity issue, which is why people resorted to booleans to begin with. Enums are equivalent to naming the function
CreateEventWhereTheFirstBooleanIsZeroWhenAutoInitializing. Your autocomplete will gladly write it out for you, but there’s a reason nobody names their functions like that.
People here complain about tedious tasks they have to do each time while they could just code their own wrapper function/class to simplify their lives. Yes, I often find myself forgetting about this too… I take the longer, harder path foolishly thinking this was the last time I had to.
Mnemonic. I meant mnemonic.
A good IDE helps to write it, and a good comment helps to read:
ev = CreateEvent(NULL, false, false, NULL); // auto-reset, non-signaled
I fully agree with the notionj not to use bool unless it’s obvious – but I still have to see a language/IDE with an enum model that makes this easy.
It would be kinda neat to have "explicit enums" that don’t implicitely convert to/from int, and don’t conflict with each other when they are used in the same namespace:
enum EEventResetType { auto, manual };
enum EBookType { manual, poetry, … };
HANDLE CreateEvent(EEventResetTypes resetType);
HANDLE CreateBook(EBookType bookType);
CreateEvent(manual);
CreateBook(manual);
Together with an intelligent IDE this would really encourage to use enumerations instead of bools…
I find that bool is often added to a parameter list as a way of controlling the internal behaviour of the function. Consequently, I consider that either the function is too complicated, or the way it is being used is overly ambiguous.
While it might be nice to have a function called "DoIt()", if you have to customize what DoIt() does with several bool parameters, then it is not doing one thing well.
That’s why the native function that implements CreateEvent on NT systems actually uses an enum for the event type:
NTSYSAPI NTSTATUS NTAPI NtCreateEvent(OUT PHANDLE EventHandle, IN ACCESS_MASK DesiredAccess, IN POBJECT_ATTRIBUTES ObjectAttributes OPTIONAL, IN EVENT_TYPE EventType, IN BOOLEAN InitialState);
typedef enum _EVENT_TYPE
{
NotificationEvent,
SynchronizationEvent
}EVENT_TYPE, *PEVENT_TYPE;
In my opinion, the native API is much cleaner than the wrappers Win32 provides on top of it. This is just one example.
The usual reason for not documenting this interface for use is that they are internal and subject to change at any time. However, the native API seems to have undergone almost no change over time– much less than Win32. Besides, kernel mode components have to use this API; changing it would break them too.
I think that this is just a special case of the problem that when reading a function call it’s not immediately clear what the parameter order is. Something like MyFunction(7, 8, 12); doesn’t say much about what the parameters mean either. Perhaps something like the VB-style named parameters isn’t such a bad idea, particularly if you can get your IDE to complete the names for you…
"Mind you, this is just my opinion. Others may disagree with me."
True. False.
I’m a big fan of putting the boolean proprety into the name, so instead of EnableWindow(BOOL), there is EnableWindow() & DisableWindow(). There’s a good chance two separate functions would be smaller than one big function, and if not, two inline functions redirecting to a private function taking a bool erases the difference.
That’s how a solved the problem of MFC’s UpdateData. I created a derived class which I based all my form on, which has a ToDialog() and FromDialog() methods.
Avoid protos that only show a type. Don’t use func(bool), use func(bool die_now). Problem solve’ed!
MFC’s UpdateData is one which I *never* can remember what the (BOOL) parameter means. *Every* single time I use it (unless it’s a cut-and-paste) I have to look it up.
I very much agree with the policy of avoiding bool in function parameters. Looking up the meanings of bool parameters time and again is tedious and it obfuscates the source code, making it more difficult to analyze.
Can I get an Amen? Yes, yes you can… here’s my version of the same gospel from March of 2006.
http://musingmarc.blogspot.com/2006/03/sometimes-it-takes-forever-to-sink-in.html
@b
But, at that point the compiler cannot check if a valid char was supplied, meaning that we have to rely on a runtime check only. What does ‘z’ mean when specifying the event type? How should CreateEvent handle this?
[i]Monday, August 28, 2006 6:13 PM by boogie
Avoid protos that only show a type. Don’t use func(bool), use func(bool die_now). Problem solve’ed! [/i]
Um, Lauren Smith already made the same point, and I already pointed out that this is still useless on the line of code that actually [i]calls[/i] the function:
func(TRUE); // ???
Compare with using an enum, the call would read something like:
func(DIE_NOW);
EnableWindow(BOOL) is only blatantly obvious if you already know that EnableWindow(BOOL) can be used to DISable a window as well as ENable it.
If EnableWindow(BOOL) had a partner in DisableWindow(BOOL) then the BOOL parameter might mean "and all its children" or "and change visibility also" or anything else that might make sense to do to a window when enabling it.
You can’t tell just by looking at EnableWindow(BOOL) whether there is a DisableWindow(BOOL).
So "blatantly obvious" actually means "obvious if you know what you’re doing". Which applies to every other use of any type of parameter!
Incidentally however, I agree.
:)
[i]But, at that point the compiler cannot check if a valid char was supplied, meaning that we have to rely on a runtime check only. What does ‘z’ mean when specifying the event type? How should CreateEvent handle this? [/i]
That may be, but keep in mind that with C/C++, people can always cast their way into passing garbage. So public functions like "CreateEvent" should do parameter validation anyway whether you used a char or a BOOL for the parameter types.
I wholeheartedly agree.
Another interesting tidbit here is that passing bools as parameters is generally frowned upon in the .NET framework class library, which makes it surprising to see it used in StreamReader.
IIRC, there’s FxCop (.NET static code analyzer for best practices) rule that recommends not using multiple boolean parameters.
Judah: Those guidelines were put in place at the time of the article I linked to above, which was after version 1 of the framework was released. You’ll notice that all new APIs in version 2 go for the "use an enum over a bool when appropriate" rule.
[i]Even in the ancient VS 2003, hovering over the calling "CreateEvent" immediately gives me a tooltip with the full function declaration including variable names. Problem solved. [/i]
That assumes you are reading the code from a laptop in an IDE. You might instead receive a copy of the code on paper, or this is a code snippet posted by someone on a forum. (Granted, with the later you can take the trouble to copy and paste to your IDE and highlight–or you might as well just look it up.)
No one’s saying this is the programmer’s worst nightmare or anything. At the same time, with the use of enumeration constants, you won’t even need to take the effort to hover your mouse over it.
Another thing to consider is that BOOL parameters don’t even have to be TRUE or FALSE, which can come up when a caller is messing around with flags somewhere upstream. This can lead to inefficient (but necessary) paranoia such as "IS_TRUE(x) ((x) ? TRUE : FALSE)".
Crazy talk. What about all those functions that take ints. How about (this is in C#/.NET, but the point is the same)
System.DateTime.DateTime(int, int, int);
System.DateTime.DateTime(int, int, int, int, int, int);
System.DateTime.DateTime(int, int, int, int, int, int, int);
How is that easier? If you can guess the first two prototypes, can you guess what the 7th parameter in the 3rd means?
What about
System.Windows.Forms.Control.Control(string, int, int, int, int);
By your arguments, we should avoid using ints as well as they’re just as meaningless!
Or maybe, we could just … use the documentation! That’s what it’s there for.
Do people assume IDEs with IntelliSense/AutoComplete?
CreateEvent is a function that can create two distinct kinds of objects that have distinct behavior. I cannot imagine a situation where I would want to pass a variable (rather than a constant) into the bManualReset argument. On the other hand, with EnableWindow, imagining such a situation is quite easy (e.g. EnableWindow(hwndOk, isValidInput(…))). One might argue, then, that CreateEvent should have been two distinct factory functions — CreateAutoResetEvent and CreateManualResetEvent — or maybe CreateNotificationEvent and CreateSynchronizationEvent.
About the only definitely-reasonable place to have a boolean parameter is in a SetFlag() function that sets a boolean state, where the boolean specifies the desired state and there is no unSetFlag() function.
I don’t feel that the readability problem is type-dependant. If you’re writing for readability, simply don’t put statics of any type into function calls.
Compare:
sql_print_table("select * from myDB.myMembersTable", true, 50, 100);
vs:
sql_print_table($listMembersSQL, $SHOW_PAGE_NAV, $pageStart, $pageEnd);
However, I almost never program in an IDE, and I agree that overusing booleans in function definitions damages my programming speed. Even in an IDE, if you have a function definition:
fn search(Str $needle, Str $haystack, Bool $case)
… you can’t be sure whether true means "ignore case" or "respect case". Where there are two options, I prefer to have two functions, rather than a boolean option.
enum bool {
True,
False,
FileNotFound
}
(a nod to dailywtf http://thedailywtf.com/forums/47844/ShowPost.aspx )
It all depends on your env I suppose…
"That may be, but keep in mind that with C/C++, people can always cast their way into passing garbage. So public functions like "CreateEvent" should do parameter validation anyway whether you used a char or a BOOL for the parameter types."
Why exactly do you need to do parameter validation on a BOOL? If they cast some random structure into a BOOL, you still get your bits to determine which state it falls into. How do you propose validating the parameter besides eliminating negative values? This wouldn’t really harm the function, other than forcing it to be included in one of the two previous states.
All it takes to "fix" this is a few #define’s, and it won’t break compatability…
I’ve been thinking for a while now that it would be useful to have a language that required something like this:
CreateEvent( lpEventAttributes=NULL, bManualReset=TRUE, bInitialState=FALSE, lpName=NULL )
Using a editor that provided auto-completion this would be practical, and clear. Or just an editor that provided the names in place, so
CreateEvent(NULL,TRUE,FALSE,NULL)
Would look like my example.
@John:
This was actually considered and rejected by C++ Standard commitee. More details in Stroustrup’s D&E book.
My code is sprinkled with CreateEvent(0,0,0,0) and CreateEvent(0,1,0,0) depending on if I want an auto-reset or manual-reset event. Is it that bad? ;-)
I should probably use:
inline HANDLE CreateAutoResetEvent(void) { return CreateEvent(NULL,FALSE,FALSE,NULL); }
[i]I assume people would look to such for a solution before suggesting a change to the Win32 API. [/i]
Non-sequitar. No one is contemplating changing an API function that’s over a decade old. We are merely discussing whether, if we could’ve turned back time, whether it is a change worth doing. Or whether we should repeat the same thing in the future.
[i]Why exactly do you need to do parameter validation on a BOOL?[/i]
That’s a good point. Using the implicit C behavior of true being anything nonzero, there would no parameter validation necessary since all possible values are legal.
But then again, you can do the same with any primitive (non-structure) parameter types. The proposed char parameters can just as well have default meanings in the cases where it doesn’t match any of the expected values. Then you could get away with no validation, and then you’re back in the same situation where people could mix things up and get unexpected results. (Of course, with char’s it gets worse since there are more values that would fall under the "default" case.)
—-
On another note, another aspect of BOOL parameters worth noting is that if you later need to add one more, you’ll need to throw in yet another parameter. Whereas with enumerations or ints, you can use a bit-flag approach and simply define new flags rather than adding in another parameter.
Yes, I suppose you can use C++ default parameters to get around this, but then what happens when, for example, you need to pack some of the parameter data into an app-defined window message? The extra BOOL parameter might end up forcing you to pack the BOOLs back into a set of bitflags anyway.
This is why I generally write calls to system code something like:
<code>
hEvent = CreateEvent(NULL, // EventAttributes
TRUE, // Manual Reset
FALSE, // Initial State
NULL); // Name
</code>
Unless it’s a system call that I use alot then I never remember it when I come back to it 6 months later. And why should I have to?
b wrote:
=================SNIP=================
Me, I’m a fan of chars.
Since Enums are typically in a global name space, they rely on verbosity to remain unique, meaning that they are inevitably verbose.
CreateEvent(NULL, EVENTTYPE_AUTORESET, EVENTTYPE_INITIALSTATEISSOMETHING, NULL);
is hard to read because it’s too long, and is hard to write because you have to look up whether it’s EVENTTYPE_AUTORESET or EVENT_TYPE_AUTO_RESET or EVENTTYPE_AUTO every time.
So my proposed alternative,
CreateEvent(NULL, ‘a’, ‘s’, NULL);
fits onto one line, provides some pneumonic assistance, doesn’t require you to remember which direction was supposed to be zero, and doesn’t read like you’re yelling. It’s not perfect either, but I’ve found that it leads to less manual-reading for me than the inevitably verbose enums. Newbies to the function will need to check what ‘a’ means, but after seeing it a few times, it becomes as familiar as the verbose equivalent.
Smart IDEs don’t help with the verbosity issue, which is why people resorted to booleans to begin with. Enums are equivalent to naming the function
CreateEventWhereTheFirstBooleanIsZeroWhenAutoInitializing. Your autocomplete will gladly write it out for you, but there’s a reason nobody names their functions like that.
=================SNIP=================
That might be true for C++, but C# enumerations can be tied to classes (they’re basically like const integers). I class-specific enumerations quite a bit. Makes things much more readable.
public class Foo
{
public enum Bar
{
Asdf,
Qwert,
Zxcv
}
public int Operate(Bar Options)
{}
}
somebody would call Operate() like
Foo x= new Foo();
x.Operate(Foo.Bar.Asdf);
@peterchen
>ev = CreateEvent(NULL, false, false, NULL); // auto-reset, non-signaled
Or even
ev = CreateEvent( /*default security*/ NULL, /*auto-reset*/ false, /*non-signaled*/ false, /*anonymous*/ NULL);
It’s a simple rule that any anonymous constant should have an explanatory comment (0 and 1 when used arithmetically rather than as bit patterns and NULL/0 as a null object being possible exceptions). As a beneficial side-effect, the programmer is discouraged from using anonymous constants.
You don’t need to change the WinAPI to get the readability benefits of enums. All you need to do is update Win*.h and add an enormous number of consts or #defines:
#define ShowWindow_SHOW 1
#define ShowWindow_HIDE 0
Now we have [paper|intellisenseless|backwards compatible] friendly code:
ShowWindow(ShowWindow_Hide);
Doesn’t mean that any new functions should consider alternatives to Bools.
You guys could just go to a real language which could allow something like the following:
CreateEvent(lpEventAttributes:=NULL, bManualReset:=TRUE, bInitialState:=FALSE, lpName:=NULL)
Oh yeah, VB/VBA has allowed this for some time and it is still valid in VB.Net. I guess a number of you are saying that VB is superior than C#/C++ after all… ;-)
I totally agree with the philosophical intent to this entry. Depending on what I was working on I might use defines/enums, but more likely is:
#define CREATE_RESET_EVENT CreateEvent(0, 1, 0, 0)
or even more likely…
#define pthread_cond_init(pobject,pattr) (*pobject=CreateEvent(NULL,FALSE,FALSE,NULL))
as long as this is in the source file or a private header, I dont see any problem, and now my code is readable.
I’m having the same problem also with int-parameters from time to time. For example I cannot remember if it’s
memset(buf, size, value) or memset(buf, value, size). I have to look it up every single time *sigh*
JCMay: "That might be true for C++, but C# enumerations can be tied to classes (they’re basically like const integers)."
C++ has that too. They can even be made private. The drawback is that they can’t be used unless the compiler knows that they are *not* private, which means the entire class definition has to be visible. Which leads to extra #includes in client code, which is bad for dependancy management.
That is, a declaration like:
class CFoo;
void doit( CFoo &f, CFoo::Mode mode );
won’t compile. Some people use ints to get around this. I tend to give the class an associated namespace and rely on VC++’s non-stand forward declaration of enums feature:
class CFoo;
namespace Foo { enum Mode; } // Not standard C++.
void doit( CFoo &f, Foo::Mode mode );
is OK in VC++, if a bit ugly.
@peterchen
I think your idea is sheer genius! In a case like (partly repeating your example):
enum EventType { manual, auto }
enum BookType { manual, novel }
CreateEvent(manual);
CreateBook(manual);
There is no ambiguity. The compiler would have rejected BookType.manual in the CreateEvent call, and EventType.manual in the CreateBook call. As long as the context it’s used in is unambiguous about what type is acceptable (and with strongly typed formal parameters this is a cinch) and the various enums are all disjoint types (also easy), there should be no problem.
I want this feature in the next hot language! I wonder if it’s too late to get it into C# 3.0? I wonder if Anders ever reads Raymond’s blog?
@Chris
Although I wouldn’t opt for chars, either, it isn’t really true that they can only be checked at runtime. All decent C compilers these days check literal printf format strings against the rest of the parameters. This could be done for CreateEvent as well.
You might object that the parameters might not be compile-time constants, but in this case that’s unlikely, and it’s still worth it for the compiler to do the check if it can.
A few people have made competing points about Intellisense vs. reading the code in a third party editor or even on paper. It leads to the question: Can you read code outside the IDE? Have we gotten to the point where our tools do so much for us that we can’t, or at least don’t want to, read code without syntax highlighting, tooltips, etc?
I haven’t had the experience yet of trying to hover over a function call on a piece of paper to see a tooltip, but is it just a matter of time before I do, and then curse? :)
@b
pneumonic, n. airy memory device
Sorry, couldn’t resist! :)
PingBack from http://adaptivity.nl/blog/?p=8
@David Conrad
I think this is exactly how the Pascal handles enum types.
John, Jim: You’ll see keyword argument passing in Python and a few other languages out there, not just VBA.
In truth I don’t find it particularly useful most of the time. Its primary utility is when I have a function with a number of default parameters and I want to only override (eg) the third. Rather than having to specify the first and second anyway, I can just call the function with the named third argument.
In Python (a dynamically typed language) it’s also quite handy for what’s essentially varargs with a hash table. Like creating, populating, and passing a std::map parameter but much simpler and more convenient.
Using keyword arguments to enhance readability of otherwise ambiguous function calls is still nice, though.
All this reminds me – IronPython just went 1.0 so you can use Python with .NET. It might be of interest to some here.
It is bad to have to remember what BOOL does but IMO it is worse when you have to deal with functions which can return HANDLE, NULL and some "special" value to signal equally "special" error.
Also, many people do not know that built-in type "bool" generates safety check code for each conditional test to determine if the value is one of "true" or "false" so using built-in type slows the code down.
PingBack from http://www.red-sweater.com/blog/316/coding-for-readability