3
\$\begingroup\$

Please help to improve my Java 7 code. I would like a best code logic to achieve this.

package main.java;

import java.util.ArrayList;
import java.util.List;

public class Student {
    private int id;
    private String grade;
    private int age;

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

    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }
    public String getGrade() {
        return grade;
    }
    public void setGrade(String grade) {
        this.grade = grade;
    }
    public int getAge() {
        return age;
    }
    public void setAge(int age) {
        this.age = age;
    }

    // group student by grade and having max of age
    public static List<Student> getStudentsWithMaxAgeOfEachGrade(List<Student> students){
        ArrayList<Student> result = new ArrayList<>();

        for(Student s : students){
            boolean found = false;
            for(Student rs : result){
                if(rs.getGrade().equals(s.getGrade()) && rs.getAge() < s.getAge()){
                    found = true;
                    result.remove(rs);
                    result.add(s);
                    break;
                }
            }
            if(found == false){
                result.add(s);
            }
        }

        return result;

    }

    public static void main(String[] args) {
        List<Student> students = new ArrayList<>();
        Student s1 = new Student(1, "a", 32);
        Student s2 = new Student(2, "a", 34);
        Student s3 = new Student(3, "b", 23);

        students.add(s1);
        students.add(s2);
        students.add(s3);

        List<Student> result = getStudentsWithMaxAgeOfEachGrade(students);

        for(Student s : result){
            System.out.println(s.getGrade() + ":" + s.getAge());
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What was the aim of your assignment? Was it implementing the sort algorithm? \$\endgroup\$ – Timothy Truckle Mar 26 '17 at 8:47
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Martin R Mar 26 '17 at 10:53
1
\$\begingroup\$

in your code, you have getter and setter for id. But those methods are not used. I have posted a similar question before. And multiple people told me "we shouldn't have dead code."

Since you want to return List of Student, I would construct the HashMap to map from string(grade) to Student instead of string to integer (where you will lose the student id).

\$\endgroup\$
0
\$\begingroup\$
public static List<Student> getStudentsWithMaxAgeOfEachGrade(final List<Student> students) {
    if (students.isEmpty()) {
        return Collections.emptyList();
    }
    // Make a copy of input, because we don't want to modify it
    List<Student> copy = new ArrayList<>(students);

    // Sort by age (desc)
    Collections.sort(copy, new Comparator<Student>() {
        @Override
        public int compare(Student o1, Student o2) {
            return Integer.compare(o2.getAge(), o1.getAge());
        }
    });

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

    // First element is always included in the result
    result.add(copy.get(0));
    // Start with the second element (index 1), we can find a maximum of 6 different grades (a-f)
    for (int i = 1; i < copy.size() && result.size() <= 6; i++) {
        Student s = copy.get(i);
        boolean hasGrade = false;
        for (Student rs : result) {
            // Do our result already have this grade?
            if (rs.getGrade().equals(s.getGrade())) {
                hasGrade = true;
                break;
            }
        }
        if (!hasGrade) {
            result.add(s);
        }
    }

    return result;
}

Test data:

id, grade, age
1, "a", 32
2, "a", 34
3, "b", 23
4, "d", 22
5, "c", 26
6, "f", 20
7, "f", 19
8, "e", 27
9, "b", 26

Result:

yours:
a:34
d:22
c:26
f:20
f:19 <-- You have a bug?! (I have not resolved this)
e:27
b:26

mine:
a:34
e:27
c:26
b:26
d:22
f:20

Edit: If you want JDK 1.8 :)

public static List<Student> getStudentsWithMaxAgeOfEachGrade(final List<Student> students) {
    if (students.isEmpty()) {
        return Collections.emptyList();
    }
    // Make a copy of input, because we don't want to modify it
    List<Student> copy = new ArrayList<>(students);

    // Sort by age (desc)
    copy.sort(Comparator.comparingInt(Student::getAge).reversed());

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

    // First element is always included in the result
    result.add(copy.get(0));
    // Start with the second element (index 1), we can find a maximum of 6 different grades (a-f)
    for (int i = 1; i < copy.size() && result.size() <= 6; i++) {
        Student s = copy.get(i);
        if (result.stream().map(Student::getGrade).noneMatch(s.getGrade()::equals)) {
            result.add(s);
        }
    }

    return result;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You have presented an alternative implementation without reviewing the code provided by the author. On Code Review, all answers should contain a review. \$\endgroup\$ – Mast Mar 26 '17 at 12:18
  • \$\begingroup\$ "You have a bug" is the only part of this answer that is considered a code review. Please explain that part more. \$\endgroup\$ – 200_success Mar 26 '17 at 13:50
  • \$\begingroup\$ I think his bug is that he goes through each element in the result and then if the student's age is less than rs->age, then found will be stay at false and then a duplicate grade will be added to result \$\endgroup\$ – Shinigami Mar 26 '17 at 15:01
0
\$\begingroup\$

A completely different solution in comparison to my first solution
Use a Map as a return type:
(But keep in mind, that you will lose information about student in this version. So no information about id anymore)

public static Map<String, Integer> getStudentsWithMaxAgeOfEachGrade(final List<Student> students) {
    Map<String, Integer> result = new HashMap<>();
    for (Student s : students) {
        Integer value = result.get(s.getGrade());
        if (value == null || value < s.getAge()) {
            result.put(s.getGrade(), s.getAge());
        }
    }
    return result;
}

Edit: If you want JDK 1.8 :)

public static Map<String, Integer> getStudentsWithMaxAgeOfEachGrade(final List<Student> students) {
    Map<String, Integer> result = new HashMap<>();
    students.forEach((s) -> {
        result.merge(s.getGrade(), s.getAge(), (oldValue, newValue) -> {
            return oldValue == null || oldValue < newValue ? newValue : oldValue;
        });
    });
    return result;
}
\$\endgroup\$
  • \$\begingroup\$ You could return new ArrayList(result.valueSet()) to comply with the original Method signature... \$\endgroup\$ – Timothy Truckle Mar 26 '17 at 10:47
  • \$\begingroup\$ Thanks, but we need a return type as list not map. \$\endgroup\$ – Rady Archuleta Mar 26 '17 at 10:48
  • \$\begingroup\$ @TimothyTruckle valueSet() is undefined of object result, do you have other way? \$\endgroup\$ – Rady Archuleta Mar 26 '17 at 10:51
  • \$\begingroup\$ @RadyArchuleta this is the kind of inconsistency in the Java API that always confuses me: the Map interface has the keySet() method to get the keys but the values() method (without set) to get the values... \$\endgroup\$ – Timothy Truckle Mar 26 '17 at 11:10
  • \$\begingroup\$ @TimothyTruckle that's because the keys can only be there once, but values can exist multiple times. So I can have a,b,c,d,e,f as keys and 34,27,26,26,22,20 as values \$\endgroup\$ – Shinigami Mar 26 '17 at 11:14

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.