more conventional way to write this ruby

Question

this ruby code works, but is there a more conventional or simplified way to write it:

options['host'] = begin
  a == :jaxon ? 'jaxon-server16.jaxon.local' : 'doric-server5'
end

I just feel like the code is a smell but I cannot put my finger on it.

Thanks.

@BorisStitnicky: I'd say, though, that this usage of `begin..end` is not very widespread. — Sergio Tulentsev, May 27 '13 at 10:10

score 6 · Answer 1 · answered May 27 '13 at 08:40

6

You don't need begin..end here.

options['host'] = a == :jaxon ? 'jaxon-server16.jaxon.local' : 'doric-server5'

I'd probably put parentheses around right side. Not necessary, just for clarity.

options['host'] = (a == :jaxon ? 'jaxon-server16.jaxon.local' : 'doric-server5')

answered May 27 '13 at 08:40

Sergio Tulentsev

226,338
43
373
367

With all due respect, I found other worthy answers by Sergio to upvote, but I will neither upvote nor downvote this one, as the `begin ... end` block somehow appeals to me, the old fan of Pascal lanuage, better than the sequence `... = a == :jaxon ? ...` Parenthesis does clarify it, though `begin ... end` has still its magic to it. – Boris Stitnicky May 27 '13 at 10:07
1

@BorisStitnicky: yeah, Pascal does that to people :) – Sergio Tulentsev May 27 '13 at 10:09
People maintain that in spite of the gaping differences, there is a [deep connection between Pascal and Ruby syntax](http://www.bitwisemag.com/2/Delphi-For-Ruby-What-s-The), which might be the reason why I became comfortable with Ruby in the end. – Boris Stitnicky May 27 '13 at 10:15

score 4 · Accepted Answer · edited May 23 '17 at 11:43

Usually symbols are used as hash keys because they save memory and are a little faster for comparisons, and the begin..end block is not necessary. Thus it becomes:

options[:host] = (a == :jaxon ? 'jaxon-server16.jaxon.local' : 'doric-server5')

This is a relatively long while, in my head the following parses easier:

options[:host] = 'doric-server5'
options[:host] = 'jaxon-server16.jaxon.local' if a == :jaxon

An iteration on top of that, is that you have what appears to be semi-hardcoded values (jaxon-server16.jaxon.local and doric-server5). You should store these in constants or another data structure in order to have these gathered in one place. For instance, if doric-server5 becomes doric-server6 one day, you'll only have to change it in the top of a class or file somewhere. Furthermore, it makes the code easier to read since they now have more humane names of whatever they represent.

# somewhere else:
JAXON_SERVER = 'jaxon-server16.jaxon.local'
DORIC_SERVER = 'doric-server5'

options[:host] = DORIC_SERVER
options[:host] = JAXON_SERVER if a == :jaxon

Since we've dealt with the original motivation to make it two lines, we could go back to one, nice line:

options[:host] = (a == :jaxon ? JAXON_SERVER : DORIC_SERVER)

If you have a lot of these kinds of statements, you could make a server hash, where e.g. server[:jaxon] = 'jaxon-server16.jaxon.local', but if you just have two, two string constants are fine.

In some cases it's nicer to have the default option (in this case DORIC_SERVER) appear wherever it would default to that value, instead of setting the host to the default value directly. Hash#fetch takes two arguments: a key, and a value to default to, if that key doesn't exist.

options[:host] = JAXON_SERVER if a == :jaxon

# somewhere else:
options.fetch(:host, DORIC_SERVER)

Without more information, it's hard to say which approach is the best in your case. :-)

I like this line - *Without more information, it's hard to say which approach is the best in your case.*. To get the best answer,OP should need to work more in theirs *question* making. — Arup Rakshit, May 27 '13 at 11:41
wow fantastic answer. especially like the use of fetch with a default. — lukemh, May 27 '13 at 12:08

score 2 · Answer 3 · answered May 27 '13 at 08:52

2

This is another way to write it

options['host'] = case a
when :jaxon
  'jaxon-server16.jaxon.local'
else
  'doric-server5'
end

It has a lot more lines but i like its readability. It also makes it easy to add more hosts:

options['host'] = case a
when :jaxon
  'jaxon-server16.jaxon.local'
when :staging
  'staging-server1'
else
  'doric-server5'
end

answered May 27 '13 at 08:52

Patrick Oscity

53,604
17
144
168

Ideally, settings like this should be external to the app. They shouldn't be in the code. – Sergio Tulentsev May 27 '13 at 08:57

score 2 · Answer 4 · edited May 27 '13 at 10:10

2

If you want to surround it with something for readability, you can share a pair of parentheses with the Hash#store method.

options.store( "host",
  a == :jaxon ? "jaxon-server16.jaxon.local" : "doric-server5"
)

edited May 27 '13 at 10:10

Boris Stitnicky

12,444
5
57
74

answered May 27 '13 at 09:41

sawa

165,429
45
277
381

Thanks, you taught me a nicely named method here. – Boris Stitnicky May 27 '13 at 10:09

more conventional way to write this ruby

4 Answers4