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(削除) I'd say it's not really needed since it could just as easily be done with one of the If needed since, it could just as easily be done with one of the std::string constructors directly. (削除ここまで)std::string constructors directly.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';
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.
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 If needed, it could be written like this:std::string constructors directly. (削除ここまで)
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';
I see a number of things that may help you imporvoeimprove your code.
This constructs the linetop, middle or bottom line and returns a single string. Here's how I wrote it:
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.
The ConsoleTableUtils object should'tshouldn'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.
In member functions, adding this-> everwhereeverywhere only clutters up the code and makes it harder to read because this-> is implicit.
I see a number of things that may help you imporvoe your code.
This constructs the line, middle or bottom line and returns a single string. Here's how I wrote it:
This code doesn't use 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.
The ConsoleTableUtils object should'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.
In member functions, adding this-> everwhere only clutters up the code and makes it harder to read because this-> is implicit.
I see a number of things that may help you improve your code.
This constructs the top, middle or bottom line and returns a single string. Here's how I wrote it:
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.
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.
In member functions, adding this-> everywhere only clutters up the code and makes it harder to read because this-> is implicit.
I see a number of things that may help you imporvoe 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 line, 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 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 should'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.
Don't write this->
In member functions, adding this-> everwhere 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;
}