Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLogMessageParameterList - Reduce code complexity for IsValidParameterList #333

Merged
merged 2 commits into from
Sep 16, 2019
Merged

NLogMessageParameterList - Reduce code complexity for IsValidParameterList #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ec45e4
NLogMessageParameterList - Reduce code complexity for IsValidParamete…
snakefoot Sep 15, 2019
311f149
NLogMessageParameterList - Reduce code complexity for IsValidParamete…
snakefoot Sep 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/NLog.Extensions.Logging/Logging/NLogBeginScopeParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ public static IDisposable CaptureScopeProperties(IReadOnlyList<KeyValuePair<stri
{
object scopeObject = scopePropertyList;

if (scopePropertyList.Count > 0 && scopePropertyList[scopePropertyList.Count - 1].Key == NLogLogger.OriginalFormatPropertyName)
if (scopePropertyList.Count > 0 && NLogLogger.OriginalFormatPropertyName.Equals(scopePropertyList[scopePropertyList.Count - 1].Key))
{
var propertyList = new List<KeyValuePair<string, object>>(scopePropertyList.Count - 1);
for (var i = 0; i < scopePropertyList.Count; ++i)
{
var property = scopePropertyList[i];
if (i == scopePropertyList.Count - 1 && i > 0 && property.Key == NLogLogger.OriginalFormatPropertyName)
if (i == scopePropertyList.Count - 1 && i > 0 && NLogLogger.OriginalFormatPropertyName.Equals(property.Key))
{
continue; // Handle BeginScope("Hello {World}", "Earth")
}
Expand Down
2 changes: 1 addition & 1 deletion src/NLog.Extensions.Logging/Logging/NLogLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ private void CaptureMessagePropertiesList(LogEventInfo eventInfo, IReadOnlyList<
if (String.IsNullOrEmpty(property.Key))
continue;

if (i == messageProperties.Count - 1 && property.Key == OriginalFormatPropertyName)
if (i == messageProperties.Count - 1 && OriginalFormatPropertyName.Equals(property.Key))
continue;

eventInfo.Properties[property.Key] = property.Value;
Expand Down
57 changes: 32 additions & 25 deletions src/NLog.Extensions.Logging/Logging/NLogMessageParameterList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace NLog.Extensions.Logging
internal class NLogMessageParameterList : IList<MessageTemplateParameter>
{
private readonly IReadOnlyList<KeyValuePair<string, object>> _parameterList;
private static readonly NLogMessageParameterList EmptyList = new NLogMessageParameterList(new KeyValuePair<string, object>[0]);
private static readonly NLogMessageParameterList EmptyList = new NLogMessageParameterList(new KeyValuePair<string, object>[0]);
private static readonly NLogMessageParameterList OriginalMessageList = new NLogMessageParameterList(new[] { new KeyValuePair<string, object>(NLogLogger.OriginalFormatPropertyName, string.Empty) });

public bool HasOriginalMessage => _originalMessageIndex.HasValue;
Expand Down Expand Up @@ -44,7 +44,7 @@ public NLogMessageParameterList(IReadOnlyList<KeyValuePair<string, object>> para
/// </remarks>
public static NLogMessageParameterList TryParse(IReadOnlyList<KeyValuePair<string, object>> parameterList)
{
if (parameterList.Count > 1 || (parameterList.Count == 1 && parameterList[0].Key != NLogLogger.OriginalFormatPropertyName))
if (parameterList.Count > 1 || (parameterList.Count == 1 && !NLogLogger.OriginalFormatPropertyName.Equals(parameterList[0].Key)))
{
return new NLogMessageParameterList(parameterList);
}
Expand Down Expand Up @@ -79,47 +79,43 @@ private static bool IsValidParameterList(IReadOnlyList<KeyValuePair<string, obje
{
hasMessageTemplateCapture = false;
isMixedPositional = false;
isPositional = false;
originalMessageIndex = null;
bool? firstParameterIsPositional = null;
isPositional = false;
string parameterName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one moved to the outer scope?

Copy link
Contributor Author

@snakefoot snakefoot Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old c++ habbit. No need to push the same parameter on the stack for every loop-iteration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience is could cause memory leaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think it's a code smell in Sonar, not sure

Copy link
Contributor Author

@snakefoot snakefoot Sep 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only tried memory leaks in C# with dangling event-handlers or dispose-handles (keeps things alive after lifetime) or dynamic-compiled-code (can never be freed).


for (int i = 0; i < parameterList.Count; ++i)
{
if (!TryGetKey(parameterList, ref originalMessageIndex, i, out var parameterKey))
if (!TryGetParameterName(parameterList, i, ref originalMessageIndex, out parameterName))
{
return false;
}

char firstChar = parameterKey[0];
if (GetCaptureType(firstChar) != CaptureType.Normal)
char firstChar = parameterName[0];
if (firstChar >= '0' && firstChar <= '9')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is more expensive than before. but maybe not really significant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most users will always use CaptureType.Normal. Doing the isPositional-check first makes the code simpler. It will only change performance for those who always use @ in the parameter-tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this PR is to reduce the complexity of IsValidParameterList with minimal impact.

{
hasMessageTemplateCapture = true;
if (!isPositional && i != 0)
isMixedPositional = true;
isPositional = true;
}
else if (parameterKey == NLogLogger.OriginalFormatPropertyName)
else
{
if (originalMessageIndex.HasValue)
if (isPositional)
{
originalMessageIndex = null;
return false;
isMixedPositional = true;
}

originalMessageIndex = i;
}
else
{
if (!firstParameterIsPositional.HasValue)
firstParameterIsPositional = char.IsDigit(firstChar);
else if (char.IsDigit(firstChar) != firstParameterIsPositional)
isMixedPositional = true;
if (GetCaptureType(firstChar) != CaptureType.Normal)
{
hasMessageTemplateCapture = true;
}
}
}

if (firstParameterIsPositional == true && !isMixedPositional)
isPositional = true;

isPositional = isPositional && !isMixedPositional;
return true;
}

private static bool TryGetKey(IReadOnlyList<KeyValuePair<string, object>> parameterList, ref int? originalMessageIndex, int i, out string parameterKey)
private static bool TryGetParameterName(IReadOnlyList<KeyValuePair<string, object>> parameterList, int i, ref int? originalMessageIndex, out string parameterKey)
{
try
{
Expand All @@ -137,6 +133,17 @@ private static bool TryGetKey(IReadOnlyList<KeyValuePair<string, object>> parame
return false;
}

if (NLogLogger.OriginalFormatPropertyName.Equals(parameterKey))
{
if (originalMessageIndex.HasValue)
{
originalMessageIndex = null;
return false;
}

originalMessageIndex = i;
}

return true;
}

Expand All @@ -152,7 +159,7 @@ private static IReadOnlyList<KeyValuePair<string, object>> CreateValidParameterL
if (string.IsNullOrEmpty(paramPair.Key))
continue;

if (paramPair.Key == NLogLogger.OriginalFormatPropertyName)
if (NLogLogger.OriginalFormatPropertyName.Equals(paramPair.Key))
{
continue;
}
Expand Down
3 changes: 2 additions & 1 deletion test/NLog.Extensions.Logging.Tests/NLogMessageParameterListTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ public void CreateNLogMessageParameterListWithEmptyKey()
{
new KeyValuePair<string, object>("", 1),
new KeyValuePair<string, object>("a", 2),
new KeyValuePair<string, object>("b", 3)
new KeyValuePair<string, object>("b", 3),
new KeyValuePair<string, object>("{OriginalFormat}", "{0}{1}{2}"),
};
var list = new NLogMessageParameterList(items);

Expand Down