DEV Community

loading...

Which is more readable?

Andy Zhao (he/him)
uh oh where'd my bio go!
Updated on ・1 min read

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"
Enter fullscreen mode Exit fullscreen mode

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 ☹️
Enter fullscreen mode Exit fullscreen mode

Discussion (18)

Collapse
cathodion profile image
Dustin King

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.

Collapse
gene profile image
Gene

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
Collapse
richjdsmith profile image
Rich Smith

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.

Collapse
tamouse profile image
Tamara Temple

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.

Collapse
rhymes profile image
rhymes • 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".

Collapse
andy profile image
Andy Zhao (he/him) Author

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.

Collapse
rhymes profile image
rhymes

nil + compact then ?

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

Thread Thread
andy profile image
Andy Zhao (he/him) Author

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

Thread Thread
rhymes profile image
rhymes

you're right, the first solution is the clearest

The alternatives are "tricks" :D

Collapse
tadman profile image
Scott Tadman • 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.

Collapse
andy profile image
Andy Zhao (he/him) Author

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

Collapse
sjmog profile image
Sam Morgan

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.

Collapse
andy profile image
Andy Zhao (he/him) Author

Yeah, leaning toward the second one, too. I like the variable idea.

Collapse
andy profile image
Andy Zhao (he/him) Author

Nice 👍