Andy Zhao (he/him)

Posted on Nov 2, 2018 • Edited on Nov 7, 2018

Which is more readable?

#codecompare #ruby

I'm currently refactoring/rewriting some code, and I can't decide on which is more readable than the other. It is in Ruby, but I think in this context readability is somewhat language-agnostic.

json_data = { user: add_user_data(mentioner) }
json_data[:comment] = add_comment_data(mentionable) if mentionable_type == "Comment"

json_data = {}
json_data[:user] = add_user_data(mentioner)
json_data[:comment] = add_comment_data(mentionable) if mentionable_type == "Comment"

I actually like this syntax, too, but unfortunately it's invalid:

json_data = {
  user: add_user_data(mentioner),
  comment: add_comment_data(mentionable) if mentionable_type == "Comment"
}

# guess you can't have conditionals when declaring hashes ☹️

Top comments (15)

Dustin King • Nov 3 '18

This could be a job for a template language like erb, but that might be overkill if the rest of the json data isn't very complicated.

I like the second form because it's more consistent, but I would put the if clause before the assignment, because IMO as part of the statement it causes confusion whether it's short-circuiting the whole assignment or just part an expression for what is being assigned.

json_data = {}
json_data[:user] = add_user_data(mentioner)
if mentionable_type == "Comment"
  json_data[:comment] = add_comment_data(mentionable) 
end

Also if the functions are side-effect free, I would drop the add_ from their names.

Gene • Nov 4 '18

I prefer this solution as well. It's simple and clear. Easy to understand.

json_data = {
  user: add_user_data(mentioner)
}

if mentionable_type == "Comment"
  json_data[:comment] = add_comment_data(mentionable) 
end

Rich Smith • Nov 6 '18

My preference goes with this one as well. Declaring json_data = {} in advanced just isn't necessary for writing code that is easy to be able to follow along with.

Tamara Temple • Nov 3 '18

This is my suggestion as well; it's extremely clear what is going on here, and I think this form conveys information to future maintainers quite well. The other suggestions are certainly doable. The tap form seems quite common, but if it's the only occurrence in your code base, it would likely be more confusing than helpful.

rhymes • Nov 2 '18 • Edited

You could technically use a parenthesis in the second example and let the key value be nil if the condition is not met.

[1] pry(main)> condition = true
=> true
[2] pry(main)> data = {
[2] pry(main)*   user: "alice",
[2] pry(main)*   comment: ("something" if condition),
[2] pry(main)*   metadata: ("something else" unless condition),
[2] pry(main)* }
=> {:user=>"alice", :comment=>"something", :metadata=>nil}
[3] pry(main)> _.compact
=> {:user=>"alice", :comment=>"something"}

Another strategy is to use tap

[1] pry(main)> condition = true
=> true
[2] pry(main)> data = { user: "alice" }
=> {:user=>"alice"}
[3] pry(main)> data.tap do |d|
[3] pry(main)*   d[:comment] = "something" if condition
[3] pry(main)*   d[:metadata] = "something else" unless condition
[3] pry(main)* end
=> {:user=>"alice", :comment=>"something"}

It's not a perfect solution but it allows you to build the dictionary in blocks. You can also attach multiple "taps".

Andy Zhao (he/him) • Nov 2 '18

Hmmmmmm interesting. I do want to avoid having a nil value. The tap is an interesting solution but I think it seems too verbose for creating a single use hash.

rhymes • Nov 2 '18

nil + compact then ?

Tap is great to create a pipe of operations, a little overkill with your use case yeah

Andy Zhao (he/him) • Nov 2 '18

nil + compact also seems overkill :/ Feels like an extra operation when I can skip it with a single line if.

rhymes • Nov 2 '18

you're right, the first solution is the clearest

The alternatives are "tricks" :D

Scott Tadman • Nov 2 '18 • Edited

I'm a big proponent of trying to avoid "surprise if conditions", in other words a line that might, for reasons of not being shown in full in your code, not actually run like you think it does, but instead (surprise!) only run under certain conditions.

So things like this are fine:

return if x
return false unless y
next if z

Since you can see at a glance exactly what's going on there even if the conditions are fairly long.

That being said, a refactoring that fixes this up probably looks like this:

json_data = {
  user: add_user_data(mentioner)
}

case (mentionable_type)
when 'Comment'
  json_data[:comment] = add_comment_data(mentionable)
end

This is the "define and conditionally augment" approach to accumulating data in an object. The use of the case here makes it clear where to add any other cases should they arise.

Andy Zhao (he/him) • Nov 2 '18

Not a bad idea, thanks for sharing! Personally never liked cases but this might be a good use for them.

Sam Morgan • Nov 3 '18

I like the second, declarative structure. I also prefer that we don’t create something only to immediately mutate it.

Perhaps you could move the conditional into the add_comment_data method as a guard? Then you wouldn’t have the conditional in the hash construction.

Small point, but I prefer methods named after what they return, rather than what they do. So could kill the add_ prefix from the methods. I find this generally helps me to think in a more ‘OO’ mindset.