-
-
Notifications
You must be signed in to change notification settings - Fork 521
GP-50 Refactor binary search tree using reflection #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@erneman1 please add task number to the PR title, as we usually do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 overall looks very nice 👍
Please read my comments, most of them can be applied to all tests:
For instance:
- follow the proper naming convention
- group tests that are related to the same method, and set a corresponding order – e.g. first we test method
insert()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 it does not have to be left exactly. What if I call my field leftChild? You should better use toLowerCase().contains("left")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 why don't you move initialization to the field declaration?
private BinarySearchTree<Integer> tree = new RecursiveBinarySearchTree<>();
JUnit creates a new instance for every test method...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 please remove should from all test methods. The patter we follow is
methodNameDoesSomethigWhenSomeCondition. In this case it can be just
of() or ofCreatesNewTree()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 according to the pattern we use, it should be called
insertThrowsExceptionWhenArgumentIsNull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 the name should be just insert() I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 should be called insertIncreasesSizeWhenElementWasAdded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertDoesNotAddDuplicateElements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertDoesNotChangeSizeWhenElementWasNotAdded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 if you test insert method, please move these tests up and change the order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erneman1 I suggest that fillTestTree() use varargs parameters. Then the test will look more obvious since you will be able to see which values have been added.
fillTestTree(10, 9, 11); assertThat(tree.contains(10)).isTrue(); assertThat(tree.contains(9)).isTrue(); assertThat(tree.contains(11)).isTrue();
No description provided.