5
\$\begingroup\$

This is a code review for a set of loggers following this question. The project is a universal windows application targeting Windows Phone.

There are concerns over the tight coupling between the loggers and App. There is way too much code in the main program that is responsible for setting up the logger just right.

There are also concerns on how to deal with the fact that a ReleaseLogger requires special behavior (establishing a session) that the DebugLogger class does not, and how to deal with this.

App.xaml.cs

using Log.Common;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices.WindowsRuntime;
using System.Threading.Tasks;
using Windows.ApplicationModel;
using Windows.ApplicationModel.Activation;
using Windows.Foundation;
using Windows.Foundation.Collections;
using Windows.Foundation.Diagnostics;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Controls.Primitives;
using Windows.UI.Xaml.Data;
using Windows.UI.Xaml.Input;
using Windows.UI.Xaml.Media;
using Windows.UI.Xaml.Media.Animation;
using Windows.UI.Xaml.Navigation;

// The Blank Application template is documented at http://go.microsoft.com/fwlink/?LinkId=234227

namespace Log
{
    /// <summary>
    /// Provides application-specific behavior to supplement the default Application class.
    /// </summary>
    public sealed partial class App : Application
    {
#if WINDOWS_PHONE_APP
        private TransitionCollection transitions;
#endif

        public const string TelemetryUrl = "http://try.count.ly";
        public const string TelemetryAppKey = "";

        private ILogger _logger;
        private ILogger Logger
        {
            get
            {
                if(null == _logger)
                {
//#if PROD
                    _logger = new ReleaseLogger();
//#else
                    //_logger = new DebugLogger();
//#endif
                }
                return _logger;
            }
        }

        /// <summary>
        /// Initializes the singleton application object.  This is the first line of authored code
        /// executed, and as such is the logical equivalent of main() or WinMain().
        /// </summary>
        public App()
        {
            this.InitializeComponent();
            this.Suspending += this.OnSuspending;
            this.Resuming += this.OnResuming;
            this.UnhandledException += OnUnhandledException;
        }

        private async Task TryStartTelemetry()
        {
            var logger = Logger as ReleaseLogger;
            if (null != logger)
            {
                await logger.StartSessionAsync(this, TelemetryUrl, TelemetryAppKey);
            }
        }

        private async Task TryEndTelemetry()
        {
            var logger = Logger as ReleaseLogger;
            if (null != logger)
            {
                await logger.EndSessionAsync();
            }
        }

        /// <summary>
        /// Invoked when the application is launched normally by the end user.  Other entry points
        /// will be used when the application is launched to open a specific file, to display
        /// search results, and so forth.
        /// </summary>
        /// <param name="e">Details about the launch request and process.</param>
        protected override async void OnLaunched(LaunchActivatedEventArgs e)
        {
#if DEBUG
            if (System.Diagnostics.Debugger.IsAttached)
            {
                this.DebugSettings.EnableFrameRateCounter = true;
            }
#endif

            Frame rootFrame = Window.Current.Content as Frame;

            // Do not repeat app initialization when the Window already has content,
            // just ensure that the window is active
            if (rootFrame == null)
            {
                // Create a Frame to act as the navigation context and navigate to the first page
                rootFrame = new Frame();

                // TODO: change this value to a cache size that is appropriate for your application
                rootFrame.CacheSize = 1;

                if (e.PreviousExecutionState == ApplicationExecutionState.Terminated)
                {
                    // TODO: Load state from previously suspended application
                }

                // Place the frame in the current Window
                Window.Current.Content = rootFrame;
            }

            if (rootFrame.Content == null)
            {
#if WINDOWS_PHONE_APP
                // Removes the turnstile navigation for startup.
                if (rootFrame.ContentTransitions != null)
                {
                    this.transitions = new TransitionCollection();
                    foreach (var c in rootFrame.ContentTransitions)
                    {
                        this.transitions.Add(c);
                    }
                }

                rootFrame.ContentTransitions = null;
                rootFrame.Navigated += this.RootFrame_FirstNavigated;
#endif

                // When the navigation stack isn't restored navigate to the first page,
                // configuring the new page by passing required information as a navigation
                // parameter
                if (!rootFrame.Navigate(typeof(MainPage), e.Arguments))
                {
                    throw new Exception("Failed to create initial page");
                }
            }

            await TryStartTelemetry();

            Logger.Log("Application Launched", LoggingLevel.Information, null);

            // Ensure the current window is active
            Window.Current.Activate();
        }

