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)
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.
Also if the functions are side-effect free, I would drop the
add_
from their names.I prefer this solution as well. It's simple and clear. Easy to understand.
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.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.You could technically use a parenthesis in the second example and let the key value be nil if the condition is not met.
Another strategy is to use tap
It's not a perfect solution but it allows you to build the dictionary in blocks. You can also attach multiple "taps".
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.nil + compact then ?
Tap is great to create a pipe of operations, a little overkill with your use case yeah
nil + compact also seems overkill :/ Feels like an extra operation when I can skip it with a single line
if
.you're right, the first solution is the clearest
The alternatives are "tricks" :D
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:
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:
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.Not a bad idea, thanks for sharing! Personally never liked cases but this might be a good use for them.
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.Yeah, leaning toward the second one, too. I like the variable idea.
What is another word for "more readable"? love binding spells with pictures
Nice 👍