Le frasi dovrebbero essere nel metodo interno o esterno?

Non penso che tu possa avere una regola generale per questo genere di cose, dipende dalla situazione.

In questo caso, suggerirei di avere le clausole if al di fuori dei metodi perché i nomi dei metodi Draw implicano che disegnano semplicemente le cose senza condizioni speciali.

Se trovi che devi eseguire i controlli prima di chiamare i metodi in molti punti, allora potresti voler inserire il controllo nei metodi e rinominarli per chiarire che ciò sta accadendo

Dico il secondo.

I metodi dovrebbero avere lo stesso livello di astrazione.

Nel secondo esempio, la responsabilità del metodo Draw() è di comportarsi come un controller, chiamando ciascuno dei singoli metodi di disegno. Tutto il codice in questo metodo Draw() è allo stesso livello di astrazione. Questa linea guida entra in gioco in scenari di riutilizzo. Ad esempio, se si è tentati di riutilizzare il metodo DrawPoints() in un altro metodo pubblico (chiamiamolo Sketch() ), non è necessario ripetere la clausola di guardia (la dichiarazione if che decide se disegnare i punti).

Nel primo esempio, tuttavia, il metodo Draw() è responsabile per determinare se chiamare ciascuno dei singoli metodi e quindi chiamare tali metodi. Draw() ha alcuni metodi di basso livello che funzionano, ma delega altri lavori di basso livello ad altri metodi, e quindi Draw() ha codice a diversi livelli di astrazione. Per riutilizzare DrawPoints() in Sketch() , dovresti replicare anche la clausola di guardia in Sketch() .

Questa idea è discussa nel libro "Codice pulito" di Robert C. Martin, che consiglio vivamente.

La mia opinione sull'argomento è piuttosto controversa, ma sopporta me, poiché credo che tutti siano d'accordo sul risultato finale. Ho solo un approccio diverso per arrivarci.

Nel mio articolo Function Hell , spiego perché non mi piacciono i metodi di divisione solo per creare metodi più piccoli. Li divido solo quando lo so , saranno riutilizzati, o naturalmente, quando posso riutilizzarli.

L'OP ha dichiarato:

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

Questo mi porta a una terza opzione (intermedia) non menzionata. Creo "blocchi di codice" o "paragrafi di codice" che altri avrebbero creato per le funzioni.

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

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

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

L'OP ha anche dichiarato:

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

... quindi questo metodo diventerà molto grande rapidamente. Quasi tutti sono d'accordo che questo diminuisce la leggibilità. A mio parere, la soluzione corretta non è solo la suddivisione in diversi metodi, ma la suddivisione del codice in classi riutilizzabili. La mia soluzione sarebbe probabilmente simile a questa.

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

Gli argomenti di Ofcourse dovrebbero ancora essere passati e tali.

Per i casi in cui stai controllando un campo che controlla se disegnare o meno, è logico che sia all'interno di Draw() . Quando questa decisione diventa più complicata, tendo a preferire quest'ultima (o dividerla). Se hai bisogno di più flessibilità, puoi sempre espanderlo.

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

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

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

Si noti che i metodi aggiuntivi sono protetti e consentono alle sottoclassi di estendere ciò di cui hanno bisogno. Ciò ti consente anche di forzare il disegno per i test o qualsiasi altra ragione tu possa avere.

Di quelle due alternative, preferisco la prima versione. La mia ragione è che voglio un metodo per fare ciò che implica il nome, senza dipendenze nascoste. DrawLegend dovrebbe disegnare una legenda, non forse disegnare una legenda.

Tuttavia, la risposta di Steven Jeuris è migliore delle due versioni della domanda.

Invece di avere singole proprietà Show___ per ogni parte, probabilmente definirei un campo di bit. Ciò semplificherebbe un po 'per:

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

A parte questo, vorrei inclinarmi a controllare la visibilità nel metodo esterno, non nell'interno. Dopotutto, è DrawLegend e non DrawLegendIfShown . Ma dipende dal resto della classe. Se ci sono altri posti che chiamano quel metodo DrawLegend e devono controllare anche ShowLegend , probabilmente sposterei il controllo in DrawLegend .

Vorrei andare con la prima opzione - con il "se" al di fuori del metodo. Descrive meglio la logica seguita, inoltre ti dà la possibilità di disegnare effettivamente un asse, ad esempio, nei casi in cui vuoi disegnarne uno indipendentemente da un'impostazione. Inoltre rimuove il sovraccarico di una chiamata di funzione aggiuntiva (presupponendo che non sia in linea), che può accumularsi se stai andando per la velocità (il tuo esempio sembra che stia semplicemente disegnando un grafico dove potrebbe non essere un fattore, ma in un'animazione o gioco potrebbe essere).

In generale, preferisco che i livelli più bassi di codice presuppongano che le precondizioni siano soddisfatte e riescano a fare tutto il lavoro che dovrebbero svolgere, con i controlli eseguiti più in alto nello stack di chiamate. Questo ha il vantaggio di salvare cicli ciclici non facendo controlli ridondanti, ma significa anche che puoi scrivere un codice piacevole come questo:

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

invece di:

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

E naturalmente questo diventa ancora più cattivo se imponi l'uscita singola, la memoria che hai bisogno di liberare, ecc.

Tutto dipende dal contesto:

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

}