10
\$\begingroup\$

As of right now I'm a beginner with programming and Java and I've only had a few months of practice so far. This is a project I've been working on as practice. It's essentially a basic course manager that a college professor or someone similar could use.

I'd like to get some feedback on any aspect of this program. Things I did wrong, bad habits, or things I could improve, shorten, simplify etc.

package courseManager;
import java.util.ArrayList;
import java.util.InputMismatchException;
import java.util.Scanner;
public class CourseManagment {
    public static ArrayList<Course> courses = new ArrayList<>();
    public static int getIndex(String viewID) {
        for(int n = 0 ; n < courses.size() ; n++) {
            if(courses.get(n).getID().equals(viewID)) {
                return n;
            }
        }
        return -1;
    }
    public static int checkYesNo() {
        Scanner input = new Scanner(System.in);
        String action = "";
        while(!action.equalsIgnoreCase("n") || (!action.equalsIgnoreCase("y"))) {
            action = input.nextLine();
            if(action.equalsIgnoreCase("y")) {
                return 1;
            }
            if(action.equalsIgnoreCase("n")) {
                return 2;
            }
            if(!action.equalsIgnoreCase("y") || !action.equalsIgnoreCase("n")) {
                System.out.println("Not a valid action. Enter Y or N. ");
            }
        }
        return 0;
    }
    public static void main(String[]args) {
        // Variables
        int refIndex = 0;
        String name = "";
        String id = "";
        int numStudents = 0;
        ArrayList<String> roster = new ArrayList<>();
        String meetDay = "";
        String meetTime = "";
        String action = "";
        String task = "";
        String viewID = "";
        String modify = "";
        String removeStudent = "";
        String addStudent = "";
        Scanner input = new Scanner(System.in);
        // Main code
        while(!task.equals("end")) {
            System.out.println("What would you like to do? Add or remove a course, modify an existing course, or view a course? "
                    + "Type 'add', 'remove', 'modify', or 'view'. Type 'end' to stop the program. ");
            task = input.nextLine();
            if(task.equals("end")) {
                break;
            }
            // Create course block
            if(task.equals("add")) {
                while(!action.equals("done")) {
                    System.out.println("Enter the name of the course. Type 'done' when you're finished adding. ");
                    courses.add(new Course(name, id, numStudents, roster, meetDay, meetTime));
                    System.out.println("Course added. Add another course? Y/N ");
                    if(checkYesNo() == 2) {
                        break;
                    }
                }
            }
            // Remove course block
            if(task.equals("remove")) {
                while(!action.equals("done")) {
                    System.out.println("Enter the name of the course you wish to remove. Type 'done' when you're finished removing. ");
                    viewID = input.nextLine();
                    if(getIndex(viewID) == -1) {
                        System.out.println("No course with this ID exists. Enter a valid course ID. \n");
                        break;
                    }
                    if(getIndex(viewID) != -1) {
                        courses.remove(getIndex(viewID));
                        System.out.println("Course removed. Remove another course? Y/N ");
                        if(checkYesNo() == 2) {
                            break;
                        }
                    }
                }
            }
            // Modify block
            if(task.equals("modify")) {
                System.out.println("Enter the ID of the course you wish to modify. ");
                viewID = input.nextLine();
                if(getIndex(viewID) == -1) {
                    System.out.println("Not a valid course ID. ");
                }
                if(getIndex(viewID) != -1) {
                    refIndex = getIndex(viewID);
                    while(!modify.equals("done")) {
                        System.out.println("What parameter of the course do you wish to modify? Type 'name', 'ID', '#students', 'meet day', or 'meet time'. "
                                + "If you wish to add or remove students type 'add student' or 'remove student'. ");
                        modify = input.nextLine();
                        if(modify.equals("back")) {
                            break;
                        }
                        if(modify.equals("name")) {
                            courses.get(refIndex).ChangeName();
                        }
                        if(modify.equals("ID")) {
                            courses.get(refIndex).ChangeID();
                        }
                        if(modify.equals("#students")) {
                            courses.get(refIndex).ChangeNumStudents();
                        }
                        if(modify.equals("meet day")) {
                            courses.get(refIndex).ChangeMeetDay();
                        }
                        if(modify.equals("meet time")) {
                            courses.get(refIndex).ChangeMeetTime();                         
                        }
                        if(modify.equals("add student")) {
                            while(!addStudent.equals("done")) {
                                System.out.println("Enter the name of the student you wish to add. Type 'done' when you're finished adding. ");
                                addStudent = input.nextLine();
                                if(addStudent.equals("done")) {
                                    break;
                                }
                                courses.get(refIndex).AddStudent(addStudent);
                            }
                        }
                        if(modify.equals("remove student")) {
                            while(!removeStudent.equals("done")) {
                                System.out.println("Enter the name of the student you wish to remove. Type 'done' when you're finished removing. ");
                                removeStudent = input.nextLine();
                                if(removeStudent.equals("done")) {
                                    break;
                                }
                                courses.get(refIndex).RemoveStudent(removeStudent);
                            }
                        }
                    }
                }
            }
            // View block
            if(task.equals("view")) {
                System.out.println("Enter the ID of the course you want to view. ");
                viewID = input.nextLine();
                if(getIndex(viewID) == -1) {
                    System.out.println("No course with this ID exists. Enter a different ID. \n");
                }
                if(getIndex(viewID) != -1) {
                    System.out.println(courses.get(getIndex(viewID)));
                }
            }
        }
        input.close();
    }
}
class Course {
    // toString override for the Course class
    @Override
    public String toString() {
        return "\n Course name: " + name + "\n Course ID: " + id + "\n Current # of students: " + roster.size() + "/" + numStudents + "\n Student roster: " + roster
            + "\n Meeting day: " + meetDay + "\n Meeting time: " + meetTime + "\n";
    }
    Scanner input = new Scanner(System.in);
    // Course properties
    String name;
    String id;
    int numStudents;
    ArrayList<String> roster;
    String meetDay;
    String meetTime;
    String addStudent;
    String removeStudent;
    // Course constructor
    Course(String name, String id, int numStudents, ArrayList<String> roster, String meetDay, String meetTime) {
        // User entered course properties
            this.name = input.nextLine();
        System.out.println("Enter the course ID. ");
            this.id = input.nextLine();
        System.out.println("Enter the max number of students. ");
        while(!input.hasNextInt()) {
            try {
                this.numStudents = input.nextInt();
                input.nextLine();
            } catch (InputMismatchException e) {
                System.out.println("This value needs to be a number...\nEnter the max number of students. ");
                input.nextLine();
            }
        }
        input.nextLine();
            this.roster = new ArrayList<String>();
        System.out.println("Enter the meeting day of the course. ");
            this.meetDay = input.nextLine();
        System.out.println("Enter the meeting time of the course. ");
            this.meetTime = input.nextLine();
    }
    // Modify Course methods
    String getID() {
        return this.id;
    }
    public void ChangeName() {
        System.out.println("Enter the new name of the course. ");
        this.name = input.nextLine();
    }
    public void ChangeID() {
        System.out.println("Enter the new ID of the course. ");
        this.id = input.nextLine();
    }
    public void ChangeNumStudents() {
        System.out.println("Enter the new max number of students of the course. ");
        while(!input.hasNextInt()) {
            try {
                this.numStudents = input.nextInt();
                input.nextLine();
            } catch (InputMismatchException e) {
                System.out.println("This value needs to be a number...\nEnter the max number of students. ");
                input.nextLine();
            }
        }
        input.nextLine();
    }
    public void ChangeMeetDay() {
        System.out.println("Enter the new meeting day of the course. ");
        this.meetDay = input.nextLine();
    }
    public void ChangeMeetTime() {
        System.out.println("Enter the new meeting time of the course. ");
        this.meetTime = input.nextLine();
    }
    public void AddStudent(String addStudent) {
        this.roster.add(addStudent);
    }
    public void RemoveStudent(String removeStudent) {
        this.roster.remove(removeStudent);
    }
}
\$\endgroup\$
5
\$\begingroup\$

I decided to restructure your code out of boredom and for a little java practice myself. Also I thought it would be nifty for you to see your project through someone else's eyes. Whether the code below is the best way to achieve the task at hand or not, there are still quite a few things you will be able to take from my code. I'll walk through each class and show you what I changed.

Things to take note of (CourseManagement Class):
First off, you generally want to separate your code into functions based on said code's functionality, instead of reading through one giant mother block of code. Not only are functions used for code reuse-ability, they're also useful for code readability, which is just as important. Speaking of readability, blank lines at certain positions can be quite useful. The reason I bring this up is because I notice you had none. If blank lines are not your thing, you should at least place them in between functions and between logical sections inside a method.

Generally when checking for string cases, its better to use a switch case method. If statements are normally reserved for Boolean logic. If you must use if statements though, at least use if, else if, else statements when able. It is generally bad practice to separate group able questions into their own if statements like you have done. Your program will continue to ask questions that it already knows the answer to.

For more code conventions take a look at: Java Code Conventions

import java.text.ParseException;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

/**
 * Created on 7/29/2016.
 *
 */


public class CourseManagement {
    private static List<Course> courses = new ArrayList<>(); // Stores course objects.

    public static void main(String[] args){
        loop:while(true){
            String input = prompt("Add, Remove, Display, Modify, Exit >>> ").toLowerCase();

            // Checks acquired input to each case expected.
            switch(input){
                case "add" : addCourse(); break;
                case "remove" : removeCourse(); break;
                case "display" : displayCourses(); break;
                case "modify" : modifyCourse(); break;
                case "exit" : break loop;
                case "" : continue loop;
                default : System.out.printf("'%s' unrecognized command.%n", input);
            }
        }
    }

    private static void addCourse(){
        try{
            String name = prompt("Course Name: ");
            String id = prompt("Course ID: ");
            String date = prompt("Course Meet Time(YYYY-MM-DD HH:MM): ");
            courses.add(new Course(name, Integer.valueOf(id), date));
        } catch(ParseException ex){
            System.out.println("Date Could Not Be Parsed.");
        }
    }

    private static void removeCourse(){
        try{
            courses.remove(courses.get(getCourseIndex(Integer.valueOf(prompt("Course ID: ")))));
        } catch(ArrayIndexOutOfBoundsException ex){
            System.out.println("Course Does Not Exist.");
        }
    }

    private static void displayCourses(){
        if(courses.size() <= 0){
            System.out.println("No Current Courses.");
        } else {
            courses.forEach(i -> System.out.printf("Course Name: %s Course ID: %s Course Date: %s%n", i.getName(), i.getID(), i.getDate()));
        }
    }

    private static void modifyCourse(){
        String firstName, lastName, fullName;
        int id;
        Course course = courses.get(getCourseIndex(Integer.valueOf(prompt("Course ID: ")))); // Selected course to modify.

        loop:while(true){
            String input = prompt("Add Student, Remove Student, Display Roster, Display Time, Change ID, Change Name, Change Time, Exit >>> ").toLowerCase();

            switch(input){
                case "add student" :
                    firstName = prompt("Student First Name: ");
                    lastName = prompt("Student Last Name: ");
                    id = Integer.valueOf(prompt("Student ID: "));
                    course.addStudent(new Student(firstName, lastName, id)); break;
                case "remove student" :
                    fullName = prompt("Student Full Name: ");
                    id = Integer.valueOf(prompt("Student ID: "));
                    course.removeStudent(fullName, id); break;
                case "display roster" : course.displayRoster(); break;
                case "display time" : course.displayTime(); break;
                case "change id" : course.changeID(Integer.valueOf(prompt("New ID: "))); break;
                case "change name" : course.changeName(prompt("New Name: ")); break;
                case "change time" :
                    try{
                        course.changeMeetTime(prompt("New Date(YYYY-MM-DD HH:MM): "));
                    } catch(ParseException ex){
                        System.out.println("Date Could Not Be Parsed.");
                    }
                    break;
                case "exit" : break loop;
                case "" : continue loop;
                default : System.out.printf("'%s' unrecognized command.", input);
            }
        }
    }

    // Finds the index of wanted course within the course collection.
    private static int getCourseIndex(int id){
        for(int i = 0; i < courses.size(); i++){
            if(courses.get(i).getID() == id){
                return i;
            }
        }
        return -1;
    }

    // Used to print a message and acquire user input.
    // Used in the interest of code reuse-ability
    private static String prompt(String message){
        System.out.print(message);
        return new Scanner(System.in).nextLine();
    }
}

Things to take note of (Course Class):
Your constructor needs work. You have the constructor set up to take arguments when a new instance of the class is being created, but you never use them. Notice in my example the parameters are actually used.

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;

/**
 * Created on 7/29/2016.
 *
 */


public class Course {
    private String name;
    private Date date;
    private int id;
    private List<Student> roster = new ArrayList<>();
    private final SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd hh:mm");
    private final SimpleDateFormat timeFormat = new SimpleDateFormat("hh:mm");

    public Course(String name, int id, String date) throws ParseException {
        this.name = name;
        this.id = id;
        this.date = dateFormat.parse(date);
    }

    public String getName(){
        return name;
    }

    public int getID() {
        return id;
    }

    public String getDate(){
        return date.toString();
    }

    public void displayRoster(){
        roster.forEach(i -> System.out.printf("%s %s ID: %s%n", i.getFirstName(), i.getLastName(), i.getID()));
        System.out.printf("Roster Size: %d%n", roster.size());
    }

    public void addStudent(Student student) {
        roster.add(student);
    }

    public void removeStudent(String fullName, int id){
        roster.removeIf(i -> i.getFullName().equals(fullName) && i.getID() == id);
    }

    public void displayTime(){
        System.out.println(timeFormat.format(date));
    }

    public void changeName(String newName) {
        name = newName;
    }

    public void changeID(int newID) {
        id = newID;
    }

    public void changeMeetTime(String newDate) throws ParseException {
        this.date = dateFormat.parse(newDate);
    }
}

An added class:
I decided to make a student class, that way the students information could be easily added onto and manipulated. If you're up for the challenge, here are some things that can be added onto the student class.

  • Student test grades and average.
  • Student homework grades and average.
  • Student overall average. (Test grades are 70%, Homework grades are 30%)

And a few things that can be added to the course class, knowing the above information.

  • Course test average.
  • Course homework average.
  • Course average.

Best of luck and happy coding.

 /**
 * Created on 7/29/2016.
 *
 */


public class Student{
    private String firstName, lastName, fullName;
    private int ID;

    public Student(String firstName, String lastName, int ID){
        this.firstName = firstName;
        this.lastName = lastName;
        this.fullName = firstName + " " + lastName;
        this.ID = ID;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public String getFullName() {
        return fullName;
    }

    public int getID() {
        return ID;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Splitting the date string seems wrong when you have the perfectly fine java.text.SimpleDateFormat class at your disposal: new SimpleDateFormat("yyyy-MM-dd hh:mm").parse(dateString) returns a Date object and doesn't use its deprecated constructor. Furthermore, you can actually use some nice format for the date and time, instead of clumsily just separating everything with a space. (In this case, you'd enter 2016-07-29 14:30 as the input string.) \$\endgroup\$ – ZeroOne Jul 29 '16 at 11:35
  • \$\begingroup\$ @ZeroOne Yea something felt off about it, and I was aware of the other java date and time tools, but just wasnt comfortable enough with them. I made the edits. Let me know if my implementation is correct. And thanks a million for the critique. \$\endgroup\$ – JSextonn Jul 29 '16 at 12:11
  • \$\begingroup\$ Looks good to me. It obviously needs some better error handling still, but anyway. And by the way, I like your review the best of these so far. :) I would've written one myself, but it probably wouldn't have been too different from yours. \$\endgroup\$ – ZeroOne Jul 29 '16 at 12:15
  • \$\begingroup\$ @ZeroOne Haha, yea the error handling is awful, started working on it right when i posted that comment. And thanks :D great minds think alike. \$\endgroup\$ – JSextonn Jul 29 '16 at 12:18
  • \$\begingroup\$ Wow thanks for the wealth of information, I learned a lot from this. I love the "prompt" method. It's so simple but effective, I'm using it all the time now. I had a question about a piece of code though. How do the while(true) statements work? What does this actually mean? \$\endgroup\$ – SteelSlayer Jul 30 '16 at 12:50
10
\$\begingroup\$
  • Method names in java start in lower case: ChangeNumStudents => changeNumStudents
  • Java is not c, no need to declare all these variables at the top of your main: declare them when you need them, with the smallest possible scope.
  • an alternative to your long series of ifs would be to use a map of actions (using Java 8 syntax - with prior versions it may be a bit cumbersome and possibly not the best approach):

    private static final Map<String, Consumer<Course>> actions = new HashMap<> ();
    static {
      actions.put("back", c -> { });
      actions.put("name", Course::changeName);
      actions.put("add student"), CourseManagment::addStudent);
      //etc.
    }
    
    //in your loop:
    Course c = course.get(refIndex);
    Consumer<Course> action = actions.get(modify);
    action.accept(course);
    

    An example of the addStudent method in CourseManagement would be:

    private static void addStudent(Course course) {
      while(true) {
        System.out.println("Enter the name of the student you wish to add. Type 'done' when you're finished adding.");
        String student = input.nextLine();
        if (student.equals("done")) break;
        course.addStudent(student);
      }
    }
    
