RSpec makes me want to write better code

Hi,

Today is a big day. I officially transitioned from manually testing by
clicking around in my app, to automated testing with RSpec + Autotest.

Yes RSpec made me find a few weaknesses in my app: while I was writing
specs for one of my models, I discovered that I had forgotten some
validations, that simply I had never tested by my super manual clicking
workflow.

Also, RSpec made me discover something else: my model has some custom
find methods. Often over time I find myself changing the name of these
custom find methods, e.g: find_all_products -> find_available_products

As some of these finds are used by more than 1 controller, changing the
name of the find_ often breaks code in different places at once. This
was painful to manually test, and it is still a bit painful to have
RSpec test, as I would also have to rename the custom find method in my
specs.

Based on this recent article:
http://www.matthewpaulmoore.com/articles/1276-ruby-on-rails-code-quality-checklist
“Each controller action only calls one model method other than an
initial find or new.”.

Would this mean that I should change my custom find naming convention to
something more general that would never have to be renamed over time
such as find_for_index, find_for_create, etc?

What are your thoughts on that. Is there a “design pattern” on naming
custom find methods in Rails models? Because I tend to be seeing one.

Also, is it clever to write specs such as:

Product.respond_to? :find_for_index

so that if I break the rule of my naming convention, one of my spec will
quickly bark at me.

Best regards,

On Thu, Sep 25, 2008 at 8:01 AM, Fernando P.
[email protected]wrote:

Also, RSpec made me discover something else: my model has some custom
find methods. Often over time I find myself changing the name of these
custom find methods, e.g: find_all_products -> find_available_products

As some of these finds are used by more than 1 controller, changing the
name of the find_ often breaks code in different places at once. This
was painful to manually test, and it is still a bit painful to have
RSpec test, as I would also have to rename the custom find method in my
specs.

Didn’t find_all_products do something different from
find_available_products? If so, it’s not really a renaming issue, as far
as
I can see. You’re changing behavior, therefore you change the specs.

Based on this recent article:

http://www.matthewpaulmoore.com/articles/1276-ruby-on-rails-code-quality-checklist
“Each controller action only calls one model method other than an
initial find or new.”.

I didn’t get that article (or, rather, that particular subarticle) at
all.
It sounded to me like some of the virulent REST advocates who want to
shoe-horn every action into an HTTP verb. The controller should do what
the
controller needs to do. If that means displaying just available products
instead of all products, it should do that “with intention,” not just as
the
default find action. That said, scopes can and should be defined, but
that’s
a well-understood idiom of Rails programming and can lead to abuse.

Would this mean that I should change my custom find naming convention to

something more general that would never have to be renamed over time
such as find_for_index, find_for_create, etc?

That puts too much controller knowledge in the model, in my opinion.

Also, is it clever to write specs such as:

Product.respond_to? :find_for_index

so that if I break the rule of my naming convention, one of my spec will
quickly bark at me.

I don’t think it really matters whether Product has a find_for_index
method
in and of itself. What matters is that the desired behavior, possibly
including and/or implemented by calling find_for_index, is achieved. If
such
a naming convention was very important to the project, then a spec like
that
would be appropriate. However, I’d regard it as a code smell if I saw
such a
test.

I can imagine other opinions on this subject and am eager to hear them.

///ark

On 25 Sep 2008, at 17:48, Mark W. wrote:

“Each controller action only calls one model method other than an
initial find or new.”.

I didn’t get that article (or, rather, that particular subarticle)
at all.

I kinda tuned out when I read, “Polymorphic associations, however, are
encouraged!”

I wouldn’t let that guy near a CSV file of my shopping list after
reading that…

It sounded to me like some of the virulent REST advocates who want
to shoe-horn every action into an HTTP verb. The controller should
do what the controller needs to do. If that means displaying just
available products instead of all products, it should do that “with
intention,” not just as the default find action.

Is a better rule “each controller action should contain no more than
two branches”? (But then, I try to apply that to all methods, and even
then, I try to push conditional code as far down as possible.)

That said, scopes can and should be defined, but that’s a well-
understood idiom of Rails programming and can lead to abuse.

Would this mean that I should change my custom find naming
convention to
something more general that would never have to be renamed over time
such as find_for_index, find_for_create, etc?

That puts too much controller knowledge in the model, in my opinion.

I’d try identify the problem that is being solved in the controller.
Why does ProductController#index need that certain subset of the
data? Maybe the model method should be something like:

  • Product.top_selling
  • Product.not_selling
  • Product.needs_updating

Does this better express the intent?

What matters is that the desired behavior, possibly including and/or
implemented by calling find_for_index, is achieved. If such a naming
convention was very important to the project, then a spec like that
would be appropriate. However, I’d regard it as a code smell if I
saw such a test.

I agree (I think). Conventions decrease the mental effort to make
something work at the risk of reduced insight. Certainly if you are
automatically checking that the code is following conventions, that
suggests the convention is artificial and unnatural. You could end up
trying to map all the business problems onto a very crude technical
interface.

Ashley


http://www.patchspace.co.uk/

On 26 Sep 2008, at 12:31, Ashley M. wrote:

are encouraged!"
Would you mind elaborating on why you don’t like these? I’m pretty
new to rails (but not programming generally) and rather naive about
such things!

Also why is the article so down on STI? What are the drawbacks? What
do people use instead?

[1]http://www.matthewpaulmoore.com/articles/1276-ruby-on-rails-code-
quality-checklist

cheers,
Matt

http://blog.mattwynne.net
http://songkick.com

In case you wondered: The opinions expressed in this email are my own
and do not necessarily reflect the views of any former, current or
future employers of mine.

On Fri, Sep 26, 2008 at 4:31 AM, Ashley M.
<[email protected]

wrote:

Is a better rule “each controller action should contain no more than two
branches”? (But then, I try to apply that to all methods, and even then, I
try to push conditional code as far down as possible.)

On an OOP mailing list, a guy asserted that there shouldn’t be two
for-loops
in the same method. It sounded ridiculous to me at the time, but after
ten
years of mulling it over, I believe he was right. :slight_smile:

///ark

On Fri, Sep 26, 2008 at 4:49 AM, Matt W. [email protected] wrote:

Also why is the article so down on STI? What are the drawbacks? What do
people use instead?

I think the guy is really just down on inheritance itself, which is not
an
unusual nor even entirely unjustified attitude. Ruby has far less need
to
use inheritance because of the ability to mix in modules. And using it,
particularly beyond two levels deep, can most definitely lead to
nightmarish
code, where changing one line in a parent class can make you spend an
hour
(or a day) trying to figure out how to handle the repercussions to every
child class.

But I do believe that where a true “is-a” relationship exists,
inheritance
is often the best approach. I’m working on a program right now that has
a
Task class and an Appointment subclass. An Appointment (in this context)
is
simply a Task that can only be performed on one day. Otherwise it’s
exactly
like a Task (again, in the context of my program). That’s an “is-a”
relationship that inheritance was designed for, and it’s working quite
well.
And, as an agile practitioner, if I find that this relationship changes
I
would throw out this class hierarchy.

STI is just a way to map inheritance to the database. I used it before I
knew what it was called. If you do use inheritance with models, I think
it’s
the best Rails way to persist the data.

Hmmm. Just realized that all this has nothing to do with RSpec…

///ark

On 26 Sep 2008, at 12:49, Matt W. wrote:

Would you mind elaborating on why you don’t like these? I’m pretty
new to rails (but not programming generally) and rather naive about
such things!

It’s quite hard to explain briefly, but basically it makes the
predicate (interpretation of the table) extremely difficult.

A normal table like

cars

reg_plate
owner_id
purchase_date

Could be interpreted as “The car with reg plate <reg_plate> was bought
by the person with ID <owner_id> on <purchase_date>” (ok, they might
have sold it since, but…)

However, from the Rails wiki[1]:

addresses

street
city
country
addressable_id
addressable_type # <- is a string

How do you interpret this? The relationship is fundamentally
different depending on the value of addressable_type, and is much
harder to enforce as an integrity constraint*.

