Question

I am in the process of refining my code to adhere more to my current understanding of the Single Responsibility Principle (SRP). Originally, I had a class called Animal that had a set of methods and several instance variables. According to the SRP, the main idea is that a class is only supposed to change for one "reason." The inclination I got is that changing the behavior of an Animal and changing the properties of an Animal would be two separate "reasons" to change, so I figured I needed to use encapsulation to separate them.

As a result, I created an AnimalInfo class to act as a wrapper and store all of the Animal's variables. The issue I see is that now instead of just calling the variables themselves, the Animal has to call its AnimalInfo class to retrieve the info, as if it's no longer the owner of its own info. There are also cases where other classes want to access the information in an AnimalInfo class but only have access to the Animal, so I figured it made most sense to make getters and setters in Animal that call the equivalent in AnimalInfo. Example:

public class Animal{

private AnimalInfo animalInfo;

public void eatFood(int amount)
{
   getFoodSupply().consume(amount); //Consume method behavior isn't important. Just an example.
}

public void sleep()
{
    setSleepMode(true);
}

public void hunt()
{
    setHuntMode(true);
}

public FoodSupply getFoodSupply()
{
    return animalInfo.getFoodSupply();
}
public void setFoodSupply(FoodSupply supply)
{
    return animalInfo.setFoodSupply(supply);
}
public boolean setSleeping(boolean sleep)
{
    return animalInfo.setSleeping(sleep);
}
public boolean setHunting(boolean hunt)
{
    return animalInfo.setHunting(hunt);
}
public boolean isHunting()
{
    return animalInfo.isHunting();
}
public boolean isSleeping()
{
    return animalInfo.isSleeping();
}

}

public class AnimalInfo()
{
  private FoodSupply foodSupply;
  private boolean sleeping;
  private boolean hunting;

public FoodSupply getFoodSupply()
{
    return foodSupply; 
}
public void setFoodSupply(FoodSupply supply)
{
    foodSupply = supply;
}
public boolean setSleeping(boolean sleep)
{
    sleeping = sleep;
}
public boolean setHunting(boolean hunt)
{
    hunting = hunt;
}
public boolean isHunting()
{
    return hunting;
}
public boolean isSleeping()
{
    return sleeping;
}

public AnimalInfo getInfo()
{
    return animalInfo;
}
public void setInfo(AnimalInfo info)
{
    animalInfo = info;
}
}

The impression I get is that these "throughline" methods are unnecessary. I only added them in the interest of readability, as one result of this strategy has been a heavy amount of multiple method calls on one line( getThis().getThat().doThis()), and I'm not certain that's best practice.

On a broader scale, am I off-base in my idea to implement an AnimalInfo class for SRP? Should the getters, setters, and variables just be part of Animal? It seems to me that I've sectioned off a class just to store the instance variables and getters/setters, but it's not actually reducing the total number of methods since I'm making more getters and setters to account for the extra method calls. Should it be encapsulated based on a different metric other than behavior/data? Should AnimalInfo be a nested class within Animal since the two will always exist together? I didn't think it was necessary since AnimalInfo never needs to access the methods in Animal, only the other way around.

Something about my current strategy seems wrong. What am I missing?

Was it helpful?

Solution

Yes, there is something wrong in your approach.

Separating the properties from the methods cannot be the right way:

  • Because the methods would need to access the properties. Using getters and setters enable this access in a controlled manner. But at the same time it exposes internal information instead of encapsulating it.
  • The consequence would be a strong coupling between classes. This is against the principle of least knowledge and might even endanger the interface segregation principle.
  • Strong coupling endangers the Open/Close principle and together with your dichotomic design will lead to a complexity explosion: When you want to extend the animal and have a Dog and a Bird class, how would you do it ? Adding a Dog and DogInfo and a Bird and BirdInfo class ?

SRP is not what you think:

  • You got it right: SRP is not about what a class does but about the resons to change. Congratulations !
  • But you got the reason to change wrong. It's not your fault, because a lot of different technical arguments are brought forward in a lot of articles, whereas in reality it's about people and decision making, as Uncle Bob, the "inventor" of this principle explains in this article.
  • Your design breaks SRP, because the Animal class could now have two reasons to change: first the Animal concept might evolve (one decision making, by the responsible person/team for Animal, for example addition of a method hideInFOrest()), second AnimalInfo might evolve (another decision making, by the responsible person/team of that class, for example for adding a property canFly, and the corresponding getters and setters, and you would have to take this into account in Animal, in its interface).
  • So better follow your instinct: if you think that things belongs together, designing a class Animal with methods and properties is perfect.

Objects shall not just be a bunch of getters and setters

  • Properties should be a mean to implement the behaviors of an object.
  • But most of what Animal does, is just using (indirectly) properties of AnimalInfo. There is no animal specific behavior. But ok, maybe it's just the demo effect of a very simplified example.

OTHER TIPS

The inclination I got is that changing the behavior of an Animal and changing the properties of an Animal would be two separate "reasons" to change, so I figured I needed to use encapsulation to separate them.

Things that can change do not imply reasons to change. For a reduction ad absurdum, a change in the naming convention would a be reason to change virtually all the code. Will you isolate your code from the names? No.

Instead, your reasons to change are always external to the system. If you do not know them, you will not find them in the code. Instead you will find them in what the code is trying to do.

For example, if you are modeling reality, and implementing things that anuimal can do in real life... a reason to change could be finding out that animals can do something you didn't know.

See also my answer at Doesn't having more than 1 method break the Single Responsibility Principle?.

The issue I see is that now instead of just calling the variables themselves, the Animal has to call its AnimalInfo class to retrieve the info, as if it's no longer the owner of its own info.

Not necessarily a problem. Converting part or the totality of the state into a separate type is valid refactoring. However, you do not seem to be earning anything by doing this. In particular from the point of view of SRP, consider that a change in AnimalInfo implies a change in Animal. I think in your case, separating AnimalInfo is unnecessary, even counterproductive.

Instead, I think your problem are the getters and setters. You are essentially making a struct (except it is Java). Are all possible combinations of the fields valid states? You should be checking that! You should not leave the object in an invalid state! That is what encapsulation is for. For example, can the animal be hunting while sleeping?

You probably want to implement this as a state machine. You could have an enum type AnimalState that has the possible states of the animal (hunting, sleeping, etc.). Then Animal has a getter for the state, and methods that change the state (at least a setter for the state※).

Done correctly, you should be able to change the list of states for the Animal, without changing the Animal class. That is separation of data and behavior, without breaking encapsulation.

In fact, done correctly, you could change the AnimalState could be changed to a class, and each possible state is an instance, which has a name. That would allow you to load the list of states from a configuration file, or database, or user input, etc.

Another advantage of having AnimalState be a class, is that you can make derived types. For example, you can have type that has FoodSupply and use it for the eating state. Although, I am not sure that is the way you want to take this.

※: There could be rules about the transitions from one state to the other. Also Thus bool TrySetState(AnimalState newState) could be useful. Also, depending on the requirements, you may find bool TransitionState(AnimalState expectedState, AnimalState newState) or similar useful.

At the end it depends on the requirements of the system. There is value is thinking how they could change, and making the code easier to change in the future. For example, in this case, we know there is a list of things the animal could be doing, we can imagine the list can change (that is a change in requirements), then it makes sense to make code that makes that change easy (e.g. using an enum type). Similarly, the requirement could change to say that the states come from a database.

Note that you should not be writing database adapters just because the client could ask for that. Separating AnimalState is enough, because it allows you change AnimalState without changing Animal.

I could be totally off-mark. Perhaps animals can hunt in sleep in this system. In doubt, ask the client. Requirements are paramount. You must understand from where requirements come from, so you can understand why they could change, which will dictate how to separate your code.

I want to mention another possible design. Instead (or in addition) of separating state, you can separate mutation of the state. That is, you could make your class immutable, then have method that return a new instance of Animal with the state change (or the same instance of no change was necessary).

am I off-base in my idea to implement an AnimalInfo class for SRP?

Yes.

There are also cases where other classes want to access the information in an AnimalInfo class but only have access to the Animal,

This is called feature envy. It's a sign you've put logic in the wrong class. The methods that need data from Animal usually belong in Animal.

Sometimes the methods exist on the other side of some boundary and can't be moved into Animal. In those cases you use a Data Transfer Object (DTO) to move the data. That's what AnimalInfo is.

DTO is actually an unfortunate name, DTO's are not true objects. They are data structures. They have no behavior. The getters and setters just give you a place to set a breakpoint.

Animal should be a true object. It should have behavior. It shouldn't have getters and setters. If you need to deal with one of those boundaries what it should do is eat and spit AnimalInfo.

I'll show you what I mean. I've taken a few liberties so that there is some meaningful business logic:

public class Animal
{    
    private FoodSupply foodSupply;
    private boolean sleeping;
    private boolean hunting;

    public Animal(AnimalInfo animalInfo) {
        foodSupply = animalInfo.getFoodSupply();
        sleeping = animalInfo.isSleeping();
        hunting = animalInfo.isHunting();
    }    

    public void eatFood(int amount)
    {
        if (!sleeping && !hunting) {
            foodSupply.consume(amount); 
        }
    }

    public void sleep()
    {
        sleeping = true;
    }

    public void wakeup()
    {
        sleeping = false;
    }

    public void hunt()
    {
        hunting = true;
    }

    public void relax()
    {
        hunting = false;
    }

    public AnimalInfo getInfo()
    {        
        return new AnimalInfo(foodSupply, sleeping, hunting);
    }

}

Notice that now Animals interface is all about behavior. You tell Animal what to do. You don't ask it what is going on.

Now Animal is fully encapsulated except for getInfo(). That's the only way to peek inside. And even that is a defensive copy that keeps coders from fiddling around inside.

Yet we can still build Animal from AnimalInfo and send AnimalInfo off to things like databases, files, networks, printers, GUI's, ya know, all that stuff on the other side of boundaries.

I think you get the point but here's what AnimalInfo looks like now:

public class AnimalInfo
{
    private FoodSupply foodSupply;
    private boolean sleeping;
    private boolean hunting;

    public AnimalInfo(FoodSupply foodSupply, boolean sleeping, boolean hunting) {
        this.foodSupply = foodSupply;
        this.sleeping = sleeping;
        this.hunting = hunting;
    }
    public FoodSupply getFoodSupply()
    {
        return foodSupply; 
    }
    public void setFoodSupply(FoodSupply supply)
    {
        foodSupply = supply;
    }
    public void setSleeping(boolean sleep)
    {
        sleeping = sleep;
    }
    public void setHunting(boolean hunt)
    {
        hunting = hunt;
    }
    public boolean isHunting()
    {
        return hunting;
    }
    public boolean isSleeping()
    {
        return sleeping;
    }
}

Again, this isn't an object. There is no real encapsulation here. This is a data structure packed with debugging code so you can tell when data is moving around.

If you haven't realized it yet this lesson isn't about SRP at all. This is about encapsulation. Real encapsulation. Not just throwing public getters and setters at everything private. Keep your privates private and you don't have to worry about what you're breaking when you change them.

If you take that idea seriously, then one thing you won't do is share AnimalInfo with local classes. No. If a local class needs this data then Animal needs that classes method moved.

Don't move data around unless you have to. Prefer moving methods.

TL;DR: It is not necessarily wrong to split this into two classes, but you are most likely over-engineering.

First of all there is no right or wrong in this case from looking at the code itself. The SRP is about reason to changes, which are reasons for the implementation of the classes to change. You didn't mention possible changes at all, leaving this all to speculation.

In general my stance is to go for the simple solution, where the simple solution works. In your case your class is not to big and you didn't mention any problems that would be solved by splitting off AnimalInfo, but I could see scenarios where it would make sense. To bring home my point I want to give two scenarios, one where it doesn't make sense to split of AnimalInfo and one where it does.

First Scenario: Imagine you are writing a paper on game theory and animal behaviour, you change your model a lot and it is mostly about adding new capabilities. Everytime you add a new capability you need to change two classes (e.g. to allow the animal to swim you need to add a method setSwiming to AnimalInfo, and a method swim() to Animal. You have two classes but they change for the same reason, it is just a lot of extra work. Splitting off AnimalInfo does not make sense.

Second Scenario: You are writing a web game where people can play with Tamagotchi like pets, you have a web interface where people give commands like "swim" or "sleep" or "hunt" and the Animal will do so. You start simple and run your game on one server, and because people just play for 2-3 hours and then forget about their animal you are fine with keeping the game state of the animals in memory only. Later you make several improvements to the game. First people complain that there is no simple way to tell the animal to stop whatever they are doing, so you add a button and a method alert() which tells the Animal to stop sleeping and stop hunting, but this doesn't change the game mechanic at all. Later you decide to store animal state in a local SQLite database, so that players can enjoy playing with their animals over multiple days even if the server goes down during nightly maintenance. Even later your game becomes so popular that you need to scale to multiple servers, so instead of connecting to a local SQLite database you connect to a separate shared MySQL database. You can see that you now are planning for different changes that can happen for different reason (change the UI, change the database) and could easily be worked on by different people without needing a lot of coordination.

As you can see, what makes sense or not, really depends on the future changes of your code you anticipate. If you are not sure, or anticipate no changes, go with the simplest solution you can refactor something simple to something more flexible anytime soon (the other way around is often harder).

The impression I get is that these "throughline" methods are unnecessary. I only added them in the interest of readability, as one result of this strategy has been a heavy amount of multiple method calls on one line( getThis().getThat().doThis()), and I'm not certain that's best practice.

I am not sure what to think about this practice myself, but it has a name: Search for Law of Demeter.

Licensed under: CC-BY-SA with attribution
scroll top