• TheCPUWizard (unregistered)

    The comments you provided are for the purpose of generating Documentation. Use a proper program, and compare the documentation output with and without those comments. Their purpose is not really to be used at the point where the person also has the code in front of them. The advantage of placing them at the start of the method (or class or..) is the hope (sometimes vain) that by having the material "right there" it is more likely to be kept up to data than if it was stored in a distinct location.

  • (nodebb)

    Which, as an aside, they're using IsFieldSet because zero-based indexes are too hard

    No, it's because the standard itself numbers the bits from 1 to 64, and it's better, in the standard-specific layers of the code (i.e. the code that calls SetField(...)), to work with the bit numbers used by the standard.

    what happens if I want to set a higher order bit explicitly to 0?

    My best guess is that if the bitmap isn't "extended", the higher bits are implicitly zero, and setting them explicitly to zero is therefore redundant. And it's possible that the spec forbids us to send an extended bitmap where all the extended bits are zero.

    So the actual WTF is the failure to use correct bit-banging procedures, instead relying on Pow(...) to calculate the bits.

  • Sauron (unregistered)

    There be dragons below

    That comment is not wrong, but inaccurate. There actually be arrays of dragon scales.

  • (nodebb)

    This is why experience matters, up to the point where you forget what you did before you had experience, and experience it all over again.

  • (nodebb)

    I'm so not going to start another fun benchmark competition here. Just for all wondering: The BinaryFormatter is removed since .net 7 (technically is marked as obsolete with an error, so you can no longer compile code using it), so that solution wouldn't even work in modern .net versions.

    Addendum 2024-03-07 09:53: @Remy Of course .net has a native 128-bit value type (both signed and unsigned): https://learn.microsoft.com/en-us/dotnet/api/system.uint128?view=net-8.0

  • Jim Jam (unregistered) in reply to MaxiTB

    Not mentioning that .net has native BitArray type of arbitrary length supporting bitwise operators out of the box. https://learn.microsoft.com/en-us/dotnet/api/system.collections.bitarray?view=net-8.0

  • Paati Sooth (unregistered)

    As someone who has had the pleasure of working with ISO 8583 recently I think I can comfortably say that this is nowhere near as horrific as other ISO 8583 parsers I've seen

  • OldCoder (unregistered)

    There indeed be dragons below! I can see nothing in that function which sets the value of i, used in the final loop.

  • (nodebb) 
    public static class ABitOfAConfessionMaxiTbExtensions
{
    //--- Public Methods ---//

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool IsExtendedBitmap(this UInt128 self) => 
        (self & 0x1) > 0;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool IsFieldSet(this UInt128 self, int field) =>
        (self & (two << (field - 1))) > 0;

    public static void SetField(this ref UInt128 self, int field, bool on)
    {
        if(on)
            self |= two << (field - 1);
        else
            self &= ~(two << (field - 1));

        if ((self & highNibble) == 0)
            self &= 1;
        else
            self |= 1;
    }

    //--- Private Fields ---//

    private static readonly UInt128 two = 2;
    private static readonly UInt128 highNibble = ~((UInt128)ulong.MaxValue);
}

    The ToMsg part is so weird, I'm not going into that one.

  • (nodebb)

    To me, the worst part of this whole thing is that the code requires the caller to know some of the encoding rules. Bit 1 means that bitmap for fields 65 - 128 are present, so SetField(67, true) only works if the user also does SetField(1, true). It would be fairly trivial for the code to set bit 1 internally when it produces the message.

  • (nodebb) in reply to Jaime

    No, because if you do SetField(67, true), there will be an implicit SetField(1, true) done by SetField itself.

    Addendum 2024-03-07 12:47: That's what the for() loop inside SetField is for.

  • (nodebb)

    I have one objection. This appears to be a class for handling a bitmap. While it's being written for a purpose the author probably tried to make it reusable--hence naming the internal array "bits" seems quite reasonable to me.

    I also think that the games with the first bit are imposed by an external requirement. Whatever's being encoded here uses the first bit to select between a 63 bit structure and a 127 bit structure. Such cases are all too common in the real world when something outgrows it's original spec.

  • Your Name (unregistered)

    The ISO8583 defines a binary message structure composed of 64 or 128 (extended) fields. Up front is a flag set which defines which fields (aka Bitmaps) exist. Bitmaps themselves are numbered 1 to 64, or to 128, pasted straight after each other, of either fixed length, variable length, or, unfortunately sometimes length depending on the value of a previous bitmap in the message.

    It would be majorly confusing if you want to set BMP4 (which is transaction amount), but call it with SetBitmap( 3, ..)

    The code is still somewhat weird, and using Math.Pow is a major red flag.

  • Gavin (unregistered) in reply to MaxiTB

    Now this is the sort of code that I hope I would have written. Except for that glaring bug which would wipe out all your bits. :D

  • MaxiTB (unregistered) in reply to Gavin

    Oh there is actually mutliple bugs, I just wrote it down while thinking about it. First off obviously two needs be one; classic brainfart of thinking power two while you shift the first bit. The second one is the last end mask, which should be UInt128.MaxValue - 1; I guess you were refering to that one. Third "bug" is using the wrong naming convention of member fields. Forth would be initializing _highNibble with UInt128(ulong,ulong) instead of using operations. I bet there is more in it, but that's what I saw reading my code again ;-)

  • MaxiTB (unregistered) in reply to Paati Sooth

    https://github.com/CBidis/CSharp8583/tree/master

    ;-)

  • (nodebb)

    One of the things that continues to surprise me is knowledgeable commenters who assume legacy code will have been written using language / API versions that are the latest and greatest.

    Our confessor John did not write this abomination last week. C# and .NET Framework v1.0 date from early 2002. I wrote a lot of C# in v1.0. A bunch of which is still (probably) running today.

    The UInt128 struct was added to .NET Core (now just ".NET") in v7, which was released in late 2022. Good bet this code is older than 18 months. It might be old enough to vote. It's almost certainly old enough to drive.

    Now conversely, the BitArray class dates from the very beginning: .Net Framework v1.0.

  • (nodebb) in reply to WTFGuy

    No it wasn't, BitArray was introduced in .net 1.1.

    But that's beyond the point, even 1.1 was over 20 years ago, that's 200 years in human years. There's no reason to stick with ancient code like that, back then the book of Moron wasn't even published.

    And .net 7 is not new either, it's also nearly 20 years old in human years. The US just had invaded Iraq for no reason back then :-)

    And even if you go back to .net Beta (which you were part of like me for sure), ulong was around since the beginning. And there's no excuse using a reference type over to value types in this case (what UInt128 is anyway cause there are no .net platfrom versions which support 128bit processors ATM).

Leave a comment on “A Bit of a Confession”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article