エクセルのVBAにツッコむ
エクセルのVBAには固有の仕様や作法があります。 また、利用者の裾野が広いため、可読性の高いコードを書くスキルを学習する機会がなかった方々も少なくない印象です。 そんなVBAのコードをJava等の他言語の美意識から見て、「汚い」と断ずるのはお門違いです。
でも、クリーンなコードでシステムを開発しようとする者にとっては、ある意味、汚いコードの見本市。（失礼）良い反面教師です。（本当に失礼）
そこで今回は、たまたま読んだVBAマクロに対して可読性という観点からツッコミを入れたいと思います。 ツッコミをいれるにあったてVBAという言語の特性は考慮しません。 VBAを他言語の美意識で一方的に叩くという暴論ですが、初学者の戯言と思ってご容赦いただければと思います。
関数が大きすぎる
1関数500行ですと。。。
途方に暮れます。 半分読んだ頃には最初の100行が頭から飛んでます。 無理です。 人間が理解できるサイズに分割してください。
この関数、ほとんど試験できてないのではないでしょうか？ 昔の人がコードカバレッジに異常にこだわった理由がわかる気がします。
循環的複雑度が高すぎ
たまたま目の前にあったFor文の複雑度を数えてみました。 For文１つで約70行。 中にはIf/ElseIf/Else文が16こ。 最大ネスト6階層。 For文１つだけで複雑度18です。 関数全体の複雑度は50を超えてそうです。 ちなみに目標値は5〜7です。
ネストも分岐も極力減らしましょう。
変数の用途が不明すぎる
変数名からある程度、用途を連想できるようにして欲しいです。 新しい変数が登場してもしばらく読まないとその変数の用途がわからないというのは読んでて辛いです。 特にスコープが広い変数は理解するまでにかかる行数も長くなるため、それだけでも説明的な名前にしてくれるとありがたいです。 変数名が多少長くなっても構いません。
逆に、用途が読み取りやすく、数行で使い捨てる、スコープの狭い変数の名前は短くても構いません。
誤解を招く命名
Dim car() As String: ReDim car(5)
carは単数形です。 コードの中に「car」が現れたとき、読み手はその型が配列だとは思いません
配列ならcars等、複数形にしましょう。
変数の再利用
使い終わった変数を別の用途に使い回すのはやめましょう。 読み手が混乱しますし、バグのリスクが増します。
マジックナンバー
If tmp(5) <= 50 Or 100 <= tmp(5) Then
条件文の意図が読み取れません。 数値を定数にするとか、条件文自体を関数にするとか、意図を伝える努力をしましょう。
古いコードのコメントアウト
すぐに戻せるように残してるのかもしれませんが、実際にはすぐに戻せるわけありません。 そうやってコメントアウトされたコードが蓄積し、さらにコメントアウトしてないコードも変化し、どれをどう戻せばどうなるか、わからなくなってしまうからです。
VBAはGit連携しにくいので、コメントアウトに頼りたくなる事情は理解しますがゴミを残してるだけです。 古いコードはコメントアウトせずに消しましょう。
コメントと一致しないコード
これもよくあります。 仕様変更時にコードは変更したが、コメントの変更が漏れた結果です。 こういうコメントが一つでもあると読み手はそのコードのコメントを全て信じることができなくなります。
コメントはなるべく避けて、コメントなしでもわかるコードを書きましょう。 特殊な事情でそれが出来ないときだけ、コメントを書きましょう。 一度書いたコメントはコードの一部としてちゃんとメンテナンスしていく覚悟を持ちましょう。
コード先頭でまとめて変数を宣言
VBAの古いプラクティスのようですが、コードの先頭で変数をまとめて宣言する人が多いです。 始めて変数が使われた行に当たったとき、いちいちコードの先頭に戻って宣言を確認しなくてはならず、上下の往復が大変です。
読み手としては始めて使うところで宣言してくれた方が圧倒的に読みやすいです。
ループの中でbreak/continueしない
そのIterationの処理は一通り終わったはずなのに、continueやbreakがないので延々と下まで読んだ結果、やはりなにも処理がなかったのでまたループの一番上に戻る、というのは読み手をがっかりさせます。 理解するために関係ない部分まで読むことを強要するコードは優さがありません。 区切りのいいところでbreakやcontinueしましょう。
