How to make this TLV Reader class look nicer and work smoother?

Question

I am quite new to Java and was trying to improve my skills a little by doing a binary file reader. The choosen one was the TLV format. I've chosen that one for being reasonably simple, but not so simple. Also I had some concrete examples with good documentation which I could follow for decoding.

The reader works, but I ended up with a class that seems way too big to be right. I wonder if anyone can break down this code into something more robust.

package tlv;

import java.io.BufferedInputStream;
import java.io.DataInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;

import tlv.errors.MissingI10ParameterException;
import tlv.errors.ParseI10ParameterException;
import tlv.errors.TLVFileException;

/**
 * Represents a TLV file.
 * Expects class File, which represents a TLV file formated as:
 *  [HEADER]
 *  [LogicalBlock-1]
 *      [DataSegmentHeader-1]
 *      [DataSegment-1]
 *      [DataSegmentTrailer-1]
 *      (...)
 *      [DataSegmentHeader-n]
 *      [DataSegment-n]
 *      [DataSegmentTrailer-n]
 *  
 */
public class DiriTlvFile {

    ArrayList<LogicalBlock> dirisegs = new ArrayList<LogicalBlock>();
    int tbc = 114;

    public DiriTlvFile(File tlv) throws TLVFileException {

        DataInputStream dis = null;
        try {

            // Here BufferedInputStream is added for fast reading.
            dis = new DataInputStream(new BufferedInputStream(new FileInputStream(tlv)));

            // Header data
            byte[] ba = new byte[3];

            /* This is the fixes header added
             We just skip it straight away. */
            dis.skipBytes(114);
            LogicalBlock lb = null;

            // Iterate over main file parts
            while (dis.available() > 0) {
                if (dis.read(ba, 0, 3)!= 3)
                    throw new TLVFileException("Error reading DataSegmentID (wrong byte count) @" + tbc);
                tbc += 3;
                String headerId = new String(ba);


                // File header
                if (headerId.equalsIgnoreCase("I01")) {

                    lb = new LogicalBlock();
                    // Skip uninteresting part of header
                    dis.skipBytes(50);
                    tbc += 50;

                // Data Segment body
                } else if (headerId.equalsIgnoreCase("I10")) {

                    if (lb == null)
                        throw new TLVFileException( "Logical Block was not initiated! (Missing I01 structure in file).");

                    byte[] dsheader = new byte[25];
                    if (dis.read(dsheader, 0, 25) != 25) // Head of the DS
                        throw new TLVFileException( "Error reading DataSegmentHeader (I10, wrong byte count) @ " + tbc);
                    tbc += 25;

                    int dssize = (dsheader[24] & 0xFF) - 1; // Size of DS body (+the header itself?)
                    byte[] dsbody = new byte[dssize];   // Body of DS 


                    // Read DataSegmentBody
                    if (dis.read(dsbody, 0, dssize) != dssize)
                        throw new TLVFileException( "Error reading DataSegmentBody (I10, wrong byte count) @ " + tbc);
                    tbc += dssize;

                    // Create the datasegments
                    try {
                        lb.addDataSegment(new I10DataSegment(headerId, dsheader, dsbody));
                    } catch (MissingI10ParameterException e) {
                        // TODO - Improve this message using the mandatory parameter list from the exception
                        throw new TLVFileException("Error parsing DataSegment body (I10) : Missing one of mandatory parameters.");
                    } catch (ParseI10ParameterException e) {
                        throw new TLVFileException("Error parsing DataSegment body (I10) : Could not parse DataSegmentParameters.");
                    }

                } else if (headerId.equalsIgnoreCase("I03")) {
                    // total records read
                    byte[] trailertotrecs = new byte[3];
                    // Have left the loop, means it found the footer
                    byte[] trailerbody = new byte[4];
                    if (dis.read(trailerbody, 0, 4) != 4) // Trailer Body
                        throw new TLVFileException( "Error reading Trailer Body (I03, wrong byte count) @ " + tbc);
                    tbc += 4;
                    System.arraycopy(trailerbody, 1, trailertotrecs, 0, 3);
                    // TODO - compare the number of records read with the information in the footer
                    dirisegs.add(lb);
                } else {
                    throw new TLVFileException("Unknown Segment ID found");
                }
            }
            // dispose all the resources after using them.
            dis.close();
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
          e.printStackTrace();
        }
    }

