0
\$\begingroup\$

I have this, I guess, new Ruby syntax in my method but Rubocop are warning me that the second last line is too long. Could you please help me to refactor this method?

def show
  identification_document = IdentificationDocument.find(params[:id])
  authorize identification_document
  return unless identification_document

  #this line below is to too long
  document = params[:size] == 'resized' ? identification_document.id_document_resized : identification_document.id_document
  send_data(document.file.read, filename: identification_document.file_name)
end
\$\endgroup\$
  • \$\begingroup\$ What does your IdentificationDocument class look like? \$\endgroup\$ – 200_success Jul 21 at 0:27
  • \$\begingroup\$ It's a model, you think there is some other space to improve than @dfhwze provide? \$\endgroup\$ – mr_muscle Jul 21 at 0:31
1
\$\begingroup\$

Guidelines Rubocop

The guidelines favor if/case over multi-line ternary operator when a line is too long.

  • Maximum Line Length Limit lines to 80 characters.

  • No Multi-line Ternary Avoid multi-line ?: (the ternary operator); use if/unless instead.

  • Use if/case Returns Leverage the fact that if and case are expressions which return a result.

Refactored Code

def show
  identification_document = IdentificationDocument.find(params[:id])
  authorize identification_document
  return unless identification_document

  document =
    if params[:size] == 'resized'
       identification_document.id_document_resized
    else
       identification_document.id_document
    end

  send_data(document.file.read, filename: identification_document.file_name)
end
\$\endgroup\$
1
\$\begingroup\$

Rubocop warns about lines that are over 80 characters long, its too easy to not notice code that is hiding off to the right hand side of the screen.

Apart from dfhwze's suggestion it might be worth modifying your model code to take a resize parameter, something like:

class IdentificationDocument
  def id_document(resized: false)
    ...
  end

And in your controller

  #this line below is to too long
  document = identification_document.id_document(resized: params[:size])

Another alternative is just to use a shorter variable name and/or use an intermediate variable for params[:size]. i.e.

def show
  id_doc = IdentificationDocument.find(params[:id])
  authorize identification_document
  return unless id_doc

  #this line below is to too long
  resize = params[:size] == 'resized'
  doc    = resize ? id_doc.id_document_resized : id_doc.id_document
  send_data(doc.file.read, filename: id_doc.file_name)
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.