• Komodo Dragon (unregistered)

    m_ConnectionService?.Position?.IsFrist ?? false

  • (nodebb)

    The post doesn't mention the age of the code and depending on which state you look at (draf, candidate, etc.) WebRTC spec is older than both null propagation (C# 6.0, July 2015) and null-coalescing (C# 8.0, September 2019) so depending on the age of the code those features might not have been available. The alternative would have been 3 null checks which is a lot more verbose and doesn't add to the readability of the code. The other question is wether IsWebRTCConnected is a property and if so whether the getter is just a simple get => value or something more complex and capable of throwing exceptions. I know that's not how getters are suppossed to be used but ultimately they're just sintactig sugar for a get_IsWebRTCConnected() method and you can do pretty much anything there. I've seen some really stupid stuff done inside a getter, because give a monkey a hammer and you know what happens.

  • Hmmmm (unregistered)

    I might also rename the function to "isCallActive", since "calling" seems like a superset of {dialing, ringing, handshaking, allocating, etc. etc.}, presumably because by the time the "webRTC is connected" — as the code reads — all that should be sorted out. (emdashes added for good measure)

  • Toasty (unregistered)

    Without context, I would have to disagree with the "information should be managed via a state machine, not via boolean flags stuffed deep in an object chain" suggestion. Moving a boolean flag check deep inside a state machine? Yikes, no. Keep it simple. Maybe you shouldn't use exceptions as flow control, but I've seen a lot worse.

  • (nodebb)

    The post doesn't mention the age of the code and depending on which state you look at (draf, candidate, etc.) WebRTC spec is older than both null propagation (C# 6.0, July 2015) and null-coalescing (C# 8.0, September 2019) so depending on the age of the code those features might not have been available.

    This may address the problem mentioned in the article, but not the "real problem". The real problem is that this code swallows all exceptions other than the handful of null reference exceptions it expects to occasionally encounter. This makes some future unexcepted problem many times more difficult to discover than if the code simply used standard logic to test for all of the indicators of "not connected", including null intermediate objects. The lack of a null-coalescing operator is no excuse for writing bad code.

  • (nodebb)

    Not knowing the code base, but just by the naming convention, I'd posit that m_ConnectionService is a nullable property whereas Core.State.IsWebRTCConnected are always existing structs inside.

    As for having a state machine, I think the State hints we already have one, and that boolean check IsX is exposing that state machine.

    Replacing a boolean by an enum is what leads to {Yes, No, FileNotFound}.

  • (nodebb)

    Maybe I'm old school but in my thinking returning "null" is a far more serious issue for a connection than "false." "Is the phone call connected?" "No" "Then dial 867-5309." As opposed to "Is the phone call connected?" "Phone? What's a phone?"

  • (nodebb)

    I don't see an issue with this code, in fact it looks perfectly as expected in this case.

    The dev is turning an state most likely from an unmanaged asynchronous API translated to a thin C# layer using accessors into an boolean state, which is a treasure chest of potential exceptions happening.

    I dun even see how nullability could be here a factor at all since C# has nullability warnings now for half a decade. And even if they are turned off, which oh god now that is a major WTF, would still be more a consequence of accessors mapping an unmanaged asynchronous API to a thin C# layer and then you just end up with one more exception to handle indirectly.

  • 516052 (unregistered) in reply to MaxiTB

    For a start because null is not always null. I've had situations with unmanaged APIs where the managed wrapper object you are interacting with in C# is newer null but the internal memory is unalocated.

    Honestly, if I had to guess how this code came about I'd wager the following:
    m_ConnectionService.Core.State is a managed C# wrapper around some sort of unmanaged service. IsWebRTCConnected maps to some sort of function call on that service. That service uses pointers in the background, most likely raw pointers. These pointers are always allocated during normal operations. However during startup / shutdown they can be in an empty state. And since startup/shutdown happens once per connection each the surrounding C# code was crashing out during what should have been normal operations. So this code was added as a failsafe to safely handle those situations. You can tell I had to deal with stuff like that.

  • (nodebb)

    @MaxiTB ref:

    I dun even see how nullability could be here a factor at all since C# has nullability warnings now for half a decade.

    Oh my. Probably 99% of corporate code in the USA is older than "half a decade". That might be an eternity ago for the people who create new .Net versions. But it's an eyeblink in the lifecycle of big IT systems. Which are not routinely refactored just to make use of new syntactic shortcuts.

  • MangusPI (unregistered) in reply to WTFGuy

    Probably 99% of corporate code in the USA is older than "half a decade".

    I really don't understand why some people like to think that the USA some how has the monopoly on that sort of thing when it is true in Europe and Asia as well (I've consulted with firms in both and sometimes it feels even worse.) Canada also.

Leave a comment on “Connection State”

Log In or post as a guest

Replying to comment #702909:

Log Out

« Return to Article