Linking issues, classifying the relationship as a cause, effect, superset, etc

Question

I first saw this gigantic if and tried to refactor it. Could only end with a endless switch statement.

Old code -

  # It is a Cause
    if @causality == "C"  
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @notice = 'New cause linked Successfully'
    end

    # It is an Inhibitor
    if @causality == "I"  
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @relationship.relationship_type = 'I'
      @notice = 'New reducing issue linked Successfully'
    end        

    # It is a Superset
    if @causality == "P"  
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @relationship.relationship_type = 'H'
      @notice = 'New superset linked Successfully'
    end

    # It is an Effect
    if @causality == "E"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid      
      @notice = 'New effect linked Successfully'
    end

    # It is an Inhibited
    if @causality == "R"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid      
      @relationship.relationship_type = 'I'
      @notice = 'New reduced issue linked Successfully'
    end

    # It is a Subset
    if @causality == "S"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid
      @relationship.relationship_type = 'H'          
      @notice = 'New subset linked Successfully'
    end 

That's in one side of the if, in the 'else' side a similar piece of code goes like this

 # Populate User_Id if relationship was created by a logged in User
      if @issue.user_id.to_s != ""
        @relationship.user_id = @issue.user_id  
      end

      # It is a Cause
      if @causality == "C"  
        @relationship.cause_id = @issue.id
        @relationship.issue_id = @causality_id          
        @notice = 'New Issue was created and linked as a cause'
      end

      # It is an Inhibitor
      if @causality == "I"  
        @relationship.cause_id = @issue.id
        @relationship.issue_id = @causality_id
        @relationship.relationship_type = 'I'            
        @notice = 'New Issue was created and linked as reducer'
      end

      # It is a Superset
      if @causality == "P"  
        @relationship.cause_id = @issue.id
        @relationship.issue_id = @causality_id 
        @relationship.relationship_type = 'H'
        @notice = 'New Issue was created and linked as a superset'
      end         

      # It is an Effect
      if @causality == "E"  
        @relationship.cause_id = @causality_id
        @relationship.issue_id = @issue.id
        @notice = 'New Issue was created and linked as an effect'
      end          

      # It is an Inhibited
      if @causality == "R"  
        @relationship.cause_id = @causality_id
        @relationship.issue_id = @issue.id
        @relationship.relationship_type = 'I'
        @notice = 'New Issue was created and linked as reduced'
      end              

      # It is a Subset
      if @causality == "S"  
        @relationship.cause_id = @causality_id
        @relationship.issue_id = @issue.id
        @relationship.relationship_type = 'H'
        @notice = 'New Issue was created and linked as a subset'
      end  

New code -

def set_type_of_relationship(already_exists)
  if !already_exists
    case @causality
    when "C"       
      @relationship.cause_id = @issue.id
      @relationship.issue_id = @causality_id  
      @notice = 'New Issue was created and linked as a cause'
    when "I"
      @relationship.cause_id = @issue.id
      @relationship.issue_id = @causality_id  
      @relationship.relationship_type = 'I'            
      @notice = 'New Issue was created and linked as reducer'
    when "P"
      @relationship.cause_id = @issue.id
      @relationship.issue_id = @causality_id  
      @relationship.relationship_type = 'H'
      @notice = 'New Issue was created and linked as a superset'
    when "E"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issue.id
      @notice = 'New Issue was created and linked as an effect'
    when "R"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issue.id
      @relationship.relationship_type = 'I'
      @notice = 'New Issue was created and linked as reduced'
    when "S"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issue.id
      @relationship.relationship_type = 'H'
      @notice = 'New Issue was created and linked as a subset'
    else 
      @notice = 'Error creating and linking issue'
    end
  else #if already_exists
    case @causality
    when "C"       
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @notice = 'New cause linked Successfully'
    when "I"
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @relationship.relationship_type = 'I'
      @notice = 'New reducing issue linked Successfully'
    when "P"
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id
      @relationship.relationship_type = 'H'
      @notice = 'New superset linked Successfully'
    when "E"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid      
      @notice = 'New effect linked Successfully'
    when "R"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid      
      @relationship.relationship_type = 'I'
      @notice = 'New reduced issue linked Successfully'
    when "S"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid
      @relationship.relationship_type = 'H'          
      @notice = 'New subset linked Successfully'
    else 
      @notice = 'Error creating and linking issue'
    end 
  end
end

This endless switch (case) statement drove me a little bit crazy but I found no way of translating the enormous list of ifs to anything easier to debug and more importantly, with less duplication. I chose to extract everything into two switch mainly because switches are implemented with an indexed branch table in Ruby – i.e its quite faster. Take a close look at this one and help me to refactor it!

Caution: Pay attention at the differences between

@relationship.cause_id = @issueid
@relationship.issue_id = @causality_id

and

@relationship.cause_id = @causality_id
@relationship.issue_id = @issue.id

Answer 1

I would actually take a more object oriented approach to this. Assume that your original object (the one you use to access the @relationship and @causality attributes) is named OriginalObject. In reality this could be a controller or simple Ruby object.