There’s another angle you can take, based on data types - you can
define a type PERSON_ID for the attribute ‘owner_id’ (and re-use it in
the ‘departments’ table as ‘manager_id’), but the type of
‘addressable_id’ depends on the value of ‘addressable_type’. How can
a value not know its own type?

Easier solution:

people

name
delivery_address_id
billing_address_id

“The person called wants items delivered to the address with ID
<delivery_address_id> and invoices delivered to the address with ID
<billing_address_id>”.

Also why is the article so down on STI? What are the drawbacks? What
do people use instead?

[1]http://www.matthewpaulmoore.com/articles/1276-ruby-on-rails-code-quality-checklist

I’ll reply to Mark…

TBC

Ashley

[1]
http://wiki.rubyonrails.org/rails/pages/UnderstandingPolymorphicAssociations

  • pah, who needs their data integrity protected anyway?


http://www.patchspace.co.uk/

On 26 Sep 2008, at 15:16, Mark W. wrote:

Also why is the article so down on STI? What are the drawbacks? What
do people use instead?

One downside to STI is it forces you to leave NULL columns for
attributes that don’t exist in all models. This is also really bad
for integrity. I once started a CTI[1] extension to AR[2][3]. A lot
of users were interested in it, but I got no response from the core
team.

There’s talk on doing it in DataMapper too. If I get some free time I
might look at it, but it’s a fairly complex bit of ORM work. (I did
want to write an ORM but I spend way too much time doing paid work…)

performed on one day. Otherwise it’s exactly like a Task (again, in
the context of my program). That’s an “is-a” relationship that
inheritance was designed for, and it’s working quite well. And, as
an agile practitioner, if I find that this relationship changes I
would throw out this class hierarchy.

That sounds like a perfect example. I assume any code that works for
“Task” will happily process an appointment.

STI is just a way to map inheritance to the database. I used it
before I knew what it was called. If you do use inheritance with
models, I think it’s the best Rails way to persist the data.

Not the best possible way (which is CTI), but yeah, the best way you
can do it with Rails (ActiveRecord). It’s still a data modelling/
integrity headache, but not on the same level as polymorphic
associations.

Ashley

[1] http://martinfowler.com/eaaCatalog/classTableInheritance.html
[2] http://dev.rubyonrails.org/ticket/6566
[3]
http://aviewfromafar.net/2008/1/19/class-table-inheritance-in-activerecord
[4]
http://wm.lighthouseapp.com/projects/4819/tickets/53-class-table-inheritance

([3] was started on my old employer’s blog, but they never maintained
it after I left)


http://www.patchspace.co.uk/

On 26 Sep 2008, at 17:28, Mark W. wrote:

I think all of your comments make sense, but I did just want to call
out that “the Rails way” is not typically concerned with this sort
of integrity at the database level. It’s handled in the model.

Ah ok, I wasn’t sure if your comment was intended to be neutral.
You’re right, the Rails way is not concerned with DB-level integrity.

Whether that’s a good or bad thing can be debated, but it does
explain why STI’s drawbacks don’t outweigh its strengths (primarily
speed, compared to CTI, because of the lack of joins) in the minds
of true Rails believers.

