17

Which of these designs is better? What are the pros and cons of each? Which one would you use? Any other suggestions of how to deal with methods like is are appreciated.

It is reasonable to assume that Draw() is the only place that the other draw methods are called from. This needs to expand to many more Draw* methods and Show* properties, not just the three shown here.

public void Draw()
{
    if (ShowAxis)
    {
        DrawAxis();
    }

    if (ShowLegend)
    {
        DrawLegend();
    }

    if (ShowPoints && Points.Count > 0)
    {
        DrawPoints();
    }
}

private void DrawAxis()
{
    // Draw things.
}

private void DrawLegend()
{
    // Draw things.
}

private void DrawPoints()
{
    // Draw things.
}

Or

public void Draw()
{
    DrawAxis();
    DrawLegend();
    DrawPoints();
}

private void DrawAxis()
{
    if (!ShowAxis)
    {
        return;
    }

    // Draw things.
}

private void DrawLegend()
{
    if (!ShowLegend)
    {
        return;
    }

    // Draw things.
}

private void DrawPoints()
{
    if (!ShowPoints ||  Points.Count <= 0))
    {
        return;
    }

    // Draw things.
}
Improve this question
2

9 Answers 9

Reset to default
28

I don't think you can have a blanket rule for this sort of thing, it depends on the situation.

In this case, I would suggest having the if clauses outside the methods because the names of the Draw methods imply that they just draw the things without any special conditions.

If you find that you have to make the checks before calling the methods in many places, then you might want to put the check inside the methods and rename them to clarify that this is happening

Improve this answer
0
7

I say the second.

Methods should have the same level of abstraction.

In the second example, the responsibility of the Draw() method is to act like a controller, calling each of the individual drawing methods. All of the code in this Draw() method is at the same level of abstraction. This guideline comes into play in re-use scenarios. For example, if you were tempted to re-use the DrawPoints() method in another public method (let's call it Sketch()), you would not need to repeat the guard clause (the if statement deciding whether to draw the points).

In the first example, though, the Draw() method is responsible for determining whether to call each of the individual methods, and then calling those methods. Draw() has some low-level methods that does work, yet it delegates other low-level work to other methods, and thus Draw() has code at different levels of abstraction. In order to re-use DrawPoints() in Sketch(), you would need to replicate the guard clause in Sketch() as well.

This idea is discussed in Robert C. Martin's book "Clean Code", which I'd highly recommend.

Improve this answer
1
  • 1
    Good stuff. To address a point made in another answer, I would suggest changing the names of DrawAxis() et. al. to indicate that their execution is conditional, maybe TryDrawAxis(). Otherwise you need to navigate from the Draw() method to each individual submethod to confirm its behavior.
    – Josh Earl
    Mar 16, 2011 at 12:41
6

My opinion about the subject is quite controversial, but bear with me, as I believe pretty much everyone agrees on the end result. I just have a different approach of getting there.

In my article Function Hell, I explain why I don't like splitting methods just for the sake of creating smaller methods. I only split them when I know, they will be reused, or ofcourse, when I can reuse them.

The OP stated:

It is reasonable to assume that Draw() is the only place that the other draw methods are called from.

This leads me to a (intermediate) third option not mentioned. I create 'code blocks' or 'code paragraphs' which others would create functions for.

public void Draw()
{
    // Draw axis.
    if (ShowAxis)
    {
        // Drawing code ...
    }

    // Draw legend.
    if (ShowLegend)
    {
        // Drawing code ...
    }

    // Draw points.
    if (ShowPoints && Points.Count > 0)
    {
        // Drawing code ...
    }
}

The OP also stated:

This needs to expand to many more Draw* methods and Show* properties, not just the three shown here.

... so this method will grow very large quickly. Almost everyone agrees this decreases readability. In my opinion the proper solution isn't just splitting into several methods, but splitting the code into reuseable classes. My solution would probably look something like this.

private void Initialize()
{               
    // Create axis.
    Axe xAxe = new Axe();
    Axe yAxe = new Axe();

    _drawObjects = new List<Drawable>
    {
        xAxe,
        yAxe,
        new Legend(),
        ...
    }
}

public void Draw()
{
    foreach ( Drawable d in _drawObjects )
    {
        d.Draw();
    }
}

Ofcourse arguments would still need to be passed and such.

Improve this answer
1
  • Although I disagree with you about Function hell, your solution is (IMHO) the correct one, and the one that would arise from proper application of Clean Code & SRP.
    – Paul Butcher
    Mar 17, 2011 at 14:20
4

For the cases where you're checking a field that controls whether or not to draw, it makes sense to have that be inside Draw(). When that decision becomes more complicated, I tend to prefer the latter (or split it). If you end up needing more flexibility you can always expand it.

private void DrawPoints()
{
    if (ShouldDrawPoints())
    {
        DoDrawPoints();
    }
}

protected void ShouldDrawPoints()
{
    return ShowPoints && Points.Count > 0;
}

protected void DoDrawPoints()
{
    // Draw things.
}

Notice the additional methods are protected which allows subclasses to extend what they need. This also allows you to force drawing for testing or whatever other reason you may have.

Improve this answer
1
  • This is nice: everything that gets repeated, is in its own method. You can now even add a DrawPointsIfItShould() method that shortcuts the if(should){do} :)
    – Konerak
    Mar 16, 2011 at 10:44
