Is error suppression acceptable in role of logic mechanism?

Question

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>';

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 '11 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](http://programmers.stackexchange.com/questions/120391/is-error-suppression-a-valid-technique-for-testing-for-an-optional-array-key). — MikeSchinkel, Nov 17 '11 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 `
@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! — James Anderson, Nov 18 '11 at 04:21

score 9 · Accepted Answer · edited Apr 12 '17 at 07:31

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.}

edited Apr 12 '17 at 07:31

Community

1

answered Nov 17 '11 at 21:49

yannis

39,547
40
183
216

2

Great point on performance penalty for my specific case, thank you. – Rarst Nov 17 '11 at 21:58
@Yannis - Exactly how much performance penalty? Have you profiled it? – MikeSchinkel Nov 17 '11 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 '11 at 22:48
@Yannis - You are answering in abstract. It does offer considerable value for [the one use-case](http://programmers.stackexchange.com/questions/120391) 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. – MikeSchinkel Nov 17 '11 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.)* – MikeSchinkel Nov 17 '11 at 23:12
I've moved the [discussion into chat](http://chat.stackexchange.com/rooms/1808/discussion-between-yannis-rizos-and-mikeschinkel), no point for commenting here unless comments are relevant to my answer. – yannis Nov 17 '11 at 23:21
@YannisRizos - I'm have no interest in having a discussion in a place where the discussion is not attached to this question . The *"facts"* you reference are only 1.) performance and 2.) suppressing critical errors. I've acknowledged we don't know about performance so can't judge yet, but #2 is *completely irrelevant* for the use-case that we are discussing and is not necessarily "a bad idea" because reason #2 does not apply. Sorry you feel it's boring; curious that you continue to debate it then. – MikeSchinkel Nov 17 '11 at 23:34
Okay, so here is **[a gist](https://gist.github.com/1375073)** showing the benchmarking. 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, or 0.0000023 seconds slower. For high load, high performance real time systems maybe this is a concern, but it's just not significant enough for me to care about the extra effort required. – MikeSchinkel Nov 18 '11 at 00:22

score 6 · Answer 2 · answered Nov 17 '11 at 21:29

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.

answered Nov 17 '11 at 21:29

FrustratedWithFormsDesigner

46,105
7
126
176

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 '11 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. ;) – FrustratedWithFormsDesigner Nov 17 '11 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 '11 at 21:42
@Rarst: just curious: *how much* code was saved this way? – FrustratedWithFormsDesigner Nov 17 '11 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 '11 at 21:44
@FrustratedWithFormsDesigner - Here is the specific use-case: http://programmers.stackexchange.com/questions/120391 – MikeSchinkel Nov 17 '11 at 22:49

Is error suppression acceptable in role of logic mechanism?

2 Answers2

Linked