先日プラハチャレンジのメンターセッションの一環でアプリケーションのコードレビューをしていた時に「そういえば自分は新規プロジェクトのコードを読むときによくprivate methodを使って処理を抽象化しているから、これを記事にしたら誰かの役に立つかも」と思い、筆を取りました。
対象読者
- 新しく参画したプロジェクトで複雑なコードをレビューしたり、仕様を追う必要がある
- Single Level of Abstraction Principle(SLAP)が気になる
- private methodを使い倒してみたい
題材
実際のコードを見ながら紹介した方が分かりやすいと思うので本人の同意を得てコードを提供していただきました。
さて今回の題材はプラハチャレンジというオンラインブートキャンプです。今回のコードに関連する仕様は以下の通り：
- 複数のUserがPairを構成し、複数のPairがTeamを構成する
- 「User -> Pair -> Team」みたいなイメージ
- Userは「在籍中」「休会」「退会」のステータスを持つ
- Teamには最小在籍User数がある（例えば在籍中Userが3名未満のTeamは存続できない）
- Pairには最小在籍User数と、最大在籍User数がある（例えば在籍中Userが1名のPairは存続できない）
- 休会・退会中のUserはTeam/Pairの人数にカウントされない
今回は「Userのステータスが変更される」というユースケースを取り上げてみたいと思います。今回はprivate methodに切り出すところだけを見ていきたいので、それ以外のところ（await忘れとかusecaseにdbクライアントをDIしてるところとか）は一切言及しません
実際に提出されたコード
直接関係なさそうなコードは削除していますが、イメージとしてはこんな感じです：
export class UpdateUserUseCase {
private readonly prismaClient: PrismaClient
private readonly userQS: IUserQS
private readonly userRepo: IUserRepository
private readonly removedUserRepo: IRemovedUserRepository
public constructor(
prismaClient: PrismaClient,
userQS: IUserQS,
userRepo: IUserRepository,
removedUserRepo: IRemovedUserRepository,
) {
this.prismaClient = prismaClient
this.userQS = userQS
this.userRepo = userRepo
this.removedUserRepo = removedUserRepo
}
public async do(params: { id: string; statusId: string }) {
const { id, statusId } = params
if (statusId === UserStatus.Enrolled) {
// RemovedUser -> User
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
// User -> RemovedUser
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
} else {
// RemovedUser -> RemovedUser
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
this.removedUserRepo.updateRemovedUser(targetUser)
}
} else {
throw new Error(
'変更するステータスは「在籍中」「休会中」「退会済」のいずれかを選択してください.',
)
}
}
}
いざコードリーディングを始めたものの、私は妻に「ハサミ取ってきて」と言われて台所に向かったのに「そういえば喉が渇いたな」と牛乳を飲んだら満足して手ぶらで帰ってくるほど脳内メモリが少なく、2つ以上のことを同時に考えられません。従ってネストが2つ以上含まれているコードを読むと頭がパンクします。
なので今回のコードも初見ではすんなり処理しきれなかったのですが、こんなミジンコ並みの脳内メモリの持ち主でもコードを読み解く際に役立つのがprivate methodです。
こんな風に活用しています：
- 処理の一部をprivate methodに切り出していく
- private methodを命名していく過程で処理の理解が深まる
- 自信がなければprivate methodを一瞬publicにしてテストする
private methodに切り出していく
まず最初のif文を見ると
if (statusId === UserStatus.Enrolled) {
// RemovedUser -> User
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
これは非常に簡単ですね。statusをenrolledに変更したいときはuserをactivateしているので、ひとまずif文の中身をactivateUserというprivate methodに移してみます。
if (statusId === UserStatus.Enrolled) {
// この辺の処理をごっそりprivate methodに切り出す
// RemovedUser -> User
// const userActivateService = new UserActivate(
// this.prismaClient,
// this.userRepo,
// this.removedUserRepo,
// )
// await userActivateService.userActivate(id)
await this.activateUser()
}
private async activateUser() {
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
}
次はelse ifに目を向けてみます。
else if (
statusId === RemovedUserStatus.Pending || // pending を 休会 の意味で使用したそうです
statusId === RemovedUserStatus.Withdrawn // withdrawn は 退会
) {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
// User -> RemovedUser
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
} else {
// RemovedUser -> RemovedUser
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
this.removedUserRepo.updateRemovedUser(targetUser)
}
ネストしているif文がちょっと集中力を奪ってくるので、ここをprivate methodに切り出してみましょう。
if (await userEnrolledCheckService.isEnrolled(id)) {
ここでは「現在このUserがenrolled（在籍中かどうか）」を判定して、trueだった時、つまり在籍中Userが休会/退会したい時の処理なので、deactivateCurrentlyEnrolledUserというメソッドに切り出してみます。
逆にfalseの時は「既に休会/退会中の人が、休会/退会したい時（例：休会から退会）」なので、private methodをdeactivateCurrentlyNotEnrolledUserと命名して切り出してみます。
呼び出し側はこうなります：
else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
await this.deactivateEnrolledUser() // private methodにまとめた
} else {
await this.deactivateNotEnrolledUser() // private methodにまとめた
}
こんな風にメソッドを切り出してみると、当初のネストしたif文が「今Userが在籍中か否かで休会/退会時の挙動が変わる。どう変わるかは知らんけど」 と、少し抽象化されてイメージしやすくなったのではないでしょうか？
結局どちらもdeactivateしていることに変わりはないことに気づいたので更にdeactivateUserというprivate methodに隠蔽してみます。
else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
// この辺の処理を全部切り出して
// const userEnrolledCheckService = new UserEnrolledCheck(
// this.userQS,
// this.removedUserRepo,
// )
// if (await userEnrolledCheckService.isEnrolled(id)) {
// this.deactivateEnrolledUser() // private methodにまとめた
// } else {
// this.deactivateNotEnrolledUser() // private methodにまとめた
// }
// private methodにまとめた
await this.deactivateUser()
}
「更新ステータスがpending/withdrawnならdeactivateUserする。まぁdeactivateが何をするのかは知らんけど」 って感じです。こうすると大元のif文の見通しが改善されるのではないでしょうか:
if (statusId === UserStatus.Enrolled) {
await this.activateUser() // 具体的に何をするのかは知らんけど
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
await this.deactivateUser() // 具体的に何をするのかは知らんけど
}
private methodに切り出した結果
処理をprivate methodに切り出したコードを大元のdo()から追ってみましょう。
public async do(params: { id: string; statusId: string }) {
const { id, statusId } = params
if (statusId === UserStatus.Enrolled) {
await this.activateUser() // 具体的に何をするのかは知らんけど
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
await this.deactivateUser() // 具体的に何をするのかは知らんけど
} else {
throw new Error(
'変更するステータスは「在籍中」「休会中」「退会済」のいずれかを選択してください.',
)
}
}
「どれどれ...Userをenrolledに更新したければactivate、pending/withdrawnに更新したければdeactivateするのね。deactivateUserの中身はどうなってるのかな？」
private async deactivateUser() {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
await this.deactivateEnrolledUser()
} else {
await this.deactivateNotEnrolledUser()
}
}
「はいはいUserが今enrolledかどうかで処理が分岐するのね。deactivateEnrolledUserだから、いま在籍中のUserをdeactivateする時ってことかな？何が起きるんだろう」
private async deactivateEnrolledUser() {
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
}
「ふむふむ、何らかの複雑な処理がdeactivateServiceに委譲されると。じゃあ、いま在籍していないUserをdeactivateする時は...?」
private async deactivateNotEnrolledUser() {
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
this.removedUserRepo.updateRemovedUser(targetUser)
}
「なるほど在籍していないUserならそもそもTeamやPairに所属していないからドメインルール（Teamの最低在籍数とか）に引っかかることもないから、そのまま更新すれば良いだけなのね」
...
といった具合に、処理を少しずつprivate methodに切り出して命名していくことで、処理の流れが脳内でイメージし易くなっていくのを実感いただけたのではないでしょうか?
一応完成形も貼っておきますね:
public async do(params: { id: string; statusId: string }) {
const { id, statusId } = params
// statusによってactivate/deactivateという抽象化された処理に分岐する事が分かる。細かいことは知らんけど
if (statusId === UserStatus.Enrolled) {
await this.activateUser()
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
await this.deactivateUser()
} else {
throw new Error(
'変更するステータスは「在籍中」「休会中」「退会済」のいずれかを選択してください.',
)
}
}
private async activateUser() {
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
}
// userがenrolledか否かによって処理が分岐することだけが分かる。細かいことは知らんけど
private deactivateUser() {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
await this.deactivateEnrolledUser()
} else {
await this.deactivateNotEnrolledUser()
}
}
private async deactivateEnrolledUser() {
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
}
private deactivateNotEnrolledUser() {
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
await this.removedUserRepo.updateRemovedUser(targetUser)
}
}
もっと例をちょうだい
他にもprivate methodに切り出すことで可読性が向上する例を探してみます。こちらのサイトから拝借しました:
public List<ResultDto> buildResult(Set<ResultEntity> resultSet) {
List<ResultDto> result = new ArrayList<>();
for (ResultEntity entity : resultSet) {
ResultDto dto = new ResultDto();
dto.setShoeSize(entity.getShoeSize());
dto.setNumberOfEarthWorms(entity.getNumberOfEarthWorms());
dto.setAge(computeAge(entity.getBirthday()));
result.add(dto);
}
return result;
}
例として簡単すぎるかもしれませんが、こんな風にprivate methodに切り出すことで可読性が向上しませんかね？
public List<ResultDto> buildResult(Set<ResultEntity> resultSet) {
List<ResultDto> result = new ArrayList<>();
for (ResultEntity entity : resultSet) {
// この辺が全部消えた
// ResultDto dto = new ResultDto();
// dto.setShoeSize(entity.getShoeSize());
// dto.setNumberOfEarthWorms(entity.getNumberOfEarthWorms());
// dto.setAge(computeAge(entity.getBirthday()));
result.add(toDto(entity));
}
return result;
}
// 代わりにこっちに切り出された
private ResultDto toDto(ResultEntity entity) {
ResultDto dto = new ResultDto();
dto.setShoeSize(entity.getShoeSize());
dto.setNumberOfEarthWorms(entity.getNumberOfEarthWorms());
dto.setAge(computeAge(entity.getBirthday()));
return dto;
}
最初のメソッドbuildResultの責務としては
「resultSetの各要素に対してtoDtoを実行した結果を返しますよ。まぁtoDtoが何してるのかは知らんけど」って感じのアバウトな処理になっているので、toDtoの細部が気にならない人に関してはここでコードを追うのを止められます。
つまりこれは「buildResultとtoDtoは処理の抽象度が異なる」と言えるのではないか、と私は考えています。「大体のことがわかれば良いよ」という方はbuildResultさえ読めば充分だけど「もうちょい詳しいことが知りたいよ」という方はtoDtoまで読み進める。
抽象度の高いところから書き始めて少しずつ抽象度を下げていくことで、全ての処理を最初から最後まで追わなくても、読み手側が必要な情報を必要な分だけ取得できるようになる。こうすると可読性が向上する...みたいなイメージなんですが、伝わりますかね...?
こんな風に処理の抽象度を揃えて分割していくパターンには「Single Level of Abstraction Principle(SLAP)」というカッコイイ名前がついているのですが、「要は関数内の処理の抽象度を揃えようってことですよ」と言ってもいまいち伝わらないので、人間同士の会話に例えてみたいと思います。
どちらの説明が分かりやすいか？
あなたは課長です。部下としてAさんとBさんが居ます。どちらの話が理解しやすいでしょうか？
Aさん「課長、コストが230万円に...あ、税込だと253万円に抑えられるA案と、月額30万円のB案がありまして。エンジニア採用の話なんですけど。こちら資料の32ページみていただくと、B案の募集言語はJavaを考えていて。B案の方が人は集まると思うんですけど。A案の方が僕は好きで。どうします？」
Bさん「課長、エンジニア採用企画に関してアドバイスをいただけないでしょうか。具体的にはA案とB案で迷っていまして、コストを抑えるのであればA案ですが、B案の方が人は集まりそうです。より具体的な比較はこちらの資料にまとめてありますので、必要でしたら説明いたします」
圧倒的にBさんの方が丁寧なコミュニケーションをしていますよね。
- 結論から話している （アドバイスをいただけないでしょうか）
- 最初はアバウトな情報を出している（A案とB案で迷っている）
- どんどん具体的になっていく（コストならA、母集団の数ならB）
- 必要な時だけ超・細部の情報を提供する（資料まとめたので必要なら説明します）
逆にAさんは自分の頭に思い浮かんだ情報を思い浮かんだ順番で出しているだけで、聞き手のことを一切考えていません。課長は、こう感じることでしょう：
結局コードの可読性がなぜ重要かと言えば エンジニア同士のコミュニケーションを円滑に進めるため であって、「丁寧なコミュニケーションを取ること」 を目指せば自ずと可読性の高いコードに近づいていくのではないでしょうか。
private methodに切り出していくことで段階的に抽象度を下げていくコードの書き方にも、丁寧なコミュニケーションに通ずるところがありそうです：
- 結論から話している
- （enrollならactivate、pending/withdrawならdeactivateします）
- 最初はアバウトな情報を出している
- （Userが今Enrolledか否かでDeactivateの挙動が変わります）
- どんどん具体的になっていく
- （EnrolledならdeactivateUserServiceを呼び出します）
- 必要な時だけ超・細部の情報を提供する
- （細部が気になる人だけdeactivateUserServiceの中身まで追ってください）
「処理の抽象度を揃える」という定義だと小難しく感じてしまいますが、よりシンプルに 「結論から話し、少しずつ説明の抽象度を下げて、丁寧に伝えていく」 と考えれば自然とSLAPに則ったコードを書けるのではないか？と感じた次第です。
悪しきパターンの代表としてよく挙げられるファットコントローラー（全ての処理をコントローラーに書く）の何がいけないかというと、HTTPやSQLみたいなインフラレベルの具体的な話からドメインロジックのような抽象的な話まで、思いついた情報を一気に並べ立てているから、ちょっと何言っているかわかんないっすな事だと思うんですよね。Aさんと似たような話し方をしている（とにかく思いついた順番に情報を並べ立てている）イメージです。
なのでファットコントローラーを読んでいる時は、いつもこんな気分です：
自分のコード読みづらいのかな？と不安になった時のコツ
自分で書いたコードの可読性を自分で判断するのは結構大変です。自分は完全に理解してますからバイアスが入ってしまいますよね。
そんな時に自分がよく使うのがあえて空行やインデントを削除しても、すんなり理解できるか試してみる、という小技です。あくまで個人的な感覚なので参考にならないかもしれませんが...
なぜインデントを消してみるのか
ネストしたコードの読みづらさが際立つからです。
「今どのif文に入っているんだろう?」とか「ここでthrowした時どのcatchに入るんだろう？」と考えた時、唯一の判断材料がインデントの幅です。なので試しにネストしたコードのインデントを全部消してみてください。マジで読めなくなります。
今回の例も、元のコードからインデントを削除すると...
public async do(params: { id: string; statusId: string }) {
const { id, statusId } = params
if (statusId === UserStatus.Enrolled) {
// RemovedUser -> User
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
// User -> RemovedUser
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
} else {
// RemovedUser -> RemovedUser
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
this.removedUserRepo.updateRemovedUser(targetUser)
}
} else {
throw new Error(
'変更するステータスは「在籍中」「休会中」「退会済」のいずれかを選択してください.',
)
}
}
}
インデントを消されてしまうと、線虫並みの脳内メモリしか持たない私では読み解けそうにありません。一方、分割後のコードはこんな感じです：
public async do(params: { id: string; statusId: string }) {
const { id, statusId } = params
if (statusId === UserStatus.Enrolled) {
await this.activateUser()
} else if (
statusId === RemovedUserStatus.Pending ||
statusId === RemovedUserStatus.Withdrawn
) {
await this.deactivateUser()
} else {
throw new Error(
'変更するステータスは「在籍中」「休会中」「退会済」のいずれかを選択してください.',
)
}
}
private async activateUser() {
const userActivateService = new UserActivate(
this.prismaClient,
this.userRepo,
this.removedUserRepo,
)
await userActivateService.userActivate(id)
}
private async deactivateUser() {
const userEnrolledCheckService = new UserEnrolledCheck(
this.userQS,
this.removedUserRepo,
)
if (await userEnrolledCheckService.isEnrolled(id)) {
await this.deactivateEnrolledUser()
} else {
await this.deactivateNotEnrolledUser()
}
}
private async deactivateEnrolledUser() {
const userDeactivateService = new UserDeactivate(
this.userRepo,
this.removedUserRepo,
)
await userDeactivateService.userDeactivate(id, statusId)
}
private async deactivateNotEnrolledUser() {
const targetUser = await this.removedUserRepo.getRemovedUserByUserId(id)
if (!targetUser) {
throw new Error('指定された参加者が見つかりませんでした.')
}
targetUser.updateStatusId(statusId)
await this.removedUserRepo.updateRemovedUser(targetUser)
}
}
これならインデントに頼らなくてもある程度読めるのではないでしょうか？
コードフォーマッターを使わずコードを書く機会はこの２１世紀滅多にないかもしれませんが、あえてそこに頼らない書き方を模索してみると新しい扉が開けるかもしれません。
なぜ空行を消してみるのか
複数のことを一つの関数で実行しようとしている可能性があるからです。
「関数内を空行で区切ると読み易くなる」ということは、裏を返すと空行が必要な時点で実はその関数は複数のことをやろうとしている可能性がある、と考えてみるわけです。めちゃくちゃ簡単で意味的にも同一の処理だったら空行で分ける必要ないじゃん、という考え方です。
今回private methodに分割しまくったコードを読んでみると、抽象度に応じて分割した結果一つの関数で一つのことしかしていないので、空行の入り込む余地は殆どありません。
とはいえテストを書くときAAAパターン（arrange,act,assert）に従って空行を入れる事もあったり、空行が全て悪だと考えているわけではありませんが、あまりにも多すぎる時は立ち止まって考えるようにしています。
private methodを命名していく過程で処理の理解が深まる
private methodを書いていくもう一つのメリットとして、流れを理解できた処理に名前を付けていくと思考が整理されます。
今回の例であれば「deactivateEnrolledUser」という名前によって「自分は今、在籍中UserをDeactivateする処理の中にいる」と考えやすくなるので、元のコードに付いていた「User -> RemovedUser」といったコメントを削除しても、メソッド名でなんとなく意図が理解できるようになりました。コメントがついていなければ条件分岐の現在状態が理解できないコードになっていたら、適切に命名したprivate methodに切り出してみることで解決するかもしれません。
今回はまだネストが2つなのでマシなのですが、より複雑な条件分岐が加わると自分が今どんな状態なのか全く思い出せなくなります。ヘンゼルとグレーテルではありませんが、自分が歩いてきた道にパンを置くような感覚でメソッド分離と命名を繰り返していくと、いざ迷った時に帰り道を見つけやすくなるのではないでしょうか。原作ではパン食われたけど
自信がなければprivate methodを一瞬publicにしてテストする
今回は不要だったのですが、めちゃくちゃ複雑な処理を分解しているときはprivateメソッドを一時的にpublicにして単体テストを書く事もあります。privateメソッドをテストできるテストライブラリならそれを使うだけでも構いません。
以前私がリーダーを務めたプロジェクトではテストケースを仕様書代わりに使ったぐらい、テストは仕様を表すのに適したツールです。「このユースケースでは、どう動くんだろう？」「境界値のときはどんな挙動になるんだろう？」と色々なケースを作ればガンガン思考が整理されていくので、試してみてね！
まとめ
というわけで今回はナメクジ並みの脳内メモリを持った私が新規プロジェクトのコードレビューで工夫しているポイントを紹介しました。
- 処理の一部をprivate methodに切り出していく
- private methodを命名していく過程で処理の理解が深まる
- 自信がなければprivate methodを一瞬publicにしてテストする
今回のアプローチでは「処理をprivate methodに切り出す」以外のことは一切していません。誤って破壊的な変更を加えてしまう心配は（ほぼ）ないので、複雑な処理を紐解く際には重宝しています。
ついでに、切り出したメソッド名が他の人にも気に入られたらそのまま本番コードに反映しても良いわけですしね。エントリーポイントのpublicメソッド（今回の場合はdo）は一切挙動が変わっていないのでテストもそのまま通るはずです。
ただ細かく切りすぎるとジャンプが多発して逆に読みづらくなるデメリットもあるので、本当に分け方の感覚は非常にすり合わせづらいところがあるのですが...よかったら参考にしてちょ
Discussion