London | 25-SDC-July | Mikiyas Gebremichael | Sprint 4 | Implement laptop allocation #36
London | 25-SDC-July | Mikiyas Gebremichael | Sprint 4 | Implement laptop allocation #36Mikiyas-STP wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
Mikiyas-STP
added
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Hi, good implementation, but I've left a comment where you might want to think about the efficiency of your solution.
laptopalloc.py
Outdated
| for person in people: | ||
| for laptop in laptops: | ||
| if laptop.operating_system in person.preferred_operating_system: | ||
| sadness = person.preferred_operating_system.index(laptop.operating_system) |
There was a problem hiding this comment.
One thing to be aware of is efficiency of your solution. This works, but think about how long it will take to complete this. You have an example with about 3 people and laptops. How many times would this need to run if that increased?
Something to think about is when you know you have a suitable answer. If you find a matching laptop, do you need to keep checking every other one?
There was a problem hiding this comment.
Thank-you @LonMcGregor i have replaced the original sadness for every person laptop pairs with a greedy assignment to reduce unnecessary computation and memory usage.
There was a problem hiding this comment.
let me know if this PRis completed
LonMcGregor
added
Reviewed
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
good work adding an early exit if a good laptop is found
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
Implement laptop allocation
Questions
none