CODEWITHSUNDEEP
ruby-on-railsruby
Matt Elhotiby
Matt Elhotiby

Reputation: 44066

What is a more Ruby-like way of doing this command?

I want do this:

sender_email = @request.user.paypal_email if @request.user.paypal_email == "[email protected]"

So basically I want to only execute the command if the users paypal email is "[email protected]". This works fine but it seems like there is room for refactoring.

Upvotes: 3

Views: 3609

Answers (3)

monocle
monocle

Reputation: 5896

@request.user.paypal_email

Some would advocate that you 'use only one dot'. (See 'Law of Demeter'.) You might want to consider using Rails 'delegate' method

class User < ActiveRecord::Base
  has_many :requests
end

class Request < ActiveRecord::Base
  belongs_to :user

  delegate :paypal_email, :to => :user
end

Then you can write

@request.paypal_email

or if you prefer

class Request < ActiveRecord::Base
  belongs_to :user

  delegate :paypal_email, :to => :user, :prefix => true
end

@request.user_paypal_email

Upvotes: 10

Peter Brown
Peter Brown

Reputation: 51697

Since this code is in your controller, then it can definitely be refactored. You typically want logic like this to be in the models since it's easy to write unit tests and it's not the job of the controller to know so much about the user model.

There's a few ways you could refactor this, but I would recommend moving the logic to the user model like so:

def User < ActiveRecord::Base
  def sender_email
    paypal_email if paypal_email == "[email protected]"
  end
end

Then your controller wouldn't need to know as much and could just do:

sender_email = @request.user.sender_email

Upvotes: 5

etteyafed
etteyafed

Reputation: 211

Of course you could rewrite using a block if, but the only difference is future flexibility or "readability". Maybe your question should be "Does rails provide something that does this?". The answer to that question is not that I know of. If your code does what you want I see no reason to change it.

Upvotes: 0

Related Questions