5

This came up in code review at work in context of PHP and @ operator. However I want to try keep this in more generic form, since few question about it I found on SO got bogged down in technical specifics.

Accessing array field, which is not set, results in error message and is commonly handled by following logic (pseudo code):

if field value is set
   output field value

Code in question was doing it like:

start ignoring errors
output field value
stop ignoring errors

The reasoning for latter was that it's more compact and readable code in this specific case. I feel that those benefits do not justify misuse (IMO) of language mechanics.

  • Is such code is being "clever" in a bad way?

  • Is discarding possible error (for any reason) acceptable practice over explicitly handling it (even if that leads to more extensive and/or intensive code)?

  • Is it acceptable for programming operators to cross boundaries of intended use (like in this case using error handling for controlling output)?

Edit

I wanted to keep it more generic, but specific code being discussed was like this:

if ( isset($array['field']) ) {
    echo '<li>' . $array['field'] . '</li>';
}

vs the following example:

echo '<li>' . @$array['field'] . '</li>';
MikeSchinkel
  • 234
Rarst
  • 153
  • 8

2 Answers2

9

It's an extremely bad habit:

  • Code is essentially un-testable: I can barely understand people that don't unit test, but altogether suppress errors?
  • It carries a performance penalty, something that's addressed in php5.4.
  • The operator will suppress critical errors, that lead to script exit (good luck finding that mistyped function name)

As for your specific questions:

Is such code is being "clever" in a bad way?

Yes it is, it's a prime example of being "clever". We're using a PHP_CodeSniffer pre-commit hook to detect such "cleverness".

Is discarding possible error (for any reason) acceptable practice over explicitly handling it (even if that leads to more extensive and/or intensive code)?

There is only one acceptable scenario for error suppression, and that's when your intention is to catch the error and throw an exception instead. Especially when you suppress a third party or undocumented function.

Is it acceptable for programming operators to cross boundaries of intended use (like in this case using error handling for controlling output)?

I've seen error suppression being used instead of isset, as discussed in this article. The performance penalty alone makes it a bad practice.

Update: It seems that MikeSchinkel took the time to verify if there's actually a performance hit when using error suppression, and it's a fairly significant one*:

It is slower, but not in all cases; the case where it exists it can be faster. Where it is slower it can be up to ~5 times slower, but typically more like ~3 times slower

Any approach that's potentially 5 times slower than another approach, without adding any other value whatsoever, is of course counter-intuitive, to say the least.

* Extracted from the comments. The script is extremely inaccurate, as all tests are run on the same script, but it does give a general idea.

Community
  • 1
yannis
  • 39,647
6

I don't think this is acceptable for code deployed to production. In development and testing stages, this might be OK if there are errors you are expecting and want to avoid for now, but suppressing errors will eventually lead to important errors getting suppressed and at that point, debugging will get a lot harder because any error message that would otherwise alert you to something bad happening has been suppressed and you might not even know where to start looking.

FrustratedWithFormsDesigner
  • 46,335