Ytukyg

I am an extreme novice in Java and thus trying to practise some online exercises. One of them involved creating a basic student record system.
This is what I've come up with but I think a lot of things are quite redundant in my code so any reviews are greatly appreciated.

Student.java

class Student {



    int rollno;

    String name;



    Student(int r, String n) {

        this.rollno = r;

        this.name = n;

    }

}

StudentDB.java

import java.util.Scanner;



class Main {



    public static void getStudents(Student ...students) {

        int i = 1;

        for (Student x : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> "+ x.name);

            System.out.println("Roll Number -> " + x.rollno + "n");

            i++;

        }

    }



    public static void main(String args) {



        Scanner sc = new Scanner(System.in);



        System.out.print("Number of students? ");

        int num = sc.nextInt();



        String name = new String[num];

        int rollno = new int[num];



        System.out.println("Enter those " + num + " students one by one");



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

            System.out.println("Enter details for student no. " + (i + 1));

            System.out.print("Enter Name: ");

            name[i] = sc.next();

            System.out.print("Enter Roll Number: ");

            rollno[i] = sc.nextInt();

            System.out.println("n");

        }



        Student students = new Student[num];



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

            students[i] = new Student(rollno[i], name[i]);

        }



        getStudents(students);

    }

}

I would really love it if someone could tell me as to how can I pass those name and rollno arrays directly to the vararg function getStudents without creating that Student object array.

Your code is efficient, and luckily your problem domain is quite limited, which is also good to get early feedback and then improve the program structure before you go on to write complex systems with millions of lines of code.

The Student class

Let's start with the very small details, the Student class. You wrote:

class Student {



    int rollno;

    String name;



    Student(int r, String n) {

        this.rollno = r;

        this.name = n;

    }

}

For the two fields, you chose to list the rollno first and the name second. This is good since rollno is the identifying field of the student, which should always come first. The describing fields should then follow, as you do in your code.

It looks strange that the fields have these pretty names but the parameters of the constructor are abbreviated for no reason. The fields, as well as their names, are considered an implementation detail and should not be visible outside the Student class. Therefore, their names are limited in scope.

The parameters of the constructor, on the other hand, are part of the "API" (application programming interface) of the Student class. These names are important so they should be chosen carefully. It is common to name the constructor parameters exactly the same as the fields. So that should be Student(int rollno, String name).

Later, when your code becomes more than a toy program, you will need to add getter and setter methods for the fields of the Student class. This is a convention in the Java world. It's a lot of code to type, but people got used to that. In fact, the IDE (integrated development environment) will generate this code for your. It is called boilerplate code because it has more text than would be ideally necessary.

Later, your Student class should look like this:

public class Student {   // The "public" is new here.



    private final int rollno;   // The "private" and "final" is new here.

    private String name;        // The "private" is new here.



    public Student(int rollno, String name) {   // The "public" is new here

        this.rollno = rollno;

        this.name = name;

    }



    public int getRollno() {

        return rollno;

    }



    public String getName() {

        return name;

    }



    public void setName(String name) {

        this.name = name;

    }

}

I made the rollno field final because once chosen, it will never change. The name of a student, on the other hand, may change. It doesn't occur very often, but it can. Therefore there is a combination of the getName/setName methods.

Above I mentioned the boilerplate code since this code is much longer than your current code but doesn't really contain more information. It could be written much shorter, if only the Java programming language would support it. Maybe it will in some future, but currently it doesn't. There is a similar programming language called Kotlin, in which the equivalent code looks like this:

data class Student(

    val rollno: Int,

    var name: String)

That's it. There is no repetition anymore, and this short code can be automatically translated to the above bloated Java code because the Java code doesn't contain any more real information than this very short code.

