Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.
        }
    }
}
share|improve this question
1  
Why do you think it should be an exception to the rule? What kind of exceptions can be thrown here, and in which cases? –  almaz Jan 18 '13 at 14:49
    
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. –  Antarr Byrd Jan 18 '13 at 14:59
add comment

2 Answers 2

up vote 12 down vote accepted

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;
                }
            }
        }
    }
share|improve this answer
    
Thanks, that looks great assuming that the only possible exception. –  Antarr Byrd Jan 18 '13 at 15:49
    
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. –  Simon Whitehead Jan 23 '13 at 4:16
1  
As a general rule, your code should only catch what it can handle and either resolve or report appropriately. –  Ryan Gates Jan 23 '13 at 21:35
    
@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. –  Kyle Dec 19 '13 at 4:32
1  
@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. –  Ryan Gates Dec 19 '13 at 14:30
add comment

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.

share|improve this answer
    
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. –  Sandeep May 29 at 9:41
    
@Sandeep, and what fallback mechanism would you add here? –  Winston Ewert May 29 at 15:03
    
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. –  Sandeep May 30 at 6:41
    
@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. –  Winston Ewert May 31 at 20:16
add comment

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.