• Tweet

• Tweet

CPP coding guidelines

MEMORY MANAGEMENT

BUFFER OVERRUNS

STATIC BUFFER OVERRUNS

A static buffer overrun occurs when a buffer, which has been declared on the stack, is written to with more data than it was allocated to hold. The less apparent versions of this error occur when unverified user input data is copied directly to a static variable, causing potential stack corruption.

HEAP OVERRUNS

Heap overruns, like static buffer overruns, can lead to memory and stack corruption. Because heap overruns occur in heap memory rather than on the stack, some people consider them to be less able to cause serious problems; nevertheless, heap overruns require real programming care and are just as able to allow system risks as static buffer overruns.

ARRAY INDEXING ERRORS

Array indexing errors also are a source of memory overruns. Careful bounds checking and index management will help prevent this type of memory overrun.
1. Use memcpy_s instead of memcpy
2. In memcpy_s destination size should be more than or equal to source size
3. If destination size is less than source size, relevant handling to be implemented. e.g.
a. creating fatal error log and throwing exception
b. copying truncated data

MEMORY LEAKS

It is the responsibility of each application to “free” dynamically requested memory when it is finished using it. When an application dynamically allocates memory, and does not free that memory when it is finished using it, that program has a memory leak.
1. Differentiate between delete of object and delete [] of array of objects.
Incorrect
char* myCharArray = new char[BUFF_SIZE];
delete myCharArray;
Correct
char* myCharArray = new char[BUFF_SIZE];
delete [] myCharArray;
2. While deleting check for NULL. If not NULL, delete and set to NULL
Correct
If(ptrData!= NULL)
{
delete ptrData;
ptrData=NULL;
}

3. Check size of STL container before iterating over its elements.

4. If STL containers contain pointers. Iterate over elements and delete each pointer individually. And clear container after this.
Incorrect

std::list<StructImageList*> listFinalImageIds //allocation and appending of pointers to list. ………

m_listFinalImageIds.clear();

Correct

std::list<StructImageList*> listFinalImageIds //allocation and appending of pointers to list. …… //clean up after list use is completed.

if(!m_listFinalImageIds.empty())

{ for(std::list<StructImageList*>::iterator it = m_listFinalImageIds.begin(); it != m_listFinalImageIds.end(); it++){ StructImageList* pData = *(it);

if(pData!= NULL) {

delete pData; pData = NULL;

}

}

m_listFinalImageIds.clear();
5. Use of BSTR. use smart pointer _bstr_t for handling of BSTRs.

https://msdn.microsoft.com/en-us/library/zthfhkd6.aspx

RULE OF THREE

The rule of three (also known as the Law of The Big Three or The Big Three) is a rule of thumb in C++ that claims that if a class defines one (or more) of the following it should probably explicitly define all three
1. destructor
2. copy constructor
3. copy assignment operator
These three functions are special member functions. If one of these functions is used without first being declared by the programmer it will be implicitly implemented by the compiler. This compiler implementation will follow the default semantics of performing the said operation on all the members of the class.

The default semantics are:
Destructor – Call the destructors of all the object’s class-type members
Copy constructor – Construct all the object’s members from the corresponding members of the copy constructor’s argument. It will call the copy constructors of the object’s class-type members, and will do a plain assignment of all non-class type (e.g., int or pointer) data members
Copy assignment operator – Assign all the object’s members from the corresponding members of the assignment operator’s argument. It will call the copy assignment operators of the object’s class-type members, and will do a plain assignment of all non-class type (e.g. int or pointer) data members.
The Rule of Three claims that if one of these had to be defined by the programmer, it means that the compiler-generated version does not fit the needs of the class in one case and it will probably not fit in the other cases either.
Because implicitly-generated constructors and assignment operators simply copy all class data members (“shallow copy”), one should define explicit copy constructors and copy assignment operators for classes that encapsulate complex data structures or have external references such as pointers.

1. As a rule always explicitly define copy constructor and copy assignment operator for class having pointers or complex structures as member variables.
2. While passing class instances are argument to functions, prefer pass by reference and pass by constant reference. Selection between these two depends on functionality being implemented.
3. To detect unknown calls to copy constructor and copy assignment operator following code changes can be done: –
Explicitly define copy constructor and copy assignment operator and make them private.

USE OF STL CONTAINERS

STL container are extensively used in out applications. While defining container elements must be pointers/reference. If we define elements as value, copy of element will be created.
Incorrect

Vector<RuleDefinition> vRule_Definitions;

Correct

Vector<RuleDefinition*> vRule_Definitions;

NESTED CONTAINERS

Use of nested containers is considered as special case.

CONVERTING STRING TO NUMBERS

For converting string to int/long/long long and their unsigned types use strtol/strtoll and strtoul/strtoull respectively. Atoi is not to be used.
Incorrect

long lAmount = atoi(szAmount);

Correct

errno = 0; long lAmount = strtol(szAmount, NULL, 0);

if((iAmount == LONG_MIN || iAmount == LONG_MAX) && errno == ERANGE)) //conversion failed

