Skip to content

Conversation

@edge-marge
Copy link

No description provided.

@orcist
Copy link
Collaborator

orcist commented Sep 12, 2025

general notes:

  • DTO stands for "data transfer object", if the class is not used to format requests or parse responses from outside APIs then it should not be labeled as DTO (in this case parsing environment variables), the class is only used to provide strongly typed access to local data

additional naming and file organization notes:

  • your EnvCustomTicketsRequestDTO is exactly the same as the existing AdvancedTicketsRequestDTO example, you can probably delete EnvCustomTicketsRequestDTO and use the Advanced one,
  • class EnvEqualityDTO should be probably named AdvancedTicketsEqualityVariables since they are modeled specifically for the Advanced Example config,
  • the same as the point above for EnvIntersectionDTO -> AdvancedTicketsIntersectionVariables,
  • both the variables classes should be moved inside the EnvHelper.cs script as they are tightly coupled together (see the same pattern for TicketsRequestDTO and SimpleTicketsRequestDTO, examples are defined alongside the generic class,
  • script EnvHelper.cs has very vague name, I'd suggest to rename to MatchmakingInjectedVariableStore.cs (same goes for the class in the script),
  • this class MatchmakingInjectedVariableStore should be modified to be a template class and not directly reference any other classes which are tightly coupled to specific configurations (i.e. advanced example configuration in this case),
  • please write a new class named MatchmakingServerDataStoreExample.cs in folder Samples~/AdvancedExample/ which follows the namespace and code templating example of Gen2ClientHandlerExample.cs and initializes the MatchmakingInjectedVariableStore instance as part of Awake method.
    • this new sample folder needs to be added to package.json file (follow simple example which is already in there), so it can be imported from Unity Package Manager UI (test this please),
    • use type aliases to assign the AdvancedTicketsXYZVariables classes mentioned above to the alias classes when initializing the template class:
using MyTicketsEqualityVariables = Edgegap.Gen2SDK.AdvancedTicketsEqualityVariables;
// same for intersection

// then initialize template class like this
new MatchmakingInjectedVariableStore<MyTicketsEqualityVariables, MyTicketsIntersectionVariables >();
  • add parsing for teams and groups using variables MM_TEAMS and MM_GROUPS ((see docs)[https://docs.edgegap.com/learn/matchmaking/matchmaker-in-depth#injected-environment-variables}) - expose a new attribute in MatchmakingInjectedVariableStore.Teams which is a list of ticket IDs
    • the developer is expected to retrieve ticket details using the ticket ID from TicketsData.

Copy link
Collaborator

@orcist orcist left a comment

Choose a reason for hiding this comment

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

see comments, please make changes and resubmit. thank you for the PR! 🙌

envEntry.Value.ToString()
);
}
else if (key.Contains("_TICKET_"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this condition pass for both MM_TICKET_IDS and MM_TICKET_cusfn10msflc73beiik0? I realize that you're relying on the first if to capture the list, but this is prone to issues if a developer comes later and reorders your if statements. If the branches of your if statements are meant to be exclusive it's best to write the conditions more explicit and specific.

Assuming you only want to pass for the first case (list of IDs) you might as well:

Suggested change
else if (key.Contains("_TICKET_"))
else if (key == "MM_TICKET_IDS")

envEntry.Value.ToString()
);
}
else if (key.Contains("_MATCH_ID"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same applies as above, if there's ever an injected variable MM_FOOBAR_MATCH_ID this condition will attempt to parse it and possibly fail. A more explicit and better condition would be:

Suggested change
else if (key.Contains("_MATCH_ID"))
else if (key == "MM_MATCH_ID")

{
MatchId = envEntry.Value.ToString();
}
else if (key.Contains("_EQUALITY"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above.

envEntry.Value.ToString()
);
}
else if (key.Contains("_INTERSECTION"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

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