SRP: "It says that your class, or method should do only one thing"
When do I know my method is doing more than one thing?
Example:
I have a class Bus
with a List<Passenger>
and a enum BusState
in it. The state of the Bus
depends on the size of List<Passenger>
.
public void addPassenger(Passenger p){
this.passengerList.add(p);
if (passengerList.size < 10)
this.state = BusState.EMPTY;
else if (passengerList.size < 30)
this.state = BusState.HALF_FULL;
else if (passengerList.size >= 30)
this.state = BusState.FULL;
}
And even if I refactor this:
public void addPassenger(Passenger p){
this.passengerList.add(p);
changeBusState();
}
private void changeBusState(){
if (passengerList.size < 10)
this.state = BusState.EMPTY;
else if (passengerList.size < 30)
this.state = BusState.HALF_FULL;
else if (passengerList.size >= 30)
this.state = BusState.FULL;
}
In my opinion the method addPassenger()
is doing more than one thing:
- adding a new passenger to the List
- checking the current number of passengers
- changing the state of the bus if necessary
How can I understand the SRP? Is this method doing more than one thing?
Robert Martin explains the "one thing" as "one business reason to change". There is no universal definition of "one" for all code bases because the APIs we create work on different abstraction levels. So it all depends on who is the client of your class, and what changes they might require.
In your case in makes sense to say that the method is doing two things: it manages the contents of the bus and calculates the state. Thus it can have two reasons to change:
different business logic for adding passengers: for example one might wish to verify that there is enough place in the bus for yet another passenger and throw an exception if not
different business logic regarding the semantics of BusState
(simplest example: one might wish that full bus starts from 31 passengers, not 30)
In this context you could change addPassenger
to focus on adding only:
public void addPassenger(Passenger p){
this.passengerList.add(p);
}
and change the BusState
getter to perform calculations on demand (similarly to what Sweeper proposed in their answer):
public BusState getBusState() {
if (passengerList.size < 10)
return BusState.EMPTY;
else if (passengerList.size < 30)
return BusState.HALF_FULL;
else if (passengerList.size >= 30)
return BusState.FULL;
else
throw ...
}