Creating students and instructors

It's simple. Person is a superclass. Student and Instructor are its subclasses. The program suppose to allow user create a student or an instructor.

Run

```
package com.exercise.inheritance1;

import java.util.Scanner;
import org.joda.time.LocalDate;

public class Run {
public static void main(String args[])
{
Boolean done = false;
do
{
System.out.println("Select one of the following:");
System.out.println("\t1 -- Create Student");
System.out.println("\t2 -- Create Instructor");
System.out.println("Enter your selection (0 to exit): ");
Scanner scan = new Scanner(System.in);
String userInput = scan.nextLine();
int selection;
String name;
LocalDate dateOfBirth;
String major;
Double salary;
try
{
selection = Integer.parseInt(userInput);
}
catch(Exception ex)
{
System.out.println("What?");
System.out.println(ex.getMessage());
continue;
}
System.out.println("You selected " + selection);
switch(selection)
{
case 0:
done = true;
break;
case 1:
//Create Student
System.out.println("Name:");
name = scan.nextLine();
System.out.println("Birthdate(YYYY-MM-DD):");
try
{
dateOfBirth = LocalDate.parse(scan.nextLine());
}
catch(IllegalArgumentException ex)
{
System.out.println(String.format("Ooops! Message: \n%s", ex.getMessage()));
continue;
}
System.out.println("Major:");

Two things: if you enter the date wrong, you reset the input. Not a big deal since there are only two fields, but if it was 15 fields, it would be really annoying. Secondly, when entering dates, it's often better to ask the user to enter as three separate fields (YYYY, MM, DD) rather than having them format the string (it's ok to ask them in something that they can edit, but here it's "write once".

. . .

Generally it's good practice to declare variables as late as possible. When you expose things like

String name;
LocalDate dateOfBirth;
String major;
Double salary;

there is no guarantee of state. Sometimes some of the variables are unset, sometimes they are all set, etc. Move them into the case statement.

. . .

Another point is not to give exception messages to the user. Format it nicely, log it, but don't show the raw exception message.

. . .

Good call on using Joda, Java Dates are terrible.

. . .

Generally it is not a good idea to switch on integer flags. It's fine for input, but as the code grows, it becomes less maintainable. You have two states right now, imagine how difficult it would be to maintain that if statement with 15 different modes.

try
    {
        selection = Integer.parseInt(userInput);
    }
    catch(Exception ex)
    {
        System.out.println("What?");
        System.out.println(ex.getMessage());
        continue;
    }

Instead, it might be better expressed as:

enum EditMode { UNKNOWN, EXIT, CREATE_STUDENT, CREATE_INSTRUCTOR }
...
EditMode editMode = EditMode.UNKNOWN;
try {
    String userInput = scan.nextLine();
    selection = Integer.parseInt(userInput);

    if(selection == 0) { editMode = EditMode.EXIT; }
    else if(selection == 1) { editMode = EditMode.CREATE_STUDENT; }
    else if(selection == 2) { editMode = EditMode.CREATE_INSTRUCTOR; }

 } catch (Exception e) {
     //... do nothing. log an error.
 }
 ...
 //Now you switch on EditMode, and it's much more clear what is going on.

This also protects you from accidentally looking at invalid modes.

Getting more advanced, in Java enums are static final children of Enum, so you can have methods and constructors on them. In particular, you can move the logic from your switch statement to within the enum types. This is generally referred to as the "strategy pattern".

enum EditMode { UNKNOWN, EXIT, CREATE_STUDENT, CREATE_INSTRUCTOR }

becomes

enum EditMode {
    UNKNOWN {
        //Note: we aren't passing in the actual input here.
        void handleEdit() { System.out.println("You have selected an invalid option"); }
    },

    EXIT {
        void handleEdit() { /* Do nothing. Handle this specially */ }
    },

    CREATE_STUDENT {
        void handleEdit() { /* Code from "case 1:" goes here */ }
    },         
    CREATE_INSTRUCTOR {
        void handleEdit() { /* Code from "case 2:" goes here */ }
    };

    abstract void handleEdit();
}

with this, your main loop becomes much simpler.

while(true) {
    EditMode editMode = EditMode.UNKNOWN;
    try {
        String userInput = scan.nextLine();
        selection = Integer.parseInt(userInput);

        if(selection == 0) { editMode = EditMode.EXIT; }
        else if(selection == 1) { editMode = EditMode.CREATE_STUDENT; }
        else if(selection == 2) { editMode = EditMode.CREATE_INSTRUCTOR; }
    } catch (Exception e) {
         //... do nothing / log an error. editMode will be UNKNOWN, so you can
         // pretty much just ignore the exception.
    }

    if(editMode == EditMode.EXIT) { break; }
    editMode.handleEdit();
}

which is much cleaner, I think.

Notice however that UNKNOWN no longer displays the user input. Using an enum, the only option to fix this is to have an argument to handleEdit - which would be ignored by most others. The other choice is to create actual objects for your strategies, subclassing each one, and then creating them while inspecting the input.

StackExchange Code Review Q#8375, answer score: 3