Scambio tra codice pulito, codice duplicato ed efficienza del codice in java

Penso che stai prendendo il consiglio "fai una cosa" troppo alla lettera. A mio avviso, questo metodo fa solo una cosa: analizza l'input in un formato di testo in una struttura dati interna. A mio parere, la struttura dei dati in due parti (i dati validi e l'elenco di articoli non validi) è irrilevante. Il processo di analisi è una singola preoccupazione atomica.

Le cose potrebbero essere più chiare se raccogli la mappa risultante di elementi validi e l'elenco di quelli non validi in una classe in modo che tu possa tornare entrambi come oggetto. I parametri di output sono un odore di progettazione; i risultati di una funzione dovrebbero essere nel suo valore di ritorno, ove possibile.

Per i miei gusti stai facendo troppo con questo metodo. Si crea una matrice di un parametro e poi si itera su di esso. Ogni elemento è analizzato in qualche modo. Addizionalmente ogni elemento viene raccolto in uno dei due contenitori (mapOfAttributes e invalidLines).

Imho è ok di avere un metodo "createMapOfAttributes" che orchestra queste diverse operazioni, ma è molto più facile capire se si refactoring gli interni del metodo in diversi più piccoli.

Quindi avrei il ciclo for nel metodo esterno (così com'è), e l'analisi e la raccolta di elementi in metodi separati.

When I try to separate those two things I would come up with the following: one method returns the mapOfFeatures and a second method returns the invalidLines map.

Si

In the [this] second method I would need to somehow test for each entry if it’s a dublicate entry, possibly by adding it again to a map and thereby doing the same as in the first method (leading to dublicate code and dublicate computational burden). Furthermore, I would need to have the code to extract family and feature in both methods, also leading to dublicate code.

puoi eseguire l'estrazione in un metodo separato, in modo da non avere il codice duplicato per questo. Inoltre, non devi necessariamente reinserirlo in una mappa per il rilevamento dei duplicati.

Inoltre, in generale, dovresti preferire quasi sempre un design pulito e leggibilità su una soluzione super ottimizzata. Fai questo solo se hai davvero bisogno di (questo codice viene eseguito con mappe veramente grandi e molte volte in un'operazione timecritica?).

Altri problemi:

• C'è una variabile non dichiarata: mapOfFeatures.
• Non sono sicuro della linea con invalidLines.put(Id, family) . Questo mi sembra un bug.
• invalidLines è un parametro di output che considero almeno in questo esempio una progettazione errata. Prova a cercare una soluzione diversa.

Supponendo che Readability sia l'obiettivo principale, sei assolutamente corretto per refactoring questo metodo.

Ci sono poche cose che cambierei dal pipistrello.

Il commento! - Ogni volta che hai un commento come estrai famiglia e attributo , è tempo di refactoring. Giustamente l'hai già scoperto, quindi ora è solo questione di far funzionare l'implementazione.

Hai detto che hai già una soluzione, ma non ti piace a causa dell'efficienza del codice e della pubblicità del codice , e in parte sei corretto. dublicity del codice è considerato cattivo come codice illeggibile, introdurrà bug e diminuirà la manutenibilità. efficienza del codice tuttavia è molto più specifico del dominio, ed è davvero difficile dirti cosa fare, ma nella mia esperienza, nulla di simile non è degno di nota.

Quindi - per rispondere alla tua domanda su come codice leggibile, efficienza del codice e dublicità del codice tutto si riferisce - Elimina la duplicità e rendila leggibile . Solitamente la leggibilità non introduce la duplicazione poiché si rifatta tutta la logica in metodi più piccoli che sono più facili da riutilizzare, quindi, se la vedi, potrebbe essere perché non hai eseguito il refactoring.

Il tuo esempio

private static FamilyAttribute createMapOfAttributes(
        final String Id,
        final String attributes
)
{
    final String[] arrayOfAttributes = attributes.split(";");
    final int numberOfAttributes = arrayOfAttributes.length;
    final FamilyAttribute familyAttribute = new FamilyAttribute();

    for(int i = 0; i < numberOfAttributes; i++)
    {
        final String attributeEntry = arrayOfAttributes[i];
        if(isEmpty(attributeEntry)){ //Note
            continue;
        }
        //extract family and attribute - remove this comment!
        final String family = getFamily(familyAttribute, attributeEntry); //Note
        if(isFamilyInvalid()){ //Note
            familyAttribute.addInvalidLine(Id, family);
        }
    }
    return familyAttribute;
}

E FamilyAttribute (potrebbe volere qualche altro nome) :

//Your class could look something like this.
public class FamilyAttribute{
    //your private maps here

    public void addAttribute(String name, string value){
        //add to map
    }

    public void addInvalidLine(String name, String value){
        //add to invalid lines map
    }

    public Map<String, String> getAttributes(){
        //return map of attributes
    }

    public Map<String, String> getInvalidLines(){
        //return map of invalid lines
    }
}

Questo è solo un inizio e si può fare di più, ma spero che i metodi che ho chiamato siano abbastanza intuitivi per farti sapere cosa metterli dentro. Questo ti permette di scrivere più inglese e lo rende più leggibile - clean code

Nota a margine

Ho l'idea che pensi che "una funzione deve fare solo una cosa" significhi che un cliente di questa classe ha bisogno di chiamare un metodo per ogni cosa che vuole fare. Questo non è completamente vero , un'API pulita richiede metodi pubblici per eseguire diverse operazioni come get data - > errore controlla i dati - > manipola i dati - > restituire i dati . Questo è assolutamente soddisfacente, ma quando si tratta di clean code la magia sta nella delega dei metodi. Il client deve solo chiamare un solo metodo per ottenere i dati, ma quel metodo chiama molti altri metodi privati che ognuno fa quella cosa.