Questo metodo viola SOLID o ha odore di codice?

Question

Ciò conferisce a qualsiasi codice odore o violazione dei principi SOLID?

public string Summarize() 
{ 
IList<IDisplayable> displayableItems = getAllDisplayableItems(); 
StringBuilder summary = new StringBuilder(); 

foreach(IDisplayable item in displayableItems) 
{ 
    if(item is Human) 
     summary.Append("The person is " + item.GetInfo()); 

    else if(item is Animal) 
     summary.Append("The animal is " + item.GetInfo()); 

    else if(item is Building) 
     summary.Append("The building is " + item.GetInfo()); 

    else if(item is Machine) 
     summary.Append("The machine is " + item.GetInfo()); 
} 

return summary.ToString(); 
} 

Come si può vedere, il mio Riassumere() è legata a classi di implementazione, come umano, Animale, ecc

Ha odore? Sto violando LSP? Qualche altro principio SOLID?

Answer 1

Dato il commento this answer dall'OP, penso che l'approccio migliore sarebbe quello di creare una classe contenitore personalizzato per sostituire IList<IDisplayable> displayableItems che ha metodi come containsHumans() e containsAnimals() in modo da poter incapsulare la non icky codice polimorfico in un posto e mantenere la logica nella tua funzione Summarize() pulita.

class MyCollection : List<IDisplayable> 
{ 
    public bool containsHumans() 
    { 
     foreach (IDisplayable item in this) 
     { 
      if (item is Human) 
       return true; 
     } 

     return false; 
    } 

    // likewise for containsAnimals(), etc 
} 

public string Summarize() 
{ 
    MyCollection displayableItems = getAllDisplayableItems(); 
    StringBuilder summary = new StringBuilder(); 

    if (displayableItems.containsHumans() && !displayableItems.containsAnimals()) 
    { 
     // do human-only logic here 
    } 
    else if (!displayableItems.containsHumans() && displayableItems.containsAnimals()) 
    { 
     // do animal-only logic here 
    } 
    else 
    { 
     // do logic for both here 
    } 

    return summary.ToString(); 
} 

Ovviamente il mio esempio è troppo semplice e forzato.Ad esempio, sia come parte della logica nelle tue affermazioni Summarize() se/else, o forse intorno all'intero blocco, ti consigliamo di eseguire iterazioni sulla raccolta displayableItems. Inoltre, è possibile ottenere prestazioni migliori se si sostituisce Add() e Remove() in MyCollection e fare in modo che controllino il tipo dell'oggetto e impostino un flag, quindi la funzione containsHumans() (e altre) può semplicemente restituire lo stato della bandiera e non avere per iterare la collezione ogni volta che vengono chiamati.

Answer 2

I odore un po 'qualcosa ...

Se le classi tutte implementano IDisplayable, dovrebbero ogni implementare una propria logica per visualizzare se stessi. In questo modo il ciclo cambierebbe a qualcosa di molto più pulito:

public interface IDisplayable 
{ 
    void Display(); 
    string GetInfo(); 
} 

public class Human : IDisplayable 
{ 
    public void Display() { return String.Format("The person is {0}", 
     GetInfo()); 

    // Rest of Implementation 
} 

public class Animal : IDisplayable 
{ 
    public void Display() { return String.Format("The animal is {0}", 
     GetInfo()); 

    // Rest of Implementation 
} 

public class Building : IDisplayable 
{ 
    public void Display() { return String.Format("The building is {0}", 
     GetInfo()); 

    // Rest of Implementation 
} 

public class Machine : IDisplayable 
{ 
    public void Display() { return String.Format("The machine is {0}", 
     GetInfo()); 

    // Rest of Implementation 
} 

Quindi è possibile modificare il ciclo per qualcosa di molto più pulito (e permettere alle classi di implementare la propria logica di visualizzazione):

foreach(IDisplayable item in displayableItems) 
    summary.Append(item.Display()); 

Answer 3

sembra IDisplayable dovrebbe avere un metodo per il nome visualizzato in modo da poter ridurre tale metodo per qualcosa come

summary.Append("The " + item.displayName() + " is " + item.getInfo()); 

Answer 4

ne dite:

summary.Append("The " + item.getType() + " is " + item.GetInfo()); 

Answer 5

Sì.

Perché non avere ogni classe implementare un metodo da IDisplayable che mostra il loro tipo:

interface IDisplayable 
{ 
    void GetInfo(); 
    public string Info; 
} 
class Human : IDisplayable 
{ 
    public string Info 
    { 
    get 
    { 
     return "";//your info here 
    } 
    set; 
    } 

    public void GetInfo() 
    { 
     Console.WriteLine("The person is " + Info) 
    } 
} 

Poi basta chiamare il metodo come segue:

foreach(IDisplayable item in displayableItems) 
{ 
    Console.WriteLine(item.GetInfo()); 
} 

Answer 6

Come minimo viola LSP e Open- Principio chiuso.

La soluzione è quella di avere una proprietà Description all'interfaccia IDisplayable, in modo che riassumono può semplicemente chiamare

summary.Append(string.Format("The {0} is {1}", item.Description, item.GetInfo())); 

Questo potrebbe anche essere risolto attraverso la riflessione, dal momento che si sta solo ricevendo il nome della classe.

La soluzione migliore è restituire qualcosa come un IDisplayableInfo dal metodo GetInfo(). Questo sarebbe un punto di estensibilità per aiutare a preservare l'OCP.

Answer 7

Se non è possibile modificare IDisplayable o le implementazioni di classe e si utilizza .NET 3.5 o versioni successive, è possibile utilizzare i metodi di estensione. Ma non è che molto meglio