Update the API to use class method. #4

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

So it could be used like this:

   [SORelativeDateTransformer transformedValue:[datePicker date]]];

Zhigang, thanks for this work. It is a compact way to get a transformed value.

My only concern is that your approach does ignore the capabilities already built into NSValueTransformer.

In particular, NSValueTransformer maintains an internal cache of all registered value transformers. The +setValueTransformer:forName: method lets you explicitly register a single instance of a given transformer that can then be used throughout an application.

I had given some thought to making a convenience class method such as you've created here, but decided against it. It is pretty easy to use the existing NSValueTransformer API to implement what you want to do.

For example:

+ (id) transformedValue:(id)value
{
    SORelativeDateTransformer *registered;
    registered = [NSValueTransformer valueTransformerForName:@"SORelativeDateTransformer"];
    return [registered transformedValue:value];
}

The NSValueTransformer class method +valueTransformerForName: looks for a registered transformer with the given name. If it doesn't find an already registered instance, it will create one and cache it. The next invocation of +valueTransformerForName: would then return the now-cached instance.

I believe it would be a better API design for me to add a +registeredTransformer class method to SORelativeDateTransformer that wraps the NSValueTransformer API into something moderately less lengthy to type.

+ (id) registeredTransformer
{
    return [NSValueTransformer valueTransformerForName:NSStringFromClass(self)];
}

From anywhere in an application, then, a transformed date value could be obtained using something like

NSDate *someDate = ...;
NSString *dateText = [[SORelativeDataTransformer registeredTransformer] transformedValue:someDate];

The NSValueTransformer API is already verbose and the name "SORelativeDateTransformer" doesn't do much to make it less so. A +registeredTransformer class method might help a little.

Alternately, you could define a global inline function for use in your app:

inline NSString *RelativeDateStringFromDate(NSDate *date)
{
    return [[NSValueTransformer valueTransformerForName:@"SORelativeDateTransformer"] transformedValue:date];
}

or a macro

#define RelativeDateStringFromDate(date) [[NSValueTransformer valueTransformerForName:@"SORelativeDateTransformer"] transformedValue:date]

Your approach of explicitly creating a static instance is fine, but does ignore NSValueTransformer's existing caching feature. I'd rather keep the class as simple as possible and not duplicate existing functionality. I'm going to reject this pull request, but let me know if you think the +registeredTransformer method would be a reasonable substitute for use in your app. If so, I'll put that in.

Thanks again,

Bill

Owner

zhigang1992 replied Jun 12, 2013

MailBox just messed up github reply, sorry about that.

Wow, that's really really detailed reply.
Thank you...
​Honestly, I didn't know about valueTransformerForName. ​All I'm aware of is creating new instance every time I need one... Which is not cool at all. ​ ​
In my opinion, less advanced user like me, we just want to get the string from date. And since it returns the same string for the same date... Multiple instance doesn't make much sense . ​
As for using valueTransformerForName or static value. valueTransformerForName has my vote...;)
And if class method couldn't work. Macro or global inline function should be provided by the transformer instead of let user come up with their own.
And if none of it could work. Please update the example code to use other than alloc init, it's kind misguiding.

Cheers.

Owner

billgarrison commented Jun 22, 2013

I've added a class method +registeredTransformer that will give you an instance of SORelativeDateTransformer cached globally by NSValueTransformer. The readme and example app now use `[SORelativeDateTransformer registeredTransformer] to get a working instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment