Here are a few comments:
- you should not start a thread from the constructor 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 aninit
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 beList<PageStatus> list = new ArrayList<PageStatus>();
mPagesStatus
is only assigned once, I would make itfinal
- same thing for the lock:
private final ReentrantLock mLock = new ReentrantLock();
- your
PageCacheThread
class is aRunnable
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 ifpage == mLastPageRequested
- if two threads callgetPage
concurrently, you might fail to wake the conditions that are waiting. I would simply remove theif
and signal every time a page is loaded - For similar reasons, your
getNextPageToDownload
could miss an update if thegetPage
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 thegetPage
method andtake
from the queue in theRunnable
.
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 aninit
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 beList<PageStatus> list = new ArrayList<PageStatus>();
mPagesStatus
is only assigned once, I would make itfinal
- same thing for the lock:
private final ReentrantLock mLock = new ReentrantLock();
- your
PageCacheThread
class is aRunnable
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 ifpage == mLastPageRequested
- if two threads callgetPage
concurrently, you might fail to wake the conditions that are waiting. I would simply remove theif
and signal every time a page is loaded - For similar reasons, your
getNextPageToDownload
could miss an update if thegetPage
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 thegetPage
method andtake
from the queue in theRunnable
.
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 aninit
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 beList<PageStatus> list = new ArrayList<PageStatus>();
mPagesStatus
is only assigned once, I would make itfinal
- same thing for the lock:
private final ReentrantLock mLock = new ReentrantLock();
- your
PageCacheThread
class is aRunnable
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 ifpage == mLastPageRequested
- if two threads callgetPage
concurrently, you might fail to wake the conditions that are waiting. I would simply remove theif
and signal every time a page is loaded - For similar reasons, your
getNextPageToDownload
could miss an update if thegetPage
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 thegetPage
method andtake
from the queue in theRunnable
.
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 aninit
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 beList<PageStatus> list = new ArrayList<PageStatus>();
mPagesStatus
is only assigned once, I would make itfinal
- same thing for the lock:
private final ReentrantLock mLock = new ReentrantLock();
- your
PageCacheThread
class is aRunnable
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 ifpage == mLastPageRequested
- if two threads callgetPage
concurrently, you might fail to wake the conditions that are waiting. I would simply remove theif
and signal every time a page is loaded - For similar reasons, your
getNextPageToDownload
could miss an update if thegetPage
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 thegetPage
method andtake
from the queue in theRunnable
.
lang-java