Glasgow | Sheetal Kharab | Module-Decomposition | Sprint 4 | Implement laptop allocation#29
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
sheetalkharab
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Apologies for the lateness in the review, this is a good implementation, but you might want to consider the efficiency. I've left a comment about that in the code. Can you think of any changes that could improve that?
allocate_laptop.py
Outdated
| for person in people: | ||
| # Find laptop that gives this person minimum sadness | ||
| if available_laptops: | ||
| best_laptop = min(available_laptops, key=lambda l: calculate_sadness(person, l)) |
There was a problem hiding this comment.
This method works, one thing to be careful of is this think how often this function will run.
It will always run, for every laptop, even if the very first one were a perfect match.
It's not an issue in your example here, but imagine if we were running this with thousands of laptop and people entries
LonMcGregor
added
Reviewed
|
@LonMcGregor sorry for late resonse and thanks for the feedback. I’ve updated the allocation logic to avoid calling calculate_sadness on every available laptop as you suggested. |
sheetalkharab
added
the
Needs Review
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
github-actions
bot
removed
the
Needs Review
LonMcGregor
added
Complete
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
2 participants
Learners, PR Template
Self checklist
Changelist
laptop allocation function
Questions
No