Forum: RSpec Design Pattern proposal for better Rails development

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.
059ed46172a087063ce26250e44c8627?d=identicon&s=25 Fernando Perez (fernando)
on 2008-11-05 14:17
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,
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2008-11-05 14:29
(Received via mailing list)
On Wed, Nov 5, 2008 at 7:17 AM, Fernando Perez <lists@ruby-forum.com>
wrote:
>
> time. We want our models and controllers to work in a black box manner.
> product_id, then again I must change all code that calls it, because in
> 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
2ce9c0106b5851b2294ba5eb9f5c04bd?d=identicon&s=25 Ashley Moran (Guest)
on 2008-11-05 15:15
(Received via mailing list)
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/
059ed46172a087063ce26250e44c8627?d=identicon&s=25 Fernando Perez (fernando)
on 2008-11-05 15:28
> 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?
42172acdf3c6046f84d644cb0b94642c?d=identicon&s=25 Pat Maddox (pergesu)
on 2008-11-05 15:47
(Received via mailing list)
Fernando Perez <lists@ruby-forum.com> writes:

> end
> other controllers? That's the main problem. How would you do that?
I don't think this is a problem.

Pat
059ed46172a087063ce26250e44c8627?d=identicon&s=25 Fernando Perez (fernando)
on 2008-11-05 15:53
> 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) ?
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2008-11-05 16:03
(Received via mailing list)
On Wed, Nov 5, 2008 at 8:42 AM, Pat Maddox <pergesu@gmail.com> wrote:
>>   Subscription.find_if_purchased
>> 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
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2008-11-05 16:04
(Received via mailing list)
On Wed, Nov 5, 2008 at 8:53 AM, Fernando Perez <lists@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?
85d99e7678d8720f6e00ab0f60fe6ea9?d=identicon&s=25 Andrew Premdas (Guest)
on 2008-11-05 16:14
(Received via mailing list)
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@ruby-forum.com>
F68f69615423aa3851bd445409754dbf?d=identicon&s=25 Joseph Wilk (joesniff)
on 2008-11-05 16:20
(Received via mailing list)
Fernando Perez wrote:
> 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
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2008-11-05 16:33
(Received via mailing list)
On Wed, Nov 5, 2008 at 9:12 AM, Andrew Premdas <apremdas@gmail.com>
wrote:
> find_if_purchased, you now have methods like find_for_index. What you've
> 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.

> Hope this helps
>
> All best
>
> Andrew

Other than my note above about "up front", I generally agree with what
Andrew says here.

Cheers,
David
059ed46172a087063ce26250e44c8627?d=identicon&s=25 Fernando Perez (fernando)
on 2008-11-05 16:42
Okay, I wanted to get some external point of views on an idea. Thanks
for your input.
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2008-11-05 16:46
(Received via mailing list)
On Wed, Nov 5, 2008 at 9:42 AM, Fernando Perez <lists@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?
059ed46172a087063ce26250e44c8627?d=identicon&s=25 Fernando Perez (fernando)
on 2008-11-05 16:51
> 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 ;-)
A91bd6cef23eb3516245a092e196c4da?d=identicon&s=25 Maurício Linhares (mauricio)
on 2008-11-05 16:57
(Received via mailing list)
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
85d99e7678d8720f6e00ab0f60fe6ea9?d=identicon&s=25 Andrew Premdas (Guest)
on 2008-11-05 16:58
(Received via mailing list)
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@gmail.com>
2ce9c0106b5851b2294ba5eb9f5c04bd?d=identicon&s=25 Ashley Moran (Guest)
on 2008-11-05 17:27
(Received via mailing list)
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.


> 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/
This topic is locked and can not be replied to.