Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

5 Answers 5

up vote 13 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

I would vote for overkill! Instead, you should prefix functions taking a single boolean parameter with is or has to improve readability:

void is_hidden( bool value );
// or
void has_animation( bool value );

Personally I would go with something like this:

class Button
{
    public:
        // ... other stuff ...

        bool is_animated() const
        {
            return is_animated_;
        }

        // Overloaded version - the return value probably wouldn't ever be used.
        bool is_animated( const bool animated )
        {
            is_animated_ = animated;
            return animated;
        }
    private:
        bool is_animated_;
};

Elsewhere:

Button button;
button.is_animated( true ); // Ignore return value

// Later on...
if ( button.is_animated() )
{
    // ...
}
share|improve this answer
    
The first problem here is that sometimes you want to animate the button and sometimes you don't. You would need to set the property first and then call the hide function. The second problem is that it's not clear which actions will be animated and which will not. –  daknøk Jun 8 '12 at 11:10
5  
How did this get upvoted? It’s all backwards. Getters, not setters, should be prefixed with is or has. Furthermore, OP’s code actually does something completely different, it isn’t concerned with getters or setters. –  Konrad Rudolph Jun 8 '12 at 13:24
    
Getters and setters are the opposite of OOP. Calling a bunch of setters to set flags then calling the function... Yuck. That's what function/method parameters are for. –  dash-tom-bang Jan 31 '13 at 2:23

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.