Is `if( hidden ) return;` an antipattern?

Question

Sometimes, when you're doing something like displaying a bunch of elements, you can avoid running a function entirely if an object is hidden.

So you can either write the check from the loop:

for( Displayable * d : displayables )
    if( d->isShowing() )
        d->draw() ;

Or you can put an "early return" in d->draw() if d is hidden:

void Displayable::draw()
{
    if( !isShowing() )  return ;
    // continue with draw..
}

With this 2nd way, you don't have to check any condition when looping, so the loop "looks cleaner"

for( Displayable * d : displayables )
    d->draw() ;

But with the 1st way, you don't have to check isShowing() at the beginning of the function, so then that looks cleaner,

void Displayable::draw()
{
    // simply draw
}

Which is better? Is the latter an antipattern of sorts?

Answer 1

There's a semantic difference between the two of these. In the first, the draw method always draws the object whether the object is visible or not. I.e. it is the caller's responsibility to check the visibility of the object. In the second, the object checks it's own visibility and recognises that it does not need to do anything if the object is hidden.

The difference in the loop, then, is the difference between saying "Draw all of the non-hidden elements", and "Draw all of the elements, (where draw means draw the element if it is visible)"

One focuses on the fact that we're drawing visible elements, the other adds the concept of visbility to the method draw.

In my opinion, the second of these better fits the semantic of what draw is supposed to do, and for that reason it is clearly better to perform the visibility check inside draw rather than inside the loop.

Answer 2

You should focus on whatever methods avoid code duplication.

// Some code
for( Displayable * d : displayables )
    if( d->isShowing() )
        d->draw() ;

// Some code elsewhere
for( Displayable * d : displayables )
        // Oops, we forgot to check if it's showing.
        d->draw() ;

There are two ways I can think of off the top of my head to avoid this situation.

1. Use your second approach and perform the check in the draw() method.
2. Create a drawAllShowing() function/method.

Creating a drawAllShowing() function would allow you to keep the draw() method simple just in case there are circumstances where you do not want to perform the if(d->isShowing()) check when calling draw(). If you always want to perform that check, then just put it in the draw() method.

Just remember, some famous guy once said that a function should do one thing, and it should do that one thing well.

Answer 3

I think I'd leave the responsibility with the loop, but instead of a loop with the logic in its body, I'd write an algorithm that made the intent a little more explicit:

template <class InIter, class Cond, class Action>
void for_each_if(InIter b, InIter e, Action action, Cond cond) {
    while (b != e) {
        if (cond(*b))
            action(*b);
        ++b;
    }
}

For a testing, I did a mock implementation of Displayable:

struct Displayable {
    bool showing;
    std::string label;

    Displayable(bool showing, std::string label) : showing(showing), label(label) {}
    bool isShowing() const { return showing; }
    void draw() const { std::cout << label; }
};

To exercise it, I wrote a simple main like:

int main(void){
    std::vector<Displayable> d{ { true, "Showing" }, { false, "not showing" } };

    for_each_if(d.begin(), d.end(), 
            [](Displayable const &d) { d.draw(); }, 
            [](Displayable const &d){return d.isShowing(); });
}

Although it's even more severe overkill, and doesn't fit the scenario quite as portrayed above, there's another possibility that might be worth considering under the right circumstances. First, I'll assume that in reality, you specify some surface when you do drawing. This would be a class that represents a device context on Windows, a graphics context on iOS, etc.

For the moment, I'll define a really trivial "surface" that just acts as a wrapper for an ostream:

struct surface {
    std::ostream &os;
public:
    surface(std::ostream &os) : os(os) {}

    void draw(std::string const &s) { os << s; }
};

Using that, we can define an ostream-like interface to a surface:

struct Displayable {
    bool showing;
    std::string label;

    Displayable(bool showing, std::string label) : showing(showing), label(label) {}
    bool isShowing() const { return showing; }

    friend surface &operator<<(surface &s, Displayable const &d) {
        s.draw(d.label);
        return s;
    }
};

Then we get to a somewhat ugly part: defining an iterator to write Displayables to a surface. This is quite a bit longer than we'd like, but most of it is pure boiler-plate. The only part that really matters at all is the assignment operator, which writes a Displayable to a surface:

template <class T, class charT = char, class traits = std::char_traits<charT>>
struct draw_iterator : public std::iterator<std::output_iterator_tag, void, void, void, void> {
    surface *os;
public:
    draw_iterator(surface& s) 
        : os(&s)
    {}
    draw_iterator<T, charT, traits>& operator=(T const &item)
    {
        *os << item;
        return *this;
    }

    draw_iterator<T, charT, traits> &operator*() {
        return *this;
    }
    draw_iterator<T, charT, traits> &operator++() {
        return *this;
    }
    draw_iterator<T, charT, traits> &operator++(int) {
        return *this;
    }
};

With those in place, we can then use a standard algorithm to write the chosen Displayable objects to the surface:

int main(void){
    std::vector<Displayable> d{ { true, "Showing" }, { false, "not showing" } };

    surface s(std::cout);

    std::copy_if(d.begin(), d.end(),
        draw_iterator<Displayable>(s),
        [](Displayable const &d){return d.isShowing(); });
}

Of course, the "surface" I've defined is extremely limited (only knows how to display strings right now), but it's intended purely as a mock-up. As I already said, in a real implementation you probably already have something that corresponds to it, so you wouldn't typically be implementing that part at all, just setting up the stream-like interface, and the iterator for it.

The iterator, in particular is more extra work than you'd typically like. The good point, however, is that when you're done it acts like any other iterator--it'll support all the standard algorithms, not just this particular one.

Answer 4

I personally think that a third option would be best: render().

void Displayable::render()
{
    //this could include other checks
    if( !isShowing() )  return ;
    // call draw
    this->draw();
}

This make the sole responsibility of draw actually drawing the object, and render's job is then determining what gets drawn when.

Answer 5

I'd keep it so Draw draws no matter what. That is, Draw doesn't decide if it draws, whomever called it decided to draw the object when they called draw. This gives you more flexibility because it decouples the functionality from the decision to leverage it.

One simple advantage of doing it this way is that now you can have a debug view that draws everything no matter what.

There is however another big reason to do it this way that really depends on the nature of the application you're developing. Lets say there are vastly more objects in existence than you will ever want to attempt to draw (or even loop over if you can help it), and you want to package up just the things you actually want to draw into a list and process that on another threading using the GPU while you do something else with the CPU. This would certainly be the case if you were developing an MMO for instance. In this scenario you're going to want the functionality to draw decoupled from decision to draw.