else //conversion successful.

Remember that no standard C library function ever sets errno to 0. Therefore, to be reliable, you must set errno to zero before calling strtol().

CODING STYLE

NAMING

1. Names should be descriptive; eschew abbreviation.
Give as descriptive a name as possible, within reason. Do not worry about saving horizontal space as it is far more important to make your code immediately understandable by a new reader. Do not use abbreviations that are ambiguous or unfamiliar to readers outside your project, and do not abbreviate by deleting letters within a word.
Incorrect

int n; // Meaningless.

int nerr; // abbreviation.

int n_comp_conns; // abbreviation.

int wgc_connections; // what this stands for.

int pc_reader; // “pc” ?. int cstmr_id; // internal letters deleted.

Correct

int iPrice_Before_Discount; // No abbreviation.

int iNumber_Errors; // int iNumber_dns_Connections; // “DNS” is well known.
2. It is always recommended that class Member Variables should start with ‘m_’ followed by variable type ‘i<integer>/cs<CString>/b<bool>/’
e.g. int m_iAmount

bool m_bIsRuleFilePresent

CString m_csMerchantCategoryCode

CONDITIONAL STATEMENTS, CONDITIONS IN CONDITIONAL STATEMENT

1. Make the intent clear. Enclose statements after conditional in braces.
Incorrect

if(bIsRuleFound) ProcessRule();

MarkCompelete();

Correct

if(bIsRuleFound) {

ProcessRule();

MarkCompelete(); }
Incorrect

if(bIsCompleted) PrintSlip();

Correct

if(bIsCompleted) { PrintSlip(); }
2. Enclose multiple conditionals in braces.
Incorrect

if(iCount==1 && iProcessCount=0)

Correct

if((iCount==1) && (iProcessCount=0))

MAGIC NUMBERS

Do not use magic numbers/string literals in code. Instead use named constants or enums.
Incorrect

if(GetPNode(1,ptrPNode))

UINT iSSLAcceptTimeOut = GetPrivateProfileInt( “Settings”,”SSLAcceptTimeout_ms”,10000,csCurrentDir);

Correct

Add in header (stdafx.h ) enum _en_TBL_P_ID{ P_H_IP = 1, P_H_PORT = 2, };

const int DEFAULT_SSL_ACCEPT_TIMEOUT = 10000;
Use as: UINT iSSLAcceptTimeOut = GetPrivateProfileInt( “Settings”,”SSLAcceptTimeout_ms”,DEFAULT_SSL_ACCEPT_TIMEOUT,csCurrentDir); if(GetPNode(P_H_IP,ptr_Node))

CASTING

1. Use C++-style casts like static_cast<long long>(int)
2. Do not use cast formats like int y = (int)x 3. Use const_cast to remove the const qualifier
COMMENTS Though a pain to write, comments are absolutely vital to keeping our code readable. But remember: while comments are very important, the best code is self-documenting. Giving sensible names to types and variables is much better than using obscure names that you must then explain through comments.

GLOBAL VARIABLES

As with data members, all global variables should have a comment describing what they are and what they are used for.

For example:

// to indicate state of service. It is set to true if service is stopping.

//This variable is used to exit loops, message queues and other implementation specific

//method calls at the time of service shut down.

bool g_bStopService= false;

IMPLEMENTATION COMMENTS

In your implementation you should have comments in tricky, non-obvious, interesting, or important parts of your code. Do not state the obvious. In particular, don’t literally describe what code does, unless the behaviour is nonobvious to a reader who understands C++ well. Instead, provide higher level comments that describe why the code does what it does, or make the code self-describing.
Incorrect

// Find the element in the vector. <–obvious! auto iter = std::find(v.begin(), v.end(), element); if (iter != v.end()) { Process(element); }

Correct

