javaでのネスト解消

Question

こちらのjavaソースですがネストが深いと指摘を受けています。

どう解消すればよいか余裕のある方回答お願いいたします。

 /**
     *  位置情報取得
     */
    public void getDeviceLocation() {
        try {
            if (locationPermissionGranted) {
                Task<Location> locationResult = fusedLocationProviderClient.getLastLocation();
                locationResult.addOnCompleteListener((Activity) mApplicationManager.getContext(), new OnCompleteListener<Location>() {
                    @Override
                    public void onComplete(@NonNull Task<Location> task) {
                        // 測位完了
                        if (task.isSuccessful()) {
                            // 測位通知
                            location = task.getResult();
                            //　正常に帰ってきたが、Nullの場合もあるのではじく
                            if(location != null){
                                if (isJapan(location.getLatitude(), location.getLongitude())) {
                                    // [国内]
                                    mApplicationManager.getMap().clear();
                                    mApplicationManager.getCommunication().executeByConcurrentExecutor();
                                    // 現在位置に設定
                                    LatLng currentLocation = new LatLng(location.getLatitude(), location.getLongitude());
                                    mApplicationManager.getMap().moveCamera(CameraUpdateFactory.newLatLngZoom(currentLocation, 16.0f));
                                } else {
                                    // [海外]
                                    // 日本地図を表示
                                    mApplicationManager.getMap().moveCamera(CameraUpdateFactory.newLatLngZoom(defaultLocation, DEFAULT_ZOOM));
                                }
                            }else {
                                // [位置情報取得失敗]
                                // エラーダイアログ表示
                                mApplicationManager.getLoadDialog().dismiss();
                                new AlertDialog.Builder(mApplicationManager.getContext())
                                        .setTitle(R.string.error)
                                        .setMessage(R.string.null_location)
                                        .setNegativeButton(R.string.close, null)
                                        .show();
                            }
                        }
                    }
                });
            }
        } catch (SecurityException e) {
            Log.e("Exception: %s", e.getMessage(), e);
        }
    }

Comments

[dodox86]

質問内容に直接関係ないのですけど、
正：Android Studio
誤：Android Stdio
です。先のご質問を含め、少し気になったので。で、Android Studioは単にAndroidのアプリを作る為のJava/Kotlinの開発環境で、プログラムのコードと直接関係ある訳ではありません。

あと、
> どう解消すればよいか余裕のある方回答お願いいたします。

もう少しご自身で考えて質問を投稿された方が良いと思います。丸投げの印象を受けます。
ヒントだけ書くと、例えば
if (Aは真) {
　　if (Bは真) {
　　　　if (Cは真) {
　　　　　　;
　　　　}
　　}
}

のような処理である場合、
if (Aは偽) {
　　return;
}
if (Bは偽) {
　　return;
}
if (Cは偽) {
　　return;
}

// AもBもCも真のとき

