CodeSOD collection

Gribnit

@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.

What if I'm computing the projection of a fractal, HUH?

dkf

@Jaime said in CodeSOD collection:

Plenty of languages throw an exception on divide by zero. Then there's the special snowflake that simply returns INF.

It depends. With integer division, an exception is best. Or otherwise indicating an error condition. With floating point division, provided you're not dividing zero by zero you should get one of the infinities (depending on sign). When you divide floating point zero by floating point zero, you're in the crazy zone; don't do that. In that case, again you should be dealing with an exception, but if you can't then a NaN is valid (to say “your math is now all fucked”).

Nullable/option types are better than NaN.

TwelveBaud

@dkf Kind of wish that NaN acted more like Itanium's NaT: keep allowing mutations to happen that preserves NaN status, like now, but blow up immediately with a NaN Consumption Fault if anything tries to save it anywhere or use it in a comparison. It usually has performance benefits over just throwing immediately.

As for nullable/option types, I guess you could call NaN a half-assed Maybe<Float> where they didn't quite think through how to do Just<Float> and None very well. But hey, it was 1985, and option types were still ivory-tower material back then.

BernieTheBernie

Multithreading and concurrency are terrible.
In a piece of code which was supposed to run single threaded (and by some yet to be determined bug was accessed by at least 3 threads at the same time), NullReferenceExceptions were experienced.

    private void Cleanup()
    {
        DateTime oldestAcceptable = HighPrecisionClock.Now.Add(-m_MaximumAge);
        for (int i = m_AlarmEmailStatuses.Count - 1; i >= 0; i--)
        {
            if (m_AlarmEmailStatuses[i].Timestamp < oldestAcceptable)
            {
                m_AlarmEmailStatuses.RemoveAt(i);
            }
        }
    }

Guess: where?
At m_AlarmEmailStatuses[i].Timestamp. 3 times within a millisecond.

I did not read up the details of the implementation of List<T>, but with this triple parallel removal of items the internal state of that object gets f***ed.
So extremely, that even later on, a Linq Any statement will crash with a NullReferenceException too.

Well, true, could use some thread-safe item instead of the common List. But in this specific case, the mere existence of multiple threads accessing the object is already wrong. Will be a lot of fun to find out how they came into existence.

dkf

@BernieTheBernie said in CodeSOD collection:

I did not read up the details of the implementation of List<T>, but with this triple parallel removal of items the internal state of that object gets f***ed.

I'd not expect it to have any locking, as that slows things down without actually helping very much. The appropriate place to put the lock is around the whole loop (and similarly for other places that access the list).

Zecc

@BernieTheBernie said in CodeSOD collection:

I did not read up the details of the implementation of List<T>

At least know this: https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.system-collections-icollection-syncroot

Default implementations of collections in the System.Collections.Generic namespace are not synchronized.

Enumerating through a collection is intrinsically not a thread-safe procedure. To guarantee thread safety during enumeration, you can lock the collection during the entire enumeration. To allow the collection to be accessed by multiple threads for reading and writing, you must implement your own synchronization.

There ought to be a lock (m_AlarmEmailStatuses.SyncRoot) { ... } surround the whole thing.

Zecc

Also, look inside System.Collections.Concurrent. You might be looking for a ConcurrentQueue<T> or something.

PS: But this isn't a help thread. You're just here to complain about others' code.

BernieTheBernie

@Zecc said in CodeSOD collection:

this isn't a help thread

Exactly.
I know how to add the duct tape at that place; and enjoy the search for the real culprit.
Oh, by the way, did you read that:

"the mere existence of multiple threads accessing the object is already wrong"

It's just fun to see how wonderfully intellegible things turn out when concurrency wrecked havoc.

Gribnit

@BernieTheBernie usually optimistic to assume the object can control its accesses. If the function needs to be single threaded it needs to lock out others, or if maybe it needs to pin itself to a thread, it needs to hold a reference to check against. There is no hope except in paranoia.

topspin

int main(int argc, char* argv[])
{
    if (argc == 1)
        throw std::runtime_error("No input file specified.");
    // ...
}

Ugh. This is the outermost scope possible.
I guess it's in that the output will complain about an uncaught exception, usually containing the exception string somewhere.

Gribnit

@topspin I can just supply a quoted empty string? cool thanks ticket closed

error

This post is deleted!

error

(I think my previous post was .)

dkf

@Gribnit said in CodeSOD collection:

@topspin I can just supply a quoted empty string? cool thanks ticket closed

If you want. There will probably be a failure later on when opening that file, but that's your problem.

LaoC

@topspin said in CodeSOD collection:

int main(int argc, char* argv[])
{
    if (argc == 1)
        throw std::runtime_error("No input file specified.");
    // ...
}
Ugh. This is the outermost scope possible.
I guess it's in that the output will complain about an uncaught exception, usually containing the exception string somewhere.

Former Perl guy fo sho.

@ARGV or die "Aaaaaahhhrghhh!!!!1";

Gribnit

@dkf said in CodeSOD collection:

@Gribnit said in CodeSOD collection:

@topspin I can just supply a quoted empty string? cool thanks ticket closed

If you want. There will probably be a failure later on when opening that file, but that's your problem.

la la la,
ticket closed

BernieTheBernie

Optional Parameters.
A change in requirements was made, and a function needed an extra bool parameter. void Update() had to be changed to void Update(bool hmpf).

Kevin lookked at a concrete class and changed it there, To make it simple for him, he made the parameter optional and provided a default value:
void Update(bool hmpf=false). That way, he could still call the Update method without the parameter, and the runtime would automagically supply the parameterhmpfwith value false.

Unfortunately for Kevin, the original version was designed by Bernie The Bastard, and thus there was interface, and the method was defined in the interface. So it did not compile.

So Kevin had to change the interface too. What did he do there?
void Update(bool hmpf)
with the parameter being mandatory (note: with C#, optional parameters are also possible in interfaces).

Now Kevin's code compiled.
But other code by Bernie did not.

Why?
Because all of Kevin's code worked with the concrete class in all places; if he did not supply the optional parameter, so what: it was optional.
Bernie's code uses the interface. And there the parameter is now mandatory, and when the calls were not yet adjusted to supply that parameter, ...

See:
That stupid know-it-all Bernie The Bastard who always preaches abstraction and programming against interfaces instead of concrete classes was again shown to produce defective and hardly maintanable code. Why does Bernie still stubbornly fail to learn simple coding practices:

when ever possible, use concrete classes and avoid abstract interfaces!
program against concretions, never assume abstractions!

Gribnit

@BernieTheBernie supply an API artifact with the interfaces, and no means for Kevin to change them.

BernieTheBernie

Again the (presumed) sort order of data caused some inconvenience.

For Kevin and some more of my cow-orkers, it is absolutely clear to always develop against concretions instead of interfaces, and to assume things that are nowhere guaranteed to stay as they are now.

Well, in the current case, things never were as Kevin assumed.

In case of some special important events, the software on our devices dumps many locally cached data into the database. All those data come with their own timestamp. But they may be added to the database in a relatively random order, just dump them there. In such an event, they must be stored, that's the point.

Kevin of course assumes that they were stored in the order of their timestamps, and then "forgets" to display some of them because their timestamp is too old...

: Bernie, you have to store them in correct order! Sorting costs too much CPU!
: Why ?

izzion

@BernieTheBernie said in CodeSOD collection:

Again the (presumed) sort order of data caused some inconvenience.

For Kevin and some more of my cow-orkers, it is absolutely clear to always develop against concretions instead of interfaces, and to assume things that are nowhere guaranteed to stay as they are now.

Well, in the current case, things never were as Kevin assumed.

In case of some special important events, the software on our devices dumps many locally cached data into the database. All those data come with their own timestamp. But they may be added to the database in a relatively random order, just dump them there. In such an event, they must be stored, that's the point.

Kevin of course assumes that they were stored in the order of their timestamps, and then "forgets" to display some of them because their timestamp is too old...

: Bernie, you have to store them in correct order! Sorting costs too much CPU!
: Why ?

The SQL guy inside me is both laughing (because he's "right") and crying (because he's oh so wrong)

dkf

@BernieTheBernie said in CodeSOD collection:

stored in the order of their timestamps

Someone hasn't quite understood… well, a great many things. Indices are definitely on that list.

BernieTheBernie

@izzion said in CodeSOD collection:

SQL guy inside me

Well, um, not SQL. It's some Kevin designed file storage, with data spread over several files in several directories. Some files contain information on parts of several "rows", others contain information on a field of a single row, others... Anyway, it is a mess.

izzion

@BernieTheBernie said in CodeSOD collection:

@izzion said in CodeSOD collection:

SQL guy inside me

Well, um, not SQL. It's some Kevin designed file storage, with data spread over several files in several directories. Some files contain information on parts of several "rows", others contain information on a field of a single row, others... Anyway, it is a mess.

Well, I was more thinking to the very general SQL guidance of "do your sorting in the web front end" (assuming several things, including that you don't need to filter on the sort), since sorting is fairly CPU intensive and web server CPU time is a lot cheaper than SQL server CPU time. I certainly didn't mean to overestimate Kevin's capabilities

dkf

@BernieTheBernie said in CodeSOD collection:

Well, um, not SQL. It's some Kevin designed file storage, with data spread over several files in several directories. Some files contain information on parts of several "rows", others contain information on a field of a single row, others... Anyway, it is a mess.

BTDT. Replace with one SQLite database. Bonus: you get easy access to various analysis tools written by others.

BernieTheBernie

@izzion said in CodeSOD collection:

overestimate Kevin's capabilities

You cannot never overunderestimate Kevin's incapabilities!

Gribnit

@BernieTheBernie said in CodeSOD collection:

@izzion said in CodeSOD collection:

SQL guy inside me

Well, um, not SQL. It's some Kevin designed file storage, with data spread over several files in several directories. Some files contain information on parts of several "rows", others contain information on a field of a single row, others... Anyway, it is a mess.

Fucking burn it. As a 3rd party expert with 10+ years relevant experience I officially opine burn it.

...

Sure, fine, maybe then replace it with something.

Tsaukpaetra

I must be stupid, but isn't this a blatant memory leak?

WiFiManager/WiFiManager.cpp at 8984863fe6a8106bf0ead7e6ac013c745a4d1f93 · tzapu/WiFiManager

ESP8266 WiFi Connection manager with web captive portal - tzapu/WiFiManager

void WiFiManagerParameter::setValue(const char *defaultValue, int length) {
/// .... blah blah check if things are not null or whatever

  _length = length;
  _value  = new char[_length + 1]; 
  memset(_value, 0, _length + 1); // explicit null
  
  if (defaultValue != NULL) {
    strncpy(_value, defaultValue, _length);
  }
}

If this is called more than once, instant leak, right?

I'm going to put a delete if _value is not null, but I'm not certain that's the cromulent solution....

Tsaukpaetra

@BernieTheBernie said in CodeSOD collection:

Will be a lot of fun to find out how they came into existence.

Someone set a Timer and didn't know that it doesn't actually wait the completion of the routine before starting it again. BTDT, easiest solution was to make the timer a one-shot and reset it at the end of the routine...

topspin

@Tsaukpaetra said in CodeSOD collection:

I must be stupid, but isn't this a blatant memory leak?

WiFiManager/WiFiManager.cpp at 8984863fe6a8106bf0ead7e6ac013c745a4d1f93 · tzapu/WiFiManager

ESP8266 WiFi Connection manager with web captive portal - tzapu/WiFiManager
void WiFiManagerParameter::setValue(const char *defaultValue, int length) {
/// .... blah blah check if things are not null or whatever

  _length = length;
  _value  = new char[_length + 1]; 
  memset(_value, 0, _length + 1); // explicit null
  
  if (defaultValue != NULL) {
    strncpy(_value, defaultValue, _length);
  }
}
If this is called more than once, instant leak, right?

I'm going to put a delete if _value is not null, but I'm not certain that's the cromulent solution....

Have they ever heard of std::string?!

Also, WiFiManager.h said:

        //manually start the config portal, autoconnect does this automatically on connect failure
        boolean       startConfigPortal(); // auto generates apname
        boolean       startConfigPortal(char const *apName, char const *apPassword = NULL);
    
        //manually stop the config portal if started manually, stop immediatly if non blocking, flag abort if blocking
        bool          stopConfigPortal();

Can you do me a favor and click show definition on boolean?

Tsaukpaetra

@topspin said in CodeSOD collection:

Can you do me a favor and click show definition on boolean?

Arduino IDE has no such capability. The closest function simply links to the docs on the boolean keyword.

@topspin said in CodeSOD collection:

Have they ever heard of std::string?!

I'm going to assume their custom implementation is handling that. Or something. Maybe. I don't know why, other than perhaps including the std library is too large?

topspin

@Tsaukpaetra said in CodeSOD collection:

@topspin said in CodeSOD collection:

Can you do me a favor and click show definition on boolean?

Arduino IDE has no such capability. The closest function simply links to the docs on the boolean keyword.

My point was that this looks like it's C++ and there is no boolean keyword, it's called bool.

@topspin said in CodeSOD collection:

Have they ever heard of std::string?!

I'm going to assume their custom implementation is handling that. Or something. Maybe. I don't know why, other than perhaps including the std library is too large?

I doubt that. The WiFiManager header has an #include <vector> in there, so at least they use that, but instead of using string they use a funky combination of C string functions and manual C++ memory allocation. And as you have spotted, they get it wrong and leak memory. Which is idiotic if you have a class that handles memory automatically and correctly.

Just from the few lines I've seen this code base looks like a terrible hack.

Tsaukpaetra

@topspin said in CodeSOD collection:

Just from the few lines I've seen this code base looks like a terrible hack.

It's the nicest terrible hack I've found to easily provide a configuration interface for these WiFi enabled Arduinos without coding it myself.

dkf

@Tsaukpaetra said in CodeSOD collection:

I must be stupid, but isn't this a blatant memory leak?

[…]

I'm going to put a delete if _value is not null, but I'm not certain that's the cromulent solution....

I assume you also make sure that _value is initialised correctly by the constructor(s) and cleaned up by the destructor?

PleegWat

@Tsaukpaetra said in CodeSOD collection:

I must be stupid, but isn't this a blatant memory leak?

WiFiManager/WiFiManager.cpp at 8984863fe6a8106bf0ead7e6ac013c745a4d1f93 · tzapu/WiFiManager

ESP8266 WiFi Connection manager with web captive portal - tzapu/WiFiManager
void WiFiManagerParameter::setValue(const char *defaultValue, int length) {
/// .... blah blah check if things are not null or whatever

  _length = length;
  _value  = new char[_length + 1]; 
  memset(_value, 0, _length + 1); // explicit null
  
  if (defaultValue != NULL) {
    strncpy(_value, defaultValue, _length);
  }
}
If this is called more than once, instant leak, right?

I'm going to put a delete if _value is not null, but I'm not certain that's the cromulent solution....

Well, theoretically _value might be using the correct smart pointer type so that the memory is deleted automatically. It doesn't, but it might have.

Tsaukpaetra

@dkf said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

I must be stupid, but isn't this a blatant memory leak?

[…]

I'm going to put a delete if _value is not null, but I'm not certain that's the cromulent solution....

I assume you also make sure that _value is initialised correctly by the constructor(s) and cleaned up by the destructor?

I'm not doing anything, but the library writers seem to have done the needful, yes.

Tsaukpaetra

Status: When apparently you don't want to (or maybe can't?) use the "fit text to bounds" attribute...

dkf

@Tsaukpaetra Wait until they figure out that not all characters are the same width…

Gribnit

@dkf said in CodeSOD collection:

@Tsaukpaetra Wait until they figure out that not all characters are the same width…

Eh, about enough of all of enough characters are about the same width - law of medium sized numbers.

Tsaukpaetra

@dkf said in CodeSOD collection:

@Tsaukpaetra Wait until they figure out that not all characters are the same width…

Oh, I've already known that and had someone complain to me that they have an object they can't interact with because the fucking text pushed the required button off the screen. Nobody here but me to fix it though so...

BernieTheBernie

@Tsaukpaetra Use the "correct" font - an old typewriter font. Thus, all characters will have the same width.
In effect, a socialist font, isn't it?

BernieTheBernie

How does a different frame rate of the camera change the CPU load of our application? Bernie tested it, and, confusingly, found no effect at all. He analysed the log files further, and, even more strangely, the interval between two images seemed to stay constant, absofuckinglutely independent of the frame rate.

The solution is in the code:

    public override int FrameRateIndex
    {
        get => 0;
        set
        {

        }
    }

With this camera model, the frame rate cannot be changed. But you can configure any value you wish. No exception thrown. No warning logged. Just confusing behavior.

Thank you, Johnny!

That's the way how security critical software (fire detection) has to be designed.

Tsaukpaetra

@BernieTheBernie said in CodeSOD collection:

@Tsaukpaetra Use the "correct" font - an old typewriter font. Thus, all characters will have the same width.
In effect, a socialist font, isn't it?

Maybe. But, I'd rather solve by doing something more atrocious, like turning the text into a marquee!

Applied Mediocrity

@Tsaukpaetra said in CodeSOD collection:

Maybe. But, I'd rather solve by doing something more atrocious, like turning the text into a marquee!

Make it blink for extra points.

Tsaukpaetra

@Applied-Mediocrity said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

Maybe. But, I'd rather solve by doing something more atrocious, like turning the text into a marquee!

Make it blink for extra points.

Oh it already kinda does that. If the cursor is over the text it tries to adjust its size, which because UE4 is not a consistent operation, so the text will switch sizes each frame, even though the same value for the size is being sent. It's.... fucking retarded.

Vault_Dweller

@Applied-Mediocrity said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

Maybe. But, I'd rather solve by doing something more atrocious, like turning the text into a marquee!

Make it blink for extra points.

I know of some nice GeoCities websites that you can use as inspiration.

dkf

@BernieTheBernie said in CodeSOD collection:

Use the "correct" font - an old typewriter font.

That doesn't work (“thank you”, Unicode, for composing characters and stuff like that). Instead you need to actually measure the real text using the proposed font. All font engines provide this operation; it's as fundamental to text layout as actually drawing the characters, and most fonts don't have totally fucked metrics these days.

HardwareGeek

@dkf said in CodeSOD collection:

don't have totally fucked

:

Gribnit

@dkf said in CodeSOD collection:

Instead you need to actually measure the real text using the proposed font.

It's a lot more fun to never tell someone this than to tell someone this.

BernieTheBernie

What's a propertymeant to be?
Well, uhm, ... Great Porgrammers can do quite everything with it.
Kevin has a special idea:

    protected bool Connected
    {
        private get => m_Connection?.State == ConnectionState.Open;
        set
        {
            try
            {
                if (!value)
                {
                    m_Connection?.Close();
                    m_Connection = null;
                    return;
                }
                if (m_Connection == null)
                {
                    m_Connection = new MySqlConnection(ConnectionString);
                }
                if (m_Connection.State == ConnectionState.Closed)
                {
                    m_Connection.Open();
                }
            }
            catch (Exception exception)
            {
                m_Connection = null;
                Logger.LogException(nameof(PanoramaCleaner), exception);
            }
        }
    }

By the way, this C# class has also a destructor (!), which sets Connectedto false.

Well, Kevin, this kind of C# porgramming is very false.

BernieTheBernie

[TestClass]
public class PspUnitTest
{
    [TestMethod]
    public void DateTimeTest()
    {
        Assert.IsTrue(DateTime.Now.Date.CompareTo(DateTime.Today) == 0, "DT-Compare failed.");
    }
}

What's wrong with that?
The failure message, of course.
It ought to be: "Microsoft fucked it up!".