Maintainable networking layer architecture

Question

There are so many different solutions and examples on how to build a proper networking layer, but every app has different constraints, and design decisions are made based off trade-offs, leaving me uncertain about the quality of code I've written. If there are any anti-patterns, redundancies, or flat out really bad solutions within my code that I have overlooked or simply lacked the knowledge to address, please do critique. This is a project for my portfolio so I'm posting it here to get as many eyes on it as possible, with some advice/ tips.

Some characteristics of my networking layer that I think could raise eyebrows:

Method contains a GETALL case, to indicate a list of data must be fetched. I have not seen this in any of the open source code i've read. Is this a code smell?

enum Method {
    case GET
    /// Indicates how JSON response should be handled differently to abastract a list of entities
    case GETALL
    case PUT
    case DELETE
}

I've made it so each Swift Entity conforms to JSONable protocol meaning it can be initialized with JSON and converted to JSON.

protocol JSONable {
    init?(json: [String: AnyObject])
    func toJSON() -> Data?
}

JSONable in practice with one of my entities:

struct User {
    var id: String
    var name: String
    var location: String
    var rating: Double
    var keywords: NSArray
    var profileImageUrl: String
}

extension User: JSONable {
    init?(json: [String : AnyObject]) {
        guard let id = json[Constant.id] as? String, let name = json[Constant.name] as? String, let location = json[Constant.location] as? String, let rating = json[Constant.rating] as? Double, let keywords = json[Constant.keywords] as? NSArray, let profileImageUrl = json[Constant.profileImageUrl] as? String else {
            return nil
        }
        self.init(id: id, name: name, location: location, rating: rating, keywords: keywords, profileImageUrl: profileImageUrl)
    }

    func toJSON() -> Data? {

        let data: [String: Any] = [Constant.id: id, Constant.name: name, Constant.location: location, Constant.rating: rating, Constant.keywords: keywords, Constant.profileImageUrl: profileImageUrl]
        let jsonData = try? JSONSerialization.data(withJSONObject: data, options: [])
        return jsonData
    }
}

This allows me use generics to initialize all my entities in my client- FirebaseAPI, after I retrieve JSON response. I also haven't seen this technique in code I've read.

In the following code, notice how GETALL is implemented to flatten list of JSON objects. Should I have to do this at all? Is there a better way to handle any type of JSON structure response?

Entities are initialized generically, and returned as an Observable (using RxSwift).

Do you sense any code smells?

/// Responsible for Making actual API requests & Handling response
    /// Returns an observable object that conforms to JSONable protocol.
    /// Entities that confrom to JSONable just means they can be initialized with json & transformed from swift to JSON.
    func rx_fireRequest<Entity: JSONable>(_ endpoint: FirebaseEndpoint, ofType _: Entity.Type ) -> Observable<[Entity]> {

        return Observable.create { [weak self] observer in

            self?.session.dataTask(with: endpoint.request, completionHandler: { (data, response, error) in

                /// Parse response from request.
                let parsedResponse = Parser(data: data, response: response, error: error)
                    .parse()

                switch parsedResponse {

                case .error(let error):
                    observer.onError(error)
                    return

                case .success(let data):
                    var entities = [Entity]()
                    switch endpoint.method { 

                    /// Flatten JSON strucuture to retrieve a list of entities.
                    /// Denoted by 'GETALL' method.
                    case .GETALL:

                        /// Key (underscored) is unique identifier for each entity
                        /// value is k/v pairs of entity attributes.
                        for (_, value) in data {
                            if let value = value as? [String: AnyObject], let entity = Entity(json: value) {
                                entities.append(entity)
                            }
                        }
                        /// Force downcast for generic type inference.
                        observer.onNext(entities as! [Entity])
                        //observer.onCompleted()

                    /// All other methods return JSON that can be used to initialize JSONable entities 
                    default:
                        if let entity = Entity(json: data) {
                        observer.onNext([entity] as! [Entity])
                        //observer.onCompleted()
                    } else {
                        observer.onError(NetworkError.initializationFailure)
                        }
                    }
                }
            }).resume()
            return Disposables.create()
        }
    }
}

