Code Review

Hello Everyone, I have reviewed a codebase of one of our projects. It was a good experience.

Few good things.

  1. Really clean code.
  2. Small methods.
  3. I had to travel 5 months back commits to find some bad code. :)

Things to improve on:

  1. Found less specs, But it is possible that specs have been covered already.

Overall Thoughts:

The code is super clean, liked it!. fantabulous work folks!

Snippets:

Code:

 Menu.pluck(:hyphenated_name)

Suggestion:
This can be moved to class method which can be cached instead of firing the queries in the app again and again, one problem with this approach is that we need to reload the cache when a new menu is added, which can be done through Menu callbacks.

ex:

  class Menu
    def self.cached_hyphenated_names
      @hyphenated_names ||= Menu.pluck(:hyphenated_name)
    end
  end

Code:

  before(:all) do
    Menu.delete_all
    generate_menus
  end

Suggestion:

Should Avoid .delete_all in specs. These kind of stuffs make our specs unstable and we are forced to maintain the before all block for the specs that we will add in the future. We can use cleaner gem to avoid this delete_all calls.

Code:

  if current_order
    current_order.calculate_shipping_cost_and_shipping_tax
    update_guest_user_email_and_create_order_billing_address
    stripe_card_token = params[:stripe_card_token]
    customer = create_stripe_customer(stripe_card_token)
    @order = Order.find(current_order.id).decorate
    @order.update_order(customer)
    @order.delete_shipments_without_item
    params[:coupon_code].present? && update_discount_coupon
    current_order.checkout
  else
    redirect_to carts_path
  end

Suggestion:

Move methods to AR callbacks if possible, and test them in specs.
4)
Code:

  def create
    if !current_user.default_address
      params['address']['is_default'] = true
    end
    super
  end

Suggestion:

I see that we are using inherited_resources, but we are not using it to its full potential.
This can be done this way too. Also, note I have used the return value of .blank? as assignment for is_default.

  def create
    resource.is_default = current_user.default_address.blank?
    super
  end

Code:

  file = "#{Rails.root}/config/order_confirmation_email.yml"
  seller_email = YAML.load_file(file)[Rails.env]["seller_email"]

Suggestion:

I saw this code were called inside the perform method of Job. Ideally it should be present outside of the JOB and should be accessed as constants

Code:

  UserMailer.new.charged_successfully_notification_for_seller(seller_email, amount,
   customer_name, email, order_number).deliver

Suggestion:

There is a huge argument dependency here, I need to remember the order of the arguments and number of arguments that needs to passed to method. We need to change the code something similar to this.

  UserMailer.new.charged_successfully_notification_for_seller(
    seller_email:   seller_email,
    amount:         amount,
    customer_name:  customer_name,
    email:          email,
    order_number:   order_number
  ).deliver

By changing the code this way, I dont need to remember the order.

Its generally not recommended to have more than two arguments for a method

Code:

    module ActiveAdmin
      class ResourceDSL
        def filter(*args)
          field = args[0]
          klass = @config.resource_class_name.constantize
          type  = klass.columns.select{|c| c.name == field.to_s}.first.try(:type)
          if type == :datetime
            controller do
              before_filter only: :index do
                unless params["q"].blank?
                  gte = params["q"]["#{field}_gte"]
                  lte = params["q"]["#{field}_lte"]
                  params["q"]["#{field}_gte"] &&= gte.to_date.at_beginning_of_day
                  params["q"]["#{field}_lte"] &&= lte.to_date.end_of_day
                end
              end
            end
          end
          super
        end
      end
    end

Suggestion:

unless params["q"].blank?

this is confusing. it can be written this way

if params ["q"]

whole unless block can be moved to a meaningful instance method of ResourceDSL with params as argument.

May be some of my suggestions are not suitable in some cases, Just let me know if anything can be further improved.

Thanks