Reputation: 23
I am trying to create a program to delete a node at the N'th position of a linked list. According to the output, it should be:
George Betty Felix Renee George Betty Felix George Felix Felix
When running on repl.it (my submission website), it brings up that I am getting a segmentation error. However, when I run it off my personal computer on CodeBlocks, it runs without errors, however, it only outputs the first line which is George
, Betty
, Felix
, and Renee
without deleting and re-outputting.
Here's my code:
#include <iostream>
#include <string>
using namespace std;
class Node {
public:
string name;
Node* next;
};
Node* head;
class LinkedList {
public:
LinkedList();
~LinkedList();
void push(string);
void output();
void remove(int);
private:
Node *first;
};
void LinkedList::remove(int n)
{
struct Node* temp1 = head;
if(n == 1)
{
head = temp1 -> next;
delete(temp1);
return;
}
int i = 0;
for(i = 0; i < n - 2; i++)
{
temp1 = temp1 -> next;
}
struct Node* temp2 = temp1 -> next;
temp1 -> next = temp2 -> next;
delete(temp2);
}
LinkedList::LinkedList()
{
first = NULL;
}
LinkedList::~LinkedList()
{
Node *current=first;
while(current!=NULL)
{
Node *ptr=current;
current = current->next;
delete(ptr);
}
}
void LinkedList::push(string data)
{
Node *temp;
temp = new Node;
(*temp).name = data;
(*temp).next = first;
first = temp;
}
void LinkedList::output()
{
Node *current = first;
while(current!=NULL)
{
cout << (*current).name << endl;
current = (*current).next;
}
cout << endl;
}
int main() {
LinkedList students;
students.push("Renee");
students.push("Felix");
students.push("Betty");
students.push("George");
students.output();
students.remove(3);
students.output();
students.remove(1);
students.output();
students.remove(0);
students.output();
}
Upvotes: 2
Views: 64
Reputation: 311078
For starters it is unclear what is this declaration
Node* head;
doing in the program.
The class Node
should be declared within the class LinkedList
and should be inaccessible for users of the list.
In C++ indices start from 0. So it would be better if you would count indices in the list also starting from 0. The type of the parameter n
of the function remove should have unsigned integer type for example the type size_t
.
You did not check within function whether a current pointer is equal to nullptr. So the function has undefined behavior.
Moreover instead of the data member first
the function refers to the global variable head
.
struct Node* temp1 = head;
It seems you copied and pastes somewhere (in the internet) a C code of singly linked list ant tried to update it.
The code of the function can be very simple if to use pointer to pointer. For example
void LinkedList::remove( size_t n )
{
Node **current = &first;
while ( n-- != 0 && *current != nullptr )
{
current = &( *current )->next;
}
if ( *current != nullptr )
{
Node *tmp = *current;
*current = ( *current )->next;
delete tmp;
}
}
Indices passed to the function start from 0 as it should be in a C++ program.
Here is a demonstrative program.
#include <iostream>
#include <string>
#include <functional>
class LinkedList
{
public:
LinkedList() = default;
LinkedList( const LinkedList & ) = delete;
LinkedList & operator =( const LinkedList & ) = delete;
~LinkedList()
{
while ( head )
{
delete std::exchange( head, head->next );
}
}
void push( const std::string &s )
{
head = new Node { s, head };
}
bool remove( size_t n )
{
Node **current = &head;
while ( n-- != 0 && *current != nullptr )
{
current = &( *current )->next;
}
bool success = *current != nullptr;
if ( success )
{
delete std::exchange( *current, ( *current )->next );
}
return success;
}
friend std::ostream & operator <<( std::ostream &os, const LinkedList &list )
{
for ( Node *current = list.head; current != nullptr; current = current->next )
{
os << current->name << " -> ";
}
return os << "null";
}
private:
struct Node
{
std::string name;
Node* next;
} *head = nullptr;
};
int main()
{
LinkedList students;
students.push("Renee");
students.push("Felix");
students.push("Betty");
students.push("George");
std::cout << students << '\n';
students.remove(3);
std::cout << students << '\n';
students.remove(1);
std::cout << students << '\n';
students.remove(0);
std::cout << students << '\n';
return 0;
}
Its output is
George -> Betty -> Felix -> Renee -> null
George -> Betty -> Felix -> null
George -> Felix -> null
Felix -> null
Upvotes: 0
Reputation: 66244
Your code for everything but LinkedList::remove
manages the list via the first
member variable. But LinkedList::remove
references head
, a suspiciously otherwise-unused global variable. I'm confident that shouldn't even be in the code at all.
Delete the global head
, and change LinkedList::remove
to be:
void LinkedList::remove(int n)
{
Node **pp = &first;
while (*pp && n-- > 1)
pp = &(*pp)->next;
if (*pp)
{
Node *tmp = *pp;
*pp = tmp->next;
delete tmp;
}
}
Upvotes: 2