Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IsGroup to PlayerInfo #100

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ScipioWright
Copy link

I did not test this because I haven't gone through the process of building it before. It looks like the one above it so it probably works fine.

Copy link
Contributor

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On principle a unit test for this would be good, however it's trivial enough that it's probably fine.

Archipelago.MultiClient.Net/Helpers/PlayerHelper.cs Outdated Show resolved Hide resolved
@BadMagic100
Copy link
Contributor

Oh also this could and probably should be a property rather than a method

@ScipioWright
Copy link
Author

ScipioWright commented Oct 26, 2024

Sorry, not familiar enough with the syntax. Would that be something like this?

public bool IsGroup { get; set; } = GroupMembers != null && GroupMembers.Length > 0;

@BadMagic100
Copy link
Contributor

public bool IsGroup => GroupMembers != null && GroupMembers.Length > 0;

Above would be the best way to go, read-only property with an expression body syntax

@BadMagic100
Copy link
Contributor

For educational purposes yours would allow the outside world to set it, and additionally it would be set when the instance is created instead of evaluated on request.

The most verbose correct way would be like this with a number of more concise intermediate options.

public bool IsGroup
{
    get
    {
        return GroupMembers != null && GroupMembers.Length > 0;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants