Reduce cyclomatic complexity

How can I reduce cyclomatic complexity of validation and refactor it better way?

Class Document
def initialize(opts)
@opts = opts
return ArgumentError unless valid?
end

def valid?
return false if @opts.nil?
return false if @opts[:property_1].nil?
return false if @opts[:property_2].nil?
return false if !@opts[:property_4].nil? && @opts[:property_3].nil?
# etc. 12 times more, otherwise valid will return true
true
end

def usage_example
if @opts[:property_3]
"#{@opts[:property_3]} #{@opts[:property_1]}"
else
"#{@opts[:property_4]} #{@opts[:property_2]}"
end
end
end


I am trying to avoid duplication of return false if and somehow perhaps list parameters that I want to verify

Possible interpretation of what I trying to reach is "verify set of rules before processing file"

-
Welcome to Code Review. We can usually give better advice for you if you provide real code from an actual project that you are working on, without stripping out the context. As it stands, you have provided barely enough code to constitute a question for Code Review. I encourage you to edit this question to add more detail about what you are trying to accomplish. – 200_success Feb 20 '14 at 9:50
I have edited question and make it more context free – uzzer Feb 20 '14 at 10:27
@uzzer will you please extend the example? For instance, what type of class is this method contained in? I ask this because in my experience the problem caused by "duplication of return false if" is a symptom of a holistic design issue. And also the complexity of the solution depends on the scope of the project. – natedavisolds Feb 23 '14 at 23:37
I have extended the example little bit. You are probably right about design issue and I would be glad to fix it. Idea behind design is to pass freeform @opts and verify that their body meets business rules before processing, perhaps I have to avoid it and verify presence of data only when data is needed? – uzzer Feb 25 '14 at 8:40
If you are satisfied with any of the answers, you should select the one that you find most helpful. – Cary Swoveland Feb 27 '14 at 20:21

If your tests are about mandatory members in @opts you can do this:

MandatoryProperties = [:property_1, :property_2, ...]

def valid?
@opts && MandatoryProperties.all? { |k| !@opts[k].nil? }
end

-
It is definitely right direction. Currently I will try to extend this to support nil and !a && b case using array of conditions or something like this – uzzer Feb 20 '14 at 12:30

There are a few things you can do here to improve this code, but with any sort of refactoring or any attempt of reducing complexity, we have to evaluate why. It may very well be the case that this method isn't at fault for its own complexity, but is this complex due to a failure elsewhere in the system of keeping their data correct.

That aside, there are still a few things you could do to clean up this code a little

Explicitly define all the required parameters and use that to iterate over.

REQUIRED_PROPERTIES = %i(prop_1 prop_2 prop_3 prop_4)
REQUIRED_PROPERTIES.any? { |x| @opts[x].nil? } #=> true if any are nil or false if all are not nil


Use hash#fetch instead of [] so you can supply a default value for possible missing keys

return false if @opts[:property_1].nil?
return @opts.fetch(:property_1, false) #only return false if :property_1 is not defined
#fetch only returns false if :property_1 doesn't exist, not if it has been set to nil


Remember that in ruby nil and false are both false-y

def valid?
@opts && @opts[:property_1] && @opts[:property_2]
#will return true only if @opts and properties are non nil values
end

-

You could write that as

def valid?
@opts && !@opts[:property].nil? ? true : false
end


but usually it would be good enough to return a truthy or falsy value instead of a Boolean:

def valid?
@opts && !@opts[:property].nil?
end

-
I mentioned ... meaning there is about 12 different parameters and code becomes overloaded with return false if opts[:one_of_properties].nil? – uzzer Feb 20 '14 at 10:23