def display_title
if @title.blank?
APP_CONFIG[:site][:title]
else
APP_CONFIG[:site][:title] + " - #{@title}"
end
end
Refactorings
No refactoring yet !
Aaron Tinio
December 2, 2010, December 02, 2010 06:31, permalink
def display_title
APP_CONFIG[:site][:title] + (@title.blank? ? '' : " - #{@title}")
end
# or:
def display_title
APP_CONFIG[:site][:title] + (" - #{@title}" unless @title.blank?).to_s
end
Aaron Tinio
December 2, 2010, December 02, 2010 06:32, permalink
def display_title
APP_CONFIG[:site][:title] + (@title.blank? ? '' : " - #{@title}")
end
# or:
def display_title
APP_CONFIG[:site][:title] + (" - #{@title}" unless @title.blank?).to_s
end
Marc
December 2, 2010, December 02, 2010 07:57, permalink
def display_title
[APP_CONFIG[:site][:title], @title].compact.join(' -')
end
TimK
December 2, 2010, December 02, 2010 13:37, permalink
Aaron, I really like both of your suggestions. Is there a reason that your second example requires to_s? Aren't you essentially making it a string by having it in ""?
Marc, your example is awesome too. It took me a minute to see why that would work conditionally - my brain was reading right over the compact call.
steved
December 2, 2010, December 02, 2010 18:01, permalink
TimK: I think this site often views fewest lines of code as the goal. I think your original example, while five lines instead of one, is easier to read/understand than any of the refactorings.
Here's a non-repeating version that is still easy to read:
def display_title
result = APP_CONFIG[:site][:title]
result << " - #{@title}" if @title.present?
result
end
TimK
December 2, 2010, December 02, 2010 22:40, permalink
I agree Steve, there has to be a mix between readability and leanness. I really like your example, that was actually what I was trying to conceive in my head but for some reason I dismissed << as appending the item. Thanks!
Aaron Tinio
December 3, 2010, December 03, 2010 00:12, permalink
TimK, if @title is blank, the expression inside the parenthesis returns nil, hence the to_s, which does nothing if @title is not blank.
TimK
December 3, 2010, December 03, 2010 01:37, permalink
Aaron, thank you for explaining - I see what you mean.
Adam
December 4, 2010, December 04, 2010 07:35, permalink
I kind of like the explicitness of the original. @steved watch for the mutation of the stored title in your refactoring, otherwise some unexpected results may occur on subsequent calls to the method.
steved
December 4, 2010, December 04, 2010 20:43, permalink
@Adam: "mutation of the stored title": are you referring to @title or result? In either case I'm not following your concern.
James Ferguson
December 12, 2010, December 12, 2010 22:34, permalink
@steved:
You shouldn't need line 4, it'd be returned from the previous line.
I think Adam was saying that result is just a reference to APP_CONFIG[:site][:title] and << is a destructive call so your method would have the side effect of changing APP_CONFIG[:site][:title].
Try this in irb:
>> foo = "title"
=> "title"
>> bar = foo
=> "title"
>> bar << "stuff"
=> "titlestuff"
>> bar
=> "titlestuff"
>> foo
=> "titlestuff"
I have a helper that appends the title of a page if a variable exists. I hate that it repeats, but can't think of a cleaner way to approach it. Any thoughts?