9

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
Ramon Marques
  • 3,046
  • 2
  • 23
  • 34
  • 1
    Absent any context, it's difficult to give advice here. How I would approach this depends a lot on the objects in question and how they relate. What is the `self` object for this code? Why is it calling `Budget::util_name` for everything? Is this a standalone naming method or part of a larger piece of logic? – Adam Lassek Jun 10 '17 at 02:36
  • hey @AdamLassek, thank you for this. I edit my question to explain the util method, your other questions I don't get it really, or if I have I would have to explain a lot of this app – Ramon Marques Jun 10 '17 at 02:53
  • this piece of code is for a pdf report, if it's help, for contexting – Ramon Marques Jun 10 '17 at 02:56
  • Possible duplicate of [Breaking up long strings on multiple lines in Ruby without stripping newlines](https://stackoverflow.com/questions/10522414/breaking-up-long-strings-on-multiple-lines-in-ruby-without-stripping-newlines) – Jon Schneider Dec 15 '18 at 04:18

3 Answers3

17

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

Sujan Adiga
  • 1,331
  • 1
  • 11
  • 20
  • 2
    A new line for every Budget might be even more readable, depending on your preference – Rots Jun 10 '17 at 12:15
  • accepted since it's the way ruby-style-guide uses for long string https://github.com/bbatsov/ruby-style-guide#no-trailing-backslash – Ramon Marques Jun 12 '17 at 11:01
  • Nice use of the little-known trick of combining multiple strings. Although given the extra info added to the question, you can remove `Budget::util_name` completely and it will do the same thing. – Adam Lassek Jun 13 '17 at 02:46
  • No I can't. Explained why below your answer – Ramon Marques Jun 13 '17 at 11:05
7

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.

Graham
  • 7,431
  • 18
  • 59
  • 84
Kathryn
  • 1,557
  • 8
  • 12
  • It's a really good one for me since I have to call util_name() method lot of times. But accordingly to ruby-style-guide "Prefer string interpolation and string formatting instead of string concatenation" https://github.com/bbatsov/ruby-style-guide#string-interpolation – Ramon Marques Jun 12 '17 at 11:00
  • Given the additional context, it's clear that `Budget::util_name` doesn't need to exist at all, and bringing `Enumerable` into this just adds needless complexity, IMO. – Adam Lassek Jun 13 '17 at 02:44
  • 1
    Nice solution, especially #2. – Cary Swoveland Jun 13 '17 at 07:41
  • 1
    Seems like the `map` could actually be `map{ |x| x || "" }`, or it could be replaced with `compact` to remove the nils. – David Aldridge Jun 13 '17 at 08:02
  • AdamLassek and @DavidAldridge that would be nice to remove that method if it was possible, unfortunately it's not possible explained in my comment for Adam's answer #comment-76030950 – Ramon Marques Jun 13 '17 at 11:08
  • @RamonMarques is correct, I missed an important bit about the sample -- however my assessment here still stands. I've updated mine to account for the method call. – Adam Lassek Jun 13 '17 at 22:10
1

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.

Adam Lassek
  • 35,156
  • 14
  • 91
  • 107
  • Thanks for the comment on my since-deleted answer. What was I thinking? – Cary Swoveland Jun 13 '17 at 07:45
  • @AdamLassek thank you for your new and nice answer. Let me clarify something here. actually the util static method can't be removed because it's not just a "print blank method". I verify if a class 'not an attribute' is nil, then I print the 'name' attribute, that this time can be nil and the interpolation will call to_s automatically. Edit your answer if you agree with me, otherwise let me know what I'm not getting here. – Ramon Marques Jun 13 '17 at 11:00
  • Your code wouldn't work because you are never calling .name property in any of them, see? – Ramon Marques Jun 13 '17 at 11:10
  • I believe that you thought those envelope_size, _paper _color and so on were attributes because of its names, I know they are bad. Unfortunately that code is from a legacy app that I'm giving maintenance... – Ramon Marques Jun 13 '17 at 11:12
  • @RamonMarques you're right, I missed the call to `.name` in there. Still unnecessary, but I'll update to reflect that behavior. – Adam Lassek Jun 13 '17 at 21:58
  • @AdamLassek nice, I didn't know those methods, I'm going to start to use ^^ thanks. – Ramon Marques Jun 14 '17 at 02:05