5
5

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 5 years have passed since last update.

コレクションに対する無駄な処理- 良いコード書くきっかけを与えたい。5[C#リファクタリングサンプル]

Last updated at Posted at 2017-03-04

#コレクションの無駄な処理
コレクションに対して、通常のプロジェクトであればコーディング規約で「コレクションはnullを返さないで空インスタンスを返す」という規約をよく見かける。一方で、この規約を初めて聞いたという人も多かった。その結果、無駄なnullチェックを良く見かける。
また、Java経験者にC#プロジェクトを手伝ってもらったときには件数チェックをしてからforループをする人がいた。コレクション操作については言語関係なく、あまり知られていない気がする。

##サンプルコード
DBから取得してコンソールに取得した値を出力するコードを考える。
このコードの問題点はいくつかある。

  1. コレクションにnullを想定している。
  2. コレクションがnullの場合、「エラー」を出力しているが、例外の情報を握りつぶしている。
  3. foreachを使用する前に件数チェックをしている。

※ちなみに、規約に「コレクションをnullにしてはいけない」規約があるプロジェクトにも関わらず、コレクションをnullにしていた。

          internal void Main()
        {
            var items = GetDataFromDB();
            // NG1 NULLチェックをしてからforeach
            if (items != null)
            {
                foreach (var item in items)
                {
                    Console.WriteLine(item);
                }
            }

            // NG2 NULLチェックと件数チェックをしてからforeach
            if (items != null && items.Count > 0)
            {
                foreach (var item in items)
                {
                    Console.WriteLine(item);
                }
            }

            // NG3 NULLがこないことは想定しているが、何故か件数チェック
            if (items.Count > 0)
            {
                foreach (var item in items)
                {
                    Console.WriteLine(item);
                }
            }

            // NG4 listがnullならエラー
            if (items == null)
            {
                Console.WriteLine("エラー");
            }
        }

        /** DBからデータを取得する **/
        private List<string> GetDataFromDB()
        {
            // DBからデータを取得して、エラーの場合はNULLを設定する。
            // newの部分は、Daoからデータを取得している想定とする。
            try
            {
                var list = new List<string>();
                list.Add("1");
                list.Add("2");
                return list;
            }
            catch (Exception)
            {
                return null;
            }
        }

##リファクタリング後
nullの想定をなくし、例外を戻り値に含めるようにした。
※例外処理をGetDataFromDB内で行うべきかは、ここでは考えていない。

        internal void Main()
        {
            var result = GetDataFromDB();
            var items = result.Items;
            // nullチェックも件数チェックも不要
            foreach (var item in items)
            {
                Console.WriteLine(item);
            }

            // 例外が発生している場合、その例外情報を出力する。
            if (result.Exception !=null)
            {
                Console.WriteLine(result.Exception.ToString());
            }
        }

        /** DBからデータを取得する **/
        private Result GetDataFromDB()
        {
            // DBからデータを取得して、エラーの場合はNULLを設定する。
            // new Listの部分は、Daoからデータを取得している想定とする。
            try
            {
                var list = new List<string>();
                list.Add("1");
                list.Add("2");
                return new Result(list, null);
            }
            catch (Exception ex)
            {
                return new Result(new List<string>(), ex);
            }
        }

        /** 戻り値と例外処理を含めた処理結果クラス **/
        private class Result
        {
            internal List<string> Items { get; private set; }
            internal Exception Exception { get; private set; }

            internal Result(List<string> items, Exception ex)
            {
                this.Items = items;
                this.Exception = ex;
            }
        }

##まとめ
コレクションをnullにしないことで、冗長なコードを排除できた。ただ、「foreachの前に件数チェックをする」という実装をする人には別の方法を考える必要がある。不思議なことに件数チェックをする場合としない場合があるので、その基準は不明である。「forやforeachの前で件数チェックをしてはいけない」という規約にしても直らないと思う。

また、nullで例外処理する人はコレクションでなくても同じコードを書くので、こちらも対処が難しい。この例外処理の問題はコレクションに限った問題ではない。

とりあえず、「コレクションをnullにしない」ことである程度問題は解決するので、実践してほしい。ただし、一律禁止しているわけではなく、どうしてもnullにしなければならない場合は、アーキテクトに説明して許可をもらうべきである。

##サンプルコード(Java) 2017/3/24 追記

    protected void main() {
        List<String> items = getDataFromDB();
        // NG1 NULLチェックをしてからfor
        if (items != null) {
            for (String item : items) {
                System.out.println(item);
            }
        }

        // NG2 NULLチェックと件数チェックをしてからfor
        if (items != null && items.size() > 0) {
            for (String item : items) {
                System.out.println(item);
            }
        }

        // NG3 NULLがこないことは想定しているが、何故か件数チェック
        if (items.size() > 0) {
            for (String item : items) {
                System.out.println(item);
            }
        }

        // NG4 listがnullならエラー
        if (items == null) {
            System.out.println("エラー");
        }
    }

    private List<String> getDataFromDB() {
        // DBからデータを取得して、エラーの場合はNULLを設定する。
        // newの部分は、Daoからデータを取得している想定とする。
        try {

            List<String> list = new ArrayList<String>();
            list.add("1");
            list.add("2");
            return list;

        } catch (Exception e) {
            return null;
        }
    }

##リファクタリング後(Java)

    protected void main() {
        Result result = getDataFromDB();
        List<String> items = result.getItems();
        // nullチェックも件数チェックも不要
        for (String item : items) {
            System.out.println(item);
        }

        // 例外が発生している場合、その例外情報を出力する。
        if (result.getException() != null) {
            System.out.println(result.getException().toString());
        }
    }

    /** DBからデータを取得する **/
    private Result getDataFromDB() {
        // DBからデータを取得して、エラーの場合はNULLを設定する。
        // new Listの部分は、Daoからデータを取得している想定とする。
        try {
            List<String> list = new ArrayList<String>();
            list.add("1");
            list.add("2");
            return new Result(list, null);

        } catch (Exception e) {
            return new Result(new ArrayList<String>(), e);
        }
    }

    /** 戻り値と例外処理を含めた処理結果クラス **/
    protected class Result {
        private List<String> items;
        private Exception exception;

        protected Result(List<String> items, Exception exception) {
            this.items = items;
            this.exception = exception;
        }

        protected List<String> getItems() {
            return items;
        }

        protected Exception getException() {
            return exception;
        }
    }

[前の記事(冗長な代入文)]
(http://qiita.com/csharpisthebest/items/f9ebc741e40037553d5b)

次の記事(安易なNullチェック)

目次

5
5
8

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
5
5

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?