4
\$\begingroup\$

I am working on a simple personal project to build a Hoffman Tree. In my constructor, shown below, I am reading in a file, creating HoffmanNodes that represent each char in the file then I got through the file to encode it. I need to create a Scanner to go through the file once to find all the chars and their frequency, then I recreate the scanner of the same file and go through it again to encode it.

I have tried to do some research to find a more efficient way to do this, and I am coming up short. Is there an easier way to do this?

public HoffmanCoding(File inFile) {

    Scanner in = createScanner(inFile);
    //Check empty file
    if (in.hasNextLine() == false) {
        throw new IllegalArgumentException("Can not encode an empty file");
    }

    //Create a HashMap that holds all the HoffmanNodes in the HoffmanTree
    nodeHashMap = buildNodeHashMap(in);
    //Use the HashMap to create a Priority Que that I then can use to make the tree
    PriorityQueue<HoffmanTreeNode> allNodes = new PriorityQueue<>(nodeHashMap.values());
    //Build the HoffmanTree
    this.rootOfHoffmanTree = buildHoffmanTree(allNodes);
    //Assign a code to all the nodes based on their position in the tree.
    assignCode(this.rootOfHoffmanTree, "");

    // Recreate scanner reset it to the begging of the file
    in = createScanner(inFile);
    //Encode and Decode the message.
    this.encodedMessage = encodeMessage(in);
    in.close();
    this.decodedMessage = decodeMessage(this.encodedMessage);
}
\$\endgroup\$
  • \$\begingroup\$ Huffman, not Hoffman? en.wikipedia.org/wiki/Huffman_coding \$\endgroup\$ – forsvarir Jan 30 at 10:24
  • 1
    \$\begingroup\$ @forsvarir Ya i fixed that in my final version on my github but I didn't notice before I posted this. Thanks! \$\endgroup\$ – Luke Kelly Jan 30 at 22:06
5
\$\begingroup\$

I'd be very skeptical about making any changes to the current code you have when it comes to creating the scanners. However, I do want to indicate that reopening a file while it is still open is probably not a good idea. So I'd use a try-with-resources when you create the Scanner instances. I don't think reopening a file is so resource intensive that it needs to be avoided.

However, file access is commonly performed using a file pointer, and that file pointer can be reset. The question is then how to make a FileInputStream aware of that. This can be done by asking for the file channel (introduced with Java NIO2) and then setting the position on that.

So if we read the documentation of FileInputStream#getChannel() we get:

Reading bytes from this stream will increment the channel's position. Changing the channel's position, either explicitly or by reading, will change this stream's file position.

Ah! Right what we are after, so here is code that leaves the file open:

try (FileInputStream fis = new FileInputStream(inFile)) {
    Scanner scanner1 = new Scanner(fis);
    System.out.println(scanner1.nextLine());

    fis.getChannel().position(0L);

    Scanner scanner2 = new Scanner(fis);
    // prints the same line!
    System.out.println(scanner2.nextLine());

    // the underlying file stream will be closed anyway, but yeah...
    scanner2.close();
    scanner1.close();
}

Reusing a scanner itself doesn't work. The reason is that Scanner instances cache data, which means that that data is not present anymore in any underlying stream. This is why no such functionality is supplied. Scanner is not a very heavy-weight component to instantiate, so I guess that's all right.

The above code is pretty ugly because how Java handles close(). Any call to Scanner.close() - implicit or explicit - will also close the underlying FileInputStream. That's of course not very useful in this scenario.

An ugly hack is to create a FilterInputStream-based decorator that simply forwards everything and then simply doesn't call close on the underlying stream. That way you can close the scanners without closing the underlying FileInputStream. But yeah, just as yucky as above code.

The fact that it is possible to re-read the file without re-opening it is not sufficient reason to use tricks like above. If you close the file then the file handle is released and when reopening the file is probably still cached.


I don't know enough about your application to know if rewriting your code to use Java NIO directly makes sense. Scanner was added to Java to make it easy to parse simple input, it was not meant to parse complex files, as the first line of documentation shows:

A simple text scanner which can parse primitive types and strings using regular expressions.


Your code looks OK-ish. I'm very worried about the number and use of fields though. Generally a lot of fields is a red flag when it comes to class design. The fact that all the processing is performed in the constructor is not a good sign either. The separate part of encoding should take place in a separate method, which would make above solution even harder to apply.

\$\endgroup\$
2
\$\begingroup\$

I'm omitting the lines from your method not directly involved with creation of Scanner:

Scanner in = createScanner(inFile);
//Check empty file
if (in.hasNextLine() == false) {
       throw new IllegalArgumentException("Can not encode an empty file");
}
//...other lines not involved with scanners 
in = createScanner(inFile); <-- assignment of second scanner, first not closed
in.close();

The first Scanner resource is not closed both in the case of empty file (exception cause the exit from the method) and when you reassign a new Scanner to the variable containing the old Scanner instance, so you have a resource leak.

Personally I would separate the control of empty file from creation of a Scanner instance: a possible solution for it using the File.empty method (lot of debates about possible solutions how to check if one file is empty, check which is the best method for you)

public HoffmanEncoding(File inFile) throws FileNotFoundException {
    if (inFile.length() == 0L) {
        throw new IllegalArgumentException("Can not encode an empty file");
    }
    // other lines of the method
}

After, I would put the try with resources construct inside your methods encodeMessage and buildNodeHashMap to ensure the automatic closing of scanners like the code below:

public class HoffmanEncoding {

    public HoffmanEncoding(File inFile) throws FileNotFoundException {
        if (inFile.length() == 0L) {
            throw new IllegalArgumentException("Can not encode an empty file");
        }
        //omitting all lines in the method not involved with scanners
        nodeHashMap = buildNodeHashMap(inFile);
        encodedMessage = encodeMessage(inFile);
    }

    private String encodeMessage(File inFile) throws FileNotFoundException {
        StringBuilder encodedMessage = new StringBuilder();
        try (Scanner sc = new Scanner(inFile)) {
            //operations to construct the message
        }
        return encodedMessage.toString();
    }

    private Map buildNodeHashMap(File inFile) throws FileNotFoundException {
        Map nodeHashMap; //initialize it 
        try (Scanner sc = new Scanner(inFile)) {
            //operations to populate the map
        }
        return nodeHashMap;
    }
}

Note : better to add static to the two methods to avoid calls to instance methods inside the constructor.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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