rspec users - Nov 2008 - Design Pattern proposal for better Rails development

The biggest problems I have when developing (beside stupid typos) are:
1) The name of a method which I often change to a supposedly  more
descriptive name, which I will eventually change to an even more
supposedly descriptive name
2) The arguments a method takes

Let''s say I have a class method: Product.find_if_purchased(product_id,
user_id)

Let''s say it is called in 3 different controllers.

Now I want to change the name of the method to make it more clear and at
the same time I will throw a new argument to it:
Product.find_if_purchased_by_user_id(product_id, user_id, site_id)

Now this means I have to change the code in three controllers, and also
change the specs that test it with should_receive(:find_if_purchased).
This is painful.

And it demonstrates that controllers are tightly coupled to models which
shouldn''t be the case. Therefore we need names that don''t vary
over
time. We want our models and controllers to work in a black box manner.

So based on the design pattern "convention over configuration", why
not
use methods such as:

Product.find_for_products_index or ..._show, that are named after their
controller and action where they are called?

Moreover, instead of hardcoding the arguments that it requires, why not
pass a hash of arguments which is much more flexible? Indeed, when using
hardcoded arguments, if let''s say I want to define a default value for
product_id, then again I must change all code that calls it, because in
Ruby, arguments that have default values must be defined last.

What do you think about my idea? I have been trying it recently, and I
find it more pleasant to work with. It also prevents me from putting too
much code inside the controllers.

They are drawbacks though:
1) the name of the method is not descriptive, a little comment in the
controller helps well here.
2) when the method is reused by many actions, we need to create
"dummy"
methods that simply call the real method. For instance,
find_for_products_show just passes the arguments to another method
called find_for_subscriptions_show which is where the real code sits
3) there is a slight performance hit

Has anyone experienced such thing? The idea came to me yesterday when
trying to spec a chain of named_methods. I realized that the chain of
named_methods doesn''t belong to the controller, it should be wrapped in
a dummy method that calls the named_scopes.


Best regards,
-- 
Posted via http://www.ruby-forum.com/.

On Wed, Nov 5, 2008 at 7:17 AM, Fernando Perez <lists at ruby-forum.com>
wrote:
> The biggest problems I have when developing (beside stupid typos) are:
> 1) The name of a method which I often change to a supposedly  more
> descriptive name, which I will eventually change to an even more
> supposedly descriptive name
> 2) The arguments a method takes
>
> Let''s say I have a class method:
Product.find_if_purchased(product_id,
> user_id)
>
> Let''s say it is called in 3 different controllers.
>
> Now I want to change the name of the method to make it more clear and at
> the same time I will throw a new argument to it:
> Product.find_if_purchased_by_user_id(product_id, user_id, site_id)
>
> Now this means I have to change the code in three controllers, and also
> change the specs that test it with should_receive(:find_if_purchased).
> This is painful.
>
> And it demonstrates that controllers are tightly coupled to models which
> shouldn''t be the case. Therefore we need names that don''t
vary over
> time. We want our models and controllers to work in a black box manner.
>
> So based on the design pattern "convention over configuration",
why not
> use methods such as:
>
> Product.find_for_products_index or ..._show, that are named after their
> controller and action where they are called?
>
> Moreover, instead of hardcoding the arguments that it requires, why not
> pass a hash of arguments which is much more flexible? Indeed, when using
> hardcoded arguments, if let''s say I want to define a default value
for
> product_id, then again I must change all code that calls it, because in
> Ruby, arguments that have default values must be defined last.
>
> What do you think about my idea? I have been trying it recently, and I
> find it more pleasant to work with. It also prevents me from putting too
> much code inside the controllers.
>
> They are drawbacks though:
> 1) the name of the method is not descriptive, a little comment in the
> controller helps well here.
> 2) when the method is reused by many actions, we need to create
"dummy"
> methods that simply call the real method. For instance,
> find_for_products_show just passes the arguments to another method
> called find_for_subscriptions_show which is where the real code sits
> 3) there is a slight performance hit
4) as the names become more similar they become harder to find :)

