Soffro di abuso di incapsulamento?

12

Ho notato qualcosa nel mio codice in vari progetti che mi sembrano odore di codice e qualcosa di brutto da fare, ma non riesco a gestirlo.

Mentre provo a scrivere "codice pulito" tendo a sovra-usare i metodi privati per rendere il mio codice più facile da leggere. Il problema è che il codice è effettivamente più pulito ma è anche più difficile da testare (sì, so che posso testare metodi privati ...) e in generale mi sembra una cattiva abitudine.

Ecco un esempio di una classe che legge alcuni dati da un file .csv e restituisce un gruppo di clienti (un altro oggetto con vari campi e attributi).

public class GroupOfCustomersImporter {
    //... Call fields ....
    public GroupOfCustomersImporter(String filePath) {
        this.filePath = filePath;
        customers = new HashSet<Customer>();
        createCSVReader();
        read();
        constructTTRP_Instance();
    }

    private void createCSVReader() {
        //....
    }

    private void read() {
        //.... Reades the file and initializes the class attributes
    }

    private void readFirstLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readSecondLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readCustomerLine(String[] inputLine) { 
        //.... Method used by the read() method
    }

    private void constructGroupOfCustomers() {
        //this.groupOfCustomers = new GroupOfCustomers(**attributes of the class**);
    }

    public GroupOfCustomers getConstructedGroupOfCustomers() {
        return this.GroupOfCustomers;
    }
}

Come puoi vedere la classe ha solo un costruttore che chiama alcuni metodi privati per portare a termine il lavoro, so che è non sono una buona pratica in generale, ma preferisco incapsulare tutte le funzionalità nella classe invece di rendere pubblici i metodi, nel qual caso un client dovrebbe funzionare in questo modo:

GroupOfCustomersImporter importer = new GroupOfCustomersImporter(filepath)
importer.createCSVReader();
read();
GroupOfCustomer group = constructGoupOfCustomerInstance();

Lo preferisco perché non voglio mettere linee di codice inutili nel codice lato del client disturbando la classe client con i dettagli di implementazione.

Quindi, questa è una cattiva abitudine? Se sì, come posso evitarlo? Si prega di notare che quanto sopra è solo un semplice esempio. Immagina che la stessa situazione si verifichi in qualcosa di un po 'più complesso.

    
posta Florents Tselai 16.04.2012 - 02:27
fonte

4 risposte

17

Penso che tu sia effettivamente sulla strada giusta per quanto riguarda il voler nascondere i dettagli di implementazione dal client. Desiderate progettare la vostra classe in modo che l'interfaccia che vede il cliente sia l'API più semplice e concisa che siate in grado di creare. Non solo il cliente non sarà "infastidito" con i dettagli di implementazione, ma sarà anche libero di rifattorizzare il codice sottostante senza preoccuparsi che i chiamanti di quel codice debbano essere modificati. Ridurre l'accoppiamento tra i diversi moduli ha dei vantaggi reali e dovresti assolutamente cercare di farlo.

Quindi, se segui il consiglio che ho appena fornito, ti ritroverai con qualcosa che hai già notato nel tuo codice, tutta una serie di logiche che rimangono nascoste dietro un'interfaccia pubblica e non sono facilmente accessibili.

Idealmente, dovresti essere in grado di testare la tua classe contro la sua interfaccia pubblica e se ha dipendenze esterne, potresti dover introdurre oggetti falsi / simulati / stub per isolare il codice sottoposto a test.

Tuttavia, se lo fai e ritieni comunque che non puoi facilmente testare ogni parte della classe, è molto probabile che la tua classe stia facendo troppo. Nello spirito del principio SRP , puoi seguire ciò che Michael Feathers chiama" Sprout Class Pattern "ed estrae un blocco della tua classe originale in una nuova classe.

Nel tuo esempio hai una classe di importatore che è anche responsabile della lettura di un file di testo. Una delle opzioni è estrarre un blocco di codice di lettura file in una classe InputFileParser separata. Ora tutte quelle funzioni private diventano pubbliche e quindi facilmente testabili. Allo stesso tempo, la classe parser non è qualcosa che è visibile ai client esterni (contrassegnala come "interna", non pubblica i file di intestazione o semplicemente non la annuncia come parte della tua API) poiché continuerà a utilizzare l'originale importatore la cui interfaccia continuerà a rimanere breve e dolce.

    
risposta data 16.04.2012 - 04:02
fonte
1

Come alternativa a mettere un sacco di chiamate di metodi nel tuo costruttore di classi - che sono d'accordo è un'abitudine da evitare in generale - potresti invece creare un metodo factory per gestire tutte le cose aggiuntive di inizializzazione che non vuoi infastidire il lato client con. Ciò significherebbe che esponesti un metodo per visualizzare qualcosa come il tuo metodo constructGroupOfCustomers() come pubblico e utilizzarlo come metodo statico della tua classe:

GroupOfCustomersImporter importer = 
    new GroupOfCustomersImporter.CreateInstance();

o forse come metodo di una classe factory separata:

ImporterFactory factory = new ImporterFactory();
GroupOfCustomersImporter importer = factory.CreateGroupOfCustomersImporter();

Questi sono solo il primo paio di opzioni che mi vengono in mente.

Vale anche la pena considerare che l'istinto sta cercando di dirti qualcosa. Quando il tuo istinto ti dice che il codice sta iniziando a puzzare, probabilmente odora, ed è meglio fare qualcosa prima che suoni! Sicuramente in superficie potrebbe non esserci nulla di intrinsecamente sbagliato con il tuo codice, tuttavia un rapido ricontrollo e un refactoring ti aiuteranno a risolvere i tuoi pensieri riguardo al problema in modo da poter passare ad altre cose con sicurezza senza preoccuparti se stai costruendo un potenziale castello di carte. In questo caso particolare, delegare alcune delle responsabilità del costruttore a - o essere chiamato da - una fabbrica ti assicura di poter esercitare un maggiore grado di controllo dell'istanza della tua classe e dei suoi futuri discendenti potenziali.

    
risposta data 16.04.2012 - 04:11
fonte
1

Aggiungere la mia risposta, poiché è finita troppo a lungo per un commento.

Hai assolutamente ragione che l'incapsulamento e i test sono piuttosto contrari: se nascondi quasi tutto, è difficile da testare.

Potresti, tuttavia, rendere l'esempio più verificabile dando uno stream piuttosto che un nome file: in questo modo puoi fornire solo un csv noto dalla memoria o un file, se lo desideri. Renderà anche la classe più solida quando è necessario aggiungere il supporto http;)

Inoltre, cerca di usare lazy-init:

public class Csv
{
  private CustomerList list;

  public CustomerList get()
  {
    if(list == null)
        load();
     return list;
  }
}
    
risposta data 16.04.2012 - 07:17
fonte
1

Il costruttore non dovrebbe fare troppo eccetto l'inizializzazione di alcune variabili o lo stato dell'oggetto. Fare troppo nel costruttore può aumentare le eccezioni del runtime. Vorrei provare a evitare che il client faccia qualcosa del genere:

MyClass a;
try {
   a = new MyClass();
} catch (MyException e) {
   //do something
}

Invece di fare:

MyClass a = new MyClass(); // Or a factory method
try {
   a.doSomething();
} catch (MyException e) {
   //do something
}
    
risposta data 24.06.2015 - 16:06
fonte