0
\$\begingroup\$

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

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

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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