12
\$\begingroup\$

Personally I often prefer console applications over GUI applications. However, sometimes it is quite a challenge to display everything well-aligned. Therefore I decided to create a console-based table which makes it possible to display data organized into categories. You can add, remove and update entries and choose between different table designs. This is an example how it looks like.

enter image description here enter image description here

(Some characters might not be displayed correctly in the Windows console. In the Linux terminal all characters are printed without a problem.)

I'd like to get some feedback what I can improve in this class and if there are any bad habits.

ConsoleTable.h

#ifndef CONSOLETABLE_CONSOLETABLE_H
#define CONSOLETABLE_CONSOLETABLE_H

#include <string>
#include <vector>
#include <iostream>
#include "ConsoleTableRow.h"
#include "ConsoleTableUtils.h"
#include <sstream>

enum TableStyle {
    BASIC,
    LINED,
    DOUBLE_LINE,
};

enum HorizontalSeperator{
    SEPERATOR_TOP,
    SEPERATOR_MIDDLE,
    SEPERATOR_BOTTOM
};

class ConsoleTable {
public:

    ConsoleTable(TableStyle style);

    void setPadding(unsigned int width);

    void addColumn(std::string name);

    void addRow(ConsoleTableRow *item);

    bool removeRow(int index);

    bool editRow(std::string data, int row, int col);

    void printTable();

private:

    unsigned int padding = 1;

    std::vector<std::string> columns;
    std::vector<ConsoleTableRow *> entries;
    ConsoleTableUtils* utils;

    // Table Style variables
    std::string style_line_horizontal;
    std::string style_line_vertical;
    std::string style_line_cross;
    std::string style_t_intersect_right;
    std::string style_t_intersect_left;
    std::string style_t_intersect_top;
    std::string style_t_intersect_bottom;
    std::string style_edge_topleft;
    std::string style_edge_topright;
    std::string style_edge_buttomleft;
    std::string style_edge_buttomright;

    void printHorizontalSeperator(const std::vector<int> &maxWidths, HorizontalSeperator seperator) const;

    void setTableStyle(TableStyle style);

};


#endif //CONSOLETABLE_CONSOLETABLE_H

ConsoleTable.cpp

#include "ConsoleTable.h"


ConsoleTable::ConsoleTable(TableStyle style) {
    setTableStyle(style);
    this->utils = new ConsoleTableUtils();
}

void ConsoleTable::addColumn(std::string name) {
    this->columns.push_back(name);
}

void ConsoleTable::printTable() {

    // Calculate column maxima
    std::vector<int> maxWidths(this->columns.size());
    for (int row = 0; row < this->entries.size(); row++) {
        for (int col = 0; col < this->columns.size(); col++) {
            std::string cellText = this->entries[row]->getEntry()[col];
            if (this->columns[col].length() > maxWidths[col])
                maxWidths[col] = this->columns[col].length();
            if (maxWidths[col] < cellText.length()) {
                maxWidths[col] = cellText.length();
            }
        }
    }

    printHorizontalSeperator(maxWidths, SEPERATOR_TOP);

    // Print column values
    for (int col = 0; col < this->columns.size(); col++) {
        std::string cellText = this->columns[col];
        int len = cellText.length();
        std::string paddedText = cellText + std::string(maxWidths[col] - len, ' ');
        std::cout << this->style_line_vertical << std::string(this->padding, ' ') << paddedText
                  << std::string(this->padding, ' ');
        std::cout << (col == this->columns.size() - 1 ? this->style_line_vertical + "\n" : "");
    }

    printHorizontalSeperator(maxWidths, SEPERATOR_MIDDLE);

    // Print cell values
    for (int row = 0; row < this->entries.size(); row++) {
        for (int col = 0; col < this->columns.size(); col++) {
            std::string cellText = this->entries[row]->getEntry()[col];
            std::string paddedText = cellText + std::string(maxWidths[col] - cellText.length(), ' ');
            std::cout << this->style_line_vertical << std::string(this->padding, ' ') << paddedText
                      << std::string(this->padding, ' ');
        }
        std::cout << this->style_line_vertical << std::endl;
        if (row == this->entries.size() - 1)
            printHorizontalSeperator(maxWidths, SEPERATOR_BOTTOM);
        else
            printHorizontalSeperator(maxWidths, SEPERATOR_MIDDLE);
    }
}

void ConsoleTable::printHorizontalSeperator(const std::vector<int> &maxWidths, HorizontalSeperator seperator) const {
    for (int col = 0; col < columns.size(); ++col) {

        switch (seperator) {
            case SEPERATOR_TOP: {
                std::cout << (col == 0 ? this->style_edge_topleft : "");
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout << utils->repeatString(this->style_line_horizontal, maxWidths[col]);
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout << (col != columns.size() - 1 ? this->style_t_intersect_top : this->style_edge_topright);
                std::cout << (col == columns.size() - 1 ? "\n" : "");
                break;
            }
            case SEPERATOR_MIDDLE: {
                std::cout << (col == 0 ? this->style_t_intersect_left : "");
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout << utils->repeatString(this->style_line_horizontal, maxWidths[col]);
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout << (col != columns.size() - 1 ? this->style_line_cross : this->style_t_intersect_right);
                std::cout << (col == columns.size() - 1 ? "\n" : "");
                break;
            }
            case SEPERATOR_BOTTOM: {
                std::cout << (col == 0 ? this->style_edge_buttomleft : "");
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout << utils->repeatString(this->style_line_horizontal, maxWidths[col]);
                std::cout << utils->repeatString(this->style_line_horizontal, this->padding);
                std::cout
                        << (col != columns.size() - 1 ? this->style_t_intersect_bottom : this->style_edge_buttomright);
                std::cout << (col == columns.size() - 1 ? "\n" : "");
                break;
            }
        }
    }
}

void ConsoleTable::addRow(ConsoleTableRow *item) {
    this->entries.push_back(item);
}

bool ConsoleTable::removeRow(int index) {
    if (index > this->entries.size())
        return false;
    this->entries.erase(this->entries.begin() + index);
    return true;
}

bool ConsoleTable::editRow(std::string data, int row, int col) {
    if(row > this->entries.size())
        return false;

    if(col > this->columns.size())
        return false;

    auto entry = this->entries[row];
    entry->editEntry(data, col);
    return true;
}

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

void ConsoleTable::setTableStyle(TableStyle style) {
    switch (style) {
        case BASIC: {
            this->style_line_horizontal = "-";
            this->style_line_vertical = "|";
            this->style_line_cross = "+";

            this->style_t_intersect_right = "+";
            this->style_t_intersect_left = "+";
            this->style_t_intersect_top = "+";
            this->style_t_intersect_bottom = "+";

            this->style_edge_topleft = "+";
            this->style_edge_topright = "+";
            this->style_edge_buttomleft = "+";
            this->style_edge_buttomright = "+";
            break;
        }
        case LINED: {
            this->style_line_horizontal = "━";
            this->style_line_vertical = "┃";
            this->style_line_cross = "╋";

            this->style_t_intersect_right = "┫";
            this->style_t_intersect_left = "┣";
            this->style_t_intersect_top = "┳";
            this->style_t_intersect_bottom = "┻";

            this->style_edge_topleft = "┏";
            this->style_edge_topright = "┓";
            this->style_edge_buttomleft = "┗";
            this->style_edge_buttomright = "┛";
            break;
        }
        case DOUBLE_LINE: {
            this->style_line_horizontal = "═";
            this->style_line_vertical = "║";
            this->style_line_cross = "╬";

            this->style_t_intersect_right = "╣";
            this->style_t_intersect_left = "╠";
            this->style_t_intersect_top = "╦";
            this->style_t_intersect_bottom = "╩";

            this->style_edge_topleft = "╔";
            this->style_edge_topright = "╗";
            this->style_edge_buttomleft = "╚";
            this->style_edge_buttomright = "╝";
            break;
        }
    }
}

ConsoleTableRow.h

#ifndef CONSOLETABLE_CONSOLETABLEENTRY_H
#define CONSOLETABLE_CONSOLETABLEENTRY_H

#include <string>
#include <vector>

class ConsoleTableRow {
public:

    ConsoleTableRow(int width);

    void addEntry(std::string data, int column);

    void editEntry(std::string data, int column);

    std::vector <std::string> getEntry();

private:
    std::vector <std::string> row;
};


#endif //CONSOLETABLE_CONSOLETABLEENTRY_H

ConsoleTableRow.cpp

#include "ConsoleTableRow.h"

ConsoleTableRow::ConsoleTableRow(int width) {
    this->row.resize(width);
}

void ConsoleTableRow::addEntry(std::string data, int column) {
    row[column] = data;
}

std::vector<std::string> ConsoleTableRow::getEntry() {
    return this->row;
}

void ConsoleTableRow::editEntry(std::string data, int column) {
    this->row[column] = data;
}

ConsoleTableUtils.h

#ifndef CONSOLETABLE_CONSOLETABLEUTILS_H
#define CONSOLETABLE_CONSOLETABLEUTILS_H

#include <string>
#include <sstream>

class ConsoleTableUtils {
public:

    std::string repeatString(std::string input, int n) const;

};


#endif //CONSOLETABLE_CONSOLETABLEUTILS_H

ConsoleTableUtils.cpp

#include "ConsoleTableUtils.h"

std::string ConsoleTableUtils::repeatString(std::string input, int n) const {
    std::ostringstream os;
    for (int i = 0; i < n; i++)
        os << input;
    return os.str();
}

Here is an example to generate use the classes. This example will generate the tables that can be seen in the screenshot above.

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

int main() {

    ConsoleTable ct(BASIC);
    ct.setPadding(1);

    ct.addColumn("Country");
    ct.addColumn("Name");
    ct.addColumn("Profession");
    ct.addColumn("Age");

    auto entry = new ConsoleTableRow(4);
    entry->addEntry("Germany", 0);
    entry->addEntry("Michael", 1);
    entry->addEntry("Computer Engineer", 2);
    entry->addEntry("19", 3);
    ct.addRow(entry);

    auto entry2 = new ConsoleTableRow(4);
    entry2->addEntry("England", 0);
    entry2->addEntry("Robert", 1);
    entry2->addEntry("Artist", 2);
    entry2->addEntry("34", 3);
    ct.addRow(entry2);

    auto entry3 = new ConsoleTableRow(4);
    entry3->addEntry("United Kingdom", 0);
    entry3->addEntry("Julia", 1);
    entry3->addEntry("Designer", 2);
    entry3->addEntry("42", 3);
    ct.addRow(entry3);

    auto entry4 = new ConsoleTableRow(4);
    entry4->addEntry("United Staates", 0);
    entry4->addEntry("Jo", 1);
    entry4->addEntry("Actor", 2);
    entry4->addEntry("21", 3);
    ct.addRow(entry4);

    // Print all entries
    ct.printTable();

    return 0;
}

The project repository can be found on GitHub.

\$\endgroup\$
4
  • \$\begingroup\$ Please post everything needed to compile your code. Also, it would be really nice of you to add a runnable example (e.g. the code that produced your pictures) so that we can test your code and verify that it works correctly. \$\endgroup\$ Commented Apr 1, 2018 at 23:59
  • \$\begingroup\$ @BenSteffan Sorry for the inconvenience. I am new to the stackexchange plattform. I added all required classes in the start post together with an example to generate the table that can be seen in the screenshot. \$\endgroup\$
    – 766F6964
    Commented Apr 2, 2018 at 1:42
  • 1
    \$\begingroup\$ Welcome to Code Review! Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Commented Apr 3, 2018 at 13:48
  • \$\begingroup\$ @TobySpeight Sorry I wasn't aware. I will post my improved code as a new question once I processed all the suggestions mentioned here. Thanks for the reminder. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 13:57

4 Answers 4

1
\$\begingroup\$

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

Think of the user

This is rarely the first suggestion I make, but in this case, it seems particularly important. The problem is that the addition of rows seems to require that the user keep track of the column number and the syntax is quite verbose. Instead of this:

auto entry2 = new ConsoleTableRow(4);
entry2->addEntry("England", 0);
entry2->addEntry("Robert", 1);
entry2->addEntry("Artist", 2);
entry2->addEntry("34", 3);
ct.addRow(entry2);
ct.printTable();

I would greatly prefer to write this:

ct += {"England", "Robert", "Artist", "34"};
std::cout << ct;

A few of the suggestions below relate to how one might do this.

Use only necessary #includes

The #include <unistd.h> line in the main program is not necessary and can be safely removed. Also, <iostream> is certainly needed in ConsoleTable.cpp but not in ConsoleTable.h because the interface makes no use of it (even though the implemetation does.)

Think carefully about global enums

The TableStyle and HorizontalSeparator enums only really have relevance within the ConsoleTable class. For that reason, I'd put the TableStyle inside the ConsoleTable class and move the HorizontalSeparator enum to the ConsoleTable.cpp implementation file. Nothing outside that class should ever touch it.

Be consistent with naming

Speaking of TableStyle, it's strange to have LINED (with the D at the end) but DOUBLE_LINE (without the D at the end). It's a minor thing, but this sort of inconsistency can annoy users of the class.

Simplify your code

The printHorizontalSeperator code is quite long, repetitive and difficult to read. Also, there are only ever three types of lines: line, middle and bottom. I'd suggest instead to simplify this greatly. First, let's make it much easier to add styles. I'd add this to the ConsoleTable class:

static constexpr std::string_view markers[3][11] {
    { "-","|",
    "+","+","+",
    "+","+","+",
    "+","+","+"},
    { "━","┃", 
    "┏","┳","┓",
    "┣","╋","┫",
    "┗","┻","┛"},
    { "═","║",
    "╔","╦","╗",
    "╠","╬","╣",
    "╚","╩","╝"},
};

Two things to note here. First, I'm using C++17 and so std::string_view, allowing for this to be constexpr, but if you don't have that, it's simple enough to make them plain const std::string instead. Second, the way the characters are physically arranged makes it much simpler to visually verify that the characters are correct.

Next, I'd recommend creating a private member function like this:

std::string line(unsigned n) const;

This constructs the top, middle or bottom line and returns a single string. Here's how I wrote it:

std::string ConsoleTable::line(unsigned n) const {
    std::stringstream line;
    n *= 3;
    line << markers[linetype][2+n];
    for (std::size_t i{0}; i < widths.size()-1; ++i) {
        for (std::size_t j{0}; j < (widths[i] + padsize + padsize); ++j) {
            line << markers[linetype][0];
        }
        line << markers[linetype][3+n]; 
    }
    for (std::size_t j{0}; j < (widths.back() + padsize + padsize); ++j) {
        line << markers[linetype][0];
    }
    line << markers[linetype][4+n] << '\n';
    return line.str();
}

Here is how the private member data variables are declared:

std::size_t padsize;
Style linetype;
bool innerlines;
std::vector<std::string> header;
std::vector<std::size_t> widths;
std::vector<std::vector<std::string>> rows;

As you can probably infer, I've renamed TableStyle to Style and put it within the class definition.

Use std::initializer_list to simplify code

Reading the code, it seemed that the most fundamental part of the ConsoleTable was not the line style (which the current constructor uses) but instead, the names of the columns. Here's the constructor that uses std::initializer_list to greatly simplify the code:

ConsoleTable::ConsoleTable(std::initializer_list<std::string> list) :
    padsize{1},
    linetype{BASIC},
    innerlines{false},
    header{list}
{ 
    updateWidths(); 
}

Here I'm using the C++11 unified constructor syntax (with the {}) so it's unambiguous that these are not function calls

Overload operators to make the syntax clean

The use of the += operator in my proposed example above is not at all difficult to write:

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"};
    }
    std::vector<std::string> r = std::vector<std::string>{row};
    rows.push_back(r);
    for (std::size_t i{0}; i < r.size(); ++i) {
        widths[i] = std::max(r[i].size(), widths[i]);
    }
    return *this;
}

This code doesn't use explicit dynamic memory allocation, but it certainly could with minor changes. The nice thing is that if you want to change that, it's a detail that's hidden within the class implementation and the interface could remain the same.

Use const where possible

The printTable() function does not (and should not) alter the underlying data structure and should therefore be declared const. Using const wherever it's practical to do so is a very easy way to get good performance from your code.

Use an ostream &operator<< instead of printTable

The current code has void ConsoleTable::printTable() but what would make more sense and be more general purpose would be to overload an ostream operator<< instead. This allows the user of the code to direct the output to the std::cout or any other convenient ostream. The declaration looks like this:

friend std::ostream &operator<<(std::ostream &out, const ConsoleTable &t); 

The implementation looks like this:

std::ostream &operator<<(std::ostream &out, const ConsoleTable &t)
{
    out << t.line(0);
    t.printRow(out, t.header);
    auto mid = t.line(1);
    if (!t.innerlines) {
        out << mid;
        mid.erase();
    } 
    for (const auto &row : t.rows) {
        out << mid;
        t.printRow(out, row);
    }
    return out << t.line(2);
}

Note that this uses a private member function printRow() to print each std::vector<std::string> for either the header or the data. It also uses the previously shown private member line function to create the upper, middle and lower lines (each just once) to print. Also, I've added a feature which can be turned on or off via the boolean variable innerlines. When set to true, it prints lines between each data row, but when set to false, it omits those inner lines and just prints each data row with no visual separator. I find this looks cleaner and also allows more data to be shown on the screen at once.

Consolidate classes

There doesn't really seem to be a strong need for a ConsoleTableRow class here. I'd suggest instead to keep the rows internally like this:

std::vector<std::vector<std::string>> rows;

Since this is just an implementation detail once we use std::initializer_lists as mentioned above (that is, the internal representation of the rows are no longer part of the interface), it could always be added if really needed.

Use namespaces where appropriate

The ConsoleTableUtils object shouldn't really be an object at all. It should be a namespace or simply make repeatString a freestanding function if it's needed at all. I'd say it's not really needed since it could just as easily be done with one of the std::string constructors directly. If needed, it could be written like this:

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

This gives us a handy and intuitive syntax:

std::string foo{"foo"};
std::cout << "String test\n" << (foo * 3) << '\n';

Don't write this->

In member functions, adding this-> everywhere only clutters up the code and makes it harder to read because this-> is implicit.

Combine parameter setting for convenience

There is a function called setTableStyle which takes a single parameter which is actually just the type of line to be used. I'd argue that the "style" might include the padding as well, and in my version, whether to print inner lines or not. Finally, the name setTableStyle is somewhat redundant since it's a member function of a table. I's shorten that to style since Table is implicit because it's a table object, and set is implicit when you pass a value. Here's how I'd write it:

void ConsoleTable::style(ConsoleTable::Style s, std::size_t padsize, bool innerlines) {
    linetype = s;
    padsize = padsize;
    innerlines = innerlines;
}
\$\endgroup\$
8
  • 1
    \$\begingroup\$ Thanks for this detailed feedback. I really appreciate it. Definitely helps me to further improve my library. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 19:21
  • \$\begingroup\$ "I'd say it's not really needed since it could just as easily be done with one of the std::string constructors directly." - the std::string constructors only allow to repeat a char a certain amount of times. And using it would require me to cast the std::string to a char on every call. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 19:47
  • 1
    \$\begingroup\$ You're right that the std::string constructor only allows duplication of characters and not strings. I've updated my answer to reflect that and to show a relatively efficient alternative function. \$\endgroup\$
    – Edward
    Commented Apr 3, 2018 at 20:26
  • \$\begingroup\$ When you started talking about the std::initializer_list you added updateWidths(); in the constructor body. What is that referring to? \$\endgroup\$
    – 766F6964
    Commented Apr 4, 2018 at 17:00
  • \$\begingroup\$ It’s a function that stores the sizes of the longest string in each column. Purely a separate function for clarity and simplicity. \$\endgroup\$
    – Edward
    Commented Apr 4, 2018 at 17:09
4
\$\begingroup\$

I suggest you to use Fluent API for ConsoleTable class.

You can use unique_ptr instead of naked pointer, then did not worry about memory leak.

I think repeatString function does not need a class, and write like alone function in namespace like namespace console_table, if you like a class, please declare repeatString as static function to prevent extra instantiation.

Please use use keyword or typedef to rename a better name for std::vector<std::string> to something like Rows. please see When should I use typedef in C++?

I think getEntry() must be right const, and return const reference.

Use enum class instead of enum.

Use const whenever possible, right const functions follow this rule too.

I prefer decouple cout from ConsoleTable and use ostream instead of. because we able to draw table in any place I want. for example in file.

Use const reference input for std::string in this situation.

In ConsoleTableRow correct guard name, you write #ifndef CONSOLETABLE_CONSOLETABLEENTRY_H instead of #ifndef CONSOLETABLE_CONSOLETABLEROW_H

I suggest you to change TableStyle to struct and injected to ConsoleTable instead of use some limit type, and create three style like above for default instantiation of this struct. in this situation, your library has been more flexibility. and users of your library can create their favorite style.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feedback. I agree with most of the points you mentioned. I'll update my code soon. Is there anything regarding the design/structure of my library that can be improved? (Bad design choices etc) \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 1:33
  • \$\begingroup\$ I applied most of your points, feel free to re-check. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 13:48
3
\$\begingroup\$

Don't really have time to go over everything in detail, but here's some things that I noticed which haven't been mentioned by sorosh_sabz yet.

  • No need to use this-> in your member functions, it's implicit. See When should I make explicit use of the this pointer? for when it's actually required.
  • Prefer using .at() instead of operator[] when accessing vectors. E.g. in your ConsoleTableRow class functions. Since you don't have direct control over the index (the user supplies it), the given index might be out of range. The .at() function will do bounds checking for you, whereas operator[] won't. So using operator[] is only recommended when you can be 100% certain there won't be an out of bounds access.
  • As sorosh_sabs mentioned, you should use std::unique_ptr instead of raw pointers, because you've got memory leaks all over the place currently. However, I don't see why you actually need pointers add all. Why not simply pass a ConsoleTableRow instead of an std::unique_ptr<ConsoleTableRow> to ConsoleTable.addRow()?
  • Needs more DRY. E.g. the function ConsoleTable.printHorizontalSeperator has a lot of code duplication.
\$\endgroup\$
5
  • \$\begingroup\$ "Since you don't have direct control over the index (the user supplies it), the given index might be out of range." You're assuming the user cannot be trusted? :) \$\endgroup\$ Commented Apr 3, 2018 at 13:30
  • 2
    \$\begingroup\$ @CrisLuengo You seem to be assuming malicious intent, I'm assuming Hanlon's razor: "Never attribute to malice that which is adequately explained by stupidity." ;). \$\endgroup\$
    – AVH
    Commented Apr 3, 2018 at 13:42
  • \$\begingroup\$ @Darhuuk thanks for your answer! You just replied when I was about to update the code. I think I fixed all of the suggestions you made. I updated my post. Also it's hard to DRY the ConsoleTable.printHorizontalSeperator method since most of the lines have minor differences. I tried my best to clean it up. Feel free to check it again. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 13:49
  • \$\begingroup\$ @Darhuuk "because you've got memory leaks all over the place currently". I switched from passing the ConsoleTableRow as Pointer to directly passing the object. What other memory leaks are you referring to? \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 13:55
  • 1
    \$\begingroup\$ @766F6964 That accounts for the majority of the leaks. The other one is due to the ConsoleTableUtils * member in ConsoleTable. As Toby Speight mentioned, you should probably make ConsoleTableUtils' function a free function or a static one. Either choice would allow you to get rid of the utils member. \$\endgroup\$
    – AVH
    Commented Apr 3, 2018 at 17:27
1
\$\begingroup\$
  • What is the ConsoleTableUtils class for? It has no members, apart from a method that probably ought to be a function instead.

  • Instead of the TableStyle enum, consider making a style type, containing the characters to be used (and provide some standard styles). We can then decouple the contents from the formatting (e.g. we might want to use different style for printing and for on-screen display).

  • Why can I write only to std::cout? Don't make it hard to write to other streams.

  • There's no checking that addRow is given a row of the right length. And there's no std::initializer_list constructor to create a row as a one-liner.

  • If we output the "middle separator" at the start of each row, we don't need to special-case the last row:

    printHorizontalSeparator(maxWidths, SEPARATOR_TOP);
    printRow(headers);
    for (row: rows) {
        printHorizontalSeparator(maxWidths, SEPARATOR_MIDDLE);
        printRow(row);
    }
    printHorizontalSeparator(maxWidths, SEPARATOR_BOTTOM);
    
  • Reduce duplication in printHorizontalSeparator: decide which four characters you'll be using (start, middle, cross and end), and after that all three code blocks collapse into one.

  • Consider supporting other character types (specifically, std::wstring is very useful).

  • Add support for per-column alignment (left, right, centre, or numeric).

  • Spelling - "Separator"

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the suggestions. The per-column alignment is a really good idea. I will definitely add it. Regarding the std::initializer_list ... I have something else in mind. I am considering varargs to make the amount of parameters variable. However, I am not sure if that's a better choice. \$\endgroup\$
    – 766F6964
    Commented Apr 3, 2018 at 17:43
  • 1
    \$\begingroup\$ Varargs is usually the poorest choice - use it when it's your only possible option. The problem is that you lose all type checking of the variable arguments. Therefore I cannot recommend it at all for new functions. \$\endgroup\$ Commented Apr 3, 2018 at 17:45

Your Answer

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

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