Sorting numbers using Java Collections.sort()

Question

Input is a file containing numbers in this format:

1.2 3 5.1 2 7 4 6
5 2 9.22 1 0.0 4 3

I am doing the sorting in the simplest way, using Collections.sort method. The running code is as follows:

import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Scanner;


public class SortedNumbers {

    public static Double integerFromString(String str) {
        return Double.valueOf(str.substring(0, str.length()));
    }

    public static void main(String[] args) throws FileNotFoundException {
         String fileName = "";

        //scanner to read the file
        Scanner fileInput = null;

        List<String> temps = new LinkedList<String>();

        //first check if there is an input parameter
        if(args.length > 0) {
            fileName = args[0];
        }

        else {
            System.out.println("No file specified");
            System.exit(1);
        }

        //connect to the file
        FileReader file = new FileReader (fileName);
        fileInput = new Scanner(file);

        //start reading from the file, store all the lines in the list
        while(fileInput.hasNextLine()) {
            String num = fileInput.nextLine();
            temps.add(num);
        }

        //close the scanner
        fileInput.close();

        //convert the list into array of strings
        String[] str = temps.toArray(new String[0]);

        /**
         * for each string in str array, split it using space as the delimiter and store the splitted strings in an array,
         * for each string value in the array, convert it into Double and store it in a collection
         * Sort the collection, convert it into String
         */
        for(int i = 0; i < str.length; i++){
            List<Double> num = new LinkedList<Double>();
            String[] arr = str[i].split("\\s");
            for(int j = 0; j < arr.length; j++){
                Double currentNumber = integerFromString(arr[j]);
                num.add(currentNumber);
            }
            Collections.sort(num);
            String sortedNumbers = num.toString();
            int indexOfOpenBracket = sortedNumbers.indexOf("[");
            int indexOfLastBracket = sortedNumbers.lastIndexOf("]");
            System.out.println(sortedNumbers.substring(indexOfOpenBracket+1, indexOfLastBracket));
        }
    }
}

I am concerned about the performance, splitting the string and storing it in array, parsing each string from array into double and then adding in the list. It seems like there could be an efficient way.

Are there any redundant steps in the code which I can avoid?

Answer 1

There are a few improvements I can see, but not really related to performance (though it may run a bit faster anyway).

If the files are small (less than 10Megabytes or so), then the relative performance of different approaches are slight (obviously, you can bork your code, but the IO part will remain about the same). So, it comes down to readability.

For readability, I would recommend using the 'cool' toys in Java7. Consider the following:

List<String> lines = Files.readAllLines(Paths.get(args[0]));
for (String line : lines) {
    String[] parts = line.split("\\s+");
    double[] values = new double[parts.length];
    for (int i = 0; i < parts.length; i++) {
        values[i] = Double.parseDouble(values[i]);
    }
    Arrays.sort(values);
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < values.length; i++) {
        if (i > 0) {
            sb.append(", ");
        }
        sb.append(values[i]);
    }
    System.out.println(sb.toString());       
}

Note the key elements here:

• use Files and Paths classes
• use StringBuilder for the string concatenation
• use String.split, not a scanner....
• use primitive data structures (arrays of double, not Double)
• that means using Arrays.sort, not Collections.sort

Other things to consider.....

• a method called integerFromString ... that returns a double is ... confusing.
• using List.toString() and the stripping off the [ and ] is backward.

Answer 2

A few things that @rolfl didn't mention explicitly:

public static Double integerFromString(String str) {
    return Double.valueOf(str.substring(0, str.length()));
}

This code alone can be significantly improved.

• @rolfl is right that it has a confusing name as it returns a Double.
• When you use str.substring(0, str.length()) you'll get back exactly the same string you started with so simply str would be enough.

• There is no need to return the non-primitive Double, instead use the method parseDouble which returns a primitive double. If you are not aware of the differences, read up about them.

  public static double doubleFromString(String str) { return Double.parseDouble(str); }

This however makes this method more or less pointless, it would be easier and more clear to use Double.parseDouble directly.

---

There is, in my opinion, not much use for the fileName variable. Sure, it can be used to make it more clear what the contents of args[0] represent, but you're only using it once. Consider making this change:

    if (args.length == 0) {
        System.out.println("No file specified");
        System.exit(1);
    }

    FileReader file = new FileReader(args[0]);

---

I would rather use an ArrayList than a LinkedList when sorting. They have some differences in speed.

---

The variable names temps and str, arr are not very describing. The same goes for several of your other variables. Consider the names inputLines, line, splittedLine instead. String num is also a confusing name as it holds the entire line and not only a number, rename to inputLine or similar instead (or you could just do inputLines.add(fileInput.nextLine()); without using a temporary variable).

There is no need to read the input to a list and then convert to an array. You can loop through a List by using a "for-each" loop.

for (String line : inputLines) {
    ...
}

However, as all you want to do is to process the lines once, there is no need to store it in a list first just to process it later.

Overall, I totally agree with everything @rolfl said, I just wanted to provide these additional comments.

Answer 3

With Java 8 we can actually write this nicely succinct:

public static void main(String[] args) throws Exception {
    if (args.length != 1) {
        System.out.println("Expected: <filename>");
        System.exit(1);
    }
    // The number of times where using the platform default encoding is a good idea
    // is pretty much nil..
    String data = new String(Files.readAllBytes(Paths.get(args[0])), "UTF-8");
    String result = Arrays.stream(data.split("\\s+")).
            mapToDouble(Double::parseDouble).
            sorted().
            mapToObj(Double::toString).
            reduce("", (x, y) -> x + y + " ");
    System.out.println(result);
}

Obviously instead of reducing it to a simple string, we could just as easily print it directly:

    Arrays.stream(data.split("\\s+")).
            mapToDouble(Double::parseDouble).
            sorted().
            forEach(d -> System.out.print(d + " "));

The only annoyance is that Java doesn't offer any reduceLeft or similar method, which means we either have to provide one ourselves or live with the fact that there's a trailing space at the end of the output.

The whole thing is almost certainly IO limited, but from a performance point of view we avoid the unnecessary boxing to double and it is trivial to parallelize this if we were inclined to do so. So not only is it way more readable, descriptive and shorter, but also potentially more performant as well.

On the other hand this doesn't do an in-place sort which can be problematic in case of a really large file, in which case we'd want to get the array back, sort it the old way and then just print it (which can be done efficiently again using forEach).

Edit: Since the idea was to print each line itself sorted we need some small change to make it do that instead:

    Files.lines(Paths.get(args[0]), StandardCharsets.UTF_8).
            map(line -> line.split("\\s+")).
            map(vals -> Arrays.stream(vals).
                    mapToDouble(Double::parseDouble).
                    sorted().
                    mapToObj(Double::toString).
                    collect(Collectors.joining(", "))).
            forEach(System.out::println);

which shows one definite advantage of a descriptive approach. The corresponding code to reduce it to an array of Strings instead of printing directly is left as an exercise for the reader.