I'm obviously not rhinoceraptor, but a lot of what they said seems correct to me (speaking as someone who's interviewed others a lot, but never used at-home code assignments as part of the process).

_Some form_ of error handling and validation should be demonstrated, because that's a standard part of any program.

Equally SQL injection is _always_ important. That's not a 'hidden' judgement criteria. It's a fundamental security issue that should be protected against.

Again, I'm not rhinoceraptor, but the gist of their post seems correct. If I was reviewing some code and it had no error handling, no validation, lots of god classes and methods and was susceptible to the most common form of web-based security flaw, that is probably a sign that the candidate isn't suitable for the position.

Good, constructive feedback should then be provided.

>> Note how the evaluation changes from one person to other, you could be someone who expects an exception to be printed, I could be someone who expects an exception to be the wrong way, and would rather prefer a No Op.

I agree completely. Any good interviewer/code reviewer should be fine with someone using exceptions even if they prefer no ops.

If an interviewer did complain loads about that (without specifying how bad input should be handled), they are a bad interviewer.

>> The candidate could implement your spec 100% and yet get rejected.

But adding "The application should not be simple to hack" probably isn't something a company should need to add to the spec?

>> In 24 hours? What are you hiring for really? And why that kind of tearing hurry?

24 hours? really? Yes I understand your frustration, that's no-where near enough (assuming you mean a company asks for the solution back the day after requesting it).

If it was me, I'd assume that such a company doesn't value work-life balance and pass on them :)

> In 24 hours?

No, not in 24 hours. In about 2 hours. The laundry list of things - api validation, error handling, db migrations, good naming - it's pretty routine. A person writing production apis will have no trouble integrating all of it in a very short period of time. This is after all what we do everyday. If a candidate thinks not having sql injection in the code is an explicit requirement or is going to take a lot of time, then that is not the person for the job.

