Skip to content
This repository has been archived by the owner on Mar 29, 2018. It is now read-only.

Format Number #86

Closed
wants to merge 3 commits into from
Closed

Conversation

PGLongo
Copy link
Contributor

@PGLongo PGLongo commented Mar 10, 2015

Hi,

I have added a simple method to format numbers (Double, Int, Float) using a NSNumberFormatter saved as a class var in a new class ExSwiftFormatter because formatters are expensive to create.

Tell me if you are interested in this PR before merging, I will add doc and update the readme.

I have also added a NSNumberFormatter extension to set the formatter precision.

formatter.setPrecision(3)

instead of

formatter.minimumFractionDigits = 3
formatter.maximumFractionDigits = 3
var price:Double = 12356789.424
var formatter = ExSwiftFormatter.numberFormatter
formatter.numberStyle = .DecimalStyle
formatter.setPrecision(3)
price.format() // "12,356,789.424"

formatter.numberStyle = .CurrencyStyle
price.format() // "$12,356,789.424"

If you agree I would like to add other formatters and also unit converters

@pNre
Copy link
Owner

pNre commented Mar 11, 2015

Thanks, allow me a couple of days to look into the PR before merging.

@PGLongo
Copy link
Contributor Author

PGLongo commented Mar 11, 2015

No problem, take your time.

@pNre
Copy link
Owner

pNre commented Mar 14, 2015

There are a couple of things you need to review:

  1. You can't import UIKit in OS X targets (also, UIKit is not needed by these extensions)
  2. The ExSwiftFormatter class can be removed in favor of a static property in one of the Float, Int, Double extensions (or even a property for each of them).
  3. I would also allow the use of different formatting behavior and number style passing them as optional parameters to the format method.

@PGLongo
Copy link
Contributor Author

PGLongo commented Mar 14, 2015

The ExSwiftFormatter class can be removed in favor of a static property in one of the Float, Int, Double extensions (or even a property for each of them).

What do you think about adding the formatter to NSNumber?

I would also allow the use of different formatting behavior and number style passing them as optional parameters to the format method.

My first implementation allowed different formatting but then I noticed that there are a lot of possible configurations (grouping, separator, style, currency, ... ) so I considered easier and more reusable allowing editing the formatter and invoke format. Maybe another possible solution is to add an NSNumberFormatter as a parameter of format

@pNre
Copy link
Owner

pNre commented Mar 14, 2015

I would also allow the use of different formatting behavior and number style passing them as optional parameters to the format method.

My first implementation allowed different formatting but then I noticed that there are a lot of possible configurations (grouping, separator, style, currency, ... ) so I considered easier and more reusable allowing editing the formatter and invoke format. Maybe another possible solution is to add an NSNumberFormatter as a parameter of format

I didn't think this trough, there isn't a safe way to add this extension without losing the advantages you get by having a shared instance.

Exposing the formatter or mutating it in any way (by passing any parameter to the format extension) raises thread-safety concerns, on the other hand an immutable/private formatter is nearly useless.

I'm sorry but, unless you have a way to solve these problems, I can't merge this PR.

@PGLongo
Copy link
Contributor Author

PGLongo commented Mar 14, 2015

I think that a shared instance is necessary.

If you use the same format in the entire project you can access to the formatter once (e.g. in the AppDelegate) set the behavior an then invoke format() easily. Otherwise if you have different formats it's necessary to set the behavior every time, but this operation is better than alloc a new formatter and this problem remains with or without this PR.

if you don't agree it's not necessary to merge this PR, I will create my own project.

@pNre
Copy link
Owner

pNre commented Mar 16, 2015

The point is that every time you alter the shared formatter you need to synchronize the operation across the threads using the instance.

As I wrote before, can't merge this (the way it is), sorry.

@pNre pNre closed this Mar 16, 2015
@PGLongo
Copy link
Contributor Author

PGLongo commented Mar 16, 2015

Ok! Thanks anyway

@PGLongo PGLongo deleted the feature/NSNumberFormatter branch March 26, 2015 08:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants