The code overall looks reasonable and I don’t write much python but:

- requirements.txt is the standard for specifying deps, right?

- making a README explaining the project might be good (I see you have this, but maybe switch documentation.md to README.md)

- tests?

- building the flask application via a function is a bit more testable

- reading settings from ENV is preferred (12 factor apps)

- output not sanitized for /test endpoint

- generator in util could have been pulled out probably? It’s created every time

- generate test path function seems a bit longer than it should be — all those lambdas should probably just be functions since you’re going to use them a lot

- did you have to define your own Json type? Is that complete? Where’s null?

- url generate function should probably template hostname — that’s more important than host, most of the time for running in different environments

- trailing slashes matter in flask, evidently, and every request without one gets redirected (test suite would have caught this)

- on the usage of redis, I wonder if scanning + in-memory aggregation is better… zincr/zrank/zrevrange is good, but you’ll have to hold all data in memory (and receive it in one large response) and logN anyway, might as well do it simply with a set with a dynamic prefix and scan while building the output data structure as you go.

- do your API endpoints return JSON?

- error handling around points of failure like redis

— your app goes down if the connection is flaky right?

- zrevrange is deprecated now btw

Hard to tell for far they wanted you to go, but asking might have made sense… some nice-to-haves:

- healthz endpoints?

- metrics?

- tracing?

- error reporting (ex. Sentry)

Huge thanks for the feedback!

>- requirements.txt is the standard for specifying deps, right?

poetry is a new thing. I just tried it for this project it replaces requirements.txt with pyproject.toml

>- making a README explaining the project might be good

I did. It's documentation.md. I didn't title it README.md because I wanted the front page on github.com to show the takehome instructions rather then my docs on it.

>- tests?

This was addressed in the documentation.md. I was given 4 hours to work on this problem so I just used manual tests.

>- building the flask application via a function is a bit more testable

Flask is an IO app. It's inherently not testable via unit tests because it's a server. You'd have to build integration tests around an entire server which is huge overkill for this project. Testable logic is usually pure and stateless, that's located in utils.py. It's ok this is good criticism.

You can monkey patch or make your code 10x more complicated with modules that accept mockIO for dependency injection but I'm actually against this style of programming as it over complicates the code with little benefit.Trying to test Flask by mocking everything out is basically just testing the mocks. Better to do this haskell style and segregate IO from pure functions (see utils.py). IO can't be unit tested.

Not many people understand the "testing" philosophy I'm following here so this is good feedback. I should sometimes just follow what's popular but ultimately not the "best" way in my opinion.

>- - output not sanitized for /test endpoint

this I don't get? Sanitized? The http request returns typical python structs, which are automatically converted to Json by flask/quart.

>- did you have to define your own Json type? Is that complete? Where’s null?

Nope don't have to, type checking is optional in python, but why not? You have to do this to get correct type annotations to everything. You're right about this missing Null/None. It all still type checks.

>- generator in util could have been pulled out probably? It’s created every time

Why pull it out? A generator is cheaper then creating a list from a list comprehension every time. instead of storing every value in a list and iterating through it, I just iterate through a generator. Saves memory and runtime cost is still O(N) regardless.

>- - url generate function should probably template hostname — that’s more important than host, most of the time for running in different environments

Sure but the context is a takehome project and it's running in docker-compose as specified by the directions. I mean yes, I can make that utility function more general for sure.

>- trailing slashes matter in flask, evidently, and every request without one gets redirected (test suite would have caught this)

I'm aware of this, it's not a mistake. I left it in as valid. The redirect does not get counted so api/xxx/ is equivalent to api/xxx

- on the usage of redis, I wonder if scanning + in-memory aggregation is better… zincr/zrank/zrevrange is good, but you’ll have to hold all data in memory (and receive it in one large response) and logN anyway, might as well do it simply with a set with a dynamic prefix and scan while building the output data structure as you go.

Your way is NlogN sorting and constant time inserts. The current way is zero cost sorting and logN inserts.

LogN is blazing fast, while nlogn can get slow if N gets too large. See this for relative visualization: https://i.stack.imgur.com/osGBT.jpg. Given the picture I would say my way is better. .

>- - do your API endpoints return JSON?

Yes this is the default. If you return anything that's equivalent to that JSON type I defined in utils.py, flask will automatically return serialized json.

> - error handling around points of failure like redis

Flask/quart runs under the hypercorn server (see the dockerfile). Additionally If a handler throws an exception the user automatically gets a 500 error from flask it's handled exactly as you would expect an http server should handle it. The server does not crash.

>— your app goes down if the connection is flaky right?

No it does not. hypercorn remains running if the python worker crashes. But of course if hypercorn itself goes down then it's done. This of course can be mitigated by systemd but that's overkill for this project.

>- zrevrange is deprecated now btw

Yeah your right. My mistake. Still works though. So it's marked for deprecation, but not actually deprecated yet.

>-healthz endpoints?

>-metrics?

>-tracing?

>-error reporting (ex. Sentry)

For a four hour take-home project? These things weren't even in the spec and how these things and if these things are implemented are extremely variable per company across the industry.

You know overall your post has been enlightening but not in the way you think because we are clashing on most of these points. I can see how developers can literally disagree on everything and how code reviewers can make a ton of assumptions and have a ton of arbitrary opinions. Not to mention varying levels of experience and what experience for that matter causes them to make completely different judgement calls. Don't take this as a slight, I'm sure if I was in your position I would be reviewing your code in the same way and you'd see me in the same way as well.

I'm starting to think take home assignments are just bad. Even worse for evaluating programmers then algorithm interviews because there's so many biased variables here that programmers are just clashing on. Maybe it's a good filter for finding programmers who view the programming world in the exact same way they see it.

> poetry is a new thing. I just tried it for this project it replaces requirements.txt with pyproject.toml

Ah thanks, I saw this later -- the pyproject.toml was very illustrative.

> I did. It's documentation.md. I didn't title it README.md because I wanted the front page on github.com to show the takehome instructions rather then my docs on it.

Is there a reason for this? I would think this was a good chance to show off your ability to write succinct/standardized documentation w/ sections that anyone else would be glad to read/see.

> This was addressed in the documentation.md. I was given 4 hours to work on this problem so I just used manual tests.

Yeah I can see this, but I think "automated tests" are quite big in peoples' minds. Feels like it would be a checkbox on the item.

Budgeting some of the time to make 100% sure your code was tested seems like it might have been a good idea.

> Flask is an IO app. It's inherently not testable via unit tests because it's a server. You'd have to build integration tests around an entire server which is huge overkill for this project. Testable logic is usually pure and stateless, that's located in utils.py. It's ok this is good criticism. > > You can monkey patch or make your code 10x more complicated with modules that accept mockIO for dependency injection but I'm actually against this style of programming as it over complicates the code with little benefit.Trying to test Flask by mocking everything out is basically just testing the mocks. Better to do this haskell style and segregate IO from pure functions (see utils.py). IO can't be unit tested. > > Not many people understand the "testing" philosophy I'm following here so this is good feedback. I should sometimes just follow what's popular but ultimately not the "best" way in my opinion.

I disagree here -- table stakes for a good API is integration and E2E testing -- and they're the easiest to do (no need to manipulate a browser window, etc).

I agree with you on not testing the mocks, but for that I personally would bring in a library for this like testcontainers for python: https://testcontainers-python.readthedocs.io/en/latest/READM...

IMO E2E tests are the most valuable tests.

> this I don't get? Sanitized? The http request returns typical python structs, which are automatically converted to Json by flask/quart.

Sorry INPUT is what I meant to write there -- what happens if you get a -1 ?

> Nope don't have to, type checking is optional in python, but why not? You have to do this to get correct type annotations to everything. You're right about this missing Null/None. It all still type checks.

Right, but this is what I mean -- when I read that I wondered if it was complete, and it wasn't. That's an unnecessary ding to take.

If you're going to type it, it should type correctly, or you should use pull in the typings from somewhere else, IMO.

> Why pull it out? A generator is cheaper then creating a list from a list comprehension every time. instead of storing every value in a list and iterating through it, I just iterate through a generator. Saves memory and runtime cost is still O(N) regardless.

This is going to build objects dynamically every execution right? That's what I thought could be avoided. Less about lists vs generators, more about doing things that look like they allocate so much.

Do you think there's something you could do to create less garbage during the function execution? If not then you can disregard that point!

> Sure but the context is a takehome project and it's running in docker-compose as specified by the directions. I mean yes, I can make that utility function more general for sure.

It's not about making it general per say -- it's more like what is your instinct at this point.

My instinct personally (and granted I spent like... 30min to an hour just reading all the code) is that I always take my host/port from ENV, and everything else possible. It's just something you know is going to come up.

> I'm aware of this, it's not a mistake. I left it in as valid. The redirect does not get counted so api/xxx/ is equivalent to api/xxx

Ah OK, but this isn't what they asked for, right? If we want to be really pedantic, your endpoint returns a 3xx for every single error.

Redirects are not followed automatically, it depends on the HTTP client, right? So one that doesn't follow redirects would fail this test. It's unlikely, but maybe this code failed automated testing for a little reason like that.

> Your way is NlogN sorting and constant time inserts. The current way is zero cost sorting and logN inserts. > > LogN is blazing fast, while nlogn can get slow if N gets too large. See this for relative visualization: https://i.stack.imgur.com/osGBT.jpg. Given the picture I would say my way is better. .

The measurement endpoint will be called far more (100x, 1000x, ...) than the stats endpoint -- it's the hot path.

Also, note that sorting is zero cost, but retrieving the data will still be in N time including network round trips, which will likely dwarf any time you spend sorting in local memory. Even the conversion to JSON will dwarf the time you spend sorting for large N.

You're right about the complexity -- but note that ZREVRANGE is O(log(N)+M), and in this case we KNOW that M = N, so IIRC that's O(N).

Regardless of the complexity, I personally wouldn't make that tradeoff until it was necessary -- speed of the hot path is more important in my mind, and there are other ways to manage speed of the stats endpoint.

Also, the point of memory usage being potentially large is still relevant. My instinct is that when I see something returning a possibly very large piece of data, setting up for streaming is the way to go. Doing a scan you have the possibility to use far less memory for an individual request, and you could actually stream responses back in extreme cases, so you can avoid holding the entire result set in memory at all.

If you want to get really fancy, you can get creative with how data is stored on the redis side and get the size to N.

> Flask/quart runs under the hypercorn server (see the dockerfile). Additionally If a handler throws an exception the user automatically gets a 500 error from flask it's handled exactly as you would expect an http server should handle it. The server does not crash.

Ahh OK, I did see hypercorn in the Dockerfile and forgot about it, I see how that your worker would be restarted by hypercorn.

The point about error handling still stands with respect to reporting though -- you generally want to see error handling in code, rather than just leaving it up to the runtime to fail the task and carry on.

Adding this would have prompted you to think about some sort of error reporting, which is generally good practice.

> For a four hour take-home project? These things weren't even in the spec and how these things and if these things are implemented are extremely variable per company across the industry.

The things you include/don't include by instinct I think is valuable signal.

Like I said, they're nice-to-haves, but error reporting is probably the big one there. In production errors should to be gathered somewhere.

You can make the argument that they'll be collected by a logging system and that's why you've left them out, but I also don't see any structured logging.

> You know overall your post has been enlightening but not in the way you think because we are clashing on most of these points. I can see how developers can literally disagree on everything and how code reviewers can make a ton of assumptions and have a ton of arbitrary opinions. Not to mention varying levels of experience and what experience for that matter causes them to make completely different judgement calls. Don't take this as a slight, I'm sure if I was in your position I would be reviewing your code in the same way and you'd see me in the same way as well. > > I'm starting to think take home assignments are just bad. Even worse for evaluating programmers then algorithm interviews because there's so many biased variables here that programmers are just clashing on. Maybe it's a good filter for finding programmers who view the programming world in the exact same way they see it.

Yeah, that's a pretty common take (see all the comments in this thread).

I'd disagree that it's worse than algo interviews, because algos are rarely the most important thing in a professional code base, especially one with that's about web services and not something more demanding.

All this said though, the only way to know why they passed is to ask! Unfortunate that you can't get any feedback.

>Is there a reason for this? I would think this was a good chance to show off your ability to write succinct/standardized documentation w/ sections that anyone else would be glad to read/see.

I threw the docs in the README for the submission, but for HN readers they want to see the problem first. The submission didn't even include the specs.

>Yeah I can see this, but I think "automated tests" are quite big in peoples' minds. Feels like it would be a checkbox on the item.

Yeah you're completely right here. I'm gonna go with it next time.

>I disagree here -- table stakes for a good API is integration and E2E testing -- and they're the easiest to do (no need to manipulate a browser window, etc).

Yeah but many companies don't do integration testing as the infra on this is huge and writing tests is more complicated. It's certainly overkill for take home. I would say you're wrong here. Completely. This company was not expecting integration tests as part of the project at all.

I've worked for start ups most of my career, and I'm applying to start ups. It's mostly the huge companies that have the resources to spend the effort to have full coverage on that.

Integration tests are Not easy, btw. Infrastructure is not easy to emulate completely, how would I emulate google big query or say aws iot on my local machine? Can't, most likely integration tests involve actual infra, combined with meta code that manipulates docker containers.

>Sorry INPUT is what I meant to write there -- what happens if you get a -1 ?

You get a 500. Which is not bad. But you're right a 400 is better here.

>Right, but this is what I mean -- when I read that I wondered if it was complete, and it wasn't. That's an unnecessary ding to take.

I feel this is such a minor thing. I'm sure most people would agree if you dinged that you'd be the one going overboard, not the candidate.

>This is going to build objects dynamically every execution right? That's what I thought could be avoided. Less about lists vs generators, more about doing things that look like they allocate so much.

You're going to build objects dynamically every execution Anyway. This saves you the extra intermediary step of not having to save it to an extra list.

    numbers = [i*2 for i in range(500)] #allocated list in memory: 500*sizeof(int) (big) 
    for i in numbers:
       print(i + 1)

    numbers = (i*2 for i in range(500)) #allocated generator in memory: sizeof(*generator) (small)  
    for i in numbers:
       print(i + 1)

>Ah OK, but this isn't what they asked for, right? If we want to be really pedantic, your endpoint returns a 3xx for every single error.

It's a reasonable assumption that all users assume /xx/xx equals /xx/xx/. It's also reasonable for a client to handle a 302 redirect. The specs didn't specify the exact definition on either of these things.

>The measurement endpoint will be called far more (100x, 1000x, ...) than the stats endpoint -- it's the hot path.

So? logN is super fast. You have 100 objects binary or whatever indexed insert redis uses gets there in at most 4 or 5 jumps. So even if it's the hot path redis can take it.

NLogN is slow enough that if N is large there is noticeable slow downs EVEN when it's a single call. Recall that redis is single threaded sync. NlogN can block it completely.

Either way this is all opinion here right? Ideally the results are returned unsorted. the client is the best place to sort this, but that's not the requirement.

>Also, note that sorting is zero cost, but retrieving the data will still be in N time including network round trips, which will likely dwarf any time you spend sorting in local memory. Even the conversion to JSON will dwarf the time you spend sorting for large N.

Of course the act of reading and writing data will cost N regardless. But your reasoning is incorrect. Look at that picture again: https://i.stack.imgur.com/osGBT.jpg. O(NlogN) will dwarf O(N) when N is large enough. It will even dwarf IO round trip time and has the potential of blocking redis completely on processing the sort.

>Also, the point of memory usage being potentially large is still relevant. My instinct is that when I see something returning a possibly very large piece of data, setting up for streaming is the way to go. Doing a scan you have the possibility to use far less memory for an individual request, and you could actually stream responses back in extreme cases, so you can avoid holding the entire result set in memory at all.

Sure but look at the specs. It's asking for http requests. It's not a streaming protocol here. I mean at this point if you were evaluating my code for a job you'd be going to far already as your going off the rails completely.

>The point about error handling still stands with respect to reporting though -- you generally want to see error handling in code, rather than just leaving it up to the runtime to fail the task and carry on.

I mean I could build a full logger and error handler framework here. I'd put custom handling in a decorator and have the error handling invisible in the http route handlers. Flask already does this, but it just returns 500 on all exceptions. That is the most elegant way imo. But again this is a bit outside of the scope of a takehome, given the framework handles a lot it and the extra effort required.

>If you want to get really fancy, you can get creative with how data is stored on the redis side and get the size to N.

For takehomes and in general people prefer readable code. Only optimize if it's needed. I mean I can write the whole thing in assembly code for ultimate performance. I can def see some nitpicker dinging me for optimized but unreadable code.

>The things you include/don't include by instinct I think is valuable signal.

Not to be insulting here. But I think your instincts are pretty off here. There's a limited amount of time assigned to this project. All the basics were included with what I did in the time allotted.

When you have good instincts you know when to apply automated testing, when to use error handling. These aren't things you should do all the time. Your attitude is something I see in many younger engineers. This attitude of rule following and best practices is just adhered to without thinking it all through properly. For the scope of the project. most of what you wrote here isn't needed.

You're also not testing what's traditionally tested in interviews. They test intelligence, depth knowledge and ability to design and solve problems. You're testing the ability to follow written rules. I mean following these things is trivial. It doesn't take much brain power to write error handling or unit tests. Perhaps you're just testing for someone that agrees with your programming philosophy. I mean these things can be followed regardless

Your mention of integration tests is telling. I mean the code for integration tests can rival the size of the code in the project itself with the a minor benefit in safety . It's not just take home assignments but many many many companies loaded with senior engineers skip over automated integration tests and keep those tasks in todo lists for the longest time. There's a huge engineering cost to building out testing infra to the maximum ideal, and many companies just don't bother. It's mostly the bigger well established companies that have the resources to do this.

>I'd disagree that it's worse than algo interviews, because algos are rarely the most important thing in a professional code base, especially one with that's about web services and not something more demanding.

It is worse in my opinion because it's biased. What if your opinion was more correct then my opinion but I judged you based off of my world view? Would that be right? No it wouldn't. Programmers are diverse and the best organizations have diversity in opinions and expertise.

