NightOwl888 commented on code in PR #1042:
URL: https://github.com/apache/lucenenet/pull/1042#discussion_r1851990264
##########
src/Lucene.Net/Support/CRC32.cs:
##########
@@ -29,7 +27,7 @@ internal class CRC32 : IChecksum
private static uint[] InitializeCRCTable()
Review Comment:
Given that the convention we typically follow for these is to use a `Load`
prefix, let's do that here instead of `Initialize`. Also, add the comment
```c#
// LUCENENET: Avoid static constructors (see
https://github.com/apache/lucenenet/pull/224#issuecomment-469284006)
```
##########
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs:
##########
@@ -199,26 +201,26 @@ private IEnumerable<BytesRef> GetBytesRefEnumberable(int
valueCount, int[] sorte
}
}
- private IEnumerable<long?> GetOrdsEnumberable(int maxDoc)
+ private IEnumerable<long?> GetOrdCountEnumerable(int maxDoc)
{
- AppendingDeltaPackedInt64Buffer.Iterator iter =
pendingCounts.GetIterator();
+ var iter = pendingCounts.GetIterator();
- if (Debugging.AssertsEnabled) Debugging.Assert(pendingCounts.Count
== maxDoc,"MaxDoc: {0}, pending.Count: {1}", maxDoc, pending.Count);
+ if (Debugging.AssertsEnabled) Debugging.Assert(pendingCounts.Count
== maxDoc, "MaxDoc: {0}, pending.Count: {1}", maxDoc, pending.Count);
- for (int i = 0; i < maxDoc; ++i)
+ for (int docUpto = 0; docUpto < maxDoc; ++docUpto)
{
yield return iter.Next();
}
}
- private IEnumerable<long?> GetOrdCountEnumberable(int maxCountPerDoc,
int[] ordMap)
+ private IEnumerable<long?> GetOrdsEnumerable(int[] ordMap, int
maxCountPerDoc)
{
int currentUpTo = 0, currentLength = 0;
- AppendingPackedInt64Buffer.Iterator iter = pending.GetIterator();
- AppendingDeltaPackedInt64Buffer.Iterator counts =
pendingCounts.GetIterator();
- int[] currentDoc = new int[maxCountPerDoc];
+ var iter = pending.GetIterator();
+ var counts = pendingCounts.GetIterator();
Review Comment:
`var` only makes sense if it is clear from the line what type the variable
is. In this case, it is not because `GetIterator()` tells us nothing of the
type. Keep in mind, we may be reviewing this on GitHub without having an IDE
handy to tell what type `var` resolves to so having the type name visible in
the source code could be helpful.
##########
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs:
##########
@@ -175,21 +175,23 @@ public override void Flush(SegmentWriteState state,
DocValuesConsumer dvConsumer
ordMap[sortedValues[ord]] = ord;
}
- dvConsumer.AddSortedSetField(fieldInfo,
GetBytesRefEnumberable(valueCount, sortedValues),
+ dvConsumer.AddSortedSetField(fieldInfo,
+ // ord -> value
+ GetValuesEnumerable(valueCount, sortedValues),
- // doc -> ordCount
- GetOrdsEnumberable(maxDoc),
Review Comment:
I agree, these method names are easier to understand.
##########
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs:
##########
@@ -199,26 +201,26 @@ private IEnumerable<BytesRef> GetBytesRefEnumberable(int
valueCount, int[] sorte
}
}
- private IEnumerable<long?> GetOrdsEnumberable(int maxDoc)
+ private IEnumerable<long?> GetOrdCountEnumerable(int maxDoc)
{
- AppendingDeltaPackedInt64Buffer.Iterator iter =
pendingCounts.GetIterator();
+ var iter = pendingCounts.GetIterator();
Review Comment:
`var` only makes sense if it is clear from the line what type the variable
is. In this case, it is not because `GetIterator()` tells us nothing of the
type. Keep in mind, we may be reviewing this on GitHub without having an IDE
handy to tell what type `var` resolves to so having the type name visible in
the source code could be helpful.
--
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: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
