1

I sent the code for a job application and got the following review:

  • Regarding the Project Structure: Physical separation is absent. Logical separation is present but not up to best practices. Things seem to be separated "by the order they were needed to the solution”.

    Regarding Coding Conventions: White spacing is inconsistent and outside of conventions, disregard for immutability, explicit referral to "self", etc.

    Regarding readme documentation practices: Readme is not up to our best practices regarding usage, assumptions or an explanation of how the solution was setup.

    Regarding maintainability, extensibility, scalability or performance Architecture: No signs of architecture. App can be described as a massive view controller consuming services from a bunch of helper classes.

    Regarding DRY and SOLID principles: There's duplicated code in several places and in terms of SOLID principles, ViewController holds several responsibilities.

    God observations:

    Regarding the Use of a package manager for dependencies: Cocoapods was used.

    Error handling and logging: There was concern with error handling and logging in some degree.

Can someone give me a detailed explanation of each issue presented by the reviewer?

In case someone is curious about my response to the challenge:Github Link

Sorry for the long text but I think I'm helping other people by sharing my experience.

  • 3
    We cannot answer for them. You should ask them directly. Any answer or attempt would be a guess. – Adam Zuckerman Mar 11 '16 at 18:42
  • 2
    I am still not sure what the question is asking, either. Why was my question closed as "Unclear?" – user22815 Mar 11 '16 at 18:52
  • 9
    You are lucky to get any feedback at all, besides "thanks for your application". You may want to post a portion of your code to codereview.stackexchange.com. Questions focused on personal experiences are not generally useful and tend to get closed. Yours is too close to "Why didn't I get hired". There are many essays on DRY and SOLID readily accessible by Google search. – kevin cline Mar 11 '16 at 19:29
  • 2
    Sorry if I have given that impression, it's not my intention to make a "Why didn't I got hired" issue. I now have a clear idea of what I did wrong thanks to Ben Cottrell answer – Bruno Mar 11 '16 at 19:50
  • 4
    It sounds like the person (and company) you're dealing with is pedantic, beyond even what's usual for the field. I'd run- not walk- away from this opportunity. Is some of the feedback valid? Yeah, probably. I'd at least try to brush up on the terminology they used. Is this a place you want to work? No,unless you're one of those people who enjoys getting into holy wars over whether PascalCase or camelCase is more appropriate for, say, private methods. The mere fact that someone typed up all of this for you (someone they're presumably done with, in a practical sense) screams "know-it-all jerk." – user1172763 Mar 14 '16 at 20:04
22

Tackling each of these at a time:

Regarding the Project Structure

This one can be subjective at the best of times, but projects are typically structured by logically dividing your code into sub-folders or sub-projects based on their general area of responsibility, or which 'layers' they belong to. For example, Models, Views, Controllers, Core Application Logic, Shared/Common components, etc.

Regarding Coding Conventions: White spacing is inconsistent and outside of conventions, disregard for immutability, explicit referral to "self", etc.

When writing code it is good to adhere strictly to a coding standard. For example: https://github.com/raywenderlich/swift-style-guide

Where coding standards are concerned, one of the most important things is consistency. Inconsistent code is not nice to work with; it just looks sloppy and unprofessional.

Immutability is explained here: https://en.wikipedia.org/wiki/Immutable_object

Regarding readme documentation practices

It's not clear to me what the reviewer wanted in the readme, but your readme seems to be virtually empty. Presumably the reviewer expected some user-level documentation written in some user-friendly plain english language (e.g. how to use the app for the first time, what it is, what it does, how it works, etc.)

Regarding maintainability, extensibility, scalability or performance Architecture

In many ways, this crosses-over with the point about "SOLID" and "DRY" principles. But also indicates that your solution is lacking in logical separation of different "layers". In other words, the reviewer thought you had created a "Big Ball of Mud".

It's common for applications and systems to be comprised of several layers, which are each cleanly separated from each other; for example:

  • Data Layer (i.e. the Data Access Layer which works with persistent data)
  • Business Logic Layer (Core domain logic - e.g. the logic which actually handles all the requests, processes all the data, calculates results, etc.).
  • Application Layer (Application-specific logic - e.g. creating a request based on User input to call some function in the Business Logic layer).
  • Presentation Layer (View logic - e.g. MVC pattern; handling user interactivity, layout, presentation of data, etc.).

The reviewer mentioned some specific words:

  • Maintainability - for code to be maintainable it needs to be easy for someone to read, understand and follow. Classes should be loosely coupled. Functions should have low cyclomatic complexity. It needs to be easily testible, and have as much unit test coverage as possible. Again, this ties in with SOLID/DRY.

  • Extensibility - means the code should be designed in such a way whereby adding new functionality does not involve diving in to change a lot of existing classes/functions. SOLID: Open/Closed Principle

  • Scalability/Performance - presumably the reviewer considered that your code would not scale up well under heavy use.

Regarding DRY and SOLID principles

These are common software design principles which are worth spending time learning, and making sure you have a clear understanding:

SOLID - https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)

DRY - https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

Whoever reviewed your code clearly identified some violations in the solution you submitted. Typical "hallmarks" of code which violates these principles includes (but is not limited to)

  • Classes doing too many things (i.e. having too many responsibilities)
  • Long functions (somewhat subjective, but many people consider anything over 30 lines to be "too long")
  • Classes with too many functions
  • logic which is repeated in several places and could be rationalised down to a single class and/or function

I had a quick look at your code, and your ViewController definitely violates these principles, particularly your viewDidLoad() method, which seems to be doing far too many things.

Overall the impression I get (and which I expect the reviewer had) from looking at your code, is that there's not really any evidence that you have much experience in dealing with complexity or working with code written in a team of developers.

Most likely their main concern about you based on the challenge is that you wouldn't be able to take a project which is many times bigger than this challenge, with a team of people, and be able to break it down into layers and modules, or to structure the code in a way which other developers could work with.

But as a learning experience, it seems like you got some good, valuable feedback; your next step might be to take the solution that you've got, try to structure it properly with clean separation between your different layers, and use the project to learn how to apply SOLID principles.

  • 5
    I'm really really thankful for the time and feedback you have given me, I see now that I still have much to read and learn, I will do it all over again until I got it right, not for the job in question but for personal and professional growth. Best Regards! – Bruno Mar 11 '16 at 19:41
  • 3
    +1 Because you managed to explain the mistakes and still sound supportive. Kudos! – Andres F. Mar 11 '16 at 20:03
3

"DRY" means "Don't Repeat Yourself". They told you "There's duplicated code in several places...". This is bad. If you have to change it once, you potentially have to change it in a dozen (or hundred) places. (Many years ago, I had to do an overhaul on some code written by a guy who REALLY didn't understand DRY. It was not fun...)

"SOLID" refers to a bunch of things. See here. They said "ViewController holds several responsibilities", which is a Bozo no-no under SOLID.

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.