0
\$\begingroup\$

I want to load 2 files and show this data in 2 strings. I trying to do it like this and it works fine. But I think it is not the best solution... How to do more elegant solution?

var saleNumber1 = ""
var saleNumber2 = ""

override func viewDidLoad() {
        super.viewDidLoad()
        
        sale1()
        sale2()
}

func sale1() {
        
        let url = URL(string: "http://example.com/folder/sale1.txt")!

        let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
            guard let data = data else { return }
            saleNumber1 = "\(String(data: data, encoding: .utf8)!)"
        }

        task.resume()
        
    }
    
    func sale2() {
        
        let url = URL(string: "http://example.com/folder/sale2.txt")!

        let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
            guard let data = data else { return }
            saleNumber2 = "\(String(data: data, encoding: .utf8)!)"
        }

        task.resume()
        
    }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to CodeReview@SE. What is the second thing crossing your mind when you look at the code and ponder good enough? Or needs improvement?, what has been the first? \$\endgroup\$
    – greybeard
    Dec 22 '20 at 22:55
0
\$\begingroup\$

Time to learn how to DRY off your WET code!

(DRY -> "Don't Repeat Yourself"; WET -> "Write Everything Twice")

Note how the only difference between your two functions is a URL and a change in data, effectively "changing input results in changing output." Consider consolidating the two into one generic function that takes a variable url in as input and returns data which can be store in a variable of your choice.

var saleNumber1 = ""
var saleNumber2 = ""

override func viewDidLoad() {
    super.viewDidLoad()
        
    saleNumber1 = sale("http://example.com/folder/sale1.txt")
    saleNumber2 = sale("http://example.com/folder/sale2.txt")
}

func sale(path: String) -> String {
    let url = URL(string: path)!
    let saleNumber = ""
    let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
        guard let data = data else { return }
        saleNumber = "\(String(data: data, encoding: .utf8)!)"
    }

    task.resume()

    return saleNumber
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ There are two problems with your suggestion: First, it does not compile because let saleNumber defines a constant. The bigger problem is that URLSession.shared.dataTask works asynchronously, i.e. the method returns immediately, and calls the closure later, when the data has been retrieved. In other words, chances are high that the function will return an empty string. \$\endgroup\$
    – Martin R
    Dec 23 '20 at 21:52
  • \$\begingroup\$ agh my fault for not looking up swift declarations first, my js lizard brain jumped right to let == scope lifetime var. \$\endgroup\$ Dec 24 '20 at 1:16
0
\$\begingroup\$

At the very least, you should:

  • Have a single routine that performs the network request and fetches the string;

  • This routine should use a completion handler closure, so the caller can supply code to be called when the asynchronous network request finishes;

  • You should look at the HTTP status code and make sure it is 2xx, to make sure the request succeeded (which is especially important when your web service is only returning a string ... but it is also useful when you start using more structured responses);

  • If you are writing a utility method to perform the request, you probably should return the related URLSessionTask in case the caller might want to cancel the request ... You might not be using it at this point, so you might declare it as a @discardableResult, indicating that you are returning this information, but that it is OK if the caller does not avail itself of this task object at this point. But it is very useful to get in the habit of returning this URLSessionTask so the caller can cancel it if it needs to (e.g. the user dismisses the view; the user supplies new query parameters that invalidate the need for the initial request; etc.).

  • We want to disentangle the network request code from the updating of some model object. So the function performing the network request should not be updating the model objects, itself, but rather supply the returned information via the aforementioned completion handler.

  • It is a minor point, but one should probably refrain from using the forced unwrapping operator, !, when parsing server responses. When writing client code, you want to gracefully handle any server-side errors, but a forced unwrapping operator will crash your app if it fails.

So, pulling that all together:

enum NetworkError: Error {
    case failure(Data?, URLResponse?)
}

@discardableResult
func fetch(from url: URL, queue: DispatchQueue = .main, completion: @escaping (Result<String, Error>) -> Void) -> URLSessionTask {
    let task = URLSession.shared.dataTask(with: url) { data, response, error in
        guard
            error == nil,
            let responseData = data,
            let httpResponse = response as? HTTPURLResponse,
            200 ..< 300 ~= httpResponse.statusCode,
            let responseString = String(data: responseData, encoding: .utf8)
        else {
            queue.async {
                completion(.failure(error ?? NetworkError.failure(data, response)))
            }
            return
        }

        queue.async {
            completion(.success(responseString))
        }
    }

    task.resume()

    return task
}

Then, your two routines could (a) adopt a similar completion handler pattern; and (b) call this common routine:

func sale1(completion: @escaping (Result<String, Error>) -> Void) {
    let url = URL(string: "http://example.com/folder/sale1.txt")!
    fetch(from: url, completion: completion)
}

func sale2(completion: @escaping (Result<String, Error>) -> Void) {
    let url = URL(string: "http://example.com/folder/sale2.txt")!
    fetch(from: url, completion: completion)
}

And the caller might do:

sale1 { [weak self] result in
    switch result {
    case .success(let value): self?.saleNumber1 = value
    case .failure(let error): print(error)
    }
}

And, if you wanted to do something when the two requests finish:

let group = DispatchGroup()

group.enter()
sale1 { [weak self] result in
    ...
    group.leave()
}

group.enter()
sale2 { [weak self] result in
    ...
    group.leave()
}

group.notify(queue: .main) { [weak self] in
    // do whatever you want when both are finished (e.g. trigger UI update)
}

A few other observations:

  1. Generally, we do not just have web services return a string. Usually we would use structured responses from our web services, such as JSON, and then our Swift code would use JSONDecoder to parse the response out.

    This offers two capabilities: First, if a request failed for some reason, the web service might want to return meaningful error information (above and beyond a simple HTTP status code). If you are treating the response just as a string, it becomes harder to do elegant error handling. Second, right now you are apparently only returning a single string, but in the future, you might want more structured responses (e.g. a image URL, an identifier, and a string; or perhaps an array of them, etc.).

  2. It is beyond the scope of this question, but Combine offers elegant alternatives for parsing network requests, determining when they all finish, etc.

\$\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.