monoレビュー 🐶 #13
Description
全体的に良い感じだと思いました 👍 ✨
Slackコメントに返答
画面の状態はStateNotifierでまるっと提供
関連する状態はStateNotifierでまとめるべきですが、直接関連しない状態は適切な小さめの粒度で分けるのが良いです。
mono0926@1bf39ad と mono0926@081d230 のようにするのが良いと思います。
Page単位でまとめるより、関連する状態の単位でまとめる方が組みやすいはずです。
Pageまたがったり複数箇所で必要な状態もありますし。
詳細画面は↑をやるとオーバーエンジニアリングに感じたのでFutureProviderで
ただし、規模の大きなアプリになったら統一させた方がいいんかな…
基本的には適切な種類のProviderを過不足なく組み合わせるのが良いと思っています。
アプリ自体の規模というより、メンバーの統制的に多少冗長なコードになったとしても何かしらの分かりやすい方針提示して画一的なコードにしたいとかなら、仕方なくStateNotifierProviderに寄せるとかありかもしれませんが。
その他気になった点箇条書き
全体的に良い感じな前提で、気になったところのみ列挙している感じです 🙏
※ 好みによるところや、僕が誤っているところが含まれている可能性もあります。
*.freezed.dart・*.g.dartなどは、コミットに含めた方が諸々利便性高く、目立ったデメリットもなく、個人的にはそうしている- buildをメソッドで分割している箇所は、Widget分割に統一がベター(あるいは読みにくくない範囲でベタ書きで済ませると良い)
- buildメソッド内で
ref.readしているところがあるが、すべてref.watchがベター - 公式推奨の https://dart-lang.github.io/linter/lints/directives_ordering.html を無効化して import_sorter に従う意義が疑問
- https://dart-lang.github.io/linter/lints/depend_on_referenced_packages.html は普通に良いお作法なのに、なぜ無効化するのか疑問
- https://github.com/watanavex/flutter_github_search/blob/main/lib/ui/page/search/search_page_state.dart#L12 など
@Default(false) bool isSearchMode,などデフォルト値を適宜活用すると良さそう -
など名前被りしやすいので、
_Suceessにするか、SearchStateSuccessにするかのどちらかが良い -
は普通に
final repositoryApiProvider = Provider((ref) => RepositoryDetailApi(ref.watch(dioProvider)));で良いのでは？ -
の
API_TOKENはアプリに含めても良いもの？- コード直書きでも
--dart-defineでもいずれにせよアプリビルド時に含めた値は少しがんばれば容易にアクセスできるので、いかなる手段でもアプリに秘匿情報を含めるべきではない(というのを把握した上で、一応publicリポジトリに含めるのを避けたいということならOK) - 実際のアプリでAPI利用制限を緩めるにはGitHub認証にてユーザーの認証情報を利用するべきなはず
- コード直書きでも
-
は
Theme.of(context).colorScheme.errorなどが良い - はImage系の命名お作法に従うと、CircleNetworkImageあたりがベター
- https://github.com/watanavex/flutter_github_search/blob/main/lib/ui/page/search/search_page_state_notifier.dart#L25 の変数名は
_readが慣例 -
は一見良いが、その時点での最新にアクセスできるように
SearchApi get _searchApi => _reader(searchApiProvider);に統一した方が良いことが多いと思う(lateだと初回アクセス時の結果がキャッシュされるので。このコードの場合はいずれも同じ挙動になるが。) - はHooks使わないならConsumerWidget で良いのでは？(依存パッケージもhooks_riverpod削ってflutter_hooksで充分)
- buildをメソッドで分割している箇所は、Widget分割に統一がベター(あるいは読みにくくない範囲でベタ書きで済ませると良い)
例えば、
は分断されているより、以下のように書かれている方がそのまま一目で読めてむしろ読みやすいと思います。(もちろん全てベタ書きだとカオスになるので、各種要素を適切にWidget分割することとセット)
@override
Widget build(BuildContext context, WidgetRef ref) {
final searchState = ref.watch(
searchPageStateNotifierProvider.select((value) => value.searchState));
return Scaffold(
appBar: _SearchAppBar(),
body: searchState.when(
uninitialized: () {
return Container();
},
searching: () {
return const LoadingView();
},
success: (repositories, query, page, haxNext) {
return _buildListView(context, ref, repositories, haxNext);
},
fetchingNext: (repositories, query, page) {
return _buildListView(context, ref, repositories, true);
},
fail: () {
return const ErrorView();
},
empty: () {
return const ErrorView(
message: '検索結果は0件です',
);
},
),
);
}