8
\$\begingroup\$

My code generates a LaTeX table from a plain text file which uses whitespace as delimiters. It asks you the title of the columns one-by-one (mind the formatting!). Then it outputs a latex.txt file with a nicely formatted table inside.

How can I improve it?

Here's the code:

#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>

int main(int argc, char* argv[])
{
    if(argc!=2){
        std::cerr << "Error loading file." << std::endl;
        return 1;
    }
    int col = 0, row = 0;
    // First of all. Provide an input file split by whitespaces.
    std::ifstream fin(argv[1],std::ios::in);
    std::vector< std::vector<double> > matrix;
    while(!fin.eof()){
        std::vector<double> v;
        std::string line;
        getline(fin, line);
        std::istringstream nums(line);
        double x;
        while(nums >> x){
            v.push_back(x);
        }
        matrix.push_back(v);
        row++;
    }
    // Basic.
    row--;
    col = matrix[0].size();
    std::ofstream fout("latex.txt",std::ios::out);
    fout << "\\begin{center}\n\\begin{tabular}{";
    for(int i=0;i<col;i++){
        fout << "|c";
    }
    fout << "|}\n";
    fout << "\\hline\n";
    // Ask for col titles:
    for(int i=0;i<col;i++){
        std::cout << "What is the title of the " << i+1 <<  ". column?(Please mind the formatting!)" << std::endl;
        std::string title;
        getline(std::cin,title);
        fout << title;
        if(i!=col-1){
            fout << " & ";
        }else{
            fout << " \\\\\n\\hline\n";
        }
    }
    for(int i=0;i<row;i++){
        for(int j=0;j<col;j++){
            fout << matrix[i][j];
            if(j!=col-1){
                fout << " & ";
            }else{
                fout << " \\\\\n\\hline\n";
            }
        }
    }
    fout << "\\end{tabular}\n";
    fout << "\\end{center}";
    return 0;
}
\$\endgroup\$
4
\$\begingroup\$

Argument checking

You test that the user has specified exactly one input file, but if not, then print the misleading message

Error loading file.

You would be better with a usage message, such as

std::cerr << "Usage: " << argv[0] << " filename" << std::endl;

Well done for correctly using cerr for such messages.

File checking

Here, you use the supplied filename, but never check whether fin was successfully opened. You probably want:

std::ifstream fin(argv[1],std::ios::in);
if (!fin) {
    std::cerr << argv[1] << ": not readable" << std::endl;
    return 1;
}

Similarly for fout:

std::ofstream fout("latex.txt", std::ios::out);
if (!fin) {
    std::cerr << "latex.txt" << ": not writeable" << std::endl;
    return 1;
}

The reading loop also doesn't check for errors, and it has the while (!eof()) antipattern. This should be something more like (untested):

std::string line;
while (getline(fin, line)) {
    std::vector<double> v;
    std::istringstream nums(line);
    double x;
    while (nums >> x) {
        v.push_back(x);
    }
    if (!nums.eof()) {
        std::cerr << argv[1] << ": parse error" << std::endl;
    }
    matrix.push_back(v);
    row++;
}
if (!fin) {
    std::cerr << argv[1] << ": reading failed" << std::endl;
    return 1;
}

At the very end of the program, check the fail bit of fout:

fout << "\\end{tabular}\n";
fout << "\\end{center}";
return !fout;

Input values checking

If there were no values, you might want to fail before starting to write the output:

const auto rows = matrix.size();
if (rows == 0) {
    std::cerr << argv[1] << ": empty" << std::endl;
    return 1;
}

I'm not sure whether the input needs to be rectangular (i.e. all rows have the same number of columns). If so, test it:

const auto cols = matrix[0].size();
for (auto i = cols/cols;  i < matrix.size();  ++i) {
    if (matrix[i].size() != cols) {
        std::cerr << argv[1] << ":" << (i+1)
                  << ": line has " << matrix[i].size() << " columns, expected " << cols << std::endl;
        return 1;
    }
}

