Improved code readability

Author: Dmytro Voronkevych

src/androidTest/java/com/badoo/mobile/util/WeakHandlerChainedRefTest.java

@@ -28,12 +28,13 @@ public class WeakHandlerChainedRefTest {
     private Runnable mSecondRunnable;
     private Lock mLock;
     private WeakHandler.ChainedRef mRefHead;
-    private WeakHandler.ChainedRef mFirst;
     private WeakHandler.ChainedRef mSecond;
+    private WeakHandler.ChainedRef mFirst;
     private WeakHandler.WeakRunnable mHeadWeakRunnable;
     private WeakHandler.WeakRunnable mFirstWeakRunnable;
     private WeakHandler.WeakRunnable mSecondWeakRunnable;
 
+    // Creates linked list refHead <-> first <-> second
     @Before
     public void setUp() {
         mLock = new ReentrantLock();
@@ -51,80 +52,83 @@ public String toString() {
         mFirst = new WeakHandler.ChainedRef(mLock, mFirstRunnable) {
             @Override
             public String toString() {
-                return "first";
+                return "second";
             }
         };
         mSecond = new WeakHandler.ChainedRef(mLock, mSecondRunnable) {
             @Override
             public String toString() {
-                return "second";
+                return "first";
             }
         };
 
-        mRefHead.insertAbove(mFirst);
-        mRefHead.insertAbove(mSecond);
+        mRefHead.insertAfter(mSecond);
+        mRefHead.insertAfter(mFirst);
 
         mHeadWeakRunnable = mRefHead.wrapper;
         mFirstWeakRunnable = mFirst.wrapper;
         mSecondWeakRunnable = mSecond.wrapper;
     }
 
     @Test
-    public void insertAbove() {
-        assertSame(mSecond, mRefHead.next);
-        assertSame(mFirst, mRefHead.next.next);
+    public void insertAfter() {
+        assertSame(mFirst, mRefHead.next);
+        assertSame(mSecond, mRefHead.next.next);
         assertNull(mRefHead.next.next.next);
 
         assertNull(mRefHead.prev);
-        assertSame(mSecond, mFirst.prev);
-        assertSame(mRefHead, mSecond.prev);
+        assertSame(mFirst, mSecond.prev);
+        assertSame(mRefHead, mFirst.prev);
     }
 
     @Test
     public void removeFirst() {
         mFirst.remove();
+
         assertNull(mFirst.next);
         assertNull(mFirst.prev);
 
         assertSame(mSecond, mRefHead.next);
-        assertSame(mRefHead, mSecond.prev);
         assertNull(mSecond.next);
+        assertSame(mRefHead, mSecond.prev);
     }
 
     @Test
     public void removeSecond() {
         mSecond.remove();
-
         assertNull(mSecond.next);
         assertNull(mSecond.prev);
 
         assertSame(mFirst, mRefHead.next);
-        assertNull(mFirst.next);
         assertSame(mRefHead, mFirst.prev);
+        assertNull(mFirst.next);
     }
 
     @Test
     public void removeFirstByRunnable() {
         assertSame(mFirstWeakRunnable, mRefHead.remove(mFirstRunnable));
         assertSame(mRefHead.next, mSecond);
         assertSame(mRefHead, mSecond.prev);
-        assertNull(mSecond.next);
+        assertNull(mFirst.next);
+        assertNull(mFirst.prev);
     }
 
     @Test
     public void removeSecondByRunnable() {
         assertSame(mSecondWeakRunnable, mRefHead.remove(mSecondRunnable));
         assertSame(mFirst, mRefHead.next);
         assertSame(mRefHead, mFirst.prev);
+        assertNull(mSecond.next);
+        assertNull(mSecond.prev);
     }
 
     @Test
-    public void removeUnexistentRunnableReturnNull() {
+    public void removeNonExistentRunnableReturnNull() {
         assertNull(mRefHead.remove(new DummyRunnable()));
-        assertSame(mSecond, mRefHead.next);
-        assertNull(mFirst.next);
-        assertSame(mSecond, mFirst.prev);
-        assertSame(mRefHead, mSecond.prev);
+        assertSame(mFirst, mRefHead.next);
+        assertNull(mSecond.next);
+        assertSame(mFirst, mSecond.prev);
+        assertSame(mRefHead, mFirst.prev);
     }
 
     private class DummyRunnable implements Runnable {

src/androidTest/java/com/badoo/mobile/util/WeakHandlerTest.java

@@ -23,6 +23,7 @@
 import android.os.HandlerThread;
 import android.os.SystemClock;
 import android.support.test.runner.AndroidJUnit4;
+import android.test.FlakyTest;
 import android.test.suitebuilder.annotation.MediumTest;
 
 import com.badoo.mobile.util.WeakHandler.ChainedRef;
@@ -32,7 +33,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import java.lang.reflect.Field;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
@@ -70,6 +71,7 @@ public void tearDown() {
         mHandler.getLooper().quit();
     }
 
+    @FlakyTest
     @Test
     public void postDelayed() throws InterruptedException {
         final CountDownLatch latch = new CountDownLatch(1);
@@ -88,7 +90,7 @@ public void run() {
         assertTrue(executed.get());
 
         long elapsedTime = SystemClock.elapsedRealtime() - startTime;
-        assertTrue("Elapsed time should be 300, but was " + elapsedTime, elapsedTime <= 305 && elapsedTime >= 300);
+        assertTrue("Elapsed time should be 300, but was " + elapsedTime, elapsedTime <= 330 && elapsedTime >= 300);
     }
 
     @Test
@@ -146,7 +148,7 @@ public void uncaughtException(Thread thread, Throwable ex) {
                 // Before I fixed impl of WeakHandler it always caused exceptions
             }
             if (mExceptionInThread.get() != null) {
-                throw mExceptionInThread.get(); // Exceptiin from HandlerThread. Sometimes it occured as well
+                throw mExceptionInThread.get(); // Exception from HandlerThread. Sometimes it occured as well
             }
             thread.getLooper().quit();
         }
@@ -155,40 +157,27 @@ public void uncaughtException(Thread thread, Throwable ex) {
     @Test(timeout = 30000)
     public void concurrentAdd() throws NoSuchFieldException, IllegalAccessException, InterruptedException {
         ThreadPoolExecutor executor = new ThreadPoolExecutor(10, 50, 10, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(100));
-        final Set<Runnable> added = new HashSet<>();
+        final Set<Runnable> added = Collections.synchronizedSet(new HashSet());
+        final CountDownLatch latch = new CountDownLatch(999);
         // Adding 1000 Runnables from different threads
-        for (int i = 0; i < 1000; ++i) {
-            final boolean addToSet = i > 0;
-            final SleepyRunnable sleepyRunnable = new SleepyRunnable(i);
+        mHandler.post(new SleepyRunnable(0));
+        for (int i = 0; i < 999; ++i) {
+            final SleepyRunnable sleepyRunnable = new SleepyRunnable(i+1);
             executor.execute(new Runnable() {
                 @Override
                 public void run() {
                     mHandler.post(sleepyRunnable);
-                    if (!addToSet) {
-                        return;
-                    }
-                    synchronized (added) {
-                        added.add(sleepyRunnable);
-                        if (added.size() == 999) {
-                            added.notifyAll(); // #Notify1 - notifying when all runnables been added
-                        }
-                    }
+                    added.add(sleepyRunnable);
+                    latch.countDown();
                 }
             });
         }
 
-
         // Waiting until all runnables added
         // Notified by #Notify1
-        synchronized (added) {
-            while (added.size() < 999)
-                added.wait();
-        }
+        latch.await();
 
-        Field field = mHandler.getClass().getDeclaredField("mRunnables");
-        field.setAccessible(true);
-        ChainedRef ref = (ChainedRef) field.get(mHandler);
-        ref = ref.next;
+        ChainedRef ref = mHandler.mRunnables.next;
         while (ref != null) {
             assertTrue("Must remove runnable from chained list: " + ref.runnable, added.remove(ref.runnable));
             ref = ref.next;

src/main/java/com/badoo/mobile/util/WeakHandler.java

@@ -29,6 +29,7 @@
 import android.os.Message;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
+import android.support.annotation.VisibleForTesting;
 
 import java.lang.ref.WeakReference;
 import java.util.concurrent.locks.Lock;
@@ -55,7 +56,8 @@ public class WeakHandler {
     private final ExecHandler mExec;
     private Lock mLock = new ReentrantLock();
     @SuppressWarnings("ConstantConditions")
-    private final ChainedRef mRunnables = new ChainedRef(mLock, null);
+    @VisibleForTesting
+    final ChainedRef mRunnables = new ChainedRef(mLock, null);
 
     /**
      * Default constructor associates this handler with the {@link Looper} for the
@@ -382,10 +384,8 @@ private WeakRunnable wrapRunnable(@NonNull Runnable r) {
         if (r == null) {
             throw new NullPointerException("Runnable can't be null");
         }
-        // In previous version we tried to reuse ChainedRef references from pool
-        // unfortunately it caused several concurrency issues, so now we don't reuse them
         final ChainedRef hardRef = new ChainedRef(mLock, r);
-        mRunnables.insertAbove(hardRef);
+        mRunnables.insertAfter(hardRef);
         return hardRef.wrapper;
     }
 
@@ -464,8 +464,7 @@ public ChainedRef(@NonNull Lock lock, @NonNull Runnable r) {
             this.wrapper = new WeakRunnable(new WeakReference<>(r), new WeakReference<>(this));
         }
 
-        public void remove() {
-            final Lock lock = this.lock;
+        public WeakRunnable remove() {
             lock.lock();
             try {
                 if (prev != null) {
@@ -479,9 +478,10 @@ public void remove() {
             } finally {
                 lock.unlock();
             }
+            return wrapper;
         }
 
-        public void insertAbove(@NonNull ChainedRef candidate) {
+        public void insertAfter(@NonNull ChainedRef candidate) {
             lock.lock();
             try {
                 if (this.next != null) {
@@ -502,19 +502,15 @@ public WeakRunnable remove(Runnable obj) {
             try {
                 ChainedRef curr = this.next; // Skipping head
                 while (curr != null) {
-                    if (curr.runnable.equals(obj)) {
-                        break;
+                    if (curr.runnable == obj) { // We do comparison exactly how Handler does inside
+                        return curr.remove();
                     }
                     curr = curr.next;
                 }
-                if (curr == null)
-                    return null;
-                WeakRunnable result = curr.wrapper;
-                curr.remove();
-                return result;
             } finally {
                 lock.unlock();
             }
+            return null;
         }
     }
 }