Skip to content

Renamed classes and added @Before method to test class #891

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

Merged
merged 1 commit into from
Oct 9, 2017

Conversation

delbertlegg
Copy link
Contributor

Fixes #376


Reviewer Resources:

Track Policies

Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great @delbertlegg! :)

I've left a couple of comments, but they're both relatively minor things.

Thanks for taking the time to do this! :)


public class BSTTest {
public class BinarySearchTreeTest {
BinarySearchTree<Integer> bst;
Copy link
Contributor

@FridaTveit FridaTveit Oct 9, 2017

Choose a reason for hiding this comment

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

Just as a heads up, this will probably be changed as part of issue #230 so that not all the binary search trees used in the tests will be of type Integer. In which case we won't be able to save it as a private variable anymore :( So it's fine to leave it as a private variable for now, the way the tests are structured at the moment it's a really good change :) But just be aware that it might be changed back in the near future :)

Also also, if you do decide to keep it then it should be made private :)


@Before
public void setUp() {
bst = new BinarySearchTree<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably rename this as well to binarySearchTree because I find that clearer. That's just my preference though, feel free to leave it as it is if you prefer it that way :)

@delbertlegg
Copy link
Contributor Author

@FridaTveit I can make those changes, no problem. It looks like part of #376 was changed when #230 was made? It sounded like the intent was to instantiate the BinarySearchTree once for the tests, but now it looks like we can't since it may change types for each test?

@FridaTveit
Copy link
Contributor

True, I didn't see that part of #376, sorry! I think it's fine to do what you've done and instantiate it in a @Before block, that's the best thing to do at the moment. But it will probably be changed once #230 is done. So I just wanted to let you know in case you saw that change and were annoyed that your changes were being undone :) Does that make sense? Sorry if I've caused any confusion!

@delbertlegg
Copy link
Contributor Author

@FridaTveit Makes sense, and no worries! I just want to make sure I understand the problem. I'll change the variable name and make it private for now. And I won't be annoyed about any changes, here to help :).

changed variable name and made it private
@delbertlegg delbertlegg force-pushed the 376_binary_search_tree branch from 7136df5 to 56cb043 Compare October 9, 2017 15:37
Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great, a really good improvement! :) Thanks for taking the time to do this @delbertlegg! :)

@FridaTveit FridaTveit merged commit bcabf81 into exercism:master Oct 9, 2017
@delbertlegg delbertlegg deleted the 376_binary_search_tree branch October 9, 2017 16:08
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.

2 participants