nugu-developers / nugu-ios

NUGU SDK for iOS
https://developers-doc.nugu.co.kr/
Apache License 2.0
21 stars 17 forks source link

Improve structure in `NuguClient` #70

Closed yHoonyK closed 4 years ago

yHoonyK commented 5 years ago

Overviews

Category

Summary

NuguClient.Builder() is always make all of capability-agent. Some capability-agent may not be used. (like LocationAgent)

Reference

Discussion(Internal): https://github.com/orgs/nugu-developers/teams/nugu-sdk/discussions/7

yHoonyK commented 4 years ago

It contains updating protocol for capability-agent. It is that about internal mandatory property.

yHoonyK commented 4 years ago

Proposal

Improve access control by NuguInterface

Branch

https://github.com/yHoonyK/nugu-ios/tree/feature/improve-nugu-client

Detail

public protocol ExtensionAgentProtocol:
CapabilityAgentable,
ContextInfoDelegate,
HandleDirectiveDelegate {
    /// The object that acts as the delegate of extension-agent
    var delegate: ExtensionAgentDelegate? { get set }
}
public protocol ExtensionAgentConcretable {
    var messageSender: MessageSendable! { get set }
}
final public class ExtensionAgent: ExtensionAgentProtocol, ExtensionAgentConcretable {}

If we want to use it like that, the coordinating role must be involved by the NuguClient.Builder. like this,

public class NuguClient {
   public let extensionAgent: ExtensionAgentProtocol?

   init(extensionAgent: ExtensionAgentProtocol?,
      ...
    ) {
       self.extensionAgent = extensionAgent
       ...
   }
}
extension NuguClient {
    public class Builder {
       public lazy var extensionAgent: (ExtensionAgentProtocol & ExtensionAgentConcretable)? = ExtensionAgent()

       public func build() -> NuguClient {
             let client = NuguClient(...)

             // coordinate for extension-agent
             extensionAgent?.messageSender = client.networkManager
             client.directiveSequencer.add(handleDirectiveDelegate: extensionAgent)
              ...
             return client
       }
   }
}
yHoonyK commented 4 years ago

Swift official answer for access-control protected: https://developer.apple.com/swift/blog/?id=11

yHoonyK commented 4 years ago

discussion(internal): https://github.com/orgs/nugu-developers/teams/nugu-sdk-ios-team/discussions/3

Minddol commented 4 years ago

Suggestion - Use protocol initializer instead of property.

public protocol ExtensionAgentProtocol:
CapabilityAgentable,
ContextInfoDelegate,
HandleDirectiveDelegate {
    init(messageSender: MessageSendable)

    /// The object that acts as the delegate of extension-agent
    var delegate: ExtensionAgentDelegate? { get set }
}
final public class ExtensionAgent: ExtensionAgentProtocol {
    private let messageSender: MessageSendable

    public init(messageSender: MessageSendable) {
        self.messageSender = messageSender
    }
}
    let client: NuguClient = {
        let builder = NuguClient.Builder()
        return builder
            .with(extensionAgent: ExtensionAgent(messageSender: builder.networkManager))
            .build()
    }()
yHoonyK commented 4 years ago

I think @Minddol's suggestion is the best way.

yHoonyK commented 4 years ago

Proposal

Improve issue that NuguClient.Builder() is always make all of capability-agent.

Concept

extension NuguClient.Builder {
    enum DefaultType {
        case .nothing // no has default capability-agent instance (only has `SystemAgent`)
        case .basic // has basic capability-agent instances (`SystemAgent`, `ASRAgent`, `TTSAgent`, `TextAgent`..?)
        case .complete // has all capability-agent instances
    }
}
class NuguClient.Builder {
    init(defaultType: DefaultType) {}
}
let client = NuguClient.Builder(defaultType: .basic).build()
yHoonyK commented 4 years ago

Builder pattern with director. https://johngrib.github.io/wiki/builder-pattern/

yHoonyK commented 4 years ago

I consider to change architecture to 'abstract factory pattern' for capability-agent.

yHoonyK commented 4 years ago

Check-list

yHoonyK commented 4 years ago

Revert renaming capability-agents.

yHoonyK commented 4 years ago

Summary

Issue

  1. NuguClient.Builder() is always make all of capability-agent.
    • Resolved: To inject capability-agent has be changed design-pattern to 'Abstract Factory Pattern'
  2. capability-agent's access-control issue. (public properties in protocol)
    • Resolved: Remove properties to inject dependency using 'setter injection' in capability-agent protocol, concrete class has inject dependency using 'constructor injection'.

Result

yHoonyK commented 4 years ago

Resolved by #147

yHoonyK commented 4 years ago

It wasn't be completed.

yHoonyK commented 4 years ago

Improve capability-agent protocol structure to PoP (Protocol Oriented Programming)

yHoonyK commented 4 years ago

Resolved by #170

After that, we consider that NuguClient change design-pattern to IoC container. It will makes another github issue.