I manage different endpoints like so:

enum FirebaseEndpoint {
    case saveUser(data: [String: AnyObject])
    case fetchUser(id: String)
    case removeUser(id: String)

    case saveItem(data: [String: AnyObject])
    case fetchItem(id: String)
    case fetchItems
    case removeItem(id: String)

    case saveMessage(data: [String: AnyObject])
    case fetchMessages(chatroomId: String)
    case removeMessage(id: String)
}


extension FirebaseEndpoint: Endpoint {

    var base: String {
        // Add this as a constant to APP Secrts struct & dont include secrets file when pushed to github.
        return "https://AppName.firebaseio.com"
    }

    var path: String {
        switch self {
        case .saveUser(let data): return "/\(Constant.users)/\(data[Constant.id])"
        case .fetchUser(let id): return "/\(Constant.users)/\(id)"
        case .removeUser(let id): return "/\(Constant.users)/\(id)"

        case .saveItem(let data): return "/\(Constant.items)/\(data[Constant.id])"
        case .fetchItem(let id): return "/\(Constant.items)/\(id)"
        case .fetchItems: return "/\(Constant.items)"
        case .removeItem(let id): return "/\(Constant.items)/\(id)"

        case .saveMessage(let data): return "/\(Constant.messages)/\(data[Constant.id])"
        case .fetchMessages(let chatroomId): return "\(Constant.messages)/\(chatroomId)"
        case .removeMessage(let id): return "/\(Constant.messages)/\(id)"
        }
    }

    var method: Method {
        switch self {
        case .fetchUser, .fetchItem: return .GET
        case .fetchItems, .fetchMessages: return .GETALL
        case .saveUser, .saveItem, .saveMessage: return .PUT
        case .removeUser, .removeItem, .removeMessage: return .DELETE
        }
    }

    var body: [String : AnyObject]? {
        switch self {
        case .saveItem(let data), .saveUser(let data), .saveMessage(let data): return data
        default: return nil
        }
    }
}

Last things I'd like someone with professional eyes to look at is how I use MVVM. I make all network requests from view model, which comes out looking something like this:

struct SearchViewModel {

    // Outputs
    var collectionItems: Observable<[Item]>
    var error: Observable<Error>

    init(controlValue: Observable<Int>, api: FirebaseAPI, user: User) {

        let serverItems = controlValue
            .map { ItemCategory(rawValue: $0) }
            .filter { $0 != nil }.map { $0! }
            .flatMap { api.rx_fetchItems(for: user, category: $0)
                .materialize()
            }
            .filter { !$0.isCompleted }
            .shareReplayLatestWhileConnected()

        collectionItems = serverItems.filter { $0.element != nil }.dematerialize()
        error = serverItems.filter { $0.error != nil }.map { $0.error! }
    }

}

In order to call API requests in a more expressive, formalized way, I am able to call api.rx_fetchItems(for:) inside flatmap above, because I extend FirebaseAPI, which I will probably have to follow the same pattern for other requests.

extension FirebaseAPI: FetchItemsAPI {

    // MARK: Fetch Items Protocols

    func rx_fetchItems(for user: User, category: ItemCategory) -> Observable<[Item]>  {
        // fetched items returns all items in database as Observable<[Item]>
        let fetchedItems = rx_fireRequest(.fetchItems, ofType: Item.self)

        switch category {

        case .Local:
            let localItems = fetchedItems
                .flatMapLatest { (itemList) -> Observable<[Item]> in
                    return self.rx_localItems(user: user, items: itemList)
            }
            return localItems

        case .RecentlyAdded:
            // Compare current date to creation date of item. If its within 24 hours, It makes the cut. 
            let recentlyAddedItems = fetchedItems
                .flatMapLatest { (itemList) -> Observable<[Item]> in
                    return self.rx_recentlyAddedItems(items: itemList)
            }
            return recentlyAddedItems


            //TODO: Handle other categories of items that user may want to display.

        default:
            print("DEBUGGER RETURNING DEFAULT")
            let stubItem = Item(id: "DEFAULT", createdById: "createdBy", creationDate: 1.3, expirationDate: 2.4, title: "title", price: 2, info: "info", imageUrl: "url", bidCount: 4, location: "LA")
            return Observable.just([stubItem])
        }
    }

