🔒 Locked

Magic Booleans

Started by Mithrandir Feb 8, 2006 at 3:37 PM 7 replies 3.1k views

Original Post

Feb 8, 2006 at 3:37 PM

I came across an interesting problem today. I've entirely phased out "magic numbers" in my programs; haven't had one of those in at least 3-4 years (except for 0, of course). The reasoning, of course, is to avoid unreadable code:


if( x == 86.4 )  // what the F does 86.4 represent?
{
    // blah
}

Ok. So onto something else that bothered me. Magic Booleans. I had a function:


    public class EntityNotifier
    {
        public static void PreNotify( long entity, Notification notification, bool recursive )
        {
            Entity e = Database.Get( entity );
            e.PreNotify.Notify( notification );

            if( recursive )
            {
                foreach( long child in e.Children )
                {
                    PreNotify( child, notification, recursive );
                }
            }
        }
    }

(this is rough prototype code, by the way, don't nitpick it) Okay, so that's all cool and all, but what about the callers of the code? They'll do something like this:


EventNotifier.PreNotify( roomid, leaveroom, true );

The code is pretty readable... except for that boolean sticking at the end. I oftentimes find myself, whenever I see a boolean, wondering "what the F does that boolean mean?". So, I was thinking... would it not be more appropriate to use enums instead of booleans to make code more clean and robust?


public enum RecursionState
{
    None,
    Recursive
}

<snip>

EventNotifier.PreNotify( roomid, leaveroom, RecursionState.Recursive );

To me, this solution makes the code much more legible, and I think I'll prefer doing this from now on... but I was wondering what other people think? Is this too much extra work, or do you guys generally think the end result is worth it?

Mithrandir

Feb 8, 2006 at 3:44 PM

I should note that I've already had one benefit of using enums rather than bools; I had a class that accepted true/false in the constructor to tell the class that it was in one of two mutually exclusive states... and I switched it over to an enum to represent the states.

Later on, I realised that the class required 3 mutually exclusive states, unrepresentable by a bool, and luckily for me, the enum was already there, so adding the new state was trivial.

Plus, enums in C# are heaven compared to that pathetic clusterfuck of a trainwreck they call enums in C++ (grin), so I don't have to worry about playing messed up namespace games to avoid conflicting enum names (gee, there are about 100 different enums that use the state "None". Thanks for making this situation almost uncodable in C++!!)

Washu

Feb 8, 2006 at 3:47 PM

In other words you've stumbled upon: "Truth or Consequences". Enum it IMO.

the_dannobot

Feb 8, 2006 at 3:49 PM

I use something like that all the time for error reporting, instead of just returning bool from all functions to represent failure/success.

class CThing{  public:    enum TypeError    {      Error_NoError = 0,      Error_NoMemory,      Error_NoFile,      Error_BadPhaseOfMoon,      Error_MonthEndingInR,      Num_ErrorTypes    };    static const char *GetErrorText(const TypeError eType)    {      switch (eType)      {        case Error_NoMemory:        {          return "Not enough memory";        }        break;                <snip>      }    }    TypeError FunctionCall()    {       //return Error_NoError on success, whatever else on failure       <snip>    }};int main(){  CThing inst;  CThing::TypeError eError = inst.FunctionCall();  if (eError != CThing::TypeError::ErrorNoError)  {    cout << "Hey dumbass: " << CThing::GetErrorText(eError);    return 0;  }  <snip>}

superpig

Feb 8, 2006 at 3:52 PM

I think it's an improvement, but it doesn't address the problem where you need to do something like pass two strings to a function - example would be MessageBox() that takes both a message and a caption for the box. Which is the caption and which is the message? It may not be obvious at a glance.

One of the features I particularly liked in Objective C (something that I believe it pinched from Smalltalk) is function calls that explicitly name parameters; it makes code clearer and also gives you more flexibility with things like default arguments. Your call to PreNotify would look something like this:

PreNotify(entity: roomid, notification: leaveroom, recursive: true)

You can now see immediately what each parameter means, eliminating the need for extra enum types like you describe. It would also be trivial to allow switching the order of arguments or leaving 'notification' at a default value.

This is actually related to one of the things that the Direct3D team have changed for Direct3D 10 - a great number of functions have been changed to take pointers to structures instead of lists of arguments. This means that you're able to write out explicit "args.parameter = value;" code before making the call.

KaptainKomunist

Feb 8, 2006 at 3:52 PM

Yeah, that does make a lot of sense. It's kinda like when I was working with python and calling functions, I would go the extra distance and do this.

SomeFunction(xval = 21, yval = 44)

instead of

SomeFunction(21, 44)

It makes it a ton easier to learn to work with that language, since it isn't type strong. Yeah, enumerating instead of relying on raw booleans is a good idea. In C++ though, You do it with #define, not enum. ;)

Sneftel

Feb 8, 2006 at 3:54 PM

Booleans are sort of an extreme situation (because they are only two-valued) of a more general problem, which is positional argument lists. In the function call cycleWidgetDiscriminator(context, NULL, 1, true), I'd argue that all of the last three parameters are confusing.

Now, you could argue that 1 is a magic number, and do something like this:

const int WIDGETMULTIPLIER_ONE = 1;...cycleWidgetDiscriminator(context, NULL, WIDGETMULTIPLIER_ONE, true)

...but hell, why not go to town?

const int WIDGETMULTIPLIER_IDENTITY = 1;const WidgetOptions* WIDGETOPTIONS_NONE = NULL;const bool DOADJUSTCHILDREN = true;...cycleWidgetDiscriminator(context, WIDGETOPTIONS_NONE, WIDGETMULTIPLIER_ONE, DOADJUSTCHILDREN)

... ugh. I might care what the identity element is elsewhere in the program, I might not. I certainly don't like the idea of a global DOADJUSTCHILDREN, and I don't like setting up an ad-hoc CYCLEWIDGETDISCRIMINATOR_ARGUMENTS_DOADJUSTCHILDREN namespace either. I'm a grownup, and I can remember that "adjustChildren=true" means do adjust the children, and "adjustChildren=false" means don't. Let's make that a little better scoped.

{    WidgetOptions* widgetOptions = NULL;    int multiplier = 1;    bool adjustChildren = true;    cycleWidgetDiscriminator(context, widgetOptions, multiplier, adjustChildren)}

Now this, of course, is a redirection. We aren't mentioning anymore that "1" is the identity element for multiplier; instead, we're more interested in mentioning that the third argument is a multiplier. I think this is a good thing: When you hear "multiplier", you know from high school algebra that 1 is the identity element. And as a programmer, you already know what true and false are, and you probably realize that NULL in a pointer context means "don't use". Wrapping it all up in a scope makes it clear that these variables only exist as arguments.

Of course, what would be even better:

cycleWidgetDiscriminator(context=context, widgetOptions=NULL, multiplier=1, adjustChildren=true)

Cannot do, of course, at least in C++. But it seems to me that that's really what you want from the language. Some IDEs will do it for you while you're typing, but that's not much comfort when reading code (especially printed-out code).

EDIT: Beaten like a red-headed stepchild. Which, in fact, I currently am. Whoops.

Sneftel

Feb 8, 2006 at 3:58 PM

BTW: I should clarify that I do NOT agree with the idea of enumifying things that should be booleans. That's a hack which only works on discrete values, and its form (datatype) is not suited to its function (argument). Would you make an enum for every non-argument local boolean in your program? No! You'd name them correctly. Similarly, this should be a call for named arguments, not enumed bools.

Mithrandir

Feb 8, 2006 at 4:03 PM

Quote:
Original post by Sneftel
BTW: I should clarify that I do NOT agree with the idea of enumifying things that should be booleans. That's a hack which only works on discrete values, and its form (datatype) is not suited to its function (argument). Would you make an enum for every non-argument local boolean in your program? No! You'd name them correctly. Similarly, this should be a call for named arguments, not enumed bools.

Interesting.

I never thought about it this way, and it seems to make sense.

For example, in the function itself, the parameter is named "recursive", and it makes perfect sense within the context of the function.

But it doesn't on the outside.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.