First thing, rcov
compare controller size, model size, look for largest files
then wc -l on app/models
Biggest files are most important and/or biggest messes
Observer functionality with any kind of external resource go in sweepers often
observe ActiveRecord::Base
uber-sweeper
weird
not necessarily bad, certainly unusual
before_create - initializes magic number for global config - needs description, and probably relocation - no intention revealed - should be described for what it is
assigning id directly -> "major leaky abstraction"
Chad dissed some of his own code from Rails Recipes - live and learn
set associations through the association code - the value of semantic code
use Symbol#to_proc
"never make an action have more than five lines."
"whenever it's more than five lines, it's bad."
Eric Evans - Domain-Driven Design - huge recommendation
(And Kent Beck Smalltalk Best Practice Patterns)
def fullname, def reversed_fullname, no, set a :reverse keyword or even a reverse method on the attribute itself (not even the class)
Saturday, August 25, 2007
Rails Edge Notes: Chad Fowler & Marcel Molina: Live Code Review
Subscribe to:
Post Comments (Atom)







4 comments:
Don't you think rules like "Never make X have more that N lines" are just arbritary and silly? Why not six or seven or even four?
Maybe they should at least pick a number that allows the incredibly common create action with XML support to exist without being considered "bad".
It's notes, dude. It's not meant as an endorsement. It's just so I remember what was said. And yes, of course a hard and fast rule is silly. These aren't hard and fast rules. They're guidelines. You're taking this way too seriously.
def fullname, def reversed_fullname, no, set a :reverse keyword or even a reverse method on the attribute itself (not even the class)
Giles, can you expand on this any? I presume for the sake of the example, it does something sensible like like person.fullname is 'Jamie Macey' and person.reversed_fullname is 'Macey Jamie', how would a :reverse keyword help with that?
Post a Comment