6
\$\begingroup\$

Before the interview for the Junior (i hope:)) position of Java developer, I was asked to do a test task. Kindly ask you to review my code. Riht now program arguments are hardcoded and not from args[], tomorrow I will add arguments processing.

You can see my code on GitHub also. I would appreciate any feedback. Thanks!

Project structure:

enter image description here

Task description:

Write a JAVA program that will:
1. Generate a file with random numeric (range from 1 to 2^64 − 1 integers) data. Filesize is limited by command line options. The default file size limit is 64 MB. Each random number is separated by space (ASCII code 32). Program will require 1 argument, which is the file name to be generated.
2. Read the file generated in step #1, analyze it and output it to the console. The output should include:
1. 10 most frequently appeared numbers in bar chart form.
2. The count of Prime numbers.
3. The count of Armstrong numbers.
4. Output separately the time taken to read and analyze the file.
Pay attention:
1. Check error handling.
2. Keep the code clean and formatted, follow the basic JAVA naming conventions.
3. Program speed matters, you may use parallel processing.

Main Class:

package ee.raintree.test.numbers;

import java.io.File;
import java.io.IOException;
import java.math.BigInteger;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;

public class Main {
    private final static String SPACE = " ";
    private static int fileSize = 67108864;
    private static String fileName;

    public static void main(String args[]) throws InterruptedException, ExecutionException, IOException {
        fileName = "result";
        File result = new File(fileName);
        int coreCount = Runtime.getRuntime().availableProcessors();
        ExecutorService service = Executors.newFixedThreadPool(coreCount);

        // Part 1: Generate numbers and write them to file
        List<File> tmpFiles = new ArrayList<>();
        List<Future> futureTmpFiles = new ArrayList<>();
        for (int i = 0; i < coreCount; i++) {
            Future<File> futureTmpFile = service.submit(new TmpNumbersFileCreator(fileSize / coreCount));
            futureTmpFiles.add(futureTmpFile);
        }
        for (int i = 0; i < coreCount; i++) {
            Future<File> futureTmpFile = futureTmpFiles.get(i);
            tmpFiles.add(futureTmpFile.get());
        }

        IOCopier.joinFiles(result, tmpFiles);

        // Part 2: Read numbers from file and analyze them
        long readAndAnalyzeStart = System.currentTimeMillis();

        List<BigInteger> numbers = new ArrayList<>();
        for (String line : Files.readAllLines(result.toPath())) {
            for (String part : line.split(SPACE)) {
                numbers.add(new BigInteger(part));
            }
        }

        int listSize = numbers.size();
        int chunkListSize = listSize / coreCount + 1;
        List<List<BigInteger>> lists = ListSplitter.ofSize(numbers, chunkListSize);

        int countOfPrimeNumbers = 0;
        int countOfArmstrongNumbers = 0;

        List<Future> futurePrimeCounts = new ArrayList<>();
        for(int i = 0; i < coreCount; i++) {
            final int j = i;
            Future<Integer> futurePrimeCount = service.submit(new Callable<Integer>() {
                @Override
                public Integer call() throws Exception {
                    int primeCount = 0;
                    for(BigInteger number : lists.get(j)) {
                        if(number.isProbablePrime(128)) {
                            primeCount++;
                        }
                    }
                    return primeCount;
                }
            });
            futurePrimeCounts.add(futurePrimeCount);
        }

        for (int i = 0; i < coreCount; i++) {
            Future<Integer> futurePrimeCount = futurePrimeCounts.get(i);
            countOfPrimeNumbers = countOfPrimeNumbers + futurePrimeCount.get();
        }

        List<Future> futureArmstrongCounts = new ArrayList<>();
        for(int i = 0; i < coreCount; i++) {
            final int j = i;
            Future<Integer> futureArmstrongCount = service.submit(new Callable<Integer>() {
                @Override
                public Integer call() throws Exception {
                    int armstrongCount = 0;
                    for(BigInteger number : lists.get(j)) {
                        if(MathUtils.isArmstrongNumber(number)) {
                            armstrongCount++;
                        }
                    }
                    return armstrongCount;
                }
            });
            futureArmstrongCounts.add(futureArmstrongCount);
        }

        for (int i = 0; i < coreCount; i++) {
            Future<Integer> futureArmstrongCount = futureArmstrongCounts.get(i);
            countOfArmstrongNumbers = countOfArmstrongNumbers + futureArmstrongCount.get();
        }

        service.shutdown();
        long readAndAnalyzeEnd = System.currentTimeMillis();

        // Part 3: Printing result
        System.out.println("Read and analysis done. Thak took " + (readAndAnalyzeEnd - readAndAnalyzeStart) + " milliseconds.");
        System.out.println("Prime numbers count: " + countOfPrimeNumbers);
        System.out.println("Prime numbers count: " + countOfArmstrongNumbers);
        System.out.println("10 most frequently appeared numbers in bar chart form:");
        Map<BigInteger, Integer> numbersFreqMap = MapUtils.getSortedFreqMapFromList(numbers);
        BarChartPrinter printer = new BarChartPrinter(numbersFreqMap);
        printer.print();

    }
}    

BarChartPrinter Class:

package ee.raintree.test.numbers;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

public class BarChartPrinter<T> {
    private final static String BAR = "|";
    private final static String SPACE = " ";
    List<Entry<T, Integer>> listOfEntries;
    private int chartsCount = 10;
    private int longestEntrySize;
    private int barChartStep;

    public BarChartPrinter(Map<T, Integer> map) {
        listOfEntries = new ArrayList<Entry<T, Integer>>(map.entrySet());
        if (listOfEntries.size() < chartsCount) {
            chartsCount = listOfEntries.size();
        }
        barChartStep = listOfEntries.get(chartsCount - 1).getValue();
    }

    public void print() {
        setLongestEntrySize();
        printBarChart();
    }

    private void printBarChart() {
        for (int i = 0; i < chartsCount; i++) {
            Entry<T, Integer> entry = listOfEntries.get(i);
            int barsCount = entry.getValue() / barChartStep;
            System.out.print(entry.getKey() + getAdditionalSpaces(entry.getKey().toString())  + SPACE);
            for (int bars = 0; bars < barsCount; bars++) {
                System.out.print(BAR);
            }
            System.out.println();
        }
    }

    private void setLongestEntrySize() {
        int longest = 0;
        for(int i = 0; i < chartsCount; i++) {
            if(listOfEntries.get(i).getKey().toString().length() > longest) {
                longest = listOfEntries.get(i).getKey().toString().length();
            }
        }

        longestEntrySize = longest;
    }

    private String getAdditionalSpaces(String string) {
        StringBuilder sb = new StringBuilder();
        int needSpaces = longestEntrySize - string.length();
        for(int i = 0; i < needSpaces; i++) {
            sb.append(SPACE);
        }
        return sb.toString();
    }
}

IOCopier Class, totally copied from some semi-official source:

package ee.raintree.test.numbers;

import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.List;

import org.apache.commons.io.IOUtils;

class IOCopier {
    public static void joinFiles(File destination, List<File> sources) {
        try (OutputStream output = createAppendableStream(destination)) {
            for (File source : sources) {
                appendFile(output, source);
            }
        } catch (IOException e) {
            System.out.println("Error joining files");
        }
    }

    private static BufferedOutputStream createAppendableStream(File destination) throws FileNotFoundException {
        return new BufferedOutputStream(new FileOutputStream(destination, true));
    }

    private static void appendFile(OutputStream output, File source) {
        try (InputStream input = new BufferedInputStream(new FileInputStream(source))) {
            IOUtils.copy(input, output);
        } catch (IOException e) {
            System.out.println("Error appending file");
        }
    }
}

ListSplitter, totally copied from some semi-official source:

package ee.raintree.test.numbers;

import java.util.AbstractList;
import java.util.ArrayList;
import java.util.List;

public class ListSplitter<T> extends AbstractList<List<T>> {

    private final List<T> list;
    private final int chunkSize;

    public ListSplitter(List<T> list, int chunkSize) {
        this.list = new ArrayList<>(list);
        this.chunkSize = chunkSize;
    }

    public static <T> ListSplitter<T> ofSize(List<T> list, int chunkSize) {
        return new ListSplitter<>(list, chunkSize);
    }

    @Override
    public List<T> get(int index) {
        int start = index * chunkSize;
        int end = Math.min(start + chunkSize, list.size());

        if (start > end) {
            throw new IndexOutOfBoundsException("Index " + index + " is out of the list range <0," + (size() - 1) + ">");
        }

        return new ArrayList<>(list.subList(start, end));
    }

    @Override
    public int size() {
        return (int) Math.ceil((double) list.size() / (double) chunkSize);
    }
}

MapUtils Class:

package ee.raintree.test.numbers;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Map.Entry;

public class MapUtils {

    public static <T> Map<T, Integer> getSortedFreqMapFromList(List<T> list) {
        Map<T, Integer> map = getFreqMapFromList(list);
        Set<Entry<T, Integer>> entries = map.entrySet();
        List<Entry<T, Integer>> listOfEntries = new ArrayList<Entry<T, Integer>>(entries);
        Collections.sort(listOfEntries, getValueDescComparator());
        Map<T, Integer> sortedByValue = new LinkedHashMap<T, Integer>(listOfEntries.size());
        for (Entry<T, Integer> entry : listOfEntries) {
            sortedByValue.put(entry.getKey(), entry.getValue());
        }
        return sortedByValue;
    }

    private static <T> Map<T, Integer> getFreqMapFromList(List<T> list) {
        Map<T, Integer> result = new HashMap<>();
        for (T item : list) {
            if (result.get(item) == null) {
                result.put(item, 1);
            } else {
                result.put(item, result.get(item) + 1);
            }
        }
        return result;
    }

    private static <T> Comparator<Entry<T, Integer>> getValueDescComparator() {
        Comparator<Entry<T, Integer>> valueComparator = new Comparator<Entry<T, Integer>>() {
            @Override
            public int compare(Entry<T, Integer> e1, Entry<T, Integer> e2) {
                Integer v1 = e1.getValue();
                Integer v2 = e2.getValue();
                return v2.compareTo(v1);
            }
        };
        return valueComparator;
    }
}

MathUtilsClass:

package ee.raintree.test.numbers;

import java.math.BigInteger;

public class MathUtils {
    public static boolean isArmstrongNumber(BigInteger number) {
        String numberInString = number.toString();
        int digitsCount = number.toString().length();
        int power = digitsCount;
        BigInteger sum = BigInteger.ZERO;

        for (int i = 0; i < digitsCount; i++) {
            int digit = Character.getNumericValue(numberInString.charAt(i));
            BigInteger digitInPower = BigInteger.valueOf(digit).pow(power);
            sum = sum.add(digitInPower);
        }

        return sum.compareTo(number) == 0;
    }
}

TmpNumbersFileCreator Class:

package ee.raintree.test.numbers;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.PrintWriter;
import java.math.BigInteger;
import java.util.Random;
import java.util.concurrent.Callable;

public class TmpNumbersFileCreator implements Callable<File> {
    private File file;
    private PrintWriter printWriter;
    private static final String SEPARATOR = " ";
    private int size;

    public TmpNumbersFileCreator(int size) {
        this.size = size;
    }

    @Override
    public File call() throws Exception {
        return getTempFile();
    }

    public File getTempFile() {
        createTempFile();
        writeNumbersToFile();
        return file;
    }

    private void createTempFile() {
        try {
            file = File.createTempFile("numbers-", "-txt");
            file.deleteOnExit();
        } catch (IOException e) {
            System.out.println("Temporary file creation failed");
        }
    }

    private void writeNumbersToFile() {
        try {
            printWriter = new PrintWriter(file);
        } catch (FileNotFoundException e) {
            System.out.println("Temporary file not found");
        }
        while (!isFileSizeMax()) {
            printWriter.write(getRandomNumber().toString() + SEPARATOR);
        }
        printWriter.flush();
        printWriter.close();
    }

    private BigInteger getRandomNumber() {
        Random random = new Random();
        BigInteger number;
        do {
            number = new BigInteger(64, random);
        } while (number.equals(BigInteger.ZERO));
        return number;
    }

