Sharks - Sana Pournaghshband #95
Conversation
There was a problem hiding this comment.
Great work on Task List, Sana! You earned a 🟢 on this project!
From your code, I can see that you've got a nice handle on Flask and SqlAlchemy. Good to see you consistently using jsonify and I'd encourage you to reuse your instance method to_json() in your Goal and Task classes to help keep your methods concise
I added a few comments about using list comprehension and some refactors you could make to avoid repetition. For example, implementing a create class method for Goal and Task and a validate helper function will DRY your code up.
I noticed you have a couple of commits, I'd encourage you to practice making smaller frequent commits - perhaps a commit after you finish a route or create a model, or even after you complete a wave.
Let me know if you have any questions. Congrats on finishing Unit 2!
| try: | ||
| title = request_body['title'] | ||
| except KeyError: | ||
| return abort(make_response({"details": 'Invalid data'}, 400)) |
There was a problem hiding this comment.
Nice error handling here! We can't assume that clients will send valid requests to our API so we have to anticipate a scenario where the req doesn't have the keys your method will need to access.
| from app.models.task import Task | ||
| from app import db | ||
|
|
||
| goal_bp = Blueprint("goal_bp", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
You can create a routes directory under the app directory and then put goal_routes.py and task_routes.py in it to keep your project structure organized
| class Task(db.Model): | ||
| task_id = db.Column(db.Integer, primary_key=True) | ||
| task_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| title = db.Column(db.String, nullable=False) |
There was a problem hiding this comment.
Nice work using nullable=False here
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| goal_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| title = db.Column(db.String, nullable=False) |
There was a problem hiding this comment.
Nice work using nullable=False here
| goal = db.relationship("Goal", back_populates="tasks") | ||
|
|
||
| def to_dict(self): | ||
| d = { |
There was a problem hiding this comment.
Consider using a more descriptive name for this variable, something like task_dict
| if sort_query == "asc": | ||
| tasks = Task.query.order_by(Task.title.asc()).all() | ||
| elif sort_query == "desc": | ||
| tasks = Task.query.order_by(Task.title.desc()).all() |
There was a problem hiding this comment.
with order_by() you don't need to use chain .all() to the end of this statement.
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body['title'] | ||
| task.description = request_body['description'] |
There was a problem hiding this comment.
In addition to error handling in POST requests, PUT requests would also be more robust with error handing to make sure we don't have unhandled KeyErrors
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| slack.notify_completed(task.title) |
There was a problem hiding this comment.
👍 having this imported helper method keeps your method nice and short
| task = Task.query.get(task_id) | ||
|
|
||
| if not task: | ||
| return abort(make_response({"message": f"Task {task_id} is not found"}, 404)) |
There was a problem hiding this comment.
When you start to see repeated code or repeated patterns of code, it's usually a good indicator that a helper function can be created and used
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title", | ||
| } | ||
| } | ||
|
|
||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
No description provided.