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

DateInRegion init() with multiple way to express a single concept #123

Closed
malcommac opened this issue Dec 23, 2015 · 10 comments
Closed

DateInRegion init() with multiple way to express a single concept #123

malcommac opened this issue Dec 23, 2015 · 10 comments

Comments

@malcommac
Copy link
Owner

@Hout I'm thinking about the new init in DateInRegion:

public convenience init?(
        fromString date: String,
        format: DateFormat,
        calendarID: String = "",
        timeZoneID: String = "",
        localeID: String = "",
        calendarType: CalendarType? = nil,
        timeZoneRegion: TimeZoneConvertible? = nil,
        calendar aCalendar: NSCalendar? = nil,
        timeZone aTimeZone: NSTimeZone? = nil,
        locale aLocale: NSLocale? = nil,
        region aRegion: DateRegion? = nil) {
...
}

As you said rightly some time ago we should keep this library simple to use.
Here we have three different kind of parameters to express a single concept; for example we have calendarType, calendarID and calendar to express the same thing in different modes.
My idea is to use a single mode; just for example we could accept only CalendarType and create init methods to create a new CalendarType from ID string or an NSCalendar instance,keeping only one way to specify a calendar (the same thing is valid for timezone).
What do you think?

@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

You are correct! :) I wanted to make it simple to use but you certainly have a point. On the other side it is just handy to be able to initialise with any mix of parameter types as long as it produces a consistent date.

To stop confusion between calendars, time zones and locales, a purer version would be:

public convenience init?(
        fromString date: String,
        format: DateFormat = nil, // nil = medium style
        region aRegion: DateRegion? = nil) // nil = local

Better?

@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

The advantage of one big initialiser is that it can be used in various ways. E.g.:

let region1 = DateRegion() // returns local region
let region2 = DateRegion(tzName: .Asia.Shanghai) // returns time zone for Shanghai
let region3 = DateRegion(timeZoneRegion: .America.Santiago) // returns time zone for Santiago
let region4 = DateRegion(calendar: ethiopianCalendar, timeZone: ethiopianTimeZone, locale: EthiopianLocale) // an Ethiopia region
let region5 = DateRegion(localeID: "ti_ER", region: ethiopianRegion) // Eritrea region

If we split this up into dedicated initialisers, it is not possible to use default parameters on each as it would create ambiguity. I.e. the compiler would not be able to determine which initialiser DateRegion() would refer to.

Alternative 1:
We could keep one initialiser with the core parameters (NSCalendar, NSTimeZone, NSLocale) and introduce helper functions to fill it up. E.g.

let region1 = DateRegion() | "uk" | .Europe.Kiev | .Gregorian
let region2 = DateRegion() | "en_UK" | "BST" | .Gregorian

Alternative 2:
As above. But with a string input which is parsed for time zone id's, locale id's and calendar id's:

init(idStrings String...) {}

let region3 = DateRegion(NSCalendarIdentifierHebrew, "America/Adak", "en_US")
let region4 = DateRegion(timeZone: .Asia.Magadan) // with local calendar & locale

Other ideas?

@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

Or maybe even better: init(initObjects: DateRegionSpecifier...). Where the region is built based on the objects. Objects can be NSCalendar, NSTimeZone, NSLocale, CalendarType, TimeZoneConvertible, String (calendarID, localID, timeZoneAbbreviation, timeZoneName).

@Hout Hout closed this as completed Dec 24, 2015
@Hout Hout reopened this Dec 24, 2015
@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

@malcommac I created a branch. Check this one: 4dc8743. This keeps things simple and flexible.

@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

A best practice question is valid here too. Assume a class Class which contains main properties a, b and c that can be built in a number of ways. Would we prefer:

  • Class(a: b: c:) next to Class(d:e:f:) and Class(g:h:i:) where each of the parameters d..i can be optional, but one should be mentioned to prevent ambiguity. (this applies to DateRegion in the current develop branch.)
  • Class(a: b: c:) next to Class(_ x...) where x is an array of objects that can be anything that conforms to a protocol ClassSpecifier. The latter initialiser constructs Class object from the objects in x.
  • Maybe another solution?

@malcommac
Copy link
Owner Author

I would tend to discourage the use of several init parameters to express the same concept because it increases the complexity of the library.
Instead I prefer to set strong constraints into the library: for example in order to specify a calendar in init method you need to pass a single type of object (and to keep the lib flexible you can init this object in different ways).
Let me show an example:
For calendar param we set as constraint CalendarType: you can pass to the function only this type.
However you can create it starting from an enum, a string or an object.
So our init will become:

public convenience init?(
        fromString date: String,
        format: DateFormat,
        calendarType: CalendarType? = nil,
        timeZoneRegion: TimeZoneConvertible? = nil,
        locale aLocale: NSLocale? = nil,
        region aRegion: DateRegion? = nil)

And you can specify a CalendarType in 3 different ways:

  • from the enum (example: CalendarType.Gregorian)
  • from an NSCalendar instance (CalendarType.NSCalendar(object: instance-of-nscalendar) )
  • from a specified string (CalendarType.NSCalendar(identifier: String) )

So we have a single passed object type without losing the flexibility of the lib.
(I've used CalendarType and not the NSCalendar object as parameter type because this and TimeZoneConvertible are our main data types and are easy to write using swift's autocompletion).

@Hout
Copy link
Contributor

Hout commented Dec 24, 2015

Hmmm, I can go with you on this one. My main concern with the TimeZones is that I want to be able to use an abbreviation in a quick way.

Anyway I would tend to make it even more consistent and leave any duplication out of the parameters to get the required clarity. I.e. remove the calendar, time zone and locale references and do the same thing you mention above into the region and do the same trick there.

Or the other way around: get rid of the region and make calendar, time zone and locale explicitly defined in all functions. I would be in favour of the former as in the real world we would want the date conversion to be from a specific location (date region) and deal with that level of abstraction as opposed to multiple objects like time zone, calendar and/or locale.

The code would become:

public convenience init?(
        fromString date: String,
        format: DateFormat,
        region aRegion: DateRegion? = nil)

In case of quick partial regions (e.g. just time zone conversion) the code would become:

let str = NSDate().toString("dd-MMM-yy", region: DateRegion(timeZoneRegion: .Europe.Paris))

Do you agree on this approach?

@malcommac
Copy link
Owner Author

I agree with your proposed init with DateRegion which encapsulate timezone/calendar/locale params; it's really less confusing.
Only one remark: what do you think about renaming DateRegion in Region (or another better name)?
This because DateRegion will become more visible and I would to avoid confusion between DateInRegion and DateRegion.
What do you think?

@Hout Hout mentioned this issue Dec 25, 2015
@Hout
Copy link
Contributor

Hout commented Dec 25, 2015

DateRegion naming continued in #127

@Hout
Copy link
Contributor

Hout commented Dec 25, 2015

Ok let us proceed with this paradigm then 👍

@Hout Hout closed this as completed Dec 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants