Toggling a taxi between busy and available states

Question

Is there a better way to do this? That switch_status method seems like it could be made a lot more compact.

class Taxi
  def initialize
    @status = :available
  end  

  def switch_status
    if @status == :available
      @status = :busy
    else
      @status = :available
    end
  end
end

t = Taxi.new
t.switch_status

Answer 1

I voted to reopen, though I still don't consider this a good CodeReview question. Perhaps my answer here will shed some light on why I think that.

You say you want to shorten the switch_status method. Well, Ruby has ternaries, same as many other languages so:

def switch_status
  @status = (@status == :available ? :busy : :available)
end

And done. Shorter.

But the reason this question feels incomplete is because that, in and of itself, is completely equivalent to the original code. So it's not really a review, just a demonstration of an alternative if..else syntax found in many languages.

A review would require knowing more about why this method exists. What its purpose is, when you need it, why it's using symbols, and so on.

To start, just having the switch_state method is odd. You presumably have to know what state the taxi (as the class name suggests it is) is already in, in order to see if switching it makes sense. But your code contains no mechanism for determining the current state. In fact, the only way to get a Taxi instance to return something is to change its state. Calling switch_state will return the new state, and you'll have to work backwards from there or switch it back again.

E.g. suppose you have an instance, and want to know its state. Currently, you'd have to do this:

some_taxi.switch_state # flip it once
current_state = some_taxi.switch_state # flip it back again

That makes very little sense.

Also: You're switching between exactly two states - which is what booleans do already. So why aren't you using booleans? They're limited to two states, so you're certain it's either one or the other (whereas a symbol can be anything). So you could just as well do this:

def switch_status
  @busy = !@busy
end

Really, that should do the same trick, since this taxi has only two states.

And also, given the lack of context, I'd say you're entire class could, in effect, be replaced with just a boolean, since the class's only purpose right now is to store a boolean state. So cut out the middleman. A raw boolean value would even tell you its current state, which is more than your class does now.

But presumably you want the encapsulation and the symbols for something. Or do you want symbols? A more Ruby'esque way to inquire about a known set of states would be with ?-methods:

def busy?
  @busy
end

def available?
  !@busy
end

Or you can just have one of 'em and negate their value, e.g. !taxi.busy?. Either way, it's infinitely better than flipping the state twice just to find out what it currently is.

If you still want symbols, write an accessor:

def status
  @busy ? :busy : :available
end

Then again, if this is a taxi, does it really have only those two states? Maybe one is :out_of_service for instance?

Again, no idea. The question doesn't say.

Or it could be that available/busy can be - and likely should be - inferred from something else. There's no reason to manually track multiple separate state values if they're dependent on each other. In fact, it's likely harmful to try. So perhaps your code could do this instead:

def status
  @passengers.any? ? :busy : :available
end

Now status is derived from something (presumably) more meaningful.

But does this make sense? No idea. The question doesn't say.

As mentioned, just having switch_state is odd because presumably there's more to it than that: The state is connected to something. Otherwise, as I also mentioned, you could replace the whole class with a simple boolean value. Only being able to manipulate the state by flipping it seems weird. Like a light switch you can only even flip to the opposite of whatever it is - and you don't know whether that's on or off.

Answer 2

This takes the Taxi class in a different direction a little bit, but I think a switch_status method is not what you want. The status of a Taxi is a reflection of its current state. Code outside the Taxi class should not be allowed to switch the status.

• A taxi is available if it has no passengers
• A taxi is "busy" if it is currently on a trip

Seems like you need a begin_trip method that accepts a destination and any number of passengers, and a complete_trip method that returns the array of passenger objects and makes the taxi available again.

class Taxi
  def initialize
    @status = :available
  end

  def available?
    @status == :available
  end

  def begin_trip(destination, *passengers)
    raise 'Cannot accept passengers at this time' unless @passengers.nil?
    @passengers = passengers
    @status = :busy
  end

  def complete_trip
    passengers = @passengers
    @passengers = nil
    @status = :available

    passengers
  end
end

The taxi's availability is a reflection of how the object is currently being used:

taxi = Taxi.new
taxi.available? # => true
taxi.begin_trip 'The Grand Canyon', 'Bill', 'Janet'
taxi.available? # => false
passengers = taxi.complete_trip
taxi.available? # => true