3
\$\begingroup\$

This is an improvement on the code presented here

I was given the exercise of refactoring some code that was breaking the Open-Closed Principle in a SOLID design, ensuring that the new code would follow all 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, in which the animal i tries to eat the i+1 animal. What are your thoughts about the code below?

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;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ what is the basic rule for an animal to be eaten ? in your logic , it's the size, so there is a base rule which is firstAnimal.Size > secondAnimal.Size this means firstAnimal would eat the second one. or that what I understood in your logic. is there any other required logic for this part ? \$\endgroup\$ – iSR5 Mar 14 at 18:01
  • \$\begingroup\$ In this case, the rule doesn't differ much from that: An Hyena can eat a Lion if it is a bigger, however a Lion can eat a same sized Lion. Carnivore "attacks" happen when the secondAnimal is smaller. But this is just a simplification: if some more animals need to be added, that logic can change, and even more variables can come into play \$\endgroup\$ – ccoutinho Mar 14 at 18:31
3
\$\begingroup\$

that's me who did the review of your previous version.

I'd still stick to what I proposed in the other answer. So here I will just add a few additions and clarifications. Then I will mostly try to focus on where you break SOLID and maybe other OOP principles in your current version. I will also include a few references to iSR5's answer.

I realized the IEater interface is never used alone, so the Eat method could belong to IAnimal directly.

Also maybe the IDictionary would better be hidden within an implementation of a new interface

interface IEatingPreferences
{
  public bool CanEat(IDinner eater, IDinner dinner);
}

Now you see we have basically a comparator interface for two dinners, which maybe tells us that instead of IAnimal extending IDinner, it could just have a property of IDinner type and the IDinner interface itself would deserve a better name like IAnimalAppearance (assuming animals will decide their prey on its apprearance is a good desription of how its supposed to be working here).

Notice that IEatingPreferences plays the role of a strategy of the strategy pattern (in my implementation where it is held by the animal objects).

As I already mentioned (in previous post comments) and opposed to your and iSR5's solutions, it allows for animals to behave in individual manner (not every small lion has to behave like every other small lion). And I believe for this reason both your and iSR5's solution break encapsulation principle in this respect. You expose data classes to cover the behaviour from outside. You should encapsulate the data with the behaviour together. Now if you do that you are free to extend the animls for example that they can have mood which would actually influence their eating preferences at certain mood.

Further the abstract factory was there just to simplify the common cases, while nothing is preventing you from extending the factory (as oposed to modifying it) to create those individually behaved animals that dont behave like others of their kind. Or you could implement a builder for the animals. Whatever will ease the creating of the animals...

Errors in Enums

I am not sure why you added AnimalKind.Error and Size.Error. Nor I see them being referenced from anywhere nor I see reason for them to part of the respective enums.

I would actually recommend to go the other way. Instead promote the enums to classes with comparision operators etc. (again this would strenghten the encapsulation)

Dependency Inversion Principle (The "D" in SOLID)

EatingPreferencesService violates this principle because it calls static method EatingPreferencesFactory.GetEatingPreferences. The factory should be injected to the service from outside.

Single Responsibility Principle (The "S" in SOLID)

The AnimalPark class now break this principle because it manipulates list of animals and also allows to build it in the first place. We already have interface we can use (and that's something I would also argue against in iSR5's solution, why reinventing what already exists?): ICollection<IAnimal>.

Interface Segregation Principle (The "I" in SOLID)

On few places you depend on Animal but never use IsDead or Eat. This breaks the above principle. Like why is EatingPreferencesFactory allowed to kill the animals?

For this reason, in my solution, I have separated IDinner/IAnimalAppearance from IAnimal. In my solution there are places that need just the appearance and there are places which need the entire animal. As I mentioned above, doing this through composition leaves us with a more flexible base (we are now free to extend base animal to privde those individuum overrides, while allowed same crazyness for all kinds of animals)

Ie.

class CrazyAnimal : Animal
{
  // ...
}

