Ruby Method Return Best Practice

Question

I have a method that looks at variables and returns depending upon what those variables are:

def os_detect(os, zone)
  if os.start_with?('centos')
      unless zone == 'aws'
        if os == 'centos7'
            #puts "Not available yet"
            return nil
        end
      end
  else
      #puts "Invalid OS Type"
      return nil
  end
  return true
end

I'm building for CLI and Rails so I'm trying to get rid of puts...but I still want to explain to the user what's going on.

Should I just return the strings and true if nothing is blocking?

Is there a better way to handle this type of logic in Ruby?

UPDATE : The main purpose of my question was to understand how to pass context to a user based on the logic.

So this method returns false for centos7 on aws, or ANY other OS besides centos*.

How does the user know what's wrong and how to fix it?

Answer 1

I'll try to answer this more abstractly. The way I see it, you are asking two things:

1. How to simplify boolean expressions?
2. What syntactical construction to use so that you can add some additional side effects (in this case printing)?

How to simplify boolean expressions?

Just translate the English sentence to Ruby. There is usually 1:1 mapping. The oposite is also true - you can read your Ruby code as an English sentence to hear if it actually sounds understandable. First, lets read your current implementation:

def os_detect(os, zone)
  if os.start_with?('centos')
    unless zone == 'aws'
      if os == 'centos7'
        return nil
      end
    end
  else
    return nil
  end
  return true
end

If the OS is centos, unless on aws if the OS is centos7 this feature is not available, otherwise - your OS is considered invalid. If not - we do support this environment.

Even with punctuation (or indentation in the code's case) you have to stop and think for a bit to extract the essence of it. Yes, code unlike English has the perk of being unambiguous, but this does not make it understandable.

Now lets try to translate English to Ruby. In your own words:

This method returns false for centos7 on aws, or ANY other OS besides centos

def os_detect(os, zone) # "this method"
  return false if os =='centos7' && zone == 'aws' || !os.start_with?('centos')
end

Now you might see that your description didn't match what you actually wanted. That is because what you actually wanted to say was:

This method determines whether or not we run centos7 on aws or any other OS besides centos

def os_detect(os, zone) # "this method"
  os =='centos7' && zone != 'aws' || !os.start_with?('centos')
end

---

What syntactical construction to use so that you can add some aditional side effects (in this case printing)?

That again depends on how you want to structure your sentence. Mainly you will be using either guard conditions before the final return or prints in the actual conditions (similarly to what you did, but readable). Lets try:

This method says that the feature is not yet available if checking for centos7 not on aws. It says that the target OS is invalid unless checking for centos. Finally, it determines whether or not we run centos7 not on aws or any other OS besides centos.

def os_detect(os, zone) # "this method"
  puts 'Not available yet' if os == 'centos7' and zone != 'aws'
  puts 'Invalid OS Type' unless os.start_with?('centos')

  os =='centos7' && zone != 'aws' || !os.start_with?('centos')
end

This method says that the feature is not yet available and exits if checking for centos7 not on aws. It says that the target OS is invalid and exists unless checking for centos. If gives the green light otherwise.

def os_detect(os, zone) # "this method"
  if os == 'centos7' and zone != 'aws'
    puts 'Not available yet'
    return
  end

  unless os.start_with?('centos')
    puts 'Invalid OS Type'
    return
  end

  true
end

Answer 2

Solution

Just booleans

If I understand your code correctly, every 'centos' is supported except 'centos7' in zone outside 'aws'.

def os_supported?(os, zone = '')
  os.start_with?('centos') && (zone == 'aws' || os != 'centos7')
end

Booleans and warning messages

It might be a good idea to use 2 methods : one returns a warning when needed, the other returns nil or true.

def os_support_warning(os, zone = '')
  return 'Invalid OS Type'   unless os.start_with?('centos')
  return 'Not available yet' if os == 'centos7' && zone != 'aws'
end

def os_supported?(os, zone = '')
  warning = os_support_warning(os, zone)
  if warning
    puts warning
  else
    true
  end
end

If you accept getting true/false instead of true/nil, you could use :

def os_supported?(os, zone = '')
  warning = os_support_warning(os, zone)
  puts warning if warning
  warning.nil?
end

Both false and nil are falsey : for boolean logic, they're perfectly equivalent.

Test

To check that our methods are equivalent, I use :

%w(centos6 centos7 windows macosx).each do |os|
  %w(aws not_aws).each do |zone|
    @os = os
    @zone = zone
    puts '--------------------------'
    puts "OS : #{os}\t Zone : #{zone}"
    puts "#######BAD#######" if os_detect != os_supported?(os, zone) 
  end
end

It outputs

--------------------------
OS : centos6     Zone : aws
--------------------------
OS : centos6     Zone : not_aws
--------------------------
OS : centos7     Zone : aws
--------------------------
OS : centos7     Zone : not_aws
Not available yet
Not available yet
--------------------------
OS : windows     Zone : aws
Invalid OS Type
Invalid OS Type
--------------------------
OS : windows     Zone : not_aws
Invalid OS Type
Invalid OS Type
--------------------------
OS : macosx  Zone : aws
Invalid OS Type
Invalid OS Type
--------------------------
OS : macosx  Zone : not_aws
Invalid OS Type
Invalid OS Type

They all return the same boolean with the same warning.