Feedback from Valimail

[kmayer]

Hi Ken,

I just got out of our weekly hiring meeting and reviewed the details so thanks for beating me to the reach out! I was provided a list of pros/cons to share with you and if you'd like, I can give you a 2nd shot at the assessment based on this feedback (usually this is a pass or fail situation but I'd like to give you another chance if you are interested).

Ken Mayer - Homework Assignment Review

Pros:

• Project includes README.md, configuration instructions
• JSON format
• Each endpoint specifying required vs. optional parameters
• Grape having some support for Swagger, this potentially being useful in terms of documentation
• Application boots up (single worker/process)
• Automated Tests:
  • Requests specs properly implemented
  • All tests passing without failure:

$ rspec spec 
................... 
Finished in 0.29908 seconds (files took 2.07 seconds to load) 
19 examples, 0 failures 

Cons:

• Implementation notes are just explaining why he actually skipped certain validations he should have done
• Database migration files:
• Didn’t include any constraints on any columns
• Didn’t include any indexes
• Didn’t leverage “reference” types nor referential integrity (foreign key constraints)
• Database seeds: Missing
• No sample requests
• Multiple models in one file
• Model validations
  • Very light (eg: IPV4 validation)
  • Doesn’t allow IVP6
  • Allows special characters in domains/subdomains.
• Overall project:
  • No .ruby-gemset file
  • Didn’t leverage Rails::API, which is now the standard in Rails since 5.x
  • Used a different gem instead (Grape, comes with different conventions than what Rails community uses)
  • Controllers not under /controllers/ path
  • Rather default Gemfile
  • Not including any developer or test libraries
  • No linters (eg: rubocop)
  • All endpoints implemented in a single file (app/api/dns_too_simple/api.rb)
• Automated Tests:
  • No model specs written at all

Overall feedback

Your finished implementation should include all aspects of a production-ready application, applying current best practices within your selected tools and technical approach.

Based on this:

We could give another chance with 3 caveats/requirements:

1. Don't use Grape API, use Rails API mode
2. Domain names must be valid either write your own validator or use an existing one (hint!)
3. Write specs for model
4. (bonus) Use a linter

If you would like to proceed, please let me know and I look forward to hearing from you.

Best, Erica Schneider

[kmayer]

Erica,

I stand by my code given the constraints and goals supplied. There was no mention of

Your finished implementation should include all aspects of a production-ready application, applying current best practices within your selected tools and technical approach.

in the original problem set.

The changes mentioned by the reviewers are valid and would be part of a production application and a code review process, but your test is flawed and you will miss many qualified candidates.  In general, this is nitpicking. This was a coding exercise, not a complete solution. My time is valuable. This was a taste of what I can do, not what I would do for a production application.

I disagree with your evaluation for the following reasons:

1. CONS: "Implementation notes are just explaining why he actually skipped certain validations he should have done." Implementation notes were not even required.
2. CONS: "Database migration files: - Didn’t include any constraints on any columns." Database level constraints require a higher degree of error handling (because they will raise an error). Rails level validations are fine until there’s that higher requirement for database integrity. Since this was a coding exercise, I chose not to impose extra overhead. It would also require more tests that I deemed unnecessary.
3. CONS: "Didn’t include any indexes." Primary keys are indexed by default (maybe not SQLite, but Postgres), all references are through primary keys, so there’s no need for an explicitly created index.
4. CONS: "Didn’t leverage “reference” types nor referential integrity (foreign key constraints)." Reference types are mostly syntactic sugar for integer ids.
5. CONS: "Database seeds." Not required to run tests or boot up the server.
6. CONS: "No sample requests." Not required by the spec, but they are well documented via spec files and the Grape specification.
7. CONS: "Multiple models in one file." Single table inheritance with 1 base class and 2 subclasses. For a coding exercise, that’s perfectly acceptable. Of course I would break this up for production code. For sample code, this makes it much easier to read because you can see that all of the classes are super small and follow a similar pattern.
8. CONS: "Model validations. Especially, IpV4" Your specifications says, explicitly, "The Record Data must then be a valid IPv4 address.”  That’s why I didn’t include IPv6. Also, during our conversation, you said, don’t call back to ask for clarifications.
9. CONS: "Allows special characters in domains/subdomains" Did you really want me to validate against Punycode?
10. CONS: "No .ruby-gemset file." I use rbenv and bundler+Gemfile. My tooling does not require a .ruby-gemset file. I haven’t used one in years.
11. CONS: "Not using Rails::API." Grape is not an uncommon gem and it has the advantage of generating OpenAPI (it was called Swagger, but is has since been renamed) specifications that are critical for modern APIs.
12. CONS: “Controllers." The Grape gem has the added advantage of not forcing me to boiler plate any controllers.
13. CONS: “Default Gemfile." I thought this was a coding test. I made the smallest changes possible and required from the auto-generated rails app.
14. CONS: "Developer or test libraries." If you look at the Gemfile specification, I do specify development and test libraries, but on a line by line basis. This is what is generated by “bundle add —group=development,test” command.
15. CONS: "No linters." I use RubyMine, it has RuboCop built-in. If this were a production app, I would have linters as part of the continuous integration pipeline and there would be a configuration file.
16. CONS: "All endpoints in a single file." This is not a production application. I also did not include Grape’s entity library for streamlining the specification of API parameters.
17. CONS: "No model specs at all." The models were tested sufficiently at the API level that I did not feel the need to do any more. The requirement was “test appropriately” not “test as if this was going to be battle hardened production ready.” My tests do drive out the validations.

Here’s a control test for you. Have a current engineer complete this test anonymously and then you ask your reviewers to score it. Given that they are already hired, they should pass, right?

Ken Mayer