Optimize scheduling logic #145
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
khelo/lib/ui/flow/profile/profile_view_model.dart (1)
Line range hint
89-94: Consider handling potential race conditionsThe current implementation might lead to state inconsistencies if multiple toggles are triggered rapidly, as the state update happens after the async operation.
Consider this more robust implementation:
try { if (state.currentUser == null) return; state = state.copyWith(actionError: null); + + final newNotificationState = !state.enableUserNotification; + // Update local state first for immediate UI feedback + state = state.copyWith(enableUserNotification: newNotificationState); await _userService.updateUserNotificationSettings( - state.currentUser?.id ?? "INVALID ID", !state.enableUserNotification); - state = state.copyWith( - enableUserNotification: !state.enableUserNotification, - actionError: null, - ); + state.currentUser!.id, newNotificationState); } catch (error) { + // Revert state on error + state = state.copyWith( + enableUserNotification: !state.enableUserNotification, + actionError: error + ); - state = state.copyWith(actionError: error); debugPrint( "ProfileViewNotifier: error while update user notifications -> $error"); }khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (1)
637-655: Consider adding unit tests forhandleSingleMiniRobinMatchesThe new method
handleSingleMiniRobinMatchesencapsulates specific logic for mini round-robin scheduling. Adding unit tests will help ensure its correctness across different team counts and match scenarios.Would you like assistance in generating unit tests for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
khelo/lib/ui/flow/profile/profile_view_model.dart(1 hunks)khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart(4 hunks)
🔇 Additional comments (5)
khelo/lib/ui/flow/profile/profile_view_model.dart (1)
Line range hint 89-94: Verify consistent state management pattern
Let's verify if similar state management patterns are used consistently across the codebase for other async operations.
✅ Verification successful
Let me gather more information about the state management patterns in the codebase.
State management pattern is consistent with codebase practices
The verification shows that the state management pattern in the notification settings update is consistent with other async operations in the same file. The pattern consistently includes:
- Setting
actionError: nullbefore operations - Using
state.copyWith()for state updates - Proper error handling that sets
actionErrorwith the caught error - Debug logging in catch blocks
The implementation follows the established patterns seen in other async operations like sign out, URL launching, sharing, and other state updates in the same view model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar state management patterns in async operations
ast-grep --pattern 'await $_service.$_($$$);
state = state.copyWith($$$);'
# Search for updateUserNotificationSettings usage
rg "updateUserNotificationSettings" -A 5
Length of output: 1061
Script:
#!/bin/bash
# Search for similar copyWith patterns in async operations with error handling
ast-grep --pattern 'try {
await $_service.$_($$$);
state = state.copyWith($$$);
} catch (error) {
$$$
}'
# Search for other state updates in profile_view_model.dart
rg "state = state.copyWith" khelo/lib/ui/flow/profile/profile_view_model.dart -B 2 -A 2
Length of output: 2183
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (4)
51-51: Updated method call to scheduleKnockOutMatches
The method scheduleMatchesByType now calls scheduleKnockOutMatches() without arguments, reflecting the updated method signature. Ensure that the internal state used by scheduleKnockOutMatches is properly initialized before this call.
65-90: Verify loop termination in scheduleKnockOutMatches
In the while (teamPool.length > 1) loop, please confirm that teamPool is being updated correctly within each iteration, particularly inside the handleSingleKnockoutPhase method. This ensures the loop will terminate and avoids potential infinite loops.
Line range hint 97-180: Review of scheduleMiniRoundRobinMatches logic
The implementation for scheduling mini round-robin matches appears comprehensive. The calculation of matchesPerTeam based on totalTeams is logical. Ensure that the sorting of teamPoints correctly reflects tie-breaker rules and ranking criteria.
187-282: Ensure correct advancement of teams in scheduleBoxLeagueMatches
The scheduleBoxLeagueMatches method introduces logic for box league scheduling. Please verify that the calculation of expectedQualifiers and team selection for advancing to subsequent rounds correctly align with the tournament rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (5)
65-90: Consider extracting group progression logic and handling state mutations carefully.While the refactoring improves readability, there are two concerns:
- Direct mutation of
teamPoolcould lead to side effects. Consider using immutable operations.- The group progression logic (lines 79-90) is repeated in other methods and could be extracted.
+ private MatchGroup getNextGroup(int expectedQualifier) { + if (expectedQualifier > 8) { + return currentGroup; + } else if (expectedQualifier > 4) { + return MatchGroup.quarterfinal; + } else if (expectedQualifier > 2) { + return MatchGroup.semifinal; + } else { + return MatchGroup.finals; + } + }
675-697: Add documentation for the box league implementation.The box league logic is complex and would benefit from documentation explaining the algorithm and its constraints.
+ /// Handles the scheduling of matches for a single box in a box league tournament. + /// + /// @param allMatches All matches in the tournament + /// @param matches Matches in the current box + /// @param teamPool Available teams + /// @param group Current match group + /// @param number Current round number + /// @param boxSize Size of each box (number of teams) void handleSingleBoxLeague(
699-714: Consider making boxSize configurable.The box size logic is complex and the value is hardcoded in
scheduleBoxLeagueMatches. Consider making it configurable through the tournament settings.
Line range hint
387-422: Add null safety checks for team data.The code assumes team data exists without proper null checks. This could lead to runtime errors.
- MatchTeamModel team1Data = match.teams.first; - MatchTeamModel team2Data = match.teams.last; + if (match.teams.length < 2) { + continue; + } + final team1Data = match.teams.first; + final team2Data = match.teams.last;
736-741: Optimize team ID comparisons.The code makes multiple calls to access team IDs. Consider caching the values for better performance.
- element.team_ids.contains(p.first.id) && - element.team_ids.contains(p.last.id) + final elementTeamIds = element.team_ids; + elementTeamIds.contains(p.first.id) && + elementTeamIds.contains(p.last.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart(13 hunks)
🔇 Additional comments (1)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (1)
655-673: LGTM! Good extraction of mini-robin matches logic.
The method effectively encapsulates the mini-robin tournament logic, improving code organization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (4)
657-717: Add documentation for the new helper methods.While the methods are well-structured, they would benefit from documentation explaining their purpose, parameters, and return values. This is particularly important for complex tournament logic.
+ /// Handles the scheduling of matches in a mini round-robin format. + /// + /// Parameters: + /// - matches: Current matches in the tournament + /// - teamPool: Available teams for scheduling + /// - matchesPerTeam: Maximum number of matches per team + /// - group: Current match group + /// - number: Current round number void handleSingleMiniRobinMatches( List<MatchModel> matches, List<TeamModel> teamPool, int matchesPerTeam, MatchGroup group, int number) { + /// Handles the scheduling of matches in a box league format. + /// + /// Parameters: + /// - allMatches: All matches in the tournament + /// - matches: Current matches in the group + /// - teamPool: Available teams for scheduling + /// - group: Current match group + /// - number: Current round number + /// - boxSize: Size of each box in the league void handleSingleBoxLeague( List<MatchModel> allMatches, List<MatchModel> matches, List<TeamModel> teamPool, MatchGroup group, int number, int boxSize, ) {
Line range hint
739-767: Refactor complex match handling logic.The method has high cyclomatic complexity and mixes multiple responsibilities. Consider splitting it into smaller, more focused methods for better maintainability.
- for (final p in pair) { + for (final p in pair) { + _handlePairMatches(p, groupMatches, group, number, winnerTeams); + } + return winnerTeams; +} + +void _handlePairMatches( + List<TeamModel> pair, + List<MatchModel> groupMatches, + MatchGroup group, + int number, + List<String> winnerTeams, +) { final matchesForPair = groupMatches .where((element) => - element.team_ids.contains(p.first.id) && - element.team_ids.contains(p.last.id)) + element.team_ids.contains(pair.first.id) && + element.team_ids.contains(pair.last.id)) .toList(); - if (matchesForPair.isEmpty) { - addMatches(groupMatches, [p], group, number); - addMatches(groupMatches, [p], group, number); - } else if (matchesForPair.length == 1) { - addMatches(groupMatches, [p], group, number); - } else { + + _scheduleMatchesForPair(matchesForPair, pair, groupMatches, group, number); + _determineWinner(matchesForPair, pair, groupMatches, group, number, winnerTeams); +} + +void _scheduleMatchesForPair( + List<MatchModel> matchesForPair, + List<TeamModel> pair, + List<MatchModel> groupMatches, + MatchGroup group, + int number, +) { + switch (matchesForPair.length) { + case 0: + addMatches(groupMatches, [pair], group, number); + addMatches(groupMatches, [pair], group, number); + break; + case 1: + addMatches(groupMatches, [pair], group, number); + break; + } +} + +void _determineWinner( + List<MatchModel> matchesForPair, + List<TeamModel> pair, + List<MatchModel> groupMatches, + MatchGroup group, + int number, + List<String> winnerTeams, +) { + if (matchesForPair.length >= 2) { final finishedMatches = matchesForPair.where((element) => element.matchResult != null); if (finishedMatches.length >= 2) { final winner = getWinnerTeam( matchesForPair.toList(), - p.first.id, - p.last.id, + pair.first.id, + pair.last.id, ); if (winner == null && matchesForPair.length == 2) { - addMatches(groupMatches, [p], group, number); + addMatches(groupMatches, [pair], group, number); } else if (winner != null) { winnerTeams.add(winner); } } } -}
Line range hint
381-423: Optimize team points calculation performance.The team points calculation has nested loops and redundant null checks. Consider optimizing the logic with early returns and guard clauses.
for (final match in matches) { - if (match.matchResult == null) { - continue; - } - MatchResult result = match.matchResult!; + final result = match.matchResult; + if (result == null) continue; MatchTeamModel team1Data = match.teams.first; MatchTeamModel team2Data = match.teams.last; - TeamPoints team1Points = - teamPoints.firstWhere((team) => team.team.id == team1Data.team_id); - TeamPoints team2Points = - teamPoints.firstWhere((team) => team.team.id == team2Data.team_id); + // Create a map for O(1) lookup instead of O(n) firstWhere + final pointsMap = Map.fromEntries( + teamPoints.map((tp) => MapEntry(tp.team.id, tp)) + ); + + final team1Points = pointsMap[team1Data.team_id]!; + final team2Points = pointsMap[team2Data.team_id]!; // Update stats _updateTeamStats(team1Points, team1Data); _updateTeamStats(team2Points, team2Data); // Update points based on result _updatePointsBasedOnResult(result, team1Points, team2Points); }
Line range hint
1-811: Consider implementing the Strategy pattern for tournament types.The current implementation handles different tournament types through switch statements and conditional logic. Consider implementing the Strategy pattern to encapsulate each tournament type's scheduling logic in its own class. This would improve maintainability and make it easier to add new tournament types in the future.
Example structure:
abstract class TournamentScheduler { GroupedMatchMap scheduleMatches(); } class KnockoutTournamentScheduler implements TournamentScheduler { @override GroupedMatchMap scheduleMatches() { // Knockout-specific logic } } class RoundRobinTournamentScheduler implements TournamentScheduler { @override GroupedMatchMap scheduleMatches() { // Round-robin-specific logic } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart(13 hunks)
🔇 Additional comments (1)
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (1)
65-93:
Add safeguard against potential infinite loop.
While the refactored code is cleaner, there's a risk of an infinite loop if teamPool is not properly reduced in each iteration. Consider adding a safety check or maximum iteration limit.
GroupedMatchMap scheduleKnockOutMatches() {
final GroupedMatchMap additionalScheduledMatches =
Map.from(scheduledMatches);
final List<TeamModel> teamPool = List.of(teams);
MatchGroup currentGroup = MatchGroup.round;
int currentRound = 1;
+ int maxIterations = 100; // Reasonable maximum number of iterations
while (teamPool.length > 1) {
+ if (maxIterations-- <= 0) {
+ throw Exception('Maximum iterations reached in scheduleKnockOutMatches');
+ }
final group =
additionalScheduledMatches.putIfAbsent(currentGroup, () => {1: []});
Optimize scheduling logic based on tournament type
fix notification toggle not sync with data
Summary by CodeRabbit
New Features
Bug Fixes
Refactor