diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-12-10 18:09:33 +0100 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-12-11 14:46:35 +0100 |
commit | 26378511fe11a8bb23d22f4ded9f2b4050d02ed1 (patch) | |
tree | 4360b16f9a0ad5e2a15dd49c550062da478a582a /app | |
parent | eadd53b969da2704d7551069eda0b416ffb7b0e2 (diff) | |
download | gitlab-ce-26378511fe11a8bb23d22f4ded9f2b4050d02ed1.tar.gz |
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.
Diffstat (limited to 'app')
-rw-r--r-- | app/models/project.rb | 10 | ||||
-rw-r--r-- | app/models/project_import_data.rb | 8 |
2 files changed, 10 insertions, 8 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 075c07f5c8e..d64532ae21e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -749,15 +749,9 @@ class Project < ActiveRecord::Base 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.merge_data(data.to_h) + project_import_data.merge_credentials(credentials.to_h) project_import_data end diff --git a/app/models/project_import_data.rb b/app/models/project_import_data.rb index 2c3080c6d8d..525725034a5 100644 --- a/app/models/project_import_data.rb +++ b/app/models/project_import_data.rb @@ -22,4 +22,12 @@ class ProjectImportData < ActiveRecord::Base # bang doesn't work here - attr_encrypted makes it not to work self.credentials = self.credentials.deep_symbolize_keys unless self.credentials.blank? end + + def merge_data(hash) + self.data = data.to_h.merge(hash) unless hash.empty? + end + + def merge_credentials(hash) + self.credentials = credentials.to_h.merge(hash) unless hash.empty? + end end |