Refactor and clean up number helpers #10996
Conversation
|
It's hard to read classes intermingled with methods that construct them, especially in a file 800 lines long. I'd create a number_converters directory and move all the converters in there (each in their own file), Only other concern I have is potential performance in case someone is using number helper massively - what is overhead of building all these objects on each call? Perhaps the invoking methods should cache each instance of a class (of course that would mean that passing the arguments would have to move from initializer to the |
|
I will work on moving these into their own directory and files. I like that approach much better as well. |
|
Yes, I admit that the performance may lag a bit. My goal wasn't performance in this round of refactoring. It was to clarify what each helper was doing under the hood rather than relying on documentation alone for understanding what the helper was doing. I do not have benchmarks on this, but I'm guessing that using instantiated objects rather than functional class methods would be a performance hit if using these a lot. This can easily change by altering the classes to use class methods and remove instantiation. |
|
👍 I'm not sure if performance matters here. I'll review this better later but this is very good. @mattdbridges great job ❤️ |
ghost
assigned 
| -1 => :deci, -2 => :centi, -3 => :mili, -6 => :micro, -9 => :nano, -12 => :pico, -15 => :femto } | ||
|
|
||
| STORAGE_UNITS = [:byte, :kb, :mb, :gb, :tb] | ||
| autoload :NumberToRoundedConverter, "active_support/number_helper/number_to_rounded" |
There was a problem hiding this comment.
autoloads are lazy (as opposed to requires) and can be put anywhere in the class. Conventionally, similar to module includes, they are put at the very top for easier dependency tracking by maintainers. (in this case, above or right below extend self)
Also, if you add the line extend ActiveSupport::Autoload to the class, you can drop repeating the relative path on each autoloaded constant, as long as the file name exactly matches the class name (e.g. will involve renaming number_to_delimited to number_to_delimited_converter etc, but that's something you should try and do anyways, again for consistency for maintainers.
Taking it even one step further in class structure purism would involve creating a Converters namespace/directory and referring to these classes such as Converters::NumberToDelimited etc. You may or may not think this is an overkill, but just an idea.
|
Thanks for this PR, btw! I also think Rails maintainers prefer pull requests squash their commits into one, especially if they all deal with the same area. |
|
|
||
| def initialize(number, opts = {}) | ||
| @number = number | ||
| @opts = (opts || {}).symbolize_keys |
There was a problem hiding this comment.
Why is || {} needed here? opts already default to {} in the method definition. It only matters if someone passes explicit nil or false as an the options argument, which I don't think makes sense.
There was a problem hiding this comment.
I was a bit paranoid, I suppose. I suppose just allowing the error would make more sense than suppressing it in this context.
|
Yes, I know they prefer squashed commits, but as I was unsure of how many more changes would be suggested, I figured it would be easier for me to continue to commit one at a time and then squash them once the Rails core team is ready. |
|
@rafaelfranca Would you want me to squash these commits now to make it easier for you to review? |
|
Love this ❤️ |
|
@mattdbridges it'll need to be squashed before a merge, at least. |
|
bump |
Due to the overall complexity of each method individually as well as the global shared private module methods, this pulls each helper into it's own converter class inheriting from a generic `NumberBuilder` class. * The `NumberBuilder` class contains the private methods needed for each helper method an eliminates the need for special definition of specialized private module methods. * The `ActiveSupport::NumberHelper::DEFAULTS` constant has been moved into the `NumberBuilder` class because the `NumberBuilder` is the only class which needs access to it. * For each of the builders, the `#convert` method is broken down to smaller parts and extracted into private methods for clarity of purpose. * Most of the mutation that once was necessary has now been eliminated. * Several of the mathematical operations for percentage, delimited, and rounded have been moved into private methods to ease readability and clarity. * Internationalization is still a bit crufty, and definitely could be improved, but it is functional and a bit easier to follow. The following helpers were extracted into their respective classes. * `#number_to_percentage` -> `NumberToPercentageConverter` * `#number_to_delimited` -> `NumberToDelimitedConverter` * `#number_to_phone` -> `NumberToPhoneConverter` * `#number_to_currency` -> `NumberToCurrencyConverter` * `#number_to_rounded` -> `NumberToRoundedConverter` * `#number_to_human_size` -> `NumberToHumanSizeConverter` * `#number_to_human` -> `NumberToHumanConverter`
|
Anything else you guys need from me? /cc @rafaelfranca @steveklabnik |
|
Looks good, but I'd prefer @rafaelfranca to merge |
|
I need to check some eager load related things with @josevalim but the code is good. |
|
@rafaelfranca Awesome! |
|
Bump. |
|
@josevalim @rafaelfranca Any update on this? Would really love to see this merged soon. |
Refactor and clean up number helpers Conflicts: activesupport/lib/active_support/number_helper.rb
|
😍 Thanks @rafaelfranca! |
|
This change broke I'll open up an issue to track this. |
Reading through the source of the number helpers is a nightmare. There is very little that can be understood about the code aside from the documentation for each. This is intended to pull some of the hidden secrets of these helpers into their own classes.
NumberConverterobject.I don't think I am done as I still do not fully grasp a lot of the mathematical decisions being made, so help with naming would be much appreciated.