"A method should do one thing, once and only once" - what does that mean?

Question

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?

https://stackoverflow.com/questions/46541197/does-the-single-responsibility-principle-work-in-oop/46556381#46556381 — Matt Timmermans, Apr 17 '18 at 12:40

score 4 · Answer 1 · answered Apr 17 '18 at 07:52

I would agree that addPassenger is doing more than 1 thing.

One way to make it do only one thing is to delete the state field and have a getState method that returns a state based on how many passengers there are (assuming you are writing in Java and BusState is an enum):

public BusState getState() {
    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
        return BusState.UNKNOWN; // somehow the no. of passengers is negative? You can consider throwing an exception here as well...
}

score 3 · Accepted Answer · answered Apr 17 '18 at 14:40

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 ...
}

"A method should do one thing, once and only once" - what does that mean?

2 Answers2