Skip to content

Melinda H. Pine #86

Open
mhayes2019 wants to merge 13 commits intoAda-C16:masterfrom
mhayes2019:master
Open

Melinda H. Pine #86
mhayes2019 wants to merge 13 commits intoAda-C16:masterfrom
mhayes2019:master

Conversation

@mhayes2019
Copy link

No description provided.

Copy link

@jbieniosek jbieniosek left a comment

Choose a reason for hiding this comment

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

Great work on this project Melinda! You hit all of the learning goals. My only recommendation is to look for additional places to use helper functions to DRY your code. This project is green!

Comment on lines +32 to +34
list = []
for task in self.tasks:
list.append(task.to_dict)

Choose a reason for hiding this comment

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

I think there's a small bug here:

Suggested change
list = []
for task in self.tasks:
list.append(task.to_dict)
list = []
for task in self.tasks:
list.append(task.to_dict())

This loop is also a great candidate for a list comprehension similar to line 28:

Suggested change
list = []
for task in self.tasks:
list.append(task.to_dict)
list = [task.to_dict() for task in self.tasks]


}

def check_for_complete_task(self):

Choose a reason for hiding this comment

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

👍



tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks")
def valid_int(number,parameter_type):

Choose a reason for hiding this comment

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

Great helper!

Comment on lines +36 to +43
tasks_response.append(
{
"description": task.description,
"id": task.id,
"is_complete": False if has_complete == None else has_complete,
"title": task.title,
}
)

Choose a reason for hiding this comment

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

This works, I recommend using the to_dict helper function to DRY this section:

Suggested change
tasks_response.append(
{
"description": task.description,
"id": task.id,
"is_complete": False if has_complete == None else has_complete,
"title": task.title,
}
)
tasks_response.append(task.to_dict())

@tasks_bp.route("/<task_id>", methods=["GET", "PUT", "DELETE"])
def handle_one_task(task_id):
valid_int(task_id,"task_id")
task = Task.query.get_or_404(task_id)

Choose a reason for hiding this comment

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

Great use of get_or_404!

json_response = jsonify(response)
return make_response(json_response, 200)

def slack_bot(title):

Choose a reason for hiding this comment

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

Great helper!


#Wave 3
@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def handle_completed_task(task_id):

Choose a reason for hiding this comment

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

Great work, the use of helper functions and get_or_404 in this function really streamline the code!

tasks = db.relationship("Task", backref="goal", lazy=True)

def to_dict(self):
if self.list_of_task_ids():

Choose a reason for hiding this comment

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

Storing this result of function call here in a variable (ie task_ids = self.list_of_task_ids()) and then testing could create a slight optimization, because then the variable could be used on line 19 ("task_ids": task_ids) instead of calling the function again.

goal.title = form_data["title"]

db.session.commit()
return jsonify({"goal":{"goal_id":goal.id, "title":goal.title}}),200

Choose a reason for hiding this comment

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

Suggested change
return jsonify({"goal":{"goal_id":goal.id, "title":goal.title}}),200
return jsonify({"goal":goal.to_dict()}),200


#Wave # 6
@goals_bp.route("/<goal_id>/tasks", methods=["POST"])
def post_task_ids_to_goal(goal_id):

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