// yet still we can have
new CrazyAnimal(AnimalKind.Lion)
new CrazyAnimal(AnimalKind.Hyena)

// without implementing CrazyLion and CrazyHyena. And eventually CrazyElephant when elepthants become a thing.

// although maybe we have even an extra level of freedom to do this:
new Animal(AnimalKind.Lion, new CrazyEatingPreferences(/*...*/));
// if eating preferences are the crazy thing about that animal indiviuum.

In relation to this, it buffles me why you still have Lion and Hyena classes extending Animal if there is nothing abstract about the Animal except (and idk why) the constructor. Smells like SRP or Liskov Substition principle violation...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "Your (...) solution break encapsulation principle in this respect. You expose data classes to cover the behaviour from outside. You should encapsulate the data with the behaviour together" - Very good point, thanks! Regarding the Enum values, I wanted just to avoid having the value 0 given to an animal, and have 0 to be explicitly represented as an invalid Enum. Good point about breaking the dependency Inversion Principle (The "D" in SOLID). I did know the correct way was to inject the EatingPreferencesFactory, but I wasn't aware I was breaking the Dependency Inversion Principle! \$\endgroup\$ – ccoutinho Mar 19 at 2:43
  • \$\begingroup\$ Regarding your comments on both mine and iSR5 solutions breaking the Single Responsibility Principle, I also agree with you. So, what you suggest is just to pass the animals being processed as a parameter to the Process() method? \$\endgroup\$ – ccoutinho Mar 19 at 3:27
  • \$\begingroup\$ Still regarding having broken encapsulation, and summing up your two answers, it seems to me that: a) The Open Closed Principle is broken if the eating preferences logic is kept at the Animal level, thus meaning that there will be changes to these individual classes whenever new animals will be added, b) Encapsulation is broken if this logic is passed to some kind of an Animal Factory. \$\endgroup\$ – ccoutinho Mar 19 at 13:56
  • \$\begingroup\$ (...) The only solution that comes to my mind is to get rid of the service in my solution, and while still keeping the eating preferences logic at an Eating Preferences Factory, I would store the returned Objects in the Animals. Which kinda aligns to your solution in the other answer. What do you think? \$\endgroup\$ – ccoutinho Mar 19 at 13:57
  • \$\begingroup\$ Regarding what you said about having Animal as an abstract class, and Hyena and Lion inheriting from it, since the two Animals share the CanEat method, shouldn't I leave it in the super class? And when some Animal needs to override it, they can. Taking all the answers in consideration, I came up with a new solution and I will post it as an answer. I don't want to create a third post. Would be great if you could find some time to review it as well! \$\endgroup\$ – ccoutinho Mar 19 at 20:14
3
\$\begingroup\$

I think you're overdoing it. I know that you want SOLID pattern, but enforcing it like this way add more code complicity.

What happens on your code, is defining a AnimalKind and define multiple animals names (such as Lion, Hyena), which would be fine for a testable project, but it will make things much complicated in real or big projects. A simple example of this complicity would be adding more different animals from different species such as (Elephant, Parrot, Penguin, Spider ..etc). Now, think of how you would apply CanEat on these different kinds of animals. If you take Size as a factor, an Elephant would eat all animals including fish, and a Parrot would be eaten by a Penguin .. and so on.This would make a false logic. So, what you need is to use AnimalKind to define the specie type such as mammals, birds, reptiles, fishes, ..etc. Then, add AnimalName, which would holds the name of that animal such as Lion, Hyena ..etc.

Another thing you need to consider is the food system for each animal. Is the animal Herbivore, Carnivore or Omnivore ? what type of food source does it eat Plant, meat of both ? if you define that for each animal, it'll make things much smoother to compare and work with. As you'll have several factors that can be used to specify your logic.

Also, your EatingPreferences you're treating it as an attack, and not for eating purpose which makes it a misleading naming.

