From 57ecb5f7ba97704754c4d110497ec084f5d46f46 Mon Sep 17 00:00:00 2001 From: Miguel Beltran Date: Thu, 5 Dec 2024 19:16:04 +0100 Subject: [PATCH] refactor Result class, remove asOk and asError (#2542) As discussed in the PR for the Result pattern implementation (https://github.com/flutter/website/pull/11444) @parlough recommended that `asError` and `asOk` should be not be used, and instead we should use proper exhaustiveness checking. This PR removes the two "convenience" methods and refactors code. In some cases, it was enough with writing a proper `if` clause, while in others it was necessary to use a `switch`. Still, they are present in the `testing` folder, as they can be useful for testing purposes. ## Pre-launch Checklist - [x] I read the [Flutter Style Guide] _recently_, and have followed its advice. - [x] I signed the [CLA]. - [x] I read the [Contributors Guide]. - [x] I have added sample code updates to the [changelog]. - [x] I updated/added relevant documentation (doc comments with `///`). If you need help, consider asking for advice on the #hackers-devrel channel on [Discord]. [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [Contributors Guide]: https://github.com/flutter/samples/blob/main/CONTRIBUTING.md [changelog]: ../CHANGELOG.md --- .../activity/activity_repository_remote.dart | 4 +- .../booking/booking_repository_remote.dart | 53 +++++++++++-------- .../continent_repository_remote.dart | 4 +- .../destination_repository_remote.dart | 4 +- .../booking/booking_create_use_case.dart | 22 ++++---- .../view_models/activities_viewmodel.dart | 37 +++++++------ .../view_models/booking_viewmodel.dart | 2 +- .../view_models/results_viewmodel.dart | 34 ++++++------ .../view_models/search_form_viewmodel.dart | 44 +++++++-------- compass_app/app/lib/utils/result.dart | 6 --- .../activity_repository_local_test.dart | 2 + .../activity_repository_remote_test.dart | 1 + .../booking_repository_remote_test.dart | 1 + .../continent_repository_remote_test.dart | 1 + .../destination_repository_local_test.dart | 2 + .../destination_repository_remote_test.dart | 1 + .../data/services/api/api_client_test.dart | 1 + .../services/api/auth_api_client_test.dart | 1 + .../booking/booking_create_use_case_test.dart | 1 + compass_app/app/test/utils/command_test.dart | 4 +- compass_app/app/testing/utils/result.dart | 9 ++++ 21 files changed, 131 insertions(+), 103 deletions(-) create mode 100644 compass_app/app/testing/utils/result.dart diff --git a/compass_app/app/lib/data/repositories/activity/activity_repository_remote.dart b/compass_app/app/lib/data/repositories/activity/activity_repository_remote.dart index 6d2d0bcc5..be7ce02c6 100644 --- a/compass_app/app/lib/data/repositories/activity/activity_repository_remote.dart +++ b/compass_app/app/lib/data/repositories/activity/activity_repository_remote.dart @@ -24,8 +24,8 @@ class ActivityRepositoryRemote implements ActivityRepository { if (!_cachedData.containsKey(ref)) { // No cached data, request activities final result = await _apiClient.getActivityByDestination(ref); - if (result is Ok) { - _cachedData[ref] = result.asOk.value; + if (result is Ok>) { + _cachedData[ref] = result.value; } return result; } else { diff --git a/compass_app/app/lib/data/repositories/booking/booking_repository_remote.dart b/compass_app/app/lib/data/repositories/booking/booking_repository_remote.dart index 1ac1cd7cd..cdb66b86d 100644 --- a/compass_app/app/lib/data/repositories/booking/booking_repository_remote.dart +++ b/compass_app/app/lib/data/repositories/booking/booking_repository_remote.dart @@ -42,18 +42,22 @@ class BookingRepositoryRemote implements BookingRepository { try { // Get booking by ID from server final resultBooking = await _apiClient.getBooking(id); - if (resultBooking is Error) { - return Result.error(resultBooking.error); + switch (resultBooking) { + case Error(): + return Result.error(resultBooking.error); + case Ok(): } - final booking = resultBooking.asOk.value; + final booking = resultBooking.value; // Load destinations if not loaded yet if (_cachedDestinations == null) { final resultDestination = await _apiClient.getDestinations(); - if (resultDestination is Error>) { - return Result.error(resultDestination.error); + switch (resultDestination) { + case Error>(): + return Result.error(resultDestination.error); + case Ok>(): } - _cachedDestinations = resultDestination.asOk.value; + _cachedDestinations = resultDestination.value; } // Get destination for booking @@ -62,11 +66,12 @@ class BookingRepositoryRemote implements BookingRepository { final resultActivities = await _apiClient.getActivityByDestination(destination.ref); - - if (resultActivities is Error>) { - return Result.error(resultActivities.error); + switch (resultActivities) { + case Error>(): + return Result.error(resultActivities.error); + case Ok>(): } - final activities = resultActivities.asOk.value + final activities = resultActivities.value .where((activity) => booking.activitiesRef.contains(activity.ref)) .toList(); @@ -88,20 +93,22 @@ class BookingRepositoryRemote implements BookingRepository { Future>> getBookingsList() async { try { final result = await _apiClient.getBookings(); - if (result is Error>) { - return Result.error(result.error); + switch (result) { + case Ok>(): + final bookingsApi = result.value; + return Result.ok(bookingsApi + .map( + (bookingApi) => BookingSummary( + id: bookingApi.id!, + name: bookingApi.name, + startDate: bookingApi.startDate, + endDate: bookingApi.endDate, + ), + ) + .toList()); + case Error>(): + return Result.error(result.error); } - final bookingsApi = result.asOk.value; - return Result.ok(bookingsApi - .map( - (bookingApi) => BookingSummary( - id: bookingApi.id!, - name: bookingApi.name, - startDate: bookingApi.startDate, - endDate: bookingApi.endDate, - ), - ) - .toList()); } on Exception catch (e) { return Result.error(e); } diff --git a/compass_app/app/lib/data/repositories/continent/continent_repository_remote.dart b/compass_app/app/lib/data/repositories/continent/continent_repository_remote.dart index 58a6e439e..a9b32f686 100644 --- a/compass_app/app/lib/data/repositories/continent/continent_repository_remote.dart +++ b/compass_app/app/lib/data/repositories/continent/continent_repository_remote.dart @@ -24,9 +24,9 @@ class ContinentRepositoryRemote implements ContinentRepository { if (_cachedData == null) { // No cached data, request continents final result = await _apiClient.getContinents(); - if (result is Ok) { + if (result is Ok>) { // Store value if result Ok - _cachedData = result.asOk.value; + _cachedData = result.value; } return result; } else { diff --git a/compass_app/app/lib/data/repositories/destination/destination_repository_remote.dart b/compass_app/app/lib/data/repositories/destination/destination_repository_remote.dart index 191b2a6ed..a2357ba90 100644 --- a/compass_app/app/lib/data/repositories/destination/destination_repository_remote.dart +++ b/compass_app/app/lib/data/repositories/destination/destination_repository_remote.dart @@ -24,9 +24,9 @@ class DestinationRepositoryRemote implements DestinationRepository { if (_cachedData == null) { // No cached data, request destinations final result = await _apiClient.getDestinations(); - if (result is Ok) { + if (result is Ok>) { // Store value if result Ok - _cachedData = result.asOk.value; + _cachedData = result.value; } return result; } else { diff --git a/compass_app/app/lib/domain/use_cases/booking/booking_create_use_case.dart b/compass_app/app/lib/domain/use_cases/booking/booking_create_use_case.dart index 4b750f0a3..001ed7fc1 100644 --- a/compass_app/app/lib/domain/use_cases/booking/booking_create_use_case.dart +++ b/compass_app/app/lib/domain/use_cases/booking/booking_create_use_case.dart @@ -40,11 +40,13 @@ class BookingCreateUseCase { } final destinationResult = await _fetchDestination(itineraryConfig.destination!); - if (destinationResult is Error) { - _log.warning('Error fetching destination: ${destinationResult.error}'); - return Result.error(destinationResult.error); + switch (destinationResult) { + case Ok(): + _log.fine('Destination loaded: ${destinationResult.value.ref}'); + case Error(): + _log.warning('Error fetching destination: ${destinationResult.error}'); + return Result.error(destinationResult.error); } - _log.fine('Destination loaded: ${destinationResult.asOk.value.ref}'); // Get Activity objects from repository if (itineraryConfig.activities.isEmpty) { @@ -54,11 +56,13 @@ class BookingCreateUseCase { final activitiesResult = await _activityRepository.getByDestination( itineraryConfig.destination!, ); - if (activitiesResult is Error>) { - _log.warning('Error fetching activities: ${activitiesResult.error}'); - return Result.error(activitiesResult.error); + switch (activitiesResult) { + case Error>(): + _log.warning('Error fetching activities: ${activitiesResult.error}'); + return Result.error(activitiesResult.error); + case Ok>(): } - final activities = activitiesResult.asOk.value + final activities = activitiesResult.value .where( (activity) => itineraryConfig.activities.contains(activity.ref), ) @@ -74,7 +78,7 @@ class BookingCreateUseCase { final booking = Booking( startDate: itineraryConfig.startDate!, endDate: itineraryConfig.endDate!, - destination: destinationResult.asOk.value, + destination: destinationResult.value, activity: activities, ); diff --git a/compass_app/app/lib/ui/activities/view_models/activities_viewmodel.dart b/compass_app/app/lib/ui/activities/view_models/activities_viewmodel.dart index dc86b6f48..5c6fa4af9 100644 --- a/compass_app/app/lib/ui/activities/view_models/activities_viewmodel.dart +++ b/compass_app/app/lib/ui/activities/view_models/activities_viewmodel.dart @@ -8,6 +8,7 @@ import 'package:logging/logging.dart'; import '../../../data/repositories/activity/activity_repository.dart'; import '../../../data/repositories/itinerary_config/itinerary_config_repository.dart'; import '../../../domain/models/activity/activity.dart'; +import '../../../domain/models/itinerary_config/itinerary_config.dart'; import '../../../utils/command.dart'; import '../../../utils/result.dart'; @@ -45,21 +46,23 @@ class ActivitiesViewModel extends ChangeNotifier { Future> _loadActivities() async { final result = await _itineraryConfigRepository.getItineraryConfig(); - if (result is Error) { - _log.warning( - 'Failed to load stored ItineraryConfig', - result.asError.error, - ); - return result; + switch (result) { + case Error(): + _log.warning( + 'Failed to load stored ItineraryConfig', + result.error, + ); + return result; + case Ok(): } - final destinationRef = result.asOk.value.destination; + final destinationRef = result.value.destination; if (destinationRef == null) { _log.severe('Destination missing in ItineraryConfig'); return Result.error(Exception('Destination not found')); } - _selectedActivities.addAll(result.asOk.value.activities); + _selectedActivities.addAll(result.value.activities); final resultActivities = await _activityRepository.getByDestination(destinationRef); @@ -120,21 +123,23 @@ class ActivitiesViewModel extends ChangeNotifier { Future> _saveActivities() async { final resultConfig = await _itineraryConfigRepository.getItineraryConfig(); - if (resultConfig is Error) { - _log.warning( - 'Failed to load stored ItineraryConfig', - resultConfig.asError.error, - ); - return resultConfig; + switch (resultConfig) { + case Error(): + _log.warning( + 'Failed to load stored ItineraryConfig', + resultConfig.error, + ); + return resultConfig; + case Ok(): } - final itineraryConfig = resultConfig.asOk.value; + final itineraryConfig = resultConfig.value; final result = await _itineraryConfigRepository.setItineraryConfig( itineraryConfig.copyWith(activities: _selectedActivities.toList())); if (result is Error) { _log.warning( 'Failed to store ItineraryConfig', - result.asError.error, + result.error, ); } return result; diff --git a/compass_app/app/lib/ui/booking/view_models/booking_viewmodel.dart b/compass_app/app/lib/ui/booking/view_models/booking_viewmodel.dart index 725d478eb..f32d1a4bb 100644 --- a/compass_app/app/lib/ui/booking/view_models/booking_viewmodel.dart +++ b/compass_app/app/lib/ui/booking/view_models/booking_viewmodel.dart @@ -65,7 +65,7 @@ class BookingViewModel extends ChangeNotifier { case Error(): _log.warning('Booking error: ${result.error}'); notifyListeners(); - return Result.error(result.asError.error); + return Result.error(result.error); } case Error(): _log.warning('ItineraryConfig error: ${itineraryConfig.error}'); diff --git a/compass_app/app/lib/ui/results/view_models/results_viewmodel.dart b/compass_app/app/lib/ui/results/view_models/results_viewmodel.dart index 5a2690ab9..94d1f5d2d 100644 --- a/compass_app/app/lib/ui/results/view_models/results_viewmodel.dart +++ b/compass_app/app/lib/ui/results/view_models/results_viewmodel.dart @@ -50,14 +50,16 @@ class ResultsViewModel extends ChangeNotifier { Future> _search() async { // Load current itinerary config final resultConfig = await _itineraryConfigRepository.getItineraryConfig(); - if (resultConfig is Error) { - _log.warning( - 'Failed to load stored ItineraryConfig', - resultConfig.asError.error, - ); - return resultConfig; + switch (resultConfig) { + case Error(): + _log.warning( + 'Failed to load stored ItineraryConfig', + resultConfig.error, + ); + return resultConfig; + case Ok(): } - _itineraryConfig = resultConfig.asOk.value; + _itineraryConfig = resultConfig.value; notifyListeners(); final result = await _destinationRepository.getDestinations(); @@ -86,15 +88,17 @@ class ResultsViewModel extends ChangeNotifier { assert(destinationRef.isNotEmpty, "destinationRef should not be empty"); final resultConfig = await _itineraryConfigRepository.getItineraryConfig(); - if (resultConfig is Error) { - _log.warning( - 'Failed to load stored ItineraryConfig', - resultConfig.asError.error, - ); - return resultConfig; + switch (resultConfig) { + case Error(): + _log.warning( + 'Failed to load stored ItineraryConfig', + resultConfig.error, + ); + return resultConfig; + case Ok(): } - final itineraryConfig = resultConfig.asOk.value; + final itineraryConfig = resultConfig.value; final result = await _itineraryConfigRepository .setItineraryConfig(itineraryConfig.copyWith( destination: destinationRef, @@ -103,7 +107,7 @@ class ResultsViewModel extends ChangeNotifier { if (result is Error) { _log.warning( 'Failed to store ItineraryConfig', - result.asError.error, + result.error, ); } return result; diff --git a/compass_app/app/lib/ui/search_form/view_models/search_form_viewmodel.dart b/compass_app/app/lib/ui/search_form/view_models/search_form_viewmodel.dart index 565e5fc0f..c1eed2f92 100644 --- a/compass_app/app/lib/ui/search_form/view_models/search_form_viewmodel.dart +++ b/compass_app/app/lib/ui/search_form/view_models/search_form_viewmodel.dart @@ -99,14 +99,10 @@ class SearchFormViewModel extends ChangeNotifier { final result = await _continentRepository.getContinents(); switch (result) { case Ok(): - { - _continents = result.value; - _log.fine('Continents (${_continents.length}) loaded'); - } + _continents = result.value; + _log.fine('Continents (${_continents.length}) loaded'); case Error(): - { - _log.warning('Failed to load continents', result.asError.error); - } + _log.warning('Failed to load continents', result.error); } notifyListeners(); return result; @@ -116,27 +112,23 @@ class SearchFormViewModel extends ChangeNotifier { final result = await _itineraryConfigRepository.getItineraryConfig(); switch (result) { case Ok(): - { - final itineraryConfig = result.value; - _selectedContinent = itineraryConfig.continent; - if (itineraryConfig.startDate != null && - itineraryConfig.endDate != null) { - _dateRange = DateTimeRange( - start: itineraryConfig.startDate!, - end: itineraryConfig.endDate!, - ); - } - _guests = itineraryConfig.guests ?? 0; - _log.fine('ItineraryConfig loaded'); - notifyListeners(); - } - case Error(): - { - _log.warning( - 'Failed to load stored ItineraryConfig', - result.asError.error, + final itineraryConfig = result.value; + _selectedContinent = itineraryConfig.continent; + if (itineraryConfig.startDate != null && + itineraryConfig.endDate != null) { + _dateRange = DateTimeRange( + start: itineraryConfig.startDate!, + end: itineraryConfig.endDate!, ); } + _guests = itineraryConfig.guests ?? 0; + _log.fine('ItineraryConfig loaded'); + notifyListeners(); + case Error(): + _log.warning( + 'Failed to load stored ItineraryConfig', + result.error, + ); } return result; } diff --git a/compass_app/app/lib/utils/result.dart b/compass_app/app/lib/utils/result.dart index 8512963e4..407b11ca1 100644 --- a/compass_app/app/lib/utils/result.dart +++ b/compass_app/app/lib/utils/result.dart @@ -23,12 +23,6 @@ sealed class Result { /// Creates an error [Result], completed with the specified [error]. const factory Result.error(Exception error) = Error._; - - /// Convenience method to cast to Ok - Ok get asOk => this as Ok; - - /// Convenience method to cast to Error - Error get asError => this as Error; } /// Subclass of Result for values diff --git a/compass_app/app/test/data/repositories/activity/activity_repository_local_test.dart b/compass_app/app/test/data/repositories/activity/activity_repository_local_test.dart index 739f35084..a49702f9c 100644 --- a/compass_app/app/test/data/repositories/activity/activity_repository_local_test.dart +++ b/compass_app/app/test/data/repositories/activity/activity_repository_local_test.dart @@ -7,6 +7,8 @@ import 'package:compass_app/data/services/local/local_data_service.dart'; import 'package:compass_app/utils/result.dart'; import 'package:flutter_test/flutter_test.dart'; +import '../../../../testing/utils/result.dart'; + void main() { group('ActivityRepositoryLocal tests', () { // To load assets diff --git a/compass_app/app/test/data/repositories/activity/activity_repository_remote_test.dart b/compass_app/app/test/data/repositories/activity/activity_repository_remote_test.dart index a75734e54..18cb5a5e7 100644 --- a/compass_app/app/test/data/repositories/activity/activity_repository_remote_test.dart +++ b/compass_app/app/test/data/repositories/activity/activity_repository_remote_test.dart @@ -8,6 +8,7 @@ import 'package:compass_app/utils/result.dart'; import 'package:flutter_test/flutter_test.dart'; import '../../../../testing/fakes/services/fake_api_client.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('ActivityRepositoryRemote tests', () { diff --git a/compass_app/app/test/data/repositories/booking/booking_repository_remote_test.dart b/compass_app/app/test/data/repositories/booking/booking_repository_remote_test.dart index 54b907c1d..822405060 100644 --- a/compass_app/app/test/data/repositories/booking/booking_repository_remote_test.dart +++ b/compass_app/app/test/data/repositories/booking/booking_repository_remote_test.dart @@ -9,6 +9,7 @@ import 'package:flutter_test/flutter_test.dart'; import '../../../../testing/fakes/services/fake_api_client.dart'; import '../../../../testing/models/booking.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('BookingRepositoryRemote tests', () { diff --git a/compass_app/app/test/data/repositories/continent/continent_repository_remote_test.dart b/compass_app/app/test/data/repositories/continent/continent_repository_remote_test.dart index 4a41120b7..37ae24bac 100644 --- a/compass_app/app/test/data/repositories/continent/continent_repository_remote_test.dart +++ b/compass_app/app/test/data/repositories/continent/continent_repository_remote_test.dart @@ -8,6 +8,7 @@ import 'package:compass_app/utils/result.dart'; import 'package:flutter_test/flutter_test.dart'; import '../../../../testing/fakes/services/fake_api_client.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('ContinentRepositoryRemote tests', () { diff --git a/compass_app/app/test/data/repositories/destination/destination_repository_local_test.dart b/compass_app/app/test/data/repositories/destination/destination_repository_local_test.dart index 62fa01357..f53cf77b4 100644 --- a/compass_app/app/test/data/repositories/destination/destination_repository_local_test.dart +++ b/compass_app/app/test/data/repositories/destination/destination_repository_local_test.dart @@ -7,6 +7,8 @@ import 'package:compass_app/utils/result.dart'; import 'package:compass_app/data/repositories/destination/destination_repository_local.dart'; import 'package:flutter_test/flutter_test.dart'; +import '../../../../testing/utils/result.dart'; + void main() { group('DestinationRepositoryLocal tests', () { // To load assets diff --git a/compass_app/app/test/data/repositories/destination/destination_repository_remote_test.dart b/compass_app/app/test/data/repositories/destination/destination_repository_remote_test.dart index 2559f030e..d918f8789 100644 --- a/compass_app/app/test/data/repositories/destination/destination_repository_remote_test.dart +++ b/compass_app/app/test/data/repositories/destination/destination_repository_remote_test.dart @@ -8,6 +8,7 @@ import 'package:compass_app/utils/result.dart'; import 'package:flutter_test/flutter_test.dart'; import '../../../../testing/fakes/services/fake_api_client.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('DestinationRepositoryRemote tests', () { diff --git a/compass_app/app/test/data/services/api/api_client_test.dart b/compass_app/app/test/data/services/api/api_client_test.dart index e3965e4e3..2bcf8de3f 100644 --- a/compass_app/app/test/data/services/api/api_client_test.dart +++ b/compass_app/app/test/data/services/api/api_client_test.dart @@ -12,6 +12,7 @@ import '../../../../testing/models/activity.dart'; import '../../../../testing/models/booking.dart'; import '../../../../testing/models/destination.dart'; import '../../../../testing/models/user.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('ApiClient', () { diff --git a/compass_app/app/test/data/services/api/auth_api_client_test.dart b/compass_app/app/test/data/services/api/auth_api_client_test.dart index 50d20e561..18222ba62 100644 --- a/compass_app/app/test/data/services/api/auth_api_client_test.dart +++ b/compass_app/app/test/data/services/api/auth_api_client_test.dart @@ -8,6 +8,7 @@ import 'package:compass_app/data/services/api/model/login_response/login_respons import 'package:flutter_test/flutter_test.dart'; import '../../../../testing/mocks.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('AuthApiClient', () { diff --git a/compass_app/app/test/domain/use_cases/booking/booking_create_use_case_test.dart b/compass_app/app/test/domain/use_cases/booking/booking_create_use_case_test.dart index e148eb582..6e22287d1 100644 --- a/compass_app/app/test/domain/use_cases/booking/booking_create_use_case_test.dart +++ b/compass_app/app/test/domain/use_cases/booking/booking_create_use_case_test.dart @@ -12,6 +12,7 @@ import '../../../../testing/fakes/repositories/fake_destination_repository.dart' import '../../../../testing/models/activity.dart'; import '../../../../testing/models/booking.dart'; import '../../../../testing/models/destination.dart'; +import '../../../../testing/utils/result.dart'; void main() { group('BookingCreateUseCase tests', () { diff --git a/compass_app/app/test/utils/command_test.dart b/compass_app/app/test/utils/command_test.dart index a2b1c8870..964baf403 100644 --- a/compass_app/app/test/utils/command_test.dart +++ b/compass_app/app/test/utils/command_test.dart @@ -6,6 +6,8 @@ import 'package:compass_app/utils/command.dart'; import 'package:compass_app/utils/result.dart'; import 'package:flutter_test/flutter_test.dart'; +import '../../testing/utils/result.dart'; + void main() { group('Command0 tests', () { test('should complete void command', () async { @@ -89,7 +91,7 @@ void main() { test('should complete bool command, bool argument', () async { // Action that returns bool argument final command = - Command1((a) => Future.value(Result.ok(true))); + Command1((a) => Future.value(const Result.ok(true))); // Run action with result and argument await command.execute(true); diff --git a/compass_app/app/testing/utils/result.dart b/compass_app/app/testing/utils/result.dart new file mode 100644 index 000000000..457003cb5 --- /dev/null +++ b/compass_app/app/testing/utils/result.dart @@ -0,0 +1,9 @@ +import 'package:compass_app/utils/result.dart'; + +extension ResultCast on Result { + /// Convenience method to cast to Ok + Ok get asOk => this as Ok; + + /// Convenience method to cast to Error + Error get asError => this as Error; +}