こんにちは。コミュニケーションアプリ LINE のクライアントを開発している安藤です。
この記事では、DroidKaigi 2022 の企業ブースで行った Code Review Challenge の 4 問目の解説をします。Code Review Challenge についてはこちらを参照してください。
出題タイトル: What layout state class was really needed?
4 問目では、以下のようなコードが出題されました。
data class StickerLayoutState(
val stickerID: StickerID,
val stickerName: String,
val stickerImage: String,
val stickerType: StickerType,
val creatorName: String,
val creatorImage: String,
val reviewPageIndex: Int,
val reviewPageTotalCount: Int,
val currentPageReviews: List<ReviewModel>,
val isDetailPageOpened: Boolean,
val publicationDateText: String,
val contentSummaryText: String,
)
private var layoutState: StickerLayoutState = StickerLayoutState.Empty
class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl)
class PageUrl(val officialUrl: String?, val userPageUrl: String?)
fun setSticker(binding: StickerLayoutBinding, stickerModel: StickerModel) {
layoutState = layoutState.copy(
stickerID = stickerModel.id,
stickerName = stickerModel.stickerName,
creatorName = stickerModel.creatorName,
stickerImage = stickerModel.stickerImage
)
updateViewByBinding(binding)
}
fun setDetailLayoutOpened(binding: StickerLayoutBinding, isOpened: Boolean) {
layoutState = StickerLayoutState.Empty.copy(isDetailPageOpened = isOpened)
updateViewByBinding(binding)
}
fun onReviewPageLoaded(
binding: StickerLayoutBinding,
reviewPageIndex: Int,
reviewPageTotalCount: Int,
currentPageReviews: List<ReviewModel>
) {
layoutState = layoutState.copy(
reviewPageIndex = reviewPageIndex,
reviewPageTotalCount = reviewPageTotalCount,
currentPageReviews = currentPageReviews
)
updateViewByBinding(binding)
}
fun onDetailLayoutLoaded(
binding: StickerLayoutBinding,
stickerModel: StickerModel,
detaliModel: StickerDetailModel
) {
layoutState = layoutState.copy(
isDetailPageOpened = true,
stickerType = stickerModel.stickerType,
creatorName = stickerModel.creatorName,
stickerImage = detaliModel.stickerImage,
creatorImage = detaliModel.creatorImage,
publicationDateText = detaliModel.publicationDateText,
contentSummaryText = detaliModel.contentSummaryText
)
updateViewByBinding(binding)
}
private fun updateViewByBinding(binding: StickerLayoutBinding) { /*update view by `layoutState`*/ }クラス・関数のスコープの問題
まずこの"ファイル"を見たときに一番最初に目につくところは"1つのファイルの中にグローバルアクセス可能な複数のクラスや関数、プロパティがトップレベルで記述されていること"だと思います。
オフラインでのチャレンジ中に真っ先に指摘されたのもこのトップレベルで定義されているレイアウトステートでした。
ファイルオブジェクトにプロパティとして保持されている単一のレイアウトステートに依存してビューを変更するような関数が存在するのは非常に危険です。
なぜなら、このレイアウトステートを元に複数のレイアウトが生成されてしまった場合、それだけで簡単にステート管理が破綻してしまうからです。
また、このファイルに記述されているデータクラスや関数などはすべてグローバルアクセスが可能となってしまっています。
Stickerという機能に関連した名前がつけられているにもかかわらず、あらゆる箇所から呼び出せてしまうのは関数の無理な流用などさらなる問題の原因となります。
このように根本的な問題が存在する場合、設計から見直し1から作り直すというのも1つの手段です。
しかし、今回はこのクラスをベースに改善する方法を考えます。
レイアウトステートをトップレベルプロパティとして定義しないための手段として最も簡単な方法はクラスのプロパティとしてレイアウトステートを保持することでしょう。
さらに単純に言い換えると、コード全体をブレースで囲いクラスとして定義することです。
まずは`StickerViewPresenter`というクラスで全体を囲います。
class StickerViewPresenter(val binding: StickerViewBinding) {
data class StickerLayoutState(...)
class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl)
class PageUrl(val officialUrl: String?, val userPageUrl: String?)
private var layoutState: StickerLayoutState = StickerLayoutState.Empty
fun setSticker(binding: StickerLayoutBinding, stickerModel: StickerModel) { //…snip }
...snip
}こうすることで、レイアウトが複数生成された場合も問題なく動作させることができます。
また各関数が呼び出される範囲や、データクラスの使用されるスコープを制限することができます。
クラスや関数はその使用用途に合わせ適切なスコープを考えましょう。
暗黙的なクラス依存関係の問題
Topic 1を適用することで、グローバルアクセス可能なクラスや関数、トップレベルのレイアウトステートを改善することができました。
しかし、Sticker画面はたくさんのプロパティを持つクラス`StickerLayoutState`によって構成されています。
このクラスはそのプロパティの多さから状態の全体像を見渡すことが困難です。
保持されているデータの関連性が分かりづらい点を改善する必要があります。
解決するための次の目標は`StickerLayoutState`を分解し、適切な形にすることです。
多数のプロパティが含まれているクラスのリファクタリングを行う場合は、対象となるクラスが依存している末端のクラス(今回の場合は`StickerType`や`PageUrl`)からリファクタリングを行う方が良いでしょう。
`StickerType`と`PageUrl`の定義は次の通りです
class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl)
class PageUrl(val officialUrl: String?, val userPageUrl: String?)`PageUrl` クラスは`nullable String`を2つ持っています。
`StickerType`クラスは2つの`boolean`と`pageUrl`を持っています。
これらのクラスはお互いに関係がないように見えますが、実は次のような暗黙的な関係を持っています。
- `isOfficial`が`true`の時
- `officialUrl`は`non-null`である
- `isUserMade`は`false`、`userPageUrl`は`null`である
- `isUserMade`が`true`の時
- `userPageUrl`は`non-null`である
- `isOfficial`は`false`, `officialUrl`は`null`である
そのため、`isOfficial`と`isUserMade`どちらも`true`, どちらも`false`になるような組み合わせは不正と言えます。
このような暗黙的な依存関係を持つクラス構造は他の実装者による誤解を招きやすく、実装ミスを誘発します。
今回の場合は次のようにsealed classを用いて記述するのが良いでしょう。
sealed class StickerType {
data class UserMade(val userPageUrl: String) : StickerType()
data class Official(val officialUrl: String) : StickerType()
}この変更により不正な値を発生させる余地がなくなり、クラスの依存関係が明確になります。
実際の仕様に合わせて適切に依存関係を明示しながらデータ構造を作成することが重要です。
データクラス・モデルクラスの分割の問題
前述した`sealed class`の導入により、`StickerLayoutState`が依存しているクラスが整理されました。
これにより`StickerLayoutState`を適切に分割することができるようになります。
data class StickerLayoutState(
val stickerID: StickerID,
val stickerName: String,
val stickerImage: String,
val stickerType: StickerType,
val creatorName: String,
val creatorImage: String,
val reviewPageIndex: Int,
val reviewPageTotalCount: Int,
val currentPageReviews: List<ReviewModel>,
val isDetailPageOpened: Boolean,
val publicationDateText: String,
val contentSummaryText: String,
)この多数のパラメータを持つデータクラスをどのようにすればより良い形にすることができるでしょうか。
分割の方法はいくつかあると思いますが、今回は使用する画面に合わせて分割することを考えてみましょう。
現在定義されている関数は次の通りです。
fun setSticker(…) {}
fun setDetailLayoutOpened(…) {}
fun onReviewPageLoaded(…) {}
fun onDetailLayoutLoaded(…) {}インターフェースから察するに`Sticker`, `Detail`, `Review`の3つの画面をコントロールしていることが予想できます。
各画面の要素に合わせて次のようにデータクラスを分割してみましょう。
data class StickerBasicDataModel(
val stickerId: StickerId,
val stickerName: String,
val creatorName: String,
val stickerImage: String
)
data class StickerDetailPageState(
val stickerType: StickerType,
val creatorName: String,
val stickerImage: String
val creatorImage: String
val publicationDateText: String,
val contentSummaryText: String
)
data class CommentPageState(
val reviewPageIndex: Int,
val reviewPageTotalCount: Int,
val currentPageReviews: List<ReviewModel>
)このように分割することで、プレゼンターは各データクラスのインスタンスを管理することだけを責任範囲とし、保持されているプロパティを個々に見る必要がなくなります。
責任範囲を構造化して分割することで、各クラスの責任範囲やクラスの持つプロパティの関係性も見通しがよくなるでしょう。
変数・関数の命名の問題
ここまでのレビューでデータクラスや関数の構造を修正することができました。
しかし、まだ細かい部分に問題が残っています。それは変数や関数の命名についてです。
再定義したクラスには`stickerImage`や`creatorImage`という`String`型の変数が含まれています。
これは一体何を示しているのでしょうか?
`String`型の`stickerImage`に格納されるものとして想定できるものとして`Url`や`file path`, `svgのraw xml`,`base64エンコードされたバイナリー`など様々なものがあると思います。
これらは仕様によって決定するものです。設計者は仕様を理解しているため、`stickerImage`に`Urlが格納されること`を暗黙的に理解しているかもしれませんが、他の実装者はそうとは限りません。できる限り変数名から実際に代入される値を予測できるような名前にすべきでしょう。
e.g. stickerImageUrl, stickerImageFilePath,...etc
関数名に関しても同様です。
現在の関数名はその関数が呼ばれた際に何を行うのか不明瞭です。
次のように関数が具体的に何を行うかを関数名で明示できている状態がよいでしょう。
fun showStickerView(stickerModel: StickerBasicDataModel) {}
fun showStickerDetailView(
stickerDetailPageState: StickerDetailPageState,
isDetailPageOpened: Boolean
) {}
fun showCommentView(commentPageState: CommentPageState) {}
まとめ
この記事では、Code Review Challenge の 4 問目の解説として、以下の改善が必要なことを説明しました。
- クラス・関数のスコープを必要十分に設定する
- 暗黙的なクラス依存関係を排除し、クラスが示すものを明示する
- データクラス・モデルクラスを使用用途に合わせて適切に定義する
- 名前から何を行っているのか・何をするのかがわかる、明示的な変数・関数名を設定する
レビューの際にコードの差分のみを見ているとどうしてもクラス間の依存関係などを全体の構造に関する問題を見落としてしまいがちです。
今回の変更範囲で何を行おうとしているのか、広い視野を持って見渡してみるとより良いレビューができるようになると考えています。
私も視野をより広げられるように日々挑戦しています。皆さんもぜひ挑戦してみてください。
第5問目は玉木（https://engineering.linecorp.com/ja/blog/code_review_challenge_5）による解説です。
最終問題の解説をお楽しみに!