4
\$\begingroup\$

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());
                }
            }
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ When designing OO stuff you have to consider what parts of the code you want to allow future extensions for and what parts you want to keep rigid (and simple). For example, will you ever need to search for something other then Students? Do you want to support modules that allow for more specific search functionality or are you fine with just using an O(n) iterator like that? ... \$\endgroup\$
    – hugomg
    Dec 5 '11 at 13:22
  • \$\begingroup\$ By definition, anything that's static is not OOP. \$\endgroup\$ Jun 1 '14 at 14:54
10
\$\begingroup\$
  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");
        }
        ...
    }
    
\$\endgroup\$
5
\$\begingroup\$
  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?)
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ *shudders* Pro-Tip?! There's only a "use the right tool for the job" out there, not "a Pro would use a Hashmap, but you can use an Array"...if you have other suggestions, feel free to mention and/or outline them. \$\endgroup\$
    – Bobby
    Dec 5 '11 at 22:17
  • 1
    \$\begingroup\$ @Bobby I just wanted to inject a little humor in the post. I think that I experienced, 'that word doesn't mean what you think it means'. After looking it up, I see many interpretations of that phrase can be condescending. Totally not my intent and I edited the post. \$\endgroup\$
    – Tony R
    Dec 6 '11 at 4:21
1
\$\begingroup\$

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.

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