How would I refactor these four `unless` statements?

Let’s say I have the following four unless statements in a row. At
first I thought about using if and elsif statements and negate the
condition but I’m not sure which way is suitable/readable/good-practice.

unless @params.include?(:js_code) || @params.include?(:code_url)
raise BadParameters, ‘You must include at least one…’
end
unless @params.include?(:compilation_level)
raise BadParameters, ‘The :compilation_level…’
end
unless @params.include?(:output_format)
raise BadParameters, ‘The :output_format…’
end
unless @params.include?(:output_info)
raise BadParameters, ‘The :output_info…’
end

Thanks in advance!

On 06/11/2013 12:38 PM, Rafal C. wrote:

unless @params.include?(:output_format)
raise BadParameters, ‘The :output_format…’
end
unless @params.include?(:output_info)
raise BadParameters, ‘The :output_info…’
end

Thanks in advance!

For the conditions that are really similar, you could do something like:

missing_keys = [:compilation_level, :output_format, :output_info] -
@params.keys

unless missing_keys.empty?
raise BadParameters, “Missing keys: #{missing_keys.inspect}”
end

-Justin

Justin C. wrote in post #1112097:

missing_keys = [:compilation_level, :output_format, :output_info] -
@params.keys

unless missing_keys.empty?
raise BadParameters, “Missing keys: #{missing_keys.inspect}”
end

Thanks Justin.

If you’re using Ruby 2.0 then you have the option of methods with
keyword arguments, which would handle that kind of thing for you.

On Tue, Jun 11, 2013 at 9:38 PM, Rafal C. [email protected] wrote:

unless @params.include?(:output_format)
raise BadParameters, ‘The :output_format…’
end
unless @params.include?(:output_info)
raise BadParameters, ‘The :output_info…’
end

As one can see there are many options. Here’s another one

MANDATORY = [:js_code, :code_url, …]

missing = MANDATORY.reject do |key|
@params.include? key
end

raise BadParameters, "These parameters are missing: #{missing.join ",
“}”
unless missing.empty?

Cheers

robert

Rafal C. wrote in post #1112088:

Let’s say I have the following four unless statements in a row. At
first I thought about using if and elsif statements and negate the
condition but I’m not sure which way is suitable/readable/good-practice.

unless @params.include?(:js_code) || @params.include?(:code_url)
raise BadParameters, ‘You must include at least one…’
end
unless @params.include?(:compilation_level)
raise BadParameters, ‘The :compilation_level…’
end
unless @params.include?(:output_format)
raise BadParameters, ‘The :output_format…’
end
unless @params.include?(:output_info)
raise BadParameters, ‘The :output_info…’
end

Thanks in advance!

I think they’re fine. When it comes to logical statements for error
handling, keeping it simple, systematic, and easy to understand is a
good thing. Building some larger, more complex expressions just to make
the code shorter will only obfuscate it and require a reader to do more
interpretation in the head to make sense of it.
Possibly, you can define a helper method for the simple cases like this:

def required(name, message)
unless @params.include?(name)
raise BadParameters, message
end
end

and turn three of them into:

required(:compilation_level, ‘The :compilation_level…’)
required(:output_format, ‘The :output_format…’)
required(:output_info, ‘The :output_info…’)

If the messages are completely uniform (except for the parameter name of
course), you can also do it this way:

def required(names)
names.each do |name|
unless @params.include?(name)
raise BadParameters, “The #{name} …”
end
end
end

required([:compilation_level, :output_format, :output_info])