A few more comments

Author: ProfJanetDavis

rubric.md

@@ -1,13 +1,13 @@
-GRADE: 32/32
+GRADE: 31/32
 ------------
 
 Functional requirements - 16 points
 ===================================
 * 1/1 The window should have a Play Scale button.
 * 1/1 The window should have a Stop Playing button.
-* 3/3 When the user clicks the Play scale button, a dialog appears asking the user for a note number in the range 0 to 115 (see the window on the right above). 
+* 3/3 When the user clicks the Play scale button, a dialog appears asking the user for a note number in the range 0 to 115.
 * 1/1 Then an 8-note scale is played starting at the given note.
-* 1/1 The scale should be the usual Do Re Mi Fa Sol La Ti Do notes going up in pitch. 
+* 0/1 The scale should be the usual Do Re Mi Fa Sol La Ti Do notes going up in pitch.  *See https://en.wikipedia.org/wiki/Solfège#Major *
 * 1/1 After playing the scale going up, it should then play the same scale again but going down from the highest note. 
 * 1/1 The user might click the Play scale button when a scale is already being played. If the user clicks OK, then the scale being played should immediately stop and the new scale should be played instead. If the user clicks Cancel, then the scale currently playing should continue playing.
 * 1/1 When the user clicks the Stop playing button, the scale should stop playing immediately. 
@@ -30,7 +30,8 @@ Elementary style - 4 points
 ==========================
 * 1/1 Some attempt at descriptive method, parameter, and variable names.
 * 1/1 Consistent indentation.
-* 2/2 Appropriate use of basic language constructs; reasonably concise code.
+* 1/1 Appropriate use of basic language constructs; reasonably concise code.
+* 1/1 Code divided into methods.
 
 Next time I will look for "self-explanatory code" and appropriate use of 
 access modifiers.

src/scaleplayer/ScalePlayer.java

@@ -26,32 +26,39 @@
  */
 public class ScalePlayer extends Application {
 
+    //TODO: Why does this field exist? 
     private static int startingNote;
+    //TODO: Could this field be static, final?
     private MidiPlayer sequence = new MidiPlayer(2, 60);
 
     /**
      * Start: Handles the menu bar, buttons, exits, and events. 
-     * @param primaryStage 
+     * @param primaryStage //TODO: What is it for?
      */
     @Override
     public void start(Stage primaryStage) {
 
         //create menu bar
         MenuBar menuBar = new MenuBar();
+        //TODO: Is the next line necessary?
         menuBar.prefWidthProperty().bind(primaryStage.widthProperty());
         Menu file = new Menu("File");
         menuBar.getMenus().add(file);
         MenuItem exitMenuItem = new MenuItem("Exit");
         file.getItems().add(exitMenuItem);
         exitMenuItem.setOnAction(actionEvent -> System.exit(0));
         primaryStage.setOnCloseRequest(e -> System.exit(0));
+        //TODO: Confusing that the above line is here, 
+        //as it's not part of creating the menu bar
 
         //play button
         Button playBtn = new Button();
         playBtn.setText("Play Scale");
+        //NOTE: The below is ugly, but you'll fix it with CSS
         playBtn.setStyle("-fx-base: #b6e7c9;");
+        //TODO: Is an anonymous inner class necessary?
         playBtn.setOnAction(new EventHandler<ActionEvent>() {
-            
+            //TODO: This is way too much code to nest inside another method.
             /**
              * Handles the start and gets a note from the user, and starts the MidiPlayer using sequence. 
              * @param event 
@@ -61,10 +68,12 @@ public void handle(ActionEvent event) {
                 TextInputDialog dialog = new TextInputDialog();
                 dialog.setTitle("Starting Note");
                 dialog.setHeaderText("Please enter a note (0-115)");
+                //TODO: Set a default text input value
 
                 //get result, parse it into an int in a roundabout way, then play scale once its had
                 Optional<String> result = dialog.showAndWait();
                 if (result.isPresent()){
+                    //TODO: Why not just set startingNote to Integer.parseInt(result)?
                     String stringResult = result.toString();
                     stringResult = stringResult.substring(9, stringResult.length()-1);
                     startingNote = Integer.parseInt(stringResult); 
@@ -76,9 +85,11 @@ public void handle(ActionEvent event) {
         });
 
         //stop button
+        //TODO: See comments above
         Button stopBtn = new Button();
         stopBtn.setText("Stop Playing");
         stopBtn.setStyle("-fx-base: #eda6a6;");
+        //TODO: Is an anonymous inner class necessary?
         stopBtn.setOnAction(new EventHandler<ActionEvent>() {
 
             /**
@@ -100,8 +111,9 @@ public void handle(ActionEvent event) {
         hbox.setAlignment(Pos.CENTER);
 
         //Overhead for showing screen
+        //NOTE: Good choice
         BorderPane root = new BorderPane();
-        
+
         root.setTop(menuBar);
         root.setCenter(hbox);
 
@@ -110,14 +122,18 @@ public void handle(ActionEvent event) {
         primaryStage.setTitle("Scale Player");
         primaryStage.setScene(scene);
         primaryStage.show();
+
+        //NOTE: This method is much too long.
     }
 
 
     /**
      * Stops the current MidiPlayer sequence
-     * @param sequence 
+     * @param sequence //TODO: What is it?
      */
+     //TODO: Why is this public?
     public void stopScale(MidiPlayer sequence) {
+        //TODO: Why do you need to define a one line method?
         sequence.stop();
     }
 
@@ -126,6 +142,7 @@ public void stopScale(MidiPlayer sequence) {
      * @param sequence 
      */
     public void clearScale(MidiPlayer sequence) {
+        //TODO: See above
         sequence.clear();
     }
 
@@ -135,7 +152,12 @@ public void clearScale(MidiPlayer sequence) {
      * @param sequence
      * @param startingNote 
      */
+     //TODO: Why public?
     public void playScale(MidiPlayer sequence, int startingNote) {     
+        //TODO: Fix this so it's a Do-Re-Mi scale
+        //https://en.wikipedia.org/wiki/Solfège#Major
+        //TODO: Replace magic numbers with constants
+        //TODO: Fix indentation
         for(int i=1; i<9; i++)
         {
             sequence.addNote(startingNote, 100, i, 1,
@@ -161,4 +183,4 @@ public static void main(String[] args) {
         launch(args);
     }
 
-}
+}