6
\$\begingroup\$

I've written an answer to HackerRank's Angry Professor problem. Please let me know if there's a quicker/better way of solving this as I think I wrote quite a bit of code for this problem (answer has to be in 1 class btw). Also, any improvements on my code in Java 8 are welcome too.

Problem statement (as written on HackerRank):

The professor is conducting a course on Discrete Mathematics to a class of N students. He is angry at the lack of their discipline, and he decides to cancel the class if there are less than K students present after the class starts.

Given the arrival time of each student, your task is to find out if the class gets cancelled or not.

Answer:

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

public class AngryProfessor {
    public static void main(String[] args) {
        List<List<List<Integer>>> testCases = readInput();

        for(List<List<Integer>> testCase : testCases){
            int numStudents = testCase.get(0).get(0);
            int minStudents = testCase.get(0).get(1);
            // Gotta STDOUT in hacker rank for it to be graded.
            System.out.println(isLectureCancelled(calcNumAttending(testCase.get(1), numStudents), minStudents));
        }
    }

    private static void addData(List<Integer> arrayList, String[] numbersToAdd){
        for(String num : numbersToAdd){
            arrayList.add(Integer.valueOf(num));
        }
    }

    /**
     * Calculates the number of students that make the lecture on time.
     *
     * @param studentTimes    the relative time at which the student arrives (student is on time if time is <= 0)
     * @param numStudents     number of students that make the lecture on time
     * @return numStudents
     */
    private static int calcNumAttending(List<Integer> studentTimes, int numStudents){
        int numAttending = 0;
        for(int i = 0; i < numStudents; i++){
            if(studentTimes.get(i) <= 0){
                numAttending++;
            }
        }
        return numAttending;
    }

    /**
     * Determines whether the lecture is cancelled or not.
     *
     * @param numAttending    Number of students who attend the lecture
     * @param minStudents     Minimum number of students for the lecture to go ahead
     * @return whether the lecture is cancelled or not
     */
    private static String isLectureCancelled(int numAttending, int minStudents){
        return numAttending < minStudents ? "YES" : "NO";
    }

    /**
     * Reads and stores all the data given. Each index is a test case. At that index is a list containing two lists. One
     * for each line of input that's read. The first list contains numStudents and minStudents, and the second list
     * contains the student times.
     *
     * @return a List containing all the data.
     */
    private static List<List<List<Integer>>> readInput(){
        Scanner k = new Scanner(System.in);
        int numTestCases = Integer.valueOf(k.nextLine());
        List<List<List<Integer>>> testCases = new ArrayList<>(numTestCases);

        // fills up testCases with ArrayLists before filling up with numbers
        for(int i = 0; i < numTestCases; i++){
            List<List<Integer>> list = new ArrayList<>(2);
            for(int j = 0; j < 2; j++){
                list.add(new ArrayList<>());
            }
            testCases.add(list);
        }

        // Inputs data into the ArrayLists
        for(int i = 0; i < numTestCases; i++) {
            String line1 = k.nextLine();
            String line2 = k.nextLine();
            addData(testCases.get(i).get(0), line1.split(" "));
            addData(testCases.get(i).get(1), line2.split(" "));
        }
        k.close();
        return testCases;
    }
}
\$\endgroup\$
3
\$\begingroup\$

you have a nice, correct program here! ...please see what I made of it with little scope on "fewer lines" and little "performance" and of course some Java8 tweaks:

import java.util.LinkedList;//why always Array-Array?(especially for 2 - 20 entries)
import java.util.List;
import java.util.Scanner;

public class Solution {//adjusted class name for code upload

  public static void main(String[] args) {
    readInput()// don't need the variable, but same call as in original program
    .stream().forEach(//just as a showcase for streams and lambda but no real improvement
      /*TestCase*/ t -> {System.out.println(t.outcome);}
    );
  }

  private static List<TestCase> readInput() {//no changes, except the return (list) type
    final List<TestCase> testCases = new LinkedList<>();//"final" becuase it is..., LinkedList: no(0) initial capacity
    try (Scanner k = new Scanner(System.in)) {//try with ressources {@link: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html}
        int numTestCases = Integer.valueOf(k.nextLine());//original code
        for (int i = 0; i < numTestCases; i++) {//...
            testCases.add(//...
               new TestCase(//new "TestCase" with ...
                   k.nextLine(),//the next line <- blocks
                   k.nextLine() //..and the next line <-blocks
               )
            );
        }
    }
    return testCases;
  }

  private static class TestCase {//This is "nicer" to read/understand than nested lists
    //we don't really need these (they could also be local constructor variables):
    private final int students;
    private final int minAttendees;
    private final int[] arrivals;//(I) prefer arrays>list, when knowing the size in advance and size < 65536 (2^16)
    //we only need this:
    final String outcome;

    private TestCase(String courseData, String studsData) {
         /*This is the "power" but also "the weakness" of this approach:
         It does everything (possible) in one loop/line/routine - 
         it saves lines of code (and maybe little exec. time)!
         But it is less flexible, and for big routines
          (many students, and maybe not "keyboard scanner")
         this would definitely be a bottleneck.*/
        String[] tmp = courseData.split(" ");//according to original code, reuse of String[] variable
        students = Integer.valueOf(tmp[0]);//according to original code/input format 
        minAttendees = Integer.valueOf(tmp[1]);//...
        tmp = studsData.split(" ");//...
        arrivals = new int[students];//..initialize...
        int attendees = 0;//...
        for (int i = 0; i < students; i++) {//only one loop...
            arrivals[i] = Integer.valueOf(tmp[i]);//...copying...
            if (arrivals[i] <= 0) {//...evaluating...
                attendees++;
            }
        }
        outcome = attendees < minAttendees ? "YES" : "NO";//..and finally aggregating
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – Simon Forsberg Apr 6 '15 at 22:51
  • \$\begingroup\$ Thanks for the comment, but I also insist in that I reviewed! ...and I finalized my review by the decision "the program is correct." ..and even "nice" I followed the author's invitation to provide a quicker/better/java8er implementation. But you are right: I could comment it better! \$\endgroup\$ – xerx593 Apr 6 '15 at 23:07
  • \$\begingroup\$ I'd recommend adding more comments about the code and the changes you did outside the code itself. At the current state, this is essentially a "code-only" answer, which is discouraged on Code Review. \$\endgroup\$ – Simon Forsberg Apr 6 '15 at 23:18
  • \$\begingroup\$ @xerx593 You reviewed, but didn't post the review. ;) Sometimes, the original solution is too bad to be taken seriously and sometimes there's a much better solution worth posting, but you should always add a few improvement hints. \$\endgroup\$ – maaartinus Apr 6 '15 at 23:26
  • \$\begingroup\$ Welcome to CodeReview, xerx593. While I'm certain your intentions are good and commend you for having your first post be an answer, here on CR it's encouraged and even conventional to provide the context to why your answer is an improvement. Teaching a man to fish rather than simply handing a fish, as it were. \$\endgroup\$ – Legato Apr 6 '15 at 23:40
3
\$\begingroup\$

if there's a quicker/better way of solving this

What you did is to accumulate all input and then process it. It has advantages, when you plan to do more with it, but in your case, it just complicates things.

private static List<List<List<Integer>>> readInput(){

Three levels of List nesting is a code smell. Indeed, one of the lists has always two members and should be a class instead.

answer has to be in 1 class btw

I'd bet, you can use a nested class.

If you don't accumulate the input, you can get a much simpler solution. You don't have to store all the arrival times, all you need is to count the non-positives.


All your methods are static, this is fine in this case, but in general forces you to pass everything around. It'd get messy for a more complicated problem. That's what OOP is for.


private static void addData(List<Integer> arrayList, String[] numbersToAdd){

I'd call it "addDataTo".

There should be a blank between ){.

\$\endgroup\$

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.