For loop not iterating through array

[dorito200]

Hey yall, got another question for everyone. I'm working on an assignment for class and I am trying to delete objects in some arrays (what the assignment is based on). Now the rest of the code is complete, but when it comes to my delete function something is going wrong. In all the other functions this way works, which is comparing cityName[i] to nHolder (the name the user entered). But when the for loop starts cityName[i] only stays equivalent to its cityName[0] which makes the loop not work. Maybe there's a huge glaring mistake staring me in the face, but I don't think there is since this is almost the exact same loop I use for most of the other functions. It isn't the cleanest but it's still a work in progress, If anyone has any questions or recommendations Id be happy to hear them all, code is posted below.

Arrays are set up like this.

//Arrays
string cityName[100] = {""};
float x[100] = {0};
float y[100] = {0};

void deleteCityA(string *cityName, float *x, float *y)
{
    string nHolder;
    int iHolder, j, found = 0;

    cin.ignore();
    cout << "Please enter the name of the city you'd like to delete: ";
    getline(cin, nHolder);

    //cout << nHolder << endl;

    //the if and for loop are causing the problems
    for(auto i = 0; i < 100; ++i)
    {
      if((cityName[i]) == nHolder)
      {
        for(j = i; j < cityName->back()-1; ++j)
        {
          cout << "deleting";
          cityName[j] = cityName[j+1];
          x[j] = x[j+1];
          y[j] = y[j+1];
          found++;
          --i;
        }
      }
    } 
    if(found == 0)
    {cout << "Element not found";}
    else{cout << "Element deleted";}   

  return ;  
}

[Peter87]

This doesn't make a whole lot of sense j < cityName->back()-1.

Did you mean j < cityName->size()-1 ?

[dorito200]

back() and size() do almost the same thing, they work interchangeably in this program, but that's not where the problem lies.

[Peter87]

On second thought...

If cityName points the first string in the array
then cityName->back() will give you the last char of the first string in the array,
and cityName->size() will give you the size of the first string in the array.

I think you want neither.

What you want is probably the size of the array (i.e. 100).

[dorito200]

That's true, but that still doesn't solve my initial problem. I cant even get into that loop

[Peter87]

Have you made sure nHolder contains the right value and that it exists in the array? Pay attention to spaces and other whitespace characters, they are easy to miss.

[Peter87]

Looks like the --i; statement is misplaced. I know it doesn't matter for your "initial problem" but I just wanted to mention it for later.

[jonnin]

if it can't get into the inner loop, then the comparison is failing.
remember that this compare is exact, including case, spaces, etc.

fix the mentioned bugs, and add some debugging statements to tell you what is going on, eg cout << "comparing " << cityname[i] << " and " << nHolder << endl could go before the if statement.

** this is a simple debugging technique that is, for small problems where you can do it, easier than dealing with a step-by-step debug session.

[coder777]

I cant even get into that loop

If so you need to check what the content of cityName is. I suggest that you add for debugging an cout such as:

    for(auto i = 0; i < 100; ++i)
    {
      cout << i << ": " <<  cityName[i];
      if(cityName[i] == nHolder)
      {
...

It might not be properly filled.

That inner loop (line 17) doesn't make sense at all. It might be as simple as

    for(auto i = 0; i < 100; ++i)
    {
      if(cityName[i] == nHolder)
      {
          found++;
          cityName[i] = "";
      }
    } 

[Peter87]

That inner loop (line 17) doesn't make sense at all.

It obviously tries to shift all the elements after the element-to-be-removed one step to the "left", which will cause the element-to-be-removed to be overwritten. So instead of leaving "gaps" in the middle it puts empty elements at the end (there seems to be an assumption that at least the last element is empty).

[coder777]

Then I guess OP want something like this:

        found++;
        for(j = i + 1; j < 100; ++j)
        {
          cout << "deleting";
          cityName[j - 1] = cityName[j];
          x[j - 1] = x[j];
          y[j - 1] = y[j];
        }
        cityName[99] = "";
        --i;

Not tested.

[Peter87]

Yes.

[seeplus]

Consider:

#include <string>
#include <iostream>

constexpr size_t NOARR { 100 };

void deleteCityA(std::string* cityName, float* x, float* y) {
	std::string nHolder;
	size_t found {};

	//std::cin.ignore();	// NOT NEEDED FOR THIS TEST CODE
	std::cout << "Please enter the name of the city you'd like to delete: ";
	std::getline(std::cin, nHolder);

	for (size_t i {}; i < NOARR && !cityName[i].empty(); ++i)
		if ((cityName[i]) == nHolder) {
			auto j { i + 1 };

			std::cout << "deleting " << nHolder << '\n';
			++found;

			for (; j < NOARR && !cityName[j].empty(); ++j) {
				cityName[j - 1] = cityName[j];
				x[j - 1] = x[j];
				y[j - 1] = y[j];
			}

			cityName[j - 1].clear();
			x[j - 1] = 0;
			y[j - 1] = 0;
		}

	if (found == 0)
		std::cout << "Element not found\n";
	else
		std::cout << found << " element(s) deleted\n";
}

int main() {
	std::string cityName[NOARR]  { "qwet", "asdf", "zxcv", "asdf"};
	float x[NOARR] { 1, 2, 3 };
	float y[NOARR] { 2, 3, 4 };

	deleteCityA(cityName, x, y);

	for (size_t i {}; i < NOARR && !cityName[i].empty(); ++i)
		std::cout << cityName[i] << '\n';
}

Assuming the equality test for the entered string to exist is true at least once during iteration of cityName, then this will remove the found entries by shifting down the remaining ones and setting the now last entry to null/0 as appropriate. The code assumes that in cityName there are no empty names beyond the last name entry. Thus the iteration will stop when the maximum number of elements checked or an empty cityName is found.

Please enter the name of the city you'd like to delete: asdf
deleting asdf
deleting asdf
2 element(s) deleted
qwet
zxcv

Note that as mentioned above, the comparison test on L15 is for an exact comparion - including any spaces (including leading/trailing as well as within) and an exact case match. If cityNames can contain both lowercase and uppercase characters, then both these (and possibly also the entered string to find) first need to be converted to the same case (eg lowercase) before the comparison.

[seeplus]

Also note that it is much simpler if a struct and vector are used. For delete, consider for C++20:

#include <string>
#include <iostream>
#include <vector>
#include <algorithm>

struct Data {
	std::string cityName;
	double x {};
	double y {};
};

using VD_t = std::vector<Data>;

void deleteCityA(VD_t& vd) {
	std::string nHolder;

	//std::cin.ignore();	// NOT NEEDED FOR THIS TEST CODE
	std::cout << "Please enter the name of the city you'd like to delete: ";
	std::getline(std::cin, nHolder);

	if (const auto found { std::erase_if(vd, [&](const auto& s) {return s.cityName == nHolder; }) }; found == 0)
		std::cout << "Element not found\n";
	else
		std::cout << found << " element(s) deleted\n";
}

int main() {
	VD_t city { { "qwet", 1, 2}, {"asdf", 2, 3}, {"zxcv", 3, 4}, {"asdf", 4, 5 } };

	deleteCityA(city);

	for (const auto& [name, x, y] : city)
		std::cout << name << '\n';
}

[jonnin]

unrelated but good to know stuff:

a weakness of array type structures is deleting in the middle, for sure.
moving everything up is very expensive, and should be avoided if it can happen frequently: you can avoid this by using a different container design or by using the lazy delete concept (if it works for the problem at hand: not all scenarios allow this to work well). If deleting at random is a key function to what you need done, pick a different container.

in a lazy delete, you add a boolean to each location as to whether it is deleted or available. If deleted, you do not process it or print it etc when iterating the container, and when inserting, you can put the new item in the first deleted location.

if you can't use lazy delete, and still need to frequently delete in the middle, you can make it less painful by making the array all pointers, so you only move one integer when you shift the data to close the gap, much cheaper than moving fat class or struct around. Ideally you make one array of all the object locations and another of pointers (or the array index) to those, so it maintains the performance of an array across the board while allowing cheaper delete & shuffle.

The point of the exercise is probably to show you the inefficiency of using arrays this way, but its good to know, if nothing else, that there are options to make this less of an issue.

[Peter87]

jonnin wrote:

a weakness of array type structures is deleting in the middle, for sure.

If the order doesn't matter you can (move) assign the last element to the element that you want to remove and then decrease the "size".

[jonnin]

yes, another great alternative. And, if you noticed, all the alternatives struggle if the data is ordered. If the data being stored allows for it, you can also block copy (memcpy, etc) to shift.