Reputation: 15374
After running Rubocop against this code I am getting
Use a guard clause instead of wrapping the code inside a conditional expression.
So from what I have read a "Guard Clause" will bail out of the method if the condition is not met, so we don't have to waste time going through extra conditionals, please correct me if my understanding is not correct.
My question is though how would i use a guard statement with multiple conditions
def auth_creds
if %w(test1 qa demo ci).include? ENV['ENV']
{ key: 'key1', secret: 'secret1' }
elsif ENV['ENV'] == 'live'
{ key: 'key2', secret: 'secret2' }
else
fail 'Unable to set key/secret'
end
end
Thanks
Upvotes: 4
Views: 4422
Reputation: 114158
It all depends on your actual code, but with the given snippet, you could use a guard clause to ensure a valid ENV['ENV']
value:
VALID_ENVS = %w(test1 qa demo ci live)
def auth_creds
fail 'invalid environment' unless VALID_ENVS.include? ENV['ENV']
if ENV['ENV'] == 'live'
{ key: 'key2', secret: 'secret2' }
else
{ key: 'key1', secret: 'secret1' }
end
end
As noted by Sergio Tulentsev, storing your credentials in ENV
(instead of the env's name) would probably be better:
def auth_creds
{ key: ENV.fetch('KEY'), secret: ENV.fetch('SECRET') }
end
fetch
will raise a KeyError
if the given key is not found in ENV
.
Upvotes: 3
Reputation: 2916
Guard clauses generally go like this:
def do_something
return 'x' if some_condition?
# other code
end
So your code could be rewritten as
def auth_creds
return { key: 'key1', secret: 'secret1' } if %w(test1 qa demo ci).include? ENV['ENV']
return { key: 'key2', secret: 'secret2' } if ENV['ENV'] == 'live'
fail 'Unable to set key/secret'
end
However, this is quite ugly, and now rubocop will complain about lines that are too long. So let us reword the code to describe its intention:
def auth_creds
return { key: 'key1', secret: 'secret1' } if test_env?
return { key: 'key2', secret: 'secret2' } if live_env?
fail 'Unable to set key/secret'
end
private # omit if `auth_creds` is also private
def test_env?
%w(test1 qa demo ci).include? ENV['ENV']
end
def live_env?
ENV['ENV'] == 'live'
end
Bonus Points: Extract %w(test1 qa demo ci)
into a constant!
Double Bonus Points: (thanks @Sergio Tulentsev) Get your environment-specific (and probably sensitive!!!) credentials out of your code! If using Rails, put it in a secrets.yml
, else use one of the many great gems for this:
A word about Rubocop: Take its advice with a grain of Salt. For example, your code is not really a case for a guard clause, it's just returning data based on conditions. So instead you could also try refactoring the code as a case expression.
And sometime, Rubocop just talks trash :-) (not on purpose, but "measuring" code style is hard!)
Upvotes: 2
Reputation: 230326
Your snippet is not a valid example for "guard clauses". There's nothing to guard against. It's just selecting data. It'd look better as case/when
but if
chain is fine too.
def auth_creds
case ENV['ENV']
when 'test1', 'qa', 'demo', 'ci'
{ key: 'key1', secret: 'secret1' }
when 'live'
{ key: 'key2', secret: 'secret2' }
else
fail 'Unable to set key/secret'
end
end
Guard clause (or as I call them, early returns) are used when the whole body of the method is wrapped in a conditional.
def process_project
if project
# do stuff
end
end
The method won't do anything unless there's a project
. So it makes for more readable code if we reduce the nesting here.
def process_project
return unless project
# do stuff with project
end
Again, not every if
in your code can/should be transformed to this form. Only where it's appropriate.
Upvotes: 8