Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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"

share|improve this question
2  
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 at 9:50
1  
I have edited question and make it more context free –  uzzer Feb 20 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 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 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 at 20:21
add comment

3 Answers

up vote 3 down vote accepted

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
share|improve this answer
    
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 at 12:30
add comment

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
share|improve this answer
add comment

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
share|improve this answer
    
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 at 10:23
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.