From 2e9a7ddaaf8ba29ccc28cdeb98e5d29bb04d8b6d Mon Sep 17 00:00:00 2001
From: atomicint <ryley.kimmel@live.com>
Date: Wed, 3 Feb 2016 19:10:46 -0500
Subject: [PATCH] Fix issue #142

Add unit test for MobRepository#remove(null)

Add explicit null check

Remove empty line, change useless exception message
---
 .../src/main/org/apollo/game/model/World.java | 26 +++++++++----------
 .../org/apollo/game/model/area/Region.java    |  1 +
 .../game/model/entity/MobRepository.java      | 13 ++++------
 .../game/model/entity/MobRepositoryTest.java  |  9 +++++++
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/game/src/main/org/apollo/game/model/World.java b/game/src/main/org/apollo/game/model/World.java
index e34edbb5..d2dbb584 100644
--- a/game/src/main/org/apollo/game/model/World.java
+++ b/game/src/main/org/apollo/game/model/World.java
@@ -309,13 +309,12 @@ public final class World {
 	 * @param npc The npc.
 	 */
 	public void unregister(final Npc npc) {
-		if (npcRepository.remove(npc)) {
-			Region region = regions.fromPosition(npc.getPosition());
+		Preconditions.checkNotNull(npc, "Npc may not be null.");
 
-			region.removeEntity(npc);
-		} else {
-			logger.warning("Could not find npc " + npc + " to unregister!");
-		}
+		Region region = regions.fromPosition(npc.getPosition());
+		region.removeEntity(npc);
+
+		npcRepository.remove(npc);
 	}
 
 	/**
@@ -324,15 +323,14 @@ public final class World {
 	 * @param player The player.
 	 */
 	public void unregister(final Player player) {
-		if (playerRepository.remove(player)) {
-			players.remove(NameUtil.encodeBase37(player.getUsername()));
-			logger.info("Unregistered player: " + player + " [count=" + playerRepository.size() + "]");
+		Preconditions.checkNotNull(player, "Player may not be null.");
+		players.remove(NameUtil.encodeBase37(player.getUsername()));
 
-			Region region = regions.fromPosition(player.getPosition());
-			region.removeEntity(player);
-		} else {
-			logger.warning("Could not find player " + player + " to unregister!");
-		}
+		Region region = regions.fromPosition(player.getPosition());
+		region.removeEntity(player);
+
+		playerRepository.remove(player);
+		logger.info("Unregistered player: " + player + " [count=" + playerRepository.size() + "]");
 	}
 
 	/**
diff --git a/game/src/main/org/apollo/game/model/area/Region.java b/game/src/main/org/apollo/game/model/area/Region.java
index 951c97b5..4334fcd5 100644
--- a/game/src/main/org/apollo/game/model/area/Region.java
+++ b/game/src/main/org/apollo/game/model/area/Region.java
@@ -18,6 +18,7 @@ import org.apollo.game.model.area.update.GroupableEntity;
 import org.apollo.game.model.area.update.UpdateOperation;
 import org.apollo.game.model.entity.Entity;
 import org.apollo.game.model.entity.EntityType;
+import org.apollo.game.model.entity.Mob;
 import org.apollo.game.model.entity.obj.DynamicGameObject;
 
 import com.google.common.base.MoreObjects;
diff --git a/game/src/main/org/apollo/game/model/entity/MobRepository.java b/game/src/main/org/apollo/game/model/entity/MobRepository.java
index 84807d61..3a625b71 100644
--- a/game/src/main/org/apollo/game/model/entity/MobRepository.java
+++ b/game/src/main/org/apollo/game/model/entity/MobRepository.java
@@ -175,30 +175,27 @@ public final class MobRepository<T extends Mob> implements Iterable<T> {
 	 * Removes a Mob from the repository.
 	 *
 	 * @param mob The Mob to remove.
-	 * @return {@code true} if the Mob was removed, {@code false} if not.
 	 */
-	public boolean remove(T mob) {
-		Preconditions.checkNotNull(mob);
-		return remove(mob.getIndex());
+	public void remove(T mob) {
+		Preconditions.checkNotNull(mob, "Mob may not be null.");
+		remove(mob.getIndex());
 	}
 
 	/**
 	 * Removes a Mob from the repository by the specified index.
 	 *
 	 * @param index The index of the Mob to remove.
-	 * @return {@code true} if the Mob at the specified index was removed otherwise {@code false}.
 	 */
-	public boolean remove(int index) {
+	private void remove(int index) {
 		Mob mob = get(index);
 
 		if (mob.getIndex() != index) {
-			return false;
+			throw new IllegalArgumentException("MobRepository index mismatch, cannot remove Mob.");
 		}
 
 		mobs[index - 1] = null;
 		mob.setIndex(-1);
 		size--;
-		return true;
 	}
 
 	/**
diff --git a/game/src/test/org/apollo/game/model/entity/MobRepositoryTest.java b/game/src/test/org/apollo/game/model/entity/MobRepositoryTest.java
index 526c6156..ff4272ec 100644
--- a/game/src/test/org/apollo/game/model/entity/MobRepositoryTest.java
+++ b/game/src/test/org/apollo/game/model/entity/MobRepositoryTest.java
@@ -73,6 +73,15 @@ public final class MobRepositoryTest {
 		assertEquals(0, players.size());
 	}
 
+	/**
+	 * Tests failing of {@link MobRepository#remove(Mob)}
+	 */
+	@Test(expected = NullPointerException.class)
+	public void testRemoveNull() {
+		MobRepository<Player> players = new MobRepository<>(CAPACITY);
+		players.remove(null);
+	}
+
 	/**
 	 * Ensures that a MobRepository maintains a fixed capacity.
 	 */