Skip to main content
Explain why mixing '<' and '>' is not the best option + some propaganda. :)
Source Link
shycha
  • 171
  • 2

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]

have inconsistent bound checking, which may cause confusion when quickly screening the code.

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]

Typos, missing header
Source Link
shycha
  • 171
  • 2
ConsoleTable::ConsoleTable(std::initializer_list<std::string> aHeaders) : headers(aHeaders) {
    for (std::stringauto const& column : headers) {
        widths.push_back(column.length());
    }
}
  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.

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

#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 I it is how the C++ code is written nowadays.

ConsoleTable::ConsoleTable(std::initializer_list<std::string> aHeaders) : headers(aHeaders) {
    for (std::string column : headers) {
        widths.push_back(column.length());
    }
}
  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.

If I remember correctly <iosfwd> it should contain std::ostream

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

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 I it is how the C++ code is written nowadays.

ConsoleTable::ConsoleTable(std::initializer_list<std::string> aHeaders) : headers(aHeaders) {
    for (auto const& column : headers) {
        widths.push_back(column.length());
    }
}
  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.

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

#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.

Clarified intention
Source Link
shycha
  • 171
  • 2
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". ;)

if (index > rows.size()) {
    return false;
}
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". ;)

Source Link
shycha
  • 171
  • 2
Loading