リーダブルコードを読んで過去の自分のコードをリファクタリングしてみた
リーダブルコードを読んでみて、理解定着のために自分のコードをリファクタリングをしてみました。今回僕が目指したのは「正しさ」というよりは「自分の理解」なので、温かい目で見てもらえると嬉しいです。
さて今回使用したコードですが、私が学部2年の時にC言語で書いた"メモリ管理シミュレーション"で、ざっくりと説明すると、OSのメモリ管理アルゴリズム(ファーストフィットとベストフィット)を視覚的に捉えられるようにするためのプログラムです。もちろん本当にメモリ操作をしているというわけではなくて、メモリに見立てた図を標準出力するだけです。
プログラムを実行すると、コマンド入力が求められます。
利用可能なコマンドは以下の通りです。
| コマンド | 使い方 | 意味 |
|---|---|---|
| a | a メモリサイズ | "メモリサイズ" 分のメモリを割り当てる |
| d | d ノードID | "ノードID" で指定したノードを解放する |
| c | c | 空きセグメントをまとめる(デフラグ) |
| p | p | ノードリストを表示する |
動作イメージ
はじめに容量64KBのメモリが以下のように存在していて
| ----------------64(H)---------------- |
ここに「a 4」というコマンドを入力すると(プロセスに4KB分を割り当てると)
| -4(P)- | ----------60(H)--------------- |
というふうに空きセグメントを分割して割り当てます(P:プロセス,H:空きスペース)
※メモリ管理のアルゴリズムには(代表的なOSのメモリ管理アルゴリズムである)First-fitとBest-fitが利用可能で、デフォルトではFirst-fitを利用するようになっています。実行時に -bオプションを付与することによりBest-fitに変更することができます。
割り当てたプロセスにはIDが設定されており、この値は0から始まって自動でインクリメントされていきます。このIDはプロセスを解放するときに使用され、先ほどの例であれば「d 0」のように入力すれば
| -----------------64(H)--------------- |
となります。
ちなみに、先ほどのコマンド一覧で登場した「p」コマンドを使用すると、
その時点でのメモリの状況を確認することができます。
command:p P(0 0 5) P(1 5 6) P(2 11 8) H(19 45) |PPPPP|PPPPPP|PPPPPPPP|HHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHH|
上記が実行例です。メモリの状態は
| P(ID,開始アドレス,サイズ) | H(開始アドレス,サイズ) |
という表記でまず示され、続く1行で'P'or'Hで表されています(全64文字)。
実際は、pコマンドを毎回実行せずとも、aまたはdコマンドを実行した際にpコマンドも自動で呼び出されます。
次に説明するのはcコマンドです。これは断片化した空き領域を統合するコマンドで、一番わかりやすい言葉で置き換えると"デフラグ"です(ただ、dコマンドは既に別のコマンドとして使用しているため、compactionから"c"を取ってきています)
例えばメモリが以下のような状態であったとします
| -4(P)- | -4(H)- | -20(P)- | ----18(H)---- | ---18(P)--- |
この状態でcコマンドを利用すると、断片が整理されて
| -4(P)- | -20(P)- | ---18(P)--- | --------22(H)------- |
という状態になります。
コマンドの説明は以上ですが...実はこのプログラムにはもう1つ機能があります。
このプログラムは当初、First-fitとBest-fitのどちらが優れているかを調べるため(の指標を得るため)に書きました。一般に、First-fitは「メモリ効率は悪いが早い」Best-fitは「メモリ効率は良いが遅い」と言われており、僕はこの”効率”と”早さ”について調べるために、compaction()が実行された回数や連結リストを辿った回数などを記録しています。
という訳で、プログラムを終了時にこの統計情報も出力されるようになっています
（なお、-dオプションをつけて実行すると毎コマンド実行時に統計情報が出力されます）
コード
リファクタリング前:
Programs/MEM_ALLOC.c at master · ymhr-u/Programs · GitHub
リファクタリング後:
Programs/MEM_ALLOC_FIXED.c at master · ymhr-u/Programs · GitHub
（このリポジトリは、僕が学部時代に授業や課外で実装したコードをまとめたもの）
リファクタリング前のものは、パッと見ただけでも読みにくいということがわかると思います。変数の名前も使い方も雑だし、必要なコメントが無いのに不必要なコメントが残っていて...実際自分もリファクタリングをしようと2年ぶりに見返した時に嫌になりました。
変更点
変数名(関数名)
リーダブルコードでも、変数名については色んなところで言及されています。わかりやすいコードを書く上で、変数名を適当に設定することは許されません。
今回、相当な数の(8割以上?の)変数名を変更したのでそれを一から説明しても良いのですが...さすがにとんでも無い文量になるので、1つだけピックアップして紹介したいと思います。
ダメな変数名の例
int alloc_flag; /*ここが1の状態でallocate()を実行するとbest_fitを実行*/
コメントもつけていますが要はフラグ変数です。intで定義しているという点はいったん置いておいて（C言語なのでbool型がありません）、"alloc_flag"という名前を見て「メモリ管理のアルゴリズムを切り替える変数なんだな」と理解できる人がいるとは思えません。
せめて"flag_bestfit"とかであれば最低限の意図は汲み取ってもらえるかもしれませんが...仮にそうであったとしても、「値が0の時にbest_fitなのか、1の時にbest_fitなのか」までは明確に伝えられません。役割を誤解なく的確に伝えられる名前が最適なので、それを考慮して今回は
int do_bestfit; /*フラグ変数:TRUEの状態でallocate()を実行するとbest_fitを実行*/
と書き換えました（#define TRUE 1として定義しました）
この他にも例えば、
- 同じスコープで同じ役割を持つ変数が違う名前で定義されていたり
- ループ変数で外側にj、内側にiを使っていたり
- 使いもしない変数が至る所に散らばっていたり
といった感じでとにかく酷かったです。全部説明したいのですが...文字数がとんでもないことになる（実際書いたら数万字になった）ので省きます。コードを見て確認していただけますと幸いです。参考までに、変更した変数名はこの記事の最後に一覧でまとめておきます。
形による見やすさ
今回リファクタリングしてみて、意外と大切だなって感じたのが「形」です。
まずは、リファクタリング前の以下のコードを見てください。
new->begin = x->begin+size ;
new->segment=-1;
new->size=x->size-size;
new->prev = x;
new->next = x->next;
x->next= new;
x->segment = segno++;
x->size = size;
これはallocate関数の中から抜粋しました。xとnewはポインタであるため、中の要素にアクセスするのには->(アロー)を使います。ぱっと見、ごちゃごちゃしてるという印象を受けるかと思います。
これ、=の位置を縦に揃えるだけでかなり見やすくなります。
new_segment -> segment_id = -1;
new_segment -> begin_address = found_segment -> begin_address + requested_mem_size;
new_segment -> size = found_segment -> size - requested_mem_size;
new_segment -> prev_node = found_segment;
new_segment -> next_node = found_segment -> next_node;
found_segment -> segment_id = seg_id_counter++;
found_segment -> size = requested_mem_size;
found_segment -> next_node = new_segment;
実際は、変数名が長くなっていますし見づらくなってもおかしくないんですが、スッと理解できるようになっていると思います。場合によってはここからさらに():丸括弧で囲むというのもいいと思います。例えば2行目右辺を↓みたいに。
(found_segment -> begin_address) + requested_mem_size;
まぁどこまでやるかは人それぞれだと思いますが、常に意識しておくのは大切です。
あとコード整形というところでいうと、今回はブロック{}の改行位置とかも意識しました。例えば、if文を書くだけでも
//1 if(){} //2 if(){ } //3 if() { }
...のように改行の仕方は様々です。
今まで自分は1,2つをよく使っていました。ただ、ネストが深くなったりブロック内部の行数が増えた時に、3つ目を使った方が見やすいと感じるケースもあって、そのあたりの使い分けがとても難しく感じました。また「自分にとって読みやすいと思っても、統一性がなければひょっとしたら他の人からは汚く見えるかもしれない...」とかも考え、いろいろ試して、最終的に行きついたルールは
- 1,2行程度の短いブロックになるなら2番目を使う
- それ以上の行数になるなら3番目を使う
...で、原則これに則ってリファクタリングしました。
が、これが正解かと言われると難しいですね。
会社でコード書くことになったら恐らくコーディング規約とかあると思いますし、基本的にはそれを守るかたちでいいのかなと思っています。
コメント
意識したのは「コードの動作をそのまま説明したコメントは基本的に書かない」ということです。本の中では”上位レベルのコメント”とも言われていましたが、つまりは「動作そのものを説明するのではなく、その動作の意味や目的を説明する」ということを心がけていました。
そういった意味で言うと、修正前のコードには”良い”コメントはほとんどなく、「なぜか日本語と英語のコメントが混在していて」「説明が不親切すぎて伝わらない」...そんなものばかりでした。
とは言え、いざ良いコメントを書こうと意識してみても...それが意外と難しいことに気付きました。必要以上にコメントを残してしまうと見づらくなってしまうし、そもそも上位レベルのコメントってどの程度まで上位である必要があるのか...などいろいろ考えさせられました。
今回どんなコメントを書いたのかに関しては、修正後のプログラムをご確認いただければと思います。
構造
先ほど言及した ”形による見やすさ” とちょっと似た話かもしれませんが、ここで言及するのはそういった表面的なものではなくて、「制御フロー」「巨大な式の分割」「下位問題や汎用コードの抽出」といった、より内部的な整形になります。
今回のリファクタリングでも結構な量の改善点が見つかりました。
例えばこの処理
// switch文のcase 'a'から抜粋しました↓ if(bflag){ printf("allocate %dKB (best fit)\n",a); alloc_flag = 1; if(a<=0){printf("allocate size error\n");} else{allocate(a);} a = -1; break; }else{ printf("allocate %dKB (first fit)\n",a); alloc_flag =0; if(a<=0){printf("allocate size error\n");} else{allocate(a);} a = -1; break; }
そもそも改行位置や変数名などに問題があって見にくいのですがこの処理では、bflagの値によって、メモリをFirst-fitで割り当てるかBest-fitで割り当てるかを判断しています。このbflagは、実は冒頭に出てきたalloc_flagという変数と全く同じはたらきをする変数で、そもそも存在意味がありません。むしろ同じ役割の変数が2つ以上存在していることによって混乱する人が出てくるという意味で"悪い"変数です。
また、全体の構造について見てみると、if{}の中身とelse{}の中身がほとんど同じなので、共通部分はまとめてしまった方がすっきりするし確実に読みやすくなります。改善したものが↓になります。
// aという変数名は、後ほどrequest_sizeという名前に変えてますが、
//上記コードとの比較のためにあえてaのままで残しています。
if (a <= 0){
printf("Can't allocate. Request size is too small.\n");
break;
}
if (do_bestfit){
printf("allocate %dKB (best fit)\n", a);
}else{
printf("allocate %dKB (first fit)\n", a);
}
allocate(a);
break;これだとどうでしょうか。かたまりが小さくなった分、読みやすくなったと思います（修正前のコードがひどすぎただけとも言えそうですが、まぁ数年前に書いたコードなので...）
こんな感じの修正を全体的に施していく過程で、プログラムの構造をはっきりと捉えられるようになり、無駄な処理を取り除くこともでき、バグまで発見できました。ちなみに今回はプログラムを1つのファイルにまとめてしまっているため1つのファイルがとても長くなっていますが、ヘッダや実装を分けることでもっと見やすくなります（今回はそこまでやっていません）
感想
もうすでに6500字超えちゃってるのでさすがにこれ以上は書きませんが、「コードをきれいに書く」というたったそれだけのことでも相当頭を使うというのが今回のことで本当によくわかりました（今までそういったことをサボっていたから余計にそう感じるんだと思いますが...）
修正前のコードはきっと自分で書いたものじゃなければまともに読めなかったと思いますし、さすがにそういったコードを社会に出て書くわけにはいかないので...今回とてもいい機会になりました。修正後のコードが「読みやすい」かどうかはわかりませんが、少なくとも意識的な部分の矯正はできたと思います。
ー参考ー
以下、変数名の変更箇所一覧です（抜けがあるかもしれません）
外部変数
| 修正前 | 修正後 | 意味 |
|---|---|---|
| segno | seg_id_counter | 新規ノード作成時に割り当てるセグメント番号のカウンタ変数 |
| debug | -削除- | (do_debug変数と同じ) |
| n_alloc | num_allocation | allocate()を実行した回数 |
| n_dealloc | num_deallocation | deallocate()を実行した回数 |
| n_traverse | num_taverse | allocate() でリンクをたどった回数の積算 |
| alloc_flag | do_bestfit | フラグ変数:TRUEの状態でallocate()を実行するとbest_fitを実行 |
| total_req_size | total_reqested_size | allocate()で要求されたメモリサイズの積算 |
| non_exist_seg | num_notExist_segID | deallocate(id)で、id指定されたセグメントが存在しなかった回数 |
| not_enough_mem | -削除- | (num_oversize_reqestと同じ) |
| n_compaction | num_compaction | compaction()を実行した回数 |
| over_size | num_oversize_reqest | 空セグメントのサイズ合計以上の要求を行った回数 |
| dflag | do_debug | フラグ変数:TRUEにすると、allocate()実行ごとに統計表示 |
| bflag | -削除- | (do_bestfitと同じ) |
main() 内
| 修正前 | 修正後 | 意味 |
|---|---|---|
| cmd | cmd | 入力コマンド |
| a | arg | 入力引数 |
allocate(), deallocate()内
| 修正前 | 修正後 | 意味 |
|---|---|---|
| size | requested_mem_size | 割り当てるメモリサイズ |
| num_seg | target_id | 削除対象のセグメントID |
| x | found_segment | first-fit/best-fitで見つけた空きセグメント |
| new | new_segment | メモリ分割後に空きになるセグメント |
| cur | index_node | ループ変数として使われるnode型変数 |
merge(), compact()内
| 修正前 | 修正後 | 意味 |
|---|---|---|
| p | deleted_node | 削除されたnode |
| tail_node(新規追加) | 連結リストの末尾node | |
| before_begin | -削除- | (使われないのに宣言されていた) |
| after_begin | -削除- | (使われないのに宣言されていた) |
