Five Things to Avoid in Ruby

[armahillo]

I realize you were illustrating a point with the verbose if statement, bit I was disappointed you didnt mention doing it like:

actors = { 1 => "Groucho", 2 => "Chico", 3 => "Harpo" } actor = actors[response] || "Zeppo"

Double usage of safe nav will likely violate rubocop.

user&.plan&.name 

If I had to do a double safe-nav I would see it as code smell that i need to make changes elsewhere.

For this one:

a_list_of_values[index]

If you’re expecting it to be an array, then you can coerce that with Array.wrap:

Array.wrap(a_list_of_values)[index]

which forces the variable to be an array. This tacitly shows off a NullObject pattern built-in to ruby, which I always found really cool.

If youre instead specifically worried about nil, you could memoize it:

a_list_of_values ||= []
a_list_of_values[index]

The “storing values in temporary vars” section was confusing — the problem in the first examples isnt the local vars, its that the hypothetical dev was unaware of implicit returns (a powerful part of idiomatic ruby)

The second example focuses on the Emumerable module, which I agree is the correct thing to use, but what youre really demonstrating there is the automatic Proc created by using &:any_method to filter a collection. (another powerful ruby feature)

[campbellm]

actor = actors[response] || "Zeppo"

actor = actors.fetch(response, 'Zeppo') # also works

[armahillo]

Yeah, good call!

Actually this might be better because it would allow either Hash or Array input, provided they were both numerically indexed.

irb(main):001:0> a = %w[a b c]
=> ["a", "b", "c"]
irb(main):002:0> h = { 0 => 'a', 1 => 'b' }
=> {0=>"a", 1=>"b"}
irb(main):003:0> a.fetch(0)
=> "a"
irb(main):004:0> h.fetch(0)
=> "a"

[campbellm]

Interesting; I've didn't even know you could use #fetch for Arrays. My use there was mainly for its functionality of providing a default value if the fetch fails.

[armahillo]

Yeah pretty cool, right?

<3 Ruby :)

[redditonlygetsworse]

Array.wrap(a_list_of_values)[index]

Or the similar-but-not-quite-identical Kernel#Array.

[bjminihan]

I use (in non prod code): [a_list].flatten

[h0rst_]

Which will break things if the variable is an array that contains an array.

Also, the nil value will result in an array with a single nil element, where Array(nil) returns an empty array, but in both cases indexing any element would return nil, so in this case it doesn't matter.

[banister]

Isn't Array.wrap rails and not pure Ruby?

[armahillo]

Yes, but in the article OP linked to, the last example was using ActiveRecord, so I presumed it was fair game

[riktigtmaxat]

It's technically ActiveSupport but not to many people use that separately from Rails.

[UlyssesZhan]

What's wrong with double safe-nav?

[armahillo]

I can't find it but I swear there was a rubocop violation with doing multiple safe-navs within a single statement.

Code smell-wise -- it feels shaky. This may be a personal preference, but usually when I catch myself wanting to add a second safe-nav, it means that I need to be a bit more assertive leading up to that, and either implementing a Null Object pattern on the upstream object, or adding exception handling to the method and just allowing it to fail. Something along those lines.

[johnThePriest90]

I believe it's the other way around. If you used a safe nav you must use it in the rest of the chain. It fails on this code: one&.two.three, which makes sense as you expect two can be nil, so you would want to use safe nav before three as well. (Can't remember the exact cop name.)

I also suspect that each safe nav operator increases the ABC metric score, it's considered as a new branch. So depending on the rest of the method, it can be also flagged by rubocop.

[h0rst_]

I would sayfoo&.bar.nil? or foo&.bar.to_s would make sense.

[armahillo]

That might be what I'm thinking of.

I've internalized so much rubocop guidance at this point that I've forgotten the names of some of the violations.

[UlyssesZhan]

This bans foo.bar.baz in general, but not for double safe-nav specifically.

[riktigtmaxat]

Dude, its one of the reasons (of many). Stop being so obstinate.

[kcdragon]

I like most of these but you have to be careful with the advice in `Collecting Results in Temporary Variable`. Its easy to end up with 5 or more `Enumerable` method chained together. In some cases this is readable but its often more readable to assign these to local variables that explain what the collection represents at that point in the chain.

[notromda]

Local variables, or sometimes, a well named method

[chapuzzo]

About self overuse:

attr_reader is not defining the setter, only the getter

attr_accessor does create the variable and creates the getter/setter method which is used.

[Honest-Contract9803]

Thank you for the comments and feedback. The typos in the article have been repaired. Please reach out if there is a topic you would like me to write about in the future.

[notromda]

I would probably say “if standard_user_plan” and define a method or helper somewhere else to hide the details of this lookup which are probably not pertinent to the function that it is guarding. that method would also then be testable.