Skip to content

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

Open
wants to merge 207 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 207 commits into from

Conversation

github-classroom[bot]
Copy link
Contributor

@github-classroom github-classroom bot commented Mar 13, 2024

👋! 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:

  • Click the Files changed tab to see all of the changes pushed to 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”.
  • Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @sgrishkova @D31IRIUM @IslamZZZZ

github-classroom bot and others added 30 commits March 13, 2024 19:08
(it can't remove yet)
TreeIterator implements hasNext(), next() and treeToStack() funs
implement rotates to fun balanceAdd()
Copy link

github-actions bot commented Apr 3, 2024

Overall Project 94.33% -5.67% 🍏
Files changed 94.19% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.69% -3.31% 🍏
BSTree.kt 93.31% -6.69% 🍏
AVLTree.kt 90.84% -9.16% 🍏
TreeNode.kt 88.11% -11.89% 🍏
TreeBalancer.kt 85.29% -14.71% 🍏

Copy link

github-actions bot commented Apr 3, 2024

Overall Project 94.24% -5.76% 🍏
Files changed 94.1% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.69% -3.31% 🍏
BSTree.kt 93.07% -6.93% 🍏
AVLTree.kt 90.84% -9.16% 🍏
TreeNode.kt 88.11% -11.89% 🍏
TreeBalancer.kt 85.29% -14.71% 🍏

Copy link

github-actions bot commented Apr 4, 2024

Overall Project 94.22% -5.78% 🍏
Files changed 94.08% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.62% -3.38% 🍏
BSTree.kt 93.07% -6.93% 🍏
AVLTree.kt 90.84% -9.16% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

Comment on lines 172 to 178
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поэтому я и не пользуюсь табами

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ой...

Copy link

github-actions bot commented Apr 4, 2024

Overall Project 94.22% -5.78% 🍏
Files changed 94.08% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.62% -3.38% 🍏
BSTree.kt 93.07% -6.93% 🍏
AVLTree.kt 90.84% -9.16% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

Copy link

@homka122 homka122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Отличная реализация RBTree, мне очень нравится
Помимо стиля твоего кода - не мог ничего добавить дополнительно, так как все сделано очень хорошо
Отдельное уважение за тесты, даже не знаю, сколько ты убил на них сил

Архитектура очень мощная, сначала было очень страшно, но и спустя время страшно до сих пор
Тем не менее видно, что ваша команда вложила значительные свои силы в деревья и в библиотеку, бережливо все выстраивая и приводя в порядок,

Мне не прибавить, не убавить
💕💕💕

Comment on lines 49 to 78
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())
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему-то в какой-то момент я очень стал бояться и избегать вложенность и мне хочется этот страх (в адекватной мере) привить людям вокруг меня авхахв

Я всего лишь убрал вложенность первого if и отделил логические блоки новой строкой и, как по мне, мозгу стало легче воспринимать твой код

Suggested change
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())
}
}

Copy link

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
    }

Copy link

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
    }

Copy link
Contributor

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)) {
Copy link

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 местах и введение нового метода увеличит читаемость и спасет от потенциальных багов

Suggested change
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)) {

Comment on lines 163 to 173
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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В код в первом if, если и выполняет, то после ничего не идет, поэтому мы можем избавиться от вложенности

Suggested change
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
}

Copy link

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 }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

root в любом случае будет null после метода clear()

Suggested change
root?.let { root = null }
root = null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ахах действительно, как я вообще так написать метод мог и не замечать

Copy link

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это чтобы можно было напрямую сравнивать ноды между собой без обращения к ключу? Отличное решение

Comment on lines 26 to 35
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
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Одно наслаждение видеть подобную реализацию

Я бы еще отделил новой строгой логические блоки

Suggested change
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
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вы очень классно реализовали межнодовое общение с помощью методов compareTo, equals, attach, moveOn
мне нрав

Copy link

@homka122 homka122 Apr 5, 2024

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 часа ночи прикольная пришла, решил поделиться

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тесты очень хорошие, ты молодец, дамир

Copy link

@far1yg far1yg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пожелания:

  1. Для повышения читаемости кода можно:
  • разбивать большие методы на логические части пустой строкой
  • повтор кода превратить в новый метод
  • посмотреть, какую реализацию предлагают книжки/интернет
  • избегать вложенности
  1. Тесты: можно использовать другой подход в именовании, чтобы появились пробелы между словами, так легче понять идею проверки

  2. Метод getSubTree: возможно childTree это и есть subTree

  3. Модификатор доступа для метода findParent: как я поняла идея - дать доступ пользователю к этому методу, тогда нужно делать public

  4. Появились новые публичные методы. Некоторые из них работают для трех деревьев, может стоит расширить возможности?

  5. Не использовать восклицательные знаки в коде

