Let's start with the small stuff, to get in the groove.
Naming convention
Whether you use camelCase or PascalCase or snake_case does not matter, as long as you do so consistently.
Pick a style for your class names, the same or another style for your functions, etc... and stick to it.
I will assume that you go for PascalCase for classes and snake_case for methods, but do as YOU wish.
Spaces
The C++ grammar allows you not to put spaces around operators, however this really hurts readability.
Use spaces:
- after commas:
push_back(int data, Node* head)
- after keywords:
if (...)
, while (...)
, ...
- around operators:
a = b
, a + b
, ! a
, ...
The goal is to make sure that your eye does not accidentally "merge" two symbols and treat them as one. For example, !a
and la
are very close graphically speaking, yet they mean completely different things.
Useless code
Avoid writing code that does not bring anything to the table:
Node* Create(int data){
Node* root=new Node();
root->data=data;
root->next=NULL;
Node* head=root;
return head;
}
There is no reason to create a head
variable here. You can simply return root
directly.
Inconsistencies
Why does pull_front
return Node*
but pull_back
return void
?
This is even more problematic in that the user need to remember than she needs to call delete
on the result of pull_front
but there is no result to pull_back
...
Side Note: one would expect pop_front
and pop_back
, as far as names go.
Let's now move on to more interesting stuff.
Dynamic Memory Allocation
For production code, using new
or delete
is forbidden1. You should be using std::unique_ptr
, std::shared_ptr
, or one of the many Standard Library containers.
Since you do not appear to be familiar with unique_ptr
, I advise you to add them to your reading list and will use naked pointers for the rest of this answer.
1 Unless you are an experienced developer, and no other language facility or library allows you to write this code, and your colleagues have extensively reviewed your code.
Initialize your values
Whenever you build a Node
, you have to explicitly remember to initialize the next
member to nullptr
and the data
member to 0
, lest they have garbage values.
Instead, use automatic initialization and constructors:
struct Node {
Node() {}
Node(int d, Node* n): data(d), next(n) {}
int data = 0;
Node* next = nullptr;
};
Beware of recursion
While recursion is elegant, in languages like C++ it can lead to Stack Overflow.
As such, unbounded recursion should be avoided, and therefore your implementation of push_back
or pull_back
or Search
should be converted to an iterative approach (use a while
loop).
Encapsulation
It is generally recommended to encapsulate functionality. At the moment, anybody can fiddle with your Node
internals (and point its next
member to whatever they want without using your functions).
Once next
becomes private
however, only a specific set of functions will be able to access it:
- member functions (object-oriented approach)
- friend non-member functions
This ties in with the bigger question of whether you really want to expose the Node
itself, where a null node represents an empty list and the user has to explicitly call delete
on nodes you return. I would recommend to avoid exposing pointers and therefore to create a List
class with an inner Node
struct.
Let's see the code!
Note: I will not transpose all the code, only the first few methods.
class List {
public:
List() {}
bool is_empty() const { return this->head == nullptr; }
void push_front(int value) {
this->head = new Node(value, this->head);
}
void push_back(int value) {
if (this->head == nullptr) {
this->head = new Node(value, nullptr);
return;
}
Node* current = this->head;
while (current->next != nullptr) { current = current->next; }
current->next = new Node(value, nullptr);
}
int pop_front() {
assert(!this->empty(), "Cannot pop on empty list.");
Node* popped = this->head;
this->head = popped->next;
int const value = popped->data;
delete popped;
return value;
}
int pop_back() {
assert(!this->empty(), "Cannot pop on empty list.");
Node* current = this->head;
while (current->next != nullptr) { current = current->next; }
int const value = current->next->data;
delete current->next;
current->next = nullptr;
return value;
}
// ...
private:
struct Node {
Node() {}
Node(int d, Node* n): data(d), next(n) {}
int data = 0;
Node* next = nullptr;
};
Node* head = nullptr;
};