Record match event and partnership #149
Conversation
WalkthroughThis pull request introduces significant enhancements across multiple files related to sports data modeling and management. Key changes include the addition of new data models for match events and partnerships, improvements to existing ball score logic, and the introduction of services for managing these entities in Firestore. New computed properties and methods have been added to facilitate detailed statistics tracking, while existing methods have been refined for better clarity and efficiency. Overall, the changes aim to improve the functionality and integration of match scoring and event tracking within the application. 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 (
|
# Conflicts: # khelo/lib/ui/flow/score_board/score_board_view_model.dart
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
data/lib/service/match_event/match_event_service.dart (2)
29-31: SimplifyasyncMaptomapinstreamEventsByMatchesSince the
asyncMapdoes not perform any asynchronous operations, you can simplify it by usingmapinstead.Apply this diff to simplify the code:
- .snapshots() - .asyncMap((snapshot) async { + .snapshots() + .map((snapshot) { return snapshot.docs.map((mainDoc) => mainDoc.data()).toList(); })
43-44: Consider logging errors for better debuggingCurrently, errors are being converted to
AppErrorwithout logging. Adding logging statements for errors and stack traces can facilitate debugging and improve maintainability.You might add a logging statement like:
// Before throwing the error print('Error updating match event: $error\nStack trace: $stack');data/lib/service/partnership/partnership_service.dart (2)
43-45: SimplifyasyncMaptomapinstreamPartnershipByMatchesThe
asyncMapdoes not perform any asynchronous operations. You can simplify it by usingmapinstead.Apply this diff to simplify the code:
- .snapshots() - .asyncMap((snapshot) async { + .snapshots() + .map((snapshot) { return snapshot.docs.map((mainDoc) => mainDoc.data()).toList(); })
51-52: Consider logging errors for better debuggingErrors are being converted to
AppErrorwithout any logging. Adding logs for errors and stack traces can help with debugging issues in production.You might add a logging statement like:
// Before throwing the error print('Error deleting partnership: $error\nStack trace: $stack');data/lib/api/match_event/match_event_model.dart (1)
42-53: Use lowerCamelCase for enum valuesEnum values should be in lowerCamelCase to follow Dart naming conventions. For example,
hatTrickinstead ofhatTrick.Apply this diff to adjust the enum values:
enum EventType { - hatTrick(1), - century(2), - fifty(3), - wicket(4), - six(5), - four(6); + hatTrick(1), + century(2), + fifty(3), + wicket(4), + six(5), + four(6);data/lib/api/ball_score/ball_score_model.dart (3)
Line range hint
290-297: Avoid returning nullable boolean inisLegalDeliveryReturning
nullcan lead to unnecessary null checks elsewhere. Consider returningfalsewhenthisisnullto simplify the method usage.Apply this diff:
- bool? isLegalDelivery() { + bool isLegalDelivery() { if (this == null) { - return null; + return false; } return this!.extras_type != ExtrasType.penaltyRun && this!.extras_type != ExtrasType.noBall &&
Line range hint
305-309: RefineisMaidenlogic to handle overs with extra deliveriesThe current
isMaidenlogic assumes the last ball number is 6 and a legal delivery. This may not account for overs with extra deliveries due to wides or no-balls. Consider summing legal deliveries to determine the end of an over.You might revise the condition to:
bool get isMaiden => runs == 0 && wickets == 0 && extras == 0 && legalDeliveriesCount == 6;
Line range hint
325-328: EnsurelegalDeliveriesCountis not zero to avoid division by zeroIn the
overCountgetter, dividing by zero can occur iflegalDeliveriesCountis zero. Add a check to prevent runtime errors.Consider adding:
if (legalDeliveriesCount == 0) { return 0.0; }data/lib/api/match_event/match_event_model.freezed.dart (1)
1-985: Consider excluding generated code from version controlIncluding generated files such as
match_event_model.freezed.dartin version control can lead to merge conflicts and increase repository size. It's generally recommended to exclude generated files from version control and generate them during the build process.khelo/lib/ui/flow/score_board/score_board_view_model.dart (1)
844-908: Consider recording all partnerships, not just those exceeding 50 runsThe current implementation only records partnerships when
totalRuns > 50. In cricket, all partnerships are usually recorded regardless of the number of runs scored. This provides a complete statistical record of the match. Consider modifying the code to record all partnerships.data/lib/api/partnership/partnership_model.dart (1)
1-1: Typo in comment syntaxThere is an extra space in the comment on line 1. It should be corrected to
// ignore_for_file: non_constant_identifier_nameswithout a space between the slashes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
data/lib/api/ball_score/ball_score_model.dart(1 hunks)data/lib/api/match_event/match_event_model.dart(1 hunks)data/lib/api/match_event/match_event_model.freezed.dart(1 hunks)data/lib/api/match_event/match_event_model.g.dart(1 hunks)data/lib/api/partnership/partnership_model.dart(1 hunks)data/lib/api/partnership/partnership_model.freezed.dart(1 hunks)data/lib/api/partnership/partnership_model.g.dart(1 hunks)data/lib/service/ball_score/ball_score_service.dart(2 hunks)data/lib/service/match_event/match_event_service.dart(1 hunks)data/lib/service/partnership/partnership_service.dart(1 hunks)data/lib/utils/constant/firestore_constant.dart(1 hunks)khelo/lib/ui/flow/score_board/score_board_view_model.dart(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- data/lib/api/partnership/partnership_model.g.dart
- data/lib/api/match_event/match_event_model.g.dart
🔇 Additional comments (8)
data/lib/service/partnership/partnership_service.dart (1)
30-31: Verify the use of partnershipRef.id when updating
In the updatePartnership method, you replace partnership.id with partnershipRef.id. Ensure this is intentional, as it might overwrite the original id of the partnership.
Confirm whether the id should remain the same or be updated to partnershipRef.id.
data/lib/api/match_event/match_event_model.dart (1)
25-27: Verify the data type of over field
The over field represents an over number and is currently a double. Ensure that this appropriately captures over fractions and consider using a more precise type if necessary.
Confirm that using double does not introduce precision issues in calculations.
data/lib/api/partnership/partnership_model.freezed.dart (1)
1-619: No issues found in the generated code
This is a generated file (.freezed.dart). No manual changes are required.
data/lib/api/ball_score/ball_score_model.dart (1)
289-289: Potential issue in formattedOver calculation
The formattedOver getter subtracts 1 from over_number. Verify if this accurately represents the over count, as over numbering generally starts from 0 or 1. Also, ensure that string interpolation handles null values correctly.
data/lib/utils/constant/firestore_constant.dart (1)
13-14: LGTM! Verify collection usage across the codebase.
The new collection constants follow the established naming convention and are properly placed with other collection constants.
Let's verify these collections are properly used across the codebase:
✅ Verification successful
Let me check the actual usage of these collections by examining the implementation details.
Let me check for any match events service implementation since we only see the constant definition but no usage.
LGTM! Both collections are properly implemented and used in the codebase.
The verification shows that:
- The
partnershipscollection is properly used inPartnershipServicefor data access andScoreBoardViewNotifierfor managing partnerships data - The
match_eventscollection is properly implemented inMatchEventServicewith proper model conversion and is used inScoreBoardViewNotifierfor tracking match events like wickets, milestones, etc. - Both collections follow Firestore best practices with proper type conversion and collection naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper usage of the new collections
# Expected: Find references in service classes and models
# Check for match_events collection usage
echo "Checking match_events collection usage:"
rg -l "match_events" --type dart
# Check for partnerships collection usage
echo "Checking partnerships collection usage:"
rg -l "partnerships" --type dart
Length of output: 451
Script:
#!/bin/bash
# Check the implementation details of match_events collection usage
echo "=== Checking match_events collection implementation ==="
rg "match_events" --type dart -C 3
# Check the implementation details of partnerships collection usage
echo "=== Checking partnerships collection implementation ==="
rg "partnerships" --type dart -C 3
Length of output: 4336
Script:
#!/bin/bash
# Search for match events related service files and implementations
echo "=== Searching for match events service files ==="
fd "match.*event.*service" --type f
echo "=== Searching for match event model usage ==="
rg "MatchEventModel" --type dart -C 3
Length of output: 25487
data/lib/service/ball_score/ball_score_service.dart (3)
Line range hint 61-96: LGTM! Transaction scope and error handling are well-implemented
The transaction appropriately handles atomic updates across multiple entities (match, innings, and ball score) with proper error handling using AppError.fromError.
63-63: Review transaction handling and over calculation
The changes improve the code by:
- Using
formattedOverproperty for cleaner over calculation - Using
SetOptions(merge: true)for safer concurrent updates
However, we should verify the implementation of formattedOver to ensure it handles all edge cases.
Let's verify the formattedOver implementation:
Also applies to: 91-91
61-61: Verify unique ID generation for ball scores
The change from generating a new document ID to using score.id could lead to document overwrites if the IDs are not guaranteed to be unique.
Let's verify the ID generation logic:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
data/lib/api/ball_score/ball_score_model.dart (5)
226-228: Consider caching computed properties and adding documentation for cricket-specific logic.The computed properties (
isMaiden,overCount,legalDeliveriesCount, etc.) are recalculated on every access, which could impact performance when accessed frequently.Consider:
- Caching the results using late variables that are invalidated when the ball list changes
- Adding documentation explaining cricket-specific rules and edge cases
Example implementation:
+ /// Cached values for frequently accessed properties + late final bool _isMaiden = _calculateIsMaiden(); + late final int _legalDeliveriesCount = _calculateLegalDeliveriesCount(); - bool get isMaiden => - runs == 0 && - wickets == 0 && - extras == 0 && - balls.lastOrNull?.ball_number == 6 && - (balls.lastOrNull?.isLegalDelivery() ?? false); + /// Returns true if the over is a maiden (no runs scored off the bat, no extras, and six legal deliveries) + bool get isMaiden => _isMaiden;Also applies to: 246-248, 250-252, 254-256, 258-261, 263-266
Line range hint
446-448: Handle division by zero in economy calculation.The economy calculation could throw an exception if
overDeliveredis 0.Apply this fix:
- return double.parse((runsConceded / overDelivered).toStringAsFixed(1)); + return overDelivered > 0 + ? double.parse((runsConceded / overDelivered).toStringAsFixed(1)) + : 0.0;
Line range hint
574-576: Handle division by zero in strike rate calculation.The strike rate calculation could throw an exception if
ballFacedis 0.Apply this fix:
- return double.parse(((runs / ballFaced) * 100).toStringAsFixed(1)); + return ballFaced > 0 + ? double.parse(((runs / ballFaced) * 100).toStringAsFixed(1)) + : 0.0;
Line range hint
669-694: Consider using a map lookup instead of switch statement.The current implementation using switch statements is verbose and requires updating two places (add and remove) when adding new extra types.
Consider using a map-based approach:
+ static final _extraTypeToProperty = { + ExtrasType.wide: (ExtraSummary e) => e.wideBall, + ExtrasType.noBall: (ExtraSummary e) => e.noBall, + ExtrasType.bye: (ExtraSummary e) => e.bye, + ExtrasType.legBye: (ExtraSummary e) => e.legBye, + ExtrasType.penaltyRun: (ExtraSummary e) => e.penalty, + }; ExtraSummary addExtra(BallScoreModel ball) { if (ball.extras_type == null) return this; - final ExtraSummary extra; - switch (ball.extras_type!) { - case ExtrasType.wide: - extra = copyWith(wideBall: wideBall + (ball.extras_awarded ?? 0)); - // ... other cases - } + final getter = _extraTypeToProperty[ball.extras_type!]!; + return copyWith( + wideBall: getter(this) + (ball.extras_awarded ?? 0) + ); }
Line range hint
751-898: Optimize statistics calculation to reduce iterations.The statistics calculation methods make multiple passes over the same data. Consider combining calculations to reduce iterations.
For example, in
calculateBattingStats, combine the calculations:- for (final element in this) { - if (element.batsman_id == currentUserId) { - runScored += element.runs_scored; - // ... other calculations - } - if (element.player_out_id == currentUserId) { - dismissal++; - } - } + for (final element in this) { + if (element.batsman_id == currentUserId || + element.player_out_id == currentUserId) { + if (element.batsman_id == currentUserId) { + runScored += element.runs_scored; + // ... other calculations + } + if (element.player_out_id == currentUserId) { + dismissal++; + } + } + }khelo/lib/ui/flow/score_board/score_board_view_model.dart (3)
846-905: Consider tracking all partnershipsThe current implementation only records partnerships that exceed 50 runs. This could miss important partnerships that end just before 50 runs or partnerships that were crucial to the match outcome despite being lower scoring.
Consider:
- Tracking all partnerships
- Adding a
partnership_milestonefield to flag significant partnerships (e.g., 50+, 100+)- Adding partnership rate (runs per ball) for better analysis
- if (totalRuns > 50) { + // Record all partnerships with milestone flags + final partnership = PartnershipModel( + id: _partnershipService.generatePartnershipId, + match_id: ball.match_id, + inning_id: ball.inning_id, + player_ids: batsMen, + players: player, + runs: totalRuns, + extras: extras, + ball_faced: ballFaced, + start_over: scores.first.formattedOver, + end_over: scores.last.formattedOver, + run_rate: ballFaced > 0 ? (totalRuns * 6) / ballFaced : 0, + milestone: totalRuns >= 100 ? "century" : totalRuns >= 50 ? "fifty" : null + );
180-194: Consider centralizing error handlingMultiple methods have similar error handling patterns. Consider creating a centralized error handling mechanism to reduce code duplication and ensure consistent error handling across the codebase.
Example implementation:
Future<T> handleServiceError<T>(String operation, Future<T> Function() action) async { try { return await action(); } catch (e) { debugPrint("ScoreBoardViewNotifier: error while $operation -> $e"); state = state.copyWith(error: AppError.fromError(e)); throw e; } } // Usage: void _loadMatchEvent() async { await handleServiceError('loading match event', () async { if (matchId == null) return; _matchEventSubscription?.cancel(); return _matchEventService .streamEventsByMatches(matchId!) .listen((events) => matchEvents = events); }); }Also applies to: 196-211, 1317-1329
Line range hint
40-57: Consider breaking down the ScoreBoardViewNotifierThe class has grown to handle multiple responsibilities, making it harder to maintain and test. Consider splitting it into smaller, focused notifiers or services.
Suggested architecture:
ScoringNotifier: Core scoring logicMatchEventNotifier: Event trackingPartnershipNotifier: Partnership trackingPlayerStatsNotifier: Player statisticsThis would improve:
- Code organization
- Testability
- Maintainability
- State management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
data/lib/api/ball_score/ball_score_model.dart(1 hunks)data/lib/utils/constant/firestore_constant.dart(1 hunks)khelo/lib/ui/flow/score_board/score_board_screen.dart(2 hunks)khelo/lib/ui/flow/score_board/score_board_view_model.dart(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- data/lib/utils/constant/firestore_constant.dart
🔇 Additional comments (7)
khelo/lib/ui/flow/score_board/score_board_screen.dart (2)
278-278: LGTM! More explicit match status handling
The change from endMatch() to endMatchWithStatus(MatchStatus.finish) improves code clarity by making the final match status explicit.
664-664: LGTM! Consistent match status handling
The change to use endMatchWithStatus(MatchStatus.abandoned) maintains consistency with the new explicit match status handling approach.
khelo/lib/ui/flow/score_board/score_board_view_model.dart (5)
51-52: LGTM! Proper stream management
The stream subscriptions for match events and partnerships are properly initialized and cleaned up in the dispose method.
Also applies to: 1911-1912
Line range hint 1501-1539: LGTM! Comprehensive match completion handling
The endMatchWithStatus method properly:
- Updates player statistics
- Finalizes partnerships
- Updates match status
- Cleans up state
1272-1276:
Missing await before async call
The call to _matchEventService.updateMatchEvent is missing an await, which could lead to race conditions.
180-194:
Improve error handling in _loadMatchEvent
The error handling only logs errors but doesn't update the state or notify the user. This could lead to silent failures.
Consider updating the state with the error:
void _loadMatchEvent() {
try {
if (matchId == null) return;
_matchEventSubscription?.cancel();
_matchEventSubscription = _matchEventService
.streamEventsByMatches(matchId!)
.listen((events) => matchEvents = events, onError: (e) {
debugPrint(
"ScoreBoardViewNotifier: error while loading match event -> $e");
+ state = state.copyWith(error: AppError.fromError(e));
});
} catch (e) {
debugPrint(
"ScoreBoardViewNotifier: error while loading match event -> $e");
+ state = state.copyWith(error: AppError.fromError(e));
}
}Likely invalid or redundant comment.
752-797:
Fix incorrect hat-trick detection logic
The current implementation incorrectly marks multiple wickets as a hat-trick without verifying they occurred in consecutive deliveries.
Consider this fix:
Future<void> handleWicketMatchEvent(BallScoreModel ball) async {
- final lastBall = state.currentScoresList.lastWhereOrNull(
+ final lastThreeBalls = state.currentScoresList.where(
(ball) =>
ball.wicket_type != WicketType.retired &&
ball.wicket_type != WicketType.retiredHurt &&
ball.wicket_type != WicketType.timedOut,
- );
+ ).toList().reversed.take(3).toList();
MatchEventModel matchEvent;
- if (lastBall?.wicket_type != null &&
- lastBall?.bowler_id == ball.bowler_id) {
+ if (lastThreeBalls.length == 3 &&
+ lastThreeBalls.every((b) => b.bowler_id == ball.bowler_id) &&
+ _areConsecutiveDeliveries(lastThreeBalls)) {
matchEvent = matchEvents
.where((element) =>
(element.type == EventType.wicket ||
element.type == EventType.hatTrick) &&
element.bowler_id == ball.bowler_id &&
- element.ball_ids.contains(lastBall?.id))
+ element.ball_ids.contains(lastThreeBalls.last.id))
.firstOrNull ??
MatchEventModel(
id: _matchEventService.generateMatchEventId,
match_id: ball.match_id,
inning_id: ball.inning_id,
type: EventType.wicket,
bowler_id: ball.bowler_id,
time: ball.score_time ?? DateTime.now());
} else {
// ... rest of the code
}
}
+bool _areConsecutiveDeliveries(List<BallScoreModel> balls) {
+ for (int i = 0; i < balls.length - 1; i++) {
+ if (!_isNextDelivery(balls[i], balls[i + 1])) {
+ return false;
+ }
+ }
+ return true;
+}
+
+bool _isNextDelivery(BallScoreModel current, BallScoreModel next) {
+ if (current.over_number == next.over_number) {
+ return current.ball_number == next.ball_number + 1;
+ } else if (current.over_number == next.over_number + 1) {
+ return current.ball_number == 1 && next.ball_number == 6;
+ }
+ return false;
+}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
khelo/lib/ui/flow/score_board/score_board_view_model.dart (3)
704-731: Add documentation for match event handlingThe function handles different types of match events well, but would benefit from documentation explaining:
- Purpose and responsibility of the function
- Different event types handled
- Parameters and their usage
Add documentation like:
+ /// Tracks match events like boundaries, wickets and milestones. + /// + /// Takes a [BallScoreModel] and creates/updates corresponding match events: + /// - Boundaries (four/six) + /// - Wickets (single/hat-trick) + /// - Batting milestones (fifty/century) Future<void> addMatchEventIfNeeded(BallScoreModel ball) async {
733-778: Consider breaking down wicket event handlingThe wicket event handling logic is complex and would benefit from being split into smaller, focused functions for better maintainability.
Consider refactoring into smaller functions:
Future<void> handleWicketMatchEvent(BallScoreModel ball) async { + final lastWicketBall = _getLastWicketBall(); + final existingEvent = _findExistingWicketEvent(ball, lastWicketBall); + final updatedEvent = _createOrUpdateWicketEvent(ball, existingEvent); + await _matchEventService.updateMatchEvent(updatedEvent); } + BallScoreModel? _getLastWicketBall() { + return state.currentScoresList.lastWhereOrNull( + (ball) => + ball.wicket_type != WicketType.retired && + ball.wicket_type != WicketType.retiredHurt && + ball.wicket_type != WicketType.timedOut, + ); + }
1239-1310: Enhance error handling in undo operationsWhile the undo operations are comprehensive, they could benefit from better error handling and recovery mechanisms.
Consider adding transaction-like behavior:
Future<void> undoMatchEventIfNeeded(BallScoreModel ball) async { + try { final events = matchEvents.where((element) => element.ball_ids.contains(ball.id)); + List<Future> operations = []; for (final event in events) { if (event.type == EventType.six || event.type == EventType.four) { - await _matchEventService.deleteMatchEvent(event.id); + operations.add(_matchEventService.deleteMatchEvent(event.id)); } else if (event.type == EventType.hatTrick || event.type == EventType.wicket) { // ... existing code ... } } + await Future.wait(operations); + } catch (e) { + debugPrint("Error during match event undo: $e"); + // Consider adding recovery mechanism or state rollback + rethrow; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
khelo/lib/ui/flow/score_board/score_board_view_model.dart(13 hunks)
🔇 Additional comments (1)
khelo/lib/ui/flow/score_board/score_board_view_model.dart (1)
676-679: LGTM! Verify event tracking integration
The match event and partnership tracking is properly integrated into the ball scoring flow, with good error isolation.
✅ Verification successful
Let me analyze the integration points further by checking the implementation details of these methods.
Let me try a different approach using ripgrep to see the full implementation of these methods.
Let me try one last approach with simpler ripgrep patterns to see the method implementations.
Integration of match events and partnerships is properly implemented
The code shows correct implementation with:
addMatchEventIfNeeded: Records special events (fours, sixes, wickets) with proper error isolationaddMatchPartnershipIfNeeded: Tracks partnership stats between batsmen when wickets fall- Both methods are called in the right order within the ball scoring flow
- Error handling is in place with try-catch blocks
The integration points and implementation details confirm that the match event and partnership tracking is correctly integrated into the scoring system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the integration points for match events
rg -A 5 "addMatchEventIfNeeded|addMatchPartnershipIfNeeded" --type dart
Length of output: 3239
Script:
#!/bin/bash
# Check the implementation of addMatchEventIfNeeded and addMatchPartnershipIfNeeded
ast-grep --pattern $'Future<void> addMatchEventIfNeeded(BallScoreModel ball) async {
$$$
}'
ast-grep --pattern $'Future<void> addMatchPartnershipIfNeeded(BallScoreModel ball) async {
$$$
}'
Length of output: 195
Script:
#!/bin/bash
# Get the full implementation of both methods with more context
rg -U "Future<void> addMatchEventIfNeeded.*?\n}" --multiline --type dart -A 20
rg -U "Future<void> addMatchPartnershipIfNeeded.*?\n}" --multiline --type dart -A 20
Length of output: 167
Script:
#!/bin/bash
# Get method implementations with surrounding context
rg -B 2 -A 15 "^ Future<void> addMatchEventIfNeeded" --type dart
rg -B 2 -A 15 "^ Future<void> addMatchPartnershipIfNeeded" --type dart
Length of output: 3441
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
khelo/lib/ui/flow/score_board/score_board_view_model.dart (4)
724-727: Ensure consistent error handling in asynchronous methodsIn methods like
addMatchEventIfNeeded(lines 724-727) andundoPartnerShipIfNeeded(lines 1296-1307), exceptions are caught and only logged usingdebugPrint. To prevent silent failures and enhance reliability, consider updating the state with the error or implementing proper error handling mechanisms.Also applies to: 1296-1307
704-883: Consider adding unit tests for event handling methodsThe methods dealing with match events and partnerships, such as
addMatchEventIfNeeded,handleWicketMatchEvent,handleMilestonesMatchEvent, andaddMatchPartnershipIfNeeded, are critical for the application's functionality. Adding unit tests for these methods would help ensure their correctness and robustness, and can catch potential issues early in the development process.
Line range hint
12-38: Avoid redundant null checks for non-nullable fieldsIn the
_loadMatchSettingmethod, thematchIdvariable is already checked for nullity at the beginning. However, in later lines,matchId!is used without additional benefits. SincematchIdcannot be null beyond the initial check, consider removing the unnecessary null assertion operators to improve code clarity.
Line range hint
676-883: Optimize performance by reducing redundant calculationsIn the
addMatchPartnershipIfNeededmethod, the statistics for batsmen are recalculated multiple times within loops. Consider refactoring the code to compute these statistics once and reuse them, which can improve performance, especially with larger data sets.data/lib/api/match_event/match_event_model.dart (2)
27-28: Maintain consistency in naming plural list propertiesThe
MatchEventModelclass has a property namedmilestoneof typeList<MatchEventMilestone>. To maintain consistency with other list properties likeball_idsandwickets, consider renamingmilestonetomilestones.
80-87: Specify default values for all fields inMatchEventMilestoneIn the
MatchEventMilestoneclass, some fields likeover,runs,ball_faced,fours, andsixeshave default values, while others liketimeandball_iddo not. For consistency and to prevent potential null errors, ensure that default values are specified for all fields where appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
data/lib/api/match_event/match_event_model.dart(1 hunks)data/lib/api/match_event/match_event_model.freezed.dart(1 hunks)data/lib/api/match_event/match_event_model.g.dart(1 hunks)khelo/lib/ui/flow/score_board/score_board_view_model.dart(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- data/lib/api/match_event/match_event_model.g.dart
🔇 Additional comments (3)
data/lib/api/match_event/match_event_model.freezed.dart (1)
1-984: Generated code appears correct
This is an auto-generated file by the Freezed package. The generated code aligns with the model definitions and shows no issues.
khelo/lib/ui/flow/score_board/score_board_view_model.dart (2)
179-192: Handle exceptions appropriately in _loadMatchEventAndPartnership
In the onError callback of _loadMatchEventAndPartnership, the exception is only logged using debugPrint, but the error is not propagated or the state updated. This could lead to silent failures without informing the user. Consider updating the state with the error or handling it appropriately.
1889-1891:
Ensure all subscriptions are properly cancelled
In the _cancelStreamSubscription method, the _matchSettingSubscription is cancelled, but the _matchEventSubscription is also used in the class. Ensure that _matchEventSubscription is cancelled as well to prevent memory leaks.
Apply this diff to fix the issue:
_cancelStreamSubscription() async {
await _matchStreamSubscription?.cancel();
await _ballScoreStreamSubscription?.cancel();
await _matchSettingSubscription?.cancel();
+ await _matchEventSubscription?.cancel();
await _matchStreamController.close();
}Likely invalid or redundant comment.
Record Match Event and Partnership
Test steps:
Add Match and start scoring.
get Id from database and apply filter with match_id in database.
check entry as per type in database as you add or undo any major event.
Note:
while this data isn't used in UI right now, we'll incorporate it into designs soon.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation