This week's book giveaway is in the OO, Patterns, UML and Refactoring forum. We're giving away four copies of Refactoring for Software Design Smells: Managing Technical Debt and have Girish Suryanarayana, Ganesh Samarthyam & Tushar Sharma on-line! See this thread for details.
Its hard to know where to start, most of the points have been commented on elsewhere in this forum.
Quick answer : As the add (B) isn't synchronized and the size (A) is then the synchronized does make no sense.
Minor Point : ArrayList isn't thread safe ( see Collections.synchronizedList ) so calling any of its methods from outside a sync'ed block when another thread can modify, requires a language lawyer to see if it'll fall over or not e.g. step off the end of a list whatever i.e. you need to know how the list is implemented and what size does internally.
Major Point : BUT ignoring that even if calling size while another thread does an add will always succeed it may not tell you the correct size. Synchronizing on the list forces any thread reading the size to not use a cached view of the world i.e. if it did this previously and saw see 3 elements it can just keep saying three regardless of what else goes on with other threads if there was no synchronization i.e. the synchronization acts as a memory barrier.
HOWEVER not having the add (B) synchronized means another thread adding can effectively cache the add, i.e. there is no explicit memory barrier so a second thread doesn't have to publish the results to the world i.e. one thread doing a list size may never see the results of another thread doing an add.
The code looks very broke to me, if its called from more than one thread, note it may never break if you don't run it on a machine with more than one processor again you need to know how your JVM / OS does your threading or you could fix the code :-)
"Eagles may soar but weasels don't get sucked into jet engines" SCJP 1.6, SCWCD 1.4, SCJD 1.5,SCBCD 5
In case you were more just interested in how do I write a thread safe version rather than what was wrong. If you read the Sun ArrayList documentation you'll see an explanation on there and a link to Collections.SynchonizedList if required.
Note that this implementation is not synchronized. If multiple threads access an ArrayList instance concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more elements, or explicitly resizes the backing array; merely setting the value of an element is not a structural modification.) This is typically accomplished by synchronizing on some object that naturally encapsulates the list. If no such object exists, the list should be "wrapped" using the Collections.synchronizedList method. This is best done at creation time, to prevent accidental unsynchronized access to the list:
I’ve looked at a lot of different solutions, and in my humble opinion Aspose is the way to go. Here’s the link: http://aspose.com