    // Helper methods
    // CALL ONCOMPLETED? OR WILL THAT SHORT CIRCUIT FUTURE EMISSIONS. JUST LET IT RIDE AND GET DIPSOSED NATRUALLY?

    private func rx_localItems(user: User, items: [Item]) -> Observable<[Item]> {
        return Observable<[Item]>.create { observer in
            observer.onNext(items.filter { $0.location == user.location }) // LA Matches stubs in db
            return Disposables.create()
        }
    }

    func rx_recentlyAddedItems(items: [Item]) -> Observable<[Item]> {
        return Observable<[Item]>.create { observer in
            observer.onNext(items.filter { Calendar.current.component(.hour, from: Date(timeIntervalSince1970: $0.creationDate)) < 24 })
            return Disposables.create()
        }
    }

}

I'm attempting to follow SOLID principles, and level up with RxSWift + MVVM, so I'm still unsure about best practices for clean, maintainable code.

Do you cancel your requests? Have you heard about Moya, why don't you use it? — iWheelBuy
– iWheelBuy, Commented Aug 16, 2017 at 16:30
I thought it would be a good learning experience to do it myself. Also I think the less dependencies, the better. I came across this article that provides an example of a maintainable networking layer, and decided to take this route. However, in my case it is a bit over-kill since it requires creating a class for every request. If I could find a middle ground somewhere between the code i've shown here, and the article, that would probably be the optimal solution. — AnonProgrammer
– AnonProgrammer, Commented Aug 22, 2017 at 18:13
@iWheelBuy what do you mean cancel requests? do you mean call onCompleted()? I don't, because that will prevent me from making subsequent requests right? — AnonProgrammer
– AnonProgrammer, Commented Aug 23, 2017 at 2:23
cancel the request means nothing but cancel. For example, you have a search field and you make a search request every time user changes the search line (throttle or debounce). Lets say you use flatMapLatest, and your previous request is disposed, but not cancelled. You can open RxMoyaProvider.swift file in Moya framework and see that the request is cancelled on dispose. — iWheelBuy
– iWheelBuy, Commented Aug 23, 2017 at 5:20

Daniel T. · Accepted Answer · 2017-09-18 01:33:51Z

I really dislike your FirebaseEndpoint enum. I would rather see something like this:

struct FirebaseEndpoint: Endpoint {
    let path: String
    let method: Method
    let body: [String: AnyObject]?
    var base: String {
        // Add this as a constant to APP Secrts struct & dont include secrets file when pushed to github.
        return "https://AppName.firebaseio.com"
    }
}

extension FirebaseEndpoint {
    static func saveUser(data: [String: AnyObject]) -> FirebaseEndpoint {
        return FirebaseEndpoint(
            path: "/\(Constant.users)/\(data[Constant.id])", 
            method: .PUT, 
            body: data)
    }

    static func fetchUser(id: String) -> FirebaseEndpoint {
        return FirebaseEndpoint(
            path: "/\(Constant.users)/\(id)", 
            method: .GET, 
            body: nil)
    } 

    static func removeUser(id: String) -> FirebaseEndpoint {
        return FirebaseEndpoint(
            path: "/\(Constant.users)/\(id)", 
            method: .DELETE, 
            body: nil)
    } 

    // etc.
}

The above makes it far easer to add/remove/modify and read endpoints.

Interestingly, because of the way Swift is parsed, you can replace your enum with the above struct without having to change any of the code that creates instances. (A enum case constructor looks exactly like a struct static func/var that returns an instance at the calling site.)

Stack Exchange Network

Maintainable networking layer architecture

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Maintainable networking layer architecture

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions