2

Let's say I have this service abstraction exposed from a library.

public interface INavigator
{
   ImmutableList<IPageViewModel> Entries { get; }

   void NavigateForward(IPageViewModel page);

   void RemoveEntries(int indices);

   void NavigateBack();
}

The library also exposes a default implementation that takes in an ILogger as dependency. Each method uses the logger.

public class DefaultNavigator : INavigator
{
   private ILogger _logger;

   public DefaultNavigator(ILogger logger)
   {
      _logger = logger;
   }
   
   public void NavigateForward(IPageViewModel page)
   {
      // [...]

      _logger.LogInformation("Navigated to {ViewModelName}.", page.Name);
   }

   // [...]
}

The library also exposes extensions methods on INavigator to add functionality, in the same fashion as System.Linq (where the IEnumerable<T> interface is very simple and all the core logic comes from the extension methods).

In our case, it makes sense to implement some logs in those extension methods.

public static class NavigatorExtensions
{
   public static void ReturnToRoot(INavigator navigator)
   {
      var logger = ???
      
      // [...]

      logger.LogInformation("Returned to root of navigation stack.");
   }
}

Here's the question: Where should we get the ILogger from?

What we currently have

We currently can access the IServiceProvider statically, which allows us to get an ILogger from anywhere. However, we want to move away from that public static pattern because we want to have multiple IServiceProvider instances in the same process.

Options I'm considering

1. Expose ILogger from INavigator

Does it make sense though?

public interface INavigator
{
   ImmutableList<IPageViewModel> Entries { get; }

   void NavigateForward(IPageViewModel page);

   void RemoveEntries(int indices);

   void NavigateBack();

   // This isn't related to navigation.
   ILogger Logger { get; }
}
  • It doesn't feel quite right because that new member doesn't have anything to do with navigation.

2. Create a new interface that exposes an ILogger and soft cast in the extension method

// Not the best name, but I'm trying to avoid conflicts with the existing ILoggerProvider.
public interface ILoggerSource
{
   ILogger Logger { get; }
}

The extension method would do something like this:

public static class NavigatorExtensions
{
   public static void ReturnToRoot(INavigator navigator)
   {
      var logger = (navigator as ILoggerSource)?.Logger ?? NullLogger.Instance;
      
      // [...]

      logger.LogInformation("Returned to root of navigation stack.");
   }
}
  • If this was the proper solution, I think ILoggerSource would already exist in MS libraries. May be it does, but I didn't see it?
  • New implementations of INavigator don't have any clue that logs from extensions methods will not to work unless they also implement ILoggerSource.

3. I'm open to other options :)

Conclusion

As of writing this, I think option 1 is currently the lesser evil because it doesn't hide functionality, but my engineer mind is not satisfied.

4
  • Would it be an option to pass the logger as an optional argument to the extension method?
    – Rik D
    Apr 9 at 5:40
  • Are there implementations of INavigator that don't have an ILogger associated with them? I.e., if you went for your option (1), would you have to check for null? Apr 9 at 13:41
  • @RikD Passing the ILogger as an argument to the extension method shifts the responsibility to the calling site. It works, but I feel like it doesn't scale well when we add more extension methods. i.e. If all extension methods need the same argument, perhaps it should already "be there" somehow?
    – Batesias
    Apr 10 at 15:56
  • @FilipMilovanović The INavigator could be more explicit by using C# 8 nullables and having the ILogger be nullable (or not) so that extension methods would know whether to null check or assume a non-null value. That way implementers could use either null or NullLogger depending on the definition.
    – Batesias
    Apr 10 at 16:05

4 Answers 4

3

The problem isn't just logging-- it's any time you want to access a service from an extension method. If you're going to use dependencies in your methods you should give them an instance.

If you want to have helper methods that do complicated stuff, then use a proper helper class, and accept the INavigator as an argument.

class NavigatorHelper
{
    protected readonly ILogger _logger;
    protected readonly IOtherDependency _otherDependency;

    public NavigatorHelper(Ilogger logger, IOtherDependency otherDependency)
    {
        _logger = logger;
        _otherDependency = otherDependency;
    }

    public void ReturnToRoot(INavigator navigator)
    {
        _logger.LogInformation("Returned to root of navigation stack.");
    }
}

//Usage
helper.ReturnToRoot(someInstance);

I get that you want to be like LINQ but LINQ's methods are all idempotent and don't mutate anything, and they only depend on what you explicitly pass them.

1

Option 3. don't use extension methods.

public class NavigatorExtension
{
   public NavigatorExtension(ILogger logger) {...}
   public void ReturnToRoot(INavigator navigator)
   {
      navigator.. whatever
      logger.LogInformation("Returned to root of navigation stack.");
   }
}

OR

public class Navigator2 : DefaultNavigator
{
   public void ReturnToRoot()
   {
      this.Whatever...
      logger.LogInformation("Returned to root of navigation stack.");
   }
}
2
  • I'm not convinced by your second suggestion because I want ReturnToRoot to be available for INavigator (not just DefaultNavigator). Your first suggestion works, but complexifies the application code a little: Consumers would need to [resolve/receive from injection] both INavigator AND NavigatorExtension in order to use ReturnToRoot.
    – Batesias
    Apr 10 at 15:48
  • if you think about it, those are also flaws in extension methods
    – Ewan
    Apr 10 at 16:54
1

A possible alternative could be to build a custom extended INavigator which includes those extension methods as ordinary methods, then extend the DefaultNavigator to use the new interface, passing the logger in:

public interface IExtendedNavigator : INavigator
{
    void ReturnToRoot(); 
    // TODO - other replacements for extension methods here
}
public class ExtendedDefaultNavigator : DefaultNavigator, IExtendedNavigator
{
    private readonly ILogger _logger;

    public ExtendedDefaultNavigator(ILogger logger) : base(logger)
    {
        _logger = logger;
    }

    public void ReturnToRoot()
    {
        NavigatorExtensions.ReturnToRoot(this);
        _logger.LogInformation("Returned to root of navigation stack.");
    }
}

This would allow any usages in your program of INavigator to be substituted by IExtendedNavigator, then replacing DefaultNavigator with ExtendedDefaultNavigator - for example:

var logger = serviceProvider.GetService<ILoggerFactory>()
            .CreateLogger(nameof(Program));
IExtendedNavigator navigator = new ExtendedDefaultNavigator(logger);

// Calls ExtendedDefaultNavigator.ReturnToRoot() instead of the extension method.
navigator.ReturnToRoot(); 

A word of caution - there is a potential 'gotcha' here in that code attempting to consume the ExtendedDefaultNavigator should avoid using the INavigator interface because that would fall back on the extension methods:

INavigator navigator = new ExtendedDefaultNavigator(logger);

// Warning - calls the extension method without logging!
navigator.ReturnToRoot(); 
1
  • I understand the suggestion, but I think the "gotcha" you pointed could be problematic (especially error-prone). Also, it seems like the solution doesn't scale well with additional extension methods (either from within the library or from the consuming application code) because they would require changes to the IExtendedNavigator OR involve additional interfaces. One of my goal is to keep the original interface as simple as possible to easily allow new implementations.
    – Batesias
    Apr 9 at 23:14
0

New option: Use a static ConditionalWeakTable

I didn't think of this earlier but another thing that could work is use a static ConditionalWeakTable to create pairs of INavigator and ILogger.

Something like this (null scenarios could be improved, but it's just to show how it could look).

public static class NavigatorExtensions
{
    private static readonly ConditionalWeakTable<INavigator , ILogger> _loggers = new ConditionalWeakTable<INavigator , ILogger>();

    public static void SetLogger(this INavigator navigator, ILogger logger)
    {
        _loggers.Add(navigator, logger);
    }

    public static ILogger GetLogger(this INavigator navigator)
    {
        if (_loggers.TryGetValue(navigator, out var logger))
        {
            return logger;
        }

        return null;
    }


   public static void ReturnToRoot(INavigator navigator)
   {
      var logger = navigator.GetLogger() ?? NullLogger.Instance;
      
      // [...]

      logger.LogInformation("Returned to root of navigation stack.");
   }
}

Then from the startup, the INavigator would need to set its ILogger explicitly.

// [...]
var navigator = serviceProvider.GetRequiredService<INavigator>();
var logger = serviceProvider.GetRequiredService<ILogger<INavigator>>();
navigator.SetLogger(logger);

That doesn't seem too bad actually.

  • Adding new extension methods can leverage the GetLogger method.
  • Changes are restricted to library code and app configuration. (No changes are required in existing app library-usage code.)

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.