Glasgow | 25-SDC-Nov | Nataliia Volkova | Sprint 5 | Allocation Laptop #311
Conversation
Nataliia74
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.
I think the code will work, but it is way too complicated. Please follow-up on the suggestion for a much simpler approach. It's often slightly harder to right simpler code, but it's worth it for everyone else that has to work with the code afterwards.
| operating_system: OperatingSystem | ||
|
|
||
| def norm_os_values(value: str) -> OperatingSystem: | ||
| value = value.strip().lower() |
|
|
||
|
|
||
| people = [ | ||
| Person(name="Imran", age=22, preferred_operating_systems=[norm_os_values("Ubuntu"), norm_os_values("Arch Linux")]), |
There was a problem hiding this comment.
Did you consider giving Person a constructor which took a list of strings and did the normalisation into a list of OperatingSystem? Or alternatively declaring the Person directly with [OperationSystem.UBUNTU,OperatingSystem.ARCH]
|
|
||
|
|
||
| def allocate_laptops(people: List[Person], laptops: List[Laptop]) -> Dict[Tuple[str, int], int]: | ||
| sadness_table: Dict[Tuple[str, int], int] = {} |
There was a problem hiding this comment.
This feels unnecessarily complicated. Could you not return a Dict[Person,Laptop]? Then most of the processing you're doing later isn't necessary.
It feels like this is almost a database design with foreign keys which has been forced into an object based language.
Can you have a think about how much simpler it would be if you stayed with Dict[Person,Laptop] ?
OracPrime
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
02 Implement laptop allocation issue 179