Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good overall, but please review my comments, and let me know if you have any questions.
| return { | ||
| "id": goal.id, | ||
| "task_ids": task_ids | ||
| }, 200 |
There was a problem hiding this comment.
Rather than hard-coding this dict result here, we could create an additional instance method on Goal to generate this output, or a standalone helper. THat could help tidy up this function.
| "id": goal.id, | ||
| "title": goal.title, |
There was a problem hiding this comment.
Notice that the id and title keys here are the same as for a regular Goal GET, for which we wrote the to_dict helper. Here, we could use our to_dict to get the regular result dictionary, and then add in the tasks key with the loaded task data.
Or we could make a different helper that itself uses the Goal to_dict for its basic structure, but adds in the Task content itself. This would tidy up the response generation for this route.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
👍 Thanks for the update. Looks good.
| task_as_dict["is_complete"] = (False if self.completed_at is None else | ||
| self.completed_at) | ||
| True) |
There was a problem hiding this comment.
👍 This will now consistently return a boolean value.
Here, even though we only use this definition of being complete in just this function, it would still be worth moving it to a model instance method (like is_complete()) so that the calculated value could be available in other places, and here, calling that function would simplify understanding the output logic for the reader.
2 participants
No description provided.