Code review

[wattson12]

Summary

Hey, we spoke on nomad list slack. This is a 10 minute review so take any feedback with a grain of salt, but I hope its useful! I think the static vs instance point is the biggest thing I noticed, along with a lack of tests

The other general stuff I noticed included naming: e.g. action1 could have a clearer name. Default values: optional or required values for some config might be safer, since an empty string might just cause unexpected runtime issues.

PoliteReview.swift

• Internal: internal is the default so no need for this (feels a bit noisy to me)
• Enum naming: convention should be camel case starting with lower case, e.g. politeReviewTotalLaunchCount
• Static vs instance: I find the mix of static storage with instance methods used to setup strange. Especially when it results in code like PoliteReview().setup... (instance created but never stored). It would also prevent situations like calling requestPoliteReview before an app name or contact email etc had been setup. Having an instance would also make it easier to inject properties like which user defaults to use, to simplify testing
• Synchronize: synchronize is in a pseudo deprecated state and is usually not required (the discussion is not in docs but check out NSUserDefaults.h for details

RequestPoliteReview.swift

• Localisation: You could use a format string as the localised value, so that if the language would display the name in a different order that can be localised as well. Also "this app" isn't localised
• userDefaults(): The name of this function doesn't clearly describe what it does

[kevinchau]

Thanks @wattson12, I've resolved these issues, minus the Format Strings - will review that again in the future.

Tests are also coming soon, will resolve that in #3