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

github-classroom · 2023-03-21T15:07:46Z

👋! 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: @PavlushaSource @AlexShmak @ALanovaya

Added check for empty description and approvals check

In the future, you need to implement each declared method, it is possible to add private Also think about Generic and Comparable To when implementing these methods

feat: Complete binary search tree realization

… tree with the necessary balancing logic.

…string key in the neo4j database

therain7 · 2023-04-19T23:22:59Z

+        )
+        return parse(nodesRecords)
+    }
+    fun unloadTree(): RedBlackTree<String>? {


У нас оно не работает(
Всегда возвращает null

therain7 · 2023-04-19T23:25:05Z

+                "CREATE (:node {key : \"${node.key}\", " +
+                        "value: \"${node.value ?: ""}\", " +
+                        "isBlack: ${node.color == BLACK}, " +
+                        "lkey: \"${left?.key ?: ""}\", " +
+                        "rkey: \"${right?.key ?: ""}\"}) "


Такой запрос уязвим к инъекциям. Через интерполяцию вставлять параметры очень опасно

* fix: Changed the files related to the project build and CI setup, changed mergeable * Issue #12 done (#18) * fix: Changed the logic of the findByKey function and initialization of trees * fix: Changed the initialization of trees in the database * fix: Rewrote all the bst methods without recursion and rewrote the tests for it * fix: Rewrote the AVL tree and tests for it * Refactor: rewrote the RBT and wrote tests for it [remove doesn't work!] * Issue #20 done (#23) * Feature: Added the ability to save string type keys in Sqlite database * Fix: solved the problem with gradle build * Issue #9 done implemented json repo to save trees with keys of type String and Int and values of type String * fix: Renamed function names to Json and SQLite repo Also add README * feature: Add jacoco coverage * fix: jacoco wrote differently in CI * fix: Found a bug in RBT remove * feature: Added tests for SQLite database * docs: Added explanations to the functions and removed unnecessary ones

…wing a tree with the necessary functionality for searching, inserting and removing with OpenRNDR library

olgsm

В целом, я бы больше сфокусировался не на алгоритмах поворота вершин (хотя если они корректные, вы молодцы) -- задание-то, кмк, раньше было не совсем и не только про это (поправьте меня, если я не прав). Задание про аккуратный ООП дизайн, конечно, и про проектирование архитектуры своей маленькой либы -- но как ее потенциально смогут использовать? покрыты ли все сценарии? есть ли документация? насколько это все расширяемо, можно ли будет добавлять еще виды деревьев? а другие базы данных? а если я захочу что-то визуализировать, как мне к вам втыкаться?) и прочее, и прочее)

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

olgsm · 2023-05-03T16:32:53Z

+dependencies {
+
+    // Neo4j
+    implementation("org.neo4j.driver", "neo4j-java-driver", "5.7.0")


В gradle проектах сейчас все чаще используют version-каталоги, советую почитать и потом перейти на их использование. Очень удобно шарить и контролировать версии зависимостей, особенно в большой монорепе с кучей саб-проектов

olgsm · 2023-05-03T16:46:58Z

+
+import com.fasterxml.jackson.annotation.JsonIgnore
+
+@Suppress("UNCHECKED_CAST")


Обычно если приходится писать @Suppress("UNCHECKED_CAST") на весь класс - это признак того, что с архитектурой что-то не так :)

olgsm · 2023-05-03T16:49:08Z

+    /**
+     * Adds a value to the tree by this key, if the key already exists, then overwrites the value
+     */
+    open fun insert(key: K, value: V? = null) {


Публичные методы обычно ставят в начале файла, так удобнее читать код потом

olgsm · 2023-05-03T16:57:25Z

+    internal var right: Subtree? = null
+    internal var left: Subtree? = null
+    @JsonIgnore
+    open var parent: Subtree? = null


protected? или хотя бы internal, но вообще делать что-либо internal ТОЛЬКО ради тестов — обычно плохая практика, предполагается что тесты на публичные методы класс уже достаточно unit. но тут уже на вкус и цвет :)

olgsm · 2023-05-03T16:57:54Z

+    /**
+     * Searches for the next subtree in order
+     */
+    private fun findMinimumTree(givenTree: Subtree): Subtree {


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

olgsm · 2023-05-03T17:09:27Z

+    val tree = reference("tree_id", TreesTable)
+}
+
+internal class AvlSubtree(id: EntityID<Int>) : IntEntity(id) {


Аналогично комменту про TreeDatabase

olgsm · 2023-05-03T17:13:09Z

+            throw IOException("Session failed", exeption)
+        }
+
+    } override fun close() {


Форматирование?) Добавьте ktlint какой-нибудь, чтобы он орал каждый раз, если забываете пока на автомате Ctrl+Alt+L жать :)

olgsm · 2023-05-03T17:14:32Z

+     * in other cases returns exception
+     */
+    fun setTree(treeName: String, currentTree: AVLTree<*, String>, typeKey: String): Unit = transaction(db) {
+        if (typeKey != "Int" && typeKey != "String") throw IllegalStateException("Saving this type of key is not supported in the tree")


Здесь и еще по коду кое-где - лучше кидать какие-то кастомные свои эксепшны о таких вещах, а не дженерик IllegalState

olgsm · 2023-05-03T17:15:51Z

+        file.writeText(serialized)
+    }
+
+    fun getNamesTrees(): Any {


Возвращать Any - плохой дизайн, нужно лучше возиться с системой типов. Это вообще очень мощная штука, на курсе по функционалкам это хорошо дают понять)

olgsm · 2023-05-03T17:16:53Z

+
+@TestMethodOrder(MethodOrderer.OrderAnnotation::class)
+class SqliteRepAVLTest {
+    private val repo = SqliteRepAVL("src/main/kotlin/database/sqlite/TestDatabase")


Это не совсем мок, конечно, - а что если бы SQLitе была не просто файликом?)
Но в целом, идея правильная.

Почитайте еще про dependency injection (DI) и том как его правильно делать, полезно будет. В котлине юзают Koin или Kodein обычно (но может и я отстал от моды уже лол). В этих же фреймворках обычно есть все чтобы мокать такие вещи.

Implementation of graphic interface for trees

Fixed removing when input is empty

github-classroom Bot and others added 19 commits March 21, 2023 15:07

Setting up GitHub Classroom Feedback

049db4c

feat: Add GNU GPL v3.0 license

2d75418

feat: Add .gitignore

6ba015c

feat: Generate Gradle application struct on DSL Kotlin

bc7aef1

feat: Add mergeable.yml

5e39ef6

Added check for empty description and approvals check

feat: Write the architecture of future trees

50704e5

In the future, you need to implement each declared method, it is possible to add private Also think about Generic and Comparable To when implementing these methods

BST realization (#4)

49a109c

feat: Complete binary search tree realization

feat: Change the architecture of trees, create an AbstractBST

3ace175

feat: Implement methods of balance

8e90598

feat: Implement insert and remove methods

47b93a9

feat: Fix mergeable feedback, add gradle wrapper, add workflows.

9e9b2f7

feat: Implemented insertion and deletion algorithms for the red black…

8535ced

… tree with the necessary balancing logic.

feat: Implement logic of working with sqlite

bb420ce

feat: Replaced everything "!!" to exception

f9e17a8

add: Add file with implementation of storing a red-black tree with a …

e3c2ab0

…string key in the neo4j database

feat: Add test binary search tree

9494658

feat: Add test AVL tree

b15c680

feat: Add test RBT

8d771fa

feat: Add readme

4b51159

therain7 reviewed Apr 19, 2023

View reviewed changes

PavlushaSource and others added 6 commits May 3, 2023 02:08

added @JsonIgnore annotation (#26)

5201418

add: Adding files for implementation of a graphical interface for dra…

3e0b9b2

…wing a tree with the necessary functionality for searching, inserting and removing with OpenRNDR library

fix: Added task for checking test coverage

0484147

fix: Change CI settings

06637e8

fix: Change cringe README

6d61fa8

olgsm reviewed May 3, 2023

View reviewed changes

refactor: Rename title in Main.kt

b6c165f

ALanovaya and others added 6 commits May 3, 2023 21:08

Merge pull request #27 from spbu-coding-2022/gui_development

3bd53d1

Implementation of graphic interface for trees

add: Added readme

e90dbc5

fix: Fixed mistake in name of repository

af2a6b6

fix: Fixed wrong name of title

1ae8edf

fix: Fixed removing when input is empty

d73c3f0

Merge pull request #28 from spbu-coding-2022/gui_development

8060a87

Fixed removing when input is empty


		import com.fasterxml.jackson.annotation.JsonIgnore

		@Suppress("UNCHECKED_CAST")

Uh oh!

Conversation

github-classroom Bot commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olgsm 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-classroom Bot commented Mar 21, 2023 •

edited

Loading