For me, I think having descriptive names that are easy to find makes
it much easier to change the things I want to change. Just search and
replace.

FWIW,
David
>
> Has anyone experienced such thing? The idea came to me yesterday when
> trying to spec a chain of named_methods. I realized that the chain of
> named_methods doesn''t belong to the controller, it should be
wrapped in
> a dummy method that calls the named_scopes.
>
>
> Best regards,
> --
> Posted via http://www.ruby-forum.com/.
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>

On 5 Nov 2008, at 13:17, Fernando Perez wrote:
> Let''s say I have a class method:
Product.find_if_purchased(product_id,
> user_id)
>
> Let''s say it is called in 3 different controllers.
Hmmm, could you make a helper module and mix that into all three  
controllers?  Then there''d be just one place to change it.

Ashley

-- 
http://www.patchspace.co.uk/
http://aviewfromafar.net/

> Hmmm, could you make a helper module and mix that into all three
> controllers?  Then there''d be just one place to change it.
> 
Can you be more descriptive?

Let''s say I have:

def show
  Subscription.find_if_purchased
  ...
end

def update
  Subscription.find_if_purchased
  ...
end


Now if I change the method name to find_if_subscribed in the model, then 
necessarily I will have to change it twice in my controller, well I 
could use a before_filter, but what if the method is also called in 
other controllers? That''s the main problem. How would you do that?
-- 
Posted via http://www.ruby-forum.com/.

Fernando Perez <lists at ruby-forum.com> writes:
>> Hmmm, could you make a helper module and mix that into all three
>> controllers?  Then there''d be just one place to change it.
>> 
> Can you be more descriptive?
>
> Let''s say I have:
>
> def show
>   Subscription.find_if_purchased
>   ...
> end
>
> def update
>   Subscription.find_if_purchased
>   ...
> end
>
>
> Now if I change the method name to find_if_subscribed in the model, then 
> necessarily I will have to change it twice in my controller, well I 
> could use a before_filter, but what if the method is also called in 
> other controllers? That''s the main problem. How would you do that?
I don''t think this is a problem.

Pat

On Wed, Nov 5, 2008 at 8:42 AM, Pat Maddox <pergesu at gmail.com>
wrote:
> Fernando Perez <lists at ruby-forum.com> writes:
>
>>> Hmmm, could you make a helper module and mix that into all three
>>> controllers?  Then there''d be just one place to change it.
>>>
>> Can you be more descriptive?
>>
>> Let''s say I have:
>>
>> def show
>>   Subscription.find_if_purchased
>>   ...
>> end
>>
>> def update
>>   Subscription.find_if_purchased
>>   ...
>> end
>>
>>
>> Now if I change the method name to find_if_subscribed in the model,
then
>> necessarily I will have to change it twice in my controller, well I
>> could use a before_filter, but what if the method is also called in
>> other controllers? That''s the main problem. How would you do
that?
>
> I don''t think this is a problem.
Seriously.

What is motivating you here?

David
> Pat

> I don''t think this is a problem.
> 
> Pat
Don''t you find it painful to go through each controller and/or each 
controller spec to correct the name of the method that has just changed?

So you don''t mind having to change in many different places 
should_receive(:find_if_purchased) to 
should_receive(:find_if_subscribed) ?
-- 
Posted via http://www.ruby-forum.com/.

On Wed, Nov 5, 2008 at 8:53 AM, Fernando Perez <lists at ruby-forum.com>
wrote:
>> I don''t think this is a problem.
>>
>> Pat
>
> Don''t you find it painful to go through each controller and/or
each
> controller spec to correct the name of the method that has just changed?
>
> So you don''t mind having to change in many different places
> should_receive(:find_if_purchased) to
> should_receive(:find_if_subscribed) ?
Personally, no, I don''t. It takes a minute with find and replace.

