POC for GithubUserFinder

Question

I would like to request iOS developer community to please review my POC i.e. [GithubUserFinder][1] which I have created as part of job screening test at T-mobile Bengaluru.

I have around 2 years of experience in programming and I got 24 hours to complete this POC.

Below is the feedback I got through mail by recruiter: As for Pooja, I will say probably no. Their code quality is a bit messy and sparse. They are missing a portion of the assignment and their resume doesn’t look the best, they are missing a year of work.

I request them to provide some pointer on 'messy and sparse code' or what is 'missing' in assignment but there was no reply.

Kindly request you guys to please go through my POC i.e [GithubUserFinder][1] and provide your valuable comments.

Here is the code for User search feature:

1. ViewController (search users)

import UIKit
import Alamofire
class ViewController: UIViewController {
 @IBOutlet private weak var tableView: UITableView!
 @IBOutlet private weak var searchBar: UISearchBar!
 //datasource
 private var users: [User] = [] {
 didSet {
 tableView.reloadData()
 }
 }
 private enum constants {
 static let userCellNibName = "UserCell"
 static let cellReuseIdentifier = "Cell"
 static let searchUsersPlaceholderText = "Search Users"
 static let segueIdentifier = "detailSegue"
 static let placeholderImageName = "placeholder"
 }
 //MARK:- View life cycle
 override func viewDidLoad() {
 super.viewDidLoad()
 setupSearchBar()
 setUpTableView()
 }
 override func viewWillAppear(_ animated: Bool) {
 if let indexPath = tableView.indexPathForSelectedRow {
 tableView.deselectRow(at: indexPath, animated: true)
 }
 }
 //MARK:- Setup
 private func setUpTableView() {
 tableView.register(UINib(nibName: constants.userCellNibName, bundle: nil), forCellReuseIdentifier: constants.cellReuseIdentifier)
 }
 private func setupSearchBar() {
 searchBar.placeholder = constants.searchUsersPlaceholderText
 }
 private func searchUsers(for name: String) {
 ServiceManager.getUsers(for: name) {[weak self] (response) in
 guard let self = self else { return }
 guard let response = response as? SearchResponse else { return }
 self.users = response.users
 }
 }
 override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
 if segue.identifier == constants.segueIdentifier {
 guard let destinationVC = segue.destination as? UserDetailViewController else {
 return
 }
 destinationVC.user = sender as? User
 }
 }
}
//MARK:- TableView datasource
extension ViewController: UITableViewDataSource {
 func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
 return users.count
 }
 func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
 guard let cell: UserCell = tableView.dequeueReusableCell(withIdentifier: constants.cellReuseIdentifier, for: indexPath) as? UserCell else {
 return UITableViewCell()
 }
 let user = users[indexPath.row]
 cell.populate(user: user)
 return cell
 }
}
extension ViewController: UITableViewDelegate {
 func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
 tableView.deselectRow(at: indexPath, animated: true)
 performSegue(withIdentifier: constants.segueIdentifier, sender: users[indexPath.row]);
 }
}
//MARK:- SearchResultBar delegate
extension ViewController: UISearchBarDelegate {
 func searchBar(_ searchBar: UISearchBar, textDidChange searchText: String) {
 if let text = searchBar.text, !text.isEmpty {
 searchUsers(for: text)
 } else {
 users = []
 }
 }
 func searchBarSearchButtonClicked(_ searchBar: UISearchBar) {
 searchBar.endEditing(true)
 }
}

2. TableView Cell (UserCell.swift)

import UIKit
import AlamofireImage
class UserCell: UITableViewCell {
 @IBOutlet private weak var userImageView: UIImageView!
 @IBOutlet private weak var nameLabel: UILabel!
 @IBOutlet private weak var repoLable: UILabel!
 private enum constants {
 static let placeholderImageName = "placeholder"
 }
 func populate(user: User) {
 nameLabel.text = user.login
 let placeholderImage = UIImage(named: constants.placeholderImageName)
 if let imageUrl = URL(string: user.avatarUrl) {
 userImageView.af.setImage(withURL: imageUrl, cacheKey: "\(AppConstants.kCacheImagekey)\(user.id)", placeholderImage: placeholderImage);
 }
 }
}

3. Service Manager (ServiceManager.swift)

import Foundation
import Alamofire
enum ServiceType {
 case users
 case details
 case repos
}
class ServiceManager {
 typealias CompletionHandler = (_ response:Any?) -> Void
 private static let baseUrl = "https://api.github.com/"
 // Add your client Id and client secret here..
 private static let clientId = ""
 private static let clientSecret = ""
 private static func getMethod(type: ServiceType) -> String {
 switch type {
 case .users:
 return "search/users?q="
 case .details, .repos:
 return "users/"
 }
 }
 private static func clientIdSecret() -> String {
 //Add your client Id and client secret if you hit max hour limit which is only
 // 50-60 request per hour. And enable this.
 //return "&client_id=\(ServiceManager.clientId)&client_secret=\(ServiceManager.clinetSecret)"
 return ""
 }
 public static func getUsers(for query: String, completion: @escaping CompletionHandler) {
 let url = baseUrl + getMethod(type: .users) + query + clientIdSecret()
 AF.request(url, parameters: nil).validate()
 .responseDecodable(of: SearchResponse.self) { response in
 guard let result = response.value else {
 //TODO:- Currently the errors are not handled.
 return
 }
 completion(result)
 }
 }
 public static func getDetails(for query: String, completion: @escaping CompletionHandler) {
 let url = baseUrl + getMethod(type: .details) + query
 AF.request(url, parameters: nil).validate()
 .responseDecodable(of: UserDetails.self) { response in
 guard let result = response.value else { return }
 completion(result)
 }
 }
 public static func getRepos(for query: String, completion: @escaping CompletionHandler) {
 let url = baseUrl + getMethod(type: .repos) + query + "/repos"
 AF.request(url, parameters: nil).validate()
 .responseDecodable(of: [Repo].self) { response in
 guard let result = response.value else { return }
 completion(result)
 }
 }
}

