search

denisskin / todoapp

Ya To Do Flutter Application
0 stars 0 forks source link

hw3_notes #4

Open leshhub opened 1 year ago

leshhub commented 1 year ago

В принципе неплохое приложение получилось, но есть замечания.

Критические замечания.

  1. Репозиторий на GitHub-e (или любом другом Git-репозитории) - лицо приложения. Оно должно содержать в полном объёме:

    • Что приложение может (основные функции);
    • Какие есть у него особенности (фичи именно этого приложения, подчёркивающие его индивидуальность);
    • Очень желательно скриншоты приложения (чем больше, тем заинтересованнее будут другие);
    • Контроль версий приложения, путём хранения их в разделе Releases.

    За оформления пол балла заберу. Не потому что я такой душнила или злюка (；⌣̀_⌣́), так человеческий мозг устроен, который любит картинки, а не текст, а также любит пользоваться тем, у чего есть подробнейшее описание. Стоит сразу практиковаться оформлять свой проект.

  2. Нет чётко-организованного менеджера состояния. Для его реплизации можно было бы воспользоваться разными фреймворками (BloC, ChangeNotifier, ...), но постоянное обновления состояний через setState((){}) - это плохо.

  3. Думаю тут и так понятно, что нет DI, уж слишком всё друг от друга зависит. К тому же, тесты производятся над реальными данными, что плохо.
    Если кратко, то DI - это паттерн, который подразумевает, что если компонент зависит от какой-то внешней сущности (виджет завит от менеджера состояний, который зависит от внешних репозиторий), то в таких случаях просто можно передать ему эту зависимость, через которую он и будет обращаться. Тем самым, компонент перестаёт получать "неконтролируемый доступ" во внешнюю среду (снаружи компонента).
    Если видишь, что компонент (или просто файл) лезет куда-то в наружу (не через библиотеку), то стоит попробовать эту зависимость прокинуть через параметры, если это конечно необходимо (т.е. без фанатизма подходить к данному паттерну).

  4. Не реализован интеграционный тест. Да, присутствуют widget-тесты, но интеграционный следовало сделать отдельно. Он должен запускаться на реальном устройстве, а не на виртуальном, как это происходит в widget-тестов. Здесь подробно описано о том, как это можно сделать.

  5. Используется Navigator 1.0, а не 2.0. Стоит присмотреться к go_router. И соответственно нет реализации deeplink-ов. На самом деле в нём (go_router-e) нет ничего сложного, его очень легко интегрировать заместо navigator 1.0. Есть даже кучу гайдов по переезду между ними. Подробнее про go_router можно почитать здесь. Главное не делай как было в лекции (там ересь) (＞ｍ＜);

  6. Работа приложения без интернета невозможна. При установке apk (который лежит в дириктории dist), приложение не сохраняет состояния, сбрасывая количество задач к нулю. Если попытаться собрать приложение, то будет выведено следующее:

    ../../../../.pub-cache/hosted/pub.dev/device_info_plus-9.0.2/lib/src/model/android_device_info.dart:6:8: Error: Error when reading '../../../../.pub-cache/hosted/pub.dev/device_info_plus_platform_interface-7.0.0/lib/zmodel/base_device_info.dart': No such file or directory
import 'package:device_info_plus_platform_interface/zmodel/base_device_info.dart';
      ^
