About a year ago I had the great idea to enforce ago I had the great idea of proposing that we all lint our legacy code base using eslint to increase the overall quality of our JS.
I distributed the task of initially fixing all the errors eslint would find to the whole Frontend team (Luckily we only use JS there). I've finished my part in a couple of weeks and came across this piece of spaghetti.
One of the guys who has been with the company for over 10 years said, that the guy who wrote this monster was very proud of it...
In case you cannot understand what this does: It calculates the distance between 2 points on earth.

That's not spaghetti, that's Maths. Respect it.

I fail to see how this is spaghetti.

This is basic math that's memory efficient.

I have had devs write stuff like this and they broke it out across 5-8 vars so it was "easier" to maintain.

It's not easy to maintain though, it's easier to understand if you don't have a basic understanding of math.

Also, by using multiple vars the computation is slower and less efficient since it has to read and write to memory twice as much (or more).

So unless I'm missing something, this lacks spaghetti.

@sariel I don't think that's true any more. Modern js interpreters are capable of optimizing situations like that away.

The recommended approach is to break it out, and put the mathematical function it represents in a comment above it, with a link that describes it's function.

@lungdart this may be true for JS but not all languages will do this.

It's just lazy development IMO. Throw some comments in there with some workable examples and that should be enough for anyone to understand what's going on here.

Also, in this specific example this code should never need to be updated unless something catastrophic happens to the shape of our planet, and at that point I doubt anyone will care(or be alive to care).

@sariel clarity is best practice. If clarity can't be achieved because of optimization or some other janky reason, there needs to be a reason and ideally a link to back up your claims in a supporting comment.

Assuming you have to do it like this is premature optimization, it's a sign you didn't know better, and didn't test, but made it more complicated anyway.

It's bad behaviour

This is just maths and JS being to verbose to fit nicely in one line without wrapping around.

I'd just leave a comment above with the actual formula used.

Wait, why is earthRadius an instance variable?

@sariel In every remotely popular language above Assembly level local variables that are used only once are inlined.

I think that breaking it up into a function style would make it more readable...

What makes me uncomfortable mostly in this code is the mixture of floating point vars / integer vars and "undefined".

The math.min uses an integer first, then an floating point result - shouldn't matter as Math.min uses Number for comparison if I remember correctly, but still me no like it - rest of calculation mixes integer and floating point, too.

The second thing is - this is not "just" math.

Each of the calculations is done by a Javascript function which can - for more or less absurd reasons - fail.

Especially since floating point arithmetic is involved.

Breaking it up and doing a bit more validation wouldn't hurt.

Regarding performance - I'm really getting tired of that argument, especially when we're talking about a language like JavaScript, where most likely the whole Fuckton of ECMA Script / Polyfill / Framework autoloading / blablaba is rendering this pointless.