That cols/cols is just a shorter way to write (std::vector<double>::size_type)1.

Overall approach

You probably don't want to be typing column headings each time you update the data file, so you're most likely to redirect stdin from a file (and you may well want to automate with Make or similar). I'd suggest that you accept two filename arguments - column headings first, one per line; numeric data second. That way, you can use the number of lines of the first file as the expected column count when reading the data file (and you could error early when reading a non-conforming line).

Reading the column names first gives you another potential advantage: you could transform the input to output line by line, rather than having to store all lines to the end. This makes the program a much better pipeline player, and is a good practice when the inputs might be very large.

You might consider simply writing the output to standard out, rather than opening latex.txt; that gives you more flexibility in how you use it. You'd have to not prompt for the headings then, though!

My version

I've only compiled this, not run it. In particular, I may well have bad logic in reading the double values from the line string, as I don't normally read whole lines and then parse them. I've made a couple of other changes, such as reducing scope of temporary variables, closing fin as soon as we've finished reading, and sprinkling const fairy-dust where I can.

#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>

int main(int argc, char* argv[])
{
    if (argc!=2) {
        std::cerr << "Usage: " << argv[0] << " filename" << std::endl;
        return 1;
    }

    std::ifstream fin(argv[1], std::ios::in);
    if (!fin) {
        std::cerr << argv[1] << ": not readable" << std::endl;
        return 1;
    }

    std::vector< std::vector<double> > matrix;

    // Read input values into matrix
    {
        std::string line;
        while (getline(fin, line)) {
            std::vector<double> v;
            std::istringstream nums(line);
            double x;
            while (nums >> x) {
                v.push_back(x);
            }
            if (!nums) {
                std::cerr << argv[1] << ": parse error" << std::endl;
            }
            matrix.push_back(v);
        }
    }
    if (!fin) {
        std::cerr << argv[1] << ": reading failed" << std::endl;
        return 1;
    }
    fin.close();

    // Test that input isn't empty
    const auto rows = matrix.size();
    if (rows == 0) {
        std::cerr << argv[1] << ": empty" << std::endl;
        return 1;
    }
    // Test that input is rectangular
    const auto cols = matrix[0].size();
    for (auto i = cols/cols;  i < matrix.size();  ++i) {
        if (matrix[i].size() != cols) {
            std::cerr << argv[1] << ":" << (i+1)
                      << ": line has " << matrix[i].size() << " columns; expected " << cols << std::endl;
            return 1;
        }
    }

    auto const& outfile = "latex.txt";
    std::ofstream fout(outfile, std::ios::out);
    if (!fin) {
        std::cerr << outfile << ": not writeable" << std::endl;
        return 1;
    }

    static auto const col_sep = " & ";
    static auto const row_sep = " \\\\\n\\hline\n";

    fout << "\\begin{center}\n\\begin{tabular}{";
    for (int i=0;  i<col;  ++i) {
        fout << "|c";
    }
    fout << "|}\n"
         << "\\hline\n";

    // Ask for col titles:
    for (int i=0;  i<cols;  ++i) {
        std::cout << "Enter the title of column " << i+1 << ", as TeX source:" << std::endl;
        std::string title;
        getline(std::cin,title);
        fout << title
             << (i==cols-1 ? row_sep : col_sep);
    }

    // Write out the values
    for (const auto& v: matrix) {
        for (auto i=0u;  i<cols;  ++i) {
            fout << v[i]
                 << (i==cols-1 ? row_sep : col_sep);
        }
    }

    fout << "\\end{tabular}\n"
         << "\\end{center}";
    fout.close();

    if (!fout) {
        std::cerr << "Failed to write " << outfile << std::endl;
        return 1;
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ I haven't tried either whether the input file needs to be rectangular but I suppose so. I'll implement your suggestions. Thanks for the thorough answer. \$\endgroup\$ – Qbeer666 Mar 13 '17 at 13:26

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.