¿Este método viola SOLID o tiene código de olor?

Question

¿Da esto algún código que huela o viole los principios SÓLIDOS?

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(); 
} 

Como se puede ver, mi Resumir() está ligado a las clases de implementación tales como humano, animal, etc.

huele? ¿Estoy violando LSP? ¿Algún otro principio SÓLIDO?

Answer 1

Dado el comentario sobre this answer de la OP, creo que el mejor enfoque sería crear una clase de contenedor personalizado para reemplazar IList<IDisplayable> displayableItems que cuenta con métodos como containsHumans() y containsAnimals() para que pueda encapsular el no-repulsivo código polimórfico en un solo lugar y mantener la lógica en su función Summarize() limpia.

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(); 
} 

Por supuesto, mi ejemplo es demasiado simple y artificial.Por ejemplo, como parte de la lógica en sus instrucciones Summarize() if/else, o quizás rodeando todo el bloque, querrá iterar sobre la colección displayableItems. Además, es probable que obtenga un mejor rendimiento si anula Add() y Remove() en MyCollection y pídales que comprueben el tipo del objeto y establezcan un indicador, para que su función containsHumans() (y otras) simplemente devuelva el estado de la bandera y no tenga para iterar la colección cada vez que son llamados.

Answer 2

huelo un poco de algo ...

Si sus clases implementan todas IDisplayable, cada uno debe poner en práctica su propia lógica para mostrar a sí mismos. De esa manera el bucle cambiaría a algo mucho más limpio:

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 
} 

A continuación, puede cambiar su bucle a algo mucho más limpio (y permitir que las clases que implementan su propia lógica de visualización):

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

Answer 3

parece IDisplayable debe tener un método para el nombre para mostrar lo que puede reducir ese método para algo así como

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

Answer 4

¿Qué tal:

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

Answer 5

Sí.

Por qué no tener cada clase implementar un método de IDisplayable que muestra su 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) 
    } 
} 

A continuación, sólo llame a su método de la siguiente manera:

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

Answer 6

Como mínimo se viola LSP y Abierta Principio cerrado.

La solución es tener una propiedad Description a la interfaz IDisplayable, por lo que resumen simplemente puede llamar

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

Esto también podría ser resuelto a través de la reflexión, ya que sólo está recibiendo el nombre de la clase.

La mejor solución es devolver algo así como IDisplayableInfo del método GetInfo(). Este sería un punto de extensibilidad para ayudar a preservar OCP.

Answer 7

Si no puede modificar las implementaciones IDisplayable o de clase y está utilizando .NET 3.5 o posterior, puede utilizar los métodos de extensión. Pero eso no es mucho mejor