1

So I recently applied to this job for iOS Developer and was invited to complete the following code challenge:

As a next step, please complete this coding exercise in order to proceed with our interview process. If you do well & pass, you will be connected to a Software Engineer to discuss your exercise for an hour zoom video call. Take as much time as needed.

Problem statement:

Please write an (Android/iOS) app that retrieves the data from https://fetch-hiring.s3.amazonaws.com/hiring.json. This will return a json array of items. Using this list of items, display all the items grouped by "listId" to the UI. Sort the results first by "listId" then by "name" when displaying. Filter out any items where "name" is blank or null. The final result should be displayed to the user in an easy-to-read list."

My solution is here.

View Controller is as follows:

import UIKit



class ViewController: UIViewController, UITableViewDelegate, UITableViewDataSource {

func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    return tableData[section].count
}

@IBOutlet weak var dataTableView: UITableView!
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    let cell = tableView.dequeueReusableCell(withIdentifier: "DataViewCell", for: indexPath) as? DataCell
    cell?.textSpace.text=tableData[indexPath.section][tableIndices[indexPath.section][indexPath.row]]
    return cell!
}

func tableView(_ tableView: UITableView, titleForHeaderInSection section: Int) -> String? {
    return "List ID: "+String(section+1)
}

func numberOfSections(in tableView: UITableView) -> Int {
    return tableData.count
}



var tableData: [[Int:String]]=[[:]]
var tableIndices: [[Int]]=[[]]

override func viewDidLoad() {
    super.viewDidLoad()
    
    
    URLSession.shared.dataTask(with: URL(string: "https://fetch-hiring.s3.amazonaws.com/hiring.json")!)
                 {
                     data,response,error  in
                     
                    
                    let json=try? JSONSerialization.jsonObject(with: data!, options: []) as! [[String:Any]]
                    
                    for items in json!
                    {
                        if(type(of: items["name"]!) != NSNull.self && items["name"]! as! String != "")
                        {
                            let listID=Int(String(describing: items["listId"]!))!
                          
                            while(self.tableData.count < listID)
                            {
                                self.tableData.append([:])
                                self.tableIndices.append([])
                            }
                            
                            self.tableIndices[listID-1].append(Int(String(describing: items["id"]!))!)
                            self.tableData[listID-1][Int(String(describing: items["id"]!))!       ]=items["name"]! as! String
                            self.tableIndices[listID-1].sort()
                            
                        }
                    }
                 
                     DispatchQueue.main.async {
                        self.dataTableView.delegate=self
                        self.dataTableView.dataSource=self
                        self.dataTableView.reloadData()
                         
                     }
            
                 }.resume()
                  

}


}

Two days after submitting, I got the following feedback:

"The exercise featured a lot of Swift code that forced unwrapping instead of using nil safe paradigms such as let or guard, and the items displayed in the list would have benefited from a struct or class defining the object to better separate the business logic from the view controller."

Are they being too nit picky here, or is my code really that screwed up?

1 Like
2

I agree with what’s said in the feedback. I don’t know the exact requirements and expectations of your task, but it’s normally implied that the code you submit is what you would write in a production environment and so you’re judged on that. Your code “works”, but there’s a lot of unnecessary force unwrapping (leading to runtime crashes if the JSON returned changes in anyway or not returned at all) and no separation of concerns (everything is dumped into viewDidLoad()).

You can improve your submission by cutting down on the force unwrapping (unless you have a very good justification for it) and try to separate the logic of fetching and displaying the items (perhaps by utilising the MVC/MVP/MVVM pattern). Also, Codable is the go-to option for decoding JSON, instead of JSONSerialization and will simplify your code a lot.

6 Likes
3

There are a lot of force unwraps in there. It would certainly not behave gracefully for corrupted JSON, or a network interruption, for example.

1 Like
4

The problem statement is all that I received in terms of requirements. I understand that the code is not perfect, and that I'm still learning Swift, but in terms of trying to learn where I need to make improvements, do you agree that the errors would be enough to disqualify me from the interview process? No worries if you don't want to answer, I'm just trying to get some feedback so I can do better for the next one.

5

Your code is not high quality. The two items they mention are clear weaknesses. Your code would have benefitted from running SwiftLint on it and you fixing the defects it points out and also reformatting it.

Organizing your code into modules helps with what's called Separation of Concerns (google it). Your items in the list should be modeled as a Struct. You should have a separate object that does your networking and json parsing and holds the resulting array of RowData Structs. I usually have a separate object from the viewController that is the tableView dataSource and delegate. This is usually called a viewModel from the MVVM design (google it).

Your force unwrapping everywhere is an extremely dangerous coding style. It works, I guess, in your code because you were working with a fixed JSON input. If the JSON didn't exactly match what your code is expecting your app would crash. While it is a general problem that client apps need to protect themselves from bugs on the server it would be a reasonable response to bad data to throw an exception and report an error to the user. Your app should never crash due to bad data. The idiomatic way to parse JSON is using the Decodable protocol. It's both easier and safer than the code you showed.

Your code in cellForRowAt uses a force cast, not force unwrapping. This particular issue is caused by an impedance mismatch between Obj-C APIs and Swift where the Obj-C code would just use a typecast of the cell returned from dequeueReusableCell but Swift doesn't have a way to guarantee that you can return a valid cell from this method, as it requires. There are sophisticated ways to solve this. Yours is simplistic.

Other nits I would pick, never use String(describing: in production code. Using self.blah everywhere is an Obj-C anachronism. Don't do that. The code formatting is not idiomatic.

You're obviously a beginner at this.

6

Yup. I'm def a noob at Swift. I'd say I've got the fundamentals down, but not much beyond that. I've put out a simple UI-based API aggregator app on my own a few years ago written in Swift, but that's about it. I guess the frustrating thing for me is that the feedback actually makes perfect sense after they mentioned it, and I guess I just wish I had a chance to explain that to them.

7

Don’t get discouraged, though. Your code “works” and elegance can be learned. It’s good that you are getting feedback so you can learn from it.

4 Likes
8

I would’ve expected it to be more detailed i.e. include a set of detailed and specific ACs (acceptance criterias) for example, instead of leaving the interpretation of the task completely up to the candidate, especially if this is for a junior role. I think you should mention this in the feedback you send to your recruiter, if you plan to do so.

9

I’ll be honest: while this code functions correctly as far as I can tell, if I was evaluating it I would probably have considered it a reason not to hire, too. What you have here works—which means you’re on the right track in your growth as a programmer!—but there are also significant issues.

My code review, for what it’s worth:

Formatting

It’s possible that some of this was introduced when you copied this code to the forums, but...

There’s a lot of inconsistency in the way you format code; for instance:

  • Lines 7-9: This method is not indented. Actually, none of the delegate methods are. Everything else is, so this isn’t a stylistic choice.
  • Line 13 vs. 14: On one line, you use spaces around the =; on the other, you don’t. There are many other examples of this throughout the code.
  • Line 11 vs. 28-29: In one case, you have a newline before the property and none after; in the other, you have several newlines before and one after. Neither looks right.
  • Line 36 vs. 61: On one line, you put a newline before the trailing closure; on the other, you don’t. Again, there are other examples of this.

There are also some formatting choices that suggest unfamiliarity with Swift:

  • Line 19: This would read more nicely with string interpolation: return “List ID: \(section + 1)”.
  • Lines 44 and 48: Many languages require parentheses around control flow conditions, but Swift does not, and they are not considered to be idiomatic.

And some that are just careless:

  • Line 37: Two spaces before the in.
  • Line 55: Several spaces or perhaps tabs in one of the subscripts.
  • Line 11 vs. 28-29: The stored properties are not grouped together, but neither are they grouped with related declarations. They’re just sort of randomly interspersed with the methods, which makes them hard to find.

The computer doesn’t care about any of this, but your coworkers do, because they need to read and maintain your code. If it’s poorly formatted, they will struggle to understand it.

(This is especially important in an interview code sample, because they’re trying to assess your entire skill set based on about 70 lines of code. They’ll make some allowances for bad formatting when you’re whiteboard coding or possibly doing a short timed exercise, but for any interview coding exercise where you have a little time to breathe, you should put extra effort into formatting and “proofreading”. Coding for an interview is like dressing for an interview: you should put your best foot forward.)

Lack of clarity

There are several things about this code that aren’t immediately obvious:

  • What is tableIndices for? From reading the code, it looks like it maps the row numbers in a section to the keys in tableData, but the name tableIndices doesn’t make this clear, and it’s not immediately clear why tableData isn’t just already organized like this.
  • What’s the significance of the 1 in the oft-repeated expression listID-1?
  • Why do you wait to set the table view’s delegate and data source until the data has been loaded?
  • Why do you repeatedly sort the arrays in tableIndices? Sorting tends to be expensive—do you need to do this every time you add an item, or have you overlooked a possible performance issue?

Each of these could have been addressed by adding a comment explaining the code more clearly.

New programmers often struggle with comments—some use them to basically restate what each line does in plain language, while others neglect to comment at all. Comments are best employed to explain things that are not obvious from the executable code itself. And the biggest thing that’s often not obvious is why.

Lack of polish

As a small interview exercise, it’s understandable that you cut a couple of corners. For instance:

  • While the data is loading, you display only an empty list, not any sort of “loading” indicator.
  • You don’t really handle any errors (other than a nil or empty name field).

Their problem description doesn’t clearly state that you need to handle these, but it would probably have been a good to demonstrate that you had at least thought about them, either by asking them to clarify what they wanted you to do about these cases (this is a great interview tactic—thinking through a spec and identifying the unclear or tricky parts is a valuable skill!), or by at least including comments like “I’m leaving out error handling to save time, but in real code, I would have looked for X and Y”.

In particular, the main reason you used force-unwraps so heavily is that you were assuming away error cases. If there had been signs in the code that you knew this would be incorrect for production code but considered error handling to be outside the scope of the exercise, I think that might have been forgiven.

(Or you could have just written the error handling. Up to you.)

Code organization

There’s a problem sometimes jokingly called a “Massive View Controller”: people often just pile everything directly into the view controller. This causes it to grow endlessly, with no clear separation between its many different roles.

By directly initiating a network request in viewDidLoad() and then including what amounts to deserialization code in its completion closure, you’ve sort of fallen into this trap. You’re mixing things that are properly part of the model (network activity, deserialization, modeling of the data you’re fetching) with things that are the job of the view controller (updating views). You might feel like you can get away with this because it’s such a small piece of code overall, but understand that about 40% of the code in your view controller is doing things that are not a view controller’s responsibility. The evaluator probably worried that you might do the same thing in a production view controller, perhaps bloating a 400-line view controller to 740 lines.

It would have been better to introduce at least one model type and one helper function (to do the network request). Perhaps more. Even though it’s overkill for this tiny example, it would have demonstrated that you understand how to split apart model and controller logic.

(Explicitly modeling the items in the list with a type would have also allowed you to deserialize it with Codable, which would probably have eliminated a lot of the casting and force-unwrapping you’re doing to unpack data from dictionaries.)

In sum...

I think you made some mistakes during this exercise. But that’s okay—there will be other jobs and other interviews. The important thing is to learn from this experience and this feedback. In some cases, you may actually need to develop your programming or Swift skills further; in others, you may just need to adjust how you approach interview exercises. But if you accept the criticism, there’s a lot you can learn from this rejection, and you’ll be a better programmer and a better candidate for your next application.

If there’s one piece of advice I hope you take away from this post, it’s this: Don’t treat an interview exercise as though it’s throwaway code—treat it like a miniature version of a real product that will require maintenance and collaboration. They’re probably not hiring you to write 70-line apps; they’re just giving you something with a small enough scope to be a reasonable part of an interview. Use it to demonstrate what you’ll do for them in the real code they’ll be paying you to write.

18 Likes
10

The ironic thing is that I’m guessing you probably learned all this by working your way up after someone took a chance on you for a Jr Dev position at a small company. :-)

