Nested try-catches or is there a better way?

In the method below there are numerous case statements (many have been removed) that make calls to Manager classes. For example, the first one calls ApplicationManager.GetByGUID. Any time a "manager" class is used, security checks occur.

Problem: I have entities that may be permitted to some of these but not all. So when this method gets run, if one of them craps out it'll throw a security exception and crash the whole report.

Someone has suggested to me that I could just throw try-catch blocks around each case but the more I read the more I feel like that might be sloppy. I admittedly am not very knowledged about exceptions...I was hoping someone could suggest a way to do this with more finesse...I need to be able to get back good data and ignore the ones that throw security exceptions....or maybe try-catches are ok in this case?

Hope that makes sense...thanks

---

private string GetLookup(string value, string type)
{
    MySqlConnection mconn = new MySqlConnection(ConfigurationSettings.AppSettings["UnicornConnectionString_SELECT"]);

    try
    {
        mconn.Open();

        lock (reportLookups)
        {
            if (reportLookups.ContainsKey(type+value))
                return reportLookups[type+value].ToString();
            else if (reportLookups.ContainsKey(value))
                return reportLookups[value].ToString();
            else
            {
                switch (type)
                {
                    case "ATTR_APPLICATIONNAME":
                        if (value != Guid.Empty.ToString())
                        {
                            reportLookups.Add(type + value, applicationManager.GetByGUID(value).Name);
                        }
                        else
                        {
                            reportLookups.Add(type + value, "Unknown");
                        }
                        mconn.Close();
                        return reportLookups[type + value].ToString();
                        break;
                    case "ATTR_CITYNAME":
                        reportLookups.Add(type + value, UMConstantProvider.UMConstantProvider.GetConstant<UMString64>(int.Parse(value), UMMetricsResourceLibrary.Enumerations.ConstantType.CITY_NAME, ref mconn));
                        mconn.Close();
                        return reportLookups[type + value].ToString();
                        break;
                    case "ATTR_COUNTRYNAME":
                        reportLookups.Add(type + value, UMConstantProvider.UMConstantProvider.GetConstant<UMString2>(int.Parse(value), UMMetricsResourceLibrary.Enumerations.ConstantType.COUNTRY_NAME, ref mconn));
                        mconn.Close();
                        return reportLookups[type + value].ToString();
                        break;
                    case "ATTR_ITEMDURATION":

                        MediaItem mi = mediaItemManager.GetMediaItemByGUID(value);
                        if (mi.MediaItemTypeID == (int)MediaItemType.ExternalVideo || mi.MediaItemTypeID == (int)MediaItemType.ExternalAudio)
                        {
                            reportLookups.Add(type + value, mediaItemManager.GetMediaItemByGUID(value).ExternalDuration);
                            mconn.Close();
                            return reportLookups[type + value].ToString();
                        }
                        else
                        {

                            List<BinaryAsset> bins = fileSystemManager.GetBinaryAssetsByMediaItemGuid(value, mi.DraftVersion);
                            var durationasset = from d in bins
                                                where d.Duration != 0
                                                select d.Duration;

                            if (durationasset.Count() > 0)
                            {

                                reportLookups.Add(type + value, durationasset.ToList()[0]);

                            }
                            else
                            {
                                reportLookups.Add(type + value, 0);
                       开发者_JS百科         mconn.Close();
                                return reportLookups[type + value].ToString();
                            }


                        }

                        break;
                }
            }
            return string.Empty;
        }
    }
    finally 
    {
        mconn.Close();
    }
}

---

As a rule, Exceptions should indicate that something went wrong. If you're expecting exceptions during the course of a typical run through this method, you should change your APIs to allow you to avoid that exception:

if (mediaItemManager.CanAccessMediaItem(value))
{
    MediaItem mi = mediaItemManager.GetMediaItemByGUID(value);
    ....
}

Here's a quick attempt on my part to refactor this code into something more reasonable:

private string GetLookup(string value, string type)
{
    var lookupKey = type + value;                        
    using (MySqlConnection mconn = new MySqlConnection(ConfigurationSettings.AppSettings["UnicornConnectionString_SELECT"]))
    {
        mconn.Open();
        lock (reportLookups)
        {
            if (reportLookups.ContainsKey(lookupKey))
            {
                return reportLookups[lookupKey].ToString();
            }
            var value = GetLookupValue(type, value);
            reportLookups[lookupKey] = value;
            return value;
        }
    }
}

private string GetLookupValue(string type, string value)
{
    switch (type)
    {
        case "ATTR_APPLICATIONNAME":
            return value == Guid.Empty.ToString() 
                ? "Unknown"
                : applicationManager.CanGetByGUID(value)
                    ? applicationManager.GetByGUID(value).Name
                    : string.Empty;
        case "ATTR_CITYNAME":
            return UMConstantProvider.UMConstantProvider.GetConstant<UMString64>(int.Parse(value), UMMetricsResourceLibrary.Enumerations.ConstantType.CITY_NAME, ref mconn);
        case "ATTR_COUNTRYNAME":
            return UMConstantProvider.UMConstantProvider.GetConstant<UMString2>(int.Parse(value), UMMetricsResourceLibrary.Enumerations.ConstantType.COUNTRY_NAME, ref mconn);
        case "ATTR_ITEMDURATION":
            if(mediaItemManager.CanGetMediaItemByGUID(value)) {
                MediaItem mi = mediaItemManager.GetMediaItemByGUID(value);
                if (mi.MediaItemTypeID == (int)MediaItemType.ExternalVideo || mi.MediaItemTypeID == (int)MediaItemType.ExternalAudio)
                {
                    return mediaItemManager.GetMediaItemByGUID(value).ExternalDuration;
                }
                else
                {
                    List<BinaryAsset> bins = fileSystemManager.GetBinaryAssetsByMediaItemGuid(value, mi.DraftVersion);
                    var durationasset = from d in bins
                                        where d.Duration != 0
                                        select d.Duration;
                    return durationasset.FirstOrDefault() ?? "0";
                }
            }
            else 
            {
                return string.Empty;
            }
        default:
            return string.Empty;
    }
}

Since I don't understand the full scope of this code, I probably oversimplified some aspects of it, but you can see that there is a lot of refactoring to be done here. In the future, you might want to run some code by http://refactormycode.com/, until you get accustomed to using best practices.

---

Somewhere you will have some code like:

foreach(Request req in allRequests)
{
   Reply result = MakeReply(req);
   WriteReply(result);
}

Turn this into:

foreach(Request req in allRequests)
{
   Reply result;
   try
   {
      result = CreateReply(req);
   }
   catch(SecurityException ex)
   {
      result = CreateReplyUnauthorized();
   }
   catch(Exception ex) // always the last
   {
      LogException(ex); // for bug hunting

      // Don't show the exception to the user - that's a security risk
      result = CreateReplySystemError();
   }

   WriteReply(result);
}

You might want to put the try-catch into a separate function as the body of your foreach loop is getting large once you catch several types of exceptions.

StriplingWarrior is also right in his reply: "Exceptions should indicate that something went wrong." Let them propagate to the main loop and show them there.