LoginSignup
11
11

More than 5 years have passed since last update.

@opengl-8080inTIS株式会社

FizzBuzzをリファクタリングする

Last updated at Posted at 2014-02-01

特にゴールは考えず、目についた所から変更していってみる。

目次

まずは思いのまま書く

package fizzbuzz;

public class Main {
    public static void main(String[] args) {
        for (int i=1; i<=100; i++) {
            if (i % 15 == 0) {
                System.out.println("FizzBuzz");
            } else if (i % 3 == 0) {
                System.out.println("Fizz");
            } else if (i % 5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(i);
            }
        }
    }
}

もう必要十分。
リファクタリングなんて必要ない気がするけど、あえてリファクタリングしてみる。

どうなるかは僕にもわからない。

テストを書く

リファクタリングを保証するために、テストを書く。

package fizzbuzz;

import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.Matchers.*;

import java.io.File;
import java.io.IOException;
import java.io.PrintStream;

import org.apache.commons.io.FileUtils;
import org.junit.Test;

public class MainTest {

    @Test
    public void test() throws IOException {
        // setup
        File actualFile = new File("actual_fizzbuzz.txt");
        actualFile.delete();

        System.setOut(new PrintStream(actualFile));

        // exercise
        Main.main(null);

        // verify
        String actual = FileUtils.readFileToString(actualFile);
        String expected = FileUtils.readFileToString(new File("expected_fizzbuzz.txt"));

        assertThat(actual, is(expected));
    }
}

現状の出力結果をテキストファイルに書き出しておいて、その内容と比較することでプログラムの動きが変わってないことを確認している。

定数にする

package fizzbuzz;

public class Main {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            if (i % (FIZZ_NUMBER * BUZZ_NUMBER) == 0) {
                System.out.println(FIZZ_TEXT + BUZZ_TEXT);
            } else if (i % FIZZ_NUMBER == 0) {
                System.out.println(FIZZ_TEXT);
            } else if (i % BUZZ_NUMBER == 0) {
                System.out.println(BUZZ_TEXT);
            } else {
                System.out.println(i);
            }
        }
    }
}

見づらくなった？

if 条件をメソッド化

package fizzbuzz;

public class Main {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            if (isFizzBuzz(i)) {
                System.out.println(FIZZ_TEXT + BUZZ_TEXT);
            } else if (isFizz(i)) {
                System.out.println(FIZZ_TEXT);
            } else if (isBuzz(i)) {
                System.out.println(BUZZ_TEXT);
            } else {
                System.out.println(i);
            }
        }
    }

    private static boolean isFizzBuzz(int i) {
        return isFizz(i) && isBuzz(i);
    }

    private static boolean isBuzz(int i) {
        return i % BUZZ_NUMBER == 0;
    }

    private static boolean isFizz(int i) {
        return i % FIZZ_NUMBER == 0;
    }
}

if 条件のメソッド化はコードが読みやすくなるので好きです。

標準出力記述の重複をなくす

package fizzbuzz;

public class Main {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            String text;

            if (isFizzBuzz(i)) {
                text = FIZZ_TEXT + BUZZ_TEXT;
            } else if (isFizz(i)) {
                text = FIZZ_TEXT;
            } else if (isBuzz(i)) {
                text = BUZZ_TEXT;
            } else {
                text = String.valueOf(i);
            }

            System.out.println(text);
        }
    }

    private static boolean isFizzBuzz(int i) {
        return isFizz(i) && isBuzz(i);
    }

    private static boolean isBuzz(int i) {
        return i % BUZZ_NUMBER == 0;
    }

    private static boolean isFizz(int i) {
        return i % FIZZ_NUMBER == 0;
    }
}

if 文の意図が若干はっきりした？

if 分岐の塊を別メソッド化

package fizzbuzz;

public class Main {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            System.out.println(toText(i));
        }
    }

    private static String toText(int i) {
        String text;

        if (isFizzBuzz(i)) {
            text = FIZZ_TEXT + BUZZ_TEXT;
        } else if (isFizz(i)) {
            text = FIZZ_TEXT;
        } else if (isBuzz(i)) {
            text = BUZZ_TEXT;
        } else {
            text = String.valueOf(i);
        }

        return text;
    }

    private static boolean isFizzBuzz(int i) {
        return isFizz(i) && isBuzz(i);
    }

    private static boolean isBuzz(int i) {
        return i % BUZZ_NUMBER == 0;
    }

    private static boolean isFizz(int i) {
        return i % FIZZ_NUMBER == 0;
    }
}

