sashapolo commented on code in PR #3140:
URL: https://github.com/apache/ignite-3/pull/3140#discussion_r1474568356
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -592,4 +594,52 @@ public T2 next() {
}
};
}
+
+ /**
+ * Appends the specified element to the end of copy of list.
+ *
+ * @param <T> Type of elements of the list.
+ * @param list List.
+ * @param t Element to append.
+ * @return Immutable list with an appended element.
+ */
+ public static <T> List<T> addWithCopyList(List<? extends T> list, T t) {
+ if (list.isEmpty()) {
+ return List.of(t);
+ }
+
+ var newList = new ArrayList<T>(list.size() + 1);
+
+ newList.addAll(list);
+ newList.add(t);
+
+ return unmodifiableList(newList);
+ }
+
+ /**
+ * Removes the element at the specified position in copy of list.
+ *
+ * @param <T> Type of elements of the list.
+ * @param list List.
+ * @param index Index of the list element to be removed.
+ * @return Immutable list with an removed element.
+ */
+ public static <T> List<T> removeWithCopyList(List<? extends T> list, int
index) {
+ if (index == 0 && list.size() == 1) {
+ return List.of();
+ }
+
+ var newList = new ArrayList<T>(list.size() - 1);
+
+ if (index == 0) {
Review Comment:
I'm not convinced that this method is in any way better than:
```
var newList = new ArrayList(list);
newList.remove(index);
```
I'm also not convinced that:
```
int indexOf = listeners.indexOf(listener);
removeWithCopyList(listeners, indexOf);
```
Is in any way better than:
```
var newList = new ArrayList(listeners);
newList.remove(listener);
```
You are pretty much always removing a listener that is present in the list
(can you provide a scenario, when it's not?)
Please provide some benchmarks that will prove your point, otherwise I don't
think that this is a useful method.
##########
modules/core/src/main/java/org/apache/ignite/internal/event/AbstractEventProducer.java:
##########
@@ -39,30 +41,24 @@ public abstract class AbstractEventProducer<T extends
Event, P extends EventPara
@Override
public void listen(T evt, EventListener<? extends P> listener) {
listenersByEvent.compute(evt, (evt0, listeners) -> {
- List<EventListener<P>> newListeners;
-
- if (listeners == null) {
- newListeners = new ArrayList<>(1);
- } else {
- newListeners = new ArrayList<>(listeners.size() + 1);
-
- newListeners.addAll(listeners);
+ if (nullOrEmpty(listeners)) {
+ return List.of((EventListener<P>) listener);
}
- newListeners.add((EventListener<P>) listener);
-
- return unmodifiableList(newListeners);
+ return addWithCopyList(listeners, (EventListener<P>) listener);
});
}
@Override
public void removeListener(T evt, EventListener<? extends P> listener) {
listenersByEvent.computeIfPresent(evt, (evt0, listeners) -> {
- var newListeners = new ArrayList<>(listeners);
+ if (nullOrEmpty(listeners)) {
+ return null;
+ }
- newListeners.remove(listener);
+ int indexOf = listeners.indexOf(listener);
- return newListeners.isEmpty() ? null :
unmodifiableList(newListeners);
+ return indexOf < 0 ? listeners : removeWithCopyList(listeners,
indexOf);
Review Comment:
I would also add a check that if this is the last listener, then we would
return `null`, not an empty list
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java:
##########
@@ -592,4 +594,52 @@ public T2 next() {
}
};
}
+
+ /**
+ * Appends the specified element to the end of copy of list.
+ *
+ * @param <T> Type of elements of the list.
+ * @param list List.
+ * @param t Element to append.
+ * @return Immutable list with an appended element.
+ */
+ public static <T> List<T> addWithCopyList(List<? extends T> list, T t) {
Review Comment:
Do we really need this utility method? We only use it in one place and the
code for it is trivial
##########
modules/core/src/main/java/org/apache/ignite/internal/event/AbstractEventProducer.java:
##########
@@ -39,30 +41,24 @@ public abstract class AbstractEventProducer<T extends
Event, P extends EventPara
@Override
public void listen(T evt, EventListener<? extends P> listener) {
listenersByEvent.compute(evt, (evt0, listeners) -> {
- List<EventListener<P>> newListeners;
-
- if (listeners == null) {
- newListeners = new ArrayList<>(1);
- } else {
- newListeners = new ArrayList<>(listeners.size() + 1);
-
- newListeners.addAll(listeners);
+ if (nullOrEmpty(listeners)) {
+ return List.of((EventListener<P>) listener);
}
- newListeners.add((EventListener<P>) listener);
-
- return unmodifiableList(newListeners);
+ return addWithCopyList(listeners, (EventListener<P>) listener);
});
}
@Override
public void removeListener(T evt, EventListener<? extends P> listener) {
listenersByEvent.computeIfPresent(evt, (evt0, listeners) -> {
- var newListeners = new ArrayList<>(listeners);
+ if (nullOrEmpty(listeners)) {
Review Comment:
I guess that listeners can never be null or empty here, because it's inside
`computeIfPresent`
--
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]
