Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 5 |Laptop allocation #287
Conversation
sheida-shab
commented
Dec 7, 2025
sheida-shab
commented
- I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
- My changes meet the requirements of the task
- I have tested my changes
- My changes follow the style guide
sheida-shab
added
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Generally solid code, but a couple of bits need tidying up or clarifying.
.gitignore
Outdated
| @@ -1 +1,2 @@ | |||
| node_modules | |||
| implement-cowsay/.venv | |||
There was a problem hiding this comment.
I'm guessing this line is left over from a previous exercise!
There was a problem hiding this comment.
Yes, you’re right! This line is leftover from the previous cowsay exercise. I’m keeping it intentionally because the virtual environment is still needed there. I’ve added a comment in .gitignore to make this clear.
laptop_allocation.py
Outdated
| ] | ||
|
|
||
| # Global sadness counter | ||
| sadness=0 |
There was a problem hiding this comment.
Having a global variable is normally a bad idea. This could just as easily be created inside the allocate_laptops function and returned as part of a multi-value return from the function. As it is if you call the function twice the second time will include the sadness from the first time in its sadness score.
There was a problem hiding this comment.
Thanks for your feedback.
I fixed it.
| if person.preferred_operating_system[i] == laptop.operating_system : | ||
| allocated_history[person.name]=laptop # assign laptop | ||
| sadness += i # increment sadness by preference index | ||
| laptops.remove(laptop) |
There was a problem hiding this comment.
The laptops argument is passed to allocate_laptops by reference. So this remove function is modifying the original list of laptops. Now this may be intentional, in which case it should be made obvious somewhere. Alternatively make a copy of the list at the start of the function and modify that inside the function. I can see arguments for both approaches being desirable, but it would be good to make it obvious.
There was a problem hiding this comment.
Yhank you for your comment.
Laptops list is modified on purpose.
After allocation, it shows the laptops that are still left.I fixed it.
I added a comment for clarify this.
| break | ||
|
|
||
| if not allocated_flag : # assign any remaining laptop if preferred OS not found | ||
| allocated_history[person.name]=laptops[0] |
There was a problem hiding this comment.
Both here and above you're using person.name as they key in the dictionary. On line 53 you declared the key as Person. One or the other needs correcting.
OracPrime
added
Reviewed
sheida-shab
added
Needs Review
OracPrime
added
Reviewed
2 participants
