5 ms·

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

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

formulathree 3y ago
>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) Look at the above. Same concept. Generators are drop in replacements for actual state. State and functions are the isomorphic (ie the same thing). Building a generator is cheaper then building a list. You should read SICP about this topic of how functions and data are the same. >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 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.

formulathree 3y ago

>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) Look at the above. Same concept. Generators are drop in replacements for actual state. State and functions are the isomorphic (ie the same thing). Building a generator is cheaper then building a list. You should read SICP about this topic of how functions and data are the same. >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 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.