6
\$\begingroup\$

In particular, I'm not happy with case 5 in the Control class. Basically, in case 5, I would like the user to attach a student from the students array onto a module from the modules array without duplication. Any suggestions/example would be very useful.

Driver Class:

public class MyApplication {

public static void main(String[] args) {
    Control control = new Control();
    control.run();
}
}

Control class:

import java.util.Scanner;

public class Control {

// Integers represented as module codes 
private static final int UFCE1 = 0;
private static final int UFCE2 = 1;
private static final int UFCE3 = 2;
private static final int UFCE4 = 3;
private static final int UFCE5 = 4;
// Integers represented as students
private static final int STUDENT1 = 0;
private static final int STUDENT2 = 1;
private static final int STUDENT3 = 2;
private static final int STUDENT4 = 3;
private static final int STUDENT5 = 4;

public void run() {

    Student jane = new Student("jane");
    Student alex = new Student("alex");
    Student mike = new Student("mike");
    Student james = new Student("james");
    Student julia = new Student("julia");

    Student[] students = new Student[]{jane, alex, mike, james, julia};


    Module ufce1 = new Module("UFCE1");
    Module ufce2 = new Module("UFCE2");
    Module ufce3 = new Module("UFCE3");
    Module ufce4 = new Module("UFCE4");
    Module ufce5 = new Module("UFCE5");

    Module[] modules = new Module[]{ufce1, ufce2, ufce3, ufce4, ufce5};

    jane.addModule(ufce1.getName());
    jane.addModule(ufce3.getName());
    alex.addModule(ufce1.getName());
    alex.addModule(ufce2.getName());

    ufce1.addStudent(jane.getName());
    ufce3.addStudent(jane.getName());
    ufce1.addStudent(alex.getName());
    ufce2.addStudent(alex.getName());

    System.out.println("Welcome to Student Record System!");

    while (true) {
        Menu menu = new Menu();
        menu.getMainMenu();

        try {
            Scanner scan = new Scanner(System.in);

            int selection = scan.nextInt();
            switch (selection) {
                case 1:
                    for (Student student : students) {
                        System.out.print(student.getName() + " ");
                    }
                    break;
                case 2:
                    for (Module module : modules) {
                        System.out.print(module.getName() + " ");
                    }
                    break;
                case 3:
                    System.out.println("Search module code: ");
                    scan = new Scanner(System.in);
                    String searchModule = scan.nextLine().trim();

                    for (Module module : modules) {
                        if (searchModule.equalsIgnoreCase(module.getName())) {
                            for (int s = 0; s < module.getStudents().size(); s++) {
                                System.out.print(module.getStudents().get(s) + " ");
                            }
                        }
                    }
                    break;
                case 4:
                    System.out.println("Search students : ");
                    scan = new Scanner(System.in);
                    String searchStudent = scan.nextLine().trim();

                    for (Student student : students) {
                        if (searchStudent.equalsIgnoreCase(student.getName())) {
                            for (int s = 0; s < student.getModules().size(); s++) {
                                System.out.print(student.getModules().get(s) + " ");
                            }
                        }
                    }
                    break;
                case 5:
                    // Print out students 
                    System.out.println("select a student: ");
                    for (int i = 0; i < students.length; i++) {
                        System.out.println(i + " " + students[i]);
                    }

                    selection = scan.nextInt();
                    switch (selection) {

                        case STUDENT1:
                            System.out.println("select a module to enroll " + students[0].getName() + " on to:");
                            for (int i = 0; i < modules.length; i++) {
                                System.out.println(i + " " + modules[i]);
                            }

                            selection = scan.nextInt();
                            switch (selection) {
                                case UFCE1:
                                    System.out.println("You have enrolled " + students[0].getName() + " on to " + modules[0].getName());
                                    students[0].addModule(modules[0].getName());
                                    modules[0].addStudent(students[0].getName());
                                    break;
                                case UFCE2:
                                    System.out.println("You have enrolled " + students[0].getName() + " on to " + modules[1].getName());
                                    students[0].addModule(modules[1].getName());
                                    modules[1].addStudent(students[0].getName());
                                    break;
                                case UFCE3:
                                    System.out.println("You have enrolled " + students[0].getName() + " on to " + modules[2].getName());
                                    students[0].addModule(modules[2].getName());
                                    modules[2].addStudent(students[0].getName());
                                    break;
                                case UFCE4:
                                    System.out.println("You have enrolled " + students[0].getName() + " on to " + modules[3].getName());
                                    students[0].addModule(modules[3].getName());
                                    modules[3].addStudent(students[0].getName());
                                    break;
                                case UFCE5:
                                    System.out.println("You have enrolled " + students[0].getName() + " on to " + modules[4].getName());
                                    students[0].addModule(modules[4].getName());
                                    modules[4].addStudent(students[0].getName());
                                    break;
                                default:
                                    System.out.println("Invalid subject selected. Returning to main menu...");
                            }
                            break;

                        case STUDENT2:
                            System.out.println("select a module to enroll " + students[1].getName() + " on to:");
                            for (int i = 0; i < modules.length; i++) {
                                System.out.println(i + " " + modules[i]);
                            }

                            selection = scan.nextInt();
                            switch (selection) {
                                case UFCE1:
                                    System.out.println("You have enrolled " + students[1].getName() + " on to " + modules[0].getName());
                                    students[1].addModule(modules[0].getName());
                                    modules[0].addStudent(students[1].getName());
                                    break;
                                case UFCE2:
                                    System.out.println("You have enrolled " + students[1].getName() + " on to " + modules[1].getName());
                                    students[1].addModule(modules[1].getName());
                                    modules[1].addStudent(students[1].getName());
                                    break;
                                case UFCE3:
                                    System.out.println("You have enrolled " + students[1].getName() + " on to " + modules[2].getName());
                                    students[1].addModule(modules[2].getName());
                                    modules[2].addStudent(students[1].getName());
                                    break;
                                case UFCE4:
                                    System.out.println("You have enrolled " + students[1].getName() + " on to " + modules[3].getName());
                                    students[1].addModule(modules[3].getName());
                                    modules[3].addStudent(students[1].getName());
                                    break;
                                case UFCE5:
                                    System.out.println("You have enrolled " + students[1].getName() + " on to " + modules[4].getName());
                                    students[1].addModule(modules[4].getName());
                                    modules[4].addStudent(students[1].getName());
                                    break;
                                default:
                                    System.out.println("Invalid subject selected. Returning to main menu...");
                            }
                            break;

                        case STUDENT3:
                            System.out.println("select a module to enroll " + students[2].getName() + " on to:");
                            for (int i = 0; i < modules.length; i++) {
                                System.out.println(i + " " + modules[i]);
                            }

                            selection = scan.nextInt();
                            switch (selection) {
                                case UFCE1:
                                    System.out.println("You have enrolled " + students[2].getName() + " on to " + modules[0].getName());
                                    students[2].addModule(modules[0].getName());
                                    modules[0].addStudent(students[2].getName());
                                    break;
                                case UFCE2:
                                    System.out.println("You have enrolled " + students[2].getName() + " on to " + modules[1].getName());
                                    students[2].addModule(modules[1].getName());
                                    modules[1].addStudent(students[2].getName());
                                    break;
                                case UFCE3:
                                    System.out.println("You have enrolled " + students[2].getName() + " on to " + modules[2].getName());
                                    students[2].addModule(modules[2].getName());
                                    modules[2].addStudent(students[2].getName());
                                    break;
                                case UFCE4:
                                    System.out.println("You have enrolled " + students[2].getName() + " on to " + modules[3].getName());
                                    students[2].addModule(modules[3].getName());
                                    modules[3].addStudent(students[2].getName());
                                    break;
                                case UFCE5:
                                    System.out.println("You have enrolled " + students[2].getName() + " on to " + modules[4].getName());
                                    students[2].addModule(modules[4].getName());
                                    modules[4].addStudent(students[2].getName());
                                    break;
                                default:
                                    System.out.println("Invalid subject selected. Returning to main menu...");
                            }
                            break;
                        case STUDENT4:
                            System.out.println("select a module to enroll " + students[3].getName() + " on to:");
                            for (int i = 0; i < modules.length; i++) {
                                System.out.println(i + " " + modules[i]);
                            }

                            selection = scan.nextInt();
                            switch (selection) {
                                case UFCE1:
                                    System.out.println("You have enrolled " + students[3].getName() + " on to " + modules[0].getName());
                                    students[3].addModule(modules[0].getName());
                                    modules[0].addStudent(students[3].getName());
                                    break;
                                case UFCE2:
                                    System.out.println("You have enrolled " + students[3].getName() + " on to " + modules[1].getName());
                                    students[3].addModule(modules[1].getName());
                                    modules[1].addStudent(students[3].getName());
                                    break;
                                case UFCE3:
                                    System.out.println("You have enrolled " + students[3].getName() + " on to " + modules[2].getName());
                                    students[3].addModule(modules[2].getName());
                                    modules[2].addStudent(students[3].getName());
                                    break;
                                case UFCE4:
                                    System.out.println("You have enrolled " + students[3].getName() + " on to " + modules[3].getName());
                                    students[3].addModule(modules[3].getName());
                                    modules[3].addStudent(students[3].getName());
                                    break;
                                case UFCE5:
                                    System.out.println("You have enrolled " + students[3].getName() + " on to " + modules[4].getName());
                                    students[3].addModule(modules[4].getName());
                                    modules[4].addStudent(students[3].getName());
                                    break;
                                default:
                                    System.out.println("Invalid subject selected. Returning to main menu...");
                            }
                            break;
                        case STUDENT5:
                            System.out.println("select a module to enroll " + students[4].getName() + " on to:");
                            for (int i = 0; i < modules.length; i++) {
                                System.out.println(i + " " + modules[i]);
                            }

                            selection = scan.nextInt();
                            switch (selection) {
                                case UFCE1:
                                    System.out.println("You have enrolled " + students[4].getName() + " on to " + modules[0].getName());
                                    students[4].addModule(modules[0].getName());
                                    modules[0].addStudent(students[4].getName());
                                    break;
                                case UFCE2:
                                    System.out.println("You have enrolled " + students[4].getName() + " on to " + modules[1].getName());
                                    students[4].addModule(modules[1].getName());
                                    modules[1].addStudent(students[4].getName());
                                    break;
                                case UFCE3:
                                    System.out.println("You have enrolled " + students[4].getName() + " on to " + modules[2].getName());
                                    students[4].addModule(modules[2].getName());
                                    modules[2].addStudent(students[4].getName());
                                    break;
                                case UFCE4:
                                    System.out.println("You have enrolled " + students[4].getName() + " on to " + modules[3].getName());
                                    students[4].addModule(modules[3].getName());
                                    modules[3].addStudent(students[4].getName());
                                    break;
                                case UFCE5:
                                    System.out.println("You have enrolled " + students[4].getName() + " on to " + modules[4].getName());
                                    students[4].addModule(modules[4].getName());
                                    modules[4].addStudent(students[4].getName());
                                    break;
                                default:
                                    System.out.println("Invalid subject selected. Returning to main menu...");
                            }
                            break;
                        default:
                            System.out.println("Invalid student selected. Returning to main menu...");
                    }
                    break;
                case 6:
                    System.out.println("Goodbye!");
                    System.exit(0);
                    break;
                default:
                    System.out.println("Invalid option selected. You must enter a number between 1 & 6!");
            } // end switch

        } catch (Exception e) {
            System.out.println("Invalid entry. You must enter a number between 1 & 6");
        }
    } // end while

}
}

Menu Class:

public class Menu {
private String Menu;

public String getMainMenu() {
    System.out.println("\n\nSelect an option:");
        System.out.println(
                "1 List all student names"
                + "\n2 List all module details"
                + "\n3 List all students on a module"
                + "\n4 List all modules attached to a student"
                + "\n5 Attach a valid student to a valid module"
                + "\n6 Exit\n");
    return Menu;
}

}

Student Class:

import java.util.ArrayList;

public class Student {
private String name;
private ArrayList<String> modules;

public Student(String name) {
    this.name = name;
    modules = new ArrayList<String>();
}

public String getName() {
    return name;
}

public ArrayList<String> getModules() {
    return modules;
}

public void addModule(String module){
    modules.add(module);
}

@Override
public String toString() {
    return name;
}

}

Module Class:

import java.util.ArrayList;


public class Module {
private String name;
private ArrayList<String> students;

public Module(String name) {
    this.name = name;
    students = new ArrayList<String>();
}

public String getName() {
    return name;
}

public ArrayList<String> getStudents() {
    return students;
}

public void addStudent(String student){
    students.add(student);
}

@Override
public String toString() {
    return name; 
}

}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ You gotta explain what the program is supposed to be doing. Also, you can use a variable for indexing in an array. That way, you'd have a bit less duplication. \$\endgroup\$ Dec 1 '11 at 19:04
  • \$\begingroup\$ Splitting the code into several well-named methods would increase readability, and help us to get an overview... \$\endgroup\$ Dec 1 '11 at 23:15
  • \$\begingroup\$ Is the code for each student in case 5 exactly the same except for the index??? \$\endgroup\$ Dec 2 '11 at 20:33
  • \$\begingroup\$ Is there a reason why you are using arrays instead of lists? \$\endgroup\$
    – Tony R
    Dec 2 '11 at 21:01
12
\$\begingroup\$
  1. There doesn't seem to be any reason for where you've split your Menu and Controller classes. Most of the menu logic is still in controller.
  2. If you ever find yourself defining variables or constants with sequential numbers like: STUDENT1, STUDENT2, STUDENT3, you are doing it wrong.
  3. Your run method is way too long. Move the code for each menu item to its own function and call them.
  4. You've got one try/catch block around the whole function. This is a bad idea as it will catch the wrong exceptions. You should only have the actual call that can fail inside the try block
  5. Your try catch block catches any kind of exception. This is a bad idea since it may hide bugs when something other then the expected function fails.
  6. I don't understand why you wrote case 5 as you did. Every case inside your switches is the same except for the number you are using to index. And you already have that number inside a variable. Just use that variable as the index and toss the switch/case.
\$\endgroup\$
6
\$\begingroup\$

(Is this homework? If it is, please tag it as such)

This isn't a bad start, but there are a number of possible improvements.

First, your Student and Module class only need some (identical) minor tweaks. They're functionally identical, so I'm only going to detail changes to the Student class.
Change:

`private ArrayList<String> modules;` 

To:

`private List<Module> modules;`  

This will allow you to store references to the exact object, not just its name. Also, you should generally code to using an interface, not a class, as this will make certain implementation changes easier.
Wrap all returns of your module list (in this case, getModules(), so it can't be modified (this will prevent some inadverdant changes):

public List<Module> getModules() {
    return Collections.unmodifiableList(modules);
}

(Look into immutable objects, although I don't necessarily recommend it for you just yet - it requires some different thinking).
Add an equals() method (and maybe hashcode() as well). Eclipse (or any other good IDE) should be able to generate it for you, although only use name for the comparisons (ignore modules this time around).

You're going to want a small utility method for adding modules to students and students to modules (in place of all the instances where they're manually written). The interface is:

public static void crossPopulate(Student student, Module module);

The actual implementation is left to the reader. You may want to check first to verify that they don't have the cross-references already...

Your getMainMenu() isn't actually getting anything (it returns null) - change the return type to void, and rename it to printMainMenu().

You should avoid the use of 'magic numbers' like the plague. The only numbers which should be used without creating constants (and even this is context sensitive) are -1, 0, 1, 2 (and sometimes 10).

In several places in your code, you mix normal for loops with foreach loops. I haven't seen anything in here that would require the normal loop. Unless you have specific need for the index itself, you should default to using a foreach loop.

As much as possible, attempt to initialize variables outside of a loop, especially if it remains constant or only provides (static) behaviour. Only initialize variables in a loop if they are expected to change.

Your static final int variables are unnecessary, eliminate them.
Instead of arrays for Students and Modules, make them maps.

Here is the (redone) version of Control. There also appear to be some safety problems with your main loop (fixed). There are much better ways to do this, but they take more time than I have right now to design (and I keep meaning to do a text-based menu system, too...).

import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;

public class Control {

    public void run() {

        Map<String, Student> students = new HashMap<String, Student>();
        students.put("jane", new Student("jane"));
        students.put("alex", new Student("alex"));
        students.put("mike", new Student("mike"));
        students.put("james", new Student("james"));
        students.put("julia", new Student("julia"));

        Map<String, Module> modules = new HashMap<String, Module>();
        modules.put("UFCE1", new Module("UFCE1"));
        modules.put("UFCE2", new Module("UFCE2"));
        modules.put("UFCE3", new Module("UFCE3"));
        modules.put("UFCE4", new Module("UFCE4"));
        modules.put("UFCE5", new Module("UFCE5"));

        Util.crossPopulate(students.get("jane"), modules.get("UFCE1"));
        Util.crossPopulate(students.get("jane"), modules.get("UFCE3"));
        Util.crossPopulate(students.get("alex"), modules.get("UFCE1"));
        Util.crossPopulate(students.get("alex"), modules.get("UFCE2"));

        System.out.println("Welcome to Student Record System!");

        Menu menu = new Menu();

        Scanner scan = new Scanner(System.in);

        while (true) {
            menu.getMainMenu();

            try {
                String selection = scan.next();
                int sel = Integer.valueOf(selection);
                switch (sel) {
                case 1:
                    for (Student student : students.values()) {
                        System.out.print(student.getName() + " ");
                    }
                    break;
                case 2:
                    for (Module module : modules.values()) {
                        System.out.print(module.getName() + " ");
                    }
                    break;
                case 3:
                    System.out.println("Search module code: ");
                    for (Module module : modules.values()) {
                        System.out.print(module.getName() + " ");
                    }
                    String searchModule = scan.next().trim();

                    for (Module module : modules.values()) {
                        if (searchModule.equalsIgnoreCase(module.getName())) {
                            for (Student student : module.getStudents()) {
                                System.out.print(student.getName() + " ");
                            }
                        }
                    }
                    break;
                case 4:
                    System.out.println("Search students : ");
                    for (Student student : students.values()) {
                        System.out.print(student.getName() + " ");
                    }
                    String searchStudent = scan.next().trim();

                    for (Student student : students.values()) {
                        if (searchStudent.equalsIgnoreCase(student.getName())) {
                            for (Module module : student.getModules()) {
                                System.out.print(module.getName() + " ");
                            }
                        }
                    }
                    break;
                case 5:
                    // Print out students
                    System.out.println("select a student: ");
                    for (String key : students.keySet()) {
                        System.out.println(key);
                    }

                    String studentSelection = scan.next();
                    Student selectedStudent = students.get(studentSelection.trim());
                    if (selectedStudent == null) {
                        System.out.println("Invalid student selected, returning to main menu.");
                        break;
                    }

                    System.out.println("select a module to enroll " + selectedStudent.getName() + " on to:");

                    for (String key : modules.keySet()) {
                        System.out.println(key);
                    }

                    String moduleSelection = scan.next();
                    Module selectedModule = modules.get(moduleSelection.trim());

                    if (selectedModule == null) {
                        System.out.println("Invalid module selected, returning to main menu.");
                        break;
                    }

                    Util.crossPopulate(selectedStudent, selectedModule);

                    System.out.println("You have enrolled "
                            + selectedStudent.getName() + " on to "
                            + selectedModule.getName());
                    break;
                case 6:
                    System.out.println("Goodbye!");
                    System.exit(0);
                    break;
                default:
                    System.out.println("Invalid option selected. You must enter a number between 1 & 6!");
                } // end switch

            } catch (Exception e) {
                System.out
                        .println("Invalid entry. You must enter a number between 1 & 6");
            }
        } // end while

    }
}
\$\endgroup\$
0
3
\$\begingroup\$

For Case 5, your psuedocode is:

1. Display all students.
2. Ask user to select a student. error if invalid student.
3. For the student selected:
   - Display all modules
   - Ask user to select a module.
   - Add selected student to module.
   - Add selected module to student.

So your code becomes:

int studentIndex = scan.nextInt();
Student tempStudent = students[studentIndex];

int moduleIndex = scan.nextInt();
Module tempModule = modules[moduleIndex];

tempStudent.addModule(tempModule.getName());
tempModule.addStudent(tempStudent.getName());

You don't NEED the temp variables but I used them to be more explicit.

The next refactoring should be to wrap the logic (and include any logging you need) in a method, i.e. 'enrollStudent'.

\$\endgroup\$
2
\$\begingroup\$
private static final int UFCE1 = 0;
private static final int UFCE2 = 1;
private static final int UFCE3 = 2;
private static final int UFCE4 = 3;
private static final int UFCE5 = 4;
// Integers represented as students
private static final int STUDENT1 = 0;
private static final int STUDENT2 = 1;
private static final int STUDENT3 = 2;
private static final int STUDENT4 = 3;
private static final int STUDENT5 = 4;

Could be

private int UFC[5];
private int Student[5];

Also you should use lists, instead of arrays

 Student[] students = new Student[]{jane, alex, mike, james, julia};

Should be a list

 List<String> students = new ArrayList<String>();

Plus, I dont get it.. What your code is suppose to be doing?

\$\endgroup\$

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.