• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

UrlyBird: My idea of readRecord

 
Sophie Jane
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi All,

I would like your opinions on my initial thoughts on implementing the readRecord function of DB.java.

On creating the database, I create a DBSchema object that reads all the method data and creates a Hashmap of ( col -> size ) pairs. This is accessible through a public getter, I use in the readRec method.

Q1:
(1) The Valid flag is not part of the record definition. This basically means I have to add 2 bytes onto the record size to get the correct length?? Is it OK to hard this value? I am trying not to hardcode these values as if DB schema changes...the program will no longer work.

(2) I have decided against using the RandomAccessFile functions to read/write the file? Instead I read into a buffer that I manipulate later..I think the read operation should use the readFully operation, so as to decrease the chance of another Thread reading at the same time?



I'd like to know am I making a pigs ear of this? I kinda feel the above code is clunky and not elegant...Opinions are welcome?
 
Daniel Bryant
Ranch Hand
Posts: 54
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Sarah,

Welcome to the ranch! Just a few quick thoughts on your questions

1. I agree that the valid flag is not part of the record definition - but my spec says that it's only a 1 byte flag (not 2). I think it ok to hard code this value - it's not really connected with the column name/length schema data (which if you code well can accomodate changes). You could also argue that if you you received a request to change the position/size of the deleted flag what's to stop user's requesting the whole file format being re-designed (something best avoided )

2. I'm personally a big fan of RandomAccess file - it allows both reading and writing on files and can also serve as an ideal object to synchronise on. You definitely want to use readFully() regardless of whether you use a RAF of DAI - look closly at the API regarding the guarantees made for the read() and readFully() methods and it should be obvious why.

Just a couple of general comments - in my opinion your method structure is ok, but it could do with some tidying, and as you suggested being made a little more elegant . You are definitely going to need some form of synchronisation in here (to stop two threads reading/writing interfering with each other - see this post) for more info) and you really don't want to eat the exception like you are currently doing - that might cost big points!

I hope this helps,

Daniel
[ July 25, 2006: Message edited by: Daniel Bryant ]
 
Sophie Jane
Greenhorn
Posts: 7
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Daniel,

Thank you for your review and comments!

(1) I agree with your thinking for the Valid Flag, my spec says ( 2 byte flag. 00 implies valid record, 0x8000 implies deleted record ). However, I will hardcode this value for the meantime

(2) On the RandomAccessFile, I agree again, I am a fan also, however, Could the length of time positioning the filepointer while reading/writing have an impact on concurrency/performance. Surely, it is better to get the data quickley out into a cache and then play with it from there. Agree/disagree?

Thanks again for your review!

Any other comments from anyone else please?
 
Edwin Dalorzo
Ranch Hand
Posts: 961
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to Java Ranch, Sophie.

I just finished implementing my own readRecord() method this past Saturday.

These would be my comments:

1. Calculating the record position is something you will do also in the update and delete methods. Therefore you might like to refactor that.

2. I like your approach of reading the whole record into a ByteArrayInputStream, notwithstanding, at the end you have to parse it anyway. You could have done the parsing directly from the file, probably saving a few nanoseconds. The only real advantage I see in this approach is the fact that you read the whole bunch of bytes in a sole operation, reducing the possibilities of a dirty read (if another thread modifies the record while you are reading it).

3. Finally, I guess your database variable is a RandomAccessFile. Have you wondered what would happen if two threads try to read a record concurrently. Let's say thread one just seek the record 1, thread two comes immediately after it, before thread 1 performs the readFully operation, thread 2 seeks a new position. Then thread1 read the position sought by thread 2 and the read is corrupted.

4. In the URLyBird specification that I download, a couple of paragraphs after the DBAccess interface definition says: "Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.". Since the readRecord throws RecordNotFoundException you may seriously consider evaluating the delete flag.

5. I do not recommend to catch the IOException and do nothing. In this case you either throw a RecordNotFoundException or at least, log the exception for future review and evaluation.

6. Consider adding some logging to your database class. Something that might really help you in future stages of your programming to detect bugs and see what you code is doing. I added logging to determine when a read a search or an update happens, when a file is opened, when validations result successful. There I can see the executing thread, the class, the method and some message with the details of what the application is doing. That could indeed help you in future phases of your project.

7. You may consider throwing IllegalArgumentException when the record number is not between 0 and Long.MAX_VALUE.

8. You may consider accepting the default encoding as parameter when you create your database object, probably read from the suncertify.properties file.

9. If you are using JDK 1.5 you may consider using the enhanced for instead of using iterators. That adds code clarity.

10. Finally, when you have to skip some bytes in an Stream, as you do in the first iteration of your read loop, instead of wasting an iteration, use the skip(int bytes) method.

I hope this helps!
[ July 25, 2006: Message edited by: Edwin Dalorzo ]
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic