4
\$\begingroup\$

I've written a method for an API call that updates a store menu. It takes JSON as input, in the following format:

{
    "menu" : {
        "item_groups" : [
            {
                "id" : 1,
                "name" : "Drinks",
                "items" : [
                    {
                        "id" : 1,
                        "name" : "Coffee",
                        "option_groups" : [
                            {
                                "id" : 1,
                                "name" : "sizes",
                                "options" : [
                                    {
                                        "id" : 1,
                                        "name" : "small",
                                        "price" : "$1.00"
                                    },
                                    {
                                        "name" : "medium",
                                        "price" : "$2.00"
                                    },
                                ]
                            }
                        ]
                    }
                ]
            }
        ]
    }
}

So for that input JSON, assume that all objects exist in the database except for the medium $2.00 option, which is added since it has no associated id.

To cut down on code, I'll simply describe the model:

  • Each menu has many item groups
  • Each item group has many items
  • Each item has many option groups
  • Each option group has many options
  • Each option has a name and a price

There are also name, id, and priority attributes for each of these. The priority is simply an index of their order as they appear in the client view. Also each object has an appropriately named property representing the inverse relation to its parent (ex, menu.itemGroups[0].menu == menu).

The code I wrote does the following things, in a nutshell:

  • Updates the attributes of each object
  • Infers object deletion by detecting objects that are not present in the JSON request, as compared to the database
  • Infers object priority by the order in which the objects appear in the JSON request
  • Creates new objects for which there is either no id in the JSON request, or if no object can be found with that id.
  • Outputs the updated Menu object as JSON

def menu_params
  params.require(:menu).permit(
      item_groups: [
        :id,
        :name,
        items: [
          :id,
          :name,
          option_groups: [
            :id,
            :name,
            options: [
              :id,
              :name,
              :price    
            ]
          ]
        ]
      ]
    )
end

def update
  deny_access and return unless user_can_edit_store_menu?(current_user, @store)

  params_menu = menu_params

  if !params_menu
    render json: { message: "Nothing to update." }, status: :ok
    return
  end

  # get a reference to this menu's item groups
  db_itemGroups_array = @menu.itemGroups.to_ary
  new_itemGroups_array = []

  # loop through json item groups
  itemGroupIndex = 0
  params_menu[:item_groups].each { |params_itemGroup|
    next if params_itemGroup[:items].empty?

    # if id param doesn't exist
    if (ig_id = params_itemGroup[:id]) != nil
      # if an itemGroup with this id param can be found
      db_itemGroup = db_itemGroups_array.find { |o| o.id == ig_id.to_i }
    end

    # create a new item group if there isn't one already
    db_itemGroup ||= MenuItemGroup.new(menu: @menu)
    db_itemGroup.name = params_itemGroup[:name]
    new_itemGroups_array << db_itemGroup

    # get the array of items
    db_items_array = db_itemGroup.items.to_ary
    new_items_array = []

    # loop through json items
    itemIndex = 0
    params_itemGroup[:items].each { |params_item|
      next if params_item[:option_groups].empty?

      # check that param exists for item id
      if (i_id = params_item[:id]) != nil
        # search for a match in the database's cached array
        db_item = db_items_array.find { |item| item.id == i_id.to_i }
      end

      # create a new item if there isn't one already
      db_item ||= MenuItem.new(itemGroup: db_itemGroup)
      db_item.name = params_item[:name]
      new_items_array << db_item

      # get the array of option groups
      db_optionGroups_array = db_item.optionGroups.to_ary
      new_optionGroups_array = []

      # loop through json option groups
      optionGroupIndex = 0
      params_item[:option_groups].each { |params_optionGroup|
        next if params_optionGroup[:options].empty?

        # check that param exists for id
        if (og_id = params_optionGroup[:id]) != nil
          db_optionGroup = db_optionGroups_array.find { |optionGroup| optionGroup.id == og_id.to_i }
        end

        # create an optionGroup if there isn't one already
        db_optionGroup ||= MenuItemOptionGroup.new(item: db_item)
        db_optionGroup.name = params_optionGroup[:name]
        new_optionGroups_array << db_optionGroup

        # get array of options
        db_options_array = db_optionGroup.options.to_ary
        new_options_array = []

        # loop through json options
        optionIndex = 0
        params_optionGroup[:options].each { |params_option|

          # check that the param for id exists
          if (o_id = params_option[:id]) != nil
            db_option = db_options_array.find { |option| option.id == o_id.to_i }
          end

          # create the option and add it to the array if it isn't in there already
          db_option ||= MenuItemOption.new(optionGroup: db_optionGroup)
          new_options_array << db_option

          # assign params data to the model
          db_option.name = params_option[:name]
          db_option.price = params_option[:price] || "$0.00"
          db_option.priority = optionIndex+=1
          db_option.save

        }
        # clean up options array
        destroy_array = db_options_array - new_options_array

        # if all options will be destroyed, just destroy the option group
        if !destroy_array.empty? and new_options_array.empty? and db_options_array.count == destroy_array.count
          db_optionGroup.destroy unless db_optionGroup.new_record?
        else
          destroy_array.each { |d| d.destroy }
          db_optionGroup.priority = optionGroupIndex
          db_optionGroup.save
        end

        optionGroupIndex+=1
      }
      # clean up option groups array
      destroy_array = db_optionGroups_array - new_optionGroups_array

      # save the item record or destroy it if there are no option groups
      if !destroy_array.empty? and new_optionGroups_array.empty? and db_optionGroups_array.count == destroy_array.count
        db_item.destroy unless db_item.new_record?
      else 
        destroy_array.each { |d| d.destroy }
        db_item.priority = itemIndex
        db_item.save
      end
      itemIndex += 1
    }
    # clean up items array 
    destroy_array = db_items_array - new_items_array
    # if the number of items in the db equals the number of items deleted, and there are no new items
    if !destroy_array.empty? and new_items_array.empty? and db_items_array.count == destroy_array.count
      db_itemGroup.destroy unless db_itemGroup.new_record? 

    else
      destroy_array.each { |d| d.destroy }
      db_itemGroup.priority = itemGroupIndex
      db_itemGroup.save
    end
    itemGroupIndex += 1
  }

  # clean up item groups by figuring out what's left in the database and destroying it
  destroy_array = db_itemGroups_array - new_itemGroups_array
  destroy_array.each { |d| d.destroy }

  # save the menu
  @menu.save

  render json: { menu: @menu.json }, status: :ok
