99 lines
4.7 KiB
ReStructuredText
99 lines
4.7 KiB
ReStructuredText
.. include:: replace.txt
|
|
.. heading hierarchy:
|
|
------------- Chapter
|
|
************* Section (#.#)
|
|
============= Subsection (#.#.#)
|
|
############# Paragraph (no number)
|
|
|
|
.. _Best practices:
|
|
|
|
Best practices
|
|
--------------
|
|
|
|
When writing code to be contributed to the |ns3| open source project, we
|
|
recommend following the best practices listed below.
|
|
These practices apply to different phases of software development and testing,
|
|
and are organized according to the typical application lifecycle.
|
|
|
|
Development phase
|
|
*****************
|
|
|
|
Refactor-or-die: Preliminary refactoring before introducing functional changes
|
|
==============================================================================
|
|
|
|
As presented in Mikhail Matrosov's "Refactor or die" talk in CppCon 2017 [`1`_], mixing refactoring and functional
|
|
changes is a bad practice.
|
|
|
|
It is considered a bad practice because refactoring may involve a lot
|
|
of different files, classes, API changes to accommodate new parameters, for example.
|
|
If a functional change is mixed in between, there is a higher
|
|
probability of introducing a bug. This results in more code to inspect and debug
|
|
to isolate and fix the bug. Which translates to unproductive work to isolate and
|
|
fix the bug, which results in extra hours, working on weekends, etc, etc.
|
|
|
|
The figure below depicts the scope differences in refactoring and functional changes.
|
|
|
|
.. figure:: figures/cppcon2017-refactorOrDie-scope.png
|
|
:align: center
|
|
|
|
Impact of different scope changes (inverted triangle), and scopes affected
|
|
by refactoring and functional changes. [`1`_]
|
|
|
|
This is why refactoring should be done as a preliminary step,
|
|
before functional changes. This will drastically reduce the number
|
|
of lines of code which may have introduced a potential bug,
|
|
saving development/maintenance time in the long run.
|
|
|
|
The figure below depicts the effects of mixed refactoring AND functional changes (left orange rectangle),
|
|
versus the preliminary refactoring (right green rectangle) PLUS functional changes (right orange rectangle).
|
|
|
|
.. figure:: figures/cppcon2017-refactorOrDie-probabilityOfBug.png
|
|
:align: center
|
|
|
|
Assume the existence of a bug kills you. If you mix functional and refactoring changes, the number
|
|
of lines potentially containing a bug increases. As such, the probability of dying is unnecessarily
|
|
augmented. If you do not mix these changes, the probability of dying is significantly reduced. [`1`_]
|
|
|
|
Compile-or-die: Individual commit compilation and testing
|
|
#########################################################
|
|
|
|
Since preliminary refactoring is not always trivial, due to the potentially unknown requirements,
|
|
it is very unlikely to completely avoid mixing refactoring and functional changes.
|
|
However, these changes can and should be separated before merging them upstream.
|
|
|
|
This can be achieved through commit history rewriting, which can also introduce new bugs,
|
|
if commits are not properly rewritten.
|
|
|
|
A suggested practice is to at least compile and run tests for every single commit
|
|
in the branch before merging it. This can be accomplished by checking out each commit,
|
|
hard-resetting to that commit, reconfiguring the project and running test.py.
|
|
|
|
This can be done automatically using `./ns3 --compile-or-die base_commit head_commit`.
|
|
|
|
Note: ALWAYS back up your ns-3 directory and sync your local branches with the remote server.
|
|
|
|
.. sourcecode:: console
|
|
|
|
~ns-3-dev/$ ./ns3 --compile-or-die 757a2bfc2abb5f3584593609434c40e5ac678e8e 5a30398332d70646279285a2ef8997cea0ed9e43
|
|
Compile-or-die with commits: ['757a2bfc2abb5f3584593609434c40e5ac678e8e', '5a30398332d70646279285a2ef8997cea0ed9e43']
|
|
Testing commit 757a2bfc2abb5f3584593609434c40e5ac678e8e
|
|
Testing commit 5a30398332d70646279285a2ef8997cea0ed9e43
|
|
|
|
In case there are uncommitted changes, the script won't continue to prevent potential data loss.
|
|
In case there is no associated branch with the current git head, a backup branch will be created
|
|
to prevent data loss.
|
|
Then it will create a new temporary test branch, that will be used to checkout and test each
|
|
commit between base and head commits.
|
|
|
|
If there is no error message, all commits succeed. Which only means there is no issue preventing
|
|
compilation or causing one of the existing tests to fail. That is, we have anecdotal evidence that
|
|
refactoring was done correctly.
|
|
|
|
Two branches are created automatically: the current HEAD is tagged as ``compileOrDieBackup`` if not currently tagged,
|
|
and the commit being currently tested is tagged as ``compileOrDieTest``. These won't be cleaned automatically to
|
|
prevent potential data loss. So users should verify and delete tags manually.
|
|
|
|
.. _1:
|
|
|
|
[1] Mikhail Matrosov. Refactor or die. CppCon 2017. Available in `YouTube <https://youtu.be/fzmjXK9JZ9o>`_.
|