この記事は、〇〇勉強してみた Advent Calendar 2017の23日目（12/23）の記事だよ。
次の記事はtosssaurusさんの『canvasを1ヶ月勉強してみた』みたい。
☆☆☆スパゲッティコードとの出会い☆☆☆
最近は、Javaじゃなくて、C#とかTypeScriptとか使ってるんだよね。
ほら、最近のアプリケーションって非同期処理が必須じゃない？
そうすると、スパゲッティなコードを割と自然に解消できるasync/awaitってすごいなぁって思うんだよね。
と言うか、ここのところ、いろんな人のコードをいっぱい見る機会があって、
async/awaitじゃなくても、『そういった構成』にしてないからスパゲッティになっているってコードを結構見かけて、
スパゲッティなコードってこういう風に作られていくんだなぁって思ったから、まとめてみようかなって。
Wikipediaとかみても、
・goto文の濫用
・多重継承の濫用
・スコープの拡大
・技術不足
とかしか書いてないし、
これって、特に最後の技術不足って、わかりません。って書いてあるようにしか思えないしね。
少しでも、スパゲッティなコードの理解と回避につながればいいなって思うの。
☆☆☆スパゲッティの概念的な定義☆☆☆
いつものことだけど、わたしの勝手な考えだから、鵜呑みにしておかしなことになっても知らないよ！
ってことで、いきなりわたしの答えから書いちゃうと、
次の２種類の書き方をしているプログラムをスパゲッティって言うんだと思う。
A. ライフサイクルに関する処理をライフサイクルの層から外してプログラムを構成する。
B. ドメイン重視のコンポーネント単位ではなく、アクション（１連の処理の流れ）としてプログラムを構成する。
ちなみに、これはだめな書き方だから、じゃぁ、どう書けば良いの？って観点で行くと、
『ライフサイクルやドメインにそった開発をする』っていうことかな。
そういう意味では、DDDな書き方している人はスパゲッティなコード書くことないのかもしれないね。
まぁ、どう書くべきか、みたいなのは、時間とともに変わっていくと思うけど。
☆☆☆スパゲッティの具体な成長例☆☆☆
じゃ、わかりやすい例を。
例だから、現実的かどうかとかは大目に見てもらえると助かる感じ？
わかりやすさ重視で！
一番わかりやすいのはこう言うのかな……。
『画面のボタンを押して、処理が終わるまで押せなくする』みたいな。
あんまりしない気もするけど例だから気にしないっ。
１.処理の配置間違えた。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
this.doSomething();
}
private void doSomething() {
// なが〜いなが〜い処理。
Thread.Sleep(2000);
this.Button.IsEnabled = true;
}
あ、えっと、一応、UIにボタン（Button）があって、押すとButton_Clickが呼ばれるってことで。
ここの例は、VisualStudioでWPFのプロジェクトとテンプレで作って、MainWindowにButtonを配置してイベントを作っただけだよ。
さて、こんなコード書かないよ！って？いや、まぁ、普通はそうだとわたしも思うんだけど。
と言うか、そもそもボタンの切り替え動かないし……っていうのは置いといて。
特に、メソッドを切り出すような処理がなければ、こういう書き方することはあり得ないね。
これは、定義のAになっちゃっているのがよくわかると思うんだよね。
切り替えている処理が別々のメソッドに分散しちゃってるから。
じゃ、次はもうちょっと、書きそうな例ってことでちゃんとボタンが切り替わるように非同期にするよ。
２.非同期にした（非同期場所間違えた）。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
this.doSomething();
}
private void doSomething() {
// なが〜いなが〜い非同期処理。
Task.Run(() => {
Thread.Sleep(2000);
this.Dispatcher.Invoke(() => {
this.Button.IsEnabled = true;
});
});
}
このくらいになると、書く人がちょっとだけいそうな気がする。
え？あ、うん、いや、まぁ、元の問題と対応があってないのはわかってるんだけど、
この流れだと、こう考えて書く人多そうだなってことでよろしく！
JavaScriptなWebだと、元に戻すのにわざわざDispatcherとかの行はいらないから、シンプルになって、余計に書く人多そう。
＃１で書かないって人は、もちろん、＃２も書かないと思うんだけど。
じゃぁ、次でやっとUIの処理まとめる気になったことにする。
実際は、doSomethingとかがさらに複雑になるとか。呼び出し元の処理が複雑になると気づくはずだよね。
３.UIの記述場所をまとめたつもり。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
this.doSomething(() => {
this.Button.IsEnabled = true;
});
}
private void doSomething(Action callback) {
// なが〜いなが〜い非同期処理。
Task.Run(() => {
Thread.Sleep(2000);
this.Dispatcher.Invoke(callback);
});
}
さて、これは、さらに書く人多そうな形だと思うんだよね。
コールバックっていうんだっけ。なんかすごく好きな人がいるみたいだよね。
わたしは、あまり使うの好きくないんだけど。
でも、doSomethingのメソッドがUIでの後処理を意識したメソッドになってしまっているね。
もうちょっと、ちゃんとUIを寄せてみようかな。
４.コールバックでUIと分ける。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
this.doSomething(() => {
this.Dispatcher.Invoke(() => {
this.Button.IsEnabled = true;
});
});
}
private void doSomething(Action callback) {
// なが〜いなが〜い非同期処理。
Task.Run(() => {
Thread.Sleep(2000);
callback();
});
}
うん、まぁ、何となく良くなった感じ？
じゃぁ、ここで、なが〜いなが〜い処理、ずっと適当にSleepとか書いてきたんだけど、
まぁ、普通は通信処理とかになったりするわけで、当然例外とかあり得るわけだよね。
そうしたらどうなるかなぁって。
はっ、＃２でおかしい対応していたことがばれてしまう！？（いまさら
５.コールバックでUIと分ける。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
this.doSomething(() => {
this.doSomething(
() => {
this.Dispatcher.Invoke(() => {
this.Button.IsEnabled = true;
});
},
(message) => {
this.Dispatcher.Invoke(() => {
MessageBox.Show(message);
});
}
);
});
}
private void doSomething(Action callback, Action errorHandler) {
// なが〜いなが〜い非同期処理。
Task.Run(() => {
try {
Thread.Sleep(2000);
// throw new Exception("hoge");
} catch(Exception ex) {
errorHandler(ex.Message);
} finally {
callback();
}
});
}
もうすでに、これはない……って感じになってきているわけだけど、
たぶん、少しずつ『処理を追加していってる』とか、『全体の構成じゃなくて一部だけ見直している』とかだと、
こうなっちゃう人、割といるんじゃないかなぁ？
ここでは、finallyにcallback置いたけど、finallyじゃなければ、Button_Clickの実装はまた変わってくるよね。
doSomethingの作りに非常に強く影響受けている感じ。
ほんと、ちょっとした変更も一蓮托生で、動き全部把握しておかないと即死って感じ？（物騒
まぁ、もうこのくらいでいいかな。
というわけで（？）、こういう風にコードが成長していくとスパゲッティになっていくって言うのは伝わったかなって。
じゃぁ、どうすればいいのかっていうと、次のような感じ。
まずは、Taskを戻すよ。
６．制御を戻す。
private void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
Task task = this.doSomething();
Task wait = Task.Run(() => {
try {
task.Wait();
} catch(AggregateException exs) {
this.Dispatcher.Invoke(() => {
exs.Handle((ex) => {
MessageBox.Show(ex.Message);
return true;
});
});
} finally {
this.Dispatcher.Invoke(() => {
this.Button.IsEnabled = true;
});
}
});
}
private Task doSomething() {
// なが〜いなが〜い非同期処理。
Task task = Task.Run(() => {
Thread.Sleep(2000);
// throw new Exception("hoge");
});
return task;
}
スパゲッティになった状態ではっきりわかる最大の問題は、
『非同期の制御を元に戻していない』ことだから、そこを直した感じ。
Taskを作ってる箇所が複数になって、もう＃２あたりでのこともはっきり目に見えちゃうね。
じゃ、ちょっと、見にくいから、async/await使うよ。
７．async/awaitで。
private async void Button_Click(object sender, RoutedEventArgs e) {
this.Button.IsEnabled = false;
try {
await this.doSomething();
} catch(Exception ex) {
MessageBox.Show(ex.Message);
} finally {
this.Button.IsEnabled = true;
}
}
private Task doSomething() {
// なが〜いなが〜い非同期処理。
Task task = Task.Run(() => {
Thread.Sleep(2000);
// throw new Exception("hoge");
});
return task;
}
どうかな〜。だいぶわかりやすくなったかな〜。
async/awaitを使うと、非同期関係のところは、正しいライフサイクルに導いてくれる気がするんだよね。
補助してくれるというか。
まぁ、別にasync/await使おうってことじゃなくて、
非同期はTaskを戻すのがライフサイクルとかドメインを考える上で重要ってこと。
そして、ライフサイクルとかドメインを考えないとスパゲッティになっていくってこと。
どんどん処理を追加していくんじゃなくて、
ちゃんと、ライフサイクルとかドメインを考えて構成しないと、スパゲッティになるよってことっ。
たぶん、スパゲッティコードが生まれる理由は、
コンポーネント単位でプログラムを書かないで、
アクション単位でコードをがんがん追加しちゃうのが原因じゃないかな。
言葉の定義怪しいけど。
今回も、Button_ClickにUIに関する制御＆表現を、doSomethingに実際に実行＆実現したいことを、って、
分けていれば、最初から＃６とか＃７の方向に進んでいくはずだしね。
ただ、ライフサイクルは、割と簡単だけど、
実際には、ドメインにそって適宜？分割していくのはすごく難しいと思う。
個人で開発していると、ドメインが1個に見えてきちゃうし、
チームで開発していると、ドメインの内部の関係性を把握しにくい感じがする。
☆☆☆スパゲッティを探す☆☆☆
さて、視点をちょっと変えて、すでにスパゲッティになっているのをどう見つけるか、ちょっと考えてみる。
・引数を、使わずに次のメソッドにただ渡している。
使われているところから発生源を追うんじゃなくて、変数がnewとかされてから、いったいどこまでそれが持ち運ばれていくかってみていく。
そうすると、だいたい、さっきみたいにUIの最後の処理をするためーとかで、UIそのものをドメイン側の処理に横流ししているとか見つけることが……ある？
まぁ、あったのっ。すっごく驚いたけど。
・フィールド経由でたまたまそこにある呼び出し先になるメソッドに値を渡す。
たまに見かけたコードだと、ドメインのメソッド、さっきの例だとdoSomethingは別のクラスとかに分けたりするわけだけど、
省略のために規模が小さければクラス分けないこともあると思うんだよね。
と言うか、最初は小さいから分けないことも多いと思うの。
で、引数で渡すのはおかしい！みたいなことをすると、フィールド経由で渡したりして……、もちろん、これもアウトだよね。
あとで、クラス分けるとき困っちゃう。
他のケースだと、全く別のメソッド呼び出していって、
コールバックで戻ってきたときに使うから、と言う理由で、フィールドに保存しておくみたいな考えっぽいもの。
これも結構すごかった。
かければ何でもいいとはわたしは思わないのだ。
・クラスに関係ないインターフェースを実装している。
これも結構あるかな。さっきの引数で持ち回るとき用でやっていたりするコードを見かけることがあるよ。
たぶん、ラムダ式とかがわからなかったり、クラスの責任とかを考えなかったり、っていうことだと思うけど。
かるく、同じアプリケーションコードの中でVisitorパターン発生してるよねきっと。
インターフェースをクラスに実装している例がいっぱいネット上に転がっているからなのかな？関係ない？
・Task、Future、Promiseを作って、returnしていない（コントロールが手放されている）。
定番でわかりやすいよね！例としても使わせてもらったし！
ちなみに、UIとかのもともと戻り値の型がvoidなところで保持する必要がある場合は、
フィールドに維持して管理するしかない……よね。
☆☆☆まとめ☆☆☆
長々書いてきたけど、結局、最初に書いた定義のAとBによってスパゲッティはできていくと思うの。
ここまで来たら、定義のAとBの違いは、もうわかっちゃっていると思うんだけど、
プラットフォームとかシステムに依存した仕組みとか制限によるものか、
アプリケーションのドメインによる表現とかによるものかの違いかな。
Aがわかるけど、Bがわからないひとは、BをAに強引に当てはめて恐ろしいスパゲッティを作ることが多い気がする。
両方わからない人はいろいろ警戒するから、そこまで元々巨大なスパゲッティは積極的には作らないかな。
つまり、自分がいったいどんなドメインを扱っているのかしっかりとらえるのが重要ってこと。
プログラムは人によって違う書き方になるっていうけど、わたしはあまりそう思わない。
ドメインの設計は人によって結構大きく変わるけど、そこが落ち着いたら、
それを表現する最適なコードは割と誰が書いても同じになると思っているよ。
まぁ、設計っていうか構成を考えるのとプログラムを一緒にすると、人によって違うっていえるかもだけど。
