Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:");
                    major = scan.nextLine();
                    Student stu = new Student(name, dateOfBirth, major);
                    System.out.println(stu.toString());
                    break;
                case 2:
                    //Create Instructor
                    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("Salary:");
                    try
                    {
                        salary = Double.parseDouble(scan.nextLine());
                    }
                    catch(IllegalArgumentException ex)
                    {
                        System.out.println(String.format("Ooops! Message: \n%s", ex.getMessage()));
                        continue;
                    }
                    Instructor ins = new Instructor(name, dateOfBirth, salary);
                    System.out.println(ins.toString());
                    break;
                default:
                    System.out.println(String.format("You have selected an invalid number %s", selection));
                    continue;
            }      
        }while(!done);
        System.out.println("See ya!");
    }
}

Person

package com.exercise.inheritance1;

import org.joda.time.LocalDate;

public class Person {

    protected String name;
    protected LocalDate dateOfBirth;

    public Person(String name, LocalDate dateOfBirth)
    {
        setName(name);
        setDateOfBirth(dateOfBirth);
    }

    private void setName(String name)
    {
        this.name = name;
    }

    public String getName()
    {
        return this.name;
    }

    private void setDateOfBirth(LocalDate dateOfBirth)
    {
        this.dateOfBirth = dateOfBirth;
    }

    public LocalDate getDateOfBirth()
    {
        return this.dateOfBirth;
    }

    @Override
    public String toString()
    {
        return String.format("%s: %s\n%s: %s",
                "Name", this.getName(), "Date of Birth", this.getDateOfBirth());
    }


}

Student

package com.exercise.inheritance1;

import org.joda.time.LocalDate;

public class Student extends Person {

    private String major;

    public Student(String name, LocalDate dateOfBirth, String major)
    {
        super(name, dateOfBirth);
        setMajor(major);
    }

    private void setMajor(String major)
    {
        this.major = major;
    }

    public String getMajor()
    {
        return this.major;
    }

    @Override
    public String toString()
    {
        return String.format("%s\n%s: %s",
                super.toString(), "Major", this.getMajor());
    }

}

Instructor

package com.exercise.inheritance1;

import org.joda.time.LocalDate;

public class Instructor extends Person {

    private double salary;

    public Instructor(String name, LocalDate dateOfBirth, double salary)
    {
        super(name, dateOfBirth);
        setSalary(salary);
    }

    private void setSalary(double salary)
    {
        this.salary = salary;
    }

    public double getSalary()
    {
        return this.salary;
    }

    @Override
    public String toString()
    {
        return String.format("%s\n%s: %s",
                super.toString(), "Salary", this.getSalary());
    }

}
share|improve this question

3 Answers 3

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.

share|improve this answer

It seems to be good enough but you can

  1. Add more comments
  2. Add more methods to your main. Examples:

    • public Student createStudent()
    • public Instructor createInstructor()

    This will help you have a clearer Case Section

There are many ways to code your project because of the simple task. Just try to make it clearer so you can read it like you were reading a book or a story :)

I didn't get what you meant by Major? If it's a Person then you can use a Major object to handle this instead of a string. Thus you'll pass an object to the constructor which is another Person (the Major) assuming you extended this Major object by the Person object (since a Major should be a Person: class Major extends Person).

Don't forget to put a space in "Major" (change it to "Major: "):

super.toString(), "Major: ", this.getMajor();

PS: same for the other classes: Person, Instructor, ...

share|improve this answer
    
A major is not a person, but a primary area of study for a university student. –  robjb Jan 28 '12 at 4:24
    
Ok sorry for this :). Btw you can create and object for that. –  Kursion Jan 28 '12 at 12:54

You could use the Apache Commons CLI library.

http://commons.apache.org/cli/

It abstracts out the option parsing and usage printing. All you have to do is to define the Options your console app provides and bind them to keywords.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.