4 ms·

Can confirm. This is literally what happened in a codebase in the past: // OLD CODE WITH COMMENTS: // check ... 4 lines of code // drop ... 4 lines o

Can confirm. This is literally what happened in a codebase in the past: // OLD CODE WITH COMMENTS: // check ... 4 lines of code // drop ... 4 lines of code // read ... 4 lines of code // do xxx ... 4 lines of code In total, that method was like 16 lines of code, with some short commented sections. And the sections used variables from before. Situation: New coder comes in, sees comments. Says "comments are bad, they get out of date, yadda yadda". So proceeds to change comment names into method names: // PROPOSED: COMMENTS => METHODS check() drop() read() doxxx() What coder forgot was the variables and context. As said before, the total was 16 lines of code with simple variables used between it. Not a big deal. But now when it has to be split into functions, it became like this: // ACTUAL 1: COMMENTS => METHODS a, b, c = check() d, e = drop(b) f, g = read(a, d) i = doxxx(e, g) And this was a language that didn't support multiple return types. So the code was actually: // ACTUAL 2: COMMENTS => METHODS class X { a, b, c} class Y { d, e } class Z { f, g } X x = check() Y y = drop(x.b) Z z = read(x.a, y.d) i = doxxx(y.e, z.g) So now coder added 3 more classes, with more lines of boilerplate. Then the coder decided, to manage this problem better. So they created some more interfaces and did some DI. The result was around a 250 line PR, which I cannot ping here anymore. When we pointed out the same logic was now 250 lines, the answers were: "It's inherent complexity which we didn't know how to manage. His/her solution scales better, and he/she has shown us the way". All for what? Because comments were considered a smell. Go figure. I guess this is why this other HN post trended – "Please do not simplify this code": https://news.ycombinator.com/item?id=18772873 https://news.ycombinator.com/item?id=18772873

Aardappel 8y ago
This might be a good interview question to identify net negative programmers like that. Give them the "ACTUAL 2" code, and ask them how to improve it. If they start talking about DI or any other additional abstractions, you have your red flag. And of course if the programmer asks "are X Y Z used anywhere else?" or has the balls to say "just inline it all into a linear flow of code", instant hire! :P
christophilus 8y ago
This is exactly the example I had in mind, but didn’t have the patience to write up.
jonahx 8y ago
You shouldn't conclude from this that the rule is bad. Because the original code, while preferable to the version with 3 classes, is not good either. Indeed, as this makes clear: // ACTUAL 1: COMMENTS => METHODS a, b, c = wait() d, e = drop(b) f, g = read(a, d) i = execute(e, g) There are complex dependencies among the subroutines which can probably be simplified with refactoring, and which should be explicitly called out, but are allowed to "hide" in the original code where, locally, we have 7 global variables shared by 4 inline subroutines -- not a good situation.
- ball_of_lint 8y ago
  I'll agree that one case doesn't make the rule bad, but the rule hasn't been proven yet either. > There are complex dependencies among the subroutines which can probably be simplified with refactoring, and which should be explicitly called out, but are allowed to "hide" in the original code where, locally, we have 7 global variables shared by 4 inline subroutines -- not a good situation. What is locally global supposed to mean? This piece of code can be written at least two ways: as a single function with comments or as a function calling four other functions. Just because splitting it up into multiple functions a certain way requires multiple returns or long argument lists doesn't mean that the original code is necessarily bad; It could be that the splitting points were chosen badly or that this code is simply better off as a single function. These "complex dependencies" you're worried about form a DAG in the example which seems simple enough to me. Anyways, most of this is moot without real code.
  - jonahx 8y ago
    > Anyways, most of this is moot without real code. Agreed, I hesitated answering because of this, but oh well. > What is locally global supposed to mean? It means within this local context, we have 7 global variables. Just because they aren't global to the entire program doesn't make them magically exempt from the problems of shared data. Specifically, it's too difficult to reason about logic and control flow, even in this small example. You cannot have strong confidence the code is bug-free just by inspecting it.
- ummonk 8y ago
ams6110 8y ago
Great example of why I never liked OO. It always seemed so complicated.
- zmmmmm 8y ago
  Doesn't sound like it was OO'd very well either. OO would move the check, drop etc. onto methods of the relevant objects. So it's sort of worst of all worlds here.
nerdponx 8y ago
It really depends. In my own software, I really appreciate when code is "clean" like that. I find it easier to keep in my head when methods are short. It also depends on logic reuse. If you are now able to use the `check` method in other places, then I say it's overall a successful refactor.
- rdsubhas 8y ago
  > I find it easier to keep in my head when methods are short. How short? 100 lines? 10 lines? 1 line? "It depends" => i.e. discuss forever in PRs towards a one-way "shorter and shorter" ticket? More and more coders who join the project say the same thing, and reduce methods shorter and shorter and shorter, until it's dozens of classes with one method each having small lines like "twiddle-doo" and "fiddle-foo". Of course, now the method is easy to memorize, and supposedly it's "clean code" now. But the functional value and flow is just completely screwed up and can never fit in the head of anyone.
  - nerdponx 8y ago
    I should clarify - I wouldn't accept a PR just to shorten a method for its own sake, if it's already written and working.
