Refactoring on DfsBlockCache and WindowCache classes.

We suggest the following refactoring candidates to increase maintainability found using "automated refactoring candidate identification tools" developed for academic purposes: The class Entry and the class HashEntry are the inner classes. The method clean tends to access the methods and attributes in each inner class (i.e., Feature Envy design problems). Thus, it is better to move the methods to those inner classes where those methods are actually used. Signed-off-by: Ahrim Han <ahrimhan@gmail.com>
ahrimhan · Jul 2, 2017 · 29f6176 · 29f6176
1 parent 5fdbcc1
commit 29f6176
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 24 deletions.
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java
@@ -365,7 +365,8 @@ DfsBlock getOrLoad(DfsPackFile pack, long position, DfsReader ctx,
 			Ref<DfsBlock> ref = new Ref<>(key, position, v.size(), v);
 			ref.hot = true;
 			for (;;) {
-				HashEntry n = new HashEntry(clean(e2), ref);
+				HashEntry n = new HashEntry(e2 == null ? null : e2.clean(),
+						ref);
 				if (table.compareAndSet(slot, e2, n))
 					break;
 				e2 = table.get(slot);
@@ -469,7 +470,8 @@ <T> Ref<T> put(DfsPackKey key, long pos, int size, T v) {
 			ref = new Ref<>(key, pos, size, v);
 			ref.hot = true;
 			for (;;) {
-				HashEntry n = new HashEntry(clean(e2), ref);
+				HashEntry n = new HashEntry(e2 == null ? null : e2.clean(),
+						ref);
 				if (table.compareAndSet(slot, e2, n))
 					break;
 				e2 = table.get(slot);
@@ -522,14 +524,6 @@ private ReentrantLock lockFor(DfsPackKey pack, long position) {
 		return loadLocks[(hash(pack.hash, position) >>> 1) % loadLocks.length];
 	}
 
-	private static HashEntry clean(HashEntry top) {
-		while (top != null && top.ref.next == null)
-			top = top.next;
-		if (top == null)
-			return null;
-		HashEntry n = clean(top.next);
-		return n == top.next ? top : new HashEntry(n, top.ref);
-	}
 
 	private static final class HashEntry {
 		/** Next entry in the hash table's chain list. */
@@ -542,6 +536,16 @@ private static final class HashEntry {
 			next = n;
 			ref = r;
 		}
+
+		public HashEntry clean() {
+			HashEntry top = this;
+			while (top != null && top.ref.next == null)
+				top = top.next;
+			if (top == null || top.next == null)
+				return null;
+			HashEntry n = top.next.clean();
+			return n == top.next ? top : new HashEntry(n, top.ref);
+		}
 	}
 
 	static final class Ref<T> {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java
@@ -369,7 +369,7 @@ private ByteWindow getOrLoad(final PackFile pack, final long position)
 			final Ref ref = createRef(pack, position, v);
 			hit(ref);
 			for (;;) {
-				final Entry n = new Entry(clean(e2), ref);
+				final Entry n = new Entry(e2 == null ? null : e2.clean(), ref);
 				if (table.compareAndSet(slot, e2, n))
 					break;
 				e2 = table.get(slot);
@@ -438,7 +438,7 @@ private void evict() {
 				old.kill();
 				gc();
 				final Entry e1 = table.get(slot);
-				table.compareAndSet(slot, e1, clean(e1));
+				table.compareAndSet(slot, e1, e1 == null ? null : e1.clean());
 			}
 		}
 	}
@@ -488,7 +488,7 @@ private void removeAll(final PackFile pack) {
 					hasDead = true;
 			}
 			if (hasDead)
-				table.compareAndSet(s, e1, clean(e1));
+				table.compareAndSet(s, e1, e1.clean());
 		}
 		gc();
 	}
@@ -520,7 +520,7 @@ private void gc() {
 					}
 				}
 				if (found)
-					table.compareAndSet(s, e1, clean(e1));
+					table.compareAndSet(s, e1, e1.clean());
 			}
 		}
 	}
@@ -533,16 +533,9 @@ private Lock lock(final PackFile pack, final long position) {
 		return locks[(hash(pack.hash, position) >>> 1) % locks.length];
 	}
 
-	private static Entry clean(Entry top) {
-		while (top != null && top.dead) {
-			top.ref.enqueue();
-			top = top.next;
-		}
-		if (top == null)
-			return null;
-		final Entry n = clean(top.next);
-		return n == top.next ? top : new Entry(n, top.ref);
-	}
+	// private static Entry clean(Entry top) {
+	// return top.clean();
+	// }
 
 	private static class Entry {
 		/** Next entry in the hash table's chain list. */
@@ -569,6 +562,18 @@ final void kill() {
 			dead = true;
 			ref.enqueue();
 		}
+
+		public Entry clean() {
+			Entry top = this;
+			while (top != null && top.dead) {
+				top.ref.enqueue();
+				top = top.next;
+			}
+			if( top == null || top.next == null )
+				return null;
+			final Entry n = top.next.clean();
+			return n == top.next ? top : new Entry(n, top.ref);
+		}
 	}
 
 	/** A soft reference wrapped around a cached object. */