10
\$\begingroup\$

I am learning Swift and object oriented programming. I am new to design patterns and protocol oriented programming.

In my test code I have several related objects:

  • Security: the most important object that will be contained in the other ones.
  • AssetClass: Contains Security objects
  • Portfolio: Contains Security and AssetClass objects
  • Watchlist: Contains Security objects. Think about it as a Portfolio object that cannot contain AssetClass objects, only Security objects.

I want to be the most abstract as I can to avoid tight coupling.

The following code works but I am wondering how it can be improved. Do you see major weak points in my design?

import Foundation

protocol SecurityType{
    var code : String {get set}
    var name : String? {get set}
}

class Security:SecurityType{

    var code : String = ""
    var name : String?

    init(withCode aCode: String, andName aName: String?){
        self.code = aCode
        if let securityName = aName{
            self.name = securityName
        }
    }
}

protocol SecuritiesBasketType{
    var name : String {get set}
    var securities : [SecurityType]{get set}
    init()
}

extension SecuritiesBasketType{

    //Default Initializers

    init(withName aName: String){
        self.init()
        self.name = aName
    }

    init(withName aName: String, andSecurities securitiesArray:[SecurityType]){
        self.init()
        self.name = aName

        for aSecurity : SecurityType in securitiesArray{
            securities.append(aSecurity)
        }
    }

    mutating func addSecurity(security: SecurityType){
        securities.append(security)
    }
}

class AssetClass : SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

class WatchList: SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}


class Portfolio{

    var name : String = ""
    var securities = [Security]()
    var assetClasses = [AssetClass]()

    init(withName aName: String){
        self.name = aName
    }

    init(withName aName: String, andAssetClasses assetClasses:[AssetClass]){
        self.name = aName
        for assetClass in assetClasses{
            self.assetClasses.append(assetClass)
        }
    }

    init(withName aName: String, andAssetClasses assetClasses:[AssetClass], andSecurities securities:[Security]){

        self.name = aName

        for assetClass in assetClasses{
            self.assetClasses.append(assetClass)
        }

        for security in securities{
            self.securities.append(security)
        }
    }

}

//Creating Securities
var sMicrosoft = Security(withCode: "MSFT", andName: "Microsoft")
var sApple = Security(withCode: "AAPL", andName: "Apple")
var sJPMorgan = Security(withCode: "JPM", andName: "JP Morgan")
var sBankOfAmerica = Security(withCode: "BAC", andName: "Bank of America")
var sCaterpillar = Security(withCode: "CAT", andName: "Caterpillar")

//Creating Asset Classes
var acBanks = AssetClass(withName: "Banks", andSecurities: [sJPMorgan, sBankOfAmerica])
var acTechnology = AssetClass(withName: "Technology", andSecurities: [sMicrosoft, sApple])

//Creating Watchlists
var watchList1 = WatchList(withName: "List1", andSecurities: [sMicrosoft,sJPMorgan,sCaterpillar])
var watchList2 = WatchList(withName: "List2", andSecurities: [sBankOfAmerica,sApple])

//Creating Portfolios

var portfolio1 = Portfolio(withName: "Portfolio1", andAssetClasses: [acBanks,acTechnology])
var portfolio2 = Portfolio(withName: "Portfolio2", andAssetClasses: [acBanks, acTechnology], andSecurities: [sCaterpillar])

//Test

for ac : AssetClass in portfolio2.assetClasses{
    for s : SecurityType in ac.securities{
        print("\(s.name!)")
    }
}
for s : SecurityType in portfolio2.securities{
    print("\(s.name!)")
}
\$\endgroup\$
  • \$\begingroup\$ Original on SO \$\endgroup\$ – Mast Feb 7 '16 at 17:41
  • 1
    \$\begingroup\$ yes, it's where they told me to ask here :) I added a comment on SO telling people I was going to move the question here. I deleted the one on SO \$\endgroup\$ – Nicola Prada Feb 7 '16 at 17:43
  • \$\begingroup\$ Very well, welcome to Code Review :-) \$\endgroup\$ – Mast Feb 7 '16 at 17:52
  • \$\begingroup\$ Please don't add user responses to the question. Post comments below the user's answer or enter a chatroom. \$\endgroup\$ – Jamal Feb 18 '16 at 6:35
4
\$\begingroup\$

I've got a problem with your SecuritiesBasketType protocol and its extension. The problem is best highlighted by taking a look at the multiple classes you've implemented to conform to this protocol.

class AssetClass : SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

class WatchList: SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

There are a couple of problems here. First, why do we need two classes that are identical in every way except name? We don't.

I understand that we will want assets that we actually own as well as assets that we're watching to help us make future decisions, but making different classes like this simply does not make sense. Additionally, this name property makes relatively little sense, and having a zero-argument constructor plus var properties is problematic as far as I'm concerned.

But we must have the zero argument constructor because of the problem with the protocol. We have conformed to the SecuritiesBasketType, which has a zero-argument initializer as part of its conformance. This means that everything that conforms to this protocol as well as things that inherit from that must implement this zero-argument constructor with the required keyword.

In some cases, a required initializer makes sense. Like, for example, with the NSCoding protocol (more reading). But here, we've only required this zero argument constructor so that we can add two other constructors in the protocol extension.

But WHY?

What you have here is a protocol that should be a class. You've done all of the work in the protocol and protocol extension, and it has resulted in very weird things. It doesn't make much sense to subclass anything that conforms to this protocol and it doesn't make much sense to create more than one class that conforms to this protocol. And when those two things are true (and they are here), then it doesn't make much sense to have the protocol at all. We should instead have a class that looks something like this:

class SecuritiesBasket {
    let name: String
    var securities: [SecurityType]

    init(withName name: String, securities: [SecurityType]) {
        self.name = name
        self.securities = securities
    }

    func addSecurity(security: SecurityType) {
        securities.append(security)
    }
}

Now the SecuritiesBasket class does everything we wanted out of the SecuritiesBasketType protocol, but it does it without forcing any required initializer (which severely dictates a lot of the things you do with subclasses from that point).


In Swift, we want to adopt a protocol-oriented approach to programming. Protocols allow us to do some powerful things. But with that said, we need to use the right tool for the right job.

We use protocols to allow multiple implementations of the same interface. The most classic example is with UITableView and its delegate/datasource properties. All of the UI logic of the table view exists within UIKit, yet with just a handful of methods in the protocols, the table view can adapt to display whatever information you need it to display.

\$\endgroup\$
  • \$\begingroup\$ You have a valid point about using a single class for AssetClass and Watchlist objects .They share and will share the 95% of the code but my idea was to separate them to be free to add different protocols to them in the future. I thought this could add flexibility. As an example think about the capability to have a targetWeight expressed as a percentage of a Portfolio, something useful in an AssetClass but useless in a Watchlist. \$\endgroup\$ – Nicola Prada Feb 19 '16 at 13:39
  • \$\begingroup\$ But you're right: this would be a bad design choice because an AssetClass object could be linked to many Portfolio objects with a different targetWeight value for each one of them. So, as you said, it would be better to keep a single SecurityBasket object instead of AssetClass and WatchList objects and create another object to link a SecurityBasket to a Portfolio: class AssetClassInPortfolio{ var assetClass: SecurityBasket var portfolio: Portfolio var targetWeight: Double = 0.0 } \$\endgroup\$ – Nicola Prada Feb 19 '16 at 13:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.