This appears at first blush to be a C-style linked list written in C++. It's hard to say for sure since we don't have a bigger picture of your code. A C++ linked list would at least put these functions in a class, and not have a global Node.
The node is what's called an implementation detail. It helps us write the list, but users of the list class should not be able to declare their own nodes, nor should they be aware that nodes exist. Hopefully people using your class aren't calling this delete function explicitly.
Users of your List class might/should (depends) prefer to have iterators available to them. If anything writing iterators for your containers will allow them to be used with range-based for loops. Since your linked list appears to be singly linked, your iterators can only move forward. I also wrote my class with an iterator as I imagine it will help prevent copy/paste homework submissions in most cases.
Now to your function. Per my comment, passing the head as a double pointer makes no sense. Not once do you use it as a double pointer; you always de-reference it. So cut out the middle-man and pass it a node pointer from the get-go.
There's a case that's guaranteed not to happen, and that's that deleteNodePtr will never be the last node. That sounds bad. People can want to delete the last node, but they're allowed and no justification is given.
There's no code to catch an attempt to delete on an empty list.
Your specific issue seems to lie here:
while(current->next!=deleteNodePtr){
prev = current;
current=current->next;
}
You break out of the loop when current->next is the node to be deleted, and not current. You end up deleting the wrong node (current, which is 1 before deleteNodePtr), and that's likely going to cause issues down the line. For instance, if the second node is supposed to be deleted, you end up deleting your head, and that breaks stuff. I imagine that removing the ->next from your Boolean condition would fix the issue. I can't provide a deeper resolution without seeing more of your code.
=== Optional reading ===
Here's an extremely stripped down linked list written as a C++ class. You can run this code here: https://godbolt.org/z/7jnvje
Or compile it yourself.
#include <iostream>
// A simple linked list for integers
class List {
public:
List() = default;
~List();
// List(const List& other); // Copy ctor needed for Rule of 5
// List(List&& other) noexcept; // Move ctor needed for Rule of 5
void push_back(int val);
class iterator;
iterator begin();
iterator end();
iterator find(int val);
void erase(iterator it);
void clear();
// friend void swap(List& lhs, List& rhs); // Not Rule of 5; aids Rule of 5
// List& operator=(List other); // Assignment operator needed for Rule of 5
/*
* Quick note on Rule of 5 functions.
* If your class deals with heap-allocated resources, certain functions become
* required. The only one I included was the destructor. The signature of my
* assignment operator is different than you might see, but the reason is
* it's written to take advantage of the copy/swap idiom.
* https://stackoverflow.com/a/3279550/6119582
*/
private:
// Data
struct Node {
int value = 0;
Node *next = nullptr;
Node(int val) : value(val) {}
};
Node *m_head = nullptr;
Node *m_tail = nullptr;
// Functions
Node *find_node(int val);
};
class List::iterator {
public:
iterator(Node *loc) : location(loc) {}
iterator &operator++();
int operator*();
bool operator==(const List::iterator &rhs);
bool operator!=(const List::iterator &rhs);
private:
Node *location;
};
// List Implementation
List::~List() { clear(); }
void List::push_back(int val) {
if (m_tail) {
m_tail->next = new Node(val);
m_tail = m_tail->next;
return;
}
m_head = new Node(val);
m_tail = m_head;
return;
}
List::iterator List::begin() { return iterator(m_head); }
List::iterator List::end() { return iterator(nullptr); }
List::iterator List::find(int val) { return iterator(find_node(val)); }
void List::erase(iterator it) {
// Emtpy list or end()
if (!m_head || it == end())
return;
Node *toDelete = find_node(*it);
// Deleting head
if (toDelete == m_head) {
m_head = m_head->next;
delete toDelete;
return;
}
// Deleting tail
if (toDelete == m_tail) {
Node *walker = m_head;
while (walker->next != m_tail) {
walker = walker->next;
}
m_tail = walker;
delete m_tail->next;
m_tail->next = nullptr;
return;
}
// Delete any middle node; by moving value until it is the tail, then
// deleting the tail
while (toDelete->next) {
toDelete->value = toDelete->next->value;
if (toDelete->next == m_tail) {
m_tail = toDelete;
}
toDelete = toDelete->next;
}
delete toDelete;
m_tail->next = nullptr;
}
void List::clear() {
while (m_head) {
Node *tmp = m_head;
m_head = m_head->next;
delete tmp;
}
m_tail = nullptr;
}
List::Node *List::find_node(int val) {
if (!m_head) {
return nullptr;
}
Node *walker = m_head;
while (walker && walker->value != val) {
walker = walker->next;
}
return walker;
}
// List iterator implementation
List::iterator &List::iterator::operator++() {
location = location->next;
return *this;
}
int List::iterator::operator*() { return location->value; }
bool List::iterator::operator==(const List::iterator &rhs) {
return location == rhs.location;
}
bool List::iterator::operator!=(const List::iterator &rhs) {
return !(*this == rhs);
}
// Free function
// NOTE: Should take list by const reference, but I didn't add the necessary
// code for that. I'm not passing by value because I also left out Rule of 5
// code that is otherwise required.
// NOTE 2: Could also be templatized and made more generic to print any
// container, but that's outside the scope of this answer.
void print(List &list) {
for (auto i : list) {
std::cout << i << ' ';
}
std::cout << '\n';
}
int main() {
List list;
for (int i = 1; i <= 10; ++i) {
list.push_back(i);
}
print(list);
list.erase(list.find(1));
print(list);
list.erase(list.find(10));
print(list);
list.erase(list.find(6));
print(list);
auto it = list.begin();
for (int i = 0; i < 3; ++i) {
++it;
}
list.erase(it);
print(list);
list.erase(list.find(25)); // Bogus value; could throw if so desired
print(list);
}
Erasing is made much easier with a doubly-linked list, but we don't have one. My erase function makes some checks and handles the head and tail situations individually. For any node in the middle of the list, I don't bother deleting that node specifically. What I do instead is shuffle the value to be deleted to the tail of the list, and then delete the tail.
My comments indicate some things that were left out. I also didn't mark any functions as const. My iterator does not satisfy all requirements of a ForwardIterator. People can probably find other things I left out. I have a couple reasons for this. Mainly that this is quick and dirty code, and I prefer to not provide the temptation of a copy/paste solution.
It would be nice if all C++ instructors would actually teach C++, though. This form of linked list should not be taught in a C++ class anymore.
