gitlab-org--gitlab-foss/spec/models/project_import_data_spec.rb

54 lines
1.3 KiB
Ruby
Raw Normal View History

Refactor Project#create_or_update_import_data 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.
2018-12-10 12:09:33 -05:00
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe ProjectImportData do
Refactor Project#create_or_update_import_data 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.
2018-12-10 12:09:33 -05:00
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
describe '#clear_credentials' do
it 'clears out the Hash' do
row = described_class.new
row.merge_credentials('number' => 10)
row.clear_credentials
expect(row.credentials).to eq({})
end
end
Refactor Project#create_or_update_import_data 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.
2018-12-10 12:09:33 -05:00
end