4
\$\begingroup\$

I am learning Java through Stanford CS106A and I'm wondering how the code for this early assignment looks. I've worked in a proprietary scripting language before and so I'm trying to get the right programming form down for Java. It works but am I doing anything in the wrong way? Am I declaring and initializing my variables properly? Is using a counter and conditional for the first instance like this good form? Am I missing a shorter, more efficient way?

/*
 * File: FindRange.java
 */

import acm.program.*;

public class FindRange extends ConsoleProgram {

    /* Specifies the value of the sentinel */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the largest and smallest numbers entered.");
        println("Enter values, one per line, using " + SENTINEL + " to signal the end of the list.");
        int min = 0;
        int max = 0;
        int count = 0;
        while (true) {
            int value = readInt(" ? ");
            if (value == SENTINEL) break;
            count++;
            if (count == 1){
                min = value;
                max = value;
            }
            min = Math.min(min, value);
            max = Math.max(max, value);
        }
        if (count>0){
            println("Smallest: " + min);
            println("Largest: " + max);
        } else {
            println("No values were entered.");
        }
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ For a proper review I think you'd need to at least reference the code from Console Program. Seems like something that was either included with the course or part of an earlier lesson. \$\endgroup\$
    – Legato
    Commented Apr 18, 2015 at 20:28
  • \$\begingroup\$ Console Program is a part of the acm.program package that Stanford uses. I believe it just handles the readInt and println to accept values and output the response. \$\endgroup\$
    – Mark Ross
    Commented Apr 18, 2015 at 20:37
  • \$\begingroup\$ I understood that much. But could you include a reference to the package download or its docs? You wouldn't want a review just taking your guess for non-runnable aspects of the code. Especially if it includes more complex implementations you'll be using later on. As it is I'm wondering why this isn't just using built in methods. \$\endgroup\$
    – Legato
    Commented Apr 18, 2015 at 21:00
  • \$\begingroup\$ Just a minor nitpick, you might or might not want to take it into account: put that break on its own line. Single-statement ifs written on one line are confusing. Or at least separate the if from the rest with blank lines: breaking out of the while is important enough to warrant an extra couple of blank lines. \$\endgroup\$
    – s.m.
    Commented Apr 18, 2015 at 21:12
  • \$\begingroup\$ @s.m. Please write all suggestions for improvements in answers, not comments. \$\endgroup\$ Commented Apr 19, 2015 at 4:21

3 Answers 3

5
\$\begingroup\$

You could initialize min with Integer.MAX_VALUE and max with Integer.MIN_VALUE, this would make the inner if obsolete. You could also use this for your output condition and get rid of the count completely.

Without knowing the complete assignment, it's hard to optimize your code in any direction.

If you don't want to use your library and do it in 'plain' Java have a look at Scanner.

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

Just a minor nitpick - you are using integer as a stopping condition, which means you are excluding a one perfectly acceptable input. For the sake of keeping solution simple, I would suggest to simply ask user to input number of values to compare, before reading values themselves.

\$\endgroup\$
0
\$\begingroup\$

Looping with for

An alternate implementation you can consider is to use a for-loop:

for (int value = readInt(" ? "); value != SENTINEL; count++, value = readInt(" ? ")) {
    min = Math.min(min, value);
    max = Math.max(max, value);
}

I find that it communicates the steps slightly clearer:

  1. initialize value
  2. Loop if value != SENTINEL
  3. increment count and read the next input for value

The other minor point I can suggest is mentioned by @mnhg too, which is to initialize min and max to the appropriate default values so that you can make the comparisons immediately.

\$\endgroup\$
2
  • \$\begingroup\$ I think a while loop would be more appropriate. \$\endgroup\$ Commented Jan 23, 2017 at 5:46
  • \$\begingroup\$ @UsagiMiyamoto feel free to contribute a new answer. :) \$\endgroup\$
    – h.j.k.
    Commented Jan 23, 2017 at 15:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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