../../../../.pub-cache/hosted/pub.dev/device_info_plus-9.0.2/lib/src/model/android_device_info.dart:11:33: Error: Type 'BaseDeviceInfo' not found.
class AndroidDeviceInfo extends BaseDeviceInfo {
                               ^^^^^^^^^^^^^^
../../../../.pub-cache/hosted/pub.dev/device_info_plus-9.0.2/lib/device_info_plus.dart:100:16: Error: A value of type 'Future<AndroidDeviceInfo>' can't be returned from an async function with return type 'Future<BaseDeviceInfo>'.
- 'Future' is from 'dart:async'.
- 'AndroidDeviceInfo' is from 'package:device_info_plus/src/model/android_device_info.dart' ('../../../../.pub-cache/hosted/pub.dev/device_info_plus-9.0.2/lib/src/model/android_device_info.dart').
- 'BaseDeviceInfo' is from 'package:device_info_plus_plaform_interface/model/base_device_info.dart' ('../../../../.pub-cache/hosted/pub.dev/device_info_plus_platform_interface-7.0.0/lib/model/base_device_info.dart').
       return androidInfo;
              ^
../../../../.pub-cache/hosted/pub.dev/device_info_plus-9.0.2/lib/src/model/android_device_info.dart:40:14: Error: Too many positional arguments: 0 allowed, but 1 found.
Try removing the extra positional arguments.
       super(data);
            ^
Target kernel_snapshot failed: Exception

FAILURE: Build failed with an exception.

* Where:
Script '/home/master/snap/flutter/common/flutter/packages/flutter_tools/gradle/flutter.gradle' line: 1201

* What went wrong:
Execution failed for task ':app:compileFlutterBuildRelease'.
> Process 'command '/home/master/snap/flutter/common/flutter/bin/flutter'' finished with non-zero exit value 1

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 13s
Running Gradle task 'assembleRelease'...                           14.1s
Gradle task assembleRelease failed with exit code 1

    Что примечательно, если откатиться последнего комита до дедлайна (попробовать собрать приложение (homework#3 8131a4e), то там этой проблемы нет, как и тестов. Скорее всего проблема связана с пакетом device_info_plus.
    Поскольку это может быть проблема с моей стороны, то за это душнить не буду, поскольку версия приложения из последнего коммита до дедлайна работает сносно как online, так и offline, хотя есть заметные подтормаживания. Да минусов и так много нашлось уже...

Замечания (не в зачёт, просто на заметку):

  1. Не стоит хранить какие-то секреты (публичные ключи к доступу в БД и т.п.) открыто в публичном репозитории. Стоит почитать на счёт обфускации кода.

  2. Советую присмотреться к одному из менеджеров состояний, поскольку данный вариант не стоит использовать, тем более в продакшене. https://github.com/denisskin/todoapp/blob/85c513f56131ce846953ef505c101af880b89e92/lib/pages/home_page.dart#L16-L18

  3. Стоит поробовать декомпозировать код в home_page.dart и task_page.dart, скинув большую часть кода в подпапку widgets.

  4. Попробуй добавить локализацию, это поможет не харкодить множество слов, которые могут в дальнейшем измениться.

  5. Множество документации есть, которые реккомендуют (а некоторые даже требуют, freezed к примеру), чтобы классы моделей были неизменяемыми (immutable), а изменения происходили через copyWith. Это действитель во много лучше, особенно это улучшает производительность приложения. Для простоты можешь воспользоваться пакетом кодогенерации freezed.

  6. Не стоит обращаться напрямую к репозиторию из слоя pages. Будет лучше использовать некоторую "прослойку" между ними. Она помогает в дальнейшем рефакторинге кода. Зачастую этой "прослойкой" является менеджер состояний, которому ты просто посылаешь некоторые события, а уже он сам решает как поступать при определённом состоянии. https://github.com/denisskin/todoapp/blob/85c513f56131ce846953ef505c101af880b89e92/lib/pages/home_page.dart#L200-L216

Заключение

Не могу сказать, что приложение плохое. Видно, что некоторые части делались впервые, и по многим критериям приложение проходит, но вот мой совет: попробуй выкрасть время, чтобы рефакторить код под некоторые критерии и паттерны (DI, SOLID, ...).
Не стоит прям переусердствовать, но в дальнейшем это поможет писать такой код, чтобы его было и легко понять, и также легко изменить. Может по времени немного укладываться не будешь, но зато ты всегда будешь уверен в той части кода, которую написал, и всегда будешь готов его дополнить/изменить, причём, что немало важно, без головной боли (сколько я таких проектов написал, что самому не по себе от только воспоминания о них, бррр).

goliksim commented 1 year ago

Привет. Проверял твою работу в рамках третьей кросспроверки.

Из того что я заметил:

Рекомендации