Shopping cart in Rails for karaoke items

Question

This is my first cart implementation in Rails so I wanted to get some feedback on the correct method to implement it.

First of all, I am storing the data directly in the session because the items in the cart are small.

The idea is that I save a list of integers(ids of the items) in session[]. When a user clicks on "Add to Cart" then the id of that product will be added to the list stored in the rails session variable.

Application controller

class ApplicationController < ActionController::Base
  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
  protect_from_forgery with: :exception

  before_action :set_cart

  private

    def set_cart

      if session[:cart]
        @cart = session[:cart]
      else
        session[:cart] = []
        @cart = session[:cart]
      end

    end

end

Main index file lists entries with "Add to Cart" button for each entry. After the user clicks on "Add to Cart" then the add_item method of CartController is called.

<ol>

  <% @karaokes.each do |karaoke| %>

    <li>
      <%= link_to karaoke.name, karaoke_path(karaoke) %>  
      <%= link_to "Add to Cart", add_path(karaoke) %>  
    </li>
  <% end %>


</ol>

Cart Controller looks as follows:

class CartController < ApplicationController


    def add_item

       k = Karaoke.find(params[:id])

       if [email protected]?(k.id)
        @cart.push(k.id)    
       end

       redirect_to cart_path

    end

    def list_items

       @items = @cart

    end

    def remove_item

       @cart.delete_at(params[:index].to_i)

       redirect_to cart_path


    end


end

Finally the cart page looks as follows which lists all the items added to cart.

<h1> Cart </h1>

<% @items.each_with_index do |itm, index| %>

    <p> 
        <%= Karaoke.find(itm).name %>
        <%= link_to "Remove", remove_path(index) %>
    </p>

<% end %>

Answer 1

Generally looks great.

Since @cart is basically a list of Karaoke ids, you could update the list_items method to look like that:

def list_items
  @items = Karaoke.find(@cart)
end

Then the cart view can be transformed into:

<h1> Cart </h1>
<% @items.each do |item| %>
  <p>
    <%= item.name %>
    <%= link_to "Remove", remove_path(item) %>
  </p>
<% end %>

Using a Set instead of plain array enables us to remove the presence checking and make sure items are always unique in the cart:

class ApplicationController < ActionController::Base
  ...

  def set_cart
    session[:cart] ||= []
    @cart = Set.new(session[:cart])
  end
end

class CartController < ApplicationController
  def add_item
   @cart.add(karaoke_id) if Karaoke.exists?(karaoke_id)

   redirect_to cart_path
  end

  def remove_item
    @cart.delete(karaoke_id)

    redirect_to cart_path
  end

  def karaoke_id
    params[:id].to_i
  end
end

As a side note, I know it's kind of a personal preference, but consider using 2 spaces for indentation as per the style guide.