On May 6, 2011, at 10:29 AM, Eitan wrote: >If you wouldn't mind taking just a couple minutes to review this code, >I'd really appreciate it. Please provide your opinion of its quality >(maybe even a 1-10 ranking, 10 being excellent).
I see this as good an opportunity as any to discuss variable names (sorry if this turns into a hijacking of the thread, but maybe the feedback will tell Eitan whether this is an important detail for him to consider). Personally, I cringe when I see cryptic variables like: u.login and u.reset_token # in business_users.rb u = Admin.authenticate(...) # in admins.rb bus_user = valid_bus_user_token?(...) # in app_subscriptions.rb So, let's start with the variable u. OK, so maybe since it is in a file named "users" I am supposed to show I am not a moron and just recognize that u is a user. My problem is that when I read code in my head and am constantly saying "ok, then yooo login and yooo reset token" makes no sense to me. I have to mentally stop and substitute the word user. This takes mental energy and slows me down. Also if u gets used somewhere else in the app, does it mean user or something else? If the programmer would not have been so lazy (yes I feel that strongly about it), then reading this code would be much easier and working with it would be more productive. Additionally, when I now modify or extend this code, I have to remember which little abbreviations were used, which I find harder to do that remember which full word was used. A worse example: bus_user. So this is a transportation application where we're managing bus users? Not drivers but users? Obviously context will cure that question, and that diversion only lasts a minute or so. Again, though, the problem is that reading that code I am saying in my head a "bus user does this and a bus user does that." It creates mental images that are counter productive. There's style things in my code I know people won't like (I don't like 2 spaces for indents), but I thought we gave up this single-character/ short-cut naming for variables practice a long time ago. I still expect to see it in crusty, die-hard, old-schoolers writing C or PERL code, but I don't expect to see it in Ruby code. There's my rant. Is it a minority opinion? As to the rest of the code, my 5 min look at it suggests that it apears reasonably well organized--chopped up in to clearly labeled, consumable pieces. At least the file name and class name is BusinessUser instead of BusUser (so why not keep going with that clarity in the actual code?). I didn't read it enough in detail to pass judgment on its quality of Ruby-ness. -- greg willits -- SD Ruby mailing list [email protected] http://groups.google.com/group/sdruby
