SmrAlliance: disallow alliance ID of 0

Previously, we would allow creating an `SmrAlliance` instance when the input `$allianceID` was 0. This was problematic because most of the class attributes would be unset, and most methods would be unusable. Now, we will raise an AllianceNotFoundError if the ID is 0, and we remove the `isNone` method. This will ensure that all instances of the class are valid and can be used without restriction. We replace the existing `isNone` calls with checks if the alliance ID is 0.
smrealms · Sep 9, 2023 · 6e1fe8c · 6e1fe8c
1 parent 4049c50
commit 6e1fe8c
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 57 deletions.
diff --git a/src/lib/Smr/Alliance.php b/src/lib/Smr/Alliance.php
@@ -94,37 +94,35 @@ protected function __construct(
 		protected readonly int $gameID,
 		DatabaseRecord $dbRecord = null,
 	) {
-		if ($allianceID !== 0) {
-			$db = Database::getInstance();
-			$this->SQLID = [
-				'alliance_id' => $db->escapeNumber($allianceID),
-				'game_id' => $db->escapeNumber($gameID),
-			];
+		$db = Database::getInstance();
+		$this->SQLID = [
+			'alliance_id' => $db->escapeNumber($allianceID),
+			'game_id' => $db->escapeNumber($gameID),
+		];
 
-			if ($dbRecord === null) {
-				$dbResult = $db->read('SELECT * FROM alliance WHERE ' . self::SQL, $this->SQLID);
-				if ($dbResult->hasRecord()) {
-					$dbRecord = $dbResult->record();
-				}
-			}
-			if ($dbRecord === null) {
-				throw new AllianceNotFound('Invalid allianceID: ' . $allianceID . ' OR gameID: ' . $gameID);
+		if ($dbRecord === null) {
+			$dbResult = $db->read('SELECT * FROM alliance WHERE ' . self::SQL, $this->SQLID);
+			if ($dbResult->hasRecord()) {
+				$dbRecord = $dbResult->record();
 			}
-
-			$this->allianceName = $dbRecord->getString('alliance_name');
-			$this->password = $dbRecord->getString('alliance_password');
-			$this->recruiting = $dbRecord->getBoolean('recruiting');
-			$this->description = $dbRecord->getNullableString('alliance_description');
-			$this->leaderID = $dbRecord->getInt('leader_id');
-			$this->bank = $dbRecord->getInt('alliance_account');
-			$this->kills = $dbRecord->getInt('alliance_kills');
-			$this->deaths = $dbRecord->getInt('alliance_deaths');
-			$this->motd = $dbRecord->getString('mod');
-			$this->imgSrc = $dbRecord->getString('img_src');
-			$this->discordServer = $dbRecord->getNullableString('discord_server');
-			$this->discordChannel = $dbRecord->getNullableString('discord_channel');
-			$this->flagshipID = $dbRecord->getInt('flagship_id');
 		}
+		if ($dbRecord === null) {
+			throw new AllianceNotFound('Invalid allianceID: ' . $allianceID . ' OR gameID: ' . $gameID);
+		}
+
+		$this->allianceName = $dbRecord->getString('alliance_name');
+		$this->password = $dbRecord->getString('alliance_password');
+		$this->recruiting = $dbRecord->getBoolean('recruiting');
+		$this->description = $dbRecord->getNullableString('alliance_description');
+		$this->leaderID = $dbRecord->getInt('leader_id');
+		$this->bank = $dbRecord->getInt('alliance_account');
+		$this->kills = $dbRecord->getInt('alliance_kills');
+		$this->deaths = $dbRecord->getInt('alliance_deaths');
+		$this->motd = $dbRecord->getString('mod');
+		$this->imgSrc = $dbRecord->getString('img_src');
+		$this->discordServer = $dbRecord->getNullableString('discord_server');
+		$this->discordChannel = $dbRecord->getNullableString('discord_channel');
+		$this->flagshipID = $dbRecord->getInt('flagship_id');
 	}
 
 	/**
@@ -169,13 +167,6 @@ public static function createAlliance(int $gameID, string $name, bool $allowNHA
 		return self::getAlliance($allianceID, $gameID);
 	}
 
-	/**
-	 * Returns true if the alliance ID is associated with allianceless players.
-	 */
-	public function isNone(): bool {
-		return $this->allianceID === 0;
-	}
-
 	/**
 	 * Returns true if the alliance is the Newbie Help Alliance.
 	 */

diff --git a/src/pages/Player/Rankings/AllianceVsAlliance.php b/src/pages/Player/Rankings/AllianceVsAlliance.php
@@ -59,34 +59,39 @@ public function build(AbstractPlayer $player, Template $template): void {
 
 		$alliance_vs = [];
 		foreach ($alliance_vs_ids as $curr_id) {
-			$curr_alliance = Alliance::getAlliance($curr_id, $player->getGameID());
 			$container = new self($curr_id, $this->versusAllianceIDs);
 			$style = '';
-			if (!$curr_alliance->isNone() && $curr_alliance->hasDisbanded()) {
-				$style = 'class="red"';
-			}
 			if ($player->getAllianceID() === $curr_id) {
 				$style = 'class="bold"';
 			}
+			if ($curr_id === 0) {
+				$alliance_name = 'No Alliance';
+			} else {
+				$curr_alliance = Alliance::getAlliance($curr_id, $player->getGameID());
+				if ($curr_alliance->hasDisbanded()) {
+					$style = 'class="red"';
+				}
+				$alliance_name = $curr_alliance->getAllianceDisplayName();
+			}
 			$alliance_vs[] = [
 				'ID' => $curr_id,
 				'DetailsHREF' => $container->href(),
-				'Name' => $curr_alliance->isNone() ? 'No Alliance' : $curr_alliance->getAllianceDisplayName(),
+				'Name' => $alliance_name,
 				'Style' => $style,
 			];
 		}
 		$template->assign('AllianceVs', $alliance_vs);
 
 		$alliance_vs_table = [];
 		foreach ($alliance_vs_ids as $curr_id) {
-			$curr_alliance = Alliance::getAlliance($curr_id, $player->getGameID());
 			foreach ($alliance_vs_ids as $id) {
-				$row_alliance = Alliance::getAlliance($id, $player->getGameID());
-				$showRed = (!$curr_alliance->isNone() && $curr_alliance->hasDisbanded()) ||
-				           (!$row_alliance->isNone() && $row_alliance->hasDisbanded());
+				$showRed = (
+					($curr_id !== 0 && Alliance::getAlliance($curr_id, $player->getGameID())->hasDisbanded()) ||
+					($id !== 0 && Alliance::getAlliance($id, $player->getGameID())->hasDisbanded())
+				);
 				$showBold = $curr_id === $player->getAllianceID() || $id === $player->getAllianceID();
 				$style = '';
-				if ($curr_id === $id && !$row_alliance->isNone()) {
+				if ($curr_id === $id && $id !== 0) {
 					$value = '--';
 					if ($showRed) {
 						$style = 'class="red"';
@@ -120,8 +125,12 @@ public function build(AbstractPlayer $player, Template $template): void {
 		$template->assign('AllianceVsTable', $alliance_vs_table);
 
 		// Show details for a specific alliance
-		$main_alliance = Alliance::getAlliance($this->detailsAllianceID, $player->getGameID());
-		$mainName = $main_alliance->isNone() ? 'No Alliance' : $main_alliance->getAllianceDisplayName();
+		if ($this->detailsAllianceID === 0) {
+			$mainName = 'No alliance';
+		} else {
+			$main_alliance = Alliance::getAlliance($this->detailsAllianceID, $player->getGameID());
+			$mainName = $main_alliance->getAllianceDisplayName();
+		}
 		$template->assign('DetailsName', $mainName);
 
 		$kills = [];

diff --git a/test/SmrTest/lib/AllianceIntegrationTest.php b/test/SmrTest/lib/AllianceIntegrationTest.php
@@ -102,14 +102,4 @@ public function test_isNHA(): void {
 		self::assertTrue($alliance->isNHA());
 	}
 
-	public function test_isNone(): void {
-		// Create an alliance that is not "none"
-		$alliance = Alliance::createAlliance(1, 'Some alliance');
-		self::assertFalse($alliance->isNone());
-
-		// Create an alliance that is "none"
-		$alliance = Alliance::getAlliance(0, 1);
-		self::assertTrue($alliance->isNone());
-	}
-
 }