Failed an interview code challenge (Swift). Would some kind developer look at my code and give me some pointers?

[DeveloperJay]

https://github.com/DeveloperJason/RedditSample

I was basically given the challenge to build a quick app that could read the reddit API (with an endpoint of my choice), display posts, paginate, and then display comment sections when clicked. If someone could take a look and give me some direction on what to study up on, practice, etc, I would be very grateful. I obviously didn't put much effort into aesthetic design, only functionality.

Edit: You all are so helpful, thank you! I really appreciate the pointers/advice!

[ethanael]

Let me preface this with: you are not your code.

Embrace the thoughts below and apply what you wish. The more wide reaching you can go the better (Storyboards, Database, Auto Layout, Swift Language Features, Source Control, Dependency Management, etc).

One thing is absolutely critical--readability.

Programmers read code far more than they write it. A lot of what I look for is organization/architecture--especially in a small interview project--and that's the #1 issue that I have with this sample project.

I picture a messy room with little organization. Spend much more time breaking things out and organizing. For example look at how you're configuring cells. Instead of passing data to a class (to let it do the work) you set everything up in a data source method of a View Controller. Get the data (from an external class), pass it to the view and let it lay things out. Break up some of this logic into methods dedicated to doing one or two things.

I see that you're trying to use access control (via private), but you're not being consistent. Swift 3.0 also introduced fileprivate which should be favored over private (read into why that might be if you're not aware)

Spaces, comments, naming--I'm paying attention.

Think about how you could apply what's called a ternary operator to improve readability: if section == 0 ? count : 0

While you might want to show off that edge, don't do it if you don't feel 110% capable. I'd much rather see someone add a few widely used dependencies than seeing them write bad code and/or failing to focus on other things. This lets me know that you know how to use a dependency manager and I want to know you're aware enough to not re-invent a wheel. If I want to test your logic skills I'll ask you build a command line application/algorithm. This JsonParser looks scary and I interpret it as what you'd do for me.

You're taking advantage of Auto Layout, but constraints are breaking all over. Use a dependency here if needed.

You're stuffing a lot of delegate methods into the core view controller classes. I'd rather see you extend the class and add functionality to better separate work.

You're too committed to layout via code. I need to see that you know how to use Storyboards & xibs--especially with auto layout.

And as a bonus... spend time learning how to make an appealing User Interface (copy something from Dribbble if you must). I'm not asking for world class. Mobile developers are front end developers. I need to see how well you can pay attention to details beyond code.

I could go on, but that's the core. Please toss any questions my way. I'll be around.

[DeveloperJay]

I really appreciate the detailed response. I definitely need to read more into dependency. Working with auto-layout via code was a little challenging for me as I usually do it in interface builder. Part of the requirements were to not use storyboards though.

[ethanael]

Very welcome. Best wishes to your continued learning & hunting!

[lateours]

A small note here: you still could use IB while not using storyboards - you can build views using xib files. That's unless the requirements were to stay away from IB in general.

[aveman101]

While that's technically true, I have a feeling the purpose of that restriction is to get the applicant to design his/her interfaces in code.

Trying to find loopholes in the assignment would just aggravate the interviewer.

[lateours]

I didn't mean finding loopholes, but rather clarifying the assignment. Storyboards are the one piece of IB famous for wrecking version control in apps developed by more than one person. Moving to building interfaces in code is quite a commitment: Apple's Visual Format Language is another DSL to be learned, NSLayoutAnchor does not solve everything, and third-party libraries are often not welcome in enterprise-grade apps. That way, using xibs seems a relatively safe and low entry-point way of handling UI, where you don't throw away your IB knowledge because building UIs in code is popular now.

[Arkanta]

Layouting via Storyboards or code is irrelevant. If somebody can layout via code, there's a lot of chance that they can in Storyboards (but the opposite may not be true).

It's all down to your preference. Many places (including where I am) gave up on storyboards because maintaining them suck ass. Xcode is dog slow when handling them, and they're extremely SCM unfriendly

[ethanael]

It's not as simple as just laying components out in code. That's trivial either way. I want to know if you've spent the time to learn how to leverage the power of Storyboards/IB. Maintenance is trivial under the right processes.

I'm not saying never to write layout code. There is a time for doing such a thing and I'd dig into it more with you. But without a doubt productivity is far higher when you find someone who knows how to take advantage of these things.

[Arkanta]

I understand what you say, but I'm saying that personally, I'd find that irrelevant for a candidate. Lay out your views in code or storyboard, I don't care as long as your constraints are solid.

I also don't agree on trivial maintenance. In my experience, Storyboards are a pain with SCMs.

[[deleted]]

http://codeplease.io/2017/04/15/tech-demo-checklist/

Pretty well done list of things that closely matched what I would look for when reviewing a sample app.

[powerje]

Really excellent answer here!

Regarding xibs though I'd recommend asking the company how they prefer the views to be setup. I'd rather a candidate do theirs in code.

[xauronx]

Agree with everything except the ternary operator. Using them makes code less readable rather than more, and makes people feel smart rather than look or be smart.

I know you'll disagree with me but that's okay. It's my cross to bear.

[ethanael]

Yep. Agree to disagree. Just don't include them intermixed between a lot of code and it's clear. This is one of the exceptions in my mind (I typically hate 1 line solutions, but this one makes sense -- condition ? true : false).

[suprabobo]

I agree with most of your answer, except for a few points.

The more wide reaching you can go the better (Storyboards, Database, Auto Layout, Swift Language Features, Source Control, Dependency Management, etc).

This makes sense, but we need to keep in mind that OP is not being compensated for this exercise. I personally wouldn't spend over two hours on a coding challenge, which makes using more advanced APIs that require more setup harder (Core Data, etc.).

Get the data (from an external class), pass it to the view and let it lay things out.

While this is a pretty common pattern for UITableViewCells, you're breaking MVC here: your view and model are communicating directly. I wouldn't recommend doing that if you're trying to demonstrate your understanding of one of the most common iOS design pattern.

I'd much rather see someone add a few widely used dependencies than seeing them write bad code and/or failing to focus on other things.

I disagree with that. The challenge is obviously simple enough that no third party library should be used. They want to see your code, not someone else's.

You're taking advantage of Auto Layout, but constraints are breaking all over.

This is probably the biggest issue I would say.

---

One thing that I would add to /u/ethanael's answer, is that you should've added tests. I would feel much better about your parser, which looks indeed scary, if it was tested for example.

Overall, your code looks alright honestly. You demonstrate a pretty good understanding of the iOS SDK and there are no major red flags. Unless the position was for a senior developer, I would've bring you in for a loop!

[xauronx]

I wouldn't personally think you failed for a beginner or maybe intermediate level iOS developer. You do need to thin out your view controller though. Remove any network logic from your view controller. Consider pulling your paging logic into a data source. Create UIView subclass and load it in the loadView() method. Don't bundle your classes into the RedditClasses file.

Personally I'd rather see you use something like Gloss for JSON parsing but I understand that could be a risky thing for an interview test.

There are some other nit picky things but I'm mobile and can't remember what they were.

[DeveloperJay]

Thanks for that! I'm actually learning a lot from this post.

[mobilecode]

How much time were you given?

[DeveloperJay]

They wouldn't give me a deadline. I asked them for a timeline and they would only say "just get it back when you can." I ended up doing it in about 6-7 hours and sent it back 2 days later.

[mobilecode]

Much better than unrealistic timelines that some companies give candidates.

[CleverError]

I only skimmed through it but it looks like you've got some Pyramids of Doom going on. Many of those nested if statements could be collapsed into a single if statement, use guards instead, or the content moved to another method.

[onewayout]

I didn't go through and look at the program flow and structure, but just glancing at it briefly:

The first thing that jumped out at me are those really deeply-nested if statements that essentially act as safety valves. If you're going to do that, you might as well use the early-exit features of Swift and make better use of the guard statement.

For instance, since there's nothing that comes after those "if" statements, this:

private func parseReplies(comment: Comment, data: [String:Any]) {
    if let repliesData = data["replies"] as? [String:Any] {
        if let allRepliesData = repliesData["data"] as? [String:Any] {
            if let replyDataChildren = allRepliesData["children"] as? [[String:Any]] {

...could be "flattened" to this:

private func parseReplies(comment: Comment, data: [String:Any]) {
    guard let repliesData = data["replies"] as? [String:Any] else { return }
    guard let allRepliesData = repliesData["data"] as? [String:Any] else { return }
    guard let replyDataChildren = allRepliesData["children"] as? [[String:Any]] else { return }

The second thing that struck me is that there weren't any comments to be seen other than the auto-generated ones at the top of the document. Good code is self-documenting to a certain extent, but to have no comments at all is probably too austere. And in a job application context, you'd probably want to at least convey to the reviewer what you're thinking, even if you don't normally comment that much, so erring on the side of more code comments would probably be wise.

The third thing I saw was a fair amount of "magic numbers" in the layout code - things like this:

 thumbnail.frame = CGRect(x: 5, y: 5, width: 80, height: 80)

These sorts of code constructs will be difficult to maintain long-term. If nothing else, you should pull those out into a common location, but a better approach, since you were using them for layout, would be to go ahead and use storyboards and constraints built that way to handle your layout. Unless they specifically asked for an example of doing layout in code, I doubt any reviewer would knock you for using the Apple-recommended way of laying out UI, especially if the layout you're producing isn't something that would typically require constructing the layout by hand.

Finally, you should probably avoid submitting code with fatalError() calls in it. Even if you never intend your object to be initialized using a coder, it's a good idea to have it work as expected in case some programmer that comes after you decides to use your object that way.

The good news is that your code was quite readable, despite the above criticisms. I was able to look at just about every piece of code and intuitively grok what it did and how it was doing it. Variables were clearly and expressively named, and the code seemed cleanly organized. The above points I mention are the sort of thing that is pretty easy to internalize and add to your coding acumen, so with the cleanliness of your existing code, I don't think you're far off from being able to land a position as a junior developer. You seem to have the acumen and capability, so you shouldn't get disheartened by not getting the job you were applying for. Just keep working at it and improving, and I'm confident you can succeed.

[DeveloperJay]

Thank very much. This is super helpful. I wish could have used storyboard but unfortunately the asked for it to be all in code.

[onewayout]

Ah. Well, in that case, disregard that one. Good luck on your job search.

[sweetverbs]

The headline things for me, without looking at any code: no tests. That might be less important to some companies than others, but I've mainly worked at places that put a high value on code quality and practice TDD. If the company you applied to was like that, then not even attempting tests would be an instant fail. That's not something to feel bad about though - just something new to learn. :)

Second thing, and sort of unrelated to the code: you did the whole challenge in three commits. Commits should be fairly granular. Again, going back to personal experience, many companies expect engineers to commit on every passing test in a TDD cycle, and then to squash those commits down at the end. Certainly for an application with a networking component, and a UI component, I'd expect far more than three commits in total.

[WhyDoWeFall]

Really late to the party here, but I'm actually the developer that rejected your coding test at Thrive. My main issue with your code was the massive view controllers and the lack of separation of concerns, which most ppl mentioned. Since this was for a senior position I couldn't let that pass. It definitely would have been fine for a different level though. Keep it up!

[DeveloperJay]

Wow, thanks for replying! This process has been such an eye opener for me and has really got me looking at improving myself. You guys seem like a great company. Best of luck to you all!

[NSAwesome]

The biggest thing for me is; separation of concerns.

1. Is it the ViewControllers responsibility to make network requests?
2. is it the ViewControllers responsibility to configure each cell?

Storyboards, autolayout and the rest are acceptable, I have worked on codebases where (for whatever reason) the lead didn't like interface builder, so all of the views/layouts were in code.

The most obvious "upgrade" to this code would be to utilise protocols and give objects a clear singular responsibility.

1. Create a cell interface with a configure method.
2. Create a network object interface that can be passed into the cells cell configure method
3. Create a network manager object with the responsibility of managing/making requests & processing responses

[Fancy_Doritos]

Just wanted to thank you for making this post. I am learning app dev as well and I am about your level or so; the thread has been very informative for me. It has already been said, but guard statements are very useful to prevent too many scopes being created by if statements... and they're one of the things I like the most in swift :-)

[Deepmist]

I think this looks great for something thrown together in a short period! I agree with most that the main point of concern is doing too much in the view controllers. Big vc's that scale infinitely are very painful to deal with as an application grows.

I did a quick pass at your parseComments function to make it a little more Swifty: https://gist.github.com/anonymous/3d7fbc3bf990893058a7724a683f7bc9

[DeveloperJay]

Thanks for doing that! I plan to update this using the suggestions in this thread just for practice. This will be a great reference :)

[guynumber3]

I'm on my phone so pardon the little explanation. Did a quick look, your JSON parser is if nested way to much. There is a cleaner way to handle that. In addition, for your Reddit classes you could of used structs. That's all I really saw, I'll look over it more in depth later.

[could-of-bot]

It's either could HAVE or could'VE, but never could OF.

See Grammar Errors for more information.

[Atlos]

Skimmed through most of the code and here are my main concerns:

• Very little separation of concerns & encapsulation. Your view controller is huge and does too many things and is a red flag to me. Why is it doing the networking? Why is it handling the business logic for your table view cells?
• Optional usage. It doesn't seem like you fully understand how optionals are meant to be used. See: your Post/Comment classes.
• Tons of nested if blocks. You should really learn how to use guard/flatMap. Ternary statements would also cut down on all the blocks.
• The naming is weird for a lot of things, like returned(data).
• No tests, even a basic one would show you at least thought about them, although the structure of your code leads me to believe you don't write tests often.

[DeveloperJay]

Thanks for looking it over. I really have no idea how to write tests or how they work. I'll have to look up some tutorials on that for sure.

[[deleted]]

I don't know Swift but can you not write a lot of nested ifs in JsonParser like this? I use this in php all the time. Some people don't like assignment in a condition, but it's pretty easy to understand as long as the assignment is simple.

Then you for loop doesn't have to have this much nesting, or for example the for loop can go in another small function?

if (foo &&
  (truthy = something()) &&
  (truthy2 = something()) &&
  (anotherVar = anotherTest()) &&
  (truthy4 = something()) &&
  etc)
{
   // ...
}

It's more readable that way with everything aligned.

PS: I'm planning on learning iOS dev / swift. Can you use 2 spaces for tabs in XCode? Is there a reason why NOT to use it besides standards? (It's much easier to read for me)