Added an enumeration and abstract property to denote the message type…

[SteveK-GS]

… of the NmeaMessage so as to avoid all that unsightly and unnecessary casting

[dotMorten]

Thank you for your PR.

Why is this insightful? You'll still have to cast in the end. A type of check isn't so bad. An enum would also prevent extensibility.

[dotMorten]

Errr unsightly not insightful 😀

[SteveK-GS]

In a worst case scenario, you have to cast to every known type derived from NmeaMessage before you have the concrete type you need. If a type property is encapsulated, we just need a single cast - seems to me basic good design?

This ugly mess:

if (nmeaMessage is UnknownMessage)
        {
            return false;
        }
        else
        {
            var gprmc = nmeaMessage as Gprmc;

            if (gprmc != null)
            {
                if (processMinimumRecommendedDataAction == null)
                {
                    processUnsupportedMessageAction(nmeaMessage);
                }
                else
                {
                    processMinimumRecommendedDataAction(gprmc);
                }
            }
            else
            {
                var gpsgv = nmeaMessage as Gpgsv;

                if (gpsgv != null)
                {
                    if (processSatellitesInViewMessageAction == null)
                    {
                        processUnsupportedMessageAction(nmeaMessage);
                    }
                    else
                    {
                        processSatellitesInViewMessageAction(gpsgv);
                    }
                }
                else
                {
                    var gpgga = nmeaMessage as Gpgga;

                    if (gpgga != null)
                    {
                        if (processEssentialFixDataAction == null)
                        {
                            processUnsupportedMessageAction(nmeaMessage);
                        }
                        else
                        {
                            processEssentialFixDataAction(gpgga);
                        }
                    }
                    else
                    {
                        var gpgsa = nmeaMessage as Gpgsa;

                        if (gpgsa != null)
                        {
                            if (processOverallSatelliteReceptionDataAction == null)
                            {
                                processUnsupportedMessageAction(nmeaMessage);
                            }
                            else
                            {
                                processOverallSatelliteReceptionDataAction(gpgsa);
                            }
                        }
                        else
                        {

can become:

  switch (nmeaMessage.NmeaMessageClassType)
        {
            case NmeaMessageClassType.UnknownMessage:
                return false;

            case NmeaMessageClassType.Gprmc:
                if (processMinimumRecommendedDataAction == null)
                {
                    processUnsupportedMessageAction(nmeaMessage);
                }
                else
                {
                    processMinimumRecommendedDataAction((Gprmc)nmeaMessage);
                }
                break;

            case NmeaMessageClassType.Gpgsv:
                if (processSatellitesInViewMessageAction == null)
                {
                    processUnsupportedMessageAction(nmeaMessage);
                }
                else
                {
                    processSatellitesInViewMessageAction((Gpgsv)nmeaMessage);
                }
                break;

            case NmeaMessageClassType.Gpgga:
                if (processEssentialFixDataAction == null)
                {
                    processUnsupportedMessageAction(nmeaMessage);
                }
                else
                {
                    processEssentialFixDataAction((Gpgga)nmeaMessage);
                }
                break;

            case NmeaMessageClassType.Gpgsa:
                if (processOverallSatelliteReceptionDataAction == null)
                {
                    processUnsupportedMessageAction(nmeaMessage);
                }
                else
                {
                    processOverallSatelliteReceptionDataAction((Gpgsa)nmeaMessage);
                }
                break;

            default:
                processUnsupportedMessageAction?.Invoke(nmeaMessage);
                break;
        }

You're right about extensibility - I hadn't thought of people inheriting from NmeaMessage or indeed from unsealed NmeaMessage-derived classes outside of the library - they won't be able to add new members to the enumeration. Doh! I'd resolve that by adding an additional enum member in the library - call it something like Custom. If the type is Custom, the consumer will have to fall back to casting or whatever identifying properties they have included in their own derived classes.

[dotMorten]

Just use is for type checking rather than an enum:

if(nmeaMessage is Gprmc)
{
   Gprmc rmc = (Gprmc)nmeaMessage;
}

The problem with the enum is it doesn't handle abstract types / inheritance nor is it extensible with more message types without also updating the enum.\

In my mind having an enum type property have several problems:

1. It's a breaking change (showstopper)
2. It adds more things to add to a message type that quite frankly already can be done with type checking
3. There's no way to check if it's 'LaserRangeMessage' abstract type but you need to check for every single concrete subtype, even though you probably don't care about the proprietary extra properties.

[SteveK-GS]

I can see I'm probably not going to win here Morten, but let's have a crack anyway :)

1. It's only a breaking change for anyone who has derived classes external to the library, and can be made non-breaking by making the property virtual with the base class implementation returning new enum member 'Unknown' (which should be 0). Not perfect, admittedly.
2. The consuming code is so much cleaner with this property available
3. Consumers are not obliged to use the new property, they can still check 'is LaserRangeMessage' if they wish. Alternatively, override my property in LaserRangeMessage to return enum value 'LaserRangeMessage ' and remove the overrides from the derived classes; optionally add to LaserRangeMessage a new LaserRangeMessageType property.

I'm coming with the viewpoint that casting is smelly and encapsulation is good. The deep if/else tree and multiple casts I found myself having to write just look wrong. I understand your objections however, as already noted :) I think the main problem is that in the last OSS project I used this technique in all of the classes involved were internally scoped and ultimately sealed...

Irrelevant to whether you accept the pull request or not, but your example with 'is' is casting twice. It's better imho - although more verbose - to use 'as' and then check for null.

No need for anybody to feel bad if you disagree. I have my fork with this change in for our own use.

Thanks for the discussion!

[dotMorten]

It's not about 'winning' anything here. I truly appreciate the work and feedback. But generally it's better to start out with logging an issue with user-story, before going here. A breaking change is not something I take lightly, and would need a very convincing argument. The fact that there's also remaining concerns with this approach, unfortunately means I'm going to have to reject this. The great thing about open source is you can fork all you want, and also not worry about breaking changes. In fact I very much encourage forking, but need to keep some stability in the main branch.