Качмар Евгений, ИТМО ФИТиП М33331, Reverse Iterator #297
Conversation
AlexeyShik
requested review from
ikriushenkov
and removed request for
AlexeyShik
| @@ -29,6 +29,10 @@ public InMemoryDao(Config conf) { | |||
|
|
|||
| @Override | |||
| public Iterator<Entry<MemorySegment>> get(MemorySegment from, MemorySegment to) { | |||
| if (new MemSegComparatorNull().compare(from, to) > 0) { | |||
There was a problem hiding this comment.
Создание объекта на каждый вызов get. Зачем?
There was a problem hiding this comment.
И вы все еще нарушаете инвариант Comparable:
The implementor must ensure that signum(compare(x, y)) == -signum(compare(y, x)) for all x and y. (This implies that compare(x, y) must throw an exception if and only if compare(y, x) throws an exception.)
| @@ -44,6 +48,11 @@ public Iterator<Entry<MemorySegment>> get(MemorySegment from, MemorySegment to) | |||
| return new SSTableIterator(dataSlice.values().iterator(), controller, from, to); | |||
| } | |||
|
|
|||
| private Iterator<Entry<MemorySegment>> reverseIter(MemorySegment fromRightSide, MemorySegment toLeftSide) { | |||
| return new SSTableIterator(getMemTable().reversed().values().iterator(), | |||
| controller, fromRightSide, toLeftSide, true, new MemSegComparatorNull().reversed()); | |||
There was a problem hiding this comment.
Здесь вы создаете еще 2 компаратора, один из которых полностью идентичен созданному ранее. Итого создание 3 компараторов при каждом вызове get, когда можно их вообще не создавать
There was a problem hiding this comment.
Всегда передается полная мапа, начало игнорируется, поэтому тестов, где from не последний элемент нет. Непонятно тогда, зачем обрезать конец мапы в случае натурального порядка, если он все равно учитывает конец внутри
| return; | ||
| } | ||
|
|
||
| if (!mp.containsKey(kv.key())) { |
There was a problem hiding this comment.
putIfAbsent
Не должно быть лишних обращений к структурам данных (особенно конкурентным)
| if (oldInfo.isReversedToIter) { | ||
| insertNew(mp, controller, controller.getPrevInfo(oldInfo, to), to); | ||
| } else { | ||
| insertNew(mp, controller, controller.getNextInfo(oldInfo, to), to); | ||
| } |
There was a problem hiding this comment.
Лучше заменить на цикл. Рекурсия тут не оправдана
| private final MemorySegment from; | ||
| private final MemorySegment to; | ||
| private Entry<MemorySegment> head; | ||
| private Entry<MemorySegment> keeper; | ||
| private boolean isReversed; |
| public SSTableIterator(Iterator<Entry<MemorySegment>> it, SSTablesController controller, | ||
| MemorySegment from, MemorySegment to, boolean isReversed, Comparator<MemorySegment> comp) { |
There was a problem hiding this comment.
Конструктор - копия первого, в котором почему-то явно не инициализировано isReversed, из-за чего оно не final
| if (isReversed) { | ||
| IteratorUtils.insertNew(mp, controller, | ||
| controller.getPrevInfo(mp.remove(key), to), to); | ||
| return; | ||
| } | ||
| IteratorUtils.insertNew(mp, controller, | ||
| controller.getNextInfo(mp.remove(key), to), to); |
There was a problem hiding this comment.
Хочется какой-то консистентности. Или все такие места
if (isReversed) {
...
} else {
...
}Или
if (isReversed) {
...
return
}
...| return r == numberOfEntries ? -1 : r; | ||
| } | ||
|
|
||
| public static long searchKeyInFileReversed(SSTablesController controller, |
There was a problem hiding this comment.
Просто идея
Функция аналогичная предыдущей. Раз вы уже выделили компаратор в отдельные функции, то можно просто передавать его как параметр
| } | ||
|
|
||
| @DaoTest(stage = 4, maxStage = 4) | ||
| void ssTablesCheckOrdinary(Dao<String, Entry<String>> dao) throws IOException { |
There was a problem hiding this comment.
Нет смысла копировать тесты, которые уже есть в других классах, так как решение тестируется на всех тестах.
| import java.io.IOException; | ||
| import java.util.List; | ||
|
|
||
| public class ReverseIteratorTest extends BaseTest { |
There was a problem hiding this comment.
Во всех тестах представлен только один вариант - с последнего до первого индекса. Варианты не с начала не созданы видимо потому что не работают, но хотелось бы увидеть другие тесты - на пустой итератор, на один элемент, на все хранилище в обратном порядке
No description provided.