Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: add news page #28

Merged
merged 3 commits into from
Aug 21, 2021
Merged

FEATURE: add news page #28

merged 3 commits into from
Aug 21, 2021

Conversation

Shmyaks
Copy link
Contributor

@Shmyaks Shmyaks commented Aug 19, 2021

Create news screen.
1_6r9j8TlK4 (1)

Copy link
Owner

@0niel 0niel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Некорректное название файла lib/presentation/widgets/widgets.dart. Файл виджета должен называться так же, как и сам виджет.
  2. Два абсолютно одинаковых виджета: lib/presentation/widgets/loader.dart и lib/presentation/widgets/loader.dart
  3. Необходимо использовать пакет Dio, а не встроенный http. Мы используем в проекте Dio.
  4. Необходимо реализовать интерфейс для NewsService (remote datasource).
  5. Необходимо реализовать кэширование новостей и тегов с помощью Local Datasource.
  6. Необходимо в репозитории проверять соединение с интернетом и в зависимости от этого выдавать данные из разных источников данных.
  7. Сущности, имеющие логику работы с данными, называются моделями и находятся в слое с данными.
  8. Необходимо выполнить внедрение зависимостей для bloc, usecase, repository, datasource.
  9. bloc должен получать доступ к возможностям репозиториев через юзкейсы.
  10. Все usecases должны быть унаследованы от класса UseCase.

@@ -0,0 +1,24 @@
import 'package:rtu_mirea_app/domain/entities/news.dart';
import 'package:http/http.dart' as http;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мы не используем пакет http, а используем http-клиент Dio.

import 'package:rtu_mirea_app/domain/entities/tag.dart';
import 'dart:convert';

class NewsService {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если NewsService используется как Remote Datasource, то ему необходим интерфейс в виде абстрактного класса, который будет описывать его поведение. Также необходимо внедрить зависимости, а то есть в данном случае http-клиент.

_MyHomeState createState() => _MyHomeState();
}

class _MyHomeState extends State<NewsScreen> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Некорректное название у класса-состояния NewsScreen. Название должно быть _NewsScreenState

Comment on lines 20 to 24
late ScrollController controller;
late NewsDataRepository newsUsecase;
late NewsBloc _news_bloc;
static const int tags_in_general = 4;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему поля состояния публичны? Кто и зачем должен получать к ним доступ?


class _MyHomeState extends State<NewsScreen> {
late ScrollController controller;
late NewsDataRepository newsUsecase;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presentation слой не имеет права обращаться к репозиториям. Для этого существуют Usecases.

}

@override
Future<List<Tag>> getTags() async {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Необходимо добавить проверку на соединение с интернетом, а также кэширование тегов.

@@ -0,0 +1,16 @@
import 'package:equatable/equatable.dart';

class ImageModel extends Equatable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Модели должны находиться в слое данных, а не в domain. Сущности не должны содержать бизнес-логики.

import 'package:rtu_mirea_app/domain/entities/image.dart';
import 'package:rtu_mirea_app/domain/entities/tag.dart';

class NewsModel extends Equatable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Модели должны находиться в слое данных, а не в domain. Сущности не должны содержать бизнес-логики.


Tag({required this.id, required this.name});

factory Tag.from_json(Map<String, dynamic> json) =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сущности не должны содержать логики для обработки чистых данных. Для этого существуют модели в уровне с данными.

@@ -35,7 +36,7 @@ Future<void> setup() async {
getIt.registerFactory(() => HomeNavigatorBloc());
getIt.registerFactory(() => OnboardingCubit());
getIt.registerFactory(() => MapCubit());

getIt.registerFactory(() => NewsBloc());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Есть внедрение зависимостей для bloc, но нет для usecase, repository, datasource

@0niel 0niel merged commit fe00b90 into dev Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants