Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interview/ok #1

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Interview/ok #1

wants to merge 9 commits into from

Conversation

iky
Copy link

@iky iky commented Dec 26, 2018

I added a few tests with a minimal setup (testing with real db, but not involving Flask). I noticed that there are no tests provided and that candidates are not expected to write tests of any sort, but filling up solutions without testing it on the way is not my nature of coding. Especially when working with APIs of libraries and frameworks I want to see changes working. Also the second question requires quite a few steps and a number of data transformations to make which I would never approach with a "coding it first and hoping it works" way .)

I had to slightly change the existing solution to make it work first (see 57c1878, includes initial test for original occupancy calculation without room blocking)

Regarding Q1, I was tempted to add blocked rooms as additional row_type of Bookings model and see how that looks (see 00bfaaf) .) but then made a separate model for storing it with number of blocked rooms per reservation date time (task description asks for a new model). There is a third option of storing blocks also with date range but that would make the entrypoint a bit more complex not saving much of db space and work (normally I would consult product to see what is the usual use of blocking rooms and implement it accordingly).

I can see some room for improvements such as filling occupancy_per_day and revenue_per_day in Q2 together in one for loop, they're in separate loops right now as I added the occupancy and revenue curves functionalities one by one (see the commits flow).
Having some tests, fun with optimising the selects or even trying the pure SQL can start ;) (I would still prefer python solution as it would be always more readable than SQL; readability counts)

On the way I was placing TODOs with comments of what I would focus on later if this is
a real live task, you can ignore them... .)

iky added 9 commits December 26, 2018 19:43
I was tempted to try this, did it as first approach as it was really
easy to do. Will come back to this task re-implementing it as a whole new
model representing the blocks with number for room blocked on it.
To start with something.)
Copy link

@SebastianoF SebastianoF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants