過去のブログを掘り起こして、当時分からずじまいだったことをAIニキの力を借りて解決していくシリーズの6回目です。
第6回目は、2014年3月30日の内容です。
今回は分からずじまいじゃなくて「そーすが汚い！」ってことなんだけど、汚いというか意味ないものが混じってたりなんか色々あるのでお題にしてみました。
というわけで、当時のコードをAIニキと一緒にリファクタリングしてみた。
2014年頃に明解C言語のシリーズの問題を解いていってて、その総仕上げにあたる10章の問題で、単語リストを読み込んでランダムに出題、間違えたら正解を見るか選べて、5回間違えたら強制的に次に進む、最後に正解した月を昇順で表示する、みたいな要件のものだ。
当時はTCP/IPソケットプログラミングC言語を読みたいという動機で基礎からやってたんだね、懐かしいねー。
汚いポイント
グローバル変数が多い
int QNO;
char **jptr;
char **eptr;
ファイルの先頭にどかっと座っている。QNOは問題数で、jptrとeptrは日本語と英語の単語を格納するポインタの配列で、これらをグローバルに置く必要はなくて、構造体にまとめて引数で渡せばきれいだ。
グローバル変数は便利なんだけど、どこからでも触れてしまうので、プログラムが大きくなった時に何がどこで変わるのかわからなくなる、気づいたら誰も触れない変数になってたりする、という感じで、スコープの話かなあ。
変数名は書籍のものを参照したんだと思うんだけど、それと宣言は最初に書いておかないといけないとか思ってたような気もして、と思って調べると昔のC言語（ANSI C/C89）では、ブロックの先頭で変数を宣言しないとコンパイルエラーになったらしい。
なるほどそういうことなのか、と思って、初めてC言語を触った時に使ってたのがLSI C-86（試食版）というもので、これは「ANSI C（C89）」に準拠したサブセットってことで、まさにそれだった。この頃から16年経っているけれどね。
本質を知る旅だ。
インデントがぐちゃぐちゃ
タブとスペースが混在していて、ネストのレベルがぱっと見わからない。当時は多分vimで書いてて、vimrcでもそんな凝ったものとかやってなかったと思う。
こういう癖は結構後になっても残っちまうなあと今になって思うので直した方がいい。
当時メモ書きしていた「治した方がいい癖」にもうひとつ「余った変数の使い回し」というのがあって、とんでもないことを書いていたなあ、余ったってなんだ、というのと、さらに使い回しとか。どちらも書いた瞬間は動けばいいんだけど、あとで読み返すと何をしているのかわからなくなるやつだ。
gotoを使ったエラー処理
if((fp = fopen(argv, "r")) == NULL) goto Free;
C言語においてgotoはまったく使ってはいけないわけではないんだけど、ここは関数を分けることで自然に解決できる。使わない方がいいってどこかで見たと思うけれど、当時はまだ知らなかったのかな。
scanfが危ない
scanf("%s", &answer);
&answerじゃなくてanswerで良くて、しかもサイズ指定がないのでバッファオーバーフローが起きる可能性がある。scanf("%255s", answer)と書くべきところだ。
&を使うという風に覚えていて、本質を見てなかった典型だなあと思うでしかし。
ソートのロジックがおかしい
if(exam > exam){
配列の添字を比較しようとしているんだけど、実際にはポインタのアドレスを比較していて、意図した動きになっていない可能性が高い。なんだこれは。
リファクタリングするとこうなる
まず単語を構造体にまとめる。
typedef struct {
char *japanese;
char *english;
} Word;
typedef struct {
Word *words;
int count;
} WordList;
日本語と英語をセットで管理することで、jptrとeptrという2つの配列を別々に持ち歩かなくて済む。
ファイルの読み込みは関数に切り出して、エラー処理もgotoなしで書ける。
int load_wordlist(const char *filename, WordList *wl) {
FILE *fp = fopen(filename, "r");
if (fp == NULL) {
fprintf(stderr, "ファイルを開けません: %s\n", filename);
return 0;
}
// ...
fclose(fp);
return 1;
}
ランダムな出題順はFisher-Yatesというアルゴリズムで書けて、これが一番きれいなシャッフルの方法らしい。
void shuffle(int *arr, int n) {
int i, j, tmp;
for (i = n - 1; i > 0; i--) {
j = rand() % (i + 1);
tmp = arr[i];
arr[i] = arr[j];
arr[j] = tmp;
}
}
1問出題するのも関数にして、正解なら1、不正解なら0を返すようにする。
int ask_question(int stage, const Word *word) {
char answer[MAX_LEN];
int miss = 0;
printf("\n%d問目\n%s ? : ", stage, word->japanese);
while (1) {
scanf("%255s", answer);
if (strcmp(answer, word->english) == 0) {
printf("せいかい！！\n");
return 1;
}
// ...
}
}
ソートは挿入ソートでちゃんと書き直す。
void insertion_sort(int *arr, int n) {
int i, j, key;
for (i = 1; i < n; i++) {
key = arr[i];
j = i - 1;
while (j >= 0 && arr[j] > key) {
arr[j + 1] = arr[j];
j--;
}
arr[j + 1] = key;
}
}
リファクタリング後のフルソース
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#define MAX_LEN 256
#define MAX_MISS 5
typedef struct {
char *japanese;
char *english;
} Word;
typedef struct {
Word *words;
int count;
} WordList;
void free_wordlist(WordList *wl) {
int i;
if (wl->words == NULL) return;
for (i = 0; i < wl->count; i++) {
free(wl->words[i].japanese);
free(wl->words[i].english);
}
free(wl->words);
wl->words = NULL;
wl->count = 0;
}
int load_wordlist(const char *filename, WordList *wl) {
int i;
FILE *fp = fopen(filename, "r");
if (fp == NULL) {
fprintf(stderr, "ファイルを開けません: %s\n", filename);
return 0;
}
fscanf(fp, "%d", &wl->count);
wl->words = calloc(wl->count, sizeof(Word));
if (wl->words == NULL) { fclose(fp); return 0; }
for (i = 0; i < wl->count; i++) {
char etemp[MAX_LEN], jtemp[MAX_LEN];
fscanf(fp, "%s%s", etemp, jtemp);
wl->words[i].english = strdup(etemp);
wl->words[i].japanese = strdup(jtemp);
if (wl->words[i].english == NULL || wl->words[i].japanese == NULL) {
fclose(fp);
free_wordlist(wl);
return 0;
}
}
fclose(fp);
return 1;
}
void shuffle(int *arr, int n) {
int i, j, tmp;
for (i = n - 1; i > 0; i--) {
j = rand() % (i + 1);
tmp = arr[i]; arr[i] = arr[j]; arr[j] = tmp;
}
}
int ask_question(int stage, const Word *word) {
char answer[MAX_LEN];
int miss = 0;
printf("\n%d問目\n%s ? : ", stage, word->japanese);
while (1) {
scanf("%255s", answer);
if (strcmp(answer, word->english) == 0) {
printf("せいかい！！\n");
return 1;
}
printf("間違いです。\n正解をみたいですか？？ yes = 9 : ");
int choice;
scanf("%d", &choice);
if (choice == 9) {
printf("正解は%sですよ。\n\n", word->english);
return 0;
}
if (++miss >= MAX_MISS) {
printf("正解は%sですよ。\n\n", word->english);
return 0;
}
printf("入力してください\n%s ? : ", word->japanese);
}
}
void insertion_sort(int *arr, int n) {
int i, j, key;
for (i = 1; i < n; i++) {
key = arr[i];
for (j = i - 1; j >= 0 && arr[j] > key; j--)
arr[j + 1] = arr[j];
arr[j + 1] = key;
}
}
int main(int argc, char *argv[]) {
int i;
if (argc < 2) {
fprintf(stderr, "使い方: %s <単語ファイル>\n", argv[0]);
return 1;
}
WordList wl = { NULL, 0 };
printf("ファイル名 %s\n", argv[1]);
if (!load_wordlist(argv[1], &wl)) {
fprintf(stderr, "単語ファイルの読み込み失敗\n");
return 1;
}
srand((unsigned)time(NULL));
int *order = malloc(wl.count * sizeof(int));
int *correct = malloc(wl.count * sizeof(int));
if (order == NULL || correct == NULL) { free_wordlist(&wl); return 1; }
for (i = 0; i < wl.count; i++) order[i] = i;
shuffle(order, wl.count);
int ok = 0;
for (i = 0; i < wl.count; i++) {
if (ask_question(i + 1, &wl.words[order[i]]))
correct[ok++] = order[i];
}
insertion_sort(correct, ok);
printf("正解したのは");
if (ok == 0) {
puts("ない！もっとがんばれ");
} else {
printf("%d個で、", ok);
for (i = 0; i < ok; i++) printf("%s ", wl.words[correct[i]].japanese);
puts("");
}
free(order);
free(correct);
free_wordlist(&wl);
return 0;
}
まとめ
直した方がいいと思っていたことは、ちゃんと直しておくべきだったなあと思う、当時の自分に言いたいというか、でも当時の自分はたぶん次に行くことで頭がいっぱいで、目標はソケットinCを読むということだったし、それはちゃんと当時達成していたので、急いで行かないといけないことがたくさんあったんだろう、12年越しに向き合えたのでよしとしよう。