How LOLbad is this stylistically?

[Luigi Plinge]

This is based on one of the examples in Head First Java... it plays some notes using MIDI, and each time a MIDI controller event is fired, it draws a colored rectangle on screen.

Instead of using their rather klugey method of drawing shapes, I created an inner class RectShape so that I could associate some random dimensions with a color, and also conveniently store them in an ArrayList, so that I can redraw the whole lot when the panel needs repainting.

The important bits are the bottom two sections of code.

2 questions about my RectShape class:

1) Is it really bad just to declare its fields and values like this, without using a constructor method?

2) Is it OK to use the fields directly in my JPanel's paintComponent(...) method, since it's an inner class? Or should I still use getters / setters (seems a bit long-winded).

To me it seems more elegant to make the concisest code possible, but you guys might disagree...

import javax.sound.midi.*;
import java.awt.*;
import java.util.ArrayList;
import javax.swing.*;

public class MiniMusicPlayer3 {
    JFrame frame;
    MyDrawPanel panel;
    Sequencer sequencer;
    ArrayList<RectShape> rectangles = new ArrayList<RectShape>();
    int WINDOW_DIMENSION = 1000;

    public static void main(String[] args) {
        new MiniMusicPlayer3().go();
    }

    public void go(){
        setUpGUI();
        try{
            sequencer = MidiSystem.getSequencer();
            sequencer.addControllerEventListener(panel, new int[] {127});
            Sequence seq = new Sequence(Sequence.PPQ, 16);
            Track tr = seq.createTrack();

            for (int i = 0; i <= 48; i += 4) {
                tr.add(makeEvent(ShortMessage.NOTE_ON, 1, i + 40, 100, i));
                tr.add(makeEvent(ShortMessage.CONTROL_CHANGE, 1, 127, 0, i));
                tr.add(makeEvent(ShortMessage.NOTE_OFF, 1, i + 40, 100, i + 2));
            }
            sequencer.open();
            sequencer.setSequence(seq);
            sequencer.setTempoInBPM(120);
            sequencer.start();
        }catch (Exception e){
        }
    }

    public static MidiEvent makeEvent(int comd, int chan, int one, int two, int tick){
        ShortMessage sm = new ShortMessage();
        try {
            sm.setMessage(comd, chan, one, two);
        } catch (Exception e) {
        }
        return new MidiEvent(sm, tick);
    }

    public void setUpGUI(){
        frame = new JFrame("Music Maker");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        panel = new MyDrawPanel();
        frame.add(panel);

        frame.setSize(WINDOW_DIMENSION, WINDOW_DIMENSION);
        frame.setVisible(true);
    }

    class MyDrawPanel extends JPanel implements ControllerEventListener{
        @Override
        public void paintComponent(Graphics g){
            super.paintComponent(g);
            Graphics2D g2 = (Graphics2D) g;

            for (RectShape r:rectangles){     // Use inner classes fields in this for loop
                g2.setColor(r.color);
                g2.fillRect(r.x1, r.y1, r.width, r.height);
            }
        }
        public void controlChange(ShortMessage message){
            rectangles.add(new RectShape());
            panel.repaint();
        }
    }

    class RectShape{                                       // My custom methodless inner class...
        int width = (int) (Math.random()*WINDOW_DIMENSION/2);
        int height = (int) (Math.random()*WINDOW_DIMENSION/2);
        int x1 = (int) (Math.random()*(WINDOW_DIMENSION - width));
        int y1 = (int) (Math.random()*(WINDOW_DIMENSION - height));
        Color color = new Color((int)(Math.random()*256), (int)(Math.random()*256),
                    (int)(Math.random()*256), (int)(Math.random()*128 + 128));
    }
}

[Ernest Friedman-Hill]

For #2, that's really fine as it stands, there's nothing wrong with that. It's pretty much expected that inner classes will directly exchange data with their parents, in one direction or another.

For #1, that's a little harder. Normally, there's no reason not to initialize data members directly if possible, but here, those expressions are awfully long, and makes me a little uncomfortable seeing that big solid block of code at class scope. If it were me, I might define some methods to make those expressions shorter -- for example, a method random(int) that returned a random number less than the given value.

Further, it strikes me as somewhat inconvenient that there's no way to make a "plain" or "default" RectShape, but only to make one containing random values. It would be nice to be able to make one with standard values for testing, for example. This would suggest having multiple constructors: one that makes a random RectShape, and one that initializes to a given list of values.

[Luigi Plinge]

Thanks for the comments. I changed the RectShape class as you suggested:

    class RectShape{
        int width, height, x1, y1;
        Color color;

        public RectShape(){
            width = random(WINDOW_DIMENSION/2);
            height = random(WINDOW_DIMENSION/2);
            x1 = random(WINDOW_DIMENSION - width);
            y1 = random(WINDOW_DIMENSION - height);
            Color color = new Color(random(256), random(256), random(256), random(128) + 128);
        }

        public RectShape(int width, int height, int x1, int y1, Color color){
            this.width = width;
            this.height = height;
            this.x1 = x1;
            this.y1 = y1;
            this.color = color;
        }

        int random(int i){
            return (int)(Math.random() * i);
        }
    }

Netbeans is alerting me that I have an "overridable method call in constructor" for the random() method... not sure if I should be worried about that but it seems to run OK...

[Luigi Plinge]

I just put a "final" in front of the int random(..) and that gets rid of the warnings...

[Ernest Friedman-Hill]

The warning is telling you that if you were to define a subclass of RectShape, and it were to override random() in such a way that it used the chld class's member variables, then that override could be called at a point before those member variables were initialized -- which is potentially bad. You can make the warning go away by making this impossible; make RectShape final, make random() final, or make random() private. Any of these would prevent random() from being overridden.

[Ernest Friedman-Hill]

Luigi Plinge wrote:I just put a "final" in front of the int random(..) and that gets rid of the warnings...

Looks like you figured out the solution while I was explaining it!

[Luigi Plinge]

Google is my friend

[Campbell Ritchie]

Ernest Friedman-Hill wrote: . . . that override could be called . . . which is potentially bad. . . . make random() final, or make random() private. . . .

Somebody else had a similar problem, which you can read about here.