[Ruby]: Optimise KeysChecker#compare

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 - [Ruby] Minimum ruby version is now bumped from 2.3 to 2.5 ([#47](https://github.com/cucumber/compatibility-kit/pull/47))
+- [Ruby] Refactored `KeysChecker#compare` to be a lot less complex and slightly more performant ([#51](https://github.com/cucumber/compatibility-kit/pull/51))
 
 ### Fixed
 - [Ruby] The `meta` key was being erroneously checked when running CCK tests ([#43](https://github.com/cucumber/compatibility-kit/pull/43))

ruby/lib/keys_checker.rb

@@ -2,31 +2,63 @@
 
 module CCK
   class KeysChecker
-    def self.compare(found, expected)
-      KeysChecker.new.compare(found, expected)
+    def self.compare(detected, expected)
+      new(detected, expected).compare
     end
 
-    def compare(found, expected)
-      errors = []
+    attr_reader :detected, :expected
 
-      found_keys = found.to_h(reject_nil_values: true).keys
-      expected_keys = expected.to_h(reject_nil_values: true).keys
-
-      return errors if found_keys.sort == expected_keys.sort
-
-      missing_keys = (expected_keys - found_keys).reject do |key|
-        found.instance_of?(Cucumber::Messages::Meta) && key == :ci
-      end
+    def initialize(detected, expected)
+      @detected = detected
+      @expected = expected
+    end
 
-      extra_keys = (found_keys - expected_keys).reject do |key|
-        found.instance_of?(Cucumber::Messages::Meta) && key == :ci
-      end
+    def compare
+      return [] if identical_keys?
 
-      errors << "Found extra keys in message #{found.class.name}: #{extra_keys}" unless extra_keys.empty?
-      errors << "Missing keys in message #{found.class.name}: #{missing_keys}" unless missing_keys.empty?
+      errors << "Detected extra keys in message #{message_name}: #{extra_keys}" if extra_keys.any?
+      errors << "Missing keys in message #{message_name}: #{missing_keys}" if missing_keys.any?
       errors
     rescue StandardError => e
       ["Unexpected error: #{e.message}"]
     end
+
+    private
+
+    def detected_keys
+      @detected_keys ||= ordered_uniq_hash_keys(detected)
+    end
+
+    def expected_keys
+      @expected_keys ||= ordered_uniq_hash_keys(expected)
+    end
+
+    def identical_keys?
+      detected_keys == expected_keys
+    end
+
+    def missing_keys
+      (expected_keys - detected_keys).reject { |key| meta_message? && key == :ci }
+    end
+
+    def extra_keys
+      (detected_keys - expected_keys).reject { |key| meta_message? && key == :ci }
+    end
+
+    def meta_message?
+      detected.instance_of?(Cucumber::Messages::Meta)
+    end
+
+    def message_name
+      detected.class.name
+    end
+
+    def ordered_uniq_hash_keys(object)
+      object.to_h(reject_nil_values: true).keys.sort
+    end
+
+    def errors
+      @errors ||= []
+    end
   end
 end

ruby/spec/keys_checker_spec.rb

@@ -16,15 +16,11 @@
     end
 
     let(:missing_data_table) do
-      Cucumber::Messages::PickleStepArgument.new(
-        doc_string: '1'
-      )
+      Cucumber::Messages::PickleStepArgument.new(doc_string: '1')
     end
 
     let(:missing_doc_string) do
-      Cucumber::Messages::PickleStepArgument.new(
-        data_table: '12'
-      )
+      Cucumber::Messages::PickleStepArgument.new(data_table: '12')
     end
 
     let(:wrong_values) do
@@ -42,14 +38,14 @@
 
     it 'finds extra keys' do
       expect(subject.compare(complete, missing_doc_string)).to eq(
-        ['Found extra keys in message Cucumber::Messages::PickleStepArgument: [:doc_string]']
+        ['Detected extra keys in message Cucumber::Messages::PickleStepArgument: [:doc_string]']
       )
     end
 
     it 'finds extra and missing' do
       expect(subject.compare(missing_doc_string, missing_data_table)).to contain_exactly(
         'Missing keys in message Cucumber::Messages::PickleStepArgument: [:doc_string]',
-        'Found extra keys in message Cucumber::Messages::PickleStepArgument: [:data_table]'
+        'Detected extra keys in message Cucumber::Messages::PickleStepArgument: [:data_table]'
       )
     end
 
@@ -66,9 +62,7 @@
       end
 
       let(:default_not_set) do
-        Cucumber::Messages::Duration.new(
-          nanos: 12
-        )
+        Cucumber::Messages::Duration.new(nanos: 12)
       end
 
       it 'does not raise an exception' do
@@ -77,26 +71,19 @@
     end
 
     context 'when executed as part of a CI' do
-      before do
-        allow(ENV).to receive(:[]).with('CI').and_return(true)
-      end
+      before { allow(ENV).to receive(:[]).with('CI').and_return(true) }
 
       it 'ignores actual CI related messages' do
-        found = Cucumber::Messages::Meta.new(
-          ci: Cucumber::Messages::Ci.new(name: 'Some CI')
-        )
-
+        detected = Cucumber::Messages::Meta.new(ci: Cucumber::Messages::Ci.new(name: 'Some CI'))
         expected = Cucumber::Messages::Meta.new
 
-        expect(subject.compare(found, expected)).to be_empty
+        expect(subject.compare(detected, expected)).to be_empty
       end
     end
 
-    context 'when an unexcpected error occurs' do
+    context 'when an unexpected error occurs' do
       it 'does not raise error' do
-        expect {
-          subject.compare(nil, nil)
-        }.not_to raise_error
+        expect { subject.compare(nil, nil) }.not_to raise_error
       end
 
       it 'returns the error' do

ruby/spec/messages_comparator_spec.rb

@@ -5,35 +5,31 @@
 require_relative '../lib/messages_comparator'
 
 describe CCK::MessagesComparator do
-  subject() { described_class }
-
   context 'when executed as part of a CI' do
-    before do
-      allow(ENV).to receive(:[]).with('CI').and_return(true)
-    end
+    before { allow(ENV).to receive(:[]).with('CI').and_return(true) }
 
-    it 'ignores extra found CI messages' do
-      found_message_ci = Cucumber::Messages::Ci.new(name: 'Some CI')
-      found_message_meta = Cucumber::Messages::Meta.new(ci: found_message_ci)
-      found_message_envelope = Cucumber::Messages::Envelope.new(meta: found_message_meta)
+    it 'ignores any detected CI messages' do
+      detected_message_ci = Cucumber::Messages::Ci.new(name: 'Some CI')
+      detected_message_meta = Cucumber::Messages::Meta.new(ci: detected_message_ci)
+      detected_message_envelope = Cucumber::Messages::Envelope.new(meta: detected_message_meta)
 
-      expected_message_meta = Cucumber::Messages::Meta.new()
+      expected_message_meta = Cucumber::Messages::Meta.new
       expected_message_envelope = Cucumber::Messages::Envelope.new(meta: expected_message_meta)
 
-      comparator = subject.new(CCK::KeysChecker, [found_message_envelope], [expected_message_envelope])
+      comparator = described_class.new(CCK::KeysChecker, [detected_message_envelope], [expected_message_envelope])
 
       expect(comparator.errors).to be_empty
     end
 
-    it 'ignores extra expected CI messages' do
-      found_message_meta = Cucumber::Messages::Meta.new()
-      found_message_envelope = Cucumber::Messages::Envelope.new(meta: found_message_meta)
+    it 'ignores any expected CI messages' do
+      detected_message_meta = Cucumber::Messages::Meta.new
+      detected_message_envelope = Cucumber::Messages::Envelope.new(meta: detected_message_meta)
 
       expected_message_ci = Cucumber::Messages::Ci.new(name: 'Some CI')
       expected_message_meta = Cucumber::Messages::Meta.new(ci: expected_message_ci)
       expected_message_envelope = Cucumber::Messages::Envelope.new(meta: expected_message_meta)
 
-      comparator = subject.new(CCK::KeysChecker, [found_message_envelope], [expected_message_envelope])
+      comparator = described_class.new(CCK::KeysChecker, [detected_message_envelope], [expected_message_envelope])
 
       expect(comparator.errors).to be_empty
     end