-
-
Notifications
You must be signed in to change notification settings - Fork 42
London | 25-SDC-Nov | Aida Eslamimoghadam | Sprint 5 | Implement laptop allocation #300
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
Conversation
Your PR description contained template fields which weren't filled in.
Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.
If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).
If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.
@LonMcGregor
LonMcGregor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start on this sprint's tasks, I have spotted a few areas where you could improve code further
laptop/myLaptop.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these variable names clear about what they represent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You marked this comment as resolved, but I am still not sure if some of the variable names in this function are clear. allocateLp, lpNewList and lp could maybe be improved. I assume lp means laptop, from context?
@LonMcGregor
LonMcGregor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done. please see my comment in this review
laptop/myLaptop.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You marked this comment as resolved, but I am still not sure if some of the variable names in this function are clear. allocateLp, lpNewList and lp could maybe be improved. I assume lp means laptop, from context?
@LonMcGregor
LonMcGregor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! It's much more obvious at first glance what these variables are doing now. Well done
Uh oh!
There was an error while loading. Please reload this page.
PR Checklist
Changelist