Junit 4, Java

Question

Witam, od nie dawna zacząłem uczyć się pisania testów junit 4, oraz  podejścia TDD. Napisałem aplikację do prostej łamigłówki FizzBuzz wraz z testami. Pytania jakie mi się nasuwają to:

Czy funkcjonalność jest dobrze przetestowana?

Czy jest to wystarczająca ilość testów?

Co można zmienić?

Co jest źle napisane?

Jakich metod brakuje?

Prosiłbym o pomoc kogoś kto już ma doświadczenie z testami i byłby w stanie mi odrobinę pomóc i dać parę wskazówek. Poniżej podrzucam kod z klasy FizzBuzz z metodą. Oraz klasę FizzBuzzTest z testami. Pozdrawiam cieplutko.

public class FizzBuzz {
    public static String print(int i) {

        if (i % 3 ==0 && i % 5 ==0) return"FizzBuzz";
        else if(i % 3 == 0)         return "Fizz";
        else if(i % 5 == 0)         return "Buzz";
        else                        return String.valueOf(i);
    }
}

Testy:

import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

public class FizzBuzzTest {

    @Before
    public void FizzBuzzInicialization(){
        FizzBuzz fizzBuzz = new FizzBuzz();
    }

    @Test
    public void FizzBuzzCheckFirst(){
        Assert.assertEquals("1", FizzBuzz.print(1));
        Assert.assertEquals("2", FizzBuzz.print(2));
    }

    @Test
    public void FizzBuzzChceckFizz(){
        Assert.assertEquals("Fizz", FizzBuzz.print(3));
    }

    @Test
    public void FizzBuzzChceckBuzz(){
        Assert.assertEquals("Buzz", FizzBuzz.print(5));
    }

    @Test
    public void FizzBuzzChceckIfFizzBuzz(){
        Assert.assertEquals("FizzBuzz", FizzBuzz.print(15));
    }

    @Test
    public void FizzBuzzFromOneToHundret(){

        for (int i = 1; i < 101; i++) {
            System.out.println(FizzBuzz.print(i));
        }
    }
}

 

Answer 1

Jeśli chodzi o FizzBuzz to o ile się nie mylę to w przypadku gdy wartość jest inna niż podzielna przez 3, 5 to ma być jakby pomijana. Czyli w Twoim przypadku jeśli żaden if nie będzie spełniony ma zwrócić pusty String:

 private static final String EMPTY_STRING = "";
 public static String print(int i){
        
        if (i % 3 == 0 && i % 5 == 0) {
            return "FizzBuzz";
        } else if (i % 3 == 0) {
            return "Fizz";
        } else if (i % 5 == 0) {
            return "Buzz";
        }
        return EMPTY_STRING;
}

Ostatni else jest zbędny. (Być może jeszcze lepszym wyjściem będzie użycie Optionala, ale nie będę mieszać). Formatowanie mimo, że to Twoje nie utrudnia czytania, to lepiej trzymać się standardowych konwencji czyli cialo if'a w nowej linii (klamry są kwestią sporną, jednak ich obecność sprawia, że w razie potrzeby można nieco wygodniej dodawać kod do danego if'a).

Co do testów:

To:

    @Before
    public void FizzBuzzInicialization(){
        FizzBuzz fizzBuzz = new FizzBuzz();
    }

jest w tym przypadku zbędne. Pierwsza rzecz, zmienna fizzBuzz to zmienna lokalna metody FizzBuzzInicialization czyli w żaden sposób nie jesteś w stanie skorzystać z tego obiektu poza tą metodą. Inna rzecz to nawet jeśli byłbyś w stanie to klasa FizzBuzz ma metodę statyczną, co oznacza, że żeby z niej skorzystać nie trzeba tworzyć obiektu klasy FizzBuzz.

Same testy technicznie są ok jedynie co może poniekąd przeszkadzać to literały i tzw. magic numbers. Przykładowo ten test:

    @Test
    public void FizzBuzzChceckBuzz(){
        Assert.assertEquals("Buzz", FizzBuzz.print(5));
    }

Co powiesz na taki zapis?

    @Test
    public void FizzBuzzChceckBuzz(){
        final String expectedMessage = "Buzz";
        final int dividedOnlyBy5 = 5;
        Assert.assertEquals(expectedMessage, FizzBuzz.print(dividedOnlyBy5));
    }

Elegancko widać co się skąd bierze. Nazwa testu być może też nie mówi za wiele. Często używa się konwencji shouldDoSomething (i stosuje się też pseudo bloki given when then):

    @Test
    public void shouldReturnBuzz(){
        // given
        final String expectedMessage = "Buzz";
        final int dividedOnlyBy5 = 5;

        // when 
        String actualMessage = FizzBuzz.print(dividedOnlyBy5);

        // then
        Assert.assertEquals(expectedMessage, actualMessage);
    }

A i nazwy metod/testów pisz z małej.

Comments

Dzięki za rady, tak jak mówisz zapis:

@Test
public void FizzBuzzChceckBuzz(){
    final String expectedMessage = "Buzz";
    final int dividedOnlyBy5 = 5;
    Assert.assertEquals(expectedMessage, FizzBuzz.print(dividedOnlyBy5));
}

jest dużo czytelniejszy i dokładniej opisuje to zagadnienie.

Nie znałem konwencji shouldDoSomething ale intuicyjnie czuć, że ma to smak. Dziękuje w takim razie jeszcze raz.

PS: To że wypisuje liczby FizzBuzz nie ma większego znaczenia, chodziło głównie o testy.

Pozdrawiam.