Вложенные пробные ловушки или есть способ лучше?

В приведенном ниже методе есть множество операторов case (многие из них были удалены), которые обращаются к классам Manager. Например, первый вызывает ApplicationManager.GetByGUID. Каждый раз, когда используется класс «менеджер», происходят проверки безопасности.

Проблема: у меня есть объекты, которые могут быть разрешены для некоторых из них, но не для всех. Поэтому, когда этот метод запускается, если один из них выйдет из строя, он вызовет исключение безопасности и приведет к сбою всего отчета.

Кто-то предложил мне просто разбрасывать блоки try-catch вокруг каждого случая, но чем больше я читаю, тем больше мне кажется, что это может быть небрежно. По общему признанию, я не очень разбираюсь в исключениях ... Я надеялся, что кто-то может предложить способ сделать это более изящно ... Мне нужно иметь возможность возвращать хорошие данные и игнорировать те, которые вызывают исключения безопасности ... или, может быть, в этом случае подойдут пробные уловы?

Надеюсь, это имеет смысл ... спасибо


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);
                                mconn.Close();
                                return reportLookups[type + value].ToString();
                            }


                        }

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

person J Benjamin    schedule 02.11.2010    source источник
comment
Я думаю, что это скорее рефакторинг кода. ваше включение параметра type, который передается - так почему бы не разделить его на разные методы? сделает ваш код более читабельным / легким в обслуживании. кроме того, почему вы делаете mconn.Close(); в обоих операторах case и в блоке finally? просто сделай это один раз в конце.   -  person RPM1984    schedule 03.11.2010
comment
вау, это было быстро, спасибо, ребята   -  person J Benjamin    schedule 03.11.2010
comment
кое-что еще, с чем я столкнулся, заключалось в использовании using в соединении, поэтому мне не придется беспокоиться о его закрытии ... в любом случае, мне нравится идея разделения на методы лучше всего, поэтому я попробую это (и я не знаю, почему исходный кодер вставил все эти .close (), я их уберу) :)   -  person J Benjamin    schedule 03.11.2010
comment
@J Benjamin - да, используйте выражение using, когда это возможно. тогда вам вообще не нужно звонить .Close(). Преимущество using также в том, что вы можете ограничить код базы данных внутри блока, что приведет к open late, close early советам по соединениям с базой данных. удачи.   -  person RPM1984    schedule 03.11.2010
comment
Кроме того, пока мы занимаемся «бизнесом по рефакторингу кода», я бы изменил string type на перечисление. опять же, более читабельный / поддерживаемый (не говоря уже о безопасности типов).   -  person RPM1984    schedule 03.11.2010
comment
Спасибо за все ваши комментарии ... Раньше я скрывался от StackOverflow, чтобы получить ответы, но это был первый раз, когда я зарегистрировался и принял участие. Отличный сайт, отличные умы ... Я с нетерпением жду возможности участвовать в других обсуждениях в будущем   -  person J Benjamin    schedule 03.11.2010


Ответы (2)


Как правило, исключения должны указывать на то, что что-то пошло не так. Если вы ожидаете исключений во время типичного запуска этого метода, вам следует изменить свои API, чтобы избежать этого исключения:

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

Вот небольшая попытка с моей стороны преобразовать этот код во что-то более разумное:

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;
    }
}

Поскольку я не понимаю весь объем этого кода, я, вероятно, слишком упростил некоторые его аспекты, но вы можете видеть, что здесь нужно провести много рефакторинга. В будущем вы, возможно, захотите запустить некоторый код с помощью http://refactormycode.com/, пока не привыкнете к используя передовой опыт.

person StriplingWarrior    schedule 02.11.2010
comment
Хотя часто бывает необходимо иметь пробную ловушку на тот случай, если действительность может измениться между проверкой и использованием. Классическим случаем является FileNotFound, когда файл существовал, но был удален сразу после проверки. По-прежнему лучше сначала проверить, если это возможно, или иметь «пробную» версию метода, который не вызывает ошибок. - person Dan Bryant; 03.11.2010
comment
@ Дэн Брайант: Хорошая мысль. Мне особенно нравится стратегия TryGet..., поскольку она избегает условий гонки. Во многих случаях нам все еще нужно спросить себя: что на самом деле должно произойти, если это произойдет? В этом примере, если у пользователя есть права на определенный класс объектов, отозванных на полпути в процессе создания отчета, хочу ли я, чтобы они видели первую половину отчета, которая может содержать некоторые из тех значений, которыми они не являются? больше разрешено видеть? В таких нестандартных случаях я обычно удовлетворен тем, что не так уж плохо, если генерация отчета время от времени выдает исключение. - person StriplingWarrior; 03.11.2010
comment
Спасибо за все ваши комментарии ... Раньше я скрывался от StackOverflow, чтобы получить ответы, но это был первый раз, когда я зарегистрировался и принял участие. Отличный сайт, отличные умы ... Я с нетерпением жду возможности участвовать в других обсуждениях в будущем - person J Benjamin; 03.11.2010

Где-то у вас будет код вроде:

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

Превратите это в:

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);
}

Вы можете поместить try-catch в отдельную функцию, поскольку тело вашего цикла foreach становится большим после того, как вы поймаете несколько типов исключений.

StriplingWarrior также прав в своем ответе: «Исключения должны указывать на то, что что-то пошло не так». Пусть они распространятся в основной цикл и покажут их там.

person Sjoerd    schedule 02.11.2010
comment
Спасибо за все ваши комментарии ... Раньше я скрывался от StackOverflow, чтобы получить ответы, но это был первый раз, когда я зарегистрировался и принял участие. Отличный сайт, отличные умы ... Я с нетерпением жду возможности участвовать в других обсуждениях в будущем - person J Benjamin; 03.11.2010