- Home /
Using Delegates for separating UI and Logic, are these in the right place?
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.
}
}
If that's all the LearnSkill class do, don't bother. Put it all the in LearnSkillUI class without the events.
Answer by Hellium · Nov 19, 2020 at 01:03 PM
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;
}
}
Thanks very much! It looks very good and easy to understand. I agree i prefer to have logic and ui separate. It would be confusing and annoying to have some things included in UI scripts and other separated.
I only have one question though. The message box is like the typewritter style, so it will stay awake for a certain amount of time before finishing it's sentence. I originally had TryToLearnSkill(); inside that UI script, because i want TryToLearnSkill(); to run once the dialogue/message has finished (after the yield return).
$$anonymous$$y concern is once that event is called in your fixed version, and the message is playing, it could instantly skip to the next skill it's trying to learn and be overwritting itself in the UI?
I don't think putting in a WaitForSeconds (to estimate how long the dialogue plays for) before the TryToLearnSkill(); in LearnSkill(); / SkipSkill(); / ReplaceSkill(); is the right call?
I think i may end up with odd links and sub/unsub things if i tried to make $$anonymous$$essageBoxClose() an event.
What do you think?
I see, then, I would suggest uncommenting the learnSkill.TryToLearnSkill()
in the various UI methods and remove the call from the controller method.
Having the UI call controller logic is not an issue. The opposite is true in my opinion. Controller should not be aware of the presentation layer (UI). At least, this is how I architect my Unity games. I go this way because all my controller are unit tested and if UI is involved, it's almost impossible to test, unless if you use interfaces and mocks, but I find it way easier to have presentation layer (UI) hooking up to a controller instead of having the controller ask to do stuff. I$$anonymous$$O, a controller could live without UI, the opposite does not make sense.
Great, thanks for that.
So basically i can follow the rule that I can have a reference back to the logic script in the UI, but not from the logic script to the UI directly - that should be events only.
If i deleted the UIScript, I technically wouldn't get any error codes on my LogicScripts. It's just that certain functions wouldn't be called because it relies on the UI button to be pressed or its only called after certain UI plays. but that is ok.
public class LogicScript: $$anonymous$$onoBehaviour
{
public event Action newEvent;
public void Logic1 ()
{
// does logic stuff.
newEvent();
}
public void Logic2 ()
{
// does more logic stuff.
}
public void Logic3 ()
{
// does more logic stuff.
}
}
public class UIScript: $$anonymous$$onoBehaviour
{
void Start ()
{
logicScript.newEvent += UIfunction();
}
void UIFunction ()
{
// does ui stuff.
logicScript.Logic2();
}
void UIButton ()
{
logicScript.Logic3();
}
void OnDestroy ()
{
logicScript.newEvent -= UIfunction();
}
}
A class for the logic should work without the need of the UI class. I think we can both agree on that. However, with the example he provided, to me, there was not enough logic to warrant it's own class. I guess that is what you strongly disagree with. That's understandable. With your example I wouldn't suggest otherwise though.