Remove first node of the linked list

[frog1990]

What should I change in my code so programm would delete first element from the list?

  void single_llist::delete_pos()
{
    int pos, i, counter = 0;
    if (start == NULL)
    {
        cout<<"List is empty"<<endl;
        return;
    }
    cout<<"Enter the position of value to be deleted: ";
    cin>>pos;
    struct node *s, *ptr;
    s = start;
    if (pos == 1)
    {
        start = s->next;
    }
    else
    {
        while (s != NULL)
        {
            s = s->next;
            counter++;
        }
        if (pos > 0 && pos <= counter)
        {
            s = start;
            for (i = 1;i < pos;i++)
            {
                ptr = s;
                s = s->next;
            }
            ptr->next = s->next;
        }
        else
        {
            cout<<"Position out of range"<<endl;
        }
        free(s);
        cout<<"Element Deleted"<<endl;
    }
}

[seeplus]

Something like this (NOT tried) :

void single_llist::delete_pos()
{
	if (start == nullptr) {
		cout << "List is empty\n";
		return;
	}

	int pos {}, i {}, counter {};

	cout << "Enter the position of value to be deleted: ";
	cin >> pos;

	node* s {start};

	if (pos == 1) {
		start = s->next;
		delete s;
		return;
	}

	node* ptr {nullptr};

	while (s != nullptr && counter < pos) {
		ptr = s;
		s = s->next;
		++counter;
	}

	if (s != nullptr) {
		ptr->next = s->next;
		delete s;
	} else
		cout << "Position out of range\n";
}

Note that in C++, use delete and not free - as new should be used and not malloc/calloc. Also use nullptr and not NULL.

[dhayden]

Sometimes your code calls free(s) when you haven't freed anything. Sometimes you free something and don't call free(s). Otherwise the code looks like it works to me.

Why are you freeing items in a linked list by position? This is extremely inefficient. If you need to free the n'th item of a collection then you shouldn't use a linked list.


You can simplify the code by using a pointer to the pointer to the current node:

void
single_llist::delete_pos()
{
    int pos;
    cout << "Enter the position of value to be deleted: ";
    cin >> pos;

    struct node **pp, *p;
    // pp points to start, or a node's "next" pointer
    for (pp = &start; (p = *pp) && pos > 1; pp = &p->next) {
        --pos;
    }
    if (pos < 1) {
        cout << "Position out of range\n";
    } else {
        *pp = p->next;
        free(p);
        cout << "Element deleted\n";
    }
}

What if someone using your class knew which element they wanted to delete, so they didn't need to prompt for it? What if they didn't want the message to print to cout? What if they wanted to read the element from another stream? For all of these reasons, it's usually a good idea to separate the user interface from the functionality:

// Delete the element at pos. Return true on success, false if
// pos is out of range.
bool
single_llist::delete_pos(int pos)
{
    struct node **pp, *p;
    // pp points to start, or a node's "next" pointer
    for (pp = &start; (p = *pp) && pos > 1; pp = &p->next) {
        --pos;
    }
    if (pos < 1) {
        return false;
    }

    *pp = p->next;
    free(p);
    return true;
}

int main()
{
    single_llist myList;

    int pos;
    cout << "Enter the position of value to be deleted: ";
    cin >> pos;
    if (myList.delete_pos(pos)) {
        cout << "Element deleted\n";
    } else {
        cout << "Position out of range\n";
    }
}