Code review (in progress)

[aalex]

Code review by @aalex (in progress)

Note: This would fit better in a wiki page, and should eventually be split into multiple tickets. Then, each ticket should prioritized or discarded.

Coding style

Set the description field in the pubspec.yaml file

It could contain something like this:

"Travel Crew is a mobile app for friends to share travel plans, organize travel and even create full itineraries together."

Remove the package:travelcrew/ prefix from the imports of local files

Use relative imports instead.

Good:

import 'baz.dart';

BAD

import '../lib/baz.dart';

See https://dart.dev/guides/language/effective-dart/usage#prefer-relative-import-paths for why.

Specify bool return type for these methods

• utils.validators.Validators.* (those could also be changed from static methods to simple functions)

Document all the public classes and methods

At least document each class briefly.

Use /// as a prefix for documentation comments.'

Once this is done, uncomment the public_member_api_docs: true line in analysis_options.yaml.

Add some spaces before and after the following operators

Before and after these:

• +
• /
• * Between these:
• if(
• ){ After these:
• ,

Make sure all callbacks are indented in a new line

Separate each top-level class by two new lines

Cannot build the Android application on Windows

Using Android Studio on Windows 10.

C:\Users\aquessy\Documents\flutter\bin\flutter.bat --no-color build apk

Building without sound null safety
For more information see https://dart.dev/null-safety/unsound-null-safety

Running Gradle task 'assembleRelease'...                        

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':app:lintVitalRelease'.
> Could not resolve all artifacts for configuration ':app:debugAndroidTestCompileClasspath'.
   > Could not resolve androidx.test.espresso:espresso-core:3.6.0.
     Required by:
         project :app
      > Skipped due to earlier error

* 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 12m 28s
Running Gradle task 'assembleRelease'...                          773.8s
Gradle task assembleRelease failed with exit code 1

Code review - Part 2: Suggestions

Remove unused classes

Remove the following unused classes:

• ExpenseEntity (or is it used?) - ALso rename its file to expense_entity.dart

Build with sound safety

The build is currently done without sound null safety. It would be a good practice to build it with sound null safety.

Cannot build for the web

Cannont build for the web on Windows.

$ flutter build web

Building without sound null safety
For more information see https://dart.dev/null-safety/unsound-null-safety

Compiling lib\main.dart for the Web...
Target dart2js failed: Exception: lib/main.dart:18:8:
Error: Error when reading 'lib/services/initializer/project_initializer.dart': Error reading 'lib/services/initializer/project_initializer.dart'  (The system cannot find the path specified.
)
import 'package:travelcrew/services/initializer/project_initializer.dart';
       ^
lib/main.dart:22:8:
Error: Error when reading 'lib/services/responsive/responsive_wrapper.dart': Error reading 'lib/services/responsive/responsive_wrapper.dart'  (The system cannot find the path specified.
)
import 'package:travelcrew/services/responsive/responsive_wrapper.dart';
       ^
lib/main.dart:23:8:
Error: Error when reading 'lib/services/theme/theme_data.dart': Error reading 'lib/services/theme/theme_data.dart'  (The system cannot find the path specified.
)
import 'package:travelcrew/services/theme/theme_data.dart';
       ^
lib/main.dart:33:9:
Error: Method not found: 'ProjectInitializer'.
  await ProjectInitializer();
        ^^^^^^^^^^^^^^^^^^
lib/main.dart:77:26:
Error: The method 'ResponsiveWrapperBuilder' isn't defined for the class '_TravelCrewState'.
 - '_TravelCrewState' is from 'package:travelcrew/main.dart' ('lib/main.dart').
                  return ResponsiveWrapperBuilder(context, widget);
                         ^^^^^^^^^^^^^^^^^^^^^^^^
lib/main.dart:103:25:
Error: The method 'ThemeDataBuilder' isn't defined for the class '_TravelCrewState'.
 - '_TravelCrewState' is from 'package:travelcrew/main.dart' ('lib/main.dart').
                theme:  ThemeDataBuilder(),
                        ^^^^^^^^^^^^^^^^
lib/screens/login/login_form.dart:97:25:
Error: No named parameter with the name 'autovalidate'.
                        autovalidate: true,
                        ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3:
Info: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^
lib/screens/login/login_form.dart:110:25:
Error: No named parameter with the name 'autovalidate'.
                        autovalidate: true,
                        ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3:
Info: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^
lib/screens/signup/signup_form.dart:148:21:
Error: No named parameter with the name 'autovalidate'.
                    autovalidate: true,
                    ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3:
Info: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^
Error: Compilation failed.

Compiling lib\main.dart for the Web...                             34.7s
Exception: Failed to compile application for the Web.

Create a dev branch

I usually create a dev branch in which I develop. The master branch contains the latest tag.

The unit tests fail

When running flutter test:

                          lib/main.dart:18:8: Error: Error when reading 'lib/services/initializer/project_initializer.dart': The system cannot find the path specified.

import 'package:travelcrew/services/initializer/project_initializer.dart';
       ^
lib/main.dart:22:8: Error: Error when reading 'lib/services/responsive/responsive_wrapper.dart': The system cannot find the path specified.

import 'package:travelcrew/services/responsive/responsive_wrapper.dart';
       ^
lib/main.dart:23:8: Error: Error when reading 'lib/services/theme/theme_data.dart': The system cannot find the path specified.

import 'package:travelcrew/services/theme/theme_data.dart';

                          lib/main.dart:33:9: Error: Method not found: 'ProjectInitializer'.
  await ProjectInitializer();
        ^^^^^^^^^^^^^^^^^^
lib/main.dart:77:26: Error: The method 'ResponsiveWrapperBuilder' isn't defined for the class '_TravelCrewState'.
 - '_TravelCrewState' is from 'package:travelcrew/main.dart' ('lib/main.dart').
Try correcting the name to the name of an existing method, or defining a method named 'ResponsiveWrapperBuilder'.
                  return ResponsiveWrapperBuilder(context, widget);
                         ^^^^^^^^^^^^^^^^^^^^^^^^
lib/main.dart:103:25: Error: The method 'ThemeDataBuilder' isn't defined for the class '_TravelCrewState'.
 - '_TravelCrewState' is from 'package:travelcrew/main.dart' ('lib/main.dart').
Try correcting the name to the name of an existing method, or defining a method named 'ThemeDataBuilder'.
                theme:  ThemeDataBuilder(),
                        ^^^^^^^^^^^^^^^^
00:31 +0: loading C:\Users\aquessy\src\aalex\travelcrew\test\widget_test.dart                                                                                                00:32 +0: loading C:\Users\aquessy\src\aalex\travelcrew\test\widget_test.dart                                                                                                00:33 +0: loading C:\Users\aquessy\src\aalex\travelcrew\test\widget_test.dart                                                                                                                          lib/screens/login/login_form.dart:97:25: Error: No named parameter with the name 'autovalidate'.
                        autovalidate: true,
                        ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3: Context: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^
lib/screens/login/login_form.dart:110:25: Error: No named parameter with the name 'autovalidate'.
                        autovalidate: true,
                        ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3: Context: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^
lib/screens/signup/signup_form.dart:148:21: Error: No named parameter with the name 'autovalidate'.
                    autovalidate: true,
                    ^^^^^^^^^^^^
../../../Documents/flutter/packages/flutter/lib/src/material/text_form_field.dart:95:3: Context: Found this candidate, but the arguments don't match.
  TextFormField({
  ^^^^^^^^^^^^^

Right now, the test folder contains only the default test for a new flutter app. It should be changed so that it passes. Ideally, most simple business logic classes should have a matching unit test suite.

Dependencies analysis

Remove the dependency to the provider package

If we use bloc, why also use provider?

Dart analysis errors and warnings

error: Target of URI doesn't exist: 'package:cached_network_image/cached_network_image.dart'. (uri_does_not_exist at [flutter_app] lib\main.dart:1)
error: The method 'CachedNetworkImage' isn't defined for the type '_MyHomePageState'. (undefined_method at [flutter_app] lib\main.dart:144)
error: The method 'CachedNetworkImage' isn't defined for the type '_MyHomePageState'. (undefined_method at [flutter_app] lib\main.dart:163)
error: The method 'CachedNetworkImage' isn't defined for the type '_MyHomePageState'. (undefined_method at [flutter_app] lib\main.dart:196)
error: Target of URI doesn't exist: 'package:travelcrew/services/initializer/project_initializer.dart'. (uri_does_not_exist at [travelcrew] lib\main.dart:18)
error: Target of URI doesn't exist: 'package:travelcrew/services/responsive/responsive_wrapper.dart'. (uri_does_not_exist at [travelcrew] lib\main.dart:22)
error: Target of URI doesn't exist: 'package:travelcrew/services/theme/theme_data.dart'. (uri_does_not_exist at [travelcrew] lib\main.dart:23)
error: The function 'ProjectInitializer' isn't defined. (undefined_function at [travelcrew] lib\main.dart:33)
error: The method 'ResponsiveWrapperBuilder' isn't defined for the type '_TravelCrewState'. (undefined_method at [travelcrew] lib\main.dart:77)
error: The method 'ThemeDataBuilder' isn't defined for the type '_TravelCrewState'. (undefined_method at [travelcrew] lib\main.dart:103)
error: The named parameter 'autovalidate' isn't defined. (undefined_named_parameter at [travelcrew] lib\screens\login\login_form.dart:97)
error: The named parameter 'autovalidate' isn't defined. (undefined_named_parameter at [travelcrew] lib\screens\login\login_form.dart:110)
error: The named parameter 'autovalidate' isn't defined. (undefined_named_parameter at [travelcrew] lib\screens\signup\signup_form.dart:148)
info: The operand can't be null, so the condition is always true. (unnecessary_null_comparison at [flappy_search_bar] lib\flappy_search_bar.dart:85)
info: The operand can't be null, so the condition is always true. (unnecessary_null_comparison at [flappy_search_bar] lib\flappy_search_bar.dart:85)
info: The value of the field '_userRepository' isn't used. (unused_field at [travelcrew] lib\blocs\signup_bloc\signup_bloc.dart:14)
info: This function has a return type of 'Map<String, dynamic>', but doesn't end with a return statement. (missing_return at [travelcrew] lib\models\splitwise_models\expense_model.dart:65)
info: Unused import: 'balance_model.dart'. (unused_import at [travelcrew] lib\models\splitwise_models\friend_model.dart:1)
info: Unused import: 'current_user_model.dart'. (unused_import at [travelcrew] lib\models\splitwise_models\friend_model.dart:2)
info: Unused import: 'groups_model.dart'. (unused_import at [travelcrew] lib\models\splitwise_models\friend_model.dart:3)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\alerts\alert_dialogs.dart:1)
info: The value of the field '_userRepository' isn't used. (unused_field at [travelcrew] lib\screens\login\login_form.dart:22)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\main_tab_page\all_trips\ad_card.dart:1)
info: The import of 'dart:ui' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\main_tab_page\all_trips\all_trips_page.dart:1)
info: The import of 'package:flutter/widgets.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\main_tab_page\all_trips\all_trips_page.dart:4)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:1)
info: Unused import: 'package:nil/nil.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:4)
info: Unused import: 'package:travelcrew/blocs/settings_bloc/setting_state.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:6)
info: Unused import: 'package:travelcrew/models/settings_model.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:9)
info: Unused import: 'package:travelcrew/models/splitwise_models/current_user_model.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:10)
info: Unused import: 'package:travelcrew/models/splitwise_models/friend_model.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:11)
info: Unused import: 'package:travelcrew/screens/alerts/alert_dialogs.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:12)
info: Unused import: 'package:travelcrew/screens/menu_screens/settings/custom_notification_settings.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:13)
info: Unused import: 'package:travelcrew/services/database.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:15)
info: Unused import: 'package:travelcrew/services/functions/cloud_functions.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:16)
info: Unused import: 'package:travelcrew/services/notifications/notifications.dart'. (unused_import at [travelcrew] lib\screens\menu_screens\settings\settings.dart:18)
info: Unused import: 'package:nil/nil.dart'. (unused_import at [travelcrew] lib\screens\trip_details\details\activity_details.dart:5)
info: The value of the local variable '_image' isn't used. (unused_local_variable at [travelcrew] lib\screens\trip_details\explore\explore_owner_layout.dart:76)
info: The declaration '_onItemTapped' isn't referenced. (unused_element at [travelcrew] lib\screens\trip_details\explore\lists\addToListPage.dart:33)
info: The import of 'package:flutter/services.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\explore\lists\item_lists.dart:3)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\lodging\lodging_card.dart:1)
info: The import of 'package:flutter/rendering.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\split\bloc_builder.dart:2)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\split\split_package.dart:4)
info: The import of 'package:flutter/services.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/cupertino.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\split\split_package.dart:6)
info: This function has a return type of 'FutureOr<Widget>', but doesn't end with a return statement. (missing_return at [travelcrew] lib\screens\trip_details\split\split_package.dart:121)
info: This function has a return type of 'FutureOr<Widget>', but doesn't end with a return statement. (missing_return at [travelcrew] lib\screens\trip_details\split\split_package.dart:231)
info: The import of 'package:flutter/rendering.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\screens\trip_details\transportation\transportation_page.dart:2)
info: This function has a return type of 'Future<String>', but doesn't end with a return statement. (missing_return at [travelcrew] lib\services\apis\api.dart:179)
info: The value of the local variable 'options' isn't used. (unused_local_variable at [travelcrew] lib\services\apis\api.dart:182)
info: This function has a return type of 'FutureOr<UserPublicProfile>', but doesn't end with a return statement. (missing_return at [travelcrew] lib\services\database.dart:161)
info: 'accentIconTheme' is deprecated and shouldn't be used. No longer used by the framework, please remove any reference to it. For more information, consult the migration guide at https://flutter.dev/docs/release/breaking-changes/theme-data-accent-properties#migration-guide. This feature was deprecated after v2.3.0-0.1.pre.. (deprecated_member_use at [travelcrew] lib\services\widgets\appearance_widgets.dart:37)
info: 'accentIconTheme' is deprecated and shouldn't be used. No longer used by the framework, please remove any reference to it. For more information, consult the migration guide at https://flutter.dev/docs/release/breaking-changes/theme-data-accent-properties#migration-guide. This feature was deprecated after v2.3.0-0.1.pre.. (deprecated_member_use at [travelcrew] lib\services\widgets\appearance_widgets.dart:149)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\services\widgets\chat_date_display.dart:2)
info: The value of the field '_appBadgeSupported' isn't used. (unused_field at [travelcrew] lib\services\widgets\launch_icon_badger.dart:22)
info: The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\services\widgets\link_previewer.dart:1)
info: The import of 'dart:ui' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart'. (unnecessary_import at [travelcrew] lib\services\widgets\reusableWidgets.dart:1)
info: The value of the field '_passwordRegExp' isn't used. (unused_field at [travelcrew] lib\utils\validators.dart:6)

Rename the package to travel_crew

Flutter package names usually consist of underscore-separated words.

---

Incomplete review points about software architecture

Use inheritance between the following classes

• All the repositories/trip_repositories classes could extend a common class and only override a single private method. (I think)

TODO: Add more info here - and verify these suggestions.

Internationalize all the static strings

Use the intl package - that is already in the dependencies - to internationalize all the static strings in the user interface.

List of strings to internationalize:

• (TODO) There are lots of them.

Usage of the bloc pattern - too many blocs

• It seems to me like there are a lot for blocs, events and states. Is there a lot of code duplication there?
• TODO: is this done the right way?
• TODO: Spend more time studying this part.

Avoid to use dotenv for the app?

• Should we use the dotenv package? Using actual env vars on the web would be best.
• For the app, perhaps use some defaults. (FIXME)

[kaitech-corp]

Separated into multiple tickets.