linter icon indicating copy to clipboard operation
linter copied to clipboard

Lint for literal values used in expressions

Open zoechi opened this issue 10 years ago • 8 comments

literal values (String, int, double, not sure about others) should only be assigned to named constants (except 0, 1)

zoechi avatar Mar 15 '15 13:03 zoechi

Could you add some good/bad examples? How would you phrase the advice?

pq avatar Mar 16 '15 03:03 pq

Prefer creating a named constant or an enum instead of using a literal value (magic number) in an expression directly.

class Person {
  String name;
  int age;
  List<String> validate() {
    var errors = [];
    if(name == null || name.isEmpty) {
      errors.add('"name" is mandatory.');
    } else if(name.length > 32) {
      errors.add('The maximum name length is 32');
    }

    if(age == null) {
      errors.add('"age" is mandatory.');
    } else if(age < 18) {
      errors.add('The minimum age is 18 to register for this service.');
    }
    return errors;
  }
}

should be something like

class Person {
  static const maxNameLength = 32;
  static const minAge = 18;
  static const maxNameLengthExceededMessage = 'The maximum name length is $maxNameLength';
  static const minAgeBelowMinAgeMessage = 'The minimum age is $minAge to register for this service.';
  static const nameMandatoryMessage = '"name" is mandatory.';
  static const ageMandatoryMessage = '"age" is mandatory.';

  String name;
  int age;

  List<String> validate() {
    var errors = [];
    if (name == null || name.isEmpty) {
      errors.add(nameMandatoryMessage);
    } else if (name.length > maxNameLength) {
      errors.add(maxNameLengthExceededMessage);
    }

    if (age == null) {
      errors.add(ageMandatoryMessage);
    } else if (age < minAge) {
      errors.add(minAgeBelowMinAgeMessage);
    }
    return errors;
  }
}

zoechi avatar Mar 16 '15 06:03 zoechi

I'm not really tempted by this lint. Especially when the literal is used only once. This makes readability harder IMHO because you end up jumping to variable declarations all the time to really understand what is going on.

Remember that one of the most difficult task in software development is to find (variable|class|member)'s names ;)

a14n avatar Mar 16 '15 07:03 a14n

I don't think this should be forced on users. The configuration story is not settled as far as I understand but I hope

  • the lints derived from the style guide will be default with a way to opt out globally or per library, class, function/method, or expression
  • additional lints like this one should be opt-in on package level using a config file, with the ability to specify exceptions as listed for default lints above.

This would for example allow to do a quality check with this specific lint enabled where each magic number/string can be checked if it is still ok to keep the literal or create a constant instead. After that check it can be disabled for the package again or disabled on the code lines where exceptions are accepted.

zoechi avatar Mar 16 '15 08:03 zoechi

Ok, thanks for the clarification. I thought it was a general lint that would have been activated by default. Sorry for the noise ;)

a14n avatar Mar 16 '15 08:03 a14n

@a14n : no apologies necessary. Input is a good thing! And configurability is hot topic #7 (and #41).

@zoechi : thanks for the clarification. It looks like you're asking for a flavor of "magic number" checking.

For reference, checkstyle's MagicNumber check is defined like this:

Checks that there are no "magic numbers", where a magic number is a numeric literal that is not defined as a constant. By default, -1, 0, 1, and 2 are not considered to be magic numbers.

pq avatar Mar 16 '15 16:03 pq

Further rationale for a check of this sort over on this stack overflow thread.

pq avatar Mar 16 '15 16:03 pq

Not sure it's a flavor. I would say it's exactly that ;-)

zoechi avatar Mar 16 '15 17:03 zoechi

updated link for magic number https://checkstyle.sourceforge.io/checks/coding/magicnumber.html#MagicNumber

For reference, checkstyle's MagicNumber check is defined like this:

amrgetment avatar Nov 13 '23 09:11 amrgetment

Any updates regarding this lint?

amrgetment avatar Nov 13 '23 09:11 amrgetment

No updates.

srawlins avatar Nov 13 '23 15:11 srawlins