8

I am just learning java and have to do something using the java swing library, and the Graphics2D class. Basically I have to draw a construction crane that has multiple parts: a body (the body of the crane), and a number of attached arms (basically it looks like this: https://i.stack.imgur.com/raBxN.jpg).

My question revolves around if I am using the Java swing class correctly? In my code below, I left out unnecessary code, as I just want to make sure my structure is correct (Using JPanel, paintComponent(), etc. correctly). Any help would be appreciated, as I am just learning Java! Thanks guys.

public class CraneSimulator {

    ...
    public JFrame frame;
    public MyPanel panel;

    public CraneSimulator() {
        frame = new JFrame("CraneSimulator");

        ...

        panel = new MyPanel();
        frame.add(panel);

    }

    public static void main(String[] args) {
        CraneSimulator simulator = new CraneSimulator();    
    }
}   

class MyPanel extends JPanel {
    CraneBody body;
    CraneArm arm1;
        ...
    Graphics2D graphics;

    public MyPanel() {
        body = new CraneBody();
        arm1 = new CraneArm(body);
        ...
        addMouseListener(new MouseAdapter() {
            public void mousePressed(MouseEvent e) {
                ...         }
            }
            public void mouseReleased(MouseEvent e) {
                    ...
            }
        });

        addMouseMotionListener(new MouseAdapter() {
            public void mouseDragged(MouseEvent e) {
                ...             
            }
        });
    }

    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        graphics = (Graphics2D) g;
        ...
        body.paint(g);      
        arm1.paint(g);  
    }
}

class CraneBody {
    ...

    public CraneBody() {
         ....
    }
    ...
    public void paint(Graphics g) {
        Graphics2D g2 = (Graphics2D) g;

            // Use g2 to actual paint crane Body on screen here (ie. g2.drawRect, etc)      
    }
}

class CraneArm {
    ...

    public CraneArm() {
         ....
    }
    ...
    public void paint(Graphics g) {
        Graphics2D g2 = (Graphics2D) g;

            // Use g2 to actual paint the crane armon screen here (ie. g2.drawRect, etc)        
    }
}
Tesla
  • 822
  • 2
  • 13
  • 27
  • 3
    YES! Oh my lordy YES! (Sorry, so many people doing it wrong :P). I would, however, suggest that you build your crane so that each crane arm is a child of the previous. This means that you could (in theory), compound the rotation translational simply by passing the `Graphics` context to each child in the chain. Basically it would look something like `crane.getArm().getArm().getArm()` ... until you ran out of arms - but that's just me. +1 for actually using Swing Graphics correctly :D – MadProgrammer Feb 08 '13 at 06:11
  • 8
    Don't declare a Graphics class field as this invites problems if you decide to use it inadvertently. This variable should be local only. – Hovercraft Full Of Eels Feb 08 '13 at 06:13
  • 2
    @HovercraftFullOfEels Nice pick up, didn't see that – MadProgrammer Feb 08 '13 at 06:14
  • You have no idea how relieved I am to hear that haha. It's a bit stressful learning Java (having been taught c++ the first 3 years at school), and I wasn't quite sure if I was grasping everything. Follow up question if you guys don't mind: Lets say I want to drag the 3rd arm from the left and move it up and down. Where would I set this rotation? The hit test (checking to see if the mouse was clicked within an arm) is done in MyPanel, so how would I propagate the necessary rotation to the correct arm? – Tesla Feb 08 '13 at 06:15
  • Basically, I would try and setup the arms in a chain of some kind (either as child objects of each other or in a list). From here you could calculate the how far a single arm might be able to be moved and based on the influence, change the rotation of the other arms. Personally, I would make each arms pivot point the point where it joins it's parent, so you try and rotate the 3rd arm, it would pivot about the point where it joins the 2nd - IMHO – MadProgrammer Feb 08 '13 at 06:21
  • Okay cool. So if my if statement within MousePressed and MouseDragged determines that the mouse was clicked within the arm (the arms just rotate in a circle around the pivot point), would I just apply the rotation (graphics.rotate(..)) within this if statement? I guess I'm just confused on how I would make sure the rotation is done only for that one arm, and not the rest of the drawing. – Tesla Feb 08 '13 at 06:28
  • @Tesla I wouldn't. This would rotate the entire graphics context, which isn't what you want. Instead, when you need to paint a particular arm, get it's required rotation/angel and use an [AffineTransformation](http://docs.oracle.com/javase/tutorial/2d/advanced/transforming.html). This will allow you to isolate each arm's rotation. Done correctly, you could compound the rotations, but that's going to be fun from the perspective positioning ;) – MadProgrammer Feb 08 '13 at 06:32
  • Hmm, so would something along the lines of having a variable (rotationDegrees) for each arm, and the hit test in my MouseDragged calls a setRotationDegrees() function, which sets the necessary degree (based on the mouse dragging movement), and then when painting the arm, apply the rotation (using AffineTransformation) based on this variable? – Tesla Feb 08 '13 at 06:36
  • @Tesla That sounds correct. What you need to decide is, would each angle be compound of the last or an absolute value – MadProgrammer Feb 08 '13 at 06:41
  • 2
    Would this question not be better suited at http://codereview.stackexchange.com/? I tend to think so as nothing is actually going wrong/giving an error rather OP is asking how correct the code is... – David Kroukamp Feb 08 '13 at 08:51

2 Answers2

3

Your code is well-structured and follows excellent practice using Java Graphics as well as OOP.

As suggested in the comment, it is better to define your Graphics object local if you do not have a reason to make it an instance variable.

iTech
  • 18,192
  • 4
  • 57
  • 80
  • 1
    Note that the field that reference `graphics` should actually be removed. Keeping that reference is actually a really bad practice. – Guillaume Polet Feb 08 '13 at 10:10
1

Your code is almost perfect. But several suggestions:

  1. Initialization on declaration has some advantages;
  2. You just need one MouseAdapter;
  3. Use Graphics2D as argument so you don't need to cast it from Graphics again.
  4. Remove the field "graphics", instead, make it a local variable. (Thanks to @GuillaumePolet).

Some people may disagree, but according to your code, I would do these changes to make it neater.

public class CraneSimulator {
    ...
    private JFrame frame = new JFrame("CraneSimulator");
    private MyPanel panel = new JPanel();

    public CraneSimulator() {
        ...
        frame.add(panel);
    }
    public static void main(String[] args) {
        CraneSimulator simulator = new CraneSimulator();    
    }
}   

class MyPanel extends JPanel {
    CraneBody body = new CraneBody();
    CraneArm arm1 = new CraneArm(body);
    ...
    MouseAdapter mAdapter = new MouseAdapter() {
        public void mousePressed(MouseEvent e) {
            ...
        }
        public void mouseReleased(MouseEvent e) {
            ...
        }
        public void mouseDragged(MouseEvent e) {
            ...             
        }
    }

    public MyPanel() {
        ...
        addMouseListener(mAdapter);
        addMouseMotionListener(mAdapter);
    }

    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        Graphics2D graphics = (Graphics2D) g;
        ...
        body.paint(graphics);
        arm1.paint(graphics);  
    }
}

class CraneBody {
    ...
    public CraneBody() {
         ....
    }
    ...
    public void paint(Graphics2D g) {
        // You don't need to cast a Graphics again.      
    }
}

class CraneArm {
    ...
    public CraneArm() {
         ....
    }
    ...
    public void paint(Graphics2D g) {
        // You don't need to cast a Graphics again.
    }
}
Community
  • 1
  • 1
shuangwhywhy
  • 5,475
  • 2
  • 18
  • 28
  • 2
    Note that the field that reference `graphics` should actually be removed. Keeping that reference is actually a really bad practice. – Guillaume Polet Feb 08 '13 at 10:11