-
Notifications
You must be signed in to change notification settings - Fork 150
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.
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 #334
NLogMessageParameterList - Reduce code complexity for IsValidParameterList #334
Conversation
257b77f
to
fa66951
Compare
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
=========================================
- Coverage 81.98% 81.88% -0.1%
=========================================
Files 12 12
Lines 1110 1104 -6
Branches 195 193 -2
=========================================
- Hits 910 904 -6
Misses 136 136
Partials 64 64
Continue to review full report at Codecov.
|
fa66951
to
6a188d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to reduce the complexity? In short the (almost) only way is to split out code to another method/class (when keeping the same semantics)
if (!isPositional && i != 0) | ||
isMixedPositional = true; | ||
if (!isPositional) | ||
isMixedPositional = i != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantics changed here. Is that on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the result is the same. But one less condition.
{ | ||
hasMessageTemplateCapture = true; | ||
} | ||
isMixedPositional = isPositional; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the result is the same. But less conditions.
Yes while keeping performance |
Thanks! |
2nd try :)