Java testy jednostkowe w JUnit - prośba o ocenę kodu

Witam forumowiczów,
pierwszy raz zabrałem się w życiu za pisanie testów jednostkowych i nie wiem czy robię to dobrze. Jakby ktoś kto się zna mógł zerknąć, doradzić co poprawić itp. to byłoby super ^^

package pl.xezolpl.cwiczenia.basket;

public class Product {

    private String name;
    private double price;
    private int amount;
          ///GETTERS
    public String getName() {
        return name;
    }

    public double getPrice() {
        return price;
    }

    public int getAmount() {
        return amount;
    }
            ///SETTERS
    public void setName(String name) {
        this.name = name;
    }

    public void setPrice(double price) {
        this.price = price;
    }

    public void setAmount(int amount) {
        this.amount = amount;
    }
             ///CONSTRUCTOR
    Product(String name, double price, int amount) {
        this.name = name;
        this.price = price;
        this.amount = amount;
    }
}

package pl.xezolpl.cwiczenia.basket;

import java.text.DecimalFormat;
import java.util.*;

import pl.xezolpl.cwiczenia.basket.Product;

public class Basket {

    private Map<String, Product> productHashMap = new HashMap<>();

    public boolean addProductToBasket(Product product) {
        if ((product.getName() == "") || (product.getAmount() == 0) || (product.getPrice() == 0)) {
            return false;
        } else if (!productIsInTheBasket(product.getName())) {
            productHashMap.put(product.getName(), product);
            return true;
        } else if (productIsInTheBasket(product.getName())) {
            productHashMap.get(product.getName()).setAmount(
                    productHashMap.get(product.getName()).getAmount() + product.getAmount()); /// sum of amounts
            return true;
        }
        return false;
    }

    public boolean removeProductFromBasket(String productName) {
        if (productIsInTheBasket(productName)) {
            productHashMap.remove(productName);
            return true;
        }
        return false;
    }

    public boolean changeAmountOfProductinBasket(String productName, int newAmount) {
        if (productIsInTheBasket(productName)) {
            productHashMap.get(productName).setAmount(newAmount);
            return true;
        }
        return false;
    }

    private double calculateBasketContent() {
        double sum = 0.0;
        for (Map.Entry<String, Product> entry : productHashMap.entrySet()) {
            sum += entry.getValue().getAmount() * entry.getValue().getPrice(); /// sum += price*amount
        }
        return ((double) ((int) (sum*100)))/100; /// cut off to the 2 decimal places

    }

    public void display() {
        System.out.println("YOUR BASKET");
        System.out.println("---------------------------");
        System.out.println("Name      Price      Amount\n");
        for (Map.Entry<String, Product> entry : productHashMap.entrySet()) {
            System.out.println(entry.getValue().getName() + "      " + entry.getValue().getPrice() + "      "
                    + entry.getValue().getAmount());
        }
        System.out.println("----------------");
        System.out.println("Total price: " + calculateBasketContent());
    }

    private boolean productIsInTheBasket(String productName) {
        if (productHashMap.get(productName) != null) {
            return true;
        }
        return false;
    }
}

package pl.xezolpl.cwiczenia.basket;

import org.junit.*;

public class Tests {

    private Product apple;
    private Basket basket;
    private static final double price = 0.01;

    @Before
    public void setUp() {
        apple = new Product("Apple", 2.99, 4);
        basket = new Basket();
    }

    @Test
    public void shouldAllowToAddProduct() {
        Assert.assertTrue(basket.addProductToBasket(apple));
    }

    @Test
    public void shouldDenyToAddBlankProduct() {
        Assert.assertFalse(basket.addProductToBasket(new Product("", 0, 0)));
    }

    @Test
    public void shouldAllowToAddTheSameItemTwice() {
        basket.addProductToBasket(apple);
        Assert.assertTrue(basket.addProductToBasket(apple));
        basket.display();
    }

    @Test
    public void shouldAllowToChangeProductWhatIsNotInTheBasket() {
        Assert.assertFalse(basket.changeAmountOfProductinBasket("Something", 3));
    }

    @Test
    public void shouldAllowToChangeProductAmount() {
        basket.addProductToBasket(apple);
        Assert.assertTrue(basket.changeAmountOfProductinBasket(apple.getName(), 7));
    }

    @Test
    public void shouldAllowToRemoveProductFromBasket() {
        basket.addProductToBasket(apple);
        Assert.assertTrue(basket.removeProductFromBasket(apple.getName()));
    }
    @Test
    public void shouldAllowToRemoveProductWhatIsNotInTheBasket(){
        Assert.assertFalse(basket.removeProductFromBasket("Something"));
    }
}

I czy w ogóle o to w tym chodzi, czy nie przesadziłem z if-ami w części basket-owej, bo mam takie wrażenie?

Nie wyglada to źle :) fajnie, że testy sprawdzają tylko jedną rzecz. Kilka uwag:

