4
\$\begingroup\$

I was given the exercise of refactoring the following code, thus ensuring it was following the SOLID design principles. It refers to an Animal Park, where there are a couple of different species of animals, and it defines the rules in which these animals can eat other animals. The Animal Park class processes a polymorphic array of the represented animals.

    public abstract class Animal
    {
        public Size Size { get; set; }
        public bool IsDead { get; set; }

        public Animal(Size size)
        {
            this.Size = size;
            this.IsDead = false;
        }

        public abstract void Eat(Animal obj);
    }

    public enum Size
    {
        Small,
        Medium,
        Big
    };

    public class Lion : Animal
    {
        public Lion(Size size) : base(size)
        {
        }

        public override void Eat(Animal animal)
        {
            if (animal is Lion && this.Size > animal.Size)
            {
                animal.IsDead = true;
            }

            if (animal is Hyena && this.Size >= animal.Size)
            {
                animal.IsDead = true;
            }
        }
    }

    public class Hyena : Animal
    {
        public Hyena(Size size) : base(size)
        {
        }

        public override void Eat(Animal animal)
        {
            if (animal is Hyena && this.Size > animal.Size)
            {
                animal.IsDead = true;
            }
        }
    }

    public static class AnimalPark
    {
        public static void Process(IList<Animal> animals)
        {
            int length = animals.Count;
            for (int i = 1; i < length; i++)
            {
                animals[i - 1].Eat(animals[i]);
            }
        }
    }

The code above breaks the Open/Closed Principle because every time I would need to add a new animal, I would not be able to do it just by extending the existing classes or by adding new ones: I would have to modify the existing classes. But I am struggling a lot to fix it properly. I came up with a solution (below), but not only I rely on Reflection, which I think can be avoided, it won't work in the cases where the Eat() method is not implemented, thus ending calling the Eat() method in the base class in a loop.

public interface IEater
{
    public void Eat(IEater obj);
}

public abstract class Animal : IEater
{
    public Size Size { get; set; }
    public bool IsDead { get; set; }

    public Animal(Size size)
    {
        this.Size = size;
        this.IsDead = false;
    }

    public void Eat(IEater obj)
    {
        if ( this.IsDead )
        {
            return;
        }
        var method = this.GetType().GetMethod("Eat", new Type[] { obj.GetType() });
        method.Invoke(this, new object[] { obj });
    }
}

public enum Size {
    Small,
    Medium,
    Big 
};

public class Lion : Animal
{
    public Lion(Size size) : base(size)
    {
    }

    public void Eat(Hyena hyena)
    {
        if (this.Size >= hyena.Size)
        {
            hyena.IsDead = true;
        }
    }

    public void Eat(Lion lion)
    {
        if (this.Size > lion.Size)
        {
            lion.IsDead = true;
        }
    }
}

public class Hyena : Animal
{
    public Hyena(Size size) : base(size)
    {
    }

    public void Eat(Hyena hyena)
    {
        if (this.Size > hyena.Size)
        {
            hyena.IsDead = true;
        }
    }
}

public static class AnimalPark
{
    public static void Process(IList<IEater> animals)
    {
        int length = animals.Count;
        for(int i=1; i<length; i++)
        {
            animals[i - 1].Eat(animals[i]);
        }
    }
}

What are your thoughts on my solution? How would you handle this case?

\$\endgroup\$
5
\$\begingroup\$

every time I would need to add a new animal, I would not be able to do it just by extending the existing classes or by adding new ones: I would have to modify the existing classes.

The problem exists in both your approaches. It is because the eating preferences of individual species is mutualy dependent on all other existing animals. If you add new animal, you define what existing animals it likes to eat, but unless you modify existing animals, no existing animal will ever be interested in eating the new animal.

Generally relying on object being of a specific type is not polymorphism, it's a rape.

You need to abstract and centralize the eating preferences somehow.

public enum AnimalKind
{
  Lion,
  Hyena
}

public interface IDinner
{
  public AnimalKind Kind {get;}
  public Size Size {get;}
}

public interface IEater
{
  public bool Likes(IDinner dinner); 
}

public interface IAnimal : IEater, IDinner
{
  bool IsDead {get;}
  void Kill();
}

class Animal : IAnimal
{
  public AnimalKind Kind {get;}
  public Size Size {get;}
  public bool IsDead {get;}

  private IDictionary<AnimalKind, Size> Preferences {get;}

  public Animal(AnimalKind kind, Size size, IDictionary<AnimalKind, Size> preferences)
  {
    this.Kind = kind;
    this.Size = size;
    this.Preferences = preferences;
  }

  public bool Likes(IDinner dinner)
  {
     var maxSize = this.Preferences[dinner.Kind];
     return maxSize != null && dinner.Size <= maxSize;
  }

  public void Kill()
  {
    IsDead = true;
  }
}

public interface IAnimalFactory
{
  public IAnimal CreateAnimal(AnimalKind kind, Size size);
}

public class AnimalFactory : IAnimalFactory
{
  // centralized place for all animals and their eating preferences
}

public static class AnimalPark
{
    public static void Process(IList<IAnimal> animals)
    {
        int length = animals.Count;
        for(int i=1; i<length; i++)
        {
            var eater = animals[i - 1];
            var dinner = animals[i];
            if (!dinner.IsDead && !eater.IsDead && eater.Likes(dinner)) {
                dinner.Kill();
            }
        }
    }
}

Adding new animal now means just extending AnimalKind enum and modifying AnimalFactory class to be aware of the new kind.

