Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on task-list-api!
Your structure, added tests, and functionality look good!
Let me know if you have any questions about the review comments.
| from app import db | ||
|
|
||
|
|
||
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
Since this file is called goal_routes.py and the file is only concerned with the Goal class, we can rename this variable to just be bp
| @@ -0,0 +1,108 @@ | |||
| from flask import Blueprint, request, make_response, jsonify, abort | |||
There was a problem hiding this comment.
Nice work putting goal routes logic in a separate file from task routes logic. You can create a directory called routes (like we have with models) and put your two route files in it to further organize your project
|
|
||
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
|
||
| def get_validate_model(cls, model_id): |
There was a problem hiding this comment.
Nice work refactoring this method so it is generic enough to validate any kind of model.
However, you have this method repeated in both route files and they do the same thing. It would be better to put this method in a helper file and import the method into your 2 route files
| if "title" not in request_body: | ||
| return make_response({ | ||
| "details": "Invalid data" | ||
| }, 400) |
There was a problem hiding this comment.
Nice error handling. Whenever our methods rely on data sent by the client we should have error handling because we should anticipate the possibility of bad data being sent to our server.
Another way to handle invalid request body from the client is to use try/except.
try:
# create your goal here
except:
# return error message if the request body is invalid| "details": "Invalid data" | ||
| }, 400) | ||
|
|
||
| new_goal = Goal(title=request_body["title"]) |
There was a problem hiding this comment.
As we demonstrated during our roundtable, you can also create a class method in the Goal class to handle turning data from the request body into an instance of Goal.
| "details": "Invalid data" | ||
| }, 400) | ||
|
|
||
| new_task = Task(title=request_body["title"], |
There was a problem hiding this comment.
Line 58 is concerned with creating an instance of the Task class. You could argue that the logic in this method create_task() should only be concerned with creating a valid response to return to the client. Therefore, the logic on line 58 should be moved into the Task class so that the Task class is responsible for creating an instance of itself and we can just invoke the method here.
If you create a class method from_dict() in the Task class, then line 58 would just be:
new_task = Task.from_dict(request_body) That way we don't need to expose the logic that belongs in Task in a route
| task.title = request_body["title"] | ||
| task.description = request_body["description"] |
There was a problem hiding this comment.
Need some error handling here in case the request_body is invalide
|
|
||
| db.session.commit() | ||
|
|
||
| slack_bot(task) |
There was a problem hiding this comment.
Nice job using a helper function to contain the logic for making a request to Slack's API. This makes your method more concise and readable
| return make_response(jsonify(current_task_response), 200) | ||
|
|
||
|
|
||
| def slack_bot(task): |
There was a problem hiding this comment.
Consider renaming this method to incorporate a verb to clarify what this method is doing. Something like make_req_to_slack_bot would work
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
2 participants
No description provided.