7
\$\begingroup\$

Just wanted to post this here to see if anyone has any critiques of my code, if I'm drawing the background image most efficiently, or if I'm doing anything which could benefit from obvious improvements.

RectangleMover

import java.awt.BorderLayout;
import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.Image;
import java.awt.event.MouseEvent;
import java.awt.event.MouseMotionListener;
import java.io.File;
import java.io.IOException;

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

public class RectangleMover {
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                final JFrame frame = new JFrame();

                frame.setSize(FRAME_WIDTH, FRAME_HEIGHT);
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setContentPane(new JPanel(new BorderLayout()) {
                    /**
                     * 
                     */
                    private static final long serialVersionUID = 1L;

                    public void paintComponent(Graphics g) {
                        try {
                            super.paintComponent(g);
                            final Image backgroundImage = javax.imageio.ImageIO
                                    .read(new File(
                                            "/Users/langer/Desktop/7923.jpg"));
                            g.drawImage(backgroundImage, 0, 0, FRAME_WIDTH,
                                    FRAME_HEIGHT, null);
                        } catch (IOException e) {
                            e.printStackTrace();
                        }
                    }
                });
                frame.setVisible(true);

                frame.setLocationRelativeTo(null);

                final RectangleComponent b = new RectangleComponent();
                frame.add(b);
                frame.addMouseMotionListener(new MouseMotionListener() {

                    public void mouseDragged(MouseEvent e) {
                        b.addRect(e.getX(), e.getY());

                        frame.setVisible(true);
                    }

                    public void mouseMoved(MouseEvent e) {
                    }

                });

            }
        });
    }

    private static final int FRAME_WIDTH = 800;
    private static final int FRAME_HEIGHT = 1000;
}

RectangleComponent

import java.awt.BasicStroke;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Rectangle;
import java.util.ArrayList;

import javax.swing.JComponent;

/**
 * This component displays a rectangle that can be moved.
 */
public class RectangleComponent extends JComponent {

    private static final long serialVersionUID = 6596614645871812990L;
    private ArrayList<Rectangle> boxes = new ArrayList<Rectangle>();

    public void paint(Graphics g) {
        Graphics2D g2 = (Graphics2D) g;
        super.paintComponent(g);

        for (int i = 0; i < boxes.size(); i++) {
            float thickness = 2;
            g2.setStroke(new BasicStroke(thickness));
            g2.drawRect(boxes.get(i).x, boxes.get(i).y - 29,
                    boxes.get(i).width, boxes.get(i).height);
        }
    }

    public void addRect(int x, int y) {
        boxes.add(new Rectangle(x, y, BOX_WIDTH, BOX_HEIGHT));
        repaint();
    }

    private static final int BOX_WIDTH = 42;
    private static final int BOX_HEIGHT = 60;
}
\$\endgroup\$
7
\$\begingroup\$

Don't load the same resource repeatedly

I don't have much time to give a full review but I can point out one big thing that I saw while looking through the code.

                public void paintComponent(Graphics g) {
                    try {
                        super.paintComponent(g);
                        final Image backgroundImage = javax.imageio.ImageIO
                                .read(new File(
                                        "/Users/langer/Desktop/7923.jpg"));
                        g.drawImage(backgroundImage, 0, 0, FRAME_WIDTH,
                                FRAME_HEIGHT, null);
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                }

You shouldn't load and parse the background image in the paint method. This method can be called frequently even though your background image doesn't change. If you try to resize your window you'll see how you reload the background several times per second slowing down your application.

Prefer to create a program instance

Doing so allows you to encapsulate variables to the class instead of keeping them in main or as inline. Also makes your code more readable.

public class RectangleMover {
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                final JFrame frame = new JFrame();

                frame.setSize(FRAME_WIDTH, FRAME_HEIGHT);
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setContentPane(new JPanel(new BorderLayout()) {
                    /**
                     * 
                     */
                    private static final long serialVersionUID = 1L;

should be something like this:

public class RectangleMover {

    private final Image background;

    public RectangleMover(){
        background = loadimage();
        final JFrame frame = new JFrame();
        ...
    }


    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                new RectangleMover();
            }});
    }
\$\endgroup\$
  • \$\begingroup\$ Would I be better off declaring the image within my main? \$\endgroup\$ – user2546963 Apr 27 '15 at 14:38
  • \$\begingroup\$ Better, yes. Good, no. It will work and will solve the problem but it is not recommended. You are doing too much inline in your main method it is difficult to read. I would recommend breaking out functions and creating an application instance. \$\endgroup\$ – Emily L. Apr 27 '15 at 14:45

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.