Skip to content

Rich - #5

Open
rich-yea wants to merge 4 commits into
mainfrom
rm-workshop-tasks
Open

Rich#5
rich-yea wants to merge 4 commits into
mainfrom
rm-workshop-tasks

Conversation

@rich-yea

Copy link
Copy Markdown

Some mistakes were made along the way, some lessons were learned. Thanks.

Features multiply
Each fix births three new problems
Farewell, dear codebase

@sobimor sobimor self-assigned this May 28, 2025

@sobimor sobimor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Well done @rich-yea 👏🏾
There are just a few minor comments, but overall, it was really good.

Comment on lines +36 to +47
final newTodo = {
'title': title,
'createdAtSeconds': DateTime.now().millisecondsSinceEpoch ~/ 1000
};

if (description != null) {
newTodo.addAll({'description': description});
}

if (imageUrl != null) {
newTodo.addAll({'imageUrl': imageUrl});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: You could have request body DTO with a to json method to avoid creating/parsing JSON directly here

}

Future<Set<String>> getFavouriteIds() async {
final prefs = await SharedPreferences.getInstance();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Instead of directly using SharedPreferences in the repository, you could use the TodoLocalDataSource.

}

Future<void> addFavourite(String todoId) async {
final prefs = await SharedPreferences.getInstance();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Instead of directly using SharedPreferences in the repository, you could use the TodoLocalDataSource.

}

Future<void> removeFavourite(String todoId) async {
final prefs = await SharedPreferences.getInstance();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Instead of directly using SharedPreferences in the repository, you could use the TodoLocalDataSource.

}

Future<bool> isFavourite(String todoId) async {
final prefs = await SharedPreferences.getInstance();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Instead of directly using SharedPreferences in the repository, you could use the TodoLocalDataSource.


Future<void> toggleTodo(String id, bool isDone) async {
try {
// Optimistic update - update UI immediately

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

praise: Well done! 👏🏾 really like the optimistic update idea here

Comment on lines +43 to +75
Center _errorScreen(TodosError state) {
return Center(child: Text('Error: ${state.message}', style: const TextStyle(color: Colors.red)));
}

Widget _loadedScreen(TodosLoaded state) {
final todos = state.todos;

if (todos.isEmpty) {
return const Center(
child: Text(
'No todos yet!\nTap the + button to add one.',
textAlign: TextAlign.center,
style: TextStyle(fontSize: 18),
),
);
}

return ListView.builder(
itemCount: todos.length,
itemBuilder: (context, index) {
return TodoItem(
todo: todos[index],
onToggle: (bool isDone) {
context.read<TodoCubit>().toggleTodo(todos[index].id, isDone);
},
);
},
);
}

Center _loadingScreen() {
return const Center(child: CircularProgressIndicator());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue: Returning widget via method is bad practice in flutter and causing performance issue. for any widget we should create widget class instead of return it via method.
if you are interested in why we should use classes over functions you can check below link:
https://medium.com/@vortj/flutter-daily-why-splitting-widgets-into-methods-is-actually-a-bad-habit-dad3edc3eead

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