London| SDC-N0v-25| Ikenna Agulobi | Sprint 5| Laptop allocation #313
Conversation
ike-agu
added
the
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
The code is generally very good and has some nice touches in terms of normalization etc. However your algorithm doesn't scale. Can you think of a "good enough" rather than perfect one that does scale?
| @@ -0,0 +1,104 @@ | |||
| from dataclasses import dataclass | |||
| from enum import Enum | |||
| from typing import List, Dict, Optional, Tuple | |||
There was a problem hiding this comment.
I suspect you started using a Tuple and changed it to a List? It's good practice to remove unwanted imports.
There was a problem hiding this comment.
Thanks for pointing that out. I've removed unwanted imports.
|
|
||
| # =====define parse operating system====== | ||
| def parse_operating_system(raw: str) -> OperatingSystem: | ||
| normalized = raw.strip().lower() |
|
|
||
| if normalized not in aliases: | ||
| valid = ", ".join(sorted(aliases.keys())) | ||
| print(f"Error: invalid operating system. Try one of: {valid}", file=sys.stderr) |
There was a problem hiding this comment.
Like the inclusion of allowed values in the error message. Like the fact that you're using stderr
| best_assignment: Optional[Dict[Person, Laptop]] = None | ||
| best_total_sadness: Optional[int] = None | ||
|
|
||
| for chosen_laptops in itertools.combinations(laptops, len(people)): |
There was a problem hiding this comment.
This will work for 5 laptops. What happens if you've got 20 laptops?
There was a problem hiding this comment.
Hi @OracPrime , you’re absolutely right. The original brute-force approach I implemented only works for very small number of laptops as you pointed out, because it explores all possible permutations and does not scale well.
I’ve refactored my code to use a greedy approach, which allows it to scale to much larger inputs (for example, 20 or more laptops). This approach doesn’t always find the perfect solution, but it gives a reasonable result and runs efficiently even as the number of laptops increases.
Thanks for taking the time to review my code and for your helpful feedback.
OracPrime
added
Reviewed
…Remove unwanted imports
|
@OracPrime, Thanks for taking the time to review my code and for your feedback. I’ve implemented the changes you pointed out. When you’re available, could you please take another look? |
ike-agu
added
the
Needs Review
|
|
||
| # custom hash to hash the immutable version of the list | ||
| def __hash__(self) -> int: | ||
| return hash((self.name, self.age, tuple(self.preferred_operating_system))) |
There was a problem hiding this comment.
Nice! The non-hashability of List caught many people out (including me, initially - I'm more C# than Python). This is a nice workaround.
There was a problem hiding this comment.
@OracPrime Thanks for reviewing my PR and for the feedback. I really appreciate it
There was a problem hiding this comment.
It was good code - you may notice I took the liberty of quoting it in the Nov SDC slack channel!
OracPrime
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
This PR contains the Laptop allocation exercise of sprint 5