fluffynuts commented on code in PR #203:
URL: https://github.com/apache/logging-log4net/pull/203#discussion_r1825457681
##########
src/log4net.Tests/Appender/RollingFileAppenderTest.cs:
##########
@@ -524,23 +523,10 @@ private static string GetCurrentFile() =>
/// <param name="sBackupGroup"></param>
/// <param name="iBackupFileLength"></param>
/// <returns></returns>
- private static List<RollFileEntry>
MakeBackupFileEntriesFromBackupGroup(string sBackupGroup, int iBackupFileLength)
- {
- string[] sFiles = sBackupGroup.Split(' ');
-
- List<RollFileEntry> result = [];
-
- for (int i = 0; i < sFiles.Length; i++)
- {
- // Weed out any whitespace entries from the array
- if (sFiles[i].Trim().Length > 0)
- {
- result.Add(new RollFileEntry(sFiles[i], iBackupFileLength));
- }
- }
-
- return result;
- }
+ private static List<RollFileEntry>
MakeBackupFileEntriesFromBackupGroup(string sBackupGroup, int
iBackupFileLength)
+ => sBackupGroup.Split(' ').Where(file => file.Trim().Length > 0)
Review Comment:
personal note: i really don't like inline method bodies - it's like I take a
little longer to grok them over, eg
```csharp
private static List<RollFileEntry> MakeBackupFileEntriesFromBackupGroup(
string sBackupGroup,
int iBackupFileLength
)
{
return sBackupGroup.Split(' ')
.Where(file => file.Trim().Length > 0)
.Select(file => new
RollFileEntry(file, iBackupFileLength))
.ToList()
}
```
Perhaps it's just me though...
I'd also like to make two "soft" suggestions for code style going forward:
1. vertical parameters (as in my example above) - see
https://youtu.be/1WL-SudgJAo (I had to re-upload this because it had been
removed, so I got it from archive.org and re-upped)
2. dropping type prefixes for variables / parameters (eg _i_BackupFileLength
and _s_BackupGroup)
I wouldn't go on a major refactoring spree, just fix things as I'm in the
area.
##########
src/log4net.Tests/Appender/AdoNetAppenderTest.cs:
##########
@@ -80,69 +80,70 @@ public void BufferingTest()
}
log.Debug("Message");
Assert.That(Log4NetCommand.MostRecentInstance, Is.Not.Null);
- Assert.That(Log4NetCommand.MostRecentInstance!.ExecuteNonQueryCount,
Is.EqualTo(bufferSize + 1));
+ Assert.That(Log4NetCommand.MostRecentInstance.ExecuteNonQueryCount,
Is.EqualTo(bufferSize + 1));
}
[Test]
public void WebsiteExample()
{
XmlDocument log4NetConfig = new();
- log4NetConfig.LoadXml(@"
- <log4net>
- <appender name=""AdoNetAppender""
type=""log4net.Appender.AdoNetAppender"">
- <bufferSize value=""-1"" />
- <connectionType
value=""log4net.Tests.Appender.AdoNet.Log4NetConnection"" />
- <connectionString value=""data source=[database
server];initial catalog=[database name];integrated security=false;persist
security info=True;User ID=[user];Password=[password]"" />
- <commandText value=""INSERT INTO Log
([Date],[Thread],[Level],[Logger],[Message],[Exception]) VALUES (@log_date,
@thread, @log_level, @logger, @message, @exception)"" />
- <parameter>
- <parameterName value=""@log_date"" />
- <dbType value=""DateTime"" />
- <layout type=""log4net.Layout.RawTimeStampLayout"" />
- </parameter>
- <parameter>
- <parameterName value=""@thread"" />
- <dbType value=""String"" />
- <size value=""255"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%thread"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@log_level"" />
- <dbType value=""String"" />
- <size value=""50"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%level"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@logger"" />
- <dbType value=""String"" />
- <size value=""255"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%logger"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@message"" />
- <dbType value=""String"" />
- <size value=""4000"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%message"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@exception"" />
- <dbType value=""String"" />
- <size value=""2000"" />
- <layout type=""log4net.Layout.ExceptionLayout"" />
- </parameter>
- </appender>
- <root>
- <level value=""ALL"" />
- <appender-ref ref=""AdoNetAppender"" />
- </root>
- </log4net>");
+ log4NetConfig.LoadXml("""
Review Comment:
suggest: with raw strings, I find it useful to do them in the format:
```csharp
log4NetConfig.LoadXml(
"""
<log4net>
....
</log4net>
"""
```
so that the stripped whitespace is more obvious (lining up the opening """
with the start/end text and the closing """) - indeed, I had to look up what
happens here because Rider always marks raw strings with alignment like this as
an issue - to find that the whitespace preceding the _final line_ is stripped
from the beginning of each line (
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/raw-string
). So in the test code here, the entire file looks like it would be prefixed
with 2 spaces, where the example I've given is completely left-aligned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
