Sharks - Sana Pournaghshband and Camilla #20
Conversation
app/routes/planet_routes.py
Outdated
| try: | ||
| id = int(id) | ||
| except: | ||
| return abort(make_response({"message": f"planet {id} is invalid"}, 400)) | ||
|
|
||
| for planet in planets: | ||
| if planet.id == id: | ||
| return planet | ||
|
|
||
| return abort(make_response({"message": f"planet {id} is not found"}, 404)) |
|
|
||
| #Get all planets | ||
| @planet_bp.route("", methods=["GET"]) | ||
| def read_all_planets(): |
There was a problem hiding this comment.
Nice job naming the method to describe what this route does.
Since you put your routes in planet_routs.py in routes directory, you can even call this read_all() since we know that all the routes in planet_routes.py are related to the Planet class.
| for planet in planets: | ||
| planets_response.append(planet.to_json()) | ||
|
|
||
| return jsonify(planets_response) |
There was a problem hiding this comment.
Adding 200 status code adds clarity even though it happens by default
return jsonify(planets_response), 200
| def update(self, request_body): | ||
| self.title = request_body["title"] | ||
| self.description = request_body["description"] | ||
| self.moons = request_body["moons"] | ||
|
|
||
| @classmethod | ||
| def create(cls,request_body): | ||
| new_planet = cls( | ||
| title=request_body["title"], | ||
| description=request_body["description"], | ||
| moons = request_body["moons"] | ||
| ) | ||
| return new_planet |
There was a problem hiding this comment.
Nice work pulling these two methods into the model Planet to keep your routes short and sweet.
This comment applies to your create() and update() methods:
How would you add in validation to make sure that all the required fields are sent in the request to your server?
For example, if someone sent a POST request but left off moons, then line 33 would throw KeyError that it couldn't find moons.
it would be nice to handle the error and return a message so that the client knows their request was invalid and they need to include moons. Something to think about for Task List.
app/routes/helpers.py
Outdated
| from flask import Blueprint, jsonify, make_response, abort, request | ||
| from app.models.planet import Planet | ||
|
|
||
| def validate_planet(id): |
There was a problem hiding this comment.
In the future, you might need to make 'helper.py' more specific. Think about Task List, you have Goal and Task, you may need helpers for each. If you refactor helper methods into a helper file then you could have goal_helper.py and task_helper.py
| @@ -0,0 +1,89 @@ | |||
| from flask import jsonify, make_response | |||
No description provided.