-
Notifications
You must be signed in to change notification settings - Fork 47
Space - Charlotte #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ng controller action and ERB view
Task ListMajor Learning Goals/Code Review
Functional Requirements/Manual Testing
Overall Feedback
Additional FeedbackHi Charlotte! Great work on your Task List! (I like the name of it a lot) Overall, your Rails code looks good, and you definitely have a functional Task List! Great work on accomplishing all of those features!! These are the main comments I have:
Let me know what else we can talk about or ask about! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| @today = Date.today | ||
| @task = Task.find_by(id:params[:id]) | ||
| @task.completed_at = @today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since today is a variable that only matters in this action, you can leave off the @ and leave it as the local variable today! (You would make it an instance variable @today if you specifically wanted to share it with the view. However, the view that you redirect to at the end of this method doesn't need an @today instance variable shared with it!)
You may even consider just refactoring it to @task.completed_at = Date.today
|
|
||
| def mark_complete | ||
| @today = Date.today | ||
| @task = Task.find_by(id:params[:id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there is no Task found? :)
| @task = Task.find_by(id:params[:id]) | ||
| @task.completed_at = @today | ||
| @task.save | ||
| redirect_to task_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subtleties in variables: task_path is the named path that goes to the show page for one given task. tasks_path is the named path that goes to the index page for all tasks. task_path normally takes in a task as a parameter, so it'll be more clear to say redirect_to task_path(@task) or redirect_to task_path(@task.id)
| @task = Task.find_by(id:id) | ||
|
|
||
| if @task.nil? | ||
| redirect_to '/tasks' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, you are redirecting directly to a url '/tasks'. In the small chance that this would change, it'll be better to use named paths. In this case, you'd use the named path tasks_path (much like what you did in the update method)!
| <h1>All Tasks</h1> | ||
| <h3>Click on a Task name to view the details:</h3> | ||
|
|
||
| <% if @tasks.length > 0 %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your section to add a new task is within this conditional if @tasks.length > 0. That means you can't add any tasks if there are no tasks existing!
| # see https://guides.rubyonrails.org/routing.html | ||
|
|
||
| # verb 'path' to: 'name of controller#action' | ||
| get '/', to: 'tasks#home', as: 'root' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to let yourself re-think and simplify your approach to the root path. Here, you declare the rootpath to go to the home action, which is an empty action, has an empty view, and doesn't have tests! It might be interesting for some projects to have a home action or root path of simply another action.
|
|
||
| # Act | ||
| get task_path(task.id) | ||
| get tasks_path(task.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be altered-- the correct request for the show page is a GET to task_path
| let (:task) { | ||
| Task.create name: "sample task", description: "this is an example for a test", | ||
| completed_at: Time.now + 5.days | ||
| completed_at: "sample day/time for testing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why did you change this?
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions