Shop HW project feedback

[LVALL]

Models

Cart

1. for total_price method I would suggest alternative approach with SQL query instead of using Ruby map approach.

   def total_price
   line_items.joins(:product).sum('products.price * line_items.quantity')
   end

   SQL approach is more efficient so try to avoid using Ruby array methods for queries where possible. Count of select queries reduced from 4 to 2

   

Line Item

1. Fat model: The LineItem model has two after-commit callbacks that are responsible for broadcasting updates to the UI. Also you added two methods that will be used only for rendering, it can make the model code harder to read and maintain. Consider moving the broadcasting logic to a separate class or module (e.g. LineItemBroadcastable).

2. N+1 queries: The total_line_item_quantity method calls self.cart.line_items.includes(:product).sum(&:quantity), which could result in N+1 queries if you have a large number of line items. To avoid this, you can use eager loading. Also try to use SQL approach instead of Ruby methods.

3. DRY: The total_line_item_quantity and cart_total_price methods are both used to calculate the total price of items in the cart. To avoid duplication in cart_total_price you may use total_price method from Cart model.

4. There is no need to add self. in model methods. For self.cart and cart the result will be the same inside the LineItem class

Product

1. The same suggestion as for LineItem model. It's better to encapsulate the rendering logic in a separate module

2. line_items_quantity and total_line_item_quantity partially duplicated. It violates the DRY principles. (You may add the total_quantity method in LineItem model and then use this method here, in Product model)

3. The add_default_image method in the Product model seems reasonable, as it ensures that a default image is attached to a product if no image is uploaded. However, you may want to consider some potential improvements: 3.1) Move the default image to a configurable location, e.g., a constant or configuration file, so that it's easier to change or update the default image in the future. 3.2) Consider using Rails.root.join instead of File.open to construct the path to the default image. This will ensure that the path is relative to the Rails application root and avoid hardcoding the path separator. 3.3) You may want to consider adding a validation to ensure that an image is attached to the product after it's created or updated. This will prevent products from being created or updated without an image, even if the default image fails to attach for some reason.

User

I don't see any validations provided for this model, but believe that we want to have them

Controllers

Categories

I would suggest the following refactoring for this controller:

class CategoriesController < ApplicationController
  before_action :set_category
  before_action :set_products, only: :show

  def show
    flash.now[:alert] = 'Products in category not found' unless @products.exists?
  end

  private
    def set_category
      @category = Category.find(params[:id])
    rescue ActiveRecord::RecordNotFound
      flash[:alert] = 'Category not found'
      redirect_to root_path
    end

    def set_products
      @products = @category.products.with_attached_image
    end
end

Overall, the refactored version separates the responsibilities of setting the category and setting the products into two separate methods, making the code easier to read and maintain. It also adds error handling for when a category is not found, which provides a better user experience.

LineItems

1. The update method is a method in which object data must be changed (one attribute or several at once). Removing redundant methods and replacing them with the update method meant not renaming the method, leaving the body the same, but refactoring it as follows:

   
   def update
   if @line_item.update(line_item_params)
   ...
   else
   ...
   end
   end

private def line_item_params params.permit(your_params_here) end

For LineItems controller, it would be more expedient to keep the presence of the `change_quantity` method, since in addition to the update, an additional check for the type of operation (+/-) is present 

#### Orders
1. if `@order.nil? render template: 'layouts/404'` condition does not makes sense, because if the order is not found - the ActiveRecord::RecordNotFound will be raised and default 404 page rendered

2. As I've mentioned in the previous section (LineItems) - update method must be a common place for changing all the permitted params for a model. This is the U in CRUD and so `update` method shouldn't be scoped just for payment status updates. (consider creation of `pay` or `process_payment` method additionally)

#### Products

1. we don't need the ability to manage products outside of the Admin dashboard, so this controller is not needed

[LVALL]

overall the projects works good and looks presentable, well done! 🚀