Conversation
…e both GET and POST requests.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Overall, this looks good! The main thing to consider going forward is looking for ways to move shared code into helper methods, and to reduce the complexity of the individual route functions by splitting them into their own functions, following the single responsibility principle.
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
Once we import blueprints (which in turn import the models), we don't need the model import here any more.
| @@ -0,0 +1,33 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
try to remember to add a message when you create a migration. It can make it easier to know which migration files to look in if there's a problem going on during setup.
|
|
||
| def test_get_response_body_of_fixture_planet(client, two_saved_planets): | ||
| #Act | ||
| response = client.get("/planets/1") |
There was a problem hiding this comment.
We could made this test a little less brittle, though perhaps a little tougher to read, if we write two_saved_planets to return the created models as well. We could then access the id of the model here, using the id the database gave the record. This can help us future-proof our tests (in case we change the number or order of records created, directly or indirectly).
| saturn = Planet(name = "Saturn", description="Lots of rings!") | ||
|
|
||
| db.session.add_all([mars, saturn]) | ||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
Consider returning [mars, saturn] after this line, so that tests can make use of the ids of the records, and anything else that's part of the record that might be useful in the test.
| db.session.add(new_planet) | ||
| db.session.commit() | ||
|
|
||
| return jsonify(f"Planet {new_planet.name} created successfully"), 201 |
There was a problem hiding this comment.
Consider returning information about the planet created (its dictionary structure) rather than just a message here. The caller would probably like to at least know the id of the created record. Returning a dictionary with the id and message as two separate parts can make it easy to get the id programmatically, as well as a potentially human friendly message.
| @planets_bp.route("", methods=["POST", "GET"]) | ||
| def handle_planets(): |
There was a problem hiding this comment.
Consider making separate functions for the get and post (the GET method is often called index, while the POST method is often called create). This allows each function to focus on a single piece of functionality, and avoids the need for the if within the route method.
| planets_response.append({ | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description | ||
| }) |
There was a problem hiding this comment.
We strongly suggest making a helper method on the Planet class to take care of converting the model instance to a dictionary for the purpose of being returned.
| @planets_bp.route("/<planet_id>", methods=["GET", "PUT", "DELETE"]) | ||
| def handle_planet(planet_id): |
There was a problem hiding this comment.
As with the previous endpoint, here too consider splitting the various verbs into separate functions.
| return { | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description | ||
| } |
There was a problem hiding this comment.
Consider moving the code to transform a Planet into a dictionary to a helper function in the Planet class.
| planet.name = form_data["name"] | ||
| planet.description = form_data["description"] |
There was a problem hiding this comment.
We would want to perform similar error handling as we did for the POST verb here in PUT. We are replacing the record, so any requirements that were required to originally create a record, should really be applied her in the PUT as well.
3 participants
No description provided.