Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Great work Vange and Cabebe! Overall, the code looks great. My primary feedback involves utilizing helper methods and 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.
The blueprints import the models so we don't actually need the model import anymore.
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| sign = db.Column(db.String) | ||
|
|
There was a problem hiding this comment.
Consider adding a helper function to turn instances into a dictionary
| planets_response.append({ | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description, | ||
| "sign": planet.sign | ||
| }) |
There was a problem hiding this comment.
*See comment about helper function to turn instances into dictionaries. We could refactor this line from 5 lines to one by using a helper function
| planets_response.append({ | |
| "id": planet.id, | |
| "name": planet.name, | |
| "description": planet.description, | |
| "sign": planet.sign | |
| }) | |
| planets_response.append({planet.to_dict()}) |
| if not planets_response: | ||
| return jsonify("There are no planets yet! :O", 200) |
| db.session.add(new_planet) | ||
| db.session.commit() | ||
|
|
||
| return f"Planet {new_planet.name} created.", 201 |
There was a problem hiding this comment.
Don't forget to jsonify your responses
| return f"Planet {new_planet.name} created.", 201 | |
| return jsonify(f"Planet {new_planet.name} created."), 201 |
| return f"Planet {new_planet.name} created.", 201 | ||
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods = ["GET", "PUT", "DELETE"]) |
There was a problem hiding this comment.
see comment on get/post request about putting each request method into separate functions
| return jsonify(f"Planet {planet.name} successfully deleted! bye bitch") | ||
|
|
| return { | ||
| "name": planet.name, | ||
| "description": planet.description, | ||
| "sign": planet.sign | ||
| } |
There was a problem hiding this comment.
to_dict() helper method would have been nice here as well :P
| assert response_body == { | ||
| "description": "way out there", | ||
| "name": "pluto", | ||
| "sign": "Scorpio"} |
There was a problem hiding this comment.
Don't forget to display the id in the response as well
| def add_three_planets(): | ||
| pluto = Planet(name="pluto", description="way out there", sign="Scorpio") | ||
| mercury = Planet(name="mercury", description="messenger closest to sign", sign="Virgo") | ||
| saturn = Planet(name="saturn", description="forever hula hooping", sign="Capricorn") | ||
|
|
||
| db.session.add_all([pluto, mercury, saturn]) | ||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
Consider returning [pluto, mercury, 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.
| @planets_bp.route("", methods = ["GET", "POST"]) | ||
| def get_planets(): |
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.
3 participants
No description provided.