Comment on lines 33 to 36
if (curNode.right == null) {
curNode.right = BSTNode(key, value)
this.amountOfNodes += 1
return curNode.right
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В методе этот кусок кода повторяется, имеет смысл вынести его за метод для повторного использования

Comment on lines 62 to 65
if (parent.right === curNode) {
if (curNode == root) this.root = null
else parent.right = null
} else parent.left = null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Эта часть кода тоже используется несколько раз.
В первой строке можно выбрать ==

Comment on lines 81 to 82
while (child!!.left != null) {
if (child.left!!.left == null) parent_child = child
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Владимир Александрович убедительно настаивал писать код без !

Copy link

@Demon32123 Demon32123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Работа сделана в целом хорошо, нет ничего лишнего, но и добавить ничего не забыли. Хорошо покрыто тестами. И самое мое не любимое, но приятно что здесь это ты учла, это !!, а именно ты ни разу его не использовала! Из моих пожеланий и замечаний здесь будут только по читаемости кода.

Comment on lines 12 to 40
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
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут В целом все круто, но я могу предложить сделать это еще круче. Давай добавим не много пустых строк и сделаем код читаемым. Примерно так:

Suggested change
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
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Красивое...

Comment on lines 21 to 22
val nodeA = node.left
val nodeB = node.right

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот этот момент можно рассмотреть для упрощения. Зачем называть что-то nodeA и nodeB, ведь это нужно запомнить а потом каждый раз проверять точно ли мы запомнили. Сделаем чуть проще назовем переменные тем, что будет в них лежать чуть конкретнее. Примерно так:

Suggested change
val nodeA = node.left
val nodeB = node.right
val nodeLeft = node.left
val nodeRight = node.right

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, звучит разумно.

Comment on lines 139 to 151
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)
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде все классно, но есть один момент который было бы лучше поправить и это вложенность.Зачем писать два if если можно один! Примерно так:

Suggested change
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)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И правда. Красивее и компактнее.

Comment on lines 168 to 172
return if (root == null) {
0
} else {
root?.height
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь вообще ошибок нет. Я могу только посоветовать использовать ?: . Ведь смысл один и тот же, а писать на три строки меньше. Примерно так:

Suggested change
return if (root == null) {
0
} else {
root?.height
}
return root?.height ?: 0

Copy link
Contributor

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
Copy link

github-actions bot commented Apr 6, 2024

Overall Project 94.27% -5.73% 🍏
Files changed 94.12% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.62% -3.38% 🍏
BSTree.kt 93.07% -6.93% 🍏
AVLTree.kt 91.14% -8.86% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

Copy link

github-actions bot commented Apr 6, 2024

Overall Project 93.53% -6.47% 🍏
Files changed 93.37% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 96.62% -3.38% 🍏
AVLTree.kt 91.14% -8.86% 🍏
BSTree.kt 90.56% -9.44% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

Copy link

github-actions bot commented Apr 7, 2024

Overall Project 93.39% -6.61% 🍏
Files changed 93.23% 🍏

File Coverage
TreeIterator.kt 100% 🍏
BinTree.kt 100% 🍏
IterationOrder.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 95.8% -4.2% 🍏
AVLTree.kt 91.14% -8.86% 🍏
BSTree.kt 90.56% -9.44% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

@spbu-coding-2023 spbu-coding-2023 deleted a comment from github-actions bot Apr 7, 2024
@spbu-coding-2023 spbu-coding-2023 deleted a comment from github-actions bot Apr 7, 2024
@spbu-coding-2023 spbu-coding-2023 deleted a comment from github-actions bot Apr 7, 2024
Copy link

Overall Project 92.92% -7.08% 🍏
Files changed 92.69% 🍏

File Coverage
IterationOrder.kt 100% 🍏
BinTree.kt 100% 🍏
TreeIterator.kt 100% 🍏
AVLNode.kt 100% 🍏
RBNode.kt 100% 🍏
TreapNode.kt 100% 🍏
BSTNode.kt 100% 🍏
RBTree.kt 95.8% -4.2% 🍏
AVLTree.kt 91.14% -8.86% 🍏
BSTree.kt 90.56% -9.44% 🍏
Treap.kt 90.09% -9.91% 🍏
TreeNode.kt 88.11% -11.89% 🍏
BalancedTree.kt 85.92% -14.08% 🍏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants