Read about our upcoming Code of Conduct on this issue

  • Yorick Peterse's avatar
    Refactor Project#create_or_update_import_data · a96301852960
    Yorick Peterse authored
    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.
    a96301852960
project.rb 64.6 KB