Here is a pretty simple flask implementation of your laundry list - api validation, db migration, no sql injections, no n+1 queries, good naming, swagger ui...

    from flask import Flask, jsonify
    from flask_sqlalchemy import SQLAlchemy
    from flask_migrate import Migrate
    from flask_apispec import use_kwargs, marshal_with, MethodResource, FlaskApiSpec
    from marshmallow import Schema, fields, validate, ValidationError
    from sqlalchemy.orm import joinedload

    # setup

    app = Flask(__name__)
    app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite:///dev.db'
    db = SQLAlchemy(app)
    migrate = Migrate(app, db)
    docs = FlaskApiSpec(app)


    # Return validation errors as JSON
    @app.errorhandler(422)
    @app.errorhandler(400)
    def handle_error(err):
        headers = err.data.get("headers", None)
        messages = err.data.get("messages", ["Invalid request."])
        if headers:
            return jsonify({"errors": messages}), err.code, headers
        else:
            return jsonify({"errors": messages}), err.code

    # models


    POST_TITLE_LENGTH_MAX = 80
    POST_CONTENT_LENGTH_MAX = 5000


    class Post(db.Model):
        id = db.Column(db.Integer, primary_key=True)
        title = db.Column(db.String(POST_TITLE_LENGTH_MAX), nullable=False)
        content = db.Column(db.String(POST_CONTENT_LENGTH_MAX), nullable=False)

        comments = db.relationship('Comment', back_populates='post')


    COMMENTER_LENGTH_MAX = POST_TITLE_LENGTH_MAX
    COMMENT_LENGTH_MAX = POST_CONTENT_LENGTH_MAX


    class Comment(db.Model):
        id = db.Column(db.Integer, primary_key=True)
        commenter = db.Column(db.String(COMMENTER_LENGTH_MAX), nullable=False)
        comment = db.Column(db.String(COMMENT_LENGTH_MAX), nullable=False)

        post_id = db.Column(db.ForeignKey('post.id'))
        post = db.relationship('Post', uselist=False, back_populates='comments')


    # schemas

    class CommentSchema(Schema):
        id = fields.Int(required=True, dump_only=True)
        commenter = fields.Str(
            required=True, validate=validate.Length(max=COMMENTER_LENGTH_MAX))
        comment = fields.Str(
            required=True, validate=validate.Length(max=COMMENT_LENGTH_MAX))


    class PostSchema(Schema):
        id = fields.Int(required=True, dump_only=True)
        title = fields.Str(required=True, validate=validate.Length(
            max=POST_TITLE_LENGTH_MAX))
        content = fields.Str(required=True, validate=validate.Length(
            max=POST_CONTENT_LENGTH_MAX))
        comments = fields.Nested(CommentSchema, many=True, dump_only=True)


    # dal

    def get_post_list():
        return Post.query.all()


    def get_post(post_id):
        return Post.query.options(joinedload(Post.comments)).get(post_id)


    def create_post(title, content):
        post = Post(title=title, content=content)
        db.session.add(post)
        db.session.commit()
        return post


    def add_comment_to_post(post_id, commenter, comment):
        comment = Comment(commenter=commenter, comment=comment, post_id=post_id)
        db.session.add(comment)
        db.session.commit()
        return comment


    # views


    class PostListResource(MethodResource):
        @marshal_with(PostSchema(many=True))
        def get(self):
            return get_post_list()

        @marshal_with(PostSchema)
        @use_kwargs(PostSchema)
        def post(self, title, content):
            return create_post(title, content)


    class PostResource(MethodResource):
        @marshal_with(PostSchema)
        def get(self, post_id):
            return get_post(post_id)


    class PostCommentResource(MethodResource):
        @marshal_with(CommentSchema)
        @use_kwargs(CommentSchema)
        def post(self, post_id, commenter, comment):
            return add_comment_to_post(post_id, commenter, comment)


    app.add_url_rule('/posts', view_func=PostListResource.as_view('post_list'))
    docs.register(PostListResource, endpoint='post_list')

    app.add_url_rule('/posts/<int:post_id>',
                    view_func=PostResource.as_view('post'))
    docs.register(PostResource, endpoint='post')

    app.add_url_rule('/posts/<int:post_id>/comments',
                    view_func=PostCommentResource.as_view('post_comment'))
    docs.register(PostCommentResource, endpoint='post_comment')

Exactly, if you provide the laundry list its easy to validate your assignment against it.

You submitted code which doesn't have things in my laundry list. Noticeably your db calls aren't surrounded by try/except. You have written no documentation. You didn't ship the code with git repo. No unit or functional test cases. I also expect these days one would use Python's typing library. Most of this helps in code maintainability. The fact that your assignment has none of this means you can't be hired to write production code ....

... See where this is going?

Despite you having written the code, someone with a different laundry list of quality checks can fail you.

> Exactly, if you provide the laundry list its easy to validate your assignment against it.

There is no laundry list. There is an expectation that a senior engineer writes codes like a senior engineer.

> You submitted code which doesn't have things in my laundry list. Noticeably your db calls aren't surrounded by try/except. You have written no documentation. You didn't ship the code with git repo. No unit or functional test cases. I also expect these days one would use Python's typing library. Most of this helps in code maintainability. The fact that your assignment has none of this means you can't be hired to write production code ....

Not sure if you are really this dense or playing dense - the point of the comment was to show that your assumption about api validation and sql injection being explicit requirements or taking too much time is ridiculous. It' simple that it can be written in a comment in about 10 minutes, not the "24 hours" or whatever you claim it is going to take.

The very fact that you think sql injection is an explicit requirement or takes work will be an instant deal breaker for any position with the possible exception of fresh grad positions.

> ... See where this is going?

Yes, I do. You are arguing reductio ad absurdum to hide the fact that the things which you claimed unreasonable are in fact routine, and your time estimates are off by order of 10.

>>There is no laundry list.

Followed immediately by.

>>There is an expectation that a senior engineer writes codes like a senior engineer.

You have no written list, but an imaginary checklist running in your brain about how a senior engineer writes code. Other engineers have theirs. I ran some of it and guess what, in that list absence of basic documentation, unit test cases, basic exception handling, code with types. Or even input validation for functions, like null checks is a no go.

Yet, if you submitted your feature complete project under a tough deadline to me, I'm not going to sit down and split hairs about absence of a favorite add-on feature of mine. That is what we are discussing here.

The code you posted in the comment above obviously doesn't have things like documentation, or validating function variables. Not even None checks. This obviously happens when some one attempts to write and submit code in minutes. When you write code this quickly, you focus on the exact feature demand at hand. Which in this case was SQL injection.

Other people got their feature set, in which several security add-on features would have been good to have but not in the time given.

>>You are arguing reductio ad absurdum to hide the fact that the things which you claimed unreasonable are in fact routine, and your time estimates are off by order of 10.

Oh obviously anyone who can do produce thousand(s) lines of code 24 hour project in 2.4 hours, is some totally different beast altogether.

There sure could exist such people who can produce >1000 lines of highly tested, security hardened code. I have yet to meet them though.

The closest I've seen is people who could write Lisp macros. But even those people wouldn't recommend writing code at the rate of 1000 lines per hour.

Welp. This is the point I checkout. You do you, but I have to ask. Do you have any production experience[1] and/or hiring experience? Because you have a very fundamental misunderstanding of both, and I find it very hard to believe that someone who has written even trivial applications would think that not having sql injection is an extra requirement or is going to take too much time, or anyone in hiring position is not going to politely terminate the interview after someone claims their assignment has an sql injection but that's because they didn't have much time and it wasn't explicitly asked for, or even consider calling someone for in-person eval when their assignment has an sql injection.

[1] There is a lot wrong with your "improvements". db calls aren't randomly placed in try/catch - that will be absurd. And the None checks aren't there because of something which you can very clearly see in the sample code but you also very clearly don't understand.

I keep repeating that this is not about SQL injection. You could be asked to write a command line app which doesn't even involve a SQL database. And yet there could a lot of such security or even other add-on's on which your code could be evaluated. We are literally discussing code quantity + time Vs Quality here.

>>Do you have any production experience >>db calls aren't randomly placed in try/catch - that will be absurd.

Calls to databases go wrong all the time. Session objects expire, if you have cert based auths, they expire all the time. Especially if you have a largish microservices stack. Or you have locks on db objects, which you would run into. Or that network got flaky, or that just a certain type of resources like maxing out on db connections. Or that you are just trying to do write something to a database that is not allowed, like a duplicate primary key.

Never assume a database or any IO call for that matter will always go right.

Always check for every possible exception that could likely be thrown and handle it appropriately. Preferably each db interaction function should be atomic, and exception should be bubbled/raised/thrown to the application/feature main loop so that appropriate action like a retry or a roll back can be taken.

>>And the None checks aren't necessary because of something which you can very clearly see in the sample code but you also very clearly don't understand.

Pretty much any large codebase, that passes objects around should always do Null pointer checks. This is because several times resource heavy objects are initialized only on certain conditions, and if such objects are passed around they must be checked.

> I keep repeating that this is not about SQL injection.

Is that why you made that absurd claim about sql injection being an explicit requirement? And that weird figure of 24 hours for handling sql injection, and api validation?

> Never assume a database or any IO call for that matter will always go right.

I said "db calls aren't randomly placed in try/catch - that will be absurd". Because they will be handled at app level to return uniform error messages. Now I am sure you will go on pretending that when you said "db calls aren't in try/catch", what you meant was db calls can throw an exception and app will handle it.

> Pretty much any large codebase, that passes objects around should always do Null pointer checks. This is because several times resource heavy objects are initialized only on certain conditions, and if such objects are passed around they must be checked.

What did I say about None checks not necessary because of something which is visible in the code? What do you think those marshmallow schemas and use_kwargs is for?