Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 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.

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.

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.
Bounty Awarded with 50 reputation awarded by Michoel
Source Link
assylias
  • 1k
  • 5
  • 11

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.
lang-java

AltStyle によって変換されたページ (->オリジナル) /