        protected override async void OnActivated(IActivatedEventArgs args)
        {
            base.OnActivated(args);

            await TryStartTelemetry();
        }

#if WINDOWS_PHONE_APP
        /// <summary>
        /// Restores the content transitions after the app has launched.
        /// </summary>
        /// <param name="sender">The object where the handler is attached.</param>
        /// <param name="e">Details about the navigation event.</param>
        private void RootFrame_FirstNavigated(object sender, NavigationEventArgs e)
        {
            var rootFrame = sender as Frame;
            rootFrame.ContentTransitions = this.transitions ?? new TransitionCollection() { new NavigationThemeTransition() };
            rootFrame.Navigated -= this.RootFrame_FirstNavigated;
        }
#endif

        /// <summary>
        /// Invoked when application execution is being suspended.  Application state is saved
        /// without knowing whether the application will be terminated or resumed with the contents
        /// of memory still intact.
        /// </summary>
        /// <param name="sender">The source of the suspend request.</param>
        /// <param name="e">Details about the suspend request.</param>
        private async void OnSuspending(object sender, SuspendingEventArgs e)
        {
            var deferral = e.SuspendingOperation.GetDeferral();

            // TODO: Save application state and stop any background activity
            await TryEndTelemetry();

            deferral.Complete();
        }

        private async void OnResuming(object sender, object e)
        {
            await TryStartTelemetry();
        }


        private async void OnUnhandledException(object sender, UnhandledExceptionEventArgs e)
        {
            await TryEndTelemetry();
        }
    }
}

ILogger.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Windows.Foundation.Diagnostics;

namespace Log.Common
{
    public interface ILogger
    {
        void LogException(Exception ex);

        void Log(string message, LoggingLevel loggingLevel, params object[] args);
    }
}

DebugLogger.cs

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Windows.Foundation.Diagnostics;

namespace Log.Common
{
    public class DebugLogger : ILogger
    {
        public void LogException(Exception ex)
        {
            if (ex == null)
                throw new ArgumentNullException("exception");
            Debug.WriteLine("Exception: " + ex.Message);
            // Log to UI here.
        }

        public void Log(string message, LoggingLevel loggingLevel, params object[] args)
        {
            if (string.IsNullOrWhiteSpace(message))
                throw new ArgumentException("message");

            if (args != null && args.Length > 0)
            {
                message = string.Format(message, args);
            }
            //TODO: let's define when and where log 
            switch (loggingLevel)
            {
                case LoggingLevel.Critical:
                    Debug.WriteLine("Critical: " + message, args);
                    // Log to UI here.
                    break;
                case LoggingLevel.Error:
                    Debug.WriteLine("Error: " + message, args);
                    // Log to UI here.
                    break;
                case LoggingLevel.Warning:
                    Debug.WriteLine("Warning: " + message, args);
                    // Log to UI here.
                    break;
                case LoggingLevel.Information:
                    Debug.WriteLine("Info: " + message, args);
                    // Log to UI here.
                    break;
                case LoggingLevel.Verbose:
                    Debug.WriteLine("Verbose: " + message, args);
                    break;
                default:
                    throw new ArgumentOutOfRangeException(nameof(loggingLevel), loggingLevel, null);
            }
        }
    }
}

ReleaseLogger.cs

using CountlySDK;
using Log;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Windows.Foundation.Diagnostics;
using Windows.UI.Xaml;

namespace Log.Common
{
    public class ReleaseLogger : ILogger
    {
        private bool _sessionStarted;
        // A Queue of exceptions to log.
        private Queue<Exception> _exceptionQueue;

        public ReleaseLogger()
        {
            _exceptionQueue = new Queue<Exception>();
            _sessionStarted = false;
            Countly.IsLoggingEnabled = true;
        }

        public async Task StartSessionAsync(Application app, string telemetryUrl, string telemetryAppKey)
        {
            if (telemetryUrl == null)
                throw new ArgumentNullException("telemetryUrl");
            if (telemetryAppKey == null)
                throw new ArgumentNullException("telemetryAppKey");

            try
            {
                await Countly.StartSession(telemetryUrl, telemetryAppKey, app);
                while (_exceptionQueue.Count > 0)
                {
                    await LogExceptionAsync(_exceptionQueue.Dequeue());
                }
                _sessionStarted = true;
            }
            catch (Exception countlyEx)
            {
                // Log to UI here.
            }
        }

        public async Task EndSessionAsync()
        {
            try
            {
                await Countly.EndSession();
            }
            catch (Exception countlyEx)
            {
                // Log to UI here.
            }
            finally
            {
                _sessionStarted = false;
                // Log to UI here.
            }
        }

