Jump to content
GSForum - Segélyvonal

Tetris (java code review)


klenium2

Recommended Posts

klenium2

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.

Link to comment
Share on other sites

Fujitsu

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

Link to comment
Share on other sites

payskin

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

Link to comment
Share on other sites

Bowie

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

  • Haha 1
Link to comment
Share on other sites

Fujitsu

Hát szerintem én elég kevés dologhoz értek, de bóknak veszem, ha ezt ti másképp gondoljátok. :D 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.

Link to comment
Share on other sites

  • 2 weeks later...
Fujitsu

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.

Link to comment
Share on other sites

kléni

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

Link to comment
Share on other sites

Fujitsu

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.

Link to comment
Share on other sites

kléni
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.

Link to comment
Share on other sites

Fujitsu
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.

Link to comment
Share on other sites

kléni

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.

Link to comment
Share on other sites

Fujitsu

É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.)

Link to comment
Share on other sites

kléni

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.

Link to comment
Share on other sites

Fujitsu

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.

Link to comment
Share on other sites

kléni

É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.)

Link to comment
Share on other sites

Fujitsu

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.

Link to comment
Share on other sites

kléni

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.

Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×
×
  • Create New...