5
\$\begingroup\$

I have a class called FirmwareFile which represents and Intel HEX format file. The file is constructed with the name, build time and build date, at a later point file data is retrieved from the database (another class handles this) and is passed in with the setData(byte[] data) method.

After the data is set, the method isValidData() should be called. This iterates through the data and analyses the hex records using their checksum byte. How is my code looking here? What can I do to improve it?

I suspect lines like the second one here aren't necessary, because I'm already dealing with a byte:

byte checksum = (byte) (sumLSB + 1);
checksum = (byte) (checksum & 0xFF);

Here's the whole class, which works perfectly well, but how can I improve it? For example, should I validate the data as it's passed in instead of with a separate method call?

public class FirmwareFile {

    private String fileName;
    private String buildTime;
    private String buildDate;
    private byte[] data = null;
    private String[] stringData;
    /*
     * This file has been checked and verified as correct using the checksum
     * included with each record. The checksum is computed by summing the
     * decoded byte values, extracting the LSB of the sum, and calculating the
     * two's complement of the LSB.
     */
    private boolean validated = false;

    /*
     * The dataCorrupted variable is used to point towards corrupted records in
     * the FirmwareFile data row. This should correspond to the same record line
     * number in the original firmware.hex file. if the number is 0 and verified
     * == true then the file can be trusted. if dataCorrupted > 0, then there
     * are errors in the file located at the corresponding row number.
     */
    private int dataCorrupted = 0;

    final Charset charSet = Charset.forName(CommunicationState.CHARSET);
    private Logger log = Logger.getLogger(FirmwareFile.class.getName());

    public FirmwareFile(String fileName, String buildTime, String buildDate) {

        this.fileName = fileName;
        this.buildTime = buildTime;
        this.buildDate = buildDate;
    }

    /**
     * @return the fileName
     */
    public String getFileName() {
        return fileName;
    }

    /**
     * @return the buildTime
     */
    public String getBuildTime() {
        return buildTime;
    }

    /**
     * @return the buildDate
     */
    public String getBuildDate() {
        return buildDate;
    }

    /**
     * @return the hasData
     */
    public boolean hasData() {

        if (data == null || data.length < 1) {
            return false;
        } else {
            return true;
        }
    }

    /**
     * Returns a byte array of data as received from the database
     * @return byte[] data
     */
    public byte[] getData() {
        return data;
    }

    public void setData(byte[] data) {

        this.data = data;
        String tempData = new String(data);
        tempData = tempData.replace("\r", "").replace("\n", "");
        this.stringData = tempData.split(IntelHexRecord.Type.RECORD_START_CODE);
        log.debug("data size: " + data.length + " bytes.");
    }

    /**
     * Returns an array of file data as ASCII strings, excluding the preceding
     * ':' and the trailing "\r\n"
     * 
     * @return the stringData
     */
    public String[] getStringData() {
        return stringData;
    }

    /**
     * Returns false if stringData == null or the file contains corrupted data.
     * @return validated
     */
    public boolean isValidData(){
        System.out.println("Validating data, start: " + new Date());
        if(!validated && stringData != null && dataCorrupted == 0){

            for(int i = 1; i < stringData.length; i++){

                String record = stringData[i];

                int sum = 0;
                int recordLength = record.length();

                for(int j = 0; j < recordLength - 3; j += 2){
                    int hexNum = Integer.parseInt(record.substring(j, j + 2), 16);  
                    sum += hexNum;      
                }

                // Two's complement calculation
                byte sumLSB = ByteBuffer.allocate(4).putInt(sum).array()[3];                
                sumLSB = (byte) ~sumLSB;
                byte checksum = (byte) (sumLSB + 1);

                checksum = (byte) (checksum & 0xFF);
                int recordCheck = Integer.parseInt(record.substring(recordLength - 2, recordLength), 16);
                byte recordChecksum = ByteBuffer.allocate(4).putInt(recordCheck).array()[3];
                recordChecksum = (byte) (recordChecksum & 0xFF);

                if(checksum != recordChecksum){
                    dataCorrupted = i;
                    validated = false;
                    return validated;
                }

            }
            validated = true;
            log.debug("Validation complete, end: " + new Date());
            log.debug("This is a valid Firmware File");
            return validated;

        }else{

            return validated;
        }
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  • hasData can simply be return data != null && data.length >= 1; like @200_success wrote.
  • The replace calls to remove newlines can be done in one step, i.e. https://stackoverflow.com/a/2163204/2769043.
  • There are some spaces missing in isValidData.

Now to the meat of the class.

I'd go even so far as to factor out the two steps validateData and isValidData. So in isValidData you'd do the caching of validated and in validateData you'd to the actual checking. That way your indentation is a bit lower.

I think the combination of both validated and dataCorrupted is not so good; I'd say one flag for "file has been validated" and another for "error in file" would be better, so in that sense validated would be the first and dataCorrupted != 0 the second.

So:

public boolean isValidData() {
    if (!validated)
        validated = validateData();

    return validated && dataCorrupted != 0;
}

And then have validateData like:

private boolean validateData() {
    if (string == null)
        return false;

    for (...) {
        ...
        if (checksum != recordChecksum) {
            dataCorrupted = i;
            return false;
        }
    }

    return true;
}

For the actual calculation, allocating additional memory (ByteBuffer.allocate) shouldn't be necessary; I'd have to look up how to do that, maybe someone else will answer that part.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks! Yes, I've been thinking about factoring out the isValidData() method into two separate methods, so I'll get right on that. As for the call to ByteBuffer.allocate(), I've found this answer on stackoverflow: stackoverflow.com/a/1936865/571875 , I may refactor again once I'm happy with the rest of the class to extract the byte manually \$\endgroup\$
    – mal
    Commented Apr 27, 2015 at 6:44
  • 1
    \$\begingroup\$ You are right, a simple cast of the int to a byte gives me the data I need, not need for ByteBuffer at all. \$\endgroup\$
    – mal
    Commented Apr 27, 2015 at 6:59
2
\$\begingroup\$

When you call setData(), you fail to reset the validated flag to false and the dataCorrupted pointer. On that topic, what is the point of dataCorrupted? It is private, but write-only.

I am concerned about new String(data). Perhaps you meant to use charset.

These few lines are problematic:

    System.out.println("Validating data, start: " + new Date());
    if(!validated && stringData != null && dataCorrupted == 0){

        for(int i = 1; i < stringData.length; i++){

Why print to System.out? You have a logger. Stuff like this is probably more appropriate for System.err anyway.

The condition should be inverted, so that you have an early return, to prevent the awkward else way down there.

The for loop is weird, as it starts with 1. If that is intentional, it needs a comment.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for your response, I've updated all print statements to use the logger; the dataCorrupted variable is now printed to the log if a corrupted record is found. I've specified the Charset in the String constructor. The for loop starts at 1 because in the setData() method the call to tempData.split(IntelHexRecord.Type.RECORD_START_CODE) results in an empty String at position 0. This is because the record start code is always the very first character in the file. I've added a comment to clarify that. I'm looking into the awkward else statement now. \$\endgroup\$
    – mal
    Commented Apr 27, 2015 at 6:34

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.