4 ms·

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 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.
    - mike_hock 3y ago
      How about the JUCS principle - Just Use Common Sense. What do you want to see when you come back in half a year to make a change? Write that. What does YAGNI even mean if it can stand in opposition of DRY? If it just means "you don't need to do that," and "that" can mean whatever you want, then it's not a useful guiding principle. Example of a YAGNI violation: You're writing a piece of code whose purpose is to download some kind of asset for whatever thingamajigga you're working on. It just needs to HTTP GET the resource from your official server. To future-proof it, you also add support for using any of the HTTP verbs as well as adding custom request headers. Not only have you written useless code that will likely never be needed, you've leaked implementation details (all these HTTP details don't make sense if a future version wants to use a different protocol). Example of what is not a YAGNI violation: Your asset downloader also needs to parse some sidechannel information that the server returns as custom response headers. You just assume that header names are capitalized the way your server sends them and don't bother with case-insensitivity. That isn't following "YAGNI," that's leaving a ticking time bomb behind. YAGNI is about functionality, DRY is about structure.
      - lstamour 3y ago
        > What does YAGNI even mean if it can stand in opposition of DRY? If it just means "you don't need to do that," and "that" can mean whatever you want, then it's not a useful guiding principle. > YAGNI is about functionality, DRY is about structure. I absolutely see where you’re coming from and I agree, the reason DRY caught on and YAGNI hasn’t is indeed the vagueness of YAGNI. But I would equally argue that the simplicity of DRY is what gets you in trouble if you over-optimize for DRY regardless of context. DRY can be about functionality: if you’ve already implemented a feature, even if it’s a library maintained by another team, as long as the tests encourage your use case, you should re-use existing functionality over creating yet another abandoned half-baked attempt at something. YAGNI, by opposition, might suggest that the effort spent learning the other team’s library and maintaining a connection to what might be brittle code is itself overhead. A similar debate can be found in using an npm library. DRY would argue if you already use the library, keep using it. YAGNI would argue if the library is only used in one place, rip it out for simplicity and replace it, especially if the code is old. Taken to extremes, DRY is how we end up with a left-pad module on npm while YAGNI is using padStart built-in to modern JS. Likewise taken to extremes, YAGNI may result in missing functionality and unmaintainable spaghetti code, while DRY is one possible approach to taming complexity in codebases. Both are essential principles, neither actually more important than the other. I agree with JUCS, but it’s even less defined than YAGNI ;-)
- whstl 3y ago
  The encapsulation here is exactly the same as you would have in a 6-line function. And here’s a bonus: the testability is exactly the same in both cases too (unless you’re breaking encapsulation in your tests — another thing that’s awfully common in Rails). The only difference is the readability. With a 6-line function you have perfectly readable code without jumping around.
  - vidarh 3y ago
    As I think we've both pointed out, it's actually less encapsulated, since you can do dirty stuff like `send` or `instance_eval`, while we can't "break into" the internal state of a single method.
- michaelteter 3y ago
  And as most commentors totally miss, this implementation "hiding" (encapsulation) reduces coupling and makes writing tests MUCH easier. If you want to test all paths on a multi-line function, you have to do a lot of duplication and sometimes a lot more setup for each variation you're trying to test. But another source of complexity is OOP itself. Mixing code and data is a horrible affliction we have accepted. All we need is some data structure (which can be a rigid class/struct, and then a module of appropriate operations which work on that data structure (or even elements of that structure). We can have mostly pure functions that are 100% covered by tests... and if we choose appropriate names for these functions it allows us to think in terms of higher level "steps" (each of which we really don't care about how they are implemented). All that said, we're debating over relatively small differences of approach in Ruby. For a really good time (not), go look at Python codebases. Then some of the people here who are arguing for a single function with 10 lines that just does the full job will find themselves staring at 100 line Python functions and thinking, "we should break this into individual functions" :).
  - whstl 3y ago
    All methods are private except for the initializer and create_user. Therefore, there's no difference in terms of "implementation hiding (encapsulation)" between the original 4-line function [0] and the class I presented. Also, there is no testability differences, unless you break encapsulation in your tests and test private methods directly with things like send(:find_admin_role). As much as this is seen in Ruby, this is not something that would pass a code review. This class is just 9 methods in a trenchcoat pretending to be that 4-line function. And in case you think this not a strawman example. I provide a real-world case in another message. [2] [1] https://news.ycombinator.com/item?id=38492002 https://news.ycombinator.com/item?id=38492002 [2] https://news.ycombinator.com/item?id=38493809 https://news.ycombinator.com/item?id=38493809
  - vidarh 3y ago
    This implementation isn't hiding any more details than a single method. In fact, in Ruby it hides less, because bypassing "private" is trivial, and manipulating an object's instance variables is trivial. If you care about encapsulation, that makes this approach worse than a single method. If you need to maintain state across invocations (this example doesn't), then capturing local variables with a closure (lambda/proc) provides far tighter encapsulation than a regular object in Ruby.

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.
  - mike_hock 3y ago
    How about the JUCS principle - Just Use Common Sense. What do you want to see when you come back in half a year to make a change? Write that. What does YAGNI even mean if it can stand in opposition of DRY? If it just means "you don't need to do that," and "that" can mean whatever you want, then it's not a useful guiding principle. Example of a YAGNI violation: You're writing a piece of code whose purpose is to download some kind of asset for whatever thingamajigga you're working on. It just needs to HTTP GET the resource from your official server. To future-proof it, you also add support for using any of the HTTP verbs as well as adding custom request headers. Not only have you written useless code that will likely never be needed, you've leaked implementation details (all these HTTP details don't make sense if a future version wants to use a different protocol). Example of what is not a YAGNI violation: Your asset downloader also needs to parse some sidechannel information that the server returns as custom response headers. You just assume that header names are capitalized the way your server sends them and don't bother with case-insensitivity. That isn't following "YAGNI," that's leaving a ticking time bomb behind. YAGNI is about functionality, DRY is about structure.
    - lstamour 3y ago
      > What does YAGNI even mean if it can stand in opposition of DRY? If it just means "you don't need to do that," and "that" can mean whatever you want, then it's not a useful guiding principle. > YAGNI is about functionality, DRY is about structure. I absolutely see where you’re coming from and I agree, the reason DRY caught on and YAGNI hasn’t is indeed the vagueness of YAGNI. But I would equally argue that the simplicity of DRY is what gets you in trouble if you over-optimize for DRY regardless of context. DRY can be about functionality: if you’ve already implemented a feature, even if it’s a library maintained by another team, as long as the tests encourage your use case, you should re-use existing functionality over creating yet another abandoned half-baked attempt at something. YAGNI, by opposition, might suggest that the effort spent learning the other team’s library and maintaining a connection to what might be brittle code is itself overhead. A similar debate can be found in using an npm library. DRY would argue if you already use the library, keep using it. YAGNI would argue if the library is only used in one place, rip it out for simplicity and replace it, especially if the code is old. Taken to extremes, DRY is how we end up with a left-pad module on npm while YAGNI is using padStart built-in to modern JS. Likewise taken to extremes, YAGNI may result in missing functionality and unmaintainable spaghetti code, while DRY is one possible approach to taming complexity in codebases. Both are essential principles, neither actually more important than the other. I agree with JUCS, but it’s even less defined than YAGNI ;-)
whstl 3y ago
The encapsulation here is exactly the same as you would have in a 6-line function. And here’s a bonus: the testability is exactly the same in both cases too (unless you’re breaking encapsulation in your tests — another thing that’s awfully common in Rails). The only difference is the readability. With a 6-line function you have perfectly readable code without jumping around.
- vidarh 3y ago
  As I think we've both pointed out, it's actually less encapsulated, since you can do dirty stuff like `send` or `instance_eval`, while we can't "break into" the internal state of a single method.
michaelteter 3y ago
And as most commentors totally miss, this implementation "hiding" (encapsulation) reduces coupling and makes writing tests MUCH easier. If you want to test all paths on a multi-line function, you have to do a lot of duplication and sometimes a lot more setup for each variation you're trying to test. But another source of complexity is OOP itself. Mixing code and data is a horrible affliction we have accepted. All we need is some data structure (which can be a rigid class/struct, and then a module of appropriate operations which work on that data structure (or even elements of that structure). We can have mostly pure functions that are 100% covered by tests... and if we choose appropriate names for these functions it allows us to think in terms of higher level "steps" (each of which we really don't care about how they are implemented). All that said, we're debating over relatively small differences of approach in Ruby. For a really good time (not), go look at Python codebases. Then some of the people here who are arguing for a single function with 10 lines that just does the full job will find themselves staring at 100 line Python functions and thinking, "we should break this into individual functions" :).
- whstl 3y ago
  All methods are private except for the initializer and create_user. Therefore, there's no difference in terms of "implementation hiding (encapsulation)" between the original 4-line function [0] and the class I presented. Also, there is no testability differences, unless you break encapsulation in your tests and test private methods directly with things like send(:find_admin_role). As much as this is seen in Ruby, this is not something that would pass a code review. This class is just 9 methods in a trenchcoat pretending to be that 4-line function. And in case you think this not a strawman example. I provide a real-world case in another message. [2] [1] https://news.ycombinator.com/item?id=38492002 https://news.ycombinator.com/item?id=38492002 [2] https://news.ycombinator.com/item?id=38493809 https://news.ycombinator.com/item?id=38493809
- vidarh 3y ago
  This implementation isn't hiding any more details than a single method. In fact, in Ruby it hides less, because bypassing "private" is trivial, and manipulating an object's instance variables is trivial. If you care about encapsulation, that makes this approach worse than a single method. If you need to maintain state across invocations (this example doesn't), then capturing local variables with a closure (lambda/proc) provides far tighter encapsulation than a regular object in Ruby.