ShareXのソースを眺める⑤~適当リファクタ~
5日目
もうだいぶ片付いたけど。
まだ全部保護できてない。
コンストラクタをやっつける
public UploadResult() { _errors = new Errors(); IsURLExpected = true; }
至ってシンプルなコンストラクタだと思う。
ここで、レガシーコード改善ガイドより以下の文章を抜粋してみる。
テスト で 保護 し たい クラス が コンス トラクタ 内 で オブジェクト を 生成 し て いる と、 腹立たしく 思える 場合 が あり ます。
マイケル・C・フェザーズ. レガシーコード改善ガイド (Kindle の位置No.8240-8242). 翔泳社. Kindle 版.
その後、直後にこうも記載している。
初期化 処理 を コンス トラクタ 内 に 直接 書い て しまう と、 テスト 時 に その 処理 を 回避 する のが 非常 に 難しく なる 場合 が ある。
マイケル・C・フェザーズ. レガシーコード改善ガイド (Kindle の位置No.8245-8246). 翔泳社. Kindle 版.
思い当たる節はあって、このクラスはそもそもメンバ変数が全部Publicでやりたい放題できたけど、
厳密にスコープを定めていくとそうはいかないはず。
と、なってくるとコンストラクタがいまの状態であることがストレスになってくるはず。はず。
改善ガイドというぐらいだから、もちろん「こうしたらいいよ」ということも書いてある。
手順は
この よう な 状況 における 選択肢 の 1 つ は、 Factory Method【 訳注 1】 の 抽出 と オーバーライド を 行う こと です。マイケル・C・フェザーズ. レガシーコード改善ガイド (Kindle の位置No.8256-8258). 翔泳社. Kindle 版.
3.の手順はいったんおいといて、FactoryMethodの抽出はやっておこう。
Factory Method の 抽出 と オーバーライド を 行う 手順 は 次 の とおり です。
1. コンス トラクタ 内 における オブジェクト 生成 処理 を 特定 する
2. その 生成 に 関わる すべて の 処理 を Factory Method として 抽出 する
3. テスト 用 サブ クラス を 作成 し、 Factory Method を オーバーライド し て、 テスト 環境 で 問題 と なる オブジェクト 生成 に関する 依存関係 を 回避 するマイケル・C・フェザーズ. レガシーコード改善ガイド (Kindle の位置No.8280-8286). 翔泳社. Kindle 版.
public UploadResult() { _errors = makeErros(); IsURLExpected = makeIsURLExpected(); } protected virtual Errors makeErros() { return new Errors(); } protected virtual bool makeIsURLExpected() { return true; }
おうけい。いまいちこれの良さがいま実感がわかない。
protected virtual なのは、Fakeクラスを作るときにオーバーライドして書き換え放題にするのが目的。
テストのためのスコープが混じってしまうのはどうなんだろうか。とちょっと思ってしまう。
すぐ下にもう一つコンストラクタがあった
public UploadResult(string source, string url = null) : this() { Response = source; URL = url; }
例外なくこっちもFactoryMethodを切り出す。
public UploadResult(string source, string url = null) : this() { Response = makeResponse(source); URL = makeURL(url); } protected virtual string makeResponse(string source) { return source; } protected virtual string makeURL(string url) { return url; }
いい加減なメソッド名だけど、とりあえず。
この辺りを書いたところFactoryMethodの良さに何となく気付く。
・Responseはおそらく「String」型は不適切で、将来的にはRespose型になるべきなんだろうけど、
そうなった場合切り出したメソッドが効いてくる感じがする。URLのほうも同じく。
・メソッドで切っているおかけで、テスト側で何のコンストラクタを呼ぼうが値をコントロールできる。
つぎだつぎ!
ForceHTTPS
public void ForceHTTPS() { URL = URLHelpers.ForcePrefix(URL); ThumbnailURL = URLHelpers.ForcePrefix(ThumbnailURL); DeletionURL = URLHelpers.ForcePrefix(DeletionURL); ShortenedURL = URLHelpers.ForcePrefix(ShortenedURL); }
うーん。やっぱりほら。ね。URLクラスいると思うんだよ私は。
Helperいらないんじゃないのか。
URLHelpersという切り口というよりは、そのままURLクラスだと思うんだけど。
テストもどうしよう。ここは何のためのテストとよくわからなくなりそう。
Force2回読んだらどうなるんだろう。見る限り、URLPrefix群で先に消してるから2重に付与されたりはしない。置きで行こう。
コード分析をかけてみた
重大度レベル コード 説明 プロジェクト ファイル 行 抑制状態
警告 CA2214 'UploadResult.UploadResult()' には、クラスによって定義された仮想メソッドへの呼び出しの原因となる呼び出しチェーンが含まれます。予期しない結果となっていないことを次の呼び出し履歴から確認してください:
UploadResult..ctor()
UploadResult.makeIsURLExpected():Boolean ShareX.UploadersLib D:\git_refact\ShareX\ShareX.UploadersLib\UploadResult.cs 105 アクティブ
へんな警告でとるがな。
public UploadResult() { _errors = makeErros(); IsURLExpected = makeIsURLExpected(); } protected virtual Errors makeErros() { return new Errors(); }
ここの virtualの宣言がまずいらしい。
https://ufcpp.net/study/csharp/misc_construct.html
がわかりやすい。
現状のコードであれば問題ないが。といったところか。警告でも残しとくのは嫌なんだよなぁ。
すごく気持ち悪いけど、分析実行しなきゃでない警告。うーん気持ち悪いし、言ってることももっともだから、危険なコードではある
はーん。放置&放置。 overrideさえできりゃええんじゃが・・・できるからこその警告だしなぁ。