skip to Main Content

How can I make the function more efficient?

This is all the test code I wrote:


// ignore_for_file: public_member_api_docs, sort_constructors_first
import 'package:flutter_test/flutter_test.dart';

void main() {
  test(
    'find list item and apply math',
    () {
      final mockList = <Model>[
        Model(id: '1', name: 'Cortado', price: 12),
        Model(id: '2', name: 'Caffe Latte', price: 24),
        Model(id: '3', name: 'Americano', price: 48),
        Model(id: '4', name: 'Turkish Coffee', price: 96),
        Model(id: '5', name: 'Filter Coffee', price: 64),
        Model(id: '6', name: 'Flat White', price: 128),
      ];

      final selectItems = [mockList[0], mockList[3]];
      final result = findAndOperate(
        mainList: mockList,
        selectedList: selectItems,
        operations: Operations.PLUS,
        value: 40,
      );
      print(result);
    },
  );
}

List<Model> findAndOperate({
  required double value,
  required Operations operations,
  required List<Model> mainList,
  required List<Model> selectedList,
}) {
  for (final selectItem in selectedList) {
    for (final item in mainList) {
      if (item == selectItem) {
        final itemIndex = mainList.indexOf(item);
        switch (operations) {
          case Operations.PLUS:
            final changeItem =
                selectItem.copyWith(price: selectItem.price + value);
            mainList.removeAt(itemIndex);
            mainList.insert(itemIndex, changeItem);
          case Operations.MULTIPLICATION:
            final changeItem = selectItem.copyWith(
              price: selectItem.price + (selectItem.price * value),
            );
            mainList.removeAt(itemIndex);
            mainList.insert(itemIndex, changeItem);
        }
      }
    }
  }
  return mainList;
}

// ignore: constant_identifier_names
enum Operations { PLUS, MULTIPLICATION }

// ignore: one_member_abstracts

class Model {
  Model({required this.id, required this.name, required this.price});

  final String id;
  final String name;
  final double price;

  Model copyWith({
    String? id,
    String? name,
    double? price,
  }) {
    return Model(
      id: id ?? this.id,
      name: name ?? this.name,
      price: price ?? this.price,
    );
  }

  @override
  String toString() => 'MockModel(id: $id, name: $name, price: $price)';

  @override
  bool operator ==(covariant Model other) {
    if (identical(this, other)) return true;

    return other.id == id && other.name == name && other.price == price;
  }

  @override
  int get hashCode => id.hashCode ^ name.hashCode ^ price.hashCode;
}

The function I wrote works as expected, but I’m new to this and I’m curious about what I can improve or learn.

The result that comes out when I run the function:

[MockModel(id: 1, name: Cortado, price: 52.0), MockModel(id: 2, name: Caffe Latte, price: 24.0), MockModel(id: 3, name: Americano, price: 48.0), MockModel(id: 4, name: Turkish Coffee, price: 136.0), MockModel(id: 5, name: Filter Coffee, price: 64.0), MockModel(id: 6, name: Flat White, price: 128.0)]

2

Answers


    1. The break; statements are missing in the switch cases. This means that the code would continue to the next case, which in fact is not needed.

    2. I’m pretty sure you can avoid the inner loop. You’re checking every Item in mainList with every Item in selectedList

    Login or Signup to reply.
  1. I would replace this

            mainList.removeAt(itemIndex);
            mainList.insert(itemIndex, changeItem);
    

    with

            mainList[itemIndex] = changeItem;
    

    Also the inner loop is not necessary, because you can just check if indexOf is -1 (unless you expect duplicates in the list). And you can make other small changes to make the code more compact. So all in all I would make findAndOperate like this:

    List<Model> findAndOperate({
      required double value,
      required Operations operations,
      required List<Model> mainList,
      required List<Model> selectedList,
    }) {
      for (final selectItem in selectedList) {
        final itemIndex = mainList.indexOf(selectItem);
        if (itemIndex != -1) {
          final changeItem = switch (operations) {
            Operations.PLUS => selectItem.copyWith(price: selectItem.price + value),
            Operations.MULTIPLICATION => selectItem.copyWith(price: selectItem.price * value)
          };
          mainList[itemIndex] = changeItem;
        }
      }
      return mainList;
    }
    

    or even

    List<Model> findAndOperate({
      required double value,
      required Operations operations,
      required List<Model> mainList,
      required List<Model> selectedList,
    }) {
      for (final selectItem in selectedList) {
        final itemIndex = mainList.indexOf(selectItem);
        if (itemIndex != -1) {
          mainList[itemIndex] = switch (operations) {
            Operations.PLUS => selectItem.copyWith(price: selectItem.price + value),
            Operations.MULTIPLICATION => selectItem.copyWith(price: selectItem.price * value)
          };
        }
      }
      return mainList;
    }
    
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search