chore: added builder pattern to BKTConfig #13
Conversation
| @@ -1,6 +1,6 @@ | |||
| import UIKit | |||
|
|
|||
| public struct BKTConfig { | |||
| public class BKTConfig { | |||
There was a problem hiding this comment.
BKConfig
-
It has a property named
loggerwhich could be an instance of a class.
So It should be a class. Because a Struct's property should be only basic data_type, not a class. -
Next, we put a
Builderclass as an inner class here and we want to use theSwift convenience initto reuse the baseinitmethod ofBKConfig, andBuilderwill not know too much about how to init the outer class.
There was a problem hiding this comment.
Typically, Builder patterns are not common in iOS development. Still, for SDKs, this pattern is helpful when we want to add or edit some configuration without breaking changes, especially for SDKs working as a proxy such as Flutter.
There was a problem hiding this comment.
fixed by remove the breaking changes
| logger: builder.logger) | ||
| } | ||
|
|
||
| public class Builder { |
There was a problem hiding this comment.
Builder as an inner class.
The benefit here, every time the consumer wants to create a BKConfig by entering BKConfig.
The IDE will show a suggestion to use the Builder
|
Hi @cre8ivejp Could you please take a look on this changes |
| @@ -1,6 +1,6 @@ | |||
| import UIKit | |||
|
|
|||
| public struct BKTConfig { | |||
| public class BKTConfig { | |||
There was a problem hiding this comment.
Typically, Builder patterns are not common in iOS development. Still, for SDKs, this pattern is helpful when we want to add or edit some configuration without breaking changes, especially for SDKs working as a proxy such as Flutter.
| /** | ||
| * Create a new builder with your API key. | ||
| */ | ||
| public init(apiKey: String) { |
There was a problem hiding this comment.
It would be better if we don't pass the apiKey.
Usually, for builder patterns, it passes the required parameters in the init, and the optional settings are passed via .with. Or, we pass nothing in the init.
Since you have already added the .with for the apiKey, let's remove it from the init.
https://github.com/bucketeer-io/ios-client-sdk/pull/13/files#diff-a01b127e0bdeebe208c5ce7cac0dfd53041a03921bc362c81bdb3954f333d81eR100-R103
There was a problem hiding this comment.
Hi, @cre8ivejp the builder patterns are not common in ObjectC.
But it is popular with Swift.
The idea when init with apiKey is: it is an indicator for something so important and should be set by default, it should not an optional.
There was a problem hiding this comment.
I understand.
My previous message mentioned passing the required configuration. But, If we are going to pass the apiKey, we should also pass the apiEndpoint and the appVersion. But, to keep the consistency, we should do it as Android.
What do you think?
There was a problem hiding this comment.
fixed by remove init with apikey
|
|
||
| final class BKTConfigTests: XCTestCase { | ||
|
|
||
| func testCreateConfig() { |
There was a problem hiding this comment.
Can you also add the following cases to check the exceptions?
https://github.com/bucketeer-io/ios-client-sdk/pull/13/files#diff-a01b127e0bdeebe208c5ce7cac0dfd53041a03921bc362c81bdb3954f333d81eR134-R145
There was a problem hiding this comment.
sure, I will add it.
In the current test, if the user missing something, the BKTConfig will be nil
There was a problem hiding this comment.
added. Please continue review @cre8ivejp
cre8ivejp
changed the title
Changes
BKTConfigcould only init usingBKTConfig.Builder.sdk_versionfromBKTConfig.Builder. The SDK's consumer no longer needs to set the SDK version.