Feedback #1
Feedback #1
Conversation
![github-classroom[bot]](https://avatars.githubusercontent.com/in/68187?s=60&v=4){kind=small}
Traversing trees better
New helper methods
Fixes due to changes related to coverage being 100%
New methods tests
Updated Readme
Efficiency and Security Update
|
|
||
| private fun RedBlackTreeNode<K, V>.findSiblingIfNotRoot(): RedBlackTreeNode<K, V>? { | ||
| return when (this) { | ||
| this.parent!!.left -> this.parent!!.right |
There was a problem hiding this comment.
Ну выглядит !! не очень. Я понимаю, что функция типо приватная, вы расчитываете на инварианты и т.д. Но такой код (имхо) трудно поддерживать. Пришел какой-то горе-программист и написал хрень (сломав инварианты), а вам потом сидеть и разбираться почему тут NPE, что довольно тяжело (вы же никаких пояснений не добавили).
Мой вариант решение проблемы такой:
- Записать через
let
this.parent?.let {
return@let this.parent.right
} ?: throw Exception("Parent is null, is it normal???")
- Запись через
val
val parent = this.parent ?: throw Exception("Parent is null, is it normal???")
return this.parent.left -> this.parent!!.righНа мой взгляд, довольно элегантно и понятно.
There was a problem hiding this comment.
Распространите коммент на весь ваш код, а то эти !! немного глаз режут. Хотя вашу мотивацию так делать я понял из ревью на вторую команду. (Цель благая, реализация мне не нравится)
There was a problem hiding this comment.
А еше мб стоит такие функции через замыкания писать (функция в функции)... Так как используется она только в одном месте...
There was a problem hiding this comment.
Ну выглядит
!!не очень. Я понимаю, что функция типо приватная, вы расчитываете на инварианты и т.д. Но такой код (имхо) трудно поддерживать. Пришел какой-то горе-программист и написал хрень (сломав инварианты), а вам потом сидеть и разбираться почему тут NPE, что довольно тяжело (вы же никаких пояснений не добавили). Мой вариант решение проблемы такой:
- Записать через
letthis.parent?.let { return@let this.parent.right } ?: throw Exception("Parent is null, is it normal???")
- Запись через
valval parent = this.parent ?: throw Exception("Parent is null, is it normal???") return this.parent.left -> this.parent!!.righНа мой взгляд, довольно элегантно и понятно.
Знал, что меня будут ругать за это
Ну уж очень хотел coverage 100% :D (По-моему ни у кого больше нет такого + рандомных тестов тоже)
Не проверял, позволяет ли let поднимать coverage
В идеальной программе, наверное, я бы написал как раз через exception и эти exception все, которые неприватные методы, проверял. Но было лень.
There was a problem hiding this comment.
А еше мб стоит такие функции через замыкания писать (функция в функции)... Так как используется она только в одном месте...
Крутая штука, знал, но как-то не осозновал, что можно использовать. Тупо можно структурировать код таким образом, лайк
| } | ||
|
|
||
| private fun RedBlackTreeNode<K, V>.findSiblingIfNotRoot(): RedBlackTreeNode<K, V>? { | ||
| return when (this) { |
There was a problem hiding this comment.
Микро комментарий: почему when, а не if? Ветки же две...
There was a problem hiding this comment.
Аааа сори!.. Просто по-моему там раньше как раз и было три случая, просто убрал третий. Интересно, это имеет какое-то различие в производительности... (спойлер: да)
| } | ||
|
|
||
| @Test | ||
| fun searchThreeNodes() { |
There was a problem hiding this comment.
А че у вас тесты в одном месте одним образом называются, а другом -- другим... (через 'search three nodes')
There was a problem hiding this comment.
Эх, руки не доходили стандартизировать, а надо было. Наш грех. Слишком заняты новоработками
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Возможно стоит использовать @ParameterizedTest. Такой подход немного упращает логику тестирования. Обычно такой подход прекрасно ложиться на Unit тесты, в противном случае непонятно, почему у вас сразу три assert (или вам не рассказывали про unit тесты (-_-) )
@ParameterizedTest
@CsvSource(,,,)There was a problem hiding this comment.
ОООО классная фича. Как-то не знали о ней. Прям жалею что не знал.
| assertEquals("SecondNubmerTwo", regularTree.search(2)) | ||
| } | ||
|
|
||
| /* Fuzzing is disabled! Remove comments to debug regular tree |
There was a problem hiding this comment.
Мое мнение, что такое лучше выносить одним файлом и просто исключать из пайплайна. А то код немного замусоривается. Но это мое мнение, в некоторых опенсурс проектах, насколько я помню, такое приемлимо.
There was a problem hiding this comment.
Это крутая мысль, а то реально какое-то замусоривание 👍
| removeNode.left = null | ||
| removeNode.right = null | ||
| removeNode.parent = null | ||
| super.balance(balancer::remover, replaceNode.parent) |
There was a problem hiding this comment.
Воу, рефлексия... Не есть хорошо, на мой взгляд, обычно такое не стоит использовать (только в экстренных случаях). Обычно проблемы использование рефлексии говорит о косяках архитектуры-реализации ;))
There was a problem hiding this comment.
Я честно не до конца вник (оправдана она здесь или нет), поэтому тут просто пояснения мне нужны)
There was a problem hiding this comment.
На созвоне было выяснено, что использование рефлексии правильное
| return removedValue | ||
| } | ||
| } else { | ||
| setNodeRight(removeNode.parent!!, replaceNode) |
There was a problem hiding this comment.
Ну этож даже не эстетично(removeNode.parent!!) :((( Можно же написать:
val parent = removeNode.parent ?: throw Exception("I'm really beautiful....")И дальше без !! жить спокойно...
There was a problem hiding this comment.
Я поменял свою точку зрения на мир. Я подвёл своих тиммейтов с !!. Я никогда не буду стремиться за 100% coverage путём !!.
| ``` | ||
| ### Insert | ||
| ```kotlin | ||
| tree.put(23, "Apple") |
There was a problem hiding this comment.
Insert кажется более интуитивно понятным header'ом, чем put. А put - потому что interface реализовали
| nodeParent: AVLTreeNode<K, V>, | ||
| nodeChild: AVLTreeNode<K, V>?, | ||
| ) { | ||
| when (nodeChild) { |
There was a problem hiding this comment.
Из-за того, что when, а не if вложенность растет. (Я наконец-то придумал, почему это плохо)
| removeNode = nextNode | ||
| } | ||
|
|
||
| val replaceNode = if (removeNode.left != null) removeNode.left else removeNode.right |
There was a problem hiding this comment.
*Ладно, докапываюсь, но искренне непонятно почему так...
👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @kinokotakenoko9 @Mukovenkov-Roman-Sergeyevich @mshipilov5