All in all I mostly disagree but I think your right on one thing: At least one of those job candidate evaluators that are looking at my code likely had a similar attitude as you do and I should bias my code towards that more. Likely won't write integration tests though, hard no on that one.

Also good catch on the /test/-1/. That's definitely a legit ding imo.

> Yeah but many companies don't do integration testing as the infra on this is huge and writing tests is more complicated. It's certainly overkill for take home. I would say you're wrong here. Completely. This company was not expecting integration tests as part of the project at all. > > I've worked for start ups most of my career, and I'm applying to start ups. It's mostly the huge companies that have the resources to spend the effort to have full coverage on that. > > Integration tests are Not easy, btw. Infrastructure is not easy to emulate completely, how would I emulate google big query or say aws iot on my local machine? Can't, most likely integration tests involve actual infra, combined with meta code that manipulates docker containers.

I think I can agree with this -- it's true that most companies don't do it, but personally just spinning the thing up and throwing it a web request usually has rails in most languages these days. I spend more time in the NodeJS ecosystem, so I have things like supertest (https://www.npmjs.com/package/supertest) so maybe I'm spoiled.

> I feel this is such a minor thing. I'm sure most people would agree if you dinged that you'd be the one going overboard, not the candidate.

Yeah this is pretty reasonable -- opting in to typing at all is more a plus than a negative, on balance.

> You're going to build objects dynamically every execution Anyway. This saves you the extra intermediary step of not having to save it to an extra list.

It seems like I wasn't clear about how it's not about the lists/generators, so here's an explicit instance:

random_string_generator (https://github.com/anonanonme/takehome-sample/blob/master/ut...) is created as a callable every single time generate_test_paths. This is unnecessary, and could be pulled out to just be a static function.

If your argument is that the lambda gets optimized out, or that the benchmarked difference is insignificant (it very well could be!), then I could understand that.

> It's a reasonable assumption that all users assume /xx/xx equals /xx/xx/. It's also reasonable for a client to handle a 302 redirect. The specs didn't specify the exact definition on either of these things.

I'd argue the difference is so significant that it warranted a note in the documentation on the semantics, and if you google things like "trailing slash" it's been a thorn in peoples' sides for a long time.

Specs did specify test cases -- and none of them had a trailing slash.

I think we're really in the weeds here (in any reasonable working environment, this isn't a big deal), but it's wasteful to have every request become 2.

> So? logN is super fast. You have 100 objects binary or whatever indexed insert redis uses gets there in at most 4 or 5 jumps. So even if it's the hot path redis can take it. > > NLogN is slow enough that if N is large there is noticeable slow downs EVEN when it's a single call. Recall that redis is single threaded sync. NlogN can block it completely. > > Either way this is all opinion here right? Ideally the results are returned unsorted. the client is the best place to sort this, but that's not the requirement.

These are good points. Thinking about it though:

- logN is not faster than O(1) - NLogN is in the API, not redis -- redis experiences N while the scan is going

You're right that NLogN would certainly have a chance of blocking redis completely much more than N would, but I think in both cases redis experiences O(N) behavior for the stats endpoint, not NLogN.

> Of course the act of reading and writing data will cost N regardless. But your reasoning is incorrect. Look at that picture again: https://i.stack.imgur.com/osGBT.jpg. O(NlogN) will dwarf O(N) when N is large enough. It will even dwarf IO round trip time and has the potential of blocking redis completely on processing the sort.

Ahh, see the point about N/NLogN -- it's not Redis that experiences NlogN, it's the API. Redis just happily feeds all the values down, and they're aggregated at the python level.

Now, as far as the python level goes, I'm arguing that NLogN for in-memory number is going to be tiny compared to doing network requests and serializing JSON.

> Sure but look at the specs. It's asking for http requests...

That's reasonable -- the assumption is basically that the return fits in one request. I do think not trying to download the whole world at the same time is a legitimate concern, but it's not necessarily a pressing one since it wasn't mentioned.

> I mean I could build a full logger and error handler framework here...

Ah see I would expect you to pull one in -- just like you know of poetry, I'd be impressed by seeing more well-built creature comfort tools like that. Knowing good off-the-shelf tooling is a plus, IMO.

I agree with you that this is probably not what they expected (it's only a 4 hour take home!) but just wanted to make the point.

> For takehomes and in general people prefer readable code. Only optimize if it's needed. ...

Yeah that's true, too much optimization would definitely be bad -- but my point wasn't that you should optimize, it was that the approach I was talking about could be optimized (so that would be my answer to the NLogN versus N questions).

I think my version is the least surprising one -- no one has to know about pipeline or worry about atomicity. Just an O(1) operation to redis, like most people would expect to see.

> Not to be insulting here. But I think your instincts are pretty off here. There's a limited amount of time assigned to this project. All the basics were included with what I did in the time allotted. >

I think maybe I wasn't clear -- your instinct is what's on display. What you choose to include/use/not use is valuable signal (and that's the point of the take home).

> When you have good instincts you know when to apply automated testing, when to use error handling. These aren't things you should do all the time...

Well, if making sure to include automated testing for APIs and error handling is junior, I look forward to staying a junior engineer for my whole life. I don't think I'd submit a take home coding test without tests.

I'm absolutely not dogmatic about it (and I have the repos to prove I don't always write tests :), but I certainly am not proud to show anyone untested code -- it is enshrined in my mind as a measure of quality. If I'm putting my best foot forward, the code will have tests, especially when it's languages with weak type systems.

If the prompt was "write this like you're at a startup that has no money and no time", then sure. But even then -- technical debt forces rewrites at startups all the time, and the often the effect is worse when people don't write tests. Most of the time, you move slow so you can move fast.

> You're also not testing what's traditionally tested in interviews. They test intelligence, depth knowledge and ability to design and solve problems. You're testing the ability to follow written rules. I mean following these things is trivial. It doesn't take much brain power to write error handling or unit tests. Perhaps you're just testing for someone that agrees with your programming philosophy. I mean these things can be followed regardless >

I think this is the difference between take homes and whiteboard algo tests. I think the point of the take home is to see what you will do, on a realistic project that you were given full control over. That's what makes it better than the algo tests IMO -- it's more realistic and you have full control.

Everyone knows they should write tests... But do you actually? Because one thing is definitely harder/takes more discipline than the other.

> Your mention of integration tests is telling. I mean the code for integration tests can rival the size of the code in the project itself with the a minor benefit in safety . It's not just take home assignments but many many many companies loaded with senior engineers skip over automated integration tests and keep those tasks in todo lists for the longest time. There's a huge engineering cost to building out testing infra to the maximum ideal, and many companies just don't bother. It's mostly the bigger well established companies that have the resources to do this.

Yes, but this is a small API -- you literally have to write a test that hits the server once. There are libs for doing this with flask, there is documentation showing you how. It's not rocket science, and it's crucial to catching bugs down the road.

> It is worse in my opinion because it's biased. What if your opinion was more correct then my opinion but I judged you based off of my world view? Would that be right? No it wouldn't. Programmers are diverse and the best organizations have diversity in opinions and expertise. > > All in all I mostly disagree but I think your right on one thing: At least one of those job candidate evaluators that are looking at my code likely had a similar attitude as you do and I should bias my code towards that more. Likely won't write integration tests though, hard no on that one. > > Also good catch on the /test/-1/. That's definitely a legit ding imo.

Just about everything with humans in the loop is biased -- that's the world we live in. I don't know if they tried to run your test, but it could have failed with a 3xx and then they disqualified you right there, or bad input or whatever.

But yeah unfortunately it's hard to learn anything from this other than giving opinions on what rang bells for me.

Would be awesome if they gave you feedback.

Overall the code is reasonable and probably works for the normal cases -- it is a mystery why they wouldn't give feedback.

>If your argument is that the lambda gets optimized out, or that the benchmarked difference is insignificant (it very well could be!), then I could understand that.

In interpreted languages I believe the thing that gets saved to mem is a pointer to the code itself. So no inherit difference here in terms of allocation. For closures reference counting is increased for the referenced variables but this is not a closure.

Still this is a minor thing. Scoped functions are used for structural purposes performance benefits of using or not using them are negligible.

>logN is not faster than O(1) - NLogN is in the API, not redis

My point is log(n) is comparable to o(1). It's that fast.

NlogN is not comparable to O(n). In fact if you look at that pic NlogN is more comparable to O(n^2).

You definitely don't want NlogN on the application server whether its nodejs or python. That will block the python or node thread completely if n is large.

It's preferable to use SORT in redis then in python or node because redis is highly optimized for this sort of thing (punintended) as it's written in C. Even if its not in redis, in general for backend web development you want to move as much compute towards the database and off the webserver. The webserver is all about io and data transfer. The database is where compute and processing occurs. Keep sorting off web servers and leave it all in the database. You're a nodejs dev? This is more important for node given that the default programming model is single threaded.

Overall it's just better to use sets with scores because logN is blazingly fast. It's so fast that for databases it's often preferential to use logN tree based indexing over hashed indexing even when hashed indexing is appropriate.

Yes hashed indexing are O(1) average case insert and read but the memory savings of tree based indexing overshadows the negligible logN cost.

>I think my version is the least surprising one -- no one has to know about pipeline or worry about atomicity. Just an O(1) operation to redis, like most people would expect to see.

Two things here.

1st. Sorting on the web API is definitively wrong. This is especially true in nodejs where the mantra is always non blocking code.

Again the overall mantra for backend web is for compute to happen via the database.

2nd. Pipeline should 100 percent be known. Atomic operations are 100 percent required across shared storage and it's expected to occur all the time. Database transactions are fundamental and not unexpected. Pipeline should be extremely common. This is not an obscure operation. I commented about it in my code in case the reviewer was someone like you who isn't familiar with how popular atomic database transactions are, but this operation is common it is not a obscure trick.

Additionally pipeline has nothing to do with your or my own implementation. It's there to make atomic a state mutation and retrieval of that state.

I add one to a score then I retrieve the score and I want to make sure no one changes the score in between the retrieval and the add one. This is needed regardless of what internal data structure is used in redis.

>I don't know if they tried to run your test, but it could have failed with a 3xx

That'd be stupid in my opinion. 302 redirect is something I expected. Maybe I should have stated that explicitly in the docs.

>IMO -- it's more realistic and you have full control.

Unlikely. Even as a lead it's better to cater to the majority opinion of the team. 99 percent of the time you never have full control.

Developers and leads typically evolve to fit into the overall team culture while inserting a bit of their own opinions.

I'm a rust and Haskell developer applying for a python job. Do I apply my entire opinion to control the team? No, I adapt to all the bad (in my opinion) and good practices of the team. I introduce my own ideas slowly and where appropriate.

> it is a mystery why they wouldn't give feedback.

This is easy. It's not a mystery. It's because they don't give a shit about me. They don't want to hire me so the business relationship is over. Legally they owe me zero time to explain anything and it's more efficient for the business to not waste time on feedback.

>Yes, but this is a small API -- you literally have to write a test that hits the server once. There are libs for doing this with flask, there is documentation showing you how. It's not rocket science, and it's crucial to catching bugs down the road.

No there isn't. You have to spin up redis. Flask documentation doesn't talk about that. Integration testing involves code that controls docker-compose. It's a bunch of hacky scripts with external process calls that are needed to get integration tests working.

It's not rocket science but not within the scope of a take-home. Additionally hacking with docker compose is not sustainable or future proof, eventually you will hit infra that can't be replicated with docker.

I will probably do it next time just to cater to people who share your viewpoints.

>If the prompt was "write this like you're at a startup that has no money and no time", then sure.

Lol. The prompt was write it in four hours ( no time) and they aren't paying me for it ( no money).

>Specs did specify test cases -- and none of them had a trailing slash.

What? In the specs all examples have trailing slashes. All of them.

https://github.com/anonanonme/takehome-sample/blob/master/RE...

Take a look again, you remembered wrong.

What the specs didn't specify was what to do when there is no trailing slash. So why not a 302? The client can choose what to do here. Either follow the redirect or treat it as an error.

> In interpreted languages I believe the thing that gets saved to mem is a pointer to the code itself. So no inherit difference here in terms of allocation. For closures reference counting is increased for the referenced variables but this is not a closure. ...

This has all been tread over before -- I won't rehash it since it seems like we're going in circles.

The hot path is worth optimizing first, in my mind -- giving up perf there for stats which will be called much less doesn't make sense to me.

To each their own!

> 1st. Sorting on the web API is definitively wrong. This is especially true in nodejs where the mantra is always non blocking code.

This assertion is so wide that it cannot be true. No matter what language you're in, you can absolutely do sorting in the API layer.

> 2nd. Pipeline should 100 percent be known. Atomic operations are 100 percent required across shared storage and it's expected to occur all the time. Database transactions are fundamental and not unexpected. Pipeline should be extremely common. This is not an obscure operation. I commented about it in my code in case the reviewer was someone like you who isn't familiar with how popular atomic database transactions are, but this operation is common it is not a obscure trick.

You don't need to pipeline if the operation is O(1) -- it's not a question of whether transactions are good/bad, I don't know why you're assuming people don't know about them.

It's the question of taking in unnecessary complexity here when there's an O(1) operation available. You're optimizing for an operation that is not on the hot path. The reason you have to do the atomic operation is because you're using an access pattern that requires it.

I know why and how you used an atomic transaction -- I disagree on whether it was necessary.

> That'd be stupid in my opinion. 302 redirect is something I expected. Maybe I should have stated that explicitly in the docs. > > What? In the specs all examples have trailing slashes. All of them.

This is correct, I read wrong and for that I apologize, they do have all trailing slashes. I was thinking of "/api/".

> Unlikely. Even as a lead it's better to cater to the majority opinion of the team. 99 percent of the time you never have full control. > > Developers and leads typically evolve to fit into the overall team culture while inserting a bit of their own opinions. >

I referred to the context of the project -- you did* have full control.

If you were thinking of it in a team context, you should have asked more clarifying questions and found out more of what they wanted or would look for, similar to a real team context.

> No there isn't. You have to spin up redis. Flask documentation doesn't talk about that. Integration testing involves code that controls docker-compose. It's a bunch of hacky scripts with external process calls that are needed to get integration tests working. > > It's not rocket science but not within the scope of a take-home. Additionally hacking with docker compose is not sustainable or future proof, eventually you will hit infra that can't be replicated with docker. > > I will probably do it next time just to cater to people who share your viewpoints.

Again, this is a matter of what people think is hard and what isn't. Integration testing does not require that you spin up docker compose -- I literally linked to a libraries that do this in a reusable (importable) way w/ docker.

You said you don't do this kind of testing regularly, so it is understandable that it's not easy for you.

If docker was the problem, you can open subprocesses and assume that redis is installed in the test environment (i.e. your laptop, or CI environment), or take a binary to the redis binary path from ENV.

You can even remove the burden of running redis from the test suite and just make sure it's running manually or in test suite invocation/setup. There are many options, and it's just not that hard in 2023.

Don't know what (if anything) turned them off, so can't say this matters -- but it would matter to me, at the very least it would be a large plus for people who found the time to implement it.

> Lol. The prompt was write it in four hours ( no time) and they aren't paying me for it ( no money).

This is true, you did not have much time and they weren't paying you so I guess the situations are similar.

>The hot path is worth optimizing first, in my mind -- giving up perf there for stats which will be called much less doesn't make sense to me.

You aren't getting what I'm saying. logN is already a huge optimization for a hot path. Look at that pic: https://dev-to-uploads.s3.amazonaws.com/i/ya7xk4n9ch50xgwibt...

Seriously look at O(logN) and look at NlogN. Databases everywhere already take logN as acceptable. You are optimizing past that and introducing an NlogN sort step that will block all hot path calls because redis is single threaded. I don't think it's a to each their own thing here, i believe the user experience metrics will definitively play out towards my methodology but we can't know until we actually have metrics so we're at an impasse here.

>This assertion is so wide that it cannot be true. No matter what language you're in, you can absolutely do sorting in the API layer.

No this is absolutely true. Especially if you're a nodejs programmer. Nodejs is single threaded, sorting imposes NlogN penalty on a single thread.

Backend web development for the api layer is all about transmitting as much compute as possible to the database layer. You can get away with small sorts and things can still work but in general the practice of backend optimization is indeed to let the database handle as much as possible.

We can debate it but from my experience I know I'm pretty much categorically right on this. It's a fundamental principle. And it has special application to things that use Async IO like NodeJS. That's why there's huge emphasis in nodejs in writing non-blocking code. A sort is a potential block.

There are very few cases where you would handle compute on the api server rather then the database. The first instance is obvious, if N is guaranteed to be small which is not the case in the takehome. In other instances of sort with large N, the server ideally needs to launch that sort in a separate non-blocking thread so that the server can continue handling oncoming requests. In nodejs, to my knowledge since I last used it, you can't easily spawn a thread at will, you just have to pray you have available workers that aren't blocked by a sorting operation. In python the GIL makes this more complicated, but slightly better then nodejs.

For node you probably have 8 workers on 8 CPU cores. If you have 100 requests/s, all you need is 8 of them calling a sort on large N and all threads become blocked.

Don't believe me? Read the official docs of nodejs: https://nodejs.org/en/docs/guides/dont-block-the-event-loop#...

NlogN stands at the cusp here. It could work for small N, but for large N it will fail. Anything above NlogN is a huge risk.

>You don't need to pipeline if the operation is O(1) -- it's not a question of whether transactions are good/bad, I don't know why you're assuming people don't know about them.

This is categorically false. The pipeline operation as I said HAS NOTHING to do with O(1) or O(N).

This is essentially the operation:

    set["key"] += 1
    # another thread can update the value here leading to print displaying something unexpected. 
    print(set["key"])

>Again, this is a matter of what people think is hard and what isn't. Integration testing does not require that you spin up docker compose -- I literally linked to a libraries that do this in a reusable (importable) way w/ docker.

You linked? Where? I'd like to know about any library that will do this. Tell me of any library that does integration tests that spins up infrastructure for you. The only one closest I can think of that you can run locally is anything that would use docker-compose or some other IAC language that controls containers. I honestly don't think any popular ones exist.

>If docker was the problem, you can open subprocesses and assume that redis is installed in the test environment (i.e. your laptop, or CI environment), or take a binary to the redis binary path from ENV.

No way I'm going to assume the user has redis installed on his local machine. Many devs don't. It's all remote development for them or everything lives in containers.

Docker is not the problem. Docker or virtual machines makes this problem more amenable to a solution, but even using docker here with testing is overkill and hacky. A take home should not expect a user to build excessive infrastructure locally just to run tests.

>You said you don't do this kind of testing regularly, so it is understandable that it's not easy for you.

I'd like to know your perspective. How is spinning up infrastructure for integration tests easy? You have to write code that launches databases, launches servers and views the entire infra externally. Do-able? Yes? Easy? depends on what you call easy. As easy as unit tests? Definitely no. Easy enough for a take home? In my opinion, no.

Maybe for this take home project you could be right. I could do some integration tests by spinning up docker-compose from within python. Hacky but doable. But in general this solution is not production scalable as production involves more things then what can be placed inside a docker-compose.

>You can even remove the burden of running redis from the test suite and just make sure it's running manually or in test suite invocation/setup. There are many options, and it's just not that hard in 2023.

So I should expect the reviewer to have redis running on his local machine? I would expect in 2023, I should have the entire project be segregated from the context it's running in. That's the cutting edge I believe.

Yeah it's 2023, you tell me how integration testing should be done as easily as unit testing on a takehome. I had one takehome project involving S3 and aws lambdas. They expected me to get an AWS account and literally set up infrastructure because there's no way around even testing what I wrote without actual infrastructure. That entire project was just integration test nightmare. Much rather run asserts locally.

>I referred to the context of the project -- you did* have full control.

Well I mean 99% of hires get into a context where they aren't in full control. So why is it logical to test what a developer will do if he/she had full control? Isn't it better to test the developers ability to code and to adapt to different contexts? Your methodology sort of just tests the programmers personal philosophy and whether it matches your own. That was my point.

> You aren't getting what I'm saying. logN is already a huge optimization for a hot path. Look at that pic: https://dev-to-uploads.s3.amazonaws.com/i/ya7xk4n9ch50xgwibt... > > Seriously look at O(logN) and look at NlogN. Databases everywhere already take logN as acceptable. You are optimizing past that and introducing an NlogN sort step that will block all hot path calls because redis is single threaded. I don't think it's a to each their own thing here, i believe the user experience metrics will definitively play out towards my methodology but we can't know until we actually have metrics so we're at an impasse here.

Just to keep the comparison in line -- we're comparing O(log(N)) @ O(1) in redis-land.

On the stats side we're comparing O(N) on the redis side PLUS either O(Nlog(N)) or O(N) on the python side.

> No this is absolutely true. Especially if you're a nodejs programmer. Nodejs is single threaded, sorting imposes NlogN penalty on a single thread.

NodeJS has both child processes and workers for heavy computation:

- https://nodejs.org/api/child_process.html

- https://nodejs.org/api/worker_threads.html

If you want to parallelize work on a thread pool there are libs that do just that:

- https://www.npmjs.com/package/piscina

((this is also part of the reason I think NodeJS is strictly superior to other "scripting" languages as mentioned briefly in the other comment))

Again, here you seem to be arguing against a strawman that doesn't know that blocking the IO loop is bad. Try arguing against one that knows ways to work around that. This is why I'm saying this rule isn't true. Extensive computation on single-threaded "scripting" languages is possible (and even if it wasn't, punt it off to a remote pool of workers, which could also be NodeJS!).

All of this is a premature optimization, but my point here is that the stats can be optimized later. You could literally compute it periodically or continuously and only return the latest cached version w/ a timestamp. This makes the stats endpoint O(1).

> This is categorically false. The pipeline operation as I said HAS NOTHING to do with O(1) or O(N). > ...

I think this is where we're talking past each other, so let me explain more of how I see the problem -- the solution I have in mind is serializing the URL and using ONE call to INCR (https://redis.io/commands/incr/) on the ingest side.

There is a lot you can do with the data storage pattern to make other operations more efficient, but on the stats side, the most basic way you can do it is to scan

I will concede that given that we know the data should fit in memory (otherwise you just crash) your approach gives you O(N) retrieval time and it's definitely superior to not have to do that on the python side (and python just streaming the response through). I am comfortable optimizing in-API computation, so I don't think it's a problem.

Here's what I mean -- you can actually solve the ordering problem in O(N) + O(M) time by keeping track of the max you've seen and building a sparse array and running through every single index from max to zero. It's overkill, but it's generally referred to as a counting sort:

https://ebrary.net/81651/geography/sorting_algorithms

This is overkill, clearly, and I will concede that ZSET is lighter and easier to get right than this.

> You linked? Where? I'd like to know about any library that will do this. Tell me of any library that does integration tests that spins up infrastructure for you. The only one closest I can think of that you can run locally is anything that would use docker-compose or some other IAC language that controls containers. I honestly don't think any popular ones exist.

https://testcontainers-python.readthedocs.io/en/latest/

I am very sure that I linked that, but in the case I didn't, here it is again -- hope you find it useful.

> No way I'm going to assume the user has redis installed on his local machine. Many devs don't. It's all remote development for them or everything lives in containers. > > Docker is not the problem. Docker or virtual machines makes this problem more amenable to a solution, but even using docker here with testing is overkill and hacky. A take home should not expect a user to build excessive infrastructure locally just to run tests.

I don't think these statements make sense -- having docker installed and having redis installed are basically equivalent work. At the end of the day, the outcome is the same -- the developer is capable of running redis locally. Having redis installed on your local machine is absolutely within range for a backend developer.

Also, remote development is not practiced by many companies -- the only companies I've seen doing thin-clients that are large.

> Maybe for this take home project you could be right. I could do some integration tests by spinning up docker-compose from within python. Hacky but doable. But in general this solution is not production scalable as production involves more things then what can be placed inside a docker-compose.

I see it as just spinning up docker, not compose -- you already have access to the app (ex. if it was buildable via a function) so you could spawn redis in a subprocess (or container) on a random port, and then spawn the app.

I agree that it is not trivial, but the value is high (in mymind.

> Yeah it's 2023, you tell me how integration testing should be done as easily as unit testing on a takehome. I had one takehome project involving S3 and aws lambdas. They expected me to get an AWS account and literally set up infrastructure because there's no way around even testing what I wrote without actual infrastructure. That entire project was just integration test nightmare. Much rather run asserts locally.

I agree that integration testing is harder -- I think there's more value there.

Also, for replicating S3, minio (https://github.com/minio/minio) is a good stand-in. For replicating lambda, localstack (https://docs.localstack.cloud/user-guide/aws/lambda/) is probably reasonable there's also frameworks with some consideration for this (https://www.serverless.com/framework/docs/providers/aws/guid...) built in.

That said I do think that's a weakness of the platform compute stuff -- it is inconvenient to test lambda outside of lambda.

> Well I mean 99% of hires get into a context where they aren't in full control. So why is it logical to test what a developer will do if he/she had full control? Isn't it better to test the developers ability to code and to adapt to different contexts? Your methodology sort of just tests the programmers personal philosophy and whether it matches your own. That was my point.

Ah, this is true -- but I think this is what people are testing in interviews. There is a predominant culture/shared values, and the test is literally whether someone can fit into those values.

Whether they should or should not be, that's at least partially what interviews are -- does the new team member feel the same way about technical culture currently shared by the team.

Now in the case of this interview your solution was just fine, even excellent (because you went out of your way to do async io, use newer/easier packaging methodologies, etc), but it's clearly not just that.

Very rare to find a rule that's absolutely true.. I clearly stated exceptions to the rule (which you repeated) but the generality is still true.

Threading in nodejs is new and didn't exist since the last time I touched it. It looks like it's not the standard use case as google searches still have websites with titles saying node is single threaded everywhere. The only way I can see this being done is multiple Processes (meaning each with a copy of v8) using OS shared memory as IPC and they're just calling it threads. It will take a shit load of work to make v8 actually multi-threaded.

Processes are expensive so you can't really follow this model per request. And we stopped following threading per request over a decade ago.

Again these are exceptions to the rule, from what I'm reading Nodejs is normally still single threaded with a fixed number of worker processes that are called "threads". Under this my general rule is still generally true: backend engineering does no typically involve writing non blocking code and offloading compute to other sources. Again, there are exceptions but as I stated before these exceptions are rare.

>Here's what I mean -- you can actually solve the ordering problem in O(N) + O(M) time by keeping track of the max you've seen and building a sparse array and running through every single index from max to zero. It's overkill, but it's generally referred to as a counting sort:

Oh come on. We both know these sorts won't work. These large numbers will throw off memory. Imagine 3 routes. One route gets 352 hits, another route gets 400 hits, and another route gets 600,000 hits. What's Big Oh for memory and sort?

It's O(600,000) for both memory and runtime. N=3 and it doesn't even matter here. Yeah these types of sorts are almost never used for this reason, they only work for things with smaller ranges. It's also especially not useful for this project. Like this project was designed so "counting sort" fails big time.

Also we don't need to talk about the O(N) read and write. That's a given it's always there.

>I don't think these statements make sense -- having docker installed and having redis installed are basically equivalent work. At the end of the day, the outcome is the same -- the developer is capable of running redis locally. Having redis installed on your local machine is absolutely within range for a backend developer.

Unfortunately these statements do make sense and your characterization seems completely dishonest to me. People like to keep their local environments pure and segregated away from daemons that run in a web server. I'm sure in your universe you are claiming web developers install redis, postgresql and kafka all locally but that just sounds absurd to me. We can agree to disagree but from my perspective I don't think you're being realistic here.

>Also, remote development is not practiced by many companies -- the only companies I've seen doing thin-clients that are large.

It's practiced by a large amount and basically every company I've worked at for the past 5 years. Every company has to at least partially do remote dev in order to fully test E2E stuff or integrations.

>I see it as just spinning up docker, not compose -- you already have access to the app (ex. if it was buildable via a function) so you could spawn redis in a subprocess (or container) on a random port, and then spawn the app.

Sure. The point is it's hacky to do this without an existing framework. I'll check out that library you linked.

>I agree that integration testing is harder -- I think there's more value there.

Of course there's more value. You get more value at higher cost. That's been my entire point.

>Also, for replicating S3, minio (https://github.com/minio/minio) is a good stand-in. For replicating lambda, localstack (https://docs.localstack.cloud/user-guide/aws/lambda/) is probably reasonable there's also frameworks with some consideration for this (https://www.serverless.com/framework/docs/providers/aws/guid...) built in.

Good finds. But what about SNS, IOT, Big Query and Redshift? Again my problem isn't about specific services, it's about infra in general.

>Ah, this is true -- but I think this is what people are testing in interviews. There is a predominant culture/shared values, and the test is literally whether someone can fit into those values.

No. I think what's going on is people aren't putting much thought into what they're actually interviewing for. They just have some made up bar in their mind whether it's a leetcode algorithm or whether the guy wrote a unit test for the one available pure function for testing.

>Whether they should or should not be, that's at least partially what interviews are -- does the new team member feel the same way about technical culture currently shared by the team.

The answer is no. There's always developers who disagree with things and just don't reveal it. Think about the places you worked at. Were you in total agreement? I doubt it. A huge amount of devs are opinionated and think company policies or practices are BS. People adapt.

>Now in the case of this interview your solution was just fine, even excellent (because you went out of your way to do async io, use newer/easier packaging methodologies, etc), but it's clearly not just that.

The testing is just a game. I can play the game and suddenly I pass all the interviews. I think this is the flaw with your methodology as I just need to write tests to get in. Google for example in spirit attempted another method which involves testing IQ via algorithms. It's a much higher bar

The problem with google is that their methodology can also be gamed but it's much harder to game it and often the bar is too high for the actual job the engineer is expected to do.

I think both methodologies are flawed, but hiring via ignoring raw ability and picking people based off of weirdly specific cultural preferences is the worse of the two hiring methodologies.

Put it this way. If a company has a strong testing culture, then engineers who don't typically test things will adapt. It's not hard to do, and testing isn't so annoying that they won't do it.