-
Notifications
You must be signed in to change notification settings - Fork 32
BALL Coding Style
All BALL files use a tab width of two. Use the command set tabstop=2 in vi or set-variable tab-width 2 if you are using emacs. The standard file header should automatically set these values
for vi or emacs (see below).
Curly braces should be put on their own line. Matching pairs of opening and closing curly braces should
be set to the same column.
while (continue == true)
{
for (int i = 0; i < 10; i++)
{
...
}
...
if (x < 7)
{
....
}
}Always use braces around a block. The main reason for this rule is to avoid constructions like:
if (isValid(a))
return 0;which might be changed to something like
if (isValid(a))
error = 0;
return 0;The resulting errors are hard to find. There are two ways to avoid these problems: (a) always use braces around a block (b) write everything in a single line. We recommend method (a). However, this is mainly a question of personal style, so no explicit checking is performed to enforce this rule.
Each BALL class has to provide the following minimal interface:
/** Class documentation...
*/
class Test
{
public:
// clone method (implemented automatically with the CREATE macro)
CREATE(Test)
// default ctor
Test();
// copy ctor
Test(const Test& test);
// destructor
virtual ̃Test();
// assignment operator
Test& operator = (const Test& test);
};Variable names are all lower case letters. Distinguished parts of the name are separated using underscores "_". If parts of the name are derived from common acronyms (e.g. PDB) they should be in upper case. Private or protected member variables of classes are suffixed by an underscore. No prefixing or suffixing is allowed to identify the variable type - it leads to completely illegible documentation and overly long variable names.
Example 1:
-
String PDB_atom_name;(using an abbreviation) -
int counter_;(private class member) -
bool is_found;(ordinary variable)
Class names and type names always start with a capital letter. Different parts of the name are separated by capital letters at the beginning of the word. No underscores are allowed in type names and class names, except for the names of protected types and nested classes which are suffixed by an underscore.
Example 2:
class PDBFile-
class ForwardIteratorTraits_(private nested class definition)
Header and source files should be named after the class they contain. Each file should contain a single
class only, although exceptions are possible for extreme light-weight classes. The file names start with a lower-case letter (for historical reasons). An exception to the rule are file names starting with common
acronyms. Hence, the file containing the AtomContainer class would be named atomContainer.C.
The file containing the PDBRecord class would be named PDBRecord.C.
Function names (including class method names) always start with a lower case letter. Parts of the name are separated using capital letters (as are class and type names). The argument of void functions is omitted in the declaration and the definition. If function arguments are pointers or references, the pointer or reference qualifier is appended to the variable type (without separating blanks). It should not prefix the variable name.
The variable names used in the declaration have to be the same as in the definition. They should be as short as possible, but nevertheless comprehensible. If arguments are not used in the implementation of the function, they have to be commented out (to avoid compiler warnings).
Example 3:
int countAtoms()bool isBondedTo(const Atom& atom)bool doSomething(int i, String& /* name */)
Each BALL class has to be documented using Doxygen. The documentation is inserted in Doxygen format in the header file in which the class is defined. Documentation includes the description of the class, of each method, type declaration, enum declaration, constant, and member variable.
In order to obtain a structured and comprehensible documentation, methods are grouped by their function. Each class usually uses one or more of the following categories to group its methods:
- Type Definitions: This group should contain all typedefs of the class (except for private or protected ones).
- Exceptions: All class specific exceptions are defined as nested classes in this group.
- Enums: This group usually contains enums or constants defined via enums such as properties.
- Constructors and Destructor: This group includes all constructors and the destructor as well as related methods such as clear or destroy.
- Persistence:
This group is present in each class derived from
PersistentObjectand always contains the two methodspersistentWriteandpersistentRead. - Converters: If an object is convertible into another class an explicit converter should be defined inside this group.
- Accessors: Accessors are all methods that access or modify attributes of the class.
- Predicates: Predicates are functions returning a boolean value. The names of predicates should start with is or has if applicable. Comparison operators are also predicates and should appear inside this group.
- Debugging and Diagnostics: This group should contain all methods used for the debugging of the class (for example the dump methods).
- Attributes: This group should contain all public class attributes. Protected and private attributes should be documented in the corresponding groups Protected Attributes or Private Attributes. However, private members do not occur in the documentation by default.
These section names are intended as a rule-of-thumb only. There are many cases where other section headings are more intuitive. However, if any of the above sections apply, they should be used. The sections should also appear in this order.
The code for each .C file has to be commented. Each piece of code in BALL has to contain at least 5%
of comments. The use of // instead of C style comments (/* */) is recommended to avoid problems
arising from nested comments. Comments should be written in plain English and describe the functionality
of the next few lines.
To check whether a header or source file fulfills this requirement, use the script check coding in source/config. Invoke check coding with the filename to check (or a list of files) and it will print the percentage of comments (including Doxygen comments) for each file.
Object persistence is a serious problem in C++. There are mainly three concepts that are responsible for the trouble: 1.multiple inheritance from a common base class leading to virtual inheritance 1.static member variables 1.void pointers
To start at the end, void pointers are very simple to handle: avoid them! Pointers are only valid inside a single instance of a program, they have no meaning for a different instance or even another machine. Any code dealing with void pointers cannot be made persistent.
Static member variables can cause trouble, if the behaviour of a class depends on the state of this
variable. There is no general rule on how to handle these variables. However, in most cases everything
works fine if you simply ignore them. The only case where a static member variable had to be considered
when implementing object persistence for the BALL kernel classes, was the internal counter of the class
Object. This counter is incremented by one for each instance of Object created. It is used to uniquely
identify objects and to define a linear order on these objects (this is the object’s handle). If this handle had
been treated as other variables when reading/writing persistent objects, reading an object could have lead to
object handles occuring twice. Persistent reading for objects of type Object was therefore implemented
to ignore the value. Objects that are read get handles according to the order in which they are read and
created. This leads to a different order of the handles but prevents handles from occuring twice. Whether
static variables have to be read/written or not has to be decided on a case-by-case basis.
Multiple inheritance is the most serious problem. All persistent objects have to be derived from exactly
one persistent object class - in this case Composite. The support classes like PropertyManager or
Selectable are not derived from PersistentObject, but are a model of storable. A class which
is a model of storable cannot be read from a persistent stream if it is not part or base class of a persistent
object. This is due to the fact that the persistence manager needs a well-defined interface to handle the
objects it stores and retrieves. This interface is implemented in the class PersistentObject. Thus, in
order to provide for object persistence the definition of two methods in the storable class’ interface is required:
read(PersistenceManager& pm) and write(PersistenceManager& pm). These methods are quite similar to persistentWrite and persistentRead.
Testing is crucial to verify the correctness of the library – especially when using C++. But why has it to be so complicated, using all these macros and stuff? C++ compilers are strange beasts and there are no two accepting the same code. Since one of the main concerns of BALL is portability, we have to ensure that every single line of code compiles on all platforms. Due to the long compilation times and the (hopefully in the future) large number of different platforms, tests to verify the correct behaviour of all classes have to be carried out automatically. This implies a well defined interface for all tests, which is the reason for all these strange macros. This fixed format also enforces the writing of complete class tests. Usually a programmer writes a few lines of code to test the parts of the code he wrote for correctness. Of the methods tested after the introduction of the test macros, about a tenth of all functions/methods showed severe errors after thorough testing. Most of these errors did not occur an all platforms or did not show up on trivial input.
Writing tests for each method of a class also ensures that each line of code is compiled. When using
class templates the compiler only compiles the methods called. Thus it is possible that a code segment
contains syntactical errors but the compiler accepts the code happily – it simply ignores most of the code.
This is quickly discovered in a complete test of all methods. The same is true for configuration dependend
preprocessor directives that stem from platform dependencies. Often untested code also hides inside the
const version of a method, when there is a non-const method with the same name and arguments (for
example most of the getName methods in BALL). In most cases, the non-const version is preferred by
the compiler and it is usually not clear to the user which version is taken. Again, explicit testing of each
single method provides help for this problem. The ideal method to tackle the problem of untested code is
the complete coverage analysis of a class. Unfortunately, this is only supported for very few compilers, so
it is not used for testing BALL.
Writing the test program is a wonderful opportunity to verify and complete the documentation! Often enough implementation details are not clear at the time the documentation is written. A lot of side effects or special cases that were added later do not appear in the documentation. Going through the documentation and the implementation in parallel is the best way to verify the documentation for consistence and (strange coincidence?!) the best way to implement a test program, too!
Each BALL class has to provide its own test program. This test program has to check each method of the
class. The test programs reside in the directory BALL/source/TEST. To create a new test program, rename
a copy of the file Skeleton test.C to the new class test name (usually <classname>_test.C).
The test program has to be coded using the class test macros as described in the BALL online reference.
These macros are defined in CONCEPT/classTest.h. Special care should be taken to cover all special
cases (e.g. what happens, if a method is called with empty strings, negative values, zero, null pointers etc.).
-
START_TEST(class name, version)END_TESTThe
START_TESTmacro starts the tests for class class name. This should occur just once in a test program, as it contains the definition of the main function. Similarly,END_TESTdefines the terminal section of the test program. -
CHECK(name)RESULTThe
CHECKmacro starts a subtest for a specific method of the class. The full signature of the class should be given as name. Variables defined after this macro are scoped and valid until the next RESULT only. ThisRESULTalso evaluates success or failure of the test. -
ABORT_IF(condition)This macro aborts the subtest, i.e. it jumps forward to the next
RESULTmacro and marks the subtest as failed. This macro is usually employed to prevent the execution of certain tests if a necessary precondition was not met and the execution would hence result in a crash. -
STATUS(...)This macro can be used to print the current status from within a subtest and is useful for debugging tests. The macro will print its argument to the console if the test is run with
-Vas an argument and stay silent otherwise. Example:STATUS("result: " << result).
-
TEST_EQUAL(a, b)Check that a and b are equal (using an appropriate
operator ==). -
TEST_NOT_EQUAL(a, b)Check that a and b are not equal (again, using an appropriate
operator ==, not theoperator !=). This is helpful, if you know what a method returns upon unsuccessful execution, e.g., if you want to make sure it did not return aNULLpointer. -
TEST_REAL_EQUAL(a, b)PRECISION(delta)The
TEST_REAL_EQUAL(a, b)macro checks if a and b are equal within a defined tolerance delta. The tolerance is defined via thePRECISIONmacro. a and b have to be convertible to double. -
NEW_TMPFILE(filename)TEST_FILE()TEST_FILE_REGEX()The
NEW_TMPFILEmacro creates a new temporary filename, which is then assigned to filename. Using these temporary filenames is recommended, as they are deleted after running the test (except if run in verbose mode). After having written something to a file (e.g. while testing output methods), the contents of this file can be compared to a template file using theTEST_FILEandTEST_FILE_REGEXmacros. -
TEST_EXCEPTION(exception type, expression)Tests whether an expression throws the exception it is supposed to throw.
No BALL class method or function should lead to the termination of a program in case of a detectable error. In particular, if BALL classes are embedded for scripting, robustness is an important topic. The recommended procedure to handle even fatal errors is to throw an exception. Uncaught exceptions will result in a call to abort thereby terminating the program while still giving the programmer a chance to handle them graciously.
All exceptions used in BALL are derived from GeneralException defined in COMMON/exception.h.
A default constructor should not be implemented for these exceptions. Instead, the constructor of all
derived exceptions should have the following signature:
AnyException(const char* file, unsigned long line);Additional arguments are possible but should provide default values (see IndexOverflow for an example).
The throw directive for each exception should be of the form
throw AnyException(__FILE__, __LINE__);to simplify debugging. __FILE__ and __LINE__ are preprocessor macros filled in by the compiler. GeneralException
provides two methods (getFile and getLine) that allow the localization of the exception’s cause.
Exceptions can be caught in 3 ways in C++:
- By value
- By reference
- By pointer
In general an exception should be caught by reference, as this avoids the thrown exception to be copied. In some (rare) cases catching an exception by value is necessary. Please leave a short comment why this is the case. Please: never catch an exception by pointer unless you really really need to. Pointer tend to do more harm than good with exception handling. Summing up, catching an exception in BALL looks like this:
try {
...
} catch(Exception1& e) { //Catch an exception and call it e
...
} catch(Exception2&) { //Catch an exception that, itself, is not needed any longer
...
} catch(...) { //Catch any exception
...
}Often exceptions are members of a whole exception hierarchy, with a single "mother exception" at the root. It is often tempting to just catch this exception and thus handle the whole hierarchy with only one catch clause. (this bad practice can be seen all over BALL, however you are not a bad programmer, are you?) However there are several pitfalls with this approach:
- You loose the ability of distinguishing exceptions
- You accidentally catch exceptions which are unexpected here and really should lead to program termination. Considering this it is the best practice to stay as low as possible in the exception hierarchy in order to avoid these undesired effects.
Every function that can guarantee that within its context only certain exceptions can be thrown should explicitly specify them using the throw specifier in the function declaration:
void foo() throw(AnyException, FurtherException);Usually however using the throw specifier can be considered harmful for the following reasons:
1. No compile time checks of the thrown exceptions are conducted, however code for runtime checks is added
to the compiled program. Thus one does not get any additional type safety, but only an increased overhead
of the exception handling.
2. If another exception than the expected ones is thrown an instance of std::bad_exception is rethrown, which obfuscates
the original exception. This can confuse custom exception handlers and generally makes debugging harder.
Thus: If no absolute guarantee on the thrown exceptions can be made or if runtime is critical omit the throws specifier. This even
holds for the empty throw specifier: throw(). Adding doxygen comments about the explicitly thrown exceptions is still required though.