Branches-Steph #48
Branches-Steph #48
Conversation
Task ListWhat We're Looking For
Fantastic work on this project, Steph! Your project looks so so so good! The code is all there and works well, and your tests are all there and work well! I have no complaints about this project I only have a few comments that are suggestions on how to refactor the code, and also some few ways to make your code more Rails-y. Lastly, I may say that your TaskList webpages didn't have a direct link to the Task's show page, and in the future, that would be convenient for me :P Otherwise, great work on this, and keep it up! |
|
|
||
| def show | ||
| task_id = params[:id].to_i | ||
| @task = Task.find_by(id: params[:id]) |
There was a problem hiding this comment.
Consider refactoring the above two lines to either
task_id = params[:id].to_i
@task = Task.find_by(id: task_id)or
@task = Task.find_by(id: params[:id])in order to help efficiently use local variables
| end | ||
|
|
||
| def create | ||
| @task = Task.new(name: params[:task][:name], description: params[:task][:description], completed: params[:task][:completed]) |
There was a problem hiding this comment.
In the future, let's use strong params, so refactor this line to:
@task = Task.new(task_params)
|
|
||
| def edit | ||
| task_id = params[:id].to_i | ||
| @task = Task.find_by(id: params[:id] ) |
There was a problem hiding this comment.
Same as my comments above in the show action, consider refactoring these two lines
| @task = Task.find_by(id: params[:id]) | ||
|
|
||
| if [email protected]? | ||
| @task.update(task_params) |
|
|
||
| def toggle_complete | ||
| task_id = params[:id].to_i | ||
| @task = Task.find_by(id: params[:id] ) |
There was a problem hiding this comment.
Same as above-- consider refactoring to use task_id or to get rid of it :)
| # | ||
| # movies = Movie.create([{ name: 'Star Wars' }, { name: 'Lord of the Rings' }]) | ||
| # Character.create(name: 'Luke', movie: movies.first) | ||
| raic |
There was a problem hiding this comment.
Nitpick: This stray code was left here and was broken. Watch out!
|
|
||
| # Assert | ||
| #Step Three once you change the fields, check to see if they are different than original | ||
| #expect(Book.find_by(id: existing_book.id).title).must_equal "Name that you changed book to" |
There was a problem hiding this comment.
Not sure if this would help, but maybe if you can spend the time to remove these comments before submission, or find a way to reword them, that may help with solidfying your understanding
|
|
||
| existing_task = Task.create(name: "cs fun", description: "recursion practice", completed: nil) | ||
| patch complete_path(existing_task) | ||
| expect(Task.find_by(id: existing_task.id).completed).must_be_instance_of ActiveSupport::TimeWithZone |
There was a problem hiding this comment.
Nice! It may look weird, but it makes sense to me and it's what works.
|
|
||
| existing_task = Task.create(name: "cs fun", description: "recursion practice", completed: nil) | ||
| patch complete_path(existing_task) | ||
| expect(Task.find_by(id: existing_task.id).completed).must_be_instance_of ActiveSupport::TimeWithZone |
There was a problem hiding this comment.
This test would be perfect and complete with an assertion to check the response, like must_redirect_to or something
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions