onmyway133 / EasyTheme

👕👚 Theme management in Swift
https://onmyway133.com
Other
242 stars 19 forks source link

Suggestion: rename API, clearer examples #2

Closed SuperWomble closed 7 years ago

SuperWomble commented 7 years ago

Firstly, thank you for this project.

I found it quite difficult to understand the usage and design of this framework, without having to go in to the code. Mostly, this was due to the framework's current naming practices.

For example, I didn't know what a "Manager" was. Was it app-dependent? View-Controller dependent? What does it "manage"?

To that end, I suggest changing the name to "ThemeManager" or "ThemesManager". And/or use a prefixing scheme.

e.g. OMYThemesManager

Similarly, the use of "theme()" is very problematic. e.g. given the following code:

theme(MyTheme.self) {
    // ...
}

Some questions I had: where did this seemingly global method come from? What does it do? Am I creating a theme? Am I setting a theme? I have to use a "Manager" at one point, but I don't use Manager to set the theme? So if I don't have to use the Manager, and I have a theme, why don't I just call myTheme.set() ? Who does "theme()" belong to? Is it local, global? Part of a framework? etc. And why am I giving it a "MyTheme.self"? What is the action that occurs? These important questions can't be answered by the current naming scheme.

Upshot: please consider renaming the API to use clearly readable nouns and verbs. The goal should be to make it crystal clear what any method/class will do.

This problem also occurs in the Read Me example. You use a type called "MyTheme". This indicated to me that it was a one-off; a particular instance of a theme. In reality, it turned out to be a general type, designed for (possible) multiple use.

It's hard to think of a good naming system for this kind of protocol adoption. The goal is to make it clear that the type itself is not the theme.

Perhaps something like a "Template", which indicates that any given struct is designed for multiple use.

e.g. given a project of name "Project":

struct OnboardingThemeTemplate: OMYTheme {
  let topImage: UIImage
  let cellColor: UIColor
  let backgroundColor: UIColor
  let name: String
  let titleFont: UIFont
  let subtitleFont: UIFont
}

struct FeedbackThemeTemplate: OMYTheme {
  let backgroundColor: UIColor
  let name: String
  let titleFont: UIFont
  let subtitleFont: UIFont
}

struct ProjectThemes {
  static let onboarding = OnboardingThemeTemplate(...)
  static let mailFeedback = FeedbackThemeTemplate(...)
  static let twitterFeedback = FeedbackThemeTemplate(...)
  static let facebookFeedback = FeedbackThemeTemplate(...)
}

Or something like that. Hopefully you get the idea; a clear separation and identification of framework naming and other code.

IMO, you should err on the side of readability, comprehension, rather than brevity and minimalism.

Thank you.

onmyway133 commented 7 years ago

@SuperWomble Hi, many many thanks for your detail suggestions.

  1. Manager is used to define the current theme, I think I should name it to ThemeManager.currentTheme to be more clear

  2. I'm sorry for your confusion, but you have your point. Theme is like a contract, you create different theme depending on our usage. It behaves more like a template

  3. theme is a function from ThemeUser, that whenever theme changes, it will call this block again. Perhaps I should name it like use(MyTheme.self) ?

  4. The use of MyTheme.self is your way of specify which theme you would like to use. You can create many many themes, but that is not recommended

SuperWomble commented 7 years ago

Cheers! Thank you for the response, and elaboration.

onmyway133 commented 7 years ago

@SuperWomble Hi, I did a rename https://github.com/onmyway133/Themes/releases/tag/1.1.0

About Theme as being a Template, I see there are 2 ways of thinking, both have their points