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.

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?

share|improve this question
2  
The question itself is language agnostic - you can do this type of thing in any language; but the code in the post is C++ –  bobobobo Dec 9 '13 at 18:11
2  
I also added a language-agnostic tag –  bobobobo Dec 9 '13 at 18:13
1  
2  
It's not the same question as that one. Here I'm asking whether the return early statement is better to even be in the method call in question. The other option is to check the condition before even invoking the function. –  bobobobo Dec 10 '13 at 0:07
1  
@bobobobo We've eliminated the language-agnostic tag. This being Code Review, our mission is to review code, not dispense general advice. Therefore, questions should be specific, and answers should take the context into account. Even if the code in the question is polyglot, the advice that reviewers give could vary depending on the language in ways that you might not anticipate. –  200_success Dec 10 '13 at 0:12

5 Answers 5

up vote 10 down vote accepted

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.

share|improve this answer
4  
And I would say the exact opposite thing. I would say draw() shouldn't look at isVisible(). If it did it should be called drawIfVisible(). Why? Because let's imagine I want a "debug view" where "hidden" things are at 50% opacity. I might want to draw everything visible, and then hidden things on different layer with 50% opacity. With your approach that is not possible as draw() will do nothing. Of course that depends on the rest of code/design, but I would opt for functions doing minimal amount of stuff and only stuff implied by name. –  elmo Dec 10 '13 at 11:35

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.

share|improve this answer
1  
codinghorror.com/blog/2007/03/curlys-law-do-one-thing.html Curly's Law: Do One Thing. –  WernerCD Dec 9 '13 at 19:55

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.

share|improve this answer
    
This is a really cool idea, but it's probably overkill for this. +1 from me. –  jliv902 Dec 10 '13 at 16:56
    
This is an interesting answer. You genericized drawing with for_each_if and passed the condition to draw as a functor arg. I've done something similar elsewhere in my code, but it was only where I really needed a functor to figure out (if x should y). –  bobobobo Dec 12 '13 at 10:58

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.

share|improve this answer
    
agreed, but I prefer to avoid negative logic when possible, so I'd do if(isShowing()) { this -> draw(); } –  GreyBeardedGeek Dec 10 '13 at 2:04
    
The only issue I have with this is that "render" and "draw" are basically synonyms, the functions could have been named the other way around as well. So people may call the wrong one. But naming this one "draw_if_showing" is ugly. –  RemcoGerlich Dec 10 '13 at 10:47
    
@RemcoGerlich You could make draw protected, then possibly make a wrapper function called forceDraw. I agree that the names could cause some confusion, but I think there are ways around that. –  MirroredFate Dec 10 '13 at 19:25

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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