12 ms·

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

by dhh 13y ago

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.html 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 13y ago
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 13y ago
  Please do share the 3 or 4 controllers all sharing this logic. I'd be happy to play code pong with them.
dablweb 13y ago
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 13y ago
  > 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 13y ago
    Do you have an example of a public controller method larger than even 25 lines that does its job clearly?
    - halostatue 13y ago
      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/mime/type.rb#L155 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/mime/types/loader.rb#L126 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.)
karmajunkie 13y ago
"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 13y ago
  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 13y ago
    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.

by dhh 13y ago

tomblomfield 13y ago

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 13y ago
Please do share the 3 or 4 controllers all sharing this logic. I'd be happy to play code pong with them.

dablweb 13y ago

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 13y ago
> 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 13y ago
  Do you have an example of a public controller method larger than even 25 lines that does its job clearly?
  - halostatue 13y ago
    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/mime/type.rb#L155 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/mime/types/loader.rb#L126 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.)

karmajunkie 13y ago

"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 13y ago
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 13y ago
  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.