読んでイライラしたコード - プログラミング初心者がアーキテクトっぽく語る

人が書いたエクセルのVBAを読んでいたらイライラした。

自分の稚拙さを棚に上げて文句を言うのも大人気ないし、VBAという言語仕様にも原因があるようだ。全てが本人のせいとは言い切れない。当然、当人は一言も言っていない。

すでにイライラは収まりも気持ちも穏やかだ。しかし感じた内容は自分に当てはまることも多いと思った。自分では正当化しているコードでも他人の目から見るとこう映るんだ、と改めて反省した。戒めの意味を込めて感じたことをメモしておく。

なお文句を言うに当たってVBAという言語の特性は考慮しない。VBAわからんから。コーディング全般に当てはまるような観点で書いていく。

大きすぎる関数

1関数500行だと。。。

途方に暮れる。半分読んだ頃には最初の100行が頭から飛んでいる。無理。人間が理解できるサイズに分割しくれ。論理的に分割されてればそこで頭をリセットできて理解しやすい。

むしろよく書けたね。途中で「自分でも訳わからなくなってきたな。分割するか。」とか思わなかったのか？

これ絶対、試験してないでしょ。試験できないでしょ。これまじで業務で使うの？もう使ってるの？？？？？？

循環的複雑度を意識していない

たまたま目の前にあったFor文の複雑度を数えてみた。約70行。中にFor文が1つ。If/ElseIf/Else文が16こ。最大ネスト6階層。このFor文だけで複雑度18。ループ文一つとっても健全性の欠片もない。循環的複雑度って知ってる？複雑度の高いコードは扱うの大変なんだよ。

簡単に減らせる分岐は極力減らせ。色々方法はある。例えばこれ。

   If XXX Then
        flag = 0
        処理A
    Else
        処理A
    End If

こういうのはこうできる。

    If XXX Then
        flag = 0
    End If
    処理A

用途が不明すぎる変数名

変数名からある程度、用途を連想できるようにして欲しい。新しい変数が登場してもしばらく読まないとその変数の用途がわからないというのは読んでて辛い。

ws
ws1
ws2
ws3

wsは迷わずシート名でいいだろ。

tmp
temp

まぁ、使ってもいいけど狭い範囲に限定して。

変数の再利用

変数の再利用はしないでくれ。なにか前の処理とつながりがあると思ってしまう。

古いコードのコメントアウト

消して。うざい。

コメントアウトされたコードにまで目を通してる前提で会話された日にはさらにうざい。そういう人いるから目を通さざるを得ない。害以外のなにものでもない。

履歴を残したいならGitってあるだろ。

Todoを残したいならチケット起票しろ。

「すぐに戻せるように残してる」って思ってるのかもしれないけど、いざとなったときにはすぐ戻せるわけないんだから。無駄。

仕込んだデバッグコードをコメントアウトするのもよくない。デバッグログならLogger使え。ログ機能として商用化後も使える。ちょっと動作が見たいのならテストコード書け。テストコードなら商用コードの可読性に影響しないし品質も上がる。コメントアウトはゴミしか生まない。非効率。

プロとは思えない。

コード先頭でまとめて変数を宣言

VBAのプラクティスのようだけど、個人的には使うところで宣言してくれた方が読みやすい。いちいち宣言箇所に戻るのが面倒くさい。

プリミティブな操作がそのまんま

例としてよくないかもしれないけど例えば下のようなネイティブなAPI丸出しのコードより

maxRow = ws.Cells(Rows.Count, 1).End(xlUp).Row

こんな風に直感的になにをしているのか理解できる方が嬉しい。

count = fooSheet.countRow(1)

publicな関数だけでもHuman Readableになるよう意識してくれてたら読む時間が1/5で済んだと思う。privateな関数は仕方ないけど。

マジックナンバー

If tmp(5) <= 50 Or 100 <= tmp(5) Then

さっぱりわからん。定数にするとか、JavaならEnumにするとか、条件文自体を関数にするとか。こう、、、色々あるだろ！

コメントと一致しないコード

コメントが間違っているのかコードが間違っているのかわからない。コメントを正確にメンテする気がないならコメントは書かないでくれ。そしてコメントなしでもわかるコードにしてくれるのが一番うれしい。

誤解を招く命名

    Dim car() As String: ReDim car(5)

carって書いてあったら配列だと思わないだろ！

配列ならcarsだろ！

国語力鍛えろ！

ループの中でcontinueしない

ループ内でそのIterationの処理は一通り終わったはずなのにcontinueやbreakがないので延々とループを下まで読んだ結果、やはりなにも処理がなくてまた一番上に戻るということが多々あった。上に戻るためだけに長いループを一番下まで読むのは時間の無駄だ。さっさと区切りのいいところでcontinueしてくれ。

処理を理解するために処理が記述されていないところまで読まなくてはいけないコードは悪だ。