chore: refactoring to `bloc` advices needed

[ethicnology]

Description

I'm working on refactoring an app and to BLoC / Cubit architecture.

The code provided works fine, but I need advices to improve this refactoring according to BLoC principles, before I start to refactor all the project this way. My main goals are to make it easy to developers to switch to this new architecture and as readable as possible.

This is the first time I'm using bloc

original version spot_widget.dart

import 'package:project/models/latlon.dart';
import 'package:project/models/spot_info.dart';
import 'package:project/models/spot_review.dart';
import 'package:flutter/widgets.dart';

class SpotWidget extends StatefulWidget {
  final LatLon location;
  const SpotWidget({super.key, required this.location});

  @override
  State<SpotWidget> createState() => _SpotWidgetState();
}

class _SpotWidgetState extends State<SpotWidget> {
  SpotInfo? info;
  List<SpotReview> reviews = [];

  Future<void> _loadInfo() async {
    final infos = await SpotInfo.get(widget.location);
    info = infos.first;
    setState(() {});
  }

  Future<void> _loadReviews() async {
    reviews = await SpotReview.get(widget.location);
    setState(() {});
  }

  @override
  void initState() {
    _loadInfo();
    _loadReviews();
    super.initState();
  }

  @override
  Widget build(BuildContext context) {
    return Column(
      children: [
        Text(widget.location.toString()),
        // TODO: Widget to display all informations about a spot
        if (info != null) Text(info!.isPublic.toString()),
        // TODO: Widget to display all reviews about a spot
        if (reviews.isNotEmpty) Text(reviews.length.toString()),
      ],
    );
  }
}

BLoC refactoring

spot_repository.dart

import 'package:project/models/latlon.dart';
import 'package:project/models/spot_info.dart';
import 'package:project/models/spot_review.dart';

class SpotRepository {
  Future<SpotInfo> getSpotInfo(LatLon location) async {
    final infos = await SpotInfo.get(location);
    return infos.first;
  }

  Future<List<SpotReview>> getSpotReviews(LatLon location) async {
    return await SpotReview.get(location);
  }
}

spot_cubit.dart

import 'package:project/models/latlon.dart';
import 'package:project/spot/spot_repository.dart';
import 'package:project/spot/spot_state.dart';
import 'package:flutter_bloc/flutter_bloc.dart';

class SpotCubit extends Cubit<SpotState> {
  final SpotRepository _repository;

  SpotCubit(this._repository) : super(SpotInitial());

  Future<void> loadSpotInfoAndReviews(LatLon location) async {
    emit(SpotLoading());
    try {
      final info = await _repository.getSpotInfo(location);
      final reviews = await _repository.getSpotReviews(location);
      emit(SpotLoaded(info, reviews));
    } catch (e) {
      emit(SpotError("Failed to load spot info and reviews"));
    }
  }
}

spot_state.dart

import 'package:project/models/spot_info.dart';
import 'package:project/models/spot_review.dart';
import 'package:flutter/material.dart';

@immutable
abstract class SpotState {}

class SpotInitial extends SpotState {}

class SpotLoading extends SpotState {}

class SpotLoaded extends SpotState {
  final SpotInfo info;
  final List<SpotReview> reviews;

  SpotLoaded(this.info, this.reviews);
}

class SpotError extends SpotState {
  final String message;

  SpotError(this.message);
}

spot_widget.dart

import 'package:project/models/latlon.dart';
import 'package:project/spot/spot_cubit.dart';
import 'package:project/spot/spot_repository.dart';
import 'package:project/spot/spot_state.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';

class SpotBlocWidget extends StatelessWidget {
  final LatLon location;

  const SpotBlocWidget({super.key, required this.location});

  @override
  Widget build(BuildContext context) {
    return BlocProvider(
      create: (context) =>
          SpotCubit(SpotRepository())..loadSpotInfoAndReviews(location),
      child: BlocBuilder<SpotCubit, SpotState>(
        builder: (context, state) {
          if (state is SpotLoading) {
            return const CircularProgressIndicator();
          } else if (state is SpotLoaded) {
            return Column(
              children: [
                Text(location.toString()),
                Text(state.info.isPublic.toString()),
                Text(state.reviews.length.toString()),
              ],
            );
          } else if (state is SpotError) {
            return Text(state.message);
          } else {
            return const Text('Something went wrong');
          }
        },
      ),
    );
  }
}

[ksyro98]

hey! overall it looks good to me

i'm a bit skeptical about the SpotInfo.get and SpotReview.get methods are you doing a network request in them (or somehow communicating with external resources)?

if yes, following the BLoC principles, I would recommend placing those requests in a DataProvider class you can read more about it here: https://bloclibrary.dev/architecture/#data-provider

[ethicnology]

Thanks for your advice @ksyro98

Yes SpotInfo.get and SpotReview.get represents database tables and functions to access the related data.

Does DataProvider has anything to do with Provider package and StateManagement ?

Should I create first in models the class without database functions and then in providers functions to access the data get add that may wrap select insert and caching when needed ?

[FarisArmoush]

Overall this looks amazing, the only thing I would improve is SpotBlocWidget, I would separate into two widgets to separate the injection logic from the UI logic, so it would look something like this :

class SpotBlocView extends StatelessWidget {
  final LatLon location;

  const SpotBlocView({super.key, required this.location});

  @override
  Widget build(BuildContext context) {
    return BlocProvider(
      create: (context) =>
          SpotCubit(SpotRepository())..loadSpotInfoAndReviews(location),
      child: SpotBlocWidget(),
    );
  }
}

class SpotBlocWidget extends StatelessWidget {
  const SpotBlocWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocBuilder<SpotCubit, SpotState>(
      builder: (context, state) {
        if (state is SpotLoading) {
          return const CircularProgressIndicator();
        } else if (state is SpotLoaded) {
          return Column(
            children: [
              Text(location.toString()),
              Text(state.info.isPublic.toString()),
              Text(state.reviews.length.toString()),
            ],
          );
        } else if (state is SpotError) {
          return Text(state.message);
        } else {
          return const Text('Something went wrong');
        }
      },
    );
  }
}

[ksyro98]

@ethicnology hey, sorry for the delayed response.

The DataProvider has nothing to do with the Provider package and with StateManagement, it's an unfortunate naming convention.

And yes, I agree with you, moving all those methods (and caching) to the provider is a good idea. It will keep your models lighter, and will help with separation of concerns.

In general a DataProvider's role is to communicate with the external sources (databases, servers, local storage, etc) and to return raw data. Then you usually have a Repository that takes those raw data and converts them to Models for your application. And then the Bloc/Cubit and UI work with these Models just like you have done in your code.