Boolean enums: improved clarity or just overkill? [closed]

Question

Suppose we are writing a GUI toolkit in C++ (though this question may also apply to other languages). We have a button class with a member function hide, which hides the button. This member function takes a Boolean parameter animated to control if the button should be hidden with an animation.

class Button {
public:
  // Rule of three, etc…

  void hide(bool animated);
};

When invoking this member function, it may not be clear what it does.

Button button;
button.hide(false); // well, does it hide the button or not?
                    // what does "false" even mean here?!

---

We could rewrite this using a Boolean enum.

class Button {
public:
  // Rule of three, etc…

  enum Animated : bool {
    Animate = true,
    DoNotAnimate = false,
  };
  void hide(Animated animated);
};

Now if we call it, everything becomes more clear.

Button button;
button.hide(Button::DoNotAnimate);

---

Is this a good thing to do? Does it improve clarity of the code, or is it just overkill and should we use separate documentation (Doxygen-like) for this instead?

Answer 1

I think the enum is a very nice solution here. And in a way I disagree with Johannes, even for single-use the enum improves readability and discoverability of the API, and writing it is a negligible effort; and I’d be wary of using comments as in his example, they scream “hack”.

Answer 2

I think it is always a good idea to improve clarity of the code, and your change does improve clarity indeed.

If this is the only use of that enum, I would consider it too high of a cost to introduce that enumeration though. I came to adopt the Clang practice on that issue

Button button;
button.hide(false /* don't animate */);

Doxygen doesn't help on impoving clarity in the calling code, which is the issue here.

Answer 3

In general I'm all for enums over booleans, especially when you have a string of "flags" that you need to pass into a function since booleans get... unreasonable at that point.

In this case and similar ones though there's another option; hide() and show() are the "do this right now" versions of the functions and animate_hide() and animate_show() do the same work but asynchronously. I bring this up because I suspect that the body of the "combined" function would largely be one if/else statement:

void Button::hide(bool animated)  // or whatever parameter type is appropriate
{
   if (animated)
   {
      // do animated work, set up timers and callbacks etc.
   }
   else
   {
      // do immediate work
   }
}

Answer 4

If the method does one thing, in this case, hides the button there shouldn't be a parameter at all: void Hide(). Conversely, you can create a method void Show() that only shows the button. You can even have another to check if it is currently visible: bool IsVisible() which just returns the current state of a member bool.

The void Show() and void Hide() methods can delegate their calls to a method that changes the state of the member bool: void Visible(bool is_visible):

Definition:

class Button {
    public:
        //Other methods...
        void Show();
        void Hide();
        bool IsVisible();
    private: //or protected:, depending on your requirements
    //Other methods...
    void Visible(bool is_visible);
    //Other members...
    bool _visible;
};

Implementation:

void Button::Show() {
    Visible(true);
}

void Button::Hide() {
    Visible(false);
}

bool Button::IsVisible() {
    return _visible;
}

void Button::Visible(bool is_visible) {
    _visible = is_visible;
}

The void Visible(bool is_visible) member method enforces that only one function has the responsibility to change the _visible member and makes it so there is only one place that needs to change if the requirement changes.

All of these methods make the code a lot cleaner and less ambiguous. Each method also only does exactly one thing.