9 ms·

Am I weird in feeling like the code in this file is really really... normal? Like, it's verbose in certain ways due to being written in Go, as well as due to no

Am I weird in feeling like the code in this file is really really... normal? Like, it's verbose in certain ways due to being written in Go, as well as due to not relying on any deep abstractions (and I don't mind this - abstractions are a double-edged sword), but in general, as code, it seems typical - and if the header text didn't exist I wouldn't think twice about the style it's written in. Maybe the disconnect here is that most of my experience is in enterprise software rather than systems software. Perhaps many of the comments in this file seem unnecessary to regular contributors within the k8s project? Whereas if I were writing this same code in an enterprise (and thus expect it to be read by people far in the future lacking context on all the technical details) I would have put -more- comments in this file, given the sheer complexity of all it's doing.

mumblemumble 2y ago
I wish code like this still felt normal to me, but over the past ~10 years it seems that many people have come to value brevity over explicitness. I strongly prefer the explicitness, at least for important code like this. More than once in my career I've encountered situations where I couldn't figure out if the current behavior of a piece of code was intentional or accidental because it involved logic that did things like consolidating different conditions and omitting comments explaining their business context and meaning. That's a somewhat dangerous practice IMO because it creates code that's resistant to change. Or at least resistant to being changed by anyone who isn't its author, though for most practical purposes that's a distinction without a difference. Unnecessarily creating Chesterton's fences is anti-maintainability.
- bhauer 2y ago
  Agreed. Explicitness and comments are very useful in understanding the intended functionality and logic, whether or not the code actually implements that intent correctly (an in providing that intent, they can help identify bugs earlier than they would be identified otherwise).
  - dskrvk 2y ago
    But comments go out of date, and the compiler doesn’t check them against the implementation. To document + enforce the intended functionality, use tests.
    - runevault 2y ago
      Note in Rust if you include comments with code that will run as tests but be inline in your main code instead of having to find the relevant test function to confirm functionality. https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html https://doc.rust-lang.org/rustdoc/write-documentation/docume...
      - rob74 2y ago
        Go has something similar: functions marked as examples that are both run as tests and shown and run as examples in the documentation (https://go.dev/blog/examples https://go.dev/blog/examples)
        runevault 2y ago
        Interesting. I don't know when that was implemented in Rust but clearly Go has had it for a long time, since that post is dated 2015. While things like syntax are important, languages adding tooling like this (along with stuff like package managers) is so important to the continued evolution of the craft of software development.
    - dijksterhuis 2y ago
      tests -> verify intended functionality implementation (the how is right). comments -> why intended functionality was implemented that specific way (marketing wanted X because of Y, so we had to do it like Z with a bit of A). > But comments go out of date Just like updating the tests when code is changed, update the comment when the code is changed.
      - jgwil2 2y ago
- spacemadness 2y ago
  Every time some code reviewer comes into my PR and says something along the lines of "you know you can just write it this way" where "this way" means obfuscating the code because "clever" and "shorter," I die a little on the inside. This is from experienced devs who should know better. At one point I wrote a comment write above a section I knew would be targeted by this kind of thinking explaining it must be written this way for clarity. Sometimes that works.
  - kstrauser 2y ago
    I only recommend that if the more explicit code is less idiomatic. For example, if someone appends to a list in a Python loop where a list comprehension would express the same thing plainly, I’ll suggest it. That’s it. Otherwise, please optimize for writing stuff I can understand at 2AM when things have broken.
  - ungreased0675 2y ago
    I’m not a dev, but I manage them. On one team they were spending many hours on code golf and nothing was being built. I pushed the devs to passing testing=PR accepted. In your opinion, what problems might come from removing opinionated code reviews? Why do some reviewers gravitate toward “Here’s how I would have written it?”
    - consteval 2y ago
      I think the biggest problem that can come of it is lack of standards. Each dev has their own way of writing things, their own little language. To them it is perfect. And it all works, passes tests. But, if you let them do this then your code becomes sloppy. Styles go in and out. Like reading a book where every other paragraph is written by a different person, and none of them talked to each other. Sometimes you get PascalCase, sometimes camel_case, sometimes snakeCase. Sometimes booleans are real bools, sometimes they're "Y" "N" (yes, real) or sometimes they're ints. Sometimes functions take in a lot of arguments, sometimes they take in a struct with options. Sometimes missing data is nullable, other times it's an empty string, othertimes its "N/A". And on and on. You can enforce a lot of this through automatic linters, but not all. You require a set standard and the PR procedure can enforce the standard.
- DSMan195276 2y ago
  I agree to a point, but I would separate explicit code from excessive commenting. Explicit code is good because it lets you explain to the reader what you're actually trying to do. Excessive comments (or even comments in general) is less so because compiler cannot check them for correctness, if someone simply forgets to update a comment or writes it incorrectly then the only thing to potentially catch it is a code review.
  - throwaway2037 2y ago
    Do you consider this example as "excessive commenting"?
    - DSMan195276 2y ago
      I haven't looked close enough at it to really know for sure. I'm not saying it's _always_ bad, comments are helpful, but the problem is that unlike code they are not required to actually match reality. In this case, I see several `if`s with no corresponding `else` even when the `if` section does not throw/return at the end, and that's largely my point. If the "space shuttle code" requirement is not actually rigorously followed, then why go on at length about it? And if it really is that important, then the comment about it is not good enough. Rather than a comment about it that can be ignored, they should set up a static analyzer to enforce it at build time. That way you're forced to follow the convention and not relying on code reviewers that probably don't even see that comment during their review.
- claytongulick 2y ago
  > I strongly prefer the explicitness I have a rule for my teams: "Don't write clever code". I try to constantly reinforce that we don't write code for ourselves, we write it for the next person. We should be doing everything in our power to decrease their cognitive load. I try to envision the person that comes after me (who may be me in months or years!) and imagine that they are having a Bad Day and they have to make changes to my code. Good code is clear, and tells a story. A story that's easy to follow, and easy to drill into. Not to knock elixir unfairly, but I think that's the basis of my mental block with that language. It seems to be designed from the ground up to violate that rule. Everything is elixir is clever. Very clever. Too clever for me.
  - koudelka 2y ago
    Can you give some examples of how elixir is too clever? It’s been a breath of explicit fresh air for me…
    - claytongulick 2y ago
      I know you can do these sorts of funky exercises [1] with pretty much any language, but elixir breaks my brain for some reason. [1] https://evuez.net/posts/cursed-elixir.html https://evuez.net/posts/cursed-elixir.html
  - imposterr 2y ago
    >I try to constantly reinforce that we don't write code for ourselves, we write it for the next person. Heck, it does't even need to be another person. Even me 10 months from now who may have forgotten some context around some code will appreciate boring code.
  - raxxorraxor 2y ago
    That is a good rule. If you want to write clever code, do some code golf. For everything else I heavily prefer explicitness. Smart software architecture hides the verbosity if you do not have business with a specific and most likely specialized piece of code. Even with ugly code, you should think about refactoring if this particular piece did run successfully for several years and there are no security related issues. There are several languages the violate this principle. Brainfuck is probably one of the most prominent. It is of course not to be taken seriously. Overall alleged "elegance" of some code parts is rather annoying if you really need to understand and adapt it.
- Buttons840 2y ago
  The first developer I ever worked with was very explicit, and I learned some important lessons from him. He had created a system in PHP that controlled printers and it was very explicit. He didn't know what a function was, his code had no functions. It was a 5000 line script that would run on a Windows timer, top to bottom. In some places the control structures were nested 17 deep, I counted; an if-statement inside an if-statement inside a while-loop inside a for-loop inside an if-statement inside a for-loop inside an if-statement, etc, 17 deep. The cyclomatic complexity was in the thousands. I never could understand that code. I know it wasn't brief, does that mean it was explicit? I think the truth is brevity and explicitness are orthogonal. Let me ask you this: can code be both brief and explicit? What would that look like?
  - mckn1ght 2y ago
    I’m not sure brevity and explicitness are totally orthogonal, explicit sort of implies spelling things out in a longer way. That doesn’t mean that something that is long is thorough, however. I like the tradeoff the Swiftlang project talks much about: brevity vs clarity (because explicit doesn’t necessarily mean clear, either, as your example shows). I think those are more orthogonal concerns, both important to think about, for different reasons that may often compete.
- motherfsck 2y ago
  I used to have opinions but I don't care anymore. After a spelunking into all manner of code bases I just match the style and move on. Verbose or terse, comments at the top or inline or nor none at all, 100 or 5 line functions, etc. I treat it like a card game; follow what's led.
bhauer 2y ago
You may be weird, but you're not alone. I too think this looks perfectly normal. I have written similar-looking code and comments (at least from a high-level point of view) for any components that I feel are critical to the reliability of the system. I've never subscribed to the "no comments" fad since my own comments have all too often been invaluable to future me when returning to the code after months or years of attention elsewhere. I can't imagine trying to piece together all of the embedded logic in a component of this complexity without solid comments.
remram 2y ago
I know. I would never remove an "empty" branch full if comments, that's the compiler's job.
zdragnar 2y ago
The one thing that stood out to me is the rather extensive nesting of if-else. I know this is go code, but there's multiple places that go five branches deep, at least. It works but it's a style that takes a lot of effort to grow what's happening IMHO.
omoikane 2y ago
Most likely, this comment was added in response to a botched attempt to simplify code, to serve as a warning for future maintainers to think twice before making a similar attempt. The commit that added the warning was "Add note about space-shuttle code style"[1], and the one before that was "Revert controller/volume: simplify sync logic in syncUnboundClaim"[2] [1] https://github.com/kubernetes/kubernetes/commit/de4d193d45f638b8e805a53945e55d6b4b745293 https://github.com/kubernetes/kubernetes/commit/de4d193d45f6... [2] https://github.com/kubernetes/kubernetes/commit/8a1baa4d64ca6c0622c17d6a421ce7b608698801 https://github.com/kubernetes/kubernetes/commit/8a1baa4d64ca...
- croshan 2y ago
  Nice find. I think you're right. FWIW, I don't think the code style in [2] is less simple (slightly more readable, to use `if (X) {} else {}` rather than `if (!X) {} else {}`, for example). So to me, this reads as the author of [1] is just overcorrecting by adding process, when some test cases or code review would've been more helpful in preventing whatever incident [2] caused.
hyperpape 2y ago
In particular, the every if has a matching else comment doesn't seem reliably true. Many of the unmatched ifs are just simple if (err != nil) { checks, but or other early returns, but outside of those, there do seem to be unmatched ifs. That said, my experience in enterprise software isn't that extra comments are necessarily present (there was a plague of "// end if" comments in the codebase, but actual descriptive comments were rare).
- wavemode 2y ago
  Yeah I should clarify that I was speaking personally about new code. I definitely encounter plenty of legacy code that is completely inscrutable. But, I can say in 2024 at least, that most teams I've been on value explaining complex logic (especially logic that can break in subtle ways if not properly maintained) with comments, in new code we write.
marcus_holmes 2y ago
I found myself thinking the same thing until I got to the hugely nested if statements. I would definitely have created some early-return branches to that thing. It does feel like the first "make it work" step (from "make it work, make it fast, make it pretty"), and then they just didn't do the "make it pretty" step. I have written code this "ugly" before, with this many comments, when working out a thorny state interaction. But I usually clean it up a bit before submitting for review. Maybe instead I should just put a huge "do not attempt to simplify this code" banner at the top hehe :) But yes, it's not that bad, for sure.
al_borland 2y ago
In most code I look at, to try and get clarity on something I’m trying to do, I have no idea what’s going on. Variables seem arbitrary, everything seems cryptic, and I can’t be bothered to try and track down what’s happening. My own code doesn’t look like that at all. I have long descriptive variables, what I think are easy to read and follow functions, etc. Sometimes I think if I want to be “good” I need my stuff to look like what I find, but at the end of the day, I want it to be easy for me (and hopefully others) to maintain. If no one else can read my code, I don’t view it as job security, I view it has handcuffs.
zzo38computer 2y ago
I looked through some parts of it, and it look like to me that there is many redundant stuff in many if/else blocks.

mumblemumble 2y ago

I wish code like this still felt normal to me, but over the past ~10 years it seems that many people have come to value brevity over explicitness. I strongly prefer the explicitness, at least for important code like this. More than once in my career I've encountered situations where I couldn't figure out if the current behavior of a piece of code was intentional or accidental because it involved logic that did things like consolidating different conditions and omitting comments explaining their business context and meaning. That's a somewhat dangerous practice IMO because it creates code that's resistant to change. Or at least resistant to being changed by anyone who isn't its author, though for most practical purposes that's a distinction without a difference. Unnecessarily creating Chesterton's fences is anti-maintainability.

bhauer 2y ago
Agreed. Explicitness and comments are very useful in understanding the intended functionality and logic, whether or not the code actually implements that intent correctly (an in providing that intent, they can help identify bugs earlier than they would be identified otherwise).
- dskrvk 2y ago
  But comments go out of date, and the compiler doesn’t check them against the implementation. To document + enforce the intended functionality, use tests.
  - runevault 2y ago
    Note in Rust if you include comments with code that will run as tests but be inline in your main code instead of having to find the relevant test function to confirm functionality. https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html https://doc.rust-lang.org/rustdoc/write-documentation/docume...
    - rob74 2y ago
      Go has something similar: functions marked as examples that are both run as tests and shown and run as examples in the documentation (https://go.dev/blog/examples https://go.dev/blog/examples)
      - runevault 2y ago
        Interesting. I don't know when that was implemented in Rust but clearly Go has had it for a long time, since that post is dated 2015. While things like syntax are important, languages adding tooling like this (along with stuff like package managers) is so important to the continued evolution of the craft of software development.
  - dijksterhuis 2y ago
    tests -> verify intended functionality implementation (the how is right). comments -> why intended functionality was implemented that specific way (marketing wanted X because of Y, so we had to do it like Z with a bit of A). > But comments go out of date Just like updating the tests when code is changed, update the comment when the code is changed.
    - jgwil2 2y ago
spacemadness 2y ago
Every time some code reviewer comes into my PR and says something along the lines of "you know you can just write it this way" where "this way" means obfuscating the code because "clever" and "shorter," I die a little on the inside. This is from experienced devs who should know better. At one point I wrote a comment write above a section I knew would be targeted by this kind of thinking explaining it must be written this way for clarity. Sometimes that works.
- kstrauser 2y ago
  I only recommend that if the more explicit code is less idiomatic. For example, if someone appends to a list in a Python loop where a list comprehension would express the same thing plainly, I’ll suggest it. That’s it. Otherwise, please optimize for writing stuff I can understand at 2AM when things have broken.
- ungreased0675 2y ago
  I’m not a dev, but I manage them. On one team they were spending many hours on code golf and nothing was being built. I pushed the devs to passing testing=PR accepted. In your opinion, what problems might come from removing opinionated code reviews? Why do some reviewers gravitate toward “Here’s how I would have written it?”
  - consteval 2y ago
    I think the biggest problem that can come of it is lack of standards. Each dev has their own way of writing things, their own little language. To them it is perfect. And it all works, passes tests. But, if you let them do this then your code becomes sloppy. Styles go in and out. Like reading a book where every other paragraph is written by a different person, and none of them talked to each other. Sometimes you get PascalCase, sometimes camel_case, sometimes snakeCase. Sometimes booleans are real bools, sometimes they're "Y" "N" (yes, real) or sometimes they're ints. Sometimes functions take in a lot of arguments, sometimes they take in a struct with options. Sometimes missing data is nullable, other times it's an empty string, othertimes its "N/A". And on and on. You can enforce a lot of this through automatic linters, but not all. You require a set standard and the PR procedure can enforce the standard.
DSMan195276 2y ago
I agree to a point, but I would separate explicit code from excessive commenting. Explicit code is good because it lets you explain to the reader what you're actually trying to do. Excessive comments (or even comments in general) is less so because compiler cannot check them for correctness, if someone simply forgets to update a comment or writes it incorrectly then the only thing to potentially catch it is a code review.
- throwaway2037 2y ago
  Do you consider this example as "excessive commenting"?
  - DSMan195276 2y ago
    I haven't looked close enough at it to really know for sure. I'm not saying it's _always_ bad, comments are helpful, but the problem is that unlike code they are not required to actually match reality. In this case, I see several `if`s with no corresponding `else` even when the `if` section does not throw/return at the end, and that's largely my point. If the "space shuttle code" requirement is not actually rigorously followed, then why go on at length about it? And if it really is that important, then the comment about it is not good enough. Rather than a comment about it that can be ignored, they should set up a static analyzer to enforce it at build time. That way you're forced to follow the convention and not relying on code reviewers that probably don't even see that comment during their review.
claytongulick 2y ago
> I strongly prefer the explicitness I have a rule for my teams: "Don't write clever code". I try to constantly reinforce that we don't write code for ourselves, we write it for the next person. We should be doing everything in our power to decrease their cognitive load. I try to envision the person that comes after me (who may be me in months or years!) and imagine that they are having a Bad Day and they have to make changes to my code. Good code is clear, and tells a story. A story that's easy to follow, and easy to drill into. Not to knock elixir unfairly, but I think that's the basis of my mental block with that language. It seems to be designed from the ground up to violate that rule. Everything is elixir is clever. Very clever. Too clever for me.
- koudelka 2y ago
  Can you give some examples of how elixir is too clever? It’s been a breath of explicit fresh air for me…
  - claytongulick 2y ago
    I know you can do these sorts of funky exercises [1] with pretty much any language, but elixir breaks my brain for some reason. [1] https://evuez.net/posts/cursed-elixir.html https://evuez.net/posts/cursed-elixir.html
- imposterr 2y ago
  >I try to constantly reinforce that we don't write code for ourselves, we write it for the next person. Heck, it does't even need to be another person. Even me 10 months from now who may have forgotten some context around some code will appreciate boring code.
- raxxorraxor 2y ago
  That is a good rule. If you want to write clever code, do some code golf. For everything else I heavily prefer explicitness. Smart software architecture hides the verbosity if you do not have business with a specific and most likely specialized piece of code. Even with ugly code, you should think about refactoring if this particular piece did run successfully for several years and there are no security related issues. There are several languages the violate this principle. Brainfuck is probably one of the most prominent. It is of course not to be taken seriously. Overall alleged "elegance" of some code parts is rather annoying if you really need to understand and adapt it.
Buttons840 2y ago
The first developer I ever worked with was very explicit, and I learned some important lessons from him. He had created a system in PHP that controlled printers and it was very explicit. He didn't know what a function was, his code had no functions. It was a 5000 line script that would run on a Windows timer, top to bottom. In some places the control structures were nested 17 deep, I counted; an if-statement inside an if-statement inside a while-loop inside a for-loop inside an if-statement inside a for-loop inside an if-statement, etc, 17 deep. The cyclomatic complexity was in the thousands. I never could understand that code. I know it wasn't brief, does that mean it was explicit? I think the truth is brevity and explicitness are orthogonal. Let me ask you this: can code be both brief and explicit? What would that look like?
- mckn1ght 2y ago
  I’m not sure brevity and explicitness are totally orthogonal, explicit sort of implies spelling things out in a longer way. That doesn’t mean that something that is long is thorough, however. I like the tradeoff the Swiftlang project talks much about: brevity vs clarity (because explicit doesn’t necessarily mean clear, either, as your example shows). I think those are more orthogonal concerns, both important to think about, for different reasons that may often compete.
motherfsck 2y ago
I used to have opinions but I don't care anymore. After a spelunking into all manner of code bases I just match the style and move on. Verbose or terse, comments at the top or inline or nor none at all, 100 or 5 line functions, etc. I treat it like a card game; follow what's led.

bhauer 2y ago

You may be weird, but you're not alone. I too think this looks perfectly normal. I have written similar-looking code and comments (at least from a high-level point of view) for any components that I feel are critical to the reliability of the system. I've never subscribed to the "no comments" fad since my own comments have all too often been invaluable to future me when returning to the code after months or years of attention elsewhere. I can't imagine trying to piece together all of the embedded logic in a component of this complexity without solid comments.

remram 2y ago

I know. I would never remove an "empty" branch full if comments, that's the compiler's job.

zdragnar 2y ago

The one thing that stood out to me is the rather extensive nesting of if-else. I know this is go code, but there's multiple places that go five branches deep, at least. It works but it's a style that takes a lot of effort to grow what's happening IMHO.

omoikane 2y ago

Most likely, this comment was added in response to a botched attempt to simplify code, to serve as a warning for future maintainers to think twice before making a similar attempt. The commit that added the warning was "Add note about space-shuttle code style"[1], and the one before that was "Revert controller/volume: simplify sync logic in syncUnboundClaim"[2] [1] https://github.com/kubernetes/kubernetes/commit/de4d193d45f638b8e805a53945e55d6b4b745293 https://github.com/kubernetes/kubernetes/commit/de4d193d45f6... [2] https://github.com/kubernetes/kubernetes/commit/8a1baa4d64ca6c0622c17d6a421ce7b608698801 https://github.com/kubernetes/kubernetes/commit/8a1baa4d64ca...

croshan 2y ago
Nice find. I think you're right. FWIW, I don't think the code style in [2] is less simple (slightly more readable, to use `if (X) {} else {}` rather than `if (!X) {} else {}`, for example). So to me, this reads as the author of [1] is just overcorrecting by adding process, when some test cases or code review would've been more helpful in preventing whatever incident [2] caused.

hyperpape 2y ago

In particular, the every if has a matching else comment doesn't seem reliably true. Many of the unmatched ifs are just simple if (err != nil) { checks, but or other early returns, but outside of those, there do seem to be unmatched ifs. That said, my experience in enterprise software isn't that extra comments are necessarily present (there was a plague of "// end if" comments in the codebase, but actual descriptive comments were rare).

wavemode 2y ago
Yeah I should clarify that I was speaking personally about new code. I definitely encounter plenty of legacy code that is completely inscrutable. But, I can say in 2024 at least, that most teams I've been on value explaining complex logic (especially logic that can break in subtle ways if not properly maintained) with comments, in new code we write.

marcus_holmes 2y ago

I found myself thinking the same thing until I got to the hugely nested if statements. I would definitely have created some early-return branches to that thing. It does feel like the first "make it work" step (from "make it work, make it fast, make it pretty"), and then they just didn't do the "make it pretty" step. I have written code this "ugly" before, with this many comments, when working out a thorny state interaction. But I usually clean it up a bit before submitting for review. Maybe instead I should just put a huge "do not attempt to simplify this code" banner at the top hehe :) But yes, it's not that bad, for sure.

al_borland 2y ago

In most code I look at, to try and get clarity on something I’m trying to do, I have no idea what’s going on. Variables seem arbitrary, everything seems cryptic, and I can’t be bothered to try and track down what’s happening. My own code doesn’t look like that at all. I have long descriptive variables, what I think are easy to read and follow functions, etc. Sometimes I think if I want to be “good” I need my stuff to look like what I find, but at the end of the day, I want it to be easy for me (and hopefully others) to maintain. If no one else can read my code, I don’t view it as job security, I view it has handcuffs.

zzo38computer 2y ago

I looked through some parts of it, and it look like to me that there is many redundant stuff in many if/else blocks.