What SOLID principles am I breaking with this code?

Question

With the below pseudo code? Am I breaking any SOLID principles?

interface i_pet
  string get_name()
  string get_species()
  color get_fur_color()
end interface

interface i_cat implements i_pet
end interface

interface i_fish implements i_pet
   bool get_is_fresh_water()
end interface

class cat implements i_cat
  string name
  string species
  color fur_color

  cat(string name, string species, color fur_color):
    this.name = name
    this.species = species
    this.fur_color = fur color

  string get_species():
    return species

  string get_name():
    return name

  color get_fur_color:
    return fur_color
end class

class fish implements i_fish
  string name
  string species
  bool is_fresh_water

  string get_species():
    return species

  string get_name():
    return name

  color get_fur_color():
    throw exception

  bool get_is_fresh_water():
    return is_fresh_water
end class

class pet_printer
  print_pet_name(i_pet pet):
    print pet.get_name()

  print_fur_color(i_pet pet):
    print pet.get_fur_color()

  print_pet_species(i_pet pet):
    print pet.get_species()

  print_pet_type(i_pet pet):
    switch type of pet:
      case i_fish:
        print "Fish";
      case i_cat:
        print "Cat";
      default:
        print "Unkown";
    end switch
end class

I think I'm breaking the Interface segregation principle with i_pet having get_fur_color but I can't identify any other issues?

Answer 1

Whether the ISP is broken or not depends on what parts of the interface the clients require.

If there is just one client as in your example, and it requires all of the interface methods, then this is no ISP violation.

Moreover, it could perfectly make sense to have a color "indefinite", which can be the return value for pets like fish. This can be a valid design decision to keep things simple and does not violate any SOLID principle.

Answer 2

• Why does class cat have an instance field for species? All cats are the same species by definition.

• As you say, fur_color has no place in i_pet because not all pets have fur. But that's not a question of the ISP, it's worse - the method isn't just more than most clients need, it's actively wrong because it may or may not be appropriate for a subclass et all.

Answer 3

Yes you are right i would do it some thing like create another interface for bird which will implement i_Pet but with "color get_fur_color()" so:

interface i_pet
  string get_name()
  string get_species()
end interface

interface i_bird implements i_pet
  color get_fur_color()
end interface

Other then that i don't think if your breaking any solid principle. As all of the pets have name and specie as the cat has specie of "vertebrates", and name "XYZ".

Edit: Remove that extra interface for cat as its empty. Also change these function to properties (Not impotent but good practices). For more information about defference between function and properties visit here:

Answer 4

There are a fair number of principles being violated here that have nothing to do with SOLID. Understand that SOLID isn't a comprehensive list of every useful principle. Even Uncle Bob himself has published principles beyond SOLID that are worth considering.

You have indicated that you only want to focus on SOLID violations but that narrow view is causing confusion about ISP.

Specifically, does it make sense to ask for fur_color in your only client: pet_printer?

The principle in play here is abstraction. Does the pet_printer client have the right to not know exactly what pet it's printing? Can it require that pet types do the heavy lifting here?

Well abstraction isn't a SOLID principle but it's still a good one. If you were following it then you wouldn't be asking to know the type of pet you have and pet_printer could ask for fur_color and sometimes get "Not Applicable" as a result. This value rejects the assumption that all pets have fur without forcing you to change the question. Such values have a long tradition in computer science and mathematics. It's why we have numbers like zero and negatives.

Do that and every pet subtype can answer the question of fur color. Thus clients asking about fur color don't have to know the subtype.

If that doesn't satisfy you then you need to dive deeper into what your client is doing. It's trying to print a pet. It's getting confused because it's trying to deal with subtype specific details that it doesn't know without asking about subtype.

Clients that do that violate an OOP principle that it should be easy to add new types without changing old code. That runs you into a SOLID principle called Open Closed Principle (OCP). This is a fancy way to say it sucks to have to change old, tested, battle proven code to add something new.

A design that lets you add parrots without changing pet_printer would be preferable.

The worst violation of this is your print_pet_type method. I'm not saying switches are evil but why are you building a type hierarchy and then not using it? Give each pet a type method and your switch can go poof. Now pet_printer doesn't have to know if parrots exist.

I would strive for a design that would leave pet printer knowing as little about the pet as possible and allow it to focus on how it's printing: to the console, to a log, to paper, to the screen...

Do that and your ISP principle issue falls away because the pet_printer doesn't know what fur is. The subtype decides if it's going to say anything about fur when it gets asked what details need to be printed.

Don't follow SOLID in a vacuum. The SOLID principles rest on the backs of older principles. Without them SOLID is a lot of pointless confusion that happens to spell a spiffy sounding word.

Answer 5

You are violating ISP but not so much because of an out-of-place method.

The idea behind ISP is to keep interfaces small, which really means they should define just one behavioral aspect. This implies that both ICat and IFish are sub-optimal and pointless from an ISP perspective.

Instead of ICat you should have interfaces like IScratcher, IJumper, IMeatEater and implement those in Cat.

Instead of IFish you should have ISwimmer and IUnderwaterBreather.

Answer 6

As written you are breaking the Liskov substitution principle and Interface segregation principle. Anything that implements i_pet needs to be able to be substituted without breaking anything. The fish class is causing this to fail with get_fur_color() not being implemented. Furthermore, the pet_printer is breaking the open closed principle, the problem here is if the printed name for fish or cat changes then this unrelated class must also change, and if dog is added it must be modified again. The Empty interface of i_cat is an example of a violation of the YAGNI principle, which could lead to further problems if the interface is naively implemented or changed.

Fixing the first problem may be as simple as changing a method name to be getColor() which is more general and can work with any pet. The color class would be responsible for how to handle primary and secondary colors if that part becomes relevant. To fix the problem with the pet printer class, the type can become a read only string that is part of the i_pet interface that can simply be returned by a getType() method. Not only does this help fix the problem with the open closed principle, it follows tell don't ask as each implementation now tells its own type instead of an outside object asking and the acting on that information.