Abhilash-Chandran / number_inc_dec

A flutter widget to accept numeric inputs with button to increment and decrement.
https://pub.dev/packages/number_inc_dec
MIT License
10 stars 7 forks source link

NumberInputPrefabbed class unnecessarily re-declares attributes #25

Closed abulka closed 3 years ago

abulka commented 4 years ago

This is not a bug but an observation re possibly improving the code. The NumberInputPrefabbed class re-declares all the attributes of its parent class NumberInputWithIncrementDecrement. This means the two attribute areas are duplicates of each other and need to be constantly synchronised and maintained, incl. the comments.

I don't think it is necessary for NumberInputPrefabbed to re-declare and mirror the attributes of its parent class - isn't that what super() is for? I did an experiment and mocked up the existing architecture, adding a few dummy types just to reduce complexity and dependencies:

// trying to understand why we have to declare 
// all the attributes twice

// dart example/temp2_inherit_eg.dart

class NumberInputPrefabbed extends NumberInputWithIncrementDecrement {
  final bool enabled;
  final num min;
  final num max;
  final int incIconDecoration; // dummy type
  final int decIconDecoration; // dummy type

  NumberInputPrefabbed.squaredButtons({
    this.enabled = true,
    this.min = -2,
    this.max = double.infinity,
  })  : incIconDecoration = 555,
        decIconDecoration = 777;
}

class NumberInputWithIncrementDecrement extends StatefulWidgetDummy { // dummy type
  final bool enabled;
  final num min;
  final num max;
  final int incIconDecoration; // dummy type
  final int decIconDecoration; // dummy type

  NumberInputWithIncrementDecrement({
    this.enabled = true,
    this.min = 0,
    this.max = double.infinity,
    this.incIconDecoration,
    this.decIconDecoration,
  });

  void build() {
    print(
        'enabled $enabled min $min max $max incIconDecoration $incIconDecoration decIconDecoration $decIconDecoration');
  }
}

class StatefulWidgetDummy {} // dummy type

main() {
  var widget = NumberInputWithIncrementDecrement(
      enabled: true,
      min: -2,
      max: 4,
      incIconDecoration: 11,
      decIconDecoration: 12);
  widget.build();

  var widget2 = NumberInputPrefabbed.squaredButtons(
      enabled: true,
      min: -1,
      max: 4,
      // no need to supply these, as these are built for us by the '.squaredButtons' constructor
      // incIconDecoration: 11,
      // decIconDecoration: 12
      );
  widget2.build();
}

output

enabled true min -2 max 4 incIconDecoration 11 decIconDecoration 12
enabled true min -1 max 4 incIconDecoration 555 decIconDecoration 777

Now let's implement NumberInputPrefabbed differently, without duplicating all the attributes and simply calling super instead:

class NumberInputPrefabbedNew extends NumberInputWithIncrementDecrement {
  // All attributes have been removed!
  NumberInputPrefabbedNew.squaredButtons({bool enabled, num min, num max})
      : super(
            enabled: enabled,
            min: min,
            max: max,
            incIconDecoration: 555,
            decIconDecoration: 777);
}

it works:

main() {
  var widget3 = NumberInputPrefabbedNew.squaredButtons(
    enabled: true,
    min: -55,
    max: 66,
  );
  widget3.build();
}

output

enabled true min -55 max 66 incIconDecoration 555 decIconDecoration 777

I think making this change would simplify the codebase and reduce duplication - perhaps I'm missing something?

Abhilash-Chandran commented 4 years ago

This is really a nice improvement and valid case. I would like to see in the code base.