Come refactoring questo snippet di codice legacy per renderlo estensibile? [chiuso]

Personalmente non ritengo che il codice sia così cattivo.

La prima cosa che considero è che è abbastanza fattibile scrivere test automatici per questo. Dovresti assolutamente farlo prima di iniziare qualsiasi refactoring. Avere dei test ti consentirebbe di sperimentare la rimozione del flag saved . O puoi capire che non è così male come sembra. Considero i seguenti problemi più urgenti della bandiera.

La prossima cosa che è stata piuttosto confusa al primo avviso è il metodo addRow . Dovresti almeno fare in modo che il parametro mapping sia il primo. Se fosse C #, avrebbe senso farlo diventare un metodo di estensione. In caso di Java che crea la tua struttura, che incapsula Map e espone il metodo addRow sarebbe l'opzione migliore.

E il tuo problema con l'estensibilità intorno a repository può essere risolto semplicemente. Basta creare repository un'interfaccia. Quindi, puoi utilizzare Pattern composito per implementare diversi modi in cui le righe sono "memorizzate" e Pattern di decoro per le statistiche. Renderà anche più facile testare, in quanto può essere deriso / falso.

Ultima nota su OCP. OCP dovrebbe essere applicato strategicamente. Rendere il codice aperto contro i cambiamenti rende più complesso. Dovresti stare attento ad aggiungere complessità laddove non è necessario. Nel tuo caso, hai identificato un'area in cui rendere più aperto il tuo codice renderà le cose più semplici, senza aggiungere molta complessità. Ma dovresti fare attenzione a creare nuove astrazioni che ritieni siano necessarie.

Prima di preoccuparti dell'estensibilità, quello che farei è mettere questa cosa sotto test unitario per essere sicuro di non apportare modifiche involontarie e rielaborarlo fino a quando freccia anti modello scompare.

public void process(Data row) {

    Value value1 = row.getValue1();
    Value value2 = row.getValue2();  

    Boolean relevant1 = ( (value1 != null) && relevantValues.contains(value1) );
    Boolean relevant2 = ( relevantValues.contains(value2) );

    if (relevant1) {
        addRow(value1, row, only1collection);
    }

    if (relevant1 && relevant2) {
        addRow(value2, row, values12collection); 
    }   

    if (relevant2) {
        addRow(value2, row, only2collection); 
    }

    if (relevant1 || relevant2) {
        store.update(row);
    }
}

Questo è un po 'più leggibile. Dal momento che il codice originale, fortunatamente, non aveva nessun accoppiamento temporale tra le sue linee, potevano essere avvolti in qualunque i condizionali erano per loro e riorganizzati a piacimento. Ecco la storia dei semplici cambiamenti che mi hanno portato qui:

1. Inserisci ogni addRow() nella sua struttura
2. Comprimi la logica booleana multilinea e raggruppa sezioni simili insieme
3. Trasforma il bool "salvato" in un "else if"
4. Trasforma "else if" in un OR
5. Semplifica le condizioni con le boolean vars

Ora potresti star guardando questo e chiedendoti "che cos'è il negozio?" È quello che vorresti che fosse.

public interface Store {
    public void update(Row row);
}

public StoreDefault implements Store {
    public void update(Row row);
        repository.save(row);
        stats.increment();
    }
}

public StoreReport implements Store {
    public void update(Row row);
        repository.save(row);
        stats.increment();
        email.send(row);
    }
}

Ho usato Il modello di strategia ma ci sono molti altri modelli (e non pattern) che potrebbero funzionare come bene. Poiché le modifiche al modo in cui process() può essere conclusa creando nuovi tipi di store , rispetta esattamente OCP . L'importante è che process() non debba sapere cosa è stato usato. Quel non-sapere è ciò che fa funzionare il polimorfismo. E il polimorfismo è il cuore e l'anima della programmazione orientata agli oggetti.

Per me sembra che repository e stats debbano vivere in un oggetto diverso:

class RepositoryStatistics{
  private final Repository repository;
  private final Statistics statistics;
  private final Collection<Row.ID> savedRows = new HashSet<>(); // no duplicates;
  RepositoryStatistics(Repository repository, Statistics statistics){
     this.repository = repository;
     this.statistics = statistics;
  }

  public void save(Row row){ 
      if(!savedRows.contains(row.getId()){
        repository.save(row);
        stats.increment();
      }
  }
}

Ma questo non risolve il problema dell'estensione .

Vorrei introdurre un'interfaccia funzionale :

class RepositoryStatistics{
  @FunctionalInterface
  interface RepositorySaveAction{
     void afterSaveOf(Row row);
  }
  private final List<RepositorySaveAction> repositorySaveActions; // keep order..
  RepositoryStatistics(Repository repository,RepositorySaveAction ...repositorySaveActions) {
     this.repository = repository;
     this.repositorySaveActions = Arrays.asList(repositorySaveActions);
  }
  public void save(Row row){ 
      if(!savedRows.contains(row.getId()){
        repository.save(row);
        for(RepositorySaveAction repositorySaveAction:repositorySaveActions)
            repositorySaveAction.afterSaveOf(row);
      }
  }
}

E stats.increment() sarebbe la mia prima implementazione:

// somehere in your code
RepositoryStatistics repo = 
   new RepositoryStatistics(realRepository,
      row->stats.increment(),
      row->mailer.send(row));

Il% nidificato di co_de può essere trasformato come segue. C'è un nome per questo stile di codifica: esecuzione prevista , o previsione in breve. Vedrai letteralmente il codice di basso livello scritto con questo stile.

Il nuovo codice calcola un sacco di bool "chiaroveggenti". Questi bool predispongono (dettano) il resto del flusso di esecuzione, prima di eseguire qualsiasi comando con effetti collaterali.

I nomi bool sono super-verbosi solo per la visualizzazione web. Scegli i nomi più brevi in base alla tua convenzione. Esempio:

• sostituisci "isValueOneNotNull" con "nn1",
• sostituisci "isValueOneRelevant" con "r1",
• sostituisci "needAddToBothOneTwoCollection" con "c12"

Si presume che "relevantValues.contains (*)" sia privo di effetti collaterali. Se ha effetti collaterali, non dovresti refactoring del codice.

Nota che il codice qui sotto non è esattamente uguale al tuo - Ho modificato leggermente il comportamento a causa di quelli che sospetto siano bug (per mancanza di simmetria). Se sai che il tuo codice è corretto, probabilmente il mio codice è leggermente sbagliato.

// existing code

Value value1 = row.getValue1();
Value value2 = row.getValue2();

// new code

final bool isValueOneNotNull = 
    value1 != null;

final bool isValueTwoNotNull = 
    value2 != null;             // your code doesn't check this

final bool isValueOneRelevant = 
    isValueOneNotNull && relevantValues.contains(value1);

final bool isValueTwoRelevant = 
    isValueTwoNotNull && relevantValues.contains(value2);

final bool needAddToOnlyOneCollection = 
    isValueOneRelevant && !isValueTwoRelevant;

final bool needAddToOnlyTwoCollection = 
    !isValueOneRelevant && isValueTwoRelevant;

final bool needAddToBothOneTwoCollection = 
    isValueOneRelevant && isValueTwoRelevant;

// There are two ways to compute the next bool.

final bool needSaveTheMoreComplicatedWay = 
    needAddToOnlyOneCollection ||
    needAddToOnlyTwoCollection ||
    needAddToBothOneTwoCollection;

// The simplified way. Should be logically equivalent.

final bool needSave = 
    isValueOneRelevant || isValueTwoRelevant; 

// The next few bools are optional - to illustrate
// how you can customize and add additional logic.

final bool needIncrementStats = needSave && statisticianHasBeenHired;
final bool needSendEMail = needSave && internetIsNotDown;

// actual "execution"

if (needAddToOnlyOneCollection) { 
    addRow(value1, row, only1collection);
}
if (needAddToOnlyTwoCollection) { 
    addRow(value2, row, only2collection);
}
if (needAddToBothOneTwoCollection) { 
    addRow(value2, row, values12collection);
}

bool saveSucceeded = false;
if (needSave) {
    repository.save(row);
    // if the next line is reached, I suppose no exception was thrown.
    saveSucceeded = true;
}

if (needIncrementStats) {
    stats.increment();
}

if (needSendEMail) {
    sendEmail(row);
}