Czy jeśli instrukcje powinny być wewnętrzne lub zewnętrzne?

Który z tych wzorów jest lepszy? Jakie są zalety i wady każdego z nich? Którego byś użył? Doceniamy wszelkie inne sugestie dotyczące tego, jak radzić sobie z takimi metodami.

Można założyć, że Draw () jest jedynym miejscem, z którego wywoływane są inne metody rysowania. Trzeba to rozszerzyć na wiele innych metod Draw * i Show *, a nie tylko na trzy pokazane tutaj.

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

Lub

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

Nie sądzę, że możesz mieć ogólną regułę dla takich rzeczy, to zależy od sytuacji.

W tym przypadku sugerowałbym posiadanie klauzul if poza metodami, ponieważ nazwy metod Draw sugerują, że po prostu rysują rzeczy bez żadnych specjalnych warunków.

Jeśli okaże się, że musisz wykonać kontrole przed wywołaniem metod w wielu miejscach, możesz umieścić czek w metodach i zmienić ich nazwę, aby wyjaśnić, że tak się dzieje

Mówię drugi.

Metody powinny mieć ten sam poziom abstrakcji.

W drugim przykładzie odpowiedzialnością Draw()metody jest działanie jak kontroler, wywoływanie poszczególnych metod rysowania. Cały kod w tej Draw()metodzie jest na tym samym poziomie abstrakcji. Wytyczne te wchodzą w grę w scenariuszach ponownego użycia. Na przykład, jeśli masz ochotę ponownie użyć tej DrawPoints()metody w innej publicznej metodzie (nazwijmy ją Sketch()), nie musisz powtarzać klauzuli ochronnej (instrukcja if decydująca o tym, czy narysować punkty).

Jednak w pierwszym przykładzie Draw()metoda jest odpowiedzialna za określenie, czy wywołać każdą z poszczególnych metod, a następnie wywołanie tych metod. Draw()ma pewne metody niskiego poziomu, które działają, ale deleguje inne prace niskiego poziomu do innych metod, a zatem Draw()ma kod na różnych poziomach abstrakcji. Aby ponownie użyć DrawPoints()w Sketch()tym przypadku, musisz także powtórzyć klauzulę ochronną Sketch().

Pomysł ten omówiono w książce Roberta C. Martina „Clean Code”, którą gorąco polecam.

Moja opinia na ten temat jest dość kontrowersyjna, ale proszę o wyrozumiałość, ponieważ uważam, że prawie wszyscy zgadzają się co do wyniku końcowego. Po prostu mam inne podejście do tego.

W moim artykule Function Hell wyjaśniam, dlaczego nie lubię dzielić metod tylko ze względu na tworzenie mniejszych metod. Dzielę je tylko wtedy , gdy wiem , że zostaną ponownie wykorzystane lub oczywiście, kiedy będę mógł je ponownie wykorzystać.

PO stwierdził:

Można założyć, że Draw () jest jedynym miejscem, z którego wywoływane są inne metody rysowania.

To prowadzi mnie do (pośredniej) trzeciej opcji, o której nie wspomniano. Tworzę „bloki kodu” lub „akapity kodu”, dla których inni tworzyliby funkcje.

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

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

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

PO stwierdził również:

Musi to zostać rozszerzone na wiele innych metod Draw * i Show *, a nie tylko na trzy pokazane tutaj.

... więc ta metoda szybko się powiększy. Prawie wszyscy zgadzają się, że zmniejsza to czytelność. Moim zdaniem właściwe rozwiązanie nie dzieli się tylko na kilka metod, ale dzieli kod na klasy do ponownego użycia. Moje rozwiązanie prawdopodobnie wyglądałoby mniej więcej tak.

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

Oczywiście należałoby przekazać argumenty i tak dalej.

W przypadkach, w których sprawdzasz pole, które kontroluje, czy rysować, warto mieć je wewnątrz Draw(). Kiedy ta decyzja staje się bardziej skomplikowana, wolę tę drugą (lub ją podzielić). Jeśli w końcu potrzebujesz większej elastyczności, zawsze możesz ją rozwinąć.

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

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

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

Zauważ, że dodatkowe metody są chronione, co pozwala podklasom rozszerzyć to, czego potrzebują. Pozwala to również na wymuszenie losowania w celu przetestowania lub z dowolnego innego powodu.

Spośród tych dwóch alternatyw wolę pierwszą wersję. Moim powodem jest to, że chcę metody, która sugeruje nazwę, bez żadnych ukrytych zależności. DrawLegend powinien narysować legendę, a nie być może legendę.

Odpowiedź Stevena Jeurisa jest jednak lepsza niż dwie wersje pytania.

Zamiast mieć indywidualne Show___właściwości dla każdej części, prawdopodobnie zdefiniowałbym pole bitowe. Uprościłoby to nieco:

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

Poza tym skłoniłbym się do sprawdzenia widoczności w metodzie zewnętrznej, a nie wewnętrznej. W końcu tak jest DrawLegendi nie DrawLegendIfShown. Ale to zależy od reszty klasy. Jeśli są inne miejsca, które wywołują tę DrawLegendmetodę i one również muszą sprawdzić ShowLegend, prawdopodobnie po prostu przeniosę czek do DrawLegend.

Wybrałbym pierwszą opcję - z „if” poza metodą. Lepiej opisuje stosowaną logikę, a także daje opcję faktycznego narysowania osi, na przykład w przypadkach, gdy chcesz narysować oś niezależnie od ustawienia. Ponadto usuwa narzut związany z dodatkowym wywołaniem funkcji (zakładając, że nie jest wstawiany), który może się kumulować, jeśli zmierzasz do szybkości (twój przykład wygląda tak, jakby rysował wykres, na którym może nie być czynnikiem, ale animacją lub może to być gra).

Ogólnie rzecz biorąc, wolę, aby niższe poziomy kodu zakładały, że spełnione są warunki wstępne, i przystąpiłem do wykonywania jakiejkolwiek pracy, jaką powinni wykonywać, a kontrole były wykonywane wyżej na stosie wywołań. Ma to tę dodatkową zaletę, że oszczędza cykle, ponieważ nie wykonuje zbędnych kontroli, ale oznacza również, że możesz napisać ładny kod w następujący sposób:

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

zamiast:

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

I oczywiście staje się to jeszcze bardziej nieprzyjemne, jeśli wymuszasz pojedyncze wyjście, masz pamięć, którą musisz zwolnić itp.

Wszystko zależy od kontekstu:

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

}