CodeSOD collection

BernieTheBernie

Johnny refactored some of Bernie's code. But why?

Let's take a look at some of the differences, Bernie wrote;

PointF? previousPoint = null;

Johnny changed that to

System.Windows.Point previousPoint = new System.Windows.Point(double.MinValue, double.MinValue);

You see: he defines the "invalid" case with double.MinValue. That point receives a "valid" value some where in a loop, but sometimes, it must be set to invalid. For Bernie, the latter task was easy:

previousPoint = null;

For Johnny, it is

previousPoint = new System.Windows.Point(double.MinValue, double.MinValue);

Later on, a line must be drawn - of course only when the previousPoint is valid. Bernie wrote:

if (previousPoint.HasValue)
{
    graphics.DrawLine(Pens.Aqua, previousPoint.Value, point.Value);
}

Johnny changed that to

if (previousPoint.X > double.MinValue && previousPoint.Y > double.MinValue)
{
    graphics.DrawLine(Pens.Aqua, (int)previousPoint.X, (int)previousPoint.Y, x, y);
}

Yeah, Johnny's code is so much clearer... Nullable value types are evil!

Well, let's wait, till he mixes double.MinValue and double.MaxValue, and ...

BernieTheBernie

And while Johnny was refactoring Bernie's code, he broke a simple test.

A 3d line had to be transformed to a 2d line, with a simple function such that the y coordinate of the resulting point simply equals its x coordinate +1. A simple case, isn't it?

Assert.AreEqual(point.X + 1, point.Y, "The point was not transformed correctly.");

Wonder how Johnny coped with breaking that... At least he saw the message ...

Assert.AreEqual failed. Expected:<2,10000002384186>. Actual:<2,09999990463257>. The point was not transformed correctly.

... and added a comment to the test:

// Die Punkte unterscheiden sich um ein Zehnmillionstel. Kann das einfach behoben werden? Oder eher Test ändern?

"Points differ by a 10,000,000th part. Can that be corrected in a simple way? Or better change the test?"

Great! I have great cow-orkers!

dkf

@BernieTheBernie I like the subtle change from PointF to Point.

JBert

@BernieTheBernie said in CodeSOD collection:

"Points differ by a 10,000,000th part. Can that be corrected in a simple way? Or better change the test?"

Ok, that comment is stupid - it should simply be asked and action should be immediately taken.

However, what about your test though? If the coordinates have always been floating point numbers then why doesn't your AreEqual method take a "delta" interval?

bobjanova

The values are probably integers in the simple test case?

BernieTheBernie

@JBert said in CodeSOD collection:

However, what about your test though? If the coordinates have always been floating point numbers then why doesn't your AreEqual method take a "delta" interval?

Because the test case is such simple that it even worked without any delta.

Of course, there's an overload available with a delta. But Johnny does not know that...

BernieTheBernie

@dkf said in CodeSOD collection:

I like the subtle change from PointF to Point.

It's actually from System.Drawing.PointF? to System.Windows.Point. The former is nullable and uses float for its coordinates, the latter uses double.

TwelveBaud

@BernieTheBernie That also explains why the DrawLine overload changed from one that accepted PointFs (i.e. your variable, exactly, modulo null) to one that has unpacked parameters. Because that's so much clearer too...

topspin

@TwelveBaud said in CodeSOD collection:

@BernieTheBernie That also explains why the DrawLine overload changed from one that accepted PointFs (i.e. your variable, exactly, modulo null) to one that has unpacked parameters. Because that's so much clearer too...

And the prevPoint’s coordinates are cast to int, while the other are not.

E: Oh walt, those aren’t a Point. Maybe they already are int.

dkf

@BernieTheBernie said in CodeSOD collection:

@dkf said in CodeSOD collection:

I like the subtle change from PointF to Point.

It's actually from System.Drawing.PointF? to System.Windows.Point. The former is nullable and uses float for its coordinates, the latter uses double.

The change from a nullable type to a non-nullable one is not subtle. I may not know C# very well, but I know enough to know that. Changing the type of the values inside though, that's a good way of causing all sorts of weird problems (which might or might not be a problem; you've got to revalidate everything…)

remi

@dkf said in CodeSOD collection:

Changing the type of the values inside though, that's a good way of causing all sorts of weird problems (which might or might not be a problem; you've got to revalidate everything…)

Once upon a time, long ago (more than 20 years ago), our code base was started using float for numbers everywhere. Then one day, much later but still long ago, we decided that it really, really, wasn't convenient and that we needed to switch to double (mostly for stuff like geographical coordinates... we had a hack in place to translate all coordinates to a local system such that float had enough precision, but keeping track of what was in which coordinates system was too painful).

So we started to carefully change some float to double, here and there, cautiously checking that the results we got were still correct (not identical, obviously). This went on for some time. But it was hard work, time consuming, and sometimes to change one float in one low-level algorithm you had to change a lot of other ones higher-up and it messed up a lot of things.

We got bored, and annoyed, and we were certainly very, very tired to hear about this.

So we did a sed on the code base, fixed compilation errors, didn't test anything and committed as a whole with a message along the lines of "numerical accuracy has changed, deal with it."

BernieTheBernie

@dkf said in CodeSOD collection:

you've got to revalidate everything

No, I am too lazy for that. Anyway, that is just for displaying purposes: a line (e.g. of a wall) is viewed from a point in space, and how does it look like from that point. Fortunately, no hazard will result from rounding errors in that place.

dkf

@BernieTheBernie said in CodeSOD collection:

No, I am too lazy for that.

Get a mathematician to have a think about it. (I wonder how many game physics engine glitches are due to this sort of thing.)

HardwareGeek

@dkf How many programmers (except, perhaps, in a university setting) actually have a mathematician available to think about it? Darned few, I'd guess.

topspin

@HardwareGeek said in CodeSOD collection:

@dkf How many programmers (except, perhaps, in a university setting) actually have a mathematician available to think about it? Darned few, I'd guess.

The mathematicians I have as colleagues do PDE stuff and still don’t think about that on this low level. (Precision/convergence is reasoned about on a higher level)

dcon

@HardwareGeek said in CodeSOD collection:

@dkf How many programmers (except, perhaps, in a university setting) actually have a mathematician available to think about it? Darned few, I'd guess.

I used to work with one. Back when I worked at a company doing disease simulation. (That was pretty cool stuff - it was basically a graphical interface to a partial-diff equation system.)

HardwareGeek

@dcon said in CodeSOD collection:

partial-diff equation system

:nope_badger_jumping_off_cliff.apng:

The last time I dealt with that sort of stuff was in uni, over 30 years ago. I never really grokked it then, and I sure as heck don't remember any of it now.

dcon

@HardwareGeek said in CodeSOD collection:

@dcon said in CodeSOD collection:

partial-diff equation system

:nope_badger_jumping_off_cliff.apng:

The last time I dealt with that sort of stuff was in uni, over 30 years ago. I never really grokked it then, and I sure as heck don't remember any of it now.

Luckily I didn't need to! I work at the UI level.

Tsaukpaetra

@remi said in CodeSOD collection:

So we started to carefully change some float to double, here and there, cautiously checking that the results we got were still correct (not identical, obviously). This went on for some time. But it was hard work, time consuming, and sometimes to change one float in one low-level algorithm you had to change a lot of other ones higher-up and it messed up a lot of things.

I did that with UE4 so servers could stay functional for more than about 3 days. Fun!

BernieTheBernie

E_BRAIN_NOT_SUPPORTED

Programming in C can distort your thoughts severely.
A cow-orker of Bernie constructed following gem (or is it rather a germ?):

private const int MIN_FOCUS_SENDING_TIME_AUTO_FOCUS_SECONDS = 60;
...
    TriggerFocusSenderFunction(TimeSpan.FromSeconds(MIN_FOCUS_SENDING_TIME_AUTO_FOCUS_SECONDS));
...
private void TriggerFocusSenderFunction(TimeSpan _minSendingTime)

Just tell me: why did he not define that timespan in milliseconds as is normal in C?

By the way, he could have defined that constant with

 private TimeSpan MINIMUM_FOCUS_SENDING_INTERVAL_FOR_AUTOFOCUS = TimeSpan.FromMinutes(1);

C# is hard. Much harder than C, actually.

Applied Mediocrity

@BernieTheBernie TimeSpan takes twice as much memory

PleegWat

@Applied-Mediocrity said in CodeSOD collection:

@BernieTheBernie TimeSpan takes twice as much memory

If you care over using 16 bytes instead of 8, you probably shouldn't be writing java.

dcon

Or is that:

@PleegWat said in CodeSOD collection:

If you care ~~over using 16 bytes instead of 8~~, you probably shouldn't be writing java.

Applied Mediocrity

@PleegWat said in CodeSOD collection:

@Applied-Mediocrity said in CodeSOD collection:

@BernieTheBernie TimeSpan takes twice as much memory

If you care over using 16 bytes instead of 8, you probably shouldn't be writing java.

() it's actually 8 over 4. And while I'm trolling here, generally I do believe using managed language doesn't give anyone a licence to waste memory

bobjanova

@Applied-Mediocrity Managed development isn't a licence to completely stop caring about memory, but +8b on the total memory footprint of an application is basically irrelevant. If you start using an object wrapper for every element in a data set of 10 million items then maybe it's worth looking at.

Also, TimeSpan is a struct wrapping a single long. So yes it's 8b not 4b but that's only because you had an int not a long. Structs in c# don't carry any extra memory overhead.

JBert

@bobjanova You took the :

@Applied-Mediocrity said in CodeSOD collection:

I'm trolling here

bobjanova

@JBert Nah I read that but, despite the troll tag, it was actually an interesting point - when is it in fact worth worrying about memory usage in managed environments?

PleegWat

@bobjanova said in CodeSOD collection:

@Applied-Mediocrity Managed development isn't a licence to completely stop caring about memory, but +8b on the total memory footprint of an application is basically irrelevant. If you start using an object wrapper for every element in a data set of 10 million items then maybe it's worth looking at.

Also, TimeSpan is a struct wrapping a single long. So yes it's 8b not 4b but that's only because you had an int not a long. Structs in c# don't carry any extra memory overhead.

I suspected as much, which is why I decided to diss java instead, which AFAIK does allocate every non-primitive type as a separate object in memory, so you'd have at least 8b object data and an 8b pointer.

topspin

@PleegWat said in CodeSOD collection:

@bobjanova said in CodeSOD collection:

@Applied-Mediocrity Managed development isn't a licence to completely stop caring about memory, but +8b on the total memory footprint of an application is basically irrelevant. If you start using an object wrapper for every element in a data set of 10 million items then maybe it's worth looking at.

Also, TimeSpan is a struct wrapping a single long. So yes it's 8b not 4b but that's only because you had an int not a long. Structs in c# don't carry any extra memory overhead.

I suspected as much, which is why I decided to diss java instead, which AFAIK does allocate every non-primitive type as a separate object in memory, so you'd have at least 8b object data and an 8b pointer.

And yet the effective cost of that "waste" is on the order of pico-cents, whereas doing it the shitty way probably cost Bernie at least 5-10 minutes of expensive developer time pondering WTF his cow-orker was thinking.

bobjanova

@topspin Also a good point - for most applications (there are exceptions, but most of us don't work on them) the cost of processing inefficiency is tiny compared to the cost of having engineers find and improve it. Sometimes just allowing the suboptimal solution to persist is actually the correct business approach.

HardwareGeek

@topspin said in CodeSOD collection:

probably cost Bernie at least 5-10 minutes of expensive developer time pondering WTF his cow-orker was thinking.

If you spend only 5-10 minutes pondering WTF you cow-orker was thinking, you're having a really great day. I spend more than that pondering WTF I was thinking.

Mason_Wheeler

@topspin said in CodeSOD collection:

And yet the effective cost of that "waste" is on the order of pico-cents,

Multiply that by the scale of modern programs and it starts turning into real money faster than you'd expect. (As I've said elsewhere, Wirth's Law applies here.) I've had to fix some very serious performance issues caused by exactly this attitude, and it turns out the real cost is somewhere closer to milli-cents.

topspin

@Mason_Wheeler said in CodeSOD collection:

@topspin said in CodeSOD collection:

And yet the effective cost of that "waste" is on the order of pico-cents,

Multiply that by the scale of modern programs and it starts turning into real money faster than you'd expect. (As I've said elsewhere, Wirth's Law applies here.) I've had to fix some very serious performance issues caused by exactly this attitude, and it turns out the real cost is somewhere closer to milli-cents.

I know that, I'm usually the first to bemoan that everything gets slower and bloated and uses JS bullshit for everything. But for the specific thing under discussion, where we were talking about 4 bytes total for something that's probably executed once, not 4 bytes in a structure that might be repeated and used millions of times, there is nothing to gain.
And the TimeSpan is also more type safe compared to a random int, more readable and maintainable, overall just better code.

Applied Mediocrity

The real cost here probably comes from TimeSpan.FromSeconds call. I can't say for a fact, but I believe it does not get inlined, and therefore creates two unnecessary stack frames every time TriggerHurrHurr is called. That creates at least order of magnitude more overhead than any trollful memory shavings.

Therefore, yes, the cow-orker seriously dun goofed.

One possible reason I can readily imagine, however, is that TriggerHurrHurr some time ago had int parameter and was then refactored to be more readable, but the constant was "fixed" the quick way - by making the syntax checker to shut up - fuck, ok, have your TimeSpan there, damn, happy now?.

boomzilla

@bobjanova said in CodeSOD collection:

when is it in fact worth worrying about memory usage in managed environments?

When it demonstrably starts causing problems. Just like any other optimization.

BernieTheBernie

@Applied-Mediocrity said in CodeSOD collection:

() it's actually 8 over 4. And while I'm trolling here, generally I do believe using managed language doesn't give anyone a licence to waste memory

And the code is running on 64bit windows, i.e. it accesses memory blocks of 64bits (or 8bytes), hence...

BernieTheBernie

What about that:

double diff = ...;
return diff > 0.0 && Math.Abs(diff) > _EvaluationTolerance;

Ehm, since _EvaluationTolerance is positive, why not simply

return diff > _EvaluationTolerance;

?

dkf

@BernieTheBernie That's horrible and exactly the wrong fix for testing whether things are close enough. The right thing would be more like:

return Math.Abs(diff) > _EvaluationTolerance;

Almost everything that is practically difficult with deployed floating point code relates to either comparisons or significance loss.

BernieTheBernie

@dkf said in CodeSOD collection:

@BernieTheBernie That's horrible and exactly the wrong fix for testing whether things are close enough. The right thing would be more like:
return Math.Abs(diff) > _EvaluationTolerance;
Almost everything that is practically difficult with deployed floating point code relates to either comparisons or significance loss.

Ääähh? What about a negative value of diff?

Unperverted Vixen

@BernieTheBernie That's what the Math.Abs() call is for.

BernieTheBernie

@Unperverted-Vixen said in CodeSOD collection:

@BernieTheBernie That's what the Math.Abs() call is for.

Did anyone read the return diff > 0.0 && part of the original code?

dkf

@BernieTheBernie Yes. That was the bit that was ghastly and very likely wrong.

Watson

Since if it's correct then the Math.abs() is an identity, so the latter test can be diff > _EvaluationTolerance, which, assuming the tolerance is nonnegative, then makes the diff > 0.0 redundant (in short, a negative number cannot be > a positive number).

topspin

@dkf said in CodeSOD collection:

@BernieTheBernie Yes. That was the bit that was ghastly and very likely wrong.

@BernieTheBernie was assuming the original code was correct but stupidly redundant, so his fix replaces it with something equivalent but not stupid.
You assume the code was wrong to begin with and your fix changes the semantics entirely. Without context, there's no way to tell which is correct.

You're checking for "not equal (within tolerance)", he's checking for "greater (plus tolerance)".

topspin

std::list<std::pair<int, double>> fooMap;

// ...

for (auto& i : fooMap)
{
    if (i.first == bar)

Um, you've already named that thing ...Map, why not actually use map?!

Also, I think I need some pre-commit hooks that check for std::list and say "this is bad, and you should feel bad."

dcon

@topspin said in CodeSOD collection:

Also, I think I need some pre-commit hooks that check for std::list and say "this is bad, and you should feel bad."

Well, there can be cases where list is better... Maybe...

topspin

@dcon said in CodeSOD collection:

@topspin said in CodeSOD collection:

Also, I think I need some pre-commit hooks that check for std::list and say "this is bad, and you should feel bad."

Well, there can be cases where list is better... Maybe...

And you can be sure that those are the cases where they use something else.

BernieTheBernie

Task.Run(()=>WhatEver()).Wait(); - Bernie just experienced E_HASHED_BRAIN when he stumbled upon this gem created by one of his cow-orkers:

public void DoSomething(double someParameter)
{
    _SomeComponent.Blah(someParameter);
    Task.Run(() => CheckBlah(someParameter)).Wait();
}

How does that differ from

public void DoSomething(double someParameter)
{
    _SomeComponent.Blah(someParameter);
    CheckBlah(someParameter);
}

Can't answer that now, because, well my poor brain....

Gąska

@BernieTheBernie if I didn't know what your cow-orkers are capable of, I'd suspect some thread-local fuckery (I've done hacks worse than that in the past).

BernieTheBernie

Let's come back to the topic of int versus TimeSpan. There can be great combinations:

private const int SET_FOCUS_MAX_WAIT_TIME_MS = 5;
...
TimeSpan.FromSeconds(SET_FOCUS_MAX_WAIT_TIME_MS)

You see, the constant explicitly has the "_MS" (i.e. "milliseconds") suffix. But later on, it's treated as a number of seconds, not milliseconds.