Accelerate - Karolina #11
Conversation
There was a problem hiding this comment.
Greta job!
Your tests are all passing and your code was really readable. There were a couple places where you might try using some of the built-in features of Flask/SqlAlchemy, like looking up a valid model instance, sorting, and relationship helpers.
Beyond that, you might have noticed that the code in CRUD applications can be very repetitive. We should think about how we can structure our code so that more of it is reusable, and also try to reduce the overall size of any given function. So for the endpoints that are currently managing more than one verb, maybe we can think of ways to separate them into their own functions, while moving any shared logic to helpers that can be used across more than one function.
So we can really dive into the Flask and SqlAlchemy docs, and take a look at other samples for more ideas!
Again, overall, really nice job!
| @@ -4,3 +4,5 @@ | |||
|
|
|||
| class Goal(db.Model): | |||
| goal_id = db.Column(db.Integer, primary_key=True) | |||
| title = db.Column(db.String) | |||
| tasks = db.relationship('Task', backref='goal', lazy=True) | |||
There was a problem hiding this comment.
👍
There are lots of interesting values that lazy can be set to. True is a synonym for "select", and works great here. But check out some of the other options. 😄
| @@ -4,3 +4,5 @@ | |||
|
|
|||
| class Goal(db.Model): | |||
| goal_id = db.Column(db.Integer, primary_key=True) | |||
| title = db.Column(db.String) | |||
There was a problem hiding this comment.
Should we be able to create a goal with a NULL title? Consider adding nullable=False here.
| title = db.Column(db.String) | ||
| description = db.Column(db.String) |
There was a problem hiding this comment.
Should title or description be allowed to be NULL? (Does that make sense from a data standpoint?) Consider adding nullable=False here.
| from .routes import tasks_bp | ||
| from .routes import goals_bp | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) |
There was a problem hiding this comment.
👍
One thing we started to touch on in the video store live code was that we can split routes into multiple files. We can make a routes folder, and put routes for each endpoint into separate files, named for their model. Then we can use the name bp for the blueprint in each file since it would be the only blueprint in the file. Then these imports might look like:
from .routes import task, goal
app.register_blueprint(task.bp)
app.register_blueprint(goal.bp)| tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks") | ||
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
We could consider putting all the routes related to the tasks endpoint in their own file, and doing the same for the goals routes.
| }) | ||
|
|
||
| @goals_bp.route("", methods=["POST", "GET"]) | ||
| def handle_goals(): |
There was a problem hiding this comment.
Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for Task will also apply to Goals. These are common themes for any model with CRUD operations.
| return { | ||
| "goal": { | ||
| "id": new_goal.goal_id, | ||
| "title": new_goal.title | ||
| } | ||
| }, 201 |
There was a problem hiding this comment.
Like for Task, this should probably be indented under the POST logic
| if 'title' not in request_body: | ||
| return {"details": "Invalid data"}, 400 |
There was a problem hiding this comment.
👍
This should get checked for a PUT as well
| return make_response("", 404) | ||
|
|
||
| if request.method == "GET": | ||
| tasks = Task.query.filter(Task.goal_id == goal_id) |
There was a problem hiding this comment.
We can retrieve the tasks this way, but also, our Goal model has a tasks member, so we could also do
tasks = goal.tasks| for id in task_ids: | ||
| task = Task.query.get(id) | ||
| task.goal_id = goal_id |
There was a problem hiding this comment.
👍
Yep. We need to retrieve each task to be able to associate it to the goal.
But what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here?
No description provided.