Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Hi Veronica and Cassandra (?) Great work! I have added feedback on how to use helper methods and refactor.
| class Planet(db.Model): | ||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String) | ||
| description = db.Column(db.String) |
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 and description. When making classes in the future, consider which columns are required (non-nullable).
| def to_string(self): | ||
| return f'{self.id}: {self.name} Description: {self.description}' |
There was a problem hiding this comment.
Also, I highly recommend adding a helper method to turn planet instances into a dictionary (this will definitely save you from writing a few lines of code down the road)!
|
|
||
| planets_bp = Blueprint("planets_py", __name__, url_prefix= "/planets") | ||
|
|
||
| @planets_bp.route("", methods=["GET","POST"]) |
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.
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append( | ||
| { | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description | ||
| }) |
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 | |
| }) | |
| 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 | |
| }) | |
| planets_response = [ planets_response.append(planet.to_dict()) for planet in planets ] | |
| request_body = request.get_json() | ||
| new_planet = Planet(name=request_body["name"], | ||
| description=request_body["description"]) |
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!
| } | ||
|
|
||
| elif request.method == "PUT": | ||
| request_body = request.get_json() |
There was a problem hiding this comment.
We should always check if the request_body is empty
| db.session.commit() | ||
|
|
||
| return make_response(f"Planet #{planet.id} successfully updated") | ||
| elif request.method == "DELETE": |
| def test_so_fancy(so_fancy): | ||
| assert so_fancy.fancy | ||
| so_fancy.or_is_it() | ||
| assert not so_fancy.fancy No newline at end of file |
| # db.session.add(ocean_book) | ||
| # db.session.add(mountain_book) | ||
| db.session.commit() | ||
|
|
There was a problem hiding this comment.
Consider returning [saturn_planet, pluto_planet] after this line, so that tests can make use of the attributes of the records (like the id or name of planet)
| # Act | ||
| response = client.get("/planets/1") | ||
| # ... | ||
|
|
There was a problem hiding this comment.
The test for the post request is missing
2 participants
No description provided.