Attrapés essayés ou y a-t-il un meilleur moyen?

Question

Dans la méthode ci-dessous, il existe de nombreuses instructions de cas (beaucoup ont été supprimées) qui appellent les classes Manager. Par exemple, le premier appelle ApplicationManager.GetByGUID. Chaque fois qu'une classe "manager" est utilisée, des contrôles de sécurité ont lieu. Problème: J'ai des entités qui peuvent être autorisées à certains d'entre eux, mais pas à tous. Donc, quand cette méthode est exécutée, si l'un d'entre eux craps, il va jeter une exception de sécurité et planter tout le rapport. Quelqu'un m'a suggéré que je pourrais juste jeter des blocs try-catch autour de chaque cas, mais plus je lis plus je me sens comme ça pourrait être bâclée. Je ne suis pas très au courant des exceptions ... J'espérais que quelqu'un pourrait suggérer un moyen de le faire avec plus de finesse ... Je dois être en mesure de récupérer de bonnes données et ignorer celles qui présentent des exceptions de sécurité .... ou peut-être essayer-attrape sont ok dans ce cas?

espoir qui fait sens ... grâce

---

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

Answer 1

En règle générale, les exceptions devraient indiquer que quelque chose a mal tourné. Si vous vous attendez à des exceptions au cours d'une course typique à travers cette méthode, vous devez changer vos API pour vous permettre d'éviter cette exception:

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

Voici une tentative rapide de ma part à factoriser ce code dans quelque chose plus raisonnable:

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

Depuis que je ne comprends pas toute la portée de ce code, je probablement certains aspects trop simplifié, mais vous pouvez voir qu'il ya beaucoup de refactoring à faire ici. À l'avenir, vous pourriez vouloir exécuter du code par http://refactormycode.com/, jusqu'à ce que vous vous habituiez à utiliser les meilleures pratiques.

Answer 2

Quelque part, vous aurez un code comme:

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

Mettez cela en:

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

Vous pouvez mettre le try-catch dans une fonction distincte que le corps de la boucle foreach devient grand une fois que vous attrapez plusieurs types d'exceptions. StriplingWarrior a également raison dans sa réponse: "Les exceptions devraient indiquer que quelque chose s'est mal passé." Laissez-les se propager à la boucle principale et montrez-les là.