Search module methods

Question

Constructive criticism is required for the 2 methods below. I'm trying to develop better OO skills.

 // Set up scanner to allow for searching of modules 
public static String searchModule() {
    Scanner scan;
    System.out.print("Search module code: ");
    scan = new Scanner(System.in);
    String searchModule = scan.nextLine().trim();
    return searchModule;
}

// Search modules and return all students enrolled on it 
public static void moduleStudentSearch(Set<Module> modules, Set<Student> students) {
    String searchModule = searchModule();

    for (Module module : modules) {
        if (searchModule.equalsIgnoreCase(module.getName())) {
            Iterator it = students.iterator();
            while (it.hasNext()) {
                Student student = (Student) it.next();
                if (student.getModules().contains(module)) {
                    System.out.printf("%s ", student.getName());
                }
            }
        }
    }
}

Answer 1

1. I'd rename the searchModule method to readModuleName since it doesn't search, it just reads the name of the name of the searched module.

2. Instead of the iterators use the new foreach loop (as you already use it for the modules):

   for (Student student: students) {
           ...
   }
   

3. If you use iterators set the type parameter which makes the casting unnecessary:

   final Iterator<Student> it = students.iterator(); // type parameter here
   while (it.hasNext()) {
       Student student = it.next(); // no casting here
       ...
   }
   

4. Declare variables on the first use:

   Scanner scan = new Scanner(System.in);
   

5. Don't forget to close the Scanner That was a bad advice, since it closes System.in too. See also: Close a Scanner linked to System.in

6. Split up the moduleStudentSearch method, it does four things:

   • read the searched module name
   • search the module by name
   • search student by module
   • print the results

   Each function should have a separate method:

   public static void moduleStudentSearch(final Set<Module> modules, final Set<Student> students) {
   
       final String searchedModuleName = readModuleName();
       final Module module = searchModule(searchedModuleName, modules);
       if (module == null) {
           printNoModuleFound(searchedModuleName);
           return;
       }
       final Set<Student> studentsWithModule = getStudentsWithModule(students, module);
       printStudents(studentsWithModule);
   }
   
   private static void printNoModuleFound(String searchedModuleName) {
       System.out.println("No module found with name: " + searchedModuleName);
   }
   
   public static String readModuleName() {
       System.out.print("Search module code: ");
       final Scanner scan = new Scanner(System.in);
       try {
           final String searchModule = scan.nextLine().trim();
           return searchModule;
       } finally {
           scan.close();
       }
   }
   
   // you could return a list/set here if there are more than one results
   public static Module searchModule(final String searchedModuleName, final Set<Module> modules) {
       for (final Module module: modules) {
           final String moduleName = module.getName();
           if (searchedModuleName.equalsIgnoreCase(moduleName)) {
               return module;
           }
       }
       return null;
   }
   
   public static Set<Student> getStudentsWithModule(final Set<Student> students, final Module module) {
       final Set<Student> result = new LinkedHashSet<Student>();
       for (final Student student: students) {
           if (student.hasModule(module)) {
               result.add(student);
           }
       }
       return result;
   }
   
   private static void printStudents(final Set<Student> students) {
       for (final Student student: students) {
           System.out.print(student.getName());
           System.out.print(" ");
       }
   }
   

7. I'd create a StudentManager and a ModuleManager class. searchModule should be in the ModuleManager and it should use its internal Set<Module> modules field instead of the method parameter. The same is true for the getStudentsWithModule method: it should be inside the StudentManager class and it should use its internal Set<Student> students field instead of the method parameter. It results high cohesion.

   Once you have these classes, remove the static modifier of the methods.

8. Check your input: check for nulls, empty Strings etc. and throw proper exceptions (usually NullPointerException or IllegalArgumentException) with a helpful message:

   public static Set<Student> getStudentsWithModule(final Set<Student> students, final Module module) {
       if (students == null ) {
           throw new NullPointerException("students cannot be null");
       }
       if (module == null) {
           throw new NullPointerException("module cannot be null");
       }
       ...
   }
   

Answer 2

1. Since you say you're wanting to improve your OO, why are the methods static?
2. Unless the documentation says that's a safe way to use Scanner, best to assume that it might over-read into a cache and cause problems. Create one scanner around System.in and then re-use it.
3. Given the way you're using modules it should clearly be a Map<string, Module> instead. Similarly, given the way you use students it seems that each Module should have a Set<Student>.
4. Use for (Student student : students) rather than the iterator unless you need to delete. (And although it's a while since I used Java much, isn't Iterator generic?)

Answer 3

I suggest having three classes.

1. Student - represents a student. Has a name and Collection of Modules.
2. Module - represents a module. Has a name and Collection of Students.
3. Enrollment - it manages a collection of students and a collection of modules.

Example usage

Student jack = new Student("Jack");
Student jill= new Student("Jill");

Module english = new Module("English");
Module math = new Module("Math");

Enrollment enrollment = new Enrollment();
enrollment.addStudent(jack);
enrollment.addStudent(jill);

enrollment.addModule(english);
enrollment.addModule(math);

enrollment.enroll(jack, english);

enrollment.enroll(jill, math);
enrollment.enroll(jill, english);

Now you have to ask yourself these questions:

• if I have a student jack, does jack know what modules/classes he's taking? In other words, does a student know its modules?

• similar to the above but in the opposite direction: does a module know its students?

Your answer to these questions will drive your design. Be careful because some choices may make things terribly complicated to code so you'll want to rethink your design :)

I wanted to be able to do the following:

1. Create a student. A student can have 0 or one modules; aka a student can exist with 0 modules defined/created.

2. Create a module. A module can have 0 or one students; aka a module can exist with 0 students defined/created.

3. Perform queries like how many students are in module 'math'? how many modules is the student 'jack' enrolled in. So I created the Enrollment class.

Answer 4

From just looking at it without knowing what your intentions are, I'd shorten the first method to:

public static String searchModule() {
    System.out.print("Search module code: ");
    Scanner scanner = new Scanner(System.in);
    return scanner.nextLine().trim();
}

The second method is badly named, maybe something like searchStudentModules. I'm also not sure why you don't use a foreach loop like you did for the modules.

public static void moduleStudentSearch(Set<Module> modules, Set<Student> students) {
    String searchModule = searchModule();

    for (Module module : modules) {
        if (searchModule.equalsIgnoreCase(module.getName())) {
            for (Student student : students) {
                if (student.getModules().contains(module)) {
                    System.out.printf("%s ", student.getName());
                }
            }
        }
    }
}

Additionally, usage of JavaDoc is always preferable.

But also listen to Peter Taylor, who has some valid points.