Skip to content

/ C-Plus-Plus

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

added OOP approach for BST #966

Open
wants to merge 2 commits into
base: master
from
Open

added OOP approach for BST #966

wants to merge 2 commits into from

Conversation

@Dharni0607
Copy link

Dharni0607 commented Jul 21, 2020 •
edited by gitpod-io bot

Added object oriented approach for BST. Please review and let me know if any changes are required.
@Dharni0607
added OOP approach for BST
Loading status checks…
e94f7f0
@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2020

This pull request introduces 4 alerts when merging e94f7f0 into 8d68e61 - view on LGTM.com

new alerts:

  • 4 for Mismatching new/free or malloc/delete
@kvedala
Copy link
Collaborator

kvedala commented Jul 21, 2020

Thank you for your contribution 👍
Please note the following:

  1. Please do not delete the author checks in the description and add appropriate description. It is meant to facilitate contribution by authors to maintain repo standards
  2. please ensure the self-tests pass
  3. please ensure to read the contribution guidelines - code needs to be documented per doxygen standards to ensure that a layman can read and learn from your code
  4. Please ensure to fix the alerts reported by LGTM.
@kvedala kvedala linked an issue that may be closed by this pull request Jul 21, 2020
[Suggestion] Better Implementation for BST #777
Open
@Dharni0607
Copy link
Author

Dharni0607 commented Jul 21, 2020

Ok. I will make the changes.
@kvedala
Copy link
Collaborator

kvedala commented Jul 21, 2020

Ok. I will make the changes.

Thank you. Please review code in this file as well for reference.
@Panquesito7
Panquesito7 requested changes Jul 21, 2020
View changes
Copy link
Member

Panquesito7 left a comment

Thank you for your contribution.
Please consider the following:

1. Document what functions do, and what parameters do.
2. Use a namespace and save all the functions and classes there.
3. Free up allocated memory.
4. Address other review comments.
data_structures/binary_search_tree_OOP_approach.cpp Outdated

#include<bits/stdc++.h>
#include<queue>
using namespace std;

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Please remove this and use std:: where necessary.
data_structures/binary_search_tree_OOP_approach.cpp
* \Object Oriented approach
*/

#include<bits/stdc++.h>

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Don't use this library as it slows down the compilation process and is quite Linux-specified.
data_structures/binary_search_tree_OOP_approach.cpp
#include<queue>
using namespace std;

struct node {

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Undocumented struct.
data_structures/binary_search_tree_OOP_approach.cpp
root = NULL;
}

void insert(int data){

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Undocumented class function.
data_structures/binary_search_tree_OOP_approach.cpp Outdated
void insert(int data){
node *newnode = new node;
newnode->value = data;
newnode->left = NULL;

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Please use nullptr here, and in other parts of the code.
data_structures/binary_search_tree_OOP_approach.cpp Outdated
return current;
}

node *deleteNode(node *current, int target){

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Same here and other functions above.
data_structures/binary_search_tree_OOP_approach.cpp
}
};

int main() {

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Please add a comment structure like:

/**
 * Main function
 */
data_structures/binary_search_tree_OOP_approach.cpp Outdated
break;
case 6:
BST.postOrder(BST.getRoot());
break;

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Missing default statement.
data_structures/binary_search_tree_OOP_approach.cpp Outdated
}

void insert(int data){
node *newnode = new node;

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

Allocated memory is not freed/released.
data_structures/binary_search_tree_OOP_approach.cpp Outdated
/**
* \file
* \brief A simple tree implementation using structured nodes
* \Object Oriented approach
*/
Comment on lines 1 to 5

This comment has been minimized.

Sign in to view
Copy link
@Panquesito7

Panquesito7 Jul 21, 2020

Member

As @kvedala mentioned, please update this comment block like in skip_list.
@Panquesito7
Copy link
Member

Panquesito7 commented Jul 21, 2020

Please fix clang-tidy warnings.
@kvedala
Copy link
Collaborator

kvedala commented Jul 21, 2020

Please fix clang-tidy warnings.

Please, avoid posting multiple reviews until at least one of them is complete. Multiple reviews confuse the authors and create an unnecessary branching of fixes.
@Dharni0607 Dharni0607 force-pushed the Dharni0607:issue_#777 branch from e92f685 to 361ec59 Jul 23, 2020
@Dharni0607
Copy link
Author

Dharni0607 commented Jul 23, 2020

Hi @kvedala @Panquesito7 , I have updated the PR, there are couple of errors and warnings for destructor method and deletion of node. Can you please guide me.
@kvedala
Copy link
Collaborator

kvedala commented Jul 23, 2020 •
edited

Thank you for the fixes, @Dharni0607.

While the documentation fixes still remain, please note the following concerning the errors reported by the CI:

  1. /home/runner/work/C-Plus-Plus/C-Plus-Plus/data_structures/binary_search_tree_oop_approach.cpp:25:7: error: class 'BinarySearchTree' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions,-warnings-as-errors]
    This error asks to create a copy or assignment operator to the class. You can define one as
class BinarySearchTree {
public:
    BinarySearchTree(BinarySearchTree const&) = delete;
    BinarySearchTree& operator =(BinarySearchTree const&) = delete;
}

as class member functions

  1. this can be avoided by the second suggestion made: Use std::shared_ptr or std::unique_ptr functions of smart pointers feature of C++11 that will take care of memory de-allocation and thus make the destructor function useless.

Without the use of smart pointers, the implementation is so rudimentary that it can be implemented almost verbatim as a C program.
@Dharni0607 Dharni0607 force-pushed the Dharni0607:issue_#777 branch 4 times, most recently from 068f8c4 to 26eebec Jul 23, 2020
@kvedala
Copy link
Collaborator

kvedala commented Jul 23, 2020

The error mentions that either

  • both copy constructor and the destructor should be defined
  • or neither should be custom defined
@Dharni0607
Merge branch 'master' of https://github.com/TheAlgorithms/C-Plus-Plus …
Loading status checks…
08a8f34 
…into issue_#777
@Dharni0607 Dharni0607 force-pushed the Dharni0607:issue_#777 branch from 26eebec to 08a8f34 Jul 23, 2020
@kvedala
Copy link
Collaborator

kvedala commented Jul 23, 2020

  1. Missing move constructor:
BinarySearchTree(BinarySearchTree const&&) = delete;
  1. Error on line 81: /home/runner/work/C-Plus-Plus/C-Plus-Plus/data_structures/binary_search_tree_oop_approach.cpp:157:41: error: parameter 'n' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param,-warnings-as-errors]
@Panquesito7
Copy link
Member

Panquesito7 commented Jul 23, 2020

@Dharni0607 open the Gitpod link above and open the file you added.
There are some clang-tidy suggestions and quick-fixes there.

If you are still a bit confused, I can make a review fixing various clang-tidy warnings. 🙂
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Panquesito7 Panquesito7

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
automated tests are failing good first issue
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

[Suggestion] Better Implementation for BST
3 participants
@Dharni0607 @kvedala @Panquesito7
You can’t perform that action at this time.