Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have implemented a program to sort a file consisting of numbers. The program generates a different output file with all numbers sorted in it. I have implemented this program using BitSet. I wanted to understand about BitSet.

public class Application {
    public static void main(String[] args) throws Exception {

        String inputFile = "c:\\temp\\numbers.txt";
        NumberFileSorter.sort(inputFile, 100);
    }
}

class NumberFileSorter {

    private NumberFileSorter() {
    }

    public static void sort(String inputFile, int maxValue) throws Exception {

        final BitSet bitSet = createBitSet(inputFile, maxValue);
        final String outputFile = deriveOutputFile(inputFile);
        writeBitSet(bitSet, outputFile);
    }

    private static BitSet createBitSet(String inputFile, int maxValue)
            throws Exception {

        final FileInputStream stream = new FileInputStream(inputFile);
        final InputStreamReader streamReader = new InputStreamReader(stream);
        final BufferedReader buffReader = new BufferedReader(streamReader);

        String line = null;
        int totalBits = maxValue + 1;
        BitSet bitSet = new BitSet(totalBits);

        try {
            while ((line = buffReader.readLine()) != null) {
                int number = Integer.parseInt(line);
                bitSet.set(number, true);
            }
        } finally {
            buffReader.close();
            streamReader.close();
            stream.close();
        }

        return bitSet;
    }

    private static String deriveOutputFile(String inputFile) {
        String outputFileName = Paths.get(inputFile).getParent().toString()
                + "/output.txt";
        return outputFileName;
    }

    private static void writeBitSet(BitSet bitSet, String outputFile)
            throws Exception {

        final File file = new File(outputFile);
        file.createNewFile();

        final FileOutputStream outputStream = new FileOutputStream(file);
        final OutputStreamWriter writer = new OutputStreamWriter(outputStream);
        final BufferedWriter buffWriter = new BufferedWriter(writer);

        try {
            for (int bitIndex = 0; bitIndex < bitSet.length(); bitIndex++) {
                if (bitSet.get(bitIndex)) {
                    buffWriter.write(Integer.toString(bitIndex));
                    buffWriter.newLine();
                }
            }
        } finally {
            buffWriter.close();
            writer.close();
            outputStream.close();
        }
    }
}

I request reviewers to provide feedback on the following:

  1. Is existing code readable? What are changes required to make it more readable?
  2. Is it advisable to unit test sorting logic by decoupling it from the file-system by introducing appropriate interfaces and using dependency injection?
  3. Can this program be made more reliable and robust?
  4. Can this code be structured differently for the better?
  5. Would you integration test it or unit test it? Could you explain the reason?
share|improve this question

2 Answers 2

Overall your code is very well formatted and readable but I have some complaints at the moment:

  • Don't declare all methods with throws Exception. Only declare the Exceptions you really need to declare, such as IOException. Declaring throws Exception will force the caller to also catch all RuntimeExceptions, which I say is a big no-no as that could potentially catch important bugs (NPEs for example) in your code. I prefer to let RuntimeExceptions cascade up the hierarchy, without catching them.

  • Your deriveOutputFile is as far as I can see only called from one place, and consists of only one line. Does it really need to be it's own method? I'd also say that it's name is somewhat ambiguous. deriveOutputFileName would be better.

  • Your entire class is static. I'm not so sure about this decision. If you'd want to test it in any way you would probably want it non-static, as you then could use a mocking framework to simulate the file input.

Regarding your questions I have some comments:

  • There's a really good question on StackOverflow about Testing internal implementation or public behaviour

  • Decoupling the sorting from the file system sounds like a very good idea and would make your code re-usable, so that you could easily use it in another project if needed. By all means, if you really want to then you can abstract things more by making the sorting an interface, and possibly the file system an interface. Remember that there's a risk of over-engineering though. Always remember the rule of YAGNIT.

share|improve this answer
    
What does NPE stands for? –  Anand Patel Feb 5 at 4:21
    
I always use Exception with throws as a short-cut that saves the need for adding a big list of checked exceptions to the list. I agree that it is bad. –  Anand Patel Feb 5 at 4:26
1  
@AnandPatel NPE = NullPointerException. When it comes to coding, it is dangerous to be lazy. Don't just declare throws Exception because it's the easy thing to do. Do the right thing. –  Simon André Forsberg Feb 5 at 7:27

This example is a good way to learn BitSet. But it can't handle the case when there may be duplicate integers in the file.

If there are duplicate numbers: for example, 2, 2 in the file. In the output, there would be only one 2.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.