Skip to content

London | Ameneh Keshavarz | Module_Decomposition | Week 4|implement laptop allocation #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ameneh-Keshavarz
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@Ameneh-Keshavarz Ameneh-Keshavarz added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 26, 2025
@chinar-amrutkar chinar-amrutkar self-assigned this Jun 20, 2025
@chinar-amrutkar chinar-amrutkar added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jun 20, 2025
@chinar-amrutkar
Copy link

Reviewing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note - It is always helpful to add comments to explain the logic of your code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the feedback, I added some explantion about the code's logic.


allocation = allocate_laptops(people, laptops)

for person, laptop in allocation.items():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What output do you expect here? Does it match the output you get after running your code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the laptops would be given to people based on what operating system they like the most.

Imran likes Ubuntu more than Arch, so I expected him to get the Ubuntu laptop (Laptop #2).

Eliza likes Arch more than macOS, so I expected her to get the Arch laptop (Laptop #1).

When I ran the code, the result was:

Imran gets Laptop #2 with Ubuntu
Eliza gets Laptop #1 with Arch Linux

This is what I expected.

Copy link

@chinar-amrutkar chinar-amrutkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the explanations to the code. Great work!

@chinar-amrutkar chinar-amrutkar added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants