CodeSOD collection

Gąska

@BernieTheBernie maybe MS is short for Many Seconds? You know, 1, 2, 3, many...

Because that's the level of intelligence shown by the author.

BernieTheBernie

@BernieTheBernie maybe MS is short for Many Seconds? You know, 1, 2, 3, many...

Because that's the level of intelligence shown by the author.

But there is also

private const int THREAD_STD_SLEEP_TIME_MS = 100;

which is treated as number of milliseconds.

Now you may have fun with that variable name, too.
Does STD stand for standard deviation? Or Sexually Transmitted Disease?
The SLEEP afterwards may indicate the latter idea.

Kamil Podlesak

@Gąska said in CodeSOD collection:

@BernieTheBernie maybe MS is short for Many Seconds? You know, 1, 2, 3, many...

Because that's the level of intelligence shown by the author.

Maybe it's Metric Seconds? I mean, this makes sense - there are milliseconds being used, instead of the natural untils like jiffs and eyeblinks.

BernieTheBernie

@BernieTheBernie said in CodeSOD collection:

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

Meanwhile, I got an answer at a programming forum. The snippets are essentially equal, the second one just adds more system load by that extra Task to it. DoSemething won't return before CheckBlah returned.

My conclusion: the cow-orker strives for job security by code obscurity.
An old motive on the front page, isn't it?

bobjanova

@BernieTheBernie said in CodeSOD collection:

How does [ Task.Run(() => CheckBlah(someParameter)).Wait(); ] differ from
[ CheckBlah(someParameter); ]

In the simple case CheckBlah will be run on the current thread. In the Task.Run case, it will be run on another thread (one in the executor's thread pool, assuming a normal config), and the current thread will wait for it to finish (including waiting for it to get pipelined, if the thread pool is limited and busy).

It's hard to see any situation where that difference would be useful. You're still blocking the current thread, and you're also adding load into the thread pool and some OS thread switching/monitoring overhead. Your coworker almost certainly did not mean to do it. But there is a difference.

dkf

@topspin said in CodeSOD collection:

Without context, there's no way to tell which is correct.

I apply the context of using a normal pattern for safe programming in floating point code. I assume that diff holds a difference in some form and that tolerance is a positive “constant” (because that's very much the common case). Given that, following the normal pattern would be expected, and doing it the other way very much not. There are cases where the other way round would be correct, but they're much much rarer and run into some extremely tricky issues with hybrid non-smooth systems. If you're doing a lot with those, you want a very good mathematician on staff because there are some ways to gloriously fuck up otherwise.

Yes, I do actually work with some of these issues…

BernieTheBernie

It's so great when you can switch off logging of "UnobservedTaskException":

TaskScheduler.UnobservedTaskException += TaskScheduler_UnobservedTaskException;

private static void TaskScheduler_UnobservedTaskException(object sender, UnobservedTaskExceptionEventArgs e)
{
    if (_UnobservedTaskExceptionLogging)
    {
        Logger.LogError("Application", $"Unhandled exception from {sender}: ");
        Logger.LogException("Application", e.Exception);
    }
    e.SetObserved();
}

By setting UnobservedTaskExceptionLogging to no (ehm, yes , "no", not "false") in App.config, you can now happily ignore all of those exceptions (in contrast to the original code, which always logged them)...

Will be great fun when trying to find out what went wrong at a customer site.

JBert

@BernieTheBernie Do they actually appear that frequently that you need to stop overflowing the logfile?

Because that would be

BernieTheBernie

@JBert said in CodeSOD collection:

@BernieTheBernie Do they actually appear that frequently that you need to stop overflowing the logfile?

Because that would be

Hm... Johnny did add some more code to that application, and obviously felt that it was necessary to add that logging prevention...

And you learned about his mastery of Tasks a few posts before (Task.Run(...).Wait())

So I guess the answer is YES.

BernieTheBernie

RemoveAll. Hm. What does that mean?

public static void RemoveAll<T>(this ICollection<T> _collection, ICollection<T> _rangeToAdd)
{
    for (int i = _rangeToAdd.Count - 1; i >= 0; i--)
    {
        _collection.Remove(_rangeToAdd.ElementAt(i));
    }
}

Oh yes, of course. Everything clear now.

It removes elements listed in _rangeToAdd from _collection.
Since removing an element from a collection may change the index of the following elements, it is absofuckinglutely necessary to iterate thru _rangeToAdd (which is not changed at all) from its end. And, of course, the name _rangeToAdd makes undoubtlessly clear that you really intended to remove those items.

dkf

@BernieTheBernie Nice how it doesn't check for the case of removing a collection from itself. In that case, things can be optimised (and all other cases are easy).

TwelveBaud

@BernieTheBernie said in CodeSOD collection:

Since removing an element from a collection may change the index of the following elements, it is absofuckinglutely necessary to iterate thru _rangeToAdd (which is not changed at all) from its end.

@dkf said in CodeSOD collection:

Nice how it doesn't check for the case of removing a collection from itself.

If you don't know that Clear() is a thing that exists, the behavior of the first point neatly compensates for the second point. It's almost like someone took Data Structures in college!

Tsaukpaetra

Status: Shirley, there must be a better way of doing this!


        private StringBuilder m_StateGetAPI = new StringBuilder(string.Empty);
        private String StateGetApi
        {
            get
            {
                m_StateGetAPI.Remove(0, m_StateGetAPI.Length);
                m_StateGetAPI.AppendFormat("{0}/state/?macAddress={1}", ServerAddress, GetMacAddress());
                return m_StateGetAPI.ToString();
            }
        }
        private StringBuilder m_StatePostAPI = new StringBuilder(string.Empty);
        private String StatePostApi
        {
            get
            {
                m_StatePostAPI.Remove(0, m_StatePostAPI.Length);
                m_StatePostAPI.AppendFormat("{0}/state/", ServerAddress);
                return m_StatePostAPI.ToString();
            }
        }
        private StringBuilder m_GamesAPI = new StringBuilder(string.Empty);
        private String GamesApi
        {
            get
            {
                m_GamesAPI.Remove(0, m_GamesAPI.Length);
                m_GamesAPI.AppendFormat("{0}/Games/", ServerAddress);
                return m_GamesAPI.ToString();
            }
        }

BernieTheBernie

@Tsaukpaetra said in CodeSOD collection:

there must be a better way of doing this!

Yes, there is. I'd prefer to initialize the StringBuilder with new StringBuilder("WTF?").

topspin

@Tsaukpaetra said in CodeSOD collection:

there must be a better way of doing this!

Not if you want race conditions, then this seems ideal.

Maybe he originally intended to cache the results (for whatever stupid reason), then discarded that idea halfway in?

dkf

@Tsaukpaetra said in CodeSOD collection:

there must be a better way of doing this!

Of course there is. That m_ prefix is superfluous.

PleegWat

@Tsaukpaetra said in CodeSOD collection:

Shirley, there must be a better way of doing this!

I wouldn't know how. I mean, it's not as if C# has a String.Format() method.

BernieTheBernie

Fritz, the head of development and quality, is not fond of long stack traces of error messages in log files. He ordered that they must not be logged. Brilliant Johnny just implemented that. Of course, he broke a unit test which checked that an InnerException gets logged also (I guess, Fritz does not like them either.)

Yeah, a great way of making a complex product unsupportable.

dkf

@BernieTheBernie said in CodeSOD collection:

long stack traces of error messages

Nobody really likes that, but shortening them is hard. Very very difficult to get that right, especially when using complex frameworks. Essentially, if you can show that the exception occurred within your own code, you can trim out the wrapping framework crap, but you can't do that blindly in case the framework has some kind of issue and throws exceptions itself. (Hit that once, where it got very unhappy with the annotations I was using to control the webservice binding.) No logging framework really gives you the right tools out of the box to do a great job of this.

Far more important is ensuring that you only ever log an exception at most once. Don't log them several times… unless you really want your production logs to be terabytes long.

He ordered that they must not be logged.

Brillant! That's going to help Ops out so much.

BernieTheBernie

@dkf said in CodeSOD collection:

Brillant! That's going to help Ops out so much.

Exactly that's why he ordered that. You know: the Dunno-Kruger-Effect.

BernieTheBernie

@PleegWat said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

Shirley, there must be a better way of doing this!

I wouldn't know how. I mean, it's not as if C# has a String.Format() method.

Even easier: string interpolation. E.g.

private string StateGetApi => $"{ServerAddress}/state/?macAddress={GetMacAddress()}";

But that's a lot less lines of code, i.e. less money. $$$!

Tsaukpaetra

@PleegWat said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

Shirley, there must be a better way of doing this!

I wouldn't know how. I mean, it's not as if C# has a String.Format() method.

It's already basically that! Just remove the stringbuilder parts!

hungrier

@BernieTheBernie said in CodeSOD collection:

You know: the Dunno-Kruger-Effect.

:freddy_shrugging_medium-light_skin_tone:

BernieTheBernie

Bernie just had to open a code file produced by Kevin. Because Kevin broke the Jenkins build on Friday afternoon...

BlahViewModel.cs(418,30): error CS0168: Die Variable "ex" ist deklariert, wird aber nie verwendet.

Aha. Looks like some catch(Exception ex){ /* empty */ }, and the command line on Jenkins uses the parameters /p:TreatWarningsAsErrors="true" /p:CheckForOverflowUnderflow="true" /p:WarningLevel=4.

That's easy to fix for Bernie, but let's bet how many hours that will take Kevin.

Now that Bernie had to open that file, he saw some easy to understand code:

private BitmapSource EmptyImage
{
    get
    {
        int bytesPerPixel = (PixelFormats.Indexed1.BitsPerPixel + 7) / 8;
        int stride = VIDEO_WIDTH * bytesPerPixel;
        byte[] pixels = new byte[VIDEO_HEIGHT * stride];
        for (int i = 0; i < VIDEO_HEIGHT; ++i)
        {
            for (int j = 0; j < stride; ++j)
            {
                pixels[stride * i + j] = 255;
            }
        }
        List<System.Windows.Media.Color> colors = new List<System.Windows.Media.Color> { Colors.Red, Colors.Blue, Colors.Green };
        BitmapPalette myPalette = new BitmapPalette(colors);
        return BitmapSource.Create(VIDEO_WIDTH, VIDEO_HEIGHT, 96, 96, PixelFormats.Indexed1, myPalette, pixels, stride);
    }
}

What does it do? According to its name, it creates an empty image. But Bernie's impression is that it just confuscatedly creates a white image.

Oh, wait, that's racist! So Kevin was very intelligently hiding that fact in such convoluted code.

Kamil Podlesak

@BernieTheBernie said in CodeSOD collection:

What does it do? According to its name, it creates an empty image. But Bernie's impression is that it just confuscatedly creates a white image.

Oh, wait, that's racist! So Kevin was very intelligently hiding that fact in such convoluted code.

Is it really white? It creates monochromatic image with all pixels being "255", but... that value is a palette index, isn't it? And the palette is never initialized, so... what is the actual color of "255"?

Unless, of course, the value of PixelFormats.Indexed1.BitsPerPixel is bigger than 1 and the values are actually full RGB values of some arbitrary color depth, used to initialize the palette... in which case, is the API and its documentation at docs.microsoft.com

dkf

@Kamil-Podlesak said in CodeSOD collection:

It creates monochromatic image with all pixels being "255", but...

It creates a monochromatic image with one bit per pixel. All bits are set. Also, compare with the sample code at:

dotnet-bot

BitmapPalette Class (System.Windows.Media.Imaging)

Defines the available color palette for a supported image type.

Kamil Podlesak

@dkf said in CodeSOD collection:

@Kamil-Podlesak said in CodeSOD collection:

It creates monochromatic image with all pixels being "255", but...

It creates a monochromatic image with one bit per pixel. All bits are set. Also, compare with the sample code at:

That does not answer the question: which bits? RGB bits? How many of them?

dotnet-bot

BitmapPalette Class (System.Windows.Media.Imaging)

Defines the available color palette for a supported image type.

Well, that one at least shows that there are 8 pixels per byte, which means that the values are actually palette index. It also means that the original "Kevin" code uses 8-times bigger array than it should.

Medinoc

@bobjanova said in CodeSOD collection:

@BernieTheBernie said in CodeSOD collection:

How does [ Task.Run(() => CheckBlah(someParameter)).Wait(); ] differ from
[ CheckBlah(someParameter); ]

In the simple case CheckBlah will be run on the current thread. In the Task.Run case, it will be run on another thread (one in the executor's thread pool, assuming a normal config), and the current thread will wait for it to finish (including waiting for it to get pipelined, if the thread pool is limited and busy).

It's hard to see any situation where that difference would be useful. You're still blocking the current thread, and you're also adding load into the thread pool and some OS thread switching/monitoring overhead. Your coworker almost certainly did not mean to do it. But there is a difference.

If the CheckBlah tasks does anything that can accept incoming events (like, say, show a modal message box) there is a big impact: The message box introduces reentrancy if it executes on the same thread (because a new event could come in while the message box is up, etc.). Using a task and Wait() would make it "truly blocking" for the calling thread.

I know this because, well... WTF confession time! I had that exact problem with a program for a college project. Of course, the reason I had message boxes in the first place was because of another sin: I was indulging in printf-debugging my network application with message boxes. As events would come, dialog boxes would pile up (leading to wackiness if closed out of order)... So I resorted to this method to make my boxes "truly blocking". Which of course, hard the unfortunate side-effect of making the main thread unresponsive.

I think I cleaned all that up by the end of the project (the program needed to work without requiring user intervention every second, after all), and there is a happy ending to this: I drew the lesson of never attempting to show any modal UI in response to events not in the user's control (like network events).

BernieTheBernie

@Kamil-Podlesak said in CodeSOD collection:

And the palette is never initialized, so... what is the actual color of "255"?

Well, the palette is initialized in BitmapPalette myPalette = new BitmapPalette(colors);, but with 3 colors only: red, blue, and green, as shown in the line before.

So, the color at index 255 is not defined.

True, a different WTF from what Bernie thought...
But a nonetheless.

dkf

@BernieTheBernie said in CodeSOD collection:

So, the color at index 255 is not defined.

It's a single bit per pixel (that's the meaning of PixelFormats.Indexed1) so the palette could only be using two colours. And it has three…

Kamil Podlesak

@dkf said in CodeSOD collection:

@BernieTheBernie said in CodeSOD collection:

So, the color at index 255 is not defined.

It's a single bit per pixel (that's the meaning of PixelFormats.Indexed1) so the palette could only be using two colours. And it has three…

I have actually read that three colors as a colorspace declaration. Guess that I should pay more attention, because declaring completely custom colorspace is probably not an option any existing API. Otherwise, we would already have frontpage story about image in BBBB color space.

boomzilla

@BernieTheBernie said in CodeSOD collection:

BlahViewModel.cs(418,30): error CS0168: Die Variable "ex"

Man, that's pretty harsh.

topspin

@boomzilla said in CodeSOD collection:

@BernieTheBernie said in CodeSOD collection:

BlahViewModel.cs(418,30): error CS0168: Die Variable "ex"

Man, that's pretty harsh.

https://www.youtube.com/watch?v=gaXigSu72A4

BernieTheBernie

@BernieTheBernie said in CodeSOD collection:

Because Kevin broke the Jenkins build on Friday afternoon...

Well, he fixed that during Tuesday.

And then, he broke the build again on Thursday:

"BlahViewModel.cs(24,21): error CS0414: Dem Feld "BlahViewModel.m_BlahCount" wurde ein Wert zugewiesen, der aber nie verwendet wird."

True, that compiles on his machine when using Visual Studio. Because that will read his project configuration (from the csproj file): he set "treat warnings as errors" to "none", but bad Bernie configured Jenkins to compile vial command line with "treat warnings as errors" set to "all" (oh, by the way, Innitech's coding guide lines require that in the csproj file...).

Let's wait and see how many days Kevin will need to get things fixed.

Medinoc

@BernieTheBernie GC.KeepAlive(m_BlahCount) instead of actually fixing the problem.

BernieTheBernie

Let's do things more complicated.
What do you think of

public class MyClass
{
    private MyClass() { }
    public static MyClass Create()
    {
        return new MyClass();
    }
    // snip
}

Exactly like that: no parameters, no further initialization, nothing. It does also not hide the concrete type (e.g. by public static IMyInterface Create()).

It's really much more convenient to call

MyClass x = MyClass.Create();

instead of

MyClass x = new  MyClass();

isn't it?

dkf

@BernieTheBernie said in CodeSOD collection:

It's really much more convenient to call

It's so much easier to reason about too, since observation can no longer tell quite at a glance whether the ref is null or not…

bobjanova

@BernieTheBernie Presumably a Pascal/Delphi programmer.

I actually do much prefer the convention in Delphi where the constructor is a static function of the class. Apart from memory management associated with creating the object, that's what it is; you can give constructor overloads sensible names; and you don't have to rename them if you rename the class.

dkf

@bobjanova said in CodeSOD collection:

I actually do much prefer the convention in Delphi where the constructor is a static function of the class.

There is the more exciting version, where new is really a dynamic method of the class object. Systems like Java have lame versions of this in their reflection system (Class.newInstance(); I don't know if C# has anything like this, but I'd be surprised if it didn't) but when you can't subclass Class that's not very interesting. Being able to subclass Class itself really turns a number of aspects of how object systems work on their head.

bobjanova

@dkf It's class.,ctor() in .Net. But afaik you can't do any weird shenanigans around object initialisation (you can trigger it by reflection, but not actually change how that process works) so it's hidden and not relevant.

dcon

@bobjanova said in CodeSOD collection:

you don't have to rename them if you rename the class.

Um. TheNewClass::MySensibleName (in C++ notation because that's how I roll)

sloosecannon

@dkf said in CodeSOD collection:

Being able to subclass Class itself really turns a number of aspects of how object systems work on their head.

I get uncomfortably tense when @dkf says things like this...

dkf

@sloosecannon said in CodeSOD collection:

I get uncomfortably tense when @dkf says things like this...

If you do this, then new Foo() becomes syntactic sugar for Foo.new() (typically with an additional constraint that the result is never null) and the class becomes, among other things, a factory for its instances. In the standard case, things work like what you expect now, but you can do other things too. For example, singletons can work by a subclass of Class that throws an exception when there is an existing instance of the class, and it is easy to enforce that:

// This is written in a mash up of several languages
class Singleton<T> extends Class<T> {
    private T instance = null;
    public T new(args...) {
        if (instance != null) {
            throw OhNoYouDont();
        }
        instance = super.new(args);
        return instance;
    }
}

Except that it becomes necessary to have additional syntax for saying what type of class you're making when you declare a class (which can default to the standard Class of course). It all gets a lot easier in languages that allow embedding sub-languages; that's rare in Java and C++ (in the latter case, the only embeddable language is assembler and it is painful) but much more common in others. C# has some of that with LINQ, but nothing like as much as you see in, say, Lisp, Ruby, ML, or Tcl.

BernieTheBernie

As everyone should know, parsing an enumeration is such a strange task that you should not expect a generic utility for that purpose supplied by the framework, or even constructable by a lazy programmer. So it's always necessary to write your own boilerplate code for every new enum you need to parse. Like Johnny:

private DeviceType ParseDeviceType(string _deviceType)
{
    switch (_deviceType.Trim().ToUpper())
    {
        case "LOGO":
            return DeviceType.Logo;

        case "S7200":
            return DeviceType.S7200;

        case "S7300_400":
            return DeviceType.S7300_400;

        case "S71200":
            return DeviceType.S71200;

        case "S71500":
            return DeviceType.S71500;

    }
    throw new ConfigurationException($"Invalid device type: {_deviceType}");
}

Oddly, C# allows you to do a simple

return (DeviceType)Enum.Parse(typeof(DeviceType), _deviceType, true);

My cow-orkers are really professional porgrammers.

dkf

@BernieTheBernie said in CodeSOD collection:

Oddly, C# allows you to do a simple
return (DeviceType)Enum.Parse(typeof(DeviceType), _deviceType, true);
My cow-orkers are really professional porgrammers.

It's a shame you can't do anything like:

return DeviceType.valueOf(_deviceType);

like you would in Java…

BernieTheBernie

@dkf Anyway, since that string _deviceType is read from the configuration, Johnny could have used the generic GetEnum<T> functionality of our configuration engine, which encapsulates that completely (including the ConfigurationException for tyops etc.).

topspin

foo(bar.operator()(0,3), bar.operator()(1,3), bar.operator()(2,3))

You obviously have no idea what you're doing.

BernieTheBernie

@topspin said in CodeSOD collection:

foo(bar.operator()(0,3), bar.operator()(1,3), bar.operator()(2,3))
You obviously have no idea what you're doing.

I don't understand it either.
But somehow 3 seems to be the number, as you call bar.operator() 3 times and then have your head hit the desk 3 times.

BernieTheBernie

We all know that multi-threading is hard. A thread could change the value of some variable, while a different thread wants to use it.
It gets harder when you do lazy initialization in a multithreaded context.

Bernie made sure the property was safely initialized:

    private IReadOnlyList<Tuple<int, int>> GetSupportedTemperatureRanges()
    {
        if (m_SupportedTemperatureRanges == null)
        {
            lock (m_EbusParameters.ParamsLocker)
            {
                if (m_SupportedTemperatureRanges == null)
                {
                    // lots of initialization code
                    // ....
                    m_SupportedTemperatureRanges = listTempRanges;
                }
            }
        }
        return m_SupportedTemperatureRanges;
    }

comes Johnny and "improves" it:

    private IReadOnlyList<Tuple<int, int>> GetSupportedTemperatureRanges()
    {
        if (m_SupportedTemperatureRanges == null)
        {
            lock (m_EbusParameters.ParamsLocker)
            {
                    // lots of initialization code
                    // ....
                    m_SupportedTemperatureRanges = listTempRanges;
            }
        }
        return m_SupportedTemperatureRanges;
    }

Why check again for null?

topspin

@BernieTheBernie said in CodeSOD collection:

@topspin said in CodeSOD collection:
foo(bar.operator()(0,3), bar.operator()(1,3), bar.operator()(2,3))
You obviously have no idea what you're doing.
I don't understand it either.
But somehow 3 seems to be the number, as you call bar.operator() 3 times and then have your head hit the desk 3 times.

It's a retarded way of writing foo(bar(0,3), bar(1,3), bar(2,3)), where foo is a function and bar is an object of type 4x4 matrix.