Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

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

  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

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

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
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 – Mihhail Sidorin 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? – Mihhail Sidorin 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
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? }
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 – Mihhail Sidorin 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
share|improve this answer

You could write that as

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

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

def valid?
  @opts && !@opts[:property].nil?
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? – Mihhail Sidorin Feb 20 '14 at 10:23

Your Answer


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.