Cyclomatic complexity for method is too high

Question

How do i refactor my code to fix the lint error i am getting?

 def value=(val)
 if val
 if [:char].include?(@type) && val.length >= args[:limit] && val == 'true'
 val = 'Y'
 elsif [:char].include?(@type) && val.length >= args[:limit] && val == 'false'
 val = 'N'
 elsif [:bit].include?(@type) && val.length >= args[:limit] && val == 'true'
 val = 1
 elsif [:bit].include?(@type) && val.length >= args[:limit] && val == 'false'
 val = 0
 elsif [:varchar, :char, :nvarchar].include?(@type) && @args[:limit]
 unless val.length <= args[:limit]
 msg = "The length of #{@name} exceeds the max limit #{@args[:limit]}"
 raise Programmability::BadRequestError.new(nil, msg)
 end
 elsif [:numeric].include?(@type) && @args[:precision] && @args[:scale]
 valid_numeric?(val, @args[:precision], @args[:scale])
 elsif [:datetime].include?(@type)
 valid_date?(val)
 end
 end
 @value = val
 end

• Tolani: Does this code run as expected or is there a bug you are currently facing?
• User7354632781: It runs as expected. just trying to fix the lint error
• 200_success: The title is too common and generic for this site. Also, you haven't given us enough context about what this function aims to achieve for us to give you a good code review. Please read How to Ask and edit both the title and the body of the question.
• Marc Rohloff: You would probably need to break it up into methods, maybe convert_bool, convert_numeric, etc. The unless inside the if is probably what is causing the most issues so consider trying to remove that. Lastly I would write if @type==:char; val = convert_bool(val) ? 'Y' : 'N'

Answer 1

• [x].include?(y) is the most roundabout way I've ever seen of saying x == y.

• You're repeating val.length >= args[:limit] for almost every. single. case.

• Why is there an instance variable called @args? That smells like you didn't bother to actually handle things in the constructor, and instead just store everything for later. Also, why are you using @args directly, when it seems you also have a reader/accessor method, args? Using the accessor is better, though you can use either - just don't mix them.

• Why isn't there code after the calls to valid_numeric? and valid_date?? The prefix valid_... and the ? in the names indicate that those methods (should!) return boolean values. But if they do, you're ignoring it - which doesn't make sense. So it smells like those methods don't so much validate anything, but actually set @value to something. No thanks.

• You're overwriting the argument val, which isn't pretty. Make a new local variable instead, or write to @value directly.

• What if val is just false? Not the word, but the boolean? In that case all the logic will be skipped, and you'll go straight to @val = false. It doesn't matter whatever @type is, or whatever args[:limit] is. I think you want your if to check for val.nil? specifically.

It seems like what you really want is 4-6 different classes to handle different datatypes. Whatever you're trying to do, you're doing too much of it in one method and one class.

• User7354632781: I created a new method for val == 'true' and val == 'false' but new method also throws same error. please see updated question
• Flambino: @User7354632781 You'll want to log in as "jose" if you want edit your question. However, you cannot add code to your question once it's been answered; please see the help section. If you want to ask a follow-up question, just post a new one - it's encouraged!

Answer 2

Off course I think you should rewrite all the related code to make this method readable. But I can suggest such kind of refactor without rewriting related methods:

def value=(val)
 case @type
 when :char
 val = {'true' => 'Y', 'false' => 'N'}[val] if val.length >= args[:limit]
 when :bit
 val = {'true' => 1, 'false' => 0}[val] if val.length >= args[:limit]
 when :numeric
 valid_numeric?(val, @args[:precision], @args[:scale]) if @args[:precision] && @args[:scale]
 when :datetime
 valid_date?(val)
 else # [:varchar, :char, :nvarchar]
 if val.length > args[:limit]
 raise Programmability::BadRequestError.new(nil, "The length of #{@name} exceeds the max limit #{@args[:limit]}")
 end
 end
 @value = val
end

Btw I assume that methods valid_numeric? and valid_date? mutate argument passed to them, so they should be named like validate_numeric (maybe even process_numeric), and not mutate, but return resulted numeric. And I would off course rewrite it all to something like:

 def value=(val)
 @value = case @type
 when :char
 process_char(val)
 when :bit
 process_bit(val)
 when :numeric
 process_numeric(val)
 when :datetime
 process_date(val)
 else # [:varchar, :char, :nvarchar]
 process_other_chars(val)
 end
 end
 def process_char(char)
 {'true' => 'Y', 'false' => 'N'}[char] if char.length >= args[:limit] || char
 end
 def process_bit(bit)
 {'true' => 1, 'false' => 0}[bit] if bit.length >= args[:limit] || bit
 end
 def process_other_chars(char)
 if val.length > args[:limit]
 raise Programmability::BadRequestError.new(nil, "The length of #{@name} exceeds the max limit #{@args[:limit]}")
 end
 end
 def process_numeric(numeric)
 #check of @args[:precision] && @args[:scale] should be within method
 #...
 end
 def process_datetime(datetime)
 #...
 end