実装理解のためのリファクタリングで品質を上げる
はじめに
業務でいわゆるレガシーなコードベースに対して開発を行ってきました。
ユニットテストがなかったり、設計思想が不明だったり、ツギハギだったり...
そんなコードをよく理解しないまま修正し、デグレや新たなバグを埋め込んでしまう...ということが私も過去にありました。
アプリ全体を含めた根本解決の時間が取れれば良いですが、普段の開発もツギハギではなく「立つ鳥跡を濁さず」的に、将来の保守者が同じ混乱に陥らない状況を作っていきたいものです。
過去の経験から、私は作業のはじめに「実装理解のためのリファクタリング」を行うようにしています。
ちなみに、以下書籍にこの記事と同じことが書いてあるので、体系的に学びたい方はこちらを読んでください。(むしろこの記事が劣化コピーです)
実装理解のためのリファクタリング
依頼の対応を完了するまで、以下のステップに分けて作業します。
最初にこの作業を挟むことで思わぬひらめきが得られることがあり、最終的なアウトプットが予想しなかった良い設計やコード量になることもあります。 (DDD 本でブレイクスルーと紹介されるものだと思います)
実装理解のためのリファクタリングでは以下を繰り返します。
ノイズを減らしながら、実装に隠された仕様や裏背景を明らかにします。
コメントをガシガシ書く
現状の実装を理解するための作業なので、自分が重要だと思ったことや不明点など、思いついたことはとにかくコード内コメントでメモしておきます。
思考のノイズとなるコードを減らす
コードを理解することに集中したいので、それを阻害するものをなるべく減らします。
ただし、振る舞いが変わる破壊的な変更は行いません。
例えば、理解しづらい命名やメソッドの粒度をリネーム・インライン化で整える程度にします。
私は普段 IntelliJ の IDE を利用していますが、長期間メンテされていないコードでは警告や改善提案が出ることがあります。
破壊的な変更でなければサッと対応し、コードリーディング中の脳への割り込みを減らします。
機械的なリファクタリングを活用する
IDE のリファクタリング機能を使えば、手作業ゆえのバグ埋め込みや対応漏れのリスクを抑えられます。
先程紹介したリネーム、インライン化、メソッド抽出なども私は基本的に IntelliJ IDE のリファクタリング機能で対応します。
まとめ
上記のようなリファクタリングを行うことで、デグレの可能性を減らした依頼対応と、既存コードの改善が可能になります。
今回は依頼対応のステップのうち最初だけ記載しましたが、それ以降のステップでもデグレを起こさない取り組みを行っているので、またそれは元気がある時に記事化しようと思います...!
Android 12 からの web intent
Android 12 から web intent の解決方法が変更され、ブラウザで開くというのがデフォルトの動作になりました。
targetSdkVersion に関わらず影響のある変更です。
Web Intent
web intent は以下の条件を満たす intent です。
- アクション:
ACTION_VIEW
- カテゴリ:
CATEGORY_DEFAULT
- Android 12 以降は
CATEGORY_BROWSABLE
の宣言も必要
- Android 12 以降は
- スキーム:
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つです。
AppLinks
Android アプリでリンクを扱う仕組みは DeepLink と言います。
そのうち上記 web intent を扱う仕組みを WebLinks、さらにその中にあるのが Android AppLinks の仕組みです。
DeepLink と Android AppLinks の違いは以下の通りです。
AppLinks の特徴は以下です。
- より安全で具体的
- よりスムーズ
- アプリチューザを表示することなくアプリを開くため
- DeepLink では、リンクを解決可能なアプリが複数あった場合チューザが表示され、ユーザが選択する必要がある
- アプリチューザを表示することなくアプリを開くため
AppLinks を有効にする手順は 2つです。
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 を使わない場合、こちらで対応できます。
ユーザを設定画面に誘導し、あるドメインへのリンクを開くことを許可してもらいます。
テストする
最後に
Android 12 のドキュメントではアプリチューザを省いて UX を簡素化するためとあります。
ただこれまでは任意のアプリが任意の URL を処理できていたので、セキュリティや安全性の観点もありそうです。
当然ですが AppLinks は対象ドメインとアプリを管理している必要があるので(json をホスティングする必要がある)、サードパーティのクライアントアプリ等にはかなり厳しい変更かもしれません。
許可設定を上手に誘導するのは大変ですし、両方管理している場合は迷わず AppLinks を採用することになりそうです。
資料
内部テストを使った Android アプリ配信
Google Play Store の内部テストトラックを使いながら、Android アプリをデプロイしてみます 🙋♀️
テスト配信
Google Play Store を用いた配信はいくつか方法があり、製品版に加えて、 3つのテストトラックと内部アプリ共有があります developers-jp.googleblog.com
今回は以下の理由から内部テストトラックを選びました
- オープン・クローズドテストと違って審査がないため、素早く確認できる
- アップロードしたバイナリをそのまま製品版に移行できる(内部アプリ共有はできない)
内部アプリ共有もかなり便利で、 versionCode の制約がなくデバッグも可能です
このあたりは開発体制やアプリによって柔軟な選択ができそうです
bitrise で内部テスト配信
ということで、ざっくり以下のフローを実現してみます
Fastlane を使う方法もありますが、今回はさくっと既存の step を使います
デプロイ用のサービスアカウントを発行し bitrise にアップロードしたら、 Google Play Deploy step を追加します
また、内部テストのテスターは Google Play Console にてホワイトリスト形式で設定できます
内部テスト配信を確認する
bitrise の workflow が無事完了すると、テスターは内部テスト版アプリをリンクからインストールできるようになります
リンクは Google Play Console のテスター設定画面から取得できます
bitrise の Create install page QR code step を使って、リンクを埋め込んで slack に通知する、とかも便利かもしれません 🙋♀️
製品版へ移行する
内部テスト版に問題ないことが分かったら、製品版へ移行させます
内部テストの一覧から該当のリリースを見つけたら、「リリースをプロモート」から「製品版」を選択します
するとドラフトの製品版リリースが作成され、編集画面に遷移します
内部テストに使ったバイナリが紐付いているので、あとはリリースノートや段階的更新など諸々を設定してリリース完了です 🎉
(このあたりはドキュメントが見つからず、プロモートを押すと何が起こるのかヒヤヒヤしました)
まとめ
神経質な自分にとって、手動でのバイナリアップロードは思ったより心理的な負荷がありました
当然ですが、自動化によってかなり楽になったと思います
運用にあわせてそれぞれのテストトラックを活用していきたいです 🙆♀️
(ちなみに、作業時点で内部アプリ共有は 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 以上ある...
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行だけ)
.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 にキャッシュさせたい
実行する 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
テストしてみる
上記の変更を加えた適当な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 も簡略化できそうです
名前空間とやりすぎに注意しつつ使おうと思います