Refactoring per loop (aspetto codice pulito)

Immagina questa funzione:

function updateOrCreateAll(array) {
  for (var i = 0; i < array.length; i++) {
    var element = array[i];
    var id = element.getId();
    if (id < 0) {
      create(element);
    }
    else {
      update(element);
    }
  }
}

Ora questa funzione ha 2 responsabilità, la prima è quella di scorrere l'array, la seconda è scegliere se creare o aggiornare. Rif. Questa funzione per avere solo 1 responsabilità più:

function updateOrCreateAll(array) {
  for (var i = 0; i < array.length; i++) {
    updateOrCreate(array[i]);
  }
}

function updateOrCreate(element) {
  return element.getId() < 0 ? create(element) : update(element);
}

Ora updateOrCreateAll ha una responsabilità che consiste nell'applicare updateOrCreate a tutti gli elementi dell'array. Il condizionale è responsabilità di updateOrCreate e create / update ha anche una sola responsabilità.

Ovviamente non è ottimale dal punto di vista delle prestazioni. Pochissime soluzioni sono ottimali e, anche se lo fossero, sarebbe difficile dimostrarlo.

Ma il codice pulito non riguarda le valutazioni ottimali su una singola dimensione. Si tratta sempre di un trade-off : per esempio, quanto tempo posso permettermi di spendere per eseguire questo codice rispetto a quanto tempo ci vorrà per scrivere vs. (questo è il bigge!) Quanto tempo ci vorrà per capire il codice se dovesse mai cambiare?

È molto improbabile che il guadagno minimo derivante dall'eliminazione di una seconda iterazione su un set di dati superi il costo di dover occuparsi di due cose diverse nello stesso momento quando si ha a che fare con questo codice (ei guadagni di prestazioni dovrebbero sempre essere misurati, mai scontato!). La cosa giusta da fare è separare le responsabilità. Se risulta che hai bisogno di una maggiore velocità puoi facilmente refactoring per la velocità in un unico luogo, ma non è possibile annullare facilmente il risultato di ottimizzazione prematura.

Poiché le funzioni che stai chiamando stanno facendo qualcosa con element , forse puoi spostare le funzioni nell'elemento?

Quindi puoi semplificare il codice rimuovendo l'istruzione if e chiamando semplicemente element->doSomething()

Ci sono molte cose che possono essere fatte con questo. Fonte: link

Probabilmente potresti prima dividere il tuo array in base ad alcune condizioni, quindi hai due array. Quindi esegui l'iterazione di ciascuno di essi e non è necessario alcun if , poiché la condizione è già valida in ciascuno di essi.

In PHP potrebbe andare così:

function split(array $a, Closure $c)
{
    $arrayWhereConditionHoldsTrue = [];
    $arrayWhereConditionDoesNotHoldTrue = [];

    foreach ($a as $e) {
        if ($c($e)) {
            $arrayWhereConditionHoldsTrue[] = $e;
        } else {
            $arrayWhereConditionDoesNotHoldTrue[] = $e;
        }
    }

    return [$arrayWhereConditionHoldsTrue, $arrayWhereConditionDoesNotHoldTrue];
}

$isPositive =
    function ($n) {
        return $n > 0;
    };

$arrays =
    split(
        [1, -2, 3, -4, 5, -6],
        $isPositive
    )
;

$positive = $arrays[0];
$negative = $arrays[1];

print_r($positive);
print_r($negative);

function doStuffWithPositiveNumbers(array $positive)
{

}

function doStuffWithNegativeNumbers(array $negative)
{

}

Ma se stiamo parlando di un oggetto, sono con Robert Andrzejuk. È possibile introdurre un'interfaccia conforme a tutti gli elementi dell'array e utilizzare il suo metodo polimorfico:

interface Number
{
    public function doSmth();
}

class Positive implements Number
{
    public function doSmth()
    {
    }
}

