3
\$\begingroup\$

I have a class that I use to manage a list to/from an API in Swift. Using a clean code approach, I'm wondering if, how, and why I should change it when I have a class that uses it for just one or two functions.

Here's the class:

import Foundation

protocol ApiWishListManagerDelegate
{
    func didFailWishList(failError: NSError?)
    func didGetWishList()
    func didAddToWishList()
    func didUpdate()
    func didDelete()
}

class ApiMyAccountWishListManager: ApiController
{
    // MARK: - Vars
    var delegateWishList: ApiWishListManagerDelegate?

    // MARK: - Inits

    override init()
    {
        super.init()
    }

    init(delegateWishList: ApiWishListManagerDelegate)
    {
        super.init()
        self.delegateWishList = delegateWishList
    }

    // MARK: - Functions

    func addToWishList(productId: Int)
    {
        var dataToPost = NSDictionary(objects: [productId], forKeys: ["pid"])
        post(MakeURL.addToWishList(), postData: dataToPost, callBack: parseAddToWishList)
    }

    func parseAddToWishList(data: NSData?, response: NSURLResponse, error: NSError?)
    {
        if let jsonResultDictionary = convertOnSuccessAPI(response, data: data!)
        {
            self.delegateWishList!.didAddToWishList()
            return
        }
        self.delegateWishList!.didFailWishList(error)
    }

    func readItems()
    {
        get(MakeURL.getWishList(), queryString: "", https: true, callBack: parseReadItems)
    }

    func parseReadItems(data: NSData?, response: NSURLResponse, error: NSError?)
    {
        if let jsonResultDictionary = convertOnSuccessAPI(response, data: data!)
        {
            MyAccountDataCache.sharedInstance.saveWishList(jsonResultDictionary)
            self.delegateWishList!.didGetWishList()
            return
        }
        self.delegateWishList!.didFailWishList(error)
    }

    func updateItemQuantity(productId: Int, quantity: Int, folderId: Int)
    {
        var dataToPost = NSDictionary(objects: [productId, quantity, folderId], forKeys: ["pid", "prodQty", "srcFolderID"])
        post(MakeURL.chageQuantity(), postData: dataToPost, callBack: parseUpdateItemQuantity)
    }

    func parseUpdateItemQuantity(data: NSData?, response: NSURLResponse, error: NSError?)
    {
        if wasCallSuccessfulAPI(response) && data?.length == 0
        {
            self.delegateWishList!.didUpdate()
            return
        }
        self.delegateWishList!.didFailWishList(error)
    }

    func deleteItemFromFolder(productId: Int, folderId: Int = 0)
    {
        var dataToPost = NSDictionary(objects: [productId, folderId], forKeys: ["pid", "srcFolderID"])
        put(MakeURL.removeProduct(), postData: dataToPost, queryString: "", callBack: parseDeleteItemFromFolder)
    }

    func parseDeleteItemFromFolder(data: NSData?, response: NSURLResponse, error: NSError?)
    {
        if let jsonResultDictionary = convertOnSuccessAPI(response, data: data!)
        {
            self.delegateWishList!.didDelete()
            return
        }
        self.delegateWishList!.didFailWishList(error)
    }
}

The main question is when a class is using this manager, for something like just getting the wishlist, we have a set of empty delegates like this:

    // MARK: - ApiWishListManager

    func didFailWishList(failError: NSError?)
    {
        hideSpinner()
    }

    func didGetWishList()
    {
        mainTableView.reloadData()
        hideSpinner()
    }

    func didAddToWishList()
    {
    }

    func didUpdate()
    {
    }

    func didDelete()
    {
    }

The add, update and delete are just sitting there never to be used. I know about making them optional, using @objc, but I don't really want to add all the extra code that goes with checking if the delegate is setup before calling each one. I've tried adding delegates within the same class, but I think that just starts looking too ugly and makes extra code for the classes that need all of the functions of this one manager.

\$\endgroup\$
3
\$\begingroup\$

We should definitely go ahead and use @objc and make the methods optional that make sense to be optional.

In Swift, it's actually not nearly as bad to check whether or not a delegate is set up or responds to a selector. In Objective-C, it was a big if statement. In Swift, we can use optional chaining.

Suppose that didUpdate is an optional method. Calling it on our delegate then looks like this:

self.delegate?.didUpdate?()

The only difference here is the ? before the parenthesis.

What this line of code says is basically:

if delegate is not nil and if delegate responds to selector "didUpdate"
    call "didUpdate" on delegate
else
    do nothing

The other important thing here is that we need to be sure that we're making our reference to the delegate a weak reference, which can only be done if our protocol is specified as @objc. In almost every case, the object acting as our delegate is already holding a strong reference to the class that needs the delegate. If this class also holds a strong reference back to the delegate (which you're currently doing), we've creating a retain cycle. The memory can never be released without annoying setting each other's reference to the other to be nil... and the hard part is often times figuring out when to do that.

So, let's make our protocol be an @objc protocol and use a weak reference to our delegate, and then use optional chaining to take care of the optional methods.

\$\endgroup\$

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.