Sea Turtles - Esther A. & Danielle W. #11
Conversation
|
|
||
| return jsonify(to_dict(planet)) | ||
|
|
||
| def validate_planet(planet_id): |
There was a problem hiding this comment.
I'd consider renaming this function. To me, validate_planet sounds like it should return a true/false based on if the planet passed as a parameter is valid. What name might better describe what the function is doing?
| app.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql+psycopg2://postgres:postgres@localhost:5432/solar_system_development' | ||
| else: | ||
| app.config["TESTING"] = True | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False |
There was a problem hiding this comment.
The line app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False is duplicated in both parts of the if/else. We could move that line to either above or below the if/else, so it only needs to be written once.
| planet = Planet.query.get(planet_id) | ||
|
|
||
| if not planet: | ||
| abort(make_response({"message": f"planet id {planet_id} not found"}, 404)) |
There was a problem hiding this comment.
For a little extra code reuse, the abort calls in this function could also use the helper jsonify_message:
abort(jsonify_message({"message": f"planet id {planet_id} not found"}, 404))|
|
||
| db.session.commit() | ||
|
|
||
| return jsonify_message(f"Planet succesfully updated. {planet.to_dict()}") |
There was a problem hiding this comment.
I'd consider returning the planet name in the message like in create_planet, or if you want to return all the data for the planet, maybe separating that into a different key/value pair in the response. Placing the dictionary contents inside a message string sounds like it might not be easy to read.
No description provided.