Skip to content

Refactor Project#create_or_update_import_data

Yorick Peterse requested to merge refactor-create-or-update-import-data into master

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?

Edited by Yorick Peterse

Merge request reports