Delete an element in an array list

[jEb91hU5]

I am attempting to delete an element in an array version of a list. When I use the function, it deletes the element, but only outputs one more element right after in the list. It should output 3 more elements post deletion instead. Is there something I'm not quite getting?

 void deletelist(listtype list[], int& numlist, int id, ofstream& outf)
{
	int i, where;
	where = 0;

	while (where < numlist && id > list[where].id) where++;

	if (id == id)
	{
		while (where < numlist - 1)
		{
			list[where] = list[where + 1];
			numlist--;
		}
	}
	else
	{
		outf << "The item doesn't appear to be in the list" << endl;
	}
}

[dutch]

Well, this is pretty wacky:

if (id == id)

And you probably only want to subtract 1 from numlist.

Also, it would be better to return a bool indicating whether or not the element was found so that the caller can decide if it wants to print a message or not.

[jEb91hU5]

I had it before:

if (list[where].id == id)

and it did the same thing.

[salem c]

> numlist--;
Do this once only, after you've shuffled all the elements down one place.

[Thomas Huxhorn]

This is solved by std::vector and std::remove_if.
Example use erase-remove idiom https://en.wikipedia.org/wiki/Erase-remove_idiom

#include <vector>
#include <algorithm>

struct listtype {
    int id;
};

void deletelist(std::vector<listtype>& list, int id, std::ostream& outf) {
    auto res = std::remove_if(list.begin(), list.end(), [&id](const listtype& x){ return x.id == id; });
    if(res == list.end()) {
        outf << "The item " << id << " doesn't appear to be in the list\n";
    } else {
        list.erase(res, list.end());
    }
}

int main() {
    std::vector<listtype> list;
    list.push_back(listtype{1});
    list.push_back(listtype{10});
    list.push_back(listtype{-2});

    deletelist(list, 10, std::cout);
    deletelist(list, 3, std::cout);
    assert(list.size() == 2);
}

[ne555]

> I had it before:
> if (list[where].id == id)
> and it did the same thing.
well, id == id wasn't working either, ¿why did you keep it?
https://en.wikipedia.org/wiki/Voodoo_programming

[lastchance]

@JLaw21,
Why don't you post some compileable code; we can't see your definition and implementation of class listtype.

Given the entity in the standard library, list isn't a great name for a variable. Actually, I thought the whole idea of (linked) lists was that you didn't have to shift lots of elements just to delete or insert something.

(Love @ness's link though! Book-marked in the same folder as "XY problem". Have to remember "cargo cult programming" as well.)

dutch wrote:

And you probably only want to subtract 1 from numlist.

I think @dutch (and @salem c) have it right - by putting the numlist decrement in that while loop you are subtracting something from numlist every time you move an element (which you will have to do a lot), rather than just once for the element removed.

But switch to a linked list anyway.

[jonnin]

if the list is not sorted, just swap with the end.

[lastchance]

Given the line
while (where < numlist && id > list[where].id) where++;
I think the implication is that this container is sorted.

[dhayden]

Just to be clear, you have two bugs.

The first bug is if (id == id). You tried changing it to if (list[where].id == id) but that didn't work either. The problem here is that there are three possible situations when you exit the loop:
1. id is greater than all id's in the array and you ran off the end of the array. You need to check this case first because otherwise where might be invalid.
2. you found id
3. You didn't find id.

The second bug is decrementing numlist inside the loop. You need to move that outside the loop, but now your loop will never exit because where and numlist don't change. This should make you realize that you need to change where inside the loop. Otherwise you just keep copying the same item to the same location.

[jEb91hU5]

So this is how I've changed it so far. But I'm not sure how you mean to change where inside the loop.

if (where < numlist && list[where].id == id)
	{
		while (where < numlist - 1)
		{
			list[where] = list[where + 1];
		}
		numlist--;
	}

[lastchance]

Use it (line 5), then change it by adding 1 (so that you move to the next element) between your current lines 5 and 6. (Actually a for loop might be more natural than a while loop here: you know exactly how many times you are going to run the operation.)

You still aren't providing complete code. (Actually you are providing less and less.)

[jEb91hU5]

I figured it out. The for loop was a better option. Thanks for the help!