11

I started programming as the most junior kind of dev at the smallest kind of company: I was still a teenager and was maintaining an elaborate website promoting my mother’s novels. There wasn’t really anyone more senior around to review my code, but I had several years to write a lot of bad code, read a lot of books and articles, and gradually figure out how to write good code instead.

Not everyone has that privilege, so I hope this helps you find your way a little faster than I did!

13 Likes
12

A few things to add:

This check:

if(type(of: items["name"]!) != NSNull.self && items["name"]! as! String != "")

  • Checking type(of:) == ... can be replaced with a shorter is

    !(items["name"] is NSNull)

  • By immediately following up the first check with as! String, you're assuming that, if it's not NSNull, it must be String. While it is true for a normal data, if someone provides an invalid data with Int, it will just crash a program. It's generally not useful to check if something is not NSNull anyway. If you expect to use String out of it, simply check if it is String. Heck, the whole checking could become:

    if let name = items["name"] as? String, !name.isEmpty {
  ...
}

    and you get to use name which is already a valid String, avoiding repeated casting altogether.

Since the data follows a rigorous structure, you can use JSONDecoder instead of JSONSerialization:

struct Item: Codable {
    var id: Int, listId: Int, name: String!
}

...

URLSession.shared... { data, response, error in
  do {
    let decoder = JSONDecoder()
    let items = try decoder.decode([Item].self, from: data).filter {
      !($0.name?.isEmpty ?? true)
    }
    // Now `items` contains only `Item`s with non-empty `name`.

    // Process `items`, and put them in `tableData`
  } catch {
    // Fail to decode data, what now?
  }
}

Since you're only using the data for display, it could be easier to make tableData a [[Item]] instead of [[Int: String]]. This way, all the data are already in order, and you don't need to keep tableIndices around. You can leverage some functionality in Dictionary as well to do all the grouping/sorting work for you. Together with items from the previous part:

// Group the arrays by listID
var result = Dictionary(grouping: items, by: \.listId).map(\.value)
// Sort outer arrays by listID
result = result.sorted { $0.first!.listId < $1.first!.listId }
// Sort inner arrays by id
for index in result.indices {
  result[index] = result[index].sorted { $0.id < $1.id }
}

// result is now a `[[Item]]` sorted by `listID`, breaking ties by `id`

You can do all this with one big call chain, but it's a little tricky if you're not used to functional programming.

This way, you can simply use indices when reading data for the table:

cell.textSpace.text = tableData[indexPath.section][indexPath.row].name

Putting it all together:

var tableData: [[Item]] = []

URLSession... { data,response,error  in
  guard let data = data else {
    #warning("Fail to retrieve data")
    return
  }

  do {
    let decoder = JSONDecoder()
    let items = try decoder.decode([Item].self, from: data).filter {
      !($0.name?.isEmpty ?? true)
    }
    // Group the arrays by listID
    var result = Dictionary(grouping: items, by: \.listId).map(\.value)
    // Sort outer arrays by listID
    result = result.sorted { $0.first!.listId < $1.first!.listId }
    // Sort inner arrays by id
    for index in result.indices {
      result[index] = result[index].sorted { $0.id < $1.id }
    }

    DispatchQueue.main.async {
      self.tableData = result
      self.dataTableView.reloadData()
    }
  } catch {
    #warning("Fail to decode data")
    return
  }
}.resume()

...

cell.textSpace.text = tableData[indexPath.section][indexPath.row].name
3 Likes
13

I've been the interviewee for many such exercises. The interviewer expects that you know the basics of good coding practices, such as those outlined by others in this thread. What the interviewer wants to know is how you approach problems, how you think. Which techniques you reach for, how you choose to factor the code. How much thought you put into error handling (e.g. for exercises such as this, make sure it doesn't crash on bad input, but little else).

This is why there is little detail given. Just enough to know what the expected output is, and the rest is up to the interviewee.

2 Likes
14

I guess IMHO anyway, the feedback I received was a little harsh for what I perceive to be a Jr./Entry Level position. I'm under no delusions that I am a Swift expert, but I also was able to push out a working app that met their functional requirements, and the feedback I've received both from the company and on this forum actually makes perfect sense and is easily correctible, I think. It's also curious that, again IMO, most of the feedback seems to be on things that, ironically, one only learns AFTER working as a Jr. Dev. on a team with more senior devs to mentor you and review your code. Maybe I'm off base though.

15

Sure, that’s a fair enough. I was just saying that I would’ve expected a little more detail instead of a single paragraph. I’m not saying it needs to list every single do’s/don’t, but something that provides a little more clarity on expectations to the candidate. As an example, here’s the one that my employer uses (for all roles, not just junior).

16

I agree with all the suggestions here. I'll add a relevant anecdote. In early 2016, I was changing careers to iOS development. I completed a coding challenge for a job. The company rejected me on the basis that my UIViewController called URLSession directly. That experience and others taught me the important of separation of concerns.

During that era, I completed a couple other unsuccessful coding challenges. Although I got no feedback on those, I suspect that the lack of unit tests was at least one reason for the lack of success. I would not now submit a coding challenge without unit tests. If you are interested in learning about unit testing, try this book and these blog posts.

1 Like
17

Maybe it's just me, but this seems awfully petty and dismissive to disqualify you for what I think could easily be corrected with an e-mail or maybe 1-2 hours of mentoring from your manager. I hear all the time that "Experience doesn't matter, enthusiasm and ability to learn does." I guess that's mostly BS?

2 Likes
18

That depends entirely on the company. If you’re being placed through a labor company, they're usually only looking for people who can drop into a role with little hand holding. Some direct hire companies do have a significant culture of mentorship and help for junior devs. You just have to find those companies.

4 Likes
19

In addition to @Jon_Shier's point about this sort of thing being heavily company-dependent, my experience has been that while issues like these rarely result in categorical disqualification, they do serve as differentiators when there are multiple candidates under consideration. If one candidate follows "best practices" (whatever that may mean to the hiring company), they'll naturally appear as the more attractive option compared to those who don't.

5 Likes
20

If you think you’ve been knocked out of contention, you’ve got nothing to lose by telling them what you learned from the feedback and how you would complete the challenge differently given that. An interviewee did something similar after I interviewed them and it was impactful. We contacted them when we had another opportunity.

3 Likes