What do you think of this Java code?

Started by
4 comments, last by superpig 14 years, 2 months ago

public class TestScoresModel{

   private Student[] students;         // Array of students
   private int indexSelectedStudent;   // Position of current student
   private int studentCount;           // Current number of students

   public TestScoresModel(){

      // Initialize the data
      indexSelectedStudent = -1;
      studentCount = 0;
      students = new Student[10];
   }

   // Mutator methods for adding and replacing students

   public String add(Student s){
      if (studentCount == students.length)
         return "SORRY: student list is full";
      else{
         students[studentCount] = s;
         indexSelectedStudent = studentCount;
         studentCount++;
         return null;
      }
   }

   public String replace(Student s){
      if (indexSelectedStudent == -1)
         return "Must add a student first";
      else{     
         students[indexSelectedStudent] = s;
         return null;
      }
   }

   // Navigation methods

   public Student first(){
      Student s = null;
      if (studentCount == 0)
         indexSelectedStudent = -1;
      else{
         indexSelectedStudent = 0;
         s = students[indexSelectedStudent];
      }
      return s;
   }

    public Student previous(){
      Student s = null;
      if (studentCount == 0)
         indexSelectedStudent = -1;
      else{
         indexSelectedStudent
             = Math.max (0, indexSelectedStudent - 1);
         s = students[indexSelectedStudent];
      }
      return s;
   }

   public Student next(){
      Student s = null;
      if (studentCount == 0)
         indexSelectedStudent = -1;
      else{
         indexSelectedStudent
             = Math.min (studentCount - 1, indexSelectedStudent + 1);
         s = students[indexSelectedStudent];
      }
      return s;
   }

   public Student last(){
      Student s = null;
      if (studentCount == 0)
         indexSelectedStudent = -1;
      else{
         indexSelectedStudent = studentCount - 1;
         s = students[indexSelectedStudent];
      }
      return s;
   }

   // Accessors to observe data

   public Student currentStudent(){
      if (indexSelectedStudent == -1)
         return null;
      else
         return students[indexSelectedStudent];
   }

   public int size(){
      return studentCount;
   }

   public int currentPosition(){
      return indexSelectedStudent;
   }

   public int getClassAverage(){
      if (studentCount == 0)
         return 0;
      int sum = 0;
      for (int i = 0; i < studentCount; i++)
         sum += students.getAverage();
      return sum / studentCount;
   }

   public Student getHighScore(){
      if (studentCount == 0)
         return null;
      else{
         Student s = students[0];
         for (int i = 1; i < studentCount; i++)
            if (s.getHighScore() < students.getHighScore())
               s = students;
         return s;
      }
   }

   public String toString(){
      String result = "";
      for (int i = 0; i < studentCount; i++)
         result = result + students + "\n";
      return result;
   }
}



import java.util.Scanner;

public class TestScoresView{

   private TestScoresModel model;

   public TestScoresView(TestScoresModel m){
      model = m;
      run();
   }

   // Menu-driven command loop
   private void run(){
      while (true){
      	 System.out.println("start of run method");
         System.out.println("Number of students: " + model.size());
         System.out.println("Index of current student: " +
                            model.currentPosition());
         displayMenu();
         int command = getCommand("Enter a number [1-11]: ", 1, 11);
         if (command == 11)
            break;
         runCommand(command);
      }
   }

   private void displayMenu(){
      System.out.println("MAIN MENU");
      System.out.println(" 1. Display the current student");
      System.out.println(" 2. Display the class average");
      System.out.println(" 3. Display the student with the highest grade");
      // missing code
      
      
      System.out.println("11. Quit the program");
   }

   // Prompts the user for a command number and runs until
   // the user enters a valid command number
   // Parameters: prompt is the string to display
   //             low is the smallest command number
   //             high is the largest command number
   // Returns: a valid command number
   private int getCommand(String prompt, int low, int high){
     // errors checking
      Scanner reader = new Scanner(System.in);
      int command = low - 1;
      while (command < low || command > high){
         System.out.print(prompt);
         command = reader.nextInt();
         if (command < low || command > high)
            System.out.println("Error: command must be between " +
                               low + " and " + high);
       }
       return command;
   }

   // Selects a command to run based on a command number,
   // runs the command, and asks the user to continue by
   // pressing the Enter key
   private void runCommand(int command){
      if (command == 1)
         displayStudent();
      else if (command == 2)
         System.out.println("Average score = " + model.getClassAverage());
   
      //missing code	     
   }

   private void displayStudent(){
      Student s = model.currentStudent();
      if (s == null)
         System.out.println("No student is currently available");
      else
         System.out.println(s);
   }

   private void displayHighScore(){
      Student s = model.getHighScore();
      if (s == null)
         //missing code
      else
         //missing code
   }

   private void addStudent(){
      Student s = new Student();
      Scanner reader = new Scanner(System.in);
      System.out.print("Enter the name: ");
      s.setName(reader.nextLine());
      for (int i = 1; i <= s.getNumberOfTests(); i++){
         //missing code
      }
      String message = s.validateData();
      if (message != null)
         System.out.println(message);
      else{
         message = model.add(s);
         if (message != null)
            System.out.println(message);
         else
            System.out.println("Student added");
      } 
   }

   private void editStudent(){
      Student s = model.currentStudent();
      if (s == null)
         System.out.println("No student is currently available");
      else{
         // Work on a temporary copy
         Student temp = new Student(s);
         String menu = "EDIT MENU\n" +
                       "1. Change the name\n" +
                       "2. Change a score\n" +
                       "3. View the student\n" +
                       "4. Quit this menu\n";
         Scanner reader = new Scanner(System.in);
         int command = 1;
         while (command != 4){
            System.out.print(menu);
            command = getCommand("Enter a number [1-4]: ", 1, 4);
            if (command == 1){         
             //lots of missing code  
            }else{
               // Check for valid data before writing to database
               String message = temp.validateData();
               if (message != null)
                  System.out.println(message);
               else
                  model.replace(temp);
             }
         }
      }
   }
}


Do you think it is good? Bad? And why? After reading through it I decided to improve on it because some parts of it looked like they could be better designed. This is my completed code for the view file, you can also see that I basically rewrote the whole thing.

/*
 *  Test Scores View
*/

import java.util.Scanner; 

public class TestScoresView
{
	private TestScoresModel model; 
	private Scanner scanner; 
		
	public TestScoresView(TestScoresModel newModel)
	{
		model = newModel;
		scanner = new Scanner(System.in);
		
		run();
	}
	
	private void run()
	{
		int command = 0; 
			
		while (command != 11)
		{
			System.out.println("Number of students: " + model.size());
			System.out.println("Current student index: " + model.currentPosition());
			System.out.println();
			
			displayMenu();
			
			command = getCommand("Enter a number [1-11]: ", 1, 11);
			runCommand(command);
		}
	}
	
	private void displayMenu()
	{
		String[] menuOptions = 
		{
			"Display current student", 
			"Display class average", 
			"Display student with highest grade", 
			"Display all students", 
			"Edit current student", 
			"Add new student", 
			"Go to first student", 
			"Go to last student", 
			"Go to next student", 
			"Go to previous student", 
			"Quit the program"
		}; 
			
		for (int c = 0; c < menuOptions.length; c++)
			System.out.println((c + 1) + ": " + menuOptions[c]);
			
		System.out.println();
	}
	
	private int getCommand(String prompt, int min, int max)
	{
		// Keep prompting for input until valid input is received
		int command = min - 1; 
			
		while (command < min || command > max)
		{
			try
			{
				System.out.print(prompt);
				command = Integer.parseInt(scanner.nextLine());
			}
			catch (Exception exception)
			{
				command = min - 1; 
				continue; 
			}
		}
		return command; 
	}
	
	private void runCommand(int command)
	{
		switch (command)
		{
			case 1:
				System.out.println((model.size() > 0) ? model.currentStudent() : "There are no students.");
				break;
				
			case 2:
				System.out.println("The class average is " + model.getClassAverage());
				break;
				
			case 3:
				Student highScoreStudent = model.getHighScore();
				System.out.println((highScoreStudent != null) ? 
					"The student " + highScoreStudent.getName() + " got the highest score of " + highScoreStudent.getHighScore()
					     : "There are no students yet.");
				break;
				
			case 4:
				System.out.println((model.size() > 0) ? model : "There are no students.");
				break;
				
			case 5:
				editStudent();
				break;

			case 6:
				addStudent();
				break;
				
			case 7:
				System.out.println((model.size() > 0) ? model.first() : "There are no students.");
				break;
				
			case 8:
				System.out.println((model.size() > 0) ? model.last() : "There are no students.");
				break;
				
			case 9:
				System.out.println((model.size() > 0) ? model.next() : "There are no students.");
				break;
				
			case 10:
				System.out.println((model.size() > 0) ? model.previous() : "There are no students.");
				break;
		}
		
		
		// Wait for enter key to be pressed before continuing
		if (command >= 1 && command <= 10)
		{
			System.out.println("Press the enter key to continue.");
			scanner.nextLine();
		}
	}
	
	private void addStudent()
	{
		Student newStudent = getStudentInput();
		String errors = newStudent.validateData();
		
		if (errors == null)
		{
			if (model.add(newStudent))
			{
				System.out.println("Student has successfully been added.");
			}
			else
			{
				System.out.println("The list of students is full, so the student was not added.");
			}
		}
		else
		{
			System.out.println(errors);
		}
	}
	
	private void editStudent()
	{
		if (model.size() > 0)
		{
			Student updatedStudent = getStudentInput();
			String errors = updatedStudent.validateData();
			
			if (errors == null)
			{
				System.out.println("Student has successfully been edited.");
				model.replace(updatedStudent);
			}
			else
			{
				System.out.println(errors);
			}
		}
		else
		{
			System.out.println("There are no students to edit.");
		}
					
	}
	
	private Student getStudentInput()
	{
		String name = ""; 
		int[] tests; 
		int testCount = 0; 
			
		while (name.length() == 0)
		{
			System.out.println("Enter the student's name: ");
			name = scanner.nextLine();
		}
		
		while (testCount <= 0)
		{
			System.out.println("How many tests does this student have: ");
			
			try
			{
				testCount = Integer.parseInt(scanner.nextLine());
			}
			catch (Exception exception)
			{
				testCount = 0; 
				continue;
			}
		}
		
		tests = new int[testCount]; 
			
		for (int c = 0; c < testCount; c++)
		{
			int score = -1; 
				
			while (score < 0 || score > 100)
			{
				System.out.println("Enter score for test #" + (c + 1) + ": ");
				
				try
				{
					score = Integer.parseInt(scanner.nextLine());
				}
				catch (Exception exception)
				{
					score = -1; 
					continue;
				}
			}
			
			tests[c] = score;
		}
		
		return new Student(name, tests);
	}
}


What do you think of this code and how do you think I can improve it?
Advertisement
Here's a few thoughts to begin with.


  • Why use a fixed array of 10 students with a count variable, instead of java.util.List<Student>?

  • The navigation methods 'first' and 'last' are a bit ambiguous as to whether they actually change the model - I'd expect collection.first() and collection.last() to return the first and last elements in the collective respectively, and I wouldn't expect them to change any state (like which one is currently 'selected'). The names selectFirst, selectNext, selectPrev, selectLast would be better.

  • getHighScore() is confusingly named because I'd expect it to return the highest score, not the student with the highest score. Try calling it getHighestScoring() instead?

  • I'd be inclined to separate the 'currently selected' functionality from the model into a separate 'cursor' class. Imagine two views trying to use the model at the same time - if the cursor is separated out, then they could each have a different 'selected' student with no problems. The model would probably need to fire a couple of events for students being added/removed so the cursors could make sure they don't become invalid.

Richard "Superpig" Fine - saving pigs from untimely fates - Microsoft DirectX MVP 2006/2007/2008/2009
"Shaders are not meant to do everything. Of course you can try to use it for everything, but it's like playing football using cabbage." - MickeyMouse

Quote:Original post by superpig
Here's a few thoughts to begin with.


  • Why use a fixed array of 10 students with a count variable, instead of java.util.List<Student>?

  • make sure they don't become invalid.




I think this looks like the homework problem typical of an intro level class, and it may be the case the teacher wants them to use arrays only. That's how it was in my classes because they wanted to see you knew how to use them properly etc etc.



@Ntvu:
As for your code, I'm not sure if there is some requirement set by the teacher that I don't know about or whether it's just personal style... but I was a little confused as to why you seperated your menu into three different methods in the TestScoresView class. One to display, another to get input, and then another to do the logic?

I could see why you might make the input one into a method because that code could get repetitive to write for multiple menus, so it would be good idea to write that one just once. Though, otherwise the setup seems a little counter intuitive to me. That's one thing that jumped out at me that I can suggest to consider.
Quote:Original post by Xelen
I think this looks like the homework problem typical of an intro level class, and it may be the case the teacher wants them to use arrays only. That's how it was in my classes because they wanted to see you knew how to use them properly etc etc.


My extensive experience has led me to the conclusion that people who teach like this are misguided, full stop.
Quote:Original post by superpig
Here's a few thoughts to begin with.


  • Why use a fixed array of 10 students with a count variable, instead of java.util.List<Student>?



The assignment requires me to.

Quote:Original post by superpig
  • The navigation methods 'first' and 'last' are a bit ambiguous as to whether they actually change the model - I'd expect collection.first() and collection.last() to return the first and last elements in the collective respectively, and I wouldn't expect them to change any state (like which one is currently 'selected'). The names selectFirst, selectNext, selectPrev, selectLast would be better.



  • Again, the assignment requires me to.

    Quote:Original post by superpig
  • getHighScore() is confusingly named because I'd expect it to return the highest score, not the student with the highest score. Try calling it getHighestScoring() instead?


  • I'd be inclined to separate the 'currently selected' functionality from the model into a separate 'cursor' class. Imagine two views trying to use the model at the same time - if the cursor is separated out, then they could each have a different 'selected' student with no problems. The model would probably need to fire a couple of events for students being added/removed so the cursors could make sure they don't become invalid.



  • All of that was part of the original code from the textbook, none of it was written by me. The same goes for the array of only 10 students, the navigation methods, etc.

    The last source block I have (the third one) is the code that I wrote.
    Quote:Original post by Ntvu
    The assignment requires me to.


    Quote:Again, the assignment requires me to.


    Quote:All of that was part of the original code from the textbook, none of it was written by me. The same goes for the array of only 10 students, the navigation methods, etc.

    Well, you asked whether the code was good or bad, and I told you. You didn't say you hadn't written it, or that you had to have it that way. In fact, you said the opposite - that you'd rewritten the second code block basically from scratch. Sucks that you can't change it, but that's not relevant to what you asked, is it?

    Quote:The last source block I have (the third one) is the code that I wrote.


    OK.

    • Tying the different commands you can perform in the system to the way they are selected and executed isn't great. You could pull each command out as a Command object, store them in a collection, and then have the menu code just take a list of commands, display them, select one, etc. Imagine that you later go on to add a GUI or web interface to your system - you can have the same command objects being passed to different interface systems. Making each command an object will also remove the nasty separation you've got between command numbers, names, and actions - if you add or remove a command you've got to update the code in 6 places, when only 1 is necessary.

    • At the moment all your output is tied to System.out, and it's scattered through the code. In methods like addStudent, it would be better to use exceptions to report errors, which you can catch at the menu-code level and display there. That way all the System.out usage is restricted to the menu stuff.

    • It'd be neater if you simply didn't offer commands that can't be executed - e.g. don't even display 'edit student' as an option if there are no students in the roster.

    Richard "Superpig" Fine - saving pigs from untimely fates - Microsoft DirectX MVP 2006/2007/2008/2009
    "Shaders are not meant to do everything. Of course you can try to use it for everything, but it's like playing football using cabbage." - MickeyMouse

    This topic is closed to new replies.

    Advertisement