実装理解のためのリファクタリングで品質を上げる

はじめに

業務でいわゆるレガシーなコードベースに対して開発を行ってきました。

ユニットテストがなかったり、設計思想が不明だったり、ツギハギだったり...
そんなコードをよく理解しないまま修正し、デグレや新たなバグを埋め込んでしまう...ということが私も過去にありました。

アプリ全体を含めた根本解決の時間が取れれば良いですが、普段の開発もツギハギではなく「立つ鳥跡を濁さず」的に、将来の保守者が同じ混乱に陥らない状況を作っていきたいものです。

過去の経験から、私は作業のはじめに「実装理解のためのリファクタリング」を行うようにしています。

ちなみに、以下書籍にこの記事と同じことが書いてあるので、体系的に学びたい方はこちらを読んでください。(むしろこの記事が劣化コピーです)

実装理解のためのリファクタリング

依頼の対応を完了するまで、以下のステップに分けて作業します。

  • 実装理解のためのリファクタリングを行う
  • (可能なら)上記を元にコードを整理・改善し、リモートリポジトリに適用する
  • 依頼の対応を行う

最初にこの作業を挟むことで思わぬひらめきが得られることがあり、最終的なアウトプットが予想しなかった良い設計やコード量になることもあります。 (DDD 本でブレイクスルーと紹介されるものだと思います)

実装理解のためのリファクタリングでは以下を繰り返します。
ノイズを減らしながら、実装に隠された仕様や裏背景を明らかにします。

コメントをガシガシ書く

現状の実装を理解するための作業なので、自分が重要だと思ったことや不明点など、思いついたことはとにかくコード内コメントでメモしておきます。

思考のノイズとなるコードを減らす

コードを理解することに集中したいので、それを阻害するものをなるべく減らします。

ただし、振る舞いが変わる破壊的な変更は行いません。
例えば、理解しづらい命名やメソッドの粒度をリネーム・インライン化で整える程度にします。

私は普段 IntelliJIDE を利用していますが、長期間メンテされていないコードでは警告や改善提案が出ることがあります。
破壊的な変更でなければサッと対応し、コードリーディング中の脳への割り込みを減らします。

機械的リファクタリングを活用する

IDEリファクタリング機能を使えば、手作業ゆえのバグ埋め込みや対応漏れのリスクを抑えられます。

先程紹介したリネーム、インライン化、メソッド抽出なども私は基本的に IntelliJ IDEリファクタリング機能で対応します。

まとめ

上記のようなリファクタリングを行うことで、デグレの可能性を減らした依頼対応と、既存コードの改善が可能になります。

今回は依頼対応のステップのうち最初だけ記載しましたが、それ以降のステップでもデグレを起こさない取り組みを行っているので、またそれは元気がある時に記事化しようと思います...!

Android 12 からの web intent

Android 12 から web intent の解決方法が変更され、ブラウザで開くというのがデフォルトの動作になりました。
targetSdkVersion に関わらず影響のある変更です。

developer.android.com

Web Intent

web intent は以下の条件を満たす intent です。

  • アクション: ACTION_VIEW
  • カテゴリ: CATEGORY_DEFAULT
    • Android 12 以降は CATEGORY_BROWSABLE の宣言も必要
  • スキーム: http または https
  • ホスト: domain.tld 形式

Android 12 未満では、 AndroidManifest.xml で上記に対応した intent-filter を設定すると、任意の URL をアプリで開くことができます。

<!-- ブラウザ等で https://example.com にアクセスすると ExampleActivity が開く -->
 
<activity android:name=".ExampleActivity" ... >
    <intent-filter>
        <action android:name="android.intent.action.VIEW" />
        <category android:name="android.intent.category.DEFAULT" />
        <category android:name="android.intent.category.BROWSABLE" />
        <data android:scheme="https" android:host="example.com" />
    </intent-filter>
</activity>

これを使って例えばブラウザや SNS から自分のアプリへ誘導することができます。

Android 12 以降、デフォルトでは上記設定をしても https://example.com はブラウザで開かれます(アプリで開かれません)。
アプリで開くようにする方法は 2つです。

  • Android AppLinks に対応する
  • ユーザに、設定アプリでドメインとアプリの関連付けを行ってもらう

AppLinks

Android アプリでリンクを扱う仕組みは DeepLink と言います。
そのうち上記 web intent を扱う仕組みを WebLinks、さらにその中にあるのが Android AppLinks の仕組みです。

f:id:takorras:20211108120722p:plain:w250
日本語ドキュメントにはこの図がなかった...

DeepLink と Android AppLinks の違いは以下の通りです。

f:id:takorras:20211106140219p:plain

AppLinks の特徴は以下です。

  • より安全で具体的
    • ドメインとアプリの関連付けが明確になるため
      • 許可するアプリ一覧を示した json ファイルを、ドメイン配下に置く
        • 指定のない外部アプリは、そのドメインに対するリンクを開けなくなる(ただユーザが設定すれば変更可能だと思われる)
  • よりスムーズ
    • アプリチューザを表示することなくアプリを開くため
      • DeepLink では、リンクを解決可能なアプリが複数あった場合チューザが表示され、ユーザが選択する必要がある

AppLinks を有効にする手順は 2つです。

  • 対象の intent-filter に android:autoVerify="true" を追加する
  • Digital Asset Links json ファイルをホスティングする

Digital Asset Links json ファイルに許可したいアプリ id とその署名証明書のフィンガープリントを記録し、対象ドメインに配置することで、 google がアプリとドメインの関連付けを確認できるようになります。
autoVerify 属性を true にすると、 Android OS が実際に確認してくれます。

json は以下を参考に作成し、
https://[対象ドメイン]/.well-known/assetlinks.json で公開します。 developer.android.com

参考に、 google.com の Digital Asset Links Json ファイルを以下から確認できます。 https://www.google.com/.well-known/assetlinks.json

ユーザに設定からドメインとアプリを紐付けてもらう

AppLinks を使わない場合、こちらで対応できます。
ユーザを設定画面に誘導し、あるドメインへのリンクを開くことを許可してもらいます。

developer.android.com

テストする

developer.android.com

最後に

Android 12 のドキュメントではアプリチューザを省いて UX を簡素化するためとあります。
ただこれまでは任意のアプリが任意の URL を処理できていたので、セキュリティや安全性の観点もありそうです。

当然ですが AppLinks は対象ドメインとアプリを管理している必要があるので(jsonホスティングする必要がある)、サードパーティのクライアントアプリ等にはかなり厳しい変更かもしれません。

許可設定を上手に誘導するのは大変ですし、両方管理している場合は迷わず AppLinks を採用することになりそうです。

資料

developer.android.com

内部テストを使った Android アプリ配信

Google Play Store の内部テストトラックを使いながら、Android アプリをデプロイしてみます 🙋‍♀️

テスト配信

Google Play Store を用いた配信はいくつか方法があり、製品版に加えて、 3つのテストトラックと内部アプリ共有があります developers-jp.googleblog.com

今回は以下の理由から内部テストトラックを選びました

  • オープン・クローズドテストと違って審査がないため、素早く確認できる
  • アップロードしたバイナリをそのまま製品版に移行できる(内部アプリ共有はできない)

内部アプリ共有もかなり便利で、 versionCode の制約がなくデバッグも可能です
このあたりは開発体制やアプリによって柔軟な選択ができそうです

bitrise で内部テスト配信

ということで、ざっくり以下のフローを実現してみます

f:id:takorras:20210206172832p:plain

Fastlane を使う方法もありますが、今回はさくっと既存の step を使います

デプロイ用のサービスアカウントを発行し bitrise にアップロードしたら、 Google Play Deploy step を追加します

devcenter.bitrise.io

また、内部テストのテスターは Google Play Console にてホワイトリスト形式で設定できます

内部テスト配信を確認する

bitrise の workflow が無事完了すると、テスターは内部テスト版アプリをリンクからインストールできるようになります
リンクは Google Play Console のテスター設定画面から取得できます

