Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remix - add null check #22077

Merged
merged 2 commits into from Apr 26, 2018
Merged

Remix - add null check #22077

merged 2 commits into from Apr 26, 2018

Conversation

epeach
Copy link

@epeach epeach commented Apr 26, 2018

In response to the continuation of this honeybadger error: https://app.honeybadger.io/projects/3240/faults/37524712#notice-trace

Null animation_responses were passing this check: 'anim_response&.empty?' when we would like to to fail there. Adding a nil? check resolves the problem.

@epeach epeach requested a review from islemaster April 26, 2018 00:18
@@ -99,7 +99,7 @@ def remix_source(src_channel, dest_channel, animation_list)
anim_response = animation_list.find do |item|
item[:filename] == "#{a['key']}.png"
end
src_body.set_animation_version(a['key'], anim_response[:versionId]) unless anim_response&.empty?
src_body.set_animation_version(a['key'], anim_response[:versionId]) unless anim_response&.empty? || anim_response.nil?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@islemaster There seems to be conflicting opinions on dev blogs about having multiple arguments in 'unless', do we have a preference at Code.org?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're fine with complex inline conditionals, if that's what you're asking. As long as you feel it reads sufficiently well.

@@ -99,7 +99,7 @@ def remix_source(src_channel, dest_channel, animation_list)
anim_response = animation_list.find do |item|
item[:filename] == "#{a['key']}.png"
end
src_body.set_animation_version(a['key'], anim_response[:versionId]) unless anim_response&.empty?
src_body.set_animation_version(a['key'], anim_response[:versionId]) unless anim_response&.empty? || anim_response.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversing the order of these checks would make the safe-navigation operator unnecessary.

... unless anim_response.nil? || anim_response.empty?

@epeach epeach merged commit 12f1fe3 into staging Apr 26, 2018
@epeach epeach deleted the null_object_access_check branch May 18, 2018 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants