SE(たぶん)の雑感記

一応SEやっている筆者の思ったことを書き連ねます。会計学もやってたので、両方を生かした記事を書きたいと考えています。 でもテーマが定まってない感がすごい。

CodeKataで遊ぶ Kata07: How'd I Do?

CodeKataやってみた記事、第六弾です。

今回は、Kata07: How'd I Do?をやっていきます。

codekata.com

概要

1年前程度、昔書いたソースを読む。
500~1000行程度あると望ましい。

と書くとあっさりしていますが、自分が書いたソースを批判的に読むことが求められています。

読み方として、

  • 優れたソースとして読む
    自分が知り限り、最高のプログラマーが書いたソースとして読む

  • ひどいコードとして読む
    自分が知り限り、最低のプログラマーが書いたソースとして読む

  • バグを見つける
    コードのすべての潜在的バグを探すつもりで読む。
    本文では、「重大なバグがあるから、お金を払わないぞ!とクライアントに言われている気持ちで読め、と書いてあります。リアルですね。

使うソース

仕事で書いたソースは、さすがに使えません。

そこで、前回記事に書いた通り、2年半前に書いたものを使います。

github.com

  • 紹介記事

hiroronn.hatenablog.jp

コードメトリクス上は265行ですが、ソースファイルの行数なら500は超えるので、ほどよい規模かと思います。

考察

今回は、自分の書いたソースなので、答えは人それぞれです。
そのため、特に回答を隠しません。

一回目:優れたソースとして読む

ツリー展開時、フォルダ探索を行う

画面中央部、フォルダを選択する部分です。
フォルダ階層を一気に取得すると、膨大な時間がかかります。
そこで、「ツリーを展開したタイミング」で、フォルダ内の検索を行うようにしています。

Viewで、TreeViewItem.IsExpandedとのバインドを設定し、ViewModelで検知します。
Modelが未検索状態だったら、非同期で検索実行後、その結果を返します。以降は結果固定*1です。

ちなみに、子ディレクトリを非同期で返す処理は、このように書いています。

/// <summary>
/// 指定ディレクトリの子孫となっているディレクトリを、絶対パスで取得します。
/// </summary>
/// <param name="dir">絶対パスのディレクトリ</param>
/// <returns></returns>
public static Task<IEnumerable<string>> GetDirectoriesAsync(string dir) {
    var result = new Func<IEnumerable<string>>(() => {
        if (!Directory.Exists(dir)) {
            return Enumerable.Empty<string>();
        }

        return Directory.EnumerateDirectories(dir);
    });

    return Task.Run(result);
}

ループを減らそうとする努力

Childs.AsParallel().ForAll(f => f.IsSelected = true);

のような形で、ループ自体を書かないようにしています。

二回目:ひどいコードとして読む

このソース、ひどいところが多いです。
自分のソースなので、言いたい放題言います。

ディレクトリ構造の親取得

フォルダ一覧の、ツリーに表示する項目を取得するのは、非同期で行われます。
取ってきた項目は、親子で相互参照しますが、この紐づけがViewModelで行われます。 そこはModelでやったほうがよくないか、と思います。

全選択、全解除

子の選択を変更するのですが、分かりづらいです。

ViewModelの役割

上にも書いた通りですが、ViewModelの役割が大きすぎます。
MVVMにおけるViewModelは、ViewModelの中継という役割を果たしますが、それ以上やっています。
親子の紐づけ等のロジックは、Modelに寄せるべきかと思います。

無駄なnullチェック

FileCompressModel.csというソースに、

if (FileSystemService.Exists(value))
{
    var item = new DirectoryModel(value, null);
    if (item != null)
    {
        _sourceItems.Add(item);
    }
}

というソースがあります。

コンストラクタを呼び出した後のnullチェックは、確実に無駄です。

Factoryだったら、意味がある可能性はありますが…そうだとしても、Null Objectパターンを適用する等で、回避すべきと思います。

三回目:バグを見つける

バグというか、未実装部分というか…

中断が機能していない

「中断」ボタンがあり、圧縮中は有効になります。

しかし、押しても処理を中断できません。

圧縮処理自体は同期で行われる*2ため、中断は以後の圧縮を行わない効果になります。

ツリー上で圧縮中かどうか表示できれば、中断の効果は見えますが、今の見た目で中断を実装しても、何が中断なのか分かりません。

ファイル単体の圧縮ができない

あるフォルダの中にファイルとフォルダがある場合、子フォルダを圧縮対象として選択すると、ファイルを圧縮する手段がなくなります。

言い訳すると、.NETのZipFileクラスには、CreateFromDirectoryという、ディレクトリを元に圧縮を行うメソッドしかなく、ファイルを起点とできません。

未検証ですが、空の圧縮ファイル作成→ファイル追加、とすれば可能そうです。

Kataの問題

その1

自身が書いたソースを、批判的に見る習慣をつけるには、どうしたらいいですか?

とのことです。

これは、私もできている自信がありません。
後から、ここはこうしておけばよかったなぁと思うことが多いです。

ただ、批判的に見るとは異なりますが、経験則として、動くだろうと思って作ったものは、大抵動きません
リリース直前で修正したソースが火を噴く、という経験もあります。

自分のソースすら信用できない、という感覚が分かるようになると、自身が書いたソースであっても、批判的に見るようになると思います。

答えはただ、システムの挙動の中にしかありません。
それを客観的に証明できない限り、自分のソースはバグっていると感じます。

テスト駆動開発の良いところの一つは、ある程度自身の正しさを証明できる点にあると感じます。

その2

これらのテクニック(良いコード、ダメなコード、バグ見つける)を、同僚のコードレビューに活かせますか?

とのことです。

3つの視点でレビューするのは、自分が3人いないと厳しいです。時間が3倍かかります。

どの視点で見るかは、相手の力量等を勘案して決めるべきかと思います。
どういう立場で見るか決める、という意味で、上記テクニックは意味があります。

また、どれだけ客観的な視点で正しさを確保できているか、についても、チェックすべきだろうと思います。。

おわりに

2年半前のソースをじっくり読んで、ひどいなぁと、正直思いました。

一晩で作ったという理由はありますが、もっときれいなソースになっていればいいのに、という感じです。

GitHubに上げたので、ブランチ切って改良するのも、楽しそうです。

自分のソースを読む。意外と勉強になるかもしれません。皆様もぜひ。

*1:これで良いかどうか、気にしたほうが良い

*2:終わるまで待つ以外の手段が無い、という意味。圧縮中の処理を止めることはできない