Live data from Hacker News

Please do not attempt to simplify this code

github.com

161–170 of 647 posts

Re: Please do not attempt to simplify this code

#161
I imagine it reduces errors and I admit it's very tempting.

But doesn't this style add a considerable cognitive load such that thinking several layers up becomes difficult? Perhaps there is simplified documentation of lower or mid level components so that thinking on the higher levels is not so difficult.

Re: Please do not attempt to simplify this code

#162
post #156

Earlier quoted context omitted.

> Kind of the whole problem is when there are weird corner cases going on that straddle function boundaries. If the problem has "hub and spokes" topology, i.e. it's relevant to multiple places in code that all reference a single location, put a comment describing the issue in that single location, and everywhere else put a comment with a reference. //Warning. See comment in [that location]. If there's no single best…

Centralized comment references sounds like a good+simple idea - I'll try to remember it, and hope I never have to of course ;-).

Yeah, references to a centralized document is such an obvious thing... once you read about it. It's another thing I recently picked up from Ousterhout's book, and looking back, I can now see the places in past codebases where I wish I thought of that myself.

Re: Please do not attempt to simplify this code

#163
post #82

Earlier quoted context omitted.

No, I do not find it indicates quality. To me, comments are noise, and code is signal; the code is what actually executes. It's one thing to have a summary of intent at the start of a listing, that should not count towards the code:comments ratio. Once the code begins however, there should be a minimum of comments necessary - especially in a high-level language not constrained to assembly-level instructions. In assem…

>To me, comments are noise, and code is signal; the code is what actually executes. Comments are noise to the compiler, but code is both a communication between humans and from humans to machines. To imply that only what executes is signal and all else noise is to ignore half the purpose of code, which is documentation. And despite what a lot of people want to believe, code itself is often not sufficiently self-docum…

More importantly, today's code is an optional basis of tomorrow's code. When NASA wrote the code for Skylab, I bet the comments in this codebase were the only useful thing in it and the code was all noise.

If you don't want to spend your career rewriting the same thing over and over again for slightly different business use cases and platforms, comments are incredibly valuable. (On the other hand, I guess there's a lot of job security in being hired to write the same thing many times....)

Re: Please do not attempt to simplify this code

#164
post #10

The comment:code ratio is higher than anything I write or that I’ve seen. However, it does give me some comfort. When it’s not gamed, do other HNers also feel that a high comment:code ratio probably indicates quality? There are reasons why this may be the case. (More thought, more time and a large team etc) I don’t advocate using this measure to reward anyone because it would be gamed immediately.

I actually would say it’s almost the opposite, if you’re writing clean, expressive code it shouldn’t need explaining. And if your code is clean, you shouldn’t have a bunch of redundant comments explaining the obvious.

As a counter example, here is a C file of 20,000 lines and no comments. I pushed this to Github long time ago, as it was the most gigantic "real" C file I have encountered.

https://github.com/miohtama/aliens-vs-predator/blob/master/s...

Comments are very barebone. There is structure, but needing to mess with this kind of code would be scary. Granted, most games are write once and never look back.

Re: Please do not attempt to simplify this code

#165
post #110

Ignoring the initial boilerplate (license, imports) and the request to preserve the verbose ("space shuttle") style, the first line is: // Design: // // [... 4 paragraphs of English prose // explaining goals and intent... ] That's exactly the type of comment that should be at the beginning of most files!

As a novice programmer, I was absolutely stunned that this was not standard practice. A typical source file provides zero context, background on the subject, pointers to reference material/blog posts/books explaining the concepts, information on how it fits into the program's 'bigger picture', or (most importantly) the thought process that resulted in the file (i.e., why the choice was made to do _this_ rather than _that_, challenges faced, trade-offs, etc.).

It still baffles me. Every programmer has to start from scratch dealing with a new codebase, and it makes improving any non-trivial program impossible unless one is willing to spend hours of archaeological examination. To open-source developers: if you want to get people contributing to a project (and make everyone's effort much more enjoyable!), these sorts of comments are essential. Not to mention they'll save everyone boatloads of time; it's a shame that every programmer has to piece together knowledge from scratch, rather than being 'tutored' by their peers' comments.

Re: Please do not attempt to simplify this code

#166

I'd like to see Uncle Bob do a point-by-point critique of this... "piece of work"

What is Uncle Bob's claim to fame? What has he written that is production-ready and sturdy and has held up to change?

I'm sure he can have opinions - he is very experienced at having convincing-sounding opinions on software development. But where is the test of whether he's right?

Re: Please do not attempt to simplify this code

#167

The comment:code ratio is higher than anything I write or that I’ve seen. However, it does give me some comfort. When it’s not gamed, do other HNers also feel that a high comment:code ratio probably indicates quality? There are reasons why this may be the case. (More thought, more time and a large team etc) I don’t advocate using this measure to reward anyone because it would be gamed immediately.

The logical conclusion to more comments is Literate Programming. This book for instance is also a program: http://www.pbr-book.org/3ed-2018/contents.html

This file isn't that heavily commented. Do you look at many OSS projects for comparison? Though when things get complicated with many branches and function reentries it makes me wonder whether the problem would have been better solved with declarative logic that handles the procedural mess for you. (It might also be much higher quality since you may unlock access to various formal methods and go beyond unit tests. Though perhaps for example there's a vetted TLA+ spec not shown that this controller is based on.)

