Printing longest lines

Challenge

Write a program which reads a file and outputs a specified number of lines, sorted on length in descending order.
Specifications

• The first argument is a path to a file.

• The file contains multiple lines.

• The first line indicates the number of lines to output.

• The following lines are of differing lengths and presented randomly.

• Print the specified number of lines in descending order of length.

Constraints

• The number in the first line is a valid positive integer.

• The input file is correctly formatted.

Input Sample

2

Hello World

Code Reviews

Quick Fox

A

San Francisco

Output Sample

San Francisco

Code Reviews

Source

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.NavigableMap;
import java.util.Scanner;
import java.util.TreeMap;

public class Solution {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new File(args[0]));
        int numberOfLinesToPrint = Integer.parseInt(input.nextLine());

        NavigableMap lineMap = new TreeMap<>();
        while (input.hasNextLine()) {
            addLine(lineMap, input.nextLine());
        }
        printLongestLines(lineMap, numberOfLinesToPrint);
    }

    private static void printLongestLines(NavigableMap lineMap, int numberOfLinesToPrint) {
        for (int i = 0; i  lineMap, String line) {
        lineMap.put(line.length(), line);
    }
}

I thought about implementing a data structure based on the size of the input that would sort itself right after adding an input -- if the input was a valid addition, but then I remembered TreeMap and thought why reinvent the wheel? This works, but I'm curious if I've misjudged and the alternative would've been more efficient.

Remember to close resources

You are opening a file with

Scanner input = new Scanner(new File(args[0]));

but you are never closing the Scanner. This can lead to resource leaks and hard to track down bugs. This is typically solved by using a try-with-resources construct:

try (Scanner input = new Scanner(new File(args[0]))) {
    // ...
}

With this change, the Scanner will be opened at the beginning of the block, and always closed, automatically, at the end. This ensures that, whatever happens in the try block (exception or not), the resource will be properly closed.
Use of methods

Your code makes use of 2 methods:

• printLongestLines, which is responsible for printing the top-most lines read.

• addLine, which adds a line into a map.

The first method, printLongestLines, is great: its name is descriptive and does the job it is intended to do.

However, the second is awkward: it takes as parameter a map that it will modify. Generally, modifying input parameters inside a method is a bad practice. It means that the code isn't correctly decoupled. In this case, it is because this method is actually part of a bigger responsibility: storing the lines of the file inside a map. Conceptually, you want to break down the program into meaningful methods that perform a task; the task here is reading the file.

As such, consider the following method readLines instead:

private static NavigableMap readLines(Scanner input) {
    NavigableMap lineMap = new TreeMap<>();
    while (input.hasNextLine()) {
        String line = input.nextLine();
        lineMap.put(line.length(), line);
    }
    return lineMap;
}

The advantage is that reading from the Scanner and populating the line map is done inside a well defined method, that the main part of the code just invokes.
Iterators

The method printLongestLines is currently using pollLastEntry() to retrieve, and remove, the last entry of the map. This is a side-effect operation: this method is modifying the input map, by removing elements from it.

It would be preferable to use an Iterator to iterate over the map. Consider the following:

private static void printLongestLines(NavigableMap lineMap, int numberOfLinesToPrint) {
    Iterator iterator = lineMap.descendingMap().values().iterator();
    int i = 0;
    while (i < numberOfLinesToPrint && iterator.hasNext()) {
        System.out.println(iterator.next());
        i++;
    }
}

This gets the reversed map (with descendingMap) and iterates over the values only, since we're not interested in the key. It also highlights a potential bug in your current implementation: if you were to print a number of lines greater than the size of the map, it would fail. This is handled here by checking whether the iterator has a next element.
You don't need a map

You will notice that, in the map that was built, we're only interested in the values, i.e. the lines themselves. This shows that, in fact, we do not need a map. We can directly use a SortedSet where the String are sorted by their length.

private static SortedSet readLines(Scanner input) {
    SortedSet set = new TreeSet<>(Comparator.comparing(String::length, Comparator.reverseOrder()));
    while (input.hasNextLine()) {
        set.add(input.nextLine());
    }
    return set;
}

This creates a set that is sorted in reverse order using a comparator comparing their length. This is using Java 8 specific code (comparing and the method reference String::length) but the same can be done with anonymous classes.

With this change, you can have a very simple printLongestLines method:

private static void printLongestLines(SortedSet set, int numberOfLinesToPrint) {
    set.stream().limit(numberOfLinesToPrint).forEach(System.out::println);
}

This is using the Stream API, but you could keep the same code as before for Java to store the lines of the file. Depending on the use-case, this also would have the advantage that duplicate lines are kept. The change to do in readLines` would simply be to explicitely sort the list once the lines are read, with:

List lines = new ArrayList<>();
// loop over input to add each line
lines.sort(Comparator.comparing(String::length).reversed());

StackExchange Code Review Q#131803, answer score: 4