Comments on D0 coding guidelines Guidelines are dated 17oct97 D. Adams 24dec97 I begin with some general comments and then give comments on specific sections. General ------- 1. It is not clear in what spirit the guidelines are intended. I take them as guidelines rather than requirements. My comments would likely change if these were strict requirements. A list of requirements would be much shorter and might include: R1 - Code must be well-designed, well documented, have easily understood headers, have easily understood implementations and behave correctly. In that order. R2 - All non-inline function and static class attribute definitions go in implementation files -- not headers. R3 - Use unique code guards in all headers. 2. I have heard the comment that the document is too long but I do not agree. The language is complex and has many pitfalls and we should expect the document to grow. Also much of the length comes from examples which generally serve to clarify. 3. What about namespaces? 4. Indent each block 2-4 spaces. 5. What about exceptions? Specific (by section) --------------------- 2.1 File layout I would advocate .h rather than .hpp as this is a more conventional choice. Regardless, it should be easy in our CVS/SRT environment to provide an external interface with either (or both) conventions for either choice by a particular package author. In addition to header and implementation files, there are template implementation files. These are where (non-inline) template functions are defined. Many compilers will find these in the header. Others will require that they be in a separate file. Many (including KCC) allow either. I advocate a separate file because 1) it reduces physical coupling and 2) it is easy to use an include statement to put them back in the header. The above does not refer to inline function definitions which some would advocate putting into another file. I would also like to see every component provide a test file as described by Lakos. 2.2 Header comment Some files will be changed 50-100 or more times. Is it intended that these changes all be documented in the files themselves or will we make use of a external mechanism? 2.3 Included header files It should be stated that the order of the headers should not affect the compilation. That is each header should include everthing it needs and not depend on something being included beforehand. Give this, the order indicated seems reasonable except that I would like to see the component header file appear first in the component implementation. This helps to guarantee that my first condition is met. The code guard is essential and its existence one item I would pull out as a requirement. I would make no requirement on the naming convention except that it should include the class name to help avoid clashes. requirement. 2.4 -- there is no section 2.4 -- 2.6 The globals section I state more strongly that there is rarely need for globals. Much better to define these within the scope of a class as enum for int or static data for any other type. If they have to be global and are immutable, then use const instead of static. 2.7 functions and 2.8 operations It is important that the definitions be well organized and understandable--I don't thnink it matters which appear first. 3.1 Class header comment Typically there will be one class per header and the appropriate place to document the class is at the top of the header. I find it easier to read the signatures if there are just one or two lines attached to each method. 3.3 Class layout One can make equally valid arguments for other class layouts. I find the String example fairly difficult to read. This is how I would write it: class String { private: // attributes char* _str; private: // methods String(size_t len, const char s[], size_t slen); void detach(); void unique(); public: // methods // constructor String(); // constructor from char string String(const char s[]); // constructor from a single char. String(char ch); // copy constructor String& String(const String& rhs) // assignment operator String& operator==(const String& rhs); // destructor ~String(); // Overwrite a character void set(int pos, char ch); // Reference a character. char& operator[](int pos); // etc... The example looks much like the compiler header which I find almost useless for understanding how the class operates. I prefer to put very short inline methods in the class definition. These are typcially get and put routines. E.g.: class MyClass private: // attributes int _length; public: // methods // constructor MyClass(int length); // set the length void set_length(int length); // get the length int get_length() const { return _length; }; }; In this example, I assume there is checking when the length is set so the constructor and setter are not inlined or would be defined in the inline section. However, the getter is so simple that its implementation aids the documentation. 4.3 Function parameters Ahhh - pointers vs. references. The document seems to come down squarely on the side of references and then backs off at the end. I do not understand the distinction made between heap and stack pointers. I almost always use references unless I want to allow for a missing object (NULL pointer). I would be even stronger about not returning with arguments. If a function needs to return two objects, then return one which contains the two. Non-const argument references should be reserved for fundtions which modify the specified object. It is probably worthwhile to point out that passing a const copy is meaningless and may upset some compilers: i.e. use energy(Jet emjet) not energy(const Jet emjet). 5.3 Pointers & references I disagree. Between Shape *pshp; and Shape* pshp; I feel the latter more clearly indicates that pshp is a Shape pointer. (I suppose we could compromise with Shape * pshp and then everyone would be confused). Of course one cannot make multiple declarations in one statement with my convention. 5.4 Global variables This section is confusing. Again the main point is don't. 7.1 The if statement Text implies there is indentation--it just did not survive translation to html. This should be fixed. 7.4 Nonlinear control flow The statement is too strong--break and continue can help to clarify the flow of control in loops with multiple tests. The same is true for multiple returns from a function. 8.1 Naming convention. I also strongly encourage prepending all pointers with p; e.g. Jet* pjet, Track* ptrk, etc. For member data, use _p: class MyTrack { Track* _ptrk; };