Scissors - Melissa Nguyen #62
Conversation
… two endpoints, the first endpoint creates a task and retrieves all tasks, the second endpoint creates, modifies and deletes a specific task
…ask titles by ascending, descending or default sort based on sort query param
…s_complete depending on whether or not the task has been completed
… task list api. Slack message bot helper function identifies and returns which tasks have been completed
…etrieves all goals in database. Second endpoint gets, puts and deletes a singular goal
…st. Endpoint retrives list of tasks associated with a specific goal and posts tasks associated with a specific goal
There was a problem hiding this comment.
Good work on your first Flask project. Your code is clear and logical. I've left some inline comments on ways you could consider refactoring. My comments are mainly focused on places where you could write instance methods to DRY up your code, and ways to enhance readability. Please let me know if you have any questions. Keep up the hard work.
| "task": { | ||
| "id": new_task.task_id, | ||
| "title": new_task.title, | ||
| "description": new_task.description, | ||
| "is_complete": new_task.task_completed() | ||
| } |
There was a problem hiding this comment.
Notice that this code is repeated many places throughout your routes.py. Consider moving the logic to build a json response into a instance method on the task model that you could then call here with something like new_task.to_json().
| db.session.commit() | ||
| return jsonify({"details": f'Task {task.task_id} "{task.title}" successfully deleted'}), 200 | ||
|
|
||
| def slack_message_bot(task): |
There was a problem hiding this comment.
Great work making a helper function.
| return jsonify({ | ||
| "task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": task.task_completed() | ||
| } | ||
| }), 200 |
There was a problem hiding this comment.
Sometimes your code uses make_response and sometimes it doesn't. Consistently using it (or not) can add readability to your code.
| return make_response({ | ||
| "goal": { | ||
| "id": new_goal.goal_id, | ||
| "title": new_goal.title, | ||
| } | ||
| }, 201) | ||
|
|
||
| elif request.method == "GET": | ||
| goals = Goal.query.all() | ||
| goals_response = [] | ||
|
|
||
| for goal in goals: | ||
| goals_response.append({ | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }) |
There was a problem hiding this comment.
Just as with the task response body, note that the goal response body is used a few times. Consider moving this logic to an instance method on the goal model that returns the needed dictionary, and then calling it where needed with something like goal.to_json()
| if task == None: | ||
| return make_response(), 404 |
There was a problem hiding this comment.
You have this code 3 places. Consider making a helper function, or even using get_or_404 https://flask-sqlalchemy.palletsprojects.com/en/2.x/api/#flask_sqlalchemy.BaseQuery.get_or_404.
Also, this is a very minor note, but I just learned this cohort that the python-y way to say if task == None is if not task
| else: | ||
| new_goal = Goal(title=request_body["title"]) | ||
|
|
||
| db.session.add(new_goal) | ||
| db.session.commit() |
There was a problem hiding this comment.
Minor note: this does not need to be nested in an else. Reducing indentation can increase readability.
| for task_id in request_body["task_ids"]: | ||
| task = Task.query.get(task_id) | ||
| task.goal_id = goal.goal_id |
There was a problem hiding this comment.
Nice work writing this logic to make the relationship between goals and tasks.
| completed_at = db.Column(db.DateTime, nullable=True) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True) | ||
|
|
||
| def task_completed(self): |
There was a problem hiding this comment.
Nice work using this helper function.
No description provided.