Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question

closed as off-topic by Jamal Jul 16 '14 at 22:20

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

    
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. – Sahuagin Oct 23 '15 at 18:10
up vote 16 down vote accepted

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

share|improve this answer
    
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. – daknøk Jun 8 '12 at 13:43

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.

share|improve this answer

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
   }
}
share|improve this answer

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.

share|improve this answer
4  
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. – daknøk Jun 8 '12 at 15:57

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