- zmmmmm 8y ago
  > I find it easier to keep in my head when methods are short I think that's true if "short" is measured in terms of internal complexity. I think each function should try to manage a small bite size amount of complexity that represents a reasonable level of cognitive load for a brand new person to reverse engineer. It could be quite a lot of lines of code if they are very simple ones, or it could be a one liner if it's super complex / obtuse.
  - nerdponx 8y ago
    Good point. Yes, I meant logically short. Maybe logically compact is more apt.

Aardappel 8y ago

This might be a good interview question to identify net negative programmers like that. Give them the "ACTUAL 2" code, and ask them how to improve it. If they start talking about DI or any other additional abstractions, you have your red flag. And of course if the programmer asks "are X Y Z used anywhere else?" or has the balls to say "just inline it all into a linear flow of code", instant hire! :P

christophilus 8y ago

This is exactly the example I had in mind, but didn’t have the patience to write up.

jonahx 8y ago

You shouldn't conclude from this that the rule is bad. Because the original code, while preferable to the version with 3 classes, is not good either. Indeed, as this makes clear: // ACTUAL 1: COMMENTS => METHODS a, b, c = wait() d, e = drop(b) f, g = read(a, d) i = execute(e, g) There are complex dependencies among the subroutines which can probably be simplified with refactoring, and which should be explicitly called out, but are allowed to "hide" in the original code where, locally, we have 7 global variables shared by 4 inline subroutines -- not a good situation.

ball_of_lint 8y ago
I'll agree that one case doesn't make the rule bad, but the rule hasn't been proven yet either. > There are complex dependencies among the subroutines which can probably be simplified with refactoring, and which should be explicitly called out, but are allowed to "hide" in the original code where, locally, we have 7 global variables shared by 4 inline subroutines -- not a good situation. What is locally global supposed to mean? This piece of code can be written at least two ways: as a single function with comments or as a function calling four other functions. Just because splitting it up into multiple functions a certain way requires multiple returns or long argument lists doesn't mean that the original code is necessarily bad; It could be that the splitting points were chosen badly or that this code is simply better off as a single function. These "complex dependencies" you're worried about form a DAG in the example which seems simple enough to me. Anyways, most of this is moot without real code.
- jonahx 8y ago
  > Anyways, most of this is moot without real code. Agreed, I hesitated answering because of this, but oh well. > What is locally global supposed to mean? It means within this local context, we have 7 global variables. Just because they aren't global to the entire program doesn't make them magically exempt from the problems of shared data. Specifically, it's too difficult to reason about logic and control flow, even in this small example. You cannot have strong confidence the code is bug-free just by inspecting it.
ummonk 8y ago

ams6110 8y ago

Great example of why I never liked OO. It always seemed so complicated.

zmmmmm 8y ago
Doesn't sound like it was OO'd very well either. OO would move the check, drop etc. onto methods of the relevant objects. So it's sort of worst of all worlds here.

nerdponx 8y ago

It really depends. In my own software, I really appreciate when code is "clean" like that. I find it easier to keep in my head when methods are short. It also depends on logic reuse. If you are now able to use the `check` method in other places, then I say it's overall a successful refactor.

rdsubhas 8y ago
> I find it easier to keep in my head when methods are short. How short? 100 lines? 10 lines? 1 line? "It depends" => i.e. discuss forever in PRs towards a one-way "shorter and shorter" ticket? More and more coders who join the project say the same thing, and reduce methods shorter and shorter and shorter, until it's dozens of classes with one method each having small lines like "twiddle-doo" and "fiddle-foo". Of course, now the method is easy to memorize, and supposedly it's "clean code" now. But the functional value and flow is just completely screwed up and can never fit in the head of anyone.
- nerdponx 8y ago
  I should clarify - I wouldn't accept a PR just to shorten a method for its own sake, if it's already written and working.
zmmmmm 8y ago
> I find it easier to keep in my head when methods are short I think that's true if "short" is measured in terms of internal complexity. I think each function should try to manage a small bite size amount of complexity that represents a reasonable level of cognitive load for a brand new person to reverse engineer. It could be quite a lot of lines of code if they are very simple ones, or it could be a one liner if it's super complex / obtuse.
- nerdponx 8y ago
  Good point. Yes, I meant logically short. Maybe logically compact is more apt.