-
Notifications
You must be signed in to change notification settings - Fork 0
Feedback #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feedback
Are you sure you want to change the base?
Feedback #1
Conversation
(it can't remove yet)
TreeIterator implements hasNext(), next() and treeToStack() funs
implement rotates to fun balanceAdd()
|
|
|
src/test/kotlin/RBTreeTest.kt
Outdated
bft15 = RBTree() //15 node tree (balanced) // 8 | ||
var k = 16 // / \ | ||
for (i in listOf(1, 2, 4, 8)) { // 4 12 | ||
for (j in 0..<i) // / \ / \ | ||
bft15.add(k / 2 + k * j, k / 2 + k * j) // 2 6 10 14 | ||
k /= 2 // / | / | | \ | \ | ||
} // 1 3 5 7 9 11 13 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поэтому я и не пользуюсь табами
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ой...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отличная реализация RBTree, мне очень нравится
Помимо стиля твоего кода - не мог ничего добавить дополнительно, так как все сделано очень хорошо
Отдельное уважение за тесты, даже не знаю, сколько ты убил на них сил
Архитектура очень мощная, сначала было очень страшно, но и спустя время страшно до сих пор
Тем не менее видно, что ваша команда вложила значительные свои силы в деревья и в библиотеку, бережливо все выстраивая и приводя в порядок,
Мне не прибавить, не убавить
💕💕💕
while (parent != null && parent.isRed && grandparent != null) { | ||
val uncle = if (parent == grandparent.right) grandparent.left else grandparent.right | ||
//uncle is red -> recolor nodes and go higher up the treeBranch | ||
if (uncle?.isRed == true) { | ||
parent.isRed = false | ||
uncle.isRed = false | ||
grandparent.isRed = true | ||
son = grandparent | ||
parent = treeBranch.removeFirstOrNull() | ||
grandparent = treeBranch.removeFirstOrNull() | ||
} else /*uncle is black or null -> do few rotations, recoloring and magic will happen*/ { | ||
if (parent == grandparent.right) { | ||
if (son == parent.left) { //we want co-directional parent and son | ||
rotateRight(parent, grandparent) | ||
parent = son | ||
} | ||
parent.isRed = false | ||
grandparent.isRed = true | ||
rotateLeft(grandparent, treeBranch.firstOrNull()) | ||
} else /*parent == grandparent.left*/ { | ||
if (son == parent.right) { //we want co-directional parent and son too | ||
rotateLeft(parent, grandparent) | ||
parent = son | ||
} | ||
parent.isRed = false | ||
grandparent.isRed = true | ||
rotateRight(grandparent, treeBranch.firstOrNull()) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему-то в какой-то момент я очень стал бояться и избегать вложенность и мне хочется этот страх (в адекватной мере) привить людям вокруг меня авхахв
Я всего лишь убрал вложенность первого if и отделил логические блоки новой строкой и, как по мне, мозгу стало легче воспринимать твой код
while (parent != null && parent.isRed && grandparent != null) { | |
val uncle = if (parent == grandparent.right) grandparent.left else grandparent.right | |
//uncle is red -> recolor nodes and go higher up the treeBranch | |
if (uncle?.isRed == true) { | |
parent.isRed = false | |
uncle.isRed = false | |
grandparent.isRed = true | |
son = grandparent | |
parent = treeBranch.removeFirstOrNull() | |
grandparent = treeBranch.removeFirstOrNull() | |
} else /*uncle is black or null -> do few rotations, recoloring and magic will happen*/ { | |
if (parent == grandparent.right) { | |
if (son == parent.left) { //we want co-directional parent and son | |
rotateRight(parent, grandparent) | |
parent = son | |
} | |
parent.isRed = false | |
grandparent.isRed = true | |
rotateLeft(grandparent, treeBranch.firstOrNull()) | |
} else /*parent == grandparent.left*/ { | |
if (son == parent.right) { //we want co-directional parent and son too | |
rotateLeft(parent, grandparent) | |
parent = son | |
} | |
parent.isRed = false | |
grandparent.isRed = true | |
rotateRight(grandparent, treeBranch.firstOrNull()) | |
} | |
} | |
} | |
while (parent != null && parent.isRed && grandparent != null) { | |
val uncle = if (parent == grandparent.right) grandparent.left else grandparent.right | |
//uncle is red -> recolor nodes and go higher up the treeBranch | |
if (uncle?.isRed == true) { | |
parent.isRed = false | |
uncle.isRed = false | |
grandparent.isRed = true | |
son = grandparent | |
parent = treeBranch.removeFirstOrNull() | |
grandparent = treeBranch.removeFirstOrNull() | |
continue | |
} | |
/*uncle is black or null -> do few rotations, recoloring and magic will happen*/ | |
if (parent == grandparent.right) { | |
if (son == parent.left) { //we want co-directional parent and son | |
rotateRight(parent, grandparent) | |
parent = son | |
} | |
parent.isRed = false | |
grandparent.isRed = true | |
rotateLeft(grandparent, treeBranch.firstOrNull()) | |
} else /*parent == grandparent.left*/ { | |
if (son == parent.right) { //we want co-directional parent and son too | |
rotateLeft(parent, grandparent) | |
parent = son | |
} | |
parent.isRed = false | |
grandparent.isRed = true | |
rotateRight(grandparent, treeBranch.firstOrNull()) | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Внутри функции balancedAdd также предлагаю разделить логические блоки новой строкой (инициализация переменных, цикл, покраска рута, а также внутри цикла)
private fun balancedAdd(treeBranch: ArrayDeque<RBNode<K, V>>) {
var son = treeBranch.removeFirst()
var parent = treeBranch.removeFirstOrNull()
var grandparent = treeBranch.removeFirstOrNull()
while(parent != null && parent.isRed && grandparent != null) {
//
}
root?.isRed = false
}
private fun balancedAdd(treeBranch: ArrayDeque<RBNode<K, V>>) {
var son = treeBranch.removeFirst()
var parent = treeBranch.removeFirstOrNull()
var grandparent = treeBranch.removeFirstOrNull()
while(parent != null && parent.isRed && grandparent != null) {
//
}
root?.isRed = false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Полный код balancedAdd
private fun balancedAdd(treeBranch: ArrayDeque<RBNode<K, V>>) {
var son = treeBranch.removeFirst()
var parent = treeBranch.removeFirstOrNull()
var grandparent = treeBranch.removeFirstOrNull()
while (parent != null && parent.isRed && grandparent != null) {
val uncle = if (parent == grandparent.right) grandparent.left else grandparent.right
//uncle is red -> recolor nodes and go higher up the treeBranch
if (uncle?.isRed == true) {
parent.isRed = false
uncle.isRed = false
grandparent.isRed = true
son = grandparent
parent = treeBranch.removeFirstOrNull()
grandparent = treeBranch.removeFirstOrNull()
continue
}
/*uncle is black or null -> do few rotations, recoloring and magic will happen*/
if (parent == grandparent.right) {
if (son == parent.left) { //we want co-directional parent and son
rotateRight(parent, grandparent)
parent = son
}
parent.isRed = false
grandparent.isRed = true
rotateLeft(grandparent, treeBranch.firstOrNull())
} else /*parent == grandparent.left*/ {
if (son == parent.right) { //we want co-directional parent and son too
rotateLeft(parent, grandparent)
parent = son
}
parent.isRed = false
grandparent.isRed = true
rotateRight(grandparent, treeBranch.firstOrNull())
}
}
root?.isRed = false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
До форматирования кода внутри методов RBTree
тоже стояли пробелы(только не по принципу деления на логические блоки, а скорее по принципу "я - художник, я так вижу"), но я решил в последний момент от них отказаться, чтобы код стилистически больше походил на то, как писали мои товарищи по команде в своих деревьях :)
А вот с continue
очень элегантно вышло, я даже не думал о нём как о полезном anti-nesting инструменте, спасибо!
//depending on nephew's colors, doing magic for balancing purposes | ||
var nephewRight = brother.right | ||
var nephewLeft = brother.left | ||
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю выделить
(nephewRight == null || !nephewRight.isRed)
В отдельный метод, потому что условие на black node применяется в 4 местах и введение нового метода увеличит читаемость и спасет от потенциальных багов
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) { | |
private fun isBlack(node: RBNode<K, V>?): Boolean { | |
return node == null || !node.isRed | |
} | |
if (isBlack(nephewRight) && isBlack(nephewLeft)) { |
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) { | ||
brother.isRed = true | ||
if (!parent.isRed) { //parent is black --> go higher up | ||
son = parent //the treeBranch and balance again | ||
parent = grandparent | ||
grandparent = treeBranch.removeFirstOrNull() | ||
} else { | ||
parent.isRed = false | ||
break | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В код в первом if, если и выполняет, то после ничего не идет, поэтому мы можем избавиться от вложенности
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) { | |
brother.isRed = true | |
if (!parent.isRed) { //parent is black --> go higher up | |
son = parent //the treeBranch and balance again | |
parent = grandparent | |
grandparent = treeBranch.removeFirstOrNull() | |
} else { | |
parent.isRed = false | |
break | |
} | |
} else { | |
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) { | |
brother.isRed = true | |
if (!parent.isRed) { //parent is black --> go higher up | |
son = parent //the treeBranch and balance again | |
parent = grandparent | |
grandparent = treeBranch.removeFirstOrNull() | |
} else { | |
parent.isRed = false | |
break | |
} | |
continue | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Разделение логических блоков новой строкой очень увеличивает читабельность кода, можешь сравнить:
До:
private fun balancedRemove(treeBranch: ArrayDeque<RBNode<K, V>?>) {
var son = treeBranch.removeFirstOrNull()
var parent = treeBranch.removeFirstOrNull()
var grandparent = treeBranch.removeFirstOrNull()
//son == son of removed node
if (son?.isRed == true) {
son.isRed = false
return
}
while (parent != null) {
var brother = if (son == parent.right) parent.left else parent.right
//we want for brother to be black (´。_。`)
if (brother?.isRed == true) {
brother.isRed = false
parent.isRed = true
if (son == parent.right) {
rotateRight(parent, grandparent)
grandparent = brother
brother = parent.left
} else /*son == parent.left*/ {
rotateLeft(parent, grandparent)
grandparent = brother
brother = parent.right
}
}
brother ?: throw IllegalStateException(
"Balance of red-black tree violated:" +
" parent node have subtrees with black height 2 and black height 1"
)
//depending on nephew's colors, doing magic for balancing purposes
var nephewRight = brother.right
var nephewLeft = brother.left
if ((nephewRight == null || !nephewRight.isRed) && (nephewLeft == null || !nephewLeft.isRed)) {
brother.isRed = true
if (!parent.isRed) { //parent is black --> go higher up
son = parent //the treeBranch and balance again
parent = grandparent
grandparent = treeBranch.removeFirstOrNull()
} else {
parent.isRed = false
break
}
} else {
if (son == parent.right) {
if (nephewRight?.isRed == true && (nephewLeft == null || !nephewLeft.isRed)) {
rotateLeft(brother, parent) // B /nL == new brother
nephewLeft = brother // / \ -> /B == new left nephew
brother = nephewRight // nL nR /nR
brother.isRed = false
nephewLeft.isRed = true
}
rotateRight(parent, grandparent)
brother.isRed = parent.isRed
parent.isRed = false
nephewLeft?.isRed = false
} else /*son == parent.left*/ {
if (nephewLeft?.isRed == true && (nephewRight == null || !nephewRight.isRed)) {
rotateRight(brother, parent) // B \nR == new brother
nephewRight = brother // / \ -> \B == new right nephew
brother = nephewLeft // nL nR \nL
brother.isRed = false
nephewRight.isRed = true
}
rotateLeft(parent, grandparent)
brother.isRed = parent.isRed
parent.isRed = false
nephewRight?.isRed = false
}
break
}
}
root?.isRed = false
}
После:
private fun balancedRemove(treeBranch: ArrayDeque<RBNode<K, V>?>) {
var son = treeBranch.removeFirstOrNull()
var parent = treeBranch.removeFirstOrNull()
var grandparent = treeBranch.removeFirstOrNull()
//son == son of removed node
if (son?.isRed == true) {
son.isRed = false
return
}
while (parent != null) {
var brother = if (son == parent.right) parent.left else parent.right
//we want for brother to be black (´。_。`)
if (brother?.isRed == true) {
brother.isRed = false
parent.isRed = true
if (son == parent.right) {
rotateRight(parent, grandparent)
grandparent = brother
brother = parent.left
} else /*son == parent.left*/ {
rotateLeft(parent, grandparent)
grandparent = brother
brother = parent.right
}
}
brother ?: throw IllegalStateException(
"Balance of red-black tree violated:" +
" parent node have subtrees with black height 2 and black height 1"
)
//depending on nephew's colors, doing magic for balancing purposes
var nephewRight = brother.right
var nephewLeft = brother.left
if (isBlack(nephewRight) && isBlack(nephewLeft)) {
brother.isRed = true
if (!parent.isRed) { //parent is black --> go higher up
son = parent //the treeBranch and balance again
parent = grandparent
grandparent = treeBranch.removeFirstOrNull()
} else {
parent.isRed = false
break
}
continue
}
if (son == parent.right) {
if (nephewRight?.isRed == true && isBlack(nephewLeft)) {
rotateLeft(brother, parent) // B /nL == new brother
nephewLeft = brother // / \ -> /B == new left nephew
brother = nephewRight // nL nR /nR
brother.isRed = false
nephewLeft.isRed = true
}
rotateRight(parent, grandparent)
brother.isRed = parent.isRed
parent.isRed = false
nephewLeft?.isRed = false
} else /*son == parent.left*/ {
if (nephewLeft?.isRed == true && isBlack(nephewRight)) {
rotateRight(brother, parent) // B \nR == new brother
nephewRight = brother // / \ -> \B == new right nephew
brother = nephewLeft // nL nR \nL
brother.isRed = false
nephewRight.isRed = true
}
rotateLeft(parent, grandparent)
brother.isRed = parent.isRed
parent.isRed = false
nephewRight?.isRed = false
}
break
}
root?.isRed = false
}
} | ||
|
||
fun clear() { | ||
root?.let { root = null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root в любом случае будет null после метода clear()
root?.let { root = null } | |
root = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ахах действительно, как я вообще так написать метод мог и не замечать
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это очень круто. У меня на такое не хватило смелости. Хотя идея кажется довольно простой. Вы молодцы
internal var left: Node_T? = null, | ||
) : Comparable<Node_T> { | ||
|
||
override fun compareTo(other: Node_T): Int = key.compareTo(other.key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это чтобы можно было напрямую сравнивать ноды между собой без обращения к ключу? Отличное решение
internal fun attach(node: Node_T?): Boolean { | ||
if (node == null) return false | ||
when { | ||
this > node -> left = node | ||
this < node -> right = node | ||
else -> return false | ||
} | ||
return true | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Одно наслаждение видеть подобную реализацию
Я бы еще отделил новой строгой логические блоки
internal fun attach(node: Node_T?): Boolean { | |
if (node == null) return false | |
when { | |
this > node -> left = node | |
this < node -> right = node | |
else -> return false | |
} | |
return true | |
} | |
internal fun attach(node: Node_T?): Boolean { | |
if (node == null) return false | |
when { | |
this > node -> left = node | |
this < node -> right = node | |
else -> return false | |
} | |
return true | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вы очень классно реализовали межнодовое общение с помощью методов compareTo, equals, attach, moveOn
мне нрав
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кстати, сейчас придумал одну штуку побаловаться:
Если ввести две вот эти функции:
val RED = "RED"
val BLACK = "BLACK"
private infix fun RBNode<Int, Int>.color(color: String): Unit {
if (color == RED) return assertTrue(this.isRed)
if (color == BLACK) return assertFalse(this.isRed)
else throw IllegalStateException()
}
то цвета в дереве можно проверять так:
@Test
@DisplayName("Add with black uncle, right ver")
fun add3R() {
val root = emptyTree.add(0, 0)!!
val left = emptyTree.add(-1, -1)!!
val right = emptyTree.add(1, 1)!!
val right2 = emptyTree.add(3, 3)!!
val right15 = emptyTree.add(2, 2)!!
root color BLACK,
left color BLACK,
right color RED,
right15 color BLACK,
right2 color RED
Ну это так - идея в 3 часа ночи прикольная пришла, решил поделиться
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
тесты очень хорошие, ты молодец, дамир
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пожелания:
- Для повышения читаемости кода можно:
- разбивать большие методы на логические части пустой строкой
- повтор кода превратить в новый метод
- посмотреть, какую реализацию предлагают книжки/интернет
- избегать вложенности
-
Тесты: можно использовать другой подход в именовании, чтобы появились пробелы между словами, так легче понять идею проверки
-
Метод getSubTree: возможно childTree это и есть subTree
-
Модификатор доступа для метода findParent: как я поняла идея - дать доступ пользователю к этому методу, тогда нужно делать public
-
Появились новые публичные методы. Некоторые из них работают для трех деревьев, может стоит расширить возможности?
-
Не использовать восклицательные знаки в коде
if (curNode.right == null) { | ||
curNode.right = BSTNode(key, value) | ||
this.amountOfNodes += 1 | ||
return curNode.right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В методе этот кусок кода повторяется, имеет смысл вынести его за метод для повторного использования
if (parent.right === curNode) { | ||
if (curNode == root) this.root = null | ||
else parent.right = null | ||
} else parent.left = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эта часть кода тоже используется несколько раз.
В первой строке можно выбрать ==
while (child!!.left != null) { | ||
if (child.left!!.left == null) parent_child = child |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Владимир Александрович убедительно настаивал писать код без !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Работа сделана в целом хорошо, нет ничего лишнего, но и добавить ничего не забыли. Хорошо покрыто тестами. И самое мое не любимое, но приятно что здесь это ты учла, это !!, а именно ты ни разу его не использовала! Из моих пожеланий и замечаний здесь будут только по читаемости кода.
override fun remove(key: K): V? { | ||
var value: V? = null | ||
fun removeRec(node: AVLNode<K, V>?, key: K): AVLNode<K, V>? { | ||
if (node == null) return null | ||
if (key < node.key) { | ||
node.left = removeRec(node.left, key) | ||
} else if (key > node.key) { | ||
node.right = removeRec(node.right, key) | ||
} else { | ||
val nodeA = node.left | ||
val nodeB = node.right | ||
value = node.value | ||
if (nodeB == null) { | ||
return nodeA | ||
} | ||
val min = findMin(nodeB) | ||
min?.right = removeMin(nodeB) | ||
min?.left = nodeA | ||
if (root?.key == key) { | ||
root = min | ||
} | ||
return balanceNode(min) | ||
} | ||
return balanceNode(node) | ||
} | ||
removeRec(root, key) | ||
amountOfNodes -= 1 | ||
return value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут В целом все круто, но я могу предложить сделать это еще круче. Давай добавим не много пустых строк и сделаем код читаемым. Примерно так:
override fun remove(key: K): V? { | |
var value: V? = null | |
fun removeRec(node: AVLNode<K, V>?, key: K): AVLNode<K, V>? { | |
if (node == null) return null | |
if (key < node.key) { | |
node.left = removeRec(node.left, key) | |
} else if (key > node.key) { | |
node.right = removeRec(node.right, key) | |
} else { | |
val nodeA = node.left | |
val nodeB = node.right | |
value = node.value | |
if (nodeB == null) { | |
return nodeA | |
} | |
val min = findMin(nodeB) | |
min?.right = removeMin(nodeB) | |
min?.left = nodeA | |
if (root?.key == key) { | |
root = min | |
} | |
return balanceNode(min) | |
} | |
return balanceNode(node) | |
} | |
removeRec(root, key) | |
amountOfNodes -= 1 | |
return value | |
} | |
override fun remove(key: K): V? { | |
var value: V? = null | |
fun removeRec(node: AVLNode<K, V>?, key: K): AVLNode<K, V>? { | |
if (node == null) return null | |
if (key < node.key) { | |
node.left = removeRec(node.left, key) | |
} else if (key > node.key) { | |
node.right = removeRec(node.right, key) | |
} else { | |
val nodeA = node.left | |
val nodeB = node.right | |
value = node.value | |
if (nodeB == null) { | |
return nodeA | |
} | |
val min = findMin(nodeB) | |
min?.right = removeMin(nodeB) | |
min?.left = nodeA | |
if (root?.key == key) { | |
root = min | |
} | |
return balanceNode(min) | |
} | |
return balanceNode(node) | |
} | |
removeRec(root, key) | |
amountOfNodes -= 1 | |
return value | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Красивое...
val nodeA = node.left | ||
val nodeB = node.right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот этот момент можно рассмотреть для упрощения. Зачем называть что-то nodeA и nodeB, ведь это нужно запомнить а потом каждый раз проверять точно ли мы запомнили. Сделаем чуть проще назовем переменные тем, что будет в них лежать чуть конкретнее. Примерно так:
val nodeA = node.left | |
val nodeB = node.right | |
val nodeLeft = node.left | |
val nodeRight = node.right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, звучит разумно.
fixHeight(node) | ||
if (balanceFactor(node) == 2) { | ||
if (node != null) { | ||
if (balanceFactor(node.right) < 0) { | ||
node.right = rotateRight(node.right) | ||
} | ||
if (node == root) { | ||
root = rotateLeft(node) | ||
return root | ||
} | ||
return rotateLeft(node) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вроде все классно, но есть один момент который было бы лучше поправить и это вложенность.Зачем писать два if если можно один! Примерно так:
fixHeight(node) | |
if (balanceFactor(node) == 2) { | |
if (node != null) { | |
if (balanceFactor(node.right) < 0) { | |
node.right = rotateRight(node.right) | |
} | |
if (node == root) { | |
root = rotateLeft(node) | |
return root | |
} | |
return rotateLeft(node) | |
} | |
} | |
fixHeight(node) | |
if (balanceFactor(node) == 2 && node != null) { | |
if (balanceFactor(node.right) < 0) { | |
node.right = rotateRight(node.right) | |
} | |
if (node == root) { | |
root = rotateLeft(node) | |
return root | |
} | |
return rotateLeft(node) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
И правда. Красивее и компактнее.
return if (root == null) { | ||
0 | ||
} else { | ||
root?.height | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь вообще ошибок нет. Я могу только посоветовать использовать ?: . Ведь смысл один и тот же, а писать на три строки меньше. Примерно так:
return if (root == null) { | |
0 | |
} else { | |
root?.height | |
} | |
return root?.height ?: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Спасибо за предложение. Думала об использовании ?:, но показалось, будто более длинный вариант более понятен, на случай если читающий код не очень знаком с тернарными операторами (и в частности с ?:).
# Conflicts: # src/main/kotlin/treeLib/bintrees/AVLTree.kt # src/main/kotlin/treeLib/bintrees/interfaces/BalancedTree.kt # src/main/kotlin/treeLib/bintrees/interfaces/BinTree.kt # src/main/kotlin/treeLib/bintrees/interfaces/TreeIterator.kt # src/main/kotlin/treeLib/nodes/TreeNode.kt
|
|
|
|
👋! 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
main
since the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
main
since 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: @sgrishkova @D31IRIUM @IslamZZZZ