Where are you getting this code from? It looks like it's very poorly written.
Jeez, what's the point of this. If you want the loop to quit when the element is found, then either use break or else put found as part of the loop condition. As it is, it will almost guaranteed waste tons of time looping unnecessarily after an element has been found.
Code:
mp_List[i] = mp_List[i+1];
You are correct in your analysis of what happens. I believe the intent of this code was really to shift all following elements forward one position (so as not to leave a 'gap' when one element is erased), but this only shifts one element forward. This should be a loop:
Code:
for(; i < m_Count - 1; ++i) //Technically shouldn't have m_Count - 1 as loop condition, should be pre-calculated for max efficiency.
{
mp_List[i] = mp_List[i + 1];
}
Now, for:
Code:
if (found)
m_Count--;
This separate test is completely unnecessary. It could be placed inside the loop, where found is set to true.
Also, since these are pointers, it makes me suspect (although you'll have to verify) that they point to dynamic memory; if they do, then simply erasing the pointers will give you a memory leak. You'd need to delete each pointer manually before doing the shuffling forward of all following elements.
So, a much improved version of the code would be:
Code:
void Subject::RemoveObserver(Observer* Item)
{
for (int i = 0; i < m_Count; ++i)
{
if (mp_List[i] == Item)
{
//Decrement right now, so we don't have to do it later.
//Also, so we don't have (m_Count - 1) as a loop condition.
--m_Count;
//Keep shifting the next element forward, until the (new) end.
//Uncomment this line if it's dynamic memory that you're responsible for freeing
//delete mp_List[i];
for(; i < m_Count; ++i)
mp_List[i] = mp_List[i+1];
}
}
}
Hope this helps!