end

The code works well, but I'm obsessed with making it better, easier to read, etc. I employed all my remedial knowledge of Rails to try to make this as efficient as possible, and I would love it if anyone pointed out the weak spots of this code.

I think I see potential for perhaps refactoring the repetitive nests into blocks or methods. I would love it if someone could point me in the right direction on the best way to go about this.

| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

Your general data model looks good that controller is just plain crazy - you got way too many levels of nesting going on here. In Rails and MVC in general your controllers should be skinny. If you controller action is longer than 10 lines you are probably doing something wrong.

The Single Responsibility Principle says that a class should have a single responsibility.

Your controller class is responsible for handling 4+ relations. And you have a mind boggling amount of possible code paths, how do you intend to test them all?

You can start by reading up on Active Record Nested Attributes.

Then consider if it really is prudent to manipulate nested records more than 1 layer down in the same action.

If you want to obey the idea of never nesting more that 1 level down then the following is a more sane restful design:

                 Prefix Verb   URI Pattern                                           Controller#Action
      menue_item_groups GET    /menues/:menue_id/item_groups(.:format)               item_groups#index
                        POST   /menues/:menue_id/item_groups(.:format)               item_groups#create
   new_menue_item_group GET    /menues/:menue_id/item_groups/new(.:format)           item_groups#new
        edit_item_group GET    /item_groups/:id/edit(.:format)                       item_groups#edit
             item_group GET    /item_groups/:id(.:format)                            item_groups#show
                        PATCH  /item_groups/:id(.:format)                            item_groups#update
                        PUT    /item_groups/:id(.:format)                            item_groups#update
                        DELETE /item_groups/:id(.:format)                            item_groups#destroy
                 menues GET    /menues(.:format)                                     menues#index
                        POST   /menues(.:format)                                     menues#create
              new_menue GET    /menues/new(.:format)                                 menues#new
             edit_menue GET    /menues/:id/edit(.:format)                            menues#edit
                  menue GET    /menues/:id(.:format)                                 menues#show
                        PATCH  /menues/:id(.:format)                                 menues#update
                        PUT    /menues/:id(.:format)                                 menues#update
                        DELETE /menues/:id(.:format)                                 menues#destroy
       item_group_items GET    /item_groups/:item_group_id/items(.:format)           items#index
                        POST   /item_groups/:item_group_id/items(.:format)           items#create
    new_item_group_item GET    /item_groups/:item_group_id/items/new(.:format)       items#new
              edit_item GET    /items/:id/edit(.:format)                             items#edit
                   item GET    /items/:id(.:format)                                  items#show
                        PATCH  /items/:id(.:format)                                  items#update
                        PUT    /items/:id(.:format)                                  items#update
                        DELETE /items/:id(.:format)                                  items#destroy
     item_option_groups GET    /items/:item_id/option_groups(.:format)               option_groups#index
                        POST   /items/:item_id/option_groups(.:format)               option_groups#create
  new_item_option_group GET    /items/:item_id/option_groups/new(.:format)           option_groups#new
      edit_option_group GET    /option_groups/:id/edit(.:format)                     option_groups#edit
           option_group GET    /option_groups/:id(.:format)                          option_groups#show
                        PATCH  /option_groups/:id(.:format)                          option_groups#update
                        PUT    /option_groups/:id(.:format)                          option_groups#update
                        DELETE /option_groups/:id(.:format)                          option_groups#destroy
   option_group_options GET    /option_groups/:option_group_id/options(.:format)     options#index
                        POST   /option_groups/:option_group_id/options(.:format)     options#create
new_option_group_option GET    /option_groups/:option_group_id/options/new(.:format) options#new
            edit_option GET    /options/:id/edit(.:format)                           options#edit
                 option GET    /options/:id(.:format)                                options#show
                        PATCH  /options/:id(.:format)                                options#update
                        PUT    /options/:id(.:format)                                options#update
                        DELETE /options/:id(.:format)                                options#destroy

Pay particular attention to the controller/action part on the far right.

The routes are as follows:

resources :menues, shallow: true do
  resources :item_groups do 
  end
end

resources :item_groups, shallow: true, only: [] do
  resources :items do 
  end
end

resources :items, shallow: true, only: [] do
  resources :option_groups do 
  end
end

resources :option_groups, shallow: true, only: [] do
  resources :options do 
  end
end
| improve this answer | | | | |
\$\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.