26378511fe
In https://gitlab.com/gitlab-org/release/framework/issues/28 we found that this method was changed a lot over the years: 43 times if our calculations were correct. Looking at the method, it had quite a few branches going on: def create_or_update_import_data(data: nil, credentials: nil) return if data.nil? && credentials.nil? project_import_data = import_data || build_import_data if data project_import_data.data ||= {} project_import_data.data = project_import_data.data.merge(data) end if credentials project_import_data.credentials ||= {} project_import_data.credentials = project_import_data.credentials.merge(credentials) end project_import_data end If we turn the || and ||= operators into regular if statements, we can see a bit more clearly that this method has quite a lot of branches in it: def create_or_update_import_data(data: nil, credentials: nil) if data.nil? && credentials.nil? return else project_import_data = if import_data import_data else build_import_data end if data if project_import_data.data # nothing else project_import_data.data = {} end project_import_data.data = project_import_data.data.merge(data) end if credentials if project_import_data.credentials # nothing else project_import_data.credentials = {} end project_import_data.credentials = project_import_data.credentials.merge(credentials) end project_import_data end end The number of if statements and branches here makes it easy to make mistakes. To resolve this, we refactor this code in such a way that we can get rid of all but the first `if data.nil? && credentials.nil?` statement. We can do this by simply sending `to_h` to `nil` in the right places, which removes the need for statements such as `if data`. Since this data gets written to a database, in ProjectImportData we do make sure to not write empty Hash values. This requires an `unless` (which is really a `if !`), but the resulting code is still very easy to read.
42 lines
1 KiB
Ruby
42 lines
1 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
require 'spec_helper'
|
|
|
|
describe ProjectImportData do
|
|
describe '#merge_data' do
|
|
it 'writes the Hash to the attribute if it is nil' do
|
|
row = described_class.new
|
|
|
|
row.merge_data('number' => 10)
|
|
|
|
expect(row.data).to eq({ 'number' => 10 })
|
|
end
|
|
|
|
it 'merges the Hash into an existing Hash if one was present' do
|
|
row = described_class.new(data: { 'number' => 10 })
|
|
|
|
row.merge_data('foo' => 'bar')
|
|
|
|
expect(row.data).to eq({ 'number' => 10, 'foo' => 'bar' })
|
|
end
|
|
end
|
|
|
|
describe '#merge_credentials' do
|
|
it 'writes the Hash to the attribute if it is nil' do
|
|
row = described_class.new
|
|
|
|
row.merge_credentials('number' => 10)
|
|
|
|
expect(row.credentials).to eq({ 'number' => 10 })
|
|
end
|
|
|
|
it 'merges the Hash into an existing Hash if one was present' do
|
|
row = described_class.new
|
|
|
|
row.credentials = { 'number' => 10 }
|
|
row.merge_credentials('foo' => 'bar')
|
|
|
|
expect(row.credentials).to eq({ 'number' => 10, 'foo' => 'bar' })
|
|
end
|
|
end
|
|
end
|