Pine Ainur and Tirhas#17
Conversation
jbieniosek
left a comment
jbieniosek
left a comment
There was a problem hiding this comment.
Great work on this project! This project is green.
|
|
||
| @solarsystem_bp.route("", methods=["GET"]) | ||
| def handle_solarsystem(): | ||
| solarsystem_response =[vars(planet) for planet in planets] |
| else: | ||
| return "please enter a valid planet", 404 |
There was a problem hiding this comment.
This is a great idea, but it doesn't quite work as intended. If the first planet name does not match, this endpoint will return the 404 message, rather than continuing to look for a matching planet. Moving the 404 return outside of the for loop will result in returning the 404 only if the planet is not found by the for loop.
| else: | |
| return "please enter a valid planet", 404 | |
| return "please enter a valid planet", 404 |
| for planet in planets: | ||
| max_distance = max(distances) |
There was a problem hiding this comment.
The max_distance is the same for every iteration of the for loop - I recommend moving it outside of the loop to improve the efficiency of the algorithm.
| for planet in planets: | |
| max_distance = max(distances) | |
| max_distance = max(distances) | |
| for planet in planets: |
| print(name) | ||
| for planet in planets: | ||
| if planet.name == name: | ||
| return vars(planet) |
There was a problem hiding this comment.
I recommend adding jsonify to the return even in the case where the Flask default behavior is doing the same thing. Using jsonify adds a layer of error protection and also makes it clear that json is the intended behavior.
| return vars(planet) | |
| return jsonify(vars(planet)) |
CheezItMan
left a comment
CheezItMan
left a comment
There was a problem hiding this comment.
Nice work Ainur & Tirhas, you hit the learning goals here. Well done, do take some time to review Query params and tests when you get to them in Task List. Those are likely to be the tricky parts on that project.
| app = Flask(__name__) | ||
|
|
||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
| app.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql+psycopg2://postgres:postgres@localhost:5432/solarsystem_db' |
There was a problem hiding this comment.
This is fine here, but in the TaskList project you should make the connection string come from an env variable
| distance_from_sun = db.Column(db.Integer) | ||
| radius = db.Column(db.Integer) | ||
|
|
||
| def to_dict(self): |
| for planet in planets: | ||
| solarsystem_response.append(planet.to_dict()) | ||
|
|
||
| return jsonify(solarsystem_response) |
There was a problem hiding this comment.
For readability I suggest using response codes, even for 200s
| return jsonify(solarsystem_response) | |
| return jsonify(solarsystem_response) , 200 |
| if 'name' not in request_data or 'surface_area' not in request_data or 'orbital_period' not in request_data \ | ||
| or 'distance_from_sun' not in request_data or 'radius' not in request_data: | ||
| return jsonify({'message': "missing data"}),400 | ||
| if type(request_data['name']) != str or type(request_data['surface_area']) != int or\ | ||
| type(request_data['orbital_period']) != int or type(request_data['distance_from_sun']) != int\ | ||
| or type(request_data['radius']) != int : | ||
| return jsonify({'message': "Unsupported data type"}),400 |
There was a problem hiding this comment.
This might make a good helper function.
4 participants
No description provided.