1
\$\begingroup\$

I'm trying to read a text file formatted as in this example:

5 3
3 1 5
6 6
6 1 3 2 5 4

Where the lines are coupled, with the line above always being of length equal to 2 and the line below having length equal to the the last digit in the line above.

(Unfortunately this is from an old note I took and I don't remember what those numbers are actually supposed to mean, so I'm limiting myself to store them in a multidimensional vector form where they can be easily recovered).

In the beginning I was tring to read the digit which represents the length below to execute a loop of finite length, but in the end this is the code I wrote:

#include <fstream>
#include <sstream>
#include <string>
#include <vector>
int main()
{
    std::string filname="data/file01.txt";
    std::ifstream file01(filname);
    std::vector<std::vector<int>> v;
    std::string line;
    while(getline(file01,line))
    {
        std::stringstream ls;
        ls<<line;
        int c;
        std::vector<int> t;
        while(!ls.eof())
        {
            ls>>c;
            t.push_back(c);
        }
        v.push_back(t);
    }
    return 0;
}

This gives me a 2-dimensional vector that can be easily accessed, but I don't like having to convert both the file and each string to a stream and I don't like using nested vectors.

Is there a way to achieve this goal with more simple and elegant code (even if it means to make the code more specific)?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ What would be the use case of your 2-dimensional vector? Because you can't just ask for nested_vector[x][y] unless you know nested_vector[x] has at least y elements -and this information is contained in line y-1. That means you could as well store lines by pair and elide the first line's last number since it is pair_of_lines.size() - 1. Actually, if the numbers are all one-digit long, you don't need to convert the chars to integers, and you can store the pairs of lines directly as strings. You could even simply store the whole file as a long string ... (1/2) \$\endgroup\$
    – papagaga
    Dec 17, 2018 at 14:22
  • \$\begingroup\$ ... and a vector of iterators / positions denoting the end of a pair of lines. It might allow you to perform faster I/O (reading the file in bulk rather than line by line), and only then parsing your lines from the copy in memory. (2/2) \$\endgroup\$
    – papagaga
    Dec 17, 2018 at 14:26

2 Answers 2

4
\$\begingroup\$

I see a number of things that may help you improve your program.

Decompose your program into functions

All of the logic here is in main in one chunk of code. It would be better to decompose this into separate functions.

Don't loop on eof()

It's almost always incorrect to loop on eof() while reading a file. The reason is that the eof indication is only set when an attempt is made to read something from the file when we're already at the end. In this case, it means we have already executed the line ls >> c; and then called t.push_back(c);. The problem is that if we're at the end of the file and we attempt to read one more c value, it will append an unwanted value to the end of the vector. See this question for more details on why using eof is usually wrong.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Allow the user to specify input file name

The file name is currently hardcoded which certainly greatly restricts the usefulness of the program. Consider using argc and argv to allow the user to specify file names on the command line.

Use standard algorithms

An alternative is to use std::copy to read in a line of integers. Here's one way to do that:

while(getline(in, line))
{
    std::stringstream ls{line};
    std::vector<int> vec;
    std::copy(std::istream_iterator<int>(ls), 
              std::istream_iterator<int>(), 
              std::back_inserter(vec)
             );
    v.push_back(vec);
}

Use a custom object

An alternative would be to use your own class, LinePair and create a single-dimensional vector of those. Here's how the LinePair object might be defined:

#include <iostream>
#include <iterator>
#include <fstream>
#include <vector>

class LinePair {
public:
    friend std::ostream& operator<<(std::ostream& out, const LinePair& lp) {
        constexpr char sep[]{" "};
        auto it{lp.data.begin()};
        out << *it++ << sep << lp.data.size() - 1 << '\n';
        std::copy(it, lp.data.end(), 
                  std::ostream_iterator<int>(out, sep));
        return out << '\n';
    }
    friend std::istream& operator>>(std::istream& in, LinePair& lp) {
        int a, len;
        in >> a >> len;
        if (in && len) {
            lp.data.clear();
            lp.data.reserve(len+1);
            lp.data.push_back(a);
            for ( ; len && in >> a; --len) {
                lp.data.push_back(a);
            }
        }
        return in;
    }
private:
    std::vector<int> data;
};

Now main is quite simple:

int main(int argc, char *argv[])
{
    if (argc != 2) {
        std::cerr << "Usage: readlines filename\n";
        return 1;
    }
    std::ifstream in{argv[1]};
    std::vector<LinePair> v;
    std::copy(std::istream_iterator<LinePair>(in), 
              std::istream_iterator<LinePair>(),
              std::back_inserter(v)
             );

    // now echo it back out
    std::copy(v.begin(), v.end(), 
              std::ostream_iterator<LinePair>(std::cout));
}
\$\endgroup\$
5
\$\begingroup\$

You're turning the whole input file into a vector of vectors. That's fine, but you state yourself that your input has structure. It would be nice to have that structure reflected in your program:

  1. since the lines are couple in pairs I'd like to see a code block that clearly processes a pair of lines
  2. you should then eliminate the redundancy that the second number of the 1 st line equals the number of items on the second.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Good point! I've added such an alternative to my answer. \$\endgroup\$
    – Edward
    Dec 17, 2018 at 18:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.