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

Only call personName if actually needed #4429

Merged
merged 1 commit into from Aug 5, 2019

Conversation

@viroulep
Copy link
Member

commented Aug 5, 2019

While testing some results stuff I ended up uploading the results for last WC, and I saw an awful lot (thousands!) of InboxPerson loads, and realized I did a pretty bad job at "lazily" calling InboxResult.name (which always fires an InboxPerson load).

Luckily this file as 100% coverage test, so I'm positive the refactor is functionally correct :p

@@ -66,18 +66,17 @@ def validate(competition_ids: [], model: Result, results: nil)
cutoff_for_round = round_info.cutoff

results_for_round.each_with_index do |result, index|
person_name = result.personName

This comment has been minimized.

Copy link
@viroulep

viroulep Aug 5, 2019

Author Member

This statement was the culprit.

@jonatanklosko
Copy link
Member

left a comment

LGTM!

@viroulep viroulep merged commit c6f218a into thewca:master Aug 5, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.303%
Details

@viroulep viroulep deleted the viroulep:fix-n+1 branch Aug 5, 2019

@jfly
Copy link
Member

left a comment

LGTM! Yay for testing =)

@@ -66,18 +66,17 @@ def validate(competition_ids: [], model: Result, results: nil)
cutoff_for_round = round_info.cutoff

results_for_round.each_with_index do |result, index|
person_name = result.personName
context = [competition_id, person_name, round_id, round_info]
context = [competition_id, result, round_id, round_info]

This comment has been minimized.

Copy link
@jfly

jfly Aug 6, 2019

Member

I think this would be a little cleaner using an OpenStruct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.