Why is getline() not working in my code (c++)?

Question

I have class 'message' with, among others, members:

protected:
      int * next;
      int amount;
      std::string ** mes;
public:
      message(std::ifstream*);

and the code for constructor is:

message :: message(ifstream * myfile){
    *myfile >> amount;
    if (amount==0){
                next = new int[1];
                *myfile >> next[0];
                mes = new string*[1];
                getline(*myfile,*mes[0]);
                }
    else{
                next = new int[amount];
                mes = new string*[amount];
                for (int i=0;i<amount;i++){
                    *myfile >> next[i];
                    getline(*myfile,*mes[i]);
                    }
                }
    }

Reading from file using operator >> works just fine, but program crashes on getline() - why? What should I change?

Any reason *not* to use a `std::vector`, instead of doing all that manual, error-prone dynamic allocation? — Tony Delroy, Mar 08 '16 at 00:33
You may suffer from this issue after fixing the crash: [c++ - Why does std::getline() skip input after a formatted extraction? - Stack Overflow](http://stackoverflow.com/questions/21567291/why-does-stdgetline-skip-input-after-a-formatted-extraction) — MikeCAT, Mar 08 '16 at 00:49
Pass streams by reference not by pointers. This reduces the injection of defects cause by pointers pointing to invalid locations. — Thomas Matthews, Mar 08 '16 at 04:34

Remy Lebeau · Answer 1 · 2016-03-08T02:44:44.723

You are not allocating any memory for the std::string variables that you are asking std::getline() to read into. You are allocating arrays of string* pointers that do not point at anything. You need to change your code to either:

keep using an array of pointers, but allocate the actual string variables for them:

std::string ** mes;

if (amount == 0) {
    ...
    mes = new string*[1];
    mes[0] = new string; // <-- here
    std::getline(*myfile, *mes[0]);
}
else {
    ...
    mes = new string*[amount];
    for (int i = 0; i < amount; ++i) {
        mes = new string[amount]; // <-- here
    }
    for (int i = 0; i < amount; ++i) {
        ...
        std::getline(*myfile, *mes[i]);
    }
}

remove the unnecessary level of indirection to begin with:

std::string * mes;

if (amount == 0) {
    ...
    mes = new string[1];
    std::getline(*myfile, mes[0]);
}
else{
    ...
    mes = new string[amount];
    for (int i = 0; i < amount; ++i) {
        ...
        std::getline(*myfile, mes[i]);
    }
}

Wit that said, you should stop using raw arrays to begin with, and instead use std::vector:

#include <vector>

protected:
    std::vector<int> next;
    std::vector<std::string> mes;
    int amount;

message :: message(ifstream * myfile) {
    *myfile >> amount;
    if (amount == 0) {
        next.resize(1);
        mes.resize(1);
        *myfile >> next[0];
        std::getline(*myfile, mes[0]);
    }
    else {
        next.resize(amount);
        mes.resize(amount);
        for (int i = 0; i < amount; ++i) {
            *myfile >> next[i];
            std::getline(*myfile, mes[i]);
        }
    }
}

Either way, you should consider getting rid of the redundant code for the amount == 0 case. Use a local variable and set it to 1 if amount is 0, otherwise set it to the actual amount, and then you can use a single code line to perform the allocations regardless of the amount value:

message :: message(ifstream * myfile) {
    *myfile >> amount;
    int numElements = (amount == 0) ? 1 : amount;
    next = new int[numElements];
    mes = new string[numElements];
    for (int i = 0; i < numElements; ++i) {
        *myfile >> next[i];
        getline(*myfile, mes[i]);
    }
}

message :: message(ifstream * myfile) {
    *myfile >> amount;
    int numElements = (amount == 0) ? 1 : amount;
    next.reserve(numElements);
    mes.reserve(numElements);
    for (int i = 0; i < numElements; ++i) {
        int value;
        *myfile >> value;
        next.push_back(value);
        std::string line;
        std::getline(*myfile, line);
        mes.push_back(line);
    }
}

First of all, thank you. I went with solution 2. because, as I realised, the indirection was indeed not needed at this point. I'm generally passing bigger objects in this program all the time so I tried using arrays of pointers for effinciency, but just for few strings that was not necessary. I'm trying to use my own classes with array members partially to learn. On my uni on the first year they are basically forbidding you from using std::vector, so you can understand how arrays work. After that you can use std::vector. — user3019593, Mar 08 '16 at 01:24
Also, amount==0 case is absolutely not redundant (though it's not so much visible in the constructor). Instead of adding another boolean variable, amount being zero indicates a certain state: a message with amount 0 and a message with amount 1 will be handled very differently, even when they have the same actual amount of strings in array. Specifically, imagine a dialog in an old game like Baldur's Gate or The Longest Journey. Amount ==0 means simple message that will lead to next one on any click, and amount>0 prompts user to pick a response (even when there's only one). — user3019593, Mar 08 '16 at 01:31
Even if the rest of the class treats `amount == 0` differently, for purposes of the constructor allocating the arrays, there is no difference. I updated my example to reflect this using a temp variable so `amount` will stay 0. — Remy Lebeau, Mar 08 '16 at 02:45

Why is getline() not working in my code (c++)?

1 Answers1