• Jeff Jefferson (unregistered)

    This is an anti-WTF. Which I suppose makes it an actual WTF but in a good way.

    The team actually did the right thing in pushing the better solution. It may just be terminology but I'd have probably pushed another commit on top and released rather than rolling back but ¯_(ツ)_/¯

  • Joakim (unregistered)

    But the solutions have different functionality, don't they? If setting is null, Austin's solution will uncheck the checkbox, while the boss's "solution" will leave it as it was.

    Not that the boss's way of doing it is ever right, but just pointing out that the member, presumably, has been declared as nullable for a reason, and returning false by default might not lead to the desired function for this checkbox, and worse, may also conceivably break other, more important things in the application. (Well, they would already have been broken, but they would switch to breaking quietly.)

    Hopefully Austin checked that this wasn't the case, though. It does seem likely that the person who wrote the original cast-to-bool line had something like this in mind. But perhaps then the member shouldn't be nullable after all.

  • (nodebb)

    Agree with Joakim -- Sometimes it is the human who should be nullable....

  • Esther (unregistered)

    The proper solution would of course be to return FILE_NOT_FOUND in case the boolean wasn’t set. 😆

  • NotAThingThatHappens (unregistered)
    checkbox.checked = null ?? fr!st!
    

    /DO NOT point out that there are comments before mine./

  • Niels (unregistered)

    I must be missing some context but would removing the ? in the line "bool? setting" not be the better solution? it will default to false, the same as the return statement. It's a private variable (I assume it was also static? else how could the method reach it) so the impact of removing the ? can be seen just by looking through the class file and see how it's used. So possible unexpected impact? very low I think.

  • DQ (unregistered) in reply to NotAThingThatHappens

    You do know there are other comments before yours, don't you?

  • Brian (unregistered) in reply to Niels

    What we're missing is the entire context. I'm assuming there's some reason (not necessarily a good one) that it's declared as nullable; perhaps there's code somewhere else that depends on it being null. In that case, changing it to non-nullable would result in some compiler errors or hidden bugs without more careful analysis, and it seems to me that the culture of this particular shop discourages that sort of refactoring.

  • Nony Mouse (unregistered) in reply to Niels

    Right - the real WTF isn't the boss (who was panicking and trying to solve a problem quick - though having his first thought being "let's catch the exception and keep running" is a bit of an ooof). The real WTF is that the original dev didn't think that 2 values for a boolean were sufficient and needed to create a three valued boolean instead of either a) deciding to write their code for an actual boolean value or b) creating an enum type with the actual 3 values needed.

  • Riho (unregistered)

    If it is for checkbox then null value would be needed to indicate that the state is "undefined". By changing it into false will make the checkbox unchecked, which is not a good idea.

  • (nodebb)

    I'm unconvinced the coalescing is the real solution, either. There was a reason why it was made nullable bool. If null means false, the solution would be to change it to be just bool. On the other hand, if null means something other than false, then this "fix" introduces potentially a nasty bug elsewhere.

  • ZZartin (unregistered)

    I'm also in the camp that defaulting to false might not be desirable. Defaulting to false assumes the setting is used as if(true) {...} else{...} and not if(true){....} else if(false){.....} else{.....}

  • RussellWakely (google) in reply to Niels

    As it represents a setting, my first thought is that maybe "null" represents "not yet explicitly set" state (which.. might be useful knowledge at some point).

  • davethepirate (unregistered)

    It sounds as if the setting is uninitialized until it has a value, so the checkbox is really neither checked nor unchecked. That function should have a different return value instead of bool and the UI should handle it appropriately.

  • (nodebb) in reply to Nony Mouse

    It's been mentioned elsewhere, but a nullable boolean can represent a choice that hasn't been made yet -- neither true nor false, but "we don't know".

    I think your idea of an enum makes sense, though. It's a lot simpler conceptually and less likely to lead to complete crashes like this one.

  • cellocgw (unregistered) in reply to konnichimade

    Yep, that's what "NULL" is for. You created something to hold a value but it's still unknown. If you force this item to initialize as either TRUE or FALSE, you will have to add a whole checking routine somewhere to ensure the user actually specified a value and didn't ignore it. In the end, it's a question of whether the syste should boot up / initialize into a fully known default state or not.

  • Duston (unregistered)

    "As it represents a setting, my first thought is that maybe "null" represents "not yet explicitly set" state" You see this in Windows security settings for example. If it's neither enabled nor disabled, then take the value from further up the hierarchy. Even though it's Windows, it's a valid way of doing things.

  • Yikes (unregistered)

    The inconsistency is frightening. This is where it may be clearer to other jabronis working on the code if you just create your own class to represent a nullable boolean, rather than using clever notation which may be overlooked, as in this case. Then you could have a "isNull()" function for the callers that want to default to a value up the hierarchy, as well as a "boolValue()" function that defaults to false for the sections of code that really only care if it's explicitly set to true.

  • Duston (unregistered)

    And as another thought, what if this property is itself used in a coalesce? Forcing the value to true or false would cause that to fail as well.

  • NotAThingThatHappens (unregistered) in reply to DQ

    Does not matter, I'm the boss.

  • Lőrinczy, Zsigmond (github)

    Which is the best way to destroy information (i.e. to reduce the three possible values to two)? I'd say it has no good ways.

  • C&S Inflatables (unregistered)
    Comment held for moderation.
  • RussellWakely (google) in reply to Duston

    That's a good example use case : a null value then wouldn't override a parent setting of TRUE i gues s ?

    Though an enum - eg: BooleanSetting { ENABLED / DISABLED / NOT_SET } would be a lot clearer if that was true here

  • Ollie Jones (unregistered)

    Just use php. All booleans are nullable.

  • Carlrobert (unregistered) in reply to Esther

    I agree. So Boolean logic really has four values: null, false, true, FILE_NOT_FOUND

  • WTFGuy (unregistered)

    The introduction of nullable value types in .Net exposed a lot of sloppy thinking in code. And also created a lot of "quick, just make the warnings go away" code changes as various modules were recompiled with the new as opposed to old value semantics.

    I'd bet money that a thorough dig into the history would find that the hasty addition of the "?" nullable indicator on the declaration was the last time that code area was touched.

    This is not to criticise the idea of .Net's nullable value types. Nor the least-bad way they chose to phase those into the type system and the languages. But "least bad" still wasn't very good. It was good in a professional logical sense. But like the slightly self-driving cars now on the market, the cool hi-tech capability crashes badly against the incompentent and untrainable user base they're deployed against. Which is .Net's case is the bottom X% of the current crop of devs worldwide and the PHBs who anti-manange them.

  • MaxiTB (unregistered)

    This code is weird. It seems to me that for some reason an optional boolean value is persisted while in the application itself a mandatory boolean value is required. There is a contractual mismatch and the correct procedure in this case is to always support the property with more states, in this case the nullable boolean. And the caller code has to implement handling of an unset value, not the (wacky) getter method. But obviously you don't implement it by creating an useless exception scope.

    So yeah, both solutions are WTFs - first introduces a default value and the second one doesn't use the property HasValue in a condition if the value should be considered.

  • ram shah (unregistered)
    Comment held for moderation.
  • Neveranull (unregistered)

    Am I the only one who thinks that a checkbox can only have two values, checked or not checked? You can’t have a checkbox that is null, or uninitialized. Any code that allows a checkbox state to be nullable is a WTF.

  • Gnasher729 (unregistered)

    In Swift this doesn’t compile. You can’t just convert a Bool? To Bool. You can convert a Bool! to Bool, but then the ! explicitly means “Please crash if I’m nil”, and that would stick out in a code review since “Please crash” is usually not what you want.

  • Gnasher729 (unregistered) in reply to Neveranull

    Never a null: The checkbox state could be nil if there is no checkbox in the first place.

  • (nodebb) in reply to Neveranull

    Actually it's possible on web and in other UI frameworks like .NET WinForms to have a three-state checkbox.

  • Watson (unregistered)

    Since the GetSetting method has no knowledge of how the value will be used outside and the fact that the value being null might be useful information, the correct way would be changing the return value to bool? and checking each call of this method as to how to handle the "no value" case for each call.

  • davethepirate (unregistered) in reply to Neveranull

    Windows has had Tri-State checkboxes since at least 3.1. As I recall, the outside box is a lot larger to indicate that it is indeterminate, or maybe the whole thing was filled in with blue instead of a checkmark. I agree that it is not immediately intuitive.

  • Old Timer (unregistered)

    PHB was a python developer? That approach is idiomatic python, 'Ask forgiveness not permission'.

    (In python, 'idiomatic' is a good thing, implying the absence of 'code smell')

  • David Mårtensson (unregistered) in reply to Nony Mouse
    Comment held for moderation.

Leave a comment on “Nullable Booleans”

Log In or post as a guest

Replying to comment #:

« Return to Article