// Divide result by two, taking into account that x

// contains the carry from the add.

for (int i = 0; i < result->size(); i++) { x = (x << 8) + (*result)[i]; (*result)[i] = x >> 1; x &= 1; }

DB STORED PROCEDURE CALLS USING OLEDB (SQLNCLI)

A SUCCESSFUL OPENALL() CALL IS TO BE FOLLOWED BY CLOSEALL()

Incorrect

HRESULT hr = cmdInsertOrUpdateHTDump.OpenAll(m_pDBConnx ->m_ConnectionPtr); if(SUCCEDDED(hr)) { return true; } else { return false; }

Correct

HRESULT hr = cmdInsertOrUpdateHTDump.OpenAll(..); if(SUCCEDDED(hr)) { cmdInsertOrUpdateHTDump.CloseAll(); return true; } else { return false; }

NESTED OPENALL() CALLS ARE NOT TO BE MADE.

Incorrect

hr = cmdInsertOrUpdateHT.OpenAll(..); // 1st OpenAll if(SUCCEDDED(hr)) { hrInner = cmdUpdateTransactionResponse.OpenAll(..) //2nd OpenAll before calling CloseAll of 1st OpenAll if(SUCCEDDED(hrUpdateResponse)) { cmdUpdateResponse.CloseAll(); } cmdInsertOrUpdateHTDump.CloseAll(); }

Correct

hr = cmdInsertOrUpdateHTDump.OpenAll(..); // 1st OpenAll if(SUCCEDDED(hr)) { cmdInsertOrUpdateHTDump.CloseAll(); hrInner = cmdUpdateTransactionResponse.OpenAll(..) //2nd OpenAll after calling CloseAll of 1st OpenAll if(SUCCEDDED(hrUpdateResponse)) { cmdUpdateResponse.CloseAll(); } }

USAGE OF VARCHAR(MAX), TEXT, DATA TYPES IN STORE PROCEDURE

Ideally varchar(max) and text data type should not be used. But in cases where it is unavoidable, use text type.
When we add stored procedure using ATL OLEDB consumer wizard, variable of type text are added as TCHAR (8000). They need to be changed to CComBSTR.

//TCHAR m_ICC_REQUEST[8000];

//TCHAR m_ICC_CONFIRMATION[8000];

CComBSTR m_ICC_REQUEST;

CComBSTR m_ICC_CONFIRMATION;

TEXT USED AS TYPE OF OUTPUT PARAMETER IN STORED PROCEDURE

In this case, Wizard generates following code SET_PARAM_TYPE(DBPARAMIO_INPUT| DBPARAMIO_OUTPUT) COLUMN_ENTRY(21, m_VISA_EMI24DATA)

Above has to be changed to SET_PARAM_TYPE( DBPARAMIO_OUTPUT) COLUMN_ENTRY(21, m_VISA_EMI24DATA)

TEXT USED AS TYPE OF INPUT PARAMETER IN STORED PROCEDURE

In this case, default initialize as follows: m_ICC_REQUEST = CComBSTR(“”);
USAGE OF VARIANT_BOOL
Following are the definitions of VARIANT_BOOL, VARIANT_TRUE and VARIANT_FALSE
typedef short VARIANT_BOOL;
#define VARIANT_TRUE ((VARIANT_BOOL)-1)
#define VARIANT_FALSE ((VARIANT_BOOL)0)
While addding Stored procedures using ATL OLEDB consumer wizard, BIT data types are added as VARIANT_BOOL. VARIANT_BOOL is not same as bool of C++. Assigning VARIANT_BOOL type to bool and vice versa is cause of numerious subtle and hard to find bugs.

CONVERTING BOOL TO VARIANT_BOOL

Incorrect

bool bIsSignatureRequired = true; VARIANT_BOOL m_IS_SIGNATURE_REQUIRED = bIsSignatureRequired;

Correct

VARIANT_BOOL m_IS_SIGNATURE_REQUIRED = (bIsSignatureRequired == true)? VARIANT_TRUE:VARIANT_FALSE;

CONVERTING VARIANT_BOOL TO BOOL

Incorrect
VARIANT_BOOL m_IS_SIGNATURE_REQUIRED = VARIANT_TRUE; bool bIsSignatureRequired = m_IS_SIGNATURE_REQUIRED;

Correct

bool bIsSignatureRequired = (m_IS_SIGNATURE_REQUIRED != VARIANT_FALSE)? true:false