CodeSOD collection

dkf

It is kind of C++ like in that it's a powerful tool that can be abused to do horrible things, but it does have its place. It's hard to think of many outside mathematical algorithms, though, to be sure.

The problem is that over and over such things get abused, where abuse specifically includes anything that might be considered to be “expensive” in an automatically-applied cast. A language feature that almost always attracts bad usage is a bad feature.

Tsaukpaetra

@Tsaukpaetra said in CodeSOD collection:

I'm willing to bet some "optimization" is happening behind the scenes and the code I'm breakpointing isn't actually the code that's running.

Hrm...

Apparently the compiler believes this will never happen, despite the very first iteration (man-crafted) resulting in a nullptr of this.

TwelveBaud

@Tsaukpaetra You know compilers. There's the possibility that undefined behavior occurs. Since undefined behavior can "never" occur, anything that leads to it likewise can never occur. If one of those things happens because shouldDelete got set to true, especially if it's completely unrelated other than occurring later in the function, then obviously shouldDelete is never true and thus the else likewise can't possibly happen.

Try throwing everything against the -Wall.

remi

@TwelveBaud said in CodeSOD collection:

Try throwing everything against the -Wall.

Friendly reminder that you need to be -Wextra-careful if you really want all warnings...

Zecc

@remi never missing an opportunity to be -pedantic.

BernieTheBernie

Always try to make your code as incomprehensible as possible. Ideally, do not understand it yourself:

if (!rawImage.IsIncomplete || rawImage.ImageStatus != ImageStatus.IMAGE_NO_ERROR)
{
    return rawImage.ManagedData;
}

return null;

Via some SDK, a rawImage was retrieved, it has some properties like IsIncomplete (note the negative wording) and ImageStatus (which is a rather C like enumeration - IMAGE_NO_ERROR looks like the famous ERROR_OK).

So, the image is OK when it is complete (i.e. not incomplete) and at the same time the status is "no error". But ...

Edit:
Since this code is so convoluted, let me explain further.
What are the cases that (!rawImage.IsIncomplete || rawImage.ImageStatus != ImageStatus.IMAGE_NO_ERROR) is true?

The image is complete. IsIncomplete is false, hence !IsIncomplete is true. In that case, we do no more care for the status of the image. It may have some errors...
The image has some error status, i.e. ImageStatus != ImageStatus.IMAGE_NO_ERROR. In that case, we can accept an incomplete image, too.

My guess is: Johnny forgot a pair of ():
if (!**(**rawImage.IsIncomplete || rawImage.ImageStatus != ImageStatus.IMAGE_NO_ERROR**)**)

BernieTheBernie

And let's add some Null Pointer:

try
{
    m_NetworkStream.Write(_data, 0, _data.Length);
    m_NetworkStream.Flush();
    return true;
}
catch (Exception exception)
{
    LastError = exception.ToString();
    return false;
}

The exception occurred at the Write instruction.
Due to the fact that there is a catch, Kevin thinks that further investigations are unnecessary and checks for null a waste of processor resources...

TwelveBaud

Kevin needs to be stabbed in the face for that error passing method, and then defenestrated for making LastError a string. However, checks for null don't gain you anything: you're still left with a stream that isn't written to and that anything written prior will eventually get flushed when the stream closes (or if the stream itself is null a big fat nothing), and at best you're trading a NullReferenceException in for an ArgumentNullException that already won't be handled properly anyway.

BernieTheBernie

@TwelveBaud There are 2 items which could be null:
m_NetworkStream and _data.
That's the bigger fun with the NPE here...

dkf

@BernieTheBernie said in CodeSOD collection:

There are 2 items which could be null:

TwelveBaud

@BernieTheBernie Yes, and I addressed that with my post edit. In either case, Kevin's "deal with problems" code will just mean you return false and squirrel away an unhelpful string somewhere, and the side effects are the same whether you manually throw an exception or have the framework throw one automatically. Does there need to be an investigation why those things could be null? Sure. Would a runtime check help with that? Not a chance in hell.

dkf

@TwelveBaud said in CodeSOD collection:

Would a runtime check help with that? Not a chance in hell.

It needs a commit-time check: if (committing.user == 'kevin') reject() would be a first cut.

izzion

@dkf said in CodeSOD collection:

@TwelveBaud said in CodeSOD collection:

Would a runtime check help with that? Not a chance in hell.

It needs a commit-time check: if (committing.user == 'kevin') reject() would be a first cut.

I am in this post and I am deeply offended.

HardwareGeek

@izzion said in CodeSOD collection:

I am in this post

Don't be Kevin.

BernieTheBernie

@izzion It says "committing". It does not say "committed".

BernieTheBernie

Just had another fun with Kevin.
In our system, several devices work independently, and their data can be displayed on some connected machine. Kevin just redesigned the program on the connected computer.
The device creates a special component for a couple of calculations it needs to do; some of those calculations might be useful for the user interface, too, and so this component can be transferred via WCF from the device to the UI machine. This component requires several parameters which are specific for the device.
Now, one of those calculations created odd results on the screen, for one specific device.
How come it?
Kevin decided NOT to retrieve the component from the device, but create it himself.
Why do you do so?
Oh, that's such a simple component, no need to get it from the device.
Now you have duplicated some logic, and a change in the original place must be duplicate in the UI code, too.
No, it's just the creation of the component, not the component proper.
Oh yes, I see...
I know your polemics, Bernie, come on, get this thing repaired!

Eventually, I got the UI software running on my machine, too. Now I took a look at the image produced when my sensor device is running. Looks OK.
But then used some more sophisticated functions than just viewing.
And

virtually everything was wrong...

The reason:
those many device-specific data have to be stored in the configuration of the UI software now, too. I.e. duplicated. For every device, of course.
So, I cannot just change the configuration on the device, I have to change it in the UI program again.
Great, Kevin, you did it!

Tsaukpaetra

Status: What the fuck?

Why are you casting the object you just created to a pointer of the same type that you just created? Am I missing something?

TwelveBaud

@Tsaukpaetra Probably because NewColumn, which appears to be declared above this snippet, has a different type (most likely UObject) that doesn't have a TimeValue field. But I have more questions. Why is DatabaseTimeColumn (omission intentional) an object that lives in the Unreal engine? The object has a transient name; how's subsequent code supposed to find it, cast it (okay, there's manually looking up the UClass, but there's no compile time support for that), read it, or free it? Why isn't this being done in Slate, using, for example, CreateWidgetForText?

Tsaukpaetra

@TwelveBaud said in CodeSOD collection:

Why isn't

IDK, apparently this function is primarily to convert a POCO to a blueprint compatible equivalent so it can be sent as a broadcast.

I personally would have had an override-required function taking in the source column type to initialize itself from, but that's not my challenge at the moment.

The challenge is, apparently this code is triggering a crash because you're not supposed to NewObject while the garbage collector is running, which I can only guess is running because each and every column in every row is being NewObject'd.

Unfortunately, there is no consistent repo, but it apparently happens randomly at level load most often.

TwelveBaud

@Tsaukpaetra It's C, so it's POD, not POCO.

Okay, yes, you have to make a full UObject to take advantage of UnrealScript/Blueprint replication magic. But why a new object for every cell in a table? Why not a bag for the entire table, with the table data replicated as a whole unit? Why isn't there a guard at the top of whatever function this is against the garbage collector? And if there is, why the hell is this function reentrant?

Tsaukpaetra

@TwelveBaud said in CodeSOD collection:

But why a new object for every cell in a table?

No fucking clue. I would have made a row descriptor blueprint class to describe the table columns (I believe this exists already?) and just make the user sift the row through that through blueprint nodes that access it. I don't see the benefit to this approach at all.

@TwelveBaud said in CodeSOD collection:

with the table data replicated as a whole unit?

Yeah, I mean it already has the source struct, not sure why there's all this going on...

@TwelveBaud said in CodeSOD collection:

Why isn't there a guard at the top of whatever function this is against the garbage collector?

You know, I've been unsuccessfully trying to find out how to even do that. Apparently the people that even attempted it just failed fast when it was detected that a garbage collection was in progress and apparently that was fucking OK. I would imagine some kind of busy loop but I'm not sure how to wait the thread (been a while) to stop it from eating CPU for no reason.

@TwelveBaud said in CodeSOD collection:

And if there is, why the hell is this function reentrant?

in theory it should never be called simultaneously I think, but there's no debug logs or anything I can put in to try it myself, and I'm not going to download UE on my PC here to try it out myself (yet). This function assumes this is the case.

BernieTheBernie

@TwelveBaud said in CodeSOD collection:

Does there need to be an investigation why those things could be null? Sure. Would a runtime check help with that? Not a chance in hell.

A NullPointerException generally means that there is some programming error. And that ought to be investigated.
A runtime check here at the last position where the items are used, just for preventing an NPE, is useless - you're right. Some logging at a warning level could be a first step in the investigation.
Fortunately, the functionality there is not safety-critical per se. It is important when other parts of the software decided for an emergeny situation, sent an alarm to other devices, which then started costly $,$$$.$$ emergency measures: the customer then wants to get convinced by documented data that it was such a bad situation - and storing the data is the purpose of this function.
It is absofuckinglutely dependent on m_NetworkStream. How could it be null? Why was it not repaired?
If _data was null, how come it that a null package of information was sent to storage?
Does not matter. Says Kevin. Because there's a catch.

BernieTheBernie

Let me add some more parts of the story when Kevin and Martin - the head of development and quality - called me on Friday morning.
They found another issue.
"Look: when I click this button in the UI app, the expected dialog (with data from the sensor device) never shows up. The cursor goes into busy mode, and stays like that."
Well, true, the UI app never shows error messages. Because ... decided that showing error messages makes us looking bad. So error messages are just discarded, and hardly ever logged.
"What about the log file of the UI app? What does it tell?"
"Some WCF communication exception."
"Hm. And what about the log file of the sensor device?"
"Nothing".
"The sensor device logs every WCF call from the UI app. But only when the log level is set to 'debug'." Bernie was sure of that: it's his code.
Martin opened the configuration of the sensor device, changed that, and showed the issue again.
Bernie analysed the log file.
"OK. The 'GetList' command gets sent. After that, the UI app is expected to iterate over the items and request their properties. Only 'GetPropertiesOf(fristItem)' is called, but not 'GetPropertiesOf(secondItem)'."
After inspecting the properties of fristItem, Bernie decided that WCF cannot have failed here: strings only.
While Martin and Kevin were talking, Bernie analysed the code of the UI app which was written by Kevin, Johnny, and Jim. Eventually he found the loop. And there was a catch.
"Wait, let's take a look at the UI app log file. I wanna see it."
And, voilà, it was not the "WCF communication exception" reported by Kevin. It was at a line in the loop, and it showed that some resource was missing causing an NPE...

Yeah, what's logging for? Why do my cow-orkers always disregard the log file, and if they look at it, why can't they read a simple error message?

BernieTheBernie

Next issue.
"When I click here, the device will look at this position. See, in this simple conetxt it works."
Martin demonstrated it.
"But when I use FunctionX, it looks at a position slightly off."
Again, Martin demonstrated it.

FunctionX opens a small dialog at the postion where it was requested. You have to enter 2 values, and then click OK. The device looked then exactly at the position where the OK button was clicked...

When FunctionX command is sent to the sensor device, it does not cause a "LookAtPosition" command. Instead, the LookAtPosition command is additionally sent by the UI app. And somehow the porgrammers of the UI app managed to use the last click position of the dialog instead of the intended position.

Perhaps because of "routed events"? Perhaps. I am sure that our masters of WPF do not even understand what a RoutedEvent is.

dkf

@TwelveBaud said in CodeSOD collection:

@Tsaukpaetra It's C++, so it's POD, not POCO.

FTFY.

The <...> bits are a distinctive mark of not being C.

dkf

@BernieTheBernie said in CodeSOD collection:

Does not matter. Says Kevin. Because there's a catch.

Has he ever been exposed to ON ERROR RESUME NEXT? If so, did he catch the habit?

BernieTheBernie

@dkf ON ERROR RESUME NEXT is old Visual Basic. Don't know if Kevin used that some when. He's rather an old-style C guy, but without understanding C properly.

Vault_Dweller

@BernieTheBernie said in CodeSOD collection:

Why do my cow-orkers always disregard the log file, and if they look at it, why can't they read a simple error message?

The second part answers the first part

Gribnit

@dkf said in CodeSOD collection:

@TwelveBaud said in CodeSOD collection:

@Tsaukpaetra It's C++, so it's POD, not POCO.

FTFY.

The <...> bits are a distinctive mark of not being C.

Could be plain C with some kind of preprocessor.

remi

@Gribnit I don't think anyone is still using Cfront.

Gribnit

@remi said in CodeSOD collection:

@Gribnit I don't think anyone is still using Cfront.

Jaime

@BernieTheBernie said in CodeSOD collection:

ON ERROR RESUME NEXT is old Visual Basic. Don't know if Kevin used that some when. He's rather an old-style C guy, but without understanding C properly.

Interesting. I hear a lot of hate of ON ERROR RESUME NEXT, but the pattern that it creates strongly resembles old-style C style error handling (both being call and check rather than enforced via flow). Raymond Chen will often sing the praises of call and check over exception handling, he believes call and check makes it easier to write correct code (while simultaneously being easier to write incorrect code).

BernieTheBernie

@Jaime You think of the hResult return type of functions, followed by ignoring the return value?
Yeah, that's about the same, when it comes to effects.

But with VB6, some guys started every method with a
ON ERROR RESUME NEXT
and functions need not return a value (especially not an hResult), while in C your functions are expected to return an hResult (though it could always say ERROR_NO_ERROR).

dkf

@Jaime said in CodeSOD collection:

he believes call and check makes it easier to write correct code

Everyone's allowed to believe something stupid in their life!

BernieTheBernie

Understanding Kevin's code is hard. Because we can be sure that even he himself in all his unbelievable wisdom does not always understand it himself.

When Bernie dug thru the abominable code of a terrible class, he found a boolean member variable with very limited use. Far too limited:

line 129    private bool m_SbConfigOk;

line 263    catch (Exception ex)
            {
                m_SbConfigOk = false;
                Logger.LogException(Name, ex);
            }

line 291    if (!m_SbConfigOk)
            {
                WriteRegister(HW_CONFIG_REGISTER, 1);
            }

Something missing? Oh, really those are absolutely all occurences of the variable. All. None left out...

See it?

Well, the variable starts off with a value of false - it's a boolean.
In some function, it gets set to false in case of an exception there.
And in some if clause it gets checked, and if false, some WriteRegistercommand gets exectued.

So what?

Never can that become true.

BernieTheBernie

And another little gem from the Gem Factory (TM) of Kevin:

HighbyteLowbyte version = ReadInputRegister(4);
string versionLow = version.LowByte < 10 ? "0" + version.LowByte : version.LowByte.ToString();
return version.HighByte + "." + versionLow;

The version of an external device gets retrieved into some structure containing 2 individual bytes. Kevin wants to make sure that a version of 2.1 gets formatted as 2.01. As a Ternary Master he knows how to accomplitsh that.

Never would he dare to write a simple

return $"{version.HighByte}.{version.LowByte:D2}";

dkf

I've been having fun today with a former colleague's code. The code behaves correctly, but he loved to call a mapping dictionary from A to B a b_to_a_map. And he did this in many places with many concrete types for A and B.

izzion

@dkf said in CodeSOD collection:

I've been having today with a former colleague's code. The code behaves correctly, but he loved to call a mapping dictionary from A to B a b_to_a_map. And he did this in many places with many concrete types for A and B.

TwelveBaud

@dkf I can sort of see why he did that (Hungarian notation), since it'd show up as

B b = b_to_a_map[(A) a];

except the correct word there is "from", not "to"...

boomzilla

@TwelveBaud said in CodeSOD collection:

@dkf I can sort of see why he did that (Hungarian notation), since it'd show up as
B b = b_to_a_map[(A) a];
except the correct word there is "from", not "to"...

I often use by. As in, fooByBar.

Gribnit

@boomzilla I got this thing where I want as many orderings as possible to be constant to avoid creating unnecessary information lately, but I used to use by until the key convinced me it wanted to be mentioned before the value.

BernieTheBernie

@dkf said in CodeSOD collection:

call a mapping dictionary from A to B a b_to_a_map

Hey, was Kevin working at your company? Due to his lack of language concepts, he is prone to such errors, and won't be able to understand when you tell him...

English has a strict subject-predicate-object (SPO) rule. If there's no subject, the sequence of PO is still kept: "edit data".
In contrast, the German version is "Daten bearbeiten". Kevin translates that with "data edit". And now have additional fun with WindowShow, DataSave, ...

dkf

@BernieTheBernie said in CodeSOD collection:

Hey, was Kevin working at your company?

No, the guy in question was significantly more competent in the actual-semantics department. Just… a bit too attached to the detail and often terrible at naming things. Also not enough inclined to murder his babies, but that's a common failing.

Gribnit

@dkf said in CodeSOD collection:

@BernieTheBernie said in CodeSOD collection:

Hey, was Kevin working at your company?

No, the guy in question was significantly more competent in the actual-semantics department. Just… a bit too attached to the detail and often terrible at naming things. Also not enough inclined to murder his babies, but that's a common failing.

I attribute this to a loss of faith in Moloch in recent generations.

BernieTheBernie

@BernieTheBernie said in CodeSOD collection:

But then used some more sophisticated functions than just viewing.
And

virtually everything was wrong...
The reason:
those many device-specific data have to be stored in the configuration of the UI software now, too. I.e. duplicated. For every device, of course.
So, I cannot just change the configuration on the device, I have to change it in the UI program again.
Great, Kevin, you did it!

Now someone saw some of those wrong things. And Kevin is about to "fix" them.
How?
By requesting them NOW from the device which he would not do before?
No, of course not.
By adding even more duplications. An interface which was available on the device machine only must now become available for the UI application, too...

Can you guess where's that heading to?
...
even more duplications
...

BernieTheBernie

How small can a couple of snippets be to cause a concurrency issue?
Very small indeed.

if (m_Timer != null)
{
    m_Timer.Elapsed -= Timer_Elapsed;
    m_Timer.Stop();
    m_Timer.Dispose();
    m_Timer = null;
}

plus

if (m_Timer != null)
{
    double remainingInterval = Math.Max(1, m_TimerInterval.TotalMilliseconds - watch.Elapsed.TotalMilliseconds);
    m_Timer.Interval = remainingInterval;
    m_Timer.Start();
}

You see: immediately after the Dispose, the variable is set to null.
And still, an ObjectDisposedException occured at Start.

OK, let me add the f*cking lockstatement there, two. Including the duble-checked-locking-pattern. Hmpf.

Tsaukpaetra

Status: NaN is of course special...

I r dumb.

Jaime

@Tsaukpaetra said in CodeSOD collection:

NaN is of course special

Yes, but this is because language designers often favor simplicity over correctness and make "NaN" a valid value for a variable of type "number". This translates into "Not a number is a number", which, of course, is nonsense.

They mostly do this because the alternative is often something like making all numeric operations return a structure of type ArithmeticResult and having IsInfinity and IsNotANumber as fields. Of course, Result would be a numeric field, which may or may not contain a valid result.

For languages that don't support nullables, then accessing Result might error, and for languages that support nullable, expressions would have to do a ton of result checking. At the end of the day, it ends up in a trade off of "check for INF/NaN" or "check if Result is valid" and neither makes simple code easy to read. So, we get little landmines in the languages that were designed to be used under less strict conditions.

dkf

@Jaime said in CodeSOD collection:

expressions would have to do a ton of result checking

That's what exceptions are for (in languages that have them). 99.999% of the time, if you're thinking about returning NaN then you should be throwing an exception instead.

Jaime

@dkf Yes.... but these particular designers thought that throwing an exception made it more complicated.

Plenty of languages throw an exception on divide by zero. Then there's the special snowflake that simply returns INF. Their justification is always "simplification". I'm not saying it's right, just that it's how it comes to be.