1
\$\begingroup\$
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Random;

class Student {
  // Student class properties
  int number;
  Double grade;

  // Student constructor
  public Student(int index) {
    Random generator = new Random();
    number = index+1;
    grade = generator.nextDouble()*20;
  }
}

class Playground {

    public static void main(String[] args) {
    //declaring variables
    int numOfApplicants = 100;
    ArrayList<Student> applicants = new ArrayList<Student>();
    ArrayList<Student> classA = new ArrayList<Student>();
    ArrayList<Student> classB = new ArrayList<Student>();
    ArrayList<Student> rejected = new ArrayList<Student>();



    //create x students
    for(int i = 0; i < numOfApplicants; i++) {
      applicants.add(new Student(i));
    }

    //assign to classes
    for(Student applicant:applicants) {
      if (applicant.grade >= 19) {
        classA.add(applicant);
        System.out.println("Student number "+applicant.number+" has been accepted in class A with a score of "+applicant.grade+" !");
        }

      else if (applicant.grade >= 16) {
      classB.add(applicant);
        System.out.println("Student number "+applicant.number+" has been accepted in class B with a score of "+applicant.grade+" !");
      }
     else {
       rejected.add(applicant);
      System.out.println("Student number "+applicant.number+" were rejected because of their "+applicant.grade+" score!");
      }
      }

    
    //final totals
    System.out.println("\nThis year, class A has " + classA.size() + " students while class B has " + classB.size() + ".\nWhile " + rejected.size() + " were rejected");
    }
  } 

So yeah here's my code, I am just starting out with Java and I am very much lost, I feel like my code could be improved in many ways so I would really appreciate any input, I am especially not proud of the ugly Double grades that I couldn't figure out how to round down. There is also a weird pattern I am seeing with the grades generated, you can kinda see that there are <10 within class A, <20 within class B and around 80 in the rest, I guess it has to do with the Random Seed? I'll read about that later I guess...

Either way, thanks!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ You said that you don't like the ugly double grades. What are the possible grades that you are wanting? Are they supposed to be whole numbers? \$\endgroup\$
    – Xtros
    Jul 20, 2021 at 18:54

3 Answers 3

3
\$\begingroup\$

This is not a full code review, but some things to start:

  • Double check your indentation
  • The properties in Student should be marked as private. It's customary in Java to always have these fields marked as private, then provide getters/setters to them. This could look like:
class Student {
    private int number;
    private double grade;

    public int getNumber() {
        return number;
    }

    public double getGrade() {
        return grade;
    }
}
  • This may feel like it's unnecessary, but it allows you to control who has access to the fields of a class.

  • Notice that I did not create setter methods, and that's because so far, it doesn't appear that they are needed. Once a student gets a number, there's not really a reason for them to get a new number. And so far, with the requirements, there's no reason to change the grade either. Ultimately, this results in read only fields, which can be made explicit by marking those fields as final.

    private final int number;
    private final double grade;
  • Next, you need to consider when creating data for an object, should that happen in the constructor or in the code that calls the constructor. Because you have this line Random generator = new Random(); that happens for each student, I lean towards the code that calls the constructor.
class Student {
    public Student(int number, double grade) {
        this.number = number;
        this.grade = grade;
    }
}

That changes the calling code to:

//create x students
Random generator = new Random();
for(int i = 0; i < numOfApplicants; i++) {
    int number = i + 1;
    double grade = generator.nextDouble() * 20;
    Student student = new Student(number, grade);
    applicants.add(student );
}
  • I also split up that line that previously handled adding the student to the applicants and creating the Student into two lines to aid with readability and to reduce the reasons that that line of code would need to change. Generally, a single line of code should do one thing. It can do multiple things provided that it is still easy to read. In a work environment, it is estimated that code is read ten times more than it is written.
  • You can also remove the comment that label the Student properties and constructor. The readers of the code will know that's what they are and the comments don't add anything. What might be a valuable comment is something briefly explaining why a grade is a random number between 0-20 (including 0, but not 20).
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your help, I really appreciate it! I'll try to avoid these mistakes and implement your advice on my next exercise :D \$\endgroup\$
    – Epsilon
    Jul 22, 2021 at 10:39
  • \$\begingroup\$ By the way, @Xtros what is exactly wrong with my indentation? \$\endgroup\$
    – Epsilon
    Jul 24, 2021 at 13:54
  • \$\begingroup\$ The Student class looks excellent, but everything after "//assign to classes" needs a closer look. You have the body of a block aligned with the curly braces, rather than indented in. \$\endgroup\$
    – Xtros
    Jul 30, 2021 at 20:05
0
\$\begingroup\$

I won't repeat what XTros and Martin Frank have said...

This is code written in Java, but it's not very Object Oriented as Martin pointed out.

To reduce the confusion here, I'm going to use the term StudyGroup for your Classes, so that when I talk of classes in the rest of this post, it's clear I'll mean Java classes.

If I were implementing this, for a start I'd read this discussion to get the Random number handling right.

Then StudyGroups would be Java class instances, each holding a list of Students. The class would decide whether to accept a student. The main method would simply generate students, offer each student to each StudyGroup in turn until one accepted the student.

