Return to Revisions

Here are a few comments:

• you should not start a thread from the constructor - for example, the thread could see mPagesStatus as null and throw a NullPointerException. So you should provide an init method for example, that must be called by the client.
• I'm not a big fan of Hungarian notation (prefixing all instance variables with m) - my IDE already shows me what I need to know with a colour scheme.
• It is good practice to code to interface, for example: ArrayList<PageStatus> list = new ArrayList<PageStatus>(); could be List<PageStatus> list = new ArrayList<PageStatus>();
• mPagesStatus is only assigned once, I would make it final
• same thing for the lock: private final ReentrantLock mLock = new ReentrantLock();
• your PageCacheThread class is a Runnable really - I would make it so
• I would use an ExecutorService to manage the threads instead of manually starting them
• You don't really use the extra features of Condition, so I would use a simple wait/notifyAll mechanism and avoid the complexity of locks (for example, you don't always unlock your lock in a finally block, which can lead to deadlock)
• There seems to be an issue with your signalling code - mPageReadyCondition.signalAll(); is only called if page == mLastPageRequested - if two threads call getPage concurrently, you might fail to wake the conditions that are waiting. I would simply remove the if and signal every time a page is loaded
• For similar reasons, your getNextPageToDownload could miss an update if the getPage is called by two threads concurrently
• I would use a BlockingQueue mechanism for the pages to update, and put the page numbers in the queue from the getPage method and take from the queue in the Runnable.