- 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
Ah, the old classic "nobody is paying us to fix this, so it stays broken" logic.
Admin
Er..........I'm pretty sure I understand what the ternary is trying to do and I'm also pretty sure the "fix" will potentially break things under certain conditions...
Admin
... because the check isset($categoryMap[$department]) is missing. This 'refactor' doesn't retain the original logic, potentially breaking things entirely unrelated to the bug report.
Admin
Yep, I think the same if the superficial similarity to Perl's exists query implies the same consequences in PHP. In Perl
exists($someHash{$keyA}{$keyB})
would end up creating and undef entry $someHash{$keyA} and then try to dereference the undef as a hash, resulting in an error. Not really a fix but perhaps someone with more knowledge of PHP can enlighten the rest of us here.
Admin
That's a horrible fix. While the original code is certainly ugly, if it's correct, the new code isn't as it doesn't do the same checks.
Admin
isset($categoryMap[$department][$classification])==true implicates isset($categoryMap[$department])==true, so the replacement is perfectly fine.
Admin
I thought the original was clear and nicely terse. Not a huge fan of nested ternary but I certainly didn't struggle to understand it.
Admin
isset() handles that case and can be safely used to check multi-dimensional arrays.
Admin
My bad. I should learn to read. The comment in the article clearly states that this behavior is a PHP quirk.
Admin
Nope.
Admin
I wonder if the fact that in PHP, the ternary operator is left associative, has any effect on the logic of the original...
Admin
so you would want isset to throw errors when key is unset? i am sure that would be nice
Admin
Never trust ternary operators in PHP! (Or rather: Never trust PHP!)
will echo "5 is smaller than 4", because PHP.
Admin
At least there's ternary operators in PHP. Ever seen the wtfery you have to do in python?
Admin
"Quirk", you say? It evaluates fine the moment surround the nested statement with brackets like PHP documentation tells you to - $result = (5 > 3) ? "5 is bigger than 3!" : ((5 < 4) ? "5 is smaller than 4!" : "4 is smaller than 5!");
Admin
there is a ternary expression in python since V2.5
a= b if <condition) else c
A bit word but self explanatory
Admin
I would've just replaced it with a single ternary. But then, I come from C.
Admin
Really? Ah yes, so I see from refreshing my PHP knowledge of isset.
That is truly horrible, since if you've used just about any other language you'd expect some_map[non_existent_index_a][non_existent_b] to cause an indexing error at the first step...
Admin
Precedence, precedence, precedence...
Admin
This is reasonable.
This is not.
Admin
YOU NEVER CHANGE CODE THAT IS NOT BROKEN! EVER!
God. Why? I've been doing this 16 years now and I've had this crap bite me in the ass a few times. I'd take code that I could not read and fixed it like this just to break some fringe case. LEAVE IT ALONE!
There is a reason all the "old guys" are scolding you.
Admin
No, it's the old classic .... "nobody is paying us to fix this, so it remains working" logic
3 outcomes are possible:
Which is the only scenario that can result in broken code?
Admin
Well, if you run without "use strict" you might get by with a silent autovivification and no error but that's not preferrable in my opinion.
Admin
In any sane language you don't need any parentheses to simply read and understand what should be going on. :P If the first statement evaluates to true, why should you care what happens in the "else" case? That's right, you shouldn't.
Admin
I never said PHP was sane (I wouldn't be sane if I did!), but if it's in the documentation then it's hardly a quirk, it's deliberate behavior (for better or for worse)
Admin
That's a fantastic way for the entire code base to rot into an un-maintainable mess. "No one can parse out what it's trying to say, but it's generating the right output for now, so let's never, ever touch it." This is a great way to trap yourself in a tangled web of unreadable garbage.
Messy code is broken. Obviously you don't want to make reckless or unnecessary changes, but properly defining your requirements and documenting and testing your code are all there to allow you to make changes when you need to.
Admin
Regardless of what your particular Software-cum-psychiatrist labels as a sane vs. an insane language, you should ALWAYS ALWAYS use parentheses. For one thing, it's even better than proper commenting as a tool for making your code comprehensible to a wide audience, not just the anointed few who are experts in the chosen language. For another, one day soon you will be a little sleepy and write something with the wrong default precedence and spend the next 2 weeks tearing your hair out trying to find the bug. Use parentheses. It doesn't cost anything at runtime.
Admin
Admin
Even with use strict, that code will run just fine, returning false, but also having that annoying side effect mentioned up-thread.
Admin
True (although only true for nested ternaries) because PHP takes the low ground on operator associativity in this case and is basically the only language that gets the associativity the wrong way round. I can't guarantee that it hasn't been fixed since PHP5 or so, but I'd doubt it, because once you mandate an associativity, you're pretty much stuck with it.
False, because without knowing more details, we don't actually care about the associativity. Either the original code worked, or it didn't. Well, I say "we don't care," but actually we do, so the apparently rational refactoring might actually change behavior precisely because of PHP's broken associativity.
Also false, because the refactoring is broken, as the earlier commenters point out.
Really, the only reason to object to the original code (provided your mandate is to write PHP code) is that the indentation is completely obfuscated. Fix that so that it more closely resembles an if-else construct, which after all is what a ternary operator does, and you're copacetic.
If I were this young man's boss, I would tell him to revert the change, apply only whatever business requirement was on the ticket, ... which appears to be what these supposedly evil bosses did .... and to go away and write a whole bunch of unit tests. Because unless you have a reasonable degree of confidence that fiddling around with tiny largely unimportant details won't change the logic, then you simply do not fiddle around with tiny largely unimportant details.
Not the most impressive submission we've seen, if I may summarize the general opinion of commenters so far.
Admin
"Drive By Coding" is evil. The change should have been rejected as part of an unrelated ticket. THat being said, there should be a light weight means of producing the necessary "ticket" so that the code could be changed [and thus validated independently]
Admin
The submitter clearly showed their naivete here, for making this change without a ticket assigned. He should have related it to the ticket he was currently working on, and then none would be the wiser.
Admin
On a more general note: if this is a "representative line," it's hard to see precisely what amorphous WTF entity it is supposed to "represent."
Other than pissing around with working code to no good purpose, of course. But I imagine that wasn't actually the intent of the post.
Admin
Just a note... the new code has a bug in it, PHP's isset provides protection and checking only for the last level of indirection reduced. An acceptable solution might have been
$categories = @$categoryMap[$department][$classification];but where I've been we like to avoid relying on the silence operator, so either write your own recursive isset checker or do each one by hand (unless, of course, code not seen here is making it impossible for that first level to not exist.Admin
The real wtf here is the comments:
issetmakes a lot of sense, or at least it's a very useful construct.Oh and the simpler fix would be (since PHP 7):
Admin
That's not true. When you use isset, it checks all the levels of the array without issuing a warning.
Doing this is fine:
isset($a[$b][$c])even if $a[$b] is not defined. What you might be thinking of is this:Admin
Do you want Lisp? Because that's how you get Lisp.
Admin
For what little PHP I know, I checked isset and the new version of code looks legit and should behave the same as the prior version. Unless I am missing something here? PHP stops execution at the first case that it can't find something at a level in the multi-dimensional array and returns false.
Regardless, all this hate for ternary operators and this looks just fine to me... Again, I'm not a regular PHP dev so sorry if I eff'd up some syntax:
$department = $product['department']; $classification = $product['classification']; $categories = isset($categoryMap[$department][$classification]) ? $categoryMap[$department][$classification] : null;
what's so bad about that..?
Admin
... if you are a wimp who is too afraid of being yelled at to do your job.
I'm one of the "old guys" who simply toughened up and isn't afraid of being bit.
If you do your job and can show that you had a good reason for the changes you make and that you exercised reasonable precautions, you won't get fired. Moreover, you won't be perpetually afraid of doing your job.
Admin
Assuming your "$department" is meant to be "$product['department']", etc., then the new code is just as valid as the original - neither check that $product['department'] and $product['classification'] both exist.
While '@' will suppress the error report, error handlers will still run, so it's best avoided if feasible.
So, as long as we're playing golf....
$categories = $categoryMap[$product['department']][$product['classification']] ?? null;
Admin
Well, don't refactor in production. But it's a perfect small fix for the next release with its proper test cycles, so just add it to the backlog.
Admin
If you're in a situation where you're afraid to change code then it sounds like you're working on a Big Ball of Mud. Fix that BBOM situation or find a job that doesn't involve working on BBOMs.
Admin
Unless, of course, $someHash{$keyA} is something other than a hashref then it'll be an error. (If it wasn't defined it'll autovivicate)
Addendum 2018-01-16 06:30: Why didn't that show "in reply to"? I was replying to the Perl thread
Admin
I've done this a few times when I brain farted and forgot how isset works which is a bit strange as a language construct, although a ternary is madness. It's common to also overlook that isset returns false for entries that are set but set to null. It gets mildly weirder in scenarios where given [a, b, c], a or a, b must be set but not c.
If you turn notices, errors, warnings on in PHP, the error reporting to full that is, then convert all errors into exceptions then that captures a hell of a lot of bugs. It's a bit like use strict in JS. This also means putting explicit checks in a lot of places to avoid notices.
Unfortunately the language isn't quite written to support that fully and in a few areas you have to put in kludges. The null coalescing operator improves things.
PHP has loads of internal error checking that you don't need to do yourself on things like IO where in most cases you simply want everything to fail/stop on an error. Most checks in real life are sanity checks for errors you can't really very gracefully handle in the local or a near scope. You still have cases with things such as when using socket, where states can be represented by errors. Then you have to use the @ operator to suppress the error and handle it yourself.
As PHP is a dynamic language (which means you save a huge number of lines of code with runtime polymorphism). This makes it important to have or turn on those checks. Otherwise it's like compiling a complex C program with the minimum warning level, etc.
Admin
It wouldn't surprise me if the original code has some values instead of just nulls for the 2 fail cases, in which case it would make a ton of sense. Regardless don't change working code just because you find the syntax hard to read.
Admin
TRWTF is the "cleverness" of the ternary, with all the language-specific side-effects et al. Apparently no one has read: https://www.simplethread.com/dont-be-clever/ or the multitude of good coding practices that practically scream the same.
Good code is self-documenting. There is NOTHING self-documenting about that ternary horror.
Unless you're writing real-time code, you always can and should make it more reader-friendly. Because code sticks around longer than you.
Admin
" It's common to also overlook that isset returns false for entries that are set but set to null." It's easy to avoid this mistake if you regard null as indicating the absence of a value. Then, isset is a test to see if an entry (an array element in this case) has a value (i.e., "is set") or not.
Thanks to the runtime polymorphism you mention, an entry can fail to have a value for two reasons: it might be null, or it might not exist at all. isset() is written to cover both cases, while other constructs exist to provide more refinement if it's needed.
It might be argued that the two cases should be semantically identical: either "absent entries evaluate to null" or "null assignment destroys lvalues". The first would silently swallow errors due to accidentally using the wrong array keys or object properties; the second would be a shotgun blast to the symbol table: anything that MIGHT be null would have to be explicitly checked before use, and code branched to handle both cases (no, you can't just assign a default a value and proceed - the whole point of nulls is that might not be any appropriate value to use).
What PHP did was "absent entries evaluate to null, but you're notified when it happens because you're probably doing something wrong".
'Course, then you get people writing isset($localvariable) because they can't work out if they initialised the variable or not.
Admin
If the syntax is hard to read then it's not doing its job. Source code is supposed to be readable.
Admin
I've also worked at places where code remained un-refactored because there was now ticket to fix it.
One of the reasons for this was that if you refactored something, you needed to prove that you didn't break anything. Proving you didn't break anything required that QA test it. Getting QA to test it required a ticket that could be signed in triplicate, sent in, sent back, queried, lost, found and then promoted to the QA server.
The red tape could've been cut with unit tests, but naturally we didn't have those either.
Admin
The most amusing thing about this thread is that someone used the behavior of Perl to model PHP behavior. PHP was invented specifically to be "Perl but somewhat sane." While there's still a couple of Perlisms in PHP, almost all of it has been relegated to the sanitarium to play checkers with Ada and IPL.