Are you thinking that this is somehow related to the DRY principle?

Fernando Perez wrote:
>> I don''t think this is a problem.
>>
>> Pat
>>     
>
> Don''t you find it painful to go through each controller and/or
each
> controller spec to correct the name of the method that has just changed?
>
> So you don''t mind having to change in many different places 
> should_receive(:find_if_purchased) to 
> should_receive(:find_if_subscribed) ?
>   
I think as David mentioned  ''search and replace'' in an IDE
helps ensure
that is not painful. I''d even go so far as to say its quite fun, buts 
thats just me :)

--
Joseph Wilk
http://www.joesniff.co.uk

What you are suggesting here changing your api to make it easier to refactor
your api. Your trying to do this by

1) Generalising the descriptivness of the api
2) Reducing the number of calls to your api

Both of these are not particularly good ideas

1) Generalising your api, reduces it descriptivness and in sample you give
actually makes you api non descriptive. Instead of methods
find_if_purchased, you now have methods like find_for_index. What
you''ve
done here is pollute the api for your model with stuff outside its concern
(namely the controller)

2) You actually want to increase the number of calls to your api. The more
it is called the more useful it is being, and the more thoroughly it is
being tested. If you have no calls, you would not even need to implement it.

As for solving the problem of refactoring api''s quickly there are two
solutions to this

1)  Use your editors search and replace functionality
2)  Use your tests/specs/stories/features combined with your experience to
ensure you design a simple and clear api up front

Finally your examples were of a poor api, being refactored into a poorer
api. By increasing the number of parameters of your interface. you have
increased by a factor of 4 the number of possible parameter combinations you
have to deal with. Refactoring your unit test for this method should make
that ubundantly clear, and encourage you to change your api. In fact in this
case the method is in completely the wrong place what you should have is

product.purchased?
product.purchased_by_user?(user)

Hope this helps

All best

Andrew


2008/11/5 Fernando Perez <lists at ruby-forum.com>
> > Hmmm, could you make a helper module and mix that into all three
> > controllers?  Then there''d be just one place to change it.
> >
> Can you be more descriptive?
>
> Let''s say I have:
>
> def show
>  Subscription.find_if_purchased
>  ...
> end
>
> def update
>  Subscription.find_if_purchased
>  ...
> end
>
>
> Now if I change the method name to find_if_subscribed in the model, then
> necessarily I will have to change it twice in my controller, well I
> could use a before_filter, but what if the method is also called in
> other controllers? That''s the main problem. How would you do that?
> --
> Posted via http://www.ruby-forum.com/.
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20081105/87df7f3a/attachment-0001.html>

On Wed, Nov 5, 2008 at 9:12 AM, Andrew Premdas <apremdas at gmail.com>
wrote:
> What you are suggesting here changing your api to make it easier to
refactor
> your api. Your trying to do this by
>
> 1) Generalising the descriptivness of the api
> 2) Reducing the number of calls to your api
>
> Both of these are not particularly good ideas
>
> 1) Generalising your api, reduces it descriptivness and in sample you give
> actually makes you api non descriptive. Instead of methods
> find_if_purchased, you now have methods like find_for_index. What
you''ve
> done here is pollute the api for your model with stuff outside its concern
> (namely the controller)
>
> 2) You actually want to increase the number of calls to your api. The more
> it is called the more useful it is being, and the more thoroughly it is
> being tested. If you have no calls, you would not even need to implement
it.
>
> As for solving the problem of refactoring api''s quickly there are
two
> solutions to this
>
> 1)  Use your editors search and replace functionality
> 2)  Use your tests/specs/stories/features combined with your experience to
> ensure you design a simple and clear api up front
Be careful with the words "up front" here. To me, they imply that once
the design is there that you should not expect it to change, which is
the antithesis of agile. Definitely use your
tests/specs/stories/features to drive out a simple and clear api, but
you should also feel absolutely free to improve it at any time.
> Finally your examples were of a poor api, being refactored into a poorer
> api. By increasing the number of parameters of your interface. you have
> increased by a factor of 4 the number of possible parameter combinations
you
> have to deal with. Refactoring your unit test for this method should make
> that ubundantly clear, and encourage you to change your api. In fact in
this
> case the method is in completely the wrong place what you should have is
>
> product.purchased?
> product.purchased_by_user?(user)
>
> Hope this helps
>
> All best
>
> Andrew
Other than my note above about "up front", I generally agree with what
Andrew says here.

Cheers,
David
>
>
> 2008/11/5 Fernando Perez <lists at ruby-forum.com>
>>
>> > Hmmm, could you make a helper module and mix that into all three
>> > controllers?  Then there''d be just one place to change
it.
>> >
>> Can you be more descriptive?
>>
>> Let''s say I have:
>>
>> def show
>>  Subscription.find_if_purchased
>>  ...
>> end
>>
>> def update
>>  Subscription.find_if_purchased
>>  ...
>> end
>>
>>
>> Now if I change the method name to find_if_subscribed in the model,
then
>> necessarily I will have to change it twice in my controller, well I
>> could use a before_filter, but what if the method is also called in
>> other controllers? That''s the main problem. How would you do
that?
>> --
>> Posted via http://www.ruby-forum.com/.
>> _______________________________________________
>> rspec-users mailing list
>> rspec-users at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/rspec-users
>
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>

Okay, I wanted to get some external point of views on an idea. Thanks 
for your input.

-- 
Posted via http://www.ruby-forum.com/.

On Wed, Nov 5, 2008 at 9:42 AM, Fernando Perez <lists at ruby-forum.com>
wrote:
> Okay, I wanted to get some external point of views on an idea. Thanks
> for your input.
You''re allowed to disagree :)

Does what we''re saying make sense to you?

Thanks David

Just to confirm "up front" was not in any way meant to imply that the
design
is fixed or should not be refactored, or even will not need refactoring.
Clarifying my point,
 2)  Use your tests/specs/stories/features combined with your experience
to ensure you design an initial api that is clear and simple and
consequently easier to refactor when the need arises

2008/11/5 David Chelimsky <dchelimsky at gmail.com>
> On Wed, Nov 5, 2008 at 9:12 AM, Andrew Premdas <apremdas at
gmail.com> wrote:
> > What you are suggesting here changing your api to make it easier to
> refactor
> > your api. Your trying to do this by
> >
> > 1) Generalising the descriptivness of the api
> > 2) Reducing the number of calls to your api
> >
> > Both of these are not particularly good ideas
> >
> > 1) Generalising your api, reduces it descriptivness and in sample you
> give
> > actually makes you api non descriptive. Instead of methods
> > find_if_purchased, you now have methods like find_for_index. What
you''ve
> > done here is pollute the api for your model with stuff outside its
> concern
> > (namely the controller)
> >
> > 2) You actually want to increase the number of calls to your api. The
> more
> > it is called the more useful it is being, and the more thoroughly it
is
> > being tested. If you have no calls, you would not even need to
implement
> it.
> >
> > As for solving the problem of refactoring api''s quickly there
are two
> > solutions to this
> >
> > 1)  Use your editors search and replace functionality
> > 2)  Use your tests/specs/stories/features combined with your
experience
> to
> > ensure you design a simple and clear api up front
>
> Be careful with the words "up front" here. To me, they imply that
once
> the design is there that you should not expect it to change, which is
> the antithesis of agile. Definitely use your
> tests/specs/stories/features to drive out a simple and clear api, but
> you should also feel absolutely free to improve it at any time.
>
> > Finally your examples were of a poor api, being refactored into a
poorer
> > api. By increasing the number of parameters of your interface. you
have
> > increased by a factor of 4 the number of possible parameter
combinations
> you
> > have to deal with. Refactoring your unit test for this method should
make
> > that ubundantly clear, and encourage you to change your api. In fact
in
> this
> > case the method is in completely the wrong place what you should have
is
> >
> > product.purchased?
> > product.purchased_by_user?(user)
> >
> > Hope this helps
> >
> > All best
> >
> > Andrew
>
> Other than my note above about "up front", I generally agree with
what
> Andrew says here.
>
> Cheers,
> David
>
> >
> >
> > 2008/11/5 Fernando Perez <lists at ruby-forum.com>
> >>
> >> > Hmmm, could you make a helper module and mix that into all
three
> >> > controllers?  Then there''d be just one place to
change it.
> >> >
> >> Can you be more descriptive?
> >>
> >> Let''s say I have:
> >>
> >> def show
> >>  Subscription.find_if_purchased
> >>  ...
> >> end
> >>
> >> def update
> >>  Subscription.find_if_purchased
> >>  ...
> >> end
> >>
> >>
> >> Now if I change the method name to find_if_subscribed in the
model, then
> >> necessarily I will have to change it twice in my controller, well
I
> >> could use a before_filter, but what if the method is also called
in
> >> other controllers? That''s the main problem. How would you
do that?
> >> --
> >> Posted via http://www.ruby-forum.com/.
> >> _______________________________________________
> >> rspec-users mailing list
> >> rspec-users at rubyforge.org
> >> http://rubyforge.org/mailman/listinfo/rspec-users
> >
> >
> > _______________________________________________
> > rspec-users mailing list
> > rspec-users at rubyforge.org
> > http://rubyforge.org/mailman/listinfo/rspec-users
> >
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20081105/4ea3f97c/attachment-0001.html>

