This program reads in integers until a sentinel number is read. And prints the largest integer read

I’m just looking for feedback on how I could improve this simple program. Whether it be comments for better understanding or better variable names. I try not to focus on memory consumption because I’m honestly not that knowledgeable as of yet. My main focus is getting a written solution on paper and then implement in syntax.

I was given the question to write a program that reads in integers one per line, once a sentinel integer is read end program, and display the largest integer. The way I solved it was with pen and paper first because I’m working on my pseudocode and algorithm design. If there are any books on the subject for recommendation. I would greatly appreciate.

/**
 * This program reads in integers until a sentinel number is read, 
 * and prints the largest. 
 *
 * @author shawn
 *
 */

//Import needed for Console Programs.
import acm.program.*;

public class LargestListValue extends ConsoleProgram {
    public void run() {

        /*Integer variables to track the current largest input 
         * and current input.
         */
        int x, y;

        println("This program reads in integers until a sentinel number is read, ");
        println("and prints the largest.");


        while (true) { //Initial while to read in an initial integer.
            x = readInt(" ? "); //Initial read integer. Also current largest.
            if (x == SENTINEL_NUMBER) { 
                println("No numbers?");
                break;
            }
            else {
                y = readInt(" ? "); //Next read integer.
                if (y == SENTINEL_NUMBER) {
                    println("Largest number is " + x + ".");
                    break;
                } else {
                    while (y <= x) { //While second read integer
                        y = readInt(" ? ");
                        if (y == SENTINEL_NUMBER) {
                            println("Largest number is " + x + ".");
                            break;
                        } else if (y >= x) {
                            while (y >= x) {
                                x = readInt(" ? ");
                                if (x == SENTINEL_NUMBER) {
                                    println("Largest number is " + y + ".");
                                    break;
                                } else if (y <= x) break;
                            }
                        }
                    }
                }
            }
            //Extra break added to the
            break;
        }
    }
    //Sentinel number to end reading integers and print largest.
    private static final int SENTINEL_NUMBER = 0;
}

                break;
            }
            else {

You don’t need an else with a break. The break ends execution inside the loop. You can never reach the code after it in the loop unless it doesn’t trigger. Without all the else clauses, you can significantly reduce the amount of indentation.

Your code could also return instead of break. That would be clearer in my opinion.

What’s x? What’s y? Why loop forever if you’re just going to break out of it?

In my opinion, your comments would be better replaced with more readable, self-commenting code.

Consider the following

    public static int readLargest(final int SENTINEL) {
        int largest = readInt(" ? ");
        for (int current = largest; current != SENTINEL; current = readInt(" ? ") ) {
            if (current > largest) {
                largest = current;
            }
        }

        return largest;
    }

Which you’d use as

    public void run() {
        int largest = readLargest(SENTINEL_NUMBER);

        if (largest == SENTINEL_NUMBER) {
            println("No numbers?");
        } else {
            println("Largest number is " + largest + ".");
        }
    }

By using a separate method, we consolidate the possibilities. We only have two println statements. In the original code, you had the latter option three times. Here we only need one.

This also makes the readLargest more reusable. Even better might be to read all the values first, then call a findLargest method on that. Then your readNumbers and findLargest methods would both be reusable separately. Of course, you may not have gotten to Collection types yet.

In your original code, sometimes y was the largest and sometimes x. In this code, largest is always the largest unless the most recent read returns a larger value.

I prefer the names current and largest as being more descriptive than single letter names. I find that more important with largest. I could live with x instead of current.

I’m not sure that it’s necessary to say SENTINEL_NUMBER. The current variable is an integer. What would SENTINEL be? A cabbage? Obviously not. You’d only compare an integer to some kind of number. Others may disagree. In particular, you may want to look up Hungarian Notation.

I prefer to pass the sentinel value as a parameter for flexibility.

Since we don’t use current outside of the loop, I think that the for loop has better scope than a while loop.

The only comment that seems necessary is that readLargest will return the sentinel value if and only if there are no other numbers in the input (prior to the sentinel value). Other than that, I feel that the code is pretty clear about what it is doing. Perhaps some explanation of the initialization of largest.