C++ Error C2451 conditional String is illegal

Question

i'm fairly new to C++. I want to write a Program/Function that checks a string input (from console or other source, not important here) if is already in an array. If it's not then it should be written into array. Otherwise do nothing. My Problem is the for loop and if the if condition. What am I missing?

#include <iostream>
#include <string>
#include <fstream>
#include <stdio.h>

using namespace std;

typedef struct {
   string id[10];
   string foo1[10];
   string type[10];
   string func[10];
}Device;

int main() {
   Device fooDevice;
   string mystring;
   int i = 0;

   mystring = "foo";

   ofstream temp;
   temp.open("temp.txt", ios::out | ios::app);


   for (fooDevice.id[i]; fooDevice.id[9]; i++) {
      if (fooDevice.id[i] != mystring) {
        fooDevice.id[i] = mystring;
        temp << mystring << endl;
      } else {
        //do nothing
      }
   }
return 0;
}

The first part in for loop is initialization. `fooDevice.id[i];` has no effect. — P0W, Jan 27 '17 at 08:41
Read more about the form of a for loop in your favourite book. — molbdnilo, Jan 27 '17 at 08:42
In addition to the broken for loop initialisation and condition, the code inside the loop is broken as well. Currently, you set all fields in your array to `mystring`. If you only want to add it if it is not in the array already, you have to go through the complete array first before deciding whether to add it. — Maximilian Köstler, Jan 27 '17 at 08:46
Yeah I noticed that in my textfile output. For me as a Beginner the textfile is to help me understand what is stored in the Array. — renton33, Jan 27 '17 at 08:57

score 2 · Accepted Answer · edited May 23 '17 at 11:53

The problem is the structure of your for loop. I'm not sure what you think your condition means, but here's how it should look:

for (std::size_t i = 0; i < 10; ++i) {

This increments the index value i from 0 to 9 (inclusive). You can then check the value of fooDevice[i].

At the moment, it seems you are trying to overwrite every element of the array with the new string. I'm not sure how you know how full the array is at any given time. Let's suppose you stop when you get to the first empty string:

for (std::size_t i = 0; i < 10; ++i) {
    if (myString == fooDevice.id[i]) {
        // already there, stop looping
        break;
    }
    else if (fooDevice.id[i].empty()) {
        // none of the currently set elements matches
        fooDevice.id[i] = myString;
        temp << myString << '\n';
        break;
    }
}

You could also replace this with a range-based-for:

for (auto& deviceId: fooDevice.id) {
    if (myString == deviceId) {
        // already there, stop looping
        break;
    }
    else if (deviceId.empty()) {
        // none of the currently set elements matches
        deviceId = myString;
        temp << myString << '\n';
        break;
    }
}

Better yet, use a std::vector with std::find from the <algorithm> header. (Warning: untested code):

struct Device {
    // ...
    std::vector<std::string> id;
    // ...
};

// ...

auto foundId = std::find(fooDevice.id.begin(), fooDevice.id.end(), myString);
if (fooDevice.id.end() == foundId) {
    // not already there
    fooDevice.id.push_back(myString);
    temp << myString << '\n';
}

It seems you are a bit confused about the difference between C and C++ as well:

The C++ version of <stdio.h> is <cstdio>, but you don't need it here at all (and you usually don't in a C++ program).
You don't need to typedef the name of a struct in C++, just do struct Device { ... };

And regarding C++ style, please reconsider your use of what are often considered bad practices: using namespace std; and endl.

Thanks for the help. I understand where my problems were. The includes were leftovers from some other Test code I did. — renton33, Jan 27 '17 at 08:50
@BoBTFish: your loops are not handling the case where a blank deviceId is possibly encountered before the searched ID is found if it actually exists in the array. You would need two loops, one to look for existence, and one to look for an empty slot if adding a new device. — Remy Lebeau, Jan 27 '17 at 09:19
@RemyLebeau Given that original array was fixed size, but could be considered to be only partially full, I was forced to assume that blank entries only existed in "empty" slots at the back of the array. I did mention that, but maybe not very clearly. — BoBTFish, Jan 27 '17 at 10:44

score 0 · Answer 2 · answered Jan 27 '17 at 08:41

0

for (fooDevice.id[i]; fooDevice.id[9]; i++) seems no scence. I think you want:

for(; i<9; ++i)

answered Jan 27 '17 at 08:41

Humam Helfawi

19,566
15
85
160

Remy Lebeau · Answer 3 · 2017-01-27T09:23:42.410

You can't use a string value as a loop condition. Only expressions that evaluate to booleans.

Also, you are also not initializing the device arrays before searching them.

Try something more like this:

#include <iostream>
#include <string>
#include <fstream>

using namespace std;

struct Device {
   string id[10];
   string foo1[10];
   string type[10];
   string func[10];
};

int main() {
   Device fooDevice;
   string mystring;

   // fill fooDevice as needed...

   mystring = "foo";

   ofstream temp;
   temp.open("temp.txt", ios::out | ios::app);    

   bool found = false;
   int idx_available = -1;

   for (int i = 0; i < 10; ++i) {
      if (fooDevice.id[i] == mystring) {
         //do nothing
         found = true;
         break;
      }

      if ((idx_available == -1) && fooDevice.id[i].empty())
         idx_available = i;
   }

   if ((!found) && (idx_available != -1)) {
        fooDevice.id[idx_available] = mystring;
        temp << mystring << endl;
   }

   return 0;
}

Which would be better handled with some rewriting:

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

struct Device {
   std::string id;
   std::string foo1;
   std::string type;
   std::string func;
};

struct isDeviceId {
   const std::string &m_id;
   isDeviceId(const std::string &id) : m_id(id) {
   }
   bool operator()(const Device &device) {
      return (device.id == m_id);
   }
};

int main() {
   std::vector<Device> devices;
   std::string mystring;

   // fill devices as needed...

   mystring = "foo";

   if (std::find_if(devices.begin(), devices.end(), isDeviceId(mystring)) == devices.end()) {
      Device device;
      device.id = mystring;
      devices.push_back(device);

      std::ofstream temp;
      temp.open("temp.txt", std::ios::out | std::ios::app);    
      temp << mystring << std::endl;
   }

   return 0;
}

Alternatively, in C++11 and later:

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

struct Device {
   std::string id;
   std::string foo1;
   std::string type;
   std::string func;
};

int main() {
   std::vector<Device> devices;
   std::string mystring;

   // fill devices as needed...

   mystring = "foo";

   if (std::find_if(devices.begin(), devices.end(),
      [mystring&](const Device &device) { return (device.id == mystring); }
    ) == devices.end()) {
      devices.emplace_back();
      devices.back().id = mystring;

      // or, in C++17:
      // devices.emplace_back().id = mystring;

      std::ofstream temp;
      temp.open("temp.txt", std::ios::out | std::ios::app);    
      temp << mystring << std::endl;
   }

   return 0;
}

C++ Error C2451 conditional String is illegal

3 Answers3