Refactor Project#create_or_update_import_data
What does this MR do?
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.
What are the relevant issue numbers?
https://gitlab.com/gitlab-org/release/framework/issues/28
Does this MR meet the acceptance criteria?
-
Tests added for this feature/bug -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer