5 ms·

While I personally prefer more functional (non-oop) approaches, the example you show is positive to me. If the functions are well named, you can read up to the

While I personally prefer more functional (non-oop) approaches, the example you show is positive to me. If the functions are well named, you can read up to the "private" line and stop reading. You now know what this thing does, and you've not had to complicate your thought about HOW it does it. If you need to know how one of the primary actions is performed, you can jump to that source with a simple keypress. With some settings, you can simply hover to see the function expanded. What's more, writing tests - 100% coverage - for this example is trivial and clean. If you want to put the actual lines all in the main function, your tests will be a lot uglier and will involve more lines of test code.

whstl 3y ago
The problem is that I need to know HOW this class does something, especially when debugging. The unnecessary branching makes me need to keep an eye on the stack, and a lot of extra information in my head. The additional state (in this case @email and @user) needs to be tracked from a distance. Local variables are 100x better. If there's any mutation, I must watch out during debugging two or three levels deep. Often there are more variables than that. If I had the "role" and the "permission" as a parameter, I would need two extra instance variables. With a regular function this is just two arguments, a couple lines away from the usage site. The original code I wrote into ChatGPT to generate the code above shows me the what and the how with much more clarity: 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
- fknorangesite 3y ago
  Yes, but that's because the example we were given in this thread is just a toy strawman in that each of the methods are just simple one-liners anyway. So yeah, given this literal code sample, I agree the private methods are extraneous. The point of this pattern is to extract each operation into its own described-by-name method because they're each longer and more complicated than just a `<<` operator or whatever.
  - whstl 3y ago
    This is why it's difficult to have discussions about this. Pretty much anything that's bad enough will be countered by a "well, this is just a toy strawman", and status quo will prevail. Well, here's some production code from a previous project which I happen to have authorization to use for educational purposes, with entity and fields anonymized as courtesy of ChatGPT. https://pastebin.com/wLk5bRqE https://pastebin.com/wLk5bRqE This is by no means a rare thing in Rails world, and I have seen this in pretty much every Rails project I worked, save for one. Every single time: newcomers would complain about it. Even senior/staff engineers. Only the people building it enjoy it.
  - vidarh 3y ago
    I agree that is the point of this pattern. But in practice most places where I see it employed are full of single line method bodies because people have heard of the pattern but don't understand the (fairly frankly rare) instances where it is appropriate and/or don't understand when an operation is long enough that it makes the code clearer to break it out. Overall I see this pattern as a net negative - I've yet to work on a project where more than a tiny minority of service object classes have any reason to be one.

whstl 3y ago

The problem is that I need to know HOW this class does something, especially when debugging. The unnecessary branching makes me need to keep an eye on the stack, and a lot of extra information in my head. The additional state (in this case @email and @user) needs to be tracked from a distance. Local variables are 100x better. If there's any mutation, I must watch out during debugging two or three levels deep. Often there are more variables than that. If I had the "role" and the "permission" as a parameter, I would need two extra instance variables. With a regular function this is just two arguments, a couple lines away from the usage site. The original code I wrote into ChatGPT to generate the code above shows me the what and the how with much more clarity: 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

fknorangesite 3y ago
Yes, but that's because the example we were given in this thread is just a toy strawman in that each of the methods are just simple one-liners anyway. So yeah, given this literal code sample, I agree the private methods are extraneous. The point of this pattern is to extract each operation into its own described-by-name method because they're each longer and more complicated than just a `<<` operator or whatever.
- whstl 3y ago
  This is why it's difficult to have discussions about this. Pretty much anything that's bad enough will be countered by a "well, this is just a toy strawman", and status quo will prevail. Well, here's some production code from a previous project which I happen to have authorization to use for educational purposes, with entity and fields anonymized as courtesy of ChatGPT. https://pastebin.com/wLk5bRqE https://pastebin.com/wLk5bRqE This is by no means a rare thing in Rails world, and I have seen this in pretty much every Rails project I worked, save for one. Every single time: newcomers would complain about it. Even senior/staff engineers. Only the people building it enjoy it.
- vidarh 3y ago
  I agree that is the point of this pattern. But in practice most places where I see it employed are full of single line method bodies because people have heard of the pattern but don't understand the (fairly frankly rare) instances where it is appropriate and/or don't understand when an operation is long enough that it makes the code clearer to break it out. Overall I see this pattern as a net negative - I've yet to work on a project where more than a tiny minority of service object classes have any reason to be one.