• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Please review my update method.

 
Olena Golub
Ranch Hand
Posts: 113
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello Everyone,

I beg you to review my update method.
All my I/O operation I make with the Database class. All methods in this clas that works with the RandomAccessFile are synchronized.
For example:


In my Data class I have update method:


where there are also two methods:

Is this update method Okey? Do you see some problem with this implementation?
Thanks a lot for your comments and help!
Regards,
Olena

[ April 24, 2005: Message edited by: Olena Golub ]

[Andrew: Modified code to make horizontal scrolling unnecesary]
[ April 25, 2005: Message edited by: Andrew Monkhouse ]
 
Andrew Monkhouse
author and jackaroo
Marshal Commander
Pie
Posts: 12014
220
C++ Firefox Browser IntelliJ IDE Java Mac Oracle
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Olena,

This is getting dangerously close to exceeding the amount of assignment code we allow anybody to post. Take a look at the JavaRanch SCJD FAQ "What is the policy on posting [...] details of how to do the assignment?". I have decided to allow your code as it does not provide complete code for the update method, but please don't post many more code sections like this.

I reformatted your code so that indentations are 4 spaces instead of tabs (which is what you had). I also broke the lines for the exception constructors and the very last "if" statement. This means that the code now fits on a screen (or at least my screen ) without the need for horizontal scrolling, which makes it easier to read. I don't think I broke your formatting doing this.

Speaking of indentations, the Sun Coding Conventions suggest that indentations should be 4 spaces, and that tabs should be the equivalent of 8 spaces. They also suggest that continuation lines should be indented twice (as I did when I split your final "if" statement.

In your check for isValidRecord(), you only seem to care whether the record is deleted or not. What about if the record number does not exist (I enter a negative number, or a number higher than the number of records in the database)?

Regards, Andrew
 
Olena Golub
Ranch Hand
Posts: 113
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello Andrew,
I appreciate deeply your help! Thanks a lot for this!
And sorry for breaking the forum ruls! I will try not to do this any more, promise

I will format my code, before submission. It's my usual problem if I have an idea in code I write it without concentraiting on the SUN Code Conventions.

The method isValidRecord() will also check if this record exists ( recNo<0 || recNo >= database.getRecordCount()) or deleted.
How do you think is this Okey now?

My main confusion is the synchronization in the update method. As I showed it above I don't synchronize nothing in the update method. Insted of this I call in update() the method where I synchronize array of locked records and the synchronized method from Database that works with RAF. Is is thread safe?

Am I right?

Thanks a lot for your help!
Regards
Olena.
[ April 25, 2005: Message edited by: Olena Golub ]
 
Andrew Monkhouse
author and jackaroo
Marshal Commander
Pie
Posts: 12014
220
C++ Firefox Browser IntelliJ IDE Java Mac Oracle
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Olena
The method isValidRecord() will also check if this record exists ( recNo<0 || recNo >= database.getRecordCount()) or deleted.
How do you think is this Okey now?


Looks good to me.

My main confusion is the synchronization in the update method. As I showed it above I don't synchronize nothing in the update method. Insted of this I call in update() the method where I synchronize array of locked records and the synchronized method from Database that works with RAF. Is is thread safe?


This should work in a thread safe manner, and is indeed correct. You only want the minimum amount of code to be synchronized (so that you are not blocking other clients) and you do not want one thread to own multiple object mutual exclusion locks (one thread inside a synchronized block that was called from a synchronized block - danger of deadlocks).

Regards, Andrew
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic