- We can probably move this sort of functionality in the ApplicationController. We probably need to access the current user in other places besides this specific controller in the future.
- In the index controller action we are building up the query based off certain params being passed in. This query generation does not belong in the controller. We can use something like the interactor pattern to pull this logic out and generate our query for us, or the next best place would be in the model.
- I think a nice added feature to this do list is being able to do a bulk update on tasks that are completed. Since we are just using RoR here with no JavaScript we get a new page load everytime we have completeted one todo item on our list.
- Also we may want a feature where we can undo a todo list item. There is no way to recover something if we accidentaly mark it as done.
- I'm not sure
save!is what we want here since it will trigger a ActiveRecord exception, and will blow up our app since we are not catching that exception anywhere. Also the error message in the else statement is not very helpful. I think we can call @list.errors and get a list of validation errors on why it did not save correctly.
- Probably an uneeded comment
https://gist.github.com/maxrice/d92042a88ab14fb80bf45b7a53b548ba#file-app-models-todo_item-rb-L1
- A nitpick but I would rather call this
Iteminstead.
https://gist.github.com/maxrice/d92042a88ab14fb80bf45b7a53b548ba#file-app-models-todo_item-rb-L6
- Also an uneeded comment
- We should use strong params here where we require a todolist and permit certain params for that todolist.
https://gist.github.com/maxrice/d92042a88ab14fb80bf45b7a53b548ba#file-app-models-todo_list-rb-L5
- I don't really see the need for this method. We can just simply assign the newly created to do list to the current user upon creation. It is probably best to avoid unnecessary callbacks here.
https://gist.github.com/maxrice/d92042a88ab14fb80bf45b7a53b548ba#file-app-models-user-rb-L10
- My understading is we can probably avoid extra callbacks to the db by simply using ||= for our current user, and we can reset the current user when they log out. Anyways current user is something that should be managed in the ApplicationController and not on the Model level.