Proszę o Code review dla String calculator Kata TDD.

0

EDIT: Początkowo nieco przesadziłem i niektóre rzeczy nad wyrost chciałem, zrobić w związku z czym nie wypełniałem konkretnych wymagań.

Hej.
Uczę się właśnie testów jednostkowych i TDD. To jest w sumie moje pierwsze podejście i nie wiem czy testy jednostkowe do końca z konwencją zgodnie są napisane. W sumie zależy mi na tym, aby porządnie było zrobione bo dostałem zadanko takie i będzie sprawdzane, w związku z czym nie wiem czy nie przesadziłem z testami.

Tutaj jest opis zadania gdyby ktoś potrzebował : http://osherove.com/tdd-kata-1/
W skrócie, napisać Kalkulator stringowy napisany z podejściem TDD.

Gdyby ktoś wolał link do GitHuba to na priv wyślę, bo oficjalne konto.

Do tej pory zrobiłem:
-Metoda przyjmuje stringa z dowolną ilością liczb. Liczby tylko całkowite.
-Sprawdza czy wprowadzone są w odpowiednim formacie (1,2,3,4) tylko i wyłącznie taki dozwolony i zwraca odpowiednie wyjątki.
-Sprawdza czy null i czy pusty string

Odnośnie samej metody, to wiem, że dużo ifów i spróbuję ją refactorować, żeby bardziej elegancka była, póki co miały testy przechodzić po prostu. Aczkolwiek uwagi odnośnie metody też bardzo chętnie przyjmę.

Klasa:

 public class StringCalculator {

    public int add(String string) {
        if (string == null) throw new NullPointerException("String can't be null");
        if (string.length() == 0) {
            return 0;
        }
        if (!Pattern.matches("(\\d+){1}([,\\.]\\d+)*", string))
            throw new IllegalArgumentException("Input have to be numeric type, separated by comma(,)");
        if (Pattern.matches("\\d+", string)) {
            int singleNum = Integer.parseInt(string);
            return singleNum;
        }
        String[] arrayOfNumbers = string.split(",");
        int sum = 0;
        int parsedNumber;
        for (String number : arrayOfNumbers) {
            parsedNumber = Integer.parseInt(number);
            sum += parsedNumber;
        }
        return sum;
    }
}

Testy:

 public class StringCalculatorTest {

    StringCalculator stringCalculator;

    @Before
    public void setUp() throws Exception {
        stringCalculator = new StringCalculator();
    }

    @Test
    public void shouldReturn0WhenPassedStringIsEmpty() {
        assertEquals("String is empty", 0, stringCalculator.add(""));
    }

    @Test(expected = NullPointerException.class)
    public void shouldThrowNullPointerExceptionWhenPassedStringIsNull() {
        stringCalculator.add(null);
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowIllegalArgumentExceptionWhenPassedStringIsNotANumber() {
        stringCalculator.add("X");
        stringCalculator.add("1,s,3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowIllegalArgumentExceptionWhenPassedStringHasCommaAsAFirstCharacter() {
        stringCalculator.add(",1,2,3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowIllegalArgumentExceptionWhenPassedStringHasDuplicatedCommas() {
        stringCalculator.add("1,,3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowIllegalArgumentExceptionWhenPassedStringHasCommaAsALastCharacter() {
        stringCalculator.add("1,2,3,");
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowIllegalArgumentExceptionWhenPassedStringHasWhiteSpaces() {
        stringCalculator.add("1,2, 3");
    }


    @Test
    public void shouldReturnTheSameNumberWhenItWasOnlyNOneNumberPassed() {
        assertEquals(3,stringCalculator.add("3"));
    }

    @Test
    public void shouldReturnSumOfEveryAmountOfNumbersWhereAmountHaveToBeBiggerThanOne() {
        assertEquals(3+2,stringCalculator.add("3,2"));
        assertEquals(3+2+6+10,stringCalculator.add("3,2,6,10"));
        assertEquals(3+2+6+10+100,stringCalculator.add("3,2,6,10,100"));
    }
}
1
  1. shouldThrowIllegalArgumentExceptionWhenPassedStringIsNotANumber jest bez sensu bo masz dwa wywołania z których tylko jedno się wykona
  2. Przepisz tą metodę wyciagając walidacje osobno a te obliczenia można ładnie zrobić za pomocą Stream API ;)
  3. Ten przypadek kiedy masz jedną liczbę nie jest potrzebny, da się to napisać tak ze zadziała automatycznie
  4. Rzucanie NPE to zło. Jeśli już musisz coś rzucać to jakiś IllegalArgumentException. Ale zalecałbym tak generalnie Optional albo Either.
2
  1. Obczaj String::isEmpty,
  2. Wydaje mi się, że niepotrzebnie oddzielasz przypadek z jedną liczbą o przypadku z wieloma liczbami,
  3. Zadanie z blogu Osherova, więc proponuje stosować zaproponowaną przez niego konwencję nazewniczą: http://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html

Ogólnie brakuje u CIebie fazy refaktoryzacji. Gdy testy są zielone, staraj się myśleć co by tu mozna w kodzie poprawić pod względem polepszenia czytelności, zmniejszenia ilości warunków itp. Jesteś bezpieczny ponieważ masz testy,

0

Tak jak pisałem w Editcie trochę zboczyłem z kursu, ale powróciłem na tor. Poprawiłem ile mogłem, wydzieliłem co się dało. Bardzo was proszę o kolejny review.

Odnośnie tego kodu:
A) Wiem, że wyliczenia itd można zrobić za pomocą Stream Api, ale jeszcze nie umiem 8, póki co się skupiam, żeby zrobić do końca zadanie w Javie 7, a na końcu jak zdążę to refactor do 8 zrobię. Ale sugestie które elementy jeszcze strumieniami i lambdami zrobić bardzo mile widziane. Bo tak czy siak to zrobię.
B)Nie wiem czy nie przesadzam, z testowaniem tych wszystkich RuntimeExceptions, głównie to IAE.
C) Zastanwiam się, żeby metody getNumbers, separateNumbers, getDelimiter, isDelimiterValid nie wydzielić do nowej klasy.
D) W związku z powyższym i tym, że wydzieliłem z jednej metody więcej metod, to czy asercje nie powinny dotyczyć dokładnie danych metod "wycinków" czy nadal mam testować jedną "główną", która skupia wszystkie?
E) Odnośnie konwencji nazewniczej ta od osherova bardzo mi się nie podoba, ciężko mi się ją czyta. Do jednej z kilku konwencji dodałem nazwę testowanej metody, ale nie wime czy to dobre wyjście jeśli miałbym teraz, zmieniać asercje...
Klasa:

public class StringCalculator {

    private static Pattern customDelimiterValidator = Pattern.compile("(//\\D\n).*");

    public static int add(String string) {
        if (string.isEmpty()) {
            return 0;
        }
        String[] arrayOfNumbers = getNumbers(string);
        checkForNegativeNumbers(arrayOfNumbers);
        return sumArray(arrayOfNumbers);
    }

    private static void checkForNegativeNumbers(String[] arrayOfNumbers) {
        String negatives = "";
        for (String number: arrayOfNumbers ){
            if (number.contains("-"))
                negatives += "," + number;
        }
        if (!negatives.isEmpty()) throw new IllegalArgumentException("Negatives not allowed: " + negatives.substring(1)); // index:0 is comma
    }

    private static String[] getNumbers(String string) {
        String delimiter = getDelimiter(string);
        return separateNumbers(string, delimiter);
    }

    private static String[] separateNumbers(String string, String delimiter) {
        if (isDelimiterValid(string))
            string = string.substring(4);
        return string.split(delimiter);
    }

    private static String getDelimiter(String string) {
        String delimiters = ",|\n";
        if (isDelimiterValid(string))
            delimiters += "|" + string.substring(2, 3);
        return delimiters;
    }

    private static boolean isDelimiterValid(String string) {
        Matcher delimiterMatcher = customDelimiterValidator.matcher(string);
        return delimiterMatcher.matches();
    }

    private static int sumArray(String[] arrayOfNumbers) {
        int sum = 0;
        int parsedNumber;
        for (String number : arrayOfNumbers) {
            parsedNumber = Integer.parseInt(number);
            sum += parsedNumber;
        }
        return sum;
    }
}

Testy:

public class StringCalculatorTest {

    StringCalculator stringCalculator;

    @Before
    public void setUp() {
        stringCalculator = new StringCalculator();
    }

    @Test
    public void add_ShouldReturn0_WhenPassedStringIsEmpty() {
        assertEquals("String is empty", 0, stringCalculator.add(""));
    }

    @Test(expected = IllegalArgumentException.class)
    public void add_ShouldThrowIllegalArgumentException_WhenPassedStringIsNotANumber() {
        stringCalculator.add("1,s,3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void add_ShouldThrowIllegalArgumentException_WhenPassedStringHasCommaAsAFirstCharacter() {
        stringCalculator.add(",1,2,3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void add_ShouldThrowIllegalArgumentException_WhenPassedStringHasDuplicatedCommas() {
        stringCalculator.add("1,,3");
    }

    @Test
    public void add_ShouldReturnSum_WhenPassedStringHasCommaAsALastCharacter() {
        assertEquals(1 + 2 + 3, stringCalculator.add("1,2,3,"));
    }

    @Test(expected = IllegalArgumentException.class)
    public void add_ShouldThrowIllegalArgumentException_WhenPassedStringHasSpaceCharacter() {
        stringCalculator.add("1,2, 3");
    }

    @Test(expected = IllegalArgumentException.class)
    public void add_ShouldThrowIllegalArgumentException_WhenPassedStringHasTabulationCharacter() {
        stringCalculator.add("1,2   ,3");
    }

    @Test
    public void add_ShouldReturnTheSameNumber_WhenItWasOnlyNOneNumberPassed() {
        assertEquals(3, stringCalculator.add("3"));
    }

    @Test
    public void add_ShouldReturnSumOfEveryAmountOfNumbers_WhenSeparatorIsOnlyComma() {
        assertEquals(3 + 2, stringCalculator.add("3,2"));
        assertEquals(3 + 2 + 6 + 10, stringCalculator.add("3,2,6,10"));
        assertEquals(3 + 2 + 6 + 10 + 100, stringCalculator.add("3,2,6,10,100"));
    }

    @Test
    public void add_ShouldReturnSumOfEveryAmountOfNumbers_WhenSeparatorIsOnlyNewLine() {
        assertEquals(3 + 2, stringCalculator.add("3\n2"));
        assertEquals(3 + 2 + 6 + 10, stringCalculator.add("3\n2\n6\n10"));
        assertEquals(3 + 2 + 6 + 10 + 100, stringCalculator.add("3\n2\n6\n10\n100"));
    }

    @Test
    public void add_ShouldReturnSumOfEveryAmountOfNumbers_WhenSeparatorsAreNewLinesAndCommas() {
        assertEquals(3 + 2 + 6 + 10, stringCalculator.add("3\n2,6\n10"));
        assertEquals(3 + 2 + 6 + 10 + 100, stringCalculator.add("3\n2,6\n10,100"));
    }

    @Test
    public void add__ShouldReturnSum_WhenDelimiterIsSpecified() {
        assertEquals(3 + 2 + 4, stringCalculator.add("//;\n3;2;4"));
    }

    @Test(expected = NumberFormatException.class)
    public void add_ShouldThrowNumberFormatException_WhenDelimiterSpecificationIsSeparatedByEveryCharButNotNewLine() {
        stringCalculator.add("//;\t3;2;4");
    }

    @Test(expected = NumberFormatException.class)
    public void add_ShouldThrowNumberFormatException_WhenDelimiterSpecificationIsNotSeparatedByNewLine() {
        stringCalculator.add("//;3;2;4");
    }

    @Test
    public void add_ShouldThrowIllegalArgumentException_WhenNegativeNumbersArePassed() {
        try {
            assertEquals(0, stringCalculator.add("-2,3,-4"));
        } catch (IllegalArgumentException e) {
            assertEquals("Negatives not allowed: -2,-4", e.getMessage());
        }
    }
}

1

Wypowiem się na szybko na twoje pytanie odnośnie tego czy testować wyekstraktowane metody czy "główne". A więc testy piszemy dla metod publicznych. Jeśli więc wydzieliłeś metody jako publiczne, to mogą być wykorzystywane gdzie indziej i powinny być otestowane. Oczywistym jest, że nikt nie będzie bawił się refleksją w teście żeby przetestować tylko jedną prywatną metodę.

4

To ja dam radę od siebie, która wiele zmienia.
Nigdy nie piszemy testów dla metod - czy publicznych, czy prywatnych. jakichkolwiek.
Testujemy tylko funkcjonalność - czyli czy coś tam dla danego inputu daje zadany output. Od biedy wykonuje jakieś działanie. Kij nas obchodzi
czy to metoda prywatna czy publiczna, czy klasa - czy lambda.

W związku z tym z tego, że coś wydzielisz do osobnej klasy, metody itp nie oznacza, że musisz więcej testować.
A najlepiej jak dodatkowo wydzielone klasy, metody nie będą publiczne.

0

@Pinek: Super dzięki, właśnie w żadnym materiale czytanym o testach nie mogłem tego znaleźć i to było duże zmartwienie dla mnie czy dobrze to robię. Czyli analogicznie do klas jeśli wydzielam, dodatkową klasę z metodami i w niej są tylko protected/private to, również zostawiam wszystko w startej klasie z testami? tutaj: StringCalculatorClass

@jarekr000000: Więc jeśli dla funkcjonalności, to jeśli wydzielamy jakieś metody to wciąż testujemy ta główną, która "obsługuje" całą funkcjonalność?
Jeśli klasa nie jest wewnętrzna to nie może być private chyba, bo wtedy w ogóle nie byłoby dostępu do niej, bezużyteczna?

0
jarekr000000 napisał(a):

To ja dam radę od siebie, która wiele zmienia.
Nigdy nie piszemy testów dla metod - czy publicznych, czy prywatnych. jakichkolwiek.
Testujemy tylko funkcjonalność - czyli czy coś tam dla danego inputu daje zadany output. Od biedy wykonuje jakieś działanie. Kij nas obchodzi
czy to metoda prywatna czy publiczna, czy klasa - czy lambda.

W związku z tym z tego, że coś wydzielisz do osobnej klasy, metody itp nie oznacza, że musisz więcej testować.
A najlepiej jak dodatkowo wydzielone klasy, metody nie będą publiczne.

@jarekr000000 Okej, testujemy funkcjonalność. Natomiast rozważmy następującą sytuację -> pewna funkcjonalność została zaimplementowana za pomocą 10 publicznych metod. Teraz według ciebie, powinien być do tego jeden test. Nie sądzisz, że jeśli on nie przejdzie, to nie wiemy dokładnie gdzie wystąpił błąd? Będzie trzeba debugować test... Gdyby zamiast tego zrobić 10 osobnych testów (do każdej publicznej metody), od razu będziemy wiedzieć, która część, który krok w naszej funkcjonalności się wysypał.

0

@Pinek pierwsze pytanie to dlaczego funkcjonalność została zaimplementowana za pomocą 10 publicznych metod? Jakaś choroba?
Poza tym może problem wynika z nieprecyzyjnej defincji słowa funkcjonalność. Może lepiej byłoby użyć słowa "wymaganie". Nie chodzi mi o funkcjonalność typu "funkcjonalność kalkulatora", a o np. jeśli stworzysz iterator z pustej listy to hasNext() ma dać false. (Jest rano zapomniałem jak taki case, regułę "biznesową" nazwać najlepiej).

1

Do tych 10 publicznych metod (czy 10 publicznych klas) dla jednej funkcjonalności robisz jedną fasadę z jedną metodą - i ją testujesz.

Jak wygląda fasada:
https://dzone.com/articles/design-patterns-uncovered-1
https://pl.wikipedia.org/wiki/Fasada_(wzorzec_projektowy)

Jak unikać 10 metod publicznych:

0

@vpiotr Okej, czyli robimy tak jak wcześniej napisał @jarekr000000. Czyli jedna metoda, zapewniająca całą funkcjonalność. I oczywiście to jest dobre. ALE jeśli chodzi o testy, no to będzie trzeba zrobić sporo przypadków testowych i jeśli wmiędzyczasie się coś zmieni w implementacji, test się nam wywali i będzie trzeba debugować i patrzyć w którym dokładnie miejscu się coś zmieniło.

0

Nie no z tą fasadą to bym uważał ;) Bo napiszesz sobie tak najwyżej test integracyjny a nie o to chyba chodzi. Czasem mimo wszystko należy przetestować też trochę "internalsów", byleby robić to z głową.

0
Pinek napisał(a):

@vpiotr Okej, czyli robimy tak jak wcześniej napisał @jarekr000000. Czyli jedna metoda, zapewniająca całą funkcjonalność. I oczywiście to jest dobre. ALE jeśli chodzi o testy, no to będzie trzeba zrobić sporo przypadków testowych i jeśli wmiędzyczasie się coś zmieni w implementacji, test się nam wywali i będzie trzeba debugować i patrzyć w którym dokładnie miejscu się coś zmieniło.

Przypadki testowe powinny wynikać ściśle z interfejsu fasady.
Jeśli zbyt nisko zejdziesz z testami to zaczniesz testować funkcje które nie mają żadnego znaczenia dla użytkownika końcowego a wprowadzenie zmian będzie pociągało za sobą potrzebę zmiany wielu testów.

Z drugiej strony - można mieć 100% unit testów na zielono i niedziałające testy integracyjne. Bo np. obliczenie pola prostokąta działa, ale wysłanie fv za arkusz blachy już nie.

0

@vpiotr gorzej, bo w twoim przykładzie "cos nie działa", a nie musi tak być. Wystarczy że funkcja wysyłająca fakturę za arkusz blachy oczekuje że serwis zwracający pole prostokąta zwraca wynik w metrach kwadratowych a on zwraca w centymetrach kwadratowych. I teraz na poziomie unit testów wszystko działa zupełnie poprawnie, a mimo to test integracyjny się wywali.

0

@Pinek > .. testowych i jeśli wmiędzyczasie się coś zmieni w implementacji, test się nam wywali i będzie trzeba debugować i patrzyć w którym dokładnie miejscu się coś zmieniło.

Jeżeli Cie coś takiego zaskoczy to chyba nie za często te testy odpalasz. Wtedy nic nie pomoże. (Jak nie odpalasz testów regularnie po małych zmianach (same się nie odpalają) - to chyba ich nie potrzebujesz :-) - czysta oszczędność ).

