Scissors - Christian C15 #60
Conversation
There was a problem hiding this comment.
Great work on your first Flask project. You've done a great job working through the logic to implement all the feature. In particular, good work using helper function and methods. I've left a few inline comments on ways you might consider refactoring. Many of the comments focus on ways to increase readability and consistency. Please let me know if you have any questions. Keep up the hard work.
| def tasks_json(self): | ||
| result = [] | ||
| for task in self.tasks: | ||
| task = task.to_json() |
There was a problem hiding this comment.
Great use of method to_json within this method.
| if self.goal_id: | ||
| return { | ||
| "id": self.task_id, | ||
| "goal_id": self.goal_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": is_complete | ||
| } | ||
| return { | ||
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": is_complete | ||
| } |
There was a problem hiding this comment.
Notice there is repeated code here. Consider refactoring by creating a variable to hold the dictionary such as
json =
{"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": is_complete }
and then conditionally adding the goal info
if self.goal_id:
json["goal_id"] = self.goal_id
and then return json
| if "title" in request_body and "description" in request_body and "completed_at" in request_body: | ||
| new_task = Task(title=request_body["title"],description=request_body["description"], | ||
| completed_at=request_body["completed_at"]) | ||
| db.session.add(new_task) | ||
| db.session.commit() | ||
| return jsonify({"task": new_task.to_json()}), 201 | ||
| else: | ||
| return make_response({"details": "Invalid data"}), 400 |
There was a problem hiding this comment.
Consider refactoring to handle the 400 response first. This pattern can increase readability.
| if "title" in request_body and "description" in request_body and "completed_at" in request_body: | |
| new_task = Task(title=request_body["title"],description=request_body["description"], | |
| completed_at=request_body["completed_at"]) | |
| db.session.add(new_task) | |
| db.session.commit() | |
| return jsonify({"task": new_task.to_json()}), 201 | |
| else: | |
| return make_response({"details": "Invalid data"}), 400 | |
| if "title" not in request_body or "description" not in request_body or "completed_at" not in request_body: | |
| return make_response({"details": "Invalid data"}), 400 | |
| new_task=Task(title=request_body["title"],description=request_body["description"], | |
| completed_at=request_body["completed_at"]) | |
| db.session.add(new_task) | |
| db.session.commit() | |
| return jsonify({"task": new_task.to_json()}), 201 |
| if not task: | ||
| return "", 404 | ||
| return make_response({"task": task.to_json()}), 200 |
There was a problem hiding this comment.
This is the pattern I was referring to above. First handle the 400 level response, and then get on with the rest of the program. Nice work!
| def mark_complete(task_id): | ||
| task = Task.query.get(task_id) | ||
| if task == None: | ||
| return make_response(), 404 |
There was a problem hiding this comment.
Your code has a few different ways of return 400 level errors. To increase readability, try to choose one way and stick with it. For instance, there is this code above return "", 404
| if task == None: | ||
| return make_response(), 404 | ||
|
|
||
| if task: |
There was a problem hiding this comment.
Since you've already dealt with if task == None, another conditional test is not necessary here.
| if task: | ||
| task.completed_at = datetime.utcnow() | ||
| db.session.commit() | ||
| call_slack(task) |
| @goal_bp.route('/<goal_id>', methods=['GET'], strict_slashes = False) | ||
| def get_single_goal(goal_id): | ||
| goal = Goal.query.get(goal_id) | ||
| if not goal: |
There was a problem hiding this comment.
if not goal is very python-y. Nice work. I encourage you to favor this way of writing the code as opposed to if goal == None
| goal = Goal.query.get(goal_id) | ||
| for task_id in task_ids: | ||
| task = Task.query.get(task_id) | ||
| if task.goal_id != goal_id: |
There was a problem hiding this comment.
I'm not sure what this line of code is doing. Let's talk about it in our 1:1. I think these two values will always not be equal because one is an int and the other is a str that comes from the route.
No description provided.