- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
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 ¯_(ツ)_/¯
Admin
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.
Admin
Agree with Joakim -- Sometimes it is the human who should be nullable....
Admin
The proper solution would of course be to return FILE_NOT_FOUND in case the boolean wasn’t set. 😆
Admin
/DO NOT point out that there are comments before mine./
Admin
You do know there are other comments before yours, don't you?
Admin
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.
Admin
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.
Admin
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.
Admin
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.
Admin
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{.....}
Admin
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).
Admin
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.
Admin
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.
Admin
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.
Admin
"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.
Admin
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.
Admin
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.
Admin
Does not matter, I'm the boss.
Admin
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.
Admin
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
Admin
Just use php. All booleans are nullable.
Admin
I agree. So Boolean logic really has four values: null, false, true, FILE_NOT_FOUND
Admin
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.
Admin
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.
Admin
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.
Admin
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.
Admin
Never a null: The checkbox state could be nil if there is no checkbox in the first place.
Admin
Actually it's possible on web and in other UI frameworks like .NET WinForms to have a three-state checkbox.
Admin
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.
Admin
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.
Admin
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')
Admin
static bool? setting; public static bool GetSetting() => setting.GetValueOrDefault();