Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 5 | Implement Laptop Allocation #294
Conversation
…est sandess level
HassanOHOsman
added
📅 Sprint 5
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
I like the readability of the code, but there are two aspects from an efficiency perspective that need addressing: the exception is a "it would be better if" so could be left alone, but the algorithm you have chosen has a problem with scalability.
| @@ -0,0 +1,42 @@ | |||
| In the prep, there was an exercise around finding possible laptops for a group of people. | |||
There was a problem hiding this comment.
Including the problem statement as a readme really helps me as a reviewer, so I don't have to go and find it. Thanks!
| name: str | ||
| age: int | ||
| # Sorted in order of preference, most preferred is first. | ||
| preferred_operating_system: List[OperatingSystem] |
There was a problem hiding this comment.
As a general naming tip, if you have something which is a list or an array, give it a plural name. So preferred_operating_systems in this case. Helps the reader understand the use of the code
| try: | ||
| return person.preferred_operating_system.index(laptop.operating_system) | ||
| except ValueError: | ||
| return 100 |
There was a problem hiding this comment.
I like the way you have separated this into a separate function, that's good. However it's bad practice to use exceptions in "normal" code execution, as they are very expensive in terms of processing time. They're great for error handling and have particular strengths there. On the next Saturday session, whether it's me or someone else, ask whoever's running it to talk about when using exceptions is a good or a bad idea, it would be a good discussion for the wider group to take part in.
How would you implement this function without try/except?
| best_scenario_assignment = None | ||
| best_total_sadness = float("inf") | ||
|
|
||
| for scenario in permutations(laptops): |
There was a problem hiding this comment.
Wow! OK, if you've got 5 laptops you're looking at 120 possibilities here, that's manageable.
If you've got 10, you're looking at nearly 4 million possibilities. Hey, it's a computer, it will cope.
How many possibilities are you enumerating if you've got 20 laptops?
How else could you do this?
OracPrime
added
Reviewed
2 participants
Self checklist
This PR implements methods to allocate one laptop per person while minimising overall sadness. Each person is assigned one of their preferred operating systems whenever possible; otherwise, their sadness defaults to the maximum value of 100