Koodikatselmointi - Githubissues

Ladattu 25.9 klo 15:30

Koodi on yleisesti selkeää, kuten on ohjelman rakenne (paketit). Testit ovat myös varsin kattavia.

Peli-luokan funktiossa voikoKortinKaantaa:

    public Kortti voikoKortinKaantaa(Kortti kortti) {

        while (kortti.onkoLoydetty() || kortti.nakyykoKuva()) {
            System.out.println("Korttia ei voi kääntää. Käännä toinen kortti.");
            int uusi = Integer.parseInt(lukija.nextLine());
            kortti = pakka.korttiSijainnilla(uusi);
        }

        return kortti;
    }

Kun voikoKortinKaantaa kysyy uutta korttia jos alkuperäistä ei voi kääntää, se ei tarkista onko kortin sijainti liian suuri tai pieni:

Pelataan
Korttien sijainnit välillä 1-4
Muut arvot lopettavat pelin
 0 0
 0 0

Käännä ensimmäinen kortti
1
 1 0
 0 0

Käännä toinen kortti
1
Korttia ei voi kääntää. Käännä toinen kortti.
-1
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: -2

Samaan tapaan ohjelma tarkistaa vain onko annettu luku liian pieni tai suuri - null kaataa ohjelman.

Pelataan
Korttien sijainnit välillä 1-4
Muut arvot lopettavat pelin
 0 0
 0 0

Käännä ensimmäinen kortti

Exception in thread "main" java.lang.NumberFormatException: For input string: ""

Lisäksi Peli.voikoKortinKaantaa on hiukan hämäävästi nimetty. Pelkästä nimestä luulisi sen palauttavan booleanin, ja ettei se oikeastaan "tee" mitään. Kenties parempi olisi koitaKaantaaKortti tai vastaava.

Muita oikeita ongelmia en löytänyt. Korttipakka.ovatkoPari

    public boolean ovatkoPari(Kortti eka, Kortti toka) {

        if (eka.arvo() == toka.arvo()) {
            return true;
        } else {
            return false;
        }
    }

olisi ehkä siistimpi muodossa

    public boolean ovatkoPari(Kortti eka, Kortti toka) {

        if (eka.arvo() == toka.arvo()) {
            return true;
        }
        return false;
    }

mutta tämä on pitkälti makuasia. Toinen vaihto ehto olisi

    public boolean ovatkoPari(Kortti eka, Kortti toka) {

        boolean onPari = false;
        if (eka.arvo() == toka.arvo()) {
            onPari = true;
        } 
        return onPari;
    }

jos pitää elegantimpana että on vain yksi return funktiossa. Sama formaatti on myös Peli.lopeta-funktiossa, jos päätät muuttaa ovatkoPari:n.

Korttipakan testeissä on toteuttamaton korttipakkaSisaltaaKaikkiaArvojaKaksiKpl(), jolla kommentti //Tätä olisi varmasti hyvä testata, mutten vielä tiedä miten. Tämä voisi olla muotoa

import java.util.ArrayList;
...
@Test
    public void korttipakkaSisaltaaKaikkiaArvojaKaksiKpl() {
        ArrayList<Kortti> kortit = pakka.kortit;

        for (int i = 1; i < 5; i++) {
            int n = 0;
            for (Kortti kortti : kortit) {
                if (kortti.arvo() == i) {
                    n++;
                }
            }
            assertEquals(n, 2);
        }
    }

Lisäksi voisi tietenkin testata, että liian pieniä ja suuria arvoja on nolla.

PeliTest.peliTunnistaaJosKorttiOnJoLoydettyEikaAnnaKaantaaSita() on samantapainen kommentti. Testin pitäisi tietää milloin pari on löydetty jotta se voisi testata, voiko löydetyn kortin kääntää. Koska tämä ilmoitetaan pelaajalle System.out.println:llä, pitäisi testin päästä käsiksi konsolin outputtiin. Nopea googlaus johti Stack Overflow -kysymykseen mistä voisi olla apua. ByteArrayInputStream taas saattaisi (ehkä) sallia myös konsolin inputin antamisen testissä.

Nuukkeli / NeanOhjelmoinninHT

Koodikatselmointi #1