Skip to content
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

added sizedbox, ordinal, roman representation extensions #6

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

athishaves
Copy link
Contributor

Description

Added few extensions which i personally use.

Space -> Converts [num] to [SizedBox].
horizontalSpace for width
verticalSpace for height

Ordinal -> Returns ordinal [String] of [int]

Roman -> Returns roman representation [String] of [int] from 1 to 3999

Issue link

If your PR closes an issue, use the closes keyword followed
by its link, for example:

closes https://github.com/nank1ro/awesome_flutter_extensions/issues/{ISSUE_NUMBER}

replacing {ISSUE_NUMBER} with the number, like: 1

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • 📝 Documentation
  • 🗑️ Chore

Copy link
Owner

@nank1ro nank1ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR, I really appreciate it! 💙
You forgot to export the lib/src/num_ext.dart file from the barrel file.

I have requested some changes.
Please add also unit tests for the ordinal and roman extensions. Thank you!

/// print(69.ordinal); // 69th
/// ```
String get ordinal {
if (this < 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes little sense to support negative numbers and turn them into ordinals. If it were necessary, it would suffice to convert it to positive before calling the function.

I would change the implementation into

String get ordinal {
    return switch (this % 100) {
      11 || 12 || 13 => '${this}th',
      _ => switch (this % 10) {
          1 => '${this}st',
          2 => '${this}nd',
          3 => '${this}rd',
          _ => '${this}th'
        }
    };
  }

/// print(12.ordinal); // 12th
/// print(69.ordinal); // 69th
/// ```
String get ordinal {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordinal function here returns the English suffixes. Specify this in the comments, because it doesn't support localisation.
For example the equivalent of 1st in French is 1er

/// print(1.toRoman); // I
/// print(3999.toRoman); // MMMCMXCIX
/// ```
String get toRoman {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a function instead of a getter, because the name is toRoman.
If you want to keep a getter name it without the to.

throw Exception('Number out of range (1 to 3999)');
}

final romanNumerals = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This algorithm could be improved using a map.
What I would write:

 String toRoman() {
    const romanTable = {
      'M': 1000,
      'CM': 900,
      'D': 500,
      'CD': 400,
      'C': 100,
      'XC': 90,
      'L': 50,
      'XL': 40,
      'X': 10,
      'IX': 9,
      'V': 5,
      'IV': 4,
      'I': 1,
    };

    final result = StringBuffer();
    var n = this;
    for (final entry in romanTable.entries) {
      final numeral = entry.key;
      final value = entry.value;
      while (n >= value) {
        result.write(numeral);
        n -= value;
      }
    }
    return result.toString();
  }
}

@athishaves
Copy link
Contributor Author

Tbh, never knew switch statements can be used like that. Thank you for you time!
I have updated the pr

@athishaves athishaves requested a review from nank1ro November 27, 2023 17:26
@nank1ro nank1ro closed this Nov 28, 2023
@nank1ro nank1ro reopened this Nov 28, 2023
@nank1ro nank1ro merged commit 4b6417a into nank1ro:main Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants