34
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I dunno if they are in C++, but a good alternative that does not require a new type to be defined is using a named parameter. button.hide(animated: false);, for example. Otherwise it may be a good idea to try to use more general-purpose enums if possible, rather than overly-specific ones, but I would definitely prefer it to the hide(false) version. Another possibility is to use two separate named methods, maybe hideWithAnimation and hideNoAnimation or something. \$\endgroup\$ – Dave Cousineau Oct 23 '15 at 18:10
  • \$\begingroup\$ hide() should hide the button, and take no parameter. If buttons need to know different ways to hide themselves, then maybe they should have state (e.g. setHideAnimationOn()), or have different types (AnimatedButton vs NormalButton), or be called with different methods (hideWithAnimation() vs hide()). \$\endgroup\$ – mabraham Sep 1 '17 at 10:50
27
\$\begingroup\$

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”.

\$\endgroup\$
  • \$\begingroup\$ I think that it's good to be consistent (i.e. either never use Boolean enums or always use them), and I agree about the comments. \$\endgroup\$ – daknøk Jun 8 '12 at 13:43
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ What does it cost exactly? Microseconds of compilation time? \$\endgroup\$ – Vladislav Toncharov Aug 16 at 14:16
  • 1
    \$\begingroup\$ @Vla the cost is in terms of cluttering your code with an enum type declarations. 7 years later and with 7 years more programming experience, I'm not too sure anymore of the generality of that assertion, though. \$\endgroup\$ – Johannes Schaub - litb Aug 16 at 20:14
  • \$\begingroup\$ Hmm... I'd say the clutter point is valid, but bool enums are typically small enough to condense to a single line and still be readable, thus rendering it a non-issue. Commentation is valid if guaranteed to be visible (such as if offline documentation is provided or the source code is available), but typically less readily available from an IDE. Overall, I'd say the cost is outweighed by the benefit. \$\endgroup\$ – Justin Time 2 Reinstate Monica Aug 21 at 19:36
7
\$\begingroup\$

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
   }
}
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ Is suggest you read the question. This member function takes a Boolean parameter animated to control if the button should be hidden with an animation. \$\endgroup\$ – daknøk Jun 8 '12 at 15:57

Not the answer you're looking for? Browse other questions tagged or ask your own question.