Tell me more ×
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 catch's are 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 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. – atbyrd Jan 18 at 14:59

2 Answers

up vote 9 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. – atbyrd Jan 18 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 at 4:16
As a general rule, your code should only catch what it can handle and either resolve or report appropriately. – Ryan Gates Jan 23 at 21:35

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

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.