Open text file and read data to an arrayfire array using Qt and C++ I'm currently writing a C++ project with a Qt GUI and would like to read numerical data from user-selected text files through QtFileDialog::getOpenFileName. There are quite a few methods around which use std::vector, but I will be using the arrayfire library and it generally seems to be a bit more convenient to either use C-style arrays instead of std::vector for storing the data in host memory, or to send the data to an arrayfire array directly. This is what I've done here.

The idea is to pull the data into arrayfire arrays that are part of a struct (dataStruct) which is defined as follows:

struct dataStruct{
    af::array d1, d2, d3;
};


To get the filename from the user I call this in the MainWindow instance after a signal from a pushButton in the GUI:

dataStruct dS;
QString tmp =
    QFileDialog::getOpenFileName(this, tr("Open data file"), "C:\\",
                                 tr("Text Files (*.txt)"));
std::string filename = tmp.toStdString();
loadData(filename, dS);


loadData is the function which extracts the data from the text file, and to do so needs to count the number of rows of data. This is done using getNumberOfLines below:

void getNumberOfLines(std::string filename, int &amp;numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}


And finally, loading the data into the struct arrayfire arrays:

void loadData(std::string filename, structType &amp;dS)
{
    int numLines = 0;
    getNumberOfLines(filename, numLines);

    std::ifstream in(filename);
    std::string line;
    int i = 0;

    af::array data1 = af::constant(0, numLines, f32);
    af::array data2 = af::constant(0, numLines, f32);
    af::array data3 = af::constant(0, numLines, f32);

    while (std::getline(in, line))
    {
        std::istringstream is(line);
        std::string 1_tmp, 2_tmp, 3_tmp;

        is &gt;&gt; 1_tmp &gt;&gt; 2_tmp &gt;&gt; 3_tmp;
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    dS.d1 = data1;
    dS.d2 = data2;
    dS.d3 = data3;
}


I'm not so happy with getNumberOfLines and loadData because I end up reading the file data twice: First to get the number of rows/lines, the second to extract the data. I think it would be much better if this process could be reduced to a single step somehow.

One thing that I would like to implement is a function similar to getNumberOfLines or extension to loadData that allows the user to select data files with an arbitrary number of columns too. This could involve assigning all data to a single arrayfire array with dimensions given by (numRows, numCols, 1, 1) or similar, and then to split this array by columns later on to get the relevant data. Not sure how to implement this yet, so ideas are welcome!

If there are improvements to the code that other people can spot I'd really appreciate it if they could let me know. This will make up a small part of my first "big" project in C++, so please flag up any screamers that I've adopted when handling data or structuring the code.
Prefer return over Output parameter

A result as an out parameter seems like a smell to me:

void getNumberOfLines(std::string filename, int &amp;numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}


Why not return the number of lines as a result of the function?

Also when counting the lines you don't actually need to read the strings out. You just need to find the end of line marker. To avoid the cost of copying and re-sizing lines for data you don't need, you can use the std::istream::ignore() method.

Lets also pass the filename by reference to avoid a copy (copying strings is relatively expensive (its const as you don't modify it)).

std::size_t getNumberOfLines(std::string const&amp; filename)
{
    std::size_t   result = 0;
    std::ifstream in(filename);

    while(in.ignore(std::numeric_limits&lt;streamsize&gt;::max(), '\n') {
        ++result;
    }
    return result;
}


You can now change the call point to:

// From
int numLines = 0;
getNumberOfLines(filename, numLines);

// To
int numLines = getNumberOfLines(filename);


Illegal identifiers:

    std::string 1_tmp, 2_tmp, 3_tmp;


Those are not legal variable names. Not sure how you think this is compiling!

Check stream reads/writes

You should always check the result of a stream input/output operation to make sure it worked. Streams are the one place in the standard where exceptions are turned off so you need to check the error codes (luckily this is easy as the stream used in a boolean context will convert itself into the current error code state).

    // Here you read value from a stream
    // But don't check that it worked.
    is &gt;&gt; 1_tmp &gt;&gt; 2_tmp &gt;&gt; 3_tmp;
    data1(i) = std::stof(1_tmp) * pow(10, -9);
    data2(i) = std::stof(2_tmp);
    data3(i) = std::stof(3_tmp);
    i++;


Should look like this:

    if (is &gt;&gt; 1_tmp &gt;&gt; 2_tmp &gt;&gt; 3_tmp)
    {
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    else
    {
        // If something went wrong with one line.
        // Then do you trust the rest of the stream?
        // I think not. So you should also mark the input stream
        // bad at this point.
        in.setstate(std::ios::failbit);

        // This will cause the outer loop reading the file to stop.
    }


Use the stream read ability

Why read a string when the data is a float?

Why go to the effort of parsing a string when the data could be a float? In your code you are effectively scanning each value twice, which seems like a lot of extra work.

 std::string 1_tmp;
 in &gt;&gt; 1_tmp;
 double 1_val = std::stof(1_tmp);


Instead, use the existing read float capabilities of the stream input operator.

 double 1_val;
 int &gt;&gt; 1_val;


Copy ss Move

This is a copy:

dS.d1 = data1;
dS.d2 = data2;
dS.d3 = data3;


But you never use data[123] again. So you may as well try and move the data.

dS.d1 = std::move(data1);
dS.d2 = std::move(data2);
dS.d3 = std::move(data3);