4
\$\begingroup\$

I created the following service for the purpose of sending in a file for parsing. I'm using the generics so that when I call parse() I can pass a custom type conforming to Codable. I'm not a huge fan of the repetition of do {} catch {} but I don't see any other way to avoid this.

import Foundation

enum JSONError: Error {
    case read,
         parse
}

struct JSONService {
    let fileName: String
    
    func parse<T: Decodable>(type: T.Type) -> T? {
        do {
            let fileContents = try self.getJsonContents()
            return try self.parseJson(data: fileContents, type: T.self)
        } catch {
            fatalError("\(error.localizedDescription)")
        }
    }
    
    private func getJsonContents() throws -> Data {
        if let file = Bundle.main.path(forResource: self.fileName, ofType: "json") {
            if let data = try String(contentsOfFile: file).data(using: .utf8) {
                return data
            }
        }
        
        throw JSONError.read
    }
    
    private func parseJson<T: Decodable>(data: Data, type: T.Type) throws -> T {
        do {
            return try JSONDecoder().decode(type.self, from: data)
        } catch {
            throw JSONError.parse
        }
    }
}

\$\endgroup\$
0
0
\$\begingroup\$

Your method reads an object from a JSON file which is embedded in the application bundle, and aborts with a runtime error if that fails for any reason. That is fine: Bad embedded data are a programming error and should be detected early.

The return type of the parse() method should not be an optional because it never returns nil.

The custom error type is used to pass errors from the helper methods to the main method. That has two disadvantages:

  • It hides (possible more informative) errors thrown from other methods, e.g. the JSON decoder method.

  • The messages printed with fatalError() show only the error type and an integer, for example

    Fatal error: The operation couldn’t be completed. (MyApp.JSONError error 0.)
    

The second issue can be improved (see for example How to provide a localized description with an Error type in Swift? on Stack OverFlow) but I would go another route and pass on existing errors instead, where possible:

In parseJson() you can pass the error which the JSON decoder already throws, instead of shadowing it by a custom error:

private func parseJson<T: Decodable>(data: Data, type: T.Type) throws -> T {
    try JSONDecoder().decode(type.self, from: data)
}

And in getJsonContents() you can use the (throwing) Data(contentsOf: url) method to read the data, that is also simpler than reading into a string and then converting the string to data.

It remains to handle an error in Bundle.main.url(forResource:withExtension:), that method does not throw an error but returns an optional.

Without the do-catching in the helper methods everything fits neatly into the main method:

func parse<T: Decodable>(type: T.Type) -> T {
    guard let url = Bundle.main.url(forResource: self.fileName, withExtension: "json") else {
        fatalError("file not found")
    }
    do {
        let data = try Data(contentsOf: url)
        return try JSONDecoder().decode(type.self, from: data)
    } catch {
        fatalError("\(error.localizedDescription)")
    }
}

You many also want to print the full error and not its localizedDescription because that can provide more information (and the string is not presented to the user of your program but only for diagnostic purposes).

Finally, I wonder if fileName should really be an instance variable. If it is only used in the parse() method and nowhere else then you can make it a parameter of a static method:

struct JSONService {
    static func parse<T: Decodable>(type: T.Type, from fileName: String) -> T {
        guard let url = Bundle.main.url(forResource: fileName, withExtension: "json") else {
            fatalError("file not found")
        }
        do {
            let data = try Data(contentsOf: url)
            return try JSONDecoder().decode(type.self, from: data)
        } catch {
            fatalError("\(error)")
        }
    }
}

which would be called as

let value = JSONService.parse(type: MyType.self, from: "filename")

Embedding it into a struct JSONService is still useful as it provides a “name space“ for its static methods. If there are only static methods then you can embed them in an enum JSONService instead, because no instance of that type needs to be created (thanks to @Shadowrun for reminding me about that).

\$\endgroup\$
3
  • \$\begingroup\$ If a struct has only static methods, you could argue that it shouldn't be possible to instantiate such a struct with let foo = JSONService(). One way to prevent that is to make it an enum with no cases and only static methods. Another idea would be to avoid having a type at all, which means you also don't need to worry about naming it FooService/FooHelper/FooSomething. By making an extension on Decodable: extension Decodable { init(from fileName: String) { ... self = try JSONDecoder().decode(Self.self, from: data) ... \$\endgroup\$ – Shadowrun Nov 17 '20 at 10:28
  • \$\begingroup\$ @Shadowrun: You are completely right, an enum is sufficient and perhaps preferable if you only want a namespace (this was also discussed on Stack Overflow: stackoverflow.com/q/38585344/1187415). \$\endgroup\$ – Martin R Nov 17 '20 at 10:32
  • \$\begingroup\$ @Shadowrun: Defining your own type or extending an existing type both have advantages. The advantage of your own type/namespace is that you cannot have conflicts with definitions in other modules or with future extensions of the exiting type. \$\endgroup\$ – Martin R Nov 17 '20 at 10:38

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.