6
\$\begingroup\$

I've just learned the OOP concept of abstraction, and I was wondering if I was implementing it correctly in my code and if you guys can point out any bad practices.

test1.java

package OOPConcepts;

import java.util.Scanner;
public class test1 {
    static public Scanner input = new Scanner(System.in);
    public static void main(String[] args) {
        String fName, lName;
        int age;
        Student newPerson = new Student();
        input.close();
    }
}

Person.java

package OOPConcepts;

public class Person
{
    static private int pNumber = 0;
    private int pID;
    public int age;
    private String type;
    public String fName, lName;

    Person()
    {
        pNumber++;
        pID = pNumber;

        System.out.println("Enter first name:");
        String fName = test1.input.next();
        System.out.println("Enter last name:");
        String lName = test1.input.next();
        System.out.println("Enter age:");
        int age = test1.input.nextInt();
        type = "person";
        describe(fName, lName, age);

        System.out.println("Made a " + type);
    }
    public void setType(String type){
        this.type = type;
    }
    public String getType()
    {
        return this.type;
    }
    public void describe(String fName, String lName, int age)
    {
        this.fName = fName;
        this.lName = lName;
        this.age = age;
        System.out.println("Name: " + this.fName + " " + this.lName + "|Age: " + this.age);
    }
}

Student.java

package OOPConcepts;

public class Student extends Person{
    static private int sNumber = 0;
    private int studentID;
    public String major;

    Student()
    {
        sNumber++;
        studentID = sNumber;
        setType("student");

        System.out.println("Who is also a " + getType());
        System.out.println("Enter major: ");
        String major = test1.input.next();
        this.major = major;
    }
    public void setStudentID(int idNumber)
    {
        this.studentID = idNumber;
    }
    public int getStudentID()
    {
        return this.studentID;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I think that the inheritance between Student and Person is not a correct relationship. While in natural language we can say a student is a type of person, but to model it in software a person is a student when they're enrolled at a school. To elaborate on what I'm describing, what if this person goes to two schools? A class such as class Student { +School school, +Person person, int studentNumber } may map better to reality. \$\endgroup\$
    – Matthew
    Apr 14, 2020 at 14:03
  • \$\begingroup\$ @Matthew: The important part is how studenthood is defined for the current application. Is it a property of a person, or is it a property of a person's enrollment in an institute? If the former, then there is no duplication issue regardless of exactly how many enrollments a person may have. If the latter, then your comment is pointing out a valid issue. \$\endgroup\$
    – Flater
    Apr 15, 2020 at 9:37

3 Answers 3

9
\$\begingroup\$

What is OOP?

There is more to OOP than just class inheritance and your example shows painfully clearly why class hierarchy is almost always completely unable to represent the real world. If a student is a specialized type of a person, then a person can not be a student and a teacher at the same time (e.g. an assistant to a teaching professor). Also, that person can no longer be a part time employee at the cafeteria either, because the person's status has been locked into being a student and student only. Clearly, class inheritance is the wrong tool here. The relation should be composition:

public class Student {
    private final StudentId studentId;
    private final Person person;

    public Student(StudentId studentId, Person person) {
        this.studentId = studentId;
        this.person = person;
    }

    ...
}
\$\endgroup\$
2
  • \$\begingroup\$ I think this is a very good point, I added a comment to the question without seeing this first. \$\endgroup\$
    – Matthew
    Apr 14, 2020 at 14:09
  • \$\begingroup\$ Hi, thank you so much for your input! How would you structure this code so that a person can have multiple occupations like Student, and Worker? I was thinking of adding an array list in the person object, and this array list will store all the occupancy objects. That way I all I have to do in order to reference to a person's occupancy is access the arraylist in the person object. I have yet to implement this in code. I wanted to ask you if there was a better approach. Thank you so much again Torben. \$\endgroup\$ Apr 20, 2020 at 20:35
8
\$\begingroup\$

Sorry, you are not implementing OOP well, since it is true that you are defining a class, declaring its fields and instantiating it, this is being done in a wrong way.

The Good

  • The abstraction is carried out properly for an exercise (because in a real design you may need more details)
  • The idea of giving a person an ID aka pNumber is something which will be closely related to ORM (Object Relational Mappings) which is a way to communicate your database(s) with your object oriented design

Review

  • Don't Declare Public Variables, you have Getter/Setter methods to access the values or modify them.
  • In the class which owns your main method is where should go all the data gathering. Doing this
    Person()
    {
        pNumber++;
        pID = pNumber;
        //...

Is a bad practice you should (must) untie the logic from the view. Even if this is concept from the MVC (model view controller) pattern it is important to do that in each project in order to improve software quality, reduce errors and having a independent work structure so that when you change the UI for the user it won't affect the logic.

The alternative is to create a parametrized constructor:

    Person(String fName, String lname, int age)
    {
        //also conssider call pID as personId (be descriptive)
        this.pID = ++pNumber;
        this.fName = fName;
        this.lName = lName;
        this.age = age;
    }

///how to use (in your main method)
    Person person = new Person("Syntëy", "Emyl", 27);
  • The variable type in Person class is something ambiguous (yes it is the type of the person but which implications does it have? which values may it take?), you should add a comment before the variable to describe it.

Some suggestions

  • Always use private fields.
  • The names given to your variables, it is quite short, it is not bad but it is better to specify them well so that your getter/setter have a quick meaning, fName, lName could be changed by firstName, lastName.
  • The static variable pNumber or sNumber could be named personIdCounter studentIdCounter
  • It is not a standard but most people works with a regular expression which matches the following: [AccessModifier] [DataType] [FieldName]; and for static variables [AccessModifier] static [DataType] [FieldName];
  • Try to not declare multiple fields (attributes) in the same line as in public String fName, lName;

Finally, I hope it has been helpful.

\$\endgroup\$
0
4
\$\begingroup\$

Miguel has made some excellent points; I won’t repeat those. Here are some more comments / suggestions:


The type field is effectively public, because you have public void setType(String type), yet it is clear that this field should not be settable externally. It shouldn’t exist at all, since we can simply ask:

if (person instanceof Student) {…}

instead of needing to check a string to see if it is equal to "student" (or was that "Student"? Or maybe it was "STUDENT"??? Maybe you should be using named constants ... or better ... an enum!)


age is a horrible field. As time goes by, it changes. If you have multiple people in a database, you are constantly updating the records. Store their birthdate, and compute their age.

\$\endgroup\$
1
  • \$\begingroup\$ imho I think that age can be a valid field, depending on the meaning/usage of the class. If it's representing a db object, I would say it's bad. If it's a domain object, a view layer class with a age field that gets for instance calculated, then it sounds acceptable to me. \$\endgroup\$ Apr 15, 2020 at 9:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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