Conversation
add: gradle init files, cbreated gradle application for project 'trees-9'
deleted .idea files
created abstract node, tree and bst pattern's
Mergeable config
removed parent in node, changed type of rootNode
changed abstract tree to binary tree, implemented find function and add in bst
deleting node.kt to split into separate files
small codestyle changes
Implemented BinarySearchTree, all nodes. Inited RBTree and AVLTree
Create tests to check tree state after balance
Implemented import, export, open and close functions to work with neo4j
Added closing statement, removed unnecessary columns, fixed bugs with table deletion
* add: menu of functions for actions on trees, implementation of the remaining windows * fix: fixed crooked ribs; window dimensions are now static
| Column() { | ||
| Column( | ||
| Modifier.width(200.dp).height(260.dp).offset(25.dp, 10.dp), | ||
| verticalArrangement = Arrangement.Top, | ||
| horizontalAlignment = Alignment.Start, | ||
| ) { | ||
| val fieldsWidth = 140 | ||
| var key by remember { mutableStateOf("") } | ||
| OutlinedTextField( | ||
| modifier = Modifier | ||
| .width(fieldsWidth.dp) | ||
| .height(65.dp) | ||
| .padding(vertical = 5.dp), | ||
| value = key, | ||
| onValueChange = { | ||
| textMessage = mutableListOf("") | ||
| key = it | ||
| enteredKey = it | ||
| }, | ||
| label = { Text("Enter key", textAlign = TextAlign.Center) } | ||
| ) | ||
| var value by remember { mutableStateOf("") } | ||
| OutlinedTextField( | ||
| modifier = Modifier | ||
| .width(fieldsWidth.dp) | ||
| .height(65.dp) | ||
| .padding(vertical = 5.dp), | ||
| value = value, | ||
| onValueChange = { | ||
| value = it | ||
| enteredValue = it | ||
| }, | ||
| label = { Text("Enter value", textAlign = TextAlign.Center) } | ||
| ) | ||
| } | ||
| val text = textMessage.toString().slice(1 until textMessage.toString().length - 1) | ||
| Text( | ||
| text = text, | ||
| fontSize = 18.sp, | ||
| modifier = Modifier | ||
| .offset((10).dp, (-55).dp) | ||
| .width(150.dp), | ||
| textAlign = TextAlign.Center, | ||
| ) | ||
| } |
There was a problem hiding this comment.
В BSTScreen.kt дублируется этот фрагмент. Вместо этого можно было бы использовать общий метод для сокращения количества кода и более быстрого фикса/рефакторинга
| import trees.BSTree | ||
| import trees.dataBases.BST.insertAllNodesToTree | ||
| import trees.dataBases.BST.removeFile | ||
| import trees.dataBases.BST.writeAllNodesToFile |
There was a problem hiding this comment.
"Unused import directive" (также в BSTScreen, RBScreen, nodeFunctions)
| val x1 = getX(end).dp | ||
| val y1 = getY(end).dp | ||
|
|
||
| if (marker == true) { |
There was a problem hiding this comment.
| if (marker == true) { | |
| if (marker) { |
| Button( | ||
| onClick = { | ||
| findMode = false | ||
| try { | ||
| textMessage = mutableListOf("") | ||
| tree.value.remove(enteredKey.toInt()) | ||
| screenReload = true | ||
| } catch (_: NumberFormatException) { | ||
| textMessage = mutableListOf("Node with this key doesn't exist") | ||
| } | ||
| }) { | ||
| Text( | ||
| text = "Remove node", | ||
| modifier = Modifier.width(110.dp), | ||
| fontSize = 16.sp, | ||
| textAlign = TextAlign.Center | ||
| ) | ||
| } | ||
| Button( | ||
| onClick = { | ||
| textMessage = mutableListOf("Write key that you are looking for") | ||
| findMode = !findMode | ||
| screenReload = true | ||
| }) { | ||
| Text( | ||
| text = "Find mode", | ||
| modifier = Modifier.width(110.dp), | ||
| fontSize = 16.sp, | ||
| textAlign = TextAlign.Center | ||
| ) | ||
| } |
There was a problem hiding this comment.
Дублирование (BSTScreen.kt 139-169), также можно было вынести в отдельный метод
| fun AVLScreen(toMenu: () -> Unit) { | ||
| val db = SQLiteDB("./app/src/main/kotlin/trees/dataBases/AVL/base.db") | ||
| val tree = remember { mutableStateOf(AVLTree<Int, String>()) } | ||
| var textMessage by remember { mutableStateOf(mutableListOf<String>()) } |
There was a problem hiding this comment.
| var textMessage by remember { mutableStateOf(mutableListOf<String>()) } | |
| val textMessage = remember { mutableStateListOf<String>()} |
Из документации
| var coordinate = Coordinate(850f, 0f) | ||
| var parent = AVLNode(0, "") | ||
| var value = | ||
| settingValue(enteredValue, coordinate.x + 50.0f, coordinate.y + .0f, 0, false) | ||
| var curNode = BSNode(enteredKey.toInt(), value) | ||
| tree.value.add(curNode.key, curNode.value) | ||
| if (tree.value.root?.key != enteredKey.toInt()) { | ||
| parent = getAVLParent(enteredKey.toInt(), tree.value.root!!) | ||
| coordinate = getCoordinate(parent.value) | ||
| println("${coordinate.x} ${coordinate.y}") | ||
| } | ||
| val point = findBracketPoint(value) | ||
| val offset = 85f | ||
| value = if (parent.left?.key == enteredKey.toInt()) { | ||
| settingValue(value, coordinate.x - offset, coordinate.y + offset, point, true) | ||
| } else { | ||
| settingValue(value, coordinate.x + offset, coordinate.y + offset, point, true) | ||
| } | ||
| curNode = BSNode(enteredKey.toInt(), value) | ||
| tree.value.add(curNode.key, curNode.value) |
There was a problem hiding this comment.
Этот фрагмент также можно вынести в отдельный метод, который будет принимать tree в качестве параметра
| } | ||
| } | ||
| } | ||
| var dop = 0 |
There was a problem hiding this comment.
Совсем не очевидно, что обозначает переменная
There was a problem hiding this comment.
Общие комментарии:
- У вас явно два раздельных модуля: библиотека и приложение -- лучше было разделить их на два Gradle-проекта, иначе весь код перемешен.
- Есть неотвеченные и непофикшенные комментарии предыдущих ревьюеров, с которыми я согласен -- я их не повторял, но стоило бы их исправить.
Комментарии к библиотеке:
- Пользователю при поиске по дереву возвращается вершина, через интерфейс которой он может изменять внутреннюю структуру дерева, нарушая любые его инварианты -- нормальная инкапсуляция отсутствует. Нужно было создать для пользователя отдельный интерфейс вершины, через который нельзя было бы изменять структуру дерева.
- Есть повторяющийся код. Нужно было пользоваться возможностями ООП (такими как наследование) и переиспользовать уже реализованную функциональность.
- Стоило добавить документацию к публичным методам и/или хотя бы примеры работы с библиотекой в ридми.
- Тесты -- это нечто... Для начала, они просто не работают: в некоторых в функции проверки не передаются те деревья, что тестируются. Более того, самих тестов недостаточно, да и основная часть тех, что есть, построены полностью на рандоме, из-за чего непонятно, тестируются ли на самом деле все крайние случаи. Помимо всего этого, тесты AVL- и красно-черного деревьев -- просто копипаста тестов обычного дерева с точностью до вызова функции проверки. И последнее: нет тестов балансировки красно-черного дерева, зато зачем-то есть тесты балансировки обычного дерева (которое не балансирующееся) 🤯.
Комментарии к интерфейсу приложения:
- При нажатии на кнопки "Binary search tree" и "AVL tree" в главном меню приложение падает :'( Судя по сообщению, в обоих случаях не может найти файлы, пути к которым захардкожены в исходниках.
- При переходе между главным меню и редактором дерева размер окна сбрасывается, что неудобно
- На единственном экране редактора, что я смог открыть -- "Red-black tree" -- ни одна кнопка, кроме выхода обратно в меню, не работает, но об этом меня предупредили.
Комментарии к коду приложения:
- При билде появляются варнинги -- такого быть не должно.
- Непродуманная архитектура. По сути, нет никакого разделения наподобие MVC/MVP/MVVM или чего-то подобного, работа с деревьями находится прямо в UI.
- Нет разделения UI на элементы с их переиспользованием: каждый экран -- это огромная монолитная функция, причем, для каждого дерева -- почему-то своя.
Общее впечатление от работы плохое. Выглядит, будто делали в спешке и кое-как. Очень неаккуратный код, особенно в приложении, где ещё и архитектура по сути отсутствует. Само приложение в текущем состоянии не работает. В библиотеке пользователь может как хочет ломать внутреннюю структуру деревьев, тесты слабые.
| with: | ||
| distribution: zulu | ||
| java-version: '17' | ||
| cache: gradle |
There was a problem hiding this comment.
Для Gradle лучше использовать специальный официальный экшн -- у него лучше кэширование (и как бонус, можно чуть более компактно через него же Gradle-таски запускать)
| run: | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] |
There was a problem hiding this comment.
На Винде у вас какой-то варнинг вылезает -- можно в Actions увидеть
README.md
Outdated
| Test coverage by Intellej IDEA: | ||
| | Tree | Methods % | Lines % | | ||
| | ------------------- |:---:|:--:| | ||
| | Binary search tree | 100 | 96 | | ||
| | AVL tree | 100 | 98 | | ||
| | Reb-Black tree | 100 | 93 | |
There was a problem hiding this comment.
Немного смущает, что оно не соответствует тому, что в CI высвечивается:
Видимо в CI статистика по всему коду, включая приложение, раз вы модули не разделили.
Но jacoco по соответствующим классам тоже чуть более скромные числа выдаёт:
В общем, такие штуки статически в ридми указывать не стоит: код поменяете, а здесь не обновите -- уже будет неправда. Если хочется, чтобы покрытие было видно в ридми, есть всякие генераторы бейджей, хотя и не уверен, что есть настолько подробные.
| */ | ||
|
|
||
| rootProject.name = "trees-9" | ||
| include("app") |
There was a problem hiding this comment.
У вас есть два явно раздельных компонента: библиотека и приложение -- их по-хорошему надо было разделить на два Gradle-проекта. Это уменьшило бы связность кода. Из очевидного для конкретно вашего случая, это позволило бы запускать таску по проверке test coverage только для библиотеки.
| testImplementation("org.junit.jupiter:junit-jupiter:5.8.1") | ||
| testImplementation("org.junit.jupiter:junit-jupiter:5.8.1") | ||
| testImplementation("org.junit.jupiter:junit-jupiter:5.8.1") |
There was a problem hiding this comment.
Gradle с первого раза понимает :)
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun AVLScreen(toMenu: () -> Unit) { | ||
| val db = SQLiteDB("./app/src/main/kotlin/trees/dataBases/AVL/base.db") |
There was a problem hiding this comment.
А это, видимо, место, из-за которого падает экран AVL-дерева. Это видимо надо было спрашивать у пользователя, а не хардкодить. Да и вообще сначала дать дерево создать и сохранить, и только потом пытаться его загружать.
|
|
||
| class SQLiteDB(private val path: String) : Closeable { | ||
| private val connection = DriverManager.getConnection("$DB_DRIVER:$path") | ||
| ?: throw SQLException("Cannot connect to database") |
There was a problem hiding this comment.
Вроде бы эта ошибка нигде не отлавливается. То есть если вы пофиксите ваш код так, чтобы спрашивать путь у пользователя, то если он случайно ошибётся в пути, то всё приложение упадёт -- такого быть не должно. Ошибки нужно отлавливать и выдавать пользователю соответствующее понятное сообщение так, чтобы приложение при этом не падало.
| } | ||
|
|
||
| fun open() { | ||
| connection.createStatement().also { statement -> |
There was a problem hiding this comment.
Не понятно, зачем тут also -- лучше было просто сохранить в переменную и использовать её. Суть была бы та же, а вложенность бы уменьшилась. А ещё лучше использовать use, как вам уже ранее писали.
| } | ||
|
|
||
| fun writeAllNodesToDB(node: AVLNode<Int, String>?, tree: AVLTree<Int, String>) { | ||
| val stack = mutableListOf(node?.key) |
There was a problem hiding this comment.
Уже писал в другом файле: используйте Stack из Java
| import java.io.Closeable | ||
| import java.io.IOException | ||
|
|
||
| class Neo4jRepository: Closeable { |
There was a problem hiding this comment.
- Пользуйтесь автоформатированием IDE:
| class Neo4jRepository: Closeable { | |
| class Neo4jRepository : Closeable { |
- Для работы с
Closableресурсами (например, сессиями) используйтеuse-- вам не придется руками их закрывать, это обезопасит вас от утечек. - У вас бросаются эксепшены -- убедитесь, что приложение будет их отлавливать и выдавать пользователю понятное сообщение так, чтобы само приложение при этом не падало.
6 participants
👋! 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: @raf-nr @vacmannnn @gladiuswq