# FreeCAD - Code Review Book

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 recipies - you can play with them, alter them - but every change should be thoughtful and intentional.

:::warning
Remember that code review is discussion, don't hasistate to ask for clarification, help etc. Reviewer may be wrong and the goal is to make code to a point where all parties all happy.
:::

In this document the **bolded** text will indicate how important each suggestion is.
- **must** will be used for fundamental things that should be non-controversial and which you really should follow
- **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
- **could** will be used for best practices, things that you should try to follow but not following them is not an error per se

## Common Rules
1. Consistency **must** be preferred over strict rule following. If, for example, in a given context code uses different naming scheme - follow it instead of one described in that document.
2. The aim of Code Review **is not** to find errors in code but to ensure code quality.
3. Reviewers **should** comment mostly on added code and discuss existing one only if making change to existing code would help the new fragment.
4. Comments **could** be made to existing comments just to note that other (potentially better) soluions are available and should be used instead when writing new code.
5. Reviewer **can** hint to make more changes to existing code in order to improve the propose solution (for example, refactor another class so it can be used)
6. 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.
7. PRs **must not** contain any remaints of development code, like debug statements other than actual logs.

## Basic Code Rules
1. 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.
2. New code **must not** introduce any new linter warnings
3. Main execution path **should** be the least indeted one, i.e. conditions should cover specific cases.
4. Early-Exit **should** be preferred to prune unwanted execution branches fast.
5. ?? Classes that do provide business logic **should** be stateless. This helps with reusability.
6. ?? Functions / Methods **should** be pure i.e. they result should only depend on arguments (and object state in case of method). This helps with reausability, predictability and testing.
7. Global state (global variables, static fields, singletons) **should be** avoided.
8. Coude **should** be written in a way that it expresses intent, not method. Instead of writing loop in place prefer to create helper method that describes what the loop does.
:::spoiler More information
Consider this code:

        void setOverlayMode(OverlayMode mode)
        {
            // ... some code ...

            QDockWidget *dock = nullptr;
            
            for (auto w = qApp->widgetAt(QCursor::pos()); w; w = w->parentWidget()) {
                dock = qobject_cast<QDockWidget*>(w);
                if (dock) {
                    break;
                }
                auto tabWidget = qobject_cast<OverlayTabWidget*>(w);
                if (tabWidget) {
                    dock = tabWidget->currentDockWidget();
                    if (dock) {
                        break;
                    }
                }
            }

            if (!dock) {
                for (auto w = qApp->focusWidget(); w; w = w->parentWidget()) {
                    dock = qobject_cast<QDockWidget*>(w);
                    if (dock) {
                        break;
                    }
                }
            }

            // some more code ...

            toggleOverlay(dock, m);
        }

It is hard to understand what is the job of the for loop inside `if (!dock)` statement.
We can refactor it to a new `QWidget* findClosestDockWidget()` private method for it to look like this:

        void setOverlayMode(OverlayMode mode)
        {
            // ... some code ...

            QDockWidget *dock = findClosestDockWidget();

            // ... some more code ...

            toggleOverlay(dock, m);
        }

That way reading through code of `setOverlayMode` we don't need to care about the details of finding the closest dock widget.
:::
9.

## Commenting the code
1. Good naming things **must** be preferred over commenting the code.
2. Comments that describe what code does **should** be avoided, instead comments **should** explain intended result.
3. All edge-cases in code **must** be described with comment describing when such edge-case can occour and why it is solved in certain way.
4. All "Hacks" **must** be described with how the hack works, why it is applied and when it no longer will be needed.
5. Commented code **must** be contain additional information on why it was commented out and when it is safe to remove it.

## Naming Things
1. Code symbols (classes, structs, methods, functions, variables...) **must** have names that are meaningful and gramatically correct.
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.
3. Names **must not** use the hungarian notation.
4. Classes/Structs **must** be written in `PamelCase`, underscores are allowed but should be avoided.
5. Class members should be written in `camelCase`, underscores are allowed but should be avoided.
6. Global functions should be written in `camelCase`, underscores are allowed but should be avoided.
7.