Spruce - Symone B. & Ngozi A.#26
Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Great work Symone and Ngozi! My primary feedback focused on helper functions and ways to refactor. :)
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
The blueprints already import the models so we don't actually need the model import in this file anymore.
| class Planet(db.Model): | ||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| color = db.Column(db.String) No newline at end of file |
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, description, and color. I do see in the POST request that y'all checked for falsy values for name, description, and color, however, null values can still be inserted manually using SQL. When making classes in the future, consider which columns are required (non-nullable).
There was a problem hiding this comment.
Also, I highly recommend adding a helper method to turn planet instances into a dictionary. In class, Ansel has named this method to_dict(). This will definitely save you from writing a few lines of code down the road)!
| planets_bp = Blueprint("planets", __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, | ||
| "color": planet.color | ||
| }) |
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, | |
| "color": planet.color | |
| }) | |
| 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, | |
| "color": planet.color | |
| }) | |
| planets_response = [ planets_response.append(planet.to_dict()) for planet in planets ] | |
| return jsonify(f"Planet {new_planet.name} successfully created"), 201 | ||
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods=["GET", "PUT", "DELETE"]) |
There was a problem hiding this comment.
See comment about separating request methods into different functions for GET/POST
| return { | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description, | ||
| "color": planet.color | ||
| } |
There was a problem hiding this comment.
The to_dict() method would be handy here
| "color": planet.color | ||
| } | ||
|
|
||
| elif request.method == "PUT": |
| db.session.commit() | ||
| return jsonify(f"Planet {planet.name} successfully updated"), 200 | ||
|
|
||
| elif request.method == "DELETE": |
|
|
||
| db.session.add_all([planet_1, planet_2]) | ||
| db.session.commit() | ||
|
|
There was a problem hiding this comment.
Consider returning [planet_1, and planet_2] after this line, so that tests can make use of the attributes of the records (like the id or name of planet)
| response = client.post("/planets", json = ({"name": "Unique Space Rock", "description": "Super somber place in the universe.", "color": "The greyest Grey"})) | ||
| response_body = response.get_json() | ||
| assert response.status_code == 201 | ||
| assert response_body == f"Planet Unique Space Rock successfully created" No newline at end of file |
There was a problem hiding this comment.
We could tweak the POST requests in routes.py so that we can return a dictionary of the actual record that was added (just to ensure that all the attributes were valid and added).
We could perform a Planet.query.get call to retrieve the record from the database to confirm that it was actually added.
3 participants
No description provided.