コードリファクタリングの手引き

1. コードリファクタリングの手引き オブジェクト指向の原則とテストをかけるようにするメンテナンス術 pixiv Inc. @Hare8563 2019.2.25
2. 2 • 晴佐久 哲士 • ピクシブ株式会社 • フルスタックエンジニア ◦ サーバサイド (Rails) ◦ フロントエンド (React/Redux, Typescript) ◦ 環境構築 (AWS, Vagrant, Docker) ◦ Unity (C#, C++) • 趣味 ◦ プログラミング ◦ お絵かき ◦ CG制作 @Hare8563 フルスタック エンジニア
3. まずはこのコードを見てください 3
4. 4 public void LoadAsync (DownloadLicense newLicense , Action <GameObject > onLoadComplete , Action <float> onDownloadProgress = null, Action <Exception > onError = null) { CachedLicense newCachedLicense = new CachedLicense (newLicense ); DownloadLicense ? cachedDownloadLicense = LoadExistLicense (newLicense .licenseid ); if (cachedDownloadLicense != null && newCachedLicense .IsSameModel (cachedDownloadLicense .GetValueOrDefault ())) { SaveCachedLicense (newCachedLicense ); StartCoroutine (LoadAsyncCachedBinary (newCachedLicense , (characterBinary ) => { if (onDownloadProgress != null) onDownloadProgress (1.0f); LoadAsyncFromBinary (characterBinary , (GameObject character ) => { onLoadComplete (character ); }, onError ); })); } else { LoginedRequest (requestPath : "/api/request_path" , methods : HTTPMethods .Get, onSuccess : (downloadHandler ) => { byte[] downloadBinary = downloadHandler .data; LoadAsyncFromBinary (downloadBinary , (GameObject character ) => { Queue.Enqueue (() => { SaveEncryptedModelFile (newCachedLicense , downloadBinary ); SaveCachedLicense (newCachedLicense ); }); onLoadComplete (character ); }, onError ); }, onProgress : onDownloadProgress , onError : (downloadHandler ) => { Exception error = new Exception (downloadHandler .text); if (onError != null) onError (error); } ); } }
5. これを読んで内容を理解できました? 5
6. じゃあ、これならどうでしょうか 6
7. 7 public void LoadAsync(DownloadLicense newLicense, Action<GameObject> onLoadComplete, Action<float> onDownloadProgress = null, Action<Exception> onError = null) { CachedLicense newCachedLicense = new CachedLicense(newLicense); var loader = Factory.Create(newCachedLicense, this); loader.OnLoadComplete = onLoadComplete; loader.OnProgress = onDownloadProgress; loader.OnError = onError; loader.Load(); }
8. なにをやりたいのかなんとなくわかる! 8
9. じつはこれ、リファクタリング前と後のコードな のです。 9
10. 今日の内容 1. なぜ、コードは読みにくくなるのか 2. どういう風にリファクタリングをしていくのか・何を意識する のか 3. 実際にどうやってリファクタリングに立ち向かったか 10
11. (注意) 本内容は、完全に私の経験に基づいた独断と偏見混じりの内 容になっております。 あまり鵜呑みにしないよう、ご了承ください 11
12. なぜ、コードは読みにくくなるのか 12
13. なぜコードは読みにくくなるのか • 複数人でコードに触れるため • 政治的な都合 • すでに手がつけられない無法地帯 13
14. コードに複数人が手をつけるのは当たり前 14
15. コードに複数人が手をつけるのは当たり前 15 とりあえず動 けばいいか
16. コードに複数人が手をつけるのは当たり前 16 こう書けばもっ とかっこよく書 ける!
17. コードに複数人が手をつけるのは当たり前 17 読みやすく書 こう
18. コードを書く人によって考え方や力量は バラバラなのだ 18
19. 結果、読みにくいコードができあがる 19
20. 政治的な都合で、適当に書かないと間に合わない 20
21. こういったのが積もり積もって 21
22. 誰も手をつけられない状態になる 22
23. こうならない対処法自体は簡単なのだ 23
24. コーディング規約やレビューをきちんとする 24 こういう風に書いて ね。 レビューも僕を必ず 通してね
25. 書くときは、間に合わせで書いていいけど 25
26. 納期に間に合ったらきちんと整理しましょう 26
27. 指示する側もきちんとその時間作ってあげてね 27
28. じゃあ、実際にどう整理していく(リファクタリン グ)していくか考えていきましょう 28
29. どういう風にリファクタリングをしていくのか・何 を意識するのか 29
30. 何を意識すればいい 30
31. 『テストが適切に書けること』 31
32. 32 public void LoadAsync (DownloadLicense newLicense , Action <GameObject > onLoadComplete , Action <float> onDownloadProgress = null, Action <Exception > onError = null) { CachedLicense newCachedLicense = new CachedLicense (newLicense ); DownloadLicense ? cachedDownloadLicense = LoadExistLicense (newLicense .licenseid ); if (cachedDownloadLicense != null && newCachedLicense .IsSameModel (cachedDownloadLicense .GetValueOrDefault ())) { SaveCachedLicense (newCachedLicense ); StartCoroutine (LoadAsyncCachedBinary (newCachedLicense , (characterBinary ) => { if (onDownloadProgress != null) onDownloadProgress (1.0f); LoadAsyncFromBinary (characterBinary , (GameObject character ) => { onLoadComplete (character ); }, onError ); })); } else { LoginedRequest (requestPath : "/api/request_path" , methods : HTTPMethods .Get, onSuccess : (downloadHandler ) => { byte[] downloadBinary = downloadHandler .data; LoadAsyncFromBinary (downloadBinary , (GameObject character ) => { Queue.Enqueue (() => { SaveEncryptedModelFile (newCachedLicense , downloadBinary ); SaveCachedLicense (newCachedLicense ); }); onLoadComplete (character ); }, onError ); }, onProgress : onDownloadProgress , onError : (downloadHandler ) => { Exception error = new Exception (downloadHandler .text); if (onError != null) onError (error); } ); } }
33. イメージにするとこんな感じになってる 33
34. LoadAsync 34 キャッシュの有無 をチェックする キャッシュから読 み込む HTTPリクエストをし てダウンロードする ダウンロードした ファイルから読み 込む ダウンロードしたの を暗号化して保存 する
35. これがいわゆる密結合の状態 35
36. これだと個々の機能をテストできない 36
37. テストをできるように分解するにはどうしたらい い? 37
38. 正しいオブジェクト指向を学ぼう 38
39. SOLIDの5原則 39
40. SOLIDの5原則 • 単一責任の原則 (Single Responsibility) • オープン・クローズドの原則 (Open/closed) • リスコフの置換原則 (Liskov substitution) • インターフェース分離の原則 (Interface segregation) • 依存関係逆転の原則 (Dependency inversion) 40
41. 単一責任の原則 • ひとつのクラスが受け持つ機能はひとつに限定 ◦ 言い換えると、変更理由はひとつにする • 例としては、ファイルの読み込み処理 ◦ ファイルの読み込みと、読み込んだものを解釈するは分ける ◦ 読み込みのフォーマットが変わった時、解釈機能だけ変更すればよくな る 41
42. オープン・クローズドの原則 • 拡張は容易にできて、修正は他のクラスに影響を及ぼさ ないようにする原則 • クラスは、他のオブジェクトに依存するのではなく、そのイ ンターフェースに依存する(あるいは、newを隠蔽する)よう にすると実現可能 42
43. 43 // このメソッドは内部で DerivedObjectに依存してしまっている // DerivedObject を別のものに変更したい時や、状況に応じて切り替えたいときに それができない public void BadMethod() { DerivedObject obj = new DerivedObject(); obj.someMethod(); // これ以降、objを使った処理が続く }
44. 44 // Factory経由でIObjectを実装した何かが受け取れる // このメソッドはInterfaceに依存しているため、比較的拡張がやりやすい public void GoodMethod() { IObject obj = Factory.Create(); obj.someMethod(); // これ以降、IObjectに依存した処理が入る }
45. リスコフの置換原則 • 基底クラスから派生したクラスは、常に基底クラスと置き 換えられる挙動にしなければならない • サブクラスへ要求すること(パラメータ等)はむやみに増や さず、できること(実装するメソッド等)は減らすべきでない 45
46. 46 class BaseClass { public abstract void DrawShape() { // このメソッドは何か形をキャンバスに書き込む } } class DerivedClass : BaseClass { public override void DrawShape() { // このメソッドはファイルに形に関するメタな情報を書き込む // このような実装をすると、すでに`BaseClass#DrawShape`を使ってキャンバスに書き込んでいるところで置き換えた時 // なんらかのシェイプが書き込まれないため、期待した挙動にならなくなる } }
47. 47 class BaseClass { public virtual void PrintWord() { Console.WriteLine("Print any message" ); } } class DerivedClass : BaseClass { public string Message; public DerivedClass() { } public override void PrintWord() { // 要求する情報が増えているため、そのまま使うと nullを参照してしまう Console.WriteLine("Print " + Message + " message"); } }
48. インターフェース分離の原則 • インターフェースはシンプルにするという原則 • 一つのインターフェースに余計な機能をつぎ込んではいけ ません ◦ 例として、「ファイルをロードする」という機能を提供するインターフェース に「ファイルをローカルに保存する」という関係のない機能は必要ない ◦ IFileLoader と IFileSave で分けるべき 48
49. 依存関係逆転の原則 • 上位のモジュールの実装は、下位のモジュールの実装に 依存してはならず、双方とも「抽象」に依存するようにしな いといけない ◦ 悪い例として、CarクラスがEngineクラスに依存した実装 49
50. 50 // エンジンオブジェクトに依存した実装 class Car { public void Run() { Engine engine = new Engine(); // エンジンの状態を変更する engine.State = EngineState.RUN; // エンジンのレバーをあげる engine.PullLever(); // ピストンを動かす engine.MovePiston(); } }
52. 52 // インターフェースに依存した状態 class Car { private IEngine engine; public Car(IEngine engine) { this.engine = engine; } public void Run() { engine.ChangeRunMode (); } } interface IEngine { // エンジンを走る状態に変更する void ChangeRunMode (); } class Engine : IEngine { private EngineState state; // 自分の状態は自分でコントロールする public void ChangeRunMode () { this.state = EngineState .RUN; lever.Pull(); piston.Move(); } }
54. 依存性の注入 (Dependency Injection) • CarクラスのコンストラクタでCarが依存する、IEngineを引 数として渡している • 外部からそのクラスが依存する機能を渡すことを依存性 の注入と呼ぶ ◦ これができると、テストで外部からモックを差し込むといったことができる のでとても重要 54
55. 個人的に重要な原則は3つ 55
56. SOLIDの5原則 • 単一責任の原則 (Single Responsibility) • オープン・クローズドの原則 (Open/closed) • リスコフの置換原則 (Liskov substitution) • インターフェース分離の原則 (Interface segregation) • 依存関係逆転の原則 (Dependency inversion) 56
57. 実際にどうやってリファクタリングに立ち向 かったか 57
58. 58 public void LoadAsync (DownloadLicense newLicense , Action <GameObject > onLoadComplete , Action <float> onDownloadProgress = null, Action <Exception > onError = null) { CachedLicense newCachedLicense = new CachedLicense (newLicense ); DownloadLicense ? cachedDownloadLicense = LoadExistLicense (newLicense .licenseid ); if (cachedDownloadLicense != null && newCachedLicense .IsSameModel (cachedDownloadLicense .GetValueOrDefault ())) { SaveCachedLicense (newCachedLicense ); StartCoroutine (LoadAsyncCachedBinary (newCachedLicense , (characterBinary ) => { if (onDownloadProgress != null) onDownloadProgress (1.0f); LoadAsyncFromBinary (characterBinary , (GameObject character ) => { onLoadComplete (character ); }, onError ); })); } else { LoginedRequest (requestPath : "/api/request_path" , methods : HTTPMethods .Get, onSuccess : (downloadHandler ) => { byte[] downloadBinary = downloadHandler .data; LoadAsyncFromBinary (downloadBinary , (GameObject character ) => { Queue.Enqueue (() => { SaveEncryptedModelFile (newCachedLicense , downloadBinary ); SaveCachedLicense (newCachedLicense ); }); onLoadComplete (character ); }, onError ); }, onProgress : onDownloadProgress , onError : (downloadHandler ) => { Exception error = new Exception (downloadHandler .text); if (onError != null) onError (error); } ); } }
59. まずは、このコードを見て、密結合になってし まっているのでテストできるように分離したい 59
60. LoadAsync 60 キャッシュの有無 をチェックする キャッシュから読 み込む HTTPリクエストをし てダウンロードする ダウンロードした ファイルから読み 込む ダウンロードしたの を暗号化して保存 する
61. こう分離できないかな 61
62. LoadAsyn c 62 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
63. LoadAsyn c 63 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
64. もともとのコード 64 CachedLicense newCachedLicense = new CachedLicense(newLicense); DownloadLicense ? cachedDownloadLicense = LoadExistLicense (newLicense.licenseid); if (cachedDownloadLicense != null && newCachedLicense .IsSameModel(cachedDownloadLicense .GetValueOrDefault ())) { } else { }
65. これを状況ごとに機能を振り分けるFactoryに 切り出す (newの隠蔽) 65
66. Factory 66 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } }
67. Factory 67 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } 状況に応じて、ロード機能を分けたモジュールを提供する Factoryを作 成
68. Factory 68 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } キャッシュがある場合は、キャッシュから読み込むローダをコールバッ クに渡す
69. Factory 69 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } キャッシュがない場合は、ライセンスをダウンロードしてきて、そこから ダウンロードする機能を渡す
70. Factory 70 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } 保存機能を提供するモジュール、ダウンロードしたライセンス情報など はDIで入れている
71. Factory 71 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } onSuccessで戻り値になるオブジェクトは実態ではなく、 Interfaceに依存している
72. LoadAsyn c 72 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
73. Factory 73 public static void Create(string Id, ICoroutineHandlable coroutineHandler, Action<ILoader> onSuccess) { CachedLicense? cachedLicense = LicenseManager.LoadExistLicense(Id); if(cachedLicense != null) { onSuccess(new CachedLoader(cachedLicense.GetValueOrDefault(), coroutineHandler, EncriptionFile.ReadBytes)); } else { API.PostDownloadLicense(Id: Id, onSuccess:(DownloadLicense license) => { var newCachedLicense = LicenseManager.LicenseCache(license); var saveModule = new EncryptSave(EncriptionFile.WriteBytes); onSuccess(new DownloadLoader(newCachedLicense, saveModule)); }); } } 暗号化、復号処理は関数として、引数に渡す . これも処理をDIし ている
74. LoadAsyn c 74 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
75. じつはこの2つはどこから読み込むかだけで、 読み込むフォーマットは同じ 75
76. 読み込む機能は継承で両方に提供し、どう取 り出すかだけ分ける 76
78. 78 public abstract class BaseLoader : ILoader { public Action<float> OnProgress { get; set; } public Action<Exception> OnError { get; set; } public Action<GameObject> OnLoaded { get; set; } // inteface が提供する部分は継承先で実装する public abstract void Load(); protected void FileLoadImpl (byte[] binary) { // バイナリデータを読み込む実際の処理 } }
79. LoadAsyn c 79 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
80. 80 public class DownloadLoader : BaseLoader { private CachedLicense cachedLicense ; private ISavable fileSavable ; public DownloadLoader (CachedLicense license, ISavable save) { cachedLicense = license; fileSavable = save; } public override void Load() { LoginedRequest (requestPath : "/api/request_path" , methods: HTTPMethods .Get, onSuccess: (downloadHandler ) => { byte[] downloadBinary = downloadHandler .data; fileSavable .Save(downloadBinary ); FileLoadImpl (downloadBinary ); }, onProgress : onDownloadProgress , onError: (downloadHandler ) => { Exception error = new Exception(downloadHandler .text); if (onError != null) onError(error); } ); } }
81. 81 public class DownloadLoader : BaseLoader { private CachedLicense cachedLicense ; private ISavable fileSavable ; public DownloadLoader (CachedLicense license, ISavable save) { cachedLicense = license; fileSavable = save; } public override void Load() { LoginedRequest (requestPath : "/api/request_path" , methods: HTTPMethods .Get, onSuccess: (downloadHandler ) => { byte[] downloadBinary = downloadHandler .data; fileSavable .Save(downloadBinary ); FileLoadImpl (downloadBinary ); }, onProgress : onDownloadProgress , onError: (downloadHandler ) => { Exception error = new Exception(downloadHandler .text); if (onError != null) onError(error); } ); } } ファイルのダウンロード機能をここでは いれて、ロード機能の実態は基底クラ スに入れている
82. LoadAsyn c 82 キャッシュから読 み込む ダウンロードした ファイルから読み 込む HTTPリクエスト をしてダウン ロードする ダウンロードしたの を暗号化して保存 する キャッシュの有 無をチェックす る
83. 83 public class CachedLoader : BaseLoader { private ICoroutineHandlable CoroutineHandler ; private CachedLicense cachedLicense ; private Func<string , byte[]> fileDecryptFunc ; // コールチンを実行する機能、ファイルを復号する機能を注入 public CachedLoader (CachedLicense license , ICoroutineHandlable coroutine , Func<string , byte[]> decryptFunc ) { cachedLicense = license ; CoroutineHandler = coroutine ; fileDecryptFunc = decryptFunc ; } public override void Load() { cachedLicense .Save(); CoroutineHandler .RunMonoCoroutine (LoadAsyncCachedBinary (cachedLicense )); } private IEnumerator LoadAsyncCachedBinary (CachedLicense cachedLicense ) { byte[] binary = new byte[] { }; UnityThreadQueue .Instance .Enqueue (() => { binary = LoadCachedBinary (cachedLicense ); }); yield return WaitFor (); if (OnProgress != null) { OnProgress (1.0f); } FileLoadImpl (binary ); } }
84. 84 public class CachedLoader : BaseLoader { private ICoroutineHandlable CoroutineHandler ; private CachedLicense cachedLicense ; private Func<string , byte[]> fileDecryptFunc ; // コールチンを実行する機能、ファイルを復号する機能を注入 public CachedLoader (CachedLicense license , ICoroutineHandlable coroutine , Func<string , byte[]> decryptFunc ) { cachedLicense = license ; CoroutineHandler = coroutine ; fileDecryptFunc = decryptFunc ; } public override void Load() { cachedLicense .Save(); CoroutineHandler .RunMonoCoroutine (LoadAsyncCachedBinary (cachedLicense )); } private IEnumerator LoadAsyncCachedBinary (CachedLicense cachedLicense ) { byte[] binary = new byte[] { }; UnityThreadQueue .Instance .Enqueue (() => { binary = LoadCachedBinary (cachedLicense ); }); yield return WaitFor (); if (OnProgress != null) { OnProgress (1.0f); } FileLoadImpl (binary ); } } 毎フレーム、キャッシュファイルからバ イナリを読むことだけやっている
85. 最終的な構成 85
87. こうすることでモックを使い適切にテストを実装 することができるようになった 87
88. まとめ • プロダクトコードは様々な要因によって読みにくくなる • リファクタリングをするには「テストを適切に書ける」ような コードを目指そう • そのためには、正しいオブジェクト指向の原則の理解が必 須 88
89. 最後に • SOLID 5原則はちょっと曖昧な表現で、もう少し具体的にどうしたらいいかわからないとい う人は、これがオススメです 89 • ThoughtWorksアンソロジー • 第5章 「オブジェクト指向エクササイズ」 • 以下のように具体的な指針が書いてあ る ◦ 一つのメソッドにインデントは一段 まで ◦ else区は使わない ◦ getter/setterを使わない など