Timeline for Console-based table structure: Revisited
Current License: CC BY-SA 3.0
14 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Apr 23, 2018 at 14:41 | comment | added | shycha | @JDługosz That wasn't the case. I've explained what I meant in the two comments to 766F6964 (see above) and also updated the response. | |
| Apr 23, 2018 at 14:38 | history | edited | shycha | CC BY-SA 3.0 |
Explain why mixing '<' and '>' is not the best option + some propaganda. :)
|
| Apr 23, 2018 at 14:09 | comment | added | shycha |
@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.
|
|
| Apr 23, 2018 at 14:08 | comment | added | shycha |
@766F6964 [1/2] Sure. There is: if (rows.size() < rowIndex) in ConsoleTable &ConsoleTable::operator-=(unsigned int rowIndex) and if (index > rows.size()) in bool ConsoleTable::removeRow(unsigned int index). In one case you use > operator and in the other <. This is not visible when you write the code (it could be even more convenient to use > and < when writing -- you have another way to distinguishing two implementations), but is more apparent when reading.
|
|
| Apr 20, 2018 at 5:52 | comment | added | JDługosz |
IMHO, there is nothing wrong with a single-statement if especially if written as a precondition at the beginning of the function.
|
|
| Apr 20, 2018 at 0:28 | comment | added | 766F6964 | @shycha Can you elaborate the bound checking issue you mentioned? What exactly is the problem in my current bound checks? | |
| Apr 19, 2018 at 17:17 | comment | added | shycha | @766F6964 I thought about moving it to the implementation file. But I must've removed it during editing. Fixed now. | |
| Apr 19, 2018 at 17:17 | history | edited | shycha | CC BY-SA 3.0 |
Typos, missing header
|
| Apr 19, 2018 at 17:12 | comment | added | 766F6964 |
Regarding your header suggestion: #include <algorithm> is required because of the std::sort() function and therefore can not be removed.
|
|
| Apr 19, 2018 at 17:12 | comment | added | shycha |
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.
|
|
| Apr 19, 2018 at 16:42 | history | edited | shycha | CC BY-SA 3.0 |
Clarified intention
|
| Apr 19, 2018 at 13:42 | comment | added | Toby Speight |
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).
|
|
| Apr 19, 2018 at 13:31 | review | First posts | |||
| Apr 19, 2018 at 13:35 | |||||
| Apr 19, 2018 at 13:26 | history | answered | shycha | CC BY-SA 3.0 |