コーディングについて

Question

独学でJavaを勉強しています。
まだ基本知識があるくらいです。

抜粋してありますが、

numが１ならmethodA()、methodB()を２回おこなう。
それ以外なら１回。ただしmethodB()でNGならそこで処理終了。
methodA()にはnumが１のときは１、それ以外のときは９を渡す。

といったとき、以下のようなコーディングを行ったのですが、
methodAとmethodBの呼び方がわかりにくいコードになってしまっている気がします。

①以下をみやすいコードに修正するにはどのように記述すべきでしょうか。
②みやすいコーディング術を身につけるのに
・おすすめの本
・勉強方法
・考え方
などありましたら教えていただけると助かります。

public class sample1 {

	public static void main(String[] args) {
		int num = 0;
		//numに何か値を入力
		boolean b = method(num);

		// bの値判定処理・・・
	}

	private static boolean method(int num){
		String flag = "";
		if(num == 1){
			methodA(1);
			flag = methodB();

			if(flag == "OK")
			{
				methodA(1);
				flag = methodB();
				if(flag == "OK"){
					return true;
				}else{
					return false;
				}
			}else{
				return false;
			}
		}else{
			methodA(9);
			flag = methodB();
			if(flag == "OK"){
				return true;
			}else{
				return false;
			}
		}
	}

	private static void methodA(int n){
		//何か処理
	}

	private static String methodB(){
		String s = "OK";
		//条件判定でOKかNGを返す
		return s;
	}
}
コード

Answer 1

methodA, methodB の呼び出し部分を actionSub() としてくくりだし、
actionSub() の呼び出しを条件分岐させるというようにしてみました。

    ...
    private static boolean method(int num) {
        if (num == 1) {
            return actionSub(1) ? actionSub(1) : false;
        }
        return actionSub(9);
    }
    private static boolean actionSub(int n) {
        methodA(n);
        return "OK".equals(methodB());
    }
    ...

短くするため３項演算をつかっていますが、その部分は素直に次のように書いてもよいかもしれません。

    if (actionSub(1)) {
        return actionSub(1);
    }
    return false;

Comments

[katoti09]

ご回答ありがとうございます。
なるほど、くくりだすだけでも凄くわかりやすいコードになっています…！
また別の考え方を教えて頂き、とても身になります。
本当にありがとうございました…！

Answer 2

読みやすい、読みにくい以前に、何のためのコードなんでしょうか？

numが１ならmethodA()、methodB()を２回おこなう。

という辺りがどういう処理をしているのかが伝わらないのです。

例えば、ATMのシステムを考えてみると、

1. 預入の時は0、引き出しの時は1を入れる
2. 引き出しの時は1なので、暗証番号の入力を行う
3. 暗証番号の入力がOKだったら引き出し額を入れる

という感じで処理の流れがありますよね。
これをコードに落としてみるとこんな感じになるのではないでしょうか？

public class ATM {

  public static int MODE_OF_DEPOSIT = 0;
  public static int MODE_OF_WITHDRAWAL = 1; 

  public void static main(String... args){
    int mode = getATMMode(); // 具体的な処理はgetATMMode()に実装する
    if(mode == MODE_OF_WITHDRAWAL) {
      int identificationNumber = getIdentificationNumber();
      if(isCorrect(identificationNumber)) {
          int money = getMoney();
          ...
      }
    }
  }
}

こんな感じです。

• メソッド名は具体的に「何を意味する処理なのか」を明示した名前にしましょう
• クラス名は「そのオブジェクトが何者なのか」を明示した名前にしましょう

そういう話がわかりやすく書かれていてわりかし薄い本に「リーダブルコード」という本があります。目を通してみるといろいろ見えてきますよ。

Comments

[katoti09]

本物はもう少し複雑で、抜粋してしまったため、なんの処理かわかりにくかったこと申し訳ありません。
ですが、ご回答くださりありがとうございます。
ご指摘内容は、とりいれ
おすすめいただいた本も読んでみようとおもいます！

Answer 3

①以下をみやすいコードに修正するにはどのように記述すべきでしょうか。

swordoneさんの解答素晴らしいです。

② みやすいコーディング術を身につけるのに
・おすすめの本
・勉強方法
・考え方

クラス単位、メソッド単位での考え方はあまり実践的ではないです。
Java言語で学ぶリファクタリング入門が入門としては最適かと思います。これを読んだ後、自分のコードを見るとどこがまずいかが理解できると思います。まずはそこまでが目標だと思います。

Comments

[katoti09]

ご回答の通り、おすすめの本を読んでみて、自身のコードを再度見直してみようと思います。
このような質問にお時間さいていただき誠にありがとうございました！

Answer 4

numが1とそれ以外の場合で違うのは「methodAの引数」と「各メソッドを行う回数」なので，
共通する部分「methodAとmethodBの実行」はまとめます．
基本的には「同じことをする部分はまとめる」ということを考えるべきかと．
本題から外れますが，methodBはStringで返す必要がありますか？
このコードを見る限り直接booleanで返したほうがわかりやすいかと思うのですが．

だいぶ手を加えましたが，こんな感じでいかがでしょう．

public class sample1 {

    public static void main(String[] args) {
        int num = 0;
        //numに何か値を入力
        boolean b = method(num);

        // bの値判定処理・・・
    }

    private static boolean method(int num){
        int n = num == 1 ? 1 : 9;  //numによってmethodAに渡す引数を分岐
        int count = num == 1 ? 2 : 1;  //次のループの回数を設定
        for(int i = 0; i < count; i++){
            methodA(n);
            if(!methodB()) return false;  //methodB()がfalseを返せばその時点でfalseを返して終了
        }
        return true;
    }

    private static void methodA(int n){
        //何か処理
    }

    private static boolean methodB(){
        boolean b = true;
        //条件判定でtrueかfalseを返す
        return b;
    }
}

---

あるいは，methodAとmethodBをまとめたメソッドsubMethodを作れれば，

    private static boolean method(int num){
        return subMethod(num) && (num != 1 || subMethod(num));
    }

    private static boolean subMethod(int num){
        methodA(num == 1 ? 1 : 9);
        return methodB();
    }

これでも行けると思います(ただしこの書き方は繰り返しがせいぜい2回程度の時)．

Comments

[katoti09]

ご回答、誠にありがとうございます。

確かにおっしゃる通りStringで返す必要はありませんでした…

とても分かり易いコードに直していただき、勉強が足りないなと身にしみました。
共通部分はまとめるということをコーディングする際にとりいれていきます。
本当にありがとうございました。

[swordone]

このコードで解決したならもう関係ないかもしれませんが，
Stringを"=="で比較するのは必ずしも望む結果が得られないので注意してください．
文字列の比較をしたければequalsメソッドを使ってください．

[katoti09]

習ったのにそのことを失念していました…追記ありがとうございました！