2
\$\begingroup\$

I tried to create a Game of Life copy with following OOD principles. What should I change in my code to make it cleaner?

import java.awt.Color;
import java.awt.Point;

public class Square {

    public Point coordinate;
    public static int squareDimension = 10;

    public Square (int x, int y) {
        coordinate = new Point(x, y);   
    }

}

The class that makes all computations:

import java.awt.Point;
import java.util.ArrayList;

public class Board {

    public int sqrDimension ;
    public int[][] allBoard;
    public ArrayList<Point> emptySquare;
    public ArrayList<Square> filledSquare;
    public ArrayList<Square> temporarySquare;
    public ArrayList<Square> sqrWillLive;

    public Board() {
        sqrDimension = Square.squareDimension;
        allBoard = new int[200][200];
        filledSquare = new ArrayList<Square>();
        temporarySquare = new ArrayList<Square>();
        sqrWillLive = new ArrayList<Square>();
        emptySquare = new ArrayList<Point>();
    }

    public void move () {     
        increaseOneNghbrd();
        theFutureOfFilledSquare();
        theFutureOfEmptySqr();  
        makeBoardEmpty();       
    }

    // adding one to value of neighberhood of filled square
    public void increaseOneNghbrd() {

      int sideOfBoard = allBoard.length -1;
      for (Square kareler : filledSquare) {
          // coordinate of filled square
           int x = kareler.coordinate.x;
           int y = kareler.coordinate.y;       

         if (x > 0 && x <sideOfBoard && y > 0 && y < sideOfBoard) {

             for (int i = -1; i < 2; i++) {          
                for (int j = -1; j < 2; j++) {                      
                     allBoard[x+j][y+i]++;              
                     addIntoEmpty(x+j, y+i);
                }
             }  
             // decrease value of filled square  
             // cause its increased as it is nghbrhood of itself
             allBoard[x][y]--;
         }
       }   
    }

    // neighberhood square is added to array 
    public void addIntoEmpty (int x, int y) {
        boolean kopya = true;
        //checking if it is in array or not
         for (Point kareler : emptySquare) {
                if (kareler.x == x && kareler.y == y) {
                    kopya = false;
                    break;
                }
         }
         // checking  if this square is filled square or empty sqr 
         if (kopya) {
             for (Square kareler1 : filledSquare) {
                 if (kareler1.coordinate.x == x && kareler1.coordinate.y == y) {
                     kopya = false;                     
                     break;
                 }  
             }
        }
         //if it is not added  before and not filled sqr then it is added
         if (kopya)     {
             emptySquare.add(new Point(x, y)) ;     
         }
      }


    //putting coordinate of mouse clicking
    public void addSquare(int x, int y){
        boolean kopya = true;
        //checking if clicked to filled square 
        for (Square kareler : filledSquare) {
             if (kareler.coordinate.x == x && kareler.coordinate.y == y) {
                 kopya = false;
                 break;
             }  
         }
        if (kopya) {
            filledSquare.add(new Square(x, y));
            temporarySquare.add(new Square(x, y));
        }
    }

    // values of each cell will return 0 after each move
    public void makeBoardEmpty () {
        allBoard  = new int[200][200];
    } 

    // filled sqr will die or live depending on its value
    public void theFutureOfFilledSquare () {
        // adding sqr that is filled and has enough ngbhrhd to live to temprry array
        for (Square kareler : filledSquare) {
            int komsu =  allBoard[kareler.coordinate.x][kareler.coordinate.y] ;
            if (komsu == 2 || komsu == 3) {
                sqrWillLive.add(kareler);           
            }
        }   
        // removing all filled square
        filledSquare.clear();
        temporarySquare.clear();
        // square that has right to live is added to array again
        for (Square point : sqrWillLive) {
            filledSquare.add(point);
            temporarySquare.add(point);
        }
        sqrWillLive.clear();
    }

    // empty array will be added to filled sqr array if it has 3 ngbhrhd
    public void theFutureOfEmptySqr () {

        for (Point kareler : emptySquare) {     
            int komsu =  allBoard[kareler.x][kareler.y] ;       
            if (komsu == 3) {           
                filledSquare.add(new Square(kareler.x, kareler.y));
                temporarySquare.add(new Square(kareler.x, kareler.y));
            }
        }
        emptySquare.clear();
    }

 }

Drawing class:

import java.awt.Color;
import java.awt.Graphics;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import java.awt.event.MouseMotionListener;

import javax.swing.JFrame;
import javax.swing.JPanel;

public class DrawingBoard extends JPanel implements MouseListener, KeyListener,MouseMotionListener{

    private static final long serialVersionUID = 1L;
    public JFrame cerceve;
    public Board board;
    int squareDimension;
    int SquareFrame ;

    public DrawingBoard () {
        board = new Board();
        cerceve  = new JFrame();
        cerceve.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setPreferredSize(getMaximumSize());
        addMouseListener(this);
        addMouseMotionListener(this);
        addKeyListener(this);
        setFocusable(true);
        cerceve.add(this);
        cerceve.setPreferredSize(getMaximumSize());
        cerceve.setResizable(false);
        cerceve.pack();
        cerceve.setVisible(true);

        squareDimension = board.sqrDimension;
        SquareFrame = (board.sqrDimension+1);
    }

    public void paintComponent (Graphics g) {
        super.paintComponent(g);
              karelericiz(g);
    }

    public void karelericiz(Graphics g) {
        g.setColor(Color.ORANGE);   
        for (Square kare : board.filledSquare) {    
            g.fillRect(kare.coordinate.x*SquareFrame+1,
                    kare.coordinate.y*SquareFrame+1,
                    board.sqrDimension, board.sqrDimension);
        }
    }

    public static void main(String[] args) {
        DrawingBoard tahta1 = new DrawingBoard();      
    }

    public void mouseClicked(MouseEvent e) {
    }
    public void mousePressed(MouseEvent e) {
        int x = e.getX() / SquareFrame;
        int y = e.getY() / SquareFrame;

        board.addSquare(x, y);
        repaint();
    }
    public void mouseReleased(MouseEvent e) {
    }
    public void mouseEntered(MouseEvent e) {
    }
    public void mouseExited(MouseEvent e) {
    }

    public void keyTyped(KeyEvent e) {
    }
    public void keyPressed(KeyEvent e) {
        if (e.getKeyCode() == KeyEvent.VK_5) {
            board.move();   
            repaint();
        }   
    }
    public void keyReleased(KeyEvent e) {
    }
    public void mouseDragged(MouseEvent e) {
        int x = e.getX() / SquareFrame;
        int y = e.getY() / SquareFrame;

        board.addSquare(x, y);
        repaint();
    }
    public void mouseMoved(MouseEvent e) {
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Jan 17 '18 at 9:52
2
\$\begingroup\$

Thanks for sharing your code.

OOP

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

  • information hiding / encapsulation
  • single responsibility
  • separation of concerns
  • KISS (Keep it simple (and) stupid.)
  • DRY (Don't repeat yourself.)
  • "Tell! Don't ask."
  • Law of demeter ("Don't talk to strangers!")

class design

At the first glance this looks like a fair Data transfer Object (DTO). It combines the library DTO Point with the squares diameter.

But the field squareDimension is static and does never change. Therefor it should be final too.

But if squareDimension is a constant it does not justify a separate (DTO) class.

Listener implementation by top level class

Your class DrawingBoard implements several *.Listener interfaces. But these interfaces are meant to be implemented as anonymous inner classes

Also this approach adds public methods to DrawingBoard which should not be accessed by users of this class and which are only there to satisfy the implemented interfaces.

Furthermore some of this additional methods are "empty".

The anonymous inner classes approach would reduce the public interface of DrawingBoard. Also you could uses the default implementations the JVM provides for (almost) all *.Listener interfaces to avoid "empty" methods:

  public DrawingBoard () {
        board = new Board();
        cerceve  = new JFrame();
        cerceve.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setPreferredSize(getMaximumSize());
        addMouseListener(new MouseAdapter(){ 
            public void mousePressed(MouseEvent e) {
               int x = e.getX() / SquareFrame;
               int y = e.getY() / SquareFrame;
               board.addSquare(x, y);
               repaint();
            }
        });
        addMouseMotionListener(new MouseAdapter(){ 
            public void  mouseDragged(MouseEvent e) {
               int x = e.getX() / SquareFrame;
               int y = e.getY() / SquareFrame;
               board.addSquare(x, y);
               repaint();
            }
        });
        addKeyListener(new KeyAdapter(){ 
            public void  keyPressed(KeyEvent e) {
               if (e.getKeyCode() == KeyEvent.VK_5) {
                 board.move();   
                 repaint();
               }    
            }
        });
       // ...

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

consistend language

Your method names sometimes are English (makeBoardEmpty) and your native language (karelericiz, kare). Stick to either one (preferably English).

avoid break to leave a loop

in your method addIntoEmpty() you have a "stack" of if statements wrapping different loops and depending on the fact that the previous loop finished or not (kopya). Instead you should use a return :

public void addIntoEmpty (int x, int y) {
    //checking if it is in array or not
     for (Point kareler : emptySquare) {
            if (kareler.x == x && kareler.y == y) {
                return;
            }
     }
     // checking  if this square is filled square or empty sqr 
         for (Square kareler1 : filledSquare) {
             if (kareler1.coordinate.x == x && kareler1.coordinate.y == y) {
                 return;
             }  
         }
     //if it is not added  before and not filled sqr then it is added
    emptySquare.add(new Point(x, y)) ;
  }
\$\endgroup\$
0
\$\begingroup\$

I changed some part for to make code more object oriented. Firstly i changed square class made their fields private and added some more field.

    import java.awt.Color;
import java.awt.Point;

public class Square {
    public static final Color aliveSqr = Color.ORANGE;
    public static final int squareWidth = 10;
    public static final int squareFrame = 11;
    public static final int[][] neighborhoods = { {-1,-1},{0,-1},{1,-1},
        {-1,0},{1,0},
        {-1,1},{0,1},{1,1} };
    private int totalNghbrhds;
    private Point coordinate;
    private boolean alive;
    private boolean added;


    public Square (Point nokta) {
        coordinate = new Point(nokta.x, nokta.y);
        totalNghbrhds = 0;
        alive = false;
        added = false;
    }

    public boolean isAlive () {
        return alive;
    }

    public boolean isAdded () {
        return added;
    }

    public int xEksen () {
        return coordinate.x;
    }

    public int yEksen () {
        return coordinate.y;
    }   

    public int nghbrhdsNumber () {
        return totalNghbrhds;
    }

    public void insert () {
        added = true;
    }

    public void addNghbrhds () {
        totalNghbrhds++;
    }

    public void returnİnital () {
        totalNghbrhds = 0;
        added = false;
    }

    public void spawn () {
        alive = true;
    }

    public void kill () {
        alive = false;
    }



}

board class hold int array to count neightborhoods in the last code. this time square class held neighborhoods and board class only held square class.

import java.awt.Point;
import java.util.ArrayList;

public class Board {
    private Square[][] allBoard;
    public ArrayList<Square> filledSquare;
    private ArrayList<Square> nghbrhdSquare;
    private ArrayList<Square> temporarySquare;


    public Board() {
        allBoard = new Square[200][200];
        for (int i = 0; i < allBoard.length; i++) {
            for (int j = 0; j < allBoard[0].length; j++) {
                allBoard[j][i] = new Square(new Point(j, i));
            }   
        }
        filledSquare = new ArrayList<Square>();
        nghbrhdSquare = new ArrayList<Square>();
        temporarySquare  = new ArrayList<Square>();
    }

    public void move () {
        increaseNghbrhd();
        nextStateOfFilledSquare();
        nextStateOfEmptySquare();
    }

    // ngbhrhd of alive sqr is increased
    private void increaseNghbrhd() {

          int boardWidth = allBoard.length -1;
           for (Square kareler : filledSquare) {
               int x = kareler.xEksen();
               int y = kareler.yEksen();       
            // checking square come to cross
             if (x > 0 && x <boardWidth && y > 0 && y < boardWidth)             // dolu karenin sekiz komsusunun degeri 1 arttırılıyor       
                 for (int[] kare : Square.neighborhoods) {
                     allBoard[x+kare[0]][y+kare[1]].addNghbrhds();
                     addIntoNgbhrhd(x+kare[0], y+kare[1]);
                }                                 

             }
           }       


    // square that is nghbrhd to alive sqr is put in array 
    private void addIntoNgbhrhd (int x, int y) {

        //checking it is in array or not
        if(allBoard[x][y].isAdded()) {
            return;
        }
         // checking this sqr is alive square or not
         if (allBoard[x][y].isAlive()) {
             return;
        }

         nghbrhdSquare.add(allBoard[x][y]);     
         allBoard[x][y].insert();
      }

    //putting points clicked by mouse
    public void addSquare(int x, int y) {
        int a = x / Square.squareFrame;
        int b = y / Square.squareFrame;
    // checking point is in alive square or not
         if (!allBoard[a][b].isAlive()) {
             allBoard[a][b].spawn();
             filledSquare.add(allBoard[a][b]);
        }
    }

    private void nextStateOfFilledSquare () {
        // alive square that will live in next state is put temporary array
        for (Square kareler : filledSquare) {
            int komsu =  kareler.nghbrhdsNumber();
            if (komsu == 2 || komsu == 3) {
                temporarySquare.add(kareler);       
            }
            else {
                kareler.kill();
            }
        //  making nghbrdhd number zero
            kareler.returnİnital();
        }   

        filledSquare.clear();
        // square that will live is put again alive squares array
        for (Square kare : temporarySquare) {
            filledSquare.add(kare);
        }
        temporarySquare.clear();
    }

    private void nextStateOfEmptySquare () {
    // cansız karenin 3 komsusu varsa dolukare arrayine konuyor     
        for (Square kareler : nghbrhdSquare) {      
            int komsu =  kareler.nghbrhdsNumber() ;     
            if (komsu == 3) {
                kareler.spawn();;
                filledSquare.add(kareler);
            }
    // komsu sayısı sıfırlanıyor        
            kareler.returnİnital();
        }
        nghbrhdSquare.clear();
    }

}

Drawing class has no change

import java.awt.Graphics;
import java.awt.event.KeyAdapter;
import java.awt.event.KeyEvent;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;

import javax.swing.JFrame;
import javax.swing.JPanel;

public class DrawingBoard extends JPanel{

    private static final long serialVersionUID = 1L;
    public JFrame cerceve;
    public Board tahta;

    public DrawingBoard () {
        tahta = new Board();
        cerceve  = new JFrame();
        cerceve.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setPreferredSize(getMaximumSize());
        addMouseListener(new MouseAdapter(){ 
                public void mousePressed(MouseEvent e) {
                   int x = e.getX() ;
                   int y = e.getY() ;
                   tahta.addSquare(x, y);
                   repaint();
                }
            });
            addMouseMotionListener(new MouseAdapter(){ 
                public void  mouseDragged(MouseEvent e) {
                    int x = e.getX() ;
                    int y = e.getY() ;
                    tahta.addSquare(x, y);
                    repaint();
                }
            });
            addKeyListener(new KeyAdapter(){ 
                public void  keyPressed(KeyEvent e) {
                   if (e.getKeyCode() == KeyEvent.VK_5) {
                     tahta.move();   
                     repaint();
                   }    
                }
            });
        setFocusable(true);
        cerceve.add(this);
        cerceve.setPreferredSize(getMaximumSize());
        cerceve.setResizable(false);
        cerceve.pack();
        cerceve.setVisible(true);       

    }

    public void paintComponent (Graphics g) {
          super.paintComponent(g);
          karelericiz(g);
}

public void karelericiz(Graphics g) {
    g.setColor(Square.aliveSqr);    
    for (Square kare : tahta.filledSquare) {    
        g.fillRect(kare.xEksen()*Square.squareFrame+1,
                kare.yEksen()*Square.squareFrame+1,
                Square.squareWidth, Square.squareWidth);
    }

}


    public static void main(String[] args) {
        DrawingBoard tahta1 = new DrawingBoard(); 
    }

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.