10
\$\begingroup\$

This is a follow-up from my previous post, which can be found here. A readme for more detailed information can be found here. It is a library to create console based tables where you can insert, remove and update entries. An example could look like this:

+---------------+-------------+-----------------+-----------------+---------------------+
|  Country      |  Capital    |  Population     |  Area           |  Currency           |
+---------------+-------------+-----------------+-----------------+---------------------+
|  Australia    |  NEW ENTRY  |  24,877,800     |  7,692,024 km2  |  Australian Dollar  |
|  China        |  Beijing    |  1,403,500,365  |  9,596,961 km2  |  Yuan               |
|  France       |  Paris      |  67,201,000     |  640,679 km2    |  Euro               |
|  Germany      |  Berlin     |  82,800,000     |  357,168 km2    |  Euro               |
|  Iceland      |  Reykjavik  |  348,580        |  102,775 km2    |  Icelandic Krona    |
|  Netherlands  |  Amsterdam  |  17,200,671     |  41,543 km2     |  Euro               |
+---------------+-------------+-----------------+-----------------+---------------------+

I got very good feedback last time and tried to improve the code as best as I could. I wonder if I can fine-tune it any further and if there are still any bad habits that can be avoided.

ConsoleTable.cpp

#include "ConsoleTable.h"


ConsoleTable::ConsoleTable(std::initializer_list<std::string> headers) {
    this->headers = {headers};

    for (std::string column : headers) {
        widths.push_back(column.length());
    }
}


void ConsoleTable::setPadding(unsigned int n) {
    this->padding = n;
}


void ConsoleTable::setStyle(unsigned int n) {
    switch (n) {
        case 0 :
            style = BasicStyle;
            break;
        case 1 :
            style = LineStyle;
            break;
        case 2 :
            style = DoubleLineStyle;
            break;
        case 3 :
            style = InvisibleStyle;
            break;
        default :
            style = BasicStyle;
            break;
    }
}


bool ConsoleTable::addRow(std::initializer_list<std::string> row) {
    if (row.size() > widths.size()) {
        throw std::invalid_argument{"Appended row size must be same as header size"};
    }

    std::vector<std::string> r = std::vector<std::string>{row};
    rows.push_back(r);
    for (int i = 0; i < r.size(); ++i) {
        widths[i] = std::max(r[i].size(), widths[i]);
    }
    return true;
}


bool ConsoleTable::removeRow(unsigned int index) {
    if (index > rows.size())
        return false;

    rows.erase(rows.begin() + index);
    return true;
}


ConsoleTable &ConsoleTable::operator+=(std::initializer_list<std::string> row) {
    if (row.size() > widths.size()) {
        throw std::invalid_argument{"Appended row size must be same as header size"};
    }

    addRow(row);
    return *this;
}


ConsoleTable &ConsoleTable::operator-=(unsigned int rowIndex) {
    if (rows.size() < rowIndex)
        throw std::out_of_range{"Row index out of range."};

    removeRow(rowIndex);

}


std::string ConsoleTable::getLine(RowType rowType) const {
    std::stringstream line;
    line << rowType.left;
    for (int i = 0; i < widths.size(); ++i) {
        for (int j = 0; j < (widths[i] + padding + padding); ++j) {
            line << style.horizontal;
        }
        line << (i == widths.size() - 1 ? rowType.right : rowType.intersect);
    }
    return line.str() + "\n";
}


std::string ConsoleTable::getHeaders(Headers headers) const {
    std::stringstream line;
    line << style.vertical;
    for (int i = 0; i < headers.size(); ++i) {
        std::string text = headers[i];
        line << space * padding + text + space * (widths[i] - text.length()) + space * padding;
        line << style.vertical;
    }
    line << "\n";
    return line.str();
}


std::string ConsoleTable::getRows(Rows rows) const {
    std::stringstream line;
    for (int i = 0; i < rows.size(); ++i) {
        line << style.vertical;
        for (int j = 0; j < rows[i].size(); ++j) {
            std::string text = rows[i][j];
            line << space * padding + text + space * (widths[j] - text.length()) + space * padding;
            line << style.vertical;
        }
        line << "\n";
    }

    return line.str();
}


std::ostream &operator<<(std::ostream &out, const ConsoleTable &consoleTable) {
    out << consoleTable.getLine(consoleTable.style.top);
    out << consoleTable.getHeaders(consoleTable.headers);
    out << consoleTable.getLine(consoleTable.style.middle);
    out << consoleTable.getRows(consoleTable.rows);
    out << consoleTable.getLine(consoleTable.style.bottom);
    return out;
}

bool ConsoleTable::sort(bool ascending) {
    if (ascending)
        std::sort(rows.begin(), rows.end(), std::less<std::vector<std::string>>());
    else
        std::sort(rows.begin(), rows.end(), std::greater<std::vector<std::string>>());
    return true;
}

void ConsoleTable::updateRow(unsigned int row, unsigned int header, std::string data) {
    if (row > rows.size() - 1)
        throw std::out_of_range{"Row index out of range."};
    if (header > headers.size() - 1)
        throw std::out_of_range{"Header index out of range."};

    rows[row][header] = data;
}

void ConsoleTable::updateHeader(unsigned int header, std::string text) {
    if (header > headers.size())
        throw std::out_of_range{"Header index out of range."};

    headers[header] = text;
}


std::string operator*(const std::string &other, int repeats) {
    std::string ret;
    ret.reserve(other.size() * repeats);
    for (; repeats; --repeats)
        ret.append(other);
    return ret;
}

ConsoleTable.h

#ifndef CONSOLETABLE_CONSOLETABLE_H
#define CONSOLETABLE_CONSOLETABLE_H

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

typedef std::vector<std::string> Headers;
typedef std::vector<std::vector<std::string>> Rows;
typedef std::vector<std::size_t> Widths;

class ConsoleTable {
public:

    /// Initialize a new ConsoleTable
    /// \param headers Stringlist of the tables headers
    ConsoleTable(std::initializer_list<std::string> headers);


    /// Sets the distance from the text to the cell border
    /// \param n Spaces between the text and the cell border
    void setPadding(unsigned int n);


    /// Sets the style of the table, default is 0
    /// n = 0 : Basic table style
    /// n = 1 : Single lined table style
    /// n = 2 : Double lined table style
    /// n = 3 : Invisivle table lines style
    /// \param n The table style number
    void setStyle(unsigned int n);


    /// Sorts the table rows based on the first column
    /// \param ascending Should table be sorted ascending or descending
    /// \return True if sorting was successful, otherwise false
    bool sort(bool ascending);


    /// Adds a new row to the table
    /// \param row A list of strings to add as row
    /// \return True if the value was added successfully, otherwise false
    bool addRow(std::initializer_list<std::string> row);


    /// Removes a row from the table by the row index
    /// \param index The index of the row that should be removed
    /// \return True if the row was removed successfully, otherwise false
    bool removeRow(unsigned int index);


    /// Update an existing table cell with new data
    /// \param row The index of the row that needs to be updated
    /// \param header The index of the column that needs to be updated
    /// \param data The new data that should be assigned to teh cell
    void updateRow(unsigned int row, unsigned int header, std::string data);


    /// Update a header with new text
    /// \param header Index of the header that should be updated
    /// \param text The new teext of the new header
    void updateHeader(unsigned int header, std::string text);


    /// Operator of the addRow() function
    /// \param row A list of strings to add as row
    /// \return this
    ConsoleTable &operator+=(std::initializer_list<std::string> row);


    /// Operator of the removeRow() function
    /// \param rowIndex The index of the row that should be removed
    /// \return this
    ConsoleTable &operator-=(unsigned int rowIndex);


    /// Holds all header strings of the table
    Headers headers;


    /// Holds all rows of the table
    Rows rows;


    /// Holds the size of widest string of each column of the table
    Widths widths;

private:

    struct RowType {
        std::string left;
        std::string intersect;
        std::string right;
    };

    struct TableStyle {

        std::string horizontal;
        std::string vertical;
        RowType top;
        RowType middle;
        RowType bottom;
    };

    TableStyle BasicStyle = {"-", "|", {"+", "+", "+"}, {"+", "+", "+"}, {"+", "+", "+"}};
    TableStyle LineStyle = {"━", "┃", {"┏", "┳", "┓"}, {"┣", "╋", "┫"}, {"┗", "┻", "┛"}};
    TableStyle DoubleLineStyle = {"═", "║", {"╔", "╦", "╗"}, {"╠", "╬", "╣"}, {"╚", "╩", "╝"}};
    TableStyle InvisibleStyle = {" ", " ", {" ", " ", " "}, {" ", " ", " "}, {" ", " ", " "}};
    TableStyle style = BasicStyle;

    std::string space = " ";


    /// The distance between the cell text and the cell border
    unsigned int padding = 1;


    /// Returns a formatted horizontal separation line for the table
    /// \param rowType The type of the row (top, middle, bottom)
    /// \return The formatted row string
    std::string getLine(RowType rowType) const;


    /// Returns a formatted header string
    /// \param headers The Headers-object that holds the header strings
    /// \return The formatted header string
    std::string getHeaders(Headers headers) const;


    /// Returns a formmatted row string
    /// \param rows The Rows-object that holds all rows of the table
    /// \return A formatted string of all rows in the table
    std::string getRows(Rows rows) const;


    /// Writes the entire table with all its contents in the output stream
    /// This can be used to display the table using the std::cout function
    /// \param out The output stream the table should be written to
    /// \param consoleTable The ConsoleTable-object
    /// \return Output stream with the formatted table string
    friend std::ostream &operator<<(std::ostream &out, const ConsoleTable &consoleTable);

};


/// Repeats a given string n times
/// \param other The string to repeat
/// \param repeats The amount the string should be repeated
/// \return The repeated string
std::string operator*(const std::string &other, int repeats);


#endif //CONSOLETABLE_CONSOLETABLE_H

To create an example table like shown above, this is how you can use the lib:

#include <iostream>
#include "ConsoleTable.h"

