No more Death March

あるSEのチラシの裏　Ｃ＃　ＷＰＦ

c# リファクタリング　小さい方の値が欲しい。

リファクタリング C#

二つのint型から小さい方の値がほしい時。

        public void Hoge(int one,int other)
        {
            int min;
            if (one < other)
            {
                min = one;
            }
            else
            {
                min = other;
            }
        }

じゃなくて。MathクラスのMinメソッドを使おう。

        public void Hoge(int one,int other)
        {
            int min;
            min = Math.Min(one, other);
        }

MSDNで見ればわかることですが、longやbyte等、大体の型のオーバーロードが用意されています。

Math.Min メソッド (System)

大きい方の値であればMath.Max

https://msdn.microsoft.com/ja-jp/library/system.math.max(v=vs.110).aspx

DateTime構造体にもこういうのないものかなぁと思ったらStackOverFlowでも似たような話がありました。

stackoverflow.com

残念な英語力なので雰囲気で提案されている解決方法を真似してみます。

まずは三項演算子

            DateTime min;
            min = one < other ? one : other;

Ticksプロパティ(long型)をMathクラスに渡す方法

            DateTime min;
            min = new DateTime(Math.Min(one.Ticks, other.Ticks));

Utilityクラス見たいなので再利用する。

    public static class DateTimeUtil
    {
        public static DateTime Min(DateTime one,DateTime other)
        {
            return new DateTime(Math.Min(one.Ticks, other.Ticks));
        }
        public static DateTime Max(DateTime one, DateTime other)
        {
            return new DateTime(Math.Max(one.Ticks, other.Ticks));
        }
    }

最後に挙げた安易にユーティリティクラスを作ることの是非は別として、
TicksをMathクラスに渡すのはへぇ～ってなりました。

C# 自作クラスの戻り値からnullを排除する。

C#

自作クラスのプロパティやメソッドでなんらかの値を返すならnullは返さないようにしたい。
そのクラス自体のnullチェックを他のクラスがするのはわかるとして、
クラスが公開する「何か」についてはそのクラスが責任を持つべき。

string型のプロパティがあるならnullの代わりにstring.Emptyを返せばいいし。
他のクラスを返すならnullObjectパターンを使えばいい。
コレクションなら空のコレクションを返す。

C# リファクタリング コレクションを変換

C# リファクタリング

コレクションの内容を一定のルールに従って違う型に変換したい場合

        public void Hoge()
        {
            var list = new List<object>();
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());

            var list2 = new List<int>();
            foreach(var obj in list)
            {
                list2.Add(obj.GetHashCode());
            }
        }

foreachを取り除くには

        public void Hoge()
        {
            var list = new List<object>();
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());
            list.Add(new object());

            var list2 = new List<int>();
            list2.AddRange(list.ConvertAll<int>(x => x.GetHashCode()));
        }

ConvertAllメソッドを使ってやる。

C# リファクタリング foreachで抽出

C# リファクタリング

Listをぐるぐる回して条件に合致するものを別のインスタンスに入れている場合

        public void Hoge()
        {

            var list = new List<String>();
            list.Add("123");
            list.Add("1234");
            list.Add("12345");
            list.Add("123456");

            // 4文字以下のものをリストから抽出したい。
            var list2 = new List<string>();
            foreach(var str in list)
            {
                if (str.Length <= 4) list2.Add(str);
            }
        }

foreachをなるべく書かないようにする。

        public void Hoge()
        {

            var list = new List<String>();
            list.Add("123");
            list.Add("1234");
            list.Add("12345");
            list.Add("123456");

            // 4文字以下のものをリストから抽出したい。
            var list2 = new List<string>();
            list2 = list.FindAll(x => x.Length <= 4);
        }

さらに行数を減らすなら

        public void Hoge()
        {

            var list = new List<String>();
            list.Add("123");
            list.Add("1234");
            list.Add("12345");
            list.Add("123456");

            // 4文字以下のものをリストから抽出したい。
            var list2 = new List<string>(list.FindAll(x => x.Length <= 4));
        }

コンストラクタでそのまま渡してしまう。
だた括弧がネストしすぎると読みにくいし、デバッグ時にステップ実行しにくいという問題もある。
個人的にはステップ実行で動作確認するくらいならテストコード書けば良いと思いますが。

FindAllメソッドをRemoveメソッドに変えた場合

        public void Hoge()
        {

            var list = new List<String>();
            list.Add("123");
            list.Add("1234");
            list.Add("12345");
            list.Add("123456");

            // 4文字以下のものをリストから抽出したい。
            list.RemoveAll(x => x.Length >= 5);
        }

FindAllメソッドとの違いとしては
・FindAllメソッドは戻り値がList
・RemoveAllメソッドは実行したコレクション自体を編集し、戻り値がint

C# リファクタリング　コレクションを返すメソッド

C# リファクタリング

なんらかの処理を行ってコレクションを返すようなメソッドがあったとして、
こんな具合にパラメータがnullだったら例外を投げ返すんだったら

        public IEnumerable<object> Hoge(object obj)
        {
            if (obj == null) throw new ArgumentNullException("obj");
            // なにか処理してコレクションを返す。
        }

こんな具合に空の配列を返してやる。

        public IEnumerable<object> Hoge(object obj)
        {
            if (obj == null) new object[] { };
            // なにか処理してコレクションを返す。
        }

メソッドの目的にもよるのだろうけど、
例えば検索処理とかだと変なパラメータを渡したら例外を投げずに空の結果を返してやれば良い。

書いてみて思ったけど、おかしなことをやっているならやっぱり例外を投げて気づく機会を設けるべきなんだろうか・・・

C# リファクタリング　早期リターン

C# リファクタリング

int型の引数が5以下だったらTrueを返すとして、

        public bool Hoge(int value)
        {
            if(value < 5)
            {
                return true;
            }else{
                return false;
            }
        }

こうだったのを

        public bool Hoge(int value)
        {
            if (value < 5) return true;
            return false;
        }

こうする。

ソースコードが５行から２行へ、
ごくごくシンプルな例ですが、複雑な条件が組み合わさる場合時は、
結果が確定している時点でさっさとメソッドから抜け出すようにすると修正時のリスクが少し軽減出来る。

オブジェクト指向もといプログラムを考える。

レガシープログラマの歴が長く、オブジェクト指向は独学、
何が良いプログラムで何が悪いプログラムなのかもやもやする・・・

日々頭の中の考えがあっちいったりこっちいったりで切りがないので、
気が付いた時に文章に書いておいた方がいいかもしれない。

今日思ったこと。

プログラムを作る時、まずごくごくシンプルなインターフェースで考える。
インターフェースが決まったらインターフェースをコンポジットする実装クラスを作る。

そして単一の処理を行う実装クラスを作る。
後はユースケースに合わせてコンポジットしたものを用意する。

同一モデルに対するチェック処理や手続き的な編集処理はこういう作りにしとくと組み換えが利いて良いかもしれない。