Sollte es sich bei Aussagen um innere oder äußere Methoden handeln?

Welches dieser Designs ist besser? Was sind die Vor- und Nachteile von jedem? Welches würdest du verwenden? Alle anderen Vorschläge, wie mit Methoden umgegangen werden soll, sind willkommen.

Es ist vernünftig anzunehmen, dass Draw () der einzige Ort ist, von dem aus die anderen Zeichenmethoden aufgerufen werden. Dies muss um viele weitere Draw * -Methoden und Show * -Eigenschaften erweitert werden, nicht nur um die drei hier gezeigten.

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

Oder

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

Ich denke nicht, dass Sie eine Pauschalregel für diese Art von Dingen haben können, es hängt von der Situation ab.

In diesem Fall würde ich vorschlagen, die if-Klauseln außerhalb der Methoden zu haben, da die Namen der Draw-Methoden implizieren, dass sie die Dinge nur ohne besondere Bedingungen zeichnen.

Wenn Sie feststellen, dass Sie die Prüfungen durchführen müssen, bevor Sie die Methoden an vielen Stellen aufrufen, sollten Sie die Prüfung in die Methoden einfügen und sie umbenennen, um zu verdeutlichen, dass dies geschieht

Ich sage das zweite.

Methoden sollten den gleichen Abstraktionsgrad haben.

Im zweiten Beispiel hat die Draw()Methode die Aufgabe , sich wie ein Controller zu verhalten und jede einzelne Zeichenmethode aufzurufen. Der gesamte Code in dieser Draw()Methode befindet sich auf derselben Abstraktionsebene. Diese Richtlinie kommt in Wiederverwendungsszenarien zum Einsatz. Wenn Sie beispielsweise versucht wären, die DrawPoints()Methode in einer anderen öffentlichen Methode erneut zu verwenden (nennen wir sie Sketch()), müssten Sie die guard-Klausel nicht wiederholen (die if-Anweisung, mit der entschieden wird, ob die Punkte gezogen werden sollen).

Im ersten Beispiel ist die Draw()Methode jedoch dafür verantwortlich, zu bestimmen, ob die einzelnen Methoden aufgerufen werden sollen, und diese Methoden dann aufzurufen. Draw()Es verfügt über einige funktionsfähige Methoden auf niedriger Ebene, delegiert jedoch andere Methoden auf niedriger Ebene und Draw()verfügt daher über Code auf verschiedenen Abstraktionsebenen. Um die Weiterverwendung DrawPoints()in Sketch(), müßten Sie die Schutzklausel in nachzubilden Sketch()als auch.

Diese Idee wird in Robert C. Martins Buch "Clean Code" diskutiert, das ich wärmstens empfehlen würde.

Meine Meinung zu diesem Thema ist ziemlich kontrovers, aber ich bin der Meinung, dass sich so ziemlich alle über das Endergebnis einig sind. Ich habe nur einen anderen Ansatz, um dorthin zu gelangen.

In meinem Artikel Function Hell erkläre ich, warum ich es nicht mag, Methoden zu teilen, nur um kleinere Methoden zu erstellen. Ich teile sie nur, wenn ich weiß , dass sie wiederverwendet werden oder natürlich, wenn ich sie wiederverwenden kann.

Das OP erklärte:

Es ist vernünftig anzunehmen, dass Draw () der einzige Ort ist, von dem aus die anderen Zeichenmethoden aufgerufen werden.

Dies führt mich zu einer (Zwischen-) dritten Option, die nicht erwähnt wird. Ich erstelle 'Codeblöcke' oder 'Codeabsätze', für die andere Funktionen erstellen würden.

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

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

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

Das OP erklärte auch:

Dies muss um viele weitere Draw * -Methoden und Show * -Eigenschaften erweitert werden, nicht nur um die drei hier gezeigten.

... so wird diese Methode sehr schnell groß. Fast alle sind sich einig, dass dies die Lesbarkeit beeinträchtigt. Meiner Meinung nach ist die richtige Lösung nicht nur die Aufteilung in mehrere Methoden, sondern die Aufteilung des Codes in wiederverwendbare Klassen. Meine Lösung würde wahrscheinlich so aussehen.

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();
    }
}

Natürlich müssten noch Argumente übergeben werden und so.

In den Fällen, in denen Sie ein Feld markieren, das steuert, ob gezeichnet werden soll oder nicht, ist es sinnvoll, dieses Feld im Inneren zu haben Draw(). Wenn diese Entscheidung komplizierter wird, bevorzuge ich die letztere (oder teile sie auf). Wenn Sie mehr Flexibilität benötigen, können Sie diese jederzeit erweitern.

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

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

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

Beachten Sie, dass die zusätzlichen Methoden geschützt sind, sodass Unterklassen ihre Anforderungen erweitern können. Auf diese Weise können Sie das Zeichnen auch zu Testzwecken oder aus anderen Gründen erzwingen.

Von diesen beiden Alternativen bevorzuge ich die erste Version. Mein Grund ist, dass ich möchte, dass eine Methode tut, was der Name impliziert, ohne versteckte Abhängigkeiten. DrawLegend sollte eine Legende zeichnen, nicht vielleicht eine Legende.

Steven Jeuris 'Antwort ist jedoch besser als die beiden Versionen in der Frage.

Anstatt Show___für jedes Teil individuelle Eigenschaften zu haben, würde ich wahrscheinlich ein Bitfeld definieren. Das würde es ein wenig vereinfachen:

[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; }
}

Abgesehen davon würde ich mich bemühen, die Sichtbarkeit in der äußeren und nicht in der inneren Methode zu überprüfen. Es ist schließlich DrawLegendund nicht DrawLegendIfShown. Aber es hängt irgendwie vom Rest der Klasse ab. Wenn es andere Stellen gibt, die diese DrawLegendMethode aufrufen, und sie auch überprüfen müssen ShowLegend, würde ich den Scheck wahrscheinlich einfach in verschieben DrawLegend.

Ich würde mit der ersten Option gehen - mit dem "Wenn" außerhalb der Methode. Es beschreibt besser die Logik, der gefolgt wird, und gibt Ihnen die Möglichkeit, eine Achse tatsächlich zu zeichnen, beispielsweise in Fällen, in denen Sie eine Achse unabhängig von einer Einstellung zeichnen möchten. Außerdem wird der Overhead eines zusätzlichen Funktionsaufrufs entfernt (vorausgesetzt, er ist nicht inline), der sich ansammeln kann, wenn Sie Geschwindigkeit anstreben (Ihr Beispiel sieht so aus, als würde es nur eine Grafik zeichnen, bei der es sich möglicherweise nicht um einen Faktor handelt, sondern um eine Animation) oder Spiel könnte es sein).

Im Allgemeinen ziehe ich es vor, dass die niedrigeren Codeebenen davon ausgehen, dass die Voraussetzungen erfüllt sind, und dass sie die Arbeit ausführen, die sie ausführen sollen, wobei die Überprüfungen weiter oben im Aufrufstapel ausgeführt werden. Dies hat den zusätzlichen Vorteil, dass Zyklen gespart werden, indem keine redundanten Überprüfungen durchgeführt werden. Es bedeutet jedoch auch, dass Sie netten Code wie den folgenden schreiben können:

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

Anstatt von:

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;
}

Und das wird natürlich noch schlimmer, wenn Sie Single-Exit erzwingen, Speicherplatz freigeben müssen usw.

Es hängt alles vom Kontext ab:

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();
    }

}