Although you might object that having the enum is also not nice. And I kinda agree, but I didn't want to invent an abstraction of the dinner description that was not provided by you. Maybe dinner description could instead be something like a description of how big teeth and paws the dinner has, how aggresive and vicious they look, if they have colors that imply being venomous, etc... If you impleent something like this and get rid of the AnimalKind enum, you will need to change the factory to contain a method per existing animal, but I would not consider it a problem, it may actually be so even with the enum existing...

Also notice I have moved the actual killing of the animal to the AnimalPark class, because that would be the same for all animals. An animal is always killed when eaten and stays alive when not eaten, right?

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yes, your assumption is right. Thank you so much for taking the time to answer! \$\endgroup\$ – ccoutinho Mar 13 at 13:59
  • \$\begingroup\$ I am wondering if the Preferences at the Animal level is the best: why have the preferences in every animal instance, knowing that animals of the same kind and size will have the same preferences? Shouldn't we pass that logic to the centralized component that deals with all the preferences? I came up with a new solution by creating the centralized module you suggested, and I also addressed the issue I just mentioned (maybe it is not really a issue, you will tell me!) I also took out some of the logic from the Animal, and ended up getting rid of IEater (as well as IDinner) interface. \$\endgroup\$ – ccoutinho Mar 13 at 23:05
  • \$\begingroup\$ @rsy "knowing that animals of the same kind and size will have the same preferences". Will they? I didnt make this assumption. What if an anorectic Lion doesnt want to eat anything? What If a crazy hyena Will attept to eat anyone? My approach allows to eventually have extra ordinary individuums. And when asking an animal object for preferences it does not need to search in dictionary, this was done just once at creation... \$\endgroup\$ – slepic Mar 14 at 5:18
  • \$\begingroup\$ It definitely makes sense what you are saying, didn't think about it. \$\endgroup\$ – ccoutinho Mar 14 at 16:29
1
\$\begingroup\$

After reading the answer given by slepic, I decided to take a different approach. I came up with the following solution, which I think doesn't violate neither the Open-Closed Principle nor the remaining SOLID premises. The main change I have made was to pass the "eating preferences" logic to a centralized component.

public abstract class Animal
{
    public AnimalKind Kind { get; set; }
    public Size Size { get; set; }
    public bool IsDead { get; set; }

    protected Animal(Size size)
    {
        this.Size = size;
    }
}

public enum AnimalKind
{
    Error = 0,
    Hyena = 1,
    Lion = 2
}

public class Lion : Animal
{
    public Lion(Size size) : base(size)
    {
        Kind = AnimalKind.Lion;
    }

    // Some more logic, which the exercise had, but I omitted because it was not necessary for this question
}

public class Hyena : Animal
{
    public Hyena(Size size) : base(size)
    {
        Kind = AnimalKind.Hyena;
    }

    // Some more logic, which the exercise had, but I omitted because it was not necessary for this question
}

public enum Size
{
    Error = 0,
    Small = 1,
    Medium = 2,
    Big = 3
}

public static class EatingPreferencesFactory
{
    public static IDictionary<AnimalKind, Size> GetEatingPreferences(Animal animal)
    {
        Size biggestPreySizeExclusive;
        IDictionary<AnimalKind, Size> animalPreys = new Dictionary<AnimalKind, Size>();

        switch(animal.Kind)
        {
            case AnimalKind.Lion: 
                                animalPreys.Add(AnimalKind.Hyena, animal.Size);

                                biggestPreySizeExclusive = animal.Size;
                                if(biggestPreySizeExclusive > Size.Small && biggestPreySizeExclusive < animal.Size)
                                {
                                    animalPreys.Add(AnimalKind.Lion, biggestPreySizeExclusive);
                                }
                                break;

            case AnimalKind.Hyena:
                                biggestPreySizeExclusive = animal.Size;
                                if (biggestPreySizeExclusive > Size.Small && biggestPreySizeExclusive < animal.Size)
                                {
                                    animalPreys.Add(AnimalKind.Hyena, animal.Size);
                                }
                                break;

            default: break;
        }


        return animalPreys;
    }
}

public interface IEatingPreferencesService
{
    public bool CanEat(Animal eater, Animal animal);
}

public class EatingPreferencesService : IEatingPreferencesService
{
    public bool CanEat(Animal eater, Animal prey)
    {
        if (eater.IsDead || prey.IsDead) return false;

        var eatingPreferences = EatingPreferencesFactory.GetEatingPreferences(eater);
        if (eatingPreferences.TryGetValue(prey.Kind, out Size ediblePreySize))
        {
            if(ediblePreySize>=prey.Size)
            {
                return true;
            }
        }

        return false;
    }
}

public class AnimalPark
{
    private readonly IEatingPreferencesService _eatingPreferencesService;
    private readonly IList<Animal> _animals = new List<Animal>();

    public AnimalPark(IEatingPreferencesService eatingPreferencesService)
    {
        _eatingPreferencesService = eatingPreferencesService;
    }

    public void AddAnimal(Animal animal)
    {
        if(animal != null)
        {
            _animals.Add(animal);
        }
    }

    public void Process()
    {
        int length = _animals.Count;
        for(int i=1; i<length; i++)
        {
            if( _eatingPreferencesService.CanEat(_animals[i - 1], _animals[i]) )
            {
                _animals[i].IsDead = true;
            }
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ This is worth posting as a new question for a comparative review. And please include concrete animals, as I don't understand why Animal is now abstract. \$\endgroup\$ – slepic Mar 14 at 7:30
  • \$\begingroup\$ The code for the animals has been posted. I had omitted because it is not relevant for the exercise. Ok, I will post this as a new question. \$\endgroup\$ – ccoutinho Mar 14 at 16:05
  • \$\begingroup\$ @slepic : codereview.stackexchange.com/questions/238890/… \$\endgroup\$ – ccoutinho Mar 14 at 16:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.