平成27年度の基本情報技術者試験の午後の問題のうち、C言語の設問にひどいコードが出ているとのことである。当方もIPA 独立行政法人 情報処理推進機構：問題冊子・配点割合・解答例・採点講評（2015、平成27年）の基本情報技術者試験 平成27年度秋季の問題のを確認したところ、問9のコードがひどいことを確認した。ここでは、どの点で望ましくないのか当方なりに説明して、改善案を提示してみたい。
何がなぜまずいのか
まず、どの点でなぜまずいなのかを説明を行いたい。当方としては、以下の点が挙げられると考えている。
- グローバル変数を使いすぎ
- マジックナンバーの使いすぎ
- 文字列の配列のサイズの根拠が不明瞭
まず、「グローバル変数を使いすぎ」の点だが、このコードでは一つのファイルしか使っていない関係上、グローバル変数の弊害がわかりづらいが、グローバル変数はどのファイルからでも自由にアクセスできてしまう関係上、気づかないところで変数の値が変わってしまい、想定していない動作を招いてしまうことがある(特にライブラリーを使う場合は特に気をつけなければならない)。そのため、基本的には極力ローカル変数、あるいは構造体の変数などを使うようにするのが鉄則である。
次にマジックナンバーの使いすぎであるが、当該コードではそのフォーマットの関係上、やはり弊害は少ないが、配列のサイズなどでマジックナンバーを使っており、配列サイズの意味が不明瞭になってしまっている。
文字列の配列のサイズの根拠が不明瞭という点については、これは上記のマジックナンバーの使いすぎという部分にも関連してくるが、C言語の文字列を表現する場合、通常は文字列のポインターあるいは配列として表現しなければならないが、基本的にはNULL文字を加えた上でのサイズとなることを明示しなければならないが、当該コードではそれが明確になっていない。
改善案
さて、上記の点を踏まえて、当方が考えた改善案を以下に記述してみたい。
[code lang=”c”]#include <stdio.h>
#include <stdlib.h>
#include <string.h>
static const size_t RECORD_CARDID_SIZE = 4;
static const size_t RECORD_DATE_MAX = 8;
static const size_t RECORD_TIME_MAX = 6;
static const size_t RECORD_DOOR_MAX = 2;
static const size_t RECORD_DIR_MAX = 1;
static const size_t RECORD_ACT_MAX = 1;
static const size_t RECORD_NAME_MAX = 10;
typedef struct _record {
char *cardid;
char *date;
char *time;
char *door;
char *dir;
char *act;
char *name;
}record;
static record *create_record();
static void destroy_record(record *obj);
static record *get_record(FILE *file);
static void put_record(record *obj, char *lastid);
int main(int argc, const char * argv[]) {
const char *filename = "Access.Log";
FILE *log_file = fopen(filename, "r");
char lastid[RECORD_CARDID_SIZE + 1];
record *record = NULL;
while (record = get_record(log_file), record) {
put_record(record, lastid);
destroy_record(record);
record = NULL;
}
fclose(log_file);
return 0;
}
static record *create_record() {
record *obj = (record *)calloc(1, sizeof(record));
obj->cardid = (char *)calloc(RECORD_CARDID_SIZE + 1, sizeof(char));
obj->date = (char *)calloc(RECORD_DATE_MAX + 1, sizeof(char));
obj->time = (char *)calloc(RECORD_TIME_MAX + 1, sizeof(char));
obj->door = (char *)calloc(RECORD_DOOR_MAX + 1, sizeof(char));
obj->dir = (char *)calloc(RECORD_DIR_MAX + 1, sizeof(char));
obj->act = (char *)calloc(RECORD_ACT_MAX + 1, sizeof(char));
obj->name = (char *)calloc(RECORD_NAME_MAX + 1, sizeof(char));
return obj;
}
static void destroy_record(record *obj) {
free(obj->cardid);
free(obj->date);
free(obj->time);
free(obj->door);
free(obj->dir);
free(obj->act);
free(obj->name);
free(obj);
}
static record *get_record(FILE *file) {
record *obj = create_record();
// これはフォーマットの関係上やむなし
if (fscanf(file, "%4c%8c%6c%2c%1c%1c%10c\n",
obj->cardid,
obj->date,
obj->time,
obj->door,
obj->dir,
obj->act,
obj->name) == EOF) {
destroy_record(obj);
return NULL;
}else{
return obj;
}
}
static void put_record(record *obj, char *lastid) {
if (strcmp(obj->cardid, lastid) == 0) {
// 18文字分空白を開ける
printf("%18s", " ");
}else{
printf("\n%4s %10s ", obj->cardid, obj->name);
strncpy(lastid, obj->cardid, RECORD_CARDID_SIZE + 1);
}
int put_space = obj->door[0] – ‘0’ – 1;
while (put_space > 0) {
printf("%20s", " ");
put_space–;
}
printf("%.2s-%.2s %.2s:%.2s %2s ",
obj->date + 4, // 月のオフセット分加算
obj->date + 6, // 日のオフセット分加算
obj->time,
obj->time + 2, // 分のオフセット分加算
obj->door);
if (strcmp(obj->act, "R") == 0) {
printf("%s", "(R)");
}
if (strcmp(obj->dir, "I") == 0) {
printf("%s\n", "IN");
}else{
printf("%s\n", "OUT");
}
}
[/code]
こうすることによって、グローバル変数は(const修飾されたものを除いて)根絶され、すべてローカル変数として使われるようになった。また、こうすることによって意図せぬアクセスも減少され、保守性も向上すると考えられる。
ハードコーディングについてはどうしても残っているものの、配列部分などで極力const修飾されたものに置き換えたことと、1回しか使わない部分はコメントを加えることで対処を行なった。
文字列のサイズについてもまずはNULL文字を配慮しない文字数をconst修飾で定義した上で、NULL文字分を+1で加算するという方式を使った。これによってある程度は明確化された。
最後に
時折試験問題に出てくるコードはひどいものが出てくることがあるらしいことはなんとなくわかった。
そういったコードを見かけたときにどうやったらより良いコードができるのかを考えてみると良いだろう。
当方が上記に書いたコードはあくまで改善例の一つの方法だが、コードを修正するにあたってより適切な方法を考えて、適宜修正していきたいものである。
ウェブマスター。本ブログでITを中心にいろいろな情報や意見などを提供しています。主にスマートフォン向けアプリやウェブアプリの開発を携わっています。ご用の方はコメントかコンタクトフォームにて。
