SOLID WORKS

SOLID principles for object oriented programming are demonstrated in action here. Whilst coding my FPS game, I observed couple of violations (SO) the second time I modified a section of code. This is the classic example of how context helps in catching such violations. Consider the following hierarchical class arrangement

class SUNOVATECHZOMBIEKILL_API ASTPickupInventory : public ASTPickup{
public:
    virtual void GiveTo(Pawn* Target) override;
...
}
class SUNOVATECHZOMBIEKILL_API ASTPickupWeapon : public ASTPickupInventory
{
...
}

and finally implementation of GiveTo() function like so

void ASTPickupInventory::GiveTo(Pawn* Target)
{
	if(Target == nullptr)
	{
		return;
	}

	Super::GiveTo(Target);

	ASTInventory* Existing = Target->FindInventoryType(InventoryType, true);

	if (Existing == nullptr)
	{
		ASTInventory* Inv = nullptr;

		// Spawn the inventory type class object and attach to pawn
		UWorld* const World = GetWorld();
		if (World != nullptr)
		{
			...
		}
		else
		{
			UE_LOG(LogSunovatechZombieKill, Log, TEXT("No world found, can't spawn actor of class %s"), *InventoryType->GetName());
		}

		// Add inventory to target and equip
		if(Target->SwitchToRecentPickup())
		{
			Target->AddInventory(Inv, true);
		}
		else // or not
		{
			Target->AddInventory(Inv, false);
		}

                // This looks out of place
		ASTWeapon* MyWeapon = Cast<ASTWeapon>(Inv);

		// Generate circular doubly linked list for switching weapons by rotation
		if(MyWeapon)
		{
			Target->GenerateWeaponCDL();
		}
	}
	else // ok this pickup already exists in inventory
	{
                // This looks out of place as well
		// if weapon, increase the ammo
		ASTWeapon* MyWeapon = Cast<ASTWeapon>(Existing);

		if(MyWeapon)
		{
			MyWeapon->AddAmmo(10); // to do: move this code to STPickupWeapon maybe
		}
	}
}

The “out of place” code may require data member (or member function) of ASTPickupWeapon class, for instance AddAmmo() may require amount belonging to the class.

The SRP (Single Responsibility Principle) says that class should have only one function, here, involving general inventory only. Specializing to ASTWeapon seems responsibility of ASTPickupWeapon. Open/Close principle says that class should be open for extension and closed for modification which also seems like a violation here. This implies violation when weapon specific modifications are applied. Hence if SRP is violated, so is Open/Close principle.

A simple solution is to write overridable methods like so

class SUNOVATECHZOMBIEKILL_API ASTPickupInventory : public ASTPickup{
protected:
	/**
	 * @brief Function for dealing with non-existant and post inventory spawn procedures
	 */
	virtual void DealWithNonExistentInventory(ASTInventory* NonExistingInventory, Pawn* Target);

	/**
	 * @brief Function for dealing with existing inventory procedure
	 */
	virtual void DealWithExistingInventory(ASTInventory* ExistingInventory, Pawn* Target);
...
}
by doing this, we have opened passage way for child class ASTPickupWeapon to do ASTWeapon specific computing.
void ASTPickupInventory::GiveTo(Pawn* Target)
{
	if(Target == nullptr)
	{
		return;
	}

	Super::GiveTo(Target);

	ASTInventory* Existing = Target->FindInventoryType(InventoryType, true);

	if (Existing == nullptr)
	{
		ASTInventory* Inv = nullptr;

		// Spawn the inventory type class object and attach to pawn
		UWorld* const World = GetWorld();
		if (World != nullptr)
		{
			...
		}
		else
		{
			UE_LOG(LogSunovatechZombieKill, Log, TEXT("No world found, can't spawn actor of class %s"), *InventoryType->GetName());
		}

		// Add inventory to target and equip
		if(Target->SwitchToRecentPickup())
		{
			Target->AddInventory(Inv, true);
		}
		else // or not
		{
			Target->AddInventory(Inv, false);
		}

                DealWithNonExistentInventory(Inv, Target);
	}
	else // ok this pickup already exists in inventory
	{
               DealWithExistingInventory(Existing, nullptr);
	}
}

Participate in discussion