I don't think doc'ing every function is unusual, usefully doing so is less common though. Comments in the function body also aren't that rare, though it might indicate a place for better factoring e.g. just more function calls on descriptive/suggestive names. (Having more functions will help in not having to stub out (and deeply stub) so much in a behavioral test, too, since you can get away with just mocking the function call instead of the potentially hairy state logic the function does underneath.)

I see an example at a random spot for a couple improvements in naming (in my ignorant opinion, I don't know about kubernetes) -- though the fact I feel able to express even a weak opinion on an improvement suggests the comments were reasonable. I've seen code less hairy but with no comments or useful tests and without a need to really understand it I just want to move along pretending I saw nothing.

Look at the set of ifs at L591. The first if is a null check with part of the explanation on L592, better to remove that part and have a function call, something like "claimWasDeleted(claim)". The matching else if on 615 checks for an empty string name, I'm not sure but I think its explanation is at L634 and the empty string check could be "isClaimPending(claim)", and maybe move the mode mismatch check to its own else if before the isClaimPending block and give it a better name. I appreciate the comment on L635 telling me why the next line of code on 641 is done (it may likely not be clear from the commit history, which can be another place for whys) though with the isClaimPending change the comment and code might be replaced with a fn call with the details in the fn doc. I'm also reminded of an idea in more expressive languages to annotate purely optimization metadata of any kind (inlining being the simplest) and being able to toggle it on/off for extra QA in a test suite. Anyway the next elif on 643 and its comment, could be something like "isVolumeBoundToClaimProperly(claim, volume)". You get the idea.

Re: Please do not attempt to simplify this code

#168
Two points...

1) One would have to make sure all the branches are accounted for during unit/integration tests.

2) The problem I am struggling with is the interviewers. So much of what I code I keep thinking about interviewers questioning it. "Why so many branches? Couldn't you have done this is in a more DRY manner? Couldn't you have done a better job at naming functions and variables so that so much commenting is not needed?".

This thing we do is truly an art form that can more easily be picked apart than properly understood. Ultimately I think we need to be like MMA fighters... able to properly pick the correct response for the situation with no ideological preference... only results matter.

Re: Please do not attempt to simplify this code

#169

Earlier quoted context omitted.

You've missed d) the codebase lives longer than a few months and someone else than the original author has to make changes. Comments describing the intent and caveats are extremely useful in ensuring the future developer gets adequate understanding quickly, and reduces the chance they'll introduce bugs. Tests can help understand the interface, but they don't help to understand the rationale behind it, the underlying…

Agree 100%, along with accompanying documentation that lays out architecture, rationale, challenges, etc. All of those are invaluable for any code that will outlive the tenure of the developers who built it. And given how often people in tech change jobs that's virtually all code.

Oh yes. And even disregarding tenure, you have cases like illness (see e.g. the Word 1.0 postmortem[0], page 14, talking about losing a key developer), or people changing project.

One time, I inherited a big steaming pile of spaghetti my co-worker wrote to meet a tight deadline, before being shifted to another project. That code implemented one of the key functions of the application, and half a year later, the customer demanded extensive changes. Believe me, I would have paid half my monthly salary the just to have a third of the comments that we see in this Kubernetes file.

--

[0] - http://antitrust.slated.org/www.iowaconsumercase.org/011607/...

Re: Please do not attempt to simplify this code

#170
post #147

Earlier quoted context omitted.

Is someone not familiar with the code competent enough to decide what is a "simple error check" and not a bug? This is very weak as they suggest that even the branches that would result in no-op are accounted for. So if someone introduce a code with a branch that is unaccounted for that automatically means the code is either faulty or is a "simple error check". With something supposedly trying to be a space shuttle w…

I think you can syntactically state that anything where the check is on the second return value (which is, by convention, the error return) is a "simple error check", and their rule for if statements is always for things that come from a first return value. For instance, this would not be a simple error check: server, err := find_current_server() if server != nil { ... } because if find_current_server() believes that…

What if the second returned variable is not err, but the code using it assumes it is? (the code breaks the convention) This will not be accounted for. This means with that in mind a lot more discipline must be used to analyse the code that is being used in that module.
Post reply on HN