Cedar: Rae Elwell & Khandice Schuhmann #8
Conversation
app/routes.py
Outdated
| self.matter = matter | ||
|
|
||
|
|
||
| planets = [Planet(1, "Mercury", "small and red", "solid"), | ||
| Planet(5, "Jupiter", "big and swirly", "gaseous"), | ||
| Planet(6, "Saturn", "rings and swirls", "gaseous")] | ||
|
|
||
|
|
||
| planets_bp = Blueprint("planets", __name__, url_prefix="/planets") |
There was a problem hiding this comment.
Minor note: to enhance readability, consider removing some blank lines and put each Planet instance on it's own line
| self.matter = matter | |
| planets = [Planet(1, "Mercury", "small and red", "solid"), | |
| Planet(5, "Jupiter", "big and swirly", "gaseous"), | |
| Planet(6, "Saturn", "rings and swirls", "gaseous")] | |
| planets_bp = Blueprint("planets", __name__, url_prefix="/planets") | |
| self.matter = matter | |
| planets = [ | |
| Planet(1, "Mercury", "small and red", "solid"), | |
| Planet(5, "Jupiter", "big and swirly", "gaseous"), | |
| Planet(6, "Saturn", "rings and swirls", "gaseous") | |
| ] | |
| planets_bp = Blueprint("planets", __name__, url_prefix="/planets") |
app/routes.py
Outdated
| def get_all_planets(): | ||
| planet_list = [] | ||
| for planet in planets: | ||
| planet_list.append(vars(planet)) |
There was a problem hiding this comment.
I just learned that vars will cause problems when we have a class that connects to our database. We can achieve the same effect by writing a to_json(self) instance method on the Planet class.
app/routes.py
Outdated
| if not planet_id.isdigit(): | ||
| return("Not a number!") |
There was a problem hiding this comment.
Nice work handling an invalid parameter. I learned from Auberon for some strange pythonic reasons, isdigit actually shouldn't be used here, and we prefer try/except
| if not planet_id.isdigit(): | |
| return("Not a number!") | |
| try: | |
| planet_id = int(planet_id) | |
| except: | |
| return ("Not a number!", 400) | |
app/routes.py
Outdated
| for planet in planets: | ||
| if planet.id == planet_id: | ||
| return jsonify(vars(planet)) | ||
| return ("Not Found!") |
There was a problem hiding this comment.
Nice work handling a planet that was not found. We can return a 404 response code with this:
| return ("Not Found!") | |
| return ("Not Found!", 404) |
There was a problem hiding this comment.
Great work on your first Flask project! You've made great use of helper functions and the instance method to_dict(). Your code is very readable. I've made a few suggestions for small refactors and ways to handle 400 level responses. Please let me know if you have any questions. 🎃
| new_planet = Planet(name=request_body["name"], | ||
| description=request_body["description"], | ||
| matter=request_body["matter"]) |
There was a problem hiding this comment.
Here's a suggestion for some input handling. If the request body is missing one of the keys, this code will raise a KeyError exception. Consider handling this way:
| new_planet = Planet(name=request_body["name"], | |
| description=request_body["description"], | |
| matter=request_body["matter"]) | |
| if "name" not in request_body or "description" not in request_body or "matter" not in request_body: | |
| return {"error": "incomplete request body"}, 400 | |
| new_planet = Planet(name=request_body["name"], | |
| description=request_body["description"], | |
| matter=request_body["matter"]) |
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods=["PUT"]) | ||
| def update_planet(planet_id): |
There was a problem hiding this comment.
Nice work separating out PUT and PATCH.
For PUT, consider how you could handle an incomplete request body (similar to the suggestion in POST)
| if make_input_valid(parameter_id) is not None: | ||
| return make_input_valid(parameter_id) |
There was a problem hiding this comment.
Python allows to simplify this conditional to check for truthy values:
| if make_input_valid(parameter_id) is not None: | |
| return make_input_valid(parameter_id) | |
| if make_input_valid(parameter_id): | |
| return make_input_valid(parameter_id) |
| if is_parameter_valid(planet_id) is not None: | ||
| return is_parameter_valid(planet_id) |
There was a problem hiding this comment.
Minor note: similar to above, we don't need the is not None
| if is_parameter_valid(planet_id) is not None: | |
| return is_parameter_valid(planet_id) | |
| if is_parameter_valid(planet_id): | |
| return is_parameter_valid(planet_id) |
| @planets_bp.route("/<planet_id>", methods=["DELETE"]) | ||
| def delete_planet(planet_id): | ||
| if is_parameter_valid(planet_id) is not None: | ||
| return is_parameter_valid(planet_id) |
There was a problem hiding this comment.
Great use of this helper method throughout!
Part 1