CODEWITHSUNDEEP
rubysyntax
lukemh
lukemh

Reputation: 5323

more conventional way to write this ruby

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.

Upvotes: 2

Views: 121

Answers (4)

Sirupsen
Sirupsen

Reputation: 2115

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. :-)

Upvotes: 4

sawa
sawa

Reputation: 168101

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"
)

Upvotes: 2

Patrick Oscity
Patrick Oscity

Reputation: 54684

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

Upvotes: 2

Sergio Tulentsev
Sergio Tulentsev

Reputation: 230336

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')

Upvotes: 6

Related Questions