refactor feature fetch message structure by ManhNTX · Pull Request #1528 · linagora/Twake-Mobile

ManhNTX · 2023-01-18T04:00:35Z

No description provided.

hoangdat · 2023-01-19T01:35:17Z

How about the use-case layer?
How about to use Either?

hoangdat · 2023-01-19T01:45:44Z

Why need BaseMessageBloc? ``Base... Base...`

lib/features/message/presentation/bloc/message_animation/message_animation_cubit.dart

lib/features/message/presentation/bloc/base_message/base_message_bloc.dart

lib/features/message/data/repository_impl/message_repository_impl.dart

lib/features/message/data/datasource/message_datasource.dart

lib/features/message/data/datasource_impl/remote_message_datasource_impl.dart

lib/features/message/data/model/message/response/message.dart

lib/features/message/data/model/message/response/reaction.dart

lib/features/message/data/repository_impl/message_repository_impl.dart

dab246 · 2023-01-19T03:08:46Z

lib/features/message/presentation/bloc/base_message/base_message_bloc.dart

+
+part 'base_message_event.dart';
+
+part 'base_message_state.dart';


It is recommended to use absolute path

Hi! Thanks for the work, now everything is done on сubits and I don't see the point in adding new blocks with events, it's better to keep everything on сubits IMHO

dab246 · 2023-01-19T03:10:02Z

lib/features/message/presentation/bloc/base_message/base_message_bloc.dart

+    final threadId = event.threadId ?? Globals.instance.threadId;
+
+    List<Message> lastList = const [];
+    await for (var list in stream) {


IMO, Should be split into smaller functions

dab246 · 2023-01-19T03:10:42Z

lib/features/message/presentation/bloc/base_message/base_message_bloc.dart

+
+      emit(MessagesLoadSuccess(
+        messages: list,
+        hash: list.fold(0, (acc, m) => acc + m.hash),


IMO, Create function to generate hash key

dab246 · 2023-01-19T03:12:07Z

lib/features/message/presentation/bloc/base_message/base_message_event.dart

+  });
+
+  @override
+  List<Object?> get props => [];


IMO, properties should be added to props for future comparison.

dab246 · 2023-01-19T03:13:48Z

lib/features/message/presentation/widgets/message_animation.dart

+      Duration(milliseconds: 50),
+      FocusManager.instance.primaryFocus?.unfocus,
+    );
+    Get.find<MessageAnimationCubit>().endAnimation();


It is recommended to use Get.find centralized in one place

dab246

Quick review:

Objects should be immutable
Fill in properties in props
Should create instance of Get.find and use it. Avoid writing: `Get.find().function()
Should create separate function for events in call from onTap, onClick
Note convension code :
- There are too many parameters of the function, each parameter should be 1 line.
- Indentation should be 1 tab

EvGen94

§

EvGen94 · 2023-01-27T07:12:00Z

lib/features/message/presentation/bloc/base_message/base_message_bloc.dart

+
+part 'base_message_event.dart';
+
+part 'base_message_state.dart';


Hi! Thanks for the work, now everything is done on сubits and I don't see the point in adding new blocks with events, it's better to keep everything on сubits IMHO

ManhNTX · 2023-01-27T10:20:26Z

§

Hi! Reason to change some cubit to bloc:

Some case i want trigger many event in 1 action or i want make action concurrency (sample when change workspace i want to cancel action fetch A workspace and start B workspace).
It make clearly, and i don't covert all cubit to bloc, just only abstract cubit.

hoangdat · 2023-01-29T23:03:23Z

lib/core/state/app_state.dart

+import 'package:twake/core/state/success.dart';
+
+@immutable
+abstract class AppState with EquatableMixin {


why we need this class?

hoangdat · 2023-01-29T23:05:10Z

lib/features/message/domain/usecases/fetch_message_usecase.dart

+      this._messageRepository,
+      );
+
+  Either<ExceptionFailure, Stream<List<Message>>> execute({ String? companyId,


hoangdat · 2023-01-31T02:46:44Z

lib/core/domain/repository/repository.dart

+import 'package:flutter/material.dart';
+import 'package:twake/models/globals/globals.dart';
+
+abstract class RepositoryHelper {


can you explain your idea?

hoangdat · 2023-01-31T02:51:33Z

lib/core/domain/repository/repository.dart

+    if (!Globals.instance.isNetworkConnected) {
+      onLocal?.call();
+      return;
+    }
+    onRemote?.call();


IMO, it is not generic enough. For example: when we need to first: fetch data from local, in parallel, fetch the data with network to update.

hoangdat · 2023-01-31T02:53:15Z

lib/features/message/presentation/bloc/message/message_bloc.dart

@@ -51,9 +51,11 @@ class MessageBloc extends Bloc<MessageEvent, MessageState> {
    if (event.threadId == null) emit(MessagesLoadInProgress());

    final result = _fetchMessageUseCase.execute(


IMO, we need an method to execute all the usecase

refactor feature fetch message structure

9f8f496

ManhNTX temporarily deployed to dev January 18, 2023 04:03 — with GitHub Actions Inactive

hoangdat reviewed Jan 19, 2023

View reviewed changes

lib/features/message/data/datasource_impl/remote_message_datasource_impl.dart Outdated Show resolved Hide resolved

hoangdat reviewed Jan 19, 2023

View reviewed changes

lib/features/message/data/model/message/response/message.dart Outdated Show resolved Hide resolved

lib/features/message/data/model/message/response/message.dart Outdated Show resolved Hide resolved

dab246 reviewed Jan 19, 2023

View reviewed changes

lib/features/message/data/model/message/response/reaction.dart Outdated Show resolved Hide resolved

dab246 reviewed Jan 19, 2023

View reviewed changes

lib/features/message/data/repository_impl/message_repository_impl.dart Outdated Show resolved Hide resolved

dab246 reviewed Jan 19, 2023

View reviewed changes

EvGen94 reviewed Jan 27, 2023

View reviewed changes

ManhNTX force-pushed the feature/refactor_code branch 2 times, most recently from 9c6fb44 to 1abcb50 Compare January 29, 2023 20:14

add use case refactor model and abstract query parameter

47a445e

ManhNTX force-pushed the feature/refactor_code branch from 1abcb50 to 47a445e Compare January 30, 2023 02:24

add abstract repo and use case

6452b55

hoangdat reviewed Jan 31, 2023

View reviewed changes


		part 'base_message_event.dart';

		part 'base_message_state.dart';

		@@ -51,9 +51,11 @@ class MessageBloc extends Bloc<MessageEvent, MessageState> {
		if (event.threadId == null) emit(MessagesLoadInProgress());

		final result = _fetchMessageUseCase.execute(

Comments

Conversation

ManhNTX commented Jan 18, 2023

Uh oh!

hoangdat commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoangdat commented Jan 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dab246 left a comment

Choose a reason for hiding this comment

Quick review:

Uh oh!

EvGen94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManhNTX commented Jan 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hoangdat commented Jan 19, 2023 •

edited

Loading