\$\endgroup\$
8
\$\begingroup\$

I'd like to also suggest you use an enum as a return type of your checkYesNo()-method. Then you can write if(checkYesNo() == NO) which is more verbose, easier to read and comprehend and therefore easier to maintain.

Furthermore: do you actually need two variables for removeStudent and addStudent? Why not use studentName or similar? I hope I'm not too picky in also pointing out that name could be substituted for courseName for better readability.

Next: I think your program could make use of the Command Pattern where each task would be defined as a command. This would help shorten your main-method and it's nice to learn and learn to implement well-established design patterns.

Edit - as pointed out by Vogel612 a small example, how you could apply a Command Pattern here: You could create a Command for each task in your program, so for add, remove and so on.

First, you define an Interface for your commands, for example:

public interface Command {
  public void execute(); // I know, the public keyword is redundant for interface methods...
} 

Then you define the commands needed, which will implement the interface defined before for example the AddCourseCommand:

public class AddCourseCommand implements Command {
  private ArrayList<Course> courseList;

  public AddCourseCommand(ArrayList<Course> courses) {
    this.courseList = courses;
  }     

  @Override
  public void execute() {
    // your addCourse logic here.
  }
}

In your main-method you could then use a HashMap to store your Commands:

HashMap<String, Command> commands = new HashMap<>();
commands.put("add", new AddCourseCommand(courses)); // note that you will modify main class' members here.
// add further commands as needed, e.g. "remove", "modify", ...

This way you can read the user input and handle the command accordingly:

Scanner input = new Scanner(System.in);
String userInput;
do
{
  // print usage message here 

  userInput = input.getLine();
  Command command = commands.get(userInput);
  if(command != null)
  {
    command.execute();
  }
} while(!userInput.equals("end");
\$\endgroup\$
  • \$\begingroup\$ You're right, the two student variables are redundant, and I've changed some variable names around like name and id to courseName and courseID for readability. Command pattern looks interesting, I'll look into it. \$\endgroup\$ – SteelSlayer Jul 30 '16 at 7:10
  • \$\begingroup\$ Thank you for pointing that out. I saw that the method returned 0, 1 and 2 and thought you could use an enum for that. I did not look as close to see that 0 should never be returned. If you want to use a method returning bool, you should refactor the method name to something more expressive. I would not know when to expect true and when to expect false from a method called checkYesNo. I added a small example to express the main idea of the command pattern. \$\endgroup\$ – vonludi Jul 31 '16 at 15:33
7
\$\begingroup\$

Your main() method is far too long. Try changing to a more object oriented approach and try to keep code out of that main method.

I notice alot of if(modify.equals(... type statements. You should consider changing these to a switch, like so:

switch(modify){
    case "add":
        //do stuff
        break;
    case "delete":
        //do stuff
        break;
}

And where those "//do stuff" comments are, consider calling a method on a service object rather than doing stuff directly. This sort of abstraction may not seem useful in this example, but it's a good habit to be in as it will keep your code tidy and allow you to increase code reuse.

With your Course class, you are mixing behaviour with data storage. Typically, we try to keep a class like course free of as much logic as possible to allow it's reuse in as many places as possible. Try extracting all of that behaviour logic out into a service class and leave your Course class as a bunch of variables with getters and setters.

Still in your Course class, I prefer to see variables and methods (even the constructor) declared with access levels (public, private, protected). It just looks weird to me without them.

\$\endgroup\$
  • \$\begingroup\$ That's a good idea to use a switch statement, I've started to use these in my program instead. I'm not familiar with "service object" and "service class." Could you explain those? \$\endgroup\$ – SteelSlayer Jul 30 '16 at 1:23
  • \$\begingroup\$ A service class is a just a class with methods containing the logic for your program. A service object is an instance of that class. So in your case, your service class (or classes) might contain methods like addStudentToCourse, removeStudentFromCourse, etc. Basically, the aim is to make your code as human readable as possible and to abstract the logic away from the user interface, allowing it be reused in other areas of your program. \$\endgroup\$ – Maybe_Factor Jul 31 '16 at 23:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.