Last week I spoke with several developers regarding coding conventions for the C programming language. Most of them responded that there is some documentation by the organization, but some of them have never found it or read it. Most of them just look at existing code and try to mimic. The issue is that on most legacy projects, there is code written using different styles. Some organizations use some tools to extract documentation and or code metrics. With time those tools may have changed leaving behind artifacts that are no longer needed.
Earlier today I read the Wikipedia entry for Coding Conventions. The article is generic. I have been developing software using multiple programming languages. Lately I have been using Haskell which provides a complete different view of coding. C is imperative while Haskell is functional. More on this in a future post.
Years ago (actually decades ago) I wrote a tool that counts LOC (Lines Of Code). The LOC metric is simple but with additional data can be quite useful to determine developer productivity and product quality among other things. In the current incarnation of a storage server product that I architected, designed and implemented a couple decades ago which is still is in service, the LOC written in C is a little over 1.5 M. I have developed several other products and services written in C and other programming languages.
I have always been interested in the software engineering aspects when developing software. A reduced subject, yet important, is the style used to write code. Code needs to be clean and easy to follow. With time, code needs to be debugged and enhanced. I am not going to get into how to document a software product / service in this post. I am just interested in the look of the actual code so it can be easily understood by me and other developers in the team which will maintain the software.
I like to use simple diagrams and examples. They seem to work much better than long documents (I like to refer to them as Victorian novels) which are never completely followed. English, being a natural language is quite ambiguous. An example with the necessary explanations and a simple diagram in most (never say all) cases will go much further.
Of course the only thing sure in life is that things will change. When an organization has multiple and complex documents, it is almost impossible and very expensive to keep them up to date. That is not the case when documentation follows the KISS (Keep It Simple Stupid or a more politically correct interpretation, Keep It Short and Simple) rule.
The coding standards, guidelines, rules or whatever your organization wishes to name them should be revised and if needed updated with time (e.g., once about every three years). I have experimented with different formatting ideas. Most of them have lasted a few years. Only best ones remain after several decades. So why not stick to the best?
I will start with an edited (to protect the innocent) header at the head of all source files.
// ***************************************************************** // - author: John C. Canessa - john.canessa@companyname.com // - copyright: Company Name * 1994 - 2017 // - date: Sunday, November 05, 2017 - jcc // - path: c:\sencorsource\source\que.c // ***************************************************************** #define __que_c__ // **** header files **** #include "sencor.h" #include "que.h" // **** LOCAL variables **** static int traceExecution = 0;
The header contains information that allows developers to get some information about the file. In this case I happen to be the author. When multiple people contribute, their names should be included at the head of the source file and the initials on each function / method they contribute to. That way, if a question comes up, if needed, you may ask (if the developer is still around) why something was coded in a specific way.
As you can see by the header, this particular set of functions / methods was initially developed in 1994 (about 25 years ago). Of course the code has been maintained and updated to make sure it is as easier to debug and maintain as possible.
Like the name may indicate, “que” seems like the abbreviation of the word “queue”. This code should contain a library to use and manipulate queues, lists and possibly stacks.
For the coding conventions I would select, if possible, a single function as an example. Allow me to insert the entire code for the function. We will then describe the parts and put a simple diagram to clarify.
Let’s start with a simple diagram.
The header (1) of the function should have a descriptive name and a brief and concise description regarding the one and only one thing the function does. Never develop functions / methods that do multiple things. Do one and only one thing and do it well. In this case our function clears a queue data structure.
LIBRARY_FUNCTION_HEADER QueClear (
SENCOR_QUEUE *queue
)
// ***************************************************************@@
// - This function clears the contents of the specified queue. It
// frees all (if any) the elements of the queue. It FREES the
// private data. If the private data contains pointers memory MAY
// leak if not released. It is up to the caller to free private
// data of the queue BEFORE calling this function.
// *****************************************************************
{
This code indicates that is part of a general library, the function does something for a queue data structure. Specifically it clears the data structure. The comments are short and simple. The first line in the comments ends with two “@@”. This is used by some software to extract information about the function.
The next part declares all variables (2).
int retVal, // value returned by this function status; // returned by function calls SENCOR_QUEUE *head, // head of the queue localQueue; // local queue time_t currentTime; // current time
Of interest are the following mandatory variables:
| Variable | Description |
| retVal | Value returned by this function. |
| status | Value returned by function calls. |
Both variables are listed in alphabetical order and are of type integer. All functions in the project MUST be of type int and should always return one of the following values:
| Returned Value | Meaning |
| 0 | All is well. |
| < 0 | A warning for the caller. |
| > 0 | An error was encountered. |
The organization generates a set of definitions with the associated warnings, errors and text. That way it is simpler to port the code to different natural languages. The degree of internationalization is based on the purpose of the code. In this case the library is used by a set of backend services with little if any exposure to end users.
Note that variable are declared but not initialized. The declaration is sorted by types and variables are also sorted in alphabetical order within each type. Variables should not be declared or initialized all over the function.
The variable initialization (3) section is where all variables are initialized.
// **** INITIALIZATION **** retVal = 0; // hope all goes well currentTime = (time_t)0; // for starters head = (SENCOR_QUEUE *)NULL; // for starters status = 0; // for starters memset((void *)&localQueue, (int)0x00, (size_t)sizeof(localQueue));
Most developers do not check the arguments. They just assume the caller will always pass values in the assumed range. You know what happens when we assume. When strange behavior is exhibited by the application / system, it makes it hard to determine the issue when parameters are out of bounds. The first affected function might not crash but a few calls later it will. Resolving the issue tends to take more time than necessary. So always check all arguments to all functions. Sanity checks (4) follows:
// **** inform the user what went on ****
if (traceExecution > 1)
EventLog(EVENT_INFO,
"QueClear <<< ENTERING line: %d\n",
__LINE__);
// **** PERFORM some sanity checks ****
if (queue == (SENCOR_QUEUE *)NULL) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue == NULL line: %d file ==>%s<==\n",
__LINE__, __FILE__);
retVal = WAR_INVALID_ARGUMENT; // flag the problem
goto done; // to perform clean up
}
// **** CHECK if the queue has NOT been initialized (blank memory) ****
status = memcmp((void *)queue, // queue being tested
(void *)&localQueue, // memory filled with nulls
sizeof(SENCOR_QUEUE)); // size of the queue data structure
if (status == 0) // the queue has NOT been initialized
{
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status);
// **** WE are done ****
goto done;
}
// **** CHECK if the queue has NOT been initialized (random memory) ****
if (queue->signature != QUEUE_SIGNATURE)
{
// **** CLEAR the memory for the queue ****
memset((void *)queue, (int)0x00, (size_t)sizeof(SENCOR_QUEUE));
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status);
// **** WE are done ****
goto done;
}
// **** LOOP removing elements from the specified queue (if any) ****
while ((SENCOR_QUEUE *)queue->next != queue) {
// **** POINT to the head element in the queue ****
head = (SENCOR_QUEUE *)queue->next; // current head element
// **** UPDATE the queue pointers ****
queue->next = head->next; // head is next element
(head->next)->prev = queue; // remove head element
// **** DECREMENT the queue counter ****
queue->count -= (unsigned long)1L; // subtract the element being removed
// **** FREE the head element from the queue ****
free((void *)head);
// **** FLAG that the head element has been released ****
head = (SENCOR_QUEUE *)NULL; // not pointing to allocated memory
}
// **** FREE the private data (if any) ****
if (queue->privateData != (void *)NULL) {
free((void *)queue->privateData);
queue->privateData = (void *)NULL;
}
// **** CHECK if the queue count is NOT set to zero (0) ****
if (queue->count != (unsigned long)0L) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->count: %lu line: %d file ==>%s<==\n",
queue->count, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
// **** CHECK if the pointers are NOT pointing to the queue ****
if (queue->next != queue) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->next: %0x%08lx != queue: 0x%08lx line: %d file ==>%s<==\n",
queue->next, queue, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
if (queue->prev != queue) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->prev: %0x%08lx != queue: 0x%08lx line: %d file ==>%s<==\n",
queue->prev, queue, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
The allocation of resources (5) section is where all resources (i.e., memory, database, network connections) are allocated.
// **** NO additional resources required ****
What is the point of continuing and having to perform additional cleanups if the function will not be able to complete successfully.
The main code (6) for this function follows.
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status);
This is where the core of the code for the function resides. It might be as simple as assigning a value to the argument or performing a set of computations. Always keep in mind the concept of doing one and only one thing in a function. Never perform multiple tasks which can be considered side effects. They might initially be fine, but with time it will make the code dependent and will have to be refactored after encountering issues in the field or having to modify the function in question to enhance the software with new features.
The done: label (7) is where all execution paths for the function have to pass.
// **** CLEAN UP **** done:
Notice that if all goes well, this point will be in the main path. On the other hand, if something fails, the code for the function will branch to the goto label.
The release of resources (8) section releases all resources not needed outside the function.
// **** FREE the head element (if needed) **** if (head != (SENCOR_QUEUE *)NULL) free((void *)head);
If the function needs to set a value in an allocated piece of memory, but the system ran out of memory or did not have sufficient memory resources, they needs to be released before the function returns. Not doing so will cause a leak of resources that depending on the function and type may cause a slow leak that will eventually cause the system to crash in a different section making it harder to diagnose.
Finally we have the return retVal (9) section.
// **** inform the user what is going on **** if (traceExecution > 1) EventLog(EVENT_INFO, "QueClear <<< retVal: %d line: %d file ==>%s<==\n", retVal, __LINE__, __FILE__); // **** INFORM the caller what went on **** return retVal; }
All functions should return an integer as was described in the initialization section. The common value should always be zero (0) if all goes well; otherwise a positive or negative number indicating the cause of failure should be returned.
How variables are names is obvious by looking at the sample code. How spacing and brackets are placed is also obvious. Of course sample code will be needed to illustrate how to declare and call functions with no, single and multiple arguments.
In conclusion, a coding convention document should take between two and four pages. To eliminate stale documents, the organization should keep coding conventions for all programming languages in a single on-line repository using web pages.
The code for the entire function follows:
LIBRARY_FUNCTION_HEADER QueClear (
SENCOR_QUEUE *queue
)
// ***************************************************************@@
// - This function clears the contents of the specified queue. It
// frees all (if any) the elements of the queue. It FREES the
// private data. If the private data contains pointers memory MAY
// leak if not released. It is up to the caller to free private
// data of the queue BEFORE calling this function.
// *****************************************************************
{
int retVal, // value returned by this function
status; // returned by function calls
SENCOR_QUEUE *head, // head of the queue
localQueue; // local queue
time_t currentTime; // current time
// **** INITIALIZATION ****
retVal = 0; // hope all goes well
currentTime = (time_t)0; // for starters
head = (SENCOR_QUEUE *)NULL; // for starters
status = 0; // for starters
memset((void *)&localQueue, (int)0x00, (size_t)sizeof(localQueue));
// **** inform the user what went on ****
if (traceExecution > 1)
EventLog(EVENT_INFO,
"QueClear <<< ENTERING line: %d\n",
__LINE__);
// **** PERFORM some sanity checks ****
if (queue == (SENCOR_QUEUE *)NULL) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue == NULL line: %d file ==>%s<==\n",
__LINE__, __FILE__);
retVal = WAR_INVALID_ARGUMENT; // flag the problem
goto done; // to perform clean up
}
// **** CHECK if the queue has NOT been initialized (blank memory) ****
status = memcmp((void *)queue, // queue being tested
(void *)&localQueue, // memory filled with nulls
sizeof(SENCOR_QUEUE)); // size of the queue data structure
if (status == 0) // the queue has NOT been initialized
{
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status); // **** WE are done **** goto done; } // **** CHECK if the queue has NOT been initialized (random memory) **** if (queue->signature != QUEUE_SIGNATURE)
{
// **** CLEAR the memory for the queue ****
memset((void *)queue, (int)0x00, (size_t)sizeof(SENCOR_QUEUE));
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status); // **** WE are done **** goto done; } // **** LOOP removing elements from the specified queue (if any) **** while ((SENCOR_QUEUE *)queue->next != queue) {
// **** POINT to the head element in the queue ****
head = (SENCOR_QUEUE *)queue->next; // current head element
// **** UPDATE the queue pointers ****
queue->next = head->next; // head is next element
(head->next)->prev = queue; // remove head element
// **** DECREMENT the queue counter ****
queue->count -= (unsigned long)1L; // subtract the element being removed
// **** FREE the head element from the queue ****
free((void *)head);
// **** FLAG that the head element has been released ****
head = (SENCOR_QUEUE *)NULL; // not pointing to allocated memory
}
// **** FREE the private data (if any) ****
if (queue->privateData != (void *)NULL) {
free((void *)queue->privateData);
queue->privateData = (void *)NULL;
}
// **** CHECK if the queue count is NOT set to zero (0) ****
if (queue->count != (unsigned long)0L) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->count: %lu line: %d file ==>%s<==\n", queue->count, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
// **** CHECK if the pointers are NOT pointing to the queue ****
if (queue->next != queue) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->next: %0x%08lx != queue: 0x%08lx line: %d file ==>%s<==\n", queue->next, queue, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
if (queue->prev != queue) {
EventLog(EVENT_ERROR,
"QueClear <<< UNEXPECTED queue->prev: %0x%08lx != queue: 0x%08lx line: %d file ==>%s<==\n", queue->prev, queue, __LINE__, __FILE__);
retVal = WAR_INVALID_VALUE; // flag the problem
goto done; // to perform clean up
}
// **** NO additional resources required ****
// **** INITIALIZE the queue ****
status = QueInit(queue);
CDP_CHECK_STATUS("QueClear <<< QueInit", status); // **** CLEAN UP **** done: // **** FREE the head element (if needed) **** if (head != (SENCOR_QUEUE *)NULL) free((void *)head); // **** inform the user what is going on **** if (traceExecution > 1)
EventLog(EVENT_INFO,
"QueClear <<< retVal: %d line: %d file ==>%s<==\n",
retVal, __LINE__, __FILE__);
// **** INFORM the caller what went on ****
return retVal;
}
A reference is made to the QueInit() function. The code for it follows:
LIBRARY_FUNCTION_HEADER QueInit (
SENCOR_QUEUE *queue
)
// ***************************************************************@@
// - This function initializes the specified queue. This function
// MUST be called before any other queue operation is performed on
// the specified data structure.
// *****************************************************************
{
int retVal, // value returned by this function
status; // returned by function calls
time_t currentTime; // current time
// **** INITIALIZATION ****
retVal = 0; // hope all goes well
currentTime = (time_t)0; // for starters
status = 0; // for starters
// **** inform the user what went on ****
if (traceExecution > 1)
EventLog(EVENT_INFO,
"QueInit <<< ENTERING line: %d\n",
__LINE__);
// **** PERFORM some sanity checks ****
if (queue == (SENCOR_QUEUE *)NULL) {
EventLog(EVENT_ERROR,
"QueInit <<< UNEXPECTED queue == NULL line: %d file ==>%s<==\n", __LINE__, __FILE__); retVal = WAR_INVALID_ARGUMENT; // flag the problem goto done; // to perform clean up } // **** CLEAR the queue data structure **** memset((void *)queue, (int)0x00, (size_t)sizeof(SENCOR_QUEUE)); // **** INITIALIZE the fields in the queue **** queue->next = queue;
queue->prev = queue;
queue->count = (unsigned long)0L; // the queue is empty
queue->maxCount = QUEUE_DEFAULT_COUNT;
queue->signature = QUEUE_SIGNATURE;
// **** GET the current time ****
time(¤tTime);
// **** SET the current time in the queue ****
status = QueSetTime(queue,
currentTime);
CDP_CHECK_STATUS("QueInit <<< QueSetTime", status); // **** CLEAN UP **** done: // **** inform the user what is going on **** if (traceExecution > 1) // do NOT check the returned value
EventLog(EVENT_INFO,
"QueInit <<< retVal: %d line: %d file ==>%s<==\n",
retVal, __LINE__, __FILE__);
// **** INFORM the caller what went on ****
return retVal;
}
If you followed the code, you probably figured out that the macro CDP_CHECK_STATUS() had to check and set the value of the retVal function. The code for the macro follows:
#define CDP_CHECK_STATUS(string,code)\
{\
if (status != 0)\
{\
PrintErrorCode(status, 0, "");\
EventLog(EVENT_ERROR,\
"%s status: %d line: %d file ==>%s<==\n",\
string, status, __LINE__, __FILE__);\
if (status < 0)\
retVal = status;\
else retVal = code;\
goto done;\
}\
else {}\
}
Hope you enjoyed this post. Always simplify things by documenting and following standards. With time all standards will change. In this subject, changes occur very infrequently, but when they occur, make sure new and when possible (i.e., fixing an issue) apply them to existing / legacy code. You and your team members will thank you.
Enjoy;
John
@john_canessa