[dhh]

Coulda, woulda, damn shoulda. You're future coding with your "what ifs". Most controller actions are not reused. The time to extract for reuse is when you need to reuse. Not crystal balling about that you probably will be in the future.

Because when the reuse case actually arrives, you might find that you need to reuse some, but not all of the action. If you're literally doing the exact same thing for both web and api, why are you using a separate API controller? Just use respond_to with formats.

Second, yes, you shouldn't have side-effects like sending email in your models. But you don't need to! Just stick that logic in your controllers. That's what it's there for -- to render the views (of which emails is one of them -- see http://david.heinemeierhansson.com/2012/emails-are-views.htm...).

The AR callbacks are wonderful for coordinating the domain model. Often times you'll want to create auxiliary objects when something else is created. That's what's it's there for. Or to otherwise keep the integrity of the domain model in place.

[tomblomfield]

Sure, I think we're saying the same thing here.

This example is from a real-life codebase, and the interactor was extracted from 3 or 4 (bloated, repetitive) controllers. Perhaps this could have been made clearer in the post.

I'm not arguing at all that every controller needs an Interactor, or all Rails codebases should start out with Interactors present.

I am saying that Interactors are very useful concepts for re-using code, and that many medium and large Rails codebases could benefit from them.

[dhh]

Please do share the 3 or 4 controllers all sharing this logic. I'd be happy to play code pong with them.

[dablweb]

Regardless of reuse, controller actions can get awfully large if unchecked. What would you say is the maximum LOCs for a public controller method?

Callbacks in Controllers and AR::Models tend to make things worse IMO for anything other than authentication; and for intricate actions interactors seem like the best defence.

I have had the (mis)fortune to jump into a number of large Rails codebases and I can say with hand-on-heart that the only ones that made any sense off-the-bat were ones using a this-or-similar pattern.

The community is embracing policies, interactors and decorators for a reason. Real developers are having real problems when Rails apps get a certain size, and there is no endorsed method on how to handle these problems.

Surely a couple of asides on the Rails docs and some official endorsement could help point new developers in the direction of a possible solution? A solution the industry already seems to be taking; regardless of whether 37Signals deems it fit for their particular domain.

[halostatue]

> What would you say is the maximum LOCs for a public controller method?

As small as it needs to be in order to get the job done, and as large as it must be to get it done clearly. Sometimes that's zero lines of code; sometimes it's 500. You can usually factor down a 500 line method, but it may be worth asking what the breadth cost is should the code really be single-use.

If your metric is anything else, then you're playing Stupid Metrics Games, like that espoused by Code Climate.

[dablweb]

You're really against Code Climate huh? Just what did it do to you to garner such hatred? I'm not personally a fan but I can see how it may help large companies in certain situations.

Anyways, when I asked about a "maximum" I was not asking in absolute terms. What do you see as a reasonable max for the majority of use cases?

At which point do you start feeling bad about your method?

[halostatue]

I'm against Code Climate because it promulgates nonsensical metrics in ways that are actively harmful to good engineering. Just like Bob Martin and his concept of “clean code”. All of these things are at best sigil markers. They aren't solutions, and they can't provide any answers.

I don't have a max length to a method, because I'm not a prescriptivist. What I have is a point where I stop understanding what a method is doing, or I can't explain it cleanly. Sometimes I keep the method as long, but better comment it. Sometimes I factor it out. The point at which I do so varies based on the code, time, and many other factors.

I've worked on a C++ codebase that had a 3,000+ line function that was nearly impossible to factor out because of a non-trivial amount of state that would have either:

a. required passing much of that state as parameters to the factored out functions/methods (even if you put it in a wrapper object/struct); or

b. extracting all of the (working) behaviour into one or more objects whose sole purpose is to encapsulate the behaviour of this function.

I absolutely hated working on that function, but it was essentially the main loop of the program. You could step through those 3k lines and get a fairly decent feel for what the program was going to do, when, and how often it would repeat. The hardest part was where people before had extracted code out…that was called exactly once. We tried three times to extract the code—and failed three times, ending up leaving the code the way it was because our extractions were more complex and less understandable than the existing crappy code.

My point is that there's no such thing as a reasonable max for the majority of use cases. It depends on what you're doing and in what code.

[dablweb]

"had a 3,000+ line function that was nearly impossible to factor out"

Sounds like the only reason it was nearly impossible to factor out was because someone didn't stop at line 500 and say "hey this is getting out of hand".

[kenrose]

I hope I wasn't a contributor to the 3,000+ line function you're describing.

[halostatue]

No, that's an earlier company I worked at. It existed before I got there, and I suspect it's still there now.

[dablweb]

Do you have an example of a public controller method larger than even 25 lines that does its job clearly?

[halostatue]

Controller method that I can share? No.

This method in mime-types is reported by Code Climate as a code smell. They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy. https://github.com/halostatue/mime-types/blob/master/lib/mim... The method is ~35 lines long. There are other cases I can provide from open source work (https://github.com/halostatue/mime-types/blob/master/lib/mim... is a good example: deprecated code, parser for a file type where splitting into multiple methods only complicates the logic flow).

It is true that the larger a method gets the less readable, understandable, and clear it gets—but that should never translate into the sort of nonsensical request you made, asking about maximum LOC. Asking that paints you as a prescriptivist who doesn't bother understanding the context the code requires.

I've been writing software for a long time, and while I try to write methods as short, clear, composable “paragraphs”, I sometimes will write something much longer than is readable because I can't figure out a meaningful way to break it down. That comes over time and reading and interaction with the code.

The best developers in my experience are pragmatic. They are aware of design patterns, but don't treat them like sewing patterns. They are aware of development practices, but don't treat them like holy writ. The GoF design patterns book is an excellent descriptivist treatise, but then people started treating it like a cookbook and looked for reasons to implement Patterns everywhere, rather than extracting the patterns from their code.

Blog posts like this one (that tell me that I should use an Interactor) pattern are actively dangerous, because they provide dicta without properly explaining the pain that the pattern evolved to solve, or the proper evolution of the pattern.

The current code base I work on shows a lot of evidence of Rails Fads just like this one, and I'm trying to get my team to step back and ask why we do things certain ways and to go back to first principles. Don't just reach for a Presenter because it's what was done before. Don't even reach for an ActiveModel::Serializer because it's what we're preferring now for API representations. Figure out your problem. State it clearly. Write the solution clearly. Find code that works similarly and figure out (a) if and (b) how they can be extracted into common code.

There is a cost to adopting things like Presenters and making smaller methods: your interface becomes larger. You can complain all you like of large files and functions, but code bases that have large numbers of classes whose purpose aren't clear…are harder to navigate and understand. (I have, in the current code base, unextracted code from external classes when that external class is used in one place and it makes the behaviour more understandable. It also provides a better place to understand where similar behaviour may appear later so that we can properly extract code if and when it is necessary later.)

[henrikn]

> They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy.

Wrong? Smallest it can possibly be? Those are strong claims.

I definitely disagree with Code Climate from time to time. But I try to keep an open mind.

I've found that trying to follow dumb rules can be enlightening, even when my first reaction is that I disagree. This goes for Code Climate as well as arbitrary rules, such as Sandi Metz': https://gist.github.com/henrik/4509394

I looked at your `priority_compare`, for example. As someone who did not write that code, I find it a little hard to follow. It has conditionals four levels deep (counting the ternaries). Your `Mime::TYPE` class is quite long, as is its test.

I would let the Code Climate feedback inspire me to attempt to extract a method object in this case, along these lines: https://gist.github.com/henrik/9355101

That would result in more code overall, but smaller objects on average, and to my eyes readability is improved because the complexity is a little abstracted. Do you truly feel it provides no value?

[halostatue]

Your extraction attempt actually proves my point: your code is more complex, is less readable, is demonstrably wrong, and destroys performance in at least two ways (I’d benchmark my “ugly” code against yours every time and win).

More complex and less readable: you’ve taken the comparison out of context of the class which is being used for comparison and put it in a different class and file that has to be opened for understanding a priority comparison. Worse, that separate class and file isn’t going to be reusable for anything else, because it depends heavily on the (admittedly large, but justifiably so) public interface of MIME::Type. So, readability is made worse for the sake of a dubious improvement in testability and no value in reuse.

If your editor supports code folding (it should, and you should be using it), then you don’t even have to see the implementation of `priority_compare` if you don’t need it.

Demonstrably wrong: you’re calling `#simplified_value` twice. You could make that not as ugly by caching, but that’s already known to be a waste of time because your extraction…

Destroys performance: `priority_compare` is used for sorting (as the code comment states outright). Creating a new object for each and every comparison is going to send sort performance to hell in a handbasket with rockets on it. It’s the same problem that plagues people who use decoration heavily: performance sucks and memory use gets ugly quick. The calling of `#simplified_value` twice doesn’t help here, and caching the value only makes memory use worse.

The code comment on `priority_compare` is clear: it’s a specialized implementation of `<=>` used for sorting from MIME::Types#[]. That should immediately halt any consideration of extracting the code into an object that needs creation, and continuing on with that is a failure of good software development in favour of purity suggested by stupid rules. Even extracting it into a class method is of highly questionable value, because now you have to (as you do with your extraction) know the left and right comparison objects, rather than (safely) assuming that the undecorated version is the object itself. It’s rather the difference between operators overloading in C++ where you need to provide both left and right values vs only providing the right value because the left value is the object itself.

I considered separating the if/elsif conditions to separate methods, but you still need the if/elsif conditions, and all you’re doing is pushing code around in ways that, once again, will not be used outside of `priority_compare`.

I had considered implementing `priority_compare` at the call-site, but it’s used at two places in MIME::Types. So, the `priority_compare` is reused, but the component parts aren’t, really.

I will admit to laying a trap with `priority_compare`: I’ve worked on this code off and on for ten years. Code Climate, being a naïve program, cannot understand how the code is going to be used, but only understands how it’s defined and measures it against some fairly simplistic and stupid metrics. They can be interesting when you are first approaching a program to understand hotspots, but their value drops to zero very quickly compared to the understanding you have from working with the code regularly.

Where I think Code Climate (and similar tools; they just happen to be fairly high-profile in the Ruby community) goes wrong is that a lot of inexperienced and naïve developers tend to treat it as Truth that Must Be Followed. Many of these folks are also fans of so-called “Clean Code” techniques and otherwise lack the experience necessary to know when not to follow these sorts of rules and assume that Code Climate is good becaus it simplifies the score to a GPA/grade.

[henrikn]

Also glanced at the load_from_v1 code.

I can't argue with the "deprecated code" bit – if you never need to understand or edit that code again, by all means leave it. There will be no payoff for spending time reducing the complexity.

But "splitting into multiple methods only complicates the logic flow" sounds less like Code Climate being wrong and more like you not having found good refactoring techniques, maybe.

I see several things that I think have a good chance of helping readability: extracting the whole thing to an object, then replacing the inline error handling with an error handling method ("begin" is a smell), extracting (and naming) whatever happens on each iteration of the "data" and so on.

[karmajunkie]

"The AR callbacks are wonderful for coordinating the domain model. Often times you'll want to create auxiliary objects when something else is created. That's what's it's there for. Or to otherwise keep the integrity of the domain model in place."

No, they're not. They're great for coordinating activities around your database. That's not a domain model, unless your domain is databases. Conflating domain with database through the use of active record doesn't mean that structural models are domain driven, and events based around database interactions aren't going to be any more domain driven than the active record objects themselves.

[tinco]

Please take a second to think about who you're saying "No, they're not." to. You'd think perhaps that he knows what he has AR callbacks in AR for, regardless of what you might think they're in there for.

The whole idea of ActiveRecord is that you to make your models with a clean interface to persistence. Among the features of that interface are the callbacks. They are there for your domain model to make use of, that's the whole story. If persistence is not part of your domain, why are you extending ActiveRecord?

[karmajunkie]

Please. I'm well aware of who the parent was. I'm well aware of what his intent was. And I'm well aware not only of the strengths of the active record pattern (which I implemented in at least two different languages five years before he put Rails out there—as did many, many others) but also its inherent weaknesses.

You keep using the term "domain model" to refer to active record objects, which makes it clear that you've bought into the Rails appropriation of the term. AR objects are not domain models. If they were, the persistence would be handled elsewhere. Compared to a real domain model, AR objects are just a short step above using a naked database driver itself (and not necessarily an improvement thereof.) So to answer your question ("why are you extending activerecord?") its because you're taking a shortcut to dumping data into your database, and failing to create a real domain model while you're at it. That's a path that works ok for plenty of apps—but you want to know why rails has a reputation as being great at prototypes and shitty at scaling complexity? Because ActiveRecord is shitty at scaling complexity, and that's a hell of a lot more relevant to the vast majority of applications than scaling performance.

[tinco]

Oh I insist.

"If they were, the persistence would be handled elsewhere."

Persistance is handled elsewhere, it's handled in the superclass. If you think that's not a good idea, you shouldn't be using ActiveRecord, because that's the way it works.

You could argue there's not enough abstraction from the DB in AR, but that's a different discussion. Here we discussed if the persistance callbacks were an appropriate tool for in the domain model, and I reckon they are.

Also, any fool knows that putting lots of code in a single file is not a good way of scaling complexity, so yeah at some point you're going to need to break your code out in concerns and what not. ActiveRecord most certainly does not stand in the way of that.

[karmajunkie]

Jesus christ. You aren't seriously arguing that handling it in a parent class removes it from the AR object's responsibilities, are you? In point of fact, that inheritance pattern is a bad idea, and I don't use it when I'm implementing an actual domain model, but that's seriously irrelevant.

1) I did argue exactly that; 2) it follows from (1) that an evented mechanism based on the database lifecycle of such objects is about the persistence of said objects, not about the domain.

lastly, if you think code complexity is about how fat your models are, I urge you to get out of "the rails way" for awhile and do some heavy reading. Moving code into concerns is just geography, and having lots of code in a single class is merely the most visible symptom of complexity.

[tinco]

No. I was not arguing anything about it, but the fact that it is a proper abstraction.

The persistence is part of the domain. You need to know about when your objects are persisted and when they are not, this can be an intrinsic part of some of your business logic. Thus the events are relevant to your domain.

Of course not all your business logic needs to be aware of persistence, and nowhere do you see me arguing that all business logic should be in concerns included into the AR class.

Don't put words in my mouth, and don't assume I am stupid. You are arguing in a rather ad-hominem way, and it is diminutive to your arguments.

[djur]

Concerns are just a complicated way of putting lots of code in a single class. The interface is the same if you put everything in the same file or if you break it out into a dozen mixins/concerns.