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>';
4
  • 4
    This reminds me of 1990s VB with On Error Resume Next. If something goes wrong don't ignore it. More serious errors await if ignored.
    – Jon Raynor
    Nov 17, 2011 at 21:51
  • Rarst asked this question without showing code examples after an offline debate with me and then expanded into a general use-case discussion which will cause people to answer the general case and not focus on the specific case so I posted another question about that specific use-case. Nov 17, 2011 at 22:46
  • @Rarst - The example you added was what triggered our debate but not what I would offer as a good example since we wouldn't want to emit <li></li>; a better example can be found here. Nov 17, 2011 at 23:24
  • @Jon -- this was sort of acceptable in VB as it was the only way to trap exceptions. The "On Error Resume Next, do something that might fail, On Error GoTo 0, Check for failure" pattern is roughly equivalent to "Try ... Catch" in better languages. Except for being massively error prone! Nov 18, 2011 at 4:21

2 Answers 2

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.

8
  • 2
    Great point on performance penalty for my specific case, thank you.
    – Rarst
    Nov 17, 2011 at 21:58
  • @Yannis - Exactly how much performance penalty? Have you profiled it? Nov 17, 2011 at 22:45
  • 1
    @MikeSchinkel Exactly as much as it takes for the triggering of the error. Suppression doesn't mean the error doesn't happen at all, the exact time depends on the type of error. You can easily profile it yourself and there are some metrics on the links I have in the answer. But it doesn't matter, it's a bad practice as it offers no considerable value compared to the alternatives.
    – yannis
    Nov 17, 2011 at 22:48
  • @Yannis - You are answering in abstract. It does offer considerable value for the one use-case that spawned this question. There is less duplication of code, hence fewer potential typo errors, and it's much easier to read at a glance. Nov 17, 2011 at 22:52
  • @YannisRizos "sacrificing code quality for readability" Seriously, you want to claim that is a given and not your opinion? I assert that code that has less chance of failing is higher quality, and typos contribute to failed code. If the performance issues are negligible, then there is not downside to using @$array['key_not_there'] assuming 'key_not_there' is an optional element for $array. (Sorry to see you are willing to give your opinion but not interesting in having your opinion challenged.) Nov 17, 2011 at 23:12
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.

6
  • For the context this is intended for production code. Also the part of justification in this specific case is that array field cannot really produce error other than expected one (since it's not a function call or something like that).
    – Rarst
    Nov 17, 2011 at 21:31
  • @Rarst: I would still say to write a proper error handler - of course if it's not actually possible for the error to occurr, why is this being done in the first place? Also, there could be a temptation later to add code into the "suppressed error" section. It could also lead to bad habits down the road, especially if new young coders see it and think it's a great way to make bugs disappear. ;) Nov 17, 2011 at 21:37
  • this is being done because in this case it is shorter code to ignore error than to check for it. Agreed on young coders, I wonder if this is sufficient factor to stay away from it just for educational/reputational aspect. It's not "not possible to occur", more like "not possible to occur anything that we care about". Still feels wrong as for me, but... Objectively it is functional code.
    – Rarst
    Nov 17, 2011 at 21:42
  • @Rarst: just curious: how much code was saved this way? Nov 17, 2011 at 21:43
  • roughly separate two-liner if block got converted into concatenated string component (per each instance of it used, which is not widely at moment).
    – Rarst
    Nov 17, 2011 at 21:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.