CodeWars Solution to "Simple Elevator"

Question

I am new to programming (less than a year), and I would like to improve.

I solved a problem on CodeWars and copied both my solution and the top voted solution below. I listed specific questions after my solution.

I would like to know, what would be considered the better overall solution in a real production environment? Is abstracting the way I did better (even if I did not execute correctly)? Could someone provide advice to help me improve?

Description:

There is a house with 4 levels. In that house there is an elevator. You can program this elevator to go up or down, depending on what button the user touches inside the elevator.

levels can be only numbers: 0,1,2,3

buttons can be strings: '0','1','2','3'

possible return values are numbers: -3,-2,-1,0,1,2,3

If the elevator is on the ground floor(0th level) and the user touches button '2' the elevator must go 2 levels up, so our function must return 2.

If the elevator is on the 3rd level and the user touches button '0' the elevator must go 3 levels down, so our function must return -3.

If the elevator is on the 2nd level, and the user touches button '2' the elevator must remain on the same level, so we return 0.

We cannot endanger the lives of our passengers, so if we get erronous inputs, our elevator must remain on the same level.

So for example:

• goto(2,'4') must return 0, because there is no button '4' in the elevator.
• goto(4,'0') must return 0, because there is no level 4.
• goto(3,undefined) must return 0.
• goto(undefined,'2') must return 0.
• goto([],'2') must return 0 because the type of the input level is array instead of a number.
• goto(3,{}) must return 0 because the type of the input button is object instead of a number.

Top Voted Solution

def goto(level, button)
  return 0 unless (0..3).include?(level) && ('0'..'3').include?(button)
  button.to_i - level
end

My Solution

def goto(level, button)
  inputs_types_are_accepted = (level.is_a? Integer) && (button.is_a? String)
  if inputs_types_are_accepted
    current_floor, desired_floor = level.to_i, button.to_i
    if current_floor.between?(0, 3) && desired_floor.between?(0, 3)
      floor_change = desired_floor - current_floor
      return floor_change
    else
      return 0
    end
  else
    return 0
  end
end

1. With a method this small, is it correct to abstract the way I did?
2. If I change if current_floor.between?(0, 3) && desired_floor.between?(0, 3) to if (current_floor && desired_floor).between?(0, 3) then I failed some tests. Is there a clearer way to represent this?
3. Is there a way to remove one set of else return 0 end? I tried and failed tests.

Answer 1

Your solution looks correct, though it is a bit long.

One way to shorten it is to remove the elses:

def goto(level, button)
  inputs_types_are_accepted = (level.is_a? Integer) && (button.is_a? String)
  if inputs_types_are_accepted
    current_floor, desired_floor = level.to_i, button.to_i
    if current_floor.between?(0, 3) && desired_floor.between?(0, 3)
      floor_change = desired_floor - current_floor
      return floor_change
    end
  end
  # Don't move elevator if the input is wrong in any way
  0
end

You don't need to do level.to_i when you have already ascertained that level is an Integer. You can also eliminate the variables inputs_types_are_accepted and floor_change.

(current_floor && desired_floor) won't work. In English, you can ask whether "the current floor and desired floor are both between 0 and 3", but you can't translate that literally into Ruby. Since you want to perform two tests, you have to write it as two tests. The way the && operator works is that it will return the left operand (current_floor) if it is considered true — and in Ruby, every integer is considered to be true.

One thing that I like about your solution and that I dislike about the "top-voted" solution is that the latter does not extend well to handle more than 10 floors. Although the problem states that there are just 4 levels, but still, using ('0'..'3').include?(button) to validate the floor in the string domain rather than the integer domain feels like a hack to me.

Obviously, there are many possible formulations. It's a new moon today, so I feel like writing it this way:

def goto(level, button)
  floors = (0..3)

  # Validation: don't move elevator on invalid input
  return 0 unless button.kind_of?(String) && floors === (dest = button.to_i)
  return 0 unless floors === level

  dest - level
end

The way you wrote your solution tends to lead to deeper nesting. As a rule of thumb, it is preferable to return early on failure, since there are usually many ways to fail, but only one way to succeed.