Cyclomatic complexity for method is too high

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]}"
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

• Does this code run as expected or is there a bug you are currently facing? – Siobhan Mar 28 '17 at 21:41
• It runs as expected. just trying to fix the lint error – User7354632781 Mar 28 '17 at 21:45
• 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. – 200_success Mar 28 '17 at 22:10
• 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' – Marc Rohloff Mar 29 '17 at 8:10

• [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.

• I created a new method for val == 'true' and val == 'false' but new method also throws same error. please see updated question – User7354632781 Mar 29 '17 at 0:02
• @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! – Flambino Mar 29 '17 at 0:13

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