Skip to content

Conversation

ttienguyen
Copy link

Ready to submit

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Great job, Thuytien! One thing I noticed is that you kept your route return statements very consistent, and that's great! Predictability is key, and you did that well!

Some things to consider for the future:

  • There was a lot of repeat code when turning a Task into a dictionary. We can turn that into a instance method. There were some other piece of code we could turn into methods as well.
  • Some of the helper functions you made can be moved into a new file, like helper.py.

Other than some duplicate code, your logic was good and your return statements were very consistent!

# request.get_json() returns a dictionary with keys: "title", "description"
# Use corresponding values to create the Task instance.
post_dict = request.get_json()
print(post_dict)

Choose a reason for hiding this comment

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

make sure you get rid of any print statements used for debugging before final submission

Suggested change
print(post_dict)

Comment on lines +34 to +37
title = post_dict['title']
description = post_dict['description']

task = Task(title=title, description=description, completed_at = completed_at)

Choose a reason for hiding this comment

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

this works just fine! we can also combine these lines, so that we aren't making new variables

Suggested change
title = post_dict['title']
description = post_dict['description']
task = Task(title=title, description=description, completed_at = completed_at)
task = Task(
title=post_dict['title'],
description=post_dict['description'],
completed_at = completed_at
)

Comment on lines +41 to +51
task_dict={}
if completed_at == None:
task_dict['is_complete'] = False
else:
task_dict['is_complete'] = True


task_dict['id'] = task.id
task_dict['title'] = task.title
task_dict['description'] = task.description
return_dict = {"task": task_dict}

Choose a reason for hiding this comment

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

this would make a great instance method in our Task model, like to_json in Solar System or Flasky

Comment on lines +69 to +80
tasks_response = []
for task in task_db:
if task.completed_at == None:
is_complete = False
else:
is_complete = True

task_dict = {'id': task.id,
'title': task.title,
'description': task.description,
'is_complete': is_complete}
tasks_response.append(task_dict)

Choose a reason for hiding this comment

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

hmm where have we seen this before? we did it on lines 41-51 as well.

Comment on lines +98 to +110
if task.completed_at == None:
is_complete = False
else:
is_complete = True

task_dict={}
task_dict['id'] = task.id
task_dict['title'] = task.title
task_dict['description'] = task.description
task_dict['is_complete'] = is_complete
if not (task.goal_id == None):
task_dict['goal_id'] = task.goal_id
return_dict = {"task": task_dict}

Choose a reason for hiding this comment

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

we have the same code here again. we should definitely turn this into an instance method inside the Task model


#-----------------------------------
@goals_bp.route("/<id>",methods = ["GET"])
def get_goal_by_id(id):

Choose a reason for hiding this comment

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

👍

return jsonify(return_dict), 200

#------------PUT Method - update goal by id---------
@goals_bp.route("/<id>", methods=["PUT"])

Choose a reason for hiding this comment

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

👍

return_dict = {"goal": goal_dict}
return jsonify(return_dict), 200
#------------remove goal by id------------
@goals_bp.route("/<id>", methods=["DELETE"])

Choose a reason for hiding this comment

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

👍

#----------------WAVE 6-------------------------
#---------------------------------------------------

@goals_bp.route("/<goal_id>/tasks", methods=["POST"])

Choose a reason for hiding this comment

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

👍

}
return jsonify(response_dict), 200

@goals_bp.route("/<goal_id>/tasks", methods=["GET"])

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants