1

In the following scenario when reading JSON data from a file, I have the following block of code:

// Fetch URL
let url = Bundle.main.url(forResource: "sampleJSON", withExtension: "json")!

// Load Data
let data = try! Data(contentsOf: url)

// Deserialize JSON
let json = try! JSONSerialization.jsonObject(with: data, options: [])

Is this block of code correct on its own, or should would it be better practice to include it inside a do-catch block? I'm asking because I have seen scenarios when pulling data from the web using URLSession, where developers do the JSONSerialization inside of a do-catch block. Is there a reason why for doing it when using URLSession, and not when simply pulling the JSON data from a file? What is best practice for something like this?

halfer
  • 18,701
  • 13
  • 79
  • 158
syedfa
  • 2,707
  • 1
  • 34
  • 71
  • You should always catch the error if your method throws or use try? and return an optional or if you don't want optional as result use nil coalescing operator and provide a default value – Leo Dabus Jun 28 '17 at 16:45
  • If available prefers allow the do/catch. You'll never know if the response from your server changes, is corrupted, etc. Also, in case of bug, it's easier to debug. – Larme Jun 28 '17 at 16:45

3 Answers3

4

1 - Is this block of code correct on its own, or should would it be better practice to include it inside a do-catch block?

A: This code is correct. It will work if your sampleJSON.json file is in your bundle AND the data in your JSON file is correctly formated AND the JSONSerialization succeds parsing the data provided.

2 - I'm asking because I have seen scenarios when pulling data from the web using URLSession, where developers do the JSONSerialization inside of a do-catch block. Is there a reason why for doing it when using URLSession, and not when simply pulling the JSON data from a file? What is best practice for something like this?

A: The do-catch statement is seen more often when consuming data(JSON in this case) from the web because the API might break for any reason(wrong specification of the data that must be shown, error in the web application itself, etc) and if this happens we do not want our application to crash.

I say CRASH because you used the ! which do not propagate the error to the upper layer of your application, it tries to force the operation and if fails would crash the app.

At this point you probably realized that the reason you don't see do-catch statement when consuming data from your bundle is because the app developer himself provided the JSON so I'd assume you are sure about the content of the file, but I'd still use the do-catch statement since something could go wrong and don't want my app to crash due to a silly thing like this.

TL;DR

I recommend to ALWAYS use error propagation with throws/rethrows or even the ? so you can test for nil results.

I have written a small article here with some tips and how it works in Swift 2.1, not much have changed in Swift 3.1 so you can use to study the do-catch statement.

I would rewrite the code you provided like this:

WARNING: UNTESTED CODE

enum JSONFromFileError: Error {
    case fileNotInBundle(String)
    case deserializationError
    case getDataError(URL)
}

func json(from file: String) throws -> Any {
    // Fetch URL in Bundle
    guard let url = Bundle.main.url(forResource: file, withExtension: "json") else {
        throw JSONFromFileError.fileNotInBundle(file)
    }

    // Load Data from url
    guard let data = try? Data(contentsOf: url) else { 
        throw JSONFromFileError.getDataError(url)
    }

    // Deserialize JSON
    guard let json = try? JSONSerialization.jsonObject(with: data, options: []) else {
        throw JSONFromFileError.deserializationError
    }

    return json
}

do {
    let myJsonObject = try json(from: "sampleJSON")
    print(myJsonObject)
} catch let error {
    print(error)
}
henrique
  • 1,017
  • 11
  • 17
3

You should in general use a try-catch block for each function that is throwable. With your current code, if either of your try blocks fail (either the data can't be downloaded from the url or it is not a valid json), the forced trys will fail and result in a runtime exception.

let url = Bundle.main.url(forResource: "sampleJSON", withExtension: "json")!
do {
    let data = try Data(contentsOf: url)
    let json = try JSONSerialization.jsonObject(with: data, options: [])
} catch {
    //handle error
}

If you don't care about the error that would be thrown be the function, you can use try? which returns a nil when the function would throw an error. This way your code won't crash.

guard let data = try? Data(contentsOf: url) else {return}
guard let json = try? JSONSerialization.jsonObject(with: data, options: []) else {return}
Dávid Pásztor
  • 40,247
  • 8
  • 59
  • 80
  • 2
    It's not a good idea to force unwrap the URL. Then again, in a case like this, you might want the app to crash if the file isn't found in the app bundle because it's a development bug. – rmaddy Jun 28 '17 at 16:49
  • 1
    I didn't even realise the force unwrap in that line, I only focused on the forced trys. In general, I don't like to include any forced unwraps in my code either, but in this case you are right, the URL can only be invalid in case of a development bug, so it might be better to leave it as force unwrapped. – Dávid Pásztor Jun 28 '17 at 16:53
1

Firstly, It is recommended that we wrap any functions that throws (with try) into a do-catch block. In my own opinion, I will also perform a do-catch block in case I modify the sampleJSON file without me knowing, that causes the format of the JSON to be disturbed.

Secondly,it is definitely a good practise to never use force unwrap at all in the code.

Lastly, we should always catch for serialization exceptions and throw to the caller, which eventually will inform the view to throw an error dialog.

Lawrence Tan
  • 465
  • 2
  • 11