Merge pull request #10 from WhitmanSWDesignSpring2017/project1-grading

Project1 grading

Author: Pablo Fernandez

rubric.md

@@ -0,0 +1,47 @@
+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.
+* 1/1 Then an 8-note scale is played starting at the given note.
+* 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. 
+* 1/1 The Play button and the Stop button should be visually distinct. 
+* 1/1 The two buttons should be centered in the window next to each other but not touching each other.
+* 1/1 If the user shrinks or expands the window (for example, by dragging an edge or corner), the two buttons should remain centered in the window.
+* 1/1 The window should have a File menu with one menu item Exit. 
+* 1/1 If the user selects this menu item, then the window disappears and the application quits.
+* 1/1 When the user clicks the close box (in the upper right or left corner of the window), the application quits just like when the user chooses the Exit menu item.
+
+Implementation requirements - 8 points
+======================================
+* 2/2 Clone the given GitHub repository and push your code to it.
+* 2/2 Use JavaFX.
+* 1/1 Use the given MidiPlayer class.
+* 1/1 Put all your code in the provided ScalePlayer class.
+* 2/2 Document all methods using JavaDoc.
+
+Elementary style - 4 points
+==========================
+* 1/1 Some attempt at descriptive method, parameter, and variable names.
+* 1/1 Consistent indentation.
+* 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.
+
+Stretch goal - Extra Credit
+===========================
+* 0/0 
+
+Reflection - 4 points
+=====================
+* 1/1 Design overview
+* 2/2 What's elegant or not?
+* 1/1 How did your team collaborate?

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);
     }
 
-}
+}