From 0225db6840e47b6894bcfbb8cd8bb889f6628383 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 5 May 2025 12:35:32 +1000 Subject: [PATCH] Refactor without setter methods This class was originally built to flexibly accept paramters in any order. It also allowed you to specify multiple of the same type of parameter, with the later one overriding the earlier. This is too flexible and likely to cause mistakes. And besides, we don't use that feature! --- .rubocop_styleguide.yml | 4 --- .../concerns/checkout_callbacks.rb | 3 +- .../admin/subscription_customer_serializer.rb | 2 +- app/serializers/api/admin/user_serializer.rb | 4 +-- lib/open_food_network/address_finder.rb | 31 +++-------------- .../open_food_network/address_finder_spec.rb | 33 ++++--------------- 6 files changed, 15 insertions(+), 62 deletions(-) diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index 7f80e7d866..ead7f299be 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -86,10 +86,6 @@ Metrics/MethodLength: Enabled: true Max: 25 # default 10 -Naming/MemoizedInstanceVariableName: - Exclude: - - 'lib/open_food_network/address_finder.rb' - Metrics/ParameterLists: CountKeywordArgs: false diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb index b6dbbfd6c1..bf6a23b860 100644 --- a/app/controllers/concerns/checkout_callbacks.rb +++ b/app/controllers/concerns/checkout_callbacks.rb @@ -48,7 +48,8 @@ module CheckoutCallbacks end def load_saved_addresses - finder = OpenFoodNetwork::AddressFinder.new(@order.email, @order.customer, spree_current_user) + finder = OpenFoodNetwork::AddressFinder.new(email: @order.email, customer: @order.customer, + user: spree_current_user) @order.bill_address ||= finder.bill_address @order.ship_address ||= finder.ship_address diff --git a/app/serializers/api/admin/subscription_customer_serializer.rb b/app/serializers/api/admin/subscription_customer_serializer.rb index 66e676aee4..16243067b2 100644 --- a/app/serializers/api/admin/subscription_customer_serializer.rb +++ b/app/serializers/api/admin/subscription_customer_serializer.rb @@ -10,7 +10,7 @@ module Api delegate :ship_address, to: :finder def finder - @finder ||= OpenFoodNetwork::AddressFinder.new(object, object.email) + @finder ||= OpenFoodNetwork::AddressFinder.new(customer: object, email: object.email) end end end diff --git a/app/serializers/api/admin/user_serializer.rb b/app/serializers/api/admin/user_serializer.rb index 462515d112..116397e245 100644 --- a/app/serializers/api/admin/user_serializer.rb +++ b/app/serializers/api/admin/user_serializer.rb @@ -11,11 +11,11 @@ module Api has_one :bill_address, serializer: Api::AddressSerializer def ship_address - OpenFoodNetwork::AddressFinder.new(object.email, object).ship_address + OpenFoodNetwork::AddressFinder.new(email: object.email, user: object).ship_address end def bill_address - OpenFoodNetwork::AddressFinder.new(object.email, object).bill_address + OpenFoodNetwork::AddressFinder.new(email: object.email, user: object).bill_address end def confirmed diff --git a/lib/open_food_network/address_finder.rb b/lib/open_food_network/address_finder.rb index 32975f9428..0e681bcb6f 100644 --- a/lib/open_food_network/address_finder.rb +++ b/lib/open_food_network/address_finder.rb @@ -10,13 +10,10 @@ module OpenFoodNetwork class AddressFinder attr_reader :email, :user, :customer - def initialize(*args) - args.each do |arg| - type = types[arg.class] - next unless type - - public_send("#{type}=", arg) - end + def initialize(email: nil, user: nil, customer: nil) + @email = email + @user = user + @customer = customer end def bill_address @@ -27,28 +24,8 @@ module OpenFoodNetwork customer_preferred_ship_address || user_preferred_ship_address || fallback_ship_address end - def email=(arg) - @email ||= arg - end - - def customer=(arg) - @customer ||= arg - end - - def user=(arg) - @user ||= arg - end - private - def types - { - String => "email", - Customer => "customer", - Spree::User => "user" - } - end - def customer_preferred_bill_address customer&.bill_address end diff --git a/spec/lib/open_food_network/address_finder_spec.rb b/spec/lib/open_food_network/address_finder_spec.rb index c7068eefe9..79462bb28b 100644 --- a/spec/lib/open_food_network/address_finder_spec.rb +++ b/spec/lib/open_food_network/address_finder_spec.rb @@ -12,39 +12,18 @@ module OpenFoodNetwork let(:customer) { create(:customer) } context "when passed any combination of instances of String, Customer or Spree::User" do - let(:finder1) { AddressFinder.new(email, customer, user) } - let(:finder2) { AddressFinder.new(customer, user, email) } + let(:finder1) { AddressFinder.new(email:, customer:, user:) } - it "stores arguments based on their class" do + it "stores arguments" do expect(finder1.email).to eq email - expect(finder2.email).to eq email expect(finder1.customer).to be customer - expect(finder2.customer).to be customer expect(finder1.user).to be user - expect(finder2.user).to be user - end - end - - context "when passed multiples instances of a class" do - let(:email2) { 'test2@example.com' } - let(:user2) { create(:user) } - let(:customer2) { create(:customer) } - let(:finder1) { AddressFinder.new(user2, email, email2, customer2, user, customer) } - let(:finder2) { AddressFinder.new(email2, customer, user, email, user2, customer2) } - - it "only stores the first encountered instance of a given class" do - expect(finder1.email).to eq email - expect(finder2.email).to eq email2 - expect(finder1.customer).to be customer2 - expect(finder2.customer).to be customer - expect(finder1.user).to be user2 - expect(finder2.user).to be user end end end describe "fallback_bill_address" do - let(:finder) { AddressFinder.new(email) } + let(:finder) { AddressFinder.new(email:) } let(:address) { double(:address, clone: 'address_clone') } context "when a last_used_bill_address is found" do @@ -65,7 +44,7 @@ module OpenFoodNetwork end describe "fallback_ship_address" do - let(:finder) { AddressFinder.new(email) } + let(:finder) { AddressFinder.new(email:) } let(:address) { double(:address, clone: 'address_clone') } context "when a last_used_ship_address is found" do @@ -92,7 +71,7 @@ module OpenFoodNetwork create(:completed_order_with_totals, user: nil, email:, distributor:, bill_address: nil) } - let(:finder) { AddressFinder.new(email) } + let(:finder) { AddressFinder.new(email:) } context "when searching by email is not allowed" do before do @@ -142,7 +121,7 @@ module OpenFoodNetwork describe "last_used_ship_address" do let(:address) { create(:address) } let(:distributor) { create(:distributor_enterprise) } - let(:finder) { AddressFinder.new(email) } + let(:finder) { AddressFinder.new(email:) } context "when searching by email is not allowed" do before do