    public Iterator<LogicalBlock> getLogicalBlocks() {
        return dirisegs.iterator();
    }

    public Iterator<DataSegment> getDataSegments(){
        ArrayList<DataSegment> a = new ArrayList<DataSegment>();
        Iterator<LogicalBlock> i = getLogicalBlocks();
        while (i.hasNext()){
            Iterator<DataSegment> j = i.next().getDataSegments();
            while (j.hasNext()){
                a.add((DataSegment)j.next());
            }
        }
        return a.iterator();
    }
}

P.S.: I've omitted the other classes as they are pretty much only byte-parsing.

Answer 1

• Program against interfaces, e.g. List<LogicalBlock> dirisegs = new ArrayList<LogicalBlock>();
• Use variable names with meaning, e.g. byteCount instead of tbc
• Don't write e.printStackTrace(), do something useful (propagate error, log error...)
• Your DataInputStream won't be close in case of an error. You should move the closing to a finally clause (of course there you need an extra try-catch)
• The constructor is way to long, split it up
• Why don't you give back a List in getDataSegments (you already have one)? Iterator is less useful
• Rename getLogicalBlocks to iterator() and add an extends Iterable<LogicalBlock> to your class, then you can use the class in enhanced for-loops.
• Try to make operations "atomic", e.g. bundle reading from a stream and increasing the counter in a little method:

.

public void read(DataInputStream dis, byte[] buffer, String errorMessage) 
    throws IOException,TLVFileException {
  if (dis.read(buffer, 0, buffer.length) != buffer.length) 
    throw new TLVFileException(errorMessage + tbc);
  tbc += buffer.length;
}

getDataSegments can be shortened to:

public Iterator<DataSegment> getDataSegments(){
    List<DataSegment> result = new ArrayList<DataSegment>();
    for(LogicalBlock lb : dirisegs) {
        Iterator<DataSegment> segments = lb.getDataSegments();
        while (segments .hasNext()){
            result.add(segments .next());
        }
    }
    return result.iterator();
} 

Answer 2

Of course the constructor is too long, but you need to be careful when you factor out some parts into separate private methods, especially with error handling. Right now error handling isn't correct: dis.close(); will never be called in case of an file error etc, leaving the stream open. Getting this right is tricky, so I'd suggest to use a tool like Findbugs.

Use interfaces for your variable types if possible, e.g. List<LogicalBlock> dirisegs = new ArrayList<LogicalBlock>();

Member variables should almost always be private. If you need extenal access, you should prefer getter methods. Even then you need to be careful with mutable objects like Collections or Date: To be on the safe side you need to make defensive copies.

You use a lot of "magical numbers" like 114, 25 etc. If possible, make them "constants" (final static int) with a meaningful name. Same holds for Strings like "I01".

Why do you return an Iterators in getLogicalBlocks and getDataSegments? I don't know the use case good enough, but I think twice before I give back something potentially less useful than I already have. But if you decide that you really need Iterators, did you consider to implement the Iterable interface? If iterating is a common operation for your class, it would be convenient to be able to use it in enhanced for-loops.

Is there a reason that getDataSegments isn't implemented that way?

public Iterator<DataSegment> getDataSegments(){
    List<DataSegment> a = new ArrayList<DataSegment>();
    for(LogicalBlock logicalBlock : dirisegs) {
        for(Iterator<DataSegment> j = logicalBlock.getDataSegments(), j.hasNext();) {
            a.add(j.next());
        }
    }
    return a.iterator();
}

Answer 3

Firstly, i should mention that i don't really understand the file format. You say it's TLV, but you seem to be reading mostly fixed-length chunks.

I'd split the tasks of parsing and holding the data. I'd write a sort of TLVInputStream or TLVParser class, which has a method readChunk, which reads a single TLV triple from an underlying stream and returns it. Usage is like:

TLVInputStream input = new TLVInputStream(new FileInputStream(someFile));
Chunk chunk = input.readChunk();

The constructor would do the header skipping. readChunk would catch an EOFException from the read of the first (and only the first) byte in the tag, and deal with that by returning null, to indicate the end of the file (like how BufferedReader.readLine returns null at end of file). EOFExceptions anywhere else in a chunk should be passed through, as they really are errors.

I wouldn't do decoding of specific chunk types in the readChunk method. Rather than treating chunk lengths as something to be validated, i would use them to control reading. readChunk would read the chunk header, read the length, then read that many bytes of input, and return the chunk. Validating the lengths of the chunks belongs in a higher level than the IO code.

Then I'd have a separate class which holds chunks. Call it ChunkFile or something. That contains the chunks, and do validation on them (eg checking the length) as they are added. If you later wanted to add methods to operate on the chunks, they would go here.

I'd have a third, small, class, which uses a TLVInputStream to read a file and make a ChunkFile object. This is simple enough that it could perhaps be a static factory method on TLVInputStream.

There are a few small details I'd change. If counting the total number of bytes is important, then move the read and count operations into a single method that does both, and always use that. That means you don't have to remember to update the count whenever you read. You do a lot of dis.read(buf, 0, len) == len - you should use readFully for that.

Answer 4

One thing you could consider, is basing your implementation on interfaces.

For example, you could make DiriTlvFile an interface like this:

interface DiriTlvFile {
  public Iterator<LogicalBlock> getLogicalBlocks();
  public Iterator<DataSegment> getDataSegments(); 
}

and since getDataSegments is most likely the same in all implementations an abstract class

abstract class AbstractDiriTlvFile implements DiriTlvFile  {
  public Iterator<DataSegment> getDataSegments() {
    // ...
  }
}

And then use a "loader" interface like this

interface DiriTlvFileLoader {
  public DiriTlvFile loadDiriTlvFile();
}

Which you'd implement using the code you used in the constructor:

class DiriTlvFileLoaderFromFile implements DiriTlvFileLoader {

  protected File tlv;

  public DiriTlvFileLoaderFromFile(File tlv) {
    this.tlv = tlv;
  }

  public DiriTlvFile loadDiriTlvFile() {
    List<LogicalBlock> dirisegs = new ArrayList<LogicalBlock>();

    // ...

    return new AbstractDiriTlvFile() {
      public Iterator<LogicalBlock> getLogicalBlocks() {
        return dirisegs.iterator();
      }
    }
  }
}

This would let you factor out variants, for example, base DiriTlvFileLoaderFromFile on DiriTlvFileLoaderFromInputStream and use the latter to create DiriTlvFileLoaderFromURL for loading from a net resource.

---

Currently your getDataSegments copies all the data into a new list each time it's called. But if you have LogicalBlock implement Iterable, you could instead implement a "flattening iterator", that actually iterates over the items without copying them. For example using:

public class FlattenIterable<T> implements Iterable<T> {
  private Iterable<Iterable<T>> iterable;

  public FlattenIterable(Iterable<Iterable<T>> iterable) {
    this.iterable = iterable;
  }

  public Iterator<T> iterator() {
    return new FlattenIterator<T>(iterable.iterator());
  }

  static class FlattenIterator<T> implements Iterator<T> {
    private Iterator<Iterable<T>> iterables;
    private Iterator<T> currentIterator;

    public FlattenIterator(Iterator<Iterable<T>> iterator) {
      this.iterables = iterator;
      currentIterator = null;
    }

    private void nextIterable() {
      currentIterator = iterables.next().iterator();
    }

    public boolean hasNext() {
      if (currentIterator == null) {
        if (iterables.hasNext()) {
          nextIterable();
        } else {
          return false;
        }
      }

      while (!currentIterator.hasNext() && iterables.hasNext()) {
        nextIterable();
      }

      return currentIterator.hasNext();
    }

    public T next() {
      return currentIterator.next();
    }

    public void remove() {
      throw new UnsupportedOperationException("Remove not supported"); 
    }
  }
}

(Based on http://langexplr.blogspot.com/2007/12/combining-iterators-in-java.html)

you can implement AbstractDiriTlvFile as

abstract class AbstractDiriTlvFile implements DiriTlvFile  {

  protected Iterable<DataSegment> dataSegmentFlattenIterable = 
    new FlattenIterable<DataSegment>(getLogicalBlocks());

  public Iterator<DataSegment> getDataSegments() {
    return dataSegmentFlattenIterable.iterator();
  }
}