Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have made a student details project. The user is asked to enter the info for two students, which is then displayed on the screen.

Questions:

  1. Have I used inheritance correctly?
  2. What would be a better way of outputting the results?

 public class Person extends Test
    {
        // Variables

            String name;
            int age;
            String address;

            // Default Constructor

            Person()
            {
                name = "";
                age = 0;
                address = "";
            }

            Person(String name, int age, String address)
            {
                this.name = name;
                this.age = age;
                this.address = address;

            }

            String getName()
            {
                return name;
            }


            public void display()
            {

                System.out.println("Name = "+ name);
                System.out.println("Age = "+ age);
                System.out.println("address = "+ address);



            }

    }


public class Student extends Person
{

    int studentNum, semester;

    Student(String name, int age, String address, int studentNum, int semester)
    {
        super(name, age, address); // calls parent class’s constructor 
        this.studentNum = studentNum;
        this.semester = semester;
        //this.course = course;

    }

    public String getName() // name
    {
        return name;
    }

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

    public int getAge() // age
    {
        return age;
    }

    public void setAge(int age)
    {
        this.age = age;
    }

    public String getAddress() // address
    {
        return address;
    }

    public void setAddress(String address) 
    {
        this.address = address;
    }

    public int getStudentNum() // studentNum
    {
    return studentNum;
    }

    public void setStudentNum(int studentNum) 
    {
        this.studentNum = studentNum;
    }

    public int getSemester() // semester
    {
    return semester;
    }

    public void setSemester(int semester) 
    {
        this.semester = semester;
    }





    void Display() // Method Overriding
    {



    }





}



public class Course extends Student
{
    String course;

    Course(String name, int age, String address, int studentNum, int semester, String course) 
    {
        super(name, age, address, studentNum, semester);
        this.course = course;


    }

    public void display()
    {       

    }

}

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.List;

public class Test  implements StudentFees
{
    public static void main(String args[ ]) throws IOException
    {
        System.out.println("============" + "=================");
        System.out.println("Students " + "Personal Details");
        System.out.println("============" + "=================");

        String name, address, course;
        int age, studentNum, semester;

        List<Student> studentsList = new ArrayList<Student>(); // array list to store user input



        for (int i = 0; i < 2; i++) 
        {

        int studentNumber = (i + 1);

        //System.out.println("");
        //System.out.println("Please enter " + "data for student " + studentNumber);

        InputStreamReader converter = new InputStreamReader(System.in);
        BufferedReader in = new BufferedReader(converter);

        System.out.println("Enter Student "+ studentNumber + " Name:"); 
        name = in.readLine();

        System.out.println("Enter Student " + studentNumber + " Age (Integer):");
        age = Integer.valueOf(in.readLine());

        System.out.println("Enter Student " + studentNumber + " Address:");
        address = in.readLine();

        System.out.println("Enter Student " + studentNumber + "  Number:"); 
        studentNum = Integer.valueOf(in.readLine());

        System.out.println("Enter Student " + studentNumber + " Semester:");
        semester = Integer.valueOf(in.readLine());

        System.out.println("Enter Student " + studentNumber + " Course:");
        course = in.readLine();


        Student student = new Student(name, age, address, studentNum, studentNum);

        studentsList.add(student); // add student
        }

        for (int j = 0; j < studentsList.size(); j++)
        {

            Student st = studentsList.get(j);

            System.out.println("Information of Student : " + (j + 1)); 
            System.out.println("");
            System.out.println("Name: " + st.getName() + " - Age: "+st.getAge() + " - Address: " + st.getAddress() + " - Student Number: " + st.getStudentNum() + " - Semester: " + st.getSemester() +  " - Course: " + st.getCourse()); // print out results entered by user
            System.out.println("");
        }

    }

    String course;

    public String getCourse() 
    {
        return course;
    }

    public void setCourse(String course) 
    {
        this.course = course;
    }

    public void payFees(float fees) 
    {

    }

}
share|improve this question
3  
Course definitely should not extend Student. Inheritance represents an is-a relationship, and a Course is not a Student. – S.L. Barth Dec 9 '12 at 8:43
    
Ok should I make any new classes like EveningCourse extends course? – David Needham Dec 9 '12 at 14:33
    
If you need a new class EveningCourse, it should extend Course. Don't make them until you need them. – S.L. Barth Dec 11 '12 at 9:23

Alright, let's go through everything that is wrong with this code...

public class Person extends Test

^ A person is not a test. ^

// Variables

            String name;

^ Too much vertical white space. ^

        String name;
        int age;
        String address;

        // Default Constructor

        Person()
        {
            name = "";
            age = 0;
            address = "";
        }

^ Very verbose. You can set the default values at the site of variable declaration and leave the default constructor empty. ^

        String name;
        int age;
        String address;

^ You didn't specify that these member variables are private. They are public by default (which is unacceptable except for final values). ^

        public void display()
        {

            System.out.println("Name = "+ name);
            System.out.println("Age = "+ age);
            System.out.println("address = "+ address);



        }

^ Waaaaay too much vertical whitespace. ^

int studentNum, semester;

^ Avoid declaring multiple variables on one line in Java. It hampers readability and promotes using the same modifiers for all your variables when in reality they should be different (ex. final, volatile, etc). ^

public void setAge(int age)
{
    this.age = age;
}

^ The whole point of not accessing variables directly is to be able to control how they are modified. In this case, it doesn't make sense to change your age to any integer. You probably want to get rid of the setter and instead use a method that increments your age (assuming that your age can't ever go down). At the very least check to make sure that your age isn't negative.

void Display() // Method Overriding
{



}

^ Don't leave a comment saying "// Method Overriding"". Instead, on the above line, write "@Override" (without the quotation marks). ^

public class Course extends Student

^ BIG HUGE TERRIBLE MISTAKE. NEVER EVER EVER EVER USE INHERITANCE THIS WAY. A course is not a specific type of student. Student has an instance of course. Student has an array of courses and student should be able to add and drop courses from the array.^

public class Test  implements StudentFees

You're using inheritance wrong. Every time you use inheritance, ask yourself "is type X a subclassification of type Y".

Example:

public class Dog extends Animal

Is type dog a sub-classification of type animal? Are all dogs animals? Yes. Good. We are using inheritance the right way.

Now try this:

public class Fish extends Bird

Is type fish a sub-classification of type bird? Are all fishes birds? No. Bad. We are using inheritance the wrong way.

share|improve this answer
    
You wrote a good answer Michael! Though it's the first time I see this kind of writing style (Using ^^), I'm not 100% it's the best format to write answers. But, you're answer is great! – TopinFrassi Oct 21 '15 at 13:21
    
I most certainly agree with TopinFrassi on both points! Excellent answer! I would also consider using a line separator and headings instead of ^^. The line separator in Markdown is formatted by three hyphens (---), and headings with #. – Quill Oct 21 '15 at 13:25

Talk yourself through your "problem domain" and write it as you go. Say a little, write a little. The details will come along. Don't worry about them all at once. Make sure the little you write each time compiles.

For example

Well, there's a Person

public Class Person {}

Is a Student a Person? Yes. Is a Person a student? Well, sometimes.

public Class Student extends Person {}

What's the basic stuff of a Person/Student? name, age, address, studentID, semester....

public class Student extends Person () {
    public string Name;
    public string Age;
    public string Address;
    public string ID;
    public string semester;
}

Oh, some of those belong to all Persons, some are students only

public class Person () {
        public string Name;
        public string Age;
        public string Address;
}

public class Student extends Person () {
        public string ID;
        public string semester;
}

Students take courses. I need a course class. Is a course a student? no. vice versa? no. Oh - a student takes a course. Or a student has courses - has means composition not inheritance

public class Student extends Person () {
            public string ID;
            public string semester;
            ArrayList courses;
 }
 public class Course {} // don't know what's inside this yet.

OK, You get the idea.

Displaying

Your basic idea of making strings of "label: value" is the right idea, but implemented wrong. And once implemented - use it!!

All objects have a toString() method. Override it. Then all you have to "say" to use it is System.out.println(someStudent.toString()) BAM! object oriented programming. Also, DO NOT use println in your toString(). toString() should build all the object information into a single formatted string and pass that. Then you use it as shown.

// in the Person class
public override string toString(){
   string me = "";
   me += "Name: " + this.Name + "\n"  //new line
   me += "Age: " + this.Age + "\n";
   me += "Address: " + this.Address + "\n";
   return me;
}

Now, what class would this go into? Person. What about Student? The above already does most of what we need for Student too. Do we duplicate it in Student class? NO! Object Oriented Design to the rescue!

// in the Student class
public string toString(){
    string me = "";
    me += base.toString();
    me += "semester: " + this.semester + "\n";
    ......
    return me;
}
share|improve this answer

You must remember that inheritance is good to avoid code duplication, but also you must know that you will have messy code if you will extends for one field or method, they must be connected logically Student extends Person it's OK, Cat extends Dog isn't OK. I think you don't need make Person extends Test.

share|improve this answer

There is an approach: prefer composition instead of inheritance.

Inherit only and only when you need to "avoid copy-paste" or use the same behaviour(the same method) and extend them.

Student is a Person - ok. Course have few Students - it is a composition(Student should be a field in class Course, and will be collection of students - List), not a inheritance.

Why you did " public class Person extends Test" is unclear for me, can not explain it at all.

share|improve this answer

Already said: Course should probably not extend a student. Always ask yourself: Is subclass a special thing of a parentclass?

You could add a List<Course> courses attribute to Student and a List<Student> students attribute to Course (Both if n:m, only one if 1:n).

Depending on the requirements, you could override the toString() method, then you could write print(student) instead of all the concatenations.

Try to have a look at the Scanner class and some examples. It makes command line reading much easier.

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.