Gfrktyl

I've created a utility class that expects a logger to be passed at a method level, since I prefer legitimate errors to be written out for troubleshooting later on. Another developer hates passing the logger to the method, because he does not care.

1. Cannot use optional attribute, since ILogger is not a constant.


2. Cannot use params because it would be a singleton, not a collection.

So I implemented it in the following manner:

public static class EmbeddedResourceUtility

{

     public static string GetSqlQuery(string namespaceAndFileNameWithExtension, ILogger logger)

     {

          try

          {

               using(var stream = typeof(EmbeddedResourceUtility).GetTypeInfo().Assembly.GetManifestResourceStream(namespaceAndFileNameWithExtension))

                   using(var reader = new StreamReader(stream, Encoding.UTF8))

                        return reader.ReadToEnd();

          }



          catch(Exception exception)

          {

               logger.Error($"Failed to read query file {namespaceAndFileNameWithExtension}. {Environment.NewLine}{exception.HResult}: {exception.Message}");

               throw new Exception(exception.Message);

          }

     }

}

Would be incorrect to expect someone to pass an ILogger even if they do not intend to use it? Or would there be a better way to handle this?

The most unforgivable aspect of this code is that it rethrows a degraded exception. The original type of the exception (likely an IOException) and the stack trace information are lost when you throw new Exception(…).

Two acceptable approaches are:

1. 
   

   Log the failure (if a non-null logger was passed). Then re-throw the original exception.

   
   
   

    public static string GetSqlQuery(string namespaceAndFileNameWithExtension, ILogger logger)
   
    {
   
         try
   
         {
   
              using(var stream = typeof(EmbeddedResourceUtility).GetTypeInfo().Assembly.GetManifestResourceStream(namespaceAndFileNameWithExtension))
   
              using(var reader = new StreamReader(stream, Encoding.UTF8))
   
              {
   
                   return reader.ReadToEnd();
   
              }
   
         }
   
         catch (Exception exception)
   
         {
   
              logger?.Error($"Failed to read query file {namespaceAndFileNameWithExtension}.{Environment.NewLine}{exception.HResult}: {exception.Message}");
   
              throw;
   
         }
   
    }
   
   

   
   


2. 
   

   Throw a custom exception, wrapping the original exception.

   
   
   

    public static string GetSqlQuery(string namespaceAndFileNameWithExtension)
   
    {
   
         try
   
         {
   
              using(var stream = typeof(EmbeddedResourceUtility).GetTypeInfo().Assembly.GetManifestResourceStream(namespaceAndFileNameWithExtension))
   
              using(var reader = new StreamReader(stream, Encoding.UTF8))
   
              {
   
                  return reader.ReadToEnd();
   
              }
   
         }
   
         catch (Exception exception)
   
         {
   
              throw new EmbeddedResourceException($"Failed to read query file {namespaceAndFileNameWithExtension}", exception);
   
         }
   
    }
   
   

   
   
   

   In this case, requiring an ILogger just isn't worth it. The caller has all of the information in the EmbeddedResourceException, and can log it if it wants.

   
   

I recommend the second approach, because it respects the principle of separation of concerns.

Would be incorrect to expect someone to pass an ILogger even if they do not intend to use it?

• If I wouldn't want to pass an ILogger I would just pass (ILogger)null and your method would throw a NullReferenceException if an exception occurs. If you use C# 6 you could use the Null-conditional operator




• If you keep the method signature like posted anyone who uses this method needs to pass an ILogger. To overcome this problem you can either make the ILogger argument optional or you implement an overloaded version of this method which doesn't take an ILogger argument.




• Omitting braces althought they might be optional should be avoided.




• Throwing throw new Exception(exception.Message) will destroy the stacktrace. It would be better to just throw.




• By using the ctor of StreamReader which only takes a Stream as argument the default encoding is UTF8Encoding.

Implementing the mentioned points would lead to

public static class EmbeddedResourceUtility

{

     public static string GetSqlQuery(string namespaceAndFileNameWithExtension)

     {

          return GetSqlQuery(namespaceAndFileNameWithExtension, (ILogger)null);

     }

     public static string GetSqlQuery(string namespaceAndFileNameWithExtension, ILogger logger)

     {

          try

          {

               using(var stream = typeof(EmbeddedResourceUtility).GetTypeInfo().Assembly.GetManifestResourceStream(namespaceAndFileNameWithExtension))

               using(var reader = new StreamReader(stream))

               {

                    return reader.ReadToEnd();

               }

          }

          catch(Exception exception)

          {

               logger?.Error($"Failed to read query file {namespaceAndFileNameWithExtension}. {Environment.NewLine}{exception.HResult}: {exception.Message}");

               throw;

          }

     }

}