Refactor tournament detail stats filter #130
Conversation
WalkthroughThe changes in this pull request introduce a new extension Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart
Outdated
Show resolved
Hide resolved
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart
Show resolved
Hide resolved
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart
Outdated
Show resolved
Hide resolved
![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 (9)
data/lib/service/tournament/tournament_service.dart (1)
Line range hint
134-147: Consider optimizing thegetKeyStatsmethod for better performance and maintainability.The method currently handles multiple responsibilities and uses nested loops which could impact performance with large datasets. Consider these improvements:
- Split the method into smaller, focused functions
- Use more efficient data structures to reduce loop complexity
- Add specific error handling for score calculations
Here's a suggested refactoring:
Future<List<PlayerKeyStat>> getKeyStats(List<MatchModel> matches) async { + try { + final scores = await _fetchBallScores(matches); + final playerStats = _calculatePlayerStats(matches, scores); + final keyStats = playerStats.getTopKeyStats(); + return _filterActivePlayers(keyStats); + } catch (error, stack) { + throw AppError.fromError(error, stack); + } } +Future<List<BallScore>> _fetchBallScores(List<MatchModel> matches) async { + final matchIds = matches.map((match) => match.id).toList(); + return _ballScoreService.getBallScoresByMatchIds(matchIds); +} +List<PlayerKeyStat> _calculatePlayerStats( + List<MatchModel> matches, + List<BallScore> scores, +) { + return matches.expand((match) { + return match.teams.expand((team) { + return team.squad.map((player) { + final stats = scores.calculateUserStats(player.id); + return PlayerKeyStat( + player: player.player, + teamName: team.team.name, + stats: stats, + ); + }); + }); + }).toList(); +} +List<PlayerKeyStat> _filterActivePlayers(List<PlayerKeyStat> stats) { + return stats.where((element) => element.player.isActive).toList(); +}khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart (1)
225-243: Enhance accessibility for filter chipsThe filter chips should include semantic labels for better accessibility.
Wrap the Chip widget with a Semantics widget:
- child: Chip( + child: Semantics( + selected: isSelected, + value: 'Filter by ${element.getString(context)}', + child: Chip(data/lib/api/match/match_model.dart (1)
428-439: Add documentation and optimize run rate calculation.Helper methods lack documentation explaining their purpose and assumptions. Also, run rate calculation requires an additional iteration over the list.
Add documentation and optimize the code:
+ /// Returns the team model for the specified team ID from a match + /// Throws [StateError] if team is not found MatchTeamModel _getTeam(MatchModel match, String teamId) => match.teams.firstWhere((team) => team.team.id == teamId); + /// Returns the opponent team model for the specified team ID from a match + /// Throws [StateError] if team is not found MatchTeamModel _getOpponentTeam(MatchModel match, String teamId) => match.teams.firstWhere((team) => team.team.id != teamId); double _runRate(String teamId, int runs) { - final totalOvers = map((match) => _getTeam(match, teamId)) - .fold<double>(0.0, (total, team) => total + team.over); - - return totalOvers > 0 ? runs / totalOvers : 0; + // Calculate total overs during the main iteration in teamStat method + double totalOvers = 0.0; + for (final match in this) { + totalOvers += _getTeam(match, teamId).over; + } + return totalOvers > 0 ? runs / totalOvers : 0; }khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (2)
153-154: Consider simplifying the sort comparison.The sort implementation can be made more concise while maintaining null safety.
- final keyStatsList = List.of(keyStats) - ..sort((a, b) => b.value?.compareTo(a.value ?? 0) ?? 0); + final keyStatsList = List.of(keyStats) + ..sort((a, b) => (b.value ?? 0).compareTo(a.value ?? 0));
171-171: Consider caching the truncated list length.To avoid recomputing
take(4).lengthon every build, consider storing it in a variable.final keyStatsList = List.of(keyStats) ..sort((a, b) => (b.value ?? 0).compareTo(a.value ?? 0)); + final displayCount = keyStatsList.take(4).length; return Padding( // ... GridView.builder( shrinkWrap: true, physics: NeverScrollableScrollPhysics(), - itemCount: keyStatsList.take(4).length, + itemCount: displayCount,Also applies to: 180-180
khelo/assets/locales/app_en.arb (2)
243-249: LGTM! Consider adding pluralization support.The new filter keys follow the existing naming convention and integrate well with the refactored tournament detail stats filter. However, consider adding pluralization support for these statistics, similar to how it's implemented for other count-based strings in the file.
Example implementation:
- "key_stat_filter_runs": "Runs", + "key_stat_filter_runs": "{count, plural, =0{No runs} =1{Run} other{Runs}}", + "@key_stat_filter_runs": { + "placeholders": { + "count": {} + } + },
243-249: Add descriptions for translation context.To help translators better understand the context and usage of these new filter keys, consider adding description metadata.
Example implementation:
"key_stat_filter_runs": "Runs", + "@key_stat_filter_runs": { + "description": "Filter option for viewing runs statistics in tournament details" + },khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (2)
125-129: Consider sorting the filtered stats by runs scored.Currently, the filtered list for
KeyStatFilterTag.runsis not sorted. Sorting the players by the number of runs scored will display the top performers first and enhance the user experience.Apply this diff to sort the filtered stats:
filteredStats = filteredStats .where((element) => (element.stats.battingStat?.runScored ?? 0) > 0) + ..sort((a, b) => (b.stats.battingStat?.runScored ?? 0) + .compareTo(a.stats.battingStat?.runScored ?? 0)) .toList();
130-135: Consider sorting the filtered stats by wickets taken.Similarly, sorting the players by the number of wickets taken in the
KeyStatFilterTag.wicketscase will highlight leading bowlers and improve data presentation.Apply this diff to sort the filtered stats:
filteredStats = filteredStats .where((element) => (element.stats.bowlingStat?.wicketTaken ?? 0.0) > 0.0) + ..sort((a, b) => (b.stats.bowlingStat?.wicketTaken ?? 0.0) + .compareTo(a.stats.bowlingStat?.wicketTaken ?? 0.0)) .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
data/lib/api/match/match_model.dart(1 hunks)data/lib/service/tournament/tournament_service.dart(1 hunks)khelo/assets/locales/app_en.arb(1 hunks)khelo/lib/ui/flow/team/detail/team_detail_view_model.dart(1 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart(2 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart(3 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(3 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart(14 hunks)
🔇 Additional comments (13)
data/lib/service/tournament/tournament_service.dart (1)
144-144: Verify the sorting behavior in getTopKeyStats extension.
The sorting logic has been moved from this method to the getTopKeyStats() extension method. While this improves code organization, we should ensure that the extension method maintains the expected sorting behavior.
Let's verify the implementation and usage:
✅ Verification successful
The sorting logic removal is correct and safe
After reviewing the complete implementation, I can confirm that the removal of sorting from the original method is correct. The getTopKeyStats() extension method doesn't need explicit sorting because:
- It maintains a map (
playerMap) that tracks the highest stats for each player - For each player, it tracks their best performance across runs, wickets, fours, and sixes
- When a player has multiple stats, it only keeps the highest value through the comparison
highestValue > (existingPlayerStat.value ?? 0) - The final list returned by
playerMap.values.toList()contains only the top performers for each category
The implementation ensures that each player appears only once with their best statistical achievement, making additional sorting unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getTopKeyStats extension and its usage
# Test 1: Find the extension implementation
echo "Looking for getTopKeyStats extension implementation..."
rg "getTopKeyStats" -A 10
# Test 2: Find other usages of getTopKeyStats to ensure consistent behavior
echo "Looking for other usages of getTopKeyStats..."
rg "\.getTopKeyStats\(\)"
# Test 3: Look for any sorting-related tests
echo "Looking for related test files..."
fd -e dart -e test "tournament.*test|stat.*test"
Length of output: 2232
Script:
#!/bin/bash
# Let's examine the complete implementation of PlayerKeyStat and related sorting logic
# Look for PlayerKeyStat class definition and implementation
echo "Looking for PlayerKeyStat class..."
ast-grep --pattern 'class PlayerKeyStat {
$$$
}'
# Check for any sorting or comparison logic in the model
echo "Looking for sorting/comparison implementations..."
rg "compareTo|sort" data/lib/api/tournament/tournament_model.dart -A 5
# Get more context around getTopKeyStats implementation
echo "Getting full context of getTopKeyStats..."
rg "getTopKeyStats" data/lib/api/tournament/tournament_model.dart -B 5 -A 20
Length of output: 1253
Script:
#!/bin/bash
# Let's get the complete implementation of getTopKeyStats and any related tests
# Get the complete file content to see the full implementation
echo "Getting complete tournament model file..."
cat data/lib/api/tournament/tournament_model.dart
# Look for any test files that might verify the sorting behavior
echo "Looking for tournament model tests..."
fd -e dart "tournament_model_test|tournament_service_test"
# Check for any usage of KeyStatTag enum
echo "Looking for KeyStatTag definition and usage..."
rg "KeyStatTag" -A 5
Length of output: 14884
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart (3)
19-19: LGTM: Type change improves clarity
The callback type change from KeyStatTag to KeyStatFilterTag provides better type safety and clearer intent for the filtering mechanism.
46-49: LGTM: Consistent naming improvements
The parameter updates from statFilter to selectedFilterTag improve code readability and maintain consistency with the new filtering mechanism.
201-250: 🛠️ Refactor suggestion
Add haptic feedback for filter selection
The bottom sheet implementation looks good, but as mentioned in the previous review, we should add haptic feedback for the filter selection.
Add this before context.pop():
onTap: () {
+ HapticFeedback.mediumImpact();
context.pop();
onTap(element);
},data/lib/api/match/match_model.dart (1)
344-348: Consider using BigInt for runs accumulation.
For tournaments with many matches, integer overflow could occur when accumulating runs. Consider using BigInt for the accumulation and converting back to int for the final result.
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (1)
163-163: LGTM!
Consistently using keyStatsList for the view all condition.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart (3)
Line range hint 1-4: LGTM: Generated code maintains consistency.
The generated code correctly implements all the necessary Freezed functionality for the updated property.
252-252: Verify the default value change impact.
The default value is now set to KeyStatFilterTag.runs. This change in default behavior could affect existing functionality.
#!/bin/bash
# Description: Find all direct instantiations of TournamentDetailState to verify default value usage
echo "Searching for TournamentDetailState instantiations..."
rg "TournamentDetailState\(" --type dart27-27: Verify the impact of KeyStatTag to KeyStatFilterTag migration.
The type change from KeyStatTag to KeyStatFilterTag for the renamed property selectedFilterTag (previously statFilter) is a breaking change that could affect dependent code. Let's verify all usages to ensure proper migration.
Also applies to: 53-53, 84-84, 115-118, 163-163, 193-193, 224-227, 252-252, 288-288, 309-309, 331-332, 350-350, 374-374, 395-395
✅ Verification successful
Let's analyze the search results. The output shows:
- No occurrences of
KeyStatTagfound, suggesting complete removal - All
KeyStatFilterTagusages are properly contained within the tournament detail feature files:- tournament_detail_view_model.freezed.dart (generated)
- tournament_detail_view_model.dart (source)
- tournament_detail_stats_tab.dart (UI)
- No old
statFilterusages found
Let's verify if there are any imports of KeyStatTag that might need cleanup:
Migration from KeyStatTag to KeyStatFilterTag is complete and consistent
The verification shows that:
- No traces of the old
KeyStatTagenum exist in the codebase - The new
KeyStatFilterTagis properly scoped within the tournament detail feature - All related files (view model, generated code, and UI) have been updated consistently
- No test files are affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of both KeyStatTag and KeyStatFilterTag to ensure complete migration
echo "Searching for old KeyStatTag usages that might need updating..."
rg "KeyStatTag" --type dart
echo -e "\nSearching for new KeyStatFilterTag usages..."
rg "KeyStatFilterTag" --type dart
echo -e "\nSearching for old statFilter usages that might need updating..."
rg "statFilter" --type dart
Length of output: 5176
Script:
#!/bin/bash
# Search for potential import statements of KeyStatTag
rg "import.*KeyStatTag" --type dart
# Also check for any potential references in test files
rg "KeyStatTag" --type dart --glob "**/*test*.dart"
Length of output: 89
khelo/lib/ui/flow/team/detail/team_detail_view_model.dart (1)
61-64: Simplified team statistics calculation enhances maintainability
The refactored _calculateTeamStat method now efficiently computes team statistics by utilizing the teamStat extension method on the filtered list of finished matches. This change eliminates the need for multiple helper methods, improving both readability and maintainability of the code.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (3)
11-11: Approved addition of necessary import.
The import of context_extensions.dart is required for localization and context extensions used in the code.
197-199: Approved the streamlined retrieval of finished matches.
Filtering finished matches directly from the tournament's matches improves readability and efficiency.
207-207: Confirmed usage of the teamStat extension method.
Utilizing matches.teamStat(team.id) simplifies the calculation of team statistics and aligns with the refactored logic introduced in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
data/lib/service/tournament/tournament_service.dart (1)
Line range hint
119-146: Consider optimizing the statistics calculation process.The current implementation of
getKeyStatsprocesses all matches and players sequentially. As part of the refactoring effort, consider these improvements:
- Batch processing of ball scores to reduce database calls
- Parallel processing for large datasets
- Caching frequently accessed statistics
Example optimization:
Future<List<PlayerKeyStat>> getKeyStats(List<MatchModel> matches) async { // Batch fetch all scores at once final matchIds = matches.map((match) => match.id).toSet().toList(); final scores = await _ballScoreService.getBallScoresByMatchIds(matchIds); // Process statistics in parallel for better performance final playerStats = await Future.wait( matches.expand((match) => match.teams.expand((team) => team.squad.map((player) => PlayerKeyStat( player: player.player, teamName: team.team.name, stats: scores.calculateUserStats(player.id), ) ) )).toList() ); return playerStats .getTopKeyStats() .where((element) => element.player.isActive) .toList(); }khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart (1)
198-250: Consider refactoring for better maintainability.The new filter sheet implementation is a good improvement with better UX, but there are some opportunities for enhancement:
- Simplify padding composition
- Extract color management to reduce duplication
- Consider moving chip styling to a theme or constant
- Break down the method into smaller, more focused methods
Here's a suggested refactoring:
+ // Extract to a separate widget + class FilterOptionChip extends StatelessWidget { + final KeyStatFilterTag tag; + final KeyStatFilterTag? selectedTag; + final ValueChanged<KeyStatFilterTag> onTap; + + const FilterOptionChip({ + required this.tag, + required this.selectedTag, + required this.onTap, + }); + + @override + Widget build(BuildContext context) { + final isSelected = selectedTag == tag; + return OnTapScale( + onTap: () { + context.pop(); + onTap(tag); + }, + child: Chip( + label: Text( + tag.getString(context), + style: AppTextStyle.body2.copyWith( + color: isSelected + ? context.colorScheme.onPrimary + : context.colorScheme.textSecondary, + ), + ), + padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), + side: const BorderSide(color: Colors.transparent), + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(30), + ), + backgroundColor: isSelected + ? context.colorScheme.primary + : context.colorScheme.containerLowOnSurface, + ), + ); + } + } void showFilterOptionSelectionSheet( BuildContext context, { required Function(KeyStatFilterTag) onTap, KeyStatFilterTag? selectedTag, }) async { return await showModalBottomSheet( context: context, showDragHandle: true, enableDrag: true, isScrollControlled: true, useRootNavigator: true, backgroundColor: context.colorScheme.surface, builder: (context) { return Container( width: double.infinity, - padding: EdgeInsets.all(16) - .copyWith(bottom: context.mediaQueryPadding.bottom + 24), + padding: EdgeInsets.fromLTRB(16, 16, 16, context.mediaQueryPadding.bottom + 24), child: Wrap( alignment: WrapAlignment.start, spacing: 8, runSpacing: 8, children: KeyStatFilterTag.values.map( - (element) { - final isSelected = selectedTag == element; - return OnTapScale( - onTap: () { - context.pop(); - onTap(element); - }, - child: Chip( - label: Text( - element.getString(context), - style: AppTextStyle.body2.copyWith( - color: isSelected - ? context.colorScheme.onPrimary - : context.colorScheme.textSecondary, - ), - ), - padding: - const EdgeInsets.symmetric(horizontal: 16, vertical: 8), - side: const BorderSide(color: Colors.transparent), - shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(30), - ), - backgroundColor: isSelected - ? context.colorScheme.primary - : context.colorScheme.containerLowOnSurface, - ), - ); - }, + (tag) => FilterOptionChip( + tag: tag, + selectedTag: selectedTag, + onTap: onTap, + ), ).toList(), ), ); }, ); }data/lib/api/match/match_model.dart (3)
340-381: Add documentation for complex statistical calculations.The
teamStatmethod performs complex cricket statistics calculations. Adding documentation would help maintainers understand the business logic and formulas used.Add documentation like this:
+ /// Calculates comprehensive statistics for a team across multiple matches. + /// + /// Parameters: + /// - teamId: The ID of the team to calculate statistics for + /// + /// Returns a TeamStat object containing: + /// - Batting average = Total runs / Total wickets lost + /// - Bowling average = Total runs conceded / Total wickets taken + /// - Run rate = Total runs / Total overs + /// - Match status (wins/losses/ties) + /// - Highest and lowest team scores TeamStat teamStat(String teamId) {
383-426: Refactor match status calculation for better readability.The current implementation has deep nesting and complex logic that makes it hard to maintain. Consider:
- Extracting the winner determination logic
- Handling abandoned matches
- Simplifying the nested conditions
Consider this refactor:
TeamMatchStatus _teamMatchStatus(String teamId) { return fold<TeamMatchStatus>( const TeamMatchStatus(), (status, match) { + // Skip abandoned matches + if (match.match_status == MatchStatus.abandoned) { + return status; + } + + final winner = _determineMatchWinner(match); + if (winner.isTie) { return TeamMatchStatus( win: status.win, lost: status.lost, tie: status.tie + 1, ); + } else if (winner.teamId == teamId) { return TeamMatchStatus( win: status.win + 1, lost: status.lost, tie: status.tie, ); } else { return TeamMatchStatus( win: status.win, lost: status.lost + 1, tie: status.tie, ); } }, ); } + /// Determines the winner of a match + /// Returns a MatchWinner object containing the winning team's ID + /// or indicates a tie + MatchWinner _determineMatchWinner(MatchModel match) { + final firstTeam = match.teams.firstWhere( + (team) => team.team.id == ( + match.toss_decision == TossDecision.bat + ? match.toss_winner_id + : match.teams + .firstWhere((team) => team.team.id != match.toss_winner_id) + .team + .id + ), + ); + + final secondTeam = match.teams.firstWhere( + (team) => team.team.id != firstTeam.team.id + ); + + if (firstTeam.run == secondTeam.run) { + return MatchWinner.tie(); + } + + return MatchWinner( + teamId: firstTeam.run > secondTeam.run + ? firstTeam.team.id + : secondTeam.team.id + ); + }
434-439: Optimize run rate calculation.The current implementation makes multiple passes over the match list. Consider combining the runs and overs calculation in a single pass.
double _runRate(String teamId, int runs) { - final totalOvers = map((match) => _getTeam(match, teamId)) - .fold<double>(0.0, (total, team) => total + team.over); - - return totalOvers > 0 ? runs / totalOvers : 0; + final totalOvers = fold<double>( + 0.0, + (total, match) => total + _getTeam(match, teamId).over + ); + + return totalOvers > 0 ? runs / totalOvers : 0; }khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (2)
163-163: Optimize length check for view all button.The length check could be done on the original list since sorting doesn't affect the length.
-showViewAll: keyStatsList.length > 4, +showViewAll: keyStats.length > 4,
171-171: Optimize list operations in GridView.builder.The current implementation creates unnecessary temporary lists and may perform redundant index access operations.
Consider these optimizations:
-itemCount: keyStatsList.take(4).length, +itemCount: keyStatsList.length.clamp(0, 4), -_keyStatsCellView(context, keyStatsList[index]), +_keyStatsCellView(context, keyStatsList[index.clamp(0, 3)]),This approach:
- Avoids creating a temporary list with
take(4)- Ensures safe index access with
clamp- Improves performance by reducing object allocations
Also applies to: 180-180
khelo/assets/locales/app_en.arb (1)
243-251: LGTM! Consider adding pluralization support.The new filter keys are well-organized and follow consistent naming conventions. However, for better internationalization support, consider adding pluralization rules for numerical statistics, similar to how it's implemented in other keys like
common_runs_title.Example implementation for runs:
- "key_stat_filter_runs": "Runs", + "key_stat_filter_runs": "{count, plural, =0{Runs} =1{Run} other{Runs}}", + "@key_stat_filter_runs": { + "placeholders": { + "count": {} + } + },khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (2)
119-191: RefactoronStatFiltermethod to reduce code duplicationThe
onStatFiltermethod contains repetitive code across multiple switch cases for filteringfilteredStats. Refactoring this method can enhance maintainability and readability.Consider mapping each
KeyStatFilterTagto its corresponding filter logic:void onStatFilter(KeyStatFilterTag? tag) { if (state.tournament == null) return; var filteredStats = state.tournament!.keyStats; final filterMap = { KeyStatFilterTag.runs: (PlayerKeyStat e) => (e.stats.battingStat?.runScored ?? 0) > 0, KeyStatFilterTag.wickets: (PlayerKeyStat e) => (e.stats.bowlingStat?.wicketTaken ?? 0.0) > 0.0, KeyStatFilterTag.battingAverage: (PlayerKeyStat e) => (e.stats.battingStat?.average ?? 0.0) > 0.0, KeyStatFilterTag.bowlingAverage: (PlayerKeyStat e) => (e.stats.bowlingStat?.average ?? 0.0) > 0.0, // Add other filters as needed }; if (tag != null && filterMap.containsKey(tag)) { filteredStats = filteredStats.where(filterMap[tag]!).toList(); } state = state.copyWith( filteredStats: filteredStats, selectedFilterTag: tag ?? state.selectedFilterTag, ); }
244-277: Avoid usingBuildContextwithin enumKeyStatFilterTagEmbedding
BuildContextin theKeyStatFilterTagenum ties it to the Flutter framework, reducing the enum's testability and reusability. Consider moving the localization logic outside the enum.You can create an extension method for localization:
extension KeyStatFilterTagExtension on KeyStatFilterTag { String getString(BuildContext context) { switch (this) { case KeyStatFilterTag.runs: return context.l10n.key_stat_filter_runs; case KeyStatFilterTag.wickets: return context.l10n.key_stat_filter_wickets; // Add other cases } } }This approach keeps the enum pure and separates UI concerns from the data model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
data/lib/api/match/match_model.dart(1 hunks)data/lib/service/tournament/tournament_service.dart(1 hunks)khelo/assets/locales/app_en.arb(1 hunks)khelo/lib/ui/flow/team/detail/team_detail_view_model.dart(1 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart(2 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart(3 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(3 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart(14 hunks)
🔇 Additional comments (8)
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart (3)
9-9: LGTM! Good UX enhancement.
The addition of on_tap_scale.dart import enhances user interaction feedback with animation effects.
46-49: LGTM! Clean and readable implementation.
The changes to FilterTabView usage are well-structured with clear named parameters and proper state management.
19-19: Verify type change consistency across the codebase.
The parameter type change from KeyStatTag to KeyStatFilterTag looks good, but let's ensure all related code is updated.
data/lib/api/match/match_model.dart (1)
368-368: 🛠️ Refactor suggestion
Add null check for division by zero in bowling average calculation.
The bowling average calculation should handle the case where length is 0 more explicitly.
- final bowlingAverage = length > 0.0 ? bowlingAverageTotal / length : 0.0;
+ final bowlingAverage = length > 0 ? bowlingAverageTotal / length : 0.0;Likely invalid or redundant comment.
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (1)
163-163: Verify architectural impact of moving sorting logic.
The PR moves sorting from the service layer to the UI layer. Please ensure:
- Other views using the same data aren't affected by this change
- The unimplemented view all functionality (
onViewAll: () {}) has a follow-up task
Also applies to: 171-171, 180-180
✅ Verification successful
Let me analyze the sorting implementation to verify the architectural impact.
Moving sorting to UI layer is architecturally sound
The changes are well-structured and don't impact other views:
- The service layer still provides
keyStatsdata throughgetKeyStats()method - Sorting is appropriately moved to UI layer since it's view-specific filtering based on user selection (hundreds, fifties, sixes)
- Other views using
keyStatsfromTournamentModelcan implement their own sorting as needed
The view all functionality is implemented through showViewAll prop and list limiting (take(4)), with proper UI handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of keyStats that might expect sorted data
rg "keyStats" --type dart
# Search for TODOs related to view all functionality
rg "TODO.*view.*all" --type dart
Length of output: 4464
Script:
#!/bin/bash
# Let's check the implementation details of sorting logic in both service and UI layers
ast-grep --pattern 'getTopKeyStats()'
# Check for any sorting related code in the tournament detail files
rg "sort" -A 3 --type dart khelo/lib/ui/flow/tournament/detail/
Length of output: 2351
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart (1)
27-27: Verify the impact of the enum value change.
The generated code changes look correct and consistent. However, please verify that changing from KeyStatTag.mostRuns to KeyStatFilterTag.runs maintains the same semantic meaning and doesn't break existing functionality.
Let's verify the enum definitions and their usage:
Also applies to: 53-53, 84-84, 115-118, 163-163, 193-193, 224-227, 252-252, 288-288, 309-309, 331-332, 350-350, 374-374, 395-395
✅ Verification successful
Let me gather more context about the relationship between these enums by checking their usage patterns.
Let me check the specific implementations in these files to understand the relationship between these enums.
Based on the code analysis, I can now provide the final response:
The enum change is safe and represents an intentional refactoring.
The change from KeyStatTag to KeyStatFilterTag is a deliberate refactoring where:
KeyStatTag(in data layer) is used for API/data models to represent basic stats (mostRuns, mostWickets, mostFours, mostSixes)KeyStatFilterTag(in UI layer) is a new, more comprehensive enum for UI filtering with additional options (runs, wickets, battingAverage, bowlingAverage, etc.)
The change from mostRuns to runs is semantically equivalent, as evidenced by the filtering logic in onStatFilter() method which properly handles the runs case by filtering based on battingStat?.runScored.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definitions and usage of both enums to ensure compatibility
# Find the enum definitions
echo "=== Enum Definitions ==="
rg -A 5 "enum KeyStatTag|enum KeyStatFilterTag"
# Find usages of both enum values
echo -e "\n=== Enum Value Usage ==="
rg "KeyStatTag\.mostRuns|KeyStatFilterTag\.runs"
Length of output: 1826
Script:
#!/bin/bash
# Check for any mapping or conversion between the two enum types
echo "=== Enum Conversion/Mapping ==="
rg -B 2 -A 2 "KeyStatTag.*KeyStatFilterTag|KeyStatFilterTag.*KeyStatTag"
# Check the full implementation of files where both enums are used
echo -e "\n=== Files Using Both Enums ==="
fd -e dart -x sh -c 'if grep -l "KeyStatTag" {} && grep -l "KeyStatFilterTag" {}; then echo "=== {}:"; cat {}; fi'
Length of output: 546
Script:
#!/bin/bash
# Check the content of key files
echo "=== enum_extensions.dart ==="
cat "khelo/lib/domain/extensions/enum_extensions.dart"
echo -e "\n=== tournament_detail_view_model.dart ==="
cat "khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart"
Length of output: 21275
khelo/lib/ui/flow/team/detail/team_detail_view_model.dart (1)
61-64: Refactored team statistics calculation enhances simplicity and maintainability.
The use of the teamStat extension method in _calculateTeamStat simplifies the code by delegating statistical calculations, reducing code duplication, and improving readability.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (1)
11-11: Import statement added appropriately
The addition of context_extensions.dart is necessary for localization extensions used in the code.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
style/lib/button/chip_button.dart (3)
1-6: Consider organizing imports following Flutter conventions.Consider organizing imports into three sections separated by blank lines:
- Dart/Flutter SDK imports
- Package imports
- Local imports
import 'package:flutter/material.dart'; + import 'package:style/extensions/context_extensions.dart'; - import '../animations/on_tap_scale.dart'; import '../text/app_text_style.dart';
7-11: Add documentation for better code maintainability.Consider adding documentation comments to describe:
- The widget's purpose and use cases
- Parameter descriptions
- Example usage
+/// A selectable chip button widget that changes appearance based on selection state. +/// +/// Parameters: +/// - [isSelected]: Controls the visual state of the button +/// - [title]: The text to display on the chip +/// - [onTap]: Callback function triggered when the chip is tapped +/// +/// Example: +/// ```dart +/// ChipButton( +/// isSelected: true, +/// title: 'Select Me', +/// onTap: () => print('Chip tapped'), +/// ) +/// ``` class ChipButton extends StatelessWidget { final bool isSelected; final String title; final VoidCallback? onTap;
19-42: Consider these improvements for better maintainability and accessibility.While the implementation is functional, consider these enhancements:
- Extract magic numbers into named constants
- Add semantic properties for accessibility
- Use theme-based text styles instead of direct style modifications
+ static const double _horizontalPadding = 16.0; + static const double _verticalPadding = 8.0; + static const double _borderRadius = 30.0; @override Widget build(BuildContext context) { return OnTapScale( onTap: onTap, + child: Semantics( + button: true, + selected: isSelected, + label: '$title chip button', child: Chip( label: Text( title, - style: AppTextStyle.body2.copyWith( - color: isSelected - ? context.colorScheme.onPrimary - : context.colorScheme.textSecondary, - ), + style: Theme.of(context).textTheme.bodyMedium?.copyWith( + color: isSelected + ? context.colorScheme.onPrimary + : context.colorScheme.textSecondary, + ), ), - padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), + padding: const EdgeInsets.symmetric( + horizontal: _horizontalPadding, + vertical: _verticalPadding + ), side: const BorderSide(color: Colors.transparent), shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(30), + borderRadius: BorderRadius.circular(_borderRadius), ), backgroundColor: isSelected ? context.colorScheme.primary : context.colorScheme.containerLowOnSurface, ), + ), ); }khelo/lib/ui/flow/score_board/components/select_wicket_type_sheet.dart (1)
66-69: Consider enhancing accessibility for wicket type selection.The
ChipButtonimplementation is clean and effective. However, consider adding accessibility support for better user experience.Consider these accessibility enhancements:
return ChipButton( isSelected: isSelected, title: element.getString(context), onTap: () => setState(() => selectedType = element), + semanticsLabel: '${element.getString(context)} wicket type option${isSelected ? ', selected' : ''}', );khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (1)
227-266: LGTM: Well-structured enum with localization support.The KeyStatFilterTag enum is well-implemented with:
- Clear, descriptive names
- Proper localization integration
- Comprehensive coverage of stat types
Consider adding documentation comments to describe each enum value's purpose and any specific business rules associated with them.
/// Enum representing different types of statistical filters for tournament details. enum KeyStatFilterTag { /// Filter by total runs scored runs, /// Filter by total wickets taken wickets, // Add documentation for other values... }data/lib/api/match/match_model.dart (2)
383-426: Simplify team selection logic in _teamMatchStatus method.The team selection logic in the
_teamMatchStatusmethod is deeply nested and could be simplified for better readability.Consider extracting the team selection logic:
- final firstTeam = match.teams.firstWhere( - (team) => - team.team.id == - (match.toss_decision == TossDecision.bat - ? match.toss_winner_id - : match.teams - .firstWhere( - (team) => team.team.id != match.toss_winner_id, - ) - .team - .id), - ); + final firstBattingTeamId = match.toss_decision == TossDecision.bat + ? match.toss_winner_id + : match.teams + .firstWhere((team) => team.team.id != match.toss_winner_id) + .team + .id; + final firstTeam = + match.teams.firstWhere((team) => team.team.id == firstBattingTeamId);
434-439: Improve null safety in _runRate calculation.The
_runRatemethod should handle potential null or invalid over values more robustly.Consider adding validation:
double _runRate(String teamId, int runs) { final totalOvers = map((match) => _getTeam(match, teamId)) - .fold<double>(0.0, (total, team) => total + team.over); + .fold<double>(0.0, (total, team) => total + (team.over ?? 0.0)); - return totalOvers > 0 ? runs / totalOvers : 0; + return totalOvers > 0.0 ? (runs / totalOvers).clamp(0.0, double.infinity) : 0.0; }khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart (3)
20-20: Use explicit function type instead of 'Function'Using the generic
Functiontype can reduce type safety and readability. It's recommended to specify the function type explicitly asvoid Function(KeyStatFilterTag)to ensure better type checking and clarity.Apply this diff to enhance type safety:
- final Function(KeyStatFilterTag) onFiltered; + final void Function(KeyStatFilterTag) onFiltered;
199-200: Specify function type explicitly for 'onTap' parameterFor better type safety and readability, replace the generic
Functiontype with an explicit function typevoid Function(KeyStatFilterTag).Apply this diff to update the function signature:
void showFilterOptionSelectionSheet( BuildContext context, { - required Function(KeyStatFilterTag) onTap, + required void Function(KeyStatFilterTag) onTap, KeyStatFilterTag? selectedTag, }) async {
221-226: Avoid potential confusion with identical parameter and local variable namesUsing
onTapas both a parameter name and a local variable within the callback can lead to confusion. Consider renaming the parameter to provide clearer differentiation.Apply this diff to rename the parameter:
children: KeyStatFilterTag.values.map( (element) { return ChipButton( title: element.getString(context), isSelected: selectedTag == element, onTap: () { context.pop(); - onTap(element); + onFilterTap(element); }, ); }, ).toList(),And update the parameter name accordingly:
- required void Function(KeyStatFilterTag) onTap, + required void Function(KeyStatFilterTag) onFilterTap,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
data/lib/api/match/match_model.dart(1 hunks)khelo/assets/locales/app_en.arb(1 hunks)khelo/lib/ui/flow/matches/add_match/components/pitch_selection_view.dart(2 hunks)khelo/lib/ui/flow/score_board/components/select_wicket_type_sheet.dart(2 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_stats_tab.dart(3 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(3 hunks)style/lib/button/chip_button.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- khelo/assets/locales/app_en.arb
🔇 Additional comments (7)
style/lib/button/chip_button.dart (1)
12-18: LGTM! Constructor follows Flutter best practices.
The constructor is well-designed with:
- Const constructor for better performance
- Appropriate use of required annotation
- Sensible default value for isSelected
khelo/lib/ui/flow/matches/add_match/components/pitch_selection_view.dart (2)
6-6: LGTM: Import added for new ChipButton component
The import is correctly placed and properly utilized in the code.
32-38: LGTM: Clean implementation using standardized ChipButton
The refactoring simplifies the code by using the new ChipButton component while maintaining the same functionality. The implementation is clean and follows good Flutter practices.
Let's verify the consistent usage of ChipButton across the codebase:
✅ Verification successful
ChipButton implementation is consistent across the codebase
The verification shows that:
- ChipButton is used consistently in 3 different views with the same props pattern (title, isSelected, onTap)
- The base ChipButton component in style/lib/button/chip_button.dart provides a standardized implementation
- Only two legitimate Chip usages exist - one in ChipButton itself and another in SecondaryButton, both in the style library
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent ChipButton usage patterns
# Expected: Similar padding and styling across different views
# Search for ChipButton usage patterns
rg -l "ChipButton" | xargs rg -A 3 "ChipButton\("
# Check for any remaining custom chip implementations that should be migrated
rg -l "Chip\(" | xargs rg -A 3 "Chip\("
Length of output: 2303
khelo/lib/ui/flow/score_board/components/select_wicket_type_sheet.dart (1)
10-10: LGTM! Good use of the shared button component.
The change from custom implementation to using the shared ChipButton component promotes better code reuse and UI consistency.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (2)
11-11: LGTM: Import for localization.
The added import for context extensions is necessary for localization support in the KeyStatFilterTag enum.
186-196: LGTM: Improved match filtering and team stats calculation.
The changes effectively simplify the code by:
- Using a clear filter for finished matches
- Leveraging the new teamStat extension method
data/lib/api/match/match_model.dart (1)
428-433: LGTM: Utility methods are well-implemented.
The _getTeam and _getOpponentTeam methods are concise and serve their purpose well.
refactor_filter.webm
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
UI Enhancements