Search module methods

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 modules, Set 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());
                }
            }
        }
    }
}

-
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.

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

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

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

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

-
Declare variables on the first use:

Scanner scan = new Scanner(System.in);

-
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

-
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 modules, final Set students) {

    final String searchedModuleName = readModuleName();
    final Module module = searchModule(searchedModuleName, modules);
    if (module == null) {
        printNoModuleFound(searchedModuleName);
        return;
    }
    final Set 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 modules) {
    for (final Module module: modules) {
        final String moduleName = module.getName();
        if (searchedModuleName.equalsIgnoreCase(moduleName)) {
            return module;
        }
    }
    return null;
}

public static Set getStudentsWithModule(final Set students, final Module module) {
    final Set result = new LinkedHashSet();
    for (final Student student: students) {
        if (student.hasModule(module)) {
            result.add(student);
        }
    }
    return result;
}

private static void printStudents(final Set students) {
    for (final Student student: students) {
        System.out.print(student.getName());
        System.out.print(" ");
    }
}

-
I'd create a StudentManager and a ModuleManager class. searchModule should be in the ModuleManager and it should use its internal Set 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 students field instead of the method parameter. It results high cohesion.

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

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

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

StackExchange Code Review Q#6550, answer score: 10