Design Pattern proposal for better Rails development

Testing RSpec
1

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,

2

On Wed, Nov 5, 2008 at 7:17 AM, Fernando P. [email protected]
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

  1. as the names become more similar they become harder to find :slight_smile:

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

3

On 5 Nov 2008, at 13:17, Fernando P. 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/

4

Fernando P. [email protected] writes:

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

Pat

5

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?

6

On Wed, Nov 5, 2008 at 8:42 AM, Pat M. [email protected] 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

7

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) ?

8

Fernando P. 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 :slight_smile:

–
Joseph W.
http://www.joesniff.co.uk

9

On Wed, Nov 5, 2008 at 8:53 AM, Fernando P. [email protected]
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?

10

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 P. [email protected]

11

On Wed, Nov 5, 2008 at 9:12 AM, Andrew P. [email protected]
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

12

On Wed, Nov 5, 2008 at 9:42 AM, Fernando P. [email protected]
wrote:

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

You’re allowed to disagree :slight_smile:

Does what we’re saying make sense to you?

13

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

14

You’re allowed to disagree :slight_smile:

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 :wink:

15

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

16

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 C. [email protected]

17

On 5 Nov 2008, at 14:28, Fernando P. 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/