uibk_703807-Advanced-CXX/assignment1/feedback.md

Feedback

• Use an auto formatting tool!!11!

  • Reading inconsistently formatted code gets cumbersome over long periods of time.
  • Prefer tabs for indentation, see https://www.reddit.com/r/javascript/comments/c8drjo.
  • Personally I prefer a tab-size ≥ 4.

• Check return values!!! (especially system())

  • I hope I don't have to explain why.
  • The exit-code of an application is often used to indicate failure.
  • Your application should also adhere to this.

• Remove commented-out code.

  • You are probably using a version control system anyway.
  • At least keep them out of the final submission.

• Avoid putting lots of logic in header files.

  • You have to do this often enough when using templates.
  • It negatively impacts your development cycle; changes to a header usually cause a large number of translation units to get re-compiled.
  • You loose some benefits of shared libraries.

• Avoid overusing libraries.

  • By introducing library boundaries to your code, you create additional interfaces that may require additional effort to maintain. As always, ensure this is worth the tradeoff.
  • Look at typical open-source libraries to get a feeling for how big a typical library actually is.
  • For the size of Boost, this makes sense; for lit sub-commands, not so much.

• Avoid classes with only static member functions.

  • While possible, this is kind of an anti-pattern in C++.
  • Use namespaces instead.

• Nesting depth / length of functions.

  • Keeping functions short and linear enhances readability of your code a lot.
  • See Linux Kernel Coding Style.

• Naming conventions.

  • Consider Core Guidelines.
  • Consider Google C++ Style Guide
  • Adhere to what-ever style a code-base is already using.
  • Here my personal conventions for reference:
    • Types: PascalCase
    • Functions: camelCase
    • Constants: ALL_CAPS
    • Variables: camelCase
    • Function-like macros: camelCase
    • Other macros: ALL_CAPS
    • Namespaces: snail_case
      • Trying to avoid underscores
    • enum class members: PascalCase
    • enum members: ALL_CAPS
      • Prefixed with enum name (e.g. PRESENT_MODE_IMMEDIATE, PRESENT_MODE_MAILBOX)
    • Private member variables prefixed with m_
      • This allows getters to be named without the get prefix (e.g. Vec3 position() const;).
      • Setters get prefixed with set (e.g. void setPosition(const Vec3 &);).

• Leverage RAII!

  • No need for .close() or alike with classes like std::ifstream.
  • Consider wrapping non-RAII objects with dedicated classes or std::unique_ptr with a custom deleter.

• Do not use .h for C++ header files.

  • C++ header files typically cannot be used by C projects, this should be highlighted by the file extension.
  • Only use .h if that header is meant to be used in C projects.
  • I am aware of the corresponding core guideline.
  • Personally, I prefer .hpp and .cpp.

• Include order

  • See Google C++ Style Guide.
  • Only use < > for system-wide installed libraries, use " " otherwise.

• Member order

  • See Google C++ Style Guide.
  • Show the public interface first.
  • Be consistent across the code-base.

• Header guards

  • See Core Guidelines
  • Personally, I am okay with #pragma once but the argument of not being standard conform stands.
  • A header guard should span the entire file (maybe except the license header).

• -Werror

  • Do not add this by default.
  • Newer compiler versions my emit warnings, causing builds to break.
  • This can potentially impact other people.
  • Only have this enabled on continuous integration to enforce warning-free code on contributions.

• Unused parameters

  • In C++ just omit the name; no more warning.

• Use namespaces!

  • Do not prefix symbols with the project name as you'd do in C.
  • Prefer at least one top-level namespace (e.g. lit).
  • Do not hesitate adding sub-namespaces (detail is rather common for in-file utility functions).
  • Namespaces commonly model the directory structure of a project.

• Avoid using namespace inside header files.

  • I've already talked about this on stream.
  • Personally, I avoid using namespace inside headers.
  • Be considerate.
  • Keep namespace aliases inside your namespace.
    • namespace fs = std::filesystem; is fine in a header, but not in global scope.

• Use exceptions as error reporting for library functions.

  • A library function should never terminate the whole application.
  • Throw a custom error indicating failure, or resort to std::runtime_error.

• Libraries should not directly use stdin / stdout / stderr.

  • Input / output should always be done via function arguments / return value.
  • Use some configurable logging mechanism.

• Use enum class over enum, over preprocessor define.

  • enum class gets type-checked and is therefore preferred.
  • enum is still valid as it allows you to combine multiple members out-of-the-box (flag enum).
  • Do not use preprocessor defines as alternatives to enums.
    • Modern compilers emit debug symbols and warnings for enums, but not for preprocessor defines.

• Avoid std::endl.

  • Just prefer \n.
  • std::endl also flushes the stream which can cause performance issues.

• Avoid low-level functions like strcmp.

  • Usually it's better to convert your data to better typed C++ objects (e.g. std::string)
    • With std::string you can just write myString == "foo".
  • Only use low-level functions directly when encountering performance issues.

• Consider std::string_view.

  • This can prevent unnecessary conversions to std::string from C strings.
  • Take std::string_view by value.
  • Only use std::string_view as function argument.
  • String may not be null-terminated!

• Do not use symbols prefixed with _ or __ (e.g. std::__cxx11::string).

  • Two underscores are reserved for compiler / standard library internals.
    • These may differ between compiler and versions; therefore, do not use them.
  • Symbols with a single underscore as prefix are to be viewed as private or internal.
    • Do not use this pattern yourself, it's often unnecessary.
    • Putting stuff into a detail or anonymous namespace is the way to go.

• Use Out-of-source builds!

  • Do not pollute your source files with build artifacts.
  • Out-of-source builds allow you to have different build configurations side-by-side.
  • Build folders should not be tracked by your version control system (use .gitignore).

Project structure

Single Library

This structure is preferred for projects that focus around a single library. There can be multiple executables which use this library.

.
├── apps/                   # All executables go here, usually one .cpp file per executable.
│   ├── myapp1.cpp
│   └── myapp2.cpp
├── cmake/                  # Contains all the project specific CMake modules.
├── docs/                   # Handwritten documentation goes here.
├── include/                # This includes all public headers defining the interface of the library.
│   └── mylib/              # If there is more than one header file, put them in a folder with the library's name.
│       ├── bar.hpp
│       └── foo.hpp
├── src/                    # Contains all sources (and private headers) of the library.
│   ├── bar.cpp
│   ├── foo.cpp
│   └── utils.hpp
├── tests/                  # Commonly contains unit tests. Adjust structure as needed.
│   ├── bar_test.cpp
│   └── foo_test.cpp
├── vendor/                 # All third party libraries that are maintained along with this project go here.
└── CMakeLists.txt

Separating the public headers clarifies which elements are part of the library's interface and which are implementation details. Furthermore, upon installing the library, we can simply copy the contents of include to the corresponding installation target.

Multiple Libraries

This is very similar to the single library case. Just replicate include, src, tests for each library.

.
├── apps/
│   ├── myapp1.cpp
│   └── myapp2.cpp
├── cmake/
├── docs/
├── mylib1/
│   ├── include/
│   │   └── mylib1/
│   ├── src/
│   └── tests/              # Contains only tests specific to mylib1.
├── mylib2/
│   ├── include/
│   │   └── mylib2/
│   ├── src/
│   └── tests/
├── tests/                  # Contains tests that utilize more than one library.
├── vendor/
└── CMakeLists.txt

Framework

Sometimes the library approach with its clear separation between public interface and implementation details does not fit the project. Yet you may still keep some form of separation between components.

For creating a video game, a common approach is to divide code into game specific code and engine code. Engine code deals with low-level elements and provides (often generic) tools, while the game specific code typically covers one dedicated use-case. Building the engine as a library is certainly a possibility, but I suggest keeping the border fuzzy and allowing game code to handle implementation details.

.
├── cmake/
├── docs/
├── myapp1/                 # Always use dedicated folders here. With this approach applications get bigger quickly.
│   └── myapp1.cpp
├── myapp2/
│   └── myapp2.cpp
├── myframework/
│   ├── bar.cpp
│   ├── bar.hpp
│   ├── foo.cpp
│   └── foo.hpp
├── tests/
│   ├── bar_test.cpp
│   └── foo_test.cpp
├── vendor/
└── CMakeLists.txt

Essentially we ditch the include directory and just keep headers next to the source files. For simplicity we can use the whole repository as include path, giving all translation units access to everything. This is a very open approach that may only fit some projects.

You'd then use #include "myframework/foo.hpp" for instance.

Consider watching John Romero's talk at 2016 GDC Europe regarding some paradigms to follow during development.