class OriginalObject
  attr_accessor :issue, :causality_id, :issueid

  def set_type_of_relationship(already_exists)
    obj = causality_object(already_exists)

    if obj.nil?
      @notice = 'Error creating and linking issue'
      return
    end

    @relationship.cause_id = obj.cause_id(self)
    @relationship.issue_id = obj.issue_id(self)
    @relationship.relationship_type = obj.relationship_type
    @notice = obj.notice
  end

  private

  def causality_object(already_exists)
    case @causality
    when 'C' then Cause
    when 'I' then Inhibitor
    when 'E' then Effect
    ...
    else nil
    end.new(already_exists)
  end
end

I would then subclass each cause object to remove the conditionals and simplify the code:

class CauseObject
  def initialize(already_exists)
    @already_exists = already_exists
  end

  def notice(created_and_linked, linked)
    if @already_exists
      "New Issue was created and linked as #{created_and_linked}"
    else
      "New #{linked} linked Successfully"
    end
  end
end

class CIPCauseObject < CauseObject
  def cause_id(obj)
    @already_exists ? obj.issueid : obj.issue.id
  end

  def issue_id(obj)
    obj.causality_id
  end
end

class ERSCauseObject < CauseObject
  def cause_id(obj)
    obj.causality_id
  end

  def issue_id(obj)
    @already_exists ? obj.issueid : obj.issue.id
  end
end

This would make it easy to create new Cause, Effect, Inhibitor, etc. objects.

class Cause < CIPCauseObject
  def relationship_type; nil; end

  def notice
    super('a cause', 'cause')
  end
end

class Inhibitor < CIPCauseObject
  def relationship_type; 'I'; end

  def notice
    super('a reducer', 'reducer')
  end
end

class Effect < ERSCauseObject
  def relationship_type; nil; end

  def notice
    super('an effect', 'effect')
  end
end

This approach allows for further functionality to be built on the cause objects without having to re-write that original conditional. For example the notice response could be modified without having to fit the original message formatting. It's also easier to unit test this code in isolation. In my opinion the set_type_of_relationship function is also much easier to read since there is no longer a large and confusing conditional in it.

Answer 2

This chunk here:

if already_exists
  case @causality 
  when "C", "I", "P"
  @relationship.cause_id = @issueid
  @relationship.issue_id = @causality_id 
  when "E", "R", "S"
  @relationship.cause_id = @causality_id
  @relationship.issue_id = @issueid
  end   
  @relationship.relationship_type = args[0].try(:to_s)            
else
  case @causality 
  when "C", "I", "P"
  @relationship.cause_id = @issue.id
  @relationship.issue_id = @causality_id 
  when "E", "R", "S"
  @relationship.cause_id = @causality_id
  @relationship.issue_id = @issue.id
  end
  @relationship.relationship_type = args[0].try(:to_s)            
end

Is this:

issue_id = already_exists ? @issueid : @issue.id
ids = [issue_id, @causality_id]
ids.rotate! if %W[E R S].member? @causality
@relationship.cause_id, @relationship.issue_id = ids
@relationship.relationship_type = args[0].try(:to_s)            

(Untested, but pretty close.)

As a point of interest, here was the middle step between the two.

if already_exists
  vars = [@issueid, @causality_id]
  vars.rotate! if %W[E R S].member? @causality
  @relationship.cause_id, @relationship.issue_id = vars
else
  vars = [@issue.id, @causality_id]
  vars.rotate! if %W[E R S].member? @causality
  @relationship.cause_id, @relationship.issue_id = vars
end
# Don't know why you repeated this.
@relationship.relationship_type = args[0].try(:to_s)            

For me, these things progress in stages. The first thing I noticed was that the logic of the two chunks was identical, just using different values. In my head:

• Each chunk uses different values
• Only one is different
• @causality causes them to flip
• Ruby has an easy way to express this

Answer 3

Ok, I am the author of the question but I have no account on CR, this is the final refactoring :)

def set_type_of_relationship(already_exists)
    args = { 
        C: [nil, 'a cause',    'cause'],
        I: [:I,  'a reducer',  'reducer issue'],
        P: [:H,  'a superset', 'superset'],
        E: [nil, 'an effect',  'effect'],
        R: [:I,  'reduced',    'reduced issue'],
        S: [:H,  'a subset',   'subset'] 
      }[@causality.to_sym]

    (@notice = 'Error creating and linking issue' and return) if args.nil?

    if already_exists
      case @causality 
      when "C", "I", "P"
      @relationship.cause_id = @issueid
      @relationship.issue_id = @causality_id 
      when "E", "R", "S"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issueid
      end   
      @relationship.relationship_type = args[0].try(:to_s)            
    else
      case @causality 
      when "C", "I", "P"
      @relationship.cause_id = @issue.id
      @relationship.issue_id = @causality_id 
      when "E", "R", "S"
      @relationship.cause_id = @causality_id
      @relationship.issue_id = @issue.id
      end
      @relationship.relationship_type = args[0].try(:to_s)            
    end

    @notice = if already_exists
                "New Issue was created and linked as #{args[1]}"
              else
                "New #{args[2]} linked Successfully"
              end
  end