Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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>';
share|improve this question
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 '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. –  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 <li></li>; a better example can be found here. –  MikeSchinkel Nov 17 '11 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! –  James Anderson Nov 18 '11 at 4:21

2 Answers 2

up vote 9 down vote accepted

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.

share|improve this answer
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 Rizos Nov 17 '11 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. –  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 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.

share|improve this answer
    
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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