Is it a bad practice to declare variables in build method

Question

I am working on a musicplayer application. I have to retrieve data like thumbnail,name,authors,etc for every song. So I have used the following code just above the return of build method

...@override
Widget build(BuildContext context) {
//For song index and details
final themeNotifier = Provider.of<ThemeNotifier>(context, listen: true);
audioFunctions = themeNotifier.getAudioFunctions();
themeNotifier.getSongIndex().then((value) {
  setState(() {
    index = value;
  });
});
thumbnailPath =
    audioFunctions.songs[index].albumArtwork ?? 'assets/thumbnail.jpg';
title = audioFunctions.optimiseSongTitles(index) ?? title;
author = audioFunctions.songs[index].artist ?? author;

//For the bg-gradient and device height
double deviceHeight = MediaQuery.of(context).size.height;
final light = Theme.of(context).primaryColorLight;
final dark = Theme.of(context).primaryColorDark;

return Scaffold(...

Is this a bad practise? If so, how can I do it the right way?

Answer 1

Yes it is. According to Flutter Docs:

Although it’s convenient, it’s not recommended to put an API call in a build() method.

Flutter calls the build() method every time it needs to change anything in the view, and this happens surprisingly often. Leaving the fetch call in your build() method floods the API with unnecessary calls and slows down your app.

The solution: Either use a FutureBuilder widget like so:

class _MyAppState extends State<MyApp> {
  Future<int> songIndex;
  
  @override
  void didChangeDependencies(){
  songIndex = Provider.of<ThemeNotifier>(context, listen: false).getSongIndex();
  super.didChangeDependencies();
  }
  
  @override
  Widget build(BuildContext context) {

    //For the bg-gradient and device height
    double deviceHeight = MediaQuery.of(context).size.height;
    final light = Theme.of(context).primaryColorLight;
    final dark = Theme.of(context).primaryColorDark;
    return FutureBuilder<int>(
      future: songIndex,
      builder: (context, snapshot) {
        if (snapshot.hasData) {
          final index = snapshot.data;
          thumbnailPath = audioFunctions.songs[index].albumArtwork ??
              'assets/thumbnail.jpg';
          title = audioFunctions.optimiseSongTitles(index) ?? title;
          author = audioFunctions.songs[index].artist ?? author;
          return Scaffold(body: Container());
        } else if (snapshot.hasError) {
          return Text("${snapshot.error}");
        }

        return CircularProgressIndicator();
      },
    );
  }
}

Notice that:

final themeNotifier = Provider.of<ThemeNotifier>(context, listen: false);

listen is false because I don't think we need to rebuild the widget each time the provider changes its state (because we only use it for executing the method).

Or [the better approach] to put the logic inside the ThemeNotifier class. Like so:

In the theme notifier class:

class ThemeNotifier with ChangeNotifier {
  //whatever state and logic you have

  //state for you current song index
  int _index;

  int get index => _index;

  Future<void> getSongIndex() {
    // some logic to retreive the new index
    _index = newIndex;
    notifyListeners();
  }
}

In the place you provided the ThemeNotifier

Widget build() => ChangeNotifierProvider(
      create: (_) => ThemeNotifier()..getSongIndex(),
      child: //whatever,
    );

In the place you used it (consumed it):

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    //For the bg-gradient and device height
    double deviceHeight = MediaQuery.of(context).size.height;
    final light = Theme.of(context).primaryColorLight;
    final dark = Theme.of(context).primaryColorDark;
    return Consumer<ThemeNotifier>(
      builder: (context, themeNotifier, _) {
        final index = themeNotifier.index;
        if (index == null) return CircularProgressIndicator();

        thumbnailPath =
            audioFunctions.songs[index].albumArtwork ?? 'assets/thumbnail.jpg';
        title = audioFunctions.optimiseSongTitles(index) ?? title;
        author = audioFunctions.songs[index].artist ?? author;
        return Scaffold(
          body: // continue here,
        );
      },
    );
  }
}

This was is using the Consumer syntax which is better if you don't want to rebuild the whole MyApp widget in this case each time the provider calls notifyListeners(). But a simpler way if the rebuilding doesn't matter (like in this case) is this:

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    //For the bg-gradient and device height
    double deviceHeight = MediaQuery.of(context).size.height;
    final light = Theme.of(context).primaryColorLight;
    final dark = Theme.of(context).primaryColorDark;
    final themeNotifier = Provider.of<ThemeNotifier>(context);
    final index = themeNotifier.index;
    if (index == null) return CircularProgressIndicator();

    thumbnailPath =
        audioFunctions.songs[index].albumArtwork ?? 'assets/thumbnail.jpg';
    title = audioFunctions.optimiseSongTitles(index) ?? title;
    author = audioFunctions.songs[index].artist ?? author;
    return Scaffold(body: null // continue here,
        );
  }
}

Final thoughts: I advise you to extract this logic:

   thumbnailPath = audioFunctions.songs[index].albumArtwork ??
              'assets/thumbnail.jpg';
   title = audioFunctions.optimiseSongTitles(index) ?? title;
   author = audioFunctions.songs[index].artist ?? author;

Into the provider itself.

Answer 2

For any kind of widget, you will most likely need some preparation (e.g. setting a variable's initial value or adding a listener to a FocusNode).

That being said, for StatelessWidget I've not come across any way to do it then doing so in the beginning of the build function.

For StatefulWidget, you can do all this by overriding the initState method. This where you typically set up listeners or set the initial value of a TextEditingController.

For any widget that requires awaiting some Future before rendering, I would recommend FutureBuilder since this easily allows you to handle all the different snapshot conditions and / or ConnectionState.

---

In your case, I don't see the problem with things like

//For the bg-gradient and device height
double deviceHeight = MediaQuery.of(context).size.height;
final light = Theme.of(context).primaryColorLight;
final dark = Theme.of(context).primaryColorDark;

since you can think of these as just making a sort of abbreviation for the otherwise long expressions. For example, repeatedly writing light would be much easier compared to Theme.of(context).primaryColorLight.

However, for things like this

themeNotifier.getSongIndex().then((value) {
  setState(() {
    index = value;
  });
});

depending on what exactly getSongIndex() does, what types of errors or delays it can cause, you might want to consider other options such as FutureBuilder.