AC2-Ocelots:Catherine_Bandarchuk(co-worker: Yuan Du -Anna) - Solar_system_API#12
AC2-Ocelots:Catherine_Bandarchuk(co-worker: Yuan Du -Anna) - Solar_system_API#12CatherineBandarchuk wants to merge 97 commits intoada-ac2:mainfrom
Conversation
…y.github.com> add planet
…y.github.com> add endpoint by id
Kate solar system
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Anna & Kate! I've left a few comments & questions, feel free to reply here or message me on Slack if you have questions on the feedback 🙂
| new_planet = Planet(name=planet_data["name"], | ||
| length_of_year=planet_data["length_of_year"], | ||
| description=planet_data["description"]) |
There was a problem hiding this comment.
This is a completely valid way to break the line up, but I wanted to share another format that's common across languages. This involves dropping all the arguments to their own lines. The benefit of this is making the arguments very clear from other statements and the use of indentation to show we're inside something spanning multiple lines:
# Original code
new_planet = Planet(name=planet_data["name"],
length_of_year=planet_data["length_of_year"],
description=planet_data["description"])
# Alternate formatting
new_planet = Planet(
name=planet_data["name"],
length_of_year=planet_data["length_of_year"],
description=planet_data["description"]
)| from app.models.planet import Planet | ||
|
|
||
| return app | ||
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
It looks like Planet is imported twice in this file, we can remove one of these lines.
There was a problem hiding this comment.
Thank you. :) I didn't notice. Fixed it.
| moon_names = [] | ||
| for moon in self.moons: | ||
| moon_names.append(moon.name) | ||
| planet_as_dict["moons"] = moon_names |
There was a problem hiding this comment.
I know this is the way I grabbed cat names during the class livecode, but this is a great place for a list comprehension!
planet_as_dict["moons"] = [moon.name for moon in self.moons]There was a problem hiding this comment.
I'm still working to be comfortable to use comprehension the same as lambda function. But I'll try to use it more often.
| # Filtering by moon name (return all records which name contains planet_name_query) | ||
| moon_name_query = request.args.get("name") | ||
|
|
||
| if moon_name_query: | ||
| moon_query = moon_query.filter(Moon.name.ilike(f"%{moon_name_query}%")) |
There was a problem hiding this comment.
Since these lines of code are tightly related, I would consider removing the blank line on line 17. This feedback applies across this function where there is a separation between getting a request argument and immediately using it.
There was a problem hiding this comment.
Ok. Thank you. I used the same structure during all project, sorry.
There was a problem hiding this comment.
It's all good, teams and languages may have guidelines, but there is a lot in code style that is subjective. This feedback is not a strict requirement, it is based on the principles of keeping tightly related code closer together and what reads easiest for me. A lot of these small code adjustments often come when you have time to review and refactor your code once it's working, which I know there wasn't a lot of space for with this project and retro video store.
| moon_response = [] | ||
| for moon in moon_query: | ||
| moon_response.append(moon.to_dict()) |
There was a problem hiding this comment.
This would be another great place for a list comprehension - what could that look like?
There was a problem hiding this comment.
moon_response+= [moon.to_dict() for moon in moon_query]
There was a problem hiding this comment.
Nice, you're about there! We would use = since the comprehension creates the whole list we want, we don't need to append to, or extend a list that already exists.
| from app.models.moon import Moon | ||
| from app.models.planet import Planet | ||
| from flask import Blueprint, jsonify, abort, make_response, request | ||
| from .validate_routes import validate_model, validate_moon_user_input, validate_planet_user_input |
There was a problem hiding this comment.
It looks like validate_planet_user_input is imported to this file but isn't used. We should check our imports & remove ones that aren't used.
|
|
||
| # Validating the user input to create or update the table planet | ||
| # Returning the valid JSON if valid input | ||
| def validate_planet_user_input(planet_value): |
There was a problem hiding this comment.
This function is only used by planet_routes.py, as such, I'd consider placing this function in that file to keep it close to where it is used.
| or planet_value["name"] == "" \ | ||
| or "length_of_year" not in planet_value \ | ||
| or not isinstance(planet_value["length_of_year"], int) \ | ||
| or planet_value["length_of_year"] <=0 \ |
There was a problem hiding this comment.
Reminder that we want to leave a space on either side of comparison operators.
3 participants
No description provided.