Even though this data structure passes basic test cases I was not sure what will be the performance if the amount of input is high. Also, if there is any optimisation, please tell.

java tree generics

java tree generics

share|improve this question

edited Jan 6 at 21:32

Jamal♦

30.3k11116226

asked Jan 6 at 14:08

user3878073user3878073

241

share|improve this question

edited Jan 6 at 21:32

Jamal♦

30.3k11116226

asked Jan 6 at 14:08

user3878073user3878073

241

share|improve this question

share|improve this question

edited Jan 6 at 21:32

Jamal♦

30.3k11116226

edited Jan 6 at 21:32

Jamal♦

30.3k11116226

edited Jan 6 at 21:32

Jamal♦

30.3k11116226

30.3k11116226

asked Jan 6 at 14:08

user3878073user3878073

241

asked Jan 6 at 14:08

user3878073user3878073

241

asked Jan 6 at 14:08

user3878073user3878073

241

241

add a comment |

add a comment |

3 Answers3

active

oldest

votes

0

If you are using a binary search tree, you may want to “balance” it, so that the tree does not get too deep, and searches are fast.

I suggest having a look at https://en.wikipedia.org/wiki/Tree_rotation

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

add a comment |

0

In this answer I’ll comment on things like style, robustness or API design, not analyzing your algorithm.

package com.solo.workouts.collections.Tree;

The uppercase package name Tree doesn’t follow the Java naming conventions, and this will confuse any colleage that you might cooperate with. 99% of the Java folks follow these conventions, so any deviation irritates.

private Comparator comparator;

You surely got a warning from your IDE about using a raw type. This declaration can benefit from generics, so the compiler already checks that the comparator is capable of comparing objects of type T:

private Comparator<? super T> comparator;

You need a comparator that can compare objects of type T, e.g. if you want a BinarySearchTree<Double>, a Comparator<Double> or a Comparator<Number> will do, but a Comparator<String> won’t. And that’s what <? super T> says.

private BinarySearchTree(Comparator comparator , T type) { ... }

I’d delete this constructor. The T type argument has a confusing name (you have to pass a concrete instance of T and not a type like Double.class) and isn’t used at all. Let the private BinarySearchTree(Comparator comparator) { ... } constructor directly do the work.

private BinarySearchTree(Comparator comparator) { ... }

As already said for the field, add the generics specification here:

private BinarySearchTree(Comparator<? super T> comparator) {

In the add() method, you have numerous places where you create a new node for the new element: new Node<>(element). Especially the ones that you create just for the compare() call immediatelay after the comparison become garbage, and it happens repeatedly in the while loop. As all these nodes get exactly the same contents, it’s enough to create one Node in the very beginning of the add() method, and use it in all the places instead of creation.

You use Objects.requireNonNull(e); quite often, probably to avoid getting a NullPointerException later, deep inside some method call stack. Of course, this also throws a NullPointerException, but from a controlled place (I’m typically too lazy to do that). It would be even better to always add a descriptive text which variable was null.

I’m using an early return nested inside the while and if constructs. Some developers don’t like that, but I think it makes the code clearer. But it’s a metter of taste.

And I added curly braces, which I highly recommend to always do. It’s too easy to think you can add a second statement to dependent block, but without the braces it’s just one conditional statement, and the next one will be executed unconditionally.

One hint on formatting: your formatting is somewhat inconsistent. You’re probably formatting your code by hand. IDEs like Eclipse have automated formatting tools, e.g. Ctrl-I corrects the indentation of the block you marked in the source file, and Ctrl-Shift-F completely reformats the code. Very useful, makes formatting easy.

Documentation: for such a general-use class, you should write Javadocs for the public constructors and methods. Your IDE will create the boilerplate, and you have to write down what the methods and their arguments do (not how they do it). Have a look at the Java API specification to get an idea of rather good Javadocs.

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

add a comment |

0

Java uses camelCasing. Every word after the first (or including the first if it’s a class name) should be a capital. This make names easier to read, since it’s clearer where the words start and end. All your names should follow this convention. For example: getSuccessor and getNode. You use proper camel casing in a couple places, but you’re inconsistent.

Be more careful with your spacing.

while (currentnode!=null && 0!= compare(currentnode,node)) {

has a lot of inconsistent style that’s making it harder to read than necessary. Putting spaces around operators is the cleanest way to go in my opinion. I also prefer to add spaces after commas:

while (currentnode != null && 0 != compare(currentnode, node)) {

Just be consistent.

While your indentation for containsElement is poor, the idea behind the function is good. You’ve put most of the logic away in a general function contains that could be reused elsewhere if needed, then just delegate to it in containsElement. This is much better than the alternative of duplicating logic.

Overall, there’s nothing glaringly wrong with your logic, but your code is quite messy. You should practice making sure that your code is readable, for your own and other’s sake. Have more consistent spacing after methods and if lines, ensure that your indentation is correct, and make sure your spacing around operators is less haphazard. Ideally, I should be able to glance at your code and “break it down” in my head easily using spacing alone. That’s not as possible when there isn’t a convention that can be relied on.

Post as a guest

3 Answers3

3 Answers3

If you are using a binary search tree, you may want to “balance” it, so that the tree does not get too deep, and searches are fast.

I suggest having a look at https://en.wikipedia.org/wiki/Tree_rotation

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

add a comment |

0

If you are using a binary search tree, you may want to “balance” it, so that the tree does not get too deep, and searches are fast.

I suggest having a look at https://en.wikipedia.org/wiki/Tree_rotation

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

add a comment |

0

0

0

If you are using a binary search tree, you may want to “balance” it, so that the tree does not get too deep, and searches are fast.

I suggest having a look at https://en.wikipedia.org/wiki/Tree_rotation

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

If you are using a binary search tree, you may want to “balance” it, so that the tree does not get too deep, and searches are fast.

I suggest having a look at https://en.wikipedia.org/wiki/Tree_rotation

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

share|improve this answer

share|improve this answer

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

answered Jan 6 at 15:25

George BarwoodGeorge Barwood

847

847

add a comment |

add a comment |

0

In this answer I’ll comment on things like style, robustness or API design, not analyzing your algorithm.

package com.solo.workouts.collections.Tree;

The uppercase package name Tree doesn’t follow the Java naming conventions, and this will confuse any colleage that you might cooperate with. 99% of the Java folks follow these conventions, so any deviation irritates.

private Comparator comparator;

You surely got a warning from your IDE about using a raw type. This declaration can benefit from generics, so the compiler already checks that the comparator is capable of comparing objects of type T:

private Comparator<? super T> comparator;

You need a comparator that can compare objects of type T, e.g. if you want a BinarySearchTree<Double>, a Comparator<Double> or a Comparator<Number> will do, but a Comparator<String> won’t. And that’s what <? super T> says.

private BinarySearchTree(Comparator comparator , T type) { ... }

I’d delete this constructor. The T type argument has a confusing name (you have to pass a concrete instance of T and not a type like Double.class) and isn’t used at all. Let the private BinarySearchTree(Comparator comparator) { ... } constructor directly do the work.

private BinarySearchTree(Comparator comparator) { ... }

As already said for the field, add the generics specification here:

private BinarySearchTree(Comparator<? super T> comparator) {

In the add() method, you have numerous places where you create a new node for the new element: new Node<>(element). Especially the ones that you create just for the compare() call immediatelay after the comparison become garbage, and it happens repeatedly in the while loop. As all these nodes get exactly the same contents, it’s enough to create one Node in the very beginning of the add() method, and use it in all the places instead of creation.

You use Objects.requireNonNull(e); quite often, probably to avoid getting a NullPointerException later, deep inside some method call stack. Of course, this also throws a NullPointerException, but from a controlled place (I’m typically too lazy to do that). It would be even better to always add a descriptive text which variable was null.

I’m using an early return nested inside the while and if constructs. Some developers don’t like that, but I think it makes the code clearer. But it’s a metter of taste.

And I added curly braces, which I highly recommend to always do. It’s too easy to think you can add a second statement to dependent block, but without the braces it’s just one conditional statement, and the next one will be executed unconditionally.

One hint on formatting: your formatting is somewhat inconsistent. You’re probably formatting your code by hand. IDEs like Eclipse have automated formatting tools, e.g. Ctrl-I corrects the indentation of the block you marked in the source file, and Ctrl-Shift-F completely reformats the code. Very useful, makes formatting easy.

Documentation: for such a general-use class, you should write Javadocs for the public constructors and methods. Your IDE will create the boilerplate, and you have to write down what the methods and their arguments do (not how they do it). Have a look at the Java API specification to get an idea of rather good Javadocs.

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

add a comment |

0

In this answer I’ll comment on things like style, robustness or API design, not analyzing your algorithm.

package com.solo.workouts.collections.Tree;

The uppercase package name Tree doesn’t follow the Java naming conventions, and this will confuse any colleage that you might cooperate with. 99% of the Java folks follow these conventions, so any deviation irritates.

private Comparator comparator;

You surely got a warning from your IDE about using a raw type. This declaration can benefit from generics, so the compiler already checks that the comparator is capable of comparing objects of type T:

private Comparator<? super T> comparator;

You need a comparator that can compare objects of type T, e.g. if you want a BinarySearchTree<Double>, a Comparator<Double> or a Comparator<Number> will do, but a Comparator<String> won’t. And that’s what <? super T> says.

private BinarySearchTree(Comparator comparator , T type) { ... }

I’d delete this constructor. The T type argument has a confusing name (you have to pass a concrete instance of T and not a type like Double.class) and isn’t used at all. Let the private BinarySearchTree(Comparator comparator) { ... } constructor directly do the work.

private BinarySearchTree(Comparator comparator) { ... }

As already said for the field, add the generics specification here:

private BinarySearchTree(Comparator<? super T> comparator) {

In the add() method, you have numerous places where you create a new node for the new element: new Node<>(element). Especially the ones that you create just for the compare() call immediatelay after the comparison become garbage, and it happens repeatedly in the while loop. As all these nodes get exactly the same contents, it’s enough to create one Node in the very beginning of the add() method, and use it in all the places instead of creation.

You use Objects.requireNonNull(e); quite often, probably to avoid getting a NullPointerException later, deep inside some method call stack. Of course, this also throws a NullPointerException, but from a controlled place (I’m typically too lazy to do that). It would be even better to always add a descriptive text which variable was null.

I’m using an early return nested inside the while and if constructs. Some developers don’t like that, but I think it makes the code clearer. But it’s a metter of taste.

And I added curly braces, which I highly recommend to always do. It’s too easy to think you can add a second statement to dependent block, but without the braces it’s just one conditional statement, and the next one will be executed unconditionally.

One hint on formatting: your formatting is somewhat inconsistent. You’re probably formatting your code by hand. IDEs like Eclipse have automated formatting tools, e.g. Ctrl-I corrects the indentation of the block you marked in the source file, and Ctrl-Shift-F completely reformats the code. Very useful, makes formatting easy.

Documentation: for such a general-use class, you should write Javadocs for the public constructors and methods. Your IDE will create the boilerplate, and you have to write down what the methods and their arguments do (not how they do it). Have a look at the Java API specification to get an idea of rather good Javadocs.

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

add a comment |

0

0

0

In this answer I’ll comment on things like style, robustness or API design, not analyzing your algorithm.

package com.solo.workouts.collections.Tree;

The uppercase package name Tree doesn’t follow the Java naming conventions, and this will confuse any colleage that you might cooperate with. 99% of the Java folks follow these conventions, so any deviation irritates.

private Comparator comparator;

You surely got a warning from your IDE about using a raw type. This declaration can benefit from generics, so the compiler already checks that the comparator is capable of comparing objects of type T:

private Comparator<? super T> comparator;

You need a comparator that can compare objects of type T, e.g. if you want a BinarySearchTree<Double>, a Comparator<Double> or a Comparator<Number> will do, but a Comparator<String> won’t. And that’s what <? super T> says.

private BinarySearchTree(Comparator comparator , T type) { ... }

I’d delete this constructor. The T type argument has a confusing name (you have to pass a concrete instance of T and not a type like Double.class) and isn’t used at all. Let the private BinarySearchTree(Comparator comparator) { ... } constructor directly do the work.

private BinarySearchTree(Comparator comparator) { ... }

As already said for the field, add the generics specification here:

private BinarySearchTree(Comparator<? super T> comparator) {

In the add() method, you have numerous places where you create a new node for the new element: new Node<>(element). Especially the ones that you create just for the compare() call immediatelay after the comparison become garbage, and it happens repeatedly in the while loop. As all these nodes get exactly the same contents, it’s enough to create one Node in the very beginning of the add() method, and use it in all the places instead of creation.

You use Objects.requireNonNull(e); quite often, probably to avoid getting a NullPointerException later, deep inside some method call stack. Of course, this also throws a NullPointerException, but from a controlled place (I’m typically too lazy to do that). It would be even better to always add a descriptive text which variable was null.

I’m using an early return nested inside the while and if constructs. Some developers don’t like that, but I think it makes the code clearer. But it’s a metter of taste.

And I added curly braces, which I highly recommend to always do. It’s too easy to think you can add a second statement to dependent block, but without the braces it’s just one conditional statement, and the next one will be executed unconditionally.

One hint on formatting: your formatting is somewhat inconsistent. You’re probably formatting your code by hand. IDEs like Eclipse have automated formatting tools, e.g. Ctrl-I corrects the indentation of the block you marked in the source file, and Ctrl-Shift-F completely reformats the code. Very useful, makes formatting easy.

Documentation: for such a general-use class, you should write Javadocs for the public constructors and methods. Your IDE will create the boilerplate, and you have to write down what the methods and their arguments do (not how they do it). Have a look at the Java API specification to get an idea of rather good Javadocs.

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

In this answer I’ll comment on things like style, robustness or API design, not analyzing your algorithm.

package com.solo.workouts.collections.Tree;

The uppercase package name Tree doesn’t follow the Java naming conventions, and this will confuse any colleage that you might cooperate with. 99% of the Java folks follow these conventions, so any deviation irritates.

private Comparator comparator;

You surely got a warning from your IDE about using a raw type. This declaration can benefit from generics, so the compiler already checks that the comparator is capable of comparing objects of type T:

private Comparator<? super T> comparator;

You need a comparator that can compare objects of type T, e.g. if you want a BinarySearchTree<Double>, a Comparator<Double> or a Comparator<Number> will do, but a Comparator<String> won’t. And that’s what <? super T> says.

private BinarySearchTree(Comparator comparator , T type) { ... }

I’d delete this constructor. The T type argument has a confusing name (you have to pass a concrete instance of T and not a type like Double.class) and isn’t used at all. Let the private BinarySearchTree(Comparator comparator) { ... } constructor directly do the work.

private BinarySearchTree(Comparator comparator) { ... }

As already said for the field, add the generics specification here:

private BinarySearchTree(Comparator<? super T> comparator) {

In the add() method, you have numerous places where you create a new node for the new element: new Node<>(element). Especially the ones that you create just for the compare() call immediatelay after the comparison become garbage, and it happens repeatedly in the while loop. As all these nodes get exactly the same contents, it’s enough to create one Node in the very beginning of the add() method, and use it in all the places instead of creation.

You use Objects.requireNonNull(e); quite often, probably to avoid getting a NullPointerException later, deep inside some method call stack. Of course, this also throws a NullPointerException, but from a controlled place (I’m typically too lazy to do that). It would be even better to always add a descriptive text which variable was null.

I’m using an early return nested inside the while and if constructs. Some developers don’t like that, but I think it makes the code clearer. But it’s a metter of taste.

And I added curly braces, which I highly recommend to always do. It’s too easy to think you can add a second statement to dependent block, but without the braces it’s just one conditional statement, and the next one will be executed unconditionally.

One hint on formatting: your formatting is somewhat inconsistent. You’re probably formatting your code by hand. IDEs like Eclipse have automated formatting tools, e.g. Ctrl-I corrects the indentation of the block you marked in the source file, and Ctrl-Shift-F completely reformats the code. Very useful, makes formatting easy.

Documentation: for such a general-use class, you should write Javadocs for the public constructors and methods. Your IDE will create the boilerplate, and you have to write down what the methods and their arguments do (not how they do it). Have a look at the Java API specification to get an idea of rather good Javadocs.

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

share|improve this answer

share|improve this answer

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

answered Jan 6 at 17:54

Ralf KleberhoffRalf Kleberhoff

84027

84027

add a comment |

add a comment |

0

Java uses camelCasing. Every word after the first (or including the first if it’s a class name) should be a capital. This make names easier to read, since it’s clearer where the words start and end. All your names should follow this convention. For example: getSuccessor and getNode. You use proper camel casing in a couple places, but you’re inconsistent.

Be more careful with your spacing.

while (currentnode!=null && 0!= compare(currentnode,node)) {

has a lot of inconsistent style that’s making it harder to read than necessary. Putting spaces around operators is the cleanest way to go in my opinion. I also prefer to add spaces after commas:

while (currentnode != null && 0 != compare(currentnode, node)) {

Just be consistent.

While your indentation for containsElement is poor, the idea behind the function is good. You’ve put most of the logic away in a general function contains that could be reused elsewhere if needed, then just delegate to it in containsElement. This is much better than the alternative of duplicating logic.

Overall, there’s nothing glaringly wrong with your logic, but your code is quite messy. You should practice making sure that your code is readable, for your own and other’s sake. Have more consistent spacing after methods and if lines, ensure that your indentation is correct, and make sure your spacing around operators is less haphazard. Ideally, I should be able to glance at your code and “break it down” in my head easily using spacing alone. That’s not as possible when there isn’t a convention that can be relied on.

share|improve this answer

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

add a comment |

0

Java uses camelCasing. Every word after the first (or including the first if it’s a class name) should be a capital. This make names easier to read, since it’s clearer where the words start and end. All your names should follow this convention. For example: getSuccessor and getNode. You use proper camel casing in a couple places, but you’re inconsistent.

Be more careful with your spacing.

while (currentnode!=null && 0!= compare(currentnode,node)) {

has a lot of inconsistent style that’s making it harder to read than necessary. Putting spaces around operators is the cleanest way to go in my opinion. I also prefer to add spaces after commas:

while (currentnode != null && 0 != compare(currentnode, node)) {

Just be consistent.

While your indentation for containsElement is poor, the idea behind the function is good. You’ve put most of the logic away in a general function contains that could be reused elsewhere if needed, then just delegate to it in containsElement. This is much better than the alternative of duplicating logic.

Overall, there’s nothing glaringly wrong with your logic, but your code is quite messy. You should practice making sure that your code is readable, for your own and other’s sake. Have more consistent spacing after methods and if lines, ensure that your indentation is correct, and make sure your spacing around operators is less haphazard. Ideally, I should be able to glance at your code and “break it down” in my head easily using spacing alone. That’s not as possible when there isn’t a convention that can be relied on.

share|improve this answer

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

add a comment |

0

0

0

Java uses camelCasing. Every word after the first (or including the first if it’s a class name) should be a capital. This make names easier to read, since it’s clearer where the words start and end. All your names should follow this convention. For example: getSuccessor and getNode. You use proper camel casing in a couple places, but you’re inconsistent.

Be more careful with your spacing.

while (currentnode!=null && 0!= compare(currentnode,node)) {

has a lot of inconsistent style that’s making it harder to read than necessary. Putting spaces around operators is the cleanest way to go in my opinion. I also prefer to add spaces after commas:

while (currentnode != null && 0 != compare(currentnode, node)) {

Just be consistent.

While your indentation for containsElement is poor, the idea behind the function is good. You’ve put most of the logic away in a general function contains that could be reused elsewhere if needed, then just delegate to it in containsElement. This is much better than the alternative of duplicating logic.

Overall, there’s nothing glaringly wrong with your logic, but your code is quite messy. You should practice making sure that your code is readable, for your own and other’s sake. Have more consistent spacing after methods and if lines, ensure that your indentation is correct, and make sure your spacing around operators is less haphazard. Ideally, I should be able to glance at your code and “break it down” in my head easily using spacing alone. That’s not as possible when there isn’t a convention that can be relied on.

share|improve this answer

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

Java uses camelCasing. Every word after the first (or including the first if it’s a class name) should be a capital. This make names easier to read, since it’s clearer where the words start and end. All your names should follow this convention. For example: getSuccessor and getNode. You use proper camel casing in a couple places, but you’re inconsistent.

Be more careful with your spacing.

while (currentnode!=null && 0!= compare(currentnode,node)) {

has a lot of inconsistent style that’s making it harder to read than necessary. Putting spaces around operators is the cleanest way to go in my opinion. I also prefer to add spaces after commas:

while (currentnode != null && 0 != compare(currentnode, node)) {

Just be consistent.

While your indentation for containsElement is poor, the idea behind the function is good. You’ve put most of the logic away in a general function contains that could be reused elsewhere if needed, then just delegate to it in containsElement. This is much better than the alternative of duplicating logic.

Overall, there’s nothing glaringly wrong with your logic, but your code is quite messy. You should practice making sure that your code is readable, for your own and other’s sake. Have more consistent spacing after methods and if lines, ensure that your indentation is correct, and make sure your spacing around operators is less haphazard. Ideally, I should be able to glance at your code and “break it down” in my head easily using spacing alone. That’s not as possible when there isn’t a convention that can be relied on.

share|improve this answer

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

share|improve this answer

share|improve this answer

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

answered Jan 6 at 21:03

CarcigenicateCarcigenicate

2,78811229

2,78811229

add a comment |

add a comment |

Thanks for contributing an answer to Code Review Stack Exchange!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

Use MathJax to format equations. MathJax reference.

To learn more, see our tips on writing great answers.

draft saved

draft discarded

Thanks for contributing an answer to Code Review Stack Exchange!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.