長いこと、1人チームだった私のプロジェクトですが、チーム開発をすることが多くなりました。
全てのコードに目を通し、ソースレビューを行っていますが、レビュー中に時々背中がゾワゾワっとすることがあります。
そのゾワゾワについて、書いてみたいと思います。
基本的にプログラミング言語ならなんでも同じですが、C#のコードレビューをしているので、例はC#になります。
ゾワゾワ発生源の種類としては
１．無駄なコード
２．お作法違反なコード
３．危険なコード
があります。
私がプログラムを書く上で、重要視しているのは以下の3点です。
１．シンプル
２．メンテナンスしやすい
３．分かりやすい
大抵の場合、半年もすると自分の書いたコードすら何をやっていたのか分からなくなってしまうものです。プログラムは、文章を書くように、1行1行意味があることが理想です。
具体的な例を挙げてみます。
例１）イベントメソッドの再利用
C#のコードを初めて書いた人たちがやりがちなこととして、イベントメソッドのダメな再利用があります。
例えば、検索ボタンに検索コードを書いておきます。その後、登録ボタン内で、登録後に再検索をする、というような処理があるとします。
private System.Windows.Forms.Button search;
private System.Windows.Forms.Button update;
private void InitializeComponent() {
this.search.Click += new System.EventHandler(this.search_Click);
this.update.Click += new System.EventHandler(this.update_Click);
}
//検索ボタンのクリック
private void search_Click(object sender, EventArgs e) {
//DBにデータの問い合わせ
}
//登録ボタンのクリック
private void update_Click(object sender, EventArgs e) {
//登録処理
//・・・・
//再検索
search_Click(sender, e); //←検索ボタンのイベントメソッドを呼んでいる。
}
このコードには、NGを出します。
senderや、eには、イベントを発生させたプログラムが適切な値を設定します。searchボタンのClickイベントの引数を、そのままupdateボタンに引き継いでよい、などと開発者が勝手に判断すべきではありません。
この例は、たまたま両方ボタンだし、イベントの引数も使っていないし、誤動作をすることはなさそうに見えますが、「動けばいいんでしょ？」という考えはダメです。
private void update_Click(object sender, EventArgs e) {
//登録処理
//・・・・
//再検索
button1.PerformClick(); //←クリックを発生させている
}
このコードもNGです。
最初の例と違って、しかるべく手順でクリックイベントを発生させているので、正しいように思えます。
しかし、クリックのイベントというものは「マウスのボタンを手でカチッとやった」ときに起こるものです。普通は、クリックイベントがユーザーのマウス操作以外で起きることまで想定しません。そのような理由から、PerformClick()メソッドのコールは裏技であり、トリッキーな処理です。どうしてもそれ以外の方法がない場合にのみ、仕方なくやることはあるかもしれませんが、少なくとも通常の画面内の処理の実装でそれが必要な場面は滅多にありません。
素直に、検索処理をメソッドにして、検索ボタンと登録ボタンの両方から呼べばいいのです。
private void search_Click(object sender, EventArgs e) {
//検索処理を行うメソッドを呼ぶ
search();
}
private void update_Click(object sender, EventArgs e) {
//登録処理
//・・・・
//再検索→検索処理を行うメソッドを呼ぶ
search();
}
private void search() {
//DBにデータの問い合わせ
}
これなら誰が見ても検索を行うメソッドがどれかわかります。
上記はごく単純な例で、あまり真剣に取り扱うような問題には見えないかもしれません。しかし、ディティールから全ては成り立っているのです。
