Java swing sokoban opinie o programie

marcinkk · 18 Luty 2013 21:06

witam, zrobiłem grę sokoban w javie. chciałem prosić o uwagi i porady odnośnie kodu. gra składa się z klas, ogólnie wszystko działa ale brakuje kilku warunków podczas poruszania, gra jest oparta o tablice dwu wymiarową. pozdrawiam i wstawiam kod dla potomnych

/*

 * To change this template, choose Tools | Templates

 * and open the template in the editor.

 */

package javaswingsokoban;


import java.awt.Color;

import java.awt.Graphics;

import javax.swing.JOptionPane;

import javax.swing.JPanel;


/**

 *

 * @author marcin kaszuba

 */

public class ClassBoard extends JPanel{


    private ClassMove classMove = new ClassMove(this);

    private int sizeSquare = 40;

    private int positionX = 50;

    private int positionY = 50;

    //0 - mur czarny

    //1 - puste pole bialy

    //2 - pudelko zielony

    //3 - ludzik czerwony

    //4 - pole końcowe

    static int[][] plansza = {

        {0,0,0,0,0,0,0,0,0,0,0,0,1,1},

        {0,4,4,1,3,0,1,1,1,1,1,0,0,0},

        {0,4,4,1,1,0,1,2,1,1,2,1,1,0},

        {0,4,4,1,1,0,2,0,0,0,0,1,1,0},

        {0,4,4,1,1,1,1,1,1,0,0,1,1,0},

        {0,4,4,1,1,0,1,0,1,1,2,1,0,0},

        {0,0,0,0,0,0,1,0,0,2,1,2,1,0},

        {1,1,0,1,2,1,1,2,1,2,1,2,1,0},

        {1,1,0,1,1,1,1,0,1,1,1,1,1,0},

        {1,1,0,0,0,0,0,0,0,0,0,0,0,0}

    };


    public ClassBoard() {

        this.setFocusable(true);

        addKeyListener(classMove);

    }


    @Override

    public void paint(Graphics graphics){

        drawBoard(graphics);

    }


    void drawBoard(Graphics graphics){

        super.paint(graphics);

        for(int i = 0; i < plansza.length; ++i){

            for(int j = 0; j < plansza[0].length; ++j){

                if(plansza[i][j] == 0){

                    graphics.setColor(Color.BLACK);

                    graphics.fillRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare);

                } else if(plansza[i][j] == 1){

                    graphics.setColor(Color.WHITE);

                    graphics.fillRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare);

                } else if(plansza[i][j] == 2){

                    graphics.setColor(Color.GREEN);

                    graphics.fillRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare);

                } else if(plansza[i][j] == 3){

                    graphics.setColor(Color.RED);

                    graphics.fillRoundRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare, 360, 360);                  

                } else if(plansza[i][j] == 4){

                    graphics.setColor(Color.blue);

                    graphics.fillRoundRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare, 0, 0);                    

                }

            }

        }

    }


    static void checkingFinish(){

        if((plansza[1][1] == 2) && (plansza[1][2] == 2) && (plansza[2][1] == 2) && (plansza[2][2] == 2 &&

                (plansza[3][1] == 2) && (plansza[3][2] == 2)) && (plansza[4][1] == 2) && (plansza[4][2] == 2) &&

                (plansza[5][1] == 2) && (plansza[5][2] == 2)){

            JOptionPane.showMessageDialog(null, "wygrana", "wygrales !!", JOptionPane.INFORMATION_MESSAGE);

        }

    }


}

/*

 * To change this template, choose Tools | Templates

 * and open the template in the editor.

 */

package javaswingsokoban;


import java.awt.event.KeyEvent;

import java.awt.event.KeyListener;


/**

 *

 * @author marcin kaszuba

 */

public class ClassMove implements KeyListener{


    private ClassBoard classBoard;

    private int x = 1;

    private int y = 4;


    public ClassMove(ClassBoard classBoard) {

        this.classBoard = classBoard;

    }


    @Override

    public void keyTyped(KeyEvent e) {

    }


    @Override

    public void keyPressed(KeyEvent keyEvent) {

        switch(keyEvent.getKeyCode()){

            case KeyEvent.VK_UP:

                System.out.println("wcisles up");

                if((ClassBoard.plansza[x - 1][y] == 2) && (ClassBoard.plansza[x - 2][y] == 4)){

                    ClassBoard.plansza[x - 2][y] = 2;

                    ClassBoard.plansza[x - 1][y] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    x--;

                } else if(ClassBoard.plansza[x - 1][y] == 1){

                    ClassBoard.plansza[x - 1][y] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    x--;

                } else if((ClassBoard.plansza[x - 1][y] == 2) && (ClassBoard.plansza[x - 2][y] == 1)){

                    ClassBoard.plansza[x - 2][y] = 2;

                    ClassBoard.plansza[x - 1][y] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    x--;

                } else if(ClassBoard.plansza[x - 1][y] == 4){

                    ClassBoard.plansza[x - 1][y] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    x--;

                }

                break;

            case KeyEvent.VK_DOWN:

                System.out.println("wcisles down");

                if((ClassBoard.plansza[x + 1][y] == 2) && (ClassBoard.plansza[x + 2][y] == 4)){

                    ClassBoard.plansza[x + 2][y] = 2;

                    ClassBoard.plansza[x + 1][y] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    x++;

                } else if((ClassBoard.plansza[x + 1][y] == 2) && (ClassBoard.plansza[x + 2][y] == 1)){

                    ClassBoard.plansza[x + 2][y] = 2;

                    ClassBoard.plansza[x + 1][y] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    x++;

                } else if(ClassBoard.plansza[x + 1][y] == 1){

                    ClassBoard.plansza[x + 1][y] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    x++;

                } else if(ClassBoard.plansza[x + 1][y] == 4){

                    ClassBoard.plansza[x + 1][y] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    x++;

                }

                break;

            case KeyEvent.VK_LEFT:

                System.out.println("wcisles left");

                if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y - 2] == 2)){

                    ClassBoard.plansza[x][y - 2] = 2;

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y--;

                } else if((ClassBoard.plansza[x][y - 1] == 2) && (ClassBoard.plansza[x][y - 2] == 4)){

                    ClassBoard.plansza[x][y - 2] = 2;

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y--;

                } else if((ClassBoard.plansza[x][y - 1] == 2) && (ClassBoard.plansza[x][y - 2] == 1)){

                    ClassBoard.plansza[x][y - 2] = 2;

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y--;

                } else if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y - 2] == 4)){

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y--;

                } else if(ClassBoard.plansza[x][y - 1] == 4){

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    y--;

                } else if(ClassBoard.plansza[x][y - 1] == 1){

                    ClassBoard.plansza[x][y - 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y--;

                }

                break;

            case KeyEvent.VK_RIGHT:

                System.out.println("wcisles right");

                if((ClassBoard.plansza[x][y - 1] == 2) && (ClassBoard.plansza[x][y - 2] == 0) && (ClassBoard.plansza[x - 1][y] == 2) && (ClassBoard.plansza[x - 1][y - 1] == 1)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 2) && (ClassBoard.plansza[x][y - 2] == 0) && (ClassBoard.plansza[x - 1][y] == 1)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y - 2] == 0)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y + 1] == 1)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y - 2] == 3)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 2) && (ClassBoard.plansza[x][y - 2] == 0)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 0) && (ClassBoard.plansza[x][y + 1] == 4)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 4;

                    y++;

                } else if((ClassBoard.plansza[x][y - 1] == 4) && (ClassBoard.plansza[x][y - 2] == 4)){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if((ClassBoard.plansza[x][y + 1] == 2) && (ClassBoard.plansza[x][y + 2] == 1)){

                    ClassBoard.plansza[x][y + 2] = 2;

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                } else if(ClassBoard.plansza[x][y + 1] == 1){

                    ClassBoard.plansza[x][y + 1] = 3;

                    ClassBoard.plansza[x][y] = 1;

                    y++;

                }

                break; 

        }

        classBoard.repaint();

        ClassBoard.checkingFinish();

    }


    @Override

    public void keyReleased(KeyEvent e) {

    }

}

/*

 * To change this template, choose Tools | Templates

 * and open the template in the editor.

 */

package javaswingsokoban;


import java.awt.Dimension;

import java.awt.Point;

import javax.swing.JFrame;

import javax.swing.SwingUtilities;


/**

 *

 * @author marcin kaszuba

 */

public class JavaSwingSokoban{


    /**

     * @param args the command line arguments

     */

    public static void main(String[] args) {

        SwingUtilities.invokeLater(new Runnable() {


            @Override

            public void run() {

                JFrame jFrame = new JFrame();

                int widthWindow = 660;

                int heightWindow = 500;

                jFrame.setTitle("sokoban !!");

                jFrame.setLocation(new Point(100, 100));

                jFrame.setSize(new Dimension(widthWindow, heightWindow));

                jFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

                jFrame.setResizable(false);

                jFrame.setVisible(true);

                ClassBoard classBoard = new ClassBoard();

                jFrame.add(classBoard);

            }


        });

    }

}[/code]

grzelix · 19 Luty 2013 16:42

Pisząc, że prosisz uwagi odnośnie kodu nie wiem do końca o jaki stopień uwag prosisz, ale skoro w drugim wątku uznałeś za marną moją odpowiedź postaram się tutaj napisać nieco więcej.

Magic Numbers - w kodzie używasz liczb które określają rodzaj kafelka na planszy. Same liczby nie wiele mówią i czytając taki kod trzeba co chwilę wracać do ich definicji która jest w komentarzu. Bardzo zła praktyka. Cytując Roberta Martina (Clean Code) - jeśli musiałeś napisać komentarz, żeby wyjaśnić co się dzieje w twoim kodzie, to znaczy że źle napisałeś ten fragment i musisz go poprawić.
Skalowalność - nie w ogólności (do tego jeszcze dojdziemy) ale pod względem innej planszy wejściowej. W tym momencie masz tylko jedną planszą z tylko jednym możliwym ustawieniem. Co gorsza nie tyle plansza jest stała ale nawet funkcja sprawdzająca koniec gry jest stała. Metodę checkingFinish można spokojnie wyliczyć na podstawie początkowej tablicy.
Czytelność warunków -

if((ClassBoard.plansza[x - 1][y] == 2) && (ClassBoard.plansza[x - 2][y] == 1))

(pomijając nadmiarowość nawiasów - tu już bardzo się czepiam ) Co ta linijka mi mówi, kiedy widzę ją pierwszy raz (albo kolejny po weekendzie, po miesiącu). Kompletnie nic. Nie bardzo mam siłę ją analizować ale powiedzmy że chodzi o to czy pole wyżej jest puste. Stworzyłbyś metodą zwracającą bool o nazwie IsEmptyTileUpHere(Point currentSquare) i wtedy wiadomo że sprawdzasz po wciśnięciu strzałki w górę czy jest tam wolne pole i możesz się tam przesunąć. 4. Metoda keyPressed w klasie ClassMove ma 130 linii. We wspomnianej książce Roberta Martina jest zalecenie aby metoda mieściła się na jednym ekranie monitora. (Kiedyś było to 8 linijek.) 5. Wracając do skalowalności - twoja aplikacja narusza zasadę Open\Close Principle. Otwarta na rozszerzenia, zamknięta na modyfikację. Dlaczego? Ponieważ załóżmy, że dodajesz nowy rodzaj kafelka. Teraz jest mnóstwo miejsc gdzie musisz dokonać zmian. Choćby w metodzie rysującej. Poprawnie zaprojektowana aplikacja (zgodnie z tą zasadą) powinna pozwolić na dodanie tego rozszerzenie przez napisanie nowej klasy i dodanie jej gdzieś w konfiguracji (ewentualnie konstruktorze). Należałoby stworzyć interfejs ITile z metodą drawTile(); - odpowiednie parametry i w metoda drawBoard wyglądała by tak:

void drawBoard(Graphics graphics){

        super.paint(graphics);

        for(int i = 0; i < plansza.length; ++i){

            for(int j = 0; j < plansza[0].length; ++j){


                plansza[i][j].drawTile();

        }

    }

wówczas zmienna plansza byłaby tablicą typu ITile. Podobną metodę można napisać do warunków przesuwanie np ściana by była nie przesuwalna a płytka przesuwalna miałaby wartość pozwalającą na przesuwanie. Tutaj zadziała by zasada zwana zasadą Hollywood Don’t call us we call you. Jest też wykorzystanie polimorfizmu zawartego w języku Java. Ok, może na razie wystarczy. Szkoda że piszesz w Javie, ponieważ ja zajmuję się .Net’em i może pokusiłbym się refactoring tego kodu.To o czym pisałem powyżej na razie staram się wdrażać we własnych projektach i jeszcze daleko mi do bycia ekspertem w dziedzinie architektury aplikacji, ale mam nadzieję że znajdziesz coś w tym pożytecznego dla siebie. Jeśli zainteresowała się ta tematyka to polecam zapoznanie się z zasadami programowania SOLID i prócz wspomnianej książki Clean Code polecam jeszcze pozycję The Pragmatic Programmer: From Journeyman to Master Jeśli gdzieś popełniłem ewidentny błąd bądź jest coś nie jasne w moich sformułowaniach bardzo chętnie dopowiem\zweryfikuję. EDIT: Jeszcze dwa słowa odnośnie asercji i sposobu używania jej. W kodzie masz taką asercję:

System.out.println("wcisles down");

Słowo wcisles nie występuje ani w polskim ani angielskim słowniku. Taki potworek bardzo kuje w oczy. Może wydać się mało ważne co wpisałeś jako asercja do sprawdzenie metody, ale lepiej mieć dobry nawyki. Znam historię, w której firma programistyczna miała sprawę sądową ponieważ nie wyczyściła wszystkich asercji z kodu przed wysłaniem do klienta, a jako asercji używali słów ogólnie uznawanych za wulgarne. 2. Do takich asercji powinieneś użyć metody statycznej klasy debug np

Debug.println(“Method keyPressed”, “KeyEvent.VK_UP”);

Wówczas kompilując projekt w trybie Release te “śmieci” nie będą wypisywane na konsole - i tak powinno być.

EnterBioS · 21 Luty 2013 20:50

Pytasz o uwagi i ocenę kodu jako amator? Czy profesjonalista?

Jeżeli jako amator to super! Fajna gierka! Działa i w ogóle

Jeżeli jako profesjonalista to gorzej…

Oprócz tego co wypisał kolega wyżej dołożę kilka kolejnych kujących w oczy rozwiązań których użyłeś, a które z clean codem nie mają nic wspólnego.

Umieszczasz logikę wewnątrz wywołań metod:

graphics.fillRect((sizeSquare * j) + positionX, (sizeSquare * i) + positionY, sizeSquare, sizeSquare);

Nie rób tego. Będzie czytelniej jak wydzielisz te obliczenia do zmiennej lokalnej. A w tym konkretnym przypadku całkowicie do innej funkcji - zaoszczędzisz sobie pisania i zmniejszysz duplikację kodu (powód wszelkiego zła na świecie). 2. Dużo nienazwanej logiki wewnątrz warunków:

if((plansza[1][1] == 2) && (plansza[1][2] == 2) && (plansza[2][1] == 2) && (plansza[2][2] == 2 &&

                (plansza[3][1] == 2) && (plansza[3][2] == 2)) && (plansza[4][1] == 2) && (plansza[4][2] == 2) &&

                (plansza[5][1] == 2) && (plansza[5][2] == 2))

Gwarantuje Ci że za miesiąc będziesz przez pół godziny próbował odszyfrować co sprawdza ten warunek. 3. Pomieszanie struktury danych i obiektu:

//0 - mur czarny

    //1 - puste pole bialy

    //2 - pudelko zielony

    //3 - ludzik czerwony

    //4 - pole końcowe

    static int[][] plansza = {

        {0,0,0,0,0,0,0,0,0,0,0,0,1,1},

        {0,4,4,1,3,0,1,1,1,1,1,0,0,0},

        {0,4,4,1,1,0,1,2,1,1,2,1,1,0},

        {0,4,4,1,1,0,2,0,0,0,0,1,1,0},

        {0,4,4,1,1,1,1,1,1,0,0,1,1,0},

        {0,4,4,1,1,0,1,0,1,1,2,1,0,0},

        {0,0,0,0,0,0,1,0,0,2,1,2,1,0},

        {1,1,0,1,2,1,1,2,1,2,1,2,1,0},

        {1,1,0,1,1,1,1,0,1,1,1,1,1,0},

        {1,1,0,0,0,0,0,0,0,0,0,0,0,0}

    };

To powinno być wydzielone do struktury danych, albo do obiektu który zamknie hermetycznie implementacje i udostępni tylko niezbędny interfejs.

W twoim kodzie klasa ClassBoard to tak zwana hybryda:

"Hybrids

This confusion sometimes leads to unfortunate hybrid structures that are half object and

half data structure. They have functions that do significant things, and they also have either

public variables or public accessors and mutators that, for all intents and purposes, make

the private variables public, tempting other external functions to use those variables the

way a procedural program would use a data structure.4

Such hybrids make it hard to add new functions but also make it hard to add new data

structures. They are the worst of both worlds. Avoid creating them. They are indicative of a

muddled design whose authors are unsure of—or worse, ignorant of—whether they need

protection from functions or types."

Łamiesz wszędzie regułę “Do one thing”. Metoda ClassMove.keyPressed(KeyEvent) ewidentnie nie wykonuje jednej czynności tylko wiele.

Dodatkowo jest bardzo nieczytelna. Jeżeli zamiast 0,1,2,3,4 stworzysz enum mówiący o tym co jest na danej pozycji kod będzie o wiele czytelniejszy.

Mogę jeszcze wiele wypisać takich rzeczy, ale widzę że nie masz pojęcia o Clean Code, więc także nie zrozumiesz o czym piszę. Polecam książkę wujka Bob’a (Robert C. Martin). Jak przeczytasz to popraw swój kod i wklej znowu, wtedy podyskutujemy

anon89827741 · 21 Luty 2013 21:07

marcinkk , proszę zapoznaj się z tą stroną oraz tym tematem, a następnie, używając przycisku image.php?album_id=20&image_id=4038