Handling failed catches

Question

I know that is generally expected that you should not swallow exceptions. In this code, an Infragistic's UltraWinGrid is being configured. Is there a better way to handle the failed catches? Is this an exception to the rule?

private void HideExcludedColumns(UltraGridBase grid)
{
 if (_scExclusions == null) return;
 foreach (var strKey in _scExclusions)
 {
 //Set the appropriate column of the grid to hidden.
 foreach (var band in grid.DisplayLayout.Bands)
 {
 try
 {
 band.Columns[strKey].Hidden = true;
 break;
 }
 catch { } //go to the next band.
 }
 }
}

• almaz: Why do you think it should be an exception to the rule? What kind of exceptions can be thrown here, and in which cases?
• Antarr Byrd: I'm not sure what exceptions my be possible, other the column may not exist. But it did not write this code myself, and I'm not fully sure about how this control works.

Answer 1

I agree with almaz - a blanket catch is bad juju in just about every case. Need to know what exception(s) wind up being there. If the column doesn't exist, there should be a better, non-exceptional way of doing that:

 private void HideExcludedColumns(UltraGridBase grid)
 {
 if (_scExclusions == null) return;
 foreach (var strKey in _scExclusions)
 {
 //Set the appropriate column of the grid to hidden.
 foreach (var band in grid.DisplayLayout.Bands)
 {
 var columnIndex = band.Columns.IndexOf(strKey);
 if (columnIndex > -1)
 {
 band.Columns[columnIndex].Hidden = true;
 break;
 }
 }
 }
 }

• Antarr Byrd: Thanks, that looks great assuming that the only possible exception.
• Simon Whitehead: To expand: I am currently in the process of auditing all of the "catch-all"'s in a legacy code base at my work. It is one of the most painful things I have ever had to do. Catch-alls are badbadbad.
• Ryan Gates: As a general rule, your code should only catch what it can handle and either resolve or report appropriately.
• Kyle: @RyanGates Do you think there are cases where a catch-rethrow is valid and not bad practice? Eg: catch { logOrDoSomething(); throw; }. I find that some state information on logged on a debug level can be infinitely useful like this.
• Ryan Gates: @Kyle I think there are certain cases where it makes sense, but I would argue that they are few and far between. I have found that it holds pretty well as a general rule.

Answer 2

I'd introduce an assert in that catch block. That way whenever it gets entered during debug, the program will die and you can see what exception is being thrown. In production, the assert will not apply and the current behavior will remain. Then as you find exceptions being caught there, add code to handle them properly.

• Sandeep: It is a good suggestion but it doesn't cover all scenarios. Murphy's law states exactly that. In production you may run into exceptions that you didn't encounter during development. It is better to have a fallback mechanism to deal with such cases.
• Winston Ewert: @Sandeep, and what fallback mechanism would you add here?
• Sandeep: Fallback mechanism might be a wrong choice of word. But after one has changed his code to handle all the possible exceptions, the rest of the code should be written as given in the previous answers, which is do not handle the error (in case of an library) or have global handlers for unhandled exception or thread exceptions.
• Winston Ewert: @Sandeep, fair enough. I only suggested this approach as a transitional step to keep the current behavior until you're sure that you've identified all the cases it was detecting.