From 0bb23df27b2a75149cdcab5760edcef02760a496 Mon Sep 17 00:00:00 2001 From: Teon Banek <teon.banek@memgraph.io> Date: Wed, 1 Aug 2018 17:15:10 +0200 Subject: [PATCH] Update our code conventions and add required reading Reviewers: mtomic, buda, ipaljak, mpetricevic, mferencevic, vkasljevic, mculinovic, msantl Reviewed By: buda Differential Revision: https://phabricator.memgraph.io/D1525 --- docs/dev/cpp-code-conventions.md | 77 +++++++++++++++-- docs/dev/other-code-conventions.md | 15 ++++ docs/dev/quick-start.md | 10 +++ docs/dev/required-reading.md | 129 +++++++++++++++++++++++++++++ 4 files changed, 224 insertions(+), 7 deletions(-) create mode 100644 docs/dev/other-code-conventions.md create mode 100644 docs/dev/required-reading.md diff --git a/docs/dev/cpp-code-conventions.md b/docs/dev/cpp-code-conventions.md index 8610a87ed..b5582bf6d 100644 --- a/docs/dev/cpp-code-conventions.md +++ b/docs/dev/cpp-code-conventions.md @@ -5,15 +5,80 @@ C++ code. ## Code Style -Memgraph uses the Google Style Guide for C++ in most of its code. You should -follow them whenever writing new code. The style guide can be found -[here](https://google.github.io/styleguide/cppguide.html). +Memgraph uses the +[Google Style Guide for C++](https://google.github.io/styleguide/cppguide.html) +in most of its code. You should follow them whenever writing new code. + +### Often Overlooked Style Conventions + +#### Pointers & References + +References provide a shorter syntax for accessing members and better declare +the intent that a pointer *should* not be `nullptr`. They do not prevent +accessing a `nullptr` and obfuscate the client/calling code because the +reference argument is passed just like a value. Errors with such code have +been very difficult to debug. Therefore, pointers are always used. They will +not prevent bugs but will make some of them more obvious when reading code. + +The only time a reference can be used is if it is `const`. Note that this +kind of reference is not allowed if it is stored somewhere, i.e. in a class. +You should use a pointer to `const` then. The primary reason being is that +references obscure the semantics of moving an object, thus making bugs with +references pointing to invalid memory harder to track down. + +[Style guide reference](https://google.github.io/styleguide/cppguide.html#Reference_Arguments) + +#### Constructors & RAII + +RAII (Resource Acquisition is Initialization) is a nice mechanism for managing +resources. It is especially useful when exceptions are used, such as in our +code. Unfortunately, they do have 2 major downsides. + + * Only exceptions can be used for to signal failure. + * Calls to virtual methods are not resolved as expected. + +For those reasons the style guide recommends minimal work that cannot fail. +Using virtual methods or doing a lot more should be delegated to some form of +`Init` method, possibly coupled with factory functions. Similar rules apply to +destructors, which are not allowed to even throw exceptions. + +[Style guide reference](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors) ### Additional Style Conventions -Code style conventions which are left undefined by Google are specified here. +Old code may have broken Google C++ Style accidentally, but the new code +should adhere to it as close as possible. We do have some exceptions +to Google style as well as additions for unspecified conventions. -#### Template parameter naming +#### Using C++ Exceptions + +Unlike Google, we do not forbid using exceptions. + +But, you should be very careful when using them and introducing new ones. They +are indeed handy, but cause problems with understanding the control flow since +exceptions are another form of `goto`. It also becomes very hard to determine +that the program is in correct state after the stack is unwound and the thrown +exception handled. Other than those issues, throwing exceptions in destructors +will terminate the program. The same will happen if a thread doesn't handle an +exception even though it is not the main thread. + +[Style guide reference](https://google.github.io/styleguide/cppguide.html#Exceptions) + +#### Assertions + +We use `CHECK` and `DCHECK` macros from glog library. You are encouraged to +use them as often as possible to both document and validate various pre and +post conditions of a function. + +`CHECK` remains even in release build and should be preferred over it's cousin +`DCHECK` which only exists in debug builds. The primary reason is that you +want to trigger assertions in release builds in case the tests didn't +completely validate all code paths. It is better to fail fast and crash the +program, than to leave it in undefined state and potentially corrupt end +user's work. In cases when profiling shows that `CHECK` is causing visible +slowdown you should switch to `DCHECK`. + +#### Template Parameter Naming Template parameter names should start with capital letter 'T' followed by a short descriptive name. For example: @@ -63,11 +128,9 @@ containing 3 slashes (`///`). Take a look at the 2 examples below. ### Line Comment ```cpp -/// /// One sentence, brief description. /// /// Long form description. -/// ``` If you only have a brief description, you may collapse the documentation into diff --git a/docs/dev/other-code-conventions.md b/docs/dev/other-code-conventions.md new file mode 100644 index 000000000..21d8a6f0a --- /dev/null +++ b/docs/dev/other-code-conventions.md @@ -0,0 +1,15 @@ +# Other Code Conventions + +While we are mainly programming in C++, we do use other programming languages +when appropriate. This chapter describes conventions for such code. + +## Python + +Code written in Python should adhere to +[PEP 8](https://www.python.org/dev/peps/pep-0008/). You should run `flake8` on +your code to automatically check compliance. + +## Common Lisp + +Code written in Common Lisp should adhere to +[Google Common Lisp Style](https://google.github.io/styleguide/lispguide.xml). diff --git a/docs/dev/quick-start.md b/docs/dev/quick-start.md index 9aa4eec36..570e9053a 100644 --- a/docs/dev/quick-start.md +++ b/docs/dev/quick-start.md @@ -76,3 +76,13 @@ To make extra sure, run the unit tests: If you have any trouble running the above commands, contact your nearest developer who successfully built Memgraph. Ask for help and insist on getting this document updated with correct steps! + +## Next Steps + +Familiarise yourself with our code conventions: + + * [C++ Code](cpp-code-conventions.md) + * [Other Code](other-code-conventions.md) + +Take a look at the list of [required reading](required-reading.md) for +brushing up on technical skills. diff --git a/docs/dev/required-reading.md b/docs/dev/required-reading.md new file mode 100644 index 000000000..128360fac --- /dev/null +++ b/docs/dev/required-reading.md @@ -0,0 +1,129 @@ +# Required Reading + +This chapter lists a few books that should be read by everyone working on +Memgraph. Since Memgraph is developed primarily with C++, Python and Common +Lisp, books are oriented around those languages. Of course, there are plenty +of general books which will help you improve your technical skills (such as +"The Pragmatic Programmer", "The Mythical Man-Month", etc.), but they are not +listed here. This way the list should be kept short and the *required* part in +"Required Reading" more easily honored. + +Some of these books you may find in our office, so feel free to pick them up. +If any are missing and you would like a physical copy, don't be afraid to +request the book for our office shelves. + +Besides reading, don't get stuck in a rut and be a +[Blub Programmer](http://www.paulgraham.com/avg.html). + +## Effective C++ by Scott Meyers + +Required for C++ developers. + +The book is a must-read as it explains most common gotchas of using C++. After +reading this book, you are good to write competent C++ which will pass code +reviews easily. + +## Effective Modern C++ by Scott Meyers + +Required for C++ developers. + +This is a continuation of the previous book, it covers updates to C++ which +came with C++11 and later. The book isn't as imperative as the previous one, +but it will make you aware of modern features we are using in our codebase. + +## Practical Common Lisp by Peter Siebel + +Required for Common Lisp developers. + +Free: http://www.gigamonkeys.com/book/ + +We use Common Lisp to generate C++ code and make our lives easier. +Unfortunately, not many developers are familiar with the language. This book +will make you familiar very quickly as it has tons of very practical +exercises. E.g. implementing unit testing library, serialization library and +bundling all that to create a mp3 music server. + +## Effective Python by Brett Slatkin + +(Almost) required reading for Python developers. + +Why the "almost"? Well, Python is relatively easy to pick up and you will +probably learn all the gotchas during code review from someone more +experienced. This makes the book less necessary for a newcomer to Memgraph, +but the book is not advanced enough to delegate it to +[Advanced Reading](#advanced-reading). The book is written in similar vein as +the "Effective C++" ones and will make you familiar with nifty Python features +that make everyone's lives easier. + +# Advanced Reading + +The books listed below are not required reading, but you may want to read them +at some point when you feel comfortable enough. + +## Design Patterns by Gamma et. al. + +Recommended for C++ developers. + +This book is highly divisive because it introduced a culture centered around +design patterns. The main issues is overuse of patterns which complicates the +code. This has made many Java programs to serve as examples of highly +complicated, "enterprise" code. + +Unfortunately, design patterns are pretty much missing +language features. This is most evident in dynamic languages such as Python +and Lisp, as demonstrated by +[Peter Norvig](http://www.norvig.com/design-patterns/). + +Or as [Paul Graham](http://www.paulgraham.com/icad.html) put it: + +``` +This practice is not only common, but institutionalized. For example, in the +OO world you hear a good deal about "patterns". I wonder if these patterns are +not sometimes evidence of case (c), the human compiler, at work. When I see +patterns in my programs, I consider it a sign of trouble. The shape of a +program should reflect only the problem it needs to solve. Any other +regularity in the code is a sign, to me at least, that I'm using abstractions +that aren't powerful enough-- often that I'm generating by hand the expansions +of some macro that I need to write +``` + +After presenting the book so negatively, why you should even read it then? +Well, it is good to be aware of those design patterns and use them when +appropriate. They can improve modularity and reuse of the code. You will also +find examples of such patterns in our code, primarily Strategy and Visitor +patterns. The book is also a good stepping stone to more advanced reading +about software design. + +## Modern C++ Design by Andrei Alexandrescu + +Recommended for C++ developers. + +This book can be treated as a continuation of the previous "Design Patterns" +book. It introduced "dark arts of template meta-programming" to the world. +Many of the patterns are converted to use C++ templates which makes them even +better for reuse. But, like the previous book, there are downsides if used too +much. You should approach it with a critical eye and it will help you +understand ideas that are used in some parts of our codebase. + +## Large Scale C++ Software Design by John Lakos + +Recommended for C++ developers. + +An old book, but well worth the read. Lakos presents a very pragmatic view of +writing modular software and how it affects both development time as well as +program runtime. Some things are outdated or controversial, but it will help +you understand how the whole C++ process of working in a large team, compiling +and linking affects development. + +## On Lisp by Paul Graham + +Recommended for Common Lisp developers. + +Free: http://www.paulgraham.com/onlisp.html + +An excellent continuation to "Practical Common Lisp". It starts of slow, as if +introducing the language, but very quickly picks up speed. The main meat of +the book are macros and their uses. From using macros to define cooperative +concurrency to including Prolog as if it's part of Common Lisp. The book will +help you understand more advanced macros that are occasionally used in our +Lisp C++ Preprocessor (LCP).