Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Koodikatselmointi #1

Open
meklu opened this issue Apr 28, 2020 · 0 comments
Open

Koodikatselmointi #1

meklu opened this issue Apr 28, 2020 · 0 comments

Comments

@meklu
Copy link

meklu commented Apr 28, 2020

Latasin projektipuun 28.4.2020 klo 20.00 (commit 0759df4).

Yleisellä tasolla koodi näyttää hyvälukuiselta sekä loogisesti osalohkoihin jäsennetyltä. Samaa voisi sanoa myös sovelluksen arkkitehtuurista. Kommentointi oli mielestäni hyvällä tasolla kuten myös yleinen muuttujien nimien selkeys.

Ohessa kuitenkin muutama huomio.

Koodi

Huomasin dao-koodissa aika paljon tietokantayhteyden avaamista ja sulkemista, en toki välttämättä menisi sanomaan tätä suureksi ongelmaksi.

Näyttää hieman hassulta, että ohjelman elinkaaren aikana käyttöliittymän osia luodaan jatkuvasti uusiksi näkymästä toiseen siirtyessä. Javan roskankeruu toki jossain vaiheessa nämäkin käy poimimassa, mutta voisi olla hyvä olla nojaamatta siihen.

Testeihin oli jäänyt pari tyhjää hello() -testimetodia. Lisäksi yhden testiluokan nimi on taidettu typottaa sen luontivaiheessa siten, että sieltä löyty yksi liian iso i-kirjain: TopLIstLogicTest -> TopListLogicTest

Käytettävyys

TOP-listalle päästessä nimimerkin syöttöön tarkoitettu tekstikenttä sisältää edelleen merkkijonon "nimimerkki". Käytettävyyden näkökulmasta WinnerLayoutUi:nameField() -metodissa kannattanee olla antamatta TextFieldin konstruktorille merkkijonoa, vaan sen sijaan tehdä vaikkapa seuraavalla rivillä seuraavaa:

winnerName.setPromptText("nimimerkki");

Näin on näemmä jo tehtykin esimerkiksi StartMenuUi:ssa.

Alkuruudussa olisi kiva, jos enteriä painamalla pääsisi eteenpäin ohjelman suorituksessa. Ruudukkokoon tekstikentän onAction voisi siirtää fokuksen voittosuoran määrittelyruutuun ja voittosuoran määrittelyruudun onAction voisi tehdä saman asian kuin ikkunan alareunassa elelevä nappi.

Olisi ihan kiva päästä myös tarkastelemaan ennätyslistoja muutoinkin kuin voittamalla peli, esimerkiksi pelin tilastonäkymän kautta.

Pari pientä teknistä hassuutta

TopListsDao:averageMoves()
Varsinaisen keskiarvon laskemisenkin voisi tehdä vaikka suoraan tietokannassakin, niin ei tarvitsisi pyöritellä hirmuisesti dataa edestakaisin Javan ja SQLiten välillä. Esimerkiksi SQLiten sisäänrakennettu funktio avg() näyttäisi sopivahkolta. Näillä datamäärillä tämä ei toki ole mikään hirmuisen kriittinen juttu.

GameLayoutUi:setTurn()
Metodin parametria turn ei käytetä.

GameLogic:GameLogic()
Sisennys on näemmä hieman levinnyt javadoc-kommentista konstruktorin alusta.

GridUi:creatingGrid()
Sisennys javadoc-kommentin alusta hieman lintassa.

WinnerLayoutUi:addTopListButton()
Sisennys toLists:n action event callbackissa on vähän vinksallaan.

Lopputulema

Hyvältä näyttää, samaa rataa eteenpäin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant