Writing Readable Code
While visual debuggers are quite useful to identify and eliminate runtime errors or unintended program behavior, it is a better idea to write code that is less likely to have problems to begin with. One way to do that is to strive to write code that is easier to read and to understand. Then, finding problems in code becomes more like identifying contradictions between English sentences and less like solving cryptic puzzles. When you are writing code in a way that is understandable, your mistakes will often be apparent as you are making them and will be easier to spot when you come back to solve problems that slipped through.
After some unenjoyable maintenance experiences, you realize that the primary purpose of the programs that you write is not to make the computer do what you want to, but to tell the reader what the computer will do when the program runs. This usually means that you need to do more typing, which IDEs can help with. This may also mean that you sometimes write code that is not the most optimal in terms of execution time or memory used. If this goes against what you have learned, consider that you may be trading a minuscule amount of efficiency for the risk of being incorrect. With the vast processing power and memory at our disposal, you may be making your code unnecessarily cryptic and possibly buggy in the vain quest for efficiency. In the next sections, we will list some rules of thumb that may help you write code that is more readable.
Indentation and Formatting
C++ code, as in many other programming languages, is composed of program blocks. A function has a set of statements that form its body as a block. A loop's block statements will execute in iterations. An if statement's block executes if the given condition is true and the corresponding else statement's block executes otherwise.
Curly braces, or lack thereof for single-statement blocks, inform the computer, whereas indentation in the form of white space informs the human reader about the block structure. The lack of indentation, or misleading indentation, can make it very difficult for the reader to understand the structure of the code. Therefore, we should strive to keep our code well-indented. Consider the following two code blocks:
// Block 1
if (result == 2)
firstFunction();
secondFunction();
// Block 2
if (result == 2)
firstFunction();
secondFunction();
While they are identical in terms of execution, it is much clearer in the second one that firstFunction() is executed only if result is 2. Now consider the following code:
if (result == 2)
firstFunction();
secondFunction();
This is simply misleading. If the reader is not careful, they might easily assume that secondFunction() is executed only if result is 2. However, this code is identical to the two previous examples in terms of execution.
If you feel like correcting indentation is slowing you down, you can use your editor's formatting facilities to help you. In Eclipse, you can select a block of code and use Source | Correct Indentation to fix the indentation of that selection, or use Source | Format to also fix other formatting issues with code.
Beyond indentation, other formatting rules such as placing the curly brace at the correct line, inserting spaces around binary operators, and inserting a space after each comma are also very important formatting rules that you should abide by to keep your code well-formatted and easy to read.
In Eclipse, you can set formatting rules per-workspace in Window | Preferences | C/C++ | Code Style | Formatter or per-project in Project | Properties | C/C++ General | Formatter. You can either select one of the industry-standard styles such as K&R or GNU, or you can modify them and create your own. This becomes especially important when you use Source | Format to format your code. For example, if you choose to use spaces for indentation but Eclipse's formatting rules are set to tabs, your code would become a mixture of tabs and spaces.
Use Meaningful Names as Identifiers
In our code, we use identifiers to name many items—variables, functions, class names, types, and so on. For the computer, these identifiers are merely a sequence of characters to distinguish them from one another. However, for the reader, they're much more. The identifier should completely and unambiguously describe the item that it represents. At the same time, it should not be overly long. Furthermore, it should abide by the style standards that are in use.
Consider the following code:
studentsFile File = runFileCheck("students.dat");
bool flag = File.check();
if (flag) {
int Count_Names = 0;
while (File.CheckNextElement() == true) {
Count_Names += 1;
}
std::cout << Count_Names << std::endl;
}
While this is a perfectly valid piece of C++ code, it is quite difficult to read. Let's list the problems with it. First of all, let's look at the style problems of the identifiers. The studentsFile class name starts with a lowercase letter, which should have been uppercase instead. The File variable should have started with a lowercase letter. The Count_Names variable should have started with a lowercase letter and should not have had an underscore in it. The CheckNextElement method should have started with a lowercase letter. While these may seem arbitrary rules, being consistent in naming carries extra information about the name—when you see a word that starts with an uppercase letter, you immediately understand that it must be a class name. Furthermore, it is simply a distraction to have names that do not obey the standard in use.
Now, let's look beyond the style and inspect the names themselves. The first problematic name is the runFileCheck function. A method is an action that returns a value: its name should both clearly explain what it does as well as what it returns. "Check" is an overused word that is too vague for most situations. Yes, we checked it, it's there—what should we do with it then? In this case, it seems we actually read the file and create a File object. In that case, runFileCheck should have been readFile instead. This clearly explains the action being taken, and the return value is what you would expect. If you wanted to be more specific about the return value, readAsFile could be another alternative. Similarly, the check method is vague and should be exists instead. The CheckNextElement method is also vague and should be nextElementExists instead.
Another overused vague word is flag, which is often used for Boolean variables. The name suggests an on/off situation but gives no clue as to what its value would mean. In this case, its true value means that the file exists, and the false value means that the file does not exist. The trick for naming Boolean variables is to devise a question or statement that is correct when the value of the variable is true. In this example, fileExists and doesFileExist are two good choices.
Our next misnamed variable is Count_Names, or countNames with its correct capitalization. This is a bad name for an integer because the name does not suggest a number—it suggests an action that results in a number. Instead, an identifier such as numNames or nameCount would clearly communicate what the number inside means.
Keeping Algorithms Clear and Simple
When we read code, the steps that are taken and the flow should make sense. Things that are done indirectly—byproducts of functions, multiple actions being done together in the name of efficiency, and so on—are things that make it difficult to understand your code for the reader. For example, let's look at the following code:
int *input = getInputArray();
int length = getInputArrayLength();
int sum = 0;
int minVal = 0;
for (int i = 0; i < length; ++i) {
sum += input[i];
if (i == 0 || minVal > input[i]) {
minVal = input[i];
}
if (input[i] < 0) {
input[i] *= -1;
}
}
Here, we have an array that we are processing in a loop. At first glance, it is not very clear what exactly the loop is doing. The variable names are helping us understand what is going on, but we must run the algorithm in our heads to be sure that what's being advertised by those names is really happening here. There are three different operations that are taking place in this loop. Firstly, we are finding the sum of all the elements. Secondly, we are finding the minimum element in the array. Thirdly, we are taking the absolute value of each element after these operations.
Now consider this alternative version:
int *input = getInputArray();
int length = getInputArrayLength();
int sum = 0;
for (int i = 0; i < length; ++i) {
sum += input[i];
}
int minVal = 0;
for (int i = 0; i < length; ++i) {
if (i == 0 || minVal > input[i]) {
minVal = input[i];
}
}
for (int i = 0; i < length; ++i) {
if (input[i] < 0) {
input[i] *= -1;
}
}
Now everything is much clearer. The first loop finds the sum of the inputs, the second loop finds the minimum element, and the third loop finds the absolute value of each element. Although it's much clearer and more understandable, you may feel like you are doing three loops, and therefore wasting CPU resources. The drive to create more efficient code may compel you to merge these loops. Note that the efficiency gains you have here would be minuscule; your program's time complexity would still be O(n).
While creating code, readability and efficiency are two constraints that can often be in competition. If you want to develop readable and maintainable code, you should always prioritize readability. Then, you should strive to develop code that is also efficient. Otherwise, code that has low readability risks being difficult to maintain, or worse, risks having bugs that are difficult to identify and fix. Your program's high efficiency would be irrelevant when it is producing incorrect results or when the cost of adding new features to it becomes too high.
Exercise 10: Making Code Readable
There are style and indentation problems in the following code. Spaces are used inconsistently, and the indentation is incorrect. Also, the decision on single-statement if blocks having curly braces or not is inconsistent. The following piece of code has problems in terms of indentation, formatting, naming, and clarity:
//a is the input array and Len is its length
void arrayPlay(int *a, int Len) {
int S = 0;
int M = 0;
int Lim_value = 100;
bool flag = true;
for (int i = 0; i < Len; ++i) {
S += a[i];
if (i == 0 || M > a[i]) {
M = a[i];
}
if (a[i] >= Lim_value) { flag = true;
}
if (a[i] < 0) {
a[i] *= 2;
}
}
}
Let's fix these problems and make it compatible with a common C++ code style. Perform the following steps to complete this exercise:
- Open Eclipse CDT.
- Create a new ArrayPlay.cpp file in the src folder and paste the preceding code. Make sure you do not have any text selected. Then, go to Source | Format from the top menu and accept the dialog to format the entire file. This makes our code look like the following:
//a is the input array and Len is its length
void arrayPlay(int *a, int Len) {
int S = 0;
int M = 0;
int Lim_value = 100;
bool flag = true;
for (int i = 0; i < Len; ++i) {
S += a[i];
if (i == 0 || M > a[i]) {
M = a[i];
}
if (a[i] >= Lim_value) {
flag = true;
}
if (a[i] < 0) {
a[i] *= 2;
}
}
}
Now that the code is a bit easier to follow, let's try to understand what it does. Thanks to the comments, we understand that we have an input array, a, whose length is Len. Better names for these would be input and inputLength.
- Let's make that first change and rename a to input. If you are using Eclipse, you can select Refactor | Rename to rename one occurrence and all others will be renamed as well. Do the same for Len and rename it to inputLength.
- The updated code will look like the following. Note that we do not need the comment anymore since parameter names are self-explanatory:
void arrayPlay(int *input, int inputLength) {
int S = 0;
int M = 0;
int Lim_value = 100;
bool flag = true;
for (int i = 0; i < inputLength; ++i) {
S += input[i];
if (i == 0 || M > input[i]) {
M = input[i];
}
if (input[i] >= Lim_value) {
flag = true;
}
if (input[i] < 0) {
input[i] *= 2;
}
}
}
- We have a couple of other variables defined before the loop. Let's try to understand them. It seems all it does with S is to add each element to it. Therefore, S must be sum. M, on the other hand, seems to be the minimum element—let's name it smallest.
- Lim_value seems to be a threshold, where we simply want to know whether it has been crossed. Let's rename it topThreshold. The flag variable is set to true if this threshold is crossed. Let's rename it to isTopThresholdCrossed. Here is the state of the code after these changes with Refactor | Rename:
void arrayPlay(int *input, int inputLength) {
int sum = 0;
int smallest = 0;
int topThreshold = 100;
bool isTopThresholdCrossed = true;
for (int i = 0; i < inputLength; ++i) {
sum += input[i];
if (i == 0 || smallest > input[i]) {
smallest = input[i];
}
if (input[i] >= topThreshold) {
isTopThresholdCrossed = true;
}
if (input[i] < 0) {
input[i] *= 2;
}
}
}
Now, let's see how we can make this code simpler and easier to understand. The preceding code is doing these things: calculating the sum of the input elements, finding the smallest one, determining whether the top threshold was crossed, and multiplying each element by two.
- Since all of these are done in the same loop, the algorithm is not very clear now. Fix that and have four separate loops:
void arrayPlay(int *input, int inputLength) {
// find the sum of the input
int sum = 0;
for (int i = 0; i < inputLength; ++i) {
sum += input[i];
}
// find the smallest element
int smallest = 0;
for (int i = 0; i < inputLength; ++i) {
if (i == 0 || smallest > input[i]) {
smallest = input[i];
}
}
// determine whether top threshold is crossed
int topThreshold = 100;
bool isTopThresholdCrossed = true;
for (int i = 0; i < inputLength; ++i) {
if (input[i] >= topThreshold) {
isTopThresholdCrossed = true;
}
}
// multiply each element by 2
for (int i = 0; i < inputLength; ++i) {
if (input[i] < 0) {
input[i] *= 2;
}
}
}
Now the code is much clearer. While it's very easy to understand what each block is doing, we also added comments to make it even more clear. In this section, we gained a better understanding of how our code is converted to executables. Then, we discussed ways of identifying and resolving possible errors with our code. We finalized this with a discussion about how to write readable code that is less likely to have problems. In the next section, we will solve an activity wherein we will be making code more readable.
Activity 3: Making Code More Readable
You may have code that is unreadable and contains bugs, either because you wrote it in a hurry, or you received it from someone else. You want to change the code to eliminate its bugs and to make it more readable. We have a piece of code that needs to be improved. Improve it step by step and resolve the issues using a debugger. Perform the following steps to implement this activity:
- Below you will find the source for SpeedCalculator.cpp and SpeedCalculator.h. They contain the SpeedCalculator class. Add these two files to your project.
- Create an instance of this class in your main() function and call its run() method.
- Fix the style and naming problems in the code.
- Simplify the code to make it more understandable.
- Run the code and observe the problem at runtime.
- Use the debugger to fix the problem.
Here's the code for SpeedCalculator.cpp and SpeedCalculator.h that you will add to your project. You will modify them as a part of this activity:
// SpeedCalculator.h
#ifndef SRC_SPEEDCALCULATOR_H_
#define SRC_SPEEDCALCULATOR_H_
class SpeedCalculator {
private:
int numEntries;
double *positions;
double *timesInSeconds;
double *speeds;
public:
void initializeData(int numEntries);
void calculateAndPrintSpeedData();
};
#endif /* SRC_SPEEDCALCULATOR_H_ */
//SpeedCalculator.cpp
#include "SpeedCalculator.h"
#include <cstdlib>
#include <ctime>
#include <iostream>
#include <cassert>
void SpeedCalculator::initializeData(int numEntries) {
this->numEntries = numEntries;
positions = new double[numEntries];
timesInSeconds = new double[numEntries];
srand(time(NULL));
timesInSeconds[0] = 0.0;
positions[0] = 0.0;
for (int i = 0; i < numEntries; ++i) {
positions[i] = positions[i-1] + (rand()%500);
timesInSeconds[i] = timesInSeconds[i-1] + ((rand()%10) + 1);
}
}
void SpeedCalculator::calculateAndPrintSpeedData() {
double maxSpeed = 0;
double minSpeed = 0;
double speedLimit = 100;
double limitCrossDuration = 0;
for (int i = 0; i < numEntries; ++i) {
double dt = timesInSeconds[i+1] - timesInSeconds[i];
assert (dt > 0);
double speed = (positions[i+1] - positions[i]) / dt;
if (maxSpeed < speed) {
maxSpeed = speed;
}
if (minSpeed > speed) {
minSpeed = speed;
}
if (speed > speedLimit) {
limitCrossDuration += dt;
}
speeds[i] = speed;
}
std::cout << "Max speed: " << maxSpeed << std::endl;
std::cout << "Min speed: " << minSpeed << std::endl;
std::cout << "Total duration: " <<
timesInSeconds[numEntries - 1] - timesInSeconds[0] << " seconds" << std::endl;
std::cout << "Crossed the speed limit for " << limitCrossDuration << " seconds"<< std::endl;
delete[] speeds;
}
Note
The solution for this activity can be found on page 626.