4
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ I think this question leads to a much bigger problem. You need a standard. This kind of thing can't be half implemented in your system when you add it and your other dev. doesn't. Although I believe 200_success's answer covers your specific problem very well. \$\endgroup\$ – IEatBagels Dec 4 '18 at 18:21
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ but the exception would be logged to the event viewer instead of an application specific log file, which does require extra investigation and more information to find the thrown event though correct? \$\endgroup\$ – Greg Dec 4 '18 at 17:43
  • 1
    \$\begingroup\$ Who calls this GetSqlQuery() function? At some point, some caller in the stack should catch the exception and handle it in the way that you want. \$\endgroup\$ – 200_success Dec 4 '18 at 17:45
  • \$\begingroup\$ Their implemented context class where they read the query resources, so they should be actually handling any exceptions at that point or when they call their context. To know if something failed to enter the database. \$\endgroup\$ – Greg Dec 4 '18 at 17:48
4
\$\begingroup\$

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;
          }
     }
}
\$\endgroup\$
  • \$\begingroup\$ In C# 8 when they introduce nothing is null by default unless explicit, would that change your expectation? \$\endgroup\$ – Greg Dec 4 '18 at 17:49
  • \$\begingroup\$ Well I did't inform me about C# 8. I usually only work with what is avaible. \$\endgroup\$ – Heslacher Dec 4 '18 at 17:50
  • \$\begingroup\$ Oh, fair enough. I just know the paradigm for C# is about to shift. I know the feature is an opt-in, but in essence with the current way everything is null since a state has not been defined, in C# 8 nothing can be assigned as null unless the type explicitly allows it. So I was just curious if that would change your answer or you think this is still the best course of action? I am not trying to demean or dismiss your current answer, simply want to get the most information so I may make the best decision possible for implementation. \$\endgroup\$ – Greg Dec 4 '18 at 17:52
  • \$\begingroup\$ Why not use an optional parameter on the ILogger instead of an overload? \$\endgroup\$ – IEatBagels Dec 4 '18 at 18:22
  • \$\begingroup\$ Because using an overloaded method is ckearer from the caller side. \$\endgroup\$ – Heslacher Dec 4 '18 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.