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:
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
- 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
- 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
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.)
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.)
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.