int main() {

    ConsoleTable table{"Country", "Capital", "Population", "Area", "Currency"};

    table.setPadding(2);
    table.setStyle(3);

    table += {"Germany", "Berlin", "82,800,000", "357,168 km2", "Euro"};
    table += {"France", "Paris", "67,201,000", "640,679 km2 ", "Euro"};
    table += {"South Korea", "Seoul", "51,446,201", "100,210 km2 ", "South Korean Won"};
    table += {"Australia", "Canberra", "24,877,800", "7,692,024 km2", "Australian Dollar"};
    table += {"China", "Beijing", "1,403,500,365", "9,596,961 km2", "Yuan"};
    table += {"Iceland", "Reykjavik", "348,580", "102,775 km2", "Icelandic Krona"};
    table += {"Netherlands", "Amsterdam", "17,200,671", "41,543 km2", "Euro"};

    table.updateRow(3, 1, "NEW ENTRY");

    // Remove some entries
    table -= 2;
    table -= 1;
    table -= 0;
    table.sort(true);

    std::cout << table;

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Just a aside, but don't just limit this to console - works fine on a (real or virtual) terminal, and is great for a printer! \$\endgroup\$ – Toby Speight Apr 19 '18 at 13:40
  • \$\begingroup\$ @TobySpeight It isn't just limited to console output. The output is a stream and can be written in either std::cout or a file etc. I just found the name console table most fitting :) \$\endgroup\$ – 766F6964 Apr 19 '18 at 16:18
  • \$\begingroup\$ Some style note: you may skip one indent level inside switch, so your switch and case will be on one line. \$\endgroup\$ – sineemore Apr 20 '18 at 0:56
  • \$\begingroup\$ Tag quibble: I don't think most people consider simple border lines to be "ascii art". That generally means something that looks like a picture. \$\endgroup\$ – Barmar Apr 20 '18 at 3:43
7
\$\begingroup\$

This version is definitely much improved. Good job! I see a few things that might help you improve it even further.

Use all required #includes

The code uses std::initializer_list which means that it should #include <initializer_list>.

Separate interface from implementation

The header file should have just enough information for the user of the class to be able to use it and no more. Everything else is an implementation detail and should be in the .cpp file. For that reason, I'd suggest that the #includes of <sstream>, <memory>, and <algorithm> belong in the .cpp file and not in the .h file.

Make data members private

The headers, rows and widths members of ConsoleTable are public but they really ought to be private members. In a similar vein, I'd strongly suggest moving the typedefs to inside the ConsoleTable class so those definitions don't pollute the global namespace.

Use const where practical

I'd suggest passing in a const std::string & into updateRow and updateHeader.

Put friends in public section

The friend declaration really shouldn't be in the private section of the class in my view. The friend declaration means that the other class (std::ostream in this case) has access to all members of this class, so it seems to me to be somewhat misleading to put it in the private section since any other code can call it. Syntactically, there is no difference but semantically, it seems to me that it should be public.

Be careful with signed vs. unsigned

The loop counters i and j are declared as int values, and they're compared to widths.size() and headers.size(), but the size() function returns an unsigned number. I'd recommend changing the type of i and j to either std::size_t or unsigned.

Careful with That Axe, Eugene

In the operator-= function, it claims to return a ConsoleTable & but is missing return *this;

Make fixed data members static const

The various TableStyles are not really unique per ConsoleTable object instance, so they should probably be declared as static const. (You could use constexpr std::string_view if your compiler is compliant with C++17). Also space is an implementation detail that could simply be a static const variable within the .cpp file.

Don't use superfluous this->

The use of superfluous this-> in the code makes it harder to read and just adds visual clutter. So for example, instead of this:

ConsoleTable::ConsoleTable(std::initializer_list<std::string> headers) {
    this->headers = {headers};

    for (std::string column : headers) {
        widths.push_back(column.length());
    }
}

I'd write this:

ConsoleTable::ConsoleTable(std::initializer_list<std::string> headers) :
    headers{headers}
{
    for (const auto &column : headers) {
        widths.push_back(column.length());
    }
}

In addition to eliminating this->, I've used the more modern style of constructor and a const reference within the loop.

Consider the user (part 1)

Right now, the user has to specify the style by using a number. Internally, the number gets translated into BasicStyle, LineStyle, etc. I'd say that's exactly backwards from how the interface should be. Instead of this:

table.setStyle(0);

it would be much easier to understand the intent if it were instead written like this:

table.setStyle(ConsoleTable::BasicStyle);

That could easily be done by turning those styles into an enum and creating a std::array of the actual style strings. For example, we might define the enum like this:

enum class Style{BasicStyle, LineStyle, DoubleLineStyle, InvisibleStyle};

Create the styles like this:

const std::array<ConsoleTable::TableStyle, 4> ConsoleTable::st = {{
    {"-", "|", {"+", "+", "+"}, {"+", "+", "+"}, {"+", "+", "+"}},
    {"━", "┃", {"┏", "┳", "┓"}, {"┣", "╋", "┫"}, {"┗", "┻", "┛"}},
    {"═", "║", {"╔", "╦", "╗"}, {"╠", "╬", "╣"}, {"╚", "╩", "╝"}},
    {" ", " ", {" ", " ", " "}, {" ", " ", " "}, {" ", " ", " "}},
}};

And now we can use them like this:

void ConsoleTable::setStyle(Style n) {
    style = static_cast<int>(n);
}

Naturally, the corresponding changes would need to be made wherever style was used.

Consider the user (part 2)

Another way the user's life could be made easier is to allow syntax like this:

table.update("Australia", "Capital", "Brisbane");

instead of or in addition to this:

table.updateRow(3, 1, "NEW ENTRY");

One way to do that:

bool ConsoleTable::update(const std::string &item, 
                          const std::string &col, 
                          const std::string &data) 
{
    auto row{std::find_if(rows.begin(), rows.end(), 
             [&item](auto &r){return r.front() == item;}) };
    auto hdrcol{std::find(headers.begin(), headers.end(), col)};
    if ((row != rows.end()) && (hdrcol != headers.end())) {
        (*row)[hdrcol - headers.begin()] = data;
        return true;
    }
    return false;
}

Omit return 0 in main

The return 0 is automatically generated by the compiler, so I generally advocate omitting it. Others feel very strongly that it should be written anyway. Do what feels right to you.

\$\endgroup\$
  • \$\begingroup\$ Regarding the enum suggestion: You say you'd recommend to create a public enum which holds 4 items (the styles)? Would each value in the enum represent the number currently used? \$\endgroup\$ – 766F6964 Apr 20 '18 at 0:25
  • \$\begingroup\$ I've updated my answer to explain a bit more about the use of an enum class in this context. \$\endgroup\$ – Edward Apr 20 '18 at 0:36
7
\$\begingroup\$
this->headers = {headers};

In the constructor? You mean to use an initializer instead

:headers{headers}

before the opening brace of the body.

this->padding = n;

why would you use this-> to access members of the class you are in?

void ConsoleTable::setStyle(unsigned int n) {

You have consecutive integers as the cases and only get the value for the same thing each time.

    if (n<0 || n>3)  style= BasicStyle;
    else style= {BasicStyle,LineStyle,DoubleLineStyle,InvisibleStyle}[n];

But really, look at the comment on the header's declaration! You should be passing an enum for this, not making the user figure out the mapping just so you can reverse it.

std::vector<std::string> r = std::vector<std::string>{row};

Why not just

std::vector<std::string> r {row};

or

auto r = std::vector<std::string>{row};

?

ConsoleTable::operator+= is repeating the same precondition as the function it forwards to.

It is best practice in C++ to write the & with the type, not the symbol name:

ConsoleTable& foo (⋯

The operator- is "the same" as removeRow in the same way as += is to addRow. But the behavior is different with respect to error reporting; for += they are really the same meaning. This difference seems awkward.

return line.str() + "\n";

that is inefficient: copy the string out of the stringstream, then create a new one from a copy of that and appending one char, then returning the copy.

Enable NRVO and avoid all extra copies like this:

auto retval = line.str();
retval.push_back('\n');
return retval;

The heavy lifting in GetRows and GetHeaders have a lot of repetition:

line << space * padding + text + space * (widths[i] - text.length()) + space * padding;
    line << style.vertical;

line << space * padding + text + space * (widths[j] - text.length()) + space * padding;
        line << style.vertical;

see the only difference is in one letter, i vs j. This should be common code. I think both can be written in terms of a common internal helper that gets passed a row (which might actually be the headers).

You start with std::string text = headers[i] which makes a copy of the string. You never change it. So make it a const& instead.

in sort you repeat the entire statement just to make a change to one argument. I guess the obvious simplification doesn’t work because it is a template, and it’s too small to do anything more serious to it.

But why does it always return true? What is the return value supposed to indicate?

The various preconditions are common and reused in several places: header or row index out of range? Make those common helper functions. That has the further benefit of shrinking the code, since throw generates a lot of code and this is a rare code path to take.

Your operator* for strings is in the global namespace. What if I didn’t want that, and it conflicted with my own uses of *?

does operator<< really need to be a friend? It’s possible to getHeaders getRows etc. so it should be able to work with the public interface only.

It would be handy if sort() defaulted to ascending. Actually, instead of true/false it should take a predicate so the user can sort on any kind of logic, and on any column. In particular, the way words are sorted varies by locale and even has oddities for different kinds of lists, such as names vs. other words.

Are all the nested headers in ConsoleTable.h needed by the header? I wonder about a bunch of them. I think you can move a lot of those into the CPP file.


Overall architecture: Nice. Good encapsulation of a single feature.

\$\endgroup\$
6
\$\begingroup\$

Style (naming)

The constructor could be improved. Instead of:

ConsoleTable::ConsoleTable(std::initializer_list<std::string> headers) {
    this->headers = {headers};

    for (std::string column : headers) {
        widths.push_back(column.length());
    }
}

try this:

ConsoleTable::ConsoleTable(std::initializer_list<std::string> aHeaders) : headers(aHeaders) {
    for (auto const& column : headers) {
        widths.push_back(column.length());
    }
}

Which breaks down to:

  1. Don't use this->field -- in most cases it only clutters the code.

  2. I prefer to have some distinction between the c-tor parameter and the class' field, so I use "naming convention": field becomes aField in a c-tor.

  3. Using auto const& column prevents compiler from copying every column.

Ampersand (&) should be glued to the return type. Instead of

ConsoleTable &operator+=(std::initializer_list<std::string> row);

Try:

ConsoleTable& operator+=(std::initializer_list<std::string> row);

Style (parentheses)

Many add space after if, else if, for, etc... to differentiate it from functions, i.e.:

if (cond) { ... }

I disagree with that -- this is why these tokens are keywords -- and I don't use spaces. But this is only my opinion.

const-correctness

The space character could be const. Instead of:

std::string space = " ";

Try:

std::string const SPACE = " ";

(Side note: I prefer to have const modifier after the type, this frees your brain from remembering how to read such things: int const* const.) Also try to have const variable in capitals, and maybe name SPACE_CHARACTER is more descriptive.

If something does not change, it does not change. Instead of:

ConsoleTable &operator-=(unsigned int rowIndex);

Try:

ConsoleTable &operator-=(unsigned int const rowIndex);

cross-arch

So you have:

ConsoleTable &operator-=(unsigned int const rowIndex);

Does it really need to be unsigned int? How many rows do you need? Is it stable? Is it robust? unsigned int is guaranteed to be at least 16 bits long. I would use:

ConsoleTable &operator-=(uint32_t const rowIndex);

In such a case it should be 32 bits wide.

Documentation

Parameter name is duplicated in the function doc. Instead of:

/// Update a header with new text
/// \param header Index of the header that should be updated
/// \param text The new teext of the new header
void updateHeader(unsigned int header, std::string text);

Try:

/// Update a header with new text
void updateHeader(unsigned int header, /**< Index of the header
                                            that should be updated */
                  std::string text     /**< The new teext of the new header */);

You won't need to change your docs, when parameter name will change.

Also, I don't know why people use this comment style:

/// Update a header with new text

While Doxygen allows java-doc style:

/*
 * @brief Update a header with new text
 */

You won't need to think when/if the first line is only short description. Besides, it also look more consistent when adding file header, e.g.:

/**
 * @file
 * Copyright (C) 2017 shycha (shycha@email.com)
 *
 * Created on Apr 19, 2018
 *
 */

Eclipse can add this automatically. And this is also something that is useful, e.g., when your GPL-licensed code will be copy-and-pasted in some other project. If so, you'll be famous, which is what programming is about anyway. :)

Headers

Someone else pointed it out in the first part. But... Do you need all of:

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

I think it would suffice:

#include <string>
#include <vector>
#include <initializer_list>
#include <iosfwd>

If I remember correctly <iosfwd> it should contain "forward-decl" of std::ostream.

These are only used by the implementation:

#include <iostream>
#include <sstream>
#include <memory>
#include <algorithm>

I also prefer to copy all includes from the header file to the implementation file. The rationale behind this is that it ensures that when interface change is made, it will not impact the implementation file. And you get the implicit-include problem solved at no cost. But I can't guarantee it is how the C++ code is written nowadays.

Interface

Does it really belong to the interface?

// file: ConsoleTable.h
std::string operator*(const std::string &other, int repeats);

Do you want to pollute the global namespace with this operator? Is a part of the ConsoleTable interface?

Instead do this (I avoided the "try" word on purpose):

// file: ConsoleTable.cpp
namespace {
std::string operator*(const std::string &other, int repeats);
}

Putting it in an unnamed namespace in the implementation file assures that this function won't be visible in the symbol table.

Duplicated functionality?

The difference between:

ConsoleTable &operator+=(std::initializer_list<std::string> row);
ConsoleTable &operator-=(unsigned int rowIndex);

and

bool addRow(std::initializer_list<std::string> row);
bool removeRow(unsigned int index);

should be explained in the documentation. One throws, the other does not.

Also:

ConsoleTable &ConsoleTable::operator-=(unsigned int rowIndex) {
    if (rows.size() < rowIndex)
        throw std::out_of_range{"Row index out of range."};

    removeRow(rowIndex);

}

and:

bool ConsoleTable::removeRow(unsigned int index) {
    if (index > rows.size())
        return false;

    rows.erase(rows.begin() + index);
    return true;
}

have inconsistent bound checking, which may cause confusion when quickly screening the code, i.e., you need to mentally switch between < and >, which consumes your "brain cycles". :)

[added 23.04.2018, 16:36]

IMHO, the preferred way would be:

ConsoleTable& ConsoleTable::operator-=(unsigned int rowIndex) {
    if (false == removeRow(rowIndex)) {
        throw std::out_of_range{"Row index out of range."};
    }
}

This way you "re-use" the fact that ConsoleTable::removeRow returns true/false -- so it minimises the burden of analysis. You intuitively expect when some operation returned false that something went wrong. This is only if you really need the operator implementation. Personally, I think that it creates more problems than helps with anything. I seems quite concise to write:

table -= 7;

to remove seventh row. But what does it syntactically mean to subtract 7 from a table? Why is it removing the row? Why not column? Ideally, mathematical operations (+, -, etc.) should be used only with mathematical objects, e.g.: numbers, matrices, vectors -- but not std::vector, I mean mathematical vector, note that the std::vector does not have overloaded operator+=(...) to add more elements. I think that this is because library designer knew that it makes no sense syntactically to allow such an overload, etc. Such a syntactic sugar could be useful in small implementation, but I would definitely avoid it in larger designs. Consider, e.g., in a 100 kLOC code base that someone encounters:

table -= 7;

I would (incorrectly) think that it decreases some integer named table by 7. To "decipher" it you need to find what table is and the go to its implementation to figure out how is operator-=(...) overloaded. (Remember, as I said, it could, for example, remove the 7th column, there's nothing specific about rows in a real table. ;)) Compare this with:

table.removeRow_throwing(7); // Yeah, it's quite stupid to indicate
                             // that member function throws, but I
                             // used it only to differentiating naming

This shows intent more clearly. (Some may say that too clearly. ;))

There's also another problem with:

table -= 7;

It can throw. Most people won't think that an operator may throw. Especially when it looks like adding 7 to some int table;.

[/added 23.04.2018, 16:36]

Braces

I prefer:

if (index > rows.size()) {
    return false;
}

over:

if (index > rows.size())
    return false;

Or if really want to save that one line, I would do:

if (index > rows.size()) { return false; }

Having braces around the "one-liners" eases adding new code and reduces the frustration related to adding the braces after. No more "I should've added them at the first place". ;)

Field visibility

These:

/// Holds all header strings of the table
Headers headers;


/// Holds all rows of the table
Rows rows;


/// Holds the size of widest string of each column of the table
Widths widths;

should be private!

File naming

I was taught that *.h is a C header, and for C++ it should be used *.hh or *.hpp. I know it is not entirely true, but using: *.h & *.c for C only and *.hh & *.cc (or *.hpp & *.cpp, or *.hxx & *.cxx, or even *.h++ & *.c++ if you want to be particularly unique) for C++ only provides a nice distinction between C and C++ files without even opening them. It could also be used by some shell script if needed.

Further reading:

Try: https://google.github.io/styleguide/cppguide.html

\$\endgroup\$
  • 1
    \$\begingroup\$ Good review, but I do disagree on your use of uint32_t (even on platforms where it does exist). std::size_t is the absolutely appropriate type here (even if unlikely ever to be used to the full!). Oh, and making it const in the declaration is just useless clutter (but you should do so in the definition as appropriate). \$\endgroup\$ – Toby Speight Apr 19 '18 at 13:42
  • 1
    \$\begingroup\$ Thanks! In fact, this is my first post on code review and to be honest I think it could be improved. Regarding "const-ness", I sometimes overuse it. uint32_t vs std::size_t... Well... IMHO, it depends, which kinda look like avoiding the answer. But the fact is that both have limitations. I just prefer to know the exact size, which in this case is 4. \$\endgroup\$ – shycha Apr 19 '18 at 17:12
  • \$\begingroup\$ Regarding your header suggestion: #include <algorithm> is required because of the std::sort() function and therefore can not be removed. \$\endgroup\$ – 766F6964 Apr 19 '18 at 17:12
  • \$\begingroup\$ @766F6964 I thought about moving it to the implementation file. But I must've removed it during editing. Fixed now. \$\endgroup\$ – shycha Apr 19 '18 at 17:17
  • 1
    \$\begingroup\$ @766F6964 [2/2] I had to look up back again to ensure that implementation is correct when screening the code. It is faster for reviewer when the code is more concise at the first place. For the same reason I would recommend using just simply index (or rowIndex) in both implementations. I fixed it in the response. \$\endgroup\$ – shycha Apr 23 '18 at 14:09
0
\$\begingroup\$

One thing I'd suggest, is to take a lesson from established datatables in other languages/frameworks. The .net datatable class comes to mind.

For one thing a datarow is a separate class of which the datatable contains a collection. To my mind it allows the user to know exactly what they are creating.

Along with this, so that the user doesn't have to constantly look up the column information when creating a new row, a method in the datatable class will create a new row with the required empty fields, that just need to be filled or not as per the user.

\$\endgroup\$
  • \$\begingroup\$ I had a separate class for the rows in my previous version of the lib, but people advised that it's better to go with one compact class instead. \$\endgroup\$ – 766F6964 Apr 19 '18 at 19:12
  • \$\begingroup\$ Most likely a potayto/potawto type of thing. Knowing where to draw the line between usability and compactness can be hard sometimes. \$\endgroup\$ – tinstaafl Apr 19 '18 at 19:17

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.