    private boolean isFileSizeMax() {
        if (file.length() <= size) {
            return false;
        }
        return true;
    }
}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ This is a pretty extensive and complex task just to get invited to a job interview. The bar chart requirement is pretty off the hook (and underspecified, what do they actually want here, just a line of asterisks?) I'd seriously consider not working for this company. \$\endgroup\$
    – markspace
    Jan 4, 2020 at 23:14
  • \$\begingroup\$ @markspace Thank you very much for your feedback! In fact, this is a fairly well-known and old company in our area. In any case, I am now in search of my first IT job and, with my resume and lack of experience, I have no way to choose which company to work in. \$\endgroup\$ Jan 5, 2020 at 9:00
  • \$\begingroup\$ If an interview task is incomplete it is always because they want to see if and what kind of questions you ask to get clarifications. It doesn't tell anything about the company. \$\endgroup\$ Jan 6, 2020 at 19:40
  • \$\begingroup\$ The task explicitly asks for a couple of things you haven't implemented yet. When working on such projects, make sure you keep a feature-list you check-off. Tests are useful for this too. \$\endgroup\$
    – Mast
    Jan 7, 2020 at 8:39
  • \$\begingroup\$ @Mast thank you for your comment! Did you mean "Check error handling" or something else also? \$\endgroup\$ Jan 7, 2020 at 11:54

2 Answers 2

5
\$\begingroup\$

Before handing the company any code you wrote, you should ask for clarification of the task.

Write a JAVA program that will:

The correct spelling is Java, not JAVA.

  1. Generate a file with random numeric (range from 1 to 2^64 − 1 integers) data.

The grammar is slightly wrong here. They should have written "with random integers in the range from 1 to 2^64 - 1". I don't think you are supposed to squeeze 2^64 integers into a file that is only 64 MB in size.

Are duplicate numbers allowed? What is the purpose of these random numbers, after all?

Should the random numbers follow a certain distribution?

Filesize is limited by command line options.

What does this mean? In another part of the task they say "Program will require 1 argument", which contradicts this sentence.

Also, how is the file size specified? It could be -max 64MB or -max-file-size=64m or --maximal-file-size 32M or max=16000k. Also, when they say MB, do they mean 1_000_000 or rather 1_048_576?

  1. Read the file generated in step #1, analyze it and output it to the console.

This could mean you are supposed to write the whole 64 MB to the output in a single line. Is that really what they want, and if so, why?

The output should include:

Does the word "include" here mean you are allowed to output arbitrary other things?

  1. 10 most frequently appeared numbers in bar chart form.

How wide should the bar chart be? Should that be configurable by command line arguments?

  1. The count of Prime numbers.

Is it sufficient if the program outputs the count of probable primes (like your code currently does)?

Should the program output some example prime numbers, in addition to the count? This would allow a human reader to crosscheck whether the program works reliably.

  1. Output separately the time taken to read and analyze the file.

Wall time or CPU time?

Program speed matters

That's too imprecise. What run time is acceptable for the program? Is 5 minutes ok, or does it have to be less than 10 seconds? How many CPUs are available for parallel processing?


All these questions are typical for your future everyday job. Often the people who give you tasks like these don't know exactly what they really want. By asking these questions in a polite and calm way, you make them think again about their requirements and whether they make sense at all.


Regarding your code: You should install IntelliJ and load your code there. IntelliJ will produce many warnings and hints how you can improve your code. In many situations IntelliJ can also fix the code for you. Just place the text cursor on a warning and press Alt+Enter to see whether there is a fix or a refactoring for that warning.

For example, starting with Java 8 it is no longer usual to write the verbose implements Callable<X>. Instead, unnamed functions are used. These are also called lambda functions.

Your error handling is broken. If nothing else is said, when an error occurs while writing the file with the random numbers, it does not make sense to continue at all. Therefore it is wrong to just write an error message to System.out. Just let the IOException bubble up until some code really knows what to do with this exception. In your case you don't know at all, therefore no part of your code should catch this exception.

In MathUtils there should be a method isPrime. Your current code is inconsistent since isArmstrongNumber is implemented there but isPrime isn't.

