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
2025-11-08 13:58:47 +00:00 · 2024-12-05 19:16:04 +01:00
parent 5adcda3640
commit 57ecb5f7ba
21 changed files with 131 additions and 103 deletions
--- 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<List<Activity>>) {
+        _cachedData[ref] = result.value;
      }
      return result;
    } else {
--- 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<BookingApiModel>) {
+      switch (resultBooking) {
+        case Error<BookingApiModel>():
          return Result.error(resultBooking.error);
+        case Ok<BookingApiModel>():
      }
-      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<List<Destination>>) {
+        switch (resultDestination) {
+          case Error<List<Destination>>():
            return Result.error(resultDestination.error);
+          case Ok<List<Destination>>():
        }
-        _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<List<Activity>>) {
+      switch (resultActivities) {
+        case Error<List<Activity>>():
          return Result.error(resultActivities.error);
+        case Ok<List<Activity>>():
      }
-      final activities = resultActivities.asOk.value
+      final activities = resultActivities.value
          .where((activity) => booking.activitiesRef.contains(activity.ref))
          .toList();

@@ -88,10 +93,9 @@ class BookingRepositoryRemote implements BookingRepository {
  Future<Result<List<BookingSummary>>> getBookingsList() async {
    try {
      final result = await _apiClient.getBookings();
-      if (result is Error<List<BookingApiModel>>) {
-        return Result.error(result.error);
-      }
-      final bookingsApi = result.asOk.value;
+      switch (result) {
+        case Ok<List<BookingApiModel>>():
+          final bookingsApi = result.value;
          return Result.ok(bookingsApi
              .map(
                (bookingApi) => BookingSummary(
@@ -102,6 +106,9 @@ class BookingRepositoryRemote implements BookingRepository {
                ),
              )
              .toList());
+        case Error<List<BookingApiModel>>():
+          return Result.error(result.error);
+      }
    } on Exception catch (e) {
      return Result.error(e);
    }
--- 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<List<Continent>>) {
        // Store value if result Ok
-        _cachedData = result.asOk.value;
+        _cachedData = result.value;
      }
      return result;
    } else {
--- 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<List<Destination>>) {
        // Store value if result Ok
-        _cachedData = result.asOk.value;
+        _cachedData = result.value;
      }
      return result;
    } else {
--- 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<Destination>) {
+    switch (destinationResult) {
+      case Ok<Destination>():
+        _log.fine('Destination loaded: ${destinationResult.value.ref}');
+      case Error<Destination>():
        _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<List<Activity>>) {
+    switch (activitiesResult) {
+      case Error<List<Activity>>():
        _log.warning('Error fetching activities: ${activitiesResult.error}');
        return Result.error(activitiesResult.error);
+      case Ok<List<Activity>>():
    }
-    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,
    );

--- 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<Result<void>> _loadActivities() async {
    final result = await _itineraryConfigRepository.getItineraryConfig();
-    if (result is Error) {
+    switch (result) {
+      case Error<ItineraryConfig>():
        _log.warning(
          'Failed to load stored ItineraryConfig',
-        result.asError.error,
+          result.error,
        );
        return result;
+      case Ok<ItineraryConfig>():
    }

-    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<Result<void>> _saveActivities() async {
    final resultConfig = await _itineraryConfigRepository.getItineraryConfig();
-    if (resultConfig is Error) {
+    switch (resultConfig) {
+      case Error<ItineraryConfig>():
        _log.warning(
          'Failed to load stored ItineraryConfig',
-        resultConfig.asError.error,
+          resultConfig.error,
        );
        return resultConfig;
+      case Ok<ItineraryConfig>():
    }

-    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;
--- 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<Booking>():
            _log.warning('Booking error: ${result.error}');
            notifyListeners();
-            return Result.error(result.asError.error);
+            return Result.error(result.error);
        }
      case Error<ItineraryConfig>():
        _log.warning('ItineraryConfig error: ${itineraryConfig.error}');
--- 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<Result<void>> _search() async {
    // Load current itinerary config
    final resultConfig = await _itineraryConfigRepository.getItineraryConfig();
-    if (resultConfig is Error) {
+    switch (resultConfig) {
+      case Error<ItineraryConfig>():
        _log.warning(
          'Failed to load stored ItineraryConfig',
-        resultConfig.asError.error,
+          resultConfig.error,
        );
        return resultConfig;
+      case Ok<ItineraryConfig>():
    }
-    _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) {
+    switch (resultConfig) {
+      case Error<ItineraryConfig>():
        _log.warning(
          'Failed to load stored ItineraryConfig',
-        resultConfig.asError.error,
+          resultConfig.error,
        );
        return resultConfig;
+      case Ok<ItineraryConfig>():
    }

-    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;
--- 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');
-        }
      case Error():
-        {
-          _log.warning('Failed to load continents', result.asError.error);
-        }
+        _log.warning('Failed to load continents', result.error);
    }
    notifyListeners();
    return result;
@@ -116,7 +112,6 @@ class SearchFormViewModel extends ChangeNotifier {
    final result = await _itineraryConfigRepository.getItineraryConfig();
    switch (result) {
      case Ok<ItineraryConfig>():
-        {
        final itineraryConfig = result.value;
        _selectedContinent = itineraryConfig.continent;
        if (itineraryConfig.startDate != null &&
@@ -129,15 +124,12 @@ class SearchFormViewModel extends ChangeNotifier {
        _guests = itineraryConfig.guests ?? 0;
        _log.fine('ItineraryConfig loaded');
        notifyListeners();
-        }
      case Error<ItineraryConfig>():
-        {
        _log.warning(
          'Failed to load stored ItineraryConfig',
-            result.asError.error,
+          result.error,
        );
    }
-    }
    return result;
  }

--- a/compass_app/app/lib/utils/result.dart
+++ b/compass_app/app/lib/utils/result.dart
@@ -23,12 +23,6 @@ sealed class Result<T> {

  /// Creates an error [Result], completed with the specified [error].
  const factory Result.error(Exception error) = Error._;
-
-  /// Convenience method to cast to Ok
-  Ok<T> get asOk => this as Ok<T>;
-
-  /// Convenience method to cast to Error
-  Error get asError => this as Error<T>;
 }

 /// Subclass of Result for values
--- 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
--- 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', () {
--- 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', () {
--- 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', () {
--- 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
--- 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', () {
--- 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', () {
--- 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', () {
--- 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', () {
--- 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<bool, bool>((a) => Future.value(Result.ok(true)));
+          Command1<bool, bool>((a) => Future.value(const Result.ok(true)));

      // Run action with result and argument
      await command.execute(true);
--- a/compass_app/app/testing/utils/result.dart
+++ b/compass_app/app/testing/utils/result.dart
@@ -0,0 +1,9 @@
+import 'package:compass_app/utils/result.dart';
+
+extension ResultCast<T> on Result<T> {
+  /// Convenience method to cast to Ok
+  Ok<T> get asOk => this as Ok<T>;
+
+  /// Convenience method to cast to Error
+  Error get asError => this as Error<T>;
+}