Forum: Ruby on Rails I am breaking the DRY principle - could I avoid it?

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
2dd904ec5981c31e7bb7a5743a53caf8?d=identicon&s=25 Bruce Balmer (brucebalmer)
on 2005-12-14 05:10
(Received via mailing list)
Hi:

I have two controller methods, new and edit.  They use the same form,
largely because they must.

I have the following code in both methods:

     @bb_tasks = BbTask.find(:all)
     @projects = Project.find(:all)
     @clients = Client.find(:all)
     @tasks = Task.find(:all)

The form needs those objects.  But I can't help feeling that I should
have another way to do this so I avoid aggravating the DRY police.
Far from the desiccated code I wish to write, I feel it is positively
soggy.

Any suggestions?

bruce
Cee0292fffa691f1fb320d5400200e99?d=identicon&s=25 Marcel Molina Jr. (Guest)
on 2005-12-14 05:16
(Received via mailing list)
On Tue, Dec 13, 2005 at 09:07:29PM -0700, Bruce Balmer wrote:
> The form needs those objects.  But I can't help feeling that I should
> have another way to do this so I avoid aggravating the DRY police.
> Far from the desiccated code I wish to write, I feel it is positively
> soggy.
>
> Any suggestions?

Use a conditional before filter:

  before_filter :fetch_collections, :only => [:new, :edit]

  # ...

  private

    def fetch_collections
      @bb_tasks = BbTask.find(:all)
      @projects = Project.find(:all)
      @clients = Client.find(:all)
      @tasks = Task.find(:all)
    end

 marcel
6828ffc79486cd2442714bf32286a910?d=identicon&s=25 Vivek Krishna (Guest)
on 2005-12-14 05:19
(Received via mailing list)
On 12/14/05, Bruce Balmer <brucebalmer@mac.com> wrote:
>
> Hi:
>
> I have two controller methods, new and edit.  They use the same form,
> largely because they must.
>
> I have the following code in both methods:
>
> Why cant you use the render :partial => 'form' ..and the _form.rhtml has
all the fields needed.Sounds  too simple..or am i missing something?
2dd904ec5981c31e7bb7a5743a53caf8?d=identicon&s=25 Bruce Balmer (brucebalmer)
on 2005-12-14 07:31
(Received via mailing list)
Marcel:

You are a thing of beauty.  Thank you.  That is what I needed. I had
heard of before_filter but had not made the connection.

You are a desiccant, sir!

bruce
59de94a56fd2c198f33d9515d1c05961?d=identicon&s=25 Tom Mornini (Guest)
on 2005-12-14 09:07
(Received via mailing list)
> desiccant |Ë?desikÉ?nt| |Ë?dÉ?sÉ?kÉ?nt| |Ë?dÉ?sɪk(É?)nt|
> noun
> a hygroscopic substance used as a drying agent.

A compliment, I'm certain!

And Marcel, that is a nice usage that will improve some of
my code as well. Thanks.

--
-- Tom Mornini
42172acdf3c6046f84d644cb0b94642c?d=identicon&s=25 Pat Maddox (pergesu)
on 2005-12-14 09:13
(Received via mailing list)
On 12/13/05, Marcel Molina Jr. <marcel@vernix.org> wrote:
> >
>
>
>  marcel
> --
> Marcel Molina Jr. <marcel@vernix.org>
> _______________________________________________
> Rails mailing list
> Rails@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails
>

I'm curious, why not just call fetch_collections at the beginning of
the method?  For me that would just make more sense because I can look
at the method and know exactly what's happening, whereas I might
forget to check to see if there's a before_filter.  I'd like to avoid
any confusion at all.

Also, I just wanted to know if there's any difference at all between
using before_filter and just calling the method at the beginning.
Probably not, but just wanted to be sure.

Pat
6828ffc79486cd2442714bf32286a910?d=identicon&s=25 Vivek Krishna (Guest)
on 2005-12-14 09:49
(Received via mailing list)
ok..i guess i misunderstood the question..this filter thing  is actually
really useful.never had used it before.
F0223b1193ecc3a935ce41a1edd72e42?d=identicon&s=25 zdennis (Guest)
on 2005-12-14 11:26
(Received via mailing list)
Pat Maddox wrote:

>
> I'm curious, why not just call fetch_collections at the beginning of
> the method?  For me that would just make more sense because I can look
> at the method and know exactly what's happening, whereas I might
> forget to check to see if there's a before_filter.  I'd like to avoid
> any confusion at all.

If you only call the method one time then it makes sense to add that
method call at the top of your
action method. The OP was asking how-to makes things in a more DRY
manner. Marcel's suggestion does
this because the OP is now shown a better way to write code for reuse.


>
> Also, I just wanted to know if there's any difference at all between
> using before_filter and just calling the method at the beginning.
> Probably not, but just wanted to be sure.
>

before_filters are executed before your action method is called. If a
before_filter returns false it
will not continue on to call the action method. This can be ideal for
any precondition to an action.
  A good example is user authorization. You can enforce users are
authorized before accessing
certain actions, and you can do it on one line! This makes it less
error-prone then adding a call to
"authorize" at the beginning of every action, because you're reducing
the # of repeated lines you
have to maintain.

Would you prefer?

before_filter :authorize, :only=>[ :status, :admin ]

Or..

def status
   if authorize
    ...
   else
    ...
   end
end

def admin
   if authorize
     ...
   else
     .,.
   end
end


I prefer the first...

Zach
Eb234d1ee9f1dcd334657d7c5c1b1c4d?d=identicon&s=25 Jamie Macey (jamie)
on 2005-12-14 19:07
(Received via mailing list)
On Wed, 2005-12-14 at 05:27 -0500, zdennis wrote:
>     ...
>    end
> end
>
> def admin
>    if authorize
>      ...
>    else
>      .,.
>    end
> end

For the second one, you probably don't need the else clause - if the
authorize method is the same for both cases, then it should be in charge
of rendering a 'please log in' page or whatnot.

That said, this one isn't too bad, but I do prefer the filter
personally:

def status
  return unless authorize
  ...
end

def admin
  return unless authorize
  ...
end

- Jamie
F0223b1193ecc3a935ce41a1edd72e42?d=identicon&s=25 zdennis (Guest)
on 2005-12-15 17:57
(Received via mailing list)
Jamie Macey wrote:
>>    ...
>>   end
> def status
>   return unless authorize
>   ...
> end
>
> def admin
>   return unless authorize
>   ...
> end
>

Although your examples shows more concise way to duplicate more code in
a on-method basis, it is
code duplication nonetheless, and is more error-prone to having an
update being missed since you
have to search multiple spots of code if you ever needed to change the
authorize call.

I know you prefer the filter (since you mentioned it), so this is really
to just further mention
that if code is concise, and duplicated... it's still duplicated, it's
best to avoid when possible.

Zach
38a8230ed3d5c685558b4f0aad3fc74b?d=identicon&s=25 Joe Van Dyk (Guest)
on 2005-12-15 20:31
(Received via mailing list)
On 12/15/05, zdennis <zdennis@mktec.com> wrote:
> >>   if authorize
> >>     .,.
> >
>
> Although your examples shows more concise way to duplicate more code in a on-method 
basis, it is
> code duplication nonetheless, and is more error-prone to having an update being missed 
since you
> have to search multiple spots of code if you ever needed to change the authorize call.
>
> I know you prefer the filter (since you mentioned it), so this is really to just further 
mention
> that if code is concise, and duplicated... it's still duplicated, it's best to avoid 
when possible.

One thing that you can do that will eliminate all duplication is to
put the 'before_filter :authorize' call in your application controller
(works if the protected actions are the same in each controlller).
2dd904ec5981c31e7bb7a5743a53caf8?d=identicon&s=25 Bruce Balmer (brucebalmer)
on 2006-01-18 19:03
(Received via mailing list)
>  The OP was asking how-to makes things in a more DRY manner.
Ad7805c9fcc1f13efc6ed11251a6c4d2?d=identicon&s=25 Alex Young (Guest)
on 2006-01-18 19:07
(Received via mailing list)
Original Poster (I presume)

--
Alex
2dd904ec5981c31e7bb7a5743a53caf8?d=identicon&s=25 Bruce Balmer (brucebalmer)
on 2006-01-18 19:49
(Received via mailing list)
Alex:

Thanks. That makes sense.  Funny thing a brain, I just could not
derive that from the acronym. Now you say it, it seems painfully
obvious.  Thanks, my curiosity is assuaged, I can now continue with a
normal life.

bruce
This topic is locked and can not be replied to.