[Feedback] Cleaner Approach to Adding Party Members

Unity Courses Talk
,
#1

I feel that rather than adding the members by their names, it’d be cleaner (and less error-prone) to add them by their PartyMemberInfo instances. And to have the PartyMember instantiation handled by a static class. Like so:

// In PartyManager class

[SerializeField] private List<PartyMemberInfo> allMembers;
[SerializeField] private List<PartyMember> currentParty;

public void AddToParty(PartyMemberInfo memberInfo)
{
	if (!allMembers.Contains(memberInfo))
	{
		return;
	}
	
	PartyMember toAdd = PartyMember.From(memberInfo);
	currentParty.Add(toAdd);
}

// In PartyMember Class

public static PartyMember From(PartyMemberInfo info)
{
	PartyMember created = new PartyMember()
	{
		MemberName = memberInfo.MemberName;
        Level = memberInfo.StartingLevel;
        CurrentHealth = memberInfo.BaseHealth;
        MaxHealth = memberInfo.BaseHealth;
        Strength = memberInfo.BaseStr;
        Initiative = memberInfo.BaseInitiative;
        MemberBattleVisualPrefab = memberInfo.MemberBattleVisualPrefab;
        MemberOverworldVisualPrefab = memberInfo.MemberOverworldVisualPrefab;
	};
	
	return created;
}

Note how allMembers is changed from an array to a list. Reason being that the latter type has a Contains method (letting you skip the for loop), while the former doesn’t.

1 Like
#2

Just some notes

While I like this way of doing it, there is no benefit here unless you will also be creating party members from PartyMemberInfo in other places. If not, you’ve just moved the code to another part of the source base. I’m ok with this.

The Contains method of just going to do the for loop for you, so you’ve saved nothing but lines in your own code. A list is just a wrapper around an array, and Contains is going to run a for loop over the list’s internal array to find a matching PartyMemberInfo. In fact, this may be a little slower than finding it by name because the underlying list does a bunch of reflection to ensure a match, and reflection is slow (It’s negligible, but slow nonetheless).

I have looked at the course’s code and your way will save cycles however, because the course’s code doesn’t stop searching for the match after it’s found it. If I have a list (or array) of 100 members and the one I want to add is number 2, the course’s code will continue to check the remaining 97 - which isn’t ideal. It should’ve exited the loop once it found a match.

I think the best way would’ve been to use a list (like you did), but then use Find to find a matching member. It’s still just moving the loop to the list, but this doesn’t need reflection. It’s going to match on member using the predicate and exit the loop (internally).

public void AddMemberToPartyByName(string _memberName)
{
    // Find the party
    PartyMemberInfo matched - allMembers.Find(member => member.memberName == _memberName);
    // If we didn't find a match, return. Kinda bad - we need to inform the caller
    if (matched == null) return;
    
    // We found a match, create the party member
    PartyMember newPartyMember = new PartyMember();

    newPartyMember.memberName = allMembers[i].memberName;
    newPartyMember.level = allMembers[i].startingLevel;
    newPartyMember.currHealth = allMembers[i].baseHealth;
    newPartyMember.maxHealth = allMembers[i].baseHealth;
    newPartyMember.strength = allMembers[i].baseStr;
    newPartyMember.initative = allMembers[i].baseInitiative;

    newPartyMember.memberBattleVisualPrefab = allMembers[i].memberBattleVisualPrefab;
    newPartyMember.memberOverworldVisualPrefab = allMembers[i].memberOverworldVisualPrefab;

    // and add it
    currentParty.Add(newPartyMember);
}

The predicate inside Find here: member => memberName == _memberName is a delegate, and is really just ‘shorthand’ for

bool IsMatch(PartyMemberInfo member)
{
    if (member.memberName == _memberName)
    {
        return true;
    }
    else
    {
        return false;
    }
}
// or
bool IsMatch(PartyMemberInfo member)
{
    return member.memberName == _memberName;
}

The list will take each item and pass it to this shorthand IsMatch and return the first one that is true.
If you want, you could use your PartyMember.From(...) here, once you know it was found

public void AddMemberToPartyByName(string _memberName)
{
    // Find the party
    PartyMemberInfo matched - allMembers.Find(member => member.memberName == _memberName);
    // If we didn't find a match, return. Kinda bad - we need to inform the caller
    if (matched == null) return;
    // We found a match, create the party member and add it
    currentParty.Add(PartyMember.From(matched));
}

If you want to stick to the array, you could do

PartyMemberInfo match = Array.Find(allMembers, member => member.memberName == _memberName);

You’ll notice I’m still using the name here. Here’s the thing; If you’re passing the memberInfo into this function, you don’t need to find it in the list; you already have it. Then you can just

public void AddMemberToParty(PartyMemberInfo memberInfo)
{
    PartyMember newPartyMember = new PartyMember();

    newPartyMember.memberName = memberInfo.memberName;
    newPartyMember.level = memberInfo.startingLevel;
    newPartyMember.currHealth = memberInfo.baseHealth;
    newPartyMember.maxHealth = memberInfo.baseHealth;
    newPartyMember.strength = memberInfo.baseStr;
    newPartyMember.initative = memberInfo.baseInitiative;

    newPartyMember.memberBattleVisualPrefab = memberInfo.memberBattleVisualPrefab;
    newPartyMember.memberOverworldVisualPrefab = memberInfo.memberOverworldVisualPrefab;

    // and add it
    currentParty.Add(newPartyMember);
}

or with your suggestion

public void AddMemberToParty(PartyMemberInfo memberInfo)
{
    currentParty.Add(PartyMember.From(memberInfo));
}

The question is; where did you get the PartyMemberInfo from to pass in here? I didn’t do this course, so maybe you have it, Idk

1 Like
#3

The Contains method of just going to do the for loop for you, so you’ve saved nothing but lines in your own code.

Well, yeah. I was taught that such practices make for cleaner code.

I have looked at the course’s code and your way will save cycles however, because the course’s code doesn’t stop searching for the match after it’s found it. If I have a list (or array) of 100 members and the one I want to add is number 2, the course’s code will continue to check the remaining 97 - which isn’t ideal. It should’ve exited the loop once it found a match.

Thanks xD As for your methods for working with the party members, I think they’re good too. It’s great how you went into detail about them. As for where I got the PartyMemberInfo, it’s part of the PartyManager class (see one of my initial code snippets). I haven’t gone through the rest of this course myself, but I assume that later on, other classes will be working with instances of PartyMemberInfo.

Privacy & Terms