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

Posted on

Problem

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.
if (x == SENTINEL_NUMBER) {
println("No numbers?");
break;
}
else {
if (y == SENTINEL_NUMBER) {
println("Largest number is " + x + ".");
break;
} else {
while (y <= x) { //While second read integer
if (y == SENTINEL_NUMBER) {
println("Largest number is " + x + ".");
break;
} else if (y >= x) {
while (y >= x) {
if (x == SENTINEL_NUMBER) {
println("Largest number is " + y + ".");
break;
} else if (y <= x) break;
}
}
}
}
}
break;
}
}
//Sentinel number to end reading integers and print largest.
private static final int SENTINEL_NUMBER = 0;
}
``````

Solution

``````                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?

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() {

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`.