Co do reszty mamy tu naszą standardową spór numer 1. Chyba między innymi dlatego, że pojęcie testów integracyjnych to IMO jakieś ścierwo. Nie przemawia do mnie.
Rozróżniam tylko testy. (Równie dobrze Unit Test Kalkulatora to Integration test Integera i Stringa :-) ).

I teraz tak - (przypadek 1) jeśli w trakcie pisania kalkulatora urodzi się coś co może działać jako samodzielny komponent (np. Parser), i mogę wobec tego komponentu zdefiniować wymagania - to warto pokryć testami.
Jeśli natomiast (przypadek 2) urodzą się metody i klasy pomocnicze (prywatne), które tylko w kontekście Kalkulatora mają sens i nie bardzo mogą osobno żyć - to nie będę ich osobno testował. (W ogóle pominę ich istnienie milczeniem - przecież mogą wylecieć jak zmienię implementację).

A co do przypadku 1 w szczególności na pewno nie będę też testował czy Kalkulator podczas działania wywołuje Parser. Bo z punktu widzenia Kalkulatora jest to zupełnie nieważne.
Co więcej mogę zechcieć w przyszłości zmienić implementację (na jakąś sprytną która radzi sobie bez Parsera) - i nie chciałbym wtedy przepisywać testów.

1 użytkowników online, w tym zalogowanych: 0, gości: 1