Skip to content

Feedback#1

Open
github-classroom[bot] wants to merge 137 commits intofeedbackfrom
main
Open

Feedback#1
github-classroom[bot] wants to merge 137 commits intofeedbackfrom
main

Conversation

@github-classroom
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: @odiumuniverse @Ycyken @admitrievtsev

odiumuniverse and others added 22 commits March 19, 2024 16:30
-created files for required classes
-created BSTree and Vertex templates
-created vertices implementations for 3 all three types of trees
Add information
-added min() and max() public methods
-replaced traverse() method by iterator()
-implemented set public method, balance and rotates methods (with a lot
of bugs yet)
Created base structure of project, implemented main classes and simple tests
-remove in avl tree still doens't implemented
-refactored since key in vertices is var now
Ycyken and others added 5 commits April 2, 2024 14:30
-remove setWithoutBalance in trees implementations
-add setWithoutBalance common implementation to BSTreeTemplate class
-add abstract fabric method in BSTreeTemplate abstract class
Move setWithoutBalance to BSTreeTemplate
Comment on lines +18 to +19
var color: Color = Color.RED
var additionalType = false
Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Возможно, стоило сделать эти поля приватными во избежание их изменений пользователем, который мог бы поломать инварианты красно-чёрного дерева

replacedVertex = deleteNullChild(vertex)
deletedVertexColor = vertex.color
} else {
val mVertex = minVertex(vertex.right!!)!!
Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Тут очень подозрительное место. С одной стороны, видно, что в ветку else мы попадаем только в случае, когда vertex.left != null && vertex.right != null, отчего использование !! можно считать оправданным. С другой стороны, всё-таки использование !! нам настоятельно рекомендовали избегать

Но будет несправедливым не отметить, что я сам использовал идентичную конструкцию в своём методе удаления, только вместо !! поставил каст через as(это идентично в данном случае), поэтому полностью понимаю проблему в этом конкретном месте. Отметил это лишь из обязанности не оставлять подобное без внимания.

Copy link
Contributor

Choose a reason for hiding this comment

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

каст через as тоже очень такое себе решение, поэтому сделал выбор в сторону !!

@admitrievtsev Тут очень подозрительное место. С одной стороны, видно, что в ветку else мы попадаем только в случае, когда vertex.left != null && vertex.right != null, отчего использование !! можно считать оправданным. С другой стороны, всё-таки использование !! нам настоятельно рекомендовали избегать

Но будет несправедливым не отметить, что я сам использовал идентичную конструкцию в своём методе удаления, только вместо !! поставил каст через as(это идентично в данном случае), поэтому полностью понимаю проблему в этом конкретном месте. Отметил это лишь из обязанности не оставлять подобное без внимания.

Comment on lines +141 to +156
var brother = getBrother(vertex)
if (brother?.color == red) {
manageRedBrother(vertex, brother)
brother = getBrother(vertex)
}
if (brother?.left?.color == black && brother.right?.color == black) {
brother.color = red
if (vertex?.parent?.color == red) {
vertex.parent?.color = black
} else {
balanceTreeAfterDelete(vertex?.parent)
}
} else {
manageBlackRedOne(vertex, brother)
}
}
Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Аналогично методу rotateRight можно обдумать момент с оборачиванием части тела функции в ?.run{} для большей читаемости (меньше "вопросиков") и более быстрым завершением работы функции в случае null значения.
Ещё могу отметить, хоть это и не очень серьёзно, что обычно рекомендуется избегать длинные цепочки вызовов через поля, как в случае vertex?.parent?.color, и заменить их на вызовы функций-геттеров. Это позволяет быть уверенным, что в процессе работы программы одно из полей неожиданно не поменялось (например, при многопоточности). Понимаю, что это больше вопрос к архитектуре Вашей библиотеки, чем непосредственно к реализации дерева.

Copy link
Contributor

Choose a reason for hiding this comment

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

@admitrievtsev Аналогично методу rotateRight можно обдумать момент с оборачиванием части тела функции в ?.run{} для большей читаемости (меньше "вопросиков") и более быстрым завершением работы функции в случае null значения. Ещё могу отметить, хоть это и не очень серьёзно, что обычно рекомендуется избегать длинные цепочки вызовов через поля, как в случае vertex?.parent?.color, и заменить их на вызовы функций-геттеров. Это позволяет быть уверенным, что в процессе работы программы одно из полей неожиданно не поменялось (например, при многопоточности). Понимаю, что это больше вопрос к архитектуре Вашей библиотеки, чем непосредственно к реализации дерева.

