EXP20-C. 成功、真偽、等価を判定するには明示的な検査を行う

成功、真／偽、等価を判定するには、コードの読みやすさと保守しやすさの向上、および一般的な慣習との互換性のために、明示的な検査を行うこと。

特に、非 0 の検査を省略しないこと。たとえば、foo() 関数が失敗のときに 0 を返し、成功のときに 0 以外の値を返すとき、関数の返り値が 0 でないことは次のようにして検査する。

if (foo() != 0) ...

0 は失敗を示すのが慣習となっているが、上記のコードは、次のコードよりも好ましい。

if (foo()) ...

0 でないことを明示的に検査することで、のちに foo() が失敗時に 0 ではなく −1 を返すように変更された場合に保守しやすいという利点がある。

このレコメンデーションは、次の一般的な慣習に由来する。

1. 関数は偽の場合は 0 を返し、真の場合は 0 以外を返す[StackOvflw 2009]。
2. 関数呼び出しの失敗は、通常は −1 または 0 以外の数値によって示される。
3. 比較関数は、引数が同等ならば 0 を返し、それ以外の場合は 0 以外を返す (たとえば標準ライブラリ関数 strcmp() は 3 通りの返り値をもっている) (strcmp 関数を参照)。

違反コード

次のコード例では、is_banned() は偽ならば 0 を返し、真ならば 0 以外の値を返す。

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();
}

権限のないユーザが2 回リストされていると、そのユーザにはアクセスが許可される。is_banned() は、真ならば 0 以外を返すという一般的な慣習に従っているものの、processRequest は 1 と等しいかの検査しか行っていない。

適合コード

ほとんどの関数は、真の場合に 0 以外の値を返すことしか保証しないため、上記のコードは、次のように 0（偽）と等しくないことを検査することで改善される。

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)==0) {
      x++;
    }
    cur_node = cur_node->next;
  }

  return x;
}

void processRequest(User usr) {
  if (is_banned(usr) != 0) {
    return;
  }
  serveResults();
}

違反コード

次の違反コードでは、関数の処理結果が失敗の場合は −1、成功の場合は非負の数値を返している。これは C 言語標準ライブラリにおいても一般的な慣習となっているが、「ERR02-C. 正常終了時の値とエラーの値は別の手段で通知する」では推奨されていない。

返り値 0 で失敗を表している場合、返り値が 0 でないことを明示的に検査しなければ、一般的な慣習に沿ったコードとの間で問題を起こすかもしれない。以下の例では、0 でないことを検査していないため、開発者の誰かが validateUser() を変更して、失敗の場合に 0 ではなくエラーコードまたは −1 を返すようにした場合（これらもすべて一般的な慣習である）バグが生じる。

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();
  }
}

このコードは意図どおりに動作するだろうが、例えば次のようにこのコードを変更したらどうなるだろうか。

errno_t validateUser(User usr) {
  if(list_contains(allUsers, usr) == 0) {
    return 303; /* ユーザが見つからないことを示すエラーコード */
  }
  if(list_contains(validUsers, usr) == 0) {
    return 304; /* 無効なユーザであることを示すエラーコード */
  }

  return 0;
}

void processRequest(User usr, Request request) {
  if(!validateUser(usr)) {
    return "invalid user";
  }
  else {
    serveResults();
  }
}

この変更は、検証に失敗した理由をエラーコードで伝えることを意図したものである。しかし、書き換えたコードでは無効なユーザや存在しないユーザが認証されてしまう。processRequest() の中で明示的な検査を行っていないため、論理上の間違いが一見してわかりにくい。

適合コード

以下の適合コードは、保守のしやすさが改善されているため、より望ましい。何が失敗となるかを定義し、失敗かどうかの検査を明示的に行うことで動作が明確に定義され、将来変更されても当初の動作が維持される可能性が高い。前述のような変更が将来行われた場合、processRequest() の中の if 文が validateUser() の仕様を正しく活用していないことが一見してわかる。

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();
  }
}

違反コード

比較関数（標準ライブラリ関数 strcmp() など）は、引数が等しい場合は 0 を返し、等しくない場合は 0 以外の値を返す。

多くの比較関数は、等価の場合は 0 を、非等価の場合は 0 以外を返すため、等価の検査に使われるときには混乱を招くことがある。次の strcmp() の呼び出しを、等価の判定を行う関数に変更する場合を考えてみよう。その関数の挙動は strcmp() と異なるにもかかわらず、プログラマは単純に関数名を置き換えてしまうかもしれない。さらに、一見するとこのコードは等しくないことを検査しているように見えやすい。

void login(char *usr, char *pw) {
  User user = find_user(usr);
  if (!strcmp((user->password),pw)) {
    grantAccess();
  }
  else {
    denyAccess("Incorrect Password");
  }
}

上記のコードは正しく動作する。しかし、ログインコードを簡素化するため、あるいはユーザのパスワードを 2 回以上検査する処理を容易にするために、次のようにパスワード検査コードをログイン関数から切り離したらどうなるだろうか。

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();
  }
  else {
    denyAccess("Incorrect Password");
  }
}

元のロジックをそのまま残そうとして、プログラマは単に strcmp() を別の関数の呼び出しに置き換えている。しかし、このように変更したことで間違った動作が引き起こされる。この例では、間違ったパスワードを入力したすべてのユーザにアクセスが許可される。ここでも、2 つの相反する慣習のため、変更されたときに壊れやすいコードになっている。コードを保守しやすくし、上記のような矛盾を避けるために、このような結果を放置してはならない。

適合コード

この目的に比較関数を使う場合、以下のようなアプローチが望ましい。明示的な検査を行うことで、等価検査を変更しようとする際に、暗黙的な動作や従っている慣習についてプログラマが明確に理解することができる。

void login(char *usr, char *pw) {
  User user = find_user(usr);
  if (strcmp((user->password),pw) == 0) {
    grantAccess();
  }
  else {
    denyAccess("Incorrect Password");
  }
}

リスク評価

このレコメンデーションに示した一般的なコーディング手法に従わないコードは、保守が困難になる可能性がある。真／偽または成功／失敗を評価するヘルパー関数を変更するときに、バグが生じやすくなる可能性がある。また、strcmp のような標準ライブラリ関数と同じ慣習に従った比較関数を使って等価の検査を行うコードを変更するときにも、バグが生じやすくなる可能性がある。

レコメンデーション	深刻度	可能性	修正コスト	優先度	レベル
EXP20-C	中	中	低	P12	L1

自動検出（最新の情報はこちら）

Tool	Version	Checker	Description
LDRA tool suite	9.5.6	114 S	Partially implemented
Parasoft C/C++test	9.5	CODSTA-60	Partially implemented
PRQA QA-C	8.2	3344 4116

参考資料

[StackOvflw 2009]

"Should I Return TRUE/FALSE Values from a C Function?"

翻訳元

これは以下のページを翻訳したものです。

EXP20-C. Perform explicit tests to determine success, true and false, and equality (revision 73)

Top へ