Jenny Massari and Astry Brana - C22 Phoenix #8
Conversation
There was a problem hiding this comment.
I'm leaving feedback here, but the feedback is in regards to the refactoring branch of your project here: https://github.com/SpectreKitty/solar-system-api/tree/add-moon-model.
This allows me to put everything in one spot!
init.py
Remember that we use our blueprints for every route that we write. While we don't have too many routes here, they can add up in more robust databases. It might not seem like much, but writing planets_bp over and over again can be quite cumbersome!
One way we get around this is with the naming convention of just bp. As long as we separate out our routes into different files, we can name all our blueprints bp. To keep our project from getting confused, we can go ahead an alias the different bps when we import them into our init.py. This would look like:
from .routes.routes import bp as planets_bp
The rest of your configuration and blueprint registration looks good!
Models
Your moon model looks good and works well in establishing a good relationship with the planet model. One tweak to think about is how important the to_dict and from_dict methods are. It just makes it so much easier to create responses from the model and create a model from the request body. Make sure you go ahead and make that a common practice as you write these models!
You do have the to_dict and from_dict within your planet model that I love. One thing to note is how you are returning within the to_dict. It can feel safer and clearer to simply create an empty dictionary and then add each attribute to it one at a time. That being said, in industry, it is much more likely that you are going to create the dictionary in one fell swoop and return it. Feel free to get used to this approach!
Routes
-
create_planet - This looks good with some great error handling! When you work with a try/except structure, it's a good idea to try and keep only the code that could throw an error within the try block. With this in mind, I would go ahead and move the session.add session.commit and the response_body to below and outside the except block. Also, you did a good job of refactoring to use your to_dict, just make sure to remove the commented code, especially if it is no longer being used!
-
get_all_planets - Great job working with the query parameters! One extension for something like this is that you will likely have other routes from other models that may use query parameters as well! There are ways to make the query parameter logic generic and apply it to different classes. You are always allowed to extract that particular code and add it to a bank of helper functions in a different file. This allows you to use the same logic across different routes!
-
get_one_planet - Perfect!
-
validate_model - Because we may have multiple models, feel free to move this to it's own file. Something like
route_utilities.pyis usually pretty good. Then go ahead and make the appropriate imports. This is a good idea to just really make sure that you aren't repeating code for routes for different models. -
update_planet - This endpoint would be a good candidate for some error checking. I also do like that you've made this a put over a patch route. Put typically implies that we are replacing everything that we have in the model currently with a new value. Patch implies that we are changing one or more values, but not necessarily all of them. If this were a Patch Route, I think we would want to have some form of checking that only updates the values we need to update.
-
delete_planet - This looks good!
-
create_moon_with_planet - For this one, you say you are creating a moon, but you are actually trying to create a new planet (line 108). It is likely the request body for creating a moon from a particular planet is not the same as one for creating a planet, so we have to be really careful here! Remember that the moon is being added to the planet's moons list via the planet id. The best way to handle this would be to add in a from_dict method for the moon model and then add the validated planet to that request body and create the moon using that dictionary!
-
get_moons_by_planet - This one looks good!
Tests
Your conftest looks great, just make sure to remove the unused code!
Your tests look good, but I do think there could be a little more breadth here in terms of testing. You haven't tested the put route or the delete route! Similarly, your add planet tests look good, but I wonder if there is a way to check and see if the planets were added to the database as opposed to simply checking the response!
Overall this looks good, y'all! Nothing you need to fix, just some food for thought!
No description provided.