I’ve avoided STI unless the models share all the attributes (and
therefore you . But it can be used acceptably at a Ruby level (be
sure to spec what attributes your classes have if you’re scared of
pollution!) - it just means more hassle if you work in the DB directly.

Ashley


http://www.patchspace.co.uk/

On Fri, Sep 26, 2008 at 9:47 AM, Ashley M.
<[email protected]

wrote:

(be sure to spec what attributes your classes have if you’re scared of
pollution!)

As part of the TDD process, I spec all attributes, but this doesn’t seem
universal. Is this a misconception? Do people actually make sure that
all
columns exist and can be written to and read from?

///ark

On Fri, Sep 26, 2008 at 8:28 AM, Ashley M.
<[email protected]

wrote:

One downside to STI is it forces you to leave NULL columns for attributes
that don’t exist in all models. This is also really bad for integrity.

I think all of your comments make sense, but I did just want to call out
that “the Rails way” is not typically concerned with this sort of
integrity
at the database level. It’s handled in the model.

Whether that’s a good or bad thing can be debated, but it does explain
why
STI’s drawbacks don’t outweigh its strengths (primarily speed, compared
to
CTI, because of the lack of joins) in the minds of true Rails believers.

///ark

On Fri, Sep 26, 2008 at 10:24 AM, Scott T. <
[email protected]> wrote:

I usually end up doing something like this:

columns = [:email, :message]
columns.each do |column|
it “should have a reader and writer for the column #{column}” do
@invite.should respond_to(column)
@invite.should respond_to("#{column}=")
end
end

That looks like a really good way to do this - thanks! Beyond that, as
they
say, you’re testing ActiveRecord.

///ark

On Sep 26, 2008, at 1:18 PM, Mark W. wrote:

On Fri, Sep 26, 2008 at 9:47 AM, Ashley M. <[email protected]

wrote:

(be sure to spec what attributes your classes have if you’re scared
of pollution!)

As part of the TDD process, I spec all attributes, but this doesn’t
seem universal. Is this a misconception? Do people actually make
sure that all columns exist and can be written to and read from?

I usually end up doing something like this:

columns = [:email, :message]
columns.each do |column|
it “should have a reader and writer for the column #{column}” do
@invite.should respond_to(column)
@invite.should respond_to("#{column}=")
end
end

Scott

“Mark W.” [email protected] writes:

every child class. But I do believe that where a true “is-a”
it’s the best Rails way to persist the data. Hmmm. Just realized
that all this has nothing to do with RSpec…///ark
_______________________________________________ rspec-users mailing
list [email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

fwiw, my inclination would still be to model this with composition
rather than inheritance :slight_smile: An Appointment in the simplest case is just a
DateTime, but you can imagine it having a Location, Participants, and of
course a Task. If it can have a Task, it can probably have multiple
Tasks. And you can imagine Task evolving independently of Appointment,
for example a composite Task, or tasks requiring follow-up tasks under
certain conditions. You can say YAGNI of course, but I think by
splitting those out entirely, you gain a more flexible model without
introducing complexity, and you avoid the technical stickiness that
accompanies STI in Rails.

And I think it’s perfectly acceptable to talk about OOD because design
is one of the key benefits of BDD :slight_smile:

Pat

On Fri, Sep 26, 2008 at 11:46 AM, Pat M. [email protected] wrote:

“Mark W.” [email protected] writes:

Task class and an
Appointment subclass. An Appointment (in this context) is simply a
Task that can only be performed on one day. Otherwise it's exactly
like a Task (again, in the context of my program).

fwiw, my inclination would still be to model this with composition
rather than inheritance :slight_smile: An Appointment in the simplest case is just a
DateTime, but you can imagine it having a Location, Participants, and of
course a Task.

It could have all those things, but it doesn’t. :slight_smile: The program is a
Getting
Things Done-type task manager (called giterdone). It couldn’t care less
who
the appointment is with. It just knows that an Appointment is something
that
needs to “get done.” The appointment’s location is handled by a GTD
context,
just like any other Task. The only difference is that this kind of Task
can
only be done on a particular day. If you don’t giterdone on that day, it
falls off the schedule.

If it can have a Task, it can probably have multiple

Tasks. And you can imagine Task evolving independently of Appointment,
for example a composite Task, or tasks requiring follow-up tasks under
certain conditions.

A Project is another kind of Task - a sibling to Appointment - and can
have
multiple Tasks (including Appointments). This again is in line with the
GTD
philosophy, which draws a clear distinction between things with “next
actions” and things without (like a project).

You can say YAGNI of course, but I think by

splitting those out entirely, you gain a more flexible model without
introducing complexity, and you avoid the technical stickiness that
accompanies STI in Rails.

Well, I do say YAGNI. Or at least YPAGNI. And again, if this turns out
to be
incorrect, I’ll change it. I really do believe in trying the simplest
thing
that could possibly work. I’ve used STI on another side project of mine
(a
magic trick database, called abracadata), and it actually did work
pretty
damn perfectly. But if it turns out to be the wrong decision, I’ll
embrace
that change.

However, I do admit that I’m biased in favor of inheritance where it
applies

  • I’ve been using it since 1989, after all. Prior to that time, I was
    implementing it in C before I knew what it was called. I really like the
    idea of “this thing is like that thing except for one little
    difference.”

On the other hand, I have been bitten by using inheritance where
composition
would have been a better design. I think the last time that happened was
in
1991. :slight_smile:

And I think it’s perfectly acceptable to talk about OOD because design

is one of the key benefits of BDD :slight_smile:

Coolness.

///ark

On Sep 26, 2008, at 6:18 pm, Mark W. wrote:

As part of the TDD process, I spec all attributes, but this doesn’t
seem universal. Is this a misconception? Do people actually make
sure that all columns exist and can be written to and read from?

What I meant by this was that say you have classes…

Animal {weight, height, dob}
Pet < Animal {name}
ZooAnimal < Animal {cage_id}

then, using ActiveRecord, all three will have attributes {weight,
height, dob, name, cage_id}. If you don’t want your pets in a cage,
it’s worth preventing that and writing specs for it.

That’s what I was getting at, really - the extra data corruption issues.

Ashley


http://www.patchspace.co.uk/

On Fri, Sep 26, 2008 at 3:10 PM, David C.
[email protected]wrote:

Now sometimes there will be some up-front modeling discussions and you
may have a sense that a model needs a specific set of fields just
because that’s what the customer says. In those cases, I’d recommend
trying to find something about the object’s behaviour to rationalize
the presence of those fields.

I think I know what you mean, and I would reply with two points, one
agile,
and one not so agile:

  1. In a test-driven environment, you don’t write code until you write a
    failing test. Now, unless you mean to spread your table definition
    across
    multiple migrations, that means you would have to write a spec for
    Demographics Reports before you could create your table. By testing
    attributes directly, you can indeed write a failing spec, then make it
    pass
    by writing a migration. It’s not a huge deal, because you could indeed
    write
    multiple migrations as you define clients of the data.

  2. The second is more important, however, and has to do with the
    non-agile
    nature of databases. Put simply, you can’t change database content like
    you
    can change code. The example I used to use (probably outdated now) is
    fax
    number. If you’re writing a customer relationship management system, you
    might want to store customer fax numbers even if you have no use for
    them
    .
    The reason is simply that if you do need them later, you can’t go back
    and
    get them. In other words, databases can have a prophecy requirement that
    ever-malleable code does not.

All that said, I think your developer’s conversation with the customer
is
very apt. In many if not most cases, if there’s no output for the data,
there’s no need for its input. But not always.

///ark

On Fri, Sep 26, 2008 at 12:18 PM, Mark W. [email protected] wrote:

On Fri, Sep 26, 2008 at 9:47 AM, Ashley M.
[email protected] wrote:

(be sure to spec what attributes your classes have if you’re scared of
pollution!)

As part of the TDD process, I spec all attributes, but this doesn’t seem
universal. Is this a misconception? Do people actually make sure that all
columns exist and can be written to and read from?

To me, spec’ing attributes is a red flag. It is not always a bad thing
or wrong, but it suggests that the focus is on structure instead of
behaviour and results in a lot of unnecessary overhead in your code
examples, making them more brittle and more difficult to maintain.

For example, let’s say we’ve got a Person model who can be old enough
to vote or not. I might start w/ a code example like this:

person = Person.new(:birthdate => 17.years.ago)
person.should_not be_voting_age

Then I’d do the following:

  • run the example and discover that Person doesn’t have a birthdate=
    method
  • add and run a migration that adds birthdate
  • run the example and discover that Person doesn’t have a voting_age?
    method
  • def voting_age?; false; end
  • run the example and it passes
  • add another example for the positive case
  • etc, etc

In that process, adding and running the migration was the
implementation step. No need to spec the existence of the field
directly, because this example will fail without it. And the failure
message will be that the the Person doesn’t respond to birthdate=, so
it’s exactly the feedback I’d want because it would point me right to
the problem.

That all make sense?

Now sometimes there will be some up-front modeling discussions and you
may have a sense that a model needs a specific set of fields just
because that’s what the customer says. In those cases, I’d recommend
trying to find something about the object’s behaviour to rationalize
the presence of those fields.

Developer: why do we care about having a birthdate field?
Customer: because I want to know how old people are.
Developer: why? How will you find out?
Customer: we’ll want to run demographics reports
Developer: why?
Customer: they’ll help us sell advertising

OK, so now we have a feature, Demographics Reports, that warrants the
presence of a birthdate field, and it is in THAT context that the
birthdate field should come into existence.

Hope this all helps.

Cheers,
David

On Fri, Sep 26, 2008 at 5:38 PM, Mark W. [email protected] wrote:

and one not so agile:

  1. In a test-driven environment, you don’t write code until you write a
    failing test. Now, unless you mean to spread your table definition across
    multiple migrations, that means you would have to write a spec for
    Demographics Reports before you could create your table. By testing
    attributes directly, you can indeed write a failing spec, then make it pass
    by writing a migration. It’s not a huge deal, because you could indeed write
    multiple migrations as you define clients of the data.

This is really a deficiency of ActiveRecord migrations in my view.
DataMapper, for example, offers auto-migrations. You just add a
property to your model file and it takes care of the migration for
you. Of course, the way it does this is to drop the table and re-write
it, so you don’t want to do THAT in production :slight_smile: But for agile dev,
it’s a much nicer process.

  1. The second is more important, however, and has to do with the non-agile
    nature of databases. Put simply, you can’t change database content like you
    can change code. The example I used to use (probably outdated now) is fax
    number. If you’re writing a customer relationship management system, you
    might want to store customer fax numbers even if you have no use for them.
    The reason is simply that if you do need them later, you can’t go back and
    get them. In other words, databases can have a prophecy requirement that
    ever-malleable code does not.
I flatly disagree with this. Data is not inherently non-agile. But some processes for managing the evolution of data are.

If you don’t need it now, or can’t even justify the need for it in the
future, then it doesn’t belong in the code OR in the data. If somebody
thinks “we might need it later” then it should be discussed as a
requirement and either introduced as one or dropped. If “we might need
it later” then probably somebody has a good argument why. If nobody
can think of a good reason, then why add the extra weight to the db
AND to the examples. Not to mention the fact that fax numbers are
going to need some sort of format validation, which is going to be
code that costs money to write and maintain.

All that said, I think your developer’s conversation with the customer is
very apt. In many if not most cases, if there’s no output for the data,
there’s no need for its input. But not always.

I would agree with “but not always.” But, I’d say that in the “not
always” exceptions there should be a huge red flag and serious
discussion about why requirements are being imposed on a system for
which nobody can argue business value.

FWIW,
David

On Fri, Sep 26, 2008 at 4:16 PM, David C.
[email protected]wrote:

DataMapper, for example, offers auto-migrations. You just add a
property to your model file and it takes care of the migration for
you.

The relationship between schema and models in Rails is weird. The basic
source of truth for a model’s attributes is the database. But the
database
is defined in Ruby. I don’t know DataMapper, but it sounds as if it puts
the
responsibility in one place - the Ruby code.

If you don’t need it now, or can’t even justify the need for it in the
future, then it doesn’t belong in the code OR in the data.

So you would not collect fax numbers in this scenario. That’s fine, but
if
you do need them later, you’re completely, absolutely, 100% screwed.
Kent’s
white book relied on the assumption that change is possible. But data
collection is an area where change is not possible, so I don’t think
agile
principles apply to it (necessarily).

You might well ask, “how far do you go in collecting currently useless
data?” And that would be a good question. :slight_smile: I would answer that it
would
have to be decided by experience in dealing with business requirements,
but
not completely by whether there is a current need for it.

If somebody

thinks “we might need it later” then it should be discussed as a
requirement and either introduced as one or dropped. If “we might need
it later” then probably somebody has a good argument why.

Even with a good argument, “we might need it later” is unYAGNIsh.

If nobody

can think of a good reason, then why add the extra weight to the db
AND to the examples.

Again, because you’ve only got one chance to get that data.

there should be a huge red flag and serious

discussion about why requirements are being imposed on a system for
which nobody can argue business value.

Agreed, absolutely.

///ark

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs