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<String> note = new ArrayList<String>();
public static ArrayList<String> extendedNote = new ArrayList<String>();
ArrayList is an implementation detail, that consumers generally don't need to know about. The more usual declaration would look like:
public static List<String> note = new ArrayList<String>();
public static List<String> extendedNote = new ArrayList<String>();
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<String> notes = new ArrayList<String>();
public static List<String> 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<String> notes = Arrays.asList(preNote);
Or more directly
List<String> 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 VirtualKeyboard, rather than the other way around....
private final JTextField octaveChoice;
private final JTextField instrumentChoice;
public VirtualKeyboard(JTextField octaveChoice, JTextField instrumentChoice) {
this.octaveChoice = octaveChoice;
this.instrumentChoice = instrumentChoice;
}
So we need the JTextField before we create VirtualKeyboard, which means that we need to create those objects in main(). This is actually pretty common -- main is a good place to put the object assembly you need when bootstrapping your application.
public static void main (String[] args) {
JTextField instrumentChoice = new JTextField(1);
JTextField octaveChoice = new JTextField(1);
VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);
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(gui);
keyPanel.add(key);
}
JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
JLabel octaveChoiceLabel = new JLabel("Octave: ");
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);
The code should be re-organized so that you can see how the parts relate....
public static void main (String[] args) {
JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
JPanel controlPanel = new JPanel();
JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
controlPanel.add(instrumentChoiceLabel);
JLabel octaveChoiceLabel = new JLabel("Octave: ");
controlPanel.add(octaveChoiceLabel);
JTextField instrumentChoice = new JTextField(1);
controlPanel.add(instrumentChoice);
JTextField octaveChoice = new JTextField(1);
controlPanel.add(octaveChoice);
VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);
frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
JPanel keyPanel = new JPanel();
for (String currentNote : note) {
JButton key = new JButton(currentNote);
key.addActionListener(gui);
keyPanel.add(key);
}
frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
frame.setSize(500,200);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setVisible(true);
It's common to create classes that do all this work -- done well, it tends to improve both testability and re-use.
Let's look at the action code again
public void actionPerformed(ActionEvent e) {
int finalNote = 0;
Object source = e.getSource();
if (source instanceof JButton) {
JButton but = (JButton) source;
System.out.print(but.getText());
finalNote = (Integer.parseInt(octaveChoice.getText()) * 12) + extendedNote.indexOf(but.getText());
System.out.print(" (" + finalNote + ")\n");
playNote(finalNote, Integer.parseInt(instrumentChoice.getText()));
}
}
There are at least three things going on here...
public void actionPerformed(ActionEvent e) {
Object source = e.getSource();
if (source instanceof JButton) {
JButton but = (JButton) source;
String note = but.getText();
String octave = octaveChoice.getText();
String instrument = instrumentChoice.getText();
int finalNote = (Integer.parseInt(octave) * 12) + extendedNote.indexOf(note);
int finalInstrument = Integer.parseInt(instrument);
playNote(finalNote, finalInstrument);
}
}
You've got the ActionListener, which is reading Strings out of awt, you've got a Parser, which knows how to turn those strings into notes and instruments, and you've got a Player that knows how to turn notes and instruments into sounds.
This separation is important, because you should be able to do test that the parser does the right thing without assembling the entire UI for it. You should be able to replace this player with another one that uses a different set of magic numbers to generate ShortMessages.