You should add some unit tests to your code, just to prove that you tested the basic cases.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you @Roland for your detailed comment! Actually I was also a little bit confused about "Filesize is limited by command line options" and I asked for clarification and get answer, that program can take 2 arguments. Yes, task description is not very clear. Also I did as you adviced with IntelliJ and MathUtils class - now code is cleaner, thank you for that! \$\endgroup\$ Jan 5, 2020 at 10:09
3
\$\begingroup\$

Did you measure run time before implementing the multi threaded random number generator and analyser? I'm betting that combining the files takes a lot more time than you gain from concurrency (IO is slow). This would be premature optimization and a red flag.

The main method should not contain any logic other than parsing arguments to a format understood by the business logic. You should have the number generator, number analyser and number printer as a self contained classes and have the main method pass data between them. Study the single responsibility principle.

I think you were supposed to print two times: reading time and analysis time.

You read the numbers to memory and loop over them three times (so four loops). You should have been able to do the analysis while reading the numbers from the file (one loop). Again, did you measure the effect of the multi threaded analysis versus single threaded? The task did not specify upper limit to the file size so by reading the data to memory you created an unnecessary artificial limit from the JVM memory.

I was expecting some comments explaining why you chose to code as you did.

ListSplitter does a lot of unnecessary copying. It should not extend AbstractList as a simple utility method would suffice. If you submit copied code, always try to copy good code. :)

You're creating a new instance of Random every time you create a random number. That's unnecessary and complete waste of time. The Random should be an instance variable.

Concatenating the separator to the number before writing is unnecessary waste of time as it creates a new immediately disposed string object. Write the number first and then write the separator (as a character, not string).

The if-statement in the file size checking that returns true or false only creates unnecessary cognitive load. Just write:

return file.length() > size;

Checking the number of bytes written by calling file.length() is quite expensive as it goes all the way to the file system to get the result. It also fails to take into account any buffering that may occur during the writing, possibly causin errors. It would be more efficient to simply keep an integer counter of the number of bytes written.

You're using PrintWriter to write the numbers but you are not using any special functionality from it. It gives the impression that you're not familiar with the IO-classes. You should use BufferedWriter instead to get the speed benefit of buffered writing (you need to count the written bytes manually now).

Don't forget to specify the character encoding of the file! Even though you're only writing numbers and spaces and the resulting file will most likely always be ASCII-compatible, explicitely specifying it tells the reader you're not one of the people who cause character encoding problems in production by always relying on system default encoding.

This one is particularly bad copy-pasting as it is hard to read and very inefficient. You should first get the value to a variable and use it in the if-statement and assignment.

if(listOfEntries.get(i).getKey().toString().length() > longest) {
    longest = listOfEntries.get(i).getKey().toString().length();
\$\endgroup\$
6
  • \$\begingroup\$ Thank you for your useful comments, I'm already rewriting my code now. I was also thinking that Main class have too much logics, that should be in other classes. After editing TmpNumbersFileCreator (one time new Random() instantiation, checking syze by written bytes amount comparing) file creation speed increases drammatycally, thank you for that! About speeds: I cheked on my machine that file creation with MT is about 4 times faster than ST and List<BigInteger> analysis with MT is about 2.5 times faster. Merging files is surprisingly fast - about 150ms to create 64MB file. \$\endgroup\$ Jan 7, 2020 at 11:30
  • \$\begingroup\$ I'm also want to read file byte-by-byte with FileInputStream and analyze numbers during this reading, but now I dont know how to speed up this process and use parallel processing in this. \$\endgroup\$ Jan 7, 2020 at 11:39
  • \$\begingroup\$ Actually now I was testing this "analyze-on-fly" approach and that analysis took 5 minutes with 64MB file! With my old splitted-list-approach is was 13 seconds. \$\endgroup\$ Jan 7, 2020 at 12:45
  • \$\begingroup\$ Did you remove buffering from the reading too? \$\endgroup\$ Jan 7, 2020 at 12:54
  • \$\begingroup\$ I created this analyzer class \$\endgroup\$ Jan 7, 2020 at 14:45

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.