How well does this code comply with the principle of single responsibility?

HI!

void AInteractiveBuildingActorBase::OnInteract(AMainCharacter* PlayerCharacter)
{
    ...

	if (UMainGameInstance* GameInstance = ODRPGUtils::GetGameInstance<UMainGameInstance>(this))
	{
		GameInstance->GetWidgetService()->Execute_DisplayWidget(
			GameInstance->GetWidgetService().GetObject(),
			WidgetForDisplayClass,
			0
		);
	}
	CreateWidgetForDisplay()->AddToViewport();

	...
}

AInteractiveBuildingActorBase is the base class for a building that needs to be built according to a blueprint. For this, we need a Widget (it displays the required resources).
Therefore, I created a separate WidgetService to manage the UI and created a pointer to it in GameInstance.

void UMainGameInstance::Init()
{
	Super::Init();
	
	FFileLogHelper::InitFileLog();
	WRITE_LOG_TO_FILE(LOG_CATEGORY_GameInit, "Game Has Been Initialized");

	WidgetService = NewObject<UWidgetService>(this);
}

TScriptInterface<IWidgetServiceInterface> UMainGameInstance::GetWidgetService() const
{
	return TScriptInterface<IWidgetServiceInterface>(WidgetService);
}

How correct does this look from the point of view of the principle of single responsibility?

Thx

That’s a tricky question because even the vanilla UGameInstance violates the single responsibility principle at some extent. So instead of talking if it’s correct or not, perhaps we can talk about which steps might lead us closer to SRP.

My first suggestion is to consider if you are able or not to move your WidgetService logic to a UGameInstanceSubsystem instead.

It will get automatically instantiated and registered for you, so one less responsibility for your own GameInstance class.

The second point to consider is if you actually need a WidgetService or not, considering the existance of the AHUD class which can be easily overriden on your own GameMode.

Interesting topic tho, feel free to further elaborate your classes usage so we can discuss about how we can make them more SOLID oriented.

1 Like

So, should I move all this code to GameInstanceSubsystem?

#pragma once

#include "CoreMinimal.h"
#include "UObject/Object.h"
#include "WidgetService.generated.h"

class AMainPlayerController;

UINTERFACE()
class UWidgetServiceInterface : public UInterface
{
	GENERATED_BODY()
};

class IWidgetServiceInterface
{
	GENERATED_BODY()

public:
	UFUNCTION(BlueprintCallable, BlueprintNativeEvent, Category="Widget Service")
	UUserWidget* DisplayWidget(TSubclassOf<UUserWidget> WidgetClass, int32 ZOrder = 0);

	UFUNCTION(BlueprintCallable, BlueprintNativeEvent, Category="Widget Service")
	void RemoveWidget(UUserWidget* Widget);
	
	UFUNCTION(BlueprintCallable, BlueprintNativeEvent, Category="Widget Service")
	UUserWidget* FindWidgetByClass(TSubclassOf<UUserWidget> WidgetClass) const;

	UFUNCTION(BlueprintCallable, BlueprintNativeEvent, Category="Widget Service")
	void RemoveAllWidgets();
};

USTRUCT()
struct FWidgetData
{
	GENERATED_BODY()

	UPROPERTY()
	UUserWidget* Widget;

	TSubclassOf<UUserWidget> WidgetClass;

	int32 ZOrder;
};

/**
 * 
 */
UCLASS(BlueprintType, Blueprintable)
class PR_API UWidgetService : public UObject, public IWidgetServiceInterface
{
	GENERATED_BODY()

public:
	/** Begin IWidgetServiceInterface*/
	virtual UUserWidget* DisplayWidget_Implementation(TSubclassOf<UUserWidget> WidgetClass, int32 ZOrder = 0) override;
	virtual void RemoveWidget_Implementation(UUserWidget* Widget) override;
	virtual UUserWidget* FindWidgetByClass_Implementation(TSubclassOf<UUserWidget> WidgetClass) const override;
	virtual void RemoveAllWidgets_Implementation() override;
	/** End IWidgetServiceInterface*/


	UFUNCTION(BlueprintCallable, Category="Widget Service")
	void Init(APlayerController* InPlayerController);

	UFUNCTION(BlueprintCallable, Category = "Widget Service")
	UUserWidget* CreateWidgetWithParams(TSubclassOf<UUserWidget> WidgetClass, int32 ZOrder, bool AddToViewport);

	UFUNCTION(BlueprintCallable, Category = "Widget Service")
	void ShowWidget(TSubclassOf<UUserWidget> WidgetClass);

	UFUNCTION(BlueprintCallable, Category = "Widget Service")
	void HideWidget(TSubclassOf<UUserWidget> WidgetClass);

	UFUNCTION(BlueprintCallable, Category = "Widget Service")
	TArray<UUserWidget*> GetAllWidgets() const;
	
	DECLARE_DYNAMIC_MULTICAST_DELEGATE_OneParam(FOnWidgetCreated, UUserWidget*, Widget);
	DECLARE_DYNAMIC_MULTICAST_DELEGATE_OneParam(FOnWidgetRemoved, UUserWidget*, Widget);

	UPROPERTY(BlueprintAssignable, Category="Widget Service|Events")
	FOnWidgetCreated OnWidgetCreated;

	UPROPERTY(BlueprintAssignable, Category="Widget Service|Events")
	FOnWidgetRemoved OnWidgetRemoved;

protected:
	UPROPERTY()
	AMainPlayerController* PlayerController;

	UPROPERTY()
	TArray<FWidgetData> ActiveWidgets;

private:
	UFUNCTION()
	void OnWidgetDestruct(UUserWidget* Widget);
};

and instead of

void AInteractiveBuildingActorBase::OnInteract(AMainCharacter* PlayerCharacter)
{
    ...

	if (UMainGameInstance* GameInstance = ODRPGUtils::GetGameInstance<UMainGameInstance>(this))
	{
		GameInstance->GetWidgetService()->Execute_DisplayWidget(
			GameInstance->GetWidgetService().GetObject(),
			WidgetForDisplayClass,
			0
		);
	}
	...
}

use

void AInteractiveBuildingActorBase::OnInteract(AMainCharacter* PlayerCharacter)
{
    ...

	ODRPGUtils::GetGameInstanceSubsystem<UWidgetServiceSubsystem>(this)->DisplayWidget(WidgetForDisplayClass, 0)
	...
}

?

For me it feels like an improvement, since that way your WidgetService becomes self-managed and GCed for you.

yes, its hard to disagree
Thank you