-
-
Notifications
You must be signed in to change notification settings - Fork 21
West Midlands | July SDC | Gabriel Deng | Sprint 4 | Prep exercises #39
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
base: main
Are you sure you want to change the base?
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). |
|
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). |
7 similar comments
|
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). |
|
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). |
|
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). |
|
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). |
|
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). |
|
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). |
|
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). |
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.
Remember to leave a summary of what work you've done in the PR body on github.
You don't need to commit the__pycache__ directory
Good start on this sprint's tasks, I have spotted a few areas where you could improve code further
| @@ -0,0 +1,5 @@ | |||
| def double(n): | |||
| return n * 3 | |||
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.
Does this function actually double? What change could you make to resolve the issue?
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.
I think for this function, there are two possible interpretations. Firstly, the function might have been meant to return the triple value of n. In this case, I could the function name from double(n) to triple(n). Secondly, the function might actually been meant to return double the value of n. Meaning I can just change the 3 to 2.
prep exercises/bank_account.py
Outdated
| @@ -0,0 +1,30 @@ | |||
| def open_account(balances, name, amount): | |||
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.
Part of the bank account task involved making type annotations, can you try that?
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.
I have just added the type annotations on the functions
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.
did you try running the code and spotting what the error was?
| self.preferred_operating_system = preferred_operating_system | ||
| self.address = address | ||
|
|
||
| def is_adult(self): |
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.
What is a benefit or drawback of writing the function this way?
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.
Writing a method has many benefits, such as it helps in encapsulating logic, as a result, it makes the method reusable in other instances of the class. The problem of these functions is that they become difficult to test in isolation as compared to free functions.
| def is_adult(self): | ||
| today_date = date.today() | ||
| birth_date = date.fromisoformat(self.date_of_birth) | ||
| age = today_date.year - birth_date.year |
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.
Would this date calculation work all the time? What about if they are one day away from a 18th birthday in the current year?
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.
This calculation will not work in this scenario because I am only comparing the years. I have changed the functionality to include the days instead of only comparing the years
prep exercises/generic_exercise.py
Outdated
| class Person: | ||
| name: str | ||
| children: List["Person"] | ||
| age: int |
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.
Can you solve this in a way that it always prints the current age, rather than hard-coding a specific age?
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.
I have changed the age to date_of_birth and used the function i used previous to calculate the age of the person as long as the birthday is given.
|
Good work, I think this is complete now |
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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). |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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). |
Learners, PR Template
Self checklist