Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sea Turtles - Georgia A & Lili P #9

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
3 changes: 2 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@

def create_app(test_config=None):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

app = Flask(__name__)

from .routes import bp
app.register_blueprint(bp)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

If you move the routes.py file this will need to change a little.

return app
53 changes: 52 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,53 @@
from flask import Blueprint
from flask import Blueprint, jsonify, abort, make_response

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this file into a routes folder. This is useful even when we don't have another set of routes to store there, since we'll be adding additional files, and having things grouped logically can help us find our way around our code.


class Planet:
def __init__(self, id, name, description, life):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Consider renaming life to has_life. This helps us know to expect a boolean value. Boolean variables/attributes are often named as has_something or is_something. Essentially, we try to phrase the name so that it implies that this is a yes/no, or true/false value.

self.id = id
self.name = name
self.description = description
self.life = life

def to_dict(self):
Copy link

@anselrognlie anselrognlie Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Instance method for building a dictionary from an instance looks good! (Again, consider renaming life.)

return dict(
id=self.id,
name=self.name,
description=self.description,
life=self.life
)


planets = [
Planet(1, "Earth", "Best planet", True),
Planet(2, "Saturn", "Got a ring on it", False),
Planet(3, "Mars", "We want to go there", False)
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll be getting rid of this collection soon, but consider using named arguments here to help this be a little more self-documenting about what the various values are. Id and name are pretty clear, but it might be harder to tell that the second string is a description, and I'd be hard pressed to guess that the final boolean was about whether there was life there.


bp = Blueprint("planets_bp", __name__, url_prefix="/planets")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍



def validate_planet(id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice decision to pull this up above the two routes. And the validation method looks good, for both detecting a bad id, as well as an id with no record.

try:
id = int(id)
except:
abort(make_response({"message":f"planet {id} invalid"}, 400))

for planet in planets:
if planet.id == id:
return planet

abort(make_response({"message":f"planet {id} not found"}, 404))


@bp.route("", methods=["GET"])
def get_planets():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Consider using a list comprehension for generating the result list. This would be especially nice since you have the to_dict instance method.

planets_list = []

for planet in planets:
planets_list.append(planet.to_dict())

return jsonify(planets_list)

@bp.route("/<id>", methods=["GET"])
def get_one_planet(id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Single planet endpoint looks good. We're able to reuse that dictionary instance method and move most of the logic into the validation method.

planet = validate_planet(id)
return planet.to_dict()