Talk:Coding Standards

There is no mention of const-correctness in the standards page. (Sorry, there IS; I just missed it on first read.) Where it talks about the use of get/set prefixes, the example given is incomplete. It says to write,

int getAge();
void setVal( int Age );

It should say to write

int getVal() const;
void setVal( int anAge );

(Besides, someone might expect Age to be a float, or an unsigned, or a size_t, or a special class with years and months and days... There's probably something to be said for choosing a name that better reflects the underlying representation; --NOT talking about "iAge"; rather a name like "YearsOfAge", and its type to be a size_t, unless it can be negative.)

Const-correctness essentially boils down to making const anything that CAN be const. Sometimes the constness of functions is made difficult by intrinsic linkage, such as when objects have a pointer to next, and are part of an LRU cache that should update on reads. Well, such situations call for making that pointer to next mutable; --after all, it is not really a part of the class, philosophically speaking--, rather than compromise constness on the class interface.

Another subject not mentioned is command/query separation. This is an enormoulsy helpful discipline that makes for clean and readable code when adhered to religiously. Command/Query separation says that there are two types of functions that should never be mixed or combined in one: Queries return a value, ALWAYS; and as class member functions are always ...() const. Commands are NEVER const, and ALWAYS return void. And what people new to this concept ALWAYS ask is "what about returning a success or failure status?". Answer is NEVER do that. Failures should be "returned" by throw()- -ing an exception. Status returns have been depracated for years now.

Another subject not addressed is, in fact, exceptions, and the huge importance of marking functions that do NOT throw excetions simply with a comment

void f() // no-throw

Why? Because the big gottcha with exceptions is that you DON'T want them to happen inside a constructor; so by flagging non-throwing functions //no-throw you can, at a glance, tell if you can call it from a ctor.

Another subject not addressed is the use of "explicit" for single arg ctor's.

Another subject not addressed is avoiding like the plague the terrible practice of using special values with floats to convey additional state. This bad practice is a time-bomb, and a documentation nightmare. Someone modifying a piece of code using one such variable cannot be expected to look up the declaration of every variable to scan visually for comments about them; so a variable that can have special values meaning special things should be accompanied by comments about these special values and their meanings everywhere they are used!!! Just pack a bool with the float into a struct, like

struct fuel_capacity
{
   float litres;
   bool this_is_a_station__dogbreath__aint_got_no_fuel_tank;
};

rather than signify no fuel use by a -1. value. Or if you are really keen on the efficiency aspect, at least wrap that float into a class, like

class fuel_capacity
{
  float litres;
public:
  bool is_applicable() const { return litres >= 0.0f );
  float get_capacity() const
  {
    assert( is_applicable() );
    return litres;
  }
  ...
};

Another subject not addressed is variable and function names, and how to make them intuitive and clear. For example, boolean variables should always read like statements that can only be true or false. E.g,

bool light_is_on; bool tank_has_enough_fuel;

etc. A function that returns the state of a light could be similarly named, but with a verb at the front,

bool light::is_on() const;
bool tank::has_enough_fuel_for( trip const & trp ) const;

A function that turns a light on should be named like

void light::turn_on();

Or if a boolean variable controls the light, it could be named,

bool light_enable_switch;

There should NEVER be ambiguously named variables or functions like

light_on //BAD!!! Is this a switch that turns it on?, a status?, or what?

Least of all, something like

int light_on( double x, int y ); // HORROR

Another very good practice not mentioned is avoiding inheritance of concrete classes. All hierarchies trees should be abstract, having a dtor like,

virtual ~aClass() = 0;

with only the leafs being concrete.

Also, avoiding virtual functions in the public interface of classes. The preferred method is to have public non-virtual functions that call private virtual functions

class unit
{
public:
  void fly(){ _fly(); }
private:
  virtual void _fly();
};

Exceptionally, a virtual function could be protected:

class unit
{
public:
  void fly(){ _fly(); }
protected:
  virtual void _fly();
};

The semantic difference here is that if you make _fly() private, you are communicating to other programmers that derived class overrides of _fly() need not call nor know anything about the base class' _fly() function. In the latter case, you are communicating to programmers that derived class overrides of _fly() SHOULD call the base-class _fly() function; that is, that they should add to, or "decorate" the original function, rather than replace it. But if you make fly() a public virtual function, you're communicating to programmers that you don't know the language ;-)

Separating data from interfaces is also a good idea. Classes representing passive data should feature a complete set of initializing constructors and assignment operators, as well as binary operators for comparisons and/or arithmetic if applicable, and explicit initializations to/from base classes. Interface classes should often have a single data member being a pointer to a struct containing the "real" data members; --that is, they should often use the PIMPL idiom, and often should have their default assignment operator and constructor disabled by being declared private and left undefined. Functions in iface classes access variables in the struct via the single pointer member. This practice avoids excessive header information exposure. Clients of a PIMPL only need information about its public functions, --not about its data members; and having data members in its main header implies that their type definitions must also be included, which calls for additional header files, and so on, which can often lead to vicious header inclusion circles.

Global space and non-class-function calls should be prefixed :: to make it clear that they are not calls to member functions. This would include functions from static and dynamic libraries. Thus, rather than a = floor( b ), write a = ::floor( b ).

Another very helpful standard to adopt is DBC (Design By Contract). DBC cannot be done fully in C++. Long story. But at least a subset can be achieved. Every class should have a private, inline function void _chk_invariants() that is defined as empty in release mode but has code in debug mode. The debug mode code checks the state of the class for coherence.

class myString
{
  char str[32];
  size_t len;
public:
  ...
private:
  ...
  void _chk_invariants()
  {
    assert( len < 32 );
    assert( len >= 0 );
    assert( ::strlen(str) == len );
  }
};

Then, all class functions can have calls to _chk_invariants() at the start and end of the function. Additionally, every function should begin with assertions about its input parameters, to make sure the input they get is within specified range; that pointers are not null, etc. This falls far short of DBC as implemented in the Eiffel language; but it's a start. Just for kicks, adding assertions before the ends of functions, ensuring that output is within specified range; that pointers returned are not null, etceteras, would be nice. In the case of commands, the call to _chk_invariants() is sufficient.