のように書くことでネストを結果的に減らし、コードを読みやすくすることができます。
(returnは場合によりけりです）

[2r4h8k3m]

ご指摘ありがとうございます。プログラムの常識的なものが分かっていないのとほか部分修正していまして丸投げな文章になっていました。気づくことあればまたご指摘ください

Answer 1

ネストが深いことで何が一番困るかというと、思考が枝分かれしてしまって、
余計なことに頭を使わないといけなくなることだと私は思います。

(組み合わせが増えると、パターン検討が必要になるため複雑化する。
取りうるパターンが1つであれば、処理は単純明快になる。)

従って、以下の方針でネストの深さを浅くします。

---

• ガードとなる処理は先に否定してreturnで抜けられないか考える。

(先の方がヒントをくれていますね。)

今回のサンプルですと、2か所ありました。

if (locationPermissionGranted) {
if (task.isSuccessful()) {

これらは、ifの先の}が、関数の最後までいっているので、途中で抜けても処理は変わらなさそうです。ですので、

if (!locationPermissionGranted) return;
if (!task.isSuccessful()) return;

とすることで、インデントを2つ浅くすることができました。

---

• if/elseやswitch/caseが登場する箇所は別関数化することを検討する。

こちらも2か所ありました。

if(location != null){
}else {

if (isJapan(location.getLatitude(), location.getLongitude())) {
} else {

これらは、意味だったり、持っている変数のスコープだったりをちょっとだけ考えて一旦関数化してみます。

NOTE:
適切じゃない関数化については、あまり深く考え過ぎないのがポイントです。
そのうち関数同士が意味合いとしてぶつかるので、全体を見通してさらにリファクタリングする必要が
あります。
短期的なリファクタリングによって思考を整理するのと、中長期的なリファクタリングによって
処理体系を見直すのと、使い分けが必要だと私は思っていますが、往々にして後者はそれなりに
苦労します。

---

• 最後にコード整形をかけます。

Android StudioやIntelliJ IDEAなどのJetBrains社のIDEは、
Ctrl+Alt+Lでコード整形がかかります。(Windowsの場合。)
不要なインポート文を削るのはCtrl+Alt+Oなので、手癖で左Ctrl+Alt押しっぱなしで
右手でL,Oとタイプするような感じです。

これらを行ったコードを置いておきますが、オリジナルと比べて
横に目をやることが減った & 縦も見通せるようになり結果として処理がスッキリした印象になったと思いませんか?

書き方のヒントとして、横にある程度伸びたらプチリファクタリングを行い、
コードだけではなく、思考をスッキリとさせるということを検討してみて下さい。

    public void getDeviceLocation() {
        try {
            if (!locationPermissionGranted) return;
            Task<Location> locationResult = fusedLocationProviderClient.getLastLocation();
            locationResult.addOnCompleteListener((Activity) mApplicationManager.getContext(), new OnCompleteListener<Location>() {
                @Override
                public void onComplete(@NonNull Task<Location> task) {
                    // 測位完了
                    if (!task.isSuccessful()) return;
                    // 測位通知
                    location = task.getResult();
                    //　正常に帰ってきたが、Nullの場合もあるのではじく
                    if (location != null) {
                        moveCamera();
                    } else {
                        // [位置情報取得失敗]
                        // エラーダイアログ表示
                        mApplicationManager.getLoadDialog().dismiss();
                        new AlertDialog.Builder(mApplicationManager.getContext())
                                .setTitle(R.string.error)
                                .setMessage(R.string.null_location)
                                .setNegativeButton(R.string.close, null)
                                .show();
                    }
                }
            });
        } catch (SecurityException e) {
            Log.e("Exception: %s", e.getMessage(), e);
        }
    }

    private void moveCamera() {
        try {
            if (isJapan(location.getLatitude(), location.getLongitude())) {
                // [国内]
                mApplicationManager.getMap().clear();
                mApplicationManager.getCommunication().executeByConcurrentExecutor();
                // 現在位置に設定
                LatLng currentLocation = new LatLng(location.getLatitude(), location.getLongitude());
                mApplicationManager.getMap().moveCamera(CameraUpdateFactory.newLatLngZoom(currentLocation, 16.0f));
            } else {
                // [海外]
                // 日本地図を表示
                mApplicationManager.getMap().moveCamera(CameraUpdateFactory.newLatLngZoom(defaultLocation, DEFAULT_ZOOM));
            }
        } catch (SecurityException e) {
            Log.e("Exception: %s", e.getMessage(), e);
        }
    }

Comments

[2r4h8k3m]

丁寧にご返答ありがとうございます、コード整形もやり方が分かっていなかったので教えて貰えてとても助かりました。感謝しています。
1つ目の解消の仕方は理解できたのですが、2つ目の例はif elseの解消の仕方はswitchが1番最初に思いついたのですか、これで変えても大した差はないから良くないということでしょうか。

[rtazaki]

>2つ目の例はif elseの解消の仕方はswitchが1番最初に思いついたのですか、これで変えても大した差はないから良くないということでしょうか。
誰からネストが深いと指摘を受けたのかは私には分からないし興味はないです。なので良し悪しは
判断できません。レビュー者の好みだと思います。(すっとぼけ)
余裕がある(=責任がない)モノからの意見ですので、ご参考までに。(という予防線を張りつつ。)

大した差が出ていないから良くないのではなく、差が出るように工夫することが必要だと思います。
---
ネストを浅くする目的を私なりにはこのように考えています。
可読性を向上させてバグを減らす:
* 第一に、スコープ({}の範囲)を短くする。
* 第二に、処理パターンの単純化をする。
* 第三に、どうせやるならネストを浅くしつつ、 1行あたりの文字数を減らす。

これに当てはめて考えてみると、if elseをswitchに変えても、defaultを考慮する必要がありますよね。
そしてネストは一段深くなる。
switch(条件):
case 0:
break;
default:
break;

だからswitch caseを使うなと言うつもりは全くなく、その部分だけを別関数にしたほうがネストは浅くなるので、一度その処理を外に出して思考をスッキリさせませんか?という提案をしています。

---
ネストが深いことは、理解の妨げにもなるしバグの温床にもなりますが、一方で
* それでも動いているコードである。
* テストをPASSしている処理である。
* 結合試験をクリアしている。
という理由で、処理を変えることにネガティブなケースがあり、読みやすくなったので良しとならないことがあります。
コードレビューをパスしない可能性があるので、もしそうなったら別の方法を検討する必要があります。