How would you tidy up this controller logic?

问题:

I've got some logic in a controller that sets a status of an object if certain conditions are met:

if params[:concept][:consulted_legal] == 0 && params[:concept][:consulted_marketing] == 1
  @concept.attributes = {:status => 'Awaiting Compliance Approval'}
elsif params[:concept][:consulted_marketing] == 0 && params[:concept][:consulted_legal] == 1 
  @concept.attributes = {:status => 'Awaiting Marketing Approval'}
elsif params[:concept][:consulted_marketing] == 0 && params[:concept][:consulted_legal] == 0
  @concept.attributes = {:status => 'Awaiting Marketing & Legal Approval'}
else
  @concept.attributes = {:status => 'Pending Approval'}
end

that I share between create and update actions. How would you go about refactoring this nastiness? Looking for best practices.

New to programming and keen to clean up my code.

TIA.

回答1:

This is my take on it. I call it Super DRY.

statuses = 
  [
    ['Awaiting Marketing & Legal Approval','Awaiting Compliance Approval'],
    ['Awaiting Marketing Approval','Pending Approval']
  ]

{:status => statuses[params[:concept][:consulted_legal].to_i][params[:concept][:consulted_marketing].to_i]}

Alternatively, a more conventional approach -- lengthy but readable:

status = if params[:concept][:consulted_legal] == "0"
  if params[:concept][:consulted_marketing] == "1"
    'Awaiting Compliance Approval'
  else
    'Awaiting Marketing & Legal Approval'
  end
else
  if params[:concept][:consulted_marketing] == "0"
    'Awaiting Marketing Approval'
  else
    'Pending Approval'
  end
end

@concept.attributes = {:status => status}

Note: It looks like your original code is checking values of check boxes. Values in the params hash are always Strings, not Fixnums so my code compares strings. If for some reason comparing Fixnums is what is required for your situation, just take out the quotes around the numbers.

回答2:

You can make your code less dependent on both conditions and make it a lot more flexible.

waiting_on = []
waiting_on << 'Compliance' unless params[:concept][:consulted_marketing]
waiting_on << 'Legal' unless params[:concept][:consulted_legal]
status = waiting_on.empty? ? "Awaiting #{waiting_on.join(' & ')} Approval" : 'Pending Approval'
@concept.attributes = {:status => status}

Version for both create and update without filter:

def create
  set_concept_status_attribute
  ...
end

def update
  set_concept_status_attribute
  ...
end

private
  def set_concept_status_attribute
    waiting_on = []
    waiting_on << 'Compliance' unless params[:concept][:consulted_marketing]
    waiting_on << 'Legal' unless params[:concept][:consulted_legal]
    status = waiting_on.empty? ? "Awaiting #{waiting_on.join(' & ')} Approval" : 'Pending Approval'
    @concept.attributes = {:status => status}
  end

Or with a before_filter:

before_filter :set_concept_status_attribute, :only => [:create, :update]

def create
  ...
end

def update
  ...
end

If you can move it to you view, it looks even better:

module ConceptHelper
  def get_concept_status
    ...
  end
end

<%= get_concept_status %>

回答3:

That looks to be business logic, so it should be in the model really.

Your model probably needs a couple of attributes: consulted_legal and consulted_marketing, and a method to set the status when either one of them is changed something like this:

before_update :set_status

def set_status
  if consulted_legal && consulted_marketing
    status = "Pending Approval"
  elif consulted_legal && !consulted_marketing
    status = "Awaiting Marketing Approval"
  elif !consulted_legal && consulted_marketing
    status = "Awaiting Legal Approval"
  elif !consulted_legal && !consulted_marketing
    status "Awaiting Marketing & Legal Approval"
  end

  true # Needs to return true for the update to go through
end

回答4:

Break it down into nested if statements.

if params[:concept][:consulted_legal] == '0'
  if params[:concept][:consulted_marketing] == '1'
    @concept.attributes = { :status => 'Awaiting Compliance Approval' } 
  else
    @concept.attributes = { :status => 'Awaiting Marketing & Legal Approval' }
  end
else
  if params[:consulted_marketing] == '1'
    @concept.attributes = { :status => 'Awaiting Marketing Approval' }
  else
    @concept.attributes = { :status => "Pending Approval" }
  end
end

回答5:

you may think the list of departments consulted is a fixed enough concept to justify variables named consulted_marketing etc.. But for growth, and dryness (in a way), Id prefer a list of departments.

What you really have here is a workflow or state machine, I think a list of deparments with transitions would result in the cleanest, most growable code.

Code is data ! Model your data and the code will be trivial.

回答6:

This looks like a business rule to me. As such you might want to wrap it into a function:

string GetConceptStatus(bool consulted_legal, bool consulted_marketing)
{ 
    if (consulted_legal && consulted_marketing) {
        return "Pending Approval";
    }
    if (consulted_legal && !consulted_marketing) {
        return "Awaiting Marketing Approval";
    }
    if (!consulted_legal && consulted_marketing) {
        return "Awaiting Legal Approval";
    }
    if (!consulted_legal && !consulted_marketing) {
        return "Awaiting Marketing & Legal Approval";
    }
}

This also separates the details of how bools are encoded in your interface from the actual implementation of the business rule.

But the actual structure of the code looks good to me, since it probably better models the business rule.