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

Jump to bottom

Closed

ivanvirchenko wants to merge 1 commit into main from GP-50-RefactorBinarySearchTreeUsingReflection

Closed

GP-50 Refactor binary search tree using reflection #58

ivanvirchenko wants to merge 1 commit into main from GP-50-RefactorBinarySearchTreeUsingReflection

Conversation

ivanvirchenko

Copy link

@ivanvirchenko ivanvirchenko commented Feb 3, 2021

No description provided.


 refactor binary search tree using reflection

28454d7

@tboychuk

Copy link

Contributor

tboychuk commented Feb 4, 2021

@erneman1 please add task number to the PR title, as we usually do.

@ivanvirchenko ivanvirchenko changed the title ~~(削除) refactor binary search tree using reflection (削除ここまで)~~ (追記) GP-50 Refactor binary search tree using reflection (追記ここまで)

Feb 4, 2021

tboychuk

tboychuk requested changes

Feb 4, 2021

View reviewed changes

Copy link

Contributor

@tboychuk tboychuk left a 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()

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

private static final Predicate<Field> ELEMENT_FIELD = field -> field.getName().equals("element")

|| field.getName().equals("item")

|| field.getName().equals("value");

private static final Predicate<Field> LEFT_FIELD = field -> field.getName().equals("left");

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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")

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

assertThat(bst.contains(e)).isFalse(); //does not contain

assertThat(bst.size()).isEqualTo(i);

void checkProperTreeFields() {

tree = new RecursiveBinarySearchTree<>();

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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...

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(4)

void shouldCreateTreeFromListOfElements() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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()

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

}

@Test

@Order(5)

void shouldThrowNullPointerExceptionIfInsertNull() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(6)

void shouldInsertUniqueElements() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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.

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(7)

void shouldIncreaseSizeWhenInsertUnique() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(8)

void shouldNotInsertNonUniqueElements() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

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

insertDoesNotAddDuplicateElements

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(9)

void shouldNotIncreaseSizeWhenInsertNotUnique() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

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

insertDoesNotChangeSizeWhenElementWasNotAdded

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

assertThat(traversedElements).isEqualTo(List.of(sortedElements));

}

@Test

@Order(12)

void shouldInsertToRootIfEmpty() {

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/bst/BinarySearchTreeTest.java

@Test

@Order(17)

void shouldReturnTrueIfContains() {

fillTestTree(someElements);

Copy link

Contributor

@tboychuk tboychuk Feb 4, 2021

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();

@ivanvirchenko ivanvirchenko closed this

Feb 4, 2021

@ivanvirchenko ivanvirchenko deleted the GP-50-RefactorBinarySearchTreeUsingReflection branch

February 4, 2021 14:37

Labels

None yet

2 participants

@ivanvirchenko @tboychuk

Uh oh!

GP-50 Refactor binary search tree using reflection #58

GP-50 Refactor binary search tree using reflection #58

Uh oh!

Conversation

@ivanvirchenko ivanvirchenko commented Feb 3, 2021

Uh oh!

tboychuk commented Feb 4, 2021

Uh oh!

@tboychuk tboychuk left a comment

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!