Refactor InternalLogger class #2314
Conversation
Refactor InternalLogger class to improve code readabiity and create further opportunities to improve the code. There are no functional changes or changes to the public interface. The method Write() in InternalLogger class split into smaller methods * FormatMessage() * WriteToLogFile() * WriteToTextWriter() * WriteToConsole() * WriteToErrorConsole() LoggingEnabled() method renamed to IsLoggingEnabled() in alignment to IsSeriousException(). It also convey better to the reader the purpose of the method.
Codecov Report
@@ Coverage Diff @@
## master #2314 +/- ##
=======================================
- Coverage 82% 82% -<1%
=======================================
Files 317 317
Lines 22716 22736 +20
Branches 2784 2782 -2
=======================================
+ Hits 18618 18624 +6
- Misses 3396 3410 +14
Partials 702 702 |
src/NLog/Common/InternalLogger.cs
Outdated
| var formattedMessage = | ||
| (args == null) ? message : string.Format(CultureInfo.InvariantCulture, message, args); | ||
|
|
||
| var builder = new StringBuilder(formattedMessage.Length + TimeStampFormat.Length + 8); |
There was a problem hiding this comment.
Also small change here I see, maybe also check IncludeTimestamp?
(and exception?)
src/NLog/Common/InternalLogger.cs
Outdated
| @@ -367,6 +344,64 @@ private static bool LoggingEnabled(LogLevel logLevel) | |||
| LogWriter != null; | |||
| } | |||
|
|
|||
| private static void WriteToLogFile(string message) | |||
There was a problem hiding this comment.
Please add some docs, most important is here that logging will be skipped if not enabled. (multiple times)
ie-zero
force-pushed
the
InternalLogger
branch
from
cbbafdc
to
5097e5e
Compare
* Document WriteTo... methods in InternalLogger class * Reserve large enough buffer space in memory so the StringBuilder can hold the formatted output message without resizing.
src/NLog/Common/InternalLogger.cs
Outdated
| @@ -288,7 +288,7 @@ private static string FormatMessage([CanBeNull]Exception ex, LogLevel level, str | |||
| var formattedMessage = | |||
| (args == null) ? message : string.Format(CultureInfo.InvariantCulture, message, args); | |||
|
|
|||
| var builder = new StringBuilder(formattedMessage.Length + TimeStampFormat.Length + 8); | |||
| var builder = new StringBuilder(formattedMessage.Length + TimeStampFormat.Length + ex?.ToString()?.Length ?? 0 + 25); | |||
There was a problem hiding this comment.
0 + 25? Should be 8+25?
or 0 + 0+ 0+ 0+ 0+ 0+ 0+ 0 + 25 :P
There was a problem hiding this comment.
formattedMessage.Length and TimeStampFormat.Length (constant length) should never be 0. ex?.ToString()?.Length ?? 0 might be zero and the rest of white-spaces the literal 'Exception' and the LogLevel as a string sum up, if I recall correctly, to 23 characters.
To be honest, I am not certain if pre-calculating the length of the expected string in such detail for optimisation is positive.
What do you think?
There was a problem hiding this comment.
true, pre-optimized could be done later.
I think i was reading it as
ex?.ToString()?.Length ?? (0 + 25)
are you sure it's
(ex?.ToString()?.Length ?? 0) + 25
I have to lookup the priority of ??
src/NLog/Common/InternalLogger.cs
Outdated
| var formattedMessage = | ||
| (args == null) ? message : string.Format(CultureInfo.InvariantCulture, message, args); | ||
|
|
||
| var builder = new StringBuilder(formattedMessage.Length + TimeStampFormat.Length + ex?.ToString()?.Length ?? 0 + 25); |
There was a problem hiding this comment.
Resharper says this is ex?.ToString()?.Length ?? (0 + 25)
So this is wrong ;)
|
Merged. Thanks! |
Refactor InternalLogger class to improve code readabiity and create further opportunities to improve the code.
There are no functional changes or changes to the public interface.
The method Write() in InternalLogger class split into smaller methods
LoggingEnabled() method renamed to IsLoggingEnabled() in alignment to IsSeriousException(). It also convey better to the reader the purpose of the method.