tuomokar / movie-theater-seat-service Goto Github PK

Ladattu 10.10. 13.48

Ensivaikutelmana projekti vaikutti suurelta ja monimutkaiselta, mutta kyllähän se äkkiä selveni luokkakaavion ja järkevän pakkausjaon avulla.

Single responsibility -periaatetta noudatettu erittäin hyvin niin luokkien työnjossa kuin metodeissakin, jotka on pidetty hyvin yksinkertaisina. Esimerkiksi mainittakoon Hall-luokan paikkojen luonti jossa yksi kokonaisuus jaettu osiin siten, että kaikki metodit ovat vain muutaman rivin pituisia. Ei muuten olisi itselle aivan ensimmäisenä tullut mieleen nuo sisäkkäiset HashMapit teatterisalin mallinnustapana, mutta on kyllä aika näppärä idea. Myös javadocit pääosin kuvaavat metodit perusteellisesti, ja koodi on siis helposti luettavaa.

Projekti ei buildannut, ongelmina HallParser ja HallHandler, mutta luulen että satuin vain lataamaan huonoon aikaan. (Parserissa puuttuu Files.readAllLines() -kutsusta charset-argumentti ja Handlerissa kutsutaan muiden salinkäsittelyluokkien konstruktoreja väärillä argumenteilla.)

En nyt hirveästi keksinyt mitään suurempaa kehitettävää, taitaa oma projekti olla useimmilta osin huonommassa kunnossa.

-Positiivista: Luokkahahmotelmat ovat todella selkeät: Sinulla on monta luokkaa joissa jokaisella on selkeät tehtävät. Luokkiesi pituudet ovat lyhyet.

-Negatiivista: Command-luokan perivät luokat kaikki tuntee scannerin. Eikö nämä olisi tarkoitus tehdä yleisessä käyttöliittymä luokassa joka tuntisi Scanner olion? Nyt sekä käyttöliittymät että commandit tuntevat scannerin.

-Negatiivista : Add-luokallassi abort() metodi on hieman hämmentävä. Se tulostaa tekstiä ja paluttaa truen.Jos abort- metodin tarkoitus on vain tulostaa tekstiä se voisi olla void abort() ?

-Positiivista : Hyvin käytetty perintää Commandeissa.

19.9 16:36

I downloaded the project 17.09.2015 @ 21:00

-First of all, your project looks great. Your code is clean. Naming makes it
easily understandable and I found just one longer method in the HallParser class, but
that is the only one. There is no copy-paste and the Single Responsibility Principle
is well executed. Testing seems to be in good order too.

-It's still early to say what you have planned, but pay attention to keeping the program
logic and the ui apart. It seems as if some of the logic could be making it's way to
the ui parts.

-You are now checking for correct input in multiple places when creating a hall.
Is it necessary? Also, You haven't yet checked if the input is an integer in the
first place but I'm sure you have that planned.

-There are some one line long private methods, atleast in the HallRemover class.
Are those necessary? Your naming is very good so I don't think those methods
improve the readability.

-I'd probably put the HallHandler class in the seatservice.filehandling package. You
could also think again if some other packages could be combined.