Multiplying Lists

Challenge:


Given 2 lists of positive integers.

Write a program which multiplies corresponding elements in these lists

Specifications:


Your program should accept as its first argument a path to a filename.

The lists are separated with pipe char: '|' .

Numbers are separated with a space char.

The number of elements in lists are in range [1, 10].

The number of elements is the same in both lists.

Each element is a number in range [0, 99].

Solution

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class MultiplyLists {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner fileInput = (args.length > 0) ? 
            new Scanner(new File(args[0])) :
            new Scanner(System.in)
        ;

        while (fileInput.hasNextLine()) {
            printMultipliedList(fileInput.nextLine());
        }
    }

    private static void printMultipliedList(String line) {
        String[] params = line.split("\\|");
        StringBuilder multiplied = new StringBuilder();

        for (int n : getMultipliedList(params[0].trim().split("\\s+"), params[1].trim().split("\\s"))) {
            multiplied.append(n).append(' ');
        }
        multiplied.setLength(multiplied.length() - 1);

        System.out.println(multiplied.toString());
    }

    private static List getMultipliedList(String[] first, String[] second) {
        List result = new ArrayList<>();

        for (int i = 0; i < first.length; i++) {
            result.add(
                Integer.parseInt(first[i]) * Integer.parseInt(second[i])
            );
        }
        return result;
    }
}

Sample Input:


9 0 6 | 15 14 9

5 | 8

13 4 15 1 15 5 | 1 4 15 14 8 2

Sample Output:


135 0 54

40

13 16 225 14 120 10

• My solution seems really convoluted, how might I improve it?

• Is there anything I do here that I should try to avoid in th

My solution seems really convoluted, how might I improve it?

It doesn't seem so too convoluted, but maybe that's partly because the problem itself is simple. A couple of things can be improved.

Scanner fileInput = (args.length > 0) ?
            new Scanner(new File(args[0])) :
            new Scanner(System.in)
            ;

Some issues with this snippet:

• The variable is named fileInput, but it's not always a file input, sometimes it's standard input, so the name can be misleading

• The spec says the first arg is a filename, so you don't actually need to support standard input at all

• No need to put parens around args.length > 0

Following the spec, the line can be simplified to:

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

This meets the spec, but not very user-friendly.
But in the context of this question I don't think that matters.

private static void printMultipliedList(String line) {

This may sound a bit strange, but I think it's good to avoid writing code.
Every line of code is a potential bug.
Only write as much code as necessary.

The printMultipliedList(String line) method does too many things:

• It splits the input, which is part of the input parsing logic

• It formats and prints the output

It's good when a method does just one logical thing.
It would be better to have a cleaner separation of the input parsing and output formatting logic.

Consider refactoring it like this:

private static void parseInputAndPrintMultipliedList(String line) {
    List> intLists = parseIntLists(line);
    List multipliedList = getMultipliedList(intLists.get(0), intLists.get(1));
    String output = formatOutput(multipliedList);
    System.out.println(output);
}

Now this method contains only higher level logical steps,
and delegates all actions to other methods,
each doing just one logical thing.

See a sample implementation of the new methods I introduced at the end of this post.
Also notice that the getMultipliedList in this snippet is different from yours,
because this one takes two lists of integers, instead of string arrays.
The reason is that the original getMultipliedList method does too many things:
it does input parsing (string to int) and it multiplies lists.

for (int n : getMultipliedList(params[0].trim().split("\\s+"), params[1].trim().split("\\s"))) {
        multiplied.append(n).append(' ');
    }
    multiplied.setLength(multiplied.length() - 1);

There are several issues:

• The trimming and splitting is duplicated for params[0 and params[1]. It would be better to move this logic to a helper method

• You can actually avoid trimming, if you create params like this: params = line.split("\\s\\|\\s")

• The line in the for statement is too long. It's good to keep lines short enough to be visible without having to scroll horizontally. If you rewrite this line using the helper method I suggested earlier, it might become short enough. Or if it's still too long, then it will be better to put those values into local variables. The compiler will know to inline them, and it will improve readability.

• Instead of appending ' ' to all elements and cutting off the last one, it might be slightly better to add the first item, and iterate from the 2nd until the end, and in each step do .append(' ').append(n), so you don't have to cut anything off later. But this is a very minor thing.

Is there anything I do here that I should try to avoid in the future?

To summarize what I already mentioned above:

• Avoid writing code

• Keep in mind the single responsibility principle, and make sure that each method does just one logical thing

• Avoid duplication of logic, extract common logic to a helper method

Is this inefficient?

There are no serious efficiency problems.
There are minor inefficiencies,
but I don't think they are worth tuning:

• Splitting by |, and then again splitting by ` visits all characters in the input twice

• Multiplying the list items, and then iterating over the items for printing visits all elements twice

My suggestions make this worse, adding one more iteration due to parsing the list of strings to list of integers. But I don't think this is a problem. The time complexity of the solution remains \$O(n)\$, whether we process the list items in 3, 4, or 5 passes.
Clarity of the logic and following the single responsibility principle is more important.

Suggested implementation

Applying the suggestions about, the implementation becomes:

``
class MultiplyLists {
public static void main(String[] args) throws FileNotFoundException {
Scanner fileInput = new Scanner(new File(args[0]));

while (fileInput.hasNextLine()) {
parseInputAndPrintMultipliedList(fileInput.nextLine());
}
}

private static String formatOutput(List result) {
StringBuilder builder = new StringBuilder();
Iterator iterator = result.iterator();
builder.append(

StackExchange Code Review Q#78468, answer score: 4