class Negative implements Number
{
    public function doSmth()
    {
    }
}

function doStuff(array $numbers) {
    foreach ($numbers as $number) {
        $number->doSmth();
    }
}

Basta rimuovere il condizionale con polimorfismo e loop una volta.

class DoThis : Base 
{
     public void Process() {} // DoThis logic 
}

class DoThat : Base 
{
     public void Process() {} // DoThat logic 
}


void ProcessItems(List<Base> items) 
{
     foreach(var i in items)
     {
          i.Process();
     } 
}

Il tuo codice è più pulito e altrettanto efficace

Personalmente mi piace il primo, scrollata di spalle, che fa semplicemente un semplice ramo su id all'interno del ciclo. Il polimorfismo è di solito l'alternativa diretta alla ramificazione condizionale con if/switch , ma può essere o meno ingombrante se gli ID degli elementi sono dinamici e cambiano.

Solo per essere un po 'bizzarro e dato che questa domanda sembra essere indifferente alla lingua, eccoti:

// Applies 'true_func' to elements in the the random-access sequence
// that satisfies 'pred' or 'false_func' otherwise.
applyIf(array, pred, true_func, false_func)
{
    for (int i=0; i < array.length; i++) {
        if (pred(array[i]))
            true_func(array[i]);
        else
            false_func(array[i]);
    }
}

// Returns true if the element has a negative ID.
hasNegativeId(element) {
    return element.getId() < 0;
}

E nel chiamante:

applyIf(array, hasNegativeId, doThisStuff, DoOtherStuff);

O semplicemente:

applyIf(array, function(element) {element.getId() < 0;}, doThisStuff, DoOtherStuff);

In realtà non mi piace questa soluzione. Potrei anche odiarlo. Ma mette la tua semplice funzione in due ancora più semplici: un semplice predicato uner sobrio che ritorna vero se un elemento ha un ID negativo e un algoritmo molto generalmente applicabile ( applyIf ) che chiama una funzione o l'altra su ciascuna elemento in una sequenza ad accesso casuale (di cui gli array sono un solo tipo) basato su un predicato.

Ovviamente il cliente deve ancora comporre questa roba insieme, ma le singole funzioni rendono tutto ancora più semplice ora. Puoi renderlo ancora più generalizzato in base a un iteratore / enumerazione diretto anziché a una sequenza di accesso casuale, consentendo a applyIf di funzionare su qualsiasi struttura dati che sia forwarder iterabile, inclusi elenchi collegati singolarmente, elenchi con collegamento doppio, binari cerca alberi, matrici, hash table, try, quadtrees, lo chiami (purché possa essere iterato in modo sequenziale).

Sta giocando al genere di generosità uber e di riutilizzo del codice in quanto la funzione applyIf può essere fatta per funzionare praticamente con qualsiasi combinazione di predicati e funzioni da applicare, quindi ha molto, molto più riusabilità di un funzione hardcoded per il controllo degli ID (il tipo di cosa che potrebbe entrare in una libreria standard). Ma non mi piace ancora e preferisco la tua semplice prima soluzione. In questi giorni non mi interessa tanto di queste soluzioni super flessibili. Mi piace quel semplice vecchio ciclo con if dentro, a meno che tu non debba controllare in modo ridondante gli ID negativi dappertutto (quindi considererei qualcos'altro). Potremmo generalizzare ulteriormente:

// Calls 'true_func' if pred(element) is true,
// 'false_func' otherwise.
iif(pred, true_func, false_func) {
    return function(element) {
         return pred(element) ? true_func(element): false_func(element);
    }
}

// Calls 'func' for each element in the container.
forEach(container, func) {
    for (it = container.begin(); it != container.end(); ++it)
        func(it.element())
}

A questo punto possiamo fare questo:

forEach(array, iif(hasNegativeId, doThisStuff, doOtherStuff));

Ci sono modi per andare oltre e mettere a punto le cose e generalizzarle ancora di più, ma mi fermo qui. Ancora non mi piace molto questo. Mettiamola in giro per le persone che si divertono a decifrare il loro codice nelle funzioni più giovani e riusabili (qualcosa che mi piaceva, ma non così tanto in questi giorni).

Il motivo principale per cui non mi piace questa soluzione è che la programmazione funzionale perde così tanto della sua eleganza se la usi per causare effetti collaterali. Se doThisStuff e doOtherStuff causano effetti collaterali, che presumo che facciano poiché non si utilizza un valore di ritorno, si ottiene solo la difficoltà associata alla programmazione funzionale del flusso di controllo complesso combinato con gli effetti collaterali. La combinazione di questi due può rendere le cose davvero difficili da ragionare. Quindi spesso hai bisogno di un semplice flusso di controllo se vuoi causare effetti collaterali, o nessun effetto collaterale se vuoi un flusso di controllo complesso (non una combinazione dei due). Ecco perché mi piace il tuo ciclo semplice con un'istruzione if all'interno del migliore. È facile eseguire il debug e seguire in termini di controllo del flusso.

Non hai specificato la lingua, ma se la tua supporta i delegati multicast puoi sfruttarli per elaborare l'array "tutto in una volta" e ottenere comunque una buona separazione del codice.

Qualche codice psuedo:

public class ListenerA
{
    public void Register(Foo foo)
    {
        foo.ProcessRequest += Process
    }

    public void Process(ProcessRequestData data)
    {
        if (data.getId < 0)
        {
            doStuff();
        }
    }
}

public class ListenerB
{
    public void Register(Foo foo)
    {
        foo.ProcessRequest += Process
    }

    public void Process(ProcessRequestData data)
    {
        if (data.getId >= 0)
        {
            doStuff();
        }
    }
}

public class Foo
{
    public Event ProcessRequest;
    public void Process()
    {
        for(int i=0; i<array.length; i++) 
        {
            if (null != ProcessRequest)
            {
                ProcessRequest(element);
            }
    }
}

// in some controller like class

listenerA.Register(foo);
listenerB.Register(foo);

foo.Process();

La bellezza di questo approccio è che è completamente chiuso alle modifiche. Se in futuro hai bisogno di aggiungere un terzo processore, devi crearne uno nuovo e registrarlo.

Hai appena cambiato da 11 righe di codice a 18 righe di codice. Inoltre, ovunque tu abbia avuto una chiamata a una funzione, ora ne occorrono due. Congratulazioni. Ma non stai andando abbastanza lontano. Guarda la risposta di Zapadio. Ora ti chiedo: quale versione preferirei se avessi mai la (mis) fortuna di mantenere questo codice, le 11 linee, le 18 linee o le 250 linee di Zapadio (stima approssimativa)?

La tua funzione ha una responsabilità una : per iterare attraverso tutti gli elementi dell'array, e chiamare l'appropriata di due ulteriori funzioni per ogni elemento. La tua funzione fa davvero una cosa.

Ora se giri 11 linee in 250, cosa farai se hai in realtà un problema non banale? Il principio della "responsabilità unica" è probabilmente uno dei principi più fraintesi. Quando noleggi un taxi, cosa penseresti se ci fossero una dozzina di piloti, uno che maneggia il kludge, uno gli ingranaggi, uno dei freni, uno il pedale del gas, uno dei tergicristalli del parabrezza, uno l'indicatore sinistro, uno l'indicatore giusto, uno apre la porta, uno mette i bagagli nello stivale? Il tassista ONE ha una sola responsabilità: portare persone e bagagli in modo sicuro ed efficiente verso la loro destinazione.

PS. Supponiamo ora che sia necessario eseguire tutte queste chiamate di funzione nell'ordine corretto. Quale non è insolito, molte cose sono dipendenti dall'ordine. Ora sei bloccato.