        public async Task LogExceptionAsync(Exception ex)
        {
            if (ex == null)
                throw new ArgumentNullException("exception");

            if (_sessionStarted)
            {
                try
                {
                    await Countly.RecordException(ex.Message, ex.StackTrace);
                }
                catch (Exception countlyEx)
                {
                    // Log to UI here.
                }
            }
            else
            {
                _exceptionQueue.Enqueue(ex);
            }
        }

        public void LogException(Exception ex)
        {
            var result = LogExceptionAsync(ex);
        }

        public async Task LogAsync(string message, LoggingLevel loggingLevel, params object[] args)
        {
            if (string.IsNullOrWhiteSpace(message))
                throw new ArgumentException("message");

            if (args != null && args.Length > 0)
            {
                message = string.Format(message, args);
            }
            switch (loggingLevel)
            {
                case LoggingLevel.Critical:
                case LoggingLevel.Error:
                    // Log to UI here.
                    await Countly.RecordException(message);
                    break;
                case LoggingLevel.Warning:
                    // Log to UI here.
                    break;
                case LoggingLevel.Information:
                    await Countly.RecordEvent(message);
                    // Log to UI here.
                    break;
                case LoggingLevel.Verbose:
                    Debug.WriteLine(message, args);
                    break;
                default:
                    throw new ArgumentOutOfRangeException(nameof(loggingLevel), loggingLevel, null);
            }
        }

        public void Log(string message, LoggingLevel loggingLevel, params object[] args)
        {
            var result = LogAsync(message, loggingLevel, args);
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ It may help reviewers do a better job if you would add a bit more description of what the code is intended to do, and possibly a short bit of code illustrating how the loggers used. \$\endgroup\$
    – Edward
    Dec 22 '16 at 1:49
2
\$\begingroup\$

General advice for decoupling logging from application code:

Have your logger listen for events instead of injecting your logger into application code. This inverts the dependency so that business code doesn't need to even know that a logger exists.

As for the amount of code related to logging in your App class, I don't see it as something to be concerned about. If there's anywhere in a UWP app to wire these kinds of things up, this is the place.


One quick, unrelated, note about async. You do this several times.

    public void Log(string message, LoggingLevel loggingLevel, params object[] args)
    {
        var result = LogAsync(message, loggingLevel, args);
    }

Wrapping asynchronous methods in synchronous void methods defeats the purpose of having async methods to begin with. If you're going to use async/await, you really need to "Go async all the way". I'd highly recommend reading Stephen Cleary's article "Async/Await - Best Practices in Async Programming".

\$\endgroup\$
12
  • \$\begingroup\$ Thanks for the decoupling tip - my team is complaining about performing as cast checks. They would instead prefer to add the StartSession and EndSession methods to the interface, call them from the App class, and have such implenetations in the DebugLogger simply be empty. \$\endgroup\$ Dec 22 '16 at 0:42
  • \$\begingroup\$ @FranciscoAguilera listen to your team. That's the right thing to do. \$\endgroup\$
    – RubberDuck
    Dec 22 '16 at 0:45
  • \$\begingroup\$ It is not that easy as it would also be a violation of SOLID principles... My quest today is to find the optimal, correct solution and that just seems like the easy way out. \$\endgroup\$ Dec 22 '16 at 0:46
  • \$\begingroup\$ @FranciscoAguilera how exactly would that violate SOLID. If anything, it's more in line than your current solution. \$\endgroup\$
    – RubberDuck
    Dec 22 '16 at 0:48
  • \$\begingroup\$ see the question I linked. \$\endgroup\$ Dec 22 '16 at 0:49
2
\$\begingroup\$

There are a lot of boilerplate comments left in the code, such as the one about the Blank Template. Once you (as the developer) have read and understood them, you should remove them, since they just waste screen space.

In the Log method, the order of the arguments should be level, message, args, since the message and the args belong together.

Formatting the message must be done in the synchonous portion of the logging code, since the state of the args may change after the call, leading to wrong log messages.

Make sure that asynchronous log messages cannot be reordered on their way to the log destination, otherwise the log files will become an unreliable source of information.

There is no point in declaring var result when you don't actually use the result.

Make sure that when an asynchronous call to Log is executed, the logger is fully initialized. Also, after terminating the logger, further calls to Log should be ignored.

By the way, having void async methods totally makes sense here since logging doesn't produce results that could be inspected by the application.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your comments, Roland. How would you deal with the two overarching issues of coupling and SOLID violation? \$\endgroup\$ Dec 22 '16 at 13:49

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.