diff --git a/doc/contributing/source/best-practices.rst b/doc/contributing/source/best-practices.rst new file mode 100644 index 000000000..226a24d19 --- /dev/null +++ b/doc/contributing/source/best-practices.rst @@ -0,0 +1,98 @@ +.. 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 `_. diff --git a/doc/contributing/source/figures b/doc/contributing/source/figures deleted file mode 120000 index 7edfd1af6..000000000 --- a/doc/contributing/source/figures +++ /dev/null @@ -1 +0,0 @@ -../figures \ No newline at end of file diff --git a/doc/contributing/figures/README b/doc/contributing/source/figures/README similarity index 100% rename from doc/contributing/figures/README rename to doc/contributing/source/figures/README diff --git a/doc/contributing/source/figures/cppcon2017-refactorOrDie-probabilityOfBug.png b/doc/contributing/source/figures/cppcon2017-refactorOrDie-probabilityOfBug.png new file mode 100644 index 000000000..7f75d67e4 Binary files /dev/null and b/doc/contributing/source/figures/cppcon2017-refactorOrDie-probabilityOfBug.png differ diff --git a/doc/contributing/source/figures/cppcon2017-refactorOrDie-scope.png b/doc/contributing/source/figures/cppcon2017-refactorOrDie-scope.png new file mode 100644 index 000000000..62c9f4157 Binary files /dev/null and b/doc/contributing/source/figures/cppcon2017-refactorOrDie-scope.png differ diff --git a/doc/contributing/source/index.rst b/doc/contributing/source/index.rst index c309abb24..5f2d48f4b 100644 --- a/doc/contributing/source/index.rst +++ b/doc/contributing/source/index.rst @@ -32,3 +32,4 @@ This document is written in `reStructuredText