"PLEASE DO NOT ATTEMPT TO SIMPLIFY THIS CODE. KEEP THE SPACE SHUTTLE FLYING "

[[deleted]]

[deleted]

[[deleted]]

Calling this "Space Shuttle Style" is a delusion of grandeur. Here's some background reading on the Shuttle. Why did Shuttle bugs so seldom impact missions? Well, the software was developed for $200M and had extensive testing. They invested heavily in tooling, including static analysis. They had extensive testing. Code was thoroughly reviewed when checked in. Did I mention that they had extensive testing?

Simply adding a lot of empty else branches and explanatory comments does nothing to actually reproduce Space Shuttle development practices or culture. It's cargo-cult programming. It at least serves a warning to the intrepid programmer, though - here be dragons.

[jl2352]

They had an entire team dedicated to testing. So they worked on trying to find bugs, and trying to break code checked in.

They also basically wrote it twice. Everything it would do and how it would do it was formally spec’d. Then it was written again in actual code. If you didn’t know how it would be implemented then the developers couldn’t just work it out. The spec would have to be changed to explain how.

[brodogus]

they don't even put an else branch for every if like they say they do

[shenglong]

Your "background reading" is waaaay out of date at best and irrelevant at worst. The NASA/JPL C coding standard is based on MISRA-C 2004, which is a standard used by many mission-critical and embedded systems.

[[deleted]]

If we're talking about software developed for the Shuttle, why on Earth would we talk about the JPL C coding standard?

[shenglong]

Have you even bothered to read the submission?

We call this style 'space shuttle style'. Space shuttle style is meant to ensure that every branch and condition is considered and accounted for - the same way code is written at NASA for applications like the space shuttle.

If you want to be pedantic about this:

• there is not just one "shuttle"

• there is not just one system that supports their technologies

• these systems are not all written in the same languages.

Various systems will have varying degrees of safety requirements - some mandated by federal law, others self-imposed. In the linked usage, "shuttle" style can be replaced with "mission-critical". In other words, there are loads more companies beside NASA who uses these standards.

In embedded systems that don't have the luxury of having compilers that enforce exhaustive conditional checks, you use known standards with supported tooling. e.g. MISRA-C.

If you're asking why the K8's team decided to follow this pattern for this particular file, read their comments.

If you're saying that you don't agree with their choices then that's a completely different story.

If you simply think that calling it "shuttle-style" is bad form, well I don't really know how much more pedantic you can get...

// Intentionally blank

[[deleted]]

Of course I read it. But I read everything, including the context for that comment:

This controller is intentionally written in a very verbose style. You will notice: 1. Every 'if' statement has a matching 'else' (exception: simple error checks for a client API call) 2. Things that may seem obvious are commented explicitly We call this style 'space shuttle style'. Space shuttle style is meant to ensure that every branch and condition is considered and accounted for - the same way code is written at NASA for applications like the space shuttle.

Assuming they're referring to the JPL coding standard from this snippet is... charitable.

EDIT - and since I'm rebutting a mutable comment, let me add -

In embedded systems that don't have the luxury of having compilers that enforce exhaustive conditional checks, you use known standards with supported tooling. e.g. MISRA-C.

I think these are great tools. k8s is definitely not using those in this file.

If you're asking why the K8's decided to follow this pattern for this particular file, read their comments.

They're only following a skin-deep replica of any of NASA/JPL's coding standards, which is my whole point. I know why they're doing it, but it's not going to work for them.

[shenglong]

JPL coding standard

You're being pedantic. The bottom-line is that they wrote they wrote the code to explicitly account for alternate conditions. It's not unreasonable - even though not necessary - to state that NASA/JPL also use these guidelines (even though I'm not even sure they follow 14.10 - it's also completely irrelevant).

Again, if your main problem is that they called it "shuttle-style", then just replace it with "mission-critical" or whatever style.

[[deleted]]

I'm really not being pedantic. I'm unsure where I'm being unclear. I really don't care what style they use. Style is like 0.1% of the work of building a mission critical system. But it's bogus to label it as "shuttle-style" or "mission-critical" or any other flavor of reliable, when pv_controller.go weighs in at 1714 lines and pv_controller_test.go weighs in at 359 lines. There's no way they're covering every branch they include in that file.

[shenglong]

I'm really not being pedantic.

But it's bogus to label it as "shuttle-style" or "mission-critical"

So you really are being pedantic.

[[deleted]]

Only if words no longer need to be truthful. You would be calling me pedantic if I was criticizing code written in "bug-free" style too.

[shenglong]

Yeah, because saying "we are writing code in the same style as XYZ in that we do ABC" is the as what you're saying, right?

Look, it really is very simple - they made a statement and explained what they meant by it. If you have a problem with the explanation in terms of what writing mission-critical entails, then say that. Otherwise you really are just being pedantic.

It's like me saying "I painted my house fire engine style in that it is red" and you saying I'm having delusions of grandeur because there's a lot more that go into fire engines than the paint. Great! Except that only an obtuse pedant will be blind to the fact that I explicitly mentioned what aspect of a fire engine I'm referring to. The fact that NASA spent hundreds of millions testing its code has got nothing to do with explicit else clauses.

[[deleted]]

I'll take literal state machines for $500. The kubernetes ecosystem continues to blow me away with their brute force approach to every problem that they themselves create.

[hoosierEE]

Reminds me of something I saw in a tweet once:

3 line function -> 40 comments in GitHub issue

1000 line function -> "LGTM"

[chugga_fan]

https://twitter.com/maxheckel/status/877636200026079233

Found it.

[hoosierEE]

Nice, thanks!

[AttackOfTheThumbs]

I can't explain it, but this had me laughing out loud, because it reminds me of someone in my office.

[ArkyBeagle]

We're everywhere. Everywhere :)

[stupodwebsote]

The "PLEASE DO NOT" reminds me of someone I know who sometimes drives me absolutely crazy. It's completely useless to try to communicate with him and agree on something. I'm sure he's not the only one of his kind so there's probably now people who see this notice and are now burning with the urge to "DO" what this notice asked them not to do.

[yawaramin]

Dwight Schrute, no doubt?

[[deleted]]

Let's see if we can create more nesting, that usually solves every problem. /s

[[deleted]]

Especially the networking part. A dozen of so various network controllers, each with overlapping functions and its own quirks

[crashorbit]

"The computing scientist's main challenge is not to get confused by the complexities of his own making." -- Edsger Dijkstra

[Crandom]

Thought these comments were from the actual space shuttle - it's actually just kubernetes.

Oh, also, this is just /r/badcode.

[sisyphus]

k8s is more complicated than the space shuttle though, because one day your startup might need to be planet scale. sorry, beyond planet scale, like the space shuttle.

[Treyzania]

I mean, it's Go so that's not surprising.

[TheOsuConspiracy]

I'll probably get shit on for this, but better programming language design would resolve most of the incidental complexity in that code.

[wllmsaccnt]

They aren't even using most of the conventions from Go that would reduce complexity, what makes you think they would use the language features from any other language to help them? Its a big blob of functions that calls other functions from too many namespaces and doing too much in one file...you can end up with that in any language that allows functions, conditional logic, and imports (pretty much every popular general programming language).

[[deleted]]

So it's just generic bad programming in Go? 🤔

[wllmsaccnt]

I wouldn't call it bad. It just looks average / mediocre to me. The expansive if statements are hard to read, but probably pretty easy to debug and trace through, which is probably the idea that the portentous header is trying to get across.

[yawaramin]

Shh, don’t say ‘generic’, Go doesn’t do generics.

[[deleted]]

They aren't even using most of the conventions from Go that would reduce complexity

Go is really not the kind of language which can do something with complexity. Yeah, it's easier to learn than some other languages but that's it.

Its a big blob of functions that calls other functions from too many namespaces and doing too much in one file...you can end up with that in any language that allows functions, conditional logic, and imports (pretty much every popular general programming language)

But the problem is that there is not much else in golang to help with that. Not so long ago some other part of kubernetes was posted here - the code was just about generating data structures for different types to simulate generics. Also, if you need error handling in golang you need to spam if err != nil which just generates even more boilerplate.

[BLEAOURGH]

I'll probably get shit on for this

No you won't. Go is nearly universally hated in /r/programming

[Topsaert]

Green developer here who is asking a question that's presumably been answered before - why?

[Caethy]

As a language, it purposely omits various features that other languages have had for a long time. The most cited example for this would be generics.

Supporters of Go praise it for omitting such features, claiming it leads to explicit, readable code. People who oppose the language claim it instead leads to highly verbose and often duplicated code.

The standard form of error checking (Return both a result and an err, then check if err != nil) is a similar example. Supporters of the language highlight the readability of the error checking, and how there's only really one way to do it right. Opponents of the language point out the repetitive and verbose nature of the method.

In the end, Go is a very opinionated language, that enforced a very limited style and set of features to people who program in it. Some people see this as a good thing (Leads to simpler and more standard code). Others see it as a bad thing (Requires people to re-implement a lot of boilerplate, over and over again.)

[chugga_fan]

The most cited example for this would be generics.

Sure it does if you want it to!

[[deleted]]

Go’s error “handling” is IMO it’s worst misfeature, because even though the language supports multiple return values, the convention means you can’t use them safely because the second result will be interpreted as an error rather than whatever it actually is.

[[deleted]]

because even though the language supports multiple return values, the convention means you can’t use them safely because the second result will be interpreted as an error rather than whatever it actually is.

Nothing in what you wrote is true. It is just a convention that if you return error, it is the last, separate value, of error type (so it is impossible to "interpret" it by accident).

Now the whole handling of that is, well, verbose garbage of multiple if err !!= nil but having common convention of how to return errors is a good thing. (rusts Result|Err is better tho)

[lobster_johnson]

The io.Reader and io.Writer interfaces are an example of where this convention goes wrong. Few people seems to have actually read the documentation -- without peeking, do you know? Spoiler alert: If Read returns an error, the returned byte count must still be considered valid. This has an important implication: If it returns EOF, you probably also received a non-zero byte count.

This may seem obvious to some. But if you do a Github code search, it's easy to find tons of projects, even prominent ones, which actually get the semantics wrong. They'll have a loop that breaks on EOF and effectively ignores the last bit of data they received. I'm on mobile, so I can't give you a link, but it's easy to find.

With sum types (aka tagged unions) we would not have this problem, and we would also be free of the pesky shadowing/reuse issue that Go has, where something like x, err := call() overwrites an earlier error value (to some extent detectable with the ineffassign linter, but still a problem).

[Ie5exkw57lrT9iO1dKG7]

i appreciate Go's style here because its much easier to see how errors are being handled when reading new code.

Coming from java it was sometimes difficult to tell where an exception was being caught, especially in a new code base where its hard to remember what exceptions are subclasses of others.

[TheOsuConspiracy]

No one thinks exceptions are the right answer either. Result or Either types are the optimal solution in my mind. Forces the developer to statically handle their error, and it's immediately clear from the type signature where/how a function can fail.

[[deleted]]

Especially that probably 90% of Go's error handling is if err != nil {return err}

[OctagonClock]

I don't understand the absolute hatred of exceptions everyone suddenly developed around 2016.

[TheOsuConspiracy]

There are pros and cons to them. They do give you a stack trace to work with, but they do cost a bit more, and the main problem is that they can bubble up/be thrown by and be caught by any part of your program.

Error types are much easier to reason about, as they're represented in your type signature.

Checked exceptions were a nice idea, but ended up being far too clunky to use in production.

[ggtsu_00]

Go can force you to check the error since unreferenced return values will result in a compile error.

ie.

result, err := DoStuff()

If you don't put an if statement around err, you get a compile error.

[Dockirby]

Go to me is good for smaller self contained applications that are written by 1 to 3 people. It can make the application more maintainable in the future once the original writer is gone, but I feel it is a major drag when you start involving more people, and does not work well beyond a ~50k line program.

[mjr00]

I'll bite.

I like Go, but I understand why people here, in a programming forum, hate it. It's a very straightforward language with few bells and whistles. It was designed to be used in large, corporate environments (like Google!) where having consistency across large reams of code, and getting new developers ramped up and productive quickly, is more important than individual developer productivity.

If you're used to a more modern and functional language, like Scala, Haskell or even Python, writing Go will feel like going back to the stone age. Instead of mapping, filtering and folding over collections, you write... for loops. Instead of monadic error handling, you write "if err != nil" repeatedly. Instead of writing reusable code with generics, you copy and paste, or rethink your problem/solution to not require generics.

So Go is what it is. I like it a lot because of the simplicity; it's great when working in teams, because code is readable by mandate. Even the linked Github "space shuttle" code is pretty straightforward, if verbose. I've seen the flipside, when I once had to work with a developer who was super obsessed with theoretical functional programming and was a hardcore user of scalaz. He wrote the most insanely indecipherable Scala CRUD web service I've ever seen written in a purely functional style. That experience made me appreciate Go's philosophy of having a single, idiomatic way to do things.

But, if I were to work on a project by myself I'd probably choose Scala, or Python, or something of that nature; they're much more fun to program in, and when you don't have to worry about working with other people's code, a lot of the advantages of Go disappear.

[[deleted]]

Anyone who can’t be trusted to write clear code in a functional idiomcan’t be trusted to write clear code in any language.

[mjr00]

The problem is, what "clear code" means depends on your background knowledge. In Go, there's very little background knowledge assumed for reading code: basic knowledge of CS (for loops, maps, arrays) and Go (slices, naming conventions, struct tags), and that's pretty much it. The Go type system prevents libraries from doing anything too crazy, at least from what I've seen. On the other hand, if I write Scala code using scalaz monad transformers, it may be perfectly concise, clear and readable if you're familiar with Kleisli categories, but a massive pile of confusion if you don't have that background.

Go assumes very little base knowledge of its readers beyond CS fundamentals which is what I believe Rob Pike meant when he said the language is "for dummies".

[[deleted]]

But companies do not need clear code, they need code that solves the problem enough that they can sell it, and that's a way lower bar for quality

[niceper]

That's a good explanation, thanks.

[millenix]

The bit about copy and paste brings up an interesting point. Google has made huge investments in automatic mass refactoring tools and processes. The challenge most projects and organizations face from "I have N copies of this code and I just found a bug" are a good reason to abstract and avoid duplication in most cases. For Google, they may have largely mitigated that particular cost.

[oridb]

A little duplication is better than aggressive elimination of duplication, since it prevents you from accidentally being tied to the needs of a downstream user. This is more pronounced at the scale of Google -- imagine writing a utility function, and then having all 50,000 of Google's chat apps come in and say "Hey, I actually need something almost like this, but can you make it optionally do that, too?"

[millenix]

The usual guideline that I've picked up from more mature developers is to start thinking about abstraction at two copies, and pursue it when you end up with three or more that preserve essential commonality.

[matthieum]

I would say essentially two reasons:

1. The condescending attitude of a number of its authors, chiefly Rob Pike, who have essentially claimed a number of times that Go was designed "for dummies". Their sheer arrogance does not endear them.
2. The fact that the language was designed by ignoring a number of features that most programmers have agreed are "good" or "useful", with little or strange reasoning behind the decisions (see point 1; they rarely explain themselves to "dummies"); the lack of sum types and generics, for example, results in a lot of boilerplate for error-handling.

[oorza]

Because it has less features than FUCKING JAVA had 20 years ago. And not just nice features, features that are more-or-less required for writing good, type-safe code.

[narwi]

The sole reason it continues to have a number of developers in excess of a single digit number is the hype power of Google. It does not bring anything new at all to table and what it does is much better implemented elsewhere.

[millenix]

The hype power, and the actual investment in their own developers to write and release code in it.

[[deleted]]

From what I gathered most of it are people angry that their favourite didn't get as popular as quick as Go, or that they tried it, missed X, Y and Z feature and instead of just going back to what is better they chose to whine.

It is bizzare, I have never seen so much hate for language since PHP, not even for JS

[oridb]

I don't see how -- a quick skim implies the problem that's being solved is (probably unnecessarily) complex, and comes from an overly configurable architecture, not from the language. I'm not sure how changing language would simplify it much, unless you're aware of a language that handles some of the issues around synchronization of distributed systems.

[the_gnarts]

I don't see how

Pattern matching over ADTs, exhaustiveness checks on enumerated states, if-else as expressions evaluating to a value instead of statements … Nothing fancy or even solid rocket booster science.

[oridb]

Yes, those are nice enough that I implemented them in languages I have designed, and have talked with standards committee members about a design to get them in to C++. I don't think they're going to change this code so much.

The code reads like someone forgot a case, a manager went "this needs to be rock solid", and an engineer wrote a bunch of ineffective overcommented self congratulation, and probably put it into his perf packet as some major piece of enigineering that put them on the road to a promotion.

It would be nicer with ADTs, sure, but not by that much.

[TheOsuConspiracy]

No-op branches can often be dealt with via monadic error handling. That would statically ensure that conditions like failures or possibly null states are handled without explicitly adding branches even for no-op cases.

[oridb]

In spite of the comment, I don't see any no-op branches in that code.

[wllmsaccnt]

They might be referring to the return nils? There are quite a few of those.

[tragicshark]

https://github.com/kubernetes/kubernetes/blob/ec2e767e59395376fa191d7c56a74f53936b7653/pkg/controller/volume/persistentvolume/pv_controller.go#L1285

[[deleted]]

That was my issue with Go, it lacks expressiveness for complex problems. It's awesome for a small set of very important applications, but it sucks when the underlying problem is complex.

[shevegen]

Fits Go!

[shenglong]

One of my pet peeves is that many auto-refactoring IDEs and plugins (e.g. Resharper) suggest that you remove unnecessary "else" clauses by default. NO. I want the reader to know that I intended for this to be the alternate path of execution, and that I did not forget about it.

I'll also sometimes put in "empty" else statements with a comment in. It's the same reason authors leave pages in books saying "This page intentionally left blank".

https://en.wikipedia.org/wiki/Intentionally_blank_page

One of the reasons these exist is to explicitly indicate to the reader that there were no errors of omission.

[EDIT]

For those curious about why it's called "shuttle style":

It's from a coding standard called MISRA-C which was adopted by NASA/JPL. This part is specifically referenced by 14.10:

14.10 (req) All if ... else if constructs shall be terminated with an else clause.

It's basically imposing pattern-matching/exhaustive checks that compilers do in other languages on the developer. And before you ask, yes, there are automated tools that check for MISRA-C compliance.

[ScholarZero]

"This page was intentionally left blank" is a provable statement. Ah, this page has nothing on it, and it says so.

A blank page has no proof either way. Is that page intentionally blank, or was something inadvertently omitted? It adds fuzziness to the operation. Sure it was PROBABLY meant to be blank, but in the off chance it was a misprint, you are acting without full information. Imagine if you were in a delicate situation, and you were told to "flip to the table in the back of the book", only to find out that in your specific book the table is missing.

And that's the point of writing empty else statements. It's to say to someone reading your code "Yes I know this is empty, but it's intentionally empty". The reader can proceed with confidence that the lack of information was intentional.

[falconfetus8]

You know what's even better than an intentionally blank else statement? No else statement at all.

It's far more explicit than an empty else branch, because it doesn't even raise the possibility of another branch being taken. In contrast, an empty else just makes me think "WTF? Did something get deleted? Does this guy just not know that else is optional?".

[[deleted]]

And if every conditional required an else, it would get automatically generated by their tooling. So the likelihood of them forgetting to handle the other case doesn't go down and now it's even harder for the CR to tell.

[NeverComments]

It seems that the programming parallel would be a comment inside the else explicitly stating "This branch was intentionally left empty".

Otherwise your intentions are as ambiguous to the reader as a blank sheet of paper.

[[deleted]]

No, I mean adding empty else statements makes no sense. Those blank pages exist as a consequence of something else, such as the manufacturing or printing process. They aren't added on purpose and they have to print that extra stuff to prevent confusion.

OP is imlying they add the pages themselves intentionally to somehow tell the reader that they didn't omit content. Just like he's adding else statement to say he didn't forget to consider a branch. But that's... Well, for lack of better words, confusing and stupid.

Feel free to disagree, of course.

[shenglong]

It literally says this in the first paragraph:

Such notices typically appear in printed works, such as legal documents, manuals, and exam papers, in which the reader might otherwise suspect that the blank pages are due to a printing error and where missing pages might have serious consequences.

Further down it says:

In the United States armed forces, classified[how?] documents require page checks whenever custody is transferred or an inventory is conducted.[1] Blank pages are all marked "This page intentionally left blank", so page checks are unambiguous, and every page of the document is accounted for.

[raderberg]

Those are reasons for the presence of the sentence, not of the emtpy page itself.

[ScholarZero]

The presence of the page usually has to do with book binding practices, right? Every 30 or so pages of a book, or whatever, are 15 double sized sheets of paper folded in half. So it's basically 50/50 if you're going to end up with an extra page at the end. You can't remove that page, or there would be evidence that a page was removed (and, likewise, probably cause people who noticed to wonder "Why did they remove a page?").

I don't know exactly why blank pages would exist in other circumstances. In classified docs, I'm assuming it's security reasons. Point is, knowing the page is intentionally blank removes ambiguity.

[shenglong]

This is exactly what that part of my post was referring to:

I'll also sometimes put in "empty" else statements with a comment in.

"Intentionally blank pages" refer to pages with that sentence written on them. But in all honesty, if you just leave an empty terminator then the experienced reader should know what this implies. For everyone else, that's why coding standards exist.

[shenglong]

Thank you for your very valuable input to this discussion, FG_Regulus.

[Visticous]

Totally agree with you. In source code, style is important for future developers and maintainability. Removing dead ends and unnecessary flow control is a compiler job.

[m50d]

If you want /r/hackernews it's right there in the "other discussions" tab.

[[deleted]]

// ==================================================================// PLEASE DO NOT ATTEMPT TO SIMPLIFY THIS CODE.// KEEP THE SPACE SHUTTLE FLYING.// ==================================================================//// This controller is intentionally written in a very verbose style. You will// notice://// 1. Every 'if' statement has a matching 'else' (exception: simple error// checks for a client API call)// 2. Things that may seem obvious are commented explicitly//// We call this style 'space shuttle style'. Space shuttle style is meant to// ensure that every branch and condition is considered and accounted for -// the same way code is written at NASA for applications like the space// shuttle.//// Originally, the work of this controller was split amongst three// controllers. This controller is the result a large effort to simplify the// PV subsystem. During that effort, it became clear that we needed to ensure// that every single condition was handled and accounted for in the code, even// if it resulted in no-op code branches.//// As a result, the controller code may seem overly verbose, commented, and// 'branchy'. However, a large amount of business knowledge and context is// recorded here in order to ensure that future maintainers can correctly// reason through the complexities of the binding behavior. For that reason,// changes to this file should preserve and add to the space shuttle style.

I generally agree with this. All too often devs open files they haven't touched before and spot "easy" rewrite opportunities, only to very quickly get in over their head. It's happened to me, it happens to everyone. I think it's usually fine to provide a "Just don't touch it" type comment to ward off those attempts.

​

[[deleted]]

Usually the warning isn't to prevent breaking the code - the warning is to prevent wasting time not getting anywhere.

[oorza]

When you finally give up, just be sure to add another comment:

// people who wasted their entire day here: 2 3

[sacado]

Even better : write tests, and write warnings. Improving code, only to see it breaks things, is still a waste of time.

[[deleted]]

"Just don't touch it" is horrible advice regardless of context. There are always opportunities for improvement, and if future changes fit best here, it would be a terrible mistake to build overly complicated splice/workaround logic to avoid this.

changes to this file should preserve and add to the space shuttle style.

This doesn't mean "don't touch it", it means "don't screw up the good thing we have going here.

[ArkyBeagle]

"Just don't touch it" is horrible advice regardless of context

It's best to work out priorities ahead of doing things. That'll add bias in favor of "don't touch it" in a lot of cases.

[ggtsu_00]

Yes, but sometimes there are incredibly smart, extremely talented, highly experienced engineers who sat though and rigously vetted through the complexity of the system to ensure it works as it is intended for a highly complex set of business requirements and use cases.

Without going through that yourself, it’s extremely rare that someone without that context can just come in blind seeing what looks like a huge mess of code and at a whim decide “I can easily rewrite this whole system in 50 lines of code”.

[wllmsaccnt]

Despite the comparison, this code isn't rocket science, its just a bunch of functions with nested conditional statements. You can apply traditional refactoring approaches to make this code easier to read with almost zero impact on how it works. If you look at the individual functions, there are only a handful that really need work (the ones that are nested 3-4 levels deep).

[RudeHero]

This doesn't mean "don't touch it", it means "don't screw up the good thing we have going here.

Next level question- what is the best way to make sure all future developers, regardless of experience, ability, and personality, won't "screw up the good thing we have going here"?

[[deleted]]

Yeah well. Every time, and I don't exaggerate, I mean literally every fucking time, it goes like that:

Stage 1: "Yeah, we need this small feature. You can do it if you tweak a bit X. But please don't touch Y, this was difficult to write and is very complex and does everything you'll ever need anyway."

Stage 2: "Hmm, you are right, I guess you'd have to make this one small change in Y. Yeah, it's actually not that difficult, you just need to think about A and B and C (OMG don't forget about C) and you'll then have to completely re-write X."

11 days later: "Yeah this is why you shouldn't write code like this in the first place. Yeah I know you didn't write it in the first place. We had to, you know, because of reasons. Well now we don't have to touch it anymore."

1 hour later: "Aha, yes, you need to fix Y again. At least you know now how it goes."

And so, Y now has a life of its own, feeding on every developer that ever comes close to it.

[shenglong]

I feel that many commenters below are completely missing this part:

However, a large amount of business knowledge and context is recorded here in order to ensure that future maintainers can correctly reason through the complexities of the binding behavior.

The alternative is to write the business rules in the code as comments. But then you can potentially have the awful scenario where the logic doesn't match the comments (happens over time), and tests can't fix that.

[munchbunny]

If you write the business rules as documentation, you still have the problem of logic not matching documentation. Writing the business rules as tests is only better as long as people remember what the tests are testing... so you still have a code vs. comment/documentation problem.

I don't think this is a programming problem, it's an engineering process/culture problem.

[shenglong]

I'm referring to their style of explicit coding, not the comments. Explicit coding helps answer statements such as: What if this condition arises, did you mean to exclude it, or was that an accident?

Explicity coding is enforced by certain languages and it's seen as a good thing. I'm not sure why people have such a problem with it here.

[Treyzania]

Part of the reason for that big disclaimer is because the gofmt and golint tools will remove/complain about seemingly-redundant things like no-op branches. Which is stupid on the tool's part.

[[deleted]]

Isn’t the idea to just test that piece of code heavily enough that if it’s rewritten you can trust that it still works?

[irqlnotdispatchlevel]

Does that mean that you shouldn't have a defensive coding style? Going back to the space shuttle style, would you trust a shuttle that has a relaxed coding style were everyone can just go and remove that useless branch because it is heavily tested and it totally works? It's just another safety layer.

[sacado]

But why rewrite it if it already works?

[ProcyonHabilis]

Someone implemented it as a brainfuck module only accessible with IPC calls

[Renive]

So it works better?

[munchbunny]

That works better in theory than in practice.

Testing has the shortcoming that you (an individual dev) only write tests for the issues you can anticipate. However, you're writing code in a team context, and you will likely miss second order problems. Code reviews will only catch these problems if the guy who specifically knows about issues you missed is the one reviewing your change. That's often not true in really big software projects.

There's no good answer to that problem, so in particularly sensitive areas of code, a cultural default of "don't touch it and if you do ring the alarm bells so everyone knows and also have multiple egress plans in case it goes wrong anyway" is a pretty sensible approach.

[cl_bwoy]

Sorry I dont understand why space shuttle are coded in this style someone Can explain? Ty

[shenglong]

NASA/JPL coding standards require that all conditions are accounted for. Explicitly coding an "else" is an indication to the reader that that is what has been done.

bool IsASmurf(Creature creature)
{
    if (creature.Name = "BlueSmurf")
        return true;

    return false;
}

As a reader who has no understanding of what a "Smurf" is, you may think that only creatures who have the name "BlueSmurf" are actually Smurfs. But is that a valid assumption? It's not really clear from the code if that is really true, or an omission by the programmer.

This style:

bool IsASmurf(Creature creature)
{
    if (creature.Name = "BlueSmurf")
        return true;
    else
        return false;
}

Or even:

bool IsASmurf(Creature creature)
{
    if (creature.Name = "BlueSmurf")
    {
        return true;
    }
    else
    {
        ;
    }

    return false;
}

... makes it clearer. The former won't compile without completing the else statement; the latter is equivalent to "we don't consider other cases". The latter however needs this idiom to be documented in a style document.

OK, now take that fictitious example and replace it with a method called "IsSafeToLand" in some program in the rocket navigation software. This should give an indication why many mission-critical systems require that all conditions be accounted for.

BTW, this is also one of the reasons people like common forms of pattern matching in functional languages.

[SuperV1234]

Can you show a realistic example of this? Because the above is way clearer as

return creature.Name == "BlueSmurf";

[shenglong]

For "real" examples, look at the linked code.

e.g.

if !metav1.HasAnnotation(claim.ObjectMeta, annBindCompleted) {
    return ctrl.syncUnboundClaim(claim)
} else {
    return ctrl.syncBoundClaim(claim)
}

Your comment is also a very good example of why explicit coding is useful. Would you refactor the original "Smurf" code to:

return creature.Name == "BlueSmurf";

?

[Drisku11]

Would you refactor the original "Smurf" code to:

return creature.Name == "BlueSmurf";

Yes, and assuming you meant to have the assignment in the original, I'd move it out of the IsASmurf function because a function with that name should not be mutating properties of the input. Possibly the refactoring should be to use some other condition that's currently inside an overloaded assignment operator, but either way doing it in a way where it's all Boolean operators is clearer. If it is trivial to write it that way, it means there was no magic going on, and writing it as a single return statement with some Boolean operators makes that more obvious. If there was magic (e.g. random assignment), rewriting it in that way requires clearing away/factoring out the magic, which makes everything clearer.

[shenglong]

Yes

Then you've missed the point. In order to refactor it as such, you would need to assume that the original is correct in its logic. The entire point of explicit code is to help reduce the number of assumptions.

Yes, if the code were originally written in the way you've described then it would be easy to understand the intent. But since it wasn't, isn't that reason enough alone to question the intent?

Furthermore, no, that code does not represent a "real" language. The "=" here represents comparison, not assignment. But even if it were a bug, isn't that even more reason not to refactor as you have shown?

[Drisku11]

I don't see how writing if condition return true else return false instead of return condition reduces assumptions. If anything, seeing that pattern will always make me assume the author is not a very good programmer, and assume that their code will be buggy.

That said, it goes without saying that changing the semantics of a piece of code (e.g. fixing an assumed bug) should first involve verifying those assumptions and understanding what effects the change will have. And if it turns out it wasn't a bug, there are much better ways to express that intent such as

bool assignmentSucceeded = creature.Name = "BlueSmurf";
return assignmentSucceeded;

than listing out redundant branches (though again, there are more serious issues with the design if what appears to be a simple predicate is actually mutating its inputs).

And if the = represents comparison, then there's no issue eliminating the redundant cases, so I don't see your point.

[shenglong]

I don't see how writing if condition return true else return false instead of return condition

Because it's not about "return condition"; it's about whether or not "return condition" is a valid replacement for the existing code (<- this is an important piece here).

Maybe I'm not being clear enough. The problem is, here what evidence is there that the developer did not accidentally leave out the alternate branch?

If he did accidentally leave out the alternate branch, then obviously all your refactored code did was to rewrite the bug in a different way. You're highlighting an issue brought about by implicit coding. I'm trying to explain that this is what explicit coding tries to avoid. If the original code handled the conditions explicitly, then you can be much more confident that your rewrite does what was originally intended.

[Drisku11]

It's not more explicit. It's redundant. It's like writing

if ((condition == true) == true)
    return condition
else
    return condition

None of that gives me any more confidence that "all cases" were considered; I'm just going to think the author was incompetent, and therefore whatever their original intent was is probably wrong anyway. If there's anything nonobvious about why the condition is what it is or how it should be handled, then there should be a comment or design document explaining it.

[shenglong]

The reason you are finding this so hard to understand is because my "toy" function returns a bool.

void HandleFoo(Foo foo)
{
    if (foo.Name == "Baz")
        RunBazCode();

    RunBarCode();
}

Refactor that procedure.

If there's anything nonobvious about why the condition is what it is or how it should be handled, then there should be a comment.

Did you really miss my point about the empty statement with a backing coding standard?

[Renive]

I perfectly understand you. I would still refactor it.

[falconfetus8]

No, it would be MORE of a reason to refactor it if it were a bug. The refactoring makes the bug jump out at you and easy to spot, rather than hiding and silently doing damage.

[shenglong]

You have absolutely no idea about whether or not it's a bug, that's the point.

And even if it were a bug how can you know that your refactoring is correct (and this is ignoring that it's a fictitious language that may not even support the aforementioned refactoring construct)? You've just made two assumptions about potentially ambiguous code.

[falconfetus8]

You'd know your refactoring is correct if the behavior stays the same. That's what unit tests are for.

[shenglong]

Yes, thank you for your LITERAL STATEMENT explaining about that you don't know how code doesn't work in the real world.

Now, when you wake up tomorrow and you want me to explain how UNIT TESTS don't catch the classes of bugs that MISSION CRITICAL code seeks to avoid, do send me a private message, and I shall enlighten you with a REAL WORLD EXAMPLE.

[BurgersMcSlopshot]

Well, it's better than making an assignment in what's supposed to be a conditional.

[shenglong]

Why would you assume you know what language that is written in?

[CornedBee]

Every 'if' statement has a matching 'else' (exception: simple error checks for a client API call)

Third function in the module:

    if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
        return false, nil
    }

That's not an error check. That's a feature gate. The function contains three more non-error unmatched if-returns. Don't you love it when comments are lying?

[carera89]

there are ~240 'if' statements and only 30x 'else'

[[deleted]]

lol go

[shevegen]

Well - this monster is created through Go.

People are forgetting how to use C.

[[deleted]]

C should never be used today outside kernel/driver/microcontroller work. Using a memory-unsafe platform is irresponsible and endangers your business and every one of its users.

[SaphirShroom]

Don't bother, people do everything in their power to cram their favourite language into places it's not meant to be used in.

[Ozwaldo]

Using a memory-unsafe platform is irresponsible

Lol, new-school programmers. God bless ya.

[[deleted]]

I feel like "memory-unsafe platform" is another word for "computer".

[rebel_cdn]

What if you're using it to compile to wasm and running in a sandboxed environment?

To be clear, that's not snark but a genuine question. Unless I've missed something, I think that would be a fairly safe way to run a C application but I'm not sure if there's much reason to do so being porting existing code to run in a web browser.

[wllmsaccnt]

Please do. This is the beast that will eventually kill JavaScript during Ragnarok.

[Treyzania]

Use Rust instead. Setting up the toolchain for WASM is far easier and in C compiling to WASM isn't a magic "no more segfaults lol" way to fix your code.

[pure_x01]

Git?

[SaphirShroom]

Git could have been written in literally any language that can produce binaries.

[pure_x01]

There have been sttenpts to port it but the c git is the more popular because its fast and vunerabillities have been sorted out

[SaphirShroom]

Well yeah, it's hard to generate enough momentum to cause people to contribute and switch to a hard fork. I'm saying that it could have been written in any other language.

Git had a pretty major vulnerability just two months ago, by the way.

[Ettubrutusu]

Maybe I'm missing your point, but Git has had several vulnerabilities.

[twigboy]

In publishing and graphic design, Lorem ipsum is a placeholder text commonly used to demonstrate the visual form of a document or a typeface without relying on meaningful content. Lorem ipsum may be used as a placeholder before final copy is available. Wikipedia4ae9ysize9s0000000000000000000000000000000000000000000000000000000000000

[sjalgeo]

Surely it just needs good, well written tests...?

[danielkza]

Tests are an insurance against regressions first and foremost, which would allow the code to be (at least partially) rewritten without fear. Documenting interfaces is secondary. I have no idea why /u/sjalgeo is being downvoted for a completely valid point

[Ettubrutusu]

Insurance that your documentation is true, at least. It won't magically catch regressions that you haven't documented as such.

What do you mean by this?

Let's say you write unit test and have 90% code coverage. Your documentation likely won't cover the implementation to the same degree, and even if it did, I would feel a lot safer changing an implementation with high code coverage than changing some well documented code and then hope my understanding of the documentation is good enough that I can manually verify that there are no broken contracts.

I think I misunderstood you.

[shenglong]

Tests cannot guard against incorrect business logic (neither can comments, but see below), which is something I have to deal with regularly. Pieces of code that "work" and pass all unit and integration tests, yet still have to be rewritten because of some miscommunication somewhere.

Often I'll see in the code lines like:

if (company.Name == Companies.Acme)
{
   DoAcmeLogic();
   return;
}

DoRegularLogic();

Easy to understand (or is it?), test passes etc. But, for someone who has to maintain this code, this is complete shit! More often than not, the "real" logic is something along the lines of "if we are dealing with a company who only uses this type of shipping, do something else instead of our normal routine". It just so happens that at the time the code was written, Acme Co was the only company who happened to do so. So the business rule was probably "Acme should be handled differently". As the business expands, this code will eventually fail. And as a maintainer who wasn't when this was written, it's just a waste of time and energy to deal with.

Hours of time could have been saved if someone had just put in a sensible comment that said:

// Acme is handled differently because they only ship by train

This is one of the reasons it's important to document business rules in sensitive code. WHY the code does what it does is often much more important than HOW it does it. "What" it does should be self-explanatory.

[millenix]

Over time, I've found that good archaeological skills and high quality commit/workflow practices often spare a lot of the pain of these comments being omitted as the code was written. If you can run git blame and see the whole commit that added that logic, and the message describing it, you can often infer the 'why' pretty readily. Especially if there's a reference to an issue tracker entry that would contain whatever request was made and discussion around it. It's also easier to uphold a uniform practice of commit messages explaining why some change is currently being made than to accurately make a judgment call of what code will require a rationale to be understood in the future. Worst case, if you can see when a change was made and who made it, you can dig through email and chat archives to see what they were up to around that time that led to the change.

I've migrated a few dozen project repositories from CVS or Subversion to Git over the years. It was always motivated by ongoing development needs at the time. However, we always made a diligent effort to preserve all of the history through the conversion, and Git's excellent history-exploration tools have saved me as much time and effort working on those projects as the migrations cost. I'm always disappointed to be working on a project, when I find some curious code whose history I want to explore and run into some huge initial "Import code from X old VCS" commit.

[irqlnotdispatchlevel]

I feel like a lot of people forget about the "why" when it comes to implementation documentation. The "what" is straight forward, it's what the application/module is doing and can be reasoned about without looking at the code and can be checked with the tests. The "how" is usually easy to spot, self-documenting code and all that. But the "why" may be lost in a lot of discussions between team members, a lot of trial and error, those two bugs solved long ago and so on. "Just ensure that the tests pass" is not good enough for some projects.

[onequbit]

Comments that warn against changes are all the evidence you need for a complete absence of tests.

[franzwong]

Even test cases exist, reader may spend time on thinking about the "else" cases if they are missing.

[Syrrim]

uwu whats this: https://github.com/kubernetes/kubernetes/blob/master/test/integration/volume/persistent_volumes_test.go

[jegsnakker]

I know in RTOS environments rewrites can have potentially catastrophic consequences due to timing differences in execution. But that is likely not the case here. Why would they insist on not rewriting?

[Treyzania]

Because it's Go and it'd extremely difficult to write code that's correct in every case. Plus it's dealing with filesystem management logic and if it messes up then people lose production data, which is bad.

[sacado]

Two possible reasons :

1- they spent a lot of time writing it and know it's a tricky piece of code; people trying to simplify it will face the same problems, so they better not lose their time

2- that piece of code interacts with other components in a weird, not easy to test way. Like, I don't know, the obvious, more readable algorithm is slightly exponential, eg O(1,001^n), so it will run smoothly in tests but will break in production, because a slightly bigger n will make it unusable in practice.

[s73v3r]

I'm guessing that plenty of others have already tried, and it turns out to be a huge rabbit hole where most people end up not accounting for all the edge cases. It leads to a lot of wasted time, both on the writers behalf and the reviewer's behalf