2
\$\begingroup\$

I am learning java and doing some practice problems. My code works, but I feel like I used a workaround sort of way to accomplish it. Is there another way to express what I want the program to do?

Question: Problem J4: Time on task

Input specification:

First line is the total number of minutes to accomplish the chores.

Second line is number of chores to choose from

The next 'n' lines are how long each chore takes to complete.

Output specification:

The total amount of chores one can do given the amount of time

My code:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Arrays;

public class Time {

public static void main(String args[]) {
    try {
        BufferedReader input = new BufferedReader(new InputStreamReader(
                System.in));

        //get the amount of time
        int maxMins = Integer.parseInt(input.readLine());
        //get the total amount of chores
        int totalChores = Integer.parseInt(input.readLine());
        //create an array to store each chore and how long it takes to complete it
        int[] list = new int[totalChores];

        for (int i = 0; i < totalChores; i++) {
            list[i] = Integer.parseInt(input.readLine());
        }
        Arrays.sort(list);

        //initialize counters, one for minutes and the other for chores done
        int totalMins = 0;
        int chores = 0;

        //I don't like the below part, not quite sure how to change it
        for (int j = 0; totalMins < maxMins; j++) {
            totalMins = totalMins + list[j];
            if (totalMins <= maxMins) {
                chores++;
            }
        }

        System.out.println(chores);

    } catch (IOException e) {
        e.printStackTrace();
    }
}

}
\$\endgroup\$
2
\$\begingroup\$

I don't think it is bad. Just a couple of items.

  1. Naming of variables. maxMins and totalChores are fine. list not so much. Name items according to what they are and put some thought into the names. So later on in the code you are not saying to yourself "What was that variable again?".

  2. Your for loop termination is faulty. If all the chores can be done within the time frame (in other words the sum of minutes required to complete all chores is less than the maximum time) you have created a endless loop.
    You are probably best off just using the length of the list as the termination criteria. j < list.length.

\$\endgroup\$
2
\$\begingroup\$

Formatting: Indent after opening { and unindent after closing }.

Variable naming: A better name for list would be choreList.

When iterating through an array use an enhanced-for loop:

for (int chore:list) {
        chore = Integer.parseInt(input.readLine());
}

Your algorithm for computing the maximum amount of chores that can be accomplished could be simplified as follows:

for (int chore:list){
    maxMins -= chore;
    if (maxMins > 0){
        chores++;
    }
    else {
        break;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I don't think I like the way that on your final chore you decrement maxMins (I'd probably call it minutesRemaining if it's non-final) before deciding whether you can fit the chore. It may have one more step, but I'd write it as if (minutesRemaining - chore > 0) { minutesRemaining -= chore; chores++; }, so minutesRemaining and chores always remain in-sync. \$\endgroup\$ – Edd Feb 6 '15 at 12:10

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.