7
\$\begingroup\$

I've just posted this solution in my review of @adrienlucca's question, which is to read three rows of six tab-separated doubles.

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

struct Sexdoublet {
    double x, y, d, m, c, t;

    friend std::istream &operator>>(std::istream &in, Sexdoublet &r) {
        return in >> r.x >> r.y >> r.d >> r.m >> r.c >> r.t;
    }

    friend std::ostream &operator<<(std::ostream &out, const Sexdoublet &r) {
        return out << "[ x = " << r.x
                   << ", y = " << r.y
                   << ", d = " << r.d
                   << ", m = " << r.m
                   << ", c = " << r.c
                   << ", t = " << r.t << " ]";
    }
};

std::vector<Sexdoublet> import(std::istream &in) {
    std::vector<Sexdoublet> data;
    Sexdoublet s;
    do {
        in >> s;
    } while (in && (data.push_back(s), true));
    return data;
}

int main(int argc, char *argv[]) {
    // If the first command-line argument is not "-", treat it as the filename
    // from which to read the input.  Otherwise, read from STDIN.
    const char *filename = (argc >= 2 && 0 != strcmp("-", argv[1])) ?
                                argv[1] : NULL;
    std::ifstream f;
    std::istream &in = filename ? (f.open(filename), f) : std::cin;
    if (!f) {
        std::cerr << "Error opening " << filename << ": "
                  << strerror(errno) << std::endl;
        return 1;
    }

    std::vector<Sexdoublet> data = import(in);
    std::for_each(data.begin(), data.end(), [](const Sexdoublet &s) {
        std::cout << s << std::endl;
    });
    return 0;
}

I ended up using the comma operator twice — both times as a "workaround" for the fact that a function I call returns void:

  • (data.push_back(s), true) — I have to check whether the istream extractions succeeded before adding an element to the result.
  • (f.open(filename), f) — I like the ternary operator because it expresses the fact that in is going to be assigned to one value or another. Also, the in reference has to be initialized immediately and irrevocably, right?

It is my impression that the comma operator is rarely used in C and C++. So, I'd like to ask, are these reasonable uses of the comma operator? Or too clever? Or too dumb, because I've missed an entirely better solution?

\$\endgroup\$
  • \$\begingroup\$ If you primarily need to check for a successful in when looping, do you still need to check the push_back()? I'll admit I've never seen the latter being used like that before. push_back() automatically reallocates, so it'll throw bad_alloc if it fails. \$\endgroup\$ – Jamal Nov 14 '13 at 8:52
  • \$\begingroup\$ Try feeding it a partial line ending with EOF and you'll see. If in stops working prematurely, then s will contain some fields whose values came from the previous iteration. \$\endgroup\$ – 200_success Nov 14 '13 at 9:01
4
\$\begingroup\$

the first use can be replaced with:

while(in >> s) {
    data.push_back(s)
}

the second case is much more readable with a simple if else:

std::ifstream f;
if(argc >= 2 && 0 != strcmp("-", argv[1])) {
    f.open(argv[1]);
    if (!f) {
        std::cerr << "Error opening " << argv[1] << ": "
                  << strerror(errno) << std::endl;
        return 1;
    }

}
std::istream &in = f.is_open() ? f : std::cin;
\$\endgroup\$
  • \$\begingroup\$ I don't object to the while-loop being wordier. However, the while (true) is a fiction — the structure of the loop remains a mystery until the reader finds the break. \$\endgroup\$ – 200_success Nov 14 '13 at 9:37
  • \$\begingroup\$ you can add in to the condition, and change the ternary to f.is_open() \$\endgroup\$ – ratchet freak Nov 14 '13 at 9:58
  • \$\begingroup\$ while (in) is arguably worse than while (true), since it's doing a superfluous test that appears to be useful but actually isn't. \$\endgroup\$ – 200_success Nov 14 '13 at 10:21
  • 2
    \$\begingroup\$ @200_success actually you could do while(in >> s){data.push_back(s);} (operator>> returns the stream) \$\endgroup\$ – ratchet freak Nov 14 '13 at 13:01
  • \$\begingroup\$ @200_success: See? You didn't need to check (data.push_back(s), true) after all. :-) \$\endgroup\$ – Jamal Nov 14 '13 at 15:40
4
\$\begingroup\$

Let's start with this:

std::vector<Sexdoublet> import(std::istream &in) {
    std::vector<Sexdoublet> data;
    Sexdoublet s;
    do {
        in >> s;
    } while (in && (data.push_back(s), true));
    return data;
}

As far as I can see, this appears to be equivalent to:

std::vector<Sexdoublet> import(std::istream &in) { 
    return std::vector<Sexdoublet> {std::istream_iterator<Sexdoublet>(in),
                                    std::istream_iterator<Sexdoublet()};
}

Given how simple this is, however, I'd question whether it's worth writing as a separate function at all. It's probably simpler to just initialize the target vector directly from the iterators and be done with it.

Your code to write the data out:

std::for_each(data.begin(), data.end(), [](const Sexdoublet &s) {
    std::cout << s << std::endl;
});

...doesn't seem like the way I'd do things either. I rarely find std::for_each useful, and this is no exception to that rule. In C++98/03, I'd use:

std::copy(data.begin(), data.end(), 
          std::ostream_iterator<Sexdoublet>(std::cout, "\n"));

In C++11, I'd generally prefer a range-based for loop:

for (auto const &s : data)
    std::cout << s << "\n";

That ignores the larger situation though: we end up simply reading the data into the vector, then writing it back out from the vector to standard output. That being the case, we might as well skip the vector and copy directly from input to output:

std::copy(std::istream_iterator<Sexdoublet>(in),
          std::istream_iterator<Sexdoublet>(),
          std::ostream_iterator<Sexdoublet>(std::cout, "\n"));

Note that using this eliminates the import function entirely. In the end, I think the copying really belongs in a separate function, so main basically just deals with command line arguments and error handling, something on this order:

bool copy(std::istream &is, std::ostream &os) {
    if (!is) 
        return false;
    std::copy(std::istream_iterator<Sexdoublet>(is),
        std::istream_iterator<Sexdoublet>(),
        std::ostream_iterator<Sexdoublet>(os, "\n"));
    return true;
}

int main(int argc, char **argv){
    std::vector<std::string> args(argv, argv+argc);
    if (args.size() > 1 && args[1] == "-")
        copy(std::cin, std::cout);
    else if (!copy(std::ifstream(args[1]), std::cout)) 
        std::cerr << "Error opening " << args[1]
                  << ": " << strerror(errno) << "\n";
}
\$\endgroup\$
  • \$\begingroup\$ Actually, the goal is to import the data into a vector; printing the vector was done as an example to prove that it worked. Anyway, thanks for the iteration tips. \$\endgroup\$ – 200_success Dec 5 '13 at 1:08
  • \$\begingroup\$ @200_success: I thought that might be the case, which is why I still included the code to do that. \$\endgroup\$ – Jerry Coffin Dec 5 '13 at 1:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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