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.
1 Answer 1
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.)
onCompleted()? I don't, because that will prevent me from making subsequent requests right? \$\endgroup\$