3
\$\begingroup\$

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 !@cart.include?(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 %>
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.