Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The goal of my program is take some headers and fields and display a nicely formatted table like this:

+----------+----------+----------+
|First name|Last name |Middle    |
|          |          |initial   |
+----------+----------+----------+
|This is a |This is a |This is an|
|line.     |longer    |even      |
|          |line.     |longer    |
|          |          |line.     |
+----------+----------+----------+

Certain features aren't implemented yet, so this code review should focus on what is implemented.

As we all know, premature optimization is the root of all evil, but as the program scales, I think it's important to take into consideration what parts of the program might present a bottleneck or be inefficient. For example:

void display_boxed(std::vector<std::string> fields, int column_width = 10)

Here I pass fields by value to enable move semantics and because I modify fields in the algorithm. If I passed it by reference, it would be incorrect because the changes would reflect upon the caller's copy. If I passed it by const& reference, I would have to initialize a local variable since the parameter would be const. Is the correct thinking?

  • Minimize temporaries/copies. I would like comments on how parts of my code could be improved to achieve this goal.

Then I would like to focus on another aspect: scalability.

  • As many of us have suffered through before, we start writing code and then realize that in order to accommodate different requirements, a large of refactoring has to be done. Is my code scalable?

  • Code reuse. How can I refactor my code to mitigate code reuse?

And finally, in general, I'd just like comments on the algorithm and how it can be improved.

#include <algorithm>
#include <iomanip>
#include <iostream>
#include <sstream>
#include <string>
#include <vector>

// http://rosettacode.org/wiki/Word_wrap#C.2B.2B
std::string wrap(const char *text, size_t line_length = 72)
{
    std::istringstream words(text);
    std::ostringstream wrapped;
    std::string word;

    if (words >> word) {
        wrapped << word;
        size_t space_left = line_length - word.length();
        while (words >> word) {
            if (space_left < word.length() + 1) {
                wrapped << '\n' << word;
                space_left = line_length - word.length();
            } else {
                wrapped << ' ' << word;
                space_left -= word.length() + 1;
            }
        }
    }
    return wrapped.str();
};

void display_boxed(std::vector<std::string> fields, int column_width = 10)
{
    // TODO: Center justify for headers.
    auto adjustfield = std::left;

    int max_linebreaks = 0;
    for (auto&& str : fields)
    {
        int count = std::count(str.begin(), str.end(), '\n');
        if (count > max_linebreaks)
            max_linebreaks = count;
    }

    // We're operating on a copy so it's OK to change the elements
    // of fields.
    for (int i = 0; i <= max_linebreaks; ++i)
    {
        std::cout << "|";
        for (std::vector<std::string>::size_type j = 0; j < fields.size(); ++j)
        {
            auto it = std::find(fields[j].begin(), fields[j].end(), '\n');
            if (it != fields[j].end())
            {
                std::string s{fields[j].begin(), it};
                std::cout << std::setw(column_width) << adjustfield << s << "|";
                fields[j] = std::string{it + 1, fields[j].end()};
            } else {
                std::cout << std::setw(column_width) << adjustfield << fields[j] << "|";
                fields[j] = "";
            }
        }
        std::cout << "\n";
    }
}

int main()
{
    // TODO: Generate column_width.
    constexpr int column_width = 10;

    std::vector<std::string> headers;
    headers.emplace_back(wrap("First name", column_width));
    headers.emplace_back(wrap("Last name", column_width));
    headers.emplace_back(wrap("Middle initial", column_width));

    // TODO:: Account for more than 3 fields.
    std::vector<std::string> fields;
    fields.emplace_back(wrap("This is a line.", column_width));
    fields.emplace_back(wrap("This is a longer line.", column_width));
    fields.emplace_back(wrap("This is an even longer line.", column_width));

    std::string horiz_linebr("+");
    for (std::vector<std::string>::size_type j = 0; j < fields.size(); ++j)
    {
        horiz_linebr += std::string(column_width, '-');
        horiz_linebr += "+";
    }
    horiz_linebr += "\n";
    std::cout << horiz_linebr;
    display_boxed(headers);
    std::cout << horiz_linebr;
    display_boxed(fields);
    std::cout << horiz_linebr;

    return 0;
}
share|improve this question
2  
Welcome to Code Review!! Very nice looking first question! –  RubberDuck Oct 18 '14 at 15:29

1 Answer 1

  • If word length exceeds field width wrap still should do something sensible. As coded I expect the hell breaking loose (size_t is unsigned).

  • Consider renaming max_linebreaks to box_height and factoring out a get_row_height function. Along the same line, it is not quite obvious what is the task of the inner loop. Consider to factor it out into a function, just for sake of giving it a descriptive name.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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