Scissors_Gloria #73
Scissors_Gloria #73
Conversation
There was a problem hiding this comment.
Good work on your first Flask project. Your code is clear and meets nearly all the requirements. We can talk through getting those final few tests passing. I've left inline comments mainly focused on ways to use instance methods to DRY up the code. I look forward to talking through it together!
| # } | ||
| # return result | ||
|
|
||
| def notify_slack(self): |
There was a problem hiding this comment.
Great work encapsulating this functionality in a instance method.
| } | ||
| return result | ||
|
|
||
| # def serialize_two(self): |
There was a problem hiding this comment.
You're on the right track with this alternative helper instance method! What we need is to use serialize when goal_id is None (the task doesn't belong to a goal), and serialize_two when goal_id is a truthy value (the task does belong to a goal)
app/routes.py
Outdated
| goal_response = [] | ||
| for goal in goals: | ||
| goal_response.append({ | ||
| 'id': goal.goal_id, |
There was a problem hiding this comment.
You could use goal.serialize() here to DRY up your code.
app/routes.py
Outdated
| 'title': task.title, | ||
| 'description': task.description, | ||
| 'is_complete': task.completed_at != None, | ||
| 'goal_id': task.goal_id |
There was a problem hiding this comment.
Per my comment in the task.py model, this is where we need to conditionally include goal_id depending on whether it's None or an int value.
app/routes.py
Outdated
| arg = request.args | ||
| if "sort" in request.args: | ||
| if arg['sort'] == "asc": | ||
| task_response = sorted(task_response, key = order_by_title) | ||
| elif arg['sort'] == "desc": | ||
| task_response = sorted(task_response, key = order_by_title, reverse = True) |
There was a problem hiding this comment.
Minor note: it would be good to move this logic about the initial query for tasks, so that if we do have a sort query parameter we don't first build an unsorted list of tasks and then overwrite it.
| if "title" not in request_body: | ||
| return { | ||
| "details": "Invalid data" | ||
| }, 400 | ||
| elif "description" not in request_body: | ||
| return { | ||
| "details": "Invalid data" | ||
| }, 400 | ||
| elif "completed_at" not in request_body: | ||
| return { | ||
| "details": "Invalid data" | ||
| }, 400 |
There was a problem hiding this comment.
We could combine this in a compound conditional statement to DRY up the code:
if "title not in request_body or "description not in request body or "completed_at" not in request_body:
return {"details": "Invalid data"}, 400
| 'id': new_task.task_id, | ||
| 'title': new_task.title, | ||
| 'description': new_task.description, | ||
| 'is_complete': new_task.completed_at != None |
There was a problem hiding this comment.
Consider moving the logic to convert completed_at to a True or False value into an instance method and use the serialize instance method here.
| 'task':{ | ||
| 'id': task.task_id, | ||
| 'title': task.title, | ||
| 'description': task.description, | ||
| 'is_complete': task.completed_at != None | ||
| } |
There was a problem hiding this comment.
Notice that this code is repeated below. This is a great place to use an instance method.
No description provided.