Tetris (java code review)

[klenium2 0]

Készítettem egy Tetris játékot, ami java-ban lett implementálva. Minél jobb szoftver tervet akarok elérni későbbi munkál miatt, ehhez jól jönne néhány tanács, észrevétel. Akinek van kedve/ideje, megköszönném ha ránézne.  Felraktam a StackExchange-ra is, részletek itt.

[Fujitsu 812]

Eléggé végtelen mennyiségű dolgom van mostanában, de ha nem felejtem el, valamikor a jövő héten belenézek.

[payskin 917]

Vazz, esetleg van olyan dolog a világon, amihez nem értesz?  

[Bowie 16]

Motor és bicikli. Ja meg szuperhős filmek.

[Fujitsu 812]

Hát szerintem én elég kevés dologhoz értek, de bóknak veszem, ha ezt ti másképp gondoljátok.  A számítástechnika véletlenül épp a szakmám, szóval abban előfordulhat, hogy hozzá tudok szólni pár dologhoz.

[Fujitsu 812]

No, megnéztem a módosított, aktuális verziódat. Architekturálisan az a megjegyzésem, hogy az nagyon rendben van, hogy mátrixműveletek alkalmazása helyett letárolod a tetrominók elforgatott állapotait, viszont a parts és views ilyetén használatának / tárolásának oka nem teljesen világos számomra. Én a teljes játékot kétdimenziós bináris tömbök formájában tárolnám, a megjelenítést pedig egyetlen teljesen külön rétegbe (osztályba) szervezném ki, ami egyaránt kirajzolná a board 2D tömbjét és az aktuális tetromino 2D tömbjét (de nem négyzetenként, hanem egyben). Ennek implementálásához a builder és az observer design patternek a barátaid. Magának a játéknak az osztályaiban a funkciót és a megjelenítést ilyen értelemben teljesen szétválasztanám; az egyes vizsgálatokat, műveleteket közvetlenül a tetrominók és a board 2D tömbjein végezném el, a view-kat ezeknél kihagynám a játékból. A view-k, partok és a játéklogika jelenlegi relációja meglehetősen nehezen követhető (pl. setView-getView móka a board tartalmának módosításánál).

Statikus analízis szempontjából az alábbiak a meglátásaim:

– A kondíciók zárójelezését nem érdemes megspórolni soha. Itt van például ez a rész: 

fromX < 0 || fromX + data.getWidth() > columns || fromY < 0 || fromY + data.getHeight() > rows

Hadd ne legyen már rám, az alkalmazott programozási nyelvre és pláne a fordítóra bízva a precedenciasorrend. A

(fromX < 0) || ((fromX + data.getWidth()) > columns) || (fromY < 0) || ((fromY + data.getHeight()) > rows)

a világon minden és mindenki számára egyértelmű, és jóval olvashatóbb is. De szerintem ezt már valamikor írtam neked.

– A Board osztály removeFullRows tagfüggvényében az isRowFull, az addTetromino tagfüggvényben a baseX és baseY változó deklarálható a for cikluson belül. Mindig mindent a legszűkebb szkópon belül érdemes deklarálni és definiálni, ahol használod.

– Sokkal olvashatóbb lenne a kód, ha nem alkalmaznál több helyütt is obskúrus nevű segédváltozókat. Itt van például ez a ciklus:

        for (TetrominoPart part : data.getParts()) {
            int x = baseX + part.getOffsetX();
            int y = baseY + part.getOffsetY();
            table[y][x].setView(part.getView());
        }

Mi az a data, mi az a baseX, baseY? Ezek nem éppen beszédesek az adott kontextusban. Ez sokkal érthetőbb szerintem:

        for (TetrominoPart part : tetromino.getData().getParts())
        {
            int x = tetromino.getPosX() + part.getOffsetX();
            int y = tetromino.getPosY() + part.getOffsetY();
            table[y][x].setView(part.getView());
        }

Egyelőre kb. ennyi ötlött szembe, ami még esetleg lenne, az már inkább egyéni ízlés kérdése.

[kléni 7]

lol, ez még tényleg a héten belül volt.  Köszi, átnézem eszerint nemsokára.

[Fujitsu 812]

Igen, eszembe jutott, hogy megígértem. Marha kevés időm van a rengeteg feladatom és a nyakamon lévő határidők miatt (ami kevés szabadidőm van, abban meg menekülök mindenféle számítógép elől), de azért a szavam tartani szoktam.

[kléni 7]

On 7/29/2018 at 8:48 PM, Fujitsu said:

– A Board osztály removeFullRows tagfüggvényében az isRowFull, az addTetromino tagfüggvényben a baseX és baseY változó deklarálható a for cikluson belül. Mindig mindent a legszűkebb szkópon belül érdemes deklarálni és definiálni, ahol használod.

Az isRowFull-t átírtam, viszont a koordinátáknál más a helyzet. Azok az iteráció alatt nem változnak, így fölösleges lenne minden alkalommal lekérdezni ugyan azt az értéket.

Ha tényleg a legújabb változatot nézted, ami GitHub-on érhető el, akkor amit az architektúráról írsz, nem igen értem.

A nézet és a logika pont hogy el van különítve, néhány segédosztály csak azért van, hogy ez megvalósítható legyen. Az újrafelhasználhatóság érdekében úgy csináltam, hogy a Board és Tetromino osztályokban semmi olyan ne legyen, amin a megjelenítés lecserélésekor változtatni kellene.

Mivel a tábla és a mozgó forma külön van kezelve (ami logikus, és az irányítást jóval egyszerűbbé teszi), az adataik is külön lesznek tárolva, így amikor a forma leér, mindenképpen szükség van az adatok (és így a nézetek) átadására. Kicsit javítottam rajta ugyan, de a Board::addTetromino-ban nem lehet megspórolni vagy átvinni máshova a nézetek átadását. Erre nem is igen van szükség, mert a mozgó forma és a tábla nézeteinek egyező interfésze kell legyen, hiszen ugyan azokat a színes négyzeteket kezelik.

A tábla most is 2D tömbben van tárolva. A tetromino-nál azért nem 2D tömb van, hogy ne legyen szükség előzetes (null vagy false) ellenőrzésekre, ez OOP szempontból jobb, nincsenek fölösleges "lyukak" a rácsban, amiket külön kellene kezelni. Az offset eltárolásával a négyzet pozícióját még így is megkapjuk, ezért az 1 és 2 dimenziós tömbök használata között itt nem igen van különbség.

Arra én is gondoltam amúgy, hogy a tetromino-nál lehetne csak bool tömböt használni, de aztán úgy voltam vele, hogy amikor a leesett formát hozzá kell adni a táblához, az adatokat úgy is át kell adni, tehát a tetromino-nak ehhez az adatnak hozzá kell tudni férnie, akkor meg csak redundancia lenne a bool tömb külön használata. Azt pedig, hogy a tetromino nézete csak egy bool tömb alapján rajzolna (és nem lennének neki külön SquareView tagjai), azért nem tartom jónak, mert akkor a táblához adáskor kellene ezeket létrehozni, hiszen a táblánál már nem elég csak egy bool tömb (a színek miatt). Ha pedig úgy is kellenek ezek a nézetek, miért ne dolgoznék velük már az elejétől. A TetrominoPart osztály bevezetésével elkülöníthető lett a nézet a Tetromino osztálytól, tehát a nézet módosításánál a Tetrominot nem kéne már piszkálni.

[Fujitsu 812]

1 hour ago, kléni said:

Azok az iteráció alatt nem változnak, így fölösleges lenne minden alkalommal lekérdezni ugyan azt az értéket.

Az egy getter metódus, így ez nem történik meg; a compiler ki fogja optimálni a függvényhívást, és beteszi oda a konkrét változót. De mindegy, ez részletkérdés, csak a jobb olvashatóság miatt javasoltam. (Vagy adhatsz a változóknak beszédesebb nevet, az is ugyanolyan jó.)

1 hour ago, kléni said:

de a Board::addTetromino-ban nem lehet megspórolni vagy átvinni máshova a nézetek átadását.

De meg lehet. Én arról beszéltem, hogy te a board celláit view-k formájában építed fel, és ezeket a view-kat módosítod / lekérdezed, amikor a tábla változik. Ha valóban szét akarod választani a nézetet és a logikát, akkor a view nem kerülhet elő a játéklogikában, mert így a boardon nem alakzatok vannak, hanem csupán négyzetek, amiknek van valamilyen színük. Ha azt mondanám, hogy legyenek az alakzatok egyszínűek fekete szegéllyel, akkor máris meg vagy lőve, mert a jelenlegi felépítés miatt a nézet nem teljesen független a működési logikától.

A kirajzolást éppen ezért kellene függetleníteni. A tárolás adatstruktúrájában egyáltalán nem kellene előkerülnie semmilyen nézetnek. Ha nem is bináris tömbbel valósítod meg, pl. használhatnál cellaazonosítókat, amik megmondanák, hogy melyik cella melyik alakzathoz tartozik (ha tartozik).

1 hour ago, kléni said:

Ha pedig úgy is kellenek ezek a nézetek, miért ne dolgoznék velük már az elejétől.

Mert az objektumorientált tervezés lényege pontosan az a kódszintű absztrakció, ami elveszik, ha optimalizálási célokkal elkezdesz funkcionális függőségeket kialakítani az osztályok belső struktúrái között. A nézet és a játéklogika egymástól logikailag teljesen független kellene legyen.

[kléni 7]

Hm, kezdem már érteni a problémát.

A board nem tartalmazhat alakzatokat a sortörlések miatt. Ott már csak egymástól független cellák vannak, amik tök random átrendeződhetnek. Persze ez nem csak egyszínű kocka lehet, hanem bármilyen egységnyi nézet, interfésszel helyettesíthető lenne. A tetrominonál már más a helyzet, az tényleg összefüggő egység. De éppen a sortörlés miatt egyik tetris kirajzolásánál se szoktak összefüggeni a tetromino részei, legalábbis én még sose láttam ilyet. Trükközések a TetrominoView osztályban még lehetségesek, hiszen ott ki lehet nyerni a tetromino által használt rácsot, annak segítségével meg már tetszőleges szegély, színátmenet használható, de kicsit macerás lenne, és lehet felülíráshoz is vezetne.

Annyi ötletem lenne, hogy a board cellák egy általános nézetet/nézet típusát vagy azonosítóját tárolnának el, amit később a BoardView feldolgoz, a Tetromino dolgozhatna nézetet nem tároló struktúrákkal, esetleg a kirajzolást a JavaFX készletével hatékonyabban meg lehet oldani, a Board::addTetromino-ban meg lenne valami TetrominoView -> BoardCell[][] konverzió. Szélsőséges esetben, ha a tetromino kockái között volt kapcsolat, és nem csak sima egyszínűek, akkor a tetromino képét le lehet menteni, feldarabolni kockákra, és a cellák ezeket a kis képeket jelenítenék meg a board-ban.

[Fujitsu 812]

Én nem tárolnék semmilyen nézeteket, egyszerűen cellaazonosítókat tárolnék egy-egy tömbben, és ez lenne a board, illetve a tetromino adatstruktúrája. (Ahogy minden alakzatnak, úgy lehetne egy külön azonosítója az üres játéktérnek, illetve a board már "feltöltött" részének; és a teljes játéklogika csak ezekkel az azonosítókkal dolgozna, illetve ezeket módosítaná.) Lehetne ehhez az azonosítóhoz csapva akár az offset is, így az a segélyosztály sem kéne. (Az offset pedig a board módosításánál / lekérdezésénél is használható lenne, így legitim gondolat, hogy általánosan legyen az adatstruktúra elemeinek része.)

A megjelenítő modul egyben kapná meg a teljes kirajzolandó alakzat (vagy board) adatstruktúráját, és az egészet úgy rajzolná, ahogy csak a modult író programozó virágos jókedve tartja. Ha ott a teljes alakzat / board, akkor olyan színátmenetet, háttérképet, szegélyt vagy akármit csinálok, amilyet csak akarok; az már csak és kizárólag a dizájn része, a működés ettől független. S ha változik a tetromino vagy a board (elforgatom az alakzatot, hozzáadódik egy a boardhoz stb.), akkor a visitor-observer patternek mentén megvalósított kapcsolat révén változhat az alakzat / board megjelenése is. (Ha pl. ismered Qt-ben a signals & slots mechanizmust, akkor érted, mire gondolok.)

[kléni 7]

Lehetne így is, igen. Csak azért terveztem úgy, hogy a nézet legyen eltárolva, hogy ne egy külső modul, típusellenőrzést használva mondja meg, hogy kell kirajzolni a dolgokat, hanem a dolog maga, a megadott módon rajzolja ki magát. Az utóbbit általában jobbnak tartják OOP szempontból, itt ha bővülne speciális funkciót tudó formákkal a játék, a kirajzolást külön kellene felvenni. Az ilyen külső műveletek könnyebben elfelejthetők, mint amikor konkrétan el van tárolva az az objektum, ami megcsinálná a külső munkát.

[Fujitsu 812]

Egyrészt nem kell típusellenőrzés; az alakzatok és a boardok is ugyanúgy rajzolhatók, a board is voltaképpen egy alakzat. Másrészt ha különféle módon kéne kirajzolni, akkor a kirajzolást implementálhatod a tetromino és a board osztályok részeként is, ez már részletkérdés. A lényeg az, hogy a játéklogikában ne kerüljön elő a megjelenítés, a megjelenítés pedig ne függjön a játéklogikától.

[kléni 7]

Én a cellák színére vagy stílusára gondoltam. A board-ban valami információt csak el kell tárolni, az egyes cellákban milyen fajta adat található, amit aztán a megjelenítő osztálynak meg kell vizsgálnia, hogy a megfelelő színű négyzetet rajzolja ki. Ha nem is közvetlenül, de ez valamilyen szinten vizsgálat, az értékektől függően elágazik a kirajzolás menete. (jó, a színeknél pont nem, mert lehet az azonosítót egy színekből álló tömb indexelésére is használni, de ha nem egyszínű, hanem pl. mintás lenne, akkor azt ott külön kéne kezelni.)

[Fujitsu 812]

Igen, de ez nem attól függ, hogy tetromino vagy board, mert sem a tetromino, sem a board formája nem adott előre. Szóval ezt úgy célszerű megvalósítani, hogy egyforma logikával rajzolsz ki minden alakzatot. Ha csak színes négyzetek megjelenítése lesz a kirajzolás, akkor ez a logika marha egyszerű; színátmeneteknél, szegélyeknél már kell egy kicsit gondolkodni.

[kléni 7]

Na, módosítgattam rajta, az új verzió megtekinthető itt. Köszi a tippeket. Részben máshogy csináltam, de szerintem a mostani tök jó lett. Ki lehet bővíteni minden szempontból, a kirajzolás is megoldható egyben (ami jó lesz majd konzolos megjelenítés esetén), vagy ha kell majd, irányítottan, kb. mint a régiben (a cella formájának vizsgálata nélkül). Mondjuk teljesen nem független még a logika és a nézet package, de már kellően kevés függőség van a komponensek között, osztályokból is kevesebb lett.