Removing unnecessary copy on serialization#22
Removing unnecessary copy on serialization#22rwkarg wants to merge 2 commits intoemertechie:masterfrom
Conversation
grubman
left a comment
grubman
left a comment
There was a problem hiding this comment.
I Think that the upside of using "Length" over "Any()" is insignificant compare to the downside of changing the property type from IEnumerable to an Array.
(If we were using .net 4.5 I would've recommend using IReadOnlyCollection instead of IEnumerable)
Please change the PR so you only delete the unnecessary ToList.
|
|
||
| var structuredData = message.StructuredDataElements?.ToList(); | ||
| if (structuredData != null && structuredData.Any()) | ||
| var structuredData = message.StructuredDataElements; |
There was a problem hiding this comment.
The ToList is indeed not necessary in this case, it should be removed to avoid a potential costly materialization.
| } | ||
|
|
||
| public IEnumerable<StructuredDataElement> StructuredDataElements | ||
| public StructuredDataElement[] StructuredDataElements |
There was a problem hiding this comment.
unnecessary change, you should leave it as IEnumerable
| private readonly string msgId; | ||
| private readonly string message; | ||
| private readonly IEnumerable<StructuredDataElement> structuredDataElements; | ||
| private readonly StructuredDataElement[] structuredDataElements; |
There was a problem hiding this comment.
unnecessary change, you should leave it as IEnumerable
| var structuredData = message.StructuredDataElements?.ToList(); | ||
| if (structuredData != null && structuredData.Any()) | ||
| var structuredData = message.StructuredDataElements; | ||
| if (structuredData != null && structuredData.Length != 0) |
There was a problem hiding this comment.
unnecessary change, you should leave it as Any()
3 participants
SyslogMessage.StructuredDataElement is accepted as an array but was being stored and surfaced through the get property as an IEnumerable<>. This required a .ToList() on every serialization request as well as a .Any().
Changing the backing field and get property to an array removes the copy from the .ToList() and allows a .Length != 0 instead of the Linq .Any().
It is mostly a non-breaking change as any usage of the SyslogMessage.StructuredDataElement assuming an IEnumerable<> will work against the new array type. Inspection of the actual type returned will continue to be an array as before. The only potential for a breaking change would be code that depends on the defined type of the property (not the type of what's returned) being an IEnumerable<> but that seems like an odd requirement for code to depend on and is easily updated by changing the expected type.