Live! Nude! Code!
I love a good code review. Looking at real, honest-to-goodness deployed code is one of the best ways to learn the idiom of a language as well as what gets done through the exigencies of circumstance (a nice way to say, “things I done ‘cause I’m ignernt and inna hurry”). Speaking of dumb and rushed, you should attribute any goofs to my misunderstanding or poor expression of what was actually said; I am but an egg.
Watching this session with Chad Fowler and Marcel Molina, Jr. at the helm, I discovered the meta-enjoyment of watching two experts in the idiom of a given language who also have both a frankness and a generosity of spirit review code. They were funny, explicit about their methodology, candid about their ignorance, and inviting of comment from the room. This is how code reviews should be done.
Marcel likes to start with rake stats. (Note: this caused me to trip over flog, an ABC He generally is looking for as much of the code as possible to be driven down into the models (and quoted Jamis Buck’s “Skinny Controller, Fat Model” blog post). I seem to recall he liked at least a 2-to-1 ratio of model-to-controller code, but I could easily be mistaken. We were looking at the code for a real app written by someone in the room that was about 5K lines of code, which Marcel referred to as “pretty big” … which makes me think PublicSquare rates as “really big” with its 10K lines, but it hits the 2:1 ratio well. (I could wish it had 100% coverage, but we’ll get there …)
They started on the biggest model with the thought that it would offer the most opportunity for refactoring. And then the rapid-fire observations started:
Cache sweepers are much like an ActiveRecord Observer but they have access to the controller, too. I seem to recall Chad or Marcel frowning a bit about their use in this case, but I didn’t note why.
TextMate’s TODO bundle allows you to collect the traditional “# TODO”, “# FIXME”, and any others you’d like to define into a printable, saveable list. Color coding included. Nice.
Using composed_of is an excellent way to create objects which can do smart things without requiring a full ActiveRecord class (and db table) behind them. A made-up example; rather than:
student.full_namestudent.reversed_full_name
… do something like this:
student.full_namestudent.full_name.reversed
… and you can then create a method wrapper or delegation on student if you’re a strict interpreter of the Law of Demeter (more below).
Naked finds in a controller (think: @foo = Foo.find_by_bar(…)) are a red flag that some model is missing a needed method. Additionally (and it may be elementary but bears repeating), if you’ll generally follow the CRUD model in your controllers , carving out what you don’t need, you’ll end up with cleaner, more readable apps. This was in line with Chad’s observation that he actually likes to start with the scaffolding and trim back.
At one point a debate on the proper observation of the Law of Demeter broke out, largely expressed in terms of “dot tolerance” (e.g. “I don’t like to see more than one dot – foo.bar(); foo.bar.baz() should b refactored to something like foo.baz_the_bar()” vs. “I’ll usually allow two dots – foo.bar.baz(); anything past that needs refactoring.”) During the discussion, Dan Manges’ post on the Law of Demeter and the response from Alex at Pivotal Labs were mentioned; the comments on the latter are entertaining and informative. Dave Thomas ended the debate an amusing observation: “[The ‘Law of Demeter’ is] the worst named thing in computer science. There is no law. It’s just ‘The Generally Accepted Good Idea of Demeter’.”
Hey Technorati! Get this: therailsedge railsedge railsedge2007
blog comments powered by Disqus