業務でソースコードレビューを行う機会が増えたので、複数回指摘した項目や気になった実装などをまとめてみました。こういう観点をできる人と共有できるといいなあ…。

2014/09/29 23:00 一部修正しました。

業務上ソースコードレビューの名目で仕様・デザインまで見ることになっていたためこれらを先頭に書いていましたが、わかりづらかったため最後にまとめました。

Fragment関連

FragmentとActivityの密結合

Fragmentが特定のActivityから呼ばれることを想定して書かれている場合、そのFragmentとActivityは密結合である場合が多いです。具体的には、以下の様な実装です。

ActivityのViewを参照する
Activityのメソッドを直接呼び出す

なぜダメか

Fragmentの利点のひとつは優れた再利用性にあります。 Fragmentが特定のActivityに依存することで、Fragmentの再利用を妨げます。現時点でアプリ内で再利用しないという場合でも、将来においてメンテナンスが困難になる可能性があります。

解決法

ActivityのViewを参照している場合、そのViewをFragmentに配置できないか検討してください。 ActionBarのタイトルやMenuItemはFragmentでも操作できます。 Fragment内の特定のタイミングでActivityのViewを操作したい場合、後述するコールバックでの処理を検討してください。どうしてもActivityと密結合になってしまう場合、FragmentではなくCustomViewとして実装したほうが良い場合もあります。

Activityのメソッドを直接読んでいる場合、コールバックインターフェイスによる実装を検討してください。

if(getActivity() instanceof MyFragmentCallback){
    ((MyFragmentCallback)getActivity()).callback();
}

Fragmentからの不自然な画面遷移

Fragmentから新しいActivityへ遷移する際、getActivity().startActivity()するような実装は危険です。特に、getActivity().startActivityForResult()を使用するのは避けてください。

なぜダメか

上記のような実装を行うとActivityからFragmentにonActivityResult()を伝達する必要があり、密結合となる場合が多くなります。 Fragmentの再利用性が下がるだけでなく、Activity#onActivityResult()のメンテナンスも困難になります。

解決法

Fragment#startActivity()、Fragment#startActivityForResult()、Fragment#onActivityResult()を利用してください。これらが利用できない(Activityを経由する必要がある)場合、そもそも密結合である可能性高いので、コールバックインターフェイスなどを利用して疎結合にできないか検討してください。

DB関連

Columnsの定義方法が間違っている

ColumnsクラスはBaseColumnsを継承し、BaseColumns._idを主キーフィールドとして使用します。 この値を文字列として再定義しないでください。

なぜダメか

Androidの一部機能(CursorAdapterなど)は主キーフィールド名がBaseColumns._idと同一であることを期待しています。おそらく将来にわたって変わることはありませんが、わざわざアプリ内で再定義して管理する必要もありません。

解決法

ColumnsクラスはBaseColumnsを継承し、BaseColumns._idを主キーフィールドとして使用します。 http://developer.android.com/training/basics/data-storage/databases.html

WebView関連

WebViewのライフサイクルを考慮していない

WebViewの一部機能はActivityやFragmentのライフサイクルと同期させる必要があります。

なぜダメか

ライフサイクルメソッドの呼び出しを怠るとFlashやHtml5コンテンツが休止されないため、音が鳴り止まない、バッテリーを浪費するなどの問題があります。

解決法

WebViewのライフサイクルメソッドを必要に応じて呼び出してください。 API level 11以降であればWebViewFragmentが利用できます。

    @Override
    public void onPause(){
        super.onPause();
        webview.onPause();
    }
    
    @Override
    public void onResume(){
        super.onResume();
        webview.onResume();
    }

    @Override
    public void onDestroy(){
        super.onDestroy();
        webview.destroy();
    }

http://developer.android.com/reference/android/webkit/WebView.html

その他

ソースコードによるレイアウトの共通化

あまり多くありませんが、<include />を使わず、BaseActivity内でヘッダやフッタの追加を行っている場合があります。

なぜダメか

