Entering pupil information

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

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

StackExchange Code Review Q#6139, answer score: 7