4. Model (Users.swift)

import Foundation
struct SearchResponse: Decodable {
 let users: [User]
 enum CodingKeys: String, CodingKey {
 case users = "items"
 }
}
struct User: Decodable {
 let login: String
 let id: Int
 let avatarUrl: String
 let url: String
 let reposUrl: String
 enum CodingKeys: String, CodingKey {
 case login = "login"
 case id = "id"
 case avatarUrl = "avatar_url"
 case url = "url"
 case reposUrl = "repos_url"
 }
}
 [1]: https://github.com/pooja-iosTech13/GithubUserFinder

• Sawant: This code doesn't look messy at all, though there are some chances of improvement but considering your experience it looks good to me. Keep it up.

Answer 1

I cannot review all aspects of your code, but here are some remarks which might be helpful.

JSON decoding

In the definition of User.CodingKeys one can omit the raw values of keys which are equal to the case name:

struct User: Decodable {
 let login: String 
 let id: Int
 let avatarUrl: String
 let url: String
 let reposUrl: String
 enum CodingKeys: String, CodingKey {
 case login // Instead of: case login = "login" ...
 case id
 case avatarUrl = "avatar_url"
 case url
 case reposUrl = "repos_url"
 }
}

And the conversion from "snake-case" (in the JSON API) to "camel-case" (in the Swift API) can be done by setting a keyDecodingStrategy on the JSONDecoder in getUsers():

let decoder = JSONDecoder()
decoder.keyDecodingStrategy = .convertFromSnakeCase
AF.request(url, parameters: nil).validate()
 .responseDecodable(of: SearchResponse.self, decoder: decoder) { response in
 // ...
}

which makes the User.CodingKeys obsolete:

struct User: Decodable {
 let login: String
 let id: Int
 let avatarUrl: String
 let url: String
 let reposUrl: String
}

Unnecessary conversions to/from Any

The functions in ServiceManager all take a callback argument of type (_ response:Any?) -> Void. The (correctly typed) response from AlamoFile is therefore cast to Any?, and then cast back to the correct type in the caller of that function.

It becomes simpler with a callback that takes the proper type, e.g. in getUsers():

public static func getUsers(for query: String, completion: @escaping (SearchResponse) -> Void) {
 // ...
 AF.request(url, parameters: nil).validate()
 .responseDecodable(of: SearchResponse.self, decoder: decoder) { response in
 guard let result = response.value else { ... }
 completion(result)
 }
}

which allows to get rid of the conditional case in the calling function searchUsers():

ServiceManager.getUsers(for: name) {[weak self] (response) in
 guard let self = self else { return }
 // guard let response = response as? SearchResponse else { return }
 // ^-- No longer needed.
 self.users = response.users
 print("RESPONSE", name, response.users.count)
}

Handle rate limits and paginated responses

Of course the first thing I did was to download your code and compile the app. That worked flawlessly, it compiled without warnings. Great!

Then I ran the app in the iOS Simulator. At first it seemed to work correctly, but then I typed my GitHub user name into the search field: No result – what is that? OK, delete the search field and try again: Now the tableview wasn't updated at all. Nothing seemed to happen anymore.

To isolate the problem, I added a

print("ERROR", response.error)

in the failure part of getUsers(). This revealed that after some time, the GitHub API responded with an HTTP error 403. And indeed, the GitHub Search API documentation states that search requests are limited to 30 requests per minute, and even to 10 requests per minute for unauthenticated users.

There is not much one can do about this limit, but a small remedy might be a "delayed search": If the users type "ABCD" then there is no need to search for "A", then for "AB", "ABC", and finally for "ABCD". If you start a short timer after each modification to the search text, and start the actual search only if the search text is not modified for some time then the number of search requests is already reduces.

You could also cache search results in order to submit fewer requests.

Also GitHub responses are paginated to 30 items by default. This can be increased to 100 at most, but even then you'll normally not get all items with a single request.

The link header in the search response contains the necessary information. You can use that to either

• issue more request for the remaining items. Perhaps only when the user actually scrolls to the end of the current list, again in order to limit the number of requests.
• or just display "More ..." at the end of the list, so that the user knows that there are more items than are currently displayed.

• Pooja Singh: Thanks Martin. For the rate limit issue one needs to add // Add your client Id and client secret here.. private static let clientId = "" private static let clientSecret = "" Can you add your comment on the Feedback which I have got on this code from recruiter? *** As for Pooja, I will say probably no. Their code quality is a bit messy and sparse. They are missing a portion of the assignment and their resume doesn’t look the best, they are missing a year of work.***
• Martin R: @PoojaSingh: Even as authenticated user the rate of search requests is limited, therefore I suggested to reduce the number of requests, if possible. It is up to you to decide which feedback you want to incorporate in your code. – Your code does not look "messy" to me (that is a pretty vague feedback anyway). I cannot comment on the other parts because I did not set the assignment and do not know your resume. I have said everything about your code that I wanted to say.