_todo_list.html.erb

<%-# Display the current user's lists along with any uncompleted items -%>

<%= render collection: @todo_lists %>

# Changes
# * Ensure the controller includes "todo_items" in query to prevent n+1 issues
# * Make naming more conforming...list => todo_list
# * Use a presenter to format list title
# * Swap query for scope "where(done: false)" becomes "not_done".
# * Move `complete_item_todo_lists` to todo_list_items_controller
# * Extract a partial of lists under `/app/views/todo_lists/_todo_list.html.erb` (you can see this work in the show.erb file below)

show.html.erb

<h2><%= @list.formatted_title %></h2>
<ul>
  <% @list.todo_items.where(done: false).each do |item| %>
    <li><% item.title %> - <%= link_to "Done!", complete_item_todo_lists(item), method: :put %></li>
  <% end %>
</ul>

# Changes
# * This one is a bit more fun, this file and the list view are very similar. By extracting this duplicate code we can make
#   the app more easily maintained. Sample implementation below
# * Button up the naming, make names more consistent

# app/views/todo_lists/_todo_list.html.erb
<h2><%= todo_list.formatted_title %></h2>
<ul>
  <% todo_list.todo_items.each do |todo_item| %>
    <li><% todo_item.title %> - <%= link_to "Done!", complete_item_todo_lists(todo_item), method: :put %></li>
  <% end %>
</ul>

# Then reuse it for this show view.

<%= render partial: 'todo_list', todo_list: @todo_list %>

todo_item.rb

class TodoItem < ApplicationRecord
  belongs_to :todo_list

  validates :title, presence: true

  # Completes to do item, saving to database.
  def mark_as_done!
    update!(done: true)
  end
end

# Changes
# * Change method name "complete!" to represent domain language. Arguably changing the column name is equally valid.

todo_list.rb

class TodoList < ApplicationRecord
  has_many :todo_items
  belongs_to :user
  
  def formatted_title
    title + (todo_items.empty? ' (Complete)' : '')
  end
end

# Changes
# * remove the coupling to current_user in model. This is a side-effect that makes testing more difficult and is a bit more
#   "magical" than necessary. The controller code should probably be where this logic resides.
# * I'd extract "formatted_title" into a presenter or decorator object where needed. This is view/presentation code and it's
#   home is likely in one of those places. Arguably a helper method would be ok too, but these are less re-usable and more difficult
#   to test as they need a "view_context" object.

todo_lists_controller.rb

class TodoListsController < ApplicationController  
  def index
    @lists = current_user.todo_lists.order("#{sort_by} #{sort_order}")
  end
  
  def show
    @list = current_user.todo_lists.find(params[:id])
  end
  
  # Mark an item as done and return user back from whence they came!
  def complete_item
    TodoItem.find(params[:id]).complete!
    redirect_to :back
  end

  def new
    @list = TodoList.new
  end

  def create
    @list = TodoList.new(todo_list_params.merge(user: current_user))

    if @list.save!
      redirect_to todo_lists_url
    else
      flash[:notice] = "Error saving list"
      redirect_to new_todo_list_url
    end
  end
  
  private
  
  def todo_list_params
    params.require(:todo_list).permit(:title)
  end
  
  def sort_by
    @sort_by ||= params[:sort] || 'created_at'
  end
  
  def sort_order
    @sort_order ||= (params[:asc] == 'false') ? 'ASC' : 'DESC'
  end
end

class ApplicationController
  def current_user
    @current_user ||= User.find(session[:user_id])
  end
end

# Changes
# * extract current_user out into a memoized and shared method. This improves performance and likely dries up code 
#   elsewhere in the app.
# * use strong params to enforce allowed writes/behavior
# * lean on relations to implify todolist lookup in #index method
# * extract and memoize sorting logic
# * scope todolists by current_user in show method to prevent unexpected access
# * ensure current user is set when creating a todo list. This prevents unexpected access denials.
# * Move `complete_item` method and logic to todo_list_items_controller.rb

# More:
# There is more possible here like changing url helpers to path helpers, ensuring the redirect after completing is safe, etc... 
# But these and the thngs above give a good example of what I would look to improve.

User.rb

class User < ApplicationRecord
  has_many :todo_lists
  
  def self.set_current_user(id)
    @_current_user = find(id)
  end
  
  # Avoid a DB query each time we need the currently logged in user
  def self.current_user
    @_current_user
  end
end

# Changes/Advice
# * Don't memorize current_user in "set_current_user". This leads to bugs around returning the wrong user.
# * Generally speaking current_user being in the model is not a great thing. It tends not to be thread safe. If we have to do it
#   I have an implementation below.

class User < ActiveRecord
  def self.current_user(user)
    if user.present?
      Thread.current[:user] = user
    else
      Thread.current[:user]
    end
  end
end