Conversation
…ent data is called
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.
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| moons = db.Column(db.Integer) No newline at end of file |
There was a problem hiding this comment.
Consider which columns we might want to make "required" (non-nullable). As it is, with this definition, we could make a Planet with a NULL name, description, and moon count. Would we want to allow this to happen?
| @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, | ||
| "moons" : planet.moons, | ||
| }) |
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.
|
|
||
| elif request.method == "POST": | ||
| request_body = request.get_json() | ||
| new_planet = Planet(name=request_body["name"], description=request_body["description"], moons=request_body["moons"]) |
There was a problem hiding this comment.
Remember to wrap long lines. We're hitting column 80 here around the "description" mark.
There was a problem hiding this comment.
Be sure to include error handling to check whether the keys we are going to try to access are present. For instance, if we access the "name" key, and it's not in the request body, this will cause a KeyError, and crash the request. The default flask error handling returns the error messages as html, rather than json, so we should explicitly handle these situations ourselves, or look into how to customize the flask error handling.
| sa.Column('name', sa.String(), nullable=True), | ||
| sa.Column('description', sa.String(), nullable=True), | ||
| sa.Column('moons', sa.Integer(), nullable=True), |
There was a problem hiding this comment.
Notice that since your model definitions didn't say that these fields were required, the migration marks them as being OK to be null.
| earth = Planet(name="earth", description="in her flop era", moons=1) | ||
|
|
||
| db.session.add_all([venus, earth]) | ||
| db.session.commit() |
There was a problem hiding this comment.
Consider returning [venus, earth] 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.
| assert response_body == [] | ||
|
|
||
| def test_get_one_planet(client, two_planets): | ||
| 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_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).
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response_body == [ |
There was a problem hiding this comment.
Without an explicit ordering in your GET route, it's a little dangerous to require these to come back in this order. It would be a little safer to check that the two dictionaries are in the response body, without requiring this order.
| } | ||
| ] | ||
|
|
||
| def test_create_a_planet_when_none_exists(client): |
There was a problem hiding this comment.
In a test like this, it would be a good idea to ensure that the database has actually been updated with the new record as expected. We can perform a Planet.query.get() call to load the record. As the POST route is currently written, we would have to assume the id that was assigned to use to try to look it up. If the POST route returned more detailed information about the created Planet record, we could use that to retrieve the record from the database instead.
3 participants
No description provided.