adambard / adambard/learnxinyminutes-docs
[C++] Constructive Criticism
- Dominant language
- Markdown
- Stars
- 12.3k
- Forks
- 3.7k
- Avg merge
- 13h 10m
- Merged PRs (30d)
- 6
Description
### Constructive criticism (I hope :/)
* A lot of these output examples use `printf` as it's previously used in the "C standard version" of the _Hello, World_ program. The examples I'm talking about are where the calls are of the form `printf("xxx\n");` where there are no format specifiers, no following `va_args`, and the string literals end with a newline character. All of these ought to be calls to `puts` from libc without any newline character on the end. Compilers actually perform a call replacement optimisation in these scenarios to replace the call with a call to `puts`, since there's less overhead compared to the code that scans the _format string_ for format specifiers.
* Stop writing `<< "\n";` when using streams, `'\n'` is a single character. It's a bit pedantic noting this but compilers won't optimise the former version (by omitting the string literal designation and simply passing `'\n'` as an immediate to the corresponding `operator<<` overload).
* At least note that `std::endl` is a `'\n'` followed by a `std::flush`, as that's important (as it's often not necessary at all, it has just caught on as a trend and the guide makes inconsistent usages of both).
* I know it's probably for making examples less convoluted, but the usage of `using namespace std;` is notoriously bad practice outwith small projects and the associated comment is vaguely misleading as well (`"// Strings are also in the namespace std (standard library)"` - so what? it's unrelated to this and there's not even an example of limited namespace prefix usage `using std::cout;`).
* There's a total gloss over the important parts of reference collapsing semantics. The guide ought to note that `const` references can bind _rvalues_ as well, hence the introduction of the `X&&` syntax, where `X` is not a template type (the rvalue reference syntax was introduced to have higher precedence for rvalues under overload resolution).
So, there's detail missing in that other types of overloads would very quickly lead to overload resolution ambiguity (for example: `func(const obj&)` and `func(obj)` wouldn't be permitted as an overload due to ambiguity for rvalues - since they both bind them); important, I feel, as the lines after this part talk about `basic_string`'s copy and move constructor, where `const obj&` and `obj&&` are the more common overloads seen for compatibility reasons (so it may be worth mentioning overload ambiguity in a larger section on: `std::move`, universal/forwarding references, and, as a result of the previous item, `std::forward`).
* The empty user-defined default constructor for `Point` isn't commonly how one would go about retaining the implicitly-generated default constructor implicitly deleted by a user-defined non-default constructor (special member generation rules); it'd be better practice to just write `Point() = default;`.
* This comment "`// Template parameters don't have to be classes:
template`" as it implies that using the keyword `class` in the context of `template ` imposes the constraint that only _class_ types can be substituted. This is not true and I feel the majority of the tutorial's usage of `class` in these contexts ought to be changed to `typename` and the single contextual difference between them should be highlighted as well (`typename` can be used for accessing type members in a dependent scope - `using X = typename Class::inner_type;`, `class` cannot.
* The usages of `std::map`'s `insert` member function are uglier than they could be. Writing `mymap.insert(pair('A',1))` is verbose compared to the flexibility other forms of initialisation lend you, `mymap.insert({'A', 1});`. Dare I say it may be worth writing some stuff about the various forms of initialisation in C++. Also, technically, the type used for the `std::pair` is incorrect. The inner `value_type` of `std::map` is actually `std::pair` and this distinction is actually very important in other contexts. For example, the notoriously cited snippet:
```
std::map m;
for(const std::pair& : m) { }
```
This type-mismatch is problematic because you're binding to a `const` reference (rvalues are permitted to be bound), so the compiler can implicitly cast to `const`, incurring the overhead of copying the pair - so a copy of the pair is made every iteration. You can see this is true if you remove the `const`, then the code won't compile. I also feel the semantic differences of using `.insert` and `operator[]` with `std::map` ought to be highlighted as well (as both are common in real-world code even though it's not a tutorial on STL semantics).
* The map iterating code is also pretty ugly as it could easily be shortened using structured bindings (if you feel like introducing them to the guide - which you may as well given there's an example of `std::tie`; otherwise at least advocate a ranged-based `for` method using `for(const auto& x : map){/**/}`); `for (const auto& [f, s] : map) std::cout << f << "->" << s << '\n';`.
* This part of the tutorial is nonsense:
```
// You can use "auto" and not worry about the type of the elements of the container
// For example:
for(auto elem: arr) {
// Do something with each element of arr
}
```
There's a ton of detail missing. You can't just use `auto` and _not worry about the type_ in the container without also considering contextual things such as the fact that this is a range-based `for` by-value; so it won't work for containers storing non-copyable types (such as move-only types).
* All these usages of `new` yet no usage of `delete` (or `delete[]`) in sight.
* The fact that `sizeof(char)` is defined _by the standard_ to be `1` might be worth explicitly noting.
* `return 0;` can be omitted at the end of `main`'s control flow (could be noted).
* Even if it's purely for illustrative purposes, the example of having two vastly-different disjoint types for tracking dogs' information is incredibly bad practice and I wouldn't advocate such a design in any guide, let alone in real software (storing IDs in a flexible vector container and then associated data in a statically-sized array with no bounds checking?). I can gather that the usage was merely to highlight lambda capturing in the comparator predicate lambda passed to `std::sort`, but it's just such a crazy scenario to devise. A more contrived and more natural example that isn't classically fragile may work better for demonstrating capturing lambdas.
I'd be happy to amend parts of the guide and submit PRs, citing back to this post (unless someone wants to dispute areas of my post for perhaps being overly-pedantic or entailing the requirement for this guide to turn into a book). I understand that this guide is quite old and has varying authorship so my points are really just intended as a way to _refine_ the content before introducing more modern approaches to the same ideas (and introducing more ideas).
Apologies if any of my points appear flippant or aren't well-articulated. I'd be happy to elaborate on any of the points I've made in the comments. I understand that it's very-challenging to attempt to condense much of C++'s intricacies into a short guide so I'd be happy to attempt to rework certain examples in the guide in future.
Contributor guide
Assessment
This issue has not been assessed yet.