この処理の問題はメンテナンスコストが上がることです。あとからヘッダやフッタを編集する際、本当にBaseActivityだけ編集すれば良いのかどうか判断できません。ヘッダやフッタが必要でない画面や特殊なヘッダを用いる画面では特殊な処理をオーバーライドしている可能性があるためです。 Androidにはレイアウト(の一部)を流用するための仕組みがあるので、ソースコードで共通化の処理を行う必要はありません。

解決法

レイアウトxml内で<include />を使用してください。あるいは、Activity同士の継承ではなくFragmentによる画面の一部遷移という形式にできる可能性もあります。

引数における型指定

メソッド引数の型指定をより良くできる場合があります。 ArrayList<?>で指定している部分をList<?>にしたり、StringをCharSequenceにできないか検討して下さい。

なぜダメか

別にそのままでもダメではないのですが、必要最小限の実装を持つスーパークラスやインターフェイスを指定することで将来の拡張に耐えられる場合があります。特に文字列を引数で受け取り、その引数に対して編集などを行わない場合はCharSequenceにしておくことでTextView#getText()などを直接扱えるようになります。

解決法

より上位のクラスやインターフェイスで対応できないか検討して下さい。自作のクラスを引数とする場合、インターフェイス化できないか検討してください。 (冗長になってしまう場合もあるので、Utilsクラスのみを対象とする、という形でも良いと思います)

仕様面

厳密にはソースコードレビューとは異なりますが、気にかけておくべき事柄。

画面構成がiOS風

指摘してもどうしようもない場合が多いですが、一応…。

なぜダメか

AndroidにはAndroidに適したUIがあるので、なるべく沿うようにしましょう。 http://developer.android.com/design/patterns/pure-android.html

解決法

代替案を提案してみる他ないです。

下タブ
- NavigationDrawer
- ActionBarタブ
独自ヘッダー
- ActionBar
画面遷移要素の「>」マーク
- 削除

特定のアプリへの依存

TwitterやFacebookなどのSNSアプリに対してパッケージを指定した明示的なインテントを発行したり、インストールされていない場合にPlayストアに遷移するような構成は好ましくありません。

なぜダメか

これらは多くの場合、仕様を決める際の考慮漏れです。本当に必要なのはTwitter,Facebookへの投稿ではなく、Twitter/Facebookを含むSNSなどへの拡散ではないでしょうか。そうであった場合、MixiやG+など他のSNSへ拡散される機会を損失しています。また、明示的にTwitterやFacebookに投稿する場合であっても、公式アプリ以外のクライアントアプリの存在を考慮する必要があります。特にTwitterは非公式のクライアントが多く、そのユーザも少なくありません。公式アプリの有無で挙動を変えることは、それらのアプリユーザから見れば不自然な挙動となります。さらに、非公式アプリを無視できるとしても、公式アプリが挙動を変更した場合に正しく動作しなくなる可能性があります。 このような仕様は避けるべきです。 http://developer.android.com/design/patterns/pure-android.html

解決法

単純にSNSへ拡散したい場合、暗黙的Intentを発行すれば良くなります。ユーザの利便性を考慮し、ShareActionProviderを利用することもできます。 http://developer.android.com/training/sharing/shareaction.html どうしても明示的にTwitter/Facebookに投稿させたい場合、アプリがSDKを使用して直接投稿すべきです。

こやまカニ大好き

Androidソースコードレビューで指摘する事が多い項目まとめ

Fragment関連

FragmentとActivityの密結合

なぜダメか

解決法

Fragmentからの不自然な画面遷移

なぜダメか

解決法

DB関連

Columnsの定義方法が間違っている

なぜダメか

解決法

WebView関連

WebViewのライフサイクルを考慮していない

なぜダメか

解決法

その他

ソースコードによるレイアウトの共通化

なぜダメか

解決法

引数における型指定

なぜダメか

解決法

仕様面

画面構成がiOS風

なぜダメか

解決法

特定のアプリへの依存

なぜダメか

解決法