1. Tutaj masz dość prosty przykład, ale w ramach nauki możesz już teraz nadawać testom strukturę given-when-then. Zaprocentuje na przyszłość.
2. Metoda „productIsInTheBasket” -> można uprościć do jednej linii.
3. Klasa „Product” jest niepotrzebnie mutowalna. Powinieneś również rozdzielić „Produkt” jako towar od „Produktu” jako item w koszyku, który ma liczbę w ramach koszyka.
4. Używaj static imports: „Assert.assertTrue” -> „assertTrue”. Polecam w ogóle jak najszybciej przejść na AssertJ lub Spock.
5. W jednym teście masz „display()” - po co? Poza tym koszyk nie powinien umieć się sam wypisać na ekran - powinny być do tego dedykowane klasy jak np. ConsoleBasketDisplay czy JsonBasketDisplay - koszyk nie powinien mieć świadomości, jak jest wyświetlany. Oczywiście, możesz dodać po prostu „toString()” zamiast „display()”.

Charles_Ray napisał(a):

Nie wyglada to źle :) fajnie, że testy sprawdzają tylko jedną rzecz. Kilka uwag:

1. Tutaj masz dość prosty przykład, ale w ramach nauki możesz już teraz nadawać testom strukturę given-when-then. Zaprocentuje na przyszłość.
2. Metoda „productIsInTheBasket” -> można uprościć do jednej linii.
3. Klasa „Product” jest niepotrzebnie mutowalna. Powinieneś również rozdzielić „Produkt” jako towar od „Produktu” jako item w koszyku, który ma liczbę w ramach koszyka.
4. Używaj static imports: „Assert.assertTrue” -> „assertTrue”. Polecam w ogóle jak najszybciej przejść na AssertJ lub Spock.
5. W jednym teście masz „display()” - po co? Poza tym koszyk nie powinien umieć się sam wypisać na ekran - powinny być do tego dedykowane klasy jak np. ConsoleBasketDisplay czy JsonBasketDisplay - koszyk nie powinien mieć świadomości, jak jest wyświetlany. Oczywiście, możesz dodać po prostu „toString()” zamiast „display()”.

1.Doczytam, bo szczerze powiedziawszy pierwszy raz słyszę.
2.Hmmm coś takiego? return !productHashMap.get(productName).equals(null);
3.Nie rozumiem, chodzi ci o jakiś model przejściowy, komunikacyjny między tymi klasami czy co?
4.A no rzeczywiście, było na kursie ale zapomniałem użyć, racja, już zmieniam :)
5.A to tak poglądowo czy amounty się sumują. Hmm no niby tak, no powinno być, ale toString zamiast display to nie rozumiem, tylko o zmianę nazewnictwa ci chodzi czy jak?

I oczywiście dziękuję za odpowiedź, już poprawiam moje testy ;)

Mała kosmetyka, trochę poprawi czytelność.

public boolean addProductToBasket(Product product) {

     if ((product.isXxx()) { // Xxx -  Null, Blank, Empty, ContainData, cokolwiek  
            return false;
     }

	if (!productIsInTheBasket(product.getName())) {
            productHashMap.put(product.getName(), product);
            return true;
     } 

	if (productIsInTheBasket(product.getName())) {
            productHashMap.get(product.getName()).setAmount(
                    productHashMap.get(product.getName()).getAmount() + product.getAmount()); /// sum of amounts
            return true;
     }

     return false;

    }

Nad tym bym popracował:

productHashMap.get(product.getName()).setAmount(
                    productHashMap.get(product.getName()).getAmount() + product.getAmount()); /// sum of amounts

if (productIsInTheBasket(product.getName())) {
   existing(???) = productHashMap.get(product.getName())
   existing.addAmount(product) 
   ....

Mniej kodu ale i tak jest to z dupeczki, bo niby czemu Product ma pole amount? Koszyk powinien mieć taką informacje

if ((product.getName() == "") || (product.getAmount() == 0) || (product.getPrice() == 0)) {

A może po prostu walidować tworzenie Productu od razu? ;)

} else if (productIsInTheBasket(product.getName())) {

Po co drugi check? Skoro przed chwilą było że to false, to wystarczyłby else

public boolean removeProductFromBasket(String productName) {

Wiesz że remove z Map<K,V> robi dokładnie to samo? Po co tak zaimplementowałeś?

sum += entry.getValue().getAmount() * entry.getValue().getPrice();

Dramat! Czemu klasa Product nie potrafi zwrócić swojego kosztu tylko do liczysz jak zwierze wyciągając bebechy tego Producta?

return ((double) ((int) (sum*100)))/100; /// cut off to the 2 decimal places

Jeśli musisz dać taki komentarz to znaczy że należało z tego zrobić osobną metodę z ludzką nazwą.

public void display() {

A może zamiast tego metoda toString() ? ;)

private boolean productIsInTheBasket(String productName) {

jw z remove, java już to ma!

Parę uwag, które nie są bezpośrednio związane z testami:

• "amount" - w angielskim używane do określenia ilości czegoś niepodzielnego. W kontekście ilości produktów może po prostu "quantity" ?
• operacje na pieniądzach - może cenę trzymać jako Money (JSR-354)?
• getters/setters/constructors - komentarze, które wg mnie nic nie wnoszą - widać czym są wspomniane metody. Można rzecz jasna polemizować i mówić, że to "taka konwencja".