Spruce - Reid and Asha#20
Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Great work Reid and Asha! My primary feedback focuses on helper methods and refactoring.
| from .routes import planets_bp | ||
| app.register_blueprint(planets_bp) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
The blueprints already import the models so we don't actually need the model import in this file anymore.
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| color = db.Column(db.String) No newline at end of file |
There was a problem hiding this comment.
Careful! Using the class as it currently is, we'd be able to make a planet with a null name, description, and color. When making classes in the future, consider which columns are required (non-nullable).
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append( | ||
| { | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description, | ||
| "color": planet.color | ||
| } | ||
| ) |
There was a problem hiding this comment.
We could reduce these lines of code using a helper method to turn the planet object into a dictionary in the Planets class:
| planets_response = [] | |
| for planet in planets: | |
| planets_response.append( | |
| { | |
| "id": planet.id, | |
| "name": planet.name, | |
| "description": planet.description, | |
| "color": planet.color | |
| } | |
| ) | |
| planets_response = [] | |
| for planet in planets: | |
| planets_response.append(planet.to_dict()) |
And further refactor using a list comprehension :D
| planets_response = [] | |
| for planet in planets: | |
| planets_response.append( | |
| { | |
| "id": planet.id, | |
| "name": planet.name, | |
| "description": planet.description, | |
| "color": planet.color | |
| } | |
| ) | |
| planets_response = [ planets_response.append(planet.to_dict()) for planet in planets ] | |
| elif request.method == "POST": | ||
| request_body = request.get_json() | ||
| new_planet = Planet(name=request_body["name"], | ||
| description=request_body["description"], | ||
| color=request_body["color"] | ||
| ) |
There was a problem hiding this comment.
We should check if request_body contains falsy values so that a planet isn't created with empty data!
| new_planet_response = { | ||
| "id": new_planet.id, | ||
| "name": new_planet.name, | ||
| "description": new_planet.description, | ||
| "color": new_planet.color | ||
| } | ||
|
|
||
| return jsonify(new_planet_response), 201 |
There was a problem hiding this comment.
*** repeats broken record about the Planet dictionary helper method ***
| "description": planet.description, | ||
| "color": planet.color | ||
| }, 200 #testing this | ||
| elif request.method == "PUT": |
|
|
||
| return jsonify(f"Planet #{planet.id} successfully updated"), 200 | ||
|
|
||
| elif request.method == "DELETE": |
| return jsonify(new_planet_response), 201 | ||
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods=["GET", "PUT", "DELETE"]) |
There was a problem hiding this comment.
Consider separating each request method into its different functions. This makes testing the endpoints much easier to do and follows the single responsibility principle.
There was a problem hiding this comment.
The same can be said about the "/planets" endpoint for GET and POST.
| saturn_planet = Planet(name="Saturn", description="Has some rings", color="yellow-brown") | ||
|
|
||
| db.session.add_all([mars_planet, earth_planet, saturn_planet]) | ||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
Consider returning [mars_planet, earth_planet, saturn_planet] after this line, so that tests can make use of the attributes of the records (like the id or name of planet)
| assert response.status_code == 201 | ||
| assert response_body == {"id" : 1, "name" : "Post", "description" :"Posty", "color" : "Postingggg"} |
There was a problem hiding this comment.
Great work! In Hello Books, etc. we've typically returned a short message containing the id of the newly added record. What y'all did (returning the actual record in JSON form) is so much more helpful for tests.
Optionally, we could perform a Planet.query.get call to retrieve the record from the database to confirm that it was actually added.
3 participants
No description provided.