...
In particular, do not default the test for non-zerononzero. For instance, suppose a foo()
function returns 0 to indicate failure or a non-zero nonzero value to indicate success. Testing for inequality with zero
Code Block | ||||
---|---|---|---|---|
| ||||
if (foo() != 0) ...
|
is preferable to
Code Block | ||||
---|---|---|---|---|
| ||||
if (foo()) ...
|
despite the convention that 0 indicates failure. Explicitly testing for inequality with zero benefits maintainability if foo()
is later modified to return -1 −1 rather than 0 on failure.
...
- Functions return 0 if false , and non-zero nonzero if true [1StackOvflw 2009].
- Function failures can typically be indicated by -1 −1 or any non-zero nonzero number.
- Comparison functions return 0 if the arguments are equal and non-zero otherwise (such as the standard library function
strcmp()
, which has a trinary return value) [2] return 0 if the arguments are equal and nonzero otherwise (see strcmp function).
Noncompliant Code Example
In this noncompliant code example, is_banned()
returns zero if false and non-zero nonzero if true.
Code Block | ||||
---|---|---|---|---|
| ||||
LinkedList bannedUsers;
int is_banned(User usr) {
int x = 0;
Node cur_node = (bannedUsers->head);
while (cur_node != NULL) {
if(strcmp((char *)cur_node->data, usr->name) {
x++;
}
cur_node = cur_node->next;
}
return x;
}
void processRequest(User usr) {
if(is_banned(usr) == 1) {
return;
}
serveResults();
}
|
If a banned user is listed twice, the user is granted access. Although is_banned()
follows the common convention of returning non-zero nonzero for true, processRequest
only checks for equality with 1.
Compliant Solution
Because most functions only guarantee a return value of non-zero nonzero only for true, the above the preceding code is better written by checking for inequality with 0 (false) as follows:
Code Block | ||||
---|---|---|---|---|
| ||||
LinkedList bannedUsers;
int is_banned(User usr) {
int x = 0;
Node cur_node = (bannedUsers->head);
while(cur_node != NULL) {
if (strcmp((char *)cur_node->data, usr->name) {
x++;
}
cur_node = cur_node->next;
}
return x;
}
void processRequest(User usr) {
if (is_banned(usr) != 0) {
return;
}
serveResults();
}
|
...
In noncompliant code, function status can typically be indicated by returning -−1
on failure , or any non-negative nonnegative number on success. While this This is a common convention in the standard C library, it but it is discouraged in recommendation ERR02-C. Avoid in-band error indicators.
Although failures are frequently indicated by a return value of zero, there are some common conventions that may conflict in the future with code where the test for non-zero nonzero is not explicit. In this case, defaulting the test for non-zero nonzero welcomes bugs if and when a developer modifies foo()
to return an error code or -1 −1 rather than 0 to indicate a failure (all of which are also common conventions).
Code Block | ||||
---|---|---|---|---|
| ||||
int validateUser(User usr) {
if(listContains(validUsers, usr)) {
return 1;
}
return 0;
}
void processRequest(User usr, Request request) {
if(!validateUser(usr)) {
return "invalid user";
}
else {
serveResults();
}
}
|
Although the code above will work as intended, it is possible that a future modification will result in the following:
Code Block | ||||
---|---|---|---|---|
| ||||
errno_t validateUser(User usr) {
if(list_contains(allUsers, usr) == 0) {
return 303; // user not found error code
}
if(list_contains(validUsers, usr) == 0) {
return 304; // invalid user error code
}
return 0;
}
void processRequest(User usr, Request request) {
if(!validateUser(usr)) {
return "invalid user";
}
serveResults();
}
|
...
The following compliant code sample is preferable for improved maintenance. By defining what constitutes a failure and explicitly testing for it, the behavior is clearly implied and future modifications are more likely to preserve it. If there is a future modification is made, like the one abovesuch as in the previous example, it is immediately obvious that the if
statement in processRequest()
does not correctly utilize the specification of validateUser()
correctly.
Code Block | ||||
---|---|---|---|---|
| ||||
int validateUser(User usr) {
if(list_contains(validUsers, usr)) {
return 1;
}
return 0;
}
void processRequest(User usr, Request request) {
if(validateUser(usr) == 0) {
return "invalid user";
}
else {
serveResults();
}
}
|
...
Comparison functions (such as the standard library strcmp()
function) return 0 if the arguments are equal and non-zero nonzero otherwise.
Because many comparison functions return 0 for equality and non-zero nonzero for inequality, they can cause confusion when used to test for equality. If someone were to switch the following strcmp()
call with a function testing for equality, but the programmer did not follow the same convention as strcmp()
, the programmer might instinctively just replace the function name. Also, when quickly reviewed, the code could easily appear to test for inequality.
Code Block | ||||
---|---|---|---|---|
| ||||
void login(char *usr, char *pw) {
User user = find_user(usr);
if (!strcmp((user->password),pw_given)) {
grantAccess();
}
denyAccess("Incorrect Password");
}
|
The preceding code above works correctly. However, to simplify the login code or to facilitate checking a user's password more than once, a programmer can separate the password-checking code from the login function in the following way:
Code Block | ||||
---|---|---|---|---|
| ||||
int check_password(User *user, char *pw_given) {
if (!strcmp((user->password),pw_given)) {
return 1;
}
return 0;
}
void login(char *usr, char *pw) {
User user = find_user(usr);
if (!check_password(user, pw)) {
grantAccess();
}
denyAccess("Incorrect Password");
}
|
In an attempt to leave the previous logic intact, the developer just replaces the strcmp()
with a call to the new function. However, doing so produces incorrect behavior. In the this case above, any user who inputs an incorrect password is granted access. Again, two conventions conflict and produce code that is easily corrupted when modified. To make code maintainable and to avoid these conflicts, such a result should never be defaulted.
...
Code Block | ||||
---|---|---|---|---|
| ||||
void login(char *usr, char *pw) {
User user = find_user(usr);
if (strcmp((user->password),pw_given) == 0) {
grantAccess();
}
denyAccess("Incorrect Password");
}
|
...
Code that does not conform to the common practices presented will be presented is difficult to maintain. Bugs can easily arise when modifying helper functions which that evaluate true/false or success/failure. Bugs can also easily arise when modifying code that tests for equality using a comparison function that obeys the same conventions as standard library functions like strcmp
.
Recommendation | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
EXP20-C | medium | probable | low | P12 | L1 |
Related Guidelines
Bibliography
[StackOvflw 2009] "Should I return TRUE/FALSE values from a C function?"
...