согласен с претензией по ротейтам, я их в принципе писал самыми первыми
к тому же там можно переиспользовать код из метода replaceChild, который, очевидно, также был реализован сильно позже ротейтов

Comment on lines 158 to 167
private fun getBrother(vertex: RBVertex<K, V>?): RBVertex<K, V>? {
val parent = vertex?.parent
return if (vertex == parent?.left) {
parent?.right
} else if (vertex == parent?.right) {
parent?.left
} else {
throw IllegalStateException()
}
}
Copy link

Choose a reason for hiding this comment

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

@admitrievtsev В очередной раз отмечаю, что можно подумать над ?.let{}. Давайте, больше не буду поднимать эту тему при дальнейшем ревью кода, чтобы не надоедать(тем более, что я могу ошибаться)

rotateRight(brother)
brother = vertex?.parent?.left
}
brother?.color = vertex?.parent!!.color
Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Вот тут я не уверен в необходимости использования !!. Кажется, что можно было попробовать обернуть это через vertex?.let{} и локальную переменную parent?.let{}, но это тоже дискуссионный момент. Вообще, конечно, кажется логичным, что !! оправдано тем, что при наличии брата у vertex обязательно будет родитель, но, как я уже отмечал выше, я не могу оставить это без внимания, потому что это потенциальное окно для NPE в сложных случаях работы программы

Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Общий комментарий к реализации красно-чёрного дерева:
Я считаю, что у Вас получилась отличная реализация. Все сигнатуры функций понятны и без дополнительного комментирования дают понимание, что они выполняют. Все названия переменных, полей так же прозрачны для понимания. К кодстайлу, естественно, тоже нет никаких замечаний - все соглашения Вами соблюдаются(а даже если не соблюдались, то линтер исправил). Каких-то ошибок в алгоритмах мне найти не удалось, хотя, учитывая, что Ваши тесты имеют высокое покрытие (70% по веткам и 93% по строкам), найти их я бы и не смог. Конечно, я отметил небольшие замечания, но так как я сам реализовывал это дерево, то полностью понимаю трудности в указанных местах

Copy link

Choose a reason for hiding this comment

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

@admitrievtsev Я вижу, что Вы добились хорошего покрытия тестами
image

  • Также с точки зрения оптимальности кода юнит-тестов, их названий и устройства не могу сделать никаких замечаний: всё читаемо, понятно и выглядит эффективно. Разве что я не понял, в чём состоит существенное отличие simple array test и simple array set test (Я даже скажу больше, я закомментировал simple array test, после чего покрытие совершенно не изменилось, поэтому можно, в принципе, избавиться от этого теста).
  • Возможно, это уже придирка, но мне показалось немного странным, что после запуска тестов в логах остаются удалённые при работе значения в нодах, которые не несут информации для пользователя. Я понимаю, что Ваш метод remove реализован так, что он печатает значение удалённой ноды, поэтому просто так это исправить затруднительно. Короче говоря, это не очень важно, но если бы этих "паразитных" выводов не было, то, мне кажется, было бы круче
    image
  • Ещё я заметил, что при покрытии кроме очевидно недостижимых веток Вы не покрыли один случай
    image
  • Вот тут уже сложный момент: почему-то в тестах Вы не проверяете такой инвариант красно-чёрного дерева, как цвета вершин. С одной стороны, раз алгоритмы работают, происходят вращения, перебалансировки и в результате значения в тестах получаются такими, какими мы ожидаем, то можно сказать, что цвета проверять необязательно. Но, по крайней мере мне так кажется, что нужно всё равно убеждаться, что мы имеем дело именно с красно-чёрным деревом. Вдруг в нашем алгоритме есть какая-то ошибка, которая, например, игнорирует цвета и просто делает самосбалансированное дерево, тогда все Ваши тесты всё равно были бы пройдены, но дерево не обязательно бы удовлетворяло условиям красно-чёрного дерева и могло уже иметь другие вычислительные характериситки. Думаю, даже не обязательно проверять все цвета, достаточно было бы проверять чёрную высоту. Хотел бы узнать Ваше мнение по этому поводу, потому что, опять повторю, могу ошибаться в своём понимании

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Delete because the `maxVertex` method already exists
@n03d3n
Copy link

n03d3n commented Apr 3, 2024

@admitrievtsev @odiumuniverse @Ycyken Комментарии по оформлению репозитория:

  1. README содержит основные сведения о проекте, соответствует принятому оформлению. Понравилось использование кликабельных бейджей, которые дают быстрый доступ, например, к просмотру покрытия тестами и просто удобны, чтобы сразу увидеть, какие технологии поддерживаются в репозитории. Возможно, было бы неплохо ещё добавить примеры использования, а не только описания методов, но в нашем случае, в принципе, из названий способы использования кажутся очевидными
  2. CI сделан хорошо. Понравилось то, что реализован вывод покрытия тестами прямо на гитхабе. Каких-то замечаний не могу сделать
  3. Политика разработки и управление репозиторием:
  • Репорты линтера мне показались лишними в репозитории
  • Структура репозитория соответствует принятым стандартам. Кроме репортов линтера никаких лишних файлов или директорий нет. Все файлы и модули в проекте распределены по директориям понятным образом
  • Команда использовала GitHub flow с выделением отдельной ветки для работы с реализацией Красно-чёрного дерева. С этим никаких проблем тоже нет
  • Есть небольшая несогласованность в стилях коммитов: некоторые участники придерживаются “Conventional Commits”, когда другие используют более свободный стиль. Но при этом коммиты достаточно хорошо разбиты(то есть, почти всегда атомарны и отвечают за какую-то одну реализованную/изменённую функциональность), имеют понятные названия, по которым легко определить, что конкретно было изменено, а зачастую и вовсе содержат дополнительное описание
  • Таким образом, могу дать высокую оценку оформлению репозитория за исключением небольших шероховатостей.

Молодцы, хорошая работа

Copy link

@suvorovrain suvorovrain left a comment

Choose a reason for hiding this comment

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

photo_2024-04-03_19-32-30
@odiumuniverse Почему-то покрытие тестами вышло грустненькое по цифрам((, скорее всего из-за большого количества "?". Рекомендую воспользоваться гайдом Андрея по просмотру покрытия тестами прямо в IDEA (гайд в боталке), потому что у меня почему-то конкретно твой файл накрылся, и я тебе подробную информацию отправить не могу

Comment on lines 17 to 21
/**
* Associates the specified [value] with the specified [key] in the tree.
*
* @return the previous value associated with the key, or `null` if the key was not present in the tree.
*/

Choose a reason for hiding this comment

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

Опираясь на данную статью гугла: https://google.github.io/eng-practices/review/reviewer/looking-for.html "Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing."
Ваши комментарии делают как раз наоборот

Choose a reason for hiding this comment

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

причем очень много где

Copy link
Contributor

@Ycyken Ycyken Apr 3, 2024

Choose a reason for hiding this comment

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

Я писал док-комменты к методам, опираясь на то, как они написаны в стандартной библиотеке котлина. По-моему, описание функции важнее, чем указание причины, по которым нужен тот или иной публичный метод.

Copy link
Contributor

@Ycyken Ycyken Apr 3, 2024

Choose a reason for hiding this comment

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

Возможно, эта конкретная рекомендация больше относится к комментированию блоков кода непосредственно алгоритмов, каким-то более сложным системам и проектам, но для док-комментариев конкретно этих функций она явно не подходит

return Pair(newVertex, null)
}

var cur = root ?: throw IllegalStateException("Case when root is null is processed above")

Choose a reason for hiding this comment

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

Данная стратегия борьбы с null убивает вашу многопоточность. в таких случаях вмешательство другого потока может вызвать exception, что будет неправильно, этот случай необходимо точно так же обрабатывать, это не является какой-то некорректной работой программы

Copy link
Contributor

Choose a reason for hiding this comment

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

Дерево никак не приспособлено к работе с многопоточностью, у нас нет ни примитивов, ни синхронизации.
Иначе варианта это обработать я не нашел(типо таких случаев в одном потоке просто не может быть), так что считаем что exception это просто !! с доп сообщением.
Можно сказать что многопоточность была убита еще до рождения)

newVertex.parent = cur
return Pair(newVertex, null)
}
cur = cur.left ?: throw IllegalStateException("Case when cur.left is null is processed above")

Choose a reason for hiding this comment

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

Аналогично

newVertex.parent = cur
return Pair(newVertex, null)
}
cur = cur.right ?: throw IllegalStateException("Case when cur.right is null is processed above")

Choose a reason for hiding this comment

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

И тут

Comment on lines 152 to 173
protected fun maxVertex(vertex: Vertex_t?): Vertex_t? {
var cur = vertex
while (cur?.right != null) {
cur = cur.right
}
return cur
}

/**
* Returns Pair<K,V> by maximum key in the tree. If tree is empty then returns null
*/
fun max(): Pair<K, V>? {
return maxVert(root)?.toPair()
}

private fun maxVert(vertex: Vertex_t?): Vertex_t? {
var cur = vertex
while (cur?.right != null) {
cur = cur.right
}
return cur
}

Choose a reason for hiding this comment

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

Очевидно, что метод maxVert тут лишний

}

@Test
fun testCheckEmptyTree() {

Choose a reason for hiding this comment

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

@odiumuniverse Названия тестов можно через `` писать, а лучше через @DisplayName. Улучшает readability🤓

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут стоило бы пнуть за не единый кодстайл(написания имен), но в принципе согласен, исправлю

abstract class BSTreeTemplate<K : Comparable<K>, V, Vertex_t : VertexTemplate<K, V, Vertex_t>> {
var root: Vertex_t? = null
protected set
var size: Int = 0

Choose a reason for hiding this comment

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

@odiumuniverse не совсем понятно, зачем вы вообще сделали size.
Используете вы его только в тестах, но проверка на размер итогового дерева - это, объективно, слабая проверка. Андрей высказывался на этот счёт в красно-черном дереве, и это везде применимо. Проверять лишь размер дерева может быть недостаточно, для того, чтобы убедиться в корректности методов.
По мне так лучше было делать обход дерева, тем самым проверяя всю его структуру. А из за size вы еще и тратите ресурсы на постоянное поддержание текущего размера (я имею ввиду size++ size-- и т.д).

Copy link
Contributor

@odiumuniverse odiumuniverse Apr 3, 2024

Choose a reason for hiding this comment

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

Мы это сделали для пользователя, чтобы он мог использовать размер дерева в своих целях.
Это даже в стандартном дереве котлина есть, у которого под капотом Red-Black Tree
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@odiumuniverse не совсем понятно, зачем вы вообще сделали size. Используете вы его только в тестах, но проверка на размер итогового дерева - это, объективно, слабая проверка. Андрей высказывался на этот счёт в красно-черном дереве, и это везде применимо. Проверять лишь размер дерева может быть недостаточно, для того, чтобы убедиться в корректности методов. По мне так лучше было делать обход дерева, тем самым проверяя всю его структуру. А из за size вы еще и тратите ресурсы на постоянное поддержание текущего размера (я имею ввиду size++ size-- и т.д).

cringe

Choose a reason for hiding this comment

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

Мы это сделали для пользователя, чтобы он мог использовать размер дерева в своих целях. Это даже в стандартном дереве котлина есть, у которого под капотом Red-Black Tree image

оки. Но с тестами замечание не снимаю

Copy link
Contributor

Choose a reason for hiding this comment

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

Да, по-хорошему должна быть функция для тестирования размера дерева

@odiumuniverse
Copy link
Contributor

Буквально только что проверил
image
image
В IDEA покрытие вообще 100%

photo_2024-04-03_19-32-30 @odiumuniverse Почему-то покрытие тестами вышло грустненькое по цифрам((, скорее всего из-за большого количества "?". Рекомендую воспользоваться гайдом Андрея по просмотру покрытия тестами прямо в IDEA (гайд в боталке), потому что у меня почему-то конкретно твой файл накрылся, и я тебе подробную информацию отправить не могу

@suvorovrain
Copy link

@odiumuniverse Еще в общем отмечу, что кодстайл сигнатур ваших методов\классов не соответствует официальному кодстайлу котлина. Но в целом могу понять, почему вы его не придерживались https://kotlinlang.org/docs/coding-conventions.html#class-headers

Ycyken and others added 19 commits April 5, 2024 07:22
-open AvlTree, RBTree and SimpleTree for extension
-create AvlTestTree, RBTestTree and SimpleTestTree for getting root and
testing inner structure of the trees
Change root visibility modifier to protected
-rename fabricVertex to createVertex
-remove redundant argument in createVertex
-remove XTestTrees classes from test package
-change root visibility to internal
-remove template word from the class names
-rename templates package to abstracts
…-delegation

Refactor fabricVertex method, warnings, rename classes and package, remove XTestTress classes
1. The launch is removed simultaneously immediately on PUSH and PR
2. CITEST is now launched on different operating systems
3. An extra Build team has been removed
The .keep folder was removed because it was only needed to send an empty directory
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.

5 participants