CODEWITHSUNDEEP
oopmethodscoding-style
Mapi
Mapi

Reputation: 149

"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?

Upvotes: 2

Views: 2419

Answers (3)

TobadiahStane
TobadiahStane

Reputation: 1

The previous answers are useful, but I'll add some more for those who find this later.

The correct way to solve the problem is exactly what the question poster did. The only other thing I would do is changing the name of the function from addPassenger to addPassengerToBus (or boardBus).

Adding a passenger to the bus should change the fullness of the bus. You would never want to have someone added to the bus without calculating the change in bus fullness. From the "business" perspective (the business of busses) it is one thing.

What is not one thing, is the exact numerical tipping points, which should be calculated via it's own function and called whenever a passenger is added.

Had adding people to the bus truly been independent of calculating the fullness of the bus, then the thing to do would be to split the functions and have the method which calls addPassenger now call both methods directly.

Upvotes: 0

BartoszKP
BartoszKP

Reputation: 35911

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

Upvotes: 3

Sweeper
Sweeper

Reputation: 273540

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

Upvotes: 4

Related Questions