Does this method violate SOLID or has code smell?

Question

Does this give any code smell or violate SOLID principles?

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

As you can see, my Summarize() is tied to implementation classes such as Human, Animal, etc.

Does it smell? Am I violating LSP? Any other SOLID principles?

Answer 1

I smell a little something...

If your classes all implement IDisplayable, they should each implement their own logic to display themselves. That way your loop would change to something much cleaner:

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
}

Then you can change your loop to something much cleaner (and allow the classes to implement their own display logic):

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

Answer 2

seems like IDisplayable should have a method for the display name so you can reduce that method to something like

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

Answer 3

How about:

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

Answer 4

Yes.

Why not have each class implement a method from IDisplayable that shows their type:

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

Then just call your method as follows:

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

Answer 5

At a minimum it violates LSP and Open-Closed Principle.

The solution is to have a Description property to the IDisplayable interface, so that summarize can just call

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

This could also be solved via reflection, since you're only getting the name of the class.

The better solution is to return something like an IDisplayableInfo from the GetInfo() method. This would be an extensibility point to help preserve OCP.

Answer 6

If you can't modify IDisplayable or the class implementations and you're using .NET 3.5 or later you can use Extension methods. But that's not that much better

Answer 7

Given the comment on this answer from the OP, I think the best approach would be to create a custom container class to replace IList<IDisplayable> displayableItems which has methods like containsHumans() and containsAnimals() so you can encapsulate the icky non-polymorphic code in one place and keep the logic in your Summarize() function clean.

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

Of course my example is overly simple and contrived. For instance, either as part of the logic in your Summarize() if/else statements, or perhaps surrounding the entire block, you'll want to iterate over the displayableItems collection. Also, you'll likely get better performance if you override Add() and Remove() in MyCollection and have them check the type of the object and set a flag, so your containsHumans() function (and others) can simply return the state of the flag and not have to iterate the collection every time they're called.