5
\$\begingroup\$

Here are a couple of examples of one-liners that go way beyond 80 characters:

scope = DepartmentRepository.includes(:location).by_account(@request.account_id).find(approved_department_ids)
departments = ActiveModel::ArraySerializer.new(scope, each_serializer: DepartmentWithLocationSerializer).as_json

I usually simply break these kinds of chained method calls before the . and indent the next line. My IDE (Emacs) runs rubocop on my source files as I edit them and doesn't complain about this practice. It does complain about lines longer than 80 characters so I feel somewhat compelled to fix them. How would I refactor these? The first line is calling ActiveRecord finders and scopes. Scopes can be compbined but then they have ackward names that seldom see reuse (e.g. by_account_with_locations).

\$\endgroup\$
7
\$\begingroup\$

I don't really see any real trouble with those lines. But I know the feeling that a line just seems "too long" (especially if everything else in the file is nice and neat). It's often a good thing to notice, but in some cases it just doesn't make sense to worry.

Are the lines readable? Absolutely. They are very descriptive in fact. If you were talking about a very long conditional filled with && and || and negations, then yeah, it should probably be changed. But in this case, I don't see reason to fret.

But if you're intent on refactoring, you could (as an example) move the ArraySerializer instantiation into a factory on DepartmentWithLocationSerializer and get something like

DepartmentWithLocationSerializer.serialize(scope).as_json

That sort of thing helps a bit. But if I knew a good zen quote about a little imperfection being OK too, I'd drop it here :)

Anyway, don't worry about adding methods for things you'll only use once. You don't always need a very "practical" reason like DRY, to add some more methods. If adding some methods it can make the code a little neater to look at, that's a reason in itself. Everything in moderation, YMMV, etc., so just try it out and see if the code seems nicer afterwards. If it doesn't, well, then try to live with the long lines.

\$\endgroup\$
  • \$\begingroup\$ That serialize method idea works great! I created a base Serializer class that adds that method. I addded as_json to that method which cleans things up even more. This also happens to be reusable throughout our app. \$\endgroup\$ – Reed G. Law Dec 6 '13 at 21:13
  • 1
    \$\begingroup\$ @ReedG.Law Cool, glad it was helpful. Be sure to mention the inclusion of the as_json call in your serializer (or maybe leave it out). Compared to the original code, your new implementation makes the assumption that you'll always want to call as_json rather than get the ArraySerializer instance back. Probably a fair assumption (your app, so you'd know best), but still an assumption. \$\endgroup\$ – Flambino Dec 6 '13 at 22:44
2
\$\begingroup\$

My 2 cents:

The 80 char limit is relatively silly at first glance, after all when was the last time you used a vt100 where lines > 80 chars were difficult to read?

The real reason behind the 80 char limit is roughly the same reason to limit the number of arguments to any method. See

http://robots.thoughtbot.com/sandi-metz-rules-for-developers

When you chain long method calls like that you are effectively creating a one-off method with a long argument list. Every . in the chain corresponds to an argument of a method call.

Maybe that's appropriate and frankly, I love these long method chains. I get a big grin on my face every time I come up with one. They are what makes ruby fun. But they aren't what makes ruby maintainable. Maybe you should think about what class you would make that took fewer arguments and abstracted finding the "scope" of a Department.

Long method chains like that create dependencies in your current object on every object in the method chain. That's probably okay if they are all "standard" objects, but if they aren't then any change in any of those objects will require changes everywhere those objects are used.

As an aside: If you haven't read Sandi Metz's book, read it now.

\$\endgroup\$
  • 1
    \$\begingroup\$ "you are effectively creating a one-off method with a long argument list. " -- no. The main problem with long argument lists is that they're non-descriptive. This isn't the case with long chains, where each method defines the semantics of its arguments by its name. \$\endgroup\$ – John Dvorak Dec 14 '13 at 17:22
  • \$\begingroup\$ "They are what makes ruby fun. But they aren't what makes ruby maintainable. Maybe you should think about what class you would ma" -- I love Ruby for dense but very readable lines, not for very long method chains. Only sometimes does dense = chaining. If you love extreme chaining, jQuery is pretty good at letting you. \$\endgroup\$ – John Dvorak Dec 14 '13 at 17:24
  • \$\begingroup\$ "Long method chains like that create dependencies in your current object on every object in the method chain." - I assume you're talking about the "one period only rule" which pretty much forbids the (very useful) builder pattern? \$\endgroup\$ – John Dvorak Dec 14 '13 at 17:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.