2
\$\begingroup\$

I wanted to know if there was any way to improve on this solution. I believe it uses the least amount of variables. Also wanted to know how are my variable names? Are they sufficient? Do I need anymore indenting? I'm looking for perfection in a way. At least a clear, correct and readable program.

/*
 * File: FindTwoLargest.java
 * ----------------------
 * This program finds the two largest integers in a list.
 * 
 */

/* Library packages */
import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {
    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        int n = readInt("? ");
        int largest = n;
        int secLargest = n;
        while (true) {
            n = readInt("? ");
            if (n == SENTINEL) break;
            secLargest = n;
            while (true) {
                n = readInt("? ");
                if (n == SENTINEL) break;
                if (n > largest) {
                    secLargest = largest;
                    largest = n;
                } else if (n < largest && n > secLargest) {
                    secLargest = n;
                }
            }
            break;
        }
        println("The largest value is " + largest);
        println("The second largest value is " + secLargest);
    }

    /* Private constant */
    private static final int SENTINEL = 0;
}
\$\endgroup\$
0

5 Answers 5

1
\$\begingroup\$

At first, your code looks good. But there is one bug left. You should have tested your program with the following sequences:

  • 0 (basically works, but the output should indicate that there wasn't a number at all)
  • 5 0 (there is no second number at all, nevertheless your program says that the second-largest number is 5)
  • 5 5 0 (works, both numbers are accepted)
  • 5 3 1 0 (falling numbers, works)
  • 1 3 5 0 (rising numbers, works)
  • 1 5 5 0 (only one of the 5s is accepted, the other is ignored)
  • 5 5 1 0
  • 5 1 5 0

Choosing good test data for a piece of code is as important as writing the code itself. The test data should cover all code paths (there are tools that check this for you, called code coverage) and all corner cases (in your case: equal numbers, large numbers, small numbers, maybe even negative numbers).

In my opinion, you should remove the n < largest condition from the last if statement.

\$\endgroup\$
3
\$\begingroup\$

Please bare in mind that this is my first review on here :)

I see that you wanted to do the whole sorting yourself and personally I think that's good to understand some basics. However in aspect of perforfmance and readability it's not quite the best. Why re-invent the wheel when you can use already existing solutions?

 /*
 * File: FindTwoLargest.java
 * ----------------------
 * This program finds the two largest integers in a list.
 * 
 */

/* Library packages */
import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {

    /* Private constant */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        List<Integer> userInput = new ArrayList<>();
        while (true) {
            int n = readInt("? ");
            if (n == SENTINEL) 
            {
              break;
            }
            userInput.add(n);
        }
        if(!userInput.isEmpty()) {
          java.util.Collections.sort(userInput);
          java.util.Collections.reverse(userInput);
          println("The largest value is " + userInput.get(0));
          println("The second largest value is " + userInput.get(1));
       }
    }
}

So that's a solution I would use. It only uses one while loop which is enough to work and uses less If-Else logic. You can just collect every input given to the list and then use the Collections API from Java to easily sort the list. The call to reverse will change the sorting from ascending to descending meaning the hightest and second highest values are at the beginning.

However if that is not what you wanted or needed I would ask why you used two while loops? It's not necessary for the logic. You can check everything in the first while and remove duplicate code and unneeded checks, which reduce the readability and maintenance of the code.

Another hint would be that with else if you indicate that the previous condition was checked and didn't comply, meaning no need to check again what you did in the first if. You would only need to check that again if you just used a second if.

Which also improves some maintenance and readability, is moving constants and global variables before the constructor of a class. In case you didn't have contact with that yet, a Constructor is the first method in a class ( used for defining the initialization and so on but that doesn't belong here ) and comes before all custom methods. Meaning it's at the top of a class. And to easily get a grip of what is global or where the constants are you should always move those at the top of the class.

import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {

    /* Private constant */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        int n = readInt("? ");
        if (n == SENTINEL) {
            break;
        }
        int largest = n;
        int secLargest = n;
        while (true) {
            n = readInt("? ");
            if (n == SENTINEL) { // I just personally don't like inline if's
                break;
            }
            if ( n >= largest ) {
                secLargest = largest;
                largest = n;
            } else if ( n > secLargest ) {
                secLargest = n;
            }
        }
        println("The largest value is " + largest);
        println("The second largest value is " + secLargest);
    }
}

Note: I tested a slightly different variant of this in the ideone online compiler here.

So with the answer of Roland in mind here's what you can also do to fix those little bugs:

1 5 5 0 (only one of the 5s is accepted, the other is ignored)

Just change the first if too if( n >= largest ) if you don't want to ignore the second entry.

5 0 (there is no second number at all, nevertheless your program says that the second-largest number is 5)

Again this depends on what you want to do. If you don't want to display a secondlargest in this case you could do something like this:

int secondLargest = 0;

...
if(secondLargest != 0){
  println("The second largest value is " + secLargest);
}

0 (basically works, but the output should indicate that there wasn't a number at all)

I just did the sentinel check for the first input already. That allows the user to quit the program directly. If that's not what you want to do you can think about something else.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ why LinkedList? it is only good for sequential access. when you do sort, it will take longer than ArrayList \$\endgroup\$ Oct 3, 2017 at 6:17
  • \$\begingroup\$ @SharonBenAsher thank's for the tip I will change that :) \$\endgroup\$
    – Nico
    Oct 3, 2017 at 6:19
  • \$\begingroup\$ Great answer. I'm a novice. I haven't been introduced to the syntax you mentioned yet. I know there is a better solution. Just working with what I have learned so far. \$\endgroup\$ Oct 4, 2017 at 0:19
3
\$\begingroup\$

Main point: if you know the Java Collections, there is almost always a Collection that helps you with the problem. In this case, you can use PriorityQueue. It does exactly what you want :)

public class TwoInts {
    public static void main(String[] args) {
        List<Integer> input = Arrays.asList(4, 23, 13, 4, 3, 1, 330);


        PriorityQueue<Integer> pq = new PriorityQueue<>(3);

        for (Integer i : input)
        {
            pq.add(i);

            //if there are more than 2 elements, remove the smallest (which is at the head of the queue) 
            if (pq.size()>2)
            {
                pq.remove();
            }
        }
        System.out.println(pq);
    }
}
\$\endgroup\$
1
\$\begingroup\$

This is sort-of implied in another answer, but it would benefit from being made explicit:

        while (true) {
            ...
            break;
        }

with no if (...) continue; in the elided part is equivalent to

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

why do you need two loops with the same loop condition and same break condition and some other identical lines of code ? and why did you put the exit condition as a separate statement? why not while (n != SENTINEL) (by the way, there is nodefinition for SENTINEL)
The code also lacks documentation. a comment for each while statement would explain a lot.
the whole logic seems overly complex to me.
the requirement can be easily fulfilled using one loop and simpler code

largest = 0, secLargest = 0;
n = get first input
loop until end of input
  if n > largest
    secLargest = largest
    largest = n
  else if n > secLargest
    secLargest = n
  n = get next input
end loop
\$\endgroup\$
3
  • 1
    \$\begingroup\$ The definition for SENTINEL is hidden at the bottom of his code. You need to scroll down. That's another recomendation to improve. Constants usually should be before the constructor and functions. \$\endgroup\$
    – Nico
    Oct 3, 2017 at 6:11
  • \$\begingroup\$ I'm a novice. Just working with what I have been taught so far. I actually came up with a very similar solution to yours originally but I tend to make the code suitable to how I see best. May not be best in terms of pleasing everyone or best solution possible. Thanks again. \$\endgroup\$ Oct 4, 2017 at 0:24
  • \$\begingroup\$ @AlexanderJohn, no need to explain, all of us were novices at the beginning . you got many good answers here. hope you've learned from them. \$\endgroup\$ Oct 4, 2017 at 6:23

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.