Conversation
Initialize project
Implement abstractions
Implement balanced tree class
…lement private methods for balancing
Implement BST and traverse method; Refactor abstractNode
Implement 1 balance method instead of 2
…ling and get grandParent
…d instead of private
Implement red-black node class and red-black tree class
Apply fixes in AVLTree and abstractTree
Implement tests for a reb-black tree
…t coverage analysis
Implement pack of tests for BST and do refactoring (add exception for null parent of non-root node)
sgrishkova
left a comment
There was a problem hiding this comment.
Понравилось, как у вас хорошо все организовано (например, переиспользование кода для поворотов, продуманная архитектура, понятные названия методов) и выполнено в едином стиле.
| @@ -0,0 +1,6 @@ | |||
| package nodes | |||
|
|
|||
| abstract class abstractNode<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>(val key:K, var value: V) { | |||
There was a problem hiding this comment.
Согласно котлиновскому Coding conventions названия для классов лучше начинать с заглавной буквы.
| abstract class abstractNode<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>(val key:K, var value: V) { | |
| abstract class AbstractNode<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>(val key:K, var value: V) { |
|
|
||
| import nodes.abstractNode | ||
|
|
||
| abstract class abstractTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>> { |
There was a problem hiding this comment.
Тоже названия классов.
| abstract class abstractTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>> { | |
| abstract class AbstractTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>> { |
| var leftChild: someNode? = null | ||
| var rightChild: someNode? = null |
There was a problem hiding this comment.
Хорошо ли, когда ссылки на детей узла public?
Вижу, что ни один из public методов не возвращает пользователю узел, т. е. трюк в духе
node1.leftChild = node2 пользователю провернуть будет не так просто, однако можно подумать над использованием модификатора internal.
There was a problem hiding this comment.
Действительно, безопасность данных мы в первую очередь обеспечивали через устойчивость методов, так как именно за счет них происходит взаимодействие пользователя с библиотекой. Поэтому проблемы с таким решением маловероятны, но замечание на будущее хорошее, спасибо
| package nodes | ||
|
|
||
| class AVLNode<K : Comparable<K>, V>(key: K, value: V): abstractNode<K, V, AVLNode<K, V>>(key, value) { | ||
| var height = 1 |
There was a problem hiding this comment.
Стоит ли оставлять высоту узла public? Если кто-то вдруг получит доступ к узлу, то можно будет сделать что-нибудь в духе node.height = 42, а тогда и балансировка AVL поломается.
There was a problem hiding this comment.
Возможно, тут тот же случай, что и выше. Хотя если пользователь добился доступа к ноде, ему ничего не помешает обходными путями сделать то же самое и здесь :)
|
|
||
| import nodes.abstractNode | ||
|
|
||
| abstract class balancedTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>: abstractTree<K, V, someNode>() { |
There was a problem hiding this comment.
Та же история про название класса.
| abstract class balancedTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>: abstractTree<K, V, someNode>() { | |
| abstract class BalancedTree<K: Comparable<K>, V, someNode: abstractNode<K, V, someNode>>: abstractTree<K, V, someNode>() { |
| } | ||
|
|
||
| protected fun traverse(curNode: someNode?, listOfNodes: MutableList<someNode>) { | ||
| if(curNode != null) { |
There was a problem hiding this comment.
| if(curNode != null) { | |
| if (curNode != null) { |
| import org.junit.jupiter.api.Assertions.assertEquals | ||
| import kotlin.test.Test | ||
|
|
||
|
|
There was a problem hiding this comment.
Почему не используете @BeforeEach и @Nested? Кажется будто некоторые тесты для удобства можно сгруппировать и автоматически создавать для них одно и то же дерево.
There was a problem hiding this comment.
С этим согласна, учтем на будущее.
| val actualValue = tree.find(2) | ||
| assertEquals(expectedValue, actualValue) | ||
| } | ||
|
|
There was a problem hiding this comment.
Еще можно отдельными тестами проверить:
- insert на пустом дереве
- при вставке нод с меньшим ключём уходит влево
- при вставке нод с большим ключём уходит вправо
There was a problem hiding this comment.
У нас есть тесты, проверяющие insert и любой случай (вправо/влево/перезапись). Иначе бы и соответствующего покрытия не было. Выносить еще какие-то тесты было бы излишним.
There was a problem hiding this comment.
Я вижу, что у вас это проверяется, но если судить тесты только по названию, то insert node and perform left-right rotation будто проверяет только то, что узел куда-то вставляется, а потом совершается поворот.
То есть, если что-то пойдет не так, и тест завершится с ошибкой, первая мысль будет проверять повороты, хотя проблема может крыться в другом.
There was a problem hiding this comment.
Если вы еще раз посмотрите на реализацию, то увидите, что повороты вызываются исключительно внутри balance(). Так что я не вижу проблему.
| val actualKeysAndHeights = tree.preorderTraverse() | ||
| assertEquals(expectedKeysAndHeights, actualKeysAndHeights) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Много тестов на повороты, но кажется будто стоит отдельно проверить, получилось ли дерево в итоге (после вставки / удаления) сбалансированным. Правильно выполненный поворот еще не гарантирует сбалансированность (вдруг что-то было пропущено и дерево осталось несбалансированным).
There was a problem hiding this comment.
Метод preorderTraverse() организован таким образом, что по возвращенному из него списку можно однозначно восстановить расположение узлов в дереве, а следовательно и его полную структуру. В наших тестах мы в качестве expectedKeysAndHeights подаем список вершин и их высот, который вернулся бы из preorderTraverse() в случае корректной работы - сбалансированного дерева как результат (то есть, когда все дети в правильных местах, а в случае АВЛ и РБ еще и соблюдены правила балансировки). Поэтому сравнить фактический список, пришедший из метода, с составленным вручную, достаточно, чтобы утверждать корректность не только узлов дерева, но и его структуры. Тестов на повороты не много, они покрывают каждый из 4 кейсов в методе balance.
There was a problem hiding this comment.
"Много" имела в виду в хорошем смысле, что рассмотрены не отдельные кейсы, а все.
Насчет сбланасированности согласна, придирка скорее к описанию тестов.
insert node and perform left-right rotation будто проверяет только поворот, а не баланс в целом. Да, по полученному списку можно проверить и сбалансированность, но будто это что-то косвенное. То есть если тест зафейлится, не будет сразу понятно, что пошло не так: поворот не сработал, или балансируется дерево не полностью.
Кажется будто это может отнять лишнее время при дебаге (особенно если человек потеинцально не знаком с кодом).
There was a problem hiding this comment.
Опять-таки, это не что-то косвенное, так как повороты нужны только в балансировке. А если тест упадет, то найти ошибку поможет как раз actualKeysAndHeights. Конечно, мы напрямую не узнаем, в каком именно она методе, так как мы их не можем вызвать напрямую (они же не public). Но наш вывод служит хорошей подсказкой.
| fun find(key: K): V? { | ||
| return findNodeByKey(key)?.value | ||
| } |
There was a problem hiding this comment.
Кажется будто название этого метода можно уточнить. По принимаемым аргументам и по ридми понятно, что осуществляется поиск по ключу, но в теории искать можно и по value.
There was a problem hiding this comment.
Вся функциональность метода описана в ридми, и мы предполагаем, что пользователь прочитает его перед использованием библиотеки. Вообще, поиск узла по значению -- немного странное действие, учитывая, что в нашем дереве мы работаем исключительно по ключам, а значения там появляются только в контексте ввода/вывода. Реализация поиска по значению напоминала бы простой обход, что в принципе ломает концепцию дерева как эффективной структуры хранения/взаимодействия с упорядоченными данными. Как дополнительную фичу такой поиск, конечно, можно было бы реализовать, но точно не как основной метод
| private fun getGrandparent(node: RBNode<K, V>): RBNode<K, V>? { | ||
| val parent = findParent(node) ?: return null | ||
| val grandparent = findParent(parent) | ||
| return grandparent | ||
| } | ||
|
|
||
| private fun getSibling(node: RBNode<K, V>): RBNode<K, V>? { | ||
| val parent = findParent(node) ?: return null | ||
| return if (node == parent.leftChild) { | ||
| parent.rightChild | ||
| } else { | ||
| parent.leftChild | ||
| } | ||
| } | ||
|
|
||
| private fun getUncle(node: RBNode<K, V>): RBNode<K, V>? { | ||
| val parent = findParent(node) ?: return null | ||
| val grandparent = findParent(parent) | ||
| return if (parent == grandparent?.leftChild) { | ||
| grandparent.rightChild | ||
| } else { | ||
| grandparent?.leftChild | ||
| } | ||
| } |
There was a problem hiding this comment.
Каждый раз идти от корня дерева в поиске родителя нода?
В балансировке после ставки это повышает асимптотику до O(log n * log n), в теории и в балансировке после удаления тоже.
Зачем по два раза искать родителя в getGrandparent и getUncle? Переиспользование методов это прикольно, но тут ноды совсем уж рядом расположены.
There was a problem hiding this comment.
Использовать или не использовать ссылку на родителя было опциональным решением. В нашем случае, выбранная стратегия позволила повысить переиспользуемость кода (например, поворотов) и уменьшить вероятность ошибок, связанных с кривой привязкой родителя.
| fun preorderTraverse(): List<Pair<K, Color>> { | ||
| val listOfNodes = mutableListOf<RBNode<K, V>>() | ||
| traverse(root, listOfNodes) | ||
| val listOfKeysAndColors = mutableListOf<Pair<K, Color>>() | ||
| listOfNodes.forEach { listOfKeysAndColors.add(Pair(it.key, it.color)) } | ||
| return listOfKeysAndColors | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Кому нужен такой итератор, тестерам? А ой, действительно нужен...(´・ᴗ・`)
Если серьёзно, то возникает куча вопросов: почему возвращает не значение нодов, но цвет(что пользователю делать с ним?), почему итератор сделан так, а не с помощью интерфейса Iterator, например, почему preorder, а не сортированный по возрастанию ключей inorder, или вообще 3 разных метода под все order'ы?
Это даже итератором можно назвать с натяжкой, потому что массив пар - не итератор(впрочем, уже по этому массиву действительно можно итерироваться).
There was a problem hiding this comment.
Начнем с того, что слова "итератор" ни в коде, ни в презентации нашей архитектуры, ни в требованиях к реализации библиотеки не было. Цитата из условия задания: "Также необходимо поддержать возможность итерирования по ключам и значениям, хранящимся в дереве, например, в порядке обхода в ширину или глубину (или как-то ещё иначе)". Мы были вольны выбрать любой вариант, писать свой метод или реализовывать итератор. Наш вариант реализует (упомянутый в условии) обход в глубину и описан в README довольно понятно, да и примеры приведены. Идея такого вывода пришла мне, поэтому поясню: так как библиотекой пользуются программисты, показалось, что им могут быть полезны такие сведения о структуре дерева, в том числе и цвет (в АВЛ с той же целью присутствует высота, для простого и понятного представления дерева); мы не преследовали цель как-то упростить себе тестирование, но впоследствии оказалось и правда удобно использовать обход. Выбор именно preorder и отсутствие вывода значений я уже прокомментировала вашей сокоманднице.
There was a problem hiding this comment.
Я правильно понимаю, что чтобы пройти все узлы дерева(и достать значения), нужно доставать каждый ключ из preorderTraversal, затем с помощью find находить соответствующий value?
There was a problem hiding this comment.
Если вопрос в том, как доставать соответствующие ключам значения, то мы уже выяснили, что наша реализация не поддерживает этого в методе preorderTraverse, просто из-за излишней нагроможденности. Пользователь может вывести конкретное значение с помощью find, и мы посчитали это достаточным. Прикрепляю скрин, почему нам не очень импонирует идея выводить значения.
lib/src/main/kotlin/trees/RBTree.kt
Outdated
| private fun deleteCase1(node: RBNode<K, V>) { | ||
| val parent = findParent(node) | ||
| if(parent != null) deleteCase2(node) | ||
| } | ||
|
|
There was a problem hiding this comment.
Да, я тоже респектую автору страницы про rb на вики, но ссылочку можно было и оставить в readme, раз код скопирован чуть менее чем полностью.
Это же относиться к балансировке после вставки.
There was a problem hiding this comment.
Насколько я знаю, технология не является запатентованной или лицензированной, и указания авторства не требует. Более того, RB-дерево -- довольно популярная и много раз написанная-переписанная идея. А страница на вики есть и, например, у сортировки пузырьком :))
There was a problem hiding this comment.
Вообще, случаи, когда-куда-что вставлять, поворачивать и перекрашивать -- это вопрос не конкретной реализации, а устройства красно-черного дерева. Вопрос, как конкретно -- это уже реализация. Код все равно получается разный, и если вы руководствовались другой статьей, поясняющей работу КЧ, то это ваш выбор. delete() разделен на кейсы, возможно, из-за сильной громоздкости, хотя у меня есть свои причины против этого подхода.
There was a problem hiding this comment.
При чём тут технологии? Речь про код.
delete() разделён на кейсы не из-за сильной громоздкости, а потому что код в википедии разделён на кейсы.
There was a problem hiding this comment.
все еще не понимаю суть проблемы. красно-черное дерево написано задолго до нас, изобретать велосипед не было смысла изначально (и вряд ли бы получилось. учитывая количество реализаций, не знаю, как надо было извернуться, чтобы не быть ни на кого похожим. а главное, зачем). по поводу ссылки все было сказано выше. BTW чекните ссылки в about the project ридми, которые были там изначально ;))
жду появления в репо второй домашки первого сема ссылки на используемую сортировку, кстати :)
There was a problem hiding this comment.
Ладно, закрываю тред с позором. ╥﹏╥
Просто мне казалось(и, если честно, до сих пор кажется), что если можно копировать реализацию методов с сайтов, то это ничем не отличается от "списывания" у других людей. Whatever
А вот касательно ссылок - тут я знатно опростоволосился и не заметил их(хотя и похоже на то, что вдохновение принесла как раз русская страничка в википедии, а не англицкая). Всё, пойду проветривать помещение
There was a problem hiding this comment.
на лекции как-то раз объясняли, что, на наше же благо, патентованием алгоритмов занимаются только в очень сложных случаях, когда речь идет о, зачастую, огромной коммерческой выгоде и/или сложности патентуемого алгоритма. когда речь идет о предмете вроде красно-черного дерева, это все равно, что попытаться присвоить себе слово "окно" и заявлять, что никто больше не имеет права им пользоваться. пойду, кстати, тоже открою
| /* case 1: insertedNode is root */ | ||
| if (root == insertedNode) { | ||
| insertedNode.color = Color.BLACK | ||
| return | ||
| } |
There was a problem hiding this comment.
Почему для удаления выделены отдельные методы(removeCase1,2,3,...), а в балансировке после добавления тут всё в одном методе?
There was a problem hiding this comment.
В удалении кейсы решили вынести в отдельные методы просто для читаемости, потому что там код намного массивнее, чем во вставке. С заботой о ваших глазках :3
lib/src/main/kotlin/trees/RBTree.kt
Outdated
| val uncle = getUncle(insertedNode) | ||
| val grandparent = getGrandparent(insertedNode) ?: throw IllegalArgumentException("Every node by that point should have grandparent") |
There was a problem hiding this comment.
grandparent же буквально указывает на uncle, его найти - вопрос одного if else(´。_。`)
Но выглядит доходчиво и понятно, этого не отнять.
lib/src/main/kotlin/trees/RBTree.kt
Outdated
| import nodes.Color | ||
| import nodes.RBNode | ||
|
|
||
| class RBTree<K : Comparable<K>, V>: balancedTree<K, V, RBNode<K, V>>() { |
There was a problem hiding this comment.
Форматирование хромает, какие-то методы отделены друг от друга, какие-то - нет, также внутри методов тоже есть мелочи аля разное написание if else.
lib/src/main/kotlin/trees/RBTree.kt
Outdated
| private fun deleteLeaf(node: RBNode<K, V>) { | ||
| if (node.color != Color.RED) deleteCase1(node) | ||
| } |
There was a problem hiding this comment.
Используется один раз + название deleteLeaf... стоит ли говорить, что листовые узлы здесь, тащемта, null.
Да, докопался, а кому сейчас легко?
| import kotlin.test.Test | ||
|
|
||
|
|
||
| class RBTreeTest { |
There was a problem hiding this comment.
Jacoco показывает покрытие 72% веток.
Это следствие распыления балансировки после удаления на много разных методов, но всё равно неприятно. Впрочем, если отмести все ветки с exceptions, нарушающие инварианты rb, то покрытие уже неплохое.
There was a problem hiding this comment.
У меня 76%, так что мы в эпсилон-окрестности крутого покрытия
There was a problem hiding this comment.
Неполное покрытие некоторых веток в основном связано с большим количеством исключений в реализации КЧ-дерева, их тестами не получится покрыть, а также с проверкой вершин на null (проверяем, является ли вершина null, и если нет, то она по умолчанию имеет чёрный цвет), но ведь в КЧ-дереве null вершины (leaf) и так являютя чёрными... упс! Этого можно было бы избежать, сделав отдельный класс для leaf-вершин, чтобы он всегда был чёрный, но мы решили этого не делать. ну типа KISS и все такое, покрытие в любом случае вышло приемлемое. Спасибо за вклад, Дамир!
| /* case 4: uncle is black, parent is red; grandparent, parent and node form a "triangle" | ||
| * G G | ||
| * / \ | ||
| * P - left triangle P - right triangle | ||
| * \ / | ||
| * N N | ||
| * | ||
| */ |
There was a problem hiding this comment.
Схемы прикольные, я когда их увидел здесь, быстренько себе парочку подобных нарисовал :)
|
не увидел нигде никакой духоты отсутствие хоть какого-либо интереса к решению и беззубый ответ был бы намного оскорбительнее и удручающее, чем то, что я увидел я увидел интерес человека узнать больше про ваше решение, задать интересующие вопросы и высказать мысли о том, что ему показалось чем-то необычным не без заблуждений или ошибок — но мы все учимся я всегда был бы только рад увидеть подобный интерес к своей работе и получение такого фидбека (миша, твой фидбек мне очень нравится) @DronShock кто у нас душит кого? |
|
Тут была паста, которую попросили убрать |
|
@DronShock андрей... |
|
В интернете опять кто-то неправ :O |
Apply final fixes in AVL
… too many stack frames (bad practice)
Beautify RBTree's and RBTest's code
Perform a post-review refactoring that contains optimization of BSTree tests by using BeforeEach and refactoring of abstractions (abstract tree, balanced tree, abstract node) to fit them into Kotlin coding conventions
Update .gitignore
Delete unnecessary for user files
Some of comments and exceptions messages were changed same as one variable name (rightSubtree). Unnecessary "when" constructions were replaced by "if" conditions
Remove table of contents, add general info about installing lib
…on of test code achieved through designing the test part of the lib similar to the main one
Full post-review change of tests architecture. Optimization of test code achieved through designing the test part of the lib similar to the main one
👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @p1onerka @sofyak0zyreva @shvorobsofia