From 3e773656351d8dc21ae6d55f8133de3d20b29ac4 Mon Sep 17 00:00:00 2001 From: Justin Edmund Date: Fri, 30 Dec 2022 05:08:40 -0800 Subject: [PATCH] Refactor #create This method is humongous and very confusing, and often gives double render errors. This refactor breaks things up into smaller methods to help make it a bit more readable. --- .../api/v1/grid_weapons_controller.rb | 112 +++++++++++------- 1 file changed, 68 insertions(+), 44 deletions(-) diff --git a/app/controllers/api/v1/grid_weapons_controller.rb b/app/controllers/api/v1/grid_weapons_controller.rb index 8ecb573..93b1352 100644 --- a/app/controllers/api/v1/grid_weapons_controller.rb +++ b/app/controllers/api/v1/grid_weapons_controller.rb @@ -3,44 +3,24 @@ module Api module V1 class GridWeaponsController < Api::V1::ApiController + attr_reader :party, :incoming_weapon + + before_action :find_party, only: :create + before_action :find_incoming_weapon, only: :create + before_action :check_weapon_compatibility, only: :create before_action :set, except: %w[create update_uncap_level destroy] def create - # BUG: I can create grid weapons even when I'm not logged in on an authenticated party - party = Party.find(weapon_params[:party_id]) - render_unauthorized_response if current_user && (party.user != current_user) - - incoming_weapon = Weapon.find(weapon_params[:weapon_id]) - incoming_weapon.limit - - # Set up conflict_position in case it is used - conflict_position = nil - - if [9, 10, 11].include?(weapon_params[:position].to_i) && ![11, 16, 17, 28, 29].include?(incoming_weapon.series) - raise Api::V1::IncompatibleWeaponForPositionError.new(weapon: incoming_weapon) - end - - # 1. If the weapon has a limit - # 2. If the weapon does not match a weapon already in grid - # 3. If the incoming weapon has a limit and other weapons of the same series are in grid - if incoming_weapon.limit && incoming_weapon.limit.positive? - conflict_weapon = party.weapons.find do |weapon| - weapon if incoming_weapon.series == weapon.weapon.series || - ([2, 3].include?(incoming_weapon.series) && [2, 3].include?(weapon.weapon.series)) - end - - if conflict_weapon - if conflict_weapon.weapon.id != incoming_weapon.id - return render json: ConflictBlueprint.render(nil, view: :weapons, - conflict_weapons: [conflict_weapon], - incoming_weapon: incoming_weapon, - incoming_position: weapon_params[:position]) - else - # Destroy the original grid weapon - # TODO: Use conflict_position to alert the client that that position has changed - conflict_position = conflict_weapon.position - GridWeapon.destroy(conflict_weapon.id) - end + if conflict_weapon + if conflict_weapon.weapon.id != incoming_weapon.id + # Render the conflict view as a string and assign it to a variable + conflict_view = render_conflict_view(conflict_weapon, incoming_weapon, weapon_params[:position]) + return render json: conflict_view + else + # Destroy the original grid weapon + # TODO: Use conflict_position to alert the client that that position has changed + conflict_position = conflict_weapon.position + GridWeapon.destroy(conflict_weapon.id) end end @@ -60,15 +40,9 @@ module Api party.save! end - # Render the new weapon and any weapons changed - return unless weapon.save! - - render json: GridWeaponBlueprint.render(weapon, view: :full, - root: :grid_weapon, - meta: { - replaced: conflict_position - }), - status: :created + # Render the grid weapon view as a string and assign it to a variable + grid_weapon_view = render_grid_weapon_view(weapon, conflict_position) + render json: grid_weapon_view, status: :created if weapon.save! end def resolve @@ -121,6 +95,56 @@ module Api private + def check_weapon_compatibility + return if compatible_with_position?(incoming_weapon, weapon_params[:position]) + + raise Api::V1::IncompatibleWeaponForPositionError.new(weapon: incoming_weapon) + end + + # Check if the incoming weapon is compatible with the specified position + def compatible_with_position?(incoming_weapon, position) + false if [9, 10, 11].include?(position.to_i) && ![11, 16, 17, 28, 29].include?(incoming_weapon.series) + true + end + + def conflict_weapon + @conflict_weapon ||= find_conflict_weapon(party, incoming_weapon) + end + + # Find a conflict weapon if one exists + def find_conflict_weapon(party, incoming_weapon) + return unless incoming_weapon.limit + + party.weapons.find do |weapon| + weapon if incoming_weapon.series == weapon.weapon.series || [2, 3].include?(weapon.weapon.series) + end + end + + def find_incoming_weapon + @incoming_weapon = Weapon.find(weapon_params[:weapon_id]) + @incoming_weapon.limit + end + + def find_party + # BUG: I can create grid weapons even when I'm not logged in on an authenticated party + @party = Party.find(weapon_params[:party_id]) + render_unauthorized_response if current_user && (party.user != current_user) + end + + # Render the conflict view as a string + def render_conflict_view(conflict_weapon, incoming_weapon, incoming_position) + ConflictBlueprint.render(nil, view: :weapons, + conflict_weapons: [conflict_weapon], + incoming_weapon: incoming_weapon, + incoming_position: incoming_position) + end + + def render_grid_weapon_view(grid_weapon, conflict_position) + GridWeaponBlueprint.render(grid_weapon, view: :full, + root: :grid_weapon, + meta: { replaced: conflict_position }) + end + def set @weapon = GridWeapon.where('id = ?', params[:id]).first end