-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Stop using unnecessary spread operators #740
Conversation
Signed-off-by: jld3103 <jld3103yt@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that everywhere where we had a nested ...[]
should use a different approach. I don't like that we now rely on indentation for readability.
Either use an external list and conditionally add to it or use a BuiltList
like
BuiltList((final b) {
for ( element in collection ){
b.add(element);
}
})
But we will need spread operators for those again, right? |
Ah my bad. I meant BuiltList.build(): final body = SettingsList(
categories: BuiltList<SettingsCategory>.build((final b) {
for (final category in [...appImplementation.options.categories, null]) {
final options = appImplementation.options.options.where((final option) => option.category == category);
final title = category != null ? category.name(context) : AppLocalizations.of(context).optionsCategoryOther;
if (options.isEmpty) {
continue;
}
final categoryTile = SettingsCategory(
title: Text(title),
tiles: BuiltList<SettingsTile>.build((final b) {
for (final option in options) {
if (option is ToggleOption) {
b.add(
CheckBoxSettingsTile(
option: option,
),
);
} else if (option is SelectOption) {
b.add(
DropdownButtonSettingsTile(
option: option,
),
);
}
}
}).toList(),
);
b.add(categoryTile);
}
}).toList(),
); this also allows us to do the cache values like the |
Oh that looks neat! |
But how do we handle conditionals? Just put the if inside a BuiltList as well? |
can you show me an example? I only want the BuiltList approach where we have nested if/for. |
I feel like using external lists will be easier to comprehend for new people, so I would prefer that approach. Doing this refactor will be much more work than I anticipated so I will close this PR and maybe comeback later or someone else can do it as well. |
Closes #696