CodeKataやってみた記事、第七弾です。
今回は、Kata07: How'd I Do?
をやっていきます。
概要
1年前程度、昔書いたソースを読む。
500~1000行程度あると望ましい。
と書くとあっさりしていますが、自分が書いたソースを批判的に読むことが求められています。
読み方として、
優れたソースとして読む
自分が知り限り、最高のプログラマーが書いたソースとして読むひどいコードとして読む
自分が知り限り、最低のプログラマーが書いたソースとして読むバグを見つける
コードのすべての潜在的バグを探すつもりで読む。
本文では、「重大なバグがあるから、お金を払わないぞ!とクライアントに言われている気持ちで読め、と書いてあります。リアルですね。
使うソース
仕事で書いたソースは、さすがに使えません。
そこで、前回記事に書いた通り、2年半前に書いたものを使います。
- 紹介記事
コードメトリクス上は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
は、View
とModel
の中継という役割を果たしますが、それ以上やっています。
親子の紐づけ等のロジックは、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
に上げたので、ブランチ切って改良するのも、楽しそうです。
自分のソースを読む。意外と勉強になるかもしれません。皆様もぜひ。