oopmethodscoding-style

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


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?


Solution

  • 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:

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