Todo Flutter App Tech Task

critique.md

Feature-blocking Bugs & Unhandled Exceptions:

1. todos_screen.dart@62 todos list is updated within the State<TodosScreen>, but failure to call setState method does not update the UI. Could be solved by wrapping line 62 in a setState function call.
2. The Edit Todo flow opens a selected Todo item on a new screen and "awaits" on the edited result. If the user never clicks "Submit" in the editing screen, null will be returned as the value of edited Todo and NetworkClient.updateTodo(null) will be executed (the method is not null-safe), resulting in a NoSuchMethodError exception. We should not be attempting to update Todo if the user did not perform any changes; so we should only call updateTodo and showSnackBar if the edited todo is not null.
3. networkClient.dart@34 creating a new Todo constructs request body inline. Looks like "titles" is a typo, and may have the incorrect body structure (should consult with POST API docs). Should ideally be using a model along with its toMap function, so that none of the mandated properties would be forgotten.
4. Column content-bottom overflows in the edit todo screen. Wrap Column in a SingleChildScrollView to solve bottom overflow, at the expense of the user having to scroll to the content which did not fit on the screen.

UI/UX Issues:

1. After clicking "Delete", the bottom sheet does not automatically close. The Navigator route popping logic is missing in todos_screen.dart@76-77.
2. In landscape mode: edit_todo_screen.dart@23 padding of 80.0 is too large. If it's used to imitate centring in portrait, utilise the main axis alignment property of the Column instead. Otherwise, can have different padding for portrait/landscape with OrientationBuilder widget.
3. todos_screen.dart@40 Column does not take full-width, only stretches to accommodate the longest of Todo title texts - this is very evident in landscape mode. Consider either adding a CrossAxisAlignment.stretch cross-axis alignment to take full width. The same can be achieved by wrapping the column in an Expanded and horizontal flex (row).
4. todos_screen.dart@50 textAlign is set to left. Since the Text ancestor in the widget tree is a Column, it will default to CrossAxisAlignment.center, centring all children text widgets. Since text widgets may not span throughout the whole width of the screen, they will always appear centred, regardless of the textAlign setting. Only when the text widget takes full length (that is, overflows to 2nd line) this will take effect. For the text widgets to be left-aligned in the parent, we need to change the cross-axis alignment of the column to CrossAxisAlignment.start.
5. When editing a Todo title value, users have to type it from scratch. This is not desirable, because most often users want to edit the original value slightly, so it would be nice for the Text Field to default to the original value.
6. No progress indicator while Todo list is being fetched & no error handling if fetching fails. Users should be aware that they are waiting on server response & be informed if something went wrong with the steps to retry the action. The same applies to other network-based use cases like Edit & Delete.
7. Users cannot determine if a displayed Todo item is "completed" already. Each list tile should indicate the appropriate status, and ideally, users should be able to filter what they see based on whether the Todo is complete or pending.
8. The modal bottom-sheet popup cannot be closed when swiping down. Users are used to being able to close bottom-sheet modal dialogs. In todos_screen.dart@69 consider replacing ListView with a Column since ListView consumes our scrolling event & does not delegate it to the modal bottom sheet.
9. The bottom sheet may be too tall for only 2 options ("Delete" & "Edit") - it should not expand to its maximum height. To solve, if we use a Column as their parent (as advised in point 7.), we can utilise its main axis size property, and set it to MainAxisSize.min.
10. Delete should not execute immediately -- there should be a confirmation to save users from accidental deletes. Ideally, this could be replaced by an "Archive" feature which allows users to restore notes.
11. The delete action button could have a more distinctive look (e.g. different colours).
12. Many buttons & list tiles in the app are not distinguished enough to be perceived as tappable targets. Clickable tiles should be taller, take full width in the parent & have a separator between them so that the boundaries would be easily identifiable. Ideally, they should provide some feedback when touched (e.g. Ripple/Inkwell effect). The Delete/Edit buttons placed in the modal bottom sheet could also become full-width tiles.
13. network_client.dart@32 createTodo function could return a Future or Future so that callers could check if operation was successful & inform the user.
14. The user sees "Submit" in the edit screen, even without applying any modifications. Could make adaptive UI so that user is offered to "Save" only when there is something to actually be saved (or if a user at least focused on the text edit field).

Optimisations:

1. todos_screen.dart@39-40 Instead of the SingleChildScrollView and Column combination, a ListView.builder should be used to lazily build the item widgets as the current approach builds all of Todo widgets.
2. Could reuse Navigator.of(context) when calling it multiple times within the same function, because every call walks up the widget element tree.
3. Client instances could be reused to keep an open connection across multiple requests to the server. Currently, http library is used directly to execute GET and PUT requests, each of which creates a brand new web client behind the scenes and does not reuse the connection.
4. Updating a Todo item (HTTP PUT) even if no values have changed. Could do an equality check to avoid such submission.
5. todos_screen.dart@88 constructing new Todo may be excessive, since we already have the edited todo instance above.
6. Mark eligible instantiations (mostly widgets) with constant values as const.

Information leaking:

1. Only use print in debug mode. Would be nice to have a custom logging function that will respect this, and only print in debug mode. Couple of possible options to use:

• kDebugMode constant from flutter/foundation
• Custom function without depending on flutter framework. For example, such a function can rely on Dart's assert statement to return if in debug mode, since asserts are stripped out from production code at compile time.

2. Prefix private variables with an underscore. Currently, most of the values are public without a need for it.

Best practices & Guidelines:

1. Add tests
2. Many bugs were introduced because of the lack of null safety. Migrate to Dart Null Safety.
3. There is a lack of error handling. For example, none of the JSON Decodes followed by Todo.from(Map) are handling possible parsing errors.
4. edit_todo_screen.dart@38-43 creating a new instance of Todo passing all the required model values one by one, just to modify the title field. The Todo model could instead have a copyWith method, where most (or all) parameters are optional, so you only pass the ones which you want to change for your newly created instance.
5. Use /// for documentation (e.g. method docs, instead of //)
6. Many variables defined as var are not modified. Consider changing them to final.
7. Consider making models immutable, by adding @immutable and making fields final (at least those which are fine to be "polluted" with flutter-specific imports).
8. network_client.dart@17 getTodos returns a Future<dynamic> while it could return a less abstract Future<String>
9. pubspec.yaml flutter_lints & cupertino_icons dependencies are out of date.
10. Colors.green is hard-coded in todos_screen.dart. This is an anti-pattern because these parts of the application can't be easily styled by changing the main theme of the application, which is defined as ThemeData in main.dart. So, inherited widget lookup like Theme.of(context) could be used instead of hard-coded colours.

Structure & Software Design:

1. Would recommend restructuring the project to follow SOLID principles & Clean Architecture. Incorporate dependency injection where required to make components testable.

• How:
  • One option would be to introduce additional layers, for example, presentation (widgets), domain (use cases, domain models), repository (abstraction for the data source, mapping from DTO to domain models) & data (remote data source, DTO models)
• Motivation:
  • Separation of concerns. Currently, there is too much happening in widgets, for example: making network requests & parsing the response to DTOs, etc.

2. Encapsulate backend configuration. Make base server URL easily configurable, so that testing vs production builds could point to different backends. Have other properties like Content-Type or base headers part of the configuration to reuse them and avoid typos.