Skip to content

Commit 7804717

Browse files
kadet1090hyarion
kadet1090
and
hyarion
committed
Best Practices: Organize with Code Reviewer guide
This adds a guide aimed more at reviewers that takes the guide and tries to extract from it steps that are easy to follow and check. This commit also reorganizes the structure so everything lives in a new bestpractices folder. Co-Authored-By: Benjamin Nauck <[email protected]>
1 parent 30e54ae commit 7804717

File tree

6 files changed

+244
-16
lines changed

6 files changed

+244
-16
lines changed

‎codeformatting/c++practices.md renamed to ‎bestpractices/c++practices.md

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,10 @@
22
layout: default
33
---
44

5-
# C++ coding practices
5+
# C++ practices
66

7-
Although this chapter is not FreeCAD specific, it is provided here to help both developers and code reviewers to ensure
8-
clean and easily maintainable code. The practices presented should be treated like food recipies - you can play with them, alter them - but every change should be thoughtful and intentional.
9-
10-
This document is __very__ much inspired by the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines).
11-
Most rules presented here are from this document and whenever something is not covered or you are in doubt -
12-
don't hesitate to consult it.
13-
14-
> [!NOTE]
15-
> Remember that code review is a collaborative discussion. Don’t hesitate to ask for clarification or help when needed. Reviewers can also make mistakes, the goal is to work together to refine the code to a point where everyone is satisfied.
16-
17-
While this guideline might not be consistently followed throughout all of the existing codebase, adhering to these practices moving forward will help improve the overall quality of the code and make future contributions more maintainable.
7+
Most rules presented here cover the same things as the very good [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines).
8+
Whenever something is not covered or you are in doubt - don't hesitate to consult it - it is a very good source of information.
189

1910
## Avoid anonymous namespaces
2011

@@ -466,7 +457,7 @@ constexpr auto redDogColor {"red"}; // OK
466457
```
467458
See also variable sets.
468459
469-
## Out parameters
460+
# Out parameters
470461
471462
Out parameters are _non-const, by-reference, or by-pointer_, function parameters. These are known to cause hard to find bugs.
472463
@@ -601,4 +592,4 @@ constexpr std::array<Button, numButtons> buttonDefs {{
601592
}};
602593
```
603594
604-
When in doubt, use a struct.
595+
When in doubt, use a struct - it is better to have good names than not.

‎bestpractices/codereview.md

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
---
2+
layout: default
3+
---
4+
5+
# FreeCAD - Code Review Guide
6+
7+
This document aims to provide set of good practices that should be helpful for both developers and code reviewers. They should be treated like food recipes - you can play with them, alter them - but every change should be thoughtful and intentional.
8+
9+
Just like the C++ practices - this document is __very__ much inspired by the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines).
10+
You can always consult it to learn more useful stuff.
11+
12+
While points of this guide might not be consistently followed throughout all of the existing codebase, checking if new code follows them is possible and in long term should help us immensely.
13+
14+
> [!NOTE]
15+
> Remember that code review is a collaborative discussion. Don’t hesitate to ask for clarification or help when needed. Reviewers can also make mistakes, the goal is to work together to refine the code to a point where everyone is satisfied.
16+
17+
In this document the **bolded** text will indicate how important each suggestion is.
18+
- **must** will be used for fundamental things that should be non-controversial and which you really should follow
19+
- **should** will be used for important details that will apply for vast majority of cases, there could however be valid reasons to ignore them depending on context
20+
- **could** will be used for best practices, things that you should try to follow but not following them is not an error per se
21+
22+
## Common Rules
23+
1. Consistency **must** be preferred over strict rule following. If, for example, in a given context code uses different naming scheme - it should be followed instead of one described in that document.
24+
2. The aim of Code Review **is not** to find errors in code but to ensure code quality.
25+
3. There are no dedicated reviewers, everyone **can** help with the effort!
26+
4. Reviewers **should** comment mostly on added code and discuss existing one only if making change to existing code would help the new fragment.
27+
5. Comments **could** be made to existing comments just to note that other (potentially better) soluyions are available and should be used instead when writing new code.
28+
6. Reviewer **can** hint to make more changes to the existing code in order to improve the proposed solution (for example, refactor another class so it can be used).
29+
7. Code **must** follow the Scout Rule - https://biratkirat.medium.com/step-8-the-boy-scout-rule-robert-c-martin-uncle-bob-9ac839778385 - i.e. leave code in better shape than you found it.
30+
8. PRs **must not** contain any remains of development code, like debug statements other than actual logs.
31+
9. New code **must not** introduce any new linter warnings
32+
10. Developer **should** have `pre-commit` installed and working. `pre-commit-ci` commits should be avoided.
33+
34+
## Basic Code Rules
35+
1. (C++ only) New code **must** be formatted with clang-format tool or in a way that is compatible with clang-format result if file is excluded from auto formatting.
36+
1. (Python only) Code **must** follow the PEP 8 standard and formatted with Black.
37+
3. Main execution path **should** be the least indented one, i.e. conditions should cover specific cases.
38+
4. Early-Exit **should** be preferred to prune unwanted execution branches fast.
39+
7. Global state (global variables, static fields, singletons) **should be** avoided.
40+
8. (C++ only) `enum class` **should** be preferred over normal enum whenever possible.
41+
9. (C++ only) C++ libraries/structures **should** be preferred over C ones. i.e. `std::array/vector` should be used instead of C array.
42+
10. (C++ only) C++ standard library **should** be utilized if possible, especially the `<algorithms>` one.
43+
<details>
44+
<summary>Example #1</summary>
45+
46+
Consider following code:
47+
```c++
48+
std::vector<int> vertices;
49+
50+
std::set<int> vertexSet;
51+
for (auto &s : face.getSubShapes(TopAbs_VERTEX)) {
52+
int idx = shape.findShape(s) - 1;
53+
if (idx >= 0 && vertexSet.insert(idx).second) {
54+
vertices.push_back(idx);
55+
}
56+
}
57+
```
58+
59+
You can rewrite it in a following way:
60+
```c++
61+
std::vector<int> vertices;
62+
63+
std::set<int> vertexSet;
64+
for (auto &vertex : face.getSubShapes(TopAbs_VERTEX)) {
65+
int vertexId = shape.findShape(vertex);
66+
67+
if (vertexId > 0) {
68+
vertexSet.insert(vertexId);
69+
}
70+
}
71+
72+
73+
std::copy(vertexSet.begin(), vertexSet.end(), std::back_inserter(vertices));
74+
```
75+
76+
This way you split the responsibility of computing unique set with result preparation.
77+
</details>
78+
11. (C++ only) Return values **should** be preferred over out arguments.
79+
a. For methods that can fail `std::optional` should be used
80+
b. For methods that return multiple values it may be better to either provide dedicated struct for result or use `std::tuple` with expression binding.
81+
12. If expression is not obvious - it **should** be given a name by using variable.
82+
<details>
83+
<summary>Example #1</summary>
84+
TODO: Find some good example
85+
</details>
86+
13. (C++ only) Code **should not** be put in anonymous namespace.
87+
14. All members **must** be initialized.
88+
<details>
89+
<summary>Rationale</summary>
90+
Not initialized members can easily cause undefined behaviors that are really hard to find.
91+
</details>
92+
93+
## Design / Architecture
94+
1. Each class / method / function **should** be doing only one thing and should do it well.
95+
2. Classes that do provide business logic **should** be stateless. This helps with reusability.
96+
3. Functions / Methods **should** be pure i.e. their result should only depend on arguments (and object state in case of method). This helps with reusability, predictability and testing.
97+
4. Immutable data **should** be used whenever possible, (For C++ use `const`)
98+
<details>
99+
<summary>Rationale</summary>
100+
It is much easier to reason about code that deals with data that does not change.
101+
102+
Using const modifier ensures that the object will stay unmodified and compiler will make sure that it is the case.
103+
</details>
104+
5. (C++ only) If possible `constexpr` **should** be used for storing constant data.
105+
5. Long methods **should** be split into smaller, better described ones.
106+
6. (C++ only) Defining new macros **must** be avoided, unless absolutely necessary.
107+
7. Integers **must not** be used to express anything other than numbers. For enumerations enums **must** be used.
108+
8. Code **should** be written in a way that it expresses intent, i.e. what should be done, rather than just how it is done.
109+
<details>
110+
<summary>Example #1</summary>
111+
Consider this code:
112+
```c++
113+
void setOverlayMode(OverlayMode mode)
114+
{
115+
// ... some code ...
116+
117+
QDockWidget *dock = nullptr;
118+
119+
for (auto w = qApp->widgetAt(QCursor::pos()); w; w = w->parentWidget()) {
120+
dock = qobject_cast<QDockWidget*>(w);
121+
if (dock) {
122+
break;
123+
}
124+
auto tabWidget = qobject_cast<OverlayTabWidget*>(w);
125+
if (tabWidget) {
126+
dock = tabWidget->currentDockWidget();
127+
if (dock) {
128+
break;
129+
}
130+
}
131+
}
132+
133+
if (!dock) {
134+
for (auto w = qApp->focusWidget(); w; w = w->parentWidget()) {
135+
dock = qobject_cast<QDockWidget*>(w);
136+
if (dock) {
137+
break;
138+
}
139+
}
140+
}
141+
142+
// some more code ...
143+
144+
toggleOverlay(dock, m);
145+
}
146+
```
147+
148+
It is hard to understand what is the job of the for loop inside `if (!dock)` statement.
149+
We can refactor it to a new `QWidget* findClosestDockWidget()` method for it to look like this:
150+
151+
```c++
152+
void setOverlayMode(OverlayMode mode)
153+
{
154+
// ... some code ...
155+
156+
QDockWidget *dock = findClosestDockWidget();
157+
158+
// ... some more code ...
159+
160+
toggleOverlay(dock, m);
161+
}
162+
```
163+
The findClosestDockWidget() could either be implemented as private method or an inner function using lambdas.
164+
165+
```c++
166+
auto findClosestDockWidget = []() { ... }
167+
```
168+
169+
That way reading through code of `setOverlayMode` we don't need to care about the details of finding the closest dock widget.
170+
</details>
171+
10. Boolean arguments **must** be avoided. Use enumerations instead - enum with 2 values is absolutely fine.
172+
For python boolean arguments are ok, but they **must** forced to be keyword ones.
173+
<details>
174+
<summary>example #1 (c++)</summary>
175+
consider following example:
176+
177+
```c++
178+
mapper.populate(false, it.key(), it.value());
179+
```
180+
181+
it is impossible to understand what false means without consulting the documentation or at least the method signature.
182+
instead the enum should be used:
183+
184+
```c++
185+
mapper.populate(mappingstatus::modified, it.key(), it.value());
186+
```
187+
188+
now the intent is clear
189+
</details>
190+
11. (C++ only) Code **should** prefer uniform initialization `{ }` (also called brace initialization). Prevents narrowing.
191+
12. (C++ only) Class member variables **should** be initialized at declaration, not in constructors
192+
13. Magic numbers or other literals **must** be avoided.
193+
194+
195+
## Naming Things
196+
1. Code symbols (classes, structs, methods, functions, variables...) **must** have names that are meaningful and grammatically correct.
197+
2. Variables **should not** be named using abbreviations and/or 1 letter names. Iterator variables or math related ones like `i` or `u` are obviously not covered by this rule.
198+
3. Names **must not** use the hungarian notation.
199+
4. (C++ only) Classes/Structs **must** be written in `PascalCase`, underscores are allowed but should be avoided.
200+
5. (C++ only) Class members **should** be written in `camelCase`, underscores are allowed but should be avoided.
201+
6. (C++ only) Global functions **should** be written in `camelCase`, underscores are allowed but should be avoided.
202+
7. (C++ only) Enum cases **should** use `PascalCase`
203+
204+
## Commenting the code
205+
1. Good naming **must** be preferred over commenting the code.
206+
2. Comments that describe what code does **should** be avoided, instead comments **should** explain intended result.
207+
3. All edge-cases in code **must** be described with comment describing when such edge-case can occur and why it is handled in certain way.
208+
4. All "Hacks" **must** be described with how the hack works, why it is applied and when it no longer will be needed.
209+
5. Commented code **must** be contain additional information on why it was commented out and when it is safe to remove it.

‎bestpractices/index.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
---
2+
layout: default
3+
---
4+
5+
# Best Practices
6+
7+
Although this chapter is not FreeCAD specific, it is provided here to help both developers and code reviewers to ensure
8+
clean and easily maintainable code. The practices presented should be treated like food recipes - you can play with them,
9+
alter them - but every change should be thoughtful and intentional.
10+
11+
While this guideline might not be consistently followed throughout all of the existing codebase, adhering to these practices
12+
moving forward will help improve the overall quality of the code and make future contributions more maintainable. Consider it
13+
as inspiration for the better future.
14+
15+
## For Developers
16+
If you are a developer and want to read what we consider as good code please consult practices mentioned below. These guidelines
17+
try to define what the good code is and how it should be written.
18+
19+
- For C++ see [C++ practices](./c++practices.md)
20+
- For Python see [Python practices](./pythonpractices.md)
21+
22+
## For Reviewers
23+
For reviews it is good to have a set of concrete rules that are easy to follow. If you want to learn how to review code, or
24+
simply want to learn how we check code please consult the [reviewer guide](./codereview.md).

‎bestpractices/pythonpractices.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# Python Practices
2+
3+
Work in Progress - looking for help!

‎codeformatting/index.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,6 @@ The following is the standard C++ copyright header that should be included in al
4949
***************************************************************************/
5050
```
5151

52-
### [General C++ coding practices](c++practices.md)
53-
5452
## Python
5553

5654
There is no unified Python coding style agreed upon for the FreeCAD codebase: each module currently has its own style. Developers working on an existing module should format their code in the prevailing style of the module they are working in.

‎index.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ Covers guidelines for User Experience, Interaction and Interface in FreeCAD.
1414
### [Code Formatting Guide](./codeformatting/index.md)
1515
Covers the C++ and Python code formatting guidelines.
1616

17+
### [Good Practices / Code Review Guide](./bestpractices/index.md)
18+
Covers the C++ and Python code best practices and tips for code reviewers.
19+
1720
### [Maintainers Guide](./maintainersguide/index.md)
1821
Gives guidelines to maintainers about code reviews and merge procedures.
1922

0 commit comments

Comments
 (0)