11 ms·

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 repla

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 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.

hardwaresofton 3y ago
> 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/README.html 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 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.

hardwaresofton 3y ago

> 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/README.html 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 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.