C 言語で split 処理を関数化したらコピペの方が良いと言われた
区切り文字があるデータ構造を分割する場合、Python なら以下のように split メソッドを使用すれば良いです。
$ python -c "print 'a,b,c,33,e'.split(',')" ['a', 'b', 'c', '33', 'e'] $ python -c "print 'a,,c,,e'.split(',')" ['a', '', 'c', '', 'e']
今、この処理を C 言語でやりたいとします。私は C 言語の開発経験がほとんどないのですが、引き継いだコードでは、strchr() で区切り文字のポジションを取得し、そのポジションに NULL 文字をセットして strdup() で取得します。C 言語には文字列の概念がないために strchr() のような関数に渡される引数は NULL 文字で終わるバイナリデータ = 文字列として解釈します*1。つまり NULL 文字のポジションを操作することでサブ文字列を取得するような実装になっていました。簡単に表すと、以下のような実装でした。
pos = strchr(str, delim);
*pos = '\0';
info = strdup(str)
実際に動くコードを copy_and_paste_strchr_many_times() と split_by_strchr() の2つの方法で実装してみます。
2010/6/10 split_by_strchr() の仕様変更とメモリリーク対策のために修正
2010/6/12 split_by_strchr() のさらなる修正
#include <stdio.h> #include <stdlib.h> #include <string.h> #define ERROR 0 #define OK 1 typedef struct { char *first; char *second; char *third; int fourth; char *fifth; } data_info_t; int copy_and_paste_strchr_many_times(char *str, data_info_t *info) { int ret = ERROR; char *look; char *next; char *tmp_str = NULL; tmp_str = strdup(str); if (tmp_str == NULL) { fprintf(stderr, "cannot strdup: %s\n", str); goto END_OF_FUNC; } look = tmp_str; next = strchr(look, ','); if (next == NULL) { fprintf(stderr, "broken first data\n"); goto END_OF_FUNC; } if (look != next) { *next = '\0'; info->first = strdup(look); if (info->first == NULL) { fprintf(stderr, "cannot strdup: %s\n", look); goto END_OF_FUNC; } } look = next + 1; next = strchr(look, ','); if (next == NULL) { fprintf(stderr, "broken second data\n"); goto END_OF_FUNC; } if (look != next) { *next = '\0'; info->second = strdup(look); if (info->second == NULL) { fprintf(stderr, "cannot strdup: %s\n", look); goto END_OF_FUNC; } } look = next + 1; next = strchr(look, ','); if (next == NULL) { fprintf(stderr, "broken third data\n"); goto END_OF_FUNC; } if (look != next) { *next = '\0'; info->third = strdup(look); if (info->third == NULL) { fprintf(stderr, "cannot strdup: %s\n", look); goto END_OF_FUNC; } } look = next + 1; next = strchr(look, ','); if (next == NULL) { fprintf(stderr, "broken fourth data\n"); goto END_OF_FUNC; } info->fourth = atoi(look); look = next + 1; if (*look) { info->fifth = strdup(look); if (info->fifth == NULL) { fprintf(stderr, "cannot strdup: %s\n", look); goto END_OF_FUNC; } } ret = OK; END_OF_FUNC: if (tmp_str) { free(tmp_str); } return ret; } void free_split_data(char ***data, int data_num) { int i; if ( *data && data_num != 0) { for (i = 0; i < data_num; i++) { if ((*data)[i]) { free((*data)[i]); } } free(*data); data = NULL; } } int split_by_strchr(char *str, char ***data, int *data_num, int delim) { int ret = ERROR; int i; char *tmp_str, *start_pos, *pos; if (str == NULL || data_num == NULL) { fprintf(stderr, "str or data_num is NULL\n"); return ret; } *data_num = 0; start_pos = tmp_str = strdup(str); if (start_pos == NULL) { fprintf(stderr, "cannot strdup: %s\n", str); goto END_OF_FUNC; } while ( 1 ) { (*data_num)++; pos = strchr(tmp_str, delim); if (pos == NULL) { break; } tmp_str = ++pos; } *data = (char **)calloc(sizeof(char *), *data_num); if (*data == NULL) { fprintf(stderr, "cannot malloc\n"); goto END_OF_FUNC; } tmp_str = start_pos; if (*data_num == 1) { *(data)[0] = tmp_str; } else { for (i = 0; i < *data_num - 1; i++) { pos = strchr(tmp_str, delim); *pos = '\0'; (*data)[i] = strdup(tmp_str); if ((*data)[i] == NULL) { fprintf(stderr, "cannot strdup: %s\n", tmp_str); goto ERROR_END; } tmp_str = ++pos; } (*data)[i] = strdup(tmp_str); if ((*data)[i] == NULL) { fprintf(stderr, "cannot strdup: %s\n", tmp_str); goto ERROR_END; } } ret = OK; END_OF_FUNC: free(start_pos); return ret; ERROR_END: free_split_data(data, *data_num); free(start_pos); return ret; } void usage() { fprintf(stdout, "./sample_split a,b,c,0,e\n"); } int main(int argc, char **argv) { int ret, data_num; char **data; data_info_t info1, info2; memset(&info1, 0, sizeof(data_info_t)); memset(&info2, 0, sizeof(data_info_t)); memset(&data, 0, sizeof(data)); if ( argc < 2 ) { usage(); return EXIT_SUCCESS; } // もともとの処理 ret = copy_and_paste_strchr_many_times(argv[1], &info1); if (ret == ERROR) { fprintf(stderr, "copy and paste something error\n"); return EXIT_FAILURE; } fprintf(stdout, "*** copy_and_paste_strchr_many_times:\n"); fprintf(stdout, "%s\n%s\n%s\n%d\n%s\n", info1.first, info1.second, info1.third, info1.fourth, info1.fifth); free(info1.first); free(info1.second); free(info1.third); free(info1.fifth); // split 関数を使用した処理 ret = split_by_strchr(argv[1], &data, &data_num, ','); if (ret == ERROR) { fprintf(stderr, "copy and paste something error\n"); return EXIT_FAILURE; } info2.first = data[0]; info2.second = data[1]; info2.third = data[2]; if (data[3]) { info2.fourth = atoi(data[3]); } info2.fifth = data[4]; fprintf(stdout, "*** split_by_strchr:\n"); fprintf(stdout, "%s\n%s\n%s\n%d\n%s\n", info2.first, info2.second, info2.third, info2.fourth, info2.fifth); free_split_data(&data, data_num); return EXIT_SUCCESS; }
実行結果。
$ ./sample_split03 a,b,c,33,e *** copy_and_paste_strchr_many_times: a b c 33 e *** split_by_strchr: a b c 33 e $ ./sample_split03 a,,c,,e *** copy_and_paste_strchr_many_times: a (null) c 0 e *** split_by_strchr: a c 0 e
copy_and_paste_strchr_many_times(もとの処理) では引数として渡した構造体にほとんど同じような処理をコピペして何度も呼び出すことでサブ文字列を格納しています(実際に引き継いだコードでは数値や time_t 型として扱うサブ文字列もあり、全部で十数個のサブ文字列になり、もう少し複雑です)。
最初にこのコードを見たとき、split のような処理として汎用的に関数化しておけば、同じような処理に再利用できて良いし、その方がメンテナンス性も高いだろうと私は考えて split_by_strchr() を実装しました。基本的には strchr() を利用して split 処理を行い、文字列のポインタ配列を作成して、構造体に対応するメンバにセットするように変更しました。
この変更を行って3人の開発者にコードレビューをしてもらったところ、
もとのコピペした処理の方が良い
と3人の開発者全員に指摘されました。その理由は以下の通りです。
- 固定長のデータ構造を扱うシンプルな処理に汎用的な split 関数はオーバースペックである
- C 言語では要素の型が違う配列を扱えないから構造体を定義して引数として渡しているのに文字列の配列に一旦格納しているのがおかしい
- もとの処理の方が扱うデータ構造に対応する処理が関数内に全て網羅されているので、どこで何をしているかが分かり易い
- もとの処理の方がシンプルなのでパフォーマンスが良い
- もとの処理の方がデバッグし易い
- split 関数にバグがあった場合、それを利用する全てのコードに影響が及ぶ
私が考えた split 関数のメリットは以下の通りです。
- 再利用することができる
- 扱うデータ構造の何番目の要素が構造体のどのメンバに対応するのかが分かり易い
- サブ文字列に分割する処理と型変換(文字列->整数)等の処理を分離する方が保守性が高い(と私は思う)
私よりも経験のある開発者とコードレビューで何度かお話していて、関数分割の設計方法に私と根本的に違う考え方があることに気付きました。私はコードの見た目が同じような処理は共通ロジック部分を抜き出して関数化するのが良いと考えていました。
今回、コードレビューでお話した開発者は、1つの機能(関数)の中で一部の機能を提供する関数を呼び出すよりも、その関数内で網羅的に実装することの方が良いと考えていることが分かりました。コードの見た目が同じだからその共通部分を関数化するということよりも、求められる機能が全く同じかどうかで関数化するかどうかを意識しているように受けとれました。つまり、2つの関数があったとして、その中で行う処理にそれぞれ30%、70%の共通部分があった場合、私はその共通部分を抜き出して関数化しますが、コードレビューでお話した開発者はそれをしないということです。
また関数呼び出しがあると、その関数のロジックを確認するために、今、読んでいるコード部分から移動して、その関数が定義されている箇所を読みにいくことそのものが思考の分断になり、分り難いということも私によく言われます。私は関数定義のロジックを確認するためにソースの場所を移動することはあまり気になりません。しかし、よくよく考えてみると、私も複数のエディタ画面上にメイン関数とそこから呼び出されるサブ関数のソースを同時に見ています。もし、これが1つしか見れないならば、1つの関数内に網羅的にコードを書く方が分かり易いというのも理解できます。
結論として、これまでコードの見た目だけで関数化を安易に考えてしまっていましたが、機能を考慮した設計を行っていくことでコピペが良い悪いというような議論にさえならない実装になっていくのかなと思いました。いずれにしても、コピペは良くないという先入観で思考停止するのではなく、もっと背景を考慮して視野を広げることが重要だと気付きました。
