Very Basic Java Piano

I've been reading "Head First Java" and some of the early projects surround basic swing and basic MIDI.

So I've put the two together and written a basic "Synthesiser"/"Piano" using what I have learned.

I know that I need to add some safety features and there's probably some method optimisation staring me in the face but would love to know what you all think!

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

public class VirtualKeyboard implements ActionListener {

public static ArrayList note = new ArrayList();
public static ArrayList extendedNote = new ArrayList();
JTextField octaveChoice;
JTextField instrumentChoice;

public static void main (String[] args) {
VirtualKeyboard gui = new VirtualKeyboard();

String[] preNote = {"C","D","E","F","G","A","B"};
String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};

for (int i = 0; i < 7; i++) {
note.add(preNote[i]);
}

for (int i = 0; i < 12; i++) {
extendedNote.add(preExtendedNote[i]);
}

gui.setGUI();
}

public void setGUI() {
JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
JPanel keyPanel = new JPanel();
JPanel controlPanel = new JPanel();

JButton[] key = new JButton[7];

for (int i = 0; i < 7; i++) {
key[i] = new JButton(note.get(i));
key[i].addActionListener(this);
keyPanel.add(key[i]);
}

JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
instrumentChoice = new JTextField(1);

JLabel octaveChoiceLabel = new JLabel("Octave: ");
octaveChoice = new JTextField(1);

controlPanel.add(instrumentChoiceLabel);
controlPanel.add(instrumentChoice);
controlPanel.add(octaveChoiceLabel);
controlPanel.add(octaveChoice);

frame.getCon

import javax.swing.*;
import javax.sound.midi.*;
import java.awt.*;
import java.awt.event.*;

Most style guides will frown upon the wildcard import -- is it so much extra work to list the classes you actually depend on? This will also help you to avoid conflicts, where you want List to mean java.util.List rather than java.awt.List

public static ArrayList note = new ArrayList();
public static ArrayList extendedNote = new ArrayList();

ArrayList is an implementation detail, that consumers generally don't need to know about. The more usual declaration would look like:

public static List note = new ArrayList();
public static List extendedNote = new ArrayList();

Furthermore, by specifying that these are lists, you are claiming that they support insert. Is it really a good idea to allow other code to start inserting new notes into your lists?

An alternative would be to expose the notes as Iterable, rather than List, which helps disguise the fact that the underlying container supports inserts. Better would be to make that data private, and provide methods that give access to the notes.

(Note: the spelling change of the variable name; this thing isn't a note, it's a collection of notes).

private static List notes = new ArrayList();

public static List getNotes() {
    return Collections.unmodifiableList(notes);
}

The best answer isn't clear, as your example doesn't include any external consumers.

As a rule, you should try to keep variable declarations near their use -- it makes the code clearer to read, and helps with refactoring the code later.

public static void main (String[] args) {

    String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < 7; i++) {
        note.add(preNote[i]);
    }

    String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};
    for (int i = 0; i < 12; i++) {
        extendedNote.add(preExtendedNote[i]);
    }

    VirtualKeyboard gui = new VirtualKeyboard();
    gui.setGUI();
}

Magic numbers are poor form; you should avoid them where possible.

String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < preNote.length; i++) {
        note.add(preNote[i]);
    }

You should also avoid writing code that has already been written for you.

String[] preNote = {"C","D","E","F","G","A","B"};
    List notes = Arrays.asList(preNote);

Or more directly

List note = Arrays.asList("C","D","E","F","G","A","B");

As an aside - it may make more sense to treat the notes as Enumerations, rather than Strings.

JButton[] key = new JButton[7];

    for (int i = 0; i < 7; i++) {
        key[i] = new JButton(note.get(i));
        key[i].addActionListener(this);
        keyPanel.add(key[i]);
    }

Two problems here -- you're repeating your magic number again; and also you are creating an array that you never use.

for (int i = 0; i < note.size(); i++) {
        JButton key = new JButton(note.get(i));
        key.addActionListener(this);
        keyPanel.add(key);
    }

But this is silly -- you put notes in a List, you should use it

for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

OK, let's get some real meat...

public void setGUI() {
    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
    JPanel keyPanel = new JPanel();
    JPanel controlPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
    instrumentChoice = new JTextField(1);

    JLabel octaveChoiceLabel = new JLabel("Octave: ");
    octaveChoice = new JTextField(1);

    controlPanel.add(instrumentChoiceLabel);
    controlPanel.add(instrumentChoice);
    controlPanel.add(octaveChoiceLabel);
    controlPanel.add(octaveChoice);

    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);
}

OK, what you are doing here is building an object graph -- creating instances of objects and establishing the relationships between them. That shouldn't happen in an object that also does anything else (separation of concerns).

The VirtualKeyboard is an ActionListener that plays sounds. So it shouldn't also be doing the job of building it's own object graph. Building things is the domain of Factory objects, so you should implement the creation of the object graph elsewhere.

But - it's not easy here. The action listener holds two of the objects that are needed to build the UI, but all the others are created somewhere else. The riddle is solved by passing the JTextFields to the `VirtualKey

StackExchange Code Review Q#58439, answer score: 10