Add player group members pagination

[tudddorrr]

No description provided.

[claude]

Code Review - Add player group members pagination

After reviewing PR #148, I've identified several areas for improvement:

Breaking Changes:

• Method signature change: The Get method now returns PlayerGroupsGetResponse instead of Group, which breaks backwards compatibility
• Sample code impact: The change forces all existing callers to update their code (as shown in GetGroup.cs)
• Consider: Add a new method like GetWithPagination() alongside the existing Get() method to maintain backwards compatibility

API Design Inconsistencies:

• Parameter naming: Uses int page = 0 while other APIs in the codebase use dedicated options classes (see LeaderboardsAPI.cs:GetEntriesOptions)
• Missing parameters: No itemsPerPage parameter to control page size, unlike typical pagination patterns
• Query building: Direct string interpolation instead of proper query parameter handling used elsewhere in the codebase

Potential Issues:

• URL encoding: The page parameter is not URL-encoded, which could cause issues if validation fails on negative values
• Error handling: No validation for negative page numbers, which could result in unexpected API behavior
• Data inconsistency: The original Group.count field may now conflict with MembersPagination.count - unclear which represents what

Suggested Improvements:

1. Maintain backwards compatibility by keeping the original Get method and adding GetWithPagination
2. Follow existing patterns with options class similar to GetEntriesOptions
3. Add input validation for negative page numbers
4. Use proper query parameter building like other APIs in the codebase

The pagination feature itself is valuable, but the implementation could be more consistent with existing patterns and maintain backwards compatibility.

[claude]

🔍 Code Quality & Best Practices

The PR follows Unity and C# conventions well overall. However, there are a few areas for improvement:

• Parameter validation missing: The page parameter has no validation to prevent negative values or excessively large numbers
• Inconsistent pagination pattern: Other APIs in the codebase use options classes (e.g., LeaderboardsAPI.GetEntriesOptions) for complex parameters, while this uses a simple int parameter

---

🐛 Potential Bugs & Issues

• Query parameter encoding: The page parameter is directly concatenated into the URL without proper encoding, though this is low risk for integer values
• Breaking change impact: The method signature change from Task<Group> to Task<PlayerGroupsGetResponse> will break existing consumer code without proper migration guidance

---

⚡ Performance Considerations

• Unnecessary object allocation: Each API call creates a new Uri object with string interpolation - consider caching or using UriBuilder for better performance
• Missing pagination limits: No client-side limits on page size could lead to large response payloads

---

🔒 Security Concerns

• Input validation gap: No bounds checking on the page parameter could allow malicious values to be passed to the backend
• Query injection potential: While minimal risk with integer parameters, consider using parameterized query building

---

🔄 Backwards Compatibility

• Breaking API change: The return type change from Group to PlayerGroupsGetResponse breaks existing consumers
• Migration path unclear: Sample code is updated but no deprecation warnings or migration guide provided for external consumers
• Consider: Add an overload that maintains the old signature while internally calling the new method, then deprecate the old one

Suggested improvements:

1. Add parameter validation: if (page < 0) throw new ArgumentOutOfRangeException(nameof(page));
2. Consider using an options class similar to GetEntriesOptions for consistency
3. Add deprecation attributes to old method signatures before breaking changes
4. Document the pagination behavior and limits in XML comments