Feedback by github-classroom[bot] · Pull Request #1 · spbu-coding-2023/trees-3

github-classroom · 2024-03-14T09:34:14Z

👋! 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: @homka122 @Demon32123 @far1yg

AVL-Tree test

qrutyy

Очень приятный код RB, с минимальными недочетами) (поэтому ниже пару вопросов по архе). Единственное - хотелось бы получить конечную версию либы не в последний день (кхм вечер), но код не потребовал излишних усилий для разбора, и на вряд ли потребует этого от пользователя))).

qrutyy · 2024-04-04T18:11:34Z

lib/src/main/kotlin/tree/RBTree.kt

+import tree.node.RBTreeNode
+
+class RBTree<K : Comparable<K>, V> : SearchTree<K, V, RBTreeNode<K, V>> {
+    constructor() : super()


Не очень понятно, зачем конструктор не принимающий аргументов)

Чтобы создать пустое дерево без нодов

qrutyy · 2024-04-04T18:16:00Z

lib/src/main/kotlin/Main.kt

+import tree.node.BinaryTreeNode
+
+fun main() {
+    println("Hello world!")
+}


Не очень понятен смысл main файла в библиотеке. Все-таки это не фреймворк, и скорее всего будет использоваться в составе другого "приложения". Хотя кейсы разные бывают.
P.S. в ревью присутствуют некоторые вопросы по архитектуре, надеюсь это не станет проблемой. Код показался мне довольно хорошим, поэтому решил задать их здесь.

это должно было добавлено в .gitignore и использоваться для личных нужд, спасибо

qrutyy · 2024-04-04T18:17:14Z

lib/src/main/kotlin/tree/RBTree.kt

+import tree.node.RBTreeColor
+import tree.node.RBTreeNode
+
+class RBTree<K : Comparable<K>, V> : SearchTree<K, V, RBTreeNode<K, V>> {


Прошу пояснить, про момент использования size. В данной реализации не увидел взаимодействия с данным параметром. 🐈

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

более того — size это закрытое для изменения поле внутри класса и мы не можем его меня

это сделано осознанно

Насколько size полезен для пользователя?

польза size в принципе довольно понятна — информация о том, сколько у нас пар Ключ/Значений

пользователь не может менять size, только читать

а в чем вопрос?

Вопрос был только в полезности) Как доп. фича - понятно)

qrutyy · 2024-04-04T18:24:49Z

lib/src/main/kotlin/tree/node/RBTreeColor.kt

+package tree.node
+
+enum class RBTreeColor {
+    BLACK, RED
+}


В чем смысл выносить данный класс за пределы RBTreeNode?

так красивее

О вкусах не поспоришь( 😭
Но я за размещение данного класса в пределах класса ноды, все-таки это состояние ноды данного класса)

Мне не нравится, что оно лежит в папочке с именем node. Этож не нода(

qrutyy · 2024-04-04T18:50:55Z

lib/src/test/kotlin/tree/RBTreeTest.kt

+import org.junit.jupiter.api.Nested
+import kotlin.random.Random
+
+class RBTreeTest {


В некоторых случаях стоит проверять цвета. Условно в маленьких деревьях большую роль будет играть цвет, а не положение нод (в том смысле, что оно вариативно + вполне очевидно), поэтому данный метод будет не лишним.

Только сейчас, видимо, понял о чем речь

Наша библиотека подразумевает, что пользователь не знает о такой абстракции, как нода, цвет и прочие элементы деревьев

Поэтому у нас нет инструментария для проверки цвета нодов без какого-либо лишнего пробрасывания этих скрытых элементов наружу

Следовательно и unit тестирование у нас происходит сугобо на публичных методах и результате их работы, лишь ссылаясь в названиях тестов на то, что мы на самом деле проверяем под капотом

suvorovrain · 2024-04-07T14:42:48Z

не поставлю тебе звездочек за попрошайничество

Tests for Search Tree, fix bugs

homka122 · 2024-04-07T20:00:50Z

@suvorovrain оболтус

Fix AVL-Tree after review

ItIsMrLaG · 2024-04-11T14:22:43Z

Короче, чистенько, не очень много кода. Есть недочеты, хотя по большей части good.

Вывод: мне нрав, довольно прямолинейно.

-_-
Ток чет я не понял, почему вас 2 контребьютора, а не 3
-_-

ItIsMrLaG

См. сверху)

ItIsMrLaG · 2024-04-11T11:11:58Z

lib/src/main/kotlin/tree/SearchTree.kt

+    /**
+     * Returns the pair with next ascending key
+     */
+    fun successor(key: K): Pair<K?, V?> {


Насколько имеют смысл кейсы: Pair(null, null), Pair(null, Any()), Pair(Any(), null)? Если я правильно понял, то два последних не имеют ни какого смысла. А Pair(null, null) можно было бы упростить до null. Таким образом, у всей функции можно было бы поменять возвращаемый тип на Pair<K, V>?.

Согласен, что придирка, но раз вы вытащили этот метод в API, то у него должен быть понятный интерфейс взаимодействия (на мой взгляд).
Либо я не очень понял что-то, так что вы можете поменять мое мнение)))

У меня кста такой вопрос ко всем вашим публичным методам, которые возвращают тип Pair<K?, V?>.

ItIsMrLaG · 2024-04-11T11:20:31Z

lib/src/test/kotlin/tree/SearchTreeTest.kt

+        bst = BSTree()
+        bst.set(5, "A")
+        bst.set(8, "A")
+        bst.set(6, "A")
+        bst.set(1, "A")
+        bst.set(2, "A")
+        bst.set(4, "A")
+        bst.set(3, "A")
+        bst.set(10, "A")


У вас же вроде для такого есть красивый конструктор (точно есть, я его в RBTreeTest видел...), в чем мотивация так дублировать код?)))

ItIsMrLaG · 2024-04-11T11:24:32Z

lib/src/test/kotlin/tree/RBTreeTest.kt

+            val entities = arrayOf(
+                Pair(35, "A"),
+                Pair(21, "A"),
+                Pair(25, "A"),


А чего тут Pair(), а не to... ? Ладно, ладно, дальше ревью без фигни будет, обещаю)

ItIsMrLaG · 2024-04-11T11:55:03Z

lib/src/test/kotlin/tree/RBTreeTest.kt

+        fun `set one key on empty tree`() {
+            val data = arrayOf(1 to "Homka")
+            setTest(data)
+        }
+
+        @Test
+        fun `set second key as right child`() {
+            val data = arrayOf(1 to "Homka", 2 to "Dima")
+            setTest(data)
+        }
+
+        @Test
+        fun `set second key as left child`() {
+            val data = arrayOf(2 to "Homka", 1 to "Dima")
+            setTest(data)
+        }
+
+        @Test
+        fun `set third key as left child`() {
+            val data = arrayOf(2 to "Homka", 3 to "Dima", 1 to "Nastya")
+            setTest(data)
+        }


Хорошо, подробно, красиво названо, но можно иначе:

private fun checkSize(size: Long,errMsg: String, rbt: RBTree<Int, String> = this.rbt) { // Любая ваша проверка assertEquals(size, rbt.size, message = errMsg) } @ParameterizedTest(name = "Something") @MethodSource("arrProvider") fun runner(data: Array<Pair<Int, String>>, errMsg: String) { rbt.set(data) checkSize(data.size.toLong(), errMsg) } companion object { @JvmStatic fun arrProvider(): Stream<Arguments> { return Stream.of( Arguments.of(arrayOf(1 to "Homka"), "set one key on empty tree"), Arguments.of(arrayOf(1 to "Homka", 2 to "Dima"), "set second key as right child"), Arguments.of(arrayOf(2 to "Homka", 1 to "Dima"), "set second key as left child"), Arguments.of(arrayOf(2 to "Homka", 3 to "Dima", 1 to "Nastya"), "set third key as left child"), ) } }

Имхо, так меньше повторяющегося кода + добавление новых тестов -- нечто ну очень быстрое. Возможно все 14 тестов этого класса можно было бы так переписать)
Хотя не спорю, что текущий метод тоже good

ItIsMrLaG · 2024-04-11T12:03:51Z

lib/src/test/kotlin/tree/SearchTreeTest.kt

+            assertEquals(Pair(2, "A"), bst.successor(1))
+            assertEquals(Pair(3, "A"), bst.successor(2))
+            assertEquals(Pair(4, "A"), bst.successor(3))
+            assertEquals(Pair(5, "A"), bst.successor(4))
+            assertEquals(Pair(6, "A"), bst.successor(5))
+            assertEquals(Pair(8, "A"), bst.successor(6))
+            assertEquals(Pair(10, "A"), bst.successor(8))
+            assertEquals(Pair(null, null), bst.successor(10))


Ну для такой красоты существуют @ParameterizedTest + @CsvSource:

@ParameterizedTest @CsvSource( "2, A, 1", "3, A, 2", "4, A, 3", "5, A, 4", "6, A, 5", "8, A, 6", ) fun `successor() test with existed keys`(k: Int, v: String, kf: Int) { assertEquals(Pair(k, v), bst.successor(kf)) }

Мне так больше нравится)))

ItIsMrLaG · 2024-04-11T13:31:23Z

lib/src/main/kotlin/tree/AVLTree.kt

+        }
+    }
+
+    private fun getBalance(node: AVLTreeNode<K, V>): Int {


Набор вопросиков и рекомендаций от меня:

А чего именно такое название, или разность в высотах -- это баланс называется? Я просто немного не в контексте, так что реально любопытно.

Можно было бы написать короче, используя всю МоЩь островного языка:

private fun getBalance(node: AVLTreeNode<K, V>) = height(node.left) - height(node.right)

У вас эта функция в одном месте используется (+ она оч маленькая). По моему субъективному мнению, такие вспомогательные функции лучше писать через замыкания(вложенные функции) в том месте, где они используются.

ItIsMrLaG · 2024-04-11T13:39:13Z

lib/src/main/kotlin/tree/node/RBTreeColor.kt

+package tree.node
+
+enum class RBTreeColor {
+    BLACK, RED
+}


Мне не нравится, что оно лежит в папочке с именем node. Этож не нода(

ItIsMrLaG · 2024-04-11T13:42:24Z

.github/workflows/tests.yml

+      run: ./gradlew build
+
+    - name: Launch tests
+      run: ./gradlew :lib:test 


Покрытие бы посчитать где-нибудь, как-нибудь... (или у вас есть где-то спрятанное и это я слепец -_-)

ItIsMrLaG · 2024-04-11T13:45:52Z

lib/src/main/kotlin/tree/SearchTree.kt

+
+import tree.node.BinaryTreeNode
+
+abstract class SearchTree<K : Comparable<K>, V, Node : BinaryTreeNode<K, V, Node>>() {


А почему у вас тип SearchTree параметризуется BinaryTreeNode, а не SearchTreeNode. Тут надо бы как-то унифицировать, а то у вас в Search используется Binary, при этом в проекте есть BinarySearch (BS) :)

ItIsMrLaG · 2024-04-11T13:56:15Z

lib/src/main/kotlin/tree/SearchTree.kt

@@ -0,0 +1,293 @@
+package tree
+
+import tree.node.BinaryTreeNode


Подумал, что ноды, можно было бы сделать data классами. Но это так, на вкус и цвет фломастеры разные)

… and make method createNode more prettier

Refactor BST and SearchTree

github-classroom bot and others added 21 commits March 14, 2024 09:34

Setting up GitHub Classroom Feedback

df239d1

struct: gradle init

780db61

struct: update .gitignore

24c34c5

struct: get initial structure of files

6b4f63f

struct: add license

d455921

feat: created an abstract node class

523f8ba

feat: created an abstract search tree class

4a3b059

fix: сorrected methods insertNode, removeNode, createNode

95addd4

feat: created a binary search tree node class

b397449

feat: created a AVL-Tree node class

8ceb961

fix: the order of the AVL-Tree node class attributes has been changed

a2d7ba2

fix: classes AVL, BST and RBT nodes have been corrected

d712b38

feat: сreated a binary search tree class

a18c27d

feat: сreated a AVL-Tree class

cfdd4ce

feat: сreated class red black tree node

7471277

feat: сreated class red black tree

d9aefd5

fix: corrected omissions in serch tree class and formatted code

d077d9b

refactor: move RBTreeColor to seperate file

5cae855

refactor: add comments to methods of SearchTree

8abb345

fix: traversal method's argument must be function

85b70bb

refactor(BST): changed comment to method insertNode

0b3bccd

homka122 force-pushed the main branch from adb5194 to 85b70bb Compare March 27, 2024 17:51

far1yg and others added 8 commits March 27, 2024 21:24

feat(BST): add method insertNode

d2dccc2

feat(BST): add constructors

5727bfe

feat(BST): add method createNode

783a863

feat(AVL-T): implemented method createNode

8d9cf1b

feat(AVL-T): Implemented insertNode method along with auxiliary methods

59ed960

feat(BST): add private method searchParentNode

2819604

refactor(BST): remove ; in method insertNode

f6a25f0

refactor(BST): remove ; in method createNode

71f5833

Merge pull request #14 from spbu-coding-2023/AVLtest

e731a11

AVL-Tree test

qrutyy reviewed Apr 4, 2024

View reviewed changes

far1yg added 15 commits April 6, 2024 23:55

test(BST): refactor remove tests

b10fe3e

refactor: make method inOrderTraversal more prettier

7d58692

refactor: delete empty line in method inOrderTraversal

1764f72

fix(STree): fix bug in method predecessor

8748774

test(STree): add cases in successor and predecessor tests

e9220f8

test(STree): refactor getting tests

2c26904

test(STree): add cases in successor and predecessor tests

9f20685

test(STree): add remove tests and refactor search tests

f70e08e

test(STree): add setting tests, add check in remove and search tests

2691d72

test(STree): add setIfEmpty tests

dd776bd

test(STree): add Constructors tests

0a9ab71

test(STree): add resently keys tests

ccb1090

test(STree): group test of methods set

f234f69

test(STree): add case to constructor test

6cc7249

test(STree): make setting, remove tests prettier

74a11c2

Merge pull request #15 from spbu-coding-2023/refactor/Nastya

05b6698

Tests for Search Tree, fix bugs

Demon32123 and others added 3 commits April 10, 2024 01:37

fix(AVL-T): fix remove method in AVLTree

2f431ef

fix(AVL-T): add new tests in remove test

3e34279

Merge pull request #16 from spbu-coding-2023/fix/Dima

ec9a09b

Fix AVL-Tree after review

ItIsMrLaG reviewed Apr 11, 2024

View reviewed changes

far1yg and others added 5 commits April 13, 2024 22:17

refactor(STree): change if to when in method searchNode

8ab036c

refactor(BST): change if to when in methods insertNode, identifyChild…

30f48a3

… and make method createNode more prettier

test(STree): make set test in empty tree prettier

c6be325

test(BST): use simplier way to create a tree to test

810d36b

Merge pull request #17 from spbu-coding-2023/refactor/Nastya

3554f69

Refactor BST and SearchTree


		import tree.node.BinaryTreeNode

		abstract class SearchTree<K : Comparable<K>, V, Node : BinaryTreeNode<K, V, Node>>() {

		@@ -0,0 +1,293 @@
		package tree

		import tree.node.BinaryTreeNode

Conversation

github-classroom bot commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qrutyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suvorovrain commented Apr 7, 2024

Uh oh!

homka122 commented Apr 7, 2024

Uh oh!

ItIsMrLaG commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ItIsMrLaG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

github-classroom bot commented Mar 14, 2024 •

edited

Loading

ItIsMrLaG commented Apr 11, 2024 •

edited

Loading