Entering pupil information

Question

I have the following code which is located within a case, in a switch statement. Its goal is to add a Pupil to a Subject using the Collections in the respective class.

I've tried to make the code as readable as possible, but I'm sure there's a better approach.

  System.out.println("Adding a Pupil...");


  String AddPupilSubj;
  do {

      UserInput.prompt("\nEnter the Subject Code to add the Pupil to, or enter 'list' to list all Subjects: ");
      AddPupilSubj = UserInput.readString();

      if(AddPupilSubj.equals("list"))
        school.listSubjects();

  } while(AddPupilSubj.isEmpty() || AddPupilSubj.equals("list"));


  Boolean found = false;

  // Locate the Subject   
  for(Object obj : school.getSubjects()) {
      Subject subj = (Subject) obj;

      if(subj.getCode().equals(AddPupilSubj)) {

        found = true;

        // Get the ID of the Pupil
        UserInput.prompt("Enter Pupil ID: ");
        int id = UserInput.readInt();

        // Get the Pupil's Name
        UserInput.prompt("Enter Pupil Name: ");
        String name = UserInput.readString();

        // Get the Address of the Pupil
        UserInput.prompt("Enter Pupil Address: ");
        String address = UserInput.readString();

        String date; Boolean valid;
        do {

             // Get the Pupil's Date of Birth
            UserInput.prompt("Enter Pupil Date of Birth (dd/mm/yyyy): ");
            date = UserInput.readString(); 

            valid = validDOB(date);

        } while(!valid);


        // Get the Pupil's email address
        UserInput.prompt("Enter Pupil Email: ");
        String email = UserInput.readString();


        DateTime dob = DateTime.forDateOnly(Integer.parseInt(date.substring(6, 10)), 
                  Integer.parseInt(date.substring(3, 5)),
                  Integer.parseInt(date.substring(0, 2)));

        DateTime admitted = DateTime.today(TimeZone.getDefault());


        Pupil Pupil = new Pupil(id, name, address, dob, email, admitted);
        if(subj.addPupil(Pupil)) {

             System.out.println(
                 String.format("\nPupil, %s, has been added to the %s Subject.", 
                               Pupil.getName(), 
                               subj.getName()));  

        } else {

             System.out.println(
                 String.format("Pupil #%s is already in the Subject!", 
                               Pupil.getID()));

        }


        break;   
      }


  }


  if(!found) {
      System.out.println(
        String.format("A Subject with code, %s, doesn't exist!", 
          AddPupilSubj));
  }

Answer 1

The function is very long.

Functions should do one thing, do it completely, and do it well. Long functions are difficult to reason about.

Move the subject and pupil data entry into separate functions to isolate the functionality, and streamline the mainline code.

I'm assuming the presence of String getSubjectCode() and Pupil getPupilInformation() methods, each containing the code currently in the mega-function.

Naming conventions

Minor, but you call an instance of the Pupil class Pupil, which is confusing at best, misleading at worst. Non-static-final variables should always begin with lowercase letters.

Use typed collections

School.getSubjects() should return a List<Subject> (or Collection). (Or at least a properly-typed array; nothing with Objects.)

Reduce the amount of code in conditionals

There's a lot of code in block that loops over the school's subjects, most of which is the block for when the subject is found.

In order to understand what happens if the subject isn't found, I have to scroll down to the end of that block, make sure I'm reading the indentation correctly (assuming the indentation is correct), only to find there's nothing there.

In cases like that, might as well do a negative test and continue, slightly "flattening" the loop's guts. (Code turned on its side is not a graph of how awesome it is ;)

That said, this functionality is available in a contains method--looping in the mainline code isn't necessary. See next.

Localize functionality

Deciding if a school has a given subject should be moved into the School class, for example:

public class School {
    // ... existing functionality elided ...

    /** Returns subject with given code, or null if not found. */
    public Subject findSubjectByCode(String code) {
        for (Subject s : subjects) {
            if (s.getCode().equals(code)) {
                return s;
            }
        }
        return null;
    }
}

The refactored mainline code

All the above turns the mainline code into the following:

System.out.println("Adding a Pupil...");

String subjectCode = getSubjectCode();
Subject schoolSubject = school.findSubjectByCode(subjectCode);
if (schoolSubject == null) {
    System.out.printf("A Subject with code, %s, doesn't exist!%n", subjectCode);
    return;
}

Pupil pupil = getPupilInformation();
if (schoolSubject.addPupil(pupil)) {
    System.out.printf("%nPupil, %s, has been added to the %s Subject.%n", pupil.getName(), subj.getName());
} else {
    System.out.printf("Pupil #%s is already in the Subject!%n", pupil.getID());
}

IMO this is more communicative, cleaner, and easier to understand.

Answer 2

Lots of work here to do! I'll try to get you started on refactoring this:

Clarify Your Intent

I'm not seeing a class or method definition here. Please declare at least one of these so as to give your reader a clue on what this code is attempting to do.

One way to extract your class and method names is to examine your logging or println's.

"Adding a Pupil..."

A-HA! So your first step is to make a class called "Pupil" and a method "add" (or "addPupil"). Make sure that the method "add" adds a pupil to a List of pupil objects.

NOTE: If you're getting a funny feeling about "adding a pupil to a pupil" and how it seems strange, make note of it and come back to it later because we're not done yet! (i.e. perhaps there's some more abstractions to find...)

Extract Classes

The goal here is to create the appropriate abstractions. One guiding principle you can use is the Single Responsibility Principle, a class should have a single reason to change. I'm going to continue searching your println's/comments for possible hints.

• I see "Enter the Subject Code" in the prompt. So now you have a Subject class! (already there? great!)

• Add the following fields to Pupil: int id, String firstName, String lastName, String address*, String* dateOfBirth.

• I see an operations on a school variable yet one is not defined. Perhaps there is a School object? So now we can move the list of Pupils to School.

*Address should be its own class with separate fields for street1, street2, city, state, and zip.

*eventually convert dateOfBirth to a date format; I see you're using DateTime.

Move Logic Into Classes

Make methods out of existing conditionals and place in the appropriate class. If you don't have a class that it fits in, make a new class.

For example, this code snippet could be encapsulated in a method:

DateTime createDateOfBirth (String date)

DateTime dob = DateTime.forDateOnly(Integer.parseInt(date.substring(6, 10)), 
                  Integer.parseInt(date.substring(3, 5)),
                  Integer.parseInt(date.substring(0, 2)));

Minor cleanup

Make small changes like renaming fields, moving field declarations (limiting local variable scope), or make helper functions to improve your code readability.

• "String AddPupilSubj" should be lowercase. Possible that this lives 'too long' as well.

• Modify "if(AddPupilSubj.equals("list"))":

  1. change to "list".equals(AddPupilSubject)
  2. add AddPupilSubject.toLowerCase() since you are comparing it to "list".
  3. possibly call AddPupilSubject.trim() as well to delete any whitespace so that way if someone entered "list ", your function will work.

Answer 3

Seems to me like you have a couple functions here that need pulled out:

1. findSubject
2. collect input and validate
   • I'd also pull out a function for anything more involved like the date prompts that need validated
3. create pupil and add

Other questions:

• are there other places that a pupil gets input? you should avoid duplicating this code
• why note have validDob return a valid DOB instead of having two different version of this code 1) validates, 2) converts to Date object

Answer 4

I would use a couple helper methods. Most importantly, you really shouldn't have this much within a switch case, put it into a method and call the method from the switch. Then, I would put all the code for inputting the pupil information and creating the pupil (from the ID prompt through the new Pupil(...) into a separate method and return the new pupil; possibly even make it a constructor for Pupil.

Answer 5

I would extract out the input/output handling logic to an UserInputOutput interface:

public interface UserInputOutput {

    String readCommand();

    int readPupilId();

    String readPupilName();

    String readPupilAddress();

    Date readPupilBirthDate();

    ...
}

then create a ConsoleUserInputOutput class which implements the interface and pass a reference to the code.

It means looser coupling and if you write some test code you can easily create a mocked class which emulates the user input/output and you can create a TouchScreenUserInputOutput class for touch screens without the need of modification of your class.