CODEWITHSUNDEEP
ruby-on-railsrubyruby-on-rails-4ruby-style-guide
Ramon Marques
Ramon Marques

Reputation: 3264

What is the best way to refactor a long string line in ruby?

I have a line of code like this

"#{envelope_quantity} - envelope #{Budget::util_name(envelope_size)} #{Budget::util_name(envelope_paper)} #{Budget::util_name(envelope_color)} #{Budget::util_name(envelope_grammage)} #{Budget::util_name(envelope_model)} #{Budget::util_name(envelope_print)}"

too long, it's bad to read, and that's why RuboCop is warning me with it's Metrics::LineLength.

I would like to refactor it to not be a long line.

I know a lot of ways to do that, but I wonder which one would be the expected for the ruby style experts.

that static method util_name is needed to prevent nil when I need an empty string if it's nil.

def self.util_name(value)
  return '' if value.nil?
  value.name
end

Upvotes: 9

Views: 9095

Answers (3)

Kathryn
Kathryn

Reputation: 1607

One thing you might try is to not use string interpolation, instead construct the string using concatenation and join:

"#{envelope_quantity} - envelope " + 
[Budget::util_name(envelope_size), 
 Budget::util_name(envelope_paper),
 Budget::util_name(envelope_color),
 Budget::util_name(envelope_grammage),
 Budget::util_name(envelope_model),
 Budget::util_name(envelope_print)].join(' ')

Even more concisely, you could use map:

"#{envelope_quantity} - envelope " + 
[envelope_size, 
 envelope_paper,
 envelope_color,
 envelope_grammage,
 envelope_model,
 envelope_print].map{|x| Budget::util_name(x)}.join(' ')

This might be made more concise by defining an array with all of the envelope properties in the right order and applying map to that:

envelope_properties=[envelope_size, 
                     envelope_paper,
                     envelope_color,
                     envelope_grammage,
                     envelope_model,
                     envelope_print]

"#{envelope_quantity} - envelope " + 
envelope_properties.map{|x| Budget::util_name(x)}.join(' ')

Of course, it would help if you happened to have another use for the envelope_properties array.

Upvotes: 7

Adam Lassek
Adam Lassek

Reputation: 35515

that static method util_name is needed to prevent nil when I need an empty string if it's nil. 

def self.util_name(value)
  return '' if value.nil?
  value.name
end

Okay, given that bit of context you can remove the Budget::util_name method entirely because it isn't doing anything useful. There are two ways of conditionally calling a method on an object that might be nil, one provided by the framework and one by the language.

If you are using Ruby 2.2 or earlier, use the try method.

value.try(:name)

if you are using Ruby 2.3 or above, you can use the safe navigation operator

value&.name

In either case, you don't need to specifically test for nil because it will be automatically coerced into an empty string when interpolated.

"#{envelope_quantity&.name} - envelope #{envelope_size&.name} #{envelope_paper&.name} #{envelope_color&.name} #{envelope_grammage&.name} #{envelope_model&.name} #{envelope_print&.name}"

That's much more reasonable, still probably a bit too long. You could use string templates:

"%{quantity} - envelope %{size} %{paper} %{color} %{grammage} %{model} %{print}" % {
  quantity: envelope_quantity&.name,
  size:     envelope_size&.name,
  paper:    envelope_paper&.name,
  color:    envelope_color&.name,
  grammage: envelope_grammage&.name,
  model:    envelope_model&.name,
  print:    envelope_print&.name
}

But I want to focus on something I noticed about this code sample. Every single method begins with envelope which probably means that these methods are telling you they should be a separate object. If you extract this data into a value object, then the natural location for this helper method becomes obvious...

class Envelope < Struct.new(:quantity, :size, :paper, :color, :grammage, :model, :print)
  def to_s
    "#{quantity&.name} - envelope #{size&.name} #{paper&.name} #{color&.name} #{grammage&.name} #{model&.name} #{print&.name}"
  end
end

Undoubtedly the real code would be more complicated than that, just food for thought.

Upvotes: 1

Sujan Adiga
Sujan Adiga

Reputation: 1371

You can try this

str = "#{envelope_quantity} - envelope #{Budget::util_name(envelope_size)} "\
      "#{Budget::util_name(envelope_paper)} #{Budget::util_name(envelope_color)} "\
      "#{Budget::util_name(envelope_grammage)} #{Budget::util_name(envelope_model)} "\
      "#{Budget::util_name(envelope_print)}"

this way you will be able to confine the string within max line length and also its slightly more readable than using join

Upvotes: 17

Related Questions