5 ms·

This is just the Clean Code style. If you read the book (or an article with examples [0]), you'll see that it advocates exactly this kind of thing: refactoring

by pkkm 3y ago

This is just the Clean Code style. If you read the book (or an article with examples [0]), you'll see that it advocates exactly this kind of thing: refactoring functions that are "long" and "unreadable" because they have a dozen or two dozen lines into a "clean" and "maintainable" web of tiny functions calling one another. Personally, I find this style very mentally straining because I have to constantly jump around and keep track of a huge function call graph in my head; it's almost as bad as beginner code with a thousand lines per function. The popularity of fads like this in the Ruby community is one of the reasons I'm glad I switched to Python. [0] https://qntm.org/clean https://qntm.org/clean

whstl 3y ago
“Mentally straining” is a good way of putting it. It also makes debugging much harder, as you have to keep track of instance variables in your head.
ht85 3y ago
If only functions could take arguments... Doesn't that already look 100x better? class UserCreator def create_user(email) user = create_user_object(email) assign_admin_role(user) assign_invite_permission(user) call_invite_template(user) end private def create_user_object(email) User.create(email) end def assign_admin_role(user) user.roles << find_admin_role end def assign_invite_permission(user) user.permissions << find_invite_permission end def call_invite_template(user) find_invite_template.call(user: user) end def find_admin_role Roles.find_by(name: 'admin') end def find_invite_permission Permissions.find_by(name: 'invite') end def find_invite_template Templates.find_by(name: 'invite') end end
- whstl 3y ago
  Slightly less worse, but the original function that I inputted into ChatGPT to generate the code from my message was this: def create_user(email) user = User.create(email: email) user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end IMO this 4 line function is significantly better in terms of clarity, readability, and it avoids unnecessary state. Testability and encapsulation are the same. (I would argue that the encapsulation is better with a function, since encapsulation is way too easy to break in Ruby, but hey, that's me)
  - vidarh 3y ago
    I think the choice between a method (ruby doesn't have functions ;) ) and a class here may well be right. The only reason for using a class here is more as a general principle if you want it to be easier to pass your "usercreator" around as a value, but you can of course wrap it in a lambda if needed or use `#method`. Alternatively you can just make it a lambda from the start. If your main objection to the class is the ease of breaking encapsulation, lambda's are quite nice for tighter encapsulation of state in Ruby. E.g. lambda do state = 0 ->() { state += 1 } end.call (Note the `.call` at the end - we don't want the outer lambda; that is just used for creating the variables that will hold the state - we want the inner lambda) So let's say that for some stupid reason your user creator object needs to keep a count of users, you could "seal" that state into a lambda with the above technique, and make it really hard (I'm not sure if impossible) to break encapsulation. E.g. instance_eval or trying obtain the binding doesn't work, because they both will get you the state of the Proc object containing the closure, not the local variables accessible within it.
- vidarh 3y ago
  No? It provides some very marginal improvement, but doesn't address the actual problem of the code, which is the horrific verbosity. The use of attributes is the least of my problem with it. This is how I'd want it to look: class UserCreator # Arguably, I'd prefer *call* because that allows it to be interchangeable with a lambda # But frankly this thing could *be* a lambda. E.g. you could replace the above class declaration # with "UserCreator = ->(email) do" and ditch the "def" # The exception, where I'd allow for an initialize and attributes would be in cases where you'd otherwise # be passing a *lot* of *the same* state around between multiple methods # def self.create_user(email) User.create(email).tap do |user| user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end end end The whole service object pattern is heavily abused by people who don't seem to understand which (limited) situations it actually provides benefits. This isn't one of them.
  - ht85 3y ago
    I completely agree, I hate that way of coding and would go for the concise version.
trealira 3y ago
In a way, it seems to have similar problems to those of unstructured programming (i.e. rampant use of goto instead of procedures and loops), just to a lesser degree (since jumps that always return are still easier to understand than ones that don't). It's hard to understand the code when the control flow jumps everywhere, even when all the jumps are subroutine or method calls. A method having a good name doesn't necessarily mean it provides a better abstraction than writing the code inline, just like having well-named labels doesn't necessarily make the code easier to understand.
vidarh 3y ago
As a Ruby developer, I can't stand this style either. People ought to be aware that to many of us this does indeed also screams "beginner". It shows people slavishly following advice they don't understand. I kinda like the Clean Code advice of small functions/methods, but they should pack a punch, not just encapsulate barely more than their own name. E.g. here's a single-line method from the Window class of my Ruby X11 window manager: def type = (@type ||= get_property(:_NET_WM_WINDOW_TYPE, :atom)&.value.to_i) This does one thing: It queries the X server for the _NET_WM_WINDOW_TYPE atom attached to this window, and if a value is returned, it ensures the result is an integer (which will be 0 if what is returned is nil or somehow can't be parsed into an int, which would mean someone has been very naughty but defaulting to 0 is fine). It then ensures the value is memoised in `@type` (not doing more error checking here is fine, because failing is worse than defaulting to not knowing the window type, as we have no guarantee it's set in the first place) Tiny functions like that are fine, because they encapsulate a sufficient amount of non-obvious logic in a very concise result. Being able to write `my_window.type` without caring about that logic is great. Trying to reduce your methods to compact little methods is great when it makes you write little functions that tells you a lot. Unfortunately - and so I agree with your reaction to Clean Code - adhering to the rest of the Clean Code advice won't leave you with a bunch of little methods that pack a punch and remove a bunch of complexity, but a bunch of little methods that each encapsulate barely more than their method names worth of complexity.

by pkkm 3y ago

whstl 3y ago

“Mentally straining” is a good way of putting it. It also makes debugging much harder, as you have to keep track of instance variables in your head.

ht85 3y ago

If only functions could take arguments... Doesn't that already look 100x better? class UserCreator def create_user(email) user = create_user_object(email) assign_admin_role(user) assign_invite_permission(user) call_invite_template(user) end private def create_user_object(email) User.create(email) end def assign_admin_role(user) user.roles << find_admin_role end def assign_invite_permission(user) user.permissions << find_invite_permission end def call_invite_template(user) find_invite_template.call(user: user) end def find_admin_role Roles.find_by(name: 'admin') end def find_invite_permission Permissions.find_by(name: 'invite') end def find_invite_template Templates.find_by(name: 'invite') end end

whstl 3y ago
Slightly less worse, but the original function that I inputted into ChatGPT to generate the code from my message was this: def create_user(email) user = User.create(email: email) user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end IMO this 4 line function is significantly better in terms of clarity, readability, and it avoids unnecessary state. Testability and encapsulation are the same. (I would argue that the encapsulation is better with a function, since encapsulation is way too easy to break in Ruby, but hey, that's me)
- vidarh 3y ago
  I think the choice between a method (ruby doesn't have functions ;) ) and a class here may well be right. The only reason for using a class here is more as a general principle if you want it to be easier to pass your "usercreator" around as a value, but you can of course wrap it in a lambda if needed or use `#method`. Alternatively you can just make it a lambda from the start. If your main objection to the class is the ease of breaking encapsulation, lambda's are quite nice for tighter encapsulation of state in Ruby. E.g. lambda do state = 0 ->() { state += 1 } end.call (Note the `.call` at the end - we don't want the outer lambda; that is just used for creating the variables that will hold the state - we want the inner lambda) So let's say that for some stupid reason your user creator object needs to keep a count of users, you could "seal" that state into a lambda with the above technique, and make it really hard (I'm not sure if impossible) to break encapsulation. E.g. instance_eval or trying obtain the binding doesn't work, because they both will get you the state of the Proc object containing the closure, not the local variables accessible within it.
vidarh 3y ago
No? It provides some very marginal improvement, but doesn't address the actual problem of the code, which is the horrific verbosity. The use of attributes is the least of my problem with it. This is how I'd want it to look: class UserCreator # Arguably, I'd prefer *call* because that allows it to be interchangeable with a lambda # But frankly this thing could *be* a lambda. E.g. you could replace the above class declaration # with "UserCreator = ->(email) do" and ditch the "def" # The exception, where I'd allow for an initialize and attributes would be in cases where you'd otherwise # be passing a *lot* of *the same* state around between multiple methods # def self.create_user(email) User.create(email).tap do |user| user.roles << Roles.find_by(name: 'admin') user.permissions << Permissions.find_by(name: 'invite') Templates.find_by(name: 'invite').call(user: user) end end end The whole service object pattern is heavily abused by people who don't seem to understand which (limited) situations it actually provides benefits. This isn't one of them.
- ht85 3y ago
  I completely agree, I hate that way of coding and would go for the concise version.

trealira 3y ago

In a way, it seems to have similar problems to those of unstructured programming (i.e. rampant use of goto instead of procedures and loops), just to a lesser degree (since jumps that always return are still easier to understand than ones that don't). It's hard to understand the code when the control flow jumps everywhere, even when all the jumps are subroutine or method calls. A method having a good name doesn't necessarily mean it provides a better abstraction than writing the code inline, just like having well-named labels doesn't necessarily make the code easier to understand.

vidarh 3y ago

As a Ruby developer, I can't stand this style either. People ought to be aware that to many of us this does indeed also screams "beginner". It shows people slavishly following advice they don't understand. I kinda like the Clean Code advice of small functions/methods, but they should pack a punch, not just encapsulate barely more than their own name. E.g. here's a single-line method from the Window class of my Ruby X11 window manager: def type = (@type ||= get_property(:_NET_WM_WINDOW_TYPE, :atom)&.value.to_i) This does one thing: It queries the X server for the _NET_WM_WINDOW_TYPE atom attached to this window, and if a value is returned, it ensures the result is an integer (which will be 0 if what is returned is nil or somehow can't be parsed into an int, which would mean someone has been very naughty but defaulting to 0 is fine). It then ensures the value is memoised in `@type` (not doing more error checking here is fine, because failing is worse than defaulting to not knowing the window type, as we have no guarantee it's set in the first place) Tiny functions like that are fine, because they encapsulate a sufficient amount of non-obvious logic in a very concise result. Being able to write `my_window.type` without caring about that logic is great. Trying to reduce your methods to compact little methods is great when it makes you write little functions that tells you a lot. Unfortunately - and so I agree with your reaction to Clean Code - adhering to the rest of the Clean Code advice won't leave you with a bunch of little methods that pack a punch and remove a bunch of complexity, but a bunch of little methods that each encapsulate barely more than their method names worth of complexity.