Something like this is a simplistic implementation - it uses Java 8, but nothing particularly clever. A more advanced approach would probably use lambdas for the acceptance criteria, but for now a simple test for a minimum grade will do:

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.concurrent.ThreadLocalRandom;

public class StudyGroup implements Iterable<Student> {

  List<Student> roll = new ArrayList<>();
  String name;
  int minGrade;

  StudyGroup(String name, int minGrade) {
    this.name = name;
    this.minGrade = minGrade;
  }

  boolean accepted(Student candidate) {
    if (candidate.getGrade() >= minGrade) {
      roll.add(candidate);
      return true;
    }
    return false;
  }

  @Override
  public Iterator<Student> iterator() {
    return roll.iterator();
  }

  @Override
  public String toString() {
    return String.format("Study Group %s has %d members", name, roll.size());
  }

  public static void main(String[] args) {

    StudyGroup studyGroupA = new StudyGroup("SuperStars", 19);
    StudyGroup studyGroupB = new StudyGroup("HighAchievers", 16);
    StudyGroup studyGroupC = new StudyGroup("TheRest", 0);

    StudyGroup[] allStudyGroups = new StudyGroup[]{studyGroupA, studyGroupB, studyGroupC};

    for (int studentNumber = 0; studentNumber < 100; studentNumber++) {
      // Generate a student
      Student candidateStudent = Student.generate();
      // Find a study group that will accept them
      for (StudyGroup candidateStudyGroup : allStudyGroups) {
        if (candidateStudyGroup.accepted(candidateStudent)) {
          break; // found a suitable group
        }
      }
    }

    // Report on the distribution
    for (StudyGroup studyGroup : allStudyGroups) {
      System.out.println(studyGroup.toString());
      for (Student student : studyGroup) {
        System.out.format("    %s%n", student);
      }
      System.out.println();
    }
  }
}

class Student {

  private int id;
  private int grade;

  Student(int id, int grade) {
    this.id = id;
    this.grade = grade;
  }

  static int currentId = 0;

  static Student generate() {
    return new Student(currentId++, ThreadLocalRandom.current().nextInt(1, 21));
  }

  public int getGrade() {
    return grade;
  }

  @Override
  public String toString() {
    return String.format("Student %d - grade %d", id, grade);
  }
}
\$\endgroup\$
0
\$\begingroup\$

Major issue: Java Basics OOP

where are your objects, where are your methods? yes, we do have a class Students but what else? All of your logic happens within Playground, which can be a suitable class for an excersize, but right now it's totally bloated up!

here are some ideas for objects

  • StudentGenerator - has a method generate(int amount)...
  • Grade - yes, it's more than a mere collection it's a first class collection
  • a Filter, thats an obvious one
  • a class for the purpose: Enrollment who is responsible for that whole applying process

Minor Issue: duplicate code

i hopefully that will disappear, when you apply OOP but your code is duplicated:

if (applicant.grade >= 19) {
    classA.add(applicant);
    System.out.println("Student number "+applicant.number+" has been accepted in class A with a score of "+applicant.grade+" !");
}

this appears three times in a row, you should have noticed that - even tough the text is slightly different and the object (classA) do vary a bit.

Minor Issue: redudant typification, Interface over class

ArrayList<Student> applicants = new ArrayList<Student>();

well, thats just peanuts, but should be:

List<Student> applicants = new ArrayList<>();

Using Interface over Class and removing redundant type of implementing class

Already said

most issues concerning your Student clss have been properly addresses by the code Review from XTros

digging deeper: first class Collection

instead of simply adding a Student to a List<Student> named gradeA you create a Class around the Collection (here: around the List).

class Grade {
    private final Collection<Student> students = new ArrayList<>();
    void add(Student student){...}
    void remove(Student student){...}
}

now you can provide all relevant Code to that Collection! example

class Grade {
    final String name;
    private final Collection<Student> students = new ArrayList<>();
   
    Grade (String name){this.name = name;}

    void add(Student student){
        students.add(Student);
        System.out.println("Student number "+applicant.number+" has been accepted in class "+name+" with a score of "+applicant.grade+" !"); 
    }

    //first class collection can do this as well
    String getClassDescription(){
        return "grade "+name+" has "+students.size()+" students";
    }

    //first class collection can do that as well
    List<Students> orderedByName(){
        ...
    }
}

of course the first class collection should provide basic methods add remove and get and so on (you best implement the whole interface for that) but now this Collection has all methods that are Grade specific in its belly. on the place where it should belong.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I tried my hand at some C# with Unity so I should really be ashamed of myself for dumping everything in one script... I guess I haven't coded in a while and just forgot you're not supposed to do that... As for the duplicate code, I noticed it but I thought that the hassle of creating a custom method just for 3 cases was not really worth it, but if I did do things properly and applied OOP it would've been part of the enrolment class However for the rest, I have no idea what a collection is, I am going to have to read about that thanks! (I just started learning sorry >.<) \$\endgroup\$
    – Epsilon
    Jul 22, 2021 at 10:57
  • \$\begingroup\$ hello @Epsilon thank you for taking my review serious ;-) following the provided Link for the collections, the sub-tile reads Creating first class collections - which is one Point in the review. I will add an Example for this in my Answer \$\endgroup\$ Jul 22, 2021 at 11:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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