6 ms·

What is wrong with this example exactly? This is what you want in a service object.

vidarh 3y ago
It's reducing readability by introducing extra methods you have to chase down where the method content is just as clear as the method names themselves. If there are multiple clients that might use those individual methods then maybe some of it is excusable. Personally I'd refuse to accept a commit like that if reviewing that code.
- ljm 3y ago
  I notice this pattern often appears as a result of taking rubocop’s default settings as gospel. It pretty much forces you into pointless abstraction by saying a method should be less than 5 lines, or a class less than 100, or whatever, even though such abstraction would offer no actual value. Oftentimes when I suggest to change a rubocop default it leads to epic arguments as if you’re desecrating the bible. In fact, it’s just silly to not apply critical thinking to the quality of your toolchain, and that includes questioning the decisions of your linter’s maintainers.
  - vidarh 3y ago
    In this case, though, it's not even producing shorter code. It's producing longer, less readable code.
lstamour 3y ago
The methods that start with “call_” and “find_” are particularly redundant, because they aren’t any simpler than the code they contain. Technically true of all the methods given but the rest could be excused as explaining less readable syntax or for future expansion if parameters change. Modules should be deep, not shallow, is one design approach. This half fulfills that because the methods are private, but it’s also true that the method names are effectively comments yet they obscure the natural reading order for the code. See John Ousterhout’s A Philosophy of Software Design for more on this idea of deep vs shallow modules. (Edit to add: another way of justifying the above service design pattern is to suggest that the code is DRY because you’ve extracted common sections to methods. This is taking DRY to an illogical extreme. You _should_ repeat yourself if your method calls are simple enough, and if you actually do need to repeat some logic in multiple places requiring a private method, you might find a better place to put it - for example, a filter method.)
- codr7 3y ago
  It's not only about DRY, it's also about encapsulating implementation details that you might want to change.
  - vidarh 3y ago
    Then encapsulate them when you do. In this case these methods are private. They're almost certainly not used anywhere else. The amount of code I see where people preemptively extract methods and even add methods they don't use to prepare for a hypothetical future situation that never comes is staggering.
    - lstamour 3y ago
      The “YAGNI” principle is something I’ve had to learn the hard way - and continue to learn it, just earlier this week, in fact. YAGNI and DRY are sometimes in vicious opposition. A pro tip I picked up from Gary Bernhardt’s Destroy All Software screencasts is to leave some DRYing (or other refactoring for readability/maintenance purposes) until after you write tests and are in a red-green-refactor cycle. You can always DRY your code later, especially now that IDEs make it so easy to extract methods for you with automatic population of required variables/parameters.
      - vidarh 3y ago
        Personally I think DRY only makes sense as long as either the code is shorter (here it's longer...), or you encapsulate logic, especially when that logic will be called from multiple places in the code base. Here it won't... I think some of these methods might be ok in a public helper class ("go here if you want to know the correct way of finding these kinds of objects so we have a single extension point"), but not as private methods... The irony here is they try to let you read the main codepath top down, but because they unnecessarily obscure details they force you to jump back and forth between a ton of methods instead, completely ruining any benefit. I love code that lets you actually read it sequentially, but then it needs to actually let you do that.
      - 3y ago
whstl 3y ago
The operations themselves are fine but the code style gets in the way of readability. A simple 4-line function instead of the 6-7 methods allows for the same functionality, the same level of encapsulation and for the same testability, without the “jumping around”. Here's the original text I inputted into ChatGPT that generated the code above: def create_user(email) user = User.create(email) user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end Pretty much anyone writing this kind of class would be able to understand the same code if it were written in a 4-line function in one glance. This here, however severely obscures the intent, the code flow and the operations.

It's reducing readability by introducing extra methods you have to chase down where the method content is just as clear as the method names themselves. If there are multiple clients that might use those individual methods then maybe some of it is excusable. Personally I'd refuse to accept a commit like that if reviewing that code.

ljm 3y ago
I notice this pattern often appears as a result of taking rubocop’s default settings as gospel. It pretty much forces you into pointless abstraction by saying a method should be less than 5 lines, or a class less than 100, or whatever, even though such abstraction would offer no actual value. Oftentimes when I suggest to change a rubocop default it leads to epic arguments as if you’re desecrating the bible. In fact, it’s just silly to not apply critical thinking to the quality of your toolchain, and that includes questioning the decisions of your linter’s maintainers.
- vidarh 3y ago
  In this case, though, it's not even producing shorter code. It's producing longer, less readable code.

lstamour 3y ago

The methods that start with “call_” and “find_” are particularly redundant, because they aren’t any simpler than the code they contain. Technically true of all the methods given but the rest could be excused as explaining less readable syntax or for future expansion if parameters change. Modules should be deep, not shallow, is one design approach. This half fulfills that because the methods are private, but it’s also true that the method names are effectively comments yet they obscure the natural reading order for the code. See John Ousterhout’s A Philosophy of Software Design for more on this idea of deep vs shallow modules. (Edit to add: another way of justifying the above service design pattern is to suggest that the code is DRY because you’ve extracted common sections to methods. This is taking DRY to an illogical extreme. You _should_ repeat yourself if your method calls are simple enough, and if you actually do need to repeat some logic in multiple places requiring a private method, you might find a better place to put it - for example, a filter method.)

codr7 3y ago
It's not only about DRY, it's also about encapsulating implementation details that you might want to change.
- vidarh 3y ago
  Then encapsulate them when you do. In this case these methods are private. They're almost certainly not used anywhere else. The amount of code I see where people preemptively extract methods and even add methods they don't use to prepare for a hypothetical future situation that never comes is staggering.
  - lstamour 3y ago
    The “YAGNI” principle is something I’ve had to learn the hard way - and continue to learn it, just earlier this week, in fact. YAGNI and DRY are sometimes in vicious opposition. A pro tip I picked up from Gary Bernhardt’s Destroy All Software screencasts is to leave some DRYing (or other refactoring for readability/maintenance purposes) until after you write tests and are in a red-green-refactor cycle. You can always DRY your code later, especially now that IDEs make it so easy to extract methods for you with automatic population of required variables/parameters.
    - vidarh 3y ago
      Personally I think DRY only makes sense as long as either the code is shorter (here it's longer...), or you encapsulate logic, especially when that logic will be called from multiple places in the code base. Here it won't... I think some of these methods might be ok in a public helper class ("go here if you want to know the correct way of finding these kinds of objects so we have a single extension point"), but not as private methods... The irony here is they try to let you read the main codepath top down, but because they unnecessarily obscure details they force you to jump back and forth between a ton of methods instead, completely ruining any benefit. I love code that lets you actually read it sequentially, but then it needs to actually let you do that.
    - 3y ago

whstl 3y ago

The operations themselves are fine but the code style gets in the way of readability. A simple 4-line function instead of the 6-7 methods allows for the same functionality, the same level of encapsulation and for the same testability, without the “jumping around”. Here's the original text I inputted into ChatGPT that generated the code above: def create_user(email) user = User.create(email) user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end Pretty much anyone writing this kind of class would be able to understand the same code if it were written in a 4-line function in one glance. This here, however severely obscures the intent, the code flow and the operations.