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

@@ -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
 

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

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

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

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

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