4

Of those two alternatives, I prefer the first version. My reason is that I want a method to do what the name implies, without any hidden dependencies. DrawLegend should draw a legend, not perhaps draw a legend.

Steven Jeuris' answer is better than the two versions in the question, though.

Improve this answer
3

Instead of having individual Show___ properties for each part, I'd probably define a bit field. That would simplify it a bit to:

[Flags]
public enum DrawParts
{
    Axis = 1,
    Legend = 2,
    Points = 4,
    // More...
}

public class MyClass
{
    public void Draw() {
        if (VisibleParts.HasFlag(DrawPart.Axis))   DrawAxis();
        if (VisibleParts.HasFlag(DrawPart.Legend)) DrawLegend();
        if (VisibleParts.HasFlag(DrawPart.Points)) DrawPoints();
        // More...
    }

    public DrawParts VisibleParts { get; set; }
}

Aside from that, I would lean towards checking visibility in the outer method, not the inner. It is, after all, DrawLegend and not DrawLegendIfShown. But it kind of depends on the rest of the class. If there are other places that call that DrawLegend method and they need to check ShowLegend too, I'd probably just move the check into DrawLegend.

Improve this answer
2

I would go with the first option - with the "if" outside the method. It better describes the logic being followed, plus gives you the option to actually draw an axis, for example, in cases when you want to draw one regardless of a setting. Plus it removes the overhead of an additional function call (assuming its not inlined), which can accumulate if you are going for speed (your example looks like it may just be drawing a graph where it may not be a factor, but in an animation or game it could be).

Improve this answer
1

In general, I prefer to have the lower levels of code assume the preconditions are met and get to doing whatever work they are supposed to be doing, with the checks being done higher up in the call stack. This has the side benefit of saving cycles by not doing redundant checks, but it also means you can write nice code like this:

int do_something()
{
    sometype* x;
    if(!(x = sometype_new())) return -1;
    foo(x);
    bar(x);
    baz(x);
    return 0;
}

instead of:

int do_something()
{
    sometype x* = sometype_new();
    if(ERROR == foo(x)) return -1;
    if(ERROR == bar(x)) return -1;
    if(ERROR == baz(x)) return -1;
    return 0;
}

And of course this gets even nastier if you enforce single-exit, have memory you need to free, etc.

Improve this answer
0

It all depends on context:

void someThing(var1, var2)
{
    // If input fails validation then return quickly.
    if (!isValid(var1) || !isValid(var2))
    {   return;
    }


    // Otherwise do what is logical and makes the code easy to read.
    if (doTaskConditionOK())
    {   doTask();
    }

    // Return early if it is logical
    // This is OK in C++ but not C like languages.
    // You have to be careful of cleanup in C like languages while RIAA will do
    // that auto-magically in C++. 
    if (allFinished)
    {   return;
    }

    doAlternativeIfNotFinished();

    // -- Alternative to the above for C
    if (!allFinished)
    {   
        doAlternativeIfNotFinished();
    }

}
Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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