Naming convesion needs to be enforced by adding a prefix naming on each class that releated to another class. So, EatingPreferences would be AnimalEatingPreferences and Size would be AnimalSize ..etc.

Overall, I think you only need three classes that would serve your current context purpose, while having the ability to extend, add more logic to them.

First the Enums :

public enum AnimalType { Mammals, Birds, Reptiles, Amphibians, Fishes, Insects }
public enum AnimalName { Hyena, Lion }
public enum AnimalSize { Small, Medium, Big }
public enum AnimalDietType { Herbivore, Carnivore, Omnivore }
public enum AnimalFoodSource { Plant, Meat, Both }

these are just stored information, which we need an object to hold them (model), and a collection to gather them in one box (so it can be retrieved). Also, we will need a middle-ware class that defines the rules (surely each animal has some living rules), but the trick here, we want it to be extendable as we expect to have multiple unique rules for each animal and others might be shared. Finally, we need a container class that would be used to connect all of these objects.

So, we need :

  1. Model Class (Animal)
  2. Collection Class (AnimalCollection)
  3. Living Rules Class (AnimalSurvivalRole)
  4. Container class (AnimalPark)

To make things much easier to maintain, we will implement interfaces for some of the classes that I think it would be useful. an interface would implement a contract to the class and also would give you the ability to access its properties without using reflection (you'll see that in the below examples).

So, first we will implement the interfaces :

public interface IAnimal
{
    AnimalType Type { get; }
    AnimalName Name { get; }
    AnimalSize Size { get; }
    AnimalDietType DietType { get;  }
    AnimalFoodSource FoodSource { get; }
    IAnimalSurvivalRole SurvivalRoles { get; }
}

public interface IAnimalSurvivalRole
{
    bool IsEdible { get; }
    bool IsKillable { get; }
}

public interface IAnimalCollection
{
    int Count { get; }
    void Clear();
    void Add(IAnimal animal);
    bool Remove(IAnimal animal);
    IEnumerable<IAnimal> GetAnimals();
}

the interface is a key that you want to define your requirement. Now, from these interfaces, we can implement their classes :

public class Animal : IAnimal
{
    public AnimalType Type { get; set; }

    public AnimalName Name { get; set; }

    public AnimalSize Size { get; set; }

    public AnimalDietType DietType { get; set; }

    public AnimalFoodSource FoodSource { get; set; }

    public IAnimalSurvivalRole SurvivalRoles { get; set; }

    public bool IsDead { get; set; }

}

AnimalSurvivalRole would contain the rules for each animal in order to survive either feeding or attacking or any other type that would be linked to animal survival (for instance, it's not only feeding or attacking each other, some other natural events would kill an animal like drowning, or fire ..etc). So, this class would cover all these things.

// use this class to define the animal survival roles such as eating and battling to live.
public sealed class AnimalSurvivalRole : IAnimalSurvivalRole
{
    private readonly IAnimal _firstAnimal; 

    private readonly IAnimal _secondAnimal;

    public bool IsEdible => CanEat(_firstAnimal, _secondAnimal);

    public bool IsKillable => CanKill(_firstAnimal, _secondAnimal);

    public AnimalSurvivalRole(IAnimal first, IAnimal second)
    {
        _firstAnimal = first;
        _secondAnimal = second;
    }

    private bool CanEat(IAnimal first, IAnimal second)
    {
        return first.DietType != AnimalDietType.Herbivore && second.DietType == AnimalDietType.Herbivore;
    }

    private bool CanKill(IAnimal first, IAnimal second)
    {
        // by your logic, if the animals are the same or one of them is bigger than the other, it should kill the other one
        return first.Size > second.Size || first.Type == second.Type;
    }
}

For the time being, I used it as a comparable class to compare between two objects, it can be extended for other survival causes.

The AnimalPark class is used as collection and also as main class at the same time. This violates SOLID principle. You will need to implement a collection class that meant to only store animals objects only, all the storing validations should be inside this collection.

public class AnimalCollection : IAnimalCollection, IEnumerable<IAnimal>
{
    private readonly List<IAnimal> _animals = new List<IAnimal>();

    public AnimalCollection() { }

    public AnimalCollection(IEnumerable<IAnimal> animals) { _animals.AddRange(animals); }

    public IAnimal this[int index] 
    {
        get => _animals[index];
        set => _animals[index] = value;
    }

    public int Count => _animals.Count;

    public void Clear() => _animals.Clear();

    public void Add(IAnimal animal)
    {
        if(animal == null) { throw new ArgumentNullException(nameof(animal)); }

        _animals.Add(animal);
    }

    public bool Remove(IAnimal animal)
    {
        if (animal == null) { throw new ArgumentNullException(nameof(animal)); }

        return _animals.Remove(animal);
    }

    public IEnumerable<IAnimal> GetAnimals() => _animals;

    /// <summary>Returns an enumerator that iterates through the collection.</summary>
    /// <returns>An enumerator that can be used to iterate through the collection.</returns>
    public IEnumerator<IAnimal> GetEnumerator() => _animals.GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

}

IEnumerator is a way to give your class an ability to use foreach loop.

Now, use the AnimalCollection inside the AnimalPark as a storage, and make AnimalPark as main logic process (container) for all of them :

public class AnimalPark
{
    private readonly AnimalCollection _animals = new AnimalCollection();

    public AnimalPark() { Initiate(); }

    private void Initiate()
    {
        // set here the animals in this park. 

        // Sample : this park has one lion and Hyena

        var lion = new Animal
        {
            Type = AnimalType.Mammals,
            Name = AnimalName.Lion,
            Size = AnimalSize.Big,
            DietType = AnimalDietType.Carnivore,

            FoodSource = AnimalFoodSource.Meat
        };

        var hyena = new Animal
        {
            Type = AnimalType.Mammals,
            Name = AnimalName.Hyena,
            Size = AnimalSize.Big,
            DietType = AnimalDietType.Carnivore,
            FoodSource = AnimalFoodSource.Meat
        };

        // Now set the SurvivalRoles
        lion.SurvivalRoles = new AnimalSurvivalRole(lion, hyena);
        hyena.SurvivalRoles = new AnimalSurvivalRole(hyena, lion);

        // add them to the animal storage
        _animals.Add(lion);
        _animals.Add(hyena);            
    }

    public IEnumerable<IAnimal> GetAnimals()
    {
        return _animals.GetAnimals();
    }

}

The AnimalSurvivalRole can be defined differently, but since it's not clear for me what you intent to do exactly, I left it inside the model as a view so you can use it to define rule for each animal or type of animal, or you can create another class role that would handle other animal roles.

Try to always categorize the code requriements, and choose a good naming convetion that would describe the object role clearely, even if is it going to be long, its better than having short description that ended you with (I'm just here, go figure!). For instance, You will notice that IsDead I've never used it, because i'm not sure what you meant by that exactly dead how ? if is it dead why still exists in the park at the first place ? these are some of the question that I thought about when I came across it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Well picked about breaking the Single Responsibility Principle! The context of this question is an interview I had, in which I struggled a lot. So I decided to reproduce the exercise, and when I had some questions, I decided to post my solution for review. That said, I omitted some parts of the exercise, like some behaviour associated with both the Lion and the Hyena. I really like your solution, but I don't think I could have avoided having a class representation of each animal because there was more behaviour linked with both animals. \$\endgroup\$ – ccoutinho Mar 19 at 1:20
  • \$\begingroup\$ But I definitely agree that having the animal name as an Enum is a good practice! I also like more the way you coped with the survival/eating logic than in my solution. I also agree that it felt weird to leave dead animals in the AnimalPark, but that was taken directly from the given exercise. Thanks for your enriching answer! \$\endgroup\$ – ccoutinho Mar 19 at 1:24
1
\$\begingroup\$

Taking the two answers in this post in consideration, as well as in the linked post, and the discussions, I came up with this solution. I believe it now finally respects all the SOLID principles!

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

        public IAnimalEatingPreferences EatingPreferences { get; set; }


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

        public virtual bool CanEat(Animal prey)
        {
            if (this.IsDead || prey.IsDead) return false;

            if (EatingPreferences.TryGetPreys(prey.Kind, out AnimalSize ediblePreySize))
            {
                if (ediblePreySize >= prey.Size)
                {
                    return true;
                }
            }

            return false;
        }
    }

    public class Hyena : Animal
    {
        public Hyena(AnimalSize 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 class Lion : Animal
    {
        public Lion(AnimalSize 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 enum AnimalKind
    {
        None = 0,
        Hyena = 1,
        Lion = 2
    }

    public enum AnimalSize
    {
        None = 0,
        Small = 1,
        Medium = 2,
        Big = 3
    }

    public interface IEater
    {
        bool CanEat(Animal prey);
    }

    public interface IEatingPreferencesFactory
    {
        IAnimalEatingPreferences GetEatingPreferences(AnimalKind animal, AnimalSize size);
    }

public interface IAnimalPark
    {
        void AddAnimal(Animal animal);
        void Process();
    }

    public interface IAnimalEatingPreferences
    {
        void AddPrey(AnimalKind animal, AnimalSize size);
        bool TryGetPreys(AnimalKind prey, out AnimalSize returnEdiblePreySize);
    }

    public class EatingPreferencesFactory : IEatingPreferencesFactory
    {
        public IAnimalEatingPreferences GetEatingPreferences(AnimalKind animal, AnimalSize size)
        {
            AnimalEatingPreferences eatingPreferences = new AnimalEatingPreferences();
            AnimalSize biggestPreySizeExclusive;

            switch(animal)
            {
                case AnimalKind.Lion:
                                    eatingPreferences.AddPrey(AnimalKind.Hyena, size);

                                    biggestPreySizeExclusive = size;
                                    if(biggestPreySizeExclusive > AnimalSize.Small && biggestPreySizeExclusive < size)
                                    {
                                        eatingPreferences.AddPrey(AnimalKind.Lion, biggestPreySizeExclusive);
                                    }
                                    break;

                case AnimalKind.Hyena:
                                    biggestPreySizeExclusive = size;
                                    if (biggestPreySizeExclusive > AnimalSize.Small && biggestPreySizeExclusive < size)
                                    {
                                        eatingPreferences.AddPrey(AnimalKind.Hyena, size);
                                    }
                                    break;

                default: break;
            }

            return eatingPreferences;
        }
    }

public class AnimalEatingPreferences : IAnimalEatingPreferences
    {
        private readonly IDictionary<AnimalKind, AnimalSize> _animalPreys;

        public AnimalEatingPreferences()
        {
            _animalPreys = new Dictionary<AnimalKind, AnimalSize>();
        }

        public void AddPrey(AnimalKind animal, AnimalSize size)
        {
            _animalPreys.Add(animal, size);
        }

        public bool TryGetPreys(AnimalKind prey, out AnimalSize returnEdiblePreySize)
        {
            if( _animalPreys.TryGetValue(prey, out AnimalSize ediblePreySize) )
            {
                returnEdiblePreySize = ediblePreySize;
                return true;
            }

            returnEdiblePreySize = AnimalSize.None;
            return false;
        }
    }

public class AnimalPark : IAnimalPark
    {
        private readonly IList<Animal> _animals;

        public AnimalPark(IList<Animal> animals)
        {
            _animals = animals;
        }

        public void AddAnimal(Animal animal)
        {
            _animals.Add(animal);
        }

        public void Process()
        {
            int length = _animals.Count;
            for(int i=1; i<length; i++)
            {
                if( _animals[i-1].CanEat(_animals[i]) )
                {
                    _animals[i].IsDead = true;
                }
            }
        }
    }
| improve this answer | |
\$\endgroup\$

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.