Ocelots --- Monica Bao & Jennifer Dai

[Yf-Monica-Bao]

No description provided.

[kelsey-steven-ada]

Great work Monica & Jennifer! I've left a few comments & questions, feel free to reply here or message me on Slack if you have questions on the feedback 🙂

[kelsey-steven-ada]

The lines app.config['JSON_SORT_KEYS'] = False and app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False is duplicated in both parts of the if/else. We could move those lines to either above or below the if/else, so they only need to be written once.

[Yf-Monica-Bao]

Hello Kelsey, Thank you for all of this helpful feedback. I have fixed the code based on your comments.

[kelsey-steven-ada]

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]

[kelsey-steven-ada]

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"],
                 description=planet_data["description"],
                 gravity = planet_data['gravity'],
                 distance_from_earth = planet_data['distance_from_earth'])

# Alternate formatting
new_planet = Planet(
    name=planet_data["name"],
    description=planet_data["description"],
    gravity = planet_data['gravity'],
    distance_from_earth = planet_data['distance_from_earth']
)

[kelsey-steven-ada]

It looks like the route files are loose in the app folder. To keep a project organized as it grows, I recommend creating a routes folder inside of app to place these in.

[kelsey-steven-ada]

Since Moon has a to_dict function we could reduce similar code by calling that function here to get the dictionary to return to the user.

[kelsey-steven-ada]

Specific teams might have their own style guides, but it is a common practice to place helper functions at the end of a file rather than the beginning. An often-used analogy is to lay out code like a newspaper article. Headlines and the most important information comes first - Functions that use other functions in the file go at the top. That gets followed by the details and extra information in an article, or in our code, all the helper functions that support the higher up functions.

[kelsey-steven-ada]

I like the flexibility here for sorting and filtering. As Ilana shared in the wrap-up session, in practice we would usually change this into 2 separate request arguments rather than splitting the attribute from the sort direction on the backend.

[kelsey-steven-ada]

The first and last case duplicates code. If name is our default, we could remove the explicit check for attribute == "name" since if the user didn't specifically choose distance_from_earth or gravity, name will be used.

        if attribute == "distance_from_earth":
            planet_query = sort_helper(planet_query, Planet.distance_from_earth, sort_method)
        elif attribute == "gravity":
            planet_query = sort_helper(planet_query, Planet.gravity, sort_method)
        else: # If user don't specify any attribute, we would sort by name 
            planet_query = sort_helper(planet_query, Planet.name, sort_method)

[kelsey-steven-ada]

Great consideration!

[kelsey-steven-ada]

Looking at the length of this file, I would suggest splitting out the tests for moon routes into their own file.