Add chapter with C++ recomended practices #101
Conversation
Converted from a document originally posted on FreeCAD forum: https://forum.freecad.org/viewtopic.php?t=89913
|
There's a lot of good advice in here, so overall this is a welcome addition to the handbook. My first suggestion is to add a prominent link at the top to the C++ Core Guidelines, which are as close as C++ comes to having "official advice." To my reading, this document falls into the trap of assuming that the only useful tests are "unit" tests, of which we have essentially zero in our codebase. Very close to 100% of our test suite consists of integration and regression tests. The Python side of it by definition, and the C++ side by construction. And if you want to get into a fight on the internet, propose a definition of "unit" :). This leads to things like the admonition not to use anonymous namespaces (because you can't write tests for that code directly), versus the CG https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rs-unnamed2. Avoiding anonymous namespaces because you can't write unit tests for that code doesn't seem relevant to this codebase. Along the same lines, "New code should be delivered with unit tests" is not a recommendation we actually make here. Tests are always appreciated, but in almost no circumstances do we actually expect a "unit" test, and I can't see us suddenly moving to a situation in which we only merge code that has such tests. TDD is a specific development strategy that some people love and others hate: I don't see the FreeCAD team deciding to tell our (volunteer!) developers what process they must use to generate their code. That said, if you'd like to develop code that way, and ship the unit tests with your PR, by all means do so! I'm personally happy with any sort of test, unit or otherwise. So suggest dropping the references to, and advice about, "unit tests", and shift to language talking about tests in a more general sense. Otherwise, this looks good to me. |
|
@chennes, I'm just a messenger here as forum moderator does not wish a debate about code on the forum. Therefore I converted original document, which was deleted from the forum, to the markdown and put it there. I'm not going to get into any kind of fight, I'd like to get a conclusion. As it comes to C++ Core Guidelines, probably just put all C++ stuff into already existing c++.md and also reference it from here? |
|
I think the "Code Formatting" section will go away soon: in an ideal (and hopefully soon-achieved!) world we will have the vast majority of our code auto-formatted by clang-format and Black, so there's no point in having a handbook section about it. I do think it's completely reasonable to have a standalone "Best Practices in Language X" document, though. I'm open to suggestions for how to organize the repo, I don't feel strongly about it. |
|
How far in the future is that point of "Code Formatting" section being away? Since I didn't write that chapter I propose leaving offending commit as it is with following modification commits summarizing your reply. Would that work for you? |
There was a problem hiding this comment.
Overall this seems more like manifesto to me than useful guidelines. Proposed practices are not established in FreeCAD code base, lack concrete examples and sometimes even good explanation why it is the better way. Guidelines in such form will not be helpful to any new developer and can potentially scare people of because of language used. We should be welcoming for people and language used here is very much intimidating.
| Let’s make the data more expressive, more self-contained, and use STL | ||
| algorithm `find_if`: | ||
| {% raw %} | ||
| ```cpp | ||
| using Pair = std::pair<std::string_view, double>; | ||
| constexpr std::array<Pair, 5> prefs {{ | ||
| { "", 0 }, | ||
| { "k", 1e3 }, | ||
| { "M", 1e6 }, | ||
| { "G", 1e9 }, | ||
| { "T", 1e12 }, | ||
| }}; | ||
|
|
||
| auto comp = [&](auto pair) { return pair.second <= input; }; | ||
|
|
||
| const auto spec = std::find_if(prefs.rbegin(), prefs.rend(), comp); | ||
|
|
||
| auto prefix = spec->first; | ||
| ``` | ||
| {% endraw %} |
There was a problem hiding this comment.
I'm honestly not convinced that the find_if here is more readable. For me this code is clever, and clever code requires brain power to parse and understand how it works. First you need to understand what rbegin and rend means, second you have to lookup what does the comp function (awfuly named, btw), third you have to understand what pair.second means. On the other hand in previous example you have great names for every variable.
Not saying that utilities like std::find_if have no use, but this is not a good example. You could improve it substantially by using structured binding mentioned before, but honestly I don't think that it is worth.
FreeCAD code should be accessible for all people, and STL is very bad in terms of being easy to understand.
There was a problem hiding this comment.
That very idea of inclusiveness without borders is the root of decay, but more importantly it really does not help anyone. Working with naive code is a pain once you are familiar with using brainpower and does not help those who aren't to improve. So yes, it is C++, a choice already made and efficient C++ is full of containers and STL. The sooner people find out, the better.
There was a problem hiding this comment.
It is not inclusiveness. Simple code is simpler to maintain, that's it. I don't think that in this particular case using STL makes it simpler. You should aim to better express the intent of the code and this fails spectacularly as I need more brainpower to understand it. As I said, you can improve it quite a lot by simply naming things better, using structured binding instead of unnamed pairs - then it might be better. But if example is provided it should be clearly better.
It's not like I am against STL - contrary I think that we underuse it by quite a lot and a lot of places . If it makes the code better - sure go ahead, there are plenty of examples where it would be useful. But don't use STL constructs where doing simple loop would be good enough.
There was a problem hiding this comment.
You should aim to better express the intent of the code
But isn't this exactly what find_if does? For a reviewer it's immediately clear that something is searched in a container and the next look is at the passed function or lambda expression to see the search criterion.
With the old style by iterating in a for-loop the intention usually is less clear and it leads to some conflation of code and data. And it's one of the goals of the C++ handbook to write better code, isn't it? Furthermore, the code is also less efficient when using a for-loop because after each iteration step a new iterator object will be created.
There was a problem hiding this comment.
Sure, you know that we are searching for something but to guess what you are trying to find and what is the intended result you have to think, a lot.
If anything, this example should be more like this:
struct SIPrefixSpec {
std::string_view prefix;
double magnitude;
};
constexpr std::array<SIPrefixSpec, 5> prefixes {{
{ "", 0 },
{ "k", 1e3 },
{ "M", 1e6 },
{ "G", 1e9 },
{ "T", 1e12 },
}};
auto hasLowerMagnitude = [&](auto prefix) { return prefix.magnitude <= input; };
const auto highestPrefixSpec = std::find_if(prefixes.rbegin(), prefixes.rend(), hasLowerMagnitude);
auto prefix = highestPrefixSpec->prefix;
Then I can agree that it is a bit easier. However it still contains implicit reference to input in lambda, I'd much prefer something like this:
auto magnitudeLowerThan = [](auto value) {
return [&](auto prefix) { return prefix.magnitude <= value; };
}
const auto highestPrefixSpec = std::find_if(prefixes.rbegin(), prefixes.rend(), magnitudeLowerThan(input));
As said previously, rbegin is also non-ideal as the r is really easy to miss, I'd rather order array correctly in the first place:
constexpr std::array<SIPrefixSpec, 5> prefixes {{
{ "T", 1e12 },
{ "M", 1e6 },
{ "k", 1e3 },
{ "G", 1e9 },
{ "", 0 },
}};
Then standard begin and end could be used:
const auto highestPrefixSpec = std::find_if(prefixes.begin(), prefixes.end(), magnitudeLowerThan(input));
There was a problem hiding this comment.
Perhaps each of us is looking at the problem from different perspective. Let's look on practical example as shown in FreeCAD/FreeCAD@4a96d8c. Here, the original code clearly requires almost no brain power. Knowing basic C or even basic English is sufficient enough to understand it. Now, what could be the most common modification of that code required? Adding yet another unit probably. And that is far more straightforward with modified code. Looking to C++ as C on steroids and not object oriented language is what usually justifies all these for loops and ifelses...
Btw, that code snipped from example is humanReadableSize sitting in src/Mod/Start/App/DisplayedFilesModel.cpp
There was a problem hiding this comment.
The linked example is very good and in that case I absolutely agree that it is a big improvement. I am commenting however on example provided here which is not very good to illustrate why using find_if is better
There was a problem hiding this comment.
@kadet1090, for a start I opened FreeCAD/FreeCAD#17817 with approximately the same code we are discussing here. Feel free to review it, suggest changes. Once agreed upon, we can eventually transfer it back here as an example.
| **Every branch in code doubles the number of paths. Increases complexity, | ||
| maintenance burden**. | ||
|
|
||
| Conditionals create indentation, and often violate the “Tell-Don’t-Ask” | ||
| principle. Conditionals may indicate laziness, naive structure, not using | ||
| polymorphism, design patterns etc. | ||
|
|
||
| Simple `if else` may be better expressed as a ternary. `else` is rarely | ||
| necessary. Code will be better without it. Ternary is ok though. | ||
|
|
||
| Even modestly complex code in an `if` or `else` branch should be | ||
| extracted to a function or lambda. | ||
|
|
||
| A sequence of conditionals stepping through related variables may indicate | ||
| code and data conflation. | ||
|
|
||
| Can also apply to `switch` statements. See also: repetition, variable sets, raw loops. | ||
|
|
||
| Complicated `if else` code might benefit from converting to state machine. | ||
|
|
||
| One school of thought says a function should have no more than one conditional, |
There was a problem hiding this comment.
This paragraph is way too generic to be used in the guidelines. It feels more like manifesto than helpful guidelines. I'm also in anti-if camp but this needs more concrete examples. We need guidelines that are easy to follow and apply.
There was a problem hiding this comment.
Note, I'm just a messenger here. All this used to be more specific and all this is a direct response to the code found in FreeCAD repository. It originated from forum discussion and it came with examples. All those posts (and threads) were deleted by a moderator as offensive. There is nothing I can do about it, maintainers of FreeCAD are expected to come to conclusion here.
| `const` is a statement of intent that something is to be immutable | ||
| (not able to change). Immutability aids reliability. In Rust immutability | ||
| is the default. |
There was a problem hiding this comment.
This lacks explanation why we should make things const and why immutability is important.
There was a problem hiding this comment.
How far in the future is that point of "Code Formatting" section being away?
Here is a PR FreeCAD/FreeCAD#16077 to auto-format our code base (excluding 3rd party code). In the 2nd commit all files are listed that the pre-commit script has touched.
But one still has to go through all the files and look what changes are made because sometimes the clang-format makes the code to look worse and less readable than before. These code parts should be excluded beforehand (with clang-format off/on).
There was a problem hiding this comment.
This lacks explanation why we should make things const and why immutability is important.
Well, it helps to reduce possible mistakes by a developer. See also: https://isocpp.org/wiki/faq/const-correctness
And last but not least the compiler will be able to do better optimization.
There was a problem hiding this comment.
That's contrary to the @chennes' remark about "Code Formatting" section going away. I do agree, sometimes manual formatting looks better, but it is highly subjective how it should look like. Therefore some guidance in "Code Formatting" section is still useful. I have no strong feeling about it, I was trying to get idea, how to organize this document - and now I'm more confused :)
There was a problem hiding this comment.
My basic point is that I don't want developers (or maintainers) to have to consider, or argue about, code formatting. By saying "use clang-format and Black unless there are exceptional reasons not to" we can avoid a lot of that sort of thing. I have no problem with occasional use of the format-disabling comments, but they should be rare, and the exceptions are probably not documentable.
There was a problem hiding this comment.
Understood. But that still needs to be documented somewhere, something like "how to disable autoformat" and this still belongs to "Code Formatting" section which explains autoformatting is on by default. I'll try to add such a preparational commit about that, since I'm expecting autoformatting in use before we deal with the rest of comments :)
| Code cannot function, be understood, be edited or tested, without | ||
| its dependencies. So code free from dependencies is worth striving for. | ||
| Code and its dependencies are said to be _coupled._ | ||
|
|
||
| When different pieces of code _have the same_ dependency, they in turn | ||
| are coupled to each other! |
There was a problem hiding this comment.
Without proper dependency injection container this is way too hard to achieve. FreeCAD also very much relies on singletons. Is it a good thing? Of course not. This is however not something that can be changed easily.
This rule should be rewritten to "Do not create new singleton classes" as this is the most that we can expect from new code at this stage.
There was a problem hiding this comment.
Okay. Handbook probably deserves something like history tour. Codebase is ancient and effort is being make to modernise it. Consequence is that something what already is in the codebase is no excuse for new code sneaking in.
On practical level it depends on maintainers - this is where code enters repository. So those with commit rights need to find consensus, I'm just a random person watching new code flowing and finding it very inconsistent across different commiters. PR was opened in hope this chapter ends as something people agree upon when merging code and I still think such a discussion belongs to forum, but this is in direct contradiction what moderator thinks.
| ## design 👍 | ||
|
|
||
| > Any fool can write code that a computer can understand. Good programmers | ||
| write code that humans can understand | ||
| --- Martin Fowler | ||
|
|
||
| Something well-designed is _instantly_ understandable, and a pleasure to work | ||
| with. Programming principles developed over the last 50 years ensure | ||
| well-designed code not only runs well, but is also understandable by humans. | ||
|
|
||
| Well-designed code is adaptable, flexible, easily changed to suite new | ||
| circumstances. | ||
|
|
||
| Well-designed code is completely free from hard-coded data. | ||
|
|
||
| Understandable code can be more easily evaluated for correctness. | ||
|
|
||
| For a novice programmer many of these concepts are probably incomprehensible, | ||
| but with a little study and a determination to understand established | ||
| principles, better code will ensue. |
There was a problem hiding this comment.
This paragraph is another manifesto - it does not bring any concrete help for newcomers.
| ## inappropriate type 👎 | ||
|
|
||
| When everything is a string it is diffcult to understand what’s what. | ||
| Apply suitable abstraction. |
There was a problem hiding this comment.
Wording is extremely unhelpful, this should explain on examples and bring concrete rules like "Try to use the most specific type as you can" / "Do not use too generic types unless you really mean it".
|
|
||
| Avoid default constructors. If there is not yet a value for an object, | ||
| then don’t create it! Declare variables where they are used (not at | ||
| the start of the block like last century C). Joining declaration and |
There was a problem hiding this comment.
(not at the start of the block like last century C).
Do we really need such remarks?
| > One of the most popular features of Modern C++. | ||
| --- Jonathan Boccara, 2021 | ||
|
|
||
| A lambda (since C++11) is like a function that can be named, passed into and | ||
| out of functions. They accept parameters, can be generic (since C++ 14), do a | ||
| really good job of type deduction (auto), can be constexpr (since C++17). | ||
|
|
||
| Lambdas can capture data from enclosing scopes. | ||
|
|
||
| Lambdas enforce encapsulation, simplifying surrounding scopes. | ||
|
|
||
| Lambdas are indispensable when breaking up complex code into individual | ||
| responsibilities, perhaps as a precursor to moving to functions. Ditto | ||
| when removing repetition. | ||
|
|
||
| Whilst lambdas are quite happy with simple auto parameters, best not | ||
| to omit const or & as appropriate. Types may be required to help the IDE. |
There was a problem hiding this comment.
This lacks a good examples where lambdas are useful, and when they should be avoided.
| ```cpp | ||
| auto [name, value] = func("qty", 2); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This could also use remark about std::optional
| auto [name, value] = func("qty", 2); | ||
| ``` | ||
|
|
||
| ## repetition 👎 |
There was a problem hiding this comment.
This should note that not every code repetition is bad. Repetition of BUSINESS LOGIC is bad but sometimes it is better to have duplicated code than to couple two unrelated things together simply because their code looks similar.
There was a problem hiding this comment.
I guess most people got that by themselves... I will not answer every review comment above individually as these are revolving around lack of examples. Those would need to be artificially created as taking them from code base is no-go. To me that is not worth doing so.
Also I cannot alter this particular commit as I'm not the author. And author is not allowed to use github and his post are being deleted from forum. This was in fact the reason beyond creating this PR. We can continue after deciding, whenever it is okay to use code examples from FreeCAD. Every change agreed upon will be then introduced as additional commit against this one.
|
I don't understand what all the fuss is about. At only 10 pp long the document is, for my money, worth supporting. Why make this all so complicated? People can decide for themselves if it is useful. This discussion suggests the document has aspirations to be somehow "official". There is no evidence of that, so what is all the fuss is about? It should rather be treated as a useful adjunct. Good medicine.
I don't see the document suggesting "proposed practises". Looks to me more of a terse primer, a summary of relevant information that invites user research. Just an added reference work. I think it is completely wrong to suggest FreeCAD codebase is a worthwhile benchmark! There is whole wide world outside of FreeCAD!
I don't think it pretends to be a programming 101 lecture course.
Huh? Looks to me exactly the language of 21st century C++ programming. Surely nothing for grownups to be scared of!! A lot of the stuff in there is not even C++ specific, just good old-fashioned basic practises for any language. Are you proposing lowest common denominator sets the ceiling on ambition?
Then perhaps you might benefit from a bit of study of std library algorithms, what they are, and why they exist, and why the message nowadays is to not use raw loops. Look it up! There's miles of YT videos, web stuff. It is not secret or esoteric.
Simple code can be part of a complicated mess and become extremely difficult to maintain. Its not just the few lines of code ...
Again, if that is not understood then perhaps some broader study would be helpful. Immutability is basic stuff, in any language, even if in FreeCAD it is largely ignored. Note: US government is mandating C and C++ to be too unsafe for future government work.
I find after removing repetition and other evils like breaking Law of Demeter, code gets smaller and simpler. Except for tables of data (which benefit from formatting being turned off), the code then formats beautifully. All the more reason for code and data to be kept separate, as the document espouses. Codified data is just horrible.
Misses the whole point! The best DI container won't fix spaghetti code. Dependency injection is impossible until buried dependencies (eg singleton access eg Application->getInstance()) are brought out to the constructor. Then you have to think about the various methods of handling interfaces etc. If it wasn't done when the code was new, it is a nightmare to do later. When code is refactored to remove dependencies, everything gets simpler, better. From the forum: https://www.youtube.com/watch?v=kCYo2gJ3Y38 In C++ DI is particularly challenging, compared to many other languages, due in no small part to not having reflection. But that is no excuse for not making moves to approach it manually. DI containers have several problems themselves: They add a global, and they reduce performance (which for C++ programmers is often the holy grail).
Honestly, this freaks me out! There is almost nothing to hate about lambdas. I remember in the document it quotes them as being one of the most popular features of modern C++. There are few in FreeCAD. But there are absolutely tons of opportunities to use them, particularly in refactoring (but there doesn't tend to be much refactoring in FreeCAD - probably cause it's hard)! The document IMO does a good job outlining their usefulness.
It's right there a few lines down:
I'm sure there are a zillion things that could go in there. This is 10pp, as 3x380V said aimed squarely at observations about FreeCAD codebase. Core Guidelines is something like 500pp!
There are quotes in the document from seriously important C++ folk who strongly disagree! Repetition IS evil! Copy-Paste is for script kiddies, not serious programmers like us, right? That's enough! |
Remember the furious arguments against uniform code formatting? It got there in the end. |
|
Let's not characterize any of these discussions as "battles" -- there are no enemies here, only a group of people who want to improve FreeCAD. We are having a reasonable, professional discussion about how best to approach that task given the constraints of the project. A few comments above stray awfully close to ad hominem territory: please keep the Code of Conduct in mind as you participate here. |
|
"battle" is a very common figure of speech applied to the sway of the pendulum in a healthy debate such as this. Surely a stretch to suggest even remotely ad hominem ... |
|
I note the document has 23 sections, which is quite a lot, and is naturally blurring the discussion.
|
Converted from a document originally posted on FreeCAD forum: https://forum.freecad.org/viewtopic.php?t=89913 (reason to open PR is given by a moderator)
You can see preview here: https://www.triops.cz/fc/codeformatting/c++practices.html
While there are many threads of the forum about coding style, Modernisation is probably the most representative one, nothing reached FreeCAD documentation. This is an attempt to change that.
Comments welcome!