[openstreetmap/openstreetmap-website] Prefer string interpolation to concatenation (#2921)

Andy Allan notifications at github.com
Wed Nov 11 15:31:21 UTC 2020


@gravitystorm commented on this pull request.



> @@ -6,7 +6,7 @@ def self.coder
   def set_title(title = nil)
     if title
       @title = TitleHelper.coder.decode(title.gsub("<bdi>", "\u202a").gsub("</bdi>", "\u202c"))
-      response.headers["X-Page-Title"] = ERB::Util.u(@title + " | " + t("layouts.project_name.title"))
+      response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")

> my natural assumption would be that concatenation would generally be higher performance

I think concatenation is generally slower, since for `string1 + string2 + string3` you end up needing even more strings, since each `+` allocates a new string to hold the result of adding the two together, but...

> not that it matters most of the time

Indeed! :smile: So I'm much more interested in the consistency, and developer experience, unless there's some catastrophic performance difference.

> it might all become a bit unreadable.

Yeah, it's certainly not straightforward to read. My local editor has different syntax highlighting than on github, and it's definitely easier on the eye. So overall, I'm still happy with the PR, but also happy to come up with an alternative approach.

I thought I'd try out the format() approach that you suggested, e.g.

```diff
- response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")
+ response.headers["X-Page-Title"] = ERB::Util.u(format("%s | %s", @title, t('layouts.project_name.title')))
```
... but that triggers another rubocop warning about labelling the string tokens in the format string. And with labelled tokens, we need to supply a hash of values. So in the end we get something like this:

```diff
- response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")
+ response.headers["X-Page-Title"] = ERB::Util.u(format("%<title>s | %<project_title>s", :title => @title, :project_title => t("layouts.project_name.title")))
```
I find this even harder to read, since the format string `"%<title>s | %<project_title>s"` looks sort of but not quite like an interpolated string e.g. `"#{title} | #{project_title}"`, and the line ends up being quite long too.

However, one final option is then to use an interpolated string, but to avoid putting complex stuff inside it, by unrolling it slightly e.g.

```diff
- response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")
+ project_title = t 'layouts.project_name.title'
+ response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{project_title}")
```

What do you think?




-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2921#discussion_r521439457
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20201111/4868b3a7/attachment-0001.htm>


More information about the rails-dev mailing list