views:

223

answers:

6

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.

+3  A: 

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.

bjeanes
Thanks bjeanes. this is much cleaner too.
The Pied Pipes
Not sure if you are talking about the first or second attempt. My second attempt is the first one listed and uses a truth table-like approach. If they are the discreet four states, it's definitely the most expandable IMO.
bjeanes
+2  A: 

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
Ryan Bigg
A: 

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.

David Schmitt
These kinds of getters and setters are built-in methods of any ActiveRecord::Base class I believe in Rails. But thanks for taking the time to write your answer.
The Pied Pipes
That doesn't make them any prettier ;-)
David Schmitt
+5  A: 

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 %>
Samuel
+1 for most rubylicious answer until now.
David Schmitt
thanks Samuel. I agree, it's the most reuseable, and cleanest of the bunch here.
The Pied Pipes
Out of curiosity, since I'm using this longwinded business rule twice - in create and update, would you recommend putting this into a helper method shared b/w the two methods? If so, where would I package that?
The Pied Pipes
I would agree with jon that this should be in the Model since it it is BL. But if you want it in the controller, you could just put it into a helper or before filter. You could also put it into a view.
Samuel
Thanks again: like the code here for making helper methods. I've made a separate question regarding the 'move to model' issue which I've not yet managed to get working, which you may or may not have time to look at. It's at http://stackoverflow.com/questions/473325/moving-business-rules-into-model
The Pied Pipes
+3  A: 

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
Jon Wood
Thanks. I was wondering when someone would say to move it into the model, which I like as a solution. I'm going to try this out and see what the results are. I'm a little concerned that you're not checking for == "t" or "f" in the conditions here: should I be?
The Pied Pipes
will the before_update also work for create? or should I do a before_create_or_update?
The Pied Pipes
+1  A: 

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.

nice thinking Dean. I'll look at it.
The Pied Pipes