The StudentRoll class (Katie's version)

(Note: I accidentally reviewed not the code from the question, but the code from Katie's answer. Since I don't want this work to be wasted, I'm leaving it as is. See below for the review of the original class.)

class StudentRoll {



    static Student gatherStudents(Scanner scanner){

I noticed that you wrote ){ without any space in-between. That's unusual but is nothing you should worry about. Since the rest of your code is perfectly formatted (the indentation from the left is consistent, as well as the spacing between the tokens that make up the code), I assume that you are either very disciplined or are using an IDE that formats the code for you. Either of these is good. The IDE should do this boring task of layouting the code so that you can concentrate on what the code means instead of what it looks like.

        System.out.print("Number of students? ");

        int numberOfStudents = scanner.nextInt();

This pattern of asking for the number of items first and then reading the items one by one is very typical of textbooks and introductory programs. It is impractical for the user of your program, though. Does the user really know in advance how many students there will be? Probably not. Therefore the program should rather ask the user to "enter the student rollno, leave it empty to finish the input".

        System.out.println("Enter those " + numberOfStudents + " students one by one");

        Student students = new Student[numberOfStudents];

Introductory programming courses typically use arrays since technically they are simpler to understand than Java classes and the whole collections framework. Arrays are harder to use though since you need to know the number of elements when you create the array.

By using a List<Student> (pronounced: list of student) instead of a Student, you can simply say students.add(student) and don't have to worry whether the list is large enough. This is much simpler.

        for (Student student : students) {

            System.out.println("Enter details for the next student no. ");

            System.out.print("Enter Name: ");

            student.setName(scanner.next());

            System.out.print("Enter Roll Number: ");

            student.setRollNumber(scanner.nextInt());

            System.out.println("n");

In the Student class, you wrote the rollno first, for whatever reason. I thought it was because the rollno is the identifying field. But now it gets weird: you let the user enter the fields of the student in a different order than that in the Student class. This is a bad smell. It is not necessarily wrong, but it draws attention. Why do you do this. This should be explained in a comment for the human readers of your code.

The prompt to "Enter details for the next student no. " has an unnecessary space at the end. That is not visible and it has no meaning, therefore you should remove it. Before that space is "no.", which is visible and it is wrong. The user is going to enter the details for the next student, not for its number. Maybe it was "no. " + i in a previous version of the code, where i was a sequence number, and that made sense. The current version of the code doesn't make sense.

To get the user's input, you use scanner.next(). This is almost always wrong. Currently, when the user is asked to enter a name and just presses Enter, the user expects that something has been entered. Yet, the program still waits for "something", since calling scanner.next() means "read from the input until the next word".

When the user enters a double name, only the first name will end up in student.name, the other name will still be in the input queue, waiting to be read in later. Therefore, instead of scanner.next(), use scanner.nextLine().

The same applies for the rollno. When the users just presses Enter here, the program continues to wait for a word to be entered, without giving any intermediate feedback. This makes a bad user experience since the user and the program have a different idea about what is happening right now. Therefore, instead of scanner.nextInt(), use Integer.parseInt(scanner.nextLine()).

Error handling will be handled later, that would be too large a topic for now.

For the benefit of readers of your code, you should add an empty line before the "Enter roll number" line. This will structure your code into paragraphs, just like in a good book. This video explains why this is a good thing.

        }

        return students;

    }



    static void printStudents(Student students){

        int i = 1;

        for (Student x : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> " + x.getName());

            System.out.println("Roll Number -> " + x.getRollNumber() + "n");

            i++;

        }

    }

}

The printStudents method prints the students details, each separated by an empty line. This empty line is not visible when looking only at the beginnings of the code lines.

Experienced programmers often only read the first word of each line to get a general idea of what the code does. In this example, it would be:

• for Student


• System.out.println


• System.out.println


• System.out.println

From that, readers of your code will conclude that for each student, there are 3 lines printed. But this is not what the program does. It prints 4 lines. Therefore the code should look like this:

    static void printStudents(Student students) {

        int i = 1;

        for (Student student : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> " + student.getName());

            System.out.println("Roll Number -> " + student.getRollNumber());

            System.out.println();

            i++;

        }

    }

There you have it, four times println, four lines of output. Now the code has the same structure as the output it produces. No more surprises for the reader.

By the way, there are subtle differences between println() and printing "n", but only on Windows systems. When you write n in your code, the Windows Notepad editor will not show the linebreak that should be there. Granted, it's almost the only program that is living in the past, most other programs can handle these differences in line endings since about 20 years. And finally, since May 2018, Windows Notepad can do it, too. Still, your code should not generate these inconsistent line endings. Therefore: don't write n in your code, prefer println() over it. At least until you know what exactly you are doing.

The Main class

You saved the code in the file StudentDB.java, yet your class is called Main. Unless you have a very good reason, the class name should correspond to the file name. This makes for less surprises.

You wrote:

import java.util.Scanner;



class Main {



    public static void getStudents(Student ...students) {

        int i = 1;

        for (Student x : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> "+ x.name);

            System.out.println("Roll Number -> " + x.rollno + "n");

            i++;

        }

    }

This method should be called printStudents since the word get is practically reserved for methods that retrieve a little detail information (like Student.getName) and have no side-effects. Printing something is a side-effect.

The printStudents method prints the students details, each separated by an empty line. This empty line is not visible when looking only at the beginnings of the code lines.

Experienced programmers often only read the first word of each line to get a general idea of what the code does. In this example, it would be:

• for Student


• System.out.println


• System.out.println


• System.out.println

From that, readers of your code will conclude that for each student, there are 3 lines printed. But this is not what the program does. It prints 4 lines. Therefore there should be another line System.out.println(); after the other 3 lines.

There are subtle differences between println() and printing "n", but only on Windows systems. When you write n in your code, the Windows Notepad editor will not show the linebreak that should be there. Granted, it's almost the only program that is living in the past, most other programs can handle these differences in line endings since about 20 years. And finally, since May 2018, Windows Notepad can do it, too. Still, your code should not generate these inconsistent line endings. Therefore: don't write n in your code, prefer println() over it. At least until you know what exactly you are doing.

    public static void main(String args) {



        Scanner sc = new Scanner(System.in);



        System.out.print("Number of students? ");

        int num = sc.nextInt();

This pattern of asking for the number of items first and then reading the items one by one is very typical of textbooks and introductory programs. It is impractical for the user of your program, though. Does the user really know in advance how many students there will be? Probably not. Therefore the program should rather ask the user to "enter the student rollno, leave it empty to finish the input".

        String name = new String[num];

        int rollno = new int[num];

Having these two arrays is often the wrong approach. If you would explain the program (not the code) to a colleague, you would not say "the user enters a list of names and a list of roll numbers". You would say instead "the user enters a list of student details". The code should match this description as closely as possible.

Therefore your code should construct a proper Student object as soon as it has all the necessary details (in this case rollno and name).

Introductory programming courses typically use arrays since technically they are simpler to understand than Java classes and the whole collections framework. Arrays are harder to use though since you need to know the number of elements when you create the array.

By using a List<Student> (pronounced: list of student) instead of a Student, you can simply say students.add(student) and don't have to worry whether the list is large enough. This is much simpler.

        System.out.println("Enter those " + num + " students one by one");



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

            System.out.println("Enter details for student no. " + (i + 1));

Very good. This pattern of using the range [0,n) internally and [1,n] for communicating with humans is typical for all kinds of programs. It's important to get used to this pattern and apply it consistently.

There are other programming languages (Pascal, Lua, some versions of Basic) where arrays typically start at index 1. See Zero based numbering for a lengthy discussion about this topic.

            System.out.print("Enter Name: ");

            name[i] = sc.next();

            System.out.print("Enter Roll Number: ");

            rollno[i] = sc.nextInt();

In the Student class, you wrote the rollno first, for whatever reason. I thought it was because the rollno is the identifying field. But now it gets weird: you let the user enter the fields of the student in a different order than that in the Student class. This is a bad smell. It is not necessarily wrong, but it draws attention. Why do you do this. This should be explained in a comment for the human readers of your code.

To get the user's input, you use scanner.next(). This is almost always wrong. Currently, when the user is asked to enter a name and just presses Enter, the user expects that something has been entered. Yet, the program still waits for "something", since calling scanner.next() means "read from the input until the next word".

When the user enters a double name, only the first name will end up in student.name, the other name will still be in the input queue, waiting to be read in later. Therefore, instead of scanner.next(), use scanner.nextLine().

The same applies for the rollno. When the users just presses Enter here, the program continues to wait for a word to be entered, without giving any intermediate feedback. This makes a bad user experience since the user and the program have a different idea about what is happening right now. Therefore, instead of scanner.nextInt(), use Integer.parseInt(scanner.nextLine()).

Error handling will be handled later, that would be too large a topic for now.

For the benefit of readers of your code, you should add an empty line before the "Enter roll number" line. This will structure your code into paragraphs, just like in a good book. This video explains why this is a good thing.

            System.out.println("n");

        }



        Student students = new Student[num];



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

            students[i] = new Student(rollno[i], name[i]);

        }



        getStudents(students);

    }

}

My version of the Main class

class Main {



    public static void main(String args) {

        Scanner sc = new Scanner(System.in);

        Student students = readStudents(sc);

        printStudents(students);

    }

I have put the main method at the top of the file because it serves as a table of contents. It should be this short to let the reader quickly decide: Is this the code I'm looking for? What does it do when viewed from a high level perspective?

    private static Student readStudents(Scanner scanner) {

        System.out.print("Number of students? ");

        int num = Integer.parseInt(scanner.nextLine());



        System.out.println("Enter those " + num + " students one by one");



        Student students = new Student[num];

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

            System.out.println("Enter details for student no. " + (i + 1));

            students[i] = readStudent(scanner);

        }



        return students;

    }

As I said, scanner.nextLine is preferable to scanner.next.

There are no separate arrays of names and rollnos anymore since that was not the ideal design for this code. This code deals with students, not with names and rollnos. The code should always serve as an explanation of the core concepts to the human reader.

The main work is left to another method that reads a single student. Splitting up the work into several small parts makes each part understandable. For that, it's important to choose good names for each method. You should spend at least a third of the time thinking about how to name things.

    static Student readStudent(Scanner scanner) {

        System.out.print("Enter Roll Number: ");

        int rollno = Integer.parseInt(scanner.nextLine());



        System.out.print("Enter Name: ");

        String name = scanner.nextLine();



        System.out.println();



        return new Student(rollno, name);

    }

I placed rollno first because it is the identifying field. Like in the method above, I only use scanner.nextLine and then process this line further.

This method reads the individual fields and then constructs a proper Student object. From then on, the code doesn't deal with names or rollnos anymore, and that's exactly how it should be.

    public static void printStudents(Student... students) {

        int i = 1;

        for (Student x : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> " + x.name);

            System.out.println("Roll Number -> " + x.rollno);

            System.out.println();

            i++;

        }

    }

}

There are 4 lines of code corresponding to 4 lines of output. No surprise here, as it should.

Summary

Even though your program was quite short, there were a lot of topics to talk about, and many hidden assumptions and details. Knowing these from the beginning is impossible. But in the end, attention to all these small details is what differentiates between an ok program and a great program. It's worth taking time for that, the users of your program will (silently) thank you for it, or at least, they won't complain as much. :)

Main:

import java.util.Scanner;



class Main {



    public static void main(String args) {



        Scanner scanner = new Scanner(System.in);

        Student students = StudentRoll.gatherStudents(scanner);

        StudentRoll.printStudents(students);



    }

}

Student:

class Student {



    private int rollNumber;

    private String name;



    Student(int rollNumber, String name) {

        this.rollNumber = rollNumber;

        this.name = name;

    }



    int getRollNumber() {

        return rollNumber;

    }

    void setRollNumber(int rollNumber) {

        this.rollNumber = rollNumber;

    }



    String getName() {

        return name;

    }

    void setName(String name) {

        this.name = name;

    }

}

StudentRoll:

class StudentRoll {



    static Student gatherStudents(Scanner scanner){



        System.out.print("Number of students? ");

        int numberOfStudents = scanner.nextInt();



        System.out.println("Enter those " + numberOfStudents + " students one by one");

        Student students = new Student[numberOfStudents];



        for (int index = 0; index < numberOfStudents; index++) {

            System.out.println("Enter details for the next student no. ");

            System.out.print("Enter Name: ");

            String studentName = scanner.next();

            System.out.print("Enter Roll Number: ");

            int rollNumber = scanner.nextInt();

            students[index] = new Student(rollNumber, studentName);

            System.out.println("n");

        }

        return students;

    }



    static void printStudents(Student students){

        int i = 1;

        for (Student x : students) {

            System.out.println("Student Number : " + i);

            System.out.println("Name -> " + x.getName());

            System.out.println("Roll Number -> " + x.getRollNumber() + "n");

            i++;

        }

    }

}

I can't post many notes right now, but will as soon as I can. Think the main thing here is that your code isn't very object-oriented, but Java is an object-oriented language. It's also generally a good idea to name variables something other than num, so that when you're working on a method that has 5 different int variables, you aren't tempted to call them num1 , num2, etc., but what they actually represent. I'm also not a big fan of using a for-in when you reference an index, but it didn't seem like the worst thing in the world so I left it in there.