Improved Diagnostics - Add logging to exception handling

[flaser]

Having started to browse the code, I see several instances where exceptions are caught and null is returned, discarding all context.
In turn, null returns are checked at fn invocation.
This is almost like C-style return codes, and while it works it discards useful information.

Debugging (and error reproduction) could be improved with some logging in place.
We could follow this up by evaluating what exception should be propagated with potential user prompts.

An example for discarded exception:

ComicRack.Engine/ComicInfo.cs

		public static ComicInfo LoadFromSidecar(string file)
		{
			try
			{
				using (FileStream inStream = File.OpenRead(file + ".xml"))
				{
					return Deserialize(inStream);
				}
			}
			catch (Exception)
			{
				//This is where we could put some tracing/logging logic
				return null;
			}
		}

...and null check:

ComicRack.Engine/IO/Provider/Readers/ComicProvider.cs

		public ComicInfo LoadInfo(InfoLoadingMethod method)
		{
			using (LockSource(readOnly: true))
			{
				ComicInfo comicInfo = (DisableNtfs ? null : NtfsInfoStorage.LoadInfo(base.Source));
				if (comicInfo == null && !DisableSidecar)
				{
					comicInfo = ComicInfo.LoadFromSidecar(base.Source);
				}
				if (comicInfo != null && method == InfoLoadingMethod.Fast)
				{
					return comicInfo;
				}
				ComicInfo comicInfo2 = OnLoadInfo();
				return comicInfo2 ?? comicInfo;
			}
		}

[maforget]

What are you suggesting? Throwing exception everywhere instead? I get that sometimes throwing a specific exception that could have some logging attached to it. Maybe we won't know if it didn't work because either the file was moved or the folder didn't have permissions or whatever. It might be useful is SOME location to give feedback, but that seems like a hell of work to refactor this whole program and go through each and every exception to figure out what might go wrong at every possibility.

But at the end is it really useful for the end user? An exception was handled, it was excepted my the developer. So unless it crashes the program or does something weird the won't care really. If it does lead to a crash there is a system to catch unhandled exceptions and show the stack trace to the user.

Checking for null is completely normal it isn't just because of an exception. In your example, the Deserialize might have also returned a null or DisableNtfs was true and DisableSidecar was also true, then comicInfo may be null, then you want to make sure that you don't return the null value. You want to actually make sure to load from the xml embedded in the actual CBZ at the end. Also with newer framework with nullable turned on, the compiler will actually tell you this might be a null and you WILL need to do a check.

I think both approach have some merit, but just throwing exception everywhere and catching them all isn't really a solution. In that example we would have needed to add a try..catch at the second location also. Doing try..catch does have an effect on performance beside just null checking.

[flaser]

No, I'm not calling for changing the error handling style.
That feels like a very big, cosmetic refactor with little benefit.

I'm just pointing out that we might prefer to also log/trace at some of these points.
You're also correct, logging for the sake of logging is dumb, it should serve a purpose.

I know that VS has excellent debugging capabilities and is thus an alternative.
However, we shouldn't underestimate giving a debug build to users to get error/execution traces.

Overall, I'm thinking of ways to improve the traceability of program, so its behavior is not so opaque.
Adding some logging to exception handling seems like a good start.
By definition these are how .NET expresses unusual or potentially erroneous states on a type level.

---

Something concrete:
What Logger to Use? TL;DR - ILogger

I was digging around to figure out what's Loggers are available in .NET Framework + C#
This MS article nicely summarizes the various solutions used over the years:
https://learn.microsoft.com/en-us/dotnet/core/diagnostics/logging-tracing

Natively, we could use System.Diagnostics.Trace and System.Diagnostics.Debug.
However, these libraries are ancient and not supported in .NET Core.

By contrast, ILogger is the latest, recommended default with good support.
Luckily, it is available on NuGet: https://www.nuget.org/packages/Microsoft.Extensions.Logging

Finally, one long term goal of adding logging/tracing could be to generate trace view that helps visualize/analyze program behavior. Using a structured logger is a necessity if we ever consider this.

---

Meanwhile, I just realized there's a very useful area for User logs - Batch operations.
Currently CR focuses on interactive use for which Modal Dialogs are very well suited.
However batch operations/transformations do happen when managing a library.
Writing to logs seems better suited for these Use Cases and given that this is power-user area I don't think the worse accessibility of logs is major issue.

[maforget]

I've seen some Trace in some place already in the program, but it isn't ancient at all, it's core to the framework. the page you linked to is for .NET 8 (the latest). They would never remove something so basic like that.

[flaser]

ILogger in .NET Framework

I've seen some Trace in some place already in the program, but it isn't ancient at all, it's core to the framework. the page you linked to is for .NET 8 (the latest).

You're correct that the page is for .NET.
That's why I explicitly mentioned that ILogger is actually supplied in a separate library that can be added to a .NET Framework solution.

Here's a discussion on how to use it in .NET Framework:
https://stackoverflow.com/questions/42356802/how-does-microsoft-extensions-logging-work-for-full-net-framework

Documentation

They would never remove something so basic like that.
Overall, I was thinking that if we improve logging we may choose something more modern.
Not the least because of potentially better documentation.

Compared to ILogger, Trace & Debug's docs seems a lot more austere:
https://learn.microsoft.com/en-us/dotnet/framework/debug-trace-profile/tracing-and-instrumenting-applications

Drawbacks
One could argue ILogger is a lot more complex.
If all we want are a handful of traces during debug than it's likely an overkill.

[maforget]

Sure ILogger seems better for logging, just mentioning that Trace are supported.