> You''re allowed to disagree :)
> 
> Does what we''re saying make sense to you?
>
Yes, my model classes were starting to get hard to read with method 
calling other methods calling other methods.

Yes, we can change ;-)
-- 
Posted via http://www.ruby-forum.com/.

I feel the same as Andrew, generalizing all calls to the model methods
is bad, as you''re making the simple (and common) complicated just to
have the uncommon as possible.

If you''re making a big change as changing a method name and adding a
new parameter, you''re doing something new, it isn''t the same
thing or
the same feature anymore. Once you have placed a method as public in
your object it becomes part of the object''s public API and this is
always going to be hard to change, that''s why we have testing tools
like RSpec and refactoring proposals.

And isn''t this making your controller code ugly? Do you have any
samples of this to exemplify what you''re doing?

-
Maur?cio Linhares
http://alinhavado.wordpress.com/ (pt-br) | http://blog.codevader.com/ (en)
Jo?o Pessoa, PB, +55 83 8867-7208

On Wed, Nov 5, 2008 at 12:44 PM, David Chelimsky <dchelimsky at gmail.com>
wrote:
>
> You''re allowed to disagree :)
>
> Does what we''re saying make sense to you?

On 5 Nov 2008, at 14:28, Fernando Perez wrote:
>> Hmmm, could you make a helper module and mix that into all three
>> controllers?  Then there''d be just one place to change it.
>>
> Can you be more descriptive?
Well I was just thinking out loud really.  Something like

module SubscriptionStuff
   def subscriptions
     Subscription.find_if_purchased
   end
end

class MyController
   include SubscriptionStuff

   def show
     subscriptions.xyz
   end

end

I''m not saying that''s a good idea though, it was just my first
reaction to your description.

> Let''s say I have:
>
> def show
>  Subscription.find_if_purchased
>  ...
> end
>
> def update
>  Subscription.find_if_purchased
>  ...
> end
>
>
> Now if I change the method name to find_if_subscribed in the model,  
> then
> necessarily I will have to change it twice in my controller, well I
> could use a before_filter, but what if the method is also called in
> other controllers? That''s the main problem. How would you do that?

Find and replace has never bothered me.  If I had something that had a  
potentially conflicting name, I''d redefine the old method to raise an  
error and run the specs again.  But I''ve never needed to do this.

Ashley


-- 
http://www.patchspace.co.uk/
http://aviewfromafar.net/