Saving nested JSON to db, inferring order and record deletion

Question

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.

Answer 1

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