NightOwl888 commented on code in PR #809:
URL: https://github.com/apache/lucenenet/pull/809#discussion_r1161188606
##########
src/Lucene.Net/Util/RollingBuffer.cs:
##########
@@ -37,12 +36,35 @@ public interface IResettable
}
}
+ /// <summary>
+ /// LUCENENET specific interface to allow overriding rolling buffer item
creation
+ /// without having to call virtual methods from the constructor
+ /// </summary>
+ public interface IRollingBufferItemFactory<out T>
+ {
+ T Create(object rollingBuffer);
+ }
+
+ /// <summary>
+ /// LUCENENET specific class that provides default implementation for
+ /// <see cref="IRollingBufferItemFactory{T}"/>.
+ /// </summary>
+ public class RollingBufferItemFactory<T> : IRollingBufferItemFactory<T>
where T : new()
Review Comment:
I had considered what would be the case if we use the `new()` constraint
here. It works for all existing cases, but unfortunately it forces a default
constructor into implementations of this where it may not make any sense. I
realize this saves some factory implementations, but IMO forcing this
constructor to exist isn't worth the tradeoff. We can still share a
`PositionFactory` implementation in several cases.
##########
src/Lucene.Net/Util/RollingBuffer.cs:
##########
@@ -57,25 +79,17 @@ public abstract class RollingBuffer<T>
// How many valid Position are held in the
// array:
private int count;
+ private IRollingBufferItemFactory<T> factory;
- protected RollingBuffer()
- {
- for (var idx = 0; idx < buffer.Length; idx++)
- {
- buffer[idx] = NewInstance(); // TODO GIVE ROLLING BUFFER A
DELEGATE FOR NEW INSTANCE
- }
- }
-
- protected RollingBuffer(Func<T> factory)
+ protected RollingBuffer(IRollingBufferItemFactory<T> factory)
Review Comment:
Let's change the variable name to `itemFactory` to make it more clear what
it creates. And on other classes where it is injected, also.
##########
src/Lucene.Net/Util/RollingBuffer.cs:
##########
@@ -37,12 +36,35 @@ public interface IResettable
}
}
+ /// <summary>
+ /// LUCENENET specific interface to allow overriding rolling buffer item
creation
+ /// without having to call virtual methods from the constructor
+ /// </summary>
+ public interface IRollingBufferItemFactory<out T>
Review Comment:
Please eliminate the above `RollingBuffer` static class and move
`IResettable` up here to the `Lucene.Net.Util` namespace. Usages of
`IResettable` aren't constrained to `RollingBuffer` usage.
##########
src/Lucene.Net.Tests/Util/TestRollingBuffer.cs:
##########
@@ -90,23 +90,22 @@ public virtual void Test()
private sealed class RollingBufferAnonymousClass :
RollingBuffer<Position>
{
+ // LUCENENET specific - removed NewPosition override and using
factory instead
public RollingBufferAnonymousClass()
- : base(NewInstanceFunc)
+ : base(RollingBufferAnonymousClassFactory.Default)
{
}
- public static Position NewInstanceFunc()
+ private class RollingBufferAnonymousClassFactory :
IRollingBufferItemFactory<Position>
Review Comment:
Maybe just call this `PositionFactory`?
--
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
