C22 - Kristina Nguyen and Tram Hoang - Phoenix #7
C22 - Kristina Nguyen and Tram Hoang - Phoenix #7tramhn001 wants to merge 13 commits intoAda-C22:mainfrom
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looking good y'all! I left a couple suggestions of things to take forward for future projects.
I like seeing that your team was making commits as you went, in the future I would like to see folks practicing writing more detailed commit messages that talk about the specific files changed, routes added, etc.
app/routes/planet_routes.py
Outdated
| distance_from_sun = request_body["distance_from_sun"] | ||
| namesake = request_body["namesake"] | ||
|
|
||
| new_planet = Planet(name=name, surface_area=surface_area, moons=moons, distance_from_sun=distance_from_sun, namesake=namesake) |
There was a problem hiding this comment.
Another way to break this line up for length and readability could be:
new_planet = Planet(
name=name,
surface_area=surface_area,
moons=moons,
distance_from_sun=distance_from_sun,
namesake=namesake
)
app/routes/planet_routes.py
Outdated
| request_body = request.get_json() | ||
| name = request_body["name"] | ||
| surface_area = request_body["surface_area"] | ||
| moons = request_body["moons"] | ||
| distance_from_sun = request_body["distance_from_sun"] | ||
| namesake = request_body["namesake"] |
There was a problem hiding this comment.
What kind of error could be raised by this code? What's a way we could handle it?
| if sort_param: | ||
| sort_column = getattr(Planet, sort_param, None) | ||
| if sort_column: | ||
| query = query.order_by(asc(sort_column) if order_param == "asc" else desc(sort_column)) |
There was a problem hiding this comment.
How could we break up this line for length?
| distance_from_sun_param = request.args.get("distance_from_sun") | ||
| if distance_from_sun_param: | ||
| query = query.where(Planet.distance_from_sun <= distance_from_sun_param) | ||
| query = query.order_by(Planet.id) | ||
|
|
||
| planets = db.session.scalars(query) |
There was a problem hiding this comment.
Since adding the order_by isn't tightly related to filtering by distance_from_sun_param, I'd consider adding space between those statements:
distance_from_sun_param = request.args.get("distance_from_sun")
if distance_from_sun_param:
query = query.where(Planet.distance_from_sun <= distance_from_sun_param)
query = query.order_by(Planet.id)
planets = db.session.scalars(query)| @@ -0,0 +1,69 @@ | |||
| def test_get_all_planets_with_no_records(client): | |||
There was a problem hiding this comment.
Thinking about our routes vs our tests, what gaps do you see in the test suite? What functions could we add tests for to get better test coverage for the project?
| query = db.select(Planet) | ||
|
|
||
| if sort_param: | ||
| sort_column = getattr(Planet, sort_param, None) |
There was a problem hiding this comment.
Nice solution for flexibility on what folks can sort by!
3 participants
No description provided.