Using Delegates for separating UI and Logic, are these in the right place?

Questions & Answers
1

Hi,

I am trying to learn about using delegates / events to separate UI code.
I have these two scripts, where one is the logic that is run and the other is UI, but the UI needs to feed back to the logic to ‘re-run’ / ‘re-check’ things before it can completely finish.
Could you tell me if this is the correct place / way to use the functions?

I am thinking I should probably move two functions (DontReplace and DoReplace) into LearnSkillUI. I kept them in the LearnSkill class because i thought the button functions should fire from here, but if it only fires for another UI interaction, then it i should probably live in LearnSkillUI. It’s probably unnecessary events.

I am wondering if it is correct that in the LearnSkillUI, i reference back to LearnSkill to fire off that logic again? Although i’m thinking there isn’t much other ways to do it.

public class LearnSkill : MonoBehaviour
{
	public Stats character;
    public List<Skill> skillsToLearn = new List<Skill>();
	
	public delegate void OnLearnSkillEvent ();      
    public event OnLearnSkillEvent OnLearnSkill;
	
	public delegate void OnSkillReplacedEvent ();      
    public event OnSkillReplacedEvent OnSkillReplaced;
	
	public delegate void OnReqSkillReplacedEvent ();      
    public event OnReqSkillReplacedEvent OnReqSkillReplaced;
	
	public delegate void OnEndLearnSkillEvent ();      
    public event OnEndLearnSkillEvent OnEndLearnSkill;
	
	public delegate void OnDontReplaceEvent ();      
    public event OnDontReplaceEvent OnDontReplace;
	
	public delegate void OnDoReplaceEvent ();      
    public event OnDoReplaceEvent OnDoReplace;
	
	
	void WantsToLearnSkill ()
	{
		if (skillsToLearn > 0)
		{
			if (character.HasFreeSkillSlot())
			{
				LearnedSkill();
			}
			else
			{
				OnReqSkillReplaced();		// plays UI message to ask if want to replace a skill? Yes/No buttons appear, where the functions of are below.
			}
		}
		else
		{
			OnEndLearnSkill();				// closes the learnSkill UI & notifies player for other functions.
		}
	}
	
	void DontReplaceButton ()				// does this belong in the UI script entirely as it only plays a message?
	{
		OnDontReplace();					// plays UI messages from UI script.
	}
	
	void DoReplaceButton ()					// does this belong in the UI script entirely as it only opens the screen?
	{
		OnDoReplace();						// Opens Skill Replace UI Screen.
	}
	
	void ReplacedSkill ()					// Can this be logic I attach to a button, or should another function in UI script reference this function?
	{
		// replace skill in stats.
		skillsToLearn.RemoveAt(0);
		
		OnSkillReplaced();					// plays UI messages from UI script.
	}
	
	void LearnedSkill ()					// Can this be logic I attach to a button, or should another function in UI script reference this function?
	{
		// add skill to stats.
		skillsToLearn.RemoveAt(0);
		
		OnLearnSkill();						// plays UI messages from UI script.
	}
}

The UI Script:

public class LearnSkillUI : MonoBehaviour
{
    // UI components.
	
    public LearnSkill learnSkill;

    void Start ()
    {
        learnSkill.OnSkillReplaced += SkillReplacedMessage;
        learnSkill.OnLearnSkill += SkillLearnedMessage;
		learnSkill.OnReqSkillReplaced += WantToLearnMessage;		
		learnSkill.OnDontReplace += DontReplaceMessage;
		learnSkill.OnDoReplace += OpenLearnSkillScreen;	
    }

    public IEnumerator WantToLearnMessage ()
    {
		yield return MessageBox("Do you want to replace a skill?");
		// turn on Yes / no Buttons		- the functionality of the yes no is in the learnSkill class, should it be here?
    }

    public void OpenLearnSkillScreen ()
    {
		// turn on skill screen & set it up.
    }

    public void CloseLearnSkillScreen ()
    {
        // close the skill screen.
    }

    public IEnumerator SkillReplacedMessage ()
    {
        yield return MessageBox("Skill has been replaced");
		learnSkill.WantsToLearnSkill();							// needs to loop back to make sure that there are no other skills waiting to be learned.
    }

    public IEnumerator SkillLearnedMessage ()
    {
        yield return MessageBox("Skill has been learned");
		learnSkill.WantsToLearnSkill();							// needs to loop back to make sure that there are no other skills waiting to be learned.
    }

    public IEnumerator DontReplaceMessage ()
    {
        yield return MessageBox("Skill was not added");
		learnSkill.WantsToLearnSkill();						// needs to loop back to make sure that there are no other skills waiting to be learned.
    }    
}
2

I strongly disagreee with @myzzie.

Separating logic from UI is clearly good practice, even (I should say especially) if it means the classes are small. SINGLE RESPONSIBILITY PRINCIPLE is one of the essentials pillars for good software architecture.

Here is how I would have done it. The code should be clear enough. If not, don’t hesitate asking questions.

public class LearnSkill : MonoBehaviour
{
    public Stats character;
    public List<Skill> skillsToLearn = new List<Skill>();

    public event Action SkillsAvailable;
    public event Action SkillLearned;
    public event Action SkillReplaced;
    public event Action SkillSkipped;
    public event Action SkillLearningFailedBecauseSkillListFull;
    public event Action AllSkillsLearned;
    

    private void Start()
    {
        if (skillsToLearn > 0)
        {
            SkillsAvailable();
        }
    }

    public void TryToLearnSkill ()
    {
        if (skillsToLearn > 0)
        {
            if (character.HasFreeSkillSlot())
            {
                LearnSkill();
            }
            else
            {
                SkillLearningFailedBecauseSkillListFull();
            }
        }
        else
        {
            AllSkillsLearned();
        }
    }
    
    public void ReplaceSkill()
    {
        // replace skill in stats.
        Skill skillToLearn = skillsToLearn[0];
        skillsToLearn.RemoveAt(0);

        // Do something with skillToLearn
        // Something like skillLearned[0] = skillToLearn;

        SkillReplaced();

        TryToLearnSkill(); // Keep as much logic in the controller
    }
    
    private void LearnSkill()
    {
        // add skill to stats.
        Skill skillToLearn = skillsToLearn[0];
        skillsToLearn.RemoveAt(0);

        // Do something with skillToLearn
        // Something like skillLearned.Add(skillToLearn);
        
        SkillLearned();
        
        TryToLearnSkill(); // Keep as much logic in the controller
    }
    
    private void SkipSkill()
    {
        skillsToLearn.RemoveAt(0);
        
        SkillSkipped();
        
        TryToLearnSkill(); // Keep as much logic in the controller
    }
}

public class LearnSkillUI : MonoBehaviour
{
    // UI components.
    
    public LearnSkill learnSkill;

    void Start ()
    {
        learnSkill.SkillsAvailable += SkillsAvailable;
        learnSkill.SkillLearned += ShowSkillLearned;
        learnSkill.SkillReplaced += ShowSkillReplaced;
        learnSkill.SkillSkipped += SkillSkipped;
        learnSkill.SkillLearningFailedBecauseSkillListFull += AskIfReplace;
        learnSkill.AllSkillsLearned += ShowAllSkillLeerned;
    }

    public IEnumerator AskIfReplace()
    {
        // I don't know how your MessageBox work
        // But having a way to specify the function to call when pressing YES and NO would be great
        // so that you could do:
        yield return MessageBox("Do you want to replace a skill?", learnSkill.ReplaceSkill, learnSkill.SkipSkill);
    }

    public void OpenLearnSkillScreen ()
    {
        // turn on skill screen & set it up.
    }

    public void CloseLearnSkillScreen ()
    {
        // close the skill screen.
    }

    public IEnumerator ShowSkillLearned ()
    {
        yield return MessageBox("Skill has been learned");
        // learnSkill.TryToLearnSkill(); // Moved to controller class
    }

    public IEnumerator ShowSkillReplaced ()
    {
        yield return MessageBox("Skill has been replaced");
        // learnSkill.TryToLearnSkill(); // Moved to controller class
    }

    public IEnumerator ShowSkillSkipped ()
    {
        yield return MessageBox("Skill was not added");
        // learnSkill.TryToLearnSkill(); // Moved to controller class
    }

    public IEnumerator ShowAllSkillLearned ()
    {
        yield return MessageBox("You've learned all the skills!");
        CloseLearnSkillScreen();
    }
    
    // You NEED to unsubscribe from the events!
    void OnDestroy ()
    {
        learnSkill.SkillsAvailable -= SkillsAvailable;
        learnSkill.SkillLearned -= ShowSkillLearned;
        learnSkill.SkillReplaced -= ShowSkillReplaced;
        learnSkill.SkillSkipped -= SkillSkipped;
        learnSkill.SkillLearningFailedBecauseSkillListFull -= AskIfReplace;
        learnSkill.AllSkillsLearned -= ShowAllSkillLearned;
    }
}