bitrise の Create install page QR code step を使って、リンクを埋め込んで slack に通知する、とかも便利かもしれません 🙋‍♀️

f:id:takorras:20210206175939p:plain

製品版へ移行する

内部テスト版に問題ないことが分かったら、製品版へ移行させます

内部テストの一覧から該当のリリースを見つけたら、「リリースをプロモート」から「製品版」を選択します

f:id:takorras:20210206180804p:plain

するとドラフトの製品版リリースが作成され、編集画面に遷移します
内部テストに使ったバイナリが紐付いているので、あとはリリースノートや段階的更新など諸々を設定してリリース完了です 🎉

(このあたりはドキュメントが見つからず、プロモートを押すと何が起こるのかヒヤヒヤしました)

まとめ

神経質な自分にとって、手動でのバイナリアップロードは思ったより心理的な負荷がありました
当然ですが、自動化によってかなり楽になったと思います

運用にあわせてそれぞれのテストトラックを活用していきたいです 🙆‍♀️

(ちなみに、作業時点で内部アプリ共有は Google Play Deploy step で使えませんでした。 Fastlane にメソッドがあるので、そちらで組み立てられます)

bitrise がいつの間にか .gradle をキャッシュしてた

以前、 iOS アプリの bitrise workflow を cache を用いて改善した takorras.hatenablog.com

Android アプリも CI サービスに bitrise を使っているので、同様の対応を行ってみる

bitrise cache を使う

以前の記事の通り、 cache-pull step と cache-push step を適切に追加する

gem が cache され当初の目的通り時間が短縮されたが、 cache size が900MB ほどに膨れていることに気が付いた 👀
とはいえ、 gem だけでここまでのサイズになることはなさそう...

settings -> manage build cache から cache をダウンロードできるので、そこから確認する devcenter.bitrise.io

gradle がキャッシュされてて、サイズが 800MB 以上ある... f:id:takorras:20201110225310p:plain

gradle cache

どうやら cache-pull / cache-push step で Android Build step を挟んだ時に勝手に gradle をキャッシュするらしい
devcenter.bitrise.io

今回 Android Build step は使ってなくて Gradle Runner step だったけど、とりあえず .gradle をキャッシュするような仕組みになっているのかな...?

cache-push のログを見ても、勝手に config に追加されている気配がある(自分で設定してるのは上の2行だけ) f:id:takorras:20201110231411p:plain

.gradle って何者?については gradle のドキュメントに概要が書いてある docs.gradle.org

The Gradle user home directory ($USER_HOME/.gradle by default) is used to store global configuration properties and initialization scripts as well as caches and log files.

プロジェクトに依らない gradle の全体的な設定とか cache を諸々を配置しているらしい
また、このディレクトリ内も定期的に cleanup してくれるみたい

ということで、これはそのまま cache させて活用してみる

.gradle を毎回作成していたためか、 cache によって Gradle Runner step を使っていた箇所が 2, 3分短縮された😊

bitrise のビルドをちょっとだけ短縮

iOS アプリの bitrise ビルドが10分を超える事が多いので、まずは全体を見て手早く改善できるところを探してみた

最も時間がかかるのは xcode-test step だけど、これはアプリ側で色々見直さないといけなさそうで一旦スキップ...

次に目についたのが、 bundle install という script step 👀
bundler をインストールして、 fastlane や danger といった gem をインストールしている

...
+---+---------------------------------------------------------------+----------+
| ✓ | bundle install                                                | 2.1 min  |
+---+---------------------------------------------------------------+----------+
...

bitrise のキャッシュ

毎回インストールする必要はないので、 bitrise にキャッシュさせたい

devcenter.bitrise.io

実行する workflow の最初と最後に、 Cache:Pull と Cache:Push の step を挟み込むと、指定のディレクトリやファイルを s3 にキャッシュしてくれる

The cache is stored as a single archive file: if the content of the cached paths changes in any way, the entire file gets updated. Every branch of your repository on which you run a build will have its own cache archive.

キャッシュはビルドを実行するブランチごとに保存されて、7日使わないと削除される

ブランチごとにキャッシュを持っているなら、新しくブランチを作る度にキャッシュが無効になるの...?と思ったけど、もちろんそんなことはなさそう

If you only want to delete the cache which is related to a single branch, you should also delete the default branch’s cache too! This is because if a build runs on a branch which doesn’t have a cache, the Bitrise.io Cache:Pull Step will get the cache of the default branch.

新しいブランチなどキャッシュが存在しない場合、 Cache:Pull step はデフォルトブランチのキャッシュを利用してくれる

また、指定したファイルをトリガーにキャッシュを更新することもできる
何をトリガーに、何をキャッシュするのかを workflow editor の Cache Paths で指定できる

今回は、 gem のインストール先の vendor/bundle をキャッシュして、 Gemfile.lock に更新が合った時にキャッシュも更新させたいので、以下を追記した
vendor/bundle -> Gemfile.lock

f:id:takorras:20200711161743p:plain

テストしてみる

上記の変更を加えた適当なworkflowでテストしてみる 💭

初回のビルド

bundle install の実行時間は変わらなかったが、 Cache:Push でキャッシュされた(removed file があるのは、おそらく既に Cache:Pull と Cache:Push を導入していた影響...)

...

Checking for file changes
1148 files needs to be removed
0 files has changed
8086 files added
File changes found in 4.069593ms

Generating cache archive
Done in 13.97858345s

Uploading cache archive
Archive file size: 148219904 bytes / 141.353516 MB
Done in 3.691688272s

2度目のビルド

bundle install の実行時間が 6.62sec に改善された

...
+---+---------------------------------------------------------------+----------+
| ✓ | bundle install                                                | 6.62 sec |
+---+---------------------------------------------------------------+----------+
...

bundle install では、キャッシュされたgemを使っている

+ bundle install --path vendor/bundle
Using rake 13.0.1
....

また、Gemfile.lock には変更がないので、 Cache:Push ではキャッシュも更新していない

...
Checking for file changes
0 files needs to be removed
0 files has changed
0 files added
No files found in 4.308899ms

Total time: 317.307518ms

期待通り動作してくれていそう😎
bundle install の実行時間は、 2.1min → 6.6sec に短縮された

3度目のビルド

試しに適当なgemのバージョンを変更してみると、 bundle install では当然該当の gem だけインストールされた

...
Using terminal-table 1.8.0
Fetching danger 8.0.1
Installing danger 8.0.1
Using thor 0.20.3
...

Gemfile.lock の変更を検知して、キャッシュも更新されている

Checking for file changes
0 files needs to be removed
8086 files has changed
121 files added
File changes found in 5.523209ms

Generating cache archive
Done in 2.66160931s

Uploading cache archive
Archive file size: 148735488 bytes / 141.845215 MB
Done in 3.391270071s

Total time: 6.574466233s

まとめ

bitrise のキャッシュは簡単に利用できてとっても便利

今回のさくっとした変更で短縮できたビルド時間は2分程度だけど、頻繁に使うのでかなりの嬉しい改善になった
今後も手が空いた時にビルド時間の改善にしてみる👀

presenting/presented VC と dismiss

モーダル遷移なら present/dismiss ・プッシュ遷移なら push/pop を使うけど、
present の関係と、 dismiss を呼んだ時の動きについてちょっとメモ

ドキュメントに書いてある通りだけど....

https://developer.apple.com/documentation/uikit/uiviewcontroller/1621505-dismiss

presentingViewController と presentedViewController

The presenting view controller is responsible for dismissing the view controller it presented.

presenting view controller と presented view controller は紛らわしいけど、 present を使ってモーダル遷移をした時はこの関係が産まれ、プロパティにも格納される

例えば、A →(present)→ B →(present)→ C という遷移の場合は以下のような関係になる

VC presentingVC presentedVC
A nil B
B A C
C B nil

presentingViewController は自分を present した ViewController で、
presentedViewController は自分が present した ViewController になる

dismiss

この関係を把握した上でドキュメントを読むとすっきり

If you call this method on the presented view controller itself, UIKit asks the presenting view controller to handle the dismissal.

ある VC で dismiss を呼んだ時は、 presentingViewController に破棄するよう依頼する
なので push 遷移してきた場合など、 presentingViewController = nil な時に dismiss してももちろん破棄されない

If you present several view controllers in succession, thus building a stack of presented view controllers, calling this method on a view controller lower in the stack dismisses its immediate child view controller and all view controllers above that child on the stack.

複数の VC を連続して present している時に、下位の VC で dismiss すると、その presentedViewController 、さらにその presentedViewController ... と、スタックのそれより上の VC を全て破棄する

先程の例で A.dismiss すると、 B も C も dismiss される

この時、閉じるアニメーションが適用されるのは最上位の VC で、その中間の VC はアニメーションなく破棄される

まとめ

dismiss メソッドは「モーダル遷移から戻るメソッド」という理解だったけど、具体的には2つの役割があって「自身を破棄するよう依頼すること」、「自身からモーダル遷移した VC を破棄すること」をしている 👀

Protocol Extension で UICollectionView をちょっと便利に

UICollectionView を使う際、以下のようにセルを再利用していました

// CustomCell.swift

class CustomCell: UICollectionViewCell {
    static func nib() -> UINib {
        return UINib(nibName: String(describing: self), bundle: nil)
    }

    static func reuseIdentifier() -> String {
        return String(describing: self)
    }
}

// CustomViewController.swift
...

// セルを登録
collectionView.register(
    CustomCell.nib(), 
    forCellWithReuseIdentifier: CustomCell.reuseIdentifier()
)

...

// 登録したセルを取り出す
collectionView.dequeueReusableCell(
    withReuseIdentifier: CustomCell.reuseIdentifier(), 
    for: indexPath
)

この .nib .reuseIdentifier をカスタムセルごとに書くのが面倒になったので、今回は protocol extension を使って解消しようと思います

(UICollectionView については以下)
https://developer.apple.com/documentation/uikit/uicollectionview

Protocol Extension

Protocols — The Swift Programming Language (Swift 5.1)

protocol を拡張して実装し、適合した型に提供することができます

拡張してコードを共通化するなら通常の extension でも良いのですが、こちらはより柔軟で再利用性が高いです

先程の .nib .reuseIdentifier メソッドを protocol extension で実装します

// 1. protocol を定義
protocol ReusableProtocol {
    static func nib() -> UINib
    static func reuseIdentifier() -> String
}

// 2. protocol を実装
extension ReusableProtocol {
    static func nib() -> UINib {
        return UINib(nibName: String(describing: self), bundle: nil)
    }

    static func reuseIdentifier() -> String {
        return String(describing: self)
    }
}

// 3. UICollectionView に適合
extension UICollectionViewCell: ReusableProtocol {}

これで、 UICollectionViewCell のサブクラスで .nib .reuseIdentifier` メソッドが使えるようになりました

通常の extension と比較して

再利用性が高い

protocol なので、他のクラスに適合させるだけで再利用できます
UITableViewCell で再利用したい...場合でも、コードは以下の1行

extension UITableViewCell: ReusableProtocol {}

対象を限定できる

protocol を実装する際に where を使うことで、対象を絞ることができます
UICollectionViewCell でのみ使いたい際は以下のようにする

extension ReusableProtocol where Self: UICollectionViewCell {
    static func nib() -> UINib {
        return UINib(nibName: String(describing: self), bundle: nil)
    }

    static func reuseIdentifier() -> String {
        return String(describing: self)
    }
}

そのほか

デメリットとしては、そのまま objc から使えない点でしょうか...(objc 側で定義してあげれば使える?)

extension をうまく使えば UICollectionView の .register / .dequeue も簡略化できそうです
名前空間とやりすぎに注意しつつ使おうと思います