Skip to content

Feedback#1

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

Feedback#1
github-classroom[bot] wants to merge 76 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: @VersusXX @AlexandrKudrya @7gambit7

github-classroom bot and others added 30 commits March 13, 2024 20:06
Created and added MIT LICENSE.
Added initial project files.

Added README and gitignore
Fixed the incorrect MIT license link on the README badge
Minimal Working Tree. Tree class was created. All methods was realised, but not tested.
Push not tested to the main to create some fundament to other trees.
But more files to structure :)
Keys inequality and some files structure
Added RBTree class and all of it's methods.
Fixed merge conflicts
# Conflicts:
#	src/main/kotlin/Main.kt
#	src/main/kotlin/Tree.kt
Pre-Finale version of tree. Unsafe casts fixed
Fixed RBTree cast errors due to change of the Node parameter types.
Gradle now log tests. Test instruction covering - 60% on BSTree
MORE TESTS. All delete covering.
… For these two functions were added enum classes.

Also added tests for:
1) DFS modes
2) toString Normal mode
3) Negative scenarios(Adding existing key, merging "not-bigger" tree)
AlexandrKudrya and others added 15 commits April 2, 2024 05:46
Cancel the idea of creating a badge, due to the inability to change the repository settings.
Ci jacoco report in action summary
Added some more information and examples into README
merge: gradle and CI updates with README edit
Changed the sequence of the Usage examples. Moved them to the bottom.
Fixed a bug that printed colored RBTree vertically with some spacing issues.
Added an example image of colored output of Red-Black tree into README.
Little fixes of README and DFS orders naming and AVLTree.
Copy link

@Sibiri4ok Sibiri4ok left a comment

Choose a reason for hiding this comment

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

На мой взгляд все более чем хорошо, Если прям придираться,то существуют некоторые моменты, над которыми чуть-чуть нужно подумать, и , на мой взгляд, тривиальные тесты

@Test
fun `value should added correctly left-right`() {
val firstKey = -1
val secondKey = 0

Choose a reason for hiding this comment

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

Было бы более читаемым, если бы ключи добавлялись по порядку, Например firsKey = 1, secondKey = -1, thirdKey = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут будто названия больше отражают именно не порядок добавления, а порядок, кто кого больше. Вообщем понял

Comment on lines 71 to 83
fun `value should added correctly right-left`() {
val firstKey = -1
val secondKey = 0
val thirdKey = 1

tree.add(firstKey, firstKey.toString())
tree.add(thirdKey, thirdKey.toString())
tree.add(secondKey, secondKey.toString())

assert(
tree.root?.key == firstKey &&
tree.root?.right?.key == thirdKey &&
tree.root?.right?.left?.key == secondKey

Choose a reason for hiding this comment

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

Тут в общем тоже самое, что и в прошлом замечании

Comment on lines +105 to +111
fun `Value should be added after set if it not in tree`() {
val expectedResult = "A"
val key = 1

tree[key] = expectedResult

assert(tree[key] contentEquals expectedResult)

Choose a reason for hiding this comment

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

Вроде же супер очевидный тест, исходя из синтаксиса языка, всегда будет верен

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут проверял именно на то, что этот случай верно отработает, ввиду того, что оператор set переопределяю


@Test
fun `Get should return value`() {
val key = 1

Choose a reason for hiding this comment

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

А в чем отличие этого теста от позапрошлого?

Copy link
Contributor

Choose a reason for hiding this comment

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

А, тут подразумевалось, что этот тестирует оператор get, а прошлый set. Ну да, косяк

val secondKey = 0
val thirdKey = 1

tree.add(firstKey, firstKey.toString())

Choose a reason for hiding this comment

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

тоже самое замечание, что и в начале


val max = secondTree.max()
secondTree.merge(tree)

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

По названию не очевидно? Блин, ну ладно. Уточню


keys.forEach { tree.add(it, it.toString()) }

val next = tree.getNext(tree.root!!)

Choose a reason for hiding this comment

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

ban)

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут я так делаю, так как очевидно, что корень null не равен после добавления, там на это тест был. Но да, вероятно, лучше через оператор Элвиса сделать

@n03d3n
Copy link

n03d3n commented Apr 4, 2024

Вот это у вас заряженная БДСМ энергией библиотека получилась, даже какую-то боль(приятную) чуть ниже спины почувствовал после прочтения ридми. Киллерфича, как по мне, вообще заставит создателей Джавы и Котлина заменить свою реализацию TreeMap на вашу

MuhammetSultanov and others added 6 commits April 6, 2024 12:31
Removed overriden minimum and maximum methods because they already were inherited by Tree class.
Also removed unecessary minimum and maximum tests because they just inherit from Tree class.

Fixed some too many checks in one assert problem in RBTreeTests
Fixes to RBTree, RBNode and README after review
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.

6 participants