main の処理は簡潔になった。

toText() って FizzBuzz のドメインロジックだよね

package fizzbuzz;

public class Main {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            FizzBuzz fizzBuzz = new FizzBuzz(i);
            System.out.println(fizzBuzz);
        }
    }

    private static class FizzBuzz {

        private final int number;

        private FizzBuzz(int number) {
            this.number = number;
        }

        @Override
        public String toString() {
            String text;

            if (isFizzBuzz(this.number)) {
                text = FIZZ_TEXT + BUZZ_TEXT;
            } else if (isFizz(this.number)) {
                text = FIZZ_TEXT;
            } else if (isBuzz(this.number)) {
                text = BUZZ_TEXT;
            } else {
                text = String.valueOf(this.number);
            }

            return text;
        }
    }

    private static boolean isFizzBuzz(int i) {
        return isFizz(i) && isBuzz(i);
    }

    private static boolean isBuzz(int i) {
        return i % BUZZ_NUMBER == 0;
    }

    private static boolean isFizz(int i) {
        return i % FIZZ_NUMBER == 0;
    }
}

FizzBuzz クラスに格納。

ほかにも FizzBuzz クラスにあるべきものがちらほらと

package fizzbuzz;

public class Main {

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            FizzBuzz fizzBuzz = new FizzBuzz(i);
            System.out.println(fizzBuzz);
        }
    }

    private static class FizzBuzz {

        private static final String FIZZ_TEXT = "Fizz";
        private static final String BUZZ_TEXT = "Buzz";
        private static final int FIZZ_NUMBER = 3;
        private static final int BUZZ_NUMBER = 5;

        private final int number;

        private FizzBuzz(int number) {
            this.number = number;
        }

        @Override
        public String toString() {
            String text;

            if (this.isFizzBuzz()) {
                text = FIZZ_TEXT + BUZZ_TEXT;
            } else if (this.isFizz()) {
                text = FIZZ_TEXT;
            } else if (this.isBuzz()) {
                text = BUZZ_TEXT;
            } else {
                text = String.valueOf(this.number);
            }

            return text;
        }

        private boolean isFizzBuzz() {
            return this.isFizz() && this.isBuzz();
        }

        private boolean isBuzz() {
            return this.number % BUZZ_NUMBER == 0;
        }

        private boolean isFizz() {
            return this.number % FIZZ_NUMBER == 0;
        }
    }
}

is**() 系のメソッドは、 this 修飾子を付けたときに文法的に不自然じゃない場所にあると、とても安心できます。

※状態を持たないクラスに is**() というインスタンスメソッドがあるのが一番違和感がある。

インナークラスはやめよう

Main.java
package fizzbuzz;

public class Main {

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            FizzBuzz fizzBuzz = new FizzBuzz(i);
            System.out.println(fizzBuzz);
        }
    }
}
FizzBuzz.java
package fizzbuzz;

public class FizzBuzz {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private final int number;

    public FizzBuzz(int number) {
        this.number = number;
    }

    @Override
    public String toString() {
        String text;

        if (this.isFizzBuzz()) {
            text = FIZZ_TEXT + BUZZ_TEXT;
        } else if (this.isFizz()) {
            text = FIZZ_TEXT;
        } else if (this.isBuzz()) {
            text = BUZZ_TEXT;
        } else {
            text = String.valueOf(this.number);
        }

        return text;
    }

    private boolean isFizzBuzz() {
        return this.isFizz() && this.isBuzz();
    }

    private boolean isBuzz() {
        return this.number % BUZZ_NUMBER == 0;
    }

    private boolean isFizz() {
        return this.number % FIZZ_NUMBER == 0;
    }
}

役割が明確に別れた感じがする。

この toString() で一時変数はいらないか

FizzBuzz.java
package fizzbuzz;

public class FizzBuzz {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private final int number;

    public FizzBuzz(int number) {
        this.number = number;
    }

    @Override
    public String toString() {
        if (this.isFizzBuzz()) {
            return FIZZ_TEXT + BUZZ_TEXT;
        } else if (this.isFizz()) {
            return FIZZ_TEXT;
        } else if (this.isBuzz()) {
            return BUZZ_TEXT;
        } else {
            return String.valueOf(this.number);
        }
    }

