Dim ○○済 As Boolean
で良いのに、
Private Const ○○済 As String = “1”
Private Const 未○○ As String = “0”
という宣言がしてある。
コーディング
コードレビューをしていて不思議に思うこと
意味の無い変数名
読みやすいコードのためにLINQを利用する。
最初に断っておきたいこと
C#3.0の話です。 LINQやラムダ式自体の説明はしていません。 簡単なサンプルにするため仕様がシンプルで効果を感じられないかもしれませんが、 もしもっと複雑な仕様だったら等、想像してみてください。
まず最初に、下記のサンプルを見てください。
簡単な仕様として、
受け取った在庫リストの集計対象のものだけ集計したい。 集計対象とは、数量が10より大きくて、数量を1000で割った時の余りが0のものの事である。
を実現します。
class サンプル1 { private class 在庫計 { public int 数量計 { get; set; } public int 金額計 { get; set; } } public void サンプルメソッド(在庫[] 在庫リスト) { 在庫計 在庫計情報 = new 在庫計(); foreach (在庫 在庫情報 in 在庫リスト) { if (在庫情報.数量 > 10) { if ((在庫情報.数量 % 1000 == 0)) { 在庫計情報.数量計 += 在庫情報.数量; 在庫計情報.金額計 += 在庫情報.金額; } } } } }
この例の場合、たしかに仕様を実現していますが、if文が実現している仕様についてコード上から読み取ることができません。
そこで、ラムダ式を利用し、集計対象の時にtrueを返す匿名メソッドを作成して実装してみます。
class サンプル2 { private class 在庫計 { public int 数量計 { get; set; } public int 金額計 { get; set; } } public void サンプルメソッド(在庫[] 在庫リスト) { Func<在庫, Boolean> 集計対象 = 在庫情報 => { if (在庫情報.数量 <= 10) { return false; } if (在庫情報.数量 % 1000 != 0) { return false; } return true; }; 在庫計 在庫計情報 = new 在庫計(); foreach (在庫 在庫情報 in 在庫リスト) { if (集計対象(在庫情報)) { 在庫計情報.数量計 += 在庫情報.数量; 在庫計情報.金額計 += 在庫情報.金額; } } }
その結果、集計対象の在庫のみ集計するという部分と、集計対象である条件の詳細部分が分離されました。
ここでさらにLINQを利用して、ループの部分が実際に何をしているのかをコードに残すようにします。
public class サンプル3 { public void サンプルメソッド(在庫[] 在庫リスト) { Func<在庫, bool> 集計対象 = 在庫情報 => { if (在庫情報.数量 <= 10) { return false; } if (在庫情報.数量 % 1000 != 0) { return false; } return true; }; var 在庫計情報 = new { 数量計 = 在庫リスト .Where(集計対象) .Sum(x => x.数量), 金額計 = 在庫リスト .Where(集計対象) .Sum(x => x.金額) }; } }
もしくは、
public class サンプル4 { public void サンプルメソッド(在庫[] 在庫リスト) { Func<在庫, bool> 抽出条件 = 在庫情報 => { if (在庫情報.数量 <= 10) { return false; } if (在庫情報.数量 % 1000 != 0) { return false; } return true; }; var 在庫計情報 = new { 数量計 = (from 在庫情報 in 在庫リスト where 抽出条件(在庫情報) select 在庫情報.数量).Sum() , 金額計 = (from 在庫情報 in 在庫リスト where 抽出条件(在庫情報) select 在庫情報.金額).Sum() }; } }
その結果、ループがなくなりましたが、在庫リストから集計対象のもののみを抽出(Where)し、集計(Sum)している事がコードに埋め込まれました。
また、在庫計情報をループで加算しないため、サンプル2までは在庫計というinner classを作成していましたが、その必要がなくなり、その場限りの匿名型で済むようになりました。
ところでサンプルコードだけを見ているとわかりませんが、集計した結果がint型の範囲を超えた時、サンプル2まで(ループで集計している）とサンプル3以降(LINQを利用している。)で実行時の挙動が違います。
サンプル2までは、例外が発生せず、マイナス値になってしまいます。
サンプル3以降は、ちゃんとOverflowExceptionが発生します。
もしサンプル2までのコードで例外が発生するようにする場合は
単純に
//抜粋 foreach (在庫 在庫情報 in 在庫リスト) { if (集計対象(在庫情報)) { if (在庫計情報.金額計 + 在庫情報.金額 > int.MaxValue) { throw new OverflowException(); } 在庫計情報.数量計 += 在庫情報.数量; 在庫計情報.金額計 += 在庫情報.金額; } }
とすると、intの範囲を超えた場合
在庫計情報.金額計 + 在庫情報.金額
がマイナス値になるため
在庫計情報.金額計 + 在庫情報.金額 > int.MaxValue
の条件がtrueになることが無いため、いつまでも例外が発生しません。
そのためより値のとれる範囲の大きいlongに型変換したうえで、
//抜粋 foreach (在庫 在庫情報 in 在庫リスト) { if (集計対象(在庫情報)) { if ((long)在庫計情報.金額計 + (long)在庫情報.金額 > int.MaxValue) { throw new OverflowException(); } 在庫計情報.数量計 += 在庫情報.数量; 在庫計情報.金額計 += 在庫情報.金額; } }
とするか、金額にマイナス値が無い前提であれば、
//抜粋 foreach (在庫 在庫情報 in 在庫リスト) { if (集計対象(在庫情報)) { if (在庫計情報.金額計 + 在庫情報.金額 < 0) { throw new OverflowException(); } 在庫計情報.数量計 += 在庫情報.数量; 在庫計情報.金額計 += 在庫情報.金額; } }
とする必要があります。
つまり、うっかり間違った実装をしてしまうと、オーバフロー時に例外を発行させているつもりで、実は発行していないという状況になってしまいますので、そういう点でも、このくらいの集計ならLINQを利用した方が安全です。
読みやすいコードを書くために匿名型を利用する。
最初に断っておきたいこと
VB9の話です。 匿名型自体の説明はしていません。 最後になぜ今回はC#ではなくVBなのか説明します。 簡単なサンプルにするため仕様がシンプルで効果を感じられないかもしれませんが、 もしもっと複雑な仕様だったら等、想像してみてください。
まず最初に先日の匿名メソッドのエントリでも私が考える読みやすいコードの要素について書きましたが、今回は
１．処理のスコープを最小にする。
２．よりコードに意味を持たせる。
という２点について注目していきたいと思います。
まず最初に、下記のサンプルを見てください。
簡単な仕様として、引数で渡された在庫リストの数量と金額の合計を算出します。
Public Class 在庫 Public 数量 As Integer Public 金額 As Integer ''以下定義が続く End Class Public Class サンプル1 Public Sub サンプルメソッド(ByVal 在庫リスト As 在庫()) Dim 数量計 As Integer = 0 Dim 金額計 As Integer = 0 For Each 在庫情報 As 在庫 In 在庫リスト 数量計 = 在庫情報.数量 金額計 = 在庫情報.金額 Next End Sub End Class
この例の場合、確かに、数量計と金額計を算出したいのはわかりますが、数量計と金額計はそれぞれ独立した変数となっているため、関連があることをコード上に残せていません。
そこで次のサンプルを見てください。
Public Class サンプル2 Private Class 在庫計 Public 数量計 As Integer = 0 Public 金額計 As Integer = 0 End Class Public Sub サンプルメソッド(ByVal 在庫リスト As 在庫()) Dim 在庫計情報 As New 在庫計 For Each 在庫情報 As 在庫 In 在庫リスト 在庫計情報.数量計 = 在庫情報.数量 在庫計情報.金額計 = 在庫情報.金額 Next End Sub End Class
privateなinner classである在庫計を作成し、サンプルメソッド内で、インスタンスを生成し利用することで、数量計と金額計は独立した変数ではないという事と、在庫の合計を保持しているという意味を持たせる事ができました。
その反面、在庫計クラスはサンプル2クラス全体から利用できるようになってしまいましたので、ほかのメソッドでも使っているかもしれない可能性が生まれてしまいました。
ここで、次のサンプルを見てください。
Public Class サンプル3 Public Sub サンプルメソッド(ByVal 在庫リスト As 在庫()) Dim 在庫計情報 = New With {.数量計 = 0, .金額計 = 0} For Each 在庫情報 As 在庫 In 在庫リスト 在庫計情報.数量計 = 在庫情報.数量 在庫計情報.金額計 = 在庫情報.金額 Next End Sub End Class
数量計と金額計をメンバに持った匿名型としてインスタンスを生成し、変数に適切な名前を設定することで、サンプル2の利点を生かしたまま、在庫計というクラスを作成しているわけではないので、他のメソッドでも利用されているかもしれないといった可能性が発生しなくなりました。
このように、匿名型を利用することで、できるだけ少ない範囲のスコープを維持したまま、よりコードに意味を持たせる事ができます。
最後に、何故C#ではなくVBで書いたのかというと、
C#の匿名型はメンバがすべて読み取り専用のプロパティとして宣言されるため、最初に宣言した時に設定した値を変更する事ができないからです。
VBでいうところの
Public Class サンプル4 Public Sub サンプルメソッド(ByVal 在庫リスト As 在庫()) Dim 在庫計情報 = New With {key .数量計 = 0,key .金額計 = 0} For Each 在庫情報 As 在庫 In 在庫リスト 在庫計情報.数量計 = 在庫情報.数量 在庫計情報.金額計 = 在庫情報.金額 Next End Sub End Class
というコードになってしまい、コンパイルエラーになるという事ですね。
では、最初の設定時に集計した値を設定してやれば良いのですが、その話は次回以降のエントリにします。
読みやすいコードを書くためにFunc型と匿名メソッドを利用する。
最初に断っておきたいこと
C#3.0の話です。 Func型や匿名メソッド自体の説明はしていません。
まず最初に、私が考える読みやすいコードの要素について断片的に提示します。
１．処理のスコープを最小にする。
２．今、読みたい粒度のコードしか読まなくても良い。
というものがあります。
どれも、いくつかある読みやすいコードの要素の断片でしかありませんが、この１と２は矛盾を孕んでいます。
それでは、どんな矛盾を孕んでいるかサンプルを交えて説明します。
001 public class サンプル1
002 {
003 public void サンプルメソッド()
004 {
005 if (鼻毛抜きの在庫 == 0 && 鼻毛カッターの在庫 == 0)
006 {
007 return;
008 }
009
010 鼻毛処理();
011 }
012 }
013
鼻毛処理のできる道具があれば、鼻毛処理ができる。
という仕様を実現しています。
しかしながら、「鼻毛処理のできる道具があれば」の詳細部分まで同じメソッド内に記述されてしまっています。
そこで、
001 public class サンプル2
002 {
003 public void サンプルメソッド()
004 {
005 if (!鼻毛処理の道具がある())
006 {
007 return;
008 }
009
010 鼻毛処理();
011 }
012
013 private bool 鼻毛処理の道具がある()
014 {
015 if (鼻毛抜きの在庫 > 0)
016 {
017 return true;
018 }
019
020 if (鼻毛カッターの在庫 > 0)
021 {
022 return true;
023 }
024
025 return false;
026 }
027
028 public void その他の処理()
029 {
030 throw new NotImplementedException();
031 }
032
033 }
034
これで、「２．今、読みたい粒度のコードしか読まなくても良い。」という要素が満たされた実装になりました。
しかしメソッドアウトする前は、処理のスコープがメソッド内にしかなかったのに、メソッドアウトした事で処理のスコープがクラス全体に広がってしまいました。
実装したばかりの今なら、まだサンプルメソッド内でしか使われていないとわかりますが、将来このコードを見たときに、サンプルメソッド内でしか使われていないと断定することができません。
これでは、「１．処理のスコープを最小にする」という要素が満たされなくなってしまっています。
そこで、以下のようにFunc型と匿名メソッドを利用してサンプルを修正してみます。
001 public class サンプル3
002 {
003 public void サンプルメソッド()
004 {
005 Func<bool> 鼻毛処理の道具がある =
006 delegate
007 {
008 if (鼻毛抜きの在庫 > 0)
009 {
010 return true;
011 }
012
013 if (鼻毛カッターの在庫 > 0)
014 {
015 return true;
016 }
017
018 return false;
019 };
020
021 if (!鼻毛処理の道具がある())
022 {
023 return ;
024 }
025
026 鼻毛処理();
027 }
028
これで、匿名メソッドという形でメソッドアウトし「２．今、読みたい粒度のコードしか読まなくても良い。」という要素を満たした上に、処理のスコープもメソッド内にとどまりますので、「１．処理のスコープを最小にする。」という要素も満たされます。
「鼻毛処理の道具があれば」という仕様の詳細まで知る必要が無いときは匿名メソッドを読み飛ばせば良いのです。
以上のように「ひとつのメソッドの中でしか利用しないが、粒度が違うためメソッドアウトする。」といったような局面において最小のスコープを維持したままメソッドアウトする道具として匿名メソッドを使う例の紹介でした。
ちなみに、サンプル3をラムダ式で書くと
001 public class サンプル3
002 {
003 public void サンプルメソッド()
004 {
005 Func<bool> 鼻毛処理の道具がある =
006 () =>
007 {
008 if (鼻毛抜きの在庫 > 0)
009 {
010 return true;
011 }
012
013 if (鼻毛カッターの在庫 > 0)
014 {
015 return true;
016 }
017
018 return false;
019 };
020
021 if (!鼻毛処理の道具がある())
022 {
023 return ;
024 }
025
026 鼻毛処理();
027 }
028
となります。
保守フェーズっていつ始まるの？
システム開発において、保守フェーズが一番長いフェーズである事は言うまでもありません。
しかし書かれたコードにとって、保守フェーズはいつから始まるのでしょう。
運用が始まってから
ではありません。
これは、システム開発のライフサイクル上、目に見える運用・保守というフェーズではあります。
しかし、書かれたコードにとっては、
書かれた瞬間
から保守フェーズが始まっています。
なぜなら、書いたコードは保守される対象になるからです。
ライフサイクル上の運用・保守フェーズに入らなくても、修正する事があるし、修正はしなくても読むことはありますよね。
だから、どんなに時間がないときでも、「読みやすいコード」を意識しないでただ闇雲に書いていると、余計に時間がなくなります。
是非、書いた瞬間から保守が始まっているという意識でシステム開発をしてみてください。
余裕が生まれて、よりよいものができるでしょう。
というわけで、
「書くは一瞬、保守は一生」
という話でした。
「メソッドの分割」を行うきっかけ
母国語ではない言語で変数名を決定した時の時間経過による傾向
当たり前だけど、ちゃんと、最後までやる人はやるんだけど、そうじゃない人もいる。
プロジェクトには、そうじゃ無い人が入る可能性はゼロじゃない。
という前提で
最初のうち
・ちゃんと英語と日本語の対応表を作るくらい頑張って辞書を片手に変数名をつける。
そのうち
・日本語を英語に変換するのがめんどくさくなってきて、変数名も汎用的な適当なものになる。
隣に日本語で変数名の意味が書いてあるから大丈夫でしょうという甘えが出てくる。
いつしか
・英語の変数名の隣に日本語で変数名の意味が書いてある状況になっているが、英語の変数名だけだと全く想定できない名前ばかりになっている。
・同じ意味でも違う変数名が点在するようになる。
母国語ではない言語で変数名を決定した時のデメリットの一つである。