3 ms·

First of all, one of the biggest reasons to do this (and not mentioned) is unit testing. If there's several variations of input, it can simply testing because i

First of all, one of the biggest reasons to do this (and not mentioned) is unit testing. If there's several variations of input, it can simply testing because it's easier to write tests for smaller units of code (fewer or no mocks, and fewer parameters). This alone is probably enough to override any negative, in my opinion. Secondly, and in contrast to the point of the article, it can usually help readability. If you're wrapping standard library functions, then I agree, the examples in the article are bad candidates. If you're dealing with something where magic values are involved, for example, then something like the following can greatly help readability: /* Checks if a given user id was generated by the legacy system. These are either 5 digits numeric, or start with "B" or "L". */ is_legacy_user(userid) This method body can include the details about the strange legacy naming or links to documentation, and the code consuming this can focus on the logical implications without caring about these details. Put together, the code flow is easier to follow, and if you're in doubt about the method you can check out the unit tests to see what situations exist that might not be obvious at first glance. Edit: actually, I take that back. Even if you're just wrapping a standard library call, then there's really no issue so long as there's a domain reason (eg, is_legacy_userid() vs is_userid_fivedigits()), and there's unit tests. The unit tests can prove the standard library function is all that's needed.

pytester 7y ago
>First of all, one of the biggest reasons to do this (and not mentioned) is unit testing. This is one way in which unit testing can cripple code. It's especially bad when code that mainly has the job of integrating different systems is unit tested - it typically means adding two additional, unnecessary layers of indirection to test some completely trivial piece of logic while the thing that is most likely to actually fail (the integration itself) goes untested. Those unit tests covering trivial logic still fail when the APIs are changed, but they almost never fail in the presence of an actual bug and present a huge maintenance cost for no tangible benefit. In some particularly integration heavy code bases I've seen literally every unit test is an expensive waste that spaghettified the code. Indirection is a good idea if you're creating a thin abstraction over a piece of complex logic, but that shouldn't be something that is done just to accommodate an irrational desire to see unit test coverage % go up.
ktpsns 7y ago
I find your example actually being a counter-example. The comment should rather be at the top of the definition/implementation of is_legacy_user, so IDEs and users can read and extract it when neccessary (thinking of Javadoc/Python's docstrings). The obvious advantage is that you want to keep comment and function in sync, and not all the calling points distributed around the code. Also, your example is a counterexample because the function name should be spelling, and a comment like /* Checks if Foo */ is_foo(...) obviously duplicates the same information.

pytester 7y ago

>First of all, one of the biggest reasons to do this (and not mentioned) is unit testing. This is one way in which unit testing can cripple code. It's especially bad when code that mainly has the job of integrating different systems is unit tested - it typically means adding two additional, unnecessary layers of indirection to test some completely trivial piece of logic while the thing that is most likely to actually fail (the integration itself) goes untested. Those unit tests covering trivial logic still fail when the APIs are changed, but they almost never fail in the presence of an actual bug and present a huge maintenance cost for no tangible benefit. In some particularly integration heavy code bases I've seen literally every unit test is an expensive waste that spaghettified the code. Indirection is a good idea if you're creating a thin abstraction over a piece of complex logic, but that shouldn't be something that is done just to accommodate an irrational desire to see unit test coverage % go up.

ktpsns 7y ago

I find your example actually being a counter-example. The comment should rather be at the top of the definition/implementation of is_legacy_user, so IDEs and users can read and extract it when neccessary (thinking of Javadoc/Python's docstrings). The obvious advantage is that you want to keep comment and function in sync, and not all the calling points distributed around the code. Also, your example is a counterexample because the function name should be spelling, and a comment like /* Checks if Foo */ is_foo(...) obviously duplicates the same information.