    private boolean isFizzBuzz() {
        return this.isFizz() && this.isBuzz();
    }

    private boolean isBuzz() {
        return this.number % BUZZ_NUMBER == 0;
    }

    private boolean isFizz() {
        return this.number % FIZZ_NUMBER == 0;
    }
}

もう気になるところはないかな...？

FizzBuzz クラスの単体テストを書いてみる

package fizzbuzz;

import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;

import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;

@RunWith(Theories.class)
public class FizzBuzzTest {

    @DataPoints
    public static Fixture[] fixtures = {
            new Fixture("2を指定した場合、2を返す") {{
                number = 2;
                expected = "2";
            }},
            new Fixture("3を指定した場合、Fizzを返す") {{
                number = 3;
                expected = "Fizz";
            }},
            new Fixture("4を指定した場合、4を返す") {{
                number = 4;
                expected = "4";
            }},
            new Fixture("5を指定した場合、Buzzを返す") {{
                number = 5;
                expected = "Buzz";
            }},
            new Fixture("6を指定した場合、Fizzを返す") {{
                number = 6;
                expected = "Fizz";
            }},
            new Fixture("14を指定した場合、14を返す") {{
                number = 14;
                expected = "14";
            }},
            new Fixture("15を指定した場合、FizzBuzzを返す") {{
                number = 15;
                expected = "FizzBuzz";
            }},
            new Fixture("16を指定した場合、16を返す") {{
                number = 16;
                expected = "16";
            }},
    };

    @Theory
    public void toStringのパターン網羅テスト(Fixture fixture) {
        // setup
        FizzBuzz fizzBuzz = new FizzBuzz(fixture.number);

        // exercise
        String actual = fizzBuzz.toString();

        // verify
        assertThat(fixture.testCase, actual, is(fixture.expected));
    }

    public static class Fixture {
        public final String testCase;
        public int number;
        public String expected;

        public Fixture(String testCase) {
            this.testCase = testCase;
        }
    }
}

わりと書きやすい？

おわりに

変更前

package fizzbuzz;

public class Main {
    public static void main(String[] args) {
        for (int i=1; i<=100; i++) {
            if (i % 15 == 0) {
                System.out.println("FizzBuzz");
            } else if (i % 3 == 0) {
                System.out.println("Fizz");
            } else if (i % 5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(i);
            }
        }
    }
}

変更後

Main.java
package fizzbuzz;

public class Main {

    private static final int LOOP_COUNT = 100;

    public static void main(String[] args) {
        for (int i=1; i<=LOOP_COUNT; i++) {
            FizzBuzz fizzBuzz = new FizzBuzz(i);
            System.out.println(fizzBuzz);
        }
    }
}
FizzBuzz.java
package fizzbuzz;

public class FizzBuzz {

    private static final String FIZZ_TEXT = "Fizz";
    private static final String BUZZ_TEXT = "Buzz";
    private static final int FIZZ_NUMBER = 3;
    private static final int BUZZ_NUMBER = 5;

    private final int number;

    public FizzBuzz(int number) {
        this.number = number;
    }

    @Override
    public String toString() {
        if (this.isFizzBuzz()) {
            return FIZZ_TEXT + BUZZ_TEXT;
        } else if (this.isFizz()) {
            return FIZZ_TEXT;
        } else if (this.isBuzz()) {
            return BUZZ_TEXT;
        } else {
            return String.valueOf(this.number);
        }
    }

    private boolean isFizzBuzz() {
        return this.isFizz() && this.isBuzz();
    }

    private boolean isBuzz() {
        return this.number % BUZZ_NUMBER == 0;
    }

    private boolean isFizz() {
        return this.number % FIZZ_NUMBER == 0;
    }
}

思ったよりも落ち着くところに落ち着いた感じです。

リファクタリング後はコード量が増えたけど、僕はリファクタリング後の方が好きです。

FizzBuzz のルール（ロジック）が FizzBuzz クラスに隠蔽されており、ループや出力方法とかから隔離されているからです。

FizzBuzz 程度ならリファクタリングの必要はないかもしれないけれど、もっと複雑なロジックになったら...

11
11
2

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
11
11