SCJD Exam with J2SE book : DvdFileAccess Constructor

I've been reading the SCJD Exam with J2SE book by Monkhouse & Camerlengo. I came across something that I don't understand and I was wondering what people thought?

They have DvdFileAccess class that controls access to the actual file where the data is stored, all instances of this class are meant to refer to the same file, and the constructor they have is outlined below.

The "if" statement seems odd - because it will always evaluate to true, so why have an "if" condition there in the first place. The database member variable is an instance variable, so when invoking the constructor the condition will always be true - the database variable will always be null at this point in the constructor.

So the code that creates a new RandomAccessFile will always be invoked when the constructor is called, and the "else if" portion of the code will never be executed. Unless I am misunderstanding something?

Cheers, Sean.

/**
 * This is the worker class that does all the access and manipulation of the
 * physical file that is our database.
 * <br/>
 * Having all the code here, instead of in the DvdDatabase class, means that
 * private methods are only in here - the DvdDatabase class is much cleaner.
 * It is also much easier to swap this file out at a later point and put a
 * commercial database in it's place. Finally, we can limit responsibilities
 * of classes - this class does not care about locking and the locking class
 * does not care about file access.
 * <br/>
 * This class can be thought of as using the Adapter design pattern (where this
 * class is hiding the low level database access information from the end user
 * - they are unaware of whether we are accessing a file directly or whether we
 * are connecting to a commercial database).
 * <br/>
 * Note that since this should only be used by the DvdDatabase class, the class
 * has been set to have default access.
 *
 * @author Denny's DVDs
 * @version 2.0
 */
class DvdFileAccess {

/**
     * The physical file on disk containing our Dvd records.
     */
    private RandomAccessFile database = null;

/**
     * The location where the database is stored.
     */
    private static String dbPath = null;

// the rest of the code is omitted.

/**
     * Default constructor that accepts the database path as a parameter.<br/>
     *
     * All instances of this class share the same data file.
     *
     * @param suppliedDbPath the path to the dvd_db directory
     * @throws FileNotFoundException if the database file cannot be found.
     * @throws IOException if the database file cannot be written to.
     */
    public DvdFileAccess(String suppliedDbPath)
            throws FileNotFoundException, IOException {
        log.entering("DvdFileAccess", "getDvdFileAccess", suppliedDbPath);
        if (database == null) {
            database = new RandomAccessFile(suppliedDbPath, "rw");
            getDvdList(true);
            dbPath = suppliedDbPath;
            log.fine("database opened and file location table populated");
        } else if (dbPath != suppliedDbPath) {
            log.warning("Only one database location can be specified. "
                        + "Current location: " + dbPath + " "
                        + "Ignoring specified path: " + suppliedDbPath);
        }
        log.exiting("DvdFileAccess", "DvdFileAccess");
    }

}

Hmmm...I was just double checking the text in the book that describes this code. I got the source code directly from the website. Strangely enough, the code in the book is different, and seems correct to me. The "if" condition is checking whether the dbPath is null (dbPath is a static variable), as oppossed to checking the whether the database is null (database is an instance variable) like the code associated with the book checks.

It seems to me that the source code associated with the book is incorrect, whereas the the code in the book itself is correct, it's on page 139 if anyone has the book. What do you guys think?

The constructor of the DvdFileAccess class as specified in the book:

The code from the book makes indeed more sense

Andrew is a regular visitor of these forum, so maybe he might visit to comment on this one.

Maybe in the website source there is just a static keyword missing (to make database a class member). I agree that would not be very decent code, but maybe it's just to make you think about every line of code (instead of just blindly copying/pasting sources from the book). So I would say: mission accomplished

Maybe it's just an old version of sources being downloadable from the website (and maybe it's mentioned in the errata page)

Yep, I think the code in the book is correct, and the code downloadable from the site is wrong. Two main reasons, firstly because if the downloadable code were correct, then the "if else" branch of the code would be unreachable, secondly because allowing each instance of the class to reset the location of the database wouuldn't make sense and would be dangerous.

I had checked the errata, but I couldn't find anything in relation to this.

The source and errata are downloadable from here http://apress.com/book/view/1590595165

Roel De Nijs wrote:maybe it's just to make you think about every line of code (instead of just blindly copying/pasting sources from the book). So I would say: mission accomplished

My impression is that this applies to a good bit of the code in that book. Some of it is overwrought. Some of it is too austere. Some of it is incorrect. I have assumed that all of this was intentional so that the book wouldn't simply dish up a complete serving of Finished Assignment™.

That's interesting . I can understand the overwrought and too austere, this is highlighted by the author at the start of the book, so I was taking any solutions with a pinch of salt, realising that I would need to question whether any solution was too much or too little based on my particular requirements.

But I don't think any author could claim that incorrect code\solutions were left in to make the reader think

! Fair enough in an exercise, but in the main text of the book, an incorrect solution merely distracts someone who notices it away from the main train of thought.

This is a very interesting thread about the topic of copying from the Monkhouse book (including some comments from the author himself).

Sean Keane wrote:But I don't think any author could claim that incorrect code\solutions were left in to make the reader think !

Thanks for finding that thread, Roel! I hadn't seen it in a while.

Sean, this is what I meant by 'incorrect'.

Thanks for the feedback guys. I think I understand where you are coming - basically don't copy the book for your assignment

, am I correct?

My intention is to follow the book from beginning to end, understanding all the code, and noting down the main areas that need to be thought about with regards how the application is designed and implemented. I've also been reading this site which has mountains of information, so I'm noting down some thing I read in posts too. Then my intention is to then start off from scratch myself with my own design\implementation.

So all I was really doing here was questioning the code\design as I'm going through the book trying to understand "how does this work?", "is this a good design?", "is this a good implementation?".

On the thread you guys linked to, Andrew says himself that they wouldn't have deliberately added bugs to the code, which is reassuring to hear

, so I think I will mail him on my findings and maybe he can update the codebase for the book.

Cheers for the help.

Just to maybe give an idea of where I was coming from on this. My concern was around by allowing each instance of the DvdFileAccess class to reset the database location you could end up corrupting the file if someone was in the process of storing information to it - this book firstly checks the location of where to write a record, then at a later point synchronizes on the database (RandomAccessFile) to write to the file, so if another instance of the DvdFileAccess class was created with a different location in between these two actions, then the file would be corrupted.

Below is the method that is used to write to the file. But I'm happy I understand this now, and based on the code from the book my concern about the file being corrupted would not occur. Does this sound correct?

/**
     * This method persists a Dvd object to the database. If the Dvd does not
     * currently exist in the database( as determined by it's unique upc code).
     * then the Dvd will be created.
     *
     * @param dvd The Dvd to store
     * @param create true for creation, false for modification
     * @return Indicates success or failure
     * @throws IOException Indicates there is a problem accessing the data file
     */
    private boolean persistDvd(DVD dvd, boolean create) throws IOException {
        log.entering("DvdFileAccess", "persistDvd", dvd);

// Perform as many operations as we can outside of the synchronized
        // block to improve concurrent operations.

Long offset = 0L;
        recordNumbersLock.readLock().lock();      
        try {
            offset = recordNumbers.get(dvd.getUPC());
        } finally {
            recordNumbersLock.readLock().unlock();
        }

if (create == true && offset == null) {
            recordNumbersLock.writeLock().lock();
            try {
                offset = database.length();
                recordNumbers.put(dvd.getUPC(), offset);
            } finally {
                recordNumbersLock.writeLock().unlock();
            }
            log.info("creating record " + dvd.getUPC());
        } else if (create == false && offset != null) {
            log.info("updating existing record " + dvd.getUPC());
        } else {
            return false;
        }

final StringBuilder out = new StringBuilder(emptyRecordString);

/** assists in converting Strings to a byte[] */
        class RecordFieldWriter {
            /** current position in byte[] */
            private int currentPosition = 0;
            /**
             * converts a String of specified length to byte[]
             *
             * @param data the String to be converted into part of the byte[].
             * @param length the maximum size of the String
             */
            void write(String data, int length) {
                out.replace(currentPosition,
                            currentPosition + data.length(),
                            data);
                currentPosition += length;
            }
        }
        RecordFieldWriter writeRecord = new RecordFieldWriter();

writeRecord.write(dvd.getUPC(), DVD.UPC_LENGTH);
        writeRecord.write(dvd.getName(), DVD.NAME_LENGTH);
        writeRecord.write(dvd.getComposer(), DVD.COMPOSER_LENGTH);
        writeRecord.write(dvd.getDirector(), DVD.DIRECTOR_LENGTH);
        writeRecord.write(dvd.getLeadActor(), DVD.LEAD_ACTOR_LENGTH);
        writeRecord.write(dvd.getSupportingActor(),
                          DVD.SUPPORTING_ACTOR_LENGTH);
        writeRecord.write(dvd.getYear(), DVD.YEAR_LENGTH);
        writeRecord.write("" + dvd.getCopy(), DVD.COPIES_LENGTH);

// now that we have everything ready to go, we can go into our
        // synchronized block & perform our operations as quickly as possible
        // ensuring that we block other users for as little time as possible.

synchronized (database) {
            database.seek(offset);
            database.write(out.toString().getBytes());
        }

log.exiting("DvdFileAccess", "persistDvd", true);
        return true;
    }

The book's purpose (I guess) is just to make you familiar with the concepts needed to pass this certification, not to copy all sources and submit your assignment.

As far as I understand the code your assumption is wrong. If a new instance of DvdFileAccess is created, a new RandomAccessFile instance (with different path) will be created, but the old instance will still have its own RandomAccessFile instance, because database is not declared static, so no corrupting of a database file.

Thanks Roel, I knew I was missing something! Yep, you are right, there would be no corruption of the database file regardless of which constructor was used (either the one from the book or the one from the source code).

On the purpose of the book. 100% agree, it should be viewed just as a reference. Which is what I